adding a queue system to the agent runner#275
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds AI-assisted backlog planning endpoints and UI dialog (generate, stop, status, apply) plus a server-side prompt queue for the agent with persistence, queue management routes, client APIs, and UI integration for queue display and backlog-plan workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as BacklogPlanDialog
participant Server as /api/backlog-plan
participant Claude as Claude AI
participant Features as FeatureService
User->>UI: Submit prompt (projectPath, prompt)
UI->>Server: POST /generate
Server->>Features: Load current features
Server->>Claude: Streamed AI query (system + user prompt)
Claude-->>Server: Streamed content
Server-->>UI: Emit backlog_plan_complete event with result
UI->>User: Show review dialog (changes)
User->>UI: Select changes -> Apply
UI->>Server: POST /apply (filtered plan)
Server->>Features: Execute deletes (cleanup deps)
Server->>Features: Execute adds
Server->>Features: Execute updates
Features-->>Server: Acknowledge applied IDs
Server-->>UI: Return appliedChanges
sequenceDiagram
participant User
participant AgentUI as Agent View
participant Server as /api/agent/queue
participant AgentSvc as AgentService
participant Executor as Task Executor
User->>AgentUI: Send message while processing
AgentUI->>AgentUI: Detect processing
AgentUI->>Server: POST /queue/add (message, imagePaths, model?)
Server->>AgentSvc: addToQueue(sessionId, prompt)
AgentSvc->>AgentSvc: Persist queue to disk
AgentSvc-->>Server: queuedPrompt
Server-->>AgentUI: success (queued)
Executor->>AgentSvc: on current task complete -> processNextInQueue(sessionId)
AgentSvc->>Executor: Dequeue & execute next prompt
AgentSvc-->>AgentUI: Emit queue_updated event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's capabilities by introducing a queueing mechanism for the agent runner and a new AI-driven backlog planning feature. The agent can now handle a sequence of tasks, improving workflow for complex operations, while the backlog planner streamlines project management by leveraging AI to suggest and apply feature modifications. These changes provide a more powerful and intuitive user experience for managing development tasks and project backlogs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a queue system for the agent runner, allowing prompts to be queued up and executed sequentially. It also adds a new, significant feature for AI-assisted backlog planning.
The agent queue implementation is solid, with good handling of state persistence and background processing. The UI changes in the agent view are well-thought-out, providing clear feedback to the user about queued items.
The backlog planning feature is also a great addition. The API design with background generation and event-based updates is appropriate for a long-running task. The review dialog for the generated plan is a particularly strong piece of UX.
I've left a few comments with suggestions for improvement:
- A high-level design comment about the singleton nature of the backlog plan generation, which could be a bottleneck.
- A few suggestions to improve code quality and remove redundancy in the backlog plan application logic.
- A note on improving error handling for agent queue persistence.
- A suggestion for safer request body parsing in the new queue API handlers.
Overall, this is a substantial and well-executed pull request that adds powerful new capabilities. Great work!
| // State for tracking running generation | ||
| let isRunning = false; | ||
| let currentAbortController: AbortController | null = null; |
There was a problem hiding this comment.
The use of module-level variables isRunning and currentAbortController makes the backlog plan generation a singleton process for the entire server. This means only one plan can be generated at a time across all users and projects. While this might be intentional to limit resource usage, it's a significant design choice that could become a bottleneck if the server is used in a multi-user or multi-project context. If this is not the intended behavior, consider scoping this state to a session or project.
| const { sessionId, message, imagePaths, model } = req.body as { | ||
| sessionId: string; | ||
| message: string; | ||
| imagePaths?: string[]; | ||
| model?: string; | ||
| }; |
There was a problem hiding this comment.
Using a type assertion as for the request body can be unsafe. If the request body does not match the expected shape, it can lead to runtime errors. For new code, it would be more robust to perform runtime validation of the request body's shape before using it. This comment also applies to the other new queue handlers (queue-clear.ts, queue-list.ts, queue-remove.ts).
| // Before deleting, update any features that depend on this one | ||
| for (const feature of allFeatures) { | ||
| if (feature.dependencies?.includes(change.featureId)) { | ||
| const newDeps = feature.dependencies.filter((d) => d !== change.featureId); | ||
| await featureLoader.update(projectPath, feature.id, { dependencies: newDeps }); | ||
| logger.info( | ||
| `[BacklogPlan] Removed dependency ${change.featureId} from ${feature.id}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop manually removes dependencies from features that depend on a feature being deleted. However, the AI prompt for plan generation already instructs the AI to provide these dependency updates in the dependencyUpdates field of the plan. This manual logic is redundant and could potentially conflict with the AI's intended dependency management. It would be cleaner and more consistent to rely solely on the dependencyUpdates provided in the plan, which the user can review before applying. Consider removing this loop.
| const newDeps = currentDeps | ||
| .filter((d) => !depUpdate.removedDependencies.includes(d)) | ||
| .concat(depUpdate.addedDependencies.filter((d) => !currentDeps.includes(d))); |
There was a problem hiding this comment.
While the current logic correctly handles adding and removing dependencies, using a Set can make the code more readable and robust against potential duplicates. It also more clearly expresses the intent of the operation. For example:
const depsSet = new Set(currentDeps);
depUpdate.removedDependencies.forEach((d) => depsSet.delete(d));
depUpdate.addedDependencies.forEach((d) => depsSet.add(d));
const newDeps = Array.from(depsSet);| setRunningState(true); | ||
| const abortController = new AbortController(); | ||
| setRunningState(true, abortController); |
There was a problem hiding this comment.
The call to setRunningState(true) on line 39 is redundant because setRunningState(true, abortController) is called immediately after on line 41, which also sets the running state to true. You can safely remove the first call.
| setRunningState(true); | |
| const abortController = new AbortController(); | |
| setRunningState(true, abortController); | |
| const abortController = new AbortController(); | |
| setRunningState(true, abortController); |
| try { | ||
| await secureFs.writeFile(queueFile, JSON.stringify(queue, null, 2), 'utf-8'); | ||
| } catch (error) { | ||
| console.error('[AgentService] Failed to save queue state:', error); | ||
| } |
There was a problem hiding this comment.
Errors during saveQueueState are caught and logged to console.error, but not propagated. This means that if saving the queue state fails (e.g., due to disk space issues or permissions), the operation will fail silently from the caller's perspective, leading to data loss on server restart. Consider using the service's logger instead of console.error and either re-throwing the error or returning a status to the caller so that it can be handled appropriately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
apps/server/src/routes/backlog-plan/routes/status.ts (1)
8-18: Consider dependency injection pattern consistency.The
createStatusHandler()doesn't accept any parameters, while other route handler factories in the codebase typically inject dependencies likeagentService. IfgetBacklogPlanStatus()accesses module-level state, consider whether injecting the service/state would improve testability and align with the established pattern.Example pattern from other handlers
Other handlers follow this pattern:
export function createSomeHandler(agentService: AgentService) { return async (req: Request, res: Response): Promise<void> => { // handler logic using injected service }; }apps/server/src/routes/agent/routes/queue-list.ts (1)
12-20: Consider stricter sessionId validation.The validation only checks for truthiness. Consider validating that
sessionIdis a non-empty string to catch edge cases like empty strings or whitespace-only values.Suggested validation enhancement
- if (!sessionId) { + if (!sessionId || typeof sessionId !== 'string' || !sessionId.trim()) { res.status(400).json({ success: false, - error: 'sessionId is required', + error: 'Valid sessionId is required', }); return; }apps/server/src/routes/agent/routes/queue-clear.ts (1)
12-20: Consider stricter sessionId validation.Similar to the queue-list handler, consider validating that
sessionIdis a non-empty string to prevent edge cases.Suggested validation enhancement
- if (!sessionId) { + if (!sessionId || typeof sessionId !== 'string' || !sessionId.trim()) { res.status(400).json({ success: false, - error: 'sessionId is required', + error: 'Valid sessionId is required', }); return; }apps/server/src/routes/backlog-plan/routes/generate.ts (1)
39-41: Remove redundantsetRunningState(true)call.Line 39 sets the running state to
true, then line 41 immediately sets it again with theabortController. The first call is redundant.🔎 Proposed fix
- setRunningState(true); const abortController = new AbortController(); setRunningState(true, abortController);apps/server/src/routes/backlog-plan/routes/apply.ts (1)
10-10: Module-level singleton may cause issues in tests.The
featureLoaderis instantiated at module scope, making it a singleton shared across all requests. While this works for production, it can complicate testing if you need to mock or inject a different instance.Consider accepting it as a parameter to the handler factory for better testability:
🔎 Proposed refactor
-const featureLoader = new FeatureLoader(); - -export function createApplyHandler() { +export function createApplyHandler(featureLoader: FeatureLoader = new FeatureLoader()) {apps/ui/src/hooks/use-electron-agent.ts (1)
478-527: Consider extracting shared message preparation logic.The text file context building (lines 488-496) and image saving logic (lines 498-513) are duplicated across
sendMessage,sendMessageDirectly, andaddToServerQueue. This increases maintenance burden and risk of inconsistencies.🔎 Suggested approach
Extract the common logic into helper functions:
// Helper to build message content with text file context function buildMessageContent( content: string, textFiles?: TextFileAttachment[] ): string { if (!textFiles?.length) return content; const contextParts = textFiles.map((file) => `<file name="${file.filename}">\n${file.content}\n</file>` ); return `Here are some files for context:\n\n${contextParts.join('\n\n')}\n\n${content}`; } // Helper to save images and return paths async function saveImagesToTemp( api: ElectronAPI, images: ImageAttachment[], workingDirectory?: string ): Promise<string[]> { const imagePaths: string[] = []; for (const image of images) { const result = await api.saveImageToTemp( image.data, sanitizeFilename(image.filename), image.mimeType, workingDirectory ); if (result.success && result.path) { imagePaths.push(result.path); } } return imagePaths; }Then use these in all three functions to reduce duplication.
apps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsx (1)
71-99: handleGenerate: Consider preserving prompt on API failure.Currently,
setPrompt('')is called before knowing if generation succeeds. If the API returns an error, the user loses their typed prompt.🔎 Suggested improvement
// Show toast and close dialog - generation runs in background toast.info('Generating plan... This will be ready soon!', { duration: 3000, }); - setPrompt(''); onClose(); + // Clear prompt only after successful initiation and dialog close + setPrompt(''); }, [projectPath, prompt, setIsGeneratingPlan, onClose]);Or better yet, only clear on success:
const result = await api.backlogPlan.generate(projectPath, prompt); if (!result.success) { setIsGeneratingPlan(false); toast.error(result.error || 'Failed to start plan generation'); return; } // Show toast and close dialog - generation runs in background toast.info('Generating plan... This will be ready soon!', { duration: 3000, }); setPrompt(''); onClose();apps/server/src/routes/backlog-plan/generate-plan.ts (2)
41-60: JSON parsing could be more robust for edge cases.The regex ````json\n?([\s\S]*?)\n?``` ` uses non-greedy matching which is good, but consider that AI might output multiple JSON blocks or malformed blocks.
The current implementation is reasonable for MVP. For future hardening, consider:
- Validating the parsed JSON against the expected schema
- Handling cases where AI outputs explanation text before/after the JSON
13-13: Consider making featureLoader injectable for testability.The module-level
featureLoaderinstance makes unit testing harder. Consider passing it as a parameter or using dependency injection.This is a minor concern and the current approach works fine for the use case.
libs/types/src/backlog-plan.ts (1)
10-15: Consider adding validation constraints in documentation.For
updateanddeleteoperations,featureIdis required but typed as optional. Consider adding JSDoc to clarify when each field is required.export interface BacklogChange { type: 'add' | 'update' | 'delete'; /** Required for 'update' and 'delete' operations */ featureId?: string; /** Required for 'add', optional for 'update' operations */ feature?: Partial<Feature>; reason: string; }Alternatively, consider using discriminated union types for stricter typing, though the current approach is simpler and works well with AI-generated responses.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/server/src/index.tsapps/server/src/routes/agent/index.tsapps/server/src/routes/agent/routes/queue-add.tsapps/server/src/routes/agent/routes/queue-clear.tsapps/server/src/routes/agent/routes/queue-list.tsapps/server/src/routes/agent/routes/queue-remove.tsapps/server/src/routes/backlog-plan/common.tsapps/server/src/routes/backlog-plan/generate-plan.tsapps/server/src/routes/backlog-plan/index.tsapps/server/src/routes/backlog-plan/routes/apply.tsapps/server/src/routes/backlog-plan/routes/generate.tsapps/server/src/routes/backlog-plan/routes/status.tsapps/server/src/routes/backlog-plan/routes/stop.tsapps/server/src/services/agent-service.tsapps/ui/src/components/views/agent-view.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsxapps/ui/src/components/views/board-view/dialogs/index.tsapps/ui/src/hooks/use-electron-agent.tsapps/ui/src/lib/http-api-client.tslibs/types/src/backlog-plan.tslibs/types/src/event.tslibs/types/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/server/src/routes/backlog-plan/routes/status.ts (1)
apps/server/src/routes/backlog-plan/common.ts (3)
getBacklogPlanStatus(13-15)logError(35-37)getErrorMessage(28-33)
apps/server/src/routes/agent/routes/queue-list.ts (1)
apps/server/src/routes/backlog-plan/common.ts (2)
logError(35-37)getErrorMessage(28-33)
apps/server/src/routes/backlog-plan/routes/stop.ts (1)
apps/server/src/routes/backlog-plan/common.ts (4)
getAbortController(24-26)setRunningState(17-22)logError(35-37)getErrorMessage(28-33)
apps/server/src/index.ts (1)
apps/server/src/routes/backlog-plan/index.ts (1)
createBacklogPlanRoutes(14-30)
apps/server/src/services/agent-service.ts (2)
apps/ui/src/types/electron.d.ts (1)
Message(13-20)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)
🪛 Biome (2.1.2)
apps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsx
[error] 247-247: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 248-248: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 249-249: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 250-250: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 251-251: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (32)
apps/ui/src/components/views/board-view/board-header.tsx (1)
2-2: LGTM! Clean integration of the Plan button.The new Plan button follows existing patterns consistently, with proper imports, typing, and test ID. The button placement and styling align well with other header controls.
Also applies to: 6-6, 19-19, 32-32, 95-103
libs/types/src/event.ts (1)
12-12: LGTM! Event type addition aligns with the new backlog planning feature.apps/ui/src/components/views/board-view/dialogs/index.ts (1)
3-3: LGTM! Barrel export follows the established pattern.apps/server/src/routes/agent/index.ts (1)
15-18: LGTM! Queue routes are properly registered.The new queue management endpoints follow established patterns and are appropriately organized with a clarifying comment.
Also applies to: 34-42
libs/types/src/index.ts (1)
99-107: LGTM! Type exports properly expand the public API.apps/server/src/routes/agent/routes/queue-list.ts (1)
22-23: LGTM! Queue retrieval properly delegated to service.The synchronous call to
getQueueis correct since it's a simple state retrieval operation.apps/server/src/routes/agent/routes/queue-clear.ts (1)
22-23: LGTM! Async queue clearing properly handled.The awaited call to
clearQueueis appropriate for a state-mutating operation.apps/server/src/index.ts (1)
51-51: LGTM!The backlog-plan route import and mounting follow the established patterns in this file. The route is correctly placed after the
authMiddlewareapplication, ensuring proper authentication, and the dependency injection ofeventsandsettingsServiceis consistent with similar routes likesuggestionsandspec-regeneration.Also applies to: 164-164
apps/server/src/routes/agent/routes/queue-remove.ts (1)
9-31: LGTM!The handler implementation follows the established pattern from other queue endpoints. Input validation correctly checks for both required fields, and error handling is consistent with the codebase conventions using
logErrorandgetErrorMessage.apps/server/src/routes/backlog-plan/routes/generate.ts (1)
43-54: LGTM for the background task pattern.The async generation pattern correctly starts the task in the background, emits error events on failure, and properly cleans up state in the
finallyblock.apps/server/src/routes/agent/routes/queue-add.ts (1)
9-33: LGTM!The handler correctly validates required fields and delegates to the agent service. The error handling follows established patterns in the codebase.
apps/server/src/routes/backlog-plan/routes/stop.ts (1)
8-21: LGTM!The stop handler correctly implements idempotent behavior—returning success even when no generation is running. This is appropriate for a stop endpoint.
apps/server/src/routes/backlog-plan/index.ts (1)
14-29: LGTM!The route factory follows the established pattern in the codebase. Path validation middleware is correctly applied to endpoints that require
projectPath, and dependencies are properly injected into the handlers.apps/server/src/routes/backlog-plan/routes/apply.ts (1)
39-111: LGTM for the three-pass change processing.The ordering (deletes → adds → updates) is correct for avoiding dependency conflicts. The partial success pattern with per-operation error handling is appropriate for bulk operations, allowing the process to continue even if individual changes fail.
apps/ui/src/hooks/use-electron-agent.ts (3)
253-257: LGTM for the 'started' event handler.This correctly handles server-initiated processing (e.g., from queue auto-processing) where the client didn't explicitly call
sendMessage.
328-338: LGTM for queue event handlers.The
queue_updatedhandler defensively handles undefined queue, andqueue_errorappropriately only sets the error state without affectingisProcessing(since queue errors don't necessarily mean current processing should stop).
529-572: LGTM for queue management callbacks.Both
removeFromServerQueueandclearServerQueuefollow consistent patterns with proper API availability checks and error handling.apps/server/src/routes/backlog-plan/common.ts (1)
1-39: LGTM! Clean utility module for backlog plan state management.The module-scoped state pattern is appropriate for tracking single-instance generation status. The error handling utilities are well-implemented with proper type guards.
apps/ui/src/components/views/board-view.tsx (2)
589-618: LGTM! Well-structured event listener for backlog plan events.The effect properly:
- Checks for API availability before subscribing
- Handles both success and error event types
- Provides user feedback via toasts with actionable review button
- Returns unsubscribe function for cleanup
The empty dependency array is correct since this only needs to subscribe once on mount.
1215-1225: Props wiring looks complete for BacklogPlanDialog.All state and callbacks are properly passed to the dialog component, enabling bidirectional state management between the board view and the dialog.
apps/server/src/services/agent-service.ts (4)
35-51: Well-designed queue data structure.The
QueuedPromptinterface captures all necessary fields including optional model override per-prompt. The timestamp (addedAt) enables UI ordering and debugging.
357-358: Good use ofsetImmediatefor queue processing.Using
setImmediateallows the current response to complete before processing the next queued item, preventing stack buildup and ensuring proper event emission ordering.
598-630: Queue add implementation is correct.The method properly:
- Creates a unique ID for each queued prompt
- Persists immediately after modification
- Emits update event for UI synchronization
716-757: Double-check pattern forisRunningstate is well-designed defense-in-depth.The race condition window between the check at line 726 and
sendMessagecall at line 743 is minimal, and properly defended by theisRunningcheck insidesendMessageitself (which throws if already running). This intentional defense-in-depth approach safely handles any edge cases despite Node.js's single-threaded nature.apps/ui/src/components/views/agent-view.tsx (3)
141-159: Clean queue integration in handleSend.The conditional flow is intuitive: when processing, add to queue; otherwise, send directly. The early return for empty input is preserved correctly.
738-778: Well-implemented queued prompts UI.The queue display includes:
- Count indicator with proper pluralization
- Individual item removal with hover-reveal delete button
- Clear all option
- File attachment indicators per item
- Good visual hierarchy with numbering
981-996: Good visual feedback for send/queue button.The icon change (
Send→ListOrdered) and variant change (default→outline) clearly communicate the button's current behavior. The title attribute provides additional context on hover.apps/ui/src/lib/http-api-client.ts (2)
832-872: Queue API methods are well-structured.The methods follow the established patterns in this file:
- Consistent return type shapes with
success, optional data, and optionalerror- Proper use of POST for mutations
- Session-scoped operations require
sessionId
1089-1127: backlogPlan API surface is complete.All CRUD-style operations are covered:
generate: initiate plan creationstop: abort in-progress generationstatus: check if generation is runningapply: execute the planonEvent: subscribe to progress/completion eventsapps/server/src/routes/backlog-plan/generate-plan.ts (2)
179-193: Stream processing correctly handles abort and extracts text.The loop properly:
- Checks abort signal on each iteration
- Only processes 'assistant' message types
- Concatenates text blocks from content array
214-216: Good use of finally for cleanup.The
setRunningState(false, null)in the finally block ensures state is reset regardless of success or failure, preventing stuck states.libs/types/src/backlog-plan.ts (1)
1-67: Well-designed type definitions for the backlog plan feature.The types are comprehensive and properly structured:
BacklogChangeuses discriminated union pattern withtypefieldPartial<Feature>allows flexible feature payloads- Event types cover the complete lifecycle
- Clear separation between request, result, and apply result types
| case 'review': | ||
| if (!pendingPlanResult) return null; | ||
|
|
||
| const additions = pendingPlanResult.changes.filter((c) => c.type === 'add'); | ||
| const updates = pendingPlanResult.changes.filter((c) => c.type === 'update'); | ||
| const deletions = pendingPlanResult.changes.filter((c) => c.type === 'delete'); | ||
| const allSelected = selectedChanges.size === pendingPlanResult.changes.length; | ||
| const someSelected = selectedChanges.size > 0 && !allSelected; |
There was a problem hiding this comment.
Wrap switch case declarations in a block to fix lint error.
The static analyzer correctly identifies that variables declared in case 'review': can leak to other cases. Wrap the case body in braces.
🔎 Proposed fix
case 'review':
+ {
if (!pendingPlanResult) return null;
const additions = pendingPlanResult.changes.filter((c) => c.type === 'add');
const updates = pendingPlanResult.changes.filter((c) => c.type === 'update');
const deletions = pendingPlanResult.changes.filter((c) => c.type === 'delete');
const allSelected = selectedChanges.size === pendingPlanResult.changes.length;
const someSelected = selectedChanges.size > 0 && !allSelected;
return (
// ... rest of JSX
);
+ }
case 'applying':📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'review': | |
| if (!pendingPlanResult) return null; | |
| const additions = pendingPlanResult.changes.filter((c) => c.type === 'add'); | |
| const updates = pendingPlanResult.changes.filter((c) => c.type === 'update'); | |
| const deletions = pendingPlanResult.changes.filter((c) => c.type === 'delete'); | |
| const allSelected = selectedChanges.size === pendingPlanResult.changes.length; | |
| const someSelected = selectedChanges.size > 0 && !allSelected; | |
| case 'review': | |
| { | |
| if (!pendingPlanResult) return null; | |
| const additions = pendingPlanResult.changes.filter((c) => c.type === 'add'); | |
| const updates = pendingPlanResult.changes.filter((c) => c.type === 'update'); | |
| const deletions = pendingPlanResult.changes.filter((c) => c.type === 'delete'); | |
| const allSelected = selectedChanges.size === pendingPlanResult.changes.length; | |
| const someSelected = selectedChanges.size > 0 && !allSelected; | |
| return ( | |
| // ... rest of JSX | |
| ); | |
| } | |
| case 'applying': |
🧰 Tools
🪛 Biome (2.1.2)
[error] 247-247: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 248-248: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 249-249: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 250-250: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 251-251: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsx
around lines 244 to 251, the variables declared inside the case 'review': block
can leak to other switch cases and trigger a lint error; wrap the entire case
body in braces (i.e., change "case 'review':" to "case 'review': {", move the
existing statements inside those braces, and close with a "}" before the next
case or the switch end) so the const declarations are scoped to this case only
and keep the existing return null logic intact.
| <Checkbox | ||
| id="select-all" | ||
| checked={allSelected} | ||
| // @ts-expect-error - indeterminate is valid but not in types | ||
| indeterminate={someSelected} | ||
| onCheckedChange={toggleAllChanges} | ||
| /> | ||
| <label htmlFor="select-all" className="text-sm font-medium cursor-pointer"> | ||
| {allSelected ? 'Deselect all' : 'Select all'} ({selectedChanges.size}/ | ||
| {pendingPlanResult.changes.length}) | ||
| </label> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "indeterminate" apps/ui/src/components/ui/checkbox.tsxRepository: AutoMaker-Org/automaker
Length of output: 254
🏁 Script executed:
cat -n apps/ui/src/components/ui/checkbox.tsxRepository: AutoMaker-Org/automaker
Length of output: 2512
Add indeterminate prop to CheckboxProps interface or refactor to use supported checked values.
The Checkbox component's TypeScript interface doesn't define an indeterminate prop, making the @ts-expect-error necessary. The component explicitly supports 'indeterminate' as a value for the checked prop (line 11: checked?: boolean | 'indeterminate'), but the code uses indeterminate as a separate boolean prop which isn't typed. Either add proper typing for the indeterminate prop to CheckboxProps, or refactor to use checked={someSelected ? 'indeterminate' : false} to align with the component's documented API.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view/dialogs/backlog-plan-dialog.tsx
around lines 282 to 293, the Checkbox usage relies on an untyped indeterminate
prop (hence the @ts-expect-error); update the code to remove the unsupported
boolean prop and use the Checkbox's documented checked union instead by passing
checked={someSelected ? 'indeterminate' : allSelected} (or checked={someSelected
? 'indeterminate' : false} with separate allSelected handling), or alternatively
add an indeterminate?: boolean entry to the CheckboxProps interface so the prop
is properly typed — pick one approach and make the types and usage consistent so
the @ts-expect-error can be removed.
| onEvent: (callback: (data: unknown) => void): (() => void) => { | ||
| return this.subscribeToEvent('backlog-plan:event', callback as EventCallback); | ||
| }, |
There was a problem hiding this comment.
Add 'backlog-plan:event' to EventType union.
The onEvent method subscribes to 'backlog-plan:event', but this event type isn't in the EventType union defined at line 52-57. This will cause TypeScript to accept any string due to the cast, but adding it properly improves type safety.
🔎 Proposed fix at line 52-57
type EventType =
| 'agent:stream'
| 'auto-mode:event'
| 'suggestions:event'
| 'spec-regeneration:event'
- | 'issue-validation:event';
+ | 'issue-validation:event'
+ | 'backlog-plan:event';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onEvent: (callback: (data: unknown) => void): (() => void) => { | |
| return this.subscribeToEvent('backlog-plan:event', callback as EventCallback); | |
| }, | |
| type EventType = | |
| | 'agent:stream' | |
| | 'auto-mode:event' | |
| | 'suggestions:event' | |
| | 'spec-regeneration:event' | |
| | 'issue-validation:event' | |
| | 'backlog-plan:event'; |
🤖 Prompt for AI Agents
In apps/ui/src/lib/http-api-client.ts around lines 52-57 and the new usage at
1124-1126, the EventType union is missing the 'backlog-plan:event' literal which
is why onEvent casts the callback; add 'backlog-plan:event' to the EventType
union (the literal string) so the subscribeToEvent call is strongly typed
without casting, and ensure any related switch/handlers that depend on EventType
are updated accordingly.
- Adjusted the test to reflect the addition of queue state file reading, increasing the expected number of file read calls from 2 to 3. - Updated comments for clarity regarding the file reading process in the agent-service tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/tests/unit/services/agent-service.test.ts (1)
109-111: Test expectation is correct:startConversationreads three files.The assertion correctly reflects that the implementation loads the session file, metadata file, and queue state file during the first call (lines 95-107 of agent-service.ts). The test properly verifies that subsequent calls with the same session ID reuse the in-memory session without additional file reads.
Consider verifying specific files rather than just the call count. This would catch issues where wrong files are read or the order changes unexpectedly:
expect(fs.readFile).toHaveBeenCalledWith( expect.stringContaining('session-1.json'), 'utf-8' ); expect(fs.readFile).toHaveBeenCalledWith( expect.stringContaining('sessions-metadata.json'), 'utf-8' ); expect(fs.readFile).toHaveBeenCalledWith( expect.stringContaining('session-1-queue.json'), 'utf-8' );Optional: Add test coverage for queue operations. The PR introduces queue management methods (addToQueue, getQueue, removeFromQueue, clearQueue) but no dedicated unit tests exist for these operations. Consider adding tests to verify queue persistence, item ordering, and state management.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/tests/unit/services/agent-service.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.