feat: enhance UI components and branch management #161
Conversation
- Added new RadioGroup and Switch components for better UI interaction. - Introduced BranchSelector for improved branch selection in feature dialogs. - Updated Autocomplete and BranchAutocomplete components to handle error states. - Refactored feature management to archive verified features instead of deleting them. - Enhanced worktree handling by removing worktreePath from features, relying on branchName instead. - Improved auto mode functionality by integrating branch management and worktree updates. - Cleaned up unused code and optimized existing logic for better performance.
|
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. WalkthroughWorktree handling moved from pre-created paths to branch-name-driven runtime derivation; UI adds BranchSelector and Radix radio/switch components; auto-mode start/stop and autoLoopRunning were removed and replaced with on-demand per-feature execution and related API changes; many hooks/components updated to use branchName and report removed worktrees. Changes
Sequence Diagram(s)mermaid UI->>HTTP: POST /auto-mode/run-feature (projectPath, featureId, useWorktrees?) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)apps/app/tests/worktree-integration.spec.ts (4)
⏰ 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). (2)
🔇 Additional comments (5)
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 introduces significant enhancements to the application's user interface and core branch/feature management capabilities. The changes aim to provide a more intuitive and robust experience for users interacting with features and Git worktrees, particularly within the auto mode. By refining how branches are selected, features are managed, and worktrees are handled, the system becomes more efficient and user-friendly. 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 significant enhancements to the UI and branch management logic. Key changes include adding new UI components like RadioGroup and Switch, introducing a BranchSelector for better branch selection, and refactoring feature management to archive verified features instead of deleting them. A major architectural change is the removal of worktreePath from features, now relying solely on branchName, with worktree creation handled server-side at execution time. The auto-mode logic has also been moved to the client-side. Overall, these are great improvements that simplify the client-side logic and make branch management more robust. I've included a couple of suggestions to improve the new client-side auto-mode implementation for better performance and correctness.
| // Filter backlog features by the currently selected worktree branch | ||
| const primaryBranch = currentProject.path | ||
| ? getPrimaryWorktreeBranch(currentProject.path) | ||
| : null; | ||
| const backlogFeatures = hookFeatures.filter((f) => { | ||
| if (f.status !== "backlog") return false; | ||
|
|
||
| // Determine the feature's branch (default to primary branch if not set) | ||
| const featureBranch = f.branchName || primaryBranch || "main"; | ||
|
|
||
| // If no worktree is selected (currentWorktreeBranch is null or matches primary), | ||
| // show features with no branch or primary branch | ||
| if ( | ||
| !currentWorktreeBranch || | ||
| (currentProject.path && | ||
| isPrimaryWorktreeBranch( | ||
| currentProject.path, | ||
| currentWorktreeBranch | ||
| )) | ||
| ) { | ||
| return ( | ||
| !f.branchName || | ||
| (currentProject.path && | ||
| isPrimaryWorktreeBranch(currentProject.path, featureBranch)) | ||
| ); | ||
| } | ||
|
|
||
| // Otherwise, only show features matching the selected worktree branch | ||
| return featureBranch === currentWorktreeBranch; | ||
| }); |
There was a problem hiding this comment.
The logic for filtering backlogFeatures here is a bit different from the filtering logic in use-board-column-features.ts, which can lead to inconsistencies in what the user sees in the backlog and what auto-mode considers available to run.
Specifically, the line const featureBranch = f.branchName || primaryBranch || "main"; can be problematic. If a feature has no branch (f.branchName is undefined), it should only be considered for the primary worktree, but this logic might incorrectly associate it with the main branch in some edge cases.
To ensure consistency and correctness, I recommend refactoring this filtering logic to align with the clearer implementation in use-board-column-features.ts. This will make the behavior more predictable for the user.
| }, [ | ||
| autoMode.isRunning, | ||
| currentProject, | ||
| runningAutoTasks, | ||
| maxConcurrency, | ||
| hookFeatures, | ||
| currentWorktreeBranch, | ||
| currentWorktreePath, | ||
| getPrimaryWorktreeBranch, | ||
| isPrimaryWorktreeBranch, | ||
| enableDependencyBlocking, | ||
| persistFeatureUpdate, | ||
| handleStartImplementation, | ||
| ]); |
There was a problem hiding this comment.
This useEffect has a very large dependency array, and notably includes hookFeatures. Since hookFeatures will change frequently (any time a feature is updated, added, or removed), this will cause the effect to tear down and set up the setInterval repeatedly. While the isActive flag prevents race conditions, this is inefficient and can make the component's behavior harder to reason about.
To improve performance and stability, I suggest using a ref to hold the latest hookFeatures array, similar to how autoModeRunningRef is used. This will allow you to remove hookFeatures from the dependency array, making the effect setup much more stable.
You can achieve this with a pattern like:
const featuresRef = useRef(hookFeatures);
useEffect(() => {
featuresRef.current = hookFeatures;
}, [hookFeatures]);
useEffect(() => {
// ... your auto-mode logic ...
// remove hookFeatures from this dependency array
const checkAndStartFeatures = async () => {
// ...
const currentFeatures = featuresRef.current;
const backlogFeatures = currentFeatures.filter(/* ... */);
// ...
};
// ...
}, [/* ... other stable dependencies ... */]);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/server/src/services/auto-mode-service.ts (1)
1205-1345: Eliminate code duplication between executeFeature and executeFeatureWithContext.The
executeFeatureWithContextmethod duplicates ~90% ofexecuteFeature's logic. The only differences are the prompt content and passing previous context torunAgent. Consider refactoring to a shared implementation:🔎 Suggested refactoring approach
Extract common logic into a shared private method:
private async executeFeatureInternal( projectPath: string, featureId: string, useWorktrees: boolean, isAutoMode: boolean, existingContext?: string ): Promise<void> { // Common logic here const prompt = existingContext ? this.buildResumePrompt(feature, existingContext) : this.buildFeaturePrompt(feature); await this.runAgent( workDir, featureId, prompt, abortController, projectPath, imagePaths, model, existingContext ); // ... rest of common logic }Then simplify both methods:
async executeFeature(projectPath: string, featureId: string, useWorktrees = false, isAutoMode = false) { // Check context logic here return this.executeFeatureInternal(projectPath, featureId, useWorktrees, isAutoMode); } async executeFeatureWithContext(projectPath: string, featureId: string, context: string, useWorktrees: boolean) { return this.executeFeatureInternal(projectPath, featureId, useWorktrees, false, context); }apps/app/src/components/views/board-view/hooks/use-board-column-features.ts (1)
99-102: Unknown status bypasses worktree filtering.Features with unknown status are added to backlog unconditionally (line 101), whereas explicit backlog items are filtered by
matchesWorktree(lines 90-93). This could cause unassigned features to appear in all worktrees.Suggested fix
} else { // Unknown status, default to backlog - map.backlog.push(f); + if (matchesWorktree) { + map.backlog.push(f); + } }apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts (1)
17-18: Remove unused propsprojectPathandonWorktreeCreatedfrom the hook interface and function signature.These props are destructured but never referenced in the function body. After the server-side worktree migration, they're no longer needed and should be removed from both the
UseBoardDragDropPropsinterface and the function parameters.Suggested fix
interface UseBoardDragDropProps { features: Feature[]; currentProject: { path: string; id: string } | null; runningAutoTasks: string[]; persistFeatureUpdate: ( featureId: string, updates: Partial<Feature> ) => Promise<void>; handleStartImplementation: (feature: Feature) => Promise<boolean>; - projectPath: string | null; - onWorktreeCreated?: () => void; } export function useBoardDragDrop({ features, currentProject, runningAutoTasks, persistFeatureUpdate, handleStartImplementation, - projectPath, - onWorktreeCreated, }: UseBoardDragDropProps) {apps/app/tests/worktree-integration.spec.ts (1)
2356-2448: Test incorrectly expects worktreePath to be set during edit; worktrees are created at execution time, not during feature editing.The test expects
worktreePathto be populated immediately after editing a feature to a new branch. However, the architecture has been refactored: worktrees are now created server-side at execution time based onfeature.branchName, not when editing. Theuse-board-actions.tshook explicitly notes thatgetOrCreateWorktreeForFeaturehas been removed, andhandleUpdateFeatureonly persistsbranchName—it does not create worktrees or setworktreePath.Update the test to:
- Remove assertions that
worktreePathexists after editing- Verify only that
branchNameis correctly set after editing- Add a separate test for worktree creation at execution time (when feature moves to "in_progress")
🧹 Nitpick comments (18)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (1)
195-206: Duplicate state reset logic.The
setTimeouthere resets state 200ms after close, but theuseEffectat lines 68-79 already resets the same state whenopenbecomes false. This duplication is redundant and could cause timing issues.Consider removing the
setTimeoutand relying solely on theuseEffectfor cleanup.🔎 Simplify by removing setTimeout
const handleClose = () => { if (operationCompleted) { onCreated(); } onOpenChange(false); - // Reset state after dialog closes - setTimeout(() => { - setTitle(""); - setBody(""); - setCommitMessage(""); - setBaseBranch("main"); - setIsDraft(false); - setError(null); - setPrUrl(null); - setBrowserUrl(null); - setShowBrowserFallback(false); - setOperationCompleted(false); - }, 200); };The
useEffectwill handle cleanup when the dialog closes.apps/app/src/hooks/use-auto-mode.ts (1)
188-195: Remove unusedsetAutoModeRunningfrom the dependency array.
setAutoModeRunningis included in the dependency array but is never used inside this effect. This appears to be a leftover from the removed lifecycle event handling. Keeping it doesn't break functionality (zustand setters are stable), but it's misleading and should be cleaned up.🔎 Apply this diff to remove the unused dependency:
return unsubscribe; }, [ projectId, addRunningTask, removeRunningTask, - setAutoModeRunning, addAutoModeActivity, getProjectIdFromPath, ]);apps/app/src/components/views/context-view.tsx (1)
216-235: Consider consolidating the dialog reset logic.The state reset code is duplicated in both success and error paths. Additionally, the comment on line 228 mentions
loadContextFilesfailing, but this catch block also handleswriteFileerrors—which may warrant different UX (e.g., showing an error toast when the file write fails).🔎 Suggested refactor using try/finally:
const handleAddFile = async () => { const contextPath = getContextPath(); if (!contextPath || !newFileName.trim()) return; + let writeSucceeded = false; try { const api = getElectronAPI(); let filename = newFileName.trim(); // Add default extension if not provided if (newFileType === "text" && !filename.includes(".")) { filename += ".md"; } const filePath = `${contextPath}/${filename}`; if (newFileType === "image" && uploadedImageData) { // Write image data await api.writeFile(filePath, uploadedImageData); } else { // Write text file with content (or empty if no content) await api.writeFile(filePath, newFileContent); } + writeSucceeded = true; + await loadContextFiles(); + } catch (error) { + console.error("Failed to add file:", error); + // Optionally show error toast to user here + } finally { // Close dialog and reset state setIsAddDialogOpen(false); setNewFileName(""); setNewFileType("text"); setUploadedImageData(null); setNewFileContent(""); setIsDropHovering(false); - - // Load files after dialog is closed - await loadContextFiles(); - } catch (error) { - console.error("Failed to add file:", error); - // Still close the dialog even if loadContextFiles fails - setIsAddDialogOpen(false); - setNewFileName(""); - setNewFileType("text"); - setUploadedImageData(null); - setNewFileContent(""); - setIsDropHovering(false); } };apps/server/src/routes/worktree/routes/list.ts (1)
11-11: Consider using async file existence check.
existsSyncis synchronous and could block the event loop on slow filesystems. For consistency with the async pattern used elsewhere in this handler, consider usingfs.promises.access.🔎 Suggested async alternative:
-import { existsSync } from "fs"; +import { access, constants } from "fs/promises"; + +async function pathExists(p: string): Promise<boolean> { + try { + await access(p, constants.F_OK); + return true; + } catch { + return false; + } +}Then replace
existsSync(current.path)withawait pathExists(current.path).Also applies to: 78-78
apps/server/src/services/auto-mode-service.ts (1)
63-70: Consider making context-aware execution explicit.The implicit check for existing context and automatic resume creates ambiguity about
executeFeature's behavior. Callers might expect fresh execution but get resumed execution instead. Consider:
- Add a
checkExistingContextparameter to let callers control this behavior- Or document this behavior clearly and ensure all call sites understand it
- Or move this logic to the caller level for explicit control
This helps prevent confusion and makes the execution model more predictable.
apps/server/src/routes/auto-mode/routes/resume-feature.ts (1)
35-36: Remove unnecessary empty finally block.The empty
.finally(() => {})serves no purpose and can be removed for cleaner code.🔎 Apply this diff:
autoModeService .resumeFeature(projectPath, featureId, useWorktrees ?? false) .catch((error) => { logger.error(`[AutoMode] Resume feature ${featureId} error:`, error); - }) - .finally(() => {}); + });apps/app/tests/utils/views/board.ts (1)
139-160: Test utility updated for new BranchSelector flow.The branch selection logic now matches the updated UI:
- Select "Other branch" radio option
- Wait for branch input to appear
- Fill the autocomplete with the branch name
The implementation correctly handles the two-step process. However, the hard-coded
waitForTimeoutcalls (300ms, 200ms) may be fragile in CI environments.Consider replacing fixed timeouts with state-based waits:
- await page.waitForTimeout(300); + await branchInput.waitFor({ state: "visible" }); // Now click on the branch input (autocomplete) const branchInput = page.locator('[data-testid="feature-input"]'); await branchInput.waitFor({ state: "visible", timeout: 5000 }); await branchInput.click(); // Wait for the popover to open - await page.waitForTimeout(300); + await page.locator('[cmdk-input]').waitFor({ state: "visible" });apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
48-51: Clarify cleanup logic in finally block.The finally block mentions "releasing the starting slot," but there's no visible slot acquisition/release logic in this handler. If slot management happens within
autoModeService.followUpFeature, consider updating the comment to reflect that or removing it if no cleanup is actually needed here.- .finally(() => { - // Release the starting slot when follow-up completes (success or error) - // Note: The feature should be in runningFeatures by this point - }); + .finally(() => { + // Cleanup handled by autoModeService + });apps/server/src/routes/auto-mode/routes/run-feature.ts (1)
36-39: Clarify cleanup logic in finally block.Similar to the follow-up route, the finally block mentions "releasing the starting slot" without visible slot management code in this handler. If this cleanup is handled internally by
autoModeService.executeFeature, update the comment or remove it.- .finally(() => { - // Release the starting slot when execution completes (success or error) - // Note: The feature should be in runningFeatures by this point - }); + .finally(() => { + // Cleanup handled by autoModeService + });apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
17-17: Consider removing unused import.
BranchAutocompleteis imported here but no longer used directly in this file since the component now usesBranchSelector(which internally usesBranchAutocomplete). Consider removing this import to keep the imports clean.🔎 Suggested fix
-import { BranchAutocomplete } from "@/components/ui/branch-autocomplete";apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
6-6: Remove unused import.The
cnutility is imported but not used in this component.🔎 Suggested fix
-import { cn } from "@/lib/utils";apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
52-60: Consider memoizingonRemovedWorktreesat call sites to avoid effect re-runs.Including
onRemovedWorktreesin the dependency array is correct, but if the caller passes an inline function, the effect will re-run on every parent render. Document this expectation or wrap the callback inuseCallbackat call sites.apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (2)
112-121: Consider resettinguseCurrentBranchwhen dialog closes.When
featurebecomesnull,useCurrentBranchretains its previous value. While it gets re-initialized on the next feature, explicitly resetting it in theelsebranch (like you do forsetEditFeaturePreviewMap) would be more defensive.Suggested fix
useEffect(() => { setEditingFeature(feature); if (!feature) { setEditFeaturePreviewMap(new Map()); setShowEditAdvancedOptions(false); + setUseCurrentBranch(true); } else { // If feature has no branchName, default to using current branch setUseCurrentBranch(!feature.branchName); } }, [feature]);
500-513: Validation condition duplicated.The disabled condition duplicates the validation at lines 126-136. Consider extracting to a shared variable for consistency:
Suggested refactor
+ const isBranchRequired = + useWorktrees && + editingFeature?.status === "backlog" && + !useCurrentBranch && + !editingFeature?.branchName?.trim(); const handleUpdate = () => { if (!editingFeature) return; - const isBranchSelectorEnabled = editingFeature.status === "backlog"; - if ( - useWorktrees && - isBranchSelectorEnabled && - !useCurrentBranch && - !editingFeature.branchName?.trim() - ) { + if (isBranchRequired) { toast.error("Please select a branch name"); return; } ... <HotkeyButton ... - disabled={ - useWorktrees && - editingFeature.status === "backlog" && - !useCurrentBranch && - !editingFeature.branchName?.trim() - } + disabled={isBranchRequired} >apps/app/src/components/views/board-view/hooks/use-board-actions.ts (1)
672-766: Consider extracting duplicated backlog filtering logic.The backlog filtering logic (lines 675-696) is duplicated with nearly identical code in
board-view.tsx(lines 487-512). This increases maintenance burden and risks divergence.Consider extracting to a shared utility
// In a shared utility file like lib/feature-utils.ts export function filterBacklogFeaturesByBranch( features: Feature[], currentWorktreeBranch: string | null, projectPath: string | null, isPrimaryWorktreeBranch: (path: string, branch: string) => boolean, getPrimaryWorktreeBranch: (path: string) => string | null ): Feature[] { const primaryBranch = projectPath ? getPrimaryWorktreeBranch(projectPath) : null; return features.filter((f) => { if (f.status !== "backlog") return false; const featureBranch = f.branchName || primaryBranch || "main"; if (!currentWorktreeBranch || (projectPath && isPrimaryWorktreeBranch(projectPath, currentWorktreeBranch))) { return !f.branchName || (projectPath && isPrimaryWorktreeBranch(projectPath, featureBranch)); } return featureBranch === currentWorktreeBranch; }); }apps/app/src/components/views/board-view.tsx (3)
446-594: Large effect with many dependencies - consider extraction.This auto mode polling effect is quite large (~150 lines) with 13 dependencies. While the logic is correct, the size makes it harder to test and maintain.
Additionally, the
hookFeaturesin the dependency array will cause the effect to re-run on every feature change, potentially clearing and restarting the interval frequently.Consider extracting to a custom hook
Extract this logic into a dedicated hook like
useAutoModeBacklogProcessor:// In a new file: hooks/use-auto-mode-backlog-processor.ts export function useAutoModeBacklogProcessor({ isRunning, currentProject, features, runningAutoTasks, maxConcurrency, // ... other deps }) { // Move the entire effect logic here // This makes it testable in isolation }Also consider using a more stable reference for features comparison (e.g., feature IDs only for the check interval trigger).
474-481: Pending features tracking may drift from actual state.The pending features count is added to
currentRunningbut if the asynchandleStartImplementationfails before adding topendingFeaturesRef, or if the event listener misses an event, the count could become stale.This is mitigated by the 3-second polling interval, but there's no explicit staleness handling or periodic cleanup of the pending set.
Consider adding staleness tracking
// Track when features were added to pending const pendingFeaturesRef = useRef<Map<string, number>>(new Map()); // In the check function, clear stale pending entries const now = Date.now(); const STALE_THRESHOLD = 30000; // 30 seconds for (const [id, timestamp] of pendingFeaturesRef.current) { if (now - timestamp > STALE_THRESHOLD) { pendingFeaturesRef.current.delete(id); } }
905-919: Duplicated branch reset logic.The branch reset logic in
onDeleted(lines 907-914) duplicates what's inhandleRemovedWorktrees(lines 290-301). Both reset feature branch assignments when worktrees are deleted.Consider consolidating the reset logic
The
onDeletedcallback could delegate tohandleRemovedWorktrees:onDeleted={(deletedWorktree, _deletedBranch) => { - // Reset features that were assigned to the deleted worktree (by branch) - hookFeatures.forEach((feature) => { - // Match by branch name since worktreePath is no longer stored - if (feature.branchName === deletedWorktree.branch) { - // Reset the feature's branch assignment - persistFeatureUpdate(feature.id, { - branchName: null as unknown as string | undefined, - }); - } - }); + handleRemovedWorktrees([{ path: deletedWorktree.path, branch: deletedWorktree.branch }]); setWorktreeRefreshKey((k) => k + 1); setSelectedWorktreeForAction(null); }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
apps/app/package.json(1 hunks)apps/app/src/components/ui/autocomplete.tsx(3 hunks)apps/app/src/components/ui/branch-autocomplete.tsx(3 hunks)apps/app/src/components/ui/radio-group.tsx(1 hunks)apps/app/src/components/ui/switch.tsx(1 hunks)apps/app/src/components/views/board-view.tsx(17 hunks)apps/app/src/components/views/board-view/board-header.tsx(3 hunks)apps/app/src/components/views/board-view/components/kanban-card.tsx(1 hunks)apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx(10 hunks)apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx(1 hunks)apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx(4 hunks)apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx(7 hunks)apps/app/src/components/views/board-view/dialogs/index.ts(1 hunks)apps/app/src/components/views/board-view/hooks/use-board-actions.ts(13 hunks)apps/app/src/components/views/board-view/hooks/use-board-column-features.ts(3 hunks)apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts(8 hunks)apps/app/src/components/views/board-view/hooks/use-board-effects.ts(2 hunks)apps/app/src/components/views/board-view/kanban-board.tsx(4 hunks)apps/app/src/components/views/board-view/shared/branch-selector.tsx(1 hunks)apps/app/src/components/views/board-view/shared/index.ts(1 hunks)apps/app/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx(1 hunks)apps/app/src/components/views/board-view/worktree-panel/hooks/use-running-features.ts(1 hunks)apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts(4 hunks)apps/app/src/components/views/board-view/worktree-panel/types.ts(1 hunks)apps/app/src/components/views/board-view/worktree-panel/worktree-panel.tsx(3 hunks)apps/app/src/components/views/context-view.tsx(1 hunks)apps/app/src/hooks/use-auto-mode.ts(1 hunks)apps/app/src/lib/electron.ts(0 hunks)apps/app/src/lib/http-api-client.ts(0 hunks)apps/app/src/store/app-store.ts(7 hunks)apps/app/src/types/electron.d.ts(3 hunks)apps/app/tests/utils/core/constants.ts(1 hunks)apps/app/tests/utils/views/board.ts(1 hunks)apps/app/tests/worktree-integration.spec.ts(8 hunks)apps/server/src/routes/auto-mode/index.ts(0 hunks)apps/server/src/routes/auto-mode/routes/follow-up-feature.ts(2 hunks)apps/server/src/routes/auto-mode/routes/resume-feature.ts(2 hunks)apps/server/src/routes/auto-mode/routes/run-feature.ts(1 hunks)apps/server/src/routes/auto-mode/routes/start.ts(0 hunks)apps/server/src/routes/auto-mode/routes/stop.ts(0 hunks)apps/server/src/routes/running-agents/routes/index.ts(0 hunks)apps/server/src/routes/worktree/routes/list.ts(3 hunks)apps/server/src/services/auto-mode-service.ts(21 hunks)apps/server/src/services/feature-loader.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/server/src/routes/running-agents/routes/index.ts
- apps/server/src/routes/auto-mode/routes/start.ts
- apps/server/src/routes/auto-mode/routes/stop.ts
- apps/app/src/lib/http-api-client.ts
- apps/server/src/routes/auto-mode/index.ts
- apps/app/src/lib/electron.ts
🧰 Additional context used
🧬 Code graph analysis (15)
apps/app/src/components/ui/radio-group.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
apps/app/src/components/views/board-view/shared/branch-selector.tsx (3)
apps/app/src/components/ui/label.tsx (1)
Label(24-24)apps/app/src/components/ui/radio-group.tsx (2)
RadioGroup(44-44)RadioGroupItem(44-44)apps/app/src/components/ui/branch-autocomplete.tsx (1)
BranchAutocomplete(18-56)
apps/app/src/components/ui/switch.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)
apps/app/src/components/views/board-view/board-header.tsx (2)
apps/app/src/components/ui/label.tsx (1)
Label(24-24)apps/app/src/components/ui/switch.tsx (1)
Switch(29-29)
apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (5)
apps/app/src/store/app-store.ts (2)
Feature(278-302)AgentModel(258-258)apps/server/src/services/feature-loader.ts (1)
Feature(15-30)apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/utils.ts (1)
modelSupportsThinking(12-15)apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
BranchSelector(19-97)
apps/server/src/routes/worktree/routes/list.ts (1)
apps/server/src/routes/worktree/common.ts (1)
normalizePath(21-23)
apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (3)
apps/app/src/components/views/board-view/dialogs/index.ts (1)
ArchiveAllVerifiedDialog(4-4)apps/app/src/components/ui/dialog.tsx (5)
Dialog(165-165)DialogContent(167-167)DialogTitle(173-173)DialogDescription(168-168)DialogFooter(169-169)apps/app/src/components/ui/button.tsx (1)
Button(116-116)
apps/app/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (3)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/utils.ts (1)
modelSupportsThinking(12-15)apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
BranchSelector(19-97)
apps/app/src/types/electron.d.ts (1)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (6)
apps/app/tests/utils/views/board.ts (1)
addFeature(189-197)apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/utils.ts (1)
truncateDescription(32-37)apps/server/src/services/auto-mode-service.ts (1)
followUpFeature(252-450)apps/app/src/lib/dependency-resolver.ts (1)
getBlockingDependencies(209-221)apps/server/src/lib/dependency-resolver.ts (1)
getBlockingDependencies(209-221)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (1)
apps/app/src/components/ui/button.tsx (1)
Button(116-116)
apps/server/src/services/auto-mode-service.ts (3)
apps/server/src/lib/automaker-paths.ts (1)
getFeatureDir(28-30)apps/server/src/lib/model-resolver.ts (2)
resolveModelString(33-62)DEFAULT_MODELS(22-24)apps/server/src/lib/error-handler.ts (1)
classifyError(61-84)
apps/app/src/components/views/board-view.tsx (4)
apps/app/src/lib/utils.ts (1)
pathsEqual(51-54)apps/app/src/lib/electron.ts (2)
getElectronAPI(542-552)AutoModeEvent(47-47)apps/app/src/types/electron.d.ts (1)
AutoModeEvent(162-217)apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (1)
ArchiveAllVerifiedDialog(21-54)
🪛 GitHub Actions: Test Suite
apps/server/src/services/auto-mode-service.ts
[error] 1-1: startAutoLoop / stopAutoLoop APIs are missing on AutoModeService. Tests expect these methods to exist and be callable.
⏰ 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 (77)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (3)
56-67: LGTM! Proper state reset on dialog open.Resetting the PR result states (
prUrl,browserUrl,showBrowserFallback) when opening the dialog prevents stale success messages from previous operations, ensuring a clean slate for each worktree.
112-113: Good UX improvement to keep dialog open on success.Deferring
onCreated()until the user explicitly closes the dialog allows them to see the success state and interact with the PR link. This pairs well with the new two-button UI.
239-250: Good UX pattern with explicit Close action.The two-button layout gives users clear options after PR creation: view the PR immediately or close the dialog. This is more intuitive than relying on implicit close actions.
apps/app/src/hooks/use-auto-mode.ts (6)
1-31: LGTM!The imports are appropriate, and using
useShallowfor the store selector is the correct approach to prevent unnecessary re-renders when extracting multiple values from zustand state.
33-55: LGTM!The helper function and memoized state derivation are well-implemented with correct dependencies and appropriate default values for missing state.
208-221: LGTM!The stop function's behavior is intentional and well-documented. Allowing running tasks to complete naturally rather than aborting them is a good UX choice. The inline comment clearly explains the rationale for not clearing running tasks.
223-258: LGTM!The
stopFeaturefunction correctly uses the Electron API to stop individual features, with proper error handling and activity logging. The error propagation viathrowallows callers to handle failures appropriately.
260-268: LGTM!The hook's public API is clean and provides all necessary values for consumers to manage and monitor auto mode state.
197-206: Feature pickup is properly handled in board-view.tsx.The
start()function correctly delegates feature pickup to board-view.tsx, which implements the full auto mode feature selection logic. WhenautoMode.isRunningchanges to true, board-view.tsx activates an effect that periodically checks for backlog features and starts them while respecting themaxConcurrencylimit and dependency constraints.Regarding
maxConcurrencyin the dependency array: while it's only used for logging in thestart()callback, this is a negligible concern since the callback has no functional impact from recreation.Likely an incorrect or invalid review comment.
apps/app/src/components/views/board-view/hooks/use-board-effects.ts (2)
1-1: LGTM! Cleanup of unused import.The removal of the
useAutoModeimport aligns with the architectural shift away from local auto-mode state management in this hook.
90-122: The removal ofsetAutoModeRunningfrom the sync effect is correct and intentional.The architecture deliberately separates
isRunning(session state controlled by user toggle) fromrunningTasks(execution state). ThesyncRunningTaskseffect correctly syncs only the running tasks from the backend on mount; it doesn't need to updateisRunningbecause that state represents the user's session toggle, not backend execution state. When auto-mode is stopped,isRunningis set to false to prevent new tasks from being picked up, while existing tasks continue running naturally and are removed via completion events. No UI inconsistency occurs because components use the appropriate state for their purpose:isRunningcontrols the polling loop, andrunningTasksmarks executing features.Likely an incorrect or invalid review comment.
apps/app/tests/utils/core/constants.ts (1)
100-100: LGTM!The test ID update aligns with the new BranchSelector component conventions. The constant name
featureBranchInputis now slightly mismatched with its value"feature-input", but this is a minor inconsistency that doesn't affect functionality.apps/app/src/components/ui/autocomplete.tsx (1)
38-38: LGTM!Clean implementation of the error state prop with appropriate visual styling. The conditional styling using
border-destructivefollows common form validation patterns.For enhanced accessibility, consider adding
aria-invalid={error}to the Button component, though this is optional since the visual indicator is clear.Also applies to: 62-62, 135-135
apps/server/src/routes/worktree/routes/list.ts (1)
75-94: LGTM! The worktree detection and pruning logic is well-structured.The implementation correctly:
- Identifies the main worktree (always first in
git worktree listoutput)- Skips existence checks for the main worktree
- Tracks removed worktrees separately and prunes them
- Returns removal information to the client
apps/app/src/components/views/board-view/worktree-panel/hooks/use-running-features.ts (1)
23-32: LGTM!Clean simplification of the matching logic. The branch-name-based matching aligns well with the PR's architectural shift away from worktreePath storage. The fallback to
worktree.isMainfor features without a branchName is a sensible default.apps/app/src/types/electron.d.ts (2)
307-307: LGTM: API surface updated to support branch-based worktree derivation.The shift from explicit
worktreePathparameters touseWorktreesboolean flags aligns with the PR's goal of deriving worktree paths fromfeature.branchNameat runtime. This simplifies the API surface and makes worktree usage more consistent.Also applies to: 353-353
595-598: LGTM: Good addition for worktree cleanup tracking.The
removedWorktreesfield enables the UI to react to worktrees that were cleaned up during listing operations. Based on the relevant code snippet fromuse-worktrees.ts, this is properly consumed via theonRemovedWorktreescallback to maintain consistency.apps/server/src/services/auto-mode-service.ts (2)
92-131: LGTM: Well-structured worktree derivation logic.The branch-name-driven worktree derivation properly:
- Checks for existing worktrees before creating new ones
- Falls back gracefully to project path when worktrees aren't used
- Uses
path.resolve()for cross-platform compatibility- Logs the worktree path for debugging
This aligns well with the PR's objective of moving from path-based to branch-based worktree management.
706-716: LGTM: Status method simplified to match new execution model.The removal of
autoLoopRunningaligns with the shift from persistent auto-loop to on-demand execution. The method now correctly reports only active feature execution state.apps/app/src/components/views/board-view/shared/index.ts (1)
7-7: LGTM: BranchSelector exported through barrel.Clean addition to expose the new BranchSelector component via the shared index.
apps/server/src/services/feature-loader.ts (1)
27-28: LGTM: branchName field enables runtime worktree derivation.The addition of the optional
branchNamefield with clear documentation about runtime derivation is well-designed. The optional nature maintains backward compatibility with existing feature data.apps/app/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
173-174: LGTM: Improved primary branch detection.Switching from hardcoded
"main"comparison to theisMainflag is more robust and handles projects where the primary branch has a different name (e.g.,master,trunk). The updated comment clearly explains the visibility logic.apps/app/src/components/ui/branch-autocomplete.tsx (1)
14-14: LGTM: Error state support added cleanly.The addition of the
errorprop with proper typing, default value, and forwarding to the underlyingAutocompletecomponent is well-implemented. This enables visual error feedback for invalid branch selections.Also applies to: 25-25, 48-48
apps/app/package.json (1)
48-48: LGTM: Radix UI dependencies added for new components.@radix-ui/react-radio-group@1.3.8 is the latest stable version, and @radix-ui/react-switch@1.2.6 is the latest stable version. Both are well-maintained primitives consistent with existing Radix UI usage in the project, with broad ecosystem adoption.
apps/app/src/components/views/board-view/worktree-panel/types.ts (2)
23-26: LGTM - Aligns with branchName-based worktree handling.Removing
worktreePathfromFeatureInfois consistent with the PR's shift to derive worktree paths at runtime frombranchName.
35-35: LGTM - Enables worktree removal cleanup.The new
onRemovedWorktreescallback allows parent components to react when worktrees are removed during refresh operations, supporting proper state synchronization.apps/app/src/components/ui/radio-group.tsx (1)
1-45: LGTM - Well-structured Radix UI wrapper.The RadioGroup implementation follows best practices:
- Proper forwardRef typing with Radix primitives
- Accessibility features via Radix (focus-visible, disabled states)
- Clean className composition with
cnutility- Appropriate displayName assignment
This component is used by the new BranchSelector to enable "Current Branch" vs "Other Branch" selection.
apps/app/src/components/ui/switch.tsx (1)
1-30: LGTM - Clean Switch component implementation.Well-implemented toggle control using Radix UI primitives:
- Proper TypeScript typing with forwardRef
- State-driven styling via
data-[state]attributes- Accessibility features (focus-visible ring, disabled opacity)
- Smooth transitions for thumb movement
Used by BoardHeader for auto mode toggling per the PR changes.
apps/app/src/components/views/board-view/dialogs/index.ts (2)
4-4: LGTM - Semantic improvement from delete to archive.Renaming from
DeleteAllVerifiedDialogtoArchiveAllVerifiedDialogbetter reflects non-destructive feature management, where verified features are archived rather than permanently deleted.
8-8: LGTM - New follow-up dialog export.Adding
FollowUpDialogexport supports the feature refinement workflow introduced in this PR.apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
15-22: LGTM - Transitioned to branchName-based worktree derivation.Replacing
worktreePathwithuseWorktreesaligns with the PR's objective to derive worktree context at runtime fromfeature.branchName. The default oftrueforuseWorktreesmaintains consistency with the new architecture.apps/server/src/routes/auto-mode/routes/run-feature.ts (1)
15-27: LGTM - Simplified to branchName-based execution.Removing
worktreePathfrom the request parameters and relying onuseWorktrees(defaulting tofalsefor backward compatibility) aligns with the architecture shift. The executeFeature service method now derives worktree context fromfeature.branchNamewhen needed.apps/app/src/components/views/board-view/components/kanban-card.tsx (1)
256-271: Clarify the intent behind movingskipTestsvalidation from isDraggable to the drag handler.The
skipTestscheck was removed from the isDraggable condition and replaced with only checking!isCurrentAutoTask. However, the drag handler (use-board-drag-drop.ts) still validatesskipTestsfor specific transitions:
- Line 157-160: Allows
skipTestsfeatures inin_progressto be manually verified via drag toverified- Line 169-194: Handles
skipTeststransitions betweenverifiedandwaiting_approvalThis architectural change enables manual verification workflows, but it means non-skipTests (TDD) features in
in_progressare now UI-draggable even though they have no valid drop targets in the handler. Confirm whether this is intentional or if TDD features should remain non-draggable.apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (1)
21-54: LGTM! Clean dialog implementation.The component is well-structured with proper accessibility (DialogTitle, DialogDescription) and test IDs. One minor observation: the
onConfirmhandler doesn't close the dialog - this delegates closing responsibility to the parent, which is a valid pattern but ensure the parent handles this correctly.apps/app/src/components/views/board-view/kanban-board.tsx (1)
56-56: LGTM! Consistent rename from delete to archive semantics.The prop rename, icon change, label update, and test-id modification are all consistent with the archive-based approach. The changes are well-coordinated.
Also applies to: 86-86, 118-124
apps/app/src/components/views/board-view/board-header.tsx (1)
66-79: LGTM! Clean refactor to Switch-based toggle.The consolidation from two separate callbacks (
onStartAutoMode/onStopAutoMode) to a singleonAutoModeToggle(enabled: boolean)simplifies the API. The Switch component with properly associated Label provides good UX and accessibility.apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (2)
136-152: LGTM! Clear branching logic with good documentation.The convention of using empty string to mean "use current branch" is well-documented in comments (lines 148-149). The validation at lines 136-140 correctly prevents submission when "other branch" is selected without a branch name.
379-391: LGTM! Clean BranchSelector integration.The
BranchSelectoris properly wired with all required props. The conditional rendering based onuseWorktreesis appropriate.apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
19-97: LGTM! Well-designed branch selector component.The component provides a clean UX for branch selection with:
- RadioGroup for clear choice between current and other branch
- Conditional BranchAutocomplete when "other branch" is selected
- Inline validation with error state and message
- Contextual help text that adapts to disabled/selection state
- Proper accessibility with Label associations and test IDs
The prop interface is well-designed for reuse across dialogs.
apps/app/src/store/app-store.ts (3)
299-301: LGTM! Clean simplification of Feature type.Removing
worktreePathin favor of runtime derivation frombranchNamereduces state duplication. The comment clearly documents the new approach.
1394-1404: LGTM! Clean helper implementations.Both
isPrimaryWorktreeBranchandgetPrimaryWorktreeBranchcorrectly handle edge cases:
- Empty worktrees array returns
false/nullrespectively via optional chaining- Properly identifies the main worktree using
isMainflag
2263-2264: Good safety decision.Not persisting
autoModeByProjectensures auto-mode is always OFF on app refresh, preventing unexpected automated actions after restart. The comment clearly documents this intentional behavior.apps/app/src/components/views/board-view/hooks/use-board-column-features.ts (1)
106-134: Dependency-aware backlog sorting looks correct.The logic properly separates blocked from unblocked features and maintains dependency order within each group when
enableDependencyBlockingis true.apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (2)
24-46: LGTM!The
fetchWorktreesfunction cleanly returnsremovedWorktreesfrom the API response and properly handles errors by returningundefined.
69-74: LGTM!The fallback to the primary worktree's branch (or
"main"if not yet loaded) is appropriate for initialization.apps/app/src/components/views/board-view/worktree-panel/worktree-panel.tsx (2)
165-170: LGTM!The manual refresh button correctly handles
removedWorktreesindependently from therefreshTrigger-based effect, as both are valid entry points for refresh.
29-37: Clean integration with the updated hook.The
onRemovedWorktreescallback is properly threaded from props to the hook.apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (2)
145-161: LGTM!The
finalBranchNamelogic correctly uses empty string to signal "use current branch" and the update payload is properly constructed.
369-385: LGTM!The
BranchSelectoris correctly integrated with proper props, and disabling for non-backlog features prevents invalid state transitions.apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts (4)
68-78: LGTM!The simplified check correctly prevents dragging only when a task is actively running, while allowing manual intervention for stalled in-progress items.
99-110: Clean delegation to server-side worktree handling.The comment clarifies that worktree creation is now server-side, and the implementation correctly delegates to
handleStartImplementationwithout client-side worktree setup.
143-168: LGTM!The
in_progresshandling correctly allows moving to backlog for any feature, while restricting direct-to-verified transitions toskipTestsfeatures only, preserving TDD workflow integrity.
169-218: Logic is correct but conditionals are complex.The ordering of
skipTestscheck (line 169) beforeverifiedstatus check (line 195) works correctly but requires careful reading. The current structure handles all cases, though future maintainers might benefit from a brief comment explaining the condition priority.apps/app/src/components/views/board-view/hooks/use-board-actions.ts (9)
79-86: LGTM - Clean removal of client-side worktree creation.The comment clearly documents the architectural change where worktrees are now created server-side at execution time based on
feature.branchName. This aligns with the server-sidefollowUpFeatureimplementation shown in the relevant snippets.
100-119: LGTM - Simplified branch handling for feature creation.The logic correctly handles empty string as "use current branch" (resulting in
undefined) while preserving explicit branch names. The dependency array is appropriately updated.
137-152: LGTM - Consistent branch handling in feature updates.The update logic mirrors the add logic, maintaining consistency in how empty branch names are handled.
217-239: LGTM - Server-side workDir derivation.The comments clearly document that the server derives
workDirfromfeature.branchNameat execution time, which aligns with the server-side implementation inauto-mode-service.ts.
257-288: LGTM - Dependency blocking warning with proper description mapping.The blocking dependency check and warning toast are well implemented. The feature descriptions are properly truncated for display.
436-466: LGTM - Simplified follow-up call without worktreePath.The server-side derivation pattern is consistently applied. The comments clearly document the change.
483-488: LGTM - Commit feature updated for server-side path derivation.Consistent with other handlers - server derives workDir from
feature.branchName.
722-756: LGTM - Dependency blocking integration in backlog sorting.The sorting logic correctly prioritizes unblocked features while respecting priority ordering within each group. The early return when no eligible features exist provides a clear user message.
768-800: LGTM - Renamed from delete to archive with appropriate behavior.The rename from
handleDeleteAllVerifiedtohandleArchiveAllVerifiedcorrectly reflects the new behavior of setting status tocompletedrather than deleting features. The toast message is appropriately updated.apps/app/tests/worktree-integration.spec.ts (5)
744-780: LGTM - Test expectations updated for execution-time worktree creation.The test correctly expects
worktreePathto beundefinedwhen adding a feature to backlog, aligning with the new architecture where worktrees are created at execution time.
782-843: LGTM - Comprehensive test for branch name storage without worktree creation.The test properly verifies:
- Branch doesn't exist before feature creation
- Branch still doesn't exist after adding to backlog (deferred creation)
branchNameis stored in feature dataworktreePathremains undefined- Feature status is
backlogThis thoroughly validates the new execution-time worktree creation model.
899-959: LGTM - Test updated for branch-based matching.The test correctly:
- Expects
branchNameto be stored- Expects
worktreePathto be undefined (not stored at backlog time)- Verifies branch is reset when worktree is deleted
- Validates unassigned features only appear on primary worktree
1079-1087: LGTM - Test for pre-filled branch in BranchSelector.The test correctly validates the new BranchSelector UI shows the current worktree's branch in the label.
2536-2606: Same inconsistency: Test expects worktreePath on branch change.Similar to the previous test, this expects
worktreePathto be populated when editing a feature to use an existing branch (line 2605). Verify this aligns with the intended architecture.apps/app/src/components/views/board-view.tsx (10)
3-15: LGTM - Clean imports for new functionality.The added imports for
useRef,AutoModeEvent, andgetBlockingDependenciessupport the new auto mode orchestration and dependency blocking features.
77-81: LGTM - Store selectors for branch handling.The destructured functions
useWorktrees,enableDependencyBlocking,isPrimaryWorktreeBranch, andgetPrimaryWorktreeBranchare properly obtained from the store.
98-99: LGTM - State renamed for archive flow.The state variable is appropriately renamed to
showArchiveAllVerifiedDialogto reflect the archive semantics.
330-346: LGTM - Properly derived current worktree branch.The
selectedWorktreeandcurrentWorktreeBranchare correctly computed from the worktrees list, handling both primary and specific worktree selections.
400-444: LGTM - Auto mode event handling with proper cleanup.The event listener correctly:
- Filters events by project path
- Removes features from pending set on start/complete/error
- Returns the unsubscribe function for cleanup
664-670: LGTM - Clean callback for auto mode toggle.The single
onAutoModeTogglecallback simplifies the interface compared to separate start/stop handlers.
701-701: LGTM - Wired handleRemovedWorktrees to WorktreePanel.The callback is properly passed to handle worktree removal events.
799-812: LGTM - currentBranch prop passed to dialogs.Both
AddFeatureDialogandEditFeatureDialogreceive thecurrentBranchprop for BranchSelector context.
829-838: LGTM - Archive dialog integration.The
ArchiveAllVerifiedDialogis properly wired with state, count, and confirmation handler.
540-550: The code does not have a race condition here. ThepersistFeatureUpdatecall that assignsbranchName(lines 548-551) and the one inhandleStartImplementation(line 276) update different fields.handleStartImplementationonly updatesstatusandstartedAt, notbranchName.Likely an incorrect or invalid review comment.
| matchesWorktree = projectPath | ||
| ? useAppStore.getState().isPrimaryWorktreeBranch(projectPath, featureBranch) | ||
| : false; |
There was a problem hiding this comment.
Calling useAppStore.getState() inside useMemo won't react to store changes.
getState() returns the current snapshot but doesn't subscribe to updates. If isPrimaryWorktreeBranch logic changes in the store, this memo won't recalculate. Extract this via a selector at the component level:
Suggested approach
export function useBoardColumnFeatures({
...
}: UseBoardColumnFeaturesProps) {
+ const isPrimaryWorktreeBranch = useAppStore((s) => s.isPrimaryWorktreeBranch);
+
const columnFeaturesMap = useMemo(() => {
...
- matchesWorktree = projectPath
- ? useAppStore.getState().isPrimaryWorktreeBranch(projectPath, featureBranch)
- : false;
+ matchesWorktree = projectPath
+ ? isPrimaryWorktreeBranch(projectPath, featureBranch)
+ : false;
...
- }, [features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, projectPath]);
+ }, [features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, projectPath, isPrimaryWorktreeBranch]);📝 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.
| matchesWorktree = projectPath | |
| ? useAppStore.getState().isPrimaryWorktreeBranch(projectPath, featureBranch) | |
| : false; | |
| export function useBoardColumnFeatures({ | |
| ... | |
| }: UseBoardColumnFeaturesProps) { | |
| const isPrimaryWorktreeBranch = useAppStore((s) => s.isPrimaryWorktreeBranch); | |
| const columnFeaturesMap = useMemo(() => { | |
| ... | |
| matchesWorktree = projectPath | |
| ? isPrimaryWorktreeBranch(projectPath, featureBranch) | |
| : false; | |
| ... | |
| }, [features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, projectPath, isPrimaryWorktreeBranch]); |
🤖 Prompt for AI Agents
In apps/app/src/components/views/board-view/hooks/use-board-column-features.ts
around lines 71 to 73, you're calling useAppStore.getState() inside useMemo
which only reads a snapshot and won't subscribe to updates; replace this with a
store selector that subscribes (e.g. call useAppStore(selector) at the top level
to obtain isPrimaryWorktreeBranch(projectPath, featureBranch) or a boolean
result), then use that reactive value inside useMemo (or remove useMemo if no
longer needed) so the memo recomputes when the store value changes; ensure
projectPath and featureBranch are passed into the selector or listed as
dependencies so updates propagate.
|
|
||
| // Get all features to check blocking dependencies against | ||
| const allFeatures = features; | ||
| const enableDependencyBlocking = useAppStore.getState().enableDependencyBlocking; |
There was a problem hiding this comment.
Same reactivity issue with enableDependencyBlocking.
This value is read via getState() inside useMemo, so toggling the dependency-blocking setting won't trigger a re-render. Extract it as a selector:
Suggested fix
export function useBoardColumnFeatures({
...
}: UseBoardColumnFeaturesProps) {
+ const enableDependencyBlocking = useAppStore((s) => s.enableDependencyBlocking);
...
const columnFeaturesMap = useMemo(() => {
...
- const enableDependencyBlocking = useAppStore.getState().enableDependencyBlocking;
if (enableDependencyBlocking) {
...
- }, [features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, projectPath]);
+ }, [features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, projectPath, enableDependencyBlocking]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/app/src/components/views/board-view/hooks/use-board-column-features.ts
around line 114, the code reads enableDependencyBlocking via
useAppStore.getState() inside a useMemo which prevents reactivity when the
setting changes; replace the getState() call with a reactive selector (e.g.
const enableDependencyBlocking = useAppStore(state =>
state.enableDependencyBlocking)) declared at the top of the hook/component
(outside the useMemo) so toggling the dependency-blocking setting triggers
re-renders and the memoized logic sees updates.
- Refactored DialogContent component to use forwardRef for better integration with refs. - Enhanced auto mode service by introducing an auto loop for processing features concurrently. - Updated error handling and feature management logic to streamline operations. - Cleaned up code formatting and improved readability across various components and services.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/server/src/services/auto-mode-service.ts (1)
735-759:startAutoLoopandstopAutoLoopare now implemented.The past review flagged that tests expect these methods. The methods now exist with the expected signatures. Verify that tests are updated to match this implementation—
startAutoLoop(projectPath, maxConcurrency)returns aPromise<void>andstopAutoLoop()returnsPromise<number>.#!/bin/bash # Check test files for startAutoLoop/stopAutoLoop usage patterns rg -n -A3 'startAutoLoop|stopAutoLoop' --type=ts -g '*test*' -g '*spec*'
🧹 Nitpick comments (6)
apps/app/tests/utils/views/board.ts (2)
143-166: Replace hard-coded timeouts with Playwright's built-in waiting mechanisms.The branch selection flow uses multiple hard-coded timeouts (
page.waitForTimeout(300),page.waitForTimeout(200)), which can cause flaky tests in CI environments. Playwright provides more reliable alternatives.🔎 Consider this refactor to improve test stability:
if (options?.branch) { // First, select "Other branch" radio option if not already selected const otherBranchRadio = page .locator('[data-testid="feature-radio-group"]') .locator('[id="feature-other"]'); await otherBranchRadio.waitFor({ state: "visible", timeout: 5000 }); await otherBranchRadio.click(); - // Wait for the branch input to appear - await page.waitForTimeout(300); // Now click on the branch input (autocomplete) const branchInput = page.locator('[data-testid="feature-input"]'); await branchInput.waitFor({ state: "visible", timeout: 5000 }); await branchInput.click(); - // Wait for the popover to open - await page.waitForTimeout(300); + // Wait for the command input to be visible and enabled + const commandInput = page.locator("[cmdk-input]"); + await commandInput.waitFor({ state: "visible", timeout: 5000 }); - // Type in the command input - const commandInput = page.locator("[cmdk-input]"); await commandInput.fill(options.branch); // Press Enter to select/create the branch await commandInput.press("Enter"); - // Wait for popover to close - await page.waitForTimeout(200); + // Wait for popover to close by checking it's hidden + await page.locator('[role="dialog"]').waitFor({ state: "hidden", timeout: 5000 }).catch(() => {}); }
225-225: Consider input validation for defensive coding.Static analysis flagged that constructing RegExp from variable input could lead to ReDoS attacks. While this is test code with controlled input (low risk), consider adding a simple validation or escaping helper if this pattern is reused in production code.
Example defensive approach:
// Helper to escape regex special characters function escapeRegExp(str: string): string { return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } // Usage const branchButton = page.getByRole("button", { name: new RegExp(escapeRegExp(branchName), "i"), });Based on static analysis hints.
Also applies to: 253-253
apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (1)
46-49: Consider adding loading state to prevent duplicate submissions.The Archive button calls
onConfirmwithout any loading indicator or disabled state. If the operation is asynchronous, users could click multiple times, potentially causing duplicate archive operations.🔎 Example implementation with loading state:
+import { useState } from "react"; + export function ArchiveAllVerifiedDialog({ open, onOpenChange, verifiedCount, onConfirm, }: ArchiveAllVerifiedDialogProps) { + const [isLoading, setIsLoading] = useState(false); + + const handleConfirm = async () => { + setIsLoading(true); + try { + await onConfirm(); + } finally { + setIsLoading(false); + } + }; + return ( <Dialog open={open} onOpenChange={onOpenChange}> <DialogContent data-testid="archive-all-verified-dialog"> {/* ... */} <DialogFooter> - <Button variant="ghost" onClick={() => onOpenChange(false)}> + <Button variant="ghost" onClick={() => onOpenChange(false)} disabled={isLoading}> Cancel </Button> - <Button variant="default" onClick={onConfirm} data-testid="confirm-archive-all-verified"> + <Button variant="default" onClick={handleConfirm} disabled={isLoading} data-testid="confirm-archive-all-verified"> <Archive className="w-4 h-4 mr-2" /> - Archive All + {isLoading ? "Archiving..." : "Archive All"} </Button> </DialogFooter> </DialogContent> </Dialog> ); }apps/server/src/services/auto-mode-service.ts (3)
277-283: Inconsistent default foruseWorktreesparameter.
followUpFeaturedefaultsuseWorktreestotrue(line 282) whileexecuteFeatureandresumeFeaturedefault tofalse. This inconsistency could lead to unexpected behavior. Consider aligning the defaults or documenting why they differ.
787-797:getStatus()doesn't reflect auto-loop state.The
isRunningfield only checksrunningFeatures.size > 0, which won't indicate when the auto-loop is active but waiting for pending features. Consider includingthis.autoLoop?.isRunningfor a complete status picture.🔎 Suggested enhancement:
getStatus(): { isRunning: boolean; + autoLoopRunning: boolean; runningFeatures: string[]; runningCount: number; } { return { - isRunning: this.runningFeatures.size > 0, + isRunning: this.runningFeatures.size > 0 || (this.autoLoop?.isRunning ?? false), + autoLoopRunning: this.autoLoop?.isRunning ?? false, runningFeatures: Array.from(this.runningFeatures.keys()), runningCount: this.runningFeatures.size, }; }
1408-1432: Consider extracting worktree setup to a helper method.The worktree resolution logic (find existing → setup new → resolve path) is duplicated in
executeFeature,followUpFeature, andexecuteFeatureWithContext. Extracting this to a private helper likeresolveWorktreePath(projectPath, featureId, branchName, useWorktrees)would reduce duplication and ensure consistent behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/app/src/components/ui/dialog.tsx(3 hunks)apps/app/src/components/ui/radio-group.tsx(1 hunks)apps/app/src/components/ui/switch.tsx(1 hunks)apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx(1 hunks)apps/app/tests/utils/views/board.ts(4 hunks)apps/server/src/routes/app-spec/index.ts(1 hunks)apps/server/src/routes/setup/routes/delete-api-key.ts(1 hunks)apps/server/src/services/auto-mode-service.ts(24 hunks)docs/server/route-organization.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/server/src/routes/setup/routes/delete-api-key.ts
- docs/server/route-organization.md
- apps/server/src/routes/app-spec/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/app/src/components/ui/radio-group.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (3)
apps/app/src/components/views/board-view/dialogs/index.ts (1)
ArchiveAllVerifiedDialog(4-4)apps/app/src/components/ui/dialog.tsx (6)
Dialog(170-170)DialogContent(172-172)DialogHeader(175-175)DialogTitle(178-178)DialogDescription(173-173)DialogFooter(174-174)apps/app/src/components/ui/button.tsx (1)
Button(116-116)
apps/server/src/services/auto-mode-service.ts (6)
apps/server/src/lib/events.ts (1)
EventEmitter(31-34)apps/server/src/services/feature-loader.ts (1)
getFeatureDir(167-169)apps/server/src/lib/error-handler.ts (2)
isAbortError(17-22)classifyError(61-84)apps/server/src/lib/automaker-paths.ts (1)
getFeatureDir(28-30)apps/app/src/config/model-config.ts (2)
resolveModelString(29-50)DEFAULT_MODELS(21-24)apps/server/src/lib/model-resolver.ts (2)
resolveModelString(33-62)DEFAULT_MODELS(22-24)
apps/app/src/components/ui/switch.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
🪛 ast-grep (0.40.0)
apps/app/tests/utils/views/board.ts
[warning] 225-225: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(branchName, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 253-253: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(branchName, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (14)
apps/app/src/components/ui/switch.tsx (1)
1-29: Well-structured Switch component following established patterns.The implementation correctly wraps Radix UI primitives with proper TypeScript typing, ref forwarding, and className merging. The state-driven styling via
data-[state=*]attributes and the thumb translation math (20px translation for a 20px thumb within the container) are correct. Radix primitives provide built-in accessibility support (ARIA attributes, keyboard navigation).apps/app/src/components/ui/radio-group.tsx (2)
9-21: Clean implementation of RadioGroup wrapper.The component properly wraps the Radix UI primitive with appropriate TypeScript types, ref forwarding, and default styling. The
grid gap-2layout provides sensible spacing for radio groups.
23-42: Well-implemented RadioGroupItem with proper accessibility.The component includes comprehensive styling with appropriate focus states (
focus-visible:ring-2), disabled state handling, and a properly sized indicator icon. The use of Radix UI primitives ensures solid accessibility foundations.apps/app/tests/utils/views/board.ts (2)
123-125: LGTM: Improved formatting.The multi-line formatting of the options object improves readability.
221-257: LGTM: Consistent multi-line formatting.The reformatted function signatures improve readability and maintain consistency across the file.
apps/app/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx (2)
1-54: LGTM: Well-structured dialog component.The component follows React best practices with:
- Clear prop interface
- Proper Dialog composition
- Test IDs for testing
- Accessible button labels and dialog semantics
36-36: Thetext-yellow-500utility is valid and available in this project's Tailwind v4 configuration. The codebase usestext-yellow-500across multiple components (25+ instances), indicating the color is working correctly. Tailwind v4 retains its default color palette when custom theme colors don't explicitly override it.Likely an incorrect or invalid review comment.
apps/app/src/components/ui/dialog.tsx (1)
52-118: LGTM: Correct ref forwarding implementation.The conversion of
DialogContentto useReact.forwardRefis properly implemented:
- The
DialogContentPropstype correctly omits therefprop from the primitive component props- The ref is properly typed as
HTMLDivElement- The ref is forwarded to the underlying
DialogPrimitive.ContentdisplayNameis set for better debugging in React DevToolsThis enables parent components to access the dialog content DOM node, which is useful for programmatic focus management, measurements, or animations.
apps/server/src/services/auto-mode-service.ts (6)
39-50: LGTM!The
AutoLoopStateinterface and class fields are well-structured for managing concurrent feature execution with abort capability.
86-99: LGTM!Good pattern: checking for existing context early and delegating to
resumeFeatureprevents duplicate work. The temporary entry deletion before callingresumeFeaturecorrectly avoids the "already running" check sinceresumeFeaturewill manage its own registration.
871-888: LGTM with note on abort listener cleanup.The concurrency management and Promise.race pattern are well-implemented. The abort signal listener uses
{ once: true }, which helps with cleanup, but the listener will remain attached ifPromise.allSettledresolves first. This is a minor memory consideration but acceptable for the loop's lifecycle.
953-1005: LGTM!Good defensive handling: silent catch for existing branches, fallback to project path on worktree creation failure, and consistent absolute path resolution for cross-platform compatibility.
1260-1263: LGTM!Good documentation explaining why
projectPathis used instead ofworkDirfor output paths—worktrees need their output stored in the main project's.automakerdirectory.
1532-1555: LGTM!Clean abort-aware sleep implementation with proper timeout cleanup.
| // No context, start fresh - executeFeature will handle adding to runningFeatures | ||
| // Remove the temporary entry we added | ||
| this.runningFeatures.delete(featureId); | ||
| return this.executeFeature(projectPath, featureId, useWorktrees, false); |
There was a problem hiding this comment.
Stale comment and unnecessary delete.
The comment states "Remove the temporary entry we added" but resumeFeature never adds an entry to runningFeatures. The delete call at line 270 is a harmless no-op, but the comment is misleading and should be removed.
🔎 Apply this diff to fix:
- // No context, start fresh - executeFeature will handle adding to runningFeatures
- // Remove the temporary entry we added
- this.runningFeatures.delete(featureId);
+ // No context, start fresh - executeFeature will handle adding to runningFeatures
return this.executeFeature(projectPath, featureId, useWorktrees, false);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/services/auto-mode-service.ts around lines 268 to 271, the
inline comment "Remove the temporary entry we added" is stale and the subsequent
this.runningFeatures.delete(featureId) is unnecessary because resumeFeature
never adds an entry; remove the misleading comment and the delete call so the
code simply returns this.executeFeature(projectPath, featureId, useWorktrees,
false).
…h sets of changes
- Modified the test to check that runningCount is 0 when no features are running, ensuring accurate status reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (1)
677-701: Align backlog filtering logic with use-board-column-features.ts to avoid unsafe defaults.The backlog filtering in
handleStartNextFeatures(lines 677-701) uses a different approach than the UI display logic inuse-board-column-features.ts, creating inconsistency in which features are considered available:
Line 684's unsafe fallback:
const featureBranch = f.branchName || primaryBranch || "main"assigns unassigned features to "main" without verifying it's actually the primary branch. IfprimaryBranchis null, features default to "main" regardless of actual project configuration.Inconsistent unassigned feature handling:
use-board-column-features.ts(line 63-66): Unassigned features (no branchName) only appear when viewing the primary worktreeuse-board-actions.ts(line 684-700): Unassigned features are implicitly assigned a default branch via the fallback chainRefactor to match the clearer logic in
use-board-column-features.ts: avoid the fallback chain, explicitly check if features are unassigned, and useisPrimaryWorktreeBranch()rather than hardcoded defaults.apps/server/src/services/auto-mode-service.ts (2)
461-501: Race condition window and unclear error message.Line 472 throws a generic "already running" error that doesn't identify which feature. Consider:
throw new Error(\Feature ${featureId} is already running`);`Lines 476-501 create a brief race condition window: the feature is added to
runningFeaturesat line 486, but if existing context is detected (lines 490-494), it's removed at line 499 before callingresumeFeature. Between lines 486 and 499, another concurrent call toexecuteFeaturefor the same feature would be incorrectly rejected. Consider checking for existing context before adding to the map:// Check context first, before claiming the slot const hasExistingContext = await this.contextExists(projectPath, featureId); if (hasExistingContext) { return this.resumeFeature(projectPath, featureId, useWorktrees); } // Now safe to add to running features const abortController = new AbortController(); // ...
668-675: Improve error message clarity.Line 674 throws a generic "already running" error. Make it specific:
throw new Error(\Feature ${featureId} is already running`);`
♻️ Duplicate comments (5)
apps/app/src/components/views/board-view.tsx (4)
589-602: Large dependency array causes frequent effect re-setup.The past review comment (lines 589-602) correctly identifies that including
hookFeaturesin the dependency array will cause the effect to tear down and re-setup every time features change (which is frequent). While theisActiveflag prevents race conditions, this is inefficient.🔎 Use a ref pattern to stabilize the effect:
+ // Track latest features in a ref to avoid effect re-setup + const featuresRef = useRef(hookFeatures); + useEffect(() => { + featuresRef.current = hookFeatures; + }, [hookFeatures]); + useEffect(() => { if (!autoMode.isRunning || !currentProject) { return; } let isChecking = false; let isActive = true; const checkAndStartFeatures = async () => { // ... - const backlogFeatures = hookFeatures.filter((f) => { + const backlogFeatures = featuresRef.current.filter((f) => { // ... }); - const eligibleFeatures = enableDependencyBlocking - ? sortedBacklog.filter((f) => { - const blockingDeps = getBlockingDependencies(f, hookFeatures); + const eligibleFeatures = enableDependencyBlocking + ? sortedBacklog.filter((f) => { + const blockingDeps = getBlockingDependencies(f, featuresRef.current); return blockingDeps.length === 0; }) : sortedBacklog; // ... }; // ... }, [ autoMode.isRunning, currentProject, runningAutoTasks, maxConcurrency, - hookFeatures, // Remove this currentWorktreeBranch, currentWorktreePath, getPrimaryWorktreeBranch, isPrimaryWorktreeBranch, enableDependencyBlocking, persistFeatureUpdate, handleStartImplementation, ]);
294-313: Type assertion bypasses TypeScript safety (flagged previously).Line 307 uses
null as unknown as string | undefined, which is the same issue flagged in the past review comment for lines 294-313. This unsafe type cast should be replaced withundefinedor the Feature type should be updated to allownull.🔎 Apply this diff to use undefined instead of null:
- persistFeatureUpdate(feature.id, { - branchName: null as unknown as string | undefined, - }); + persistFeatureUpdate(feature.id, { + branchName: undefined, + });
491-520: Filtering logic inconsistency (flagged previously).This backlog filtering logic is identified in a past review comment as potentially inconsistent with
use-board-column-features.ts. The specific concerns are:
Line 499:
const featureBranch = f.branchName || primaryBranch || "main";- This fallback can incorrectly associate features with the "main" branch when the primary branch is different.Consistency issue: The user may see different features in the backlog UI versus what auto-mode tries to start.
Consider refactoring this to align with the filtering implementation in
use-board-column-features.tsfor consistency.
1068-1077: Type assertion bypasses TypeScript safety (third occurrence).Line 1074 uses the same unsafe type cast
null as unknown as string | undefinedthat was flagged in the past review comment and inhandleRemovedWorktreesabove. All three occurrences should be fixed consistently.🔎 Consider consolidating this logic:
Instead of duplicating the feature reset logic here, call the memoized
handleRemovedWorktreeshandler:onDeleted={(deletedWorktree, _deletedBranch) => { - // Reset features that were assigned to the deleted worktree (by branch) - hookFeatures.forEach((feature) => { - // Match by branch name since worktreePath is no longer stored - if (feature.branchName === deletedWorktree.branch) { - // Reset the feature's branch assignment - persistFeatureUpdate(feature.id, { - branchName: null as unknown as string | undefined, - }); - } - }); + handleRemovedWorktrees([{ path: deletedWorktree.path, branch: deletedWorktree.branch }]); setWorktreeRefreshKey((k) => k + 1); setSelectedWorktreeForAction(null); }}apps/server/src/services/auto-mode-service.ts (1)
700-703: Stale comment and unnecessary delete.The comment "Remove the temporary entry we added" is misleading—
resumeFeaturenever adds an entry torunningFeatures. Thedeleteat line 702 is unnecessary since no entry was added. This was also flagged in a previous review.🔎 Apply this diff to remove the stale comment and unnecessary delete:
- // No context, start fresh - executeFeature will handle adding to runningFeatures - // Remove the temporary entry we added - this.runningFeatures.delete(featureId); + // No context, start fresh - executeFeature will handle adding to runningFeatures return this.executeFeature(projectPath, featureId, useWorktrees, false);
🧹 Nitpick comments (10)
apps/app/src/hooks/use-auto-mode.ts (1)
326-335: Remove unusedsetAutoModeRunningfrom the dependency array.
setAutoModeRunningis included in the dependency array but is not used anywhere inside the effect. This causes unnecessary re-subscriptions when the function reference changes.🔎 Apply this diff to remove the unused dependency:
return unsubscribe; }, [ projectId, addRunningTask, removeRunningTask, - setAutoModeRunning, addAutoModeActivity, getProjectIdFromPath, setPendingPlanApproval, currentProject?.path, ]);apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts (1)
35-35: Consider explicitly handling or ignoring the return value.The
fetchWorktrees()calls at these locations don't capture the returned worktree data. While this appears intentional (the data is likely used internally by the parent component's implementation), consider making the intent explicit:void fetchWorktrees(); // Explicitly ignore return valueThis clarifies that the return value is intentionally unused in this context while the function serves to trigger a refresh.
Also applies to: 62-62, 90-90
apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
35-40: UnusedhtmlForattribute — Label is not associated with any element.The
Labelon line 35 hashtmlFor={${testIdPrefix}-selector}but no element in the component hasid={${testIdPrefix}-selector}. TheRadioGroupcomponent doesn't have anidprop. This means the label isn't properly associated for accessibility.🔎 Suggested fix — add id to RadioGroup or use a wrapper:
- <Label htmlFor={`${testIdPrefix}-selector`}>Target Branch</Label> + <Label id={`${testIdPrefix}-label`}>Target Branch</Label> <RadioGroup value={useCurrentBranch ? "current" : "other"} onValueChange={(value: string) => onUseCurrentBranchChange(value === "current")} disabled={disabled} data-testid={`${testIdPrefix}-radio-group`} + aria-labelledby={`${testIdPrefix}-label`} >apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
17-17: Unused import:BranchAutocomplete.
BranchAutocompleteis imported but no longer used in this file since the component now usesBranchSelector(which internally usesBranchAutocomplete).🔎 Remove the unused import:
-import { BranchAutocomplete } from "@/components/ui/branch-autocomplete";apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (1)
17-17: Unused import:BranchAutocomplete.
BranchAutocompleteis imported but no longer used in this file. It has been replaced byBranchSelector.🔎 Remove the unused import:
-import { BranchAutocomplete } from "@/components/ui/branch-autocomplete";apps/app/src/components/views/board-view/hooks/use-board-actions.ts (2)
106-109: Simplify branch name defaulting logic.The expression on lines 107-108 can be simplified. The
|| undefinedis redundant sincefeatureData.branchNamewould already be a string or undefined.🔎 Apply this diff to simplify the logic:
- const finalBranchName = - featureData.branchName === "" - ? undefined - : featureData.branchName || undefined; + const finalBranchName = + featureData.branchName === "" || !featureData.branchName + ? undefined + : featureData.branchName;Or more concisely:
- const finalBranchName = - featureData.branchName === "" - ? undefined - : featureData.branchName || undefined; + const finalBranchName = featureData.branchName || undefined;
142-148: Simplify branch name defaulting (same as handleAddFeature).The same redundant pattern appears here. Consider the same simplification as suggested for
handleAddFeature.🔎 Apply this diff:
- const finalBranchName = - updates.branchName === "" ? undefined : updates.branchName || undefined; + const finalBranchName = updates.branchName || undefined;apps/server/src/services/auto-mode-service.ts (3)
1002-1022: Consider consolidating worktree path resolution logging.Lines 1002-1022 contain repetitive console logging patterns for worktree path resolution. Consider extracting this into a helper method to reduce duplication:
private async resolveWorkDirForFeature( projectPath: string, featureId: string, providedWorktreePath?: string ): Promise<string> { // Consolidate the path resolution and logging logic }
1888-1900: Consider simplifying newline handling.The logic at lines 1893-1899 ensures proper spacing between text blocks but uses nested conditionals. Consider extracting to a helper for clarity:
private ensureTrailingNewlines(text: string, count: number): string { const currentNewlines = text.match(/\n*$/)?.[0].length ?? 0; return text + '\n'.repeat(Math.max(0, count - currentNewlines)); } // Usage: responseText = this.ensureTrailingNewlines(responseText, 2); responseText += block.text || "";
332-340: Consolidate auto-loop state management to reduce duplication.The service declares an
AutoLoopStateinterface (lines 312-317) but never uses it; instead, separate fieldsautoLoopRunning,autoLoopAbortController, andconfig(lines 336-338) manage the auto-loop state. This duplication creates maintenance confusion. While the current implementation doesn't cause runtime inconsistencies—since only one set of fields is active—consolidating to useAutoLoopStatethroughout would improve clarity and prevent future divergence.The
startAutoLoopandstopAutoLoopmethods are actively used in tests and should be retained.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
apps/app/package.json(1 hunks)apps/app/src/components/ui/dialog.tsx(3 hunks)apps/app/src/components/views/board-view.tsx(17 hunks)apps/app/src/components/views/board-view/components/kanban-card.tsx(1 hunks)apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx(11 hunks)apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx(8 hunks)apps/app/src/components/views/board-view/dialogs/index.ts(1 hunks)apps/app/src/components/views/board-view/hooks/use-board-actions.ts(13 hunks)apps/app/src/components/views/board-view/kanban-board.tsx(4 hunks)apps/app/src/components/views/board-view/shared/branch-selector.tsx(1 hunks)apps/app/src/components/views/board-view/shared/index.ts(1 hunks)apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts(1 hunks)apps/app/src/hooks/use-auto-mode.ts(1 hunks)apps/app/src/lib/electron.ts(0 hunks)apps/app/src/lib/http-api-client.ts(0 hunks)apps/app/src/store/app-store.ts(7 hunks)apps/app/src/types/electron.d.ts(3 hunks)apps/server/src/routes/auto-mode/index.ts(0 hunks)apps/server/src/services/auto-mode-service.ts(26 hunks)apps/server/src/services/feature-loader.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/app/src/lib/http-api-client.ts
- apps/server/src/routes/auto-mode/index.ts
- apps/app/src/lib/electron.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/server/src/services/feature-loader.ts
- apps/app/package.json
- apps/app/src/components/views/board-view/kanban-board.tsx
- apps/app/src/store/app-store.ts
- apps/app/src/components/views/board-view/shared/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (3)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/utils.ts (1)
modelSupportsThinking(12-15)apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
BranchSelector(19-97)
apps/app/src/components/views/board-view/shared/branch-selector.tsx (3)
apps/app/src/components/ui/label.tsx (1)
Label(24-24)apps/app/src/components/ui/radio-group.tsx (2)
RadioGroup(44-44)RadioGroupItem(44-44)apps/app/src/components/ui/branch-autocomplete.tsx (1)
BranchAutocomplete(18-56)
apps/app/src/types/electron.d.ts (1)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)
apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (4)
apps/app/src/store/app-store.ts (4)
PlanningMode(267-267)Feature(281-308)AgentModel(258-258)ThinkingLevel(264-264)apps/server/src/services/feature-loader.ts (1)
Feature(15-47)apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
BranchSelector(19-97)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (44)
apps/app/src/components/ui/dialog.tsx (3)
90-96: LGTM! Type definition follows forwardRef best practices.The exported
DialogContentPropstype correctly omitsrefsince it's handled separately by the forwardRef mechanism. The custom props align with the implementation.
98-154: LGTM! forwardRef implementation is correct.The conversion to a forwardRef component is properly implemented:
- Ref is correctly typed as
HTMLDivElement- Ref is forwarded to
DialogContentPrimitiveat line 110- All existing props and behavior are preserved
- Compatible with React 19
156-156: LGTM! Standard React practice.Setting
displayNameon forwardRef components improves debugging in React DevTools.apps/app/src/hooks/use-auto-mode.ts (7)
1-10: LGTM!Imports are minimal and appropriate. The
isPlanApprovalEventtype guard correctly uses TypeScript'sExtractutility type for narrowingAutoModeEvent.
15-47: LGTM!State selection with
useShallowis appropriate for preventing unnecessary re-renders. ThegetProjectIdFromPathhelper is correctly memoized withprojectsas a dependency.
49-62: LGTM!Derived state is correctly memoized. The fallback default state prevents undefined access when a project hasn't been registered in
autoModeByProjectyet.
337-346: LGTM!The UI-only
startbehavior is clearly documented. The comment explains that feature pickup is handled elsewhere (board-view.tsx), which aligns with the PR's architectural changes.
363-398: LGTM!The
stopFeatureimplementation correctly handles the user-initiated stop action by immediately updating UI state on success rather than waiting for an event. Error handling is appropriate with proper propagation.
400-408: Verify no consumers depend on the removedclearRunningTasksmethod.The hook's public API no longer exposes
clearRunningTasks. All usages ofclearRunningTasksin the codebase are fromuseAppStore(app-store.ts), not fromuseAutoMode, confirming the removal from this hook is safe.
348-361: Ensureauto_mode_feature_completeevents are reliably emitted by the backend.The new design depends entirely on
auto_mode_feature_completeevents to clean up running tasks from UI state. If the backend fails to emit this event (e.g., due to a crash, unexpected termination, or exception), tasks will remain orphaned in the UI indefinitely. The events are emitted from multiple locations inapps/server/src/services/auto-mode-service.tsandapps/app/src/lib/electron.ts, but there are no timeout mechanisms or fallback cleanup handlers to recover from missing events.Consider adding one of the following safeguards:
- A timeout-based cleanup that removes tasks if no completion event is received within a reasonable window
- A periodic health check that validates running tasks are still actually executing
- A manual cleanup/reset option for users to clear stuck tasks
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts (1)
8-11: The interface and implementation are consistent. ThefetchWorktreesreturn typePromise<Array<{ path: string; branch: string }> | undefined>is properly implemented inuse-worktrees.ts(returningresult.removedWorktrees), and return values are handled appropriately: the parent hook (use-worktrees.tsline 54) captures them via.then()to notify removed worktrees to callers, while action callbacks inuse-worktree-actions.tsintentionally omit the capture since the state-owning hook handles the data. This follows correct separation of concerns—no breaking changes or unhandled issues exist.Likely an incorrect or invalid review comment.
apps/app/src/components/views/board-view/components/kanban-card.tsx (1)
260-264: LGTM!The draggable condition logic is well-structured, allowing drag operations for backlog, waiting_approval, verified features, and in_progress features that aren't currently being processed by the auto task.
apps/app/src/components/views/board-view/dialogs/index.ts (1)
4-9: LGTM!The export changes correctly reflect the refactoring from deletion to archival semantics, and the new dialog exports (FollowUpDialog, PlanApprovalDialog) expand the dialog surface as intended.
apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
66-82: LGTM!The conditional rendering of
BranchAutocompletewith proper error state propagation and inline validation message provides good UX feedback when branch selection is required but missing.apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (2)
393-403: LGTM!The
BranchSelectorintegration is properly wired with all necessary props. The controlled state pattern foruseCurrentBranchandbranchNameis correctly implemented with appropriate callbacks.
145-149: Good: Validation logic is consistent.The validation in
handleAdd(lines 145-149) and thedisabledprop on the confirm button (lines 517-521) use the same condition, ensuring users cannot submit an invalid form and receive clear visual feedback.Also applies to: 517-521
apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (3)
101-104: LGTM!The state initialization and synchronization logic is correctly implemented. The
useStatelazy initializer handles the initial mount, and theuseEffectproperly syncsuseCurrentBranchwhen thefeatureprop changes. Both use consistent logic (!feature?.branchName).Also applies to: 119-130
381-395: LGTM!The
BranchSelectorintegration correctly disables branch modification for non-backlog features, preventing changes after work has started. The props are properly wired and the disabled state is appropriately communicated.
135-145: Good: Validation and disabled state are consistent.The validation logic in
handleUpdateand thedisabledprop on the save button both correctly account for thestatus === "backlog"condition, ensuring branch selection is only required/enabled for features that haven't started processing.Also applies to: 527-532
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (5)
80-86: LGTM - Clean architectural shift.The addition of primary branch helpers and the removal of client-side worktree creation aligns with the PR's goal of moving worktree management to the server side.
259-273: LGTM - Well-implemented dependency blocking.The dependency blocking logic provides clear user feedback and gracefully allows the user to proceed with a warning rather than blocking completely. This is a good UX pattern.
729-757: LGTM - Excellent priority and dependency handling.The sorting logic correctly prioritizes features by:
- Blocking dependency status (unblocked first)
- Priority within each group (lower number = higher priority)
The user feedback when all features are blocked is clear and actionable.
773-805: LGTM - Better semantics with archival instead of deletion.Changing from deletion to archival (status = "completed") is a safer approach that preserves feature history while removing them from the active workflow. This aligns well with the PR objectives.
807-827: Public API change - handleArchiveAllVerified replaces handleDeleteAllVerified.This is a breaking change in the hook's public API. Ensure all consumers of this hook (e.g., board-view.tsx, kanban-board.tsx) have been updated to use the new handler name.
apps/app/src/components/views/board-view.tsx (6)
338-358: LGTM - Clean worktree selection logic.The
selectedWorktreecomputation correctly handles both primary worktree (when path is null) and specific worktree selection (by path matching). The use ofpathsEqualfor comparison is appropriate.
408-453: LGTM - Well-implemented auto-mode state tracking.The use of refs for
autoModeRunningandpendingFeaturesis the correct pattern for avoiding stale closures in async operations. The event listener properly filters events by project and manages pending feature state.
796-802: LGTM - Clean toggle handler.The unified
onAutoModeTogglehandler is a cleaner API than separate start/stop handlers and works well with the Switch control.
833-833: LGTM - Proper callback wiring.The
onRemovedWorktreescallback correctly connects the WorktreePanel to thehandleRemovedWorktreeshandler for resetting feature branch assignments.
933-933: LGTM - Branch context passed to dialogs.Passing
currentWorktreeBranch(converted to undefined when null) to the Add/Edit dialogs provides proper branch context for feature creation and editing.Also applies to: 946-946
963-972: LGTM - Archive dialog properly wired.The
ArchiveAllVerifiedDialogcorrectly replacesDeleteAllVerifiedDialogand is properly wired to thehandleArchiveAllVerifiedhandler from the actions hook.apps/app/src/types/electron.d.ts (3)
369-377: LGTM: API signature updated to use boolean flag.The change from
worktreePath?: stringtouseWorktrees?: booleanaligns with the PR's shift from path-based pre-creation to runtime derivation usingfeature.branchName. The implementation will now derive the worktree path internally when needed.
413-423: LGTM: Consistent API update for follow-up operations.The
followUpFeaturemethod mirrors therunFeaturechange, replacing explicit path passing with a boolean flag. This maintains consistency across the auto-mode API surface.
657-676: LGTM: Added removed worktrees reporting.The new
removedWorktreesfield enables the worktree management system to report cleanup operations. This allows the UI (viause-worktrees.ts) to handle removed worktrees appropriately, improving user visibility into worktree lifecycle events.apps/server/src/services/auto-mode-service.ts (11)
22-26: LGTM: Imports updated for new functionality.The added imports support error classification (
isAbortError,classifyError), feature loading (FeatureLoader), and centralized path management (getFeatureDir,getAutomakerDir,getFeaturesDir).
519-553: LGTM: Worktree path derived from branch name.The refactored logic correctly derives
worktreePathfromfeature.branchName(lines 519-543) instead of accepting a pre-created path. This aligns with the PR's shift to branch-name-driven worktree management. The fallback tofeature/${featureId}(line 522) whenbranchNameis missing is sensible.
722-744: LGTM: Follow-up uses branch-name-driven worktree resolution.The refactored
followUpFeaturecorrectly loads the feature first (line 723) to extractbranchName(line 729), then usesfindExistingWorktreeForBranch(lines 733-736) to locate the worktree. This mirrors the approach inexecuteFeatureand eliminates the need for explicitworktreePathparameters.
1068-1082: LGTM: Context existence check supports resume logic.The new
contextExistsmethod cleanly checks for saved agent context, enablingexecuteFeature(lines 490-501) to automatically resume from prior work instead of starting fresh.
1087-1168: LGTM: Project analysis capability added.The new
analyzeProjectmethod enables autonomous project structure analysis, storing results in.automaker/project-analysis.md. The restricted tool set (Read, Glob, Grep at line 1123) appropriately limits the analysis to read-only operations.
1174-1184: LGTM: Status now reflects on-demand execution model.The updated
getStatusreturnsisRunningbased on active features (line 1180) rather than a persistent auto-loop state. This aligns with the PR's shift to on-demand per-feature execution.
1361-1408: LGTM: Robust worktree lookup with cross-platform path handling.The
findExistingWorktreeForBranchmethod correctly parsesgit worktree list --porcelainoutput and resolves paths to absolute form (lines 1385-1388, 1398-1401). The comments explicitly call out Windows compatibility, which is essential forcwdoperations.
1410-1462: LGTM: Defensive worktree setup with proper fallbacks.The
setupWorktreemethod defensively checks for existing worktrees (lines 1416-1426) before creating new ones, handles pre-existing directories gracefully (lines 1434-1441), and falls back to the project path if creation fails (line 1460). All paths are resolved to absolute form for cross-platform safety.
1748-1805: LGTM: Mock mode supports CI/CD testing.The mock agent mode (activated via
AUTOMAKER_MOCK_AGENT=true) allows automated tests to bypass real API calls while simulating agent behavior. Line 1783 correctly usesprojectPathfor the output directory rather thanworkDir, sinceworkDirmay point to a worktree path.
1856-1876: LGTM: Output correctly written to project directory.The code correctly uses
projectPath(line 1857) instead ofworkDirfor the feature directory, as explained in the comment at line 1856. This ensures agent output is stored in.automaker/features/{id}/agent-output.mdregardless of whether execution happens in a worktree.
2469-2492: LGTM: Interruptible sleep with abort signal support.The enhanced
sleepmethod now accepts anAbortSignal(line 2469), enabling responsive cancellation during delays. The implementation correctly handles pre-aborted signals (lines 2474-2478) and cleans up the timeout when aborted (line 2485), using{ once: true }to auto-remove the listener.
- Fix unknown status bypassing worktree filtering in use-board-column-features.ts - Remove unused props projectPath and onWorktreeCreated from use-board-drag-drop.ts - Fix test expecting worktreePath during edit (worktrees created at execution time) - Remove unused setAutoModeRunning from dependency array - Remove unused imports (BranchAutocomplete, cn) - Fix htmlFor accessibility issue in branch-selector.tsx - Remove empty finally block in resume-feature.ts - Remove duplicate setTimeout state reset in create-pr-dialog.tsx - Consolidate duplicate state reset logic in context-view.tsx - Simplify branch name defaulting logic in use-board-actions.ts - Fix branchName reset to null when worktree is deleted
- Updated comments in AddFeatureDialog and EditFeatureDialog to better explain the logic for determining the final branch name based on the current worktree context. - Adjusted logic to ensure that an empty string indicates "unassigned" for primary worktrees, while allowing for the use of the current branch when applicable. - Simplified branch name handling in useBoardActions to reflect these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/app/src/components/views/board-view/hooks/use-board-column-features.ts (1)
58-77: Store values read viauseAppStore.getState()insideuseMemowon’t react to setting changes
isPrimaryWorktreeBranchandenableDependencyBlockingare pulled viauseAppStore.getState()inside theuseMemo, so toggling the primary-branch settings or dependency-blocking flag won’t recomputecolumnFeaturesMapunless some other dependency changes. This is the same reactivity issue called out in earlier reviews.Proposed fix using store selectors
-import { useMemo, useCallback } from "react"; -import { Feature, useAppStore } from "@/store/app-store"; -import { resolveDependencies, getBlockingDependencies } from "@/lib/dependency-resolver"; +import { useMemo, useCallback } from "react"; +import { Feature, useAppStore } from "@/store/app-store"; +import { + resolveDependencies, + getBlockingDependencies, +} from "@/lib/dependency-resolver"; export function useBoardColumnFeatures({ ... }: UseBoardColumnFeaturesProps) { - // Memoize column features to prevent unnecessary re-renders - const columnFeaturesMap = useMemo(() => { + const isPrimaryWorktreeBranch = useAppStore( + (s) => s.isPrimaryWorktreeBranch + ); + const enableDependencyBlocking = useAppStore( + (s) => s.enableDependencyBlocking + ); + + // Memoize column features to prevent unnecessary re-renders + const columnFeaturesMap = useMemo(() => { ... - } else if (effectiveBranch === null) { + } else if (effectiveBranch === null) { // We're viewing main but branch hasn't been initialized yet // (worktrees disabled or haven't loaded yet). // Show features assigned to primary worktree's branch. - matchesWorktree = projectPath - ? useAppStore.getState().isPrimaryWorktreeBranch(projectPath, featureBranch) - : false; + matchesWorktree = projectPath + ? isPrimaryWorktreeBranch(projectPath, featureBranch) + : false; } else { // Match by branch name matchesWorktree = featureBranch === effectiveBranch; } ... - const allFeatures = features; - const enableDependencyBlocking = useAppStore.getState().enableDependencyBlocking; + const allFeatures = features; ... }, [ features, runningAutoTasks, searchQuery, currentWorktreePath, currentWorktreeBranch, - projectPath + projectPath, + isPrimaryWorktreeBranch, + enableDependencyBlocking, ]);This keeps
columnFeaturesMapin sync when store flags change, without reintroducinghookFeaturesinto the deps.Also applies to: 71-77, 111-136, 138-139
apps/app/src/components/views/board-view.tsx (1)
294-313: Avoidnull as unknown as string | undefinedwhen clearingbranchNameBoth
handleRemovedWorktreesand theDeleteWorktreeDialog’sonDeletedhandler reset feature branches with:branchName: null as unknown as string | undefined,This is the same cast that was previously flagged as a smell: it sidesteps the actual
Feature["branchName"]type and makes it unclear whether callers should treat “no branch” asnullorundefined.Consider one of:
- Make
branchName?: string | nullin the sharedFeaturetype (client and server) and assignbranchName: nulldirectly, or- Standardize on
undefinedfor “unset” and assignbranchName: undefinedhere.Either way, you can drop the unsafe cast and keep semantics explicit.
Also applies to: 1069-1077
🧹 Nitpick comments (9)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (3)
56-85: Redundant reset logic in both branches.Both the
if (open)andelsebranches perform identical state resets. Consider consolidating since the reset should happen regardless of the dialog opening or closing.🔎 Suggested simplification
useEffect(() => { - if (open) { - // Reset form fields - setTitle(""); - setBody(""); - setCommitMessage(""); - setBaseBranch("main"); - setIsDraft(false); - setError(null); - // Also reset result states when opening for a new worktree - // This prevents showing stale PR URLs from previous worktrees - setPrUrl(null); - setBrowserUrl(null); - setShowBrowserFallback(false); - // Reset operation tracking - operationCompletedRef.current = false; - } else { - // Reset everything when dialog closes - setTitle(""); - setBody(""); - setCommitMessage(""); - setBaseBranch("main"); - setIsDraft(false); - setError(null); - setPrUrl(null); - setBrowserUrl(null); - setShowBrowserFallback(false); - operationCompletedRef.current = false; - } + // Reset all state when dialog opens/closes or worktree changes + // This prevents stale data from previous operations + setTitle(""); + setBody(""); + setCommitMessage(""); + setBaseBranch("main"); + setIsDraft(false); + setError(null); + setPrUrl(null); + setBrowserUrl(null); + setShowBrowserFallback(false); + operationCompletedRef.current = false; }, [open, worktree?.path]);
179-188: Inconsistent control flow compared to other success paths.This path calls
onCreated()directly (line 187) thenonOpenChange(false)(line 188), which triggershandleClose(). Other success paths (lines 108-118, 127-141, 145-167) markoperationCompletedRef.current = trueand deferonCreated()tohandleClose().While this works correctly (the ref is false so
handleClosewon't double-callonCreated()), it creates an inconsistent pattern that could confuse future maintainers.🔎 Consistent approach
// No browser URL available, just close if (!result.result.prCreated) { if (!hasBrowserUrl) { toast.info("PR not created", { description: "Could not determine repository URL. GitHub CLI (gh) may not be installed or authenticated.", duration: 8000, }); } } - onCreated(); - onOpenChange(false); + // Mark operation as completed - branch was pushed + operationCompletedRef.current = true; + handleClose();
215-215: Minor type signature mismatch ononOpenChange.
handleClosehas signature() => void, butonOpenChangeexpects(open: boolean) => void. This works at runtime since JavaScript ignores extra arguments, but it's semantically imprecise. Since the conditional refresh logic now correctly guards against unnecessary calls, this is acceptable but could be cleaner.🔎 Type-correct alternative
- <Dialog open={open} onOpenChange={handleClose}> + <Dialog open={open} onOpenChange={() => handleClose()}>Or if you want to respect the boolean semantics:
- <Dialog open={open} onOpenChange={handleClose}> + <Dialog open={open} onOpenChange={(isOpen) => { if (!isOpen) handleClose(); }}>apps/app/src/hooks/use-auto-mode.ts (1)
336-345: Architecture verified: feature pickup is correctly handled in board-view.tsx, but document the UI-only state management pattern.The
start()andstop()functions are intentionally UI-only and rely on board-view.tsx to subscribe toautoMode.isRunningstate. When running, board-view.tsx polls every 3 seconds and callshandleRunFeature()→api.autoMode.runFeature()to initiate feature execution. The implementation mitigates risks through:
- Event subscription (
api.autoMode.onEvent()) keeps UI and backend state synchronized- Pending features tracking prevents duplicate starts
- Early component mounting reduces race condition likelihood
However, the asymmetry between start/stop (UI-only) and stopFeature (API-integrated) is unconventional. Consider adding a JSDoc comment to
start()andstop()explaining why they intentionally skip backend calls, or consolidate state management to reduce architectural complexity.apps/app/src/components/views/context-view.tsx (1)
193-230: Reloading context files only on successful write makes sense; consider user feedback on failureMoving
loadContextFiles()into the success path avoids unnecessary reloads when writes fail and keeps the list consistent. You might also want to surface a toast and/or keep the dialog open on error so users don’t lose their input silently.apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
57-82: Branch handling is wired correctly; consider avoiding the empty‑string sentinel in the public APIThe new
branchName+currentBranchprops anduseCurrentBranchstate give you the right behavior (current branch vs explicit branch, with proper validation when worktrees are enabled). Downstream normalization in board actions turns""intoundefined, which keeps persistence clean.Longer term, you may want to change
AddFeatureDialogPropsto acceptbranchName?: string(or a small union type) and stop relying on""as a sentinel; that would push the “use current branch” concept inside the dialog instead of exposing it to callers.Also applies to: 84-121
apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts (1)
27-31: Drag rules now align with auto‑mode;currentProjectprop can likely be droppedThe updated drag logic correctly blocks dragging in‑progress features while they’re running and routes backlog→in_progress via
handleStartImplementation, letting the server derive worktrees fromfeature.branchName. The additional skip‑tests branches for moving betweenwaiting_approval,verified, andbacklogare consistent and don’t introduce obvious state leaks.
currentProjectinUseBoardDragDropPropsis no longer used; you can safely remove it from the hook signature and call sites to avoid unused‑parameter warnings.Also applies to: 60-75, 95-107, 139-190, 204-207
apps/app/tests/worktree-integration.spec.ts (1)
744-780: Tests correctly capture the new “branchName only, worktree at execution time” behaviorThe updated specs around adding/editing features and deleting worktrees now assert the intended model:
- backlog features store
branchNamebut leaveworktreePathundefined,- branches for new features aren’t created until execution,
- deleting a worktree clears the feature’s branch assignment and hides unassigned features on non‑primary worktrees,
- the add‑feature dialog’s branch selector reflects the currently selected worktree via the
feature-currentlabel.This all matches the refactored client/server behavior. If you see flakes here later, consider replacing some of the new
waitForTimeoutcalls with more targeted waits on filesystem state or UI selectors.Also applies to: 782-843, 897-904, 926-931, 956-959, 1079-1084, 2356-2450
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (1)
670-767: Start‑next and archive‑all behaviors look correct; minor cleanup possible
handleStartNextFeatures:
- Filters backlog by the selected worktree branch (or primary when none is selected),
- Respects
maxConcurrencyandenableDependencyBlocking,- Prioritizes unblocked, high‑priority items.
This matches the intended “G starts the next eligible feature from what you see” behavior.
handleArchiveAllVerified:
- Stops any running agents tied to verified features,
- Then marks them completed and persists,
- Emits a single summary toast.
Both flows look good. The only minor nit is that
isPrimaryWorktreeBranch/getPrimaryWorktreeBranchare destructured at the top of the hook but not used here; you can safely drop them from the destructuring to avoid unused‑value noise.Also applies to: 769-801
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/app/src/components/views/board-view.tsx(17 hunks)apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx(11 hunks)apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx(8 hunks)apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx(8 hunks)apps/app/src/components/views/board-view/hooks/use-board-actions.ts(13 hunks)apps/app/src/components/views/board-view/hooks/use-board-column-features.ts(4 hunks)apps/app/src/components/views/board-view/hooks/use-board-drag-drop.ts(8 hunks)apps/app/src/components/views/board-view/shared/branch-selector.tsx(1 hunks)apps/app/src/components/views/context-view.tsx(1 hunks)apps/app/src/hooks/use-auto-mode.ts(1 hunks)apps/app/tests/worktree-integration.spec.ts(11 hunks)apps/server/src/routes/auto-mode/routes/resume-feature.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/server/src/routes/auto-mode/routes/resume-feature.ts
- apps/app/src/components/views/board-view/shared/branch-selector.tsx
- apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/app/src/components/views/board-view/hooks/use-board-column-features.ts (4)
apps/app/src/store/app-store.ts (2)
useAppStore(847-2346)Feature(281-308)apps/server/src/services/feature-loader.ts (1)
Feature(15-47)apps/app/src/lib/dependency-resolver.ts (1)
getBlockingDependencies(209-221)apps/server/src/lib/dependency-resolver.ts (1)
getBlockingDependencies(209-221)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (2)
apps/app/src/components/ui/delete-confirm-dialog.tsx (1)
DeleteConfirmDialog(30-88)apps/app/src/components/dialogs/file-browser-dialog.tsx (1)
onOpenChange(400-400)
apps/app/src/hooks/use-auto-mode.ts (2)
apps/app/src/lib/electron.ts (3)
AutoModeAPI(206-269)createMockAutoModeAPI(1392-1732)simulateAutoModeLoop(1738-1851)apps/app/src/types/electron.d.ts (1)
AutoModeAPI(377-485)
apps/app/tests/worktree-integration.spec.ts (3)
apps/app/tests/utils/git/worktree.ts (3)
setupProjectWithPath(309-353)waitForBoardView(462-464)listBranches(194-200)apps/app/tests/utils/core/waiting.ts (1)
waitForNetworkIdle(7-9)apps/server/src/services/auto-mode-service.ts (1)
commitFeature(964-1031)
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (3)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/utils.ts (1)
modelSupportsThinking(12-15)apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
BranchSelector(18-97)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (11)
apps/app/src/components/views/board-view/dialogs/create-pr-dialog.tsx (4)
52-53: Good use ofuseReffor operation tracking.Using a ref instead of state is the right choice here since the value is only read in event handlers and doesn't need to trigger re-renders. This correctly addresses the previous concern about unconditional refreshes.
200-208: Good fix for the conditional refresh logic.This correctly addresses the previous review concern by only calling
onCreated()when an actual operation completed. The delegation of state reset to theuseEffectis clean.
241-252: Good UX improvement with explicit close button.Adding a dedicated "Close" button alongside "View Pull Request" gives users clear control over the dialog. The flex layout with
gap-2provides good spacing.
286-290: LGTM!The close button in the browser fallback view correctly uses
handleClose, which will trigger the refresh sinceoperationCompletedRefis set totruein this path.apps/app/src/hooks/use-auto-mode.ts (1)
336-397: This architectural pattern is intentional and correctly designed—no changes needed.The asymmetry between
start()/stop()andstopFeature()is not a flaw but a deliberate separation of concerns:
start()andstop()are UI-only because they simply toggle a flag that controls the feature-picking loop inboard-view.tsx(running every 3 seconds). No backend communication is necessary for state toggling.stopFeature()calls the backend API because it must interrupt a running LLM process that the backend actively manages.This design is already well-documented in the code comments ("UI only, feature pickup is handled in board-view.tsx") and aligns with the backend implementation, which has no project-level start/stop endpoints. The architecture is sound and requires no refactoring.
Likely an incorrect or invalid review comment.
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
124-137: BranchSelector integration and validation look consistentInitializing
branchNamefromdefaultBranch, resettinguseCurrentBranchon open, and usingBranchSelectorgated byuseWorktreesgives a clear UX: “use current selected branch” by default, fall back to an explicit branch when needed. The extra guard inhandleAddplus the disabled state on the confirm button correctly prevent submitting without a branch when “Other branch” is selected.Also applies to: 391-403, 512-521
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (2)
73-82: Branch normalization in add/update handlers matches the new modelConverting empty strings to
undefinedinhandleAddFeatureandhandleUpdateFeatureand persisting onlybranchName?: stringis a clean way to represent “use current branch” without leaking the sentinel into storage. Combined with the dialog logic, this keeps persistence aligned with the server’s expectations.Also applies to: 88-120, 122-154
207-241: Server‑derived workdirs and dependency warnings are integrated cleanlyRemoving client‑side worktreePath handling in
handleRunFeature,handleSendFollowUp, andhandleCommitFeatureand relying on the server to derive the workdir fromfeature.branchNamekeeps this hook simpler and matches the rest of the refactor. The dependency‑blocking integration (warn on manual start, filter for auto‑start) is sensible and usesgetBlockingDependenciesconsistently across start/resume flows.Also applies to: 401-467, 469-524
apps/app/src/components/views/board-view.tsx (3)
62-85: Current worktree selection and branch derivation look correctDeriving
currentWorktreePathviagetCurrentWorktree, then computingselectedWorktreefromworktreesByProjectand using itsbranchascurrentWorktreeBranchensures branch filtering and defaults are based on the actual, up‑to‑date worktree list (including newly created ones). Falling back to the main worktree’s branch (or"main") forselectedWorktreeBranchgives predictable defaults for new features.Also applies to: 323-359
380-406: Auto‑mode orchestration, branch‑aware start‑next, and archive‑all wiring are consistent
- The new auto‑mode effect uses refs for
isRunning, features, and pending IDs, and scopes events byprojectPath. It respectsmaxConcurrencyandenableDependencyBlocking, and its branch filter mirrors the column/worktree semantics.useBoardActionsis now fedcurrentWorktreeBranchandonWorktreeCreated, so dialog/defaults and commit flows stay in sync with the worktree panel.BoardHeader’sonAutoModeTogglesimply callsautoMode.start()/stop(), which is exactly what the effect depends on viaautoMode.isRunning.KanbanBoard’sonArchiveAllVerifiedand the accompanyingArchiveAllVerifiedDialogcleanly route intohandleArchiveAllVerified, matching the archive‑instead‑of‑delete behavior.- Passing
currentBranch={currentWorktreeBranch || undefined}into Add/EditFeatureDialog lets the BranchSelector label correctly reflect the selected worktree’s branch.All of this lines up with the branch/worktree and auto‑mode refactor goals.
Also applies to: 670-767, 787-804, 864-895, 964-973
408-459: Auto‑mode interval effect now avoids feature‑array churn and should be stableThe auto‑mode
useEffect:
- Tracks
autoMode.isRunningvia a ref so the interval sees the latest value,- Uses
hookFeaturesRefinstead ofhookFeaturesin the dependency array, avoiding the earlier “interval recreated on every feature change” problem,- Accounts for both
runningAutoTasksand pending feature IDs when computing available slots,- Clears pending IDs and marks the effect inactive on cleanup to prevent race conditions.
This is a solid improvement over the previous implementation and should behave well under changing features and auto‑mode toggles.
Also applies to: 460-605, 814-840
| // Stop auto mode - UI only, running tasks continue until natural completion | ||
| const stop = useCallback(() => { | ||
| if (!currentProject) { | ||
| console.error("No project selected"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const api = getElectronAPI(); | ||
| if (!api?.autoMode) { | ||
| throw new Error("Auto mode API not available"); | ||
| } | ||
|
|
||
| const result = await api.autoMode.stop(currentProject.path); | ||
|
|
||
| if (result.success) { | ||
| setAutoModeRunning(currentProject.id, false); | ||
| // NOTE: We intentionally do NOT clear running tasks here. | ||
| // Stopping auto mode only turns off the toggle to prevent new features | ||
| // from being picked up. Running tasks will complete naturally and be | ||
| // removed via the auto_mode_feature_complete event. | ||
| console.log( | ||
| "[AutoMode] Stopped successfully - running tasks will continue" | ||
| ); | ||
| } else { | ||
| console.error("[AutoMode] Failed to stop:", result.error); | ||
| throw new Error(result.error || "Failed to stop auto mode"); | ||
| } | ||
| } catch (error) { | ||
| console.error("[AutoMode] Error stopping:", error); | ||
| throw error; | ||
| } | ||
| setAutoModeRunning(currentProject.id, false); | ||
| // NOTE: We intentionally do NOT clear running tasks here. | ||
| // Stopping auto mode only turns off the toggle to prevent new features | ||
| // from being picked up. Running tasks will complete naturally and be | ||
| // removed via the auto_mode_feature_complete event. | ||
| console.log("[AutoMode] Stopped - running tasks will continue"); | ||
| }, [currentProject, setAutoModeRunning]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setAutoModeRunning and backend auto mode logic
# Find the use-auto-mode.ts file and examine the stop function context
echo "=== Looking for use-auto-mode.ts ==="
fd -t f "use-auto-mode.ts" 2>/dev/null
# Search for setAutoModeRunning to understand the function signature
echo -e "\n=== Searching for setAutoModeRunning ==="
rg -n "setAutoModeRunning" --type ts --type tsx -B 3 -A 3 2>/dev/null | head -50
# Search for autoMode.stop API calls
echo -e "\n=== Searching for autoMode.stop API calls ==="
rg -n "autoMode\.stop" --type ts --type tsx -B 3 -A 3 2>/dev/null
# Search for backend auto mode endpoints
echo -e "\n=== Searching for auto mode backend logic ==="
rg -n "autoMode|auto.mode|auto_mode" apps/electron --type ts --type tsx --type js -B 2 -A 2 2>/dev/null | head -80
# Look for API endpoints
echo -e "\n=== Searching for API endpoints related to autoMode ==="
rg -n "autoMode|auto.mode|auto_mode" --type ts --type tsx -l 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Let's read the use-auto-mode.ts file to understand the implementation
echo "=== Reading use-auto-mode.ts ==="
wc -l apps/app/src/hooks/use-auto-mode.ts
echo -e "\n=== Content around lines 347-360 ==="
sed -n '340,370p' apps/app/src/hooks/use-auto-mode.ts
echo -e "\n=== Full file to understand imports and context ==="
cat apps/app/src/hooks/use-auto-mode.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 4681
🏁 Script executed:
#!/bin/bash
# Search for the store definition and setAutoModeRunning implementation
echo "=== Finding app-store file ==="
fd -t f "app-store" apps/app/src/store 2>/dev/null
echo -e "\n=== Looking for setAutoModeRunning in store ==="
rg -n "setAutoModeRunning" apps/app/src/store -B 5 -A 10 2>/dev/null
echo -e "\n=== Looking for electron API autoMode methods ==="
rg -n "autoMode" apps/app/src/lib/electron -B 3 -A 10 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Look for electron API definition and autoMode implementation
echo "=== Searching for electron API types and implementation ==="
fd -t f "electron" apps/app/src/lib -name "*.ts" -o -name "*.tsx" 2>/dev/null
echo -e "\n=== Looking at electron.ts file ==="
wc -l apps/app/src/lib/electron.ts 2>/dev/null || echo "File not found"
echo -e "\n=== Searching for autoMode in lib/electron.ts ==="
rg -n "autoMode" apps/app/src/lib/electron.ts -B 2 -A 15 2>/dev/null | head -80
echo -e "\n=== Looking for stopAutoMode or similar backend calls ==="
rg -n "stopAutoMode|stop.*AutoMode" apps/ --type ts --type tsx 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 1327
🏁 Script executed:
#!/bin/bash
# Find the AutoModeAPI definition and implementation
echo "=== Searching for AutoModeAPI type definition ==="
rg -n "interface AutoModeAPI|type AutoModeAPI" apps/ --type ts --type tsx -A 20 2>/dev/null
echo -e "\n=== Searching for createMockAutoModeAPI implementation ==="
rg -n "createMockAutoModeAPI" apps/app/src/lib/electron.ts -A 50 2>/dev/null | head -100
echo -e "\n=== Searching for AutoModeAPI methods ==="
rg -n "onEvent|stop|start" apps/app/src/lib/electron.ts -B 2 -A 5 2>/dev/null | grep -A 8 -B 2 "autoMode\|AutoMode" | head -60Repository: AutoMaker-Org/automaker
Length of output: 5251
🏁 Script executed:
#!/bin/bash
# Now let's look at stopFeature to see how it properly calls the API
echo "=== Looking at stopFeature implementation ==="
sed -n '375,420p' apps/app/src/hooks/use-auto-mode.ts
echo -e "\n=== Check if there's any API call after setAutoModeRunning in stop() ==="
sed -n '355,375p' apps/app/src/hooks/use-auto-mode.ts
echo -e "\n=== Look for api.autoMode.stop being called anywhere ==="
rg -n "api\.autoMode\.stop|autoMode\.stop" apps/app/src --type ts --type tsx -B 3 -A 3 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 1936
🏁 Script executed:
#!/bin/bash
# Let's look at the start() function to see the pattern
echo "=== Looking at the start() function ==="
sed -n '330,355p' apps/app/src/hooks/use-auto-mode.ts
echo -e "\n=== Looking at full stop function context ==="
sed -n '347,370p' apps/app/src/hooks/use-auto-mode.tsRepository: AutoMaker-Org/automaker
Length of output: 1827
🏁 Script executed:
#!/bin/bash
# Now I need to understand how the backend checks if auto mode is running
# Let's search for where the start() call happens in board-view.tsx
echo "=== Searching for board-view.tsx ==="
fd -t f "board-view.tsx" apps/app/src 2>/dev/null
echo -e "\n=== Looking at board-view.tsx to understand feature pickup ==="
rg -n "autoMode|start\(\)" apps/app/src/components/board-view.tsx -B 3 -A 5 2>/dev/null | head -80
echo -e "\n=== Searching for backend feature pickup logic ==="
rg -n "pickupFeature|runFeature|nextFeature|auto_mode" apps/electron --type ts --type tsx --type js -B 2 -A 5 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 264
🏁 Script executed:
#!/bin/bash
# Let's explore the project structure more broadly
echo "=== Repository structure ==="
ls -la apps/
echo -e "\n=== Looking for backend/server code ==="
fd -t d "backend\|server\|api" apps/ 2>/dev/null | head -20
echo -e "\n=== Check apps/electron directory structure ==="
ls -la apps/electron/ 2>/dev/null | head -30
echo -e "\n=== Search for auto mode in all TypeScript files ==="
rg -l "auto.mode\|autoMode" apps/ --type ts --type tsx 2>/dev/null
echo -e "\n=== Look at the actual implementation in board-view ==="
wc -l apps/app/src/components/views/board-view.tsx
head -150 apps/app/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 5691
🏁 Script executed:
#!/bin/bash
# Look at the server directory for backend implementation
echo "=== Server directory structure ==="
ls -la apps/server/
echo -e "\n=== Search for auto mode in server ==="
rg -l "autoMode\|auto.mode\|auto_mode" apps/server --type ts --type tsx --type js 2>/dev/null
echo -e "\n=== Search for feature pickup logic in server ==="
rg -n "pickupFeature\|runFeature\|nextFeature\|startFeature" apps/server --type ts --type tsx --type js -B 2 -A 5 2>/dev/null | head -120Repository: AutoMaker-Org/automaker
Length of output: 930
🏁 Script executed:
#!/bin/bash
# Search more broadly in the server
echo "=== All files in server/src ==="
find apps/server/src -type f -name "*.ts" -o -name "*.js" 2>/dev/null
echo -e "\n=== Search for any references to starting/running features ==="
rg -n "run\|feature\|Feature" apps/server/src --type ts -l 2>/dev/null | head -20
echo -e "\n=== Look at main server files ==="
ls -la apps/server/src/
echo -e "\n=== Search in entire apps directory for where stop API is actually called ==="
rg -n "\.stop\(" apps/app/src --type ts --type tsx -B 2 -A 2 2>/dev/null | grep -i "automode\|auto" -A 5 -B 5Repository: AutoMaker-Org/automaker
Length of output: 8312
🏁 Script executed:
#!/bin/bash
# Perfect! Let's look at the auto-mode-service and routes
echo "=== Looking at auto-mode-service.ts ==="
wc -l apps/server/src/services/auto-mode-service.ts
echo -e "\n=== Search for how backend determines if auto mode is running ==="
rg -n "isRunning\|running" apps/server/src/services/auto-mode-service.ts -B 2 -A 5 2>/dev/null | head -80
echo -e "\n=== Looking at auto-mode routes ==="
ls -la apps/server/src/routes/auto-mode/routes/
echo -e "\n=== Check if there's a stop route or status route ==="
cat apps/server/src/routes/auto-mode/routes/status.ts
echo -e "\n=== Look at run-feature.ts to see how features are picked up ==="
head -100 apps/server/src/routes/auto-mode/routes/run-feature.tsRepository: AutoMaker-Org/automaker
Length of output: 3384
🏁 Script executed:
#!/bin/bash
# Let's look at the auto-mode-service to understand how it tracks running state
echo "=== Looking at auto-mode-service initialization and running state ==="
head -200 apps/server/src/services/auto-mode-service.ts
echo -e "\n=== Search for getStatus method ==="
rg -n "getStatus\|isRunning" apps/server/src/services/auto-mode-service.ts -B 3 -A 10 2>/dev/null | head -80Repository: AutoMaker-Org/automaker
Length of output: 7994
🏁 Script executed:
#!/bin/bash
# Search for getStatus and see how auto mode tracks its running state
echo "=== Search for getStatus and runningFeatures tracking ==="
rg -n "getStatus|runningFeatures|startingSlots" apps/server/src/services/auto-mode-service.ts -B 2 -A 8 2>/dev/null
echo -e "\n=== Look at the auto-mode index/routes setup ==="
cat apps/server/src/routes/auto-mode/index.tsRepository: AutoMaker-Org/automaker
Length of output: 8336
🏁 Script executed:
#!/bin/bash
# Let's check where startAutoLoop is called and how features are actually picked up
echo "=== Search for startAutoLoop calls ==="
rg -n "startAutoLoop|stopAutoLoop" apps/ --type ts --type tsx -B 3 -A 3 2>/dev/null
echo -e "\n=== Look for where auto mode auto-pickup happens ==="
rg -n "isAutoModeRunning\|canStartNewTask" apps/app/src --type ts --type tsx -B 2 -A 5 2>/dev/null | head -100
echo -e "\n=== Check board-view for feature pickup logic ==="
rg -n "auto.*mode\|pickupFeature\|nextFeature\|canStartNewTask" apps/app/src/components/views/board-view.tsx -B 2 -A 5 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Let's look at the full board-view file and search for auto mode logic more carefully
echo "=== Search for all useAutoMode usage in board-view ==="
rg -n "useAutoMode\|isAutoModeRunning\|autoMode" apps/app/src/components/views/board-view.tsx -B 2 -A 8 2>/dev/null
echo -e "\n=== Look at board-view hooks directory ==="
ls -la apps/app/src/components/views/board-view/hooks/
echo -e "\n=== Look at all files in board-view that might handle auto mode ==="
find apps/app/src/components/views/board-view -name "*.ts" -o -name "*.tsx" | xargs rg -l "autoMode\|runFeature" 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 1154
Backend is never notified to stop picking up new features.
The stop() function updates only UI state via setAutoModeRunning(false) but does not call any backend API to halt feature pickup. The backend's AutoModeService continuously runs its auto loop and picks new features based on its own config and runningFeatures state. Since the UI-only state change never reaches the backend, the backend will continue starting new features even after the user clicks "stop", causing state desynchronization and potential zombie feature pickup.
To fix this, the stop() function must call api.autoMode.stop() to notify the backend to halt its auto loop, matching the pattern used in stopFeature() which properly calls the backend API.
🤖 Prompt for AI Agents
In apps/app/src/hooks/use-auto-mode.ts around lines 347 to 360, the stop()
handler only flips local UI state and never notifies the backend to halt feature
pickup; update stop() to also call the backend API (e.g. api.autoMode.stop())
for the current project so the server-side AutoModeService stops its loop,
handle the promise (await or .catch) and only update UI state or log errors
after the API call succeeds or fails so UI and backend stay in sync.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (1)
72-72: Clarify the comment about empty branchName.The comment states "Can be empty string to use current branch", but based on the implementation (lines 153-158), an empty string actually means "no specific branch assigned" or "primary worktree", not "use current branch". The
useCurrentBranchtoggle determines whether to use the current branch, and when that toggle is true in a primary worktree (wherecurrentBranchis undefined), it results in an empty string.Consider this clearer comment:
- branchName: string; // Can be empty string to use current branch + branchName: string; // Empty string = primary worktree (unassigned); non-empty = specific branch
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx(11 hunks)apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx(8 hunks)apps/app/src/components/views/board-view/hooks/use-board-actions.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (16)
apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (6)
100-103: LGTM!The initialization logic correctly defaults to using the current branch when no specific branch is assigned to the feature.
118-129: LGTM!The effect correctly synchronizes the toggle state when the feature prop changes, using the same logic as the initial state.
134-144: LGTM!The validation logic correctly ensures a branch is selected when required, providing both user feedback via toast and preventing invalid submissions.
153-158: LGTM!The branch name computation correctly handles all scenarios: using the current branch (in both primary and non-primary worktrees) or using an explicitly selected branch. The comments clearly explain the logic.
380-396: LGTM!The BranchSelector integration is complete and correct, with all necessary props properly wired and appropriate disabled state for non-backlog features.
527-532: LGTM!The Save button disabled state correctly mirrors the validation logic, providing good UX feedback by preventing save attempts that would fail validation.
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (10)
80-86: LGTM! Clear architectural shift to server-side worktree management.The addition of
isPrimaryWorktreeBranchandgetPrimaryWorktreeBranchhelpers, along with the comment documenting the removal of client-side worktree creation, clearly communicates the architectural change to runtime, server-side worktree derivation based onfeature.branchName.
103-120: LGTM! Simplified feature creation aligns with server-side worktree management.The logic correctly stores only
branchName(converting empty string toundefinedfor unassigned features), deferring worktree creation to execution time. The dependency array has been appropriately updated to remove worktree-related dependencies.
140-154: LGTM! Consistent with the add flow.The update logic correctly mirrors the add flow by converting empty
branchNametoundefinedand storing only branch information. Dependencies are properly updated.
219-225: LGTM! Consistent removal of worktreePath across API calls.The comments clearly document that the server now derives
workDirfromfeature.branchNameat execution time. All three functions (handleRunFeature,handleSendFollowUp,handleCommitFeature) have been consistently updated to remove the worktreePath parameter.Also applies to: 438-445, 485-489
259-264: LGTM! Improved dependency blocking UX.The enhanced message formatting with truncated descriptions and comma-separated dependency lists provides clearer feedback to users when starting features with incomplete dependencies.
674-698: LGTM! Primary branch filtering logic is sound.The implementation correctly uses
getPrimaryWorktreeBranchandisPrimaryWorktreeBranchhelpers to filter backlog features based on the selected worktree branch. The logic appropriately shows unassigned features (nobranchName) or primary-branch features when viewing the primary worktree, and filters by exact branch match for other worktrees.Note on Line 681: The fallback chain
f.branchName || primaryBranch || "main"uses"main"as a last resort whenprojectPathis falsy. While this is defensive programming, it's a reasonable fallback since the code shouldn't execute in practice without a validprojectPath.
726-740: LGTM! Excellent sorting enhancement for dependency-aware feature selection.The new sorting logic elegantly separates blocked features from unblocked ones, placing blocked features at the end while maintaining priority-based ordering within each group. This ensures that features with unmet dependencies don't prevent higher-priority eligible features from starting.
743-758: LGTM! Improved feature selection with dependency awareness.The implementation correctly identifies the first eligible feature by checking for blocking dependencies when enabled. The informative toast message when all features are blocked provides clear guidance to users.
770-802: LGTM! Archival approach preserves data and improves feature lifecycle management.The renamed function
handleArchiveAllVerifiednow sets feature status to"completed"instead of deleting them, preserving data for audit trails and potential restoration. This aligns with the PR's objective to archive verified features rather than delete them. Dependencies have been correctly updated to useupdateFeatureandpersistFeatureUpdate.
823-823: LGTM! Return statement correctly updated.The return statement properly exposes
handleArchiveAllVerified, reflecting the public API change from the previoushandleDeleteAllVerified.
- Added functionality to select "Other branch" in the edit feature dialog, enabling the branch input field. - Updated the locator for the branch input from 'edit-feature-branch' to 'edit-feature-input' for consistency across tests.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/app/tests/worktree-integration.spec.ts (1)
2542-2612: Line 2611 asserts worktreePath inconsistently with execution-time model.This test violates the established pattern across the codebase. Lines 779, 839, 903, 930, and 2453 all explicitly verify
worktreePathisundefined, with comments confirming: "worktrees are created at execution time, not when adding/editing features."Line 2611 should expect
featureData.worktreePathto beundefined, with onlyfeatureData.branchNameset toexistingBranch. The feature data should store only the branch name; the worktree path is resolved at execution time, not during backlog operations.
🧹 Nitpick comments (2)
apps/app/tests/worktree-integration.spec.ts (2)
2439-2439: Replace hard wait with explicit assertion for more deterministic dialog closure.After clicking "Save Changes", use an explicit assertion instead of a fixed timeout to wait for the dialog to close. This approach is more reliable and aligns with Playwright best practices.
- // Wait for the dialog to close - await page.waitForTimeout(2000); + // Wait for the dialog to close + await expect(editDialog).not.toBeVisible({ timeout: 5000 });This is the only instance of a timeout >= 1000ms in the test file, and page.waitForTimeout() is not recommended for real-time scripts as tests may become flaky. Using Playwright's web-first assertions like toBeVisible is recommended to wait for elements to change, appear or disappear.
2417-2419: Use Playwright's semantic locators for radio button selection.Instead of
page.locator('label[for="edit-feature-other"]'), use page.getByLabel() to locate a form control by associated label's text. This approach is more resilient and aligns with Playwright best practices. Consider:const otherBranchRadio = page.getByLabel('Other branch'); await otherBranchRadio.click();Or use the role-based alternative:
const otherBranchRadio = page.getByRole('radio', { name: 'Other branch' }); await otherBranchRadio.click();These semantic locators are more maintainable and provide better alignment with prioritizing user-facing attributes and explicit contracts for test resilience.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/app/tests/worktree-integration.spec.ts(14 hunks)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (6)
apps/app/tests/worktree-integration.spec.ts (6)
744-779: LGTM: Test correctly reflects execution-time worktree creation.The test accurately validates the new semantics where worktrees are created at execution time rather than backlog addition. The assertion that
worktreePathis undefined when adding features to the backlog correctly reflects this architectural shift.
782-843: LGTM: Comprehensive validation of execution-time semantics.This test thoroughly validates the new behavior where branches and worktrees are created at execution time. The assertions correctly verify that:
- Branches don't exist before feature creation
branchNameis stored in backlogworktreePathremains undefined until execution- Feature status is properly set to backlog
845-959: LGTM: Worktree deletion behavior correctly validated.The test accurately validates the deletion workflow:
branchNameis stored butworktreePathremains undefined when adding features- After deletion,
branchNameresets to null whileworktreePathstays undefined- Unassigned features are correctly hidden on non-primary worktrees (Line 956-958)
The visibility filtering aligns with the execution-time worktree model.
1049-1087: LGTM: UI changes correctly tested for branch pre-fill.The test accurately validates the updated UI behavior where "Use current selected branch" is displayed when a worktree is selected. The locator
label[for="feature-current"]correctly targets the current branch label, and the assertion verifies the branch name is shown in the label text.
2456-2540: LGTM: Test data attribute usage improves test stability.The update to use
[data-testid="edit-feature-input"](Line 2516) follows best practices for Playwright selectors by relying on explicit test identifiers rather than implementation details.
2438-2446: Clarify branch creation timing in test comments.The test verifies that worktrees are NOT created during editing (deferred to execution time) but branches ARE created immediately. Line 2445 uses conditional phrasing ("if branch creation is part of the autocomplete flow") that masks this intentional behavior difference. Either confirm that branch creation is always immediate during autocomplete, or update the comment to explicitly state whether this timing is guaranteed or implementation-dependent.
- Adjusted Playwright configuration to set workers to undefined for improved test execution. - Updated comments in worktree integration tests to clarify branch creation logic and ensure accurate assertions regarding branch and worktree paths.
Merge latest features from main including: - PR #161 (worktree-confusion): Clarified branch handling in dialogs - PR #160 (speckits-rebase): Planning mode functionality Resolved conflicts: - add-feature-dialog.tsx: Combined TanStack Router navigation with branch selection state - worktree-integration.spec.ts: Updated tests for new worktree behavior (created at execution time) - package-lock.json: Regenerated after merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
UI/UX Improvements
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.