feat: add branch card counts to UI components #168
Conversation
- Introduced branchCardCounts prop to various components to display unarchived card counts per branch. - Updated BranchAutocomplete, BoardView, AddFeatureDialog, EditFeatureDialog, BranchSelector, WorktreePanel, and WorktreeTab to utilize the new prop for enhanced branch management visibility. - Enhanced user experience by showing card counts alongside branch names in relevant UI elements.
|
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. WalkthroughAdds propagation of a memoized branchCardCounts map through branch selectors, dialogs, and worktree UI; client now attempts server-assisted worktree creation on feature add/update; server auto-mode stops creating worktrees on-demand and only uses pre-existing worktrees or falls back to the project path. Changes
Sequence Diagram(s)mermaid 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)
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 integrates a new feature across the application's UI to display the number of unarchived cards associated with each branch. This enhancement provides users with immediate visibility into the workload or active features linked to specific branches, improving overall branch management and user experience within the application. 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 successfully introduces the branchCardCounts prop to display unarchived card counts for branches across various UI components. The changes are well-propagated through the component tree, from BoardView where the counts are calculated, down to BranchAutocomplete and WorktreeTab where they are displayed. The logic for calculating and displaying the counts is correct. I have one suggestion to refactor the card count calculation for improved conciseness and readability.
| const branchCardCounts = useMemo(() => { | ||
| const counts: Record<string, number> = {}; | ||
|
|
||
| // Count unarchived features (status !== "completed") per branch | ||
| hookFeatures.forEach((feature) => { | ||
| if (feature.status !== "completed") { | ||
| const branch = feature.branchName || "main"; | ||
| counts[branch] = (counts[branch] || 0) + 1; | ||
| } | ||
| }); | ||
|
|
||
| return counts; | ||
| }, [hookFeatures]); |
There was a problem hiding this comment.
This useMemo hook can be made more concise by using the reduce method instead of forEach to calculate the branch card counts. This is a common pattern in functional programming and can make the code more readable and expressive.
const branchCardCounts = useMemo(() => {
return hookFeatures.reduce((counts, feature) => {
if (feature.status !== "completed") {
const branch = feature.branchName || "main";
counts[branch] = (counts[branch] || 0) + 1;
}
return counts;
}, {} as Record<string, number>);
}, [hookFeatures]);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/app/src/components/ui/branch-autocomplete.tsx(3 hunks)apps/app/src/components/views/board-view.tsx(4 hunks)apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx(8 hunks)apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx(3 hunks)apps/app/src/components/views/board-view/shared/branch-selector.tsx(3 hunks)apps/app/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(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(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/app/src/components/views/board-view/worktree-panel/worktree-panel.tsx (2)
apps/app/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
WorktreeTab(46-194)apps/app/src/components/views/board-view/worktree-panel/components/index.ts (1)
WorktreeTab(3-3)
⏰ 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: build
- GitHub Check: e2e
🔇 Additional comments (9)
apps/app/src/components/views/board-view/worktree-panel/types.ts (1)
38-38: LGTM! Clean type addition.The optional
branchCardCountsprop is well-typed and clearly documented. The comment explains the mapping purpose effectively.apps/app/src/components/views/board-view.tsx (2)
851-851: LGTM! Clean prop threading.The
branchCardCountsprop is correctly passed toWorktreePanel, enabling branch-aware card count display.
948-948: LGTM! Consistent prop threading to dialogs.The
branchCardCountsprop is correctly propagated to bothAddFeatureDialogandEditFeatureDialog, maintaining consistency across the feature creation and editing flows.Also applies to: 964-964
apps/app/src/components/views/board-view/shared/branch-selector.tsx (1)
13-13: LGTM! Clean prop threading.The
branchCardCountsprop is properly typed, destructured, and forwarded toBranchAutocomplete. The implementation is straightforward and correct.Also applies to: 25-25, 74-74
apps/app/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
114-154: LGTM! Clean card count mapping.The logic correctly maps
branchCardCountsto per-worktreecardCountusing optional chaining, and passes it toWorktreeTab. The implementation handles undefined values gracefully.apps/app/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (1)
80-80: LGTM! Consistent prop threading.The
branchCardCountsprop is properly added to the interface, destructured, and forwarded toBranchSelector. The implementation is consistent with theAddFeatureDialogpattern.Also applies to: 94-94, 394-394
apps/app/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
12-12: LGTM! Clean badge rendering logic.The
cardCountprop is properly typed and used to conditionally render the card count badge. The logic correctly handles edge cases:
- Badge only appears when
cardCountis defined and greater than 0- Rendering is consistent across both main and non-main worktrees
- Zero counts are appropriately hidden
Also applies to: 48-48, 102-106, 145-149
apps/app/src/components/views/board-view/dialogs/add-feature-dialog.tsx (2)
76-76: LGTM! Clean prop threading.The
branchCardCountsprop is properly added to the interface, destructured, and forwarded toBranchSelector. The implementation is consistent with the dialog pattern used throughout.Also applies to: 91-91, 414-414
143-149: Good practice: expanded dependency array.The
useEffectdependency array now includesdefaultPlanningModeanddefaultRequirePlanApproval, ensuring the effect re-runs when these defaults change. This maintains proper synchronization with store values.
- Fix branch fallback logic: use nullish coalescing (??) instead of || to handle empty strings correctly (empty string represents unassigned features, not main branch) - Refactor branchCardCounts calculation: use reduce instead of forEach for better conciseness and readability - Fix badge semantics in BranchAutocomplete: check branchCardCounts !== undefined first to ensure numeric badges (including 0) only appear when actual count data exists, while 'default' is reserved for when count data is unavailable
- Consolidated imports in session-manager.tsx for cleaner code. - Improved state initialization formatting for better readability. - Updated board-view.tsx to enhance feature management, including the use of refs to track running tasks and prevent unnecessary effect re-runs. - Added affectedFeatureCount prop to DeleteWorktreeDialog for better user feedback on feature assignments. - Refactored useBoardActions to ensure worktrees are created when features are added or updated, improving overall workflow efficiency.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/auto-mode-service.ts (1)
554-554: Fix type mismatch causing pipeline failure.The pipeline is failing because
branchNameisstring | undefinedbuttempRunningFeature.branchNameexpectsstring | null. Convertundefinedtonullto match the interface.🔎 Proposed fix
- tempRunningFeature.branchName = branchName; + tempRunningFeature.branchName = branchName ?? null;
♻️ Duplicate comments (2)
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (1)
176-211: Duplicate code - see comment on lines 107-142.This block is identical to the worktree creation logic in
handleAddFeature. Refer to the refactoring suggestion on lines 107-142.apps/app/src/components/views/board-view.tsx (1)
1127-1132: Consistent feature reset logic on worktree deletion.The feature reset logic correctly matches the pattern in
handleRemovedWorktrees, updating both local state and persisting changes. The type assertion concern for Line 1129 is the same as noted for Line 317.
🧹 Nitpick comments (3)
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (1)
828-829: Clarify comment to avoid confusion with new worktree creation logic.The comment "No worktree creation on client" might be confusing given that worktrees are now created client-side in
handleAddFeatureandhandleUpdateFeature. Consider rephrasing to clarify this specifically refers to the start-next-features flow.🔎 Suggested clarification
- // Start only one feature per keypress (user must press again for next) - // Simplified: No worktree creation on client - server derives workDir from feature.branchName + // Start only one feature per keypress (user must press again for next) + // Note: Worktree is pre-created during add/update; server derives workDir from feature.branchName at execution await handleStartImplementation(featureToStart);apps/app/src/components/views/board-view.tsx (2)
316-323: Simplify the type assertion for resetting branchName.The double type assertion
null as unknown as string | undefinedis unnecessarily complex. Consider usingundefineddirectly or updating the type definition to properly acceptnull.🔎 Proposed simplification
- const updates = { branchName: null as unknown as string | undefined }; + const updates = { branchName: undefined };This achieves the same result with clearer intent. The same pattern is repeated at Line 1129.
699-728: Consider optimizing callback dependencies for performance.Both
handlePlanApproveandhandlePlanRejectnow includehookFeaturesin their dependency arrays, causing them to be recreated on every feature update. Since they only need to find a single feature by ID, consider using a ref pattern (similar tohookFeaturesRefalready used elsewhere) to avoid frequent callback recreation.🔎 Suggested optimization
Access the current feature version using the existing
hookFeaturesRef:const handlePlanApprove = useCallback( async (editedPlan?: string) => { // ... existing code ... try { // ... existing code ... if (result.success) { - const currentFeature = hookFeatures.find((f) => f.id === featureId); + const currentFeature = hookFeaturesRef.current.find((f) => f.id === featureId); // ... rest of the code ... } } // ... }, [ pendingPlanApproval, currentProject, setPendingPlanApproval, updateFeature, loadFeatures, - hookFeatures, ] );Apply the same pattern to
handlePlanReject.Also applies to: 755-784
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/app/src/components/session-manager.tsx(3 hunks)apps/app/src/components/views/board-view.tsx(18 hunks)apps/app/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx(4 hunks)apps/app/src/components/views/board-view/hooks/use-board-actions.ts(4 hunks)apps/server/src/services/auto-mode-service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/app/src/components/session-manager.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/server/src/services/auto-mode-service.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 (3)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)apps/app/src/lib/electron.ts (1)
getElectronAPI(549-559)apps/app/tests/utils/views/board.ts (1)
addFeature(197-205)
🪛 GitHub Actions: E2E Tests
apps/server/src/services/auto-mode-service.ts
[error] 554-554: Type 'string | undefined' is not assignable to type 'string | null'.
🪛 GitHub Actions: PR Build Check
apps/server/src/services/auto-mode-service.ts
[error] 554-554: TS2322: Type 'string | undefined' is not assignable to type 'string | null'.
🔇 Additional comments (15)
apps/app/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (4)
15-15: LGTM: FileWarning icon import added.The import is correctly structured and the icon is appropriately used in the affected features warning block.
33-34: LGTM: Well-documented optional prop.The JSDoc comment clearly explains the purpose, and making it optional allows for gradual adoption across call sites.
43-43: LGTM: Sensible default value.The default value of
0elegantly handles the optional prop and prevents the warning from showing when the count is unknown or not provided.
106-116: LGTM: Excellent user communication for data impact.The warning block effectively communicates the consequences of deleting a worktree with assigned features:
- Clear messaging: Users understand that features will be unassigned and moved to the main worktree.
- Appropriate severity: Orange theme signals a warning that's more serious than a notice but less critical than an error.
- Correct pluralization: The logic handles singular and plural cases correctly across all three conditionals.
- Consistent styling: Matches the pattern established by the uncommitted changes warning below.
- Good prioritization: Appearing before the changes warning emphasizes the data impact.
apps/app/src/components/views/board-view/hooks/use-board-actions.ts (3)
85-86: LGTM!Clear comments explaining the worktree pre-creation strategy.
154-154: LGTM!Dependency array correctly updated to include
useWorktrees,currentProject, andonWorktreeCreated.
225-225: LGTM!Dependency array correctly updated to match the new closure dependencies.
apps/server/src/services/auto-mode-service.ts (2)
523-525: Verify downstream handling whenbranchNameis undefined.The code assigns
branchName(which can beundefinedfromfeature.branchName) directly totempRunningFeature.branchNameat line 554 without a null check. While there's a conditional check at line 529 for worktree lookup, the main assignment proceeds regardless. Ensure this doesn't cause type errors or silent failures if the feature lacks an associated branch.
527-545: Worktree lookup and fallback logic is correct.The approach of pre-creating worktrees when features are added/edited (via the UI API), then looking them up during execution with a graceful fallback to the project path, is sound. The warning log at lines 541-542 will effectively help identify cases where worktrees are missing, whether due to manual deletion, creation failures, or edge cases.
apps/app/src/components/views/board-view.tsx (6)
273-282: LGTM! Previous concerns resolved.The
branchCardCountscalculation correctly addresses both previous review comments: it now usesreducefor a more functional approach and employs nullish coalescing (??) to properly handle empty strings as distinct from null/undefined.
432-442: LGTM! Proper ref pattern to prevent effect churn.The ref pattern for
runningAutoTasksandhandleStartImplementationcorrectly prevents the auto-mode effect from re-running when these values change, which would otherwise clearpendingFeaturesRefand cause concurrency issues.
511-513: LGTM! Consistent ref usage in auto-mode effect.The ref-based access patterns are correctly implemented throughout the auto-mode effect, preventing unnecessary re-runs while maintaining access to current values.
Also applies to: 568-571, 597-597
880-880: LGTM! branchCardCounts correctly threaded to UI components.The
branchCardCountsprop is consistently passed toWorktreePanel,AddFeatureDialog, andEditFeatureDialog, aligning with the PR objectives to display per-branch card counts throughout the UI.Also applies to: 977-977, 993-993
1115-1121: LGTM! Clearer inline calculation.The inline
affectedFeatureCountcalculation usingfilteris more explicit and easier to understand than the previous implementation.
623-624: LGTM! Minor improvements to comments, formatting, and type checks.The additional comments explaining ref usage, formatting improvements, and more concise type checking all enhance code clarity without introducing issues.
Also applies to: 671-673, 795-795
| checkAndStartFeatures(); | ||
| const interval = setInterval(checkAndStartFeatures, 3000); | ||
| const interval = setInterval(checkAndStartFeatures, 1000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the file around lines 610-611 to see the actual comment and code
sed -n '600,620p' apps/app/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 659
Update comment to reflect actual interval timing.
The comment states "every 3 seconds" but the interval is set to 1000ms (1 second). Update the comment on line 609 to: // Check immediately, then every 1 second to match the implemented behavior.
🤖 Prompt for AI Agents
In apps/app/src/components/views/board-view.tsx around lines 609 to 611, the
inline comment incorrectly says "every 3 seconds" while the code calls
checkAndStartFeatures() immediately then sets setInterval(checkAndStartFeatures,
1000) (1 second). Update the comment on line 609 to read "// Check immediately,
then every 1 second" so it matches the implemented interval.
| // If worktrees enabled and a branch is specified, create the worktree now | ||
| // This ensures the worktree exists before the feature starts | ||
| if (useWorktrees && finalBranchName && currentProject) { | ||
| try { | ||
| const api = getElectronAPI(); | ||
| if (api?.worktree?.create) { | ||
| const result = await api.worktree.create( | ||
| currentProject.path, | ||
| finalBranchName | ||
| ); | ||
| if (result.success) { | ||
| console.log( | ||
| `[Board] Worktree for branch "${finalBranchName}" ${ | ||
| result.worktree?.isNew ? "created" : "already exists" | ||
| }` | ||
| ); | ||
| // Refresh worktree list in UI | ||
| onWorktreeCreated?.(); | ||
| } else { | ||
| console.error( | ||
| `[Board] Failed to create worktree for branch "${finalBranchName}":`, | ||
| result.error | ||
| ); | ||
| toast.error("Failed to create worktree", { | ||
| description: result.error || "An error occurred", | ||
| }); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error("[Board] Error creating worktree:", error); | ||
| toast.error("Failed to create worktree", { | ||
| description: | ||
| error instanceof Error ? error.message : "An error occurred", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find apps -name "use-board-actions.ts" -type fRepository: AutoMaker-Org/automaker
Length of output: 134
🏁 Script executed:
cat -n apps/app/src/components/views/board-view/hooks/use-board-actions.ts | sed -n '100,220p'Repository: AutoMaker-Org/automaker
Length of output: 5249
🏁 Script executed:
# Verify the control flow - check if feature creation is conditional on worktree success
sed -n '107,152p' apps/app/src/components/views/board-view/hooks/use-board-actions.ts | grep -A 20 "^ }"Repository: AutoMaker-Org/automaker
Length of output: 440
Extract duplicate worktree creation logic into a helper function.
Lines 107-142 and 176-211 contain identical worktree creation code (35+ lines) in handleAddFeature and handleUpdateFeature. Extract this into a reusable helper to reduce maintenance risk and inconsistency.
Clarify whether feature creation should proceed when worktree creation fails.
Currently, feature creation continues even if worktree creation fails—only an error toast is shown. Verify this is the intended behavior, as users may expect the operation to fail entirely if the worktree cannot be established. If intentional, document this design choice; otherwise, consider aborting feature creation on worktree failure.
- Modified test descriptions to clarify when worktrees are created during feature addition and editing. - Updated assertions to verify that worktrees and branches are created as expected when features are added or edited. - Enhanced test logic to ensure accurate verification of worktree existence and branch creation, reflecting recent changes in worktree management.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/server/tests/integration/services/auto-mode-service.integration.test.ts (1)
50-68: Test setup correctly simulates client-side worktree creation.The test now pre-creates the worktree before execution, aligning with the PR objective where worktrees are created upfront by the client rather than on-demand by the server. The Git command structure is correct and properly creates both the branch and worktree.
Minor: Use ES6 shorthand property syntax
branchName: branchName, + branchName,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/app/tests/worktree-integration.spec.ts(5 hunks)apps/server/tests/integration/services/auto-mode-service.integration.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/tests/integration/services/auto-mode-service.integration.test.ts (1)
apps/server/tests/integration/helpers/git-test-repo.ts (3)
createTestFeature(76-89)listBranches(94-100)listWorktrees(105-122)
🔇 Additional comments (3)
apps/server/tests/integration/services/auto-mode-service.integration.test.ts (1)
16-19: LGTM! Clean helper setup.The
execAsynchelper correctly wraps the callback-basedexecin a promise usingpromisify, enabling clean async/await usage for Git commands in the test.apps/app/tests/worktree-integration.spec.ts (2)
813-819: LGTM! Assertions correctly validate the new worktree creation timing.The test properly verifies that both the branch and worktree are created when the feature is added, which aligns with the PR's change to create worktrees at feature add/edit time rather than execution time.
2439-2447: LGTM! Edit feature test correctly validates new worktree creation behavior.The test properly verifies that when editing a feature to use a new branch:
- The worktree is created during the edit operation (line 2443)
- The branch is created (line 2447)
- The feature data is updated with the new branch name (line 2451)
This correctly validates the new behavior where worktrees are created at edit time. The same 2000ms timeout concern mentioned in the earlier comment applies here as well.
| // Wait for feature to be saved and worktree to be created | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and examine lines 810-811 with context
find . -name "worktree-integration.spec.ts" -type f 2>/dev/null | head -1Repository: AutoMaker-Org/automaker
Length of output: 112
🏁 Script executed:
# Read the test file to see the actual code at lines 810-811
if [ -f "apps/app/tests/worktree-integration.spec.ts" ]; then
wc -l "apps/app/tests/worktree-integration.spec.ts"
fiRepository: AutoMaker-Org/automaker
Length of output: 115
🏁 Script executed:
# Search for worktree creation calls and await patterns
rg -n 'createWorktree|apiCreateWorktree' --type=ts -A 3 -B 3 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 7295
🏁 Script executed:
# Search for Dialog files that might contain worktree creation
fd -e ts -e tsx '.*Dialog' 2>/dev/null | grep -i feature | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Read the test file around lines 810-811 with full context
sed -n '800,820p' apps/app/tests/worktree-integration.spec.tsRepository: AutoMaker-Org/automaker
Length of output: 795
🏁 Script executed:
# Read more context around the timeout to see the test logic
sed -n '782,845p' apps/app/tests/worktree-integration.spec.tsRepository: AutoMaker-Org/automaker
Length of output: 2533
🏁 Script executed:
# Search for feature add/edit related dialog files
fd -type f \( -name "*AddFeature*" -o -name "*EditFeature*" -o -name "*Feature*Dialog*" \) --extension=ts --extension=tsx 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for worktree creation in source code (not tests)
rg -n 'createWorktree|createWorktreeDirectly' --type=ts -g '!tests' -g '!spec' -A 5 -B 2 | head -150Repository: AutoMaker-Org/automaker
Length of output: 2160
🏁 Script executed:
# Search for confirmAddFeature implementation
rg -n 'confirmAddFeature|fillAddFeatureDialog|clickAddFeature' --type=ts -B 3 -A 10 | head -100Repository: AutoMaker-Org/automaker
Length of output: 7926
🏁 Script executed:
# Search for feature creation API endpoint or handler
rg -n 'addFeature|createFeature' --type=ts -g '!tests' -A 5 -B 2 | head -150Repository: AutoMaker-Org/automaker
Length of output: 11531
🏁 Script executed:
# Look for waitForNetworkIdle to see how other similar operations wait
rg -n 'waitForNetworkIdle|waitForResponse' apps/app/tests/worktree-integration.spec.ts -B 2 -A 2 | head -60Repository: AutoMaker-Org/automaker
Length of output: 1748
🏁 Script executed:
# Find confirmAddFeature implementation
rg -n 'export.*confirmAddFeature|function confirmAddFeature' --type=ts -A 15Repository: AutoMaker-Org/automaker
Length of output: 1171
🏁 Script executed:
# Check persistFeatureCreate to see if it awaits worktree
rg -n 'persistFeatureCreate' --type=ts -A 10 -B 2 | head -80Repository: AutoMaker-Org/automaker
Length of output: 7352
🏁 Script executed:
# Search for where worktree creation happens during feature add
rg -n 'onWorktreeCreated|useWorktrees' --type=ts apps/app/src/components/views/board-view/hooks/use-board-actions.ts -A 5 -B 5 | head -100Repository: AutoMaker-Org/automaker
Length of output: 3825
Replace page.waitForTimeout(2000) with a proper wait condition for worktree creation.
The hard wait masks an underlying async operation. While production code properly awaits api.worktree.create() when adding features with branches (lines 109-124 in use-board-actions.ts), the test helper confirmAddFeature() only waits for the dialog to close, not for the background worktree creation to complete. The timeout was doubled from 1000ms to 2000ms to accommodate slower systems—a sign that relying on arbitrary delays is flaky.
Instead, replace the hard wait with a condition-based wait:
await expect(page).toHaveURL(/.*/, { timeout: 5000 }); // Wait for any pending networkOr use a more specific check that the worktree directory exists or a related UI state updates, ensuring the test reliably detects completion regardless of system performance.
🤖 Prompt for AI Agents
In apps/app/tests/worktree-integration.spec.ts around lines 810-811, replace the
hard sleep await page.waitForTimeout(2000) with a deterministic wait for the
worktree creation to finish: update the test to wait on a condition (e.g. use
Playwright's expect(page).toHaveURL(/.*/, { timeout: 5000 }) to wait for pending
navigation/network, or better assert a specific UI change or existence of the
worktree directory/indicator that the background api.worktree.create()
completed); modify confirmAddFeature() or the test to await that condition
instead of relying on an arbitrary timeout so the test is robust across
machines.
| // Verify branch exists (was created when worktree was created) | ||
| const branches = await listBranches(testRepo.path); | ||
| expect(branches).toContain("feature/test-feature-1"); | ||
| expect(branches).toContain(branchName); | ||
|
|
||
| // Verify worktree exists and is being used | ||
| // The service should have found and used the worktree (check via logs) | ||
| // We can verify the worktree exists by checking git worktree list | ||
| const worktrees = await listWorktrees(testRepo.path); | ||
| expect(worktrees.length).toBeGreaterThan(0); | ||
| // Verify that at least one worktree path contains our feature ID | ||
| const worktreePathsMatch = worktrees.some(wt => | ||
| wt.includes("test-feature-1") || wt.includes(".worktrees") | ||
| ); | ||
| expect(worktreePathsMatch).toBe(true); |
There was a problem hiding this comment.
Tighten the worktree verification logic.
The branch verification is correct, but the worktree check on lines 110-112 is overly permissive. The condition wt.includes(".worktrees") will pass if ANY worktree exists in the .worktrees directory, even if it's not for test-feature-1. This could lead to false positives if other tests leave worktrees behind.
Suggested fix: Verify the specific worktree path
- // Verify that at least one worktree path contains our feature ID
- const worktreePathsMatch = worktrees.some(wt =>
- wt.includes("test-feature-1") || wt.includes(".worktrees")
- );
- expect(worktreePathsMatch).toBe(true);
+ // Verify that the worktree for our feature exists
+ const expectedWorktreePath = path.join(testRepo.path, ".worktrees", "test-feature-1");
+ const worktreeExists = worktrees.some(wt => wt === expectedWorktreePath);
+ expect(worktreeExists).toBe(true);Alternatively, you can keep the current approach but remove the .worktrees check:
const worktreePathsMatch = worktrees.some(wt =>
- wt.includes("test-feature-1") || wt.includes(".worktrees")
+ wt.includes("test-feature-1")
);📝 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.
| // Verify branch exists (was created when worktree was created) | |
| const branches = await listBranches(testRepo.path); | |
| expect(branches).toContain("feature/test-feature-1"); | |
| expect(branches).toContain(branchName); | |
| // Verify worktree exists and is being used | |
| // The service should have found and used the worktree (check via logs) | |
| // We can verify the worktree exists by checking git worktree list | |
| const worktrees = await listWorktrees(testRepo.path); | |
| expect(worktrees.length).toBeGreaterThan(0); | |
| // Verify that at least one worktree path contains our feature ID | |
| const worktreePathsMatch = worktrees.some(wt => | |
| wt.includes("test-feature-1") || wt.includes(".worktrees") | |
| ); | |
| expect(worktreePathsMatch).toBe(true); | |
| // Verify branch exists (was created when worktree was created) | |
| const branches = await listBranches(testRepo.path); | |
| expect(branches).toContain(branchName); | |
| // Verify worktree exists and is being used | |
| // The service should have found and used the worktree (check via logs) | |
| // We can verify the worktree exists by checking git worktree list | |
| const worktrees = await listWorktrees(testRepo.path); | |
| expect(worktrees.length).toBeGreaterThan(0); | |
| // Verify that at least one worktree path contains our feature ID | |
| const worktreePathsMatch = worktrees.some(wt => | |
| wt.includes("test-feature-1") | |
| ); | |
| expect(worktreePathsMatch).toBe(true); |
🤖 Prompt for AI Agents
In apps/server/tests/integration/services/auto-mode-service.integration.test.ts
around lines 100 to 113, the worktree verification is too permissive because
wt.includes(".worktrees") can match unrelated worktrees; change the assertion to
verify the specific worktree for this test by checking the worktree path
contains the feature identifier (e.g., "test-feature-1") or the exact expected
worktree directory/name used by the test, remove the generic ".worktrees" check,
and assert that at least one worktree path matches that specific criteria.
- Updated branchName assignment to use nullish coalescing, ensuring that unassigned features are correctly set to null instead of an empty string. This change improves the handling of feature states during the update process.
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/server/src/services/auto-mode-service.ts (1)
731-731: Align branchName derivation with executeFeature changes.Line 731 still uses a fallback pattern (
feature?.branchName || 'feature/${featureId}') that was removed fromexecuteFeatureat line 525. This creates inconsistent behavior:
executeFeature: usesfeature.branchNamedirectly (no fallback)followUpFeature: falls back tofeature/${featureId}ifbranchNameis missingThis inconsistency could cause
followUpFeatureto search for a worktree with branchfeature/${featureId}when the feature has nobranchNameset, whileexecuteFeaturewould use the project path directly.🔎 Suggested fix for consistency
- const branchName = feature?.branchName || `feature/${featureId}`; + const branchName = feature?.branchName;And consider adding the same warning as suggested for
executeFeature:if (useWorktrees && branchName) { // Try to find existing worktree for this branch worktreePath = await this.findExistingWorktreeForBranch( projectPath, branchName ); if (worktreePath) { workDir = worktreePath; console.log( `[AutoMode] Follow-up using worktree for branch "${branchName}": ${workDir}` ); + } else { + console.warn( + `[AutoMode] Follow-up: Worktree for branch "${branchName}" not found, using project path` + ); } + } else if (useWorktrees && !branchName) { + console.warn( + `[AutoMode] Follow-up: Feature ${featureId} has no branchName set, using project path` + ); }
🧹 Nitpick comments (1)
apps/server/src/services/auto-mode-service.ts (1)
523-545: Consider logging a warning when branchName is missing but useWorktrees is enabled.The current logic only searches for a worktree when
branchNameexists (line 527). IfbranchNameis undefined or empty, the code silently falls back toprojectPathwithout any warning. This could be confusing for users who expect worktree isolation but unknowingly execute in the main project path.🔎 Suggested enhancement
const branchName = feature.branchName; if (useWorktrees && branchName) { // Try to find existing worktree for this branch // Worktree should already exist (created when feature was added/edited) worktreePath = await this.findExistingWorktreeForBranch( projectPath, branchName ); if (worktreePath) { console.log( `[AutoMode] Using worktree for branch "${branchName}": ${worktreePath}` ); } else { // Worktree doesn't exist - log warning and continue with project path console.warn( `[AutoMode] Worktree for branch "${branchName}" not found, using project path` ); } +} else if (useWorktrees && !branchName) { + // No branch name set for feature - log warning + console.warn( + `[AutoMode] Feature ${featureId} has no branchName set, using project path instead of worktree` + ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/src/services/auto-mode-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/services/auto-mode-service.ts (1)
apps/app/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
useWorktrees(15-105)
⏰ 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 (1)
apps/server/src/services/auto-mode-service.ts (1)
523-545: Client-side worktree creation is properly implemented with built-in idempotency and error handling.The codebase shows worktrees are created in
use-board-actions.tswhen features are added/edited with a branch name. The implementation:
- Calls
api.worktree.create()immediately when a feature withbranchNameis created or edited (before persistence)- Handles idempotency: re-creating the same branch returns the existing worktree instead of erroring
- Includes error handling with user-facing toast notifications (failures are not silent)
- Falls back gracefully on the server: if a worktree isn't found at execution time, logs a warning and proceeds with the main project path
Valid edge cases to consider:
- Existing features created before this architectural change won't have worktrees; consider a migration strategy if needed
- Manual
.automaker/features/*/feature.jsonedits bypass client-side worktree creation- The dependency is intentional: the server assumes worktrees exist but gracefully degrades if missing
The current implementation is sound for new features going forward.
- Added a function to ensure that a git repository has at least one commit before executing worktree commands. This function creates an empty initial commit with a predefined message if the repository is empty. - Updated the create route handler to call this function, ensuring smooth operation when adding worktrees to repositories without existing commits. - Introduced integration tests to verify the creation of the initial commit when no commits are present in the repository.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/tests/integration/routes/worktree/create.integration.test.ts (1)
1-68: Consider expanding test coverage for ensureInitialCommit behavior.The test correctly verifies that an initial commit is created when HEAD is missing. However, consider adding tests for:
- Repository with existing commits - verify that
ensureInitialCommitreturns false and doesn't create an additional commit.- Worktree creation success - verify that the worktree is actually created at the expected path after the initial commit.
- Git configuration missing - verify the error message when
user.nameoruser.emailare not configured.💡 Example: Test for existing commits
it("does not create a commit when HEAD already exists", async () => { await initRepoWithoutCommit(); // Create an initial commit manually await execAsync('git commit --allow-empty -m "existing commit"', { cwd: repoPath! }); const handler = createCreateHandler(); const json = vi.fn(); const status = vi.fn().mockReturnThis(); const req = { body: { projectPath: repoPath, branchName: "feature/with-head" }, } as any; const res = { json, status } as any; await handler(req, res); // Should still succeed expect(json).toHaveBeenCalled(); const payload = json.mock.calls[0][0]; expect(payload.success).toBe(true); // Should still have exactly 1 commit (not 2) const { stdout: commitCount } = await execAsync( "git rev-list --count HEAD", { cwd: repoPath! } ); expect(Number(commitCount.trim())).toBe(1); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/routes/worktree/common.ts(2 hunks)apps/server/src/routes/worktree/routes/create.ts(2 hunks)apps/server/tests/integration/routes/worktree/create.integration.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/server/src/routes/worktree/common.ts (2)
apps/server/src/routes/templates/common.ts (2)
logger(11-11)getErrorMessageShared(14-14)apps/server/src/routes/app-spec/common.ts (1)
getErrorMessageShared(71-71)
apps/server/tests/integration/routes/worktree/create.integration.test.ts (2)
apps/server/src/routes/worktree/routes/create.ts (1)
createCreateHandler(77-181)apps/server/src/routes/worktree/common.ts (1)
AUTOMAKER_INITIAL_COMMIT_MESSAGE(16-17)
apps/server/src/routes/worktree/routes/create.ts (1)
apps/server/src/routes/worktree/common.ts (1)
ensureInitialCommit(84-105)
⏰ 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 (3)
apps/server/src/routes/worktree/common.ts (1)
16-17: LGTM!The constant is well-named and provides a clear, consistent commit message for auto-generated initial commits.
apps/server/src/routes/worktree/routes/create.ts (2)
15-21: LGTM!The import is clean and properly organized with other common utilities.
102-103: LGTM! Excellent placement and documentation.The
ensureInitialCommitcall is correctly positioned after the git repository validation and before any operations that reference HEAD. The comment clearly explains why this precondition is necessary, making the code maintainable.This ensures that:
- Line 106's
findExistingWorktreeForBranchcan safely check worktree branches- Line 137's
git rev-parse --verifyfor branch checking works correctly- Line 152's default
base = baseBranch || "HEAD"always has a valid HEAD to reference
| export async function ensureInitialCommit(repoPath: string): Promise<boolean> { | ||
| try { | ||
| await execAsync("git rev-parse --verify HEAD", { cwd: repoPath }); | ||
| return false; | ||
| } catch { | ||
| try { | ||
| await execAsync( | ||
| `git commit --allow-empty -m "${AUTOMAKER_INITIAL_COMMIT_MESSAGE}"`, | ||
| { cwd: repoPath } | ||
| ); | ||
| logger.info( | ||
| `[Worktree] Created initial empty commit to enable worktrees in ${repoPath}` | ||
| ); | ||
| return true; | ||
| } catch (error) { | ||
| const reason = getErrorMessageShared(error); | ||
| throw new Error( | ||
| `Failed to create initial git commit. Please commit manually and retry. ${reason}` | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the test file mentioned (lines 20-23) and surrounding context
git ls-files "**/common.ts" | grep -E "test|spec" | head -5Repository: AutoMaker-Org/automaker
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Find test files related to worktree/common
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "ensureInitialCommit" 2>/dev/null | head -3Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for git config setup in the codebase
echo "=== Searching for git config user setup ==="
rg "git config user\." --type ts
echo -e "\n=== Searching for execAsync git commands ==="
rg "execAsync.*git" -A 2 -B 1 --type ts | head -40
echo -e "\n=== Looking for test setup with git config ==="
rg "git config" --type ts -C 3 | head -50Repository: AutoMaker-Org/automaker
Length of output: 7899
🏁 Script executed:
#!/bin/bash
# Look at the actual ensureInitialCommit function file
cat -n apps/server/src/routes/worktree/common.ts | sed -n '70,115p'Repository: AutoMaker-Org/automaker
Length of output: 1424
🏁 Script executed:
#!/bin/bash
# Search for where ensureInitialCommit is called in the codebase
echo "=== Calls to ensureInitialCommit ==="
rg "ensureInitialCommit" --type ts -B 3 -A 3Repository: AutoMaker-Org/automaker
Length of output: 1922
🏁 Script executed:
#!/bin/bash
# Check if there's any production setup that configures git
echo "=== Searching for git setup in production code ==="
rg "git config" apps/server/src --type tsRepository: AutoMaker-Org/automaker
Length of output: 397
🏁 Script executed:
#!/bin/bash
# Check what getErrorMessageShared returns and how it handles git errors
rg "getErrorMessageShared" apps/server/src --type ts -A 5 | head -40Repository: AutoMaker-Org/automaker
Length of output: 2634
🏁 Script executed:
#!/bin/bash
# Find the implementation of getErrorMessageShared
fd -e ts | xargs grep -l "export.*getErrorMessageShared\|function getErrorMessageShared" | head -3Repository: AutoMaker-Org/automaker
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Find the actual implementation of getErrorMessage
cat apps/server/src/routes/common.ts | grep -A 20 "export.*getErrorMessage\|export function getErrorMessage"Repository: AutoMaker-Org/automaker
Length of output: 472
Consider improving error message guidance for git configuration.
Git requires user.name and user.email to be configured before committing, but the current error message doesn't mention this requirement. When git commit --allow-empty fails due to missing configuration, users will see only the underlying git error (already included via reason).
Since all test setups explicitly configure git before operations, production environments should ensure this is set up. The error message could proactively guide users:
} catch (error) {
const reason = getErrorMessageShared(error);
throw new Error(
- `Failed to create initial git commit. Please commit manually and retry. ${reason}`
+ `Failed to create initial git commit. Ensure git user.name and user.email are configured, then commit manually and retry. ${reason}`
);
}📝 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.
| export async function ensureInitialCommit(repoPath: string): Promise<boolean> { | |
| try { | |
| await execAsync("git rev-parse --verify HEAD", { cwd: repoPath }); | |
| return false; | |
| } catch { | |
| try { | |
| await execAsync( | |
| `git commit --allow-empty -m "${AUTOMAKER_INITIAL_COMMIT_MESSAGE}"`, | |
| { cwd: repoPath } | |
| ); | |
| logger.info( | |
| `[Worktree] Created initial empty commit to enable worktrees in ${repoPath}` | |
| ); | |
| return true; | |
| } catch (error) { | |
| const reason = getErrorMessageShared(error); | |
| throw new Error( | |
| `Failed to create initial git commit. Please commit manually and retry. ${reason}` | |
| ); | |
| } | |
| } | |
| } | |
| export async function ensureInitialCommit(repoPath: string): Promise<boolean> { | |
| try { | |
| await execAsync("git rev-parse --verify HEAD", { cwd: repoPath }); | |
| return false; | |
| } catch { | |
| try { | |
| await execAsync( | |
| `git commit --allow-empty -m "${AUTOMAKER_INITIAL_COMMIT_MESSAGE}"`, | |
| { cwd: repoPath } | |
| ); | |
| logger.info( | |
| `[Worktree] Created initial empty commit to enable worktrees in ${repoPath}` | |
| ); | |
| return true; | |
| } catch (error) { | |
| const reason = getErrorMessageShared(error); | |
| throw new Error( | |
| `Failed to create initial git commit. Ensure git user.name and user.email are configured, then commit manually and retry. ${reason}` | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/common.ts around lines 84 to 105, the error
thrown when creating the initial empty commit lacks proactive guidance about git
user configuration; update the thrown Error to include the original reason plus
a brief actionable suggestion to set git user.name and user.email (e.g. "git
config --global user.name 'Your Name' && git config --global user.email
'you@example.com'") or to configure them in the repo, so users get clear next
steps when commits fail due to missing git config.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Other
✏️ Tip: You can customize this high-level summary in your review settings.