feat: improve worktree and spec generation workflows #387
Conversation
Enhanced worktree management and spec generation with better UX and
comprehensive test coverage:
**Spec Generation Improvements:**
- Renamed "Target Branch" to "Worktree Branch" for clarity
- Changed from always-required input to optional checkbox pattern
- Auto-detects current git branch when "Use worktree branch" is unchecked
- Added /api/worktree/current-branch endpoint for branch detection
- Each project now uses its actual current branch (main/master/develop)
**Delete Worktree Enhancements:**
- Added "Delete backlog of this worktree (X features)" checkbox option
- Fixed bug: features now properly reassigned to main branch when not deleted
- Reduced race condition errors by removing features from state before deletion
- Fixed cancellation error toast when user cancels feature implementation
**Test Coverage:**
- Added comprehensive E2E tests for delete worktree functionality (5 tests)
- Move features to main when deleting worktree
- Delete features with checkbox checked
- Verify feature count display
- Delete/keep git branch based on checkbox state
- Added E2E tests for spec regeneration with worktrees (2 tests)
- Updated test helpers to work with worktree panel UI
- Fixed addFeature helper to detect current branch before selecting "Other"
**Technical Changes:**
- Added getCurrentBranch to WorktreeAPI interface
- Added data-testid to branch-switch-dropdown for testing
- Clean up existing features for branch before regenerating spec
- Remove features from local state before deletion to prevent updates
📝 WalkthroughWalkthroughAdds optional targetBranch/worktree support end-to-end: UI can request spec generation on a specified branch; server validates/creates git worktrees, propagates targetBranch into feature creation, and events include worktreePath/targetBranch. New worktree route and branch-name validation utilities added; tests updated/added. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant API as HttpApiClient
participant Server as SpecRegeneration Endpoint
participant Git as Git (worktree/branch)
participant FS as File system (features)
UI->>API: request generate/create spec (project, options, targetBranch?)
API->>Server: forward request (includes targetBranch)
Server->>Git: validateBranchName(targetBranch) (if provided)
alt targetBranch provided and not main/master
Server->>Git: ensure .worktrees exists, add worktree (existing or new from HEAD)
end
Server->>Server: generate spec -> responseText
Server->>Server: parseAndCreateFeatures(responseText, targetBranch)
Server->>FS: remove existing features with branchName == targetBranch (pre-clean)
Server->>FS: write new feature files with branchName = targetBranch
Server->>API: emit spec_regeneration_complete (includes targetBranch, worktreePath)
API->>UI: deliver event
UI->>Git: optionally switch UI to generated worktree using worktreePath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @yumesha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the user experience and underlying logic for managing Git worktrees and generating application specifications. It introduces more intuitive controls for creating and deleting worktrees, ensuring features are correctly handled during worktree operations, and provides greater flexibility in spec generation. The changes are backed by new API endpoints and comprehensive end-to-end tests to ensure stability and correctness. 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 significantly enhances worktree management and spec generation workflows, introducing a better user experience with more granular options and comprehensive test coverage. The changes include renaming UI elements for clarity, adding optional worktree branch creation, auto-detecting the current git branch, and improving the worktree deletion process. While the feature enhancements and added E2E tests are excellent, I've identified a critical command injection vulnerability in the new server-side worktree creation logic. The targetBranch parameter is used directly in shell commands without sanitization. I have provided a detailed comment and a code suggestion to address this security risk. Other than this critical issue, the pull request is well-structured and the new features are thoughtfully implemented.
|
|
||
| // Create worktree for target branch if specified and doesn't exist | ||
| if (targetBranch && targetBranch !== 'main' && targetBranch !== 'master') { | ||
| try { |
There was a problem hiding this comment.
The targetBranch variable, which comes from the request body, is used directly in shell commands via execAsync. This creates a critical command injection vulnerability. A malicious user could provide a crafted branch name like my-branch; rm -rf / to execute arbitrary commands on the server.
To fix this, you must validate the targetBranch name to ensure it only contains characters safe for a branch name before using it in any shell commands. I've suggested adding a validation function that checks for invalid characters and path traversal attempts.
const isValidGitBranchName = (branchName: string): boolean => {
// Prevents command injection and path traversal.
if (branchName.includes('..')) {
return false;
}
// A simplified regex for valid git branch names. It disallows characters
// often used in command injection attacks (;, &, |, $, `, etc.).
return /^[a-zA-Z0-9_./-]+$/.test(branchName);
};
if (!isValidGitBranchName(targetBranch)) {
const errorMessage = `Invalid target branch name provided: ${targetBranch}`;
logger.error(errorMessage);
// Throwing an error is important to stop execution and alert the caller.
throw new Error(errorMessage);
}
try {![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (1)
48-85: StaledeleteFeatures/deleteBranchstate across dialog opens can cause unintended data loss.
If a user checks “Delete backlog…”, cancels, then opens the dialog for another worktree, the previous selection persists. Reset state when the dialog opens/closes and disable the checkboxes whileisLoading.Proposed fix
@@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; @@ const [deleteBranch, setDeleteBranch] = useState(false); const [deleteFeatures, setDeleteFeatures] = useState(false); const [isLoading, setIsLoading] = useState(false); + + useEffect(() => { + if (!open) { + setDeleteBranch(false); + setDeleteFeatures(false); + } + }, [open]); @@ <Checkbox id="delete-branch" checked={deleteBranch} onCheckedChange={(checked) => setDeleteBranch(checked === true)} + disabled={isLoading} /> @@ <Checkbox id="delete-features" checked={deleteFeatures} onCheckedChange={(checked) => setDeleteFeatures(checked === true)} + disabled={isLoading} />Also applies to: 127-152
🤖 Fix all issues with AI agents
In @apps/server/src/routes/app-spec/generate-spec.ts:
- Around line 40-42: The code is injecting unvalidated targetBranch into shell
commands and filesystem paths (see parameter targetBranch, execAsync and
path.join(projectPath, '.worktrees', targetBranch)), enabling command injection
and path traversal; validate and sanitize targetBranch (allow only safe git-ref
characters, e.g. /^[A-Za-z0-9._\/-]+$/ but explicitly forbid sequences like
'..', null bytes, leading '/', and shell metacharacters) and reject invalid
values, then stop using interpolated shell strings—use
child_process.execFile/spawn with args or the existing git/worktree utility
functions instead of execAsync with template literals; when creating filesystem
paths use safe operations (e.g. path.basename or a whitelist mapping for
worktree names and path.resolve to ensure the result remains under projectPath)
so inputs cannot traverse out of the project, and apply the same validation/fix
for all usages of targetBranch in the file (including the block around lines
365–428).
- Around line 365-367: Replace the hardcoded check (targetBranch !== 'main' &&
targetBranch !== 'master') with a reusable primary-branch check using a
configurable list: create a helper (e.g., isPrimaryBranch(branch: string)) that
reads PRIMARY_BRANCHES from config/ENV (falling back to ['main','master']) and
returns PRIMARY_BRANCHES.includes(branch); then use if (targetBranch &&
!isPrimaryBranch(targetBranch)) when creating worktrees in generate-spec.ts (the
targetBranch conditional), parse-and-create-features.ts, and delete.ts so
additional primary branches like 'develop' are respected and the list can be
changed without code edits.
In @apps/server/src/routes/app-spec/parse-and-create-features.ts:
- Around line 68-69: TypeScript can't narrow secureFs.readFile's return from
string | Buffer to string, so explicitly assert the result as string: when
reading featureJsonPath use an assertion like (await
secureFs.readFile(featureJsonPath, 'utf-8')) as string and then pass that
asserted featureContent into JSON.parse to produce featureData; update the
reference to featureContent and ensure the assertion is applied to the
secureFs.readFile call (not to JSON.parse).
In @apps/ui/src/components/views/board-view.tsx:
- Around line 86-87: The onDeleted callback is causing race conditions and
double-deletions: compute mainBranch once (retrieve via
getCurrentWorktree/mainBranch) before processing, replace the forEach with
Promise.all over an array produced by map, and for each item await
handleDeleteFeature(...) (do not call removeFeature() separately since
handleDeleteFeature already removes it); likewise await
persistFeatureUpdate(...) in the reassign branch. After awaiting Promise.all for
all deletions/updates, then call setWorktreeRefreshKey to trigger refresh.
In @apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx:
- Around line 199-217: The onChange for the custom feature Input validates only
>0; update it to enforce the 1–200 range: parse the value in the handler used by
setCustomFeatureCount and only call onFeatureCountChange(num) when num is a
valid integer between 1 and 200 (inclusive). Keep updating the input state
(setCustomFeatureCount) for intermediate/empty values but avoid invoking
onFeatureCountChange with out-of-range numbers; reference the existing
setCustomFeatureCount, onFeatureCountChange, customFeatureCount and the Input
onChange handler to implement this check.
In @apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx:
- Around line 38-43: The component can leave featureCount unchanged when the
user selects "Custom" with an empty input; to fix, initialize customFeatureCount
from the current featureCount when toggling setIsCustom(true) (use
setCustomFeatureCount(String(featureCount)) and fall back to min/max bounds from
FEATURE_COUNT_OPTIONS if featureCount is undefined), ensure any parseInt of
customFeatureCount is clamped to the allowed min/max before calling the
emitter/updater, and when saving/closing use the clamped numeric value to set
featureCount; apply the same initialization/clamping logic wherever custom
feature input is handled (including the other block referenced around lines
164-217).
In @apps/ui/tests/git/delete-worktree-with-features.spec.ts:
- Around line 29-82: The keyboard shortcut used in createWorktree is
platform-unsafe: replace the hardcoded 'Control+a' in the createWorktree
function with the cross-platform 'ControlOrMeta+a' (so the select-all works on
macOS runners); ensure any other uses of 'Control+a' in this file are similarly
updated to 'ControlOrMeta+a' to avoid flakes.
In @apps/ui/tests/projects/regenerate-spec-with-worktree.spec.ts:
- Around line 180-203: The test currently skips verification if the features
directory doesn't exist; change it to assert its existence and that at least one
feature was found before inspecting feature.json files. Replace the conditional
checks around featuresDir and featureDirs with hard assertions (use
expect(fs.existsSync(featuresDir)).toBeTruthy() and
expect(featureDirs.length).toBeGreaterThan(0)) and then iterate featureDirs to
read each feature.json and assert featureJson has branchName and (if
worktreePath exists) that featureJson.branchName === targetBranch; keep the same
symbols: featuresDir, featureDirs, featureJsonPath, featureJson, worktreePath,
and targetBranch to locate the code to modify.
🧹 Nitpick comments (13)
apps/server/src/routes/worktree/routes/get-current-branch.ts (2)
1-3: Comment states GET but endpoint is POST.The comment on line 2 says "GET /current-branch endpoint" but based on the route registration in
index.ts, this is a POST endpoint.📝 Suggested fix
/** - * GET /current-branch endpoint - Get the current branch of a project + * POST /current-branch endpoint - Get the current branch of a project */
28-57: Consider adding a timeout to prevent indefinite hangs.The
execAsynccalls have no timeout configured. If the git command hangs (e.g., due to a corrupted repo or network issues with remote operations), this could block indefinitely.Additionally, the inner catch block (lines 53-57) silently returns
'main'as a fallback, which could mask legitimate git errors and return incorrect data to the caller.⏱️ Suggested improvement
try { - const { stdout } = await execAsync('git branch --show-current', { cwd: projectPath }); + const { stdout } = await execAsync('git branch --show-current', { + cwd: projectPath, + timeout: 10000 // 10 second timeout + }); const branch = stdout.trim(); if (!branch) { // Might be in detached HEAD state, try to get branch from symbolic-ref try { const { stdout: refStdout } = await execAsync('git symbolic-ref --short HEAD', { cwd: projectPath, + timeout: 10000 });apps/ui/tests/utils/views/board.ts (2)
134-161: ReplacewaitForTimeoutwith explicit wait conditions to reduce test flakiness.Using fixed
waitForTimeoutdelays (lines 137, 146, 153, 160) is discouraged in Playwright as it can cause flaky tests if the UI is slower or faster than expected. Consider usingwaitForconditions or waiting for specific elements/states instead.⏱️ Example improvement for one occurrence
if (useCurrentBranch) { // Select "Use current selected branch" radio option await currentBranchRadio.click(); - await page.waitForTimeout(200); + // Wait for radio to be checked + await expect(currentBranchRadio).toBeChecked(); } else {
126-132: Partial branch name matching could cause false positives.Using
labelText?.includes(options.branch)on line 129 could match unintended branches. For example, ifoptions.branchis"main", it would match labels containing"main-feature"or"maintain".Consider using a more precise match:
🔍 Suggested fix
if (currentBranchExists) { const labelText = await currentBranchLabel.textContent(); // Check if the label contains our desired branch name - if (labelText?.includes(options.branch)) { + // Use word boundary or exact match to avoid partial matches + const branchPattern = new RegExp(`\\b${options.branch}\\b`); + if (labelText && branchPattern.test(labelText)) { useCurrentBranch = true; } }apps/ui/tests/projects/regenerate-spec-with-worktree.spec.ts (1)
206-216: Silent error handling may mask test failures.Catching git command errors and only logging (lines 213-216) means the test will pass even if git operations fail. Consider failing the test or at minimum making the skip conditional on a specific known limitation.
💡 Suggested approach
// Verify git branch was created try { const { stdout: branchesOutput } = await execAsync('git branch', { cwd: projectPath }); const branches = branchesOutput.split('\n').map((b) => b.trim().replace('* ', '')); if (fs.existsSync(worktreePath)) { expect(branches).toContain(targetBranch); } } catch (error) { - // Git commands might fail if project isn't properly initialized - console.log('Git branch verification skipped:', error); + // If worktree exists but git fails, that's unexpected - fail the test + if (fs.existsSync(worktreePath)) { + throw new Error(`Git verification failed but worktree exists: ${error}`); + } + // Otherwise, project may not be initialized - acceptable to skip + console.log('Git branch verification skipped (project may not be initialized):', error); }apps/ui/src/components/views/spec-view/hooks/use-spec-generation.ts (1)
432-443: Consider adding debug logging for branch detection failures.The empty catch block at line 440 silently swallows errors. While falling back to
'main'is reasonable, logging the error would aid debugging if branch detection unexpectedly fails.📝 Suggested improvement
if (!useWorktreeBranchOnCreate) { // Detect current branch try { const branchResult = await api.worktree?.getCurrentBranch?.(currentProject.path); effectiveBranch = branchResult?.success ? branchResult.branch : 'main'; - } catch { + } catch (err) { + logger.debug('Failed to detect current branch, defaulting to main:', err); effectiveBranch = 'main'; } }apps/ui/src/components/views/board-view/hooks/use-board-features.ts (1)
225-236: Good cancellation detection pattern! Consider case-insensitive matching.The cancellation detection logic correctly avoids showing error toasts for user-initiated operations. However, the string matching is case-sensitive. Depending on error sources, you may encounter variations like "Cancelled" or "ABORTED".
🔧 Suggested improvement
// Check if this is a user cancellation (not a real error) + const errorLower = event.error?.toLowerCase() || ''; const isCancellation = - event.error && - (event.error.includes('cancelled') || - event.error.includes('canceled') || - event.error.includes('aborted')); + errorLower.includes('cancelled') || + errorLower.includes('canceled') || + errorLower.includes('aborted');apps/server/src/routes/app-spec/routes/create.ts (1)
72-82: RefactorgenerateSpecfunction signature to use an options object pattern.The
generateSpeccall passesundefinedas a positional argument forsettingsService(line 80) only to providetargetBranch. This indicates the function has too many optional parameters. Refactor the function signature to accept an options object instead, making the API clearer and avoiding undefined placeholders.Current call (lines 72-82)
generateSpec( projectPath, projectOverview, events, abortController, generateFeatures, analyzeProject, maxFeatures, undefined, targetBranch )apps/server/src/routes/app-spec/parse-and-create-features.ts (1)
58-88: Consider extracting the deletion logic into a helper function.The feature deletion logic is correct but adds significant complexity (31 lines) to this function. Consider extracting it into a separate helper function like
deleteExistingFeaturesForBranch(featuresDir, targetBranch, logger)to improve readability and testability.♻️ Proposed refactor
Extract to a new helper function:
+async function deleteExistingFeaturesForBranch( + featuresDir: string, + targetBranch: string, + logger: ReturnType<typeof createLogger> +): Promise<number> { + logger.info(`Deleting existing features for branch '${targetBranch}'...`); + try { + const existingFeatures = await secureFs.readdir(featuresDir); + let deletedCount = 0; + + for (const featureId of existingFeatures) { + const featureJsonPath = path.join(featuresDir, featureId, 'feature.json'); + try { + const featureContent = await secureFs.readFile(featureJsonPath, 'utf-8') as string; + const featureData = JSON.parse(featureContent); + + if (featureData.branchName === targetBranch) { + logger.info(`Deleting feature ${featureId} from branch ${targetBranch}`); + await secureFs.rm(path.join(featuresDir, featureId), { recursive: true }); + deletedCount++; + } + } catch (err) { + logger.debug(`Skipping ${featureId}: ${err}`); + } + } + + logger.info(`✓ Deleted ${deletedCount} existing features for branch '${targetBranch}'`); + return deletedCount; + } catch (err) { + logger.warn('Failed to delete existing features:', err); + return 0; + } +}Then simplify the main function:
- // Delete existing features for this branch before creating new ones if (targetBranch) { - logger.info(`Deleting existing features for branch '${targetBranch}'...`); - try { - const existingFeatures = await secureFs.readdir(featuresDir); - let deletedCount = 0; - - for (const featureId of existingFeatures) { - const featureJsonPath = path.join(featuresDir, featureId, 'feature.json'); - try { - const featureContent = await secureFs.readFile(featureJsonPath, 'utf-8'); - const featureData = JSON.parse(featureContent); - - // Delete if it matches the target branch - if (featureData.branchName === targetBranch) { - logger.info(`Deleting feature ${featureId} from branch ${targetBranch}`); - await secureFs.rm(path.join(featuresDir, featureId), { recursive: true }); - deletedCount++; - } - } catch (err) { - // Skip if feature.json doesn't exist or can't be read - logger.debug(`Skipping ${featureId}: ${err}`); - } - } - - logger.info(`✓ Deleted ${deletedCount} existing features for branch '${targetBranch}'`); - } catch (err) { - logger.warn('Failed to delete existing features:', err); - // Continue anyway - not critical - } + await deleteExistingFeaturesForBranch(featuresDir, targetBranch, logger); }apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (1)
81-114: Add minimal client-side validation forworktreeBranch(and avoid accidental whitespace-only values).
You already gate the action button on.trim(), which is good. I’d also validate disallowed inputs early (e.g.,.., leading-) and normalize totrim()before callingonWorktreeBranchChange.Also applies to: 235-239
apps/ui/src/lib/http-api-client.ts (1)
1311-1314: Type the newgetCurrentBranchresponse to prevent accidental contract drift.
Right now it’s inferred fromWorktreeAPI; being explicit here helps catch server/client mismatches earlier.Proposed fix
@@ worktree: WorktreeAPI = { getCurrentBranch: (projectPath: string) => - this.post('/api/worktree/current-branch', { projectPath }), + this.post<{ success: boolean; branch?: string; error?: string }>( + '/api/worktree/current-branch', + { projectPath } + ),apps/ui/src/lib/electron.ts (1)
411-418: Type updates look consistent; consider updating mock specRegeneration methods to accept/logtargetBranch.
Not required for runtime, but it helps keep dev/test behavior aligned with production flows.Also applies to: 420-436
apps/ui/tests/git/delete-worktree-with-features.spec.ts (1)
62-83: ManywaitForTimeout(...)calls—prefer deterministic waits to reduce CI flakes.
E.g., after create/delete, wait for the branch tab to appear/disappear, or for the toast to appear, instead of fixed sleeps.Also applies to: 133-220, 222-323
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/server/src/routes/app-spec/generate-features-from-spec.tsapps/server/src/routes/app-spec/generate-spec.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/routes/create.tsapps/server/src/routes/app-spec/routes/generate.tsapps/server/src/routes/worktree/index.tsapps/server/src/routes/worktree/routes/get-current-branch.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-features.tsapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/spec-view.tsxapps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/ui/src/components/views/spec-view/types.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/types/electron.d.tsapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/tests/projects/regenerate-spec-with-worktree.spec.tsapps/ui/tests/utils/views/board.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/routes/worktree/routes/get-current-branch.tsapps/server/src/routes/app-spec/generate-features-from-spec.tsapps/server/src/routes/worktree/index.tsapps/ui/tests/projects/regenerate-spec-with-worktree.spec.tsapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-features.tsapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/types/electron.d.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/routes/generate.tsapps/ui/src/components/views/spec-view/types.tsapps/server/src/routes/app-spec/routes/create.tsapps/ui/tests/utils/views/board.tsapps/server/src/routes/app-spec/generate-spec.tsapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/spec-view.tsxapps/ui/src/lib/electron.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/worktree/routes/get-current-branch.tsapps/server/src/routes/app-spec/generate-features-from-spec.tsapps/server/src/routes/worktree/index.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/routes/generate.tsapps/server/src/routes/app-spec/routes/create.tsapps/server/src/routes/app-spec/generate-spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/routes/worktree/routes/get-current-branch.tsapps/server/src/routes/app-spec/generate-features-from-spec.tsapps/server/src/routes/worktree/index.tsapps/ui/tests/projects/regenerate-spec-with-worktree.spec.tsapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/ui/src/components/views/board-view/hooks/use-board-features.tsapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/types/electron.d.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/routes/generate.tsapps/ui/src/components/views/spec-view/types.tsapps/server/src/routes/app-spec/routes/create.tsapps/ui/tests/utils/views/board.tsapps/server/src/routes/app-spec/generate-spec.tsapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/spec-view.tsxapps/ui/src/lib/electron.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/app-spec/generate-features-from-spec.tsapps/ui/tests/projects/regenerate-spec-with-worktree.spec.tsapps/ui/src/components/views/spec-view/hooks/use-spec-generation.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/generate-spec.tsapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/src/components/views/board-view.tsx
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/spec-view.tsx
🧬 Code graph analysis (9)
apps/server/src/routes/app-spec/generate-features-from-spec.ts (1)
apps/server/src/routes/app-spec/parse-and-create-features.ts (1)
parseAndCreateFeatures(14-145)
apps/ui/tests/projects/regenerate-spec-with-worktree.spec.ts (3)
apps/ui/tests/utils/git/worktree.ts (2)
createTempDirPath(52-55)cleanupTempDir(157-161)apps/ui/tests/utils/project/setup.ts (1)
setupWelcomeView(39-86)apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)
apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (2)
apps/ui/src/lib/electron.ts (1)
WorktreeInfo(191-191)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(601-606)
apps/ui/src/components/views/board-view/hooks/use-board-features.ts (1)
apps/server/src/routes/pipeline/common.ts (1)
logger(11-11)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (3)
apps/ui/src/components/views/spec-view/types.ts (1)
RegenerateSpecDialogProps(39-58)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (3)
apps/ui/src/components/views/spec-view/types.ts (1)
CreateSpecDialogProps(15-36)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/server/src/routes/app-spec/parse-and-create-features.ts (2)
apps/server/src/lib/events.ts (1)
EventEmitter(13-16)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/src/routes/app-spec/generate-spec.ts (2)
apps/server/src/routes/worktree/common.ts (1)
execAsync(13-13)apps/server/tests/integration/helpers/git-test-repo.ts (2)
worktreeExists(140-143)branchExists(132-135)
apps/ui/tests/git/delete-worktree-with-features.spec.ts (5)
apps/ui/tests/utils/git/worktree.ts (4)
createTempDirPath(52-55)waitForBoardView(494-512)cleanupTempDir(157-161)setupProjectWithPath(333-377)apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)apps/ui/tests/utils/core/waiting.ts (1)
waitForNetworkIdle(9-11)apps/ui/tests/utils/views/board.ts (2)
addFeature(190-198)getKanbanColumn(13-15)
🪛 GitHub Actions: E2E Tests
apps/server/src/routes/app-spec/parse-and-create-features.ts
[error] 69-69: TS2345: Argument of type 'string | Buffer' is not assignable to parameter of type 'string'.
🪛 GitHub Actions: PR Build Check
apps/server/src/routes/app-spec/parse-and-create-features.ts
[error] 69-69: TypeScript error TS2345: Argument of type 'string | Buffer' is not assignable to parameter of type 'string'.
🔇 Additional comments (21)
apps/ui/src/components/views/board-view/worktree-panel/components/branch-switch-dropdown.tsx (1)
53-53: LGTM! Test automation support added correctly.The data-testid attribute enables E2E tests to reliably target specific worktree dropdown triggers, supporting the new test coverage mentioned in the PR objectives.
apps/ui/src/components/views/spec-view/hooks/use-spec-generation.ts (1)
354-364: LGTM! Worktree switching on completion is well-implemented.The pattern of using
useAppStore.getState()inside the event handler to get fresh state is correct for async callbacks. The conditional switching based onevent.targetBranchandevent.worktreePathproperly integrates with the worktree workflow.apps/server/src/routes/worktree/index.ts (1)
33-45: LGTM! Route registration follows established patterns.The new
/current-branchroute is properly registered withvalidatePathParams('projectPath')middleware, consistent with similar routes like/infoand/status. The import follows the ESM convention with.jsextension.apps/ui/src/components/views/spec-view/constants.ts (1)
10-19: LGTM! Custom feature count option added correctly.The type widening from
FeatureCounttonumber | 'custom'and the new 'Custom' option align well with the PR objectives to support custom feature counts in spec generation workflows.apps/server/src/routes/app-spec/routes/generate.ts (2)
26-48: LGTM! Correct parameter extraction and logging.The
targetBranchparameter is properly extracted from the request body and logged for debugging. The optional typing matches the API contract defined in the type definitions.
72-82: LGTM! Parameter propagation is correct.The
targetBranchparameter is properly threaded through to thegenerateSpecfunction, maintaining consistency with the expanded function signature.apps/server/src/routes/app-spec/parse-and-create-features.ts (3)
14-22: LGTM! Parameter addition is correct.The
targetBranchparameter is properly added to the function signature and logged for debugging.
127-133: LGTM! Event emission includes new branch context.The event correctly includes
targetBranchandworktreePathto notify the UI about the branch-specific regeneration context. This aligns with the type definitions in electron.d.ts.
121-125: Worktree exclusion for main/master is intentional design.The exclusion of 'main' and 'master' branches from worktreePath calculation is confirmed to be intentional, validated by the test
should not create worktree for main/master branchinapps/ui/tests/projects/regenerate-spec-with-worktree.spec.ts. This pattern is consistently applied across generate-spec.ts and delete.ts.However, note that the current implementation treats both 'main' and 'master' as primary branches requiring no worktrees, regardless of which is actually configured as the default in a repository. If your project uses a different convention for the primary branch name, this code won't recognize it—features created on the actual default branch will still allocate a worktreePath. Consider making the primary branch name configurable if this is a concern.
apps/server/src/routes/app-spec/generate-features-from-spec.ts (2)
26-38: LGTM! Parameter threading is correct.The
targetBranchparameter is properly added to the function signature, logged, and will be passed downstream toparseAndCreateFeatures.
238-238: LGTM! Consistent parameter propagation.The
targetBranchis correctly passed toparseAndCreateFeatures, completing the parameter flow from the API endpoint through to feature creation.apps/ui/src/types/electron.d.ts (3)
319-320: LGTM! Event type correctly extended.The
spec_regeneration_completeevent now includes optionaltargetBranchandworktreePathfields, matching the server-side event emission in parse-and-create-features.ts.
328-351: LGTM! API signatures correctly extended.Both
createandgeneratemethods now accept an optionaltargetBranchparameter, maintaining backward compatibility while enabling the new worktree-based workflow.
639-644: LGTM! New API method for current branch detection.The
getCurrentBranchmethod provides the functionality mentioned in the PR objectives for auto-detecting the current git branch when "Use worktree branch" is unchecked.apps/ui/src/lib/electron.ts (1)
1375-1378: MockgetCurrentBranch()defaulting tomainis fine—ensure real endpoint returns the same shape.
This expects{ success: true, branch: string }.apps/ui/src/lib/http-api-client.ts (1)
1393-1424: Field naming is correct—no changes needed. Both/api/spec-regeneration/createand/api/spec-regeneration/generateendpoints accepttargetBranchas expected. The/api/worktree/current-branchendpoint returns{ success: true, branch: string }. The client code aligns correctly with the server implementation.apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (2)
81-114: Worktree branch UI implementation looks good.The checkbox pattern with conditional input provides clear UX. The explanatory text effectively communicates when the current branch is used versus when a worktree is created. The validation logic correctly requires a branch name when the checkbox is enabled.
164-197: Custom feature count button logic is well-implemented.The state management cleanly handles the transition between preset and custom values. The button selection state (lines 165-166) correctly reflects both numeric matches and the custom mode, and the click handler appropriately toggles
isCustomand applies existing custom values when available.apps/ui/src/components/views/spec-view/types.ts (2)
2-2: LGTM! Type widening appropriately supports custom feature counts.Changing
FeatureCountfrom a union of specific values (20 | 50 | 100) tonumberis the correct approach to accommodate user-defined feature counts while maintaining type safety.
26-29: Worktree branch props follow a consistent pattern.The four new properties (
useWorktreeBranch,onUseWorktreeBranchChange,worktreeBranch,onWorktreeBranchChange) are consistently defined across bothCreateSpecDialogPropsandRegenerateSpecDialogProps, maintaining symmetry in the dialog interfaces.Also applies to: 50-53
apps/ui/src/components/views/spec-view.tsx (1)
40-43: LGTM! Worktree state wiring is straightforward and consistent.The destructuring from
useSpecGenerationand prop passing to both dialog components follows a clean, symmetrical pattern. The naming convention distinguishes create vs. regenerate flows (OnCreate/OnRegeneratesuffixes) while keeping the structure identical.Also applies to: 55-58, 122-125, 162-165
| // Create worktree for target branch if specified and doesn't exist | ||
| if (targetBranch && targetBranch !== 'main' && targetBranch !== 'master') { | ||
| try { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file in question to understand the context better
cat -n apps/server/src/routes/app-spec/generate-spec.ts | sed -n '355,375p'Repository: AutoMaker-Org/automaker
Length of output: 999
🏁 Script executed:
# Search for how the codebase determines or references primary/main branches
rg -n "primary.*branch|mainBranch|develop.*branch|master.*branch" apps/server/src --type tsRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Look for branch-related constants or configurations
rg -n "const.*branch|main.*master|develop" apps/server/src --type ts -B 1 -A 1 | head -n 100Repository: AutoMaker-Org/automaker
Length of output: 7742
🏁 Script executed:
# Look for function documentation and more context around the worktree creation
cat -n apps/server/src/routes/app-spec/generate-spec.ts | sed -n '300,400p'Repository: AutoMaker-Org/automaker
Length of output: 5011
🏁 Script executed:
# Search for any configuration or documentation about primary branches
rg -n "primary" apps/server/src --type ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Check if there's documentation about the function or branch handling strategy
rg -n "develop" apps/server/src/routes/app-spec --type ts -B 1 -A 1Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Look for how baseBranch or similar concepts are handled
rg -n "baseBranch|base.*branch" apps/server/src --type ts -B 1 -A 1 | head -50Repository: AutoMaker-Org/automaker
Length of output: 2541
🏁 Script executed:
# Get function signature and documentation
cat -n apps/server/src/routes/app-spec/generate-spec.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 4632
🏁 Script executed:
# Search for where generateSpec function is called and how targetBranch is passed
rg -n "generateSpec|targetBranch" apps/server/src/routes --type ts -B 2 -A 2 | head -100Repository: AutoMaker-Org/automaker
Length of output: 8523
🏁 Script executed:
# Look for any configuration or environment variables related to default/primary branches
rg -n "default.*branch|main.*master" apps/server/src --type ts | head -50Repository: AutoMaker-Org/automaker
Length of output: 486
Add support for additional primary branches beyond just main and master.
The worktree creation logic currently skips only main and master, but doesn't account for develop or other primary branches a project might use. If a project designates develop as its primary branch, creating a worktree for it would be incorrect.
This pattern appears in multiple places (generate-spec.ts:366, parse-and-create-features.ts:123, and delete.ts:60). The worktree logic should detect or accept a configurable list of primary branches rather than hardcoding only two branch names.
🤖 Prompt for AI Agents
In @apps/server/src/routes/app-spec/generate-spec.ts around lines 365 - 367,
Replace the hardcoded check (targetBranch !== 'main' && targetBranch !==
'master') with a reusable primary-branch check using a configurable list: create
a helper (e.g., isPrimaryBranch(branch: string)) that reads PRIMARY_BRANCHES
from config/ENV (falling back to ['main','master']) and returns
PRIMARY_BRANCHES.includes(branch); then use if (targetBranch &&
!isPrimaryBranch(targetBranch)) when creating worktrees in generate-spec.ts (the
targetBranch conditional), parse-and-create-features.ts, and delete.ts so
additional primary branches like 'develop' are respected and the list can be
changed without code edits.
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
Show resolved
Hide resolved
| if (fs.existsSync(worktreePath)) { | ||
| // Worktree created successfully | ||
| expect(fs.existsSync(worktreePath)).toBe(true); | ||
|
|
||
| // Verify it's a valid git worktree | ||
| const worktreeGitDir = path.join(worktreePath, '.git'); | ||
| expect(fs.existsSync(worktreeGitDir)).toBe(true); | ||
| } |
There was a problem hiding this comment.
Redundant assertion inside conditional check.
The expect(fs.existsSync(worktreePath)).toBe(true) on line 173 is redundant since it's inside a block that only executes when fs.existsSync(worktreePath) is already true. This assertion will never fail.
Additionally, if the worktree is not created, the test silently passes without verifying the core functionality.
🔧 Suggested fix
// Verify the worktree was created
const worktreesDir = path.join(projectPath, '.worktrees');
const worktreePath = path.join(worktreesDir, targetBranch);
- if (fs.existsSync(worktreePath)) {
- // Worktree created successfully
- expect(fs.existsSync(worktreePath)).toBe(true);
-
- // Verify it's a valid git worktree
- const worktreeGitDir = path.join(worktreePath, '.git');
- expect(fs.existsSync(worktreeGitDir)).toBe(true);
- }
+ // Verify worktree was created
+ expect(fs.existsSync(worktreePath)).toBe(true);
+
+ // Verify it's a valid git worktree
+ const worktreeGitDir = path.join(worktreePath, '.git');
+ expect(fs.existsSync(worktreeGitDir)).toBe(true);📝 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.
| if (fs.existsSync(worktreePath)) { | |
| // Worktree created successfully | |
| expect(fs.existsSync(worktreePath)).toBe(true); | |
| // Verify it's a valid git worktree | |
| const worktreeGitDir = path.join(worktreePath, '.git'); | |
| expect(fs.existsSync(worktreeGitDir)).toBe(true); | |
| } | |
| // Verify worktree was created | |
| expect(fs.existsSync(worktreePath)).toBe(true); | |
| // Verify it's a valid git worktree | |
| const worktreeGitDir = path.join(worktreePath, '.git'); | |
| expect(fs.existsSync(worktreeGitDir)).toBe(true); |
| // Verify features were created with correct branchName | ||
| const featuresDir = path.join(projectPath, '.automaker', 'features'); | ||
| if (fs.existsSync(featuresDir)) { | ||
| const featureDirs = fs.readdirSync(featuresDir).filter((name) => { | ||
| const featureJsonPath = path.join(featuresDir, name, 'feature.json'); | ||
| return fs.existsSync(featureJsonPath); | ||
| }); | ||
|
|
||
| if (featureDirs.length > 0) { | ||
| // Check that features have correct branchName | ||
| for (const featureDir of featureDirs) { | ||
| const featureJsonPath = path.join(featuresDir, featureDir, 'feature.json'); | ||
| const featureJson = JSON.parse(fs.readFileSync(featureJsonPath, 'utf-8')); | ||
|
|
||
| // Feature should have branchName field | ||
| expect(featureJson).toHaveProperty('branchName'); | ||
|
|
||
| // If we successfully set the target branch, it should be in the feature | ||
| if (fs.existsSync(worktreePath)) { | ||
| expect(featureJson.branchName).toBe(targetBranch); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Feature verification wrapped in conditionals may never execute.
The feature verification block (lines 182-203) only runs if featuresDir exists. If it doesn't exist, the test passes without verifying that features were created with the correct branchName. Consider making this a hard assertion if features are expected.
🤖 Prompt for AI Agents
In @apps/ui/tests/projects/regenerate-spec-with-worktree.spec.ts around lines
180 - 203, The test currently skips verification if the features directory
doesn't exist; change it to assert its existence and that at least one feature
was found before inspecting feature.json files. Replace the conditional checks
around featuresDir and featureDirs with hard assertions (use
expect(fs.existsSync(featuresDir)).toBeTruthy() and
expect(featureDirs.length).toBeGreaterThan(0)) and then iterate featureDirs to
read each feature.json and assert featureJson has branchName and (if
worktreePath exists) that featureJson.branchName === targetBranch; keep the same
symbols: featuresDir, featureDirs, featureJsonPath, featureJson, worktreePath,
and targetBranch to locate the code to modify.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/routes/app-spec/parse-and-create-features.ts (2)
92-117: Critical: validate/sanitizefeature.idbefore using it in filesystem paths (path traversal).
feature.idoriginates from parsed agent output and is used to create directories/files (path.join(featuresDir, feature.id)). Without validation, a malicious/buggy spec can escapefeaturesDir(e.g.,../../...) or create unexpected nested paths. This is a must-fix before relying on branch cleanup/worktree isolation guarantees. (Based on learnings, features run in isolated worktrees, so keeping feature file I/O constrained is especially important.)Proposed diff (minimal containment + basic id rules)
+ const featuresDirResolved = path.resolve(featuresDir); for (const feature of parsed.features) { logger.debug('Creating feature:', feature.id); - const featureDir = path.join(featuresDir, feature.id); + if (!/^[a-zA-Z0-9][a-zA-Z0-9_-]{0,127}$/.test(feature.id)) { + throw new Error(`Invalid feature id '${feature.id}' (refusing to write files)`); + } + const featureDir = path.resolve(featuresDir, feature.id); + if (!featureDir.startsWith(featuresDirResolved + path.sep)) { + throw new Error(`Invalid feature path for id '${feature.id}'`); + } await secureFs.mkdir(featureDir, { recursive: true });
106-133:branchNamedefaults to'main'whentargetBranchis omitted;worktreePathconstruction needs sanitization to match worktree creation logic.
- Line 106:
branchName: targetBranch || 'main'defaults to 'main' whentargetBranchis undefined. ThePOST /generate-featuresendpoint never passestargetBranch(line 56 in generate-features.ts), so all features generated via that route are mislabeled as 'main' branch. OnlyPOST /generate-specpasses it (line 442 in generate-spec.ts).- Line 124:
worktreePath = path.join(projectPath, '.worktrees', targetBranch)constructs paths without sanitizing branch names. However,apps/server/src/routes/worktree/routes/create.ts(line 139) sanitizes using.replace(/[^a-zA-Z0-9_-]/g, '-')before constructing the same path. Branch names with/,:, or other special characters will create path mismatches. AsanitizeBranchName()utility exists inapps/server/src/lib/worktree-metadata.tsbut is not exported for reuse.
🤖 Fix all issues with AI agents
In @apps/server/src/routes/app-spec/parse-and-create-features.ts:
- Around line 14-25: The parseAndCreateFeatures function logs the entire content
at info level which can be large or sensitive; change logging so info-level
retains only metadata (keep logger.info('Content length: ...') and
logger.info('targetBranch: ...') using the targetBranch variable) and move the
full payload to logger.debug or log a truncated preview (e.g., first N chars
with an ellipsis) instead of logger.info(content); update references in
parseAndCreateFeatures to use logger.debug for the full content and ensure any
truncated preview is clearly labeled to bound the payload size.
- Around line 58-88: When deleting existing features in the branch cleanup loop,
make the removal call to secureFs.rm(path.join(featuresDir, featureId), {
recursive: true }) more race-tolerant by adding force: true (i.e.
secureFs.rm(..., { recursive: true, force: true })) and tighten the catch
logging by replacing logger.debug(`Skipping ${featureId}: ${err}`) with
something that stringifies errors reliably (e.g. logger.debug(`Skipping
${featureId}: ${err instanceof Error ? err.message : String(err)}`)); keep the
rest of the loop and the outer try/catch behavior unchanged.
In @apps/server/src/routes/worktree/routes/get-current-branch.ts:
- Around line 1-2: The top-of-file doc comment incorrectly says "GET
/current-branch" while the route is registered as a POST; update the comment in
get-current-branch.ts to "POST /current-branch" (or otherwise reflect POST) so
the documentation matches the actual route registration and intent for the
handler exported in this file.
🧹 Nitpick comments (1)
apps/server/src/routes/worktree/routes/get-current-branch.ts (1)
28-57: Add timeout and consider distinguishing fallback responses.Two concerns:
No timeout:
execAsynccould hang indefinitely on unresponsive file systems. Consider adding a timeout:await execAsync('git branch --show-current', { cwd: projectPath, timeout: 5000 });Silent fallback masks errors: When git fails (lines 53-57), returning
{ success: true, branch: 'main' }is indistinguishable from the branch actually beingmain. If the path isn't a git repository, the caller receives misleading data.Consider adding a
fallback: truefield or logging a warning so callers/operators can identify when detection failed.♻️ Suggested improvement
} catch (error) { logger.error('Failed to get current branch:', error); - // Return main as fallback - res.json({ success: true, branch: 'main' }); + // Return main as fallback with indication + res.json({ success: true, branch: 'main', fallback: true }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/worktree/routes/get-current-branch.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/worktree/routes/get-current-branch.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/worktree/routes/get-current-branch.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/worktree/routes/get-current-branch.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/app-spec/parse-and-create-features.ts
🧬 Code graph analysis (1)
apps/server/src/routes/app-spec/parse-and-create-features.ts (2)
apps/server/src/lib/events.ts (1)
EventEmitter(13-16)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
⏰ 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 (2)
apps/server/src/routes/worktree/routes/get-current-branch.ts (2)
13-14: Handler factory pattern looks good.The
createGetCurrentBranchHandlerfactory pattern is consistent with the codebase conventions, and the use of@automaker/utilsfor logging follows the coding guidelines.
16-26: Path validation already exists at the middleware level.The
/current-branchroute applies thevalidatePathParams('projectPath')middleware before the handler executes (line 43 ofindex.ts). This middleware callsvalidatePath()from@automaker/platform, which enforces theALLOWED_ROOT_DIRECTORYsecurity boundary. Any attempt to access paths outside the allowed directory (e.g.,/etc,/root) will be rejected with a 403 error before the handler is reached. The validation uses path traversal detection that checks for relative paths starting with.., preventing the enumeration scenario described in the original comment.Likely an incorrect or invalid review comment.
| export async function parseAndCreateFeatures( | ||
| projectPath: string, | ||
| content: string, | ||
| events: EventEmitter | ||
| events: EventEmitter, | ||
| targetBranch?: string | ||
| ): Promise<void> { | ||
| logger.info('========== parseAndCreateFeatures() started =========='); | ||
| logger.info(`Content length: ${content.length} chars`); | ||
| logger.info('targetBranch:', targetBranch); | ||
| logger.info('========== CONTENT RECEIVED FOR PARSING =========='); | ||
| logger.info(content); | ||
| logger.info('========== END CONTENT =========='); |
There was a problem hiding this comment.
Avoid logging raw content at info-level; include targetBranch but keep payload logs bounded.
Adding targetBranch logging is helpful, but this function currently logs the full content at info, which can be huge and may include sensitive data. Consider moving the full payload to debug (or truncating) and keeping info to metadata (length, branch, counts).
🤖 Prompt for AI Agents
In @apps/server/src/routes/app-spec/parse-and-create-features.ts around lines 14
- 25, The parseAndCreateFeatures function logs the entire content at info level
which can be large or sensitive; change logging so info-level retains only
metadata (keep logger.info('Content length: ...') and logger.info('targetBranch:
...') using the targetBranch variable) and move the full payload to logger.debug
or log a truncated preview (e.g., first N chars with an ellipsis) instead of
logger.info(content); update references in parseAndCreateFeatures to use
logger.debug for the full content and ensure any truncated preview is clearly
labeled to bound the payload size.
| // Delete existing features for this branch before creating new ones | ||
| if (targetBranch) { | ||
| logger.info(`Deleting existing features for branch '${targetBranch}'...`); | ||
| try { | ||
| const existingFeatures = await secureFs.readdir(featuresDir); | ||
| let deletedCount = 0; | ||
|
|
||
| for (const featureId of existingFeatures) { | ||
| const featureJsonPath = path.join(featuresDir, featureId, 'feature.json'); | ||
| try { | ||
| const featureContent = await secureFs.readFile(featureJsonPath, 'utf-8'); | ||
| const featureData = JSON.parse(featureContent.toString()); | ||
|
|
||
| // Delete if it matches the target branch | ||
| if (featureData.branchName === targetBranch) { | ||
| logger.info(`Deleting feature ${featureId} from branch ${targetBranch}`); | ||
| await secureFs.rm(path.join(featuresDir, featureId), { recursive: true }); | ||
| deletedCount++; | ||
| } | ||
| } catch (err) { | ||
| // Skip if feature.json doesn't exist or can't be read | ||
| logger.debug(`Skipping ${featureId}: ${err}`); | ||
| } | ||
| } | ||
|
|
||
| logger.info(`✓ Deleted ${deletedCount} existing features for branch '${targetBranch}'`); | ||
| } catch (err) { | ||
| logger.warn('Failed to delete existing features:', err); | ||
| // Continue anyway - not critical | ||
| } | ||
| } |
There was a problem hiding this comment.
Branch cleanup: make deletion more race-tolerant and improve error logging.
-
secureFs.rm(..., { recursive: true })can still throw on already-deleted paths;force: truemakes this less flaky under concurrent operations. -
logger.debug(\Skipping ${featureId}: ${err}`)will often log[object Object]; prefererr instanceof Error ? err.message : String(err)`.
Proposed diff
- await secureFs.rm(path.join(featuresDir, featureId), { recursive: true });
+ await secureFs.rm(path.join(featuresDir, featureId), {
+ recursive: true,
+ force: true,
+ });
deletedCount++;
}
} catch (err) {
// Skip if feature.json doesn't exist or can't be read
- logger.debug(`Skipping ${featureId}: ${err}`);
+ logger.debug(
+ `Skipping ${featureId}: ${err instanceof Error ? err.message : String(err)}`
+ );
}🤖 Prompt for AI Agents
In @apps/server/src/routes/app-spec/parse-and-create-features.ts around lines 58
- 88, When deleting existing features in the branch cleanup loop, make the
removal call to secureFs.rm(path.join(featuresDir, featureId), { recursive: true
}) more race-tolerant by adding force: true (i.e. secureFs.rm(..., { recursive:
true, force: true })) and tighten the catch logging by replacing
logger.debug(`Skipping ${featureId}: ${err}`) with something that stringifies
errors reliably (e.g. logger.debug(`Skipping ${featureId}: ${err instanceof
Error ? err.message : String(err)}`)); keep the rest of the loop and the outer
try/catch behavior unchanged.
| /** | ||
| * GET /current-branch endpoint - Get the current branch of a project |
There was a problem hiding this comment.
Documentation mismatch: comment says GET but route is POST.
According to the route registration, this endpoint is a POST route. Update the comment to match.
📝 Suggested fix
/**
- * GET /current-branch endpoint - Get the current branch of a project
+ * POST /current-branch endpoint - Get the current branch of a project
*/📝 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.
| /** | |
| * GET /current-branch endpoint - Get the current branch of a project | |
| /** | |
| * POST /current-branch endpoint - Get the current branch of a project |
🤖 Prompt for AI Agents
In @apps/server/src/routes/worktree/routes/get-current-branch.ts around lines 1
- 2, The top-of-file doc comment incorrectly says "GET /current-branch" while
the route is registered as a POST; update the comment in get-current-branch.ts
to "POST /current-branch" (or otherwise reflect POST) so the documentation
matches the actual route registration and intent for the handler exported in
this file.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/ui/src/components/views/board-view.tsx:
- Around line 1437-1463: In the onDeleted handler, reverse the delete order so
disk deletion runs before removing from state: call and await
handleDeleteFeature(feature.id) for each featureToHandle first, then call
removeFeature(feature.id) to avoid handleDeleteFeature returning early (it uses
features.find). Also replace the hardcoded main branch lookup worktrees.find((w)
=> w.isMain)?.branch || 'main' with
getPrimaryWorktreeBranch(currentProject.path) to obtain the repository’s primary
branch; use that value when building updates for reassigning features, keeping
updateFeature and persistFeatureUpdate calls as before, and preserve the
existing setWorktreeRefreshKey and setSelectedWorktreeForAction calls.
In @apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx:
- Around line 30-34: The onDeleted callback is declared/used synchronously but
callers (e.g., BoardView) pass an async handler, so change the onDeleted type to
return Promise<void>, await the call where it is invoked in DeleteWorktreeDialog
(both occurrences around the current close flow), and wrap the await in
try/catch to surface errors (log/show an error and avoid closing the dialog
prematurely if deletion failed); reference the onDeleted prop and the dialog
close flow to locate both spots to update.
🧹 Nitpick comments (3)
libs/git-utils/src/branch-validation.ts (1)
15-70: Comprehensive branch name validation with minor documentation inconsistency.The validation logic effectively prevents command injection and enforces git ref constraints. However, the comment on line 63 states "Cannot contain: space, ~, ^, :, , ?, *, [" but the regex on line 64 also checks for
]via\]. Update the comment to include]for accuracy.📝 Proposed fix for comment
// Additional git ref restrictions - // Cannot contain: space, ~, ^, :, \, ?, *, [ + // Cannot contain: space, ~, ^, :, \, ?, *, [, ] const invalidChars = /[\s~^:\\?*[\]]/;apps/server/src/routes/app-spec/generate-spec.ts (1)
366-439: Consider refactoring worktree creation to use async fs operations and extract to utility.The worktree creation logic is well-implemented with proper validation and error handling. However:
Synchronous fs operations: Lines 380, 395-399 use
fs.existsSyncandfs.mkdirSync. In async functions, prefer async variants for consistency and to avoid blocking the event loop.Complex inline logic: The 70+ line worktree creation block could be extracted to a dedicated utility function (e.g.,
ensureWorktreeForBranch) in@automaker/git-utilsfor reusability and testability.♻️ Suggested refactor for async fs operations
// Check if worktree already exists const worktreePath = path.join(projectPath, '.worktrees', targetBranch); - const worktreeExists = fs.existsSync(worktreePath); + const worktreeExists = await secureFs.access(worktreePath).then(() => true).catch(() => false); if (worktreeExists) { logger.info(`Worktree for branch '${targetBranch}' already exists at ${worktreePath}`); } else { // ... branch existence check ... // Create .worktrees directory if it doesn't exist const worktreesDir = path.join(projectPath, '.worktrees'); - if (!fs.existsSync(worktreesDir)) { - fs.mkdirSync(worktreesDir, { recursive: true }); - } + await secureFs.mkdir(worktreesDir, { recursive: true });apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (1)
109-119: Add an explicit irreversible-delete warning when “Delete backlog” is checked.Right now the “moved to main worktree” warning disappears (good), but there’s no replacement warning making it clear those features will be permanently deleted. Consider adding a short destructive callout when
deleteFeatures === true.Also applies to: 147-160
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/server/src/routes/app-spec/generate-spec.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxlibs/git-utils/src/branch-validation.tslibs/git-utils/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
libs/git-utils/src/index.tsapps/server/src/routes/app-spec/parse-and-create-features.tslibs/git-utils/src/branch-validation.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/server/src/routes/app-spec/generate-spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
libs/git-utils/src/index.tsapps/server/src/routes/app-spec/parse-and-create-features.tslibs/git-utils/src/branch-validation.tsapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsxapps/server/src/routes/app-spec/generate-spec.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/generate-spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/app-spec/parse-and-create-features.tsapps/ui/src/components/views/board-view.tsxapps/server/src/routes/app-spec/generate-spec.ts
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx
🧬 Code graph analysis (3)
libs/git-utils/src/branch-validation.ts (1)
libs/git-utils/src/index.ts (2)
isSafeBranchName(22-22)validateBranchName(22-22)
apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (2)
apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(601-606)apps/ui/src/lib/electron.ts (1)
WorktreeInfo(191-191)
apps/server/src/routes/app-spec/generate-spec.ts (2)
libs/git-utils/src/branch-validation.ts (1)
validateBranchName(78-85)libs/git-utils/src/index.ts (1)
validateBranchName(22-22)
⏰ 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 (11)
libs/git-utils/src/index.ts (1)
20-22: LGTM!Clean addition of branch validation utilities to the public API. The exports follow the established pattern in the module.
apps/server/src/routes/app-spec/parse-and-create-features.ts (4)
17-22: LGTM!The optional
targetBranchparameter enables branch-specific feature regeneration. Logging the parameter aids debugging.
58-88: Pre-regeneration cleanup prevents duplicate features.The deletion logic correctly removes existing features for the target branch before creating new ones. Error handling is appropriate—failures are logged but don't interrupt the regeneration flow.
106-106: LGTM!Setting
branchNamebased ontargetBranchensures features are correctly associated with their branch.
121-133: LGTM!The worktreePath calculation and event payload extension enable worktree-aware feature tracking in the UI. The conditional logic correctly excludes main/master branches from worktree path generation.
libs/git-utils/src/branch-validation.ts (1)
78-85: LGTM!The
validateBranchNamewrapper provides clear error messaging when branch names fail validation.apps/server/src/routes/app-spec/generate-spec.ts (3)
29-29: LGTM!Import follows coding guidelines by using the shared
@automaker/git-utilspackage.
42-51: LGTM!The optional
targetBranchparameter extends spec generation to support worktree-based workflows. Logging aids debugging.
452-454: LGTM!Correctly propagates
targetBranchto feature generation, maintaining consistency across the spec regeneration workflow.apps/ui/src/components/views/board-view.tsx (1)
70-96:removeFeatureaddition looks reasonable, but verify it’s safe to call before deletions.If
useBoardActions().handleDeleteFeature(id)(or downstream persistence) does a state lookup by id, pre-removing viaremoveFeature(id)could prevent the actual disk delete from happening. Please double-checkhandleDeleteFeatureis idempotent / doesn’t require the feature to still exist in local state.apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx (1)
51-57: Good safety net: reset destructive checkboxes on close.
Prevents accidental carry-over between dialog openings.
| onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => { | ||
| // Handle features assigned to the deleted worktree (by branch) | ||
| const featuresToHandle = hookFeatures.filter( | ||
| (f) => f.branchName === deletedWorktree.branch | ||
| ); | ||
|
|
||
| if (deleteFeatures) { | ||
| // Remove all features from state first to prevent update race conditions | ||
| featuresToHandle.forEach((feature) => removeFeature(feature.id)); | ||
|
|
||
| // Delete all features from disk in parallel | ||
| await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id))); | ||
| } else { | ||
| // Reassign all features to main branch | ||
| const mainBranch = worktrees.find((w) => w.isMain)?.branch || 'main'; | ||
| const updates = { branchName: mainBranch }; | ||
|
|
||
| featuresToHandle.forEach((feature) => { | ||
| updateFeature(feature.id, updates); | ||
| persistFeatureUpdate(feature.id, updates); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| // Refresh worktree list after all operations complete | ||
| setWorktreeRefreshKey((k) => k + 1); | ||
| setSelectedWorktreeForAction(null); | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the board-view.tsx file and understand the context around handleDeleteFeature
find . -type f -name "board-view.tsx" | head -5Repository: AutoMaker-Org/automaker
Length of output: 112
🏁 Script executed:
# Search for getPrimaryWorktreeBranch function to see if it exists
rg "getPrimaryWorktreeBranch" --type ts --type tsxRepository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Search for handleDeleteFeature to understand its implementation
rg "handleDeleteFeature" --type ts --type tsx -B 5 -A 10Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Check for primary branch resolution patterns in the codebase
rg "isMain" --type ts --type tsx -A 2 -B 2Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Look for currentProject usage in board-view.tsx
rg "currentProject" apps/ui/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 3827
🏁 Script executed:
# Search for getPrimaryWorktreeBranch and handleDeleteFeature without type restriction
rg "getPrimaryWorktreeBranch|handleDeleteFeature" -B 3 -A 8Repository: AutoMaker-Org/automaker
Length of output: 15601
🏁 Script executed:
# Read the board-view.tsx file to see lines around 1437-1463
wc -l apps/ui/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 115
🏁 Script executed:
# Get the full handleDeleteFeature implementation
rg "const handleDeleteFeature = useCallback" -A 30 apps/ui/src/components/views/board-view/hooks/use-board-actions.tsRepository: AutoMaker-Org/automaker
Length of output: 1222
🏁 Script executed:
# Read exact lines 1437-1463 from board-view.tsx
sed -n '1437,1463p' apps/ui/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 1269
🏁 Script executed:
# Check if there are other worktree deletion callbacks to see patterns
rg "onDeleted\|worktree.*delete" -B 2 -A 15 apps/ui/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 49
Worktree delete: fix removal order and use getPrimaryWorktreeBranch() instead of hardcoding 'main'.
Two issues:
-
handleDeleteFeatureneeds the feature in state: CurrentlyremoveFeature()is called beforehandleDeleteFeature(). SincehandleDeleteFeaturedoesfeatures.find((f) => f.id === featureId)and returns early if not found, the disk deletion silently fails. Reverse the order: delete from disk first, then remove from state. -
Hardcoded primary branch: Replace
worktrees.find((w) => w.isMain)?.branch || 'main'withgetPrimaryWorktreeBranch(currentProject.path), which is already imported and handles repos where the primary branch is notmain(e.g.,master,develop).
Proposed adjustment
onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => {
// Handle features assigned to the deleted worktree (by branch)
const featuresToHandle = hookFeatures.filter(
(f) => f.branchName === deletedWorktree.branch
);
if (deleteFeatures) {
- // Remove all features from state first to prevent update race conditions
- featuresToHandle.forEach((feature) => removeFeature(feature.id));
-
- // Delete all features from disk in parallel
- await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id)));
+ // Delete from disk first (handleDeleteFeature requires feature in state),
+ // then remove from local state to prevent follow-up updates/races.
+ await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id)));
+ featuresToHandle.forEach((feature) => removeFeature(feature.id));
} else {
// Reassign all features to main branch
- const mainBranch = worktrees.find((w) => w.isMain)?.branch || 'main';
+ const mainBranch = getPrimaryWorktreeBranch(currentProject.path) || 'main';
const updates = { branchName: mainBranch };
featuresToHandle.forEach((feature) => {
updateFeature(feature.id, updates);
persistFeatureUpdate(feature.id, updates);
});
}
// Refresh worktree list after all operations complete
setWorktreeRefreshKey((k) => k + 1);
setSelectedWorktreeForAction(null);
}}📝 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.
| onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => { | |
| // Handle features assigned to the deleted worktree (by branch) | |
| const featuresToHandle = hookFeatures.filter( | |
| (f) => f.branchName === deletedWorktree.branch | |
| ); | |
| if (deleteFeatures) { | |
| // Remove all features from state first to prevent update race conditions | |
| featuresToHandle.forEach((feature) => removeFeature(feature.id)); | |
| // Delete all features from disk in parallel | |
| await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id))); | |
| } else { | |
| // Reassign all features to main branch | |
| const mainBranch = worktrees.find((w) => w.isMain)?.branch || 'main'; | |
| const updates = { branchName: mainBranch }; | |
| featuresToHandle.forEach((feature) => { | |
| updateFeature(feature.id, updates); | |
| persistFeatureUpdate(feature.id, updates); | |
| } | |
| }); | |
| }); | |
| } | |
| // Refresh worktree list after all operations complete | |
| setWorktreeRefreshKey((k) => k + 1); | |
| setSelectedWorktreeForAction(null); | |
| }} | |
| onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => { | |
| // Handle features assigned to the deleted worktree (by branch) | |
| const featuresToHandle = hookFeatures.filter( | |
| (f) => f.branchName === deletedWorktree.branch | |
| ); | |
| if (deleteFeatures) { | |
| // Delete from disk first (handleDeleteFeature requires feature in state), | |
| // then remove from local state to prevent follow-up updates/races. | |
| await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id))); | |
| featuresToHandle.forEach((feature) => removeFeature(feature.id)); | |
| } else { | |
| // Reassign all features to main branch | |
| const mainBranch = getPrimaryWorktreeBranch(currentProject.path) || 'main'; | |
| const updates = { branchName: mainBranch }; | |
| featuresToHandle.forEach((feature) => { | |
| updateFeature(feature.id, updates); | |
| persistFeatureUpdate(feature.id, updates); | |
| }); | |
| } | |
| // Refresh worktree list after all operations complete | |
| setWorktreeRefreshKey((k) => k + 1); | |
| setSelectedWorktreeForAction(null); | |
| }} |
🤖 Prompt for AI Agents
In @apps/ui/src/components/views/board-view.tsx around lines 1437 - 1463, In the
onDeleted handler, reverse the delete order so disk deletion runs before
removing from state: call and await handleDeleteFeature(feature.id) for each
featureToHandle first, then call removeFeature(feature.id) to avoid
handleDeleteFeature returning early (it uses features.find). Also replace the
hardcoded main branch lookup worktrees.find((w) => w.isMain)?.branch || 'main'
with getPrimaryWorktreeBranch(currentProject.path) to obtain the repository’s
primary branch; use that value when building updates for reassigning features,
keeping updateFeature and persistFeatureUpdate calls as before, and preserve the
existing setWorktreeRefreshKey and setSelectedWorktreeForAction calls.
| onDeleted: ( | ||
| deletedWorktree: WorktreeInfo, | ||
| deletedBranch: boolean, | ||
| deleteFeatures: boolean | ||
| ) => void; |
There was a problem hiding this comment.
onDeleted should be async/awaited to avoid unhandled promise rejections.
BoardView passes an async handler; right now you call it without awaiting, and you immediately close the dialog. If the handler throws/rejects (e.g., feature deletion fails), this can become an unhandled rejection and skip follow-up UI consistency steps.
Proposed fix (type + await + error handling)
interface DeleteWorktreeDialogProps {
open: boolean;
onOpenChange: (open: boolean) => void;
projectPath: string;
worktree: WorktreeInfo | null;
onDeleted: (
deletedWorktree: WorktreeInfo,
deletedBranch: boolean,
deleteFeatures: boolean
- ) => void;
+ ) => Promise<void>;
/** Number of features assigned to this worktree's branch */
affectedFeatureCount?: number;
}
// ...
if (result.success) {
toast.success(`Worktree deleted`, {
description: deleteBranch
? `Branch "${worktree.branch}" was also deleted`
: `Branch "${worktree.branch}" was kept`,
});
- onDeleted(worktree, deleteBranch, deleteFeatures);
- onOpenChange(false); // useEffect will reset checkbox state
+ try {
+ await onDeleted(worktree, deleteBranch, deleteFeatures);
+ onOpenChange(false); // useEffect will reset checkbox state
+ } catch (err) {
+ toast.error('Worktree deleted, but follow-up cleanup failed', {
+ description: err instanceof Error ? err.message : 'Unknown error',
+ });
+ }
} else {Also applies to: 77-79
🤖 Prompt for AI Agents
In @apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx
around lines 30 - 34, The onDeleted callback is declared/used synchronously but
callers (e.g., BoardView) pass an async handler, so change the onDeleted type to
return Promise<void>, await the call where it is invoked in DeleteWorktreeDialog
(both occurrences around the current close flow), and wrap the await in
try/catch to surface errors (log/show an error and avoid closing the dialog
prematurely if deletion failed); reference the onDeleted prop and the dialog
close flow to locate both spots to update.
|
there is a lot of code rabbit reviews you need to address, but thanks for this initial pr |
|
I have manually test it and fix all coderabbit address review |
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 (1)
apps/ui/tests/utils/views/spec-editor.ts (1)
31-33: RemoveclickSaveSpecas it duplicatesclickSaveButtonand is unused.Both functions target the same element (
save-spec) butclickSaveButtonadds wait-for-confirmation logic. SinceclickSaveSpecis not used anywhere in the codebase and appears to be superseded byclickSaveButton, remove it to avoid confusion about which function to use in tests.
🤖 Fix all issues with AI agents
In @apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx:
- Around line 2-3: The dialog currently stores isCustom/customFeatureCount
independently which can desync from the incoming prop featureCount across opens;
update CreateSpecDialog (and any related handlers) to derive initial isCustom
and customFeatureCount from featureCount when the dialog opens or when
featureCount changes (e.g., compute isCustom =
!presetValues.includes(featureCount) and set customFeatureCount = featureCount),
and reset those derived state values in the dialog open/reset logic so the UI
always reflects the current featureCount instead of stale state.
In @apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx:
- Around line 2-3: The dialog's local states isCustom and customFeatureCount
must be derived/reset from the incoming featureCount so the UI reflects preset
vs custom values; in the RegenerateSpecDialog component, initialize
isCustom/customFeatureCount based on whether featureCount is one of the preset
options and add a useEffect that runs when featureCount (or the dialog open
prop) changes to set isCustom = featureCount not in presets and
customFeatureCount = featureCount when custom, otherwise clear/reset them;
ensure submission uses customFeatureCount when isCustom is true and that the
custom input is shown whenever isCustom becomes true.
🧹 Nitpick comments (6)
apps/ui/tests/git/delete-worktree-with-features.spec.ts (4)
29-60: UsePagetype instead ofanyfor better type safety.The helper function uses
anyfor thepageparameter, losing type checking benefits. Import and use thePagetype from Playwright.♻️ Suggested fix
+import { test, expect, Page } from '@playwright/test'; -import { test, expect } from '@playwright/test'; -async function enableWorktrees(page: any) { +async function enableWorktrees(page: Page) {
62-82: Same typing issue: usePagetype.Apply the same fix to use
Pageinstead ofany. Additionally, the 2-second hardcoded wait on line 81 could potentially be replaced with a more reliable wait condition if there's a UI element that indicates completion.♻️ Suggested fix
-async function createWorktree(page: any, branchName: string) { +async function createWorktree(page: Page, branchName: string) {
169-172: Fragile selector pattern for actions dropdown.Using
.locator('..')(parent traversal) combined with.last()creates a brittle selector that will break if the UI structure changes. This pattern is repeated across all test cases (lines 258-259, 354-355, 402-403, 461-462).Consider requesting a
data-testidbe added to the worktree actions dropdown button for more reliable selection.♻️ Suggested improvement
If a
data-testidis added to the UI:- const worktreeContainer = featureBranchTab.locator('..'); - const actionsDropdown = worktreeContainer.locator('button').last(); + const actionsDropdown = page.locator(`[data-testid="worktree-actions-${branchName}"]`);Alternatively, a more specific selector using aria attributes:
- const worktreeContainer = featureBranchTab.locator('..'); - const actionsDropdown = worktreeContainer.locator('button').last(); + const actionsDropdown = featureBranchTab.locator('xpath=..').locator('button[aria-haspopup="menu"]');
153-155: Consider replacing hardcoded timeouts with explicit wait conditions.There are numerous
waitForTimeoutcalls throughout the tests (e.g., lines 153, 155, 172, 195, 201, 274, 287, 293, etc.). While sometimes necessary, fixed timeouts can lead to flaky tests—either failing on slower CI environments or wasting time on faster ones.Where possible, prefer waiting for specific conditions:
await expect(element).toBeVisible()await page.waitForResponse(predicate)await page.waitForSelector('[data-testid="..."]')apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (1)
81-114: Worktree branch input: consider normalizing the branch name before storing/submitting.
Users can paste leading/trailing whitespace; you alreadytrim()for button disable, but the stored value may keep whitespace and surprise downstream validation.Proposed tweak (trim on change)
- onChange={(e) => onWorktreeBranchChange(e.target.value)} + onChange={(e) => onWorktreeBranchChange(e.target.value.trim())}Also applies to: 235-239
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (1)
81-114: Worktree branch input: consider trimming on change to avoid whitespace-only branch names.
You already guard withtrim()for button disabling; trimming at the source avoids passing whitespace downstream.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/tests/utils/views/spec-editor.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/board-view.tsxapps/ui/tests/utils/views/spec-editor.tsapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/board-view.tsxapps/ui/tests/utils/views/spec-editor.tsapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/ui/src/components/views/board-view.tsxapps/ui/tests/git/delete-worktree-with-features.spec.ts
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx
🧬 Code graph analysis (3)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (3)
apps/ui/src/components/views/spec-view/types.ts (1)
RegenerateSpecDialogProps(39-58)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/tests/git/delete-worktree-with-features.spec.ts (4)
apps/ui/tests/utils/git/worktree.ts (2)
createTempDirPath(52-55)setupProjectWithPath(333-377)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)apps/ui/tests/utils/core/waiting.ts (1)
waitForNetworkIdle(9-11)apps/ui/tests/utils/views/board.ts (2)
addFeature(190-198)getKanbanColumn(13-15)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (3)
apps/ui/src/components/views/spec-view/types.ts (1)
CreateSpecDialogProps(15-36)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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 (7)
apps/ui/tests/git/delete-worktree-with-features.spec.ts (5)
9-24: LGTM!Imports are well-organized and appropriately leverage the shared test utilities from the
../utilsbarrel export. The use ofpromisifyforexecis a good practice for async/await patterns.
84-103: LGTM!The helper function correctly reads the features directory and filters by branch name. The synchronous file operations are acceptable for test assertion purposes.
110-131: LGTM!Good test isolation pattern with proper setup and teardown. Each test gets a fresh git repository, and cleanup is properly ordered (afterEach for individual repos, afterAll for the temp directory).
309-319: LGTM!Good verification that deleted features are actually removed from disk, not just moved. The loop checking that no remaining feature contains the "deleted" text in its description ensures complete cleanup.
376-433: LGTM!Excellent coverage for the git branch deletion behavior. Using
execAsync('git branch', ...)to verify the actual git state is a robust approach that validates the integration between the UI action and the underlying git operations.Also applies to: 435-497
apps/ui/tests/utils/views/spec-editor.ts (1)
84-85: LGTM! Cross-platform keyboard shortcut improvement.The change to
ControlOrMeta+aproperly handles select-all across different operating systems (Control on Windows/Linux, Command on macOS).apps/ui/src/components/views/board-view.tsx (1)
86-86: LGTM! Store method properly exposed.The
removeFeaturemethod is correctly added to support the enhanced worktree deletion flow.
| onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => { | ||
| // Handle features assigned to the deleted worktree (by branch) | ||
| const featuresToHandle = hookFeatures.filter( | ||
| (f) => f.branchName === deletedWorktree.branch | ||
| ); | ||
|
|
||
| // Compute mainBranch once before processing | ||
| const mainBranch = worktrees.find((w) => w.isMain)?.branch || 'main'; | ||
|
|
||
| if (deleteFeatures) { | ||
| // Delete all features from disk in parallel | ||
| // handleDeleteFeature already calls removeFeature, so no need to call it separately | ||
| await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id))); | ||
| } else { | ||
| // Reassign all features to main branch | ||
| const updates = { branchName: mainBranch }; | ||
|
|
||
| // Use Promise.all to await all updates in parallel | ||
| await Promise.all( | ||
| featuresToHandle.map((feature) => { | ||
| updateFeature(feature.id, updates); | ||
| return persistFeatureUpdate(feature.id, updates); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for handleDeleteFeature definition
rg -nA20 'const handleDeleteFeature|function handleDeleteFeature' --type=ts --type=tsx | head -100Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Search for removeFeature usage and definition
rg -nB2 -A10 'removeFeature' --type=ts --type=tsx | head -150Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Check the imports and context around line 1437-1461 in board-view.tsx
rg -nB5 -A5 'onDeleted.*async.*deletedWorktree' apps/ui/src/components/views/board-view.tsx | head -50Repository: AutoMaker-Org/automaker
Length of output: 596
🏁 Script executed:
# Search for handleDeleteFeature definition without type filter
rg -nA20 'const handleDeleteFeature|function handleDeleteFeature' | head -150Repository: AutoMaker-Org/automaker
Length of output: 2269
🏁 Script executed:
# Search for removeFeature without type filter
rg -nB2 -A10 'removeFeature' | head -200Repository: AutoMaker-Org/automaker
Length of output: 6854
🏁 Script executed:
# Check imports at the top of board-view.tsx to find where handleDeleteFeature comes from
head -100 apps/ui/src/components/views/board-view.tsxRepository: AutoMaker-Org/automaker
Length of output: 3765
🏁 Script executed:
sed -n '284,326p' apps/ui/src/components/views/board-view/hooks/use-board-actions.tsRepository: AutoMaker-Org/automaker
Length of output: 1525
🏁 Script executed:
# Also search for persistFeatureDelete to understand what it does
rg -nB2 -A10 'persistFeatureDelete' | head -100Repository: AutoMaker-Org/automaker
Length of output: 8251
🏁 Script executed:
# Get the complete persistFeatureDelete implementation
sed -n '63,82p' apps/ui/src/components/views/board-view/hooks/use-board-persistence.tsRepository: AutoMaker-Org/automaker
Length of output: 555
Add error handling and user feedback for feature deletion in worktree deletion.
The deletion handler has two legitimate issues:
-
Missing error handling and user feedback: The
onDeletedcallback has no try/catch around thePromise.alloperations. WhilepersistFeatureDeletecatches errors silently (logging but not re-throwing), users receive no feedback if deletions or updates fail. Add a try/catch block to handle failures gracefully and provide toast notifications for success/failure. -
Silent error swallowing:
persistFeatureDeletecatches errors and logs them but doesn't re-throw or notify the user. If disk deletion fails, the feature is already removed from state but the user won't know the persistence operation failed.
Note: The order of state removal is already correct—handleDeleteFeature calls removeFeature before persistFeatureDelete, so the features are removed from state synchronously before any async disk operations.
🔧 Suggested error handling approach
onDeleted={async (deletedWorktree, _deletedBranch, deleteFeatures) => {
+ try {
// Handle features assigned to the deleted worktree (by branch)
const featuresToHandle = hookFeatures.filter(
(f) => f.branchName === deletedWorktree.branch
);
// Compute mainBranch once before processing
const mainBranch = worktrees.find((w) => w.isMain)?.branch || 'main';
if (deleteFeatures) {
// Delete all features from disk in parallel
// handleDeleteFeature already calls removeFeature, so no need to call it separately
- await Promise.all(featuresToHandle.map((feature) => handleDeleteFeature(feature.id)));
+ const results = await Promise.allSettled(featuresToHandle.map((feature) => handleDeleteFeature(feature.id)));
+ const failures = results.filter((r) => r.status === 'rejected').length;
+ if (failures > 0) {
+ toast.error(`Failed to delete ${failures} of ${featuresToHandle.length} features`);
+ } else if (featuresToHandle.length > 0) {
+ toast.success(`Deleted ${featuresToHandle.length} features`);
+ }
} else {
// Reassign all features to main branch
const updates = { branchName: mainBranch };
// Use Promise.all to await all updates in parallel
- await Promise.all(
+ const results = await Promise.allSettled(
featuresToHandle.map((feature) => {
updateFeature(feature.id, updates);
return persistFeatureUpdate(feature.id, updates);
})
);
+
+ const failures = results.filter((r) => r.status === 'rejected').length;
+ if (failures > 0) {
+ toast.error(`Failed to reassign ${failures} of ${featuresToHandle.length} features to ${mainBranch}`);
+ } else if (featuresToHandle.length > 0) {
+ toast.success(`Reassigned ${featuresToHandle.length} features to ${mainBranch}`);
+ }
}
+ } catch (error) {
+ logger.error('Error handling worktree deletion:', error);
+ toast.error('Failed to process features during worktree deletion');
+ }
// Refresh worktree list after all operations complete
setWorktreeRefreshKey((k) => k + 1);
setSelectedWorktreeForAction(null);
}}
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (2)
209-233: Custom count parsing should avoidparseInt+ add invalid-state UX.
parseInt("1e2") === 1andparseInt("1.5") === 1; also the user can type0/empty and still regenerate with the previous propagated count. PrefervalueAsNumber+Number.isInteger, and disable regenerate (or show an inline error) whenisCustomand the value is invalid.Proposed fix (safer parsing + validity flag)
+ const customCountNum = Number(customFeatureCount); + const isCustomCountValid = + !generateFeatures || + !isCustom || + (Number.isInteger(customCountNum) && customCountNum >= 1 && customCountNum <= 200); {isCustom && ( <Input type="number" min="1" max="200" value={customFeatureCount} onChange={(e) => { setCustomFeatureCount(e.target.value); - const num = parseInt(e.target.value, 10); - if (!isNaN(num) && num >= 1 && num <= 200) { - onFeatureCountChange(num); - } + const num = e.currentTarget.valueAsNumber; + if (Number.isInteger(num) && num >= 1 && num <= 200) onFeatureCountChange(num); }} placeholder="Enter number of features (1-200)" disabled={isDisabled} className="text-sm" data-testid="regenerate-feature-count-custom-input" /> )} + {!isCustomCountValid && ( + <p className="text-xs text-destructive">Enter a whole number from 1–200.</p> + )}And then use
isCustomCountValidin the submit button (see next comment).
19-55: Refactor preset extraction to derive from FEATURE_COUNT_OPTIONS and remove unnecessary casting.The hard-coded
[20, 50, 100]preset list creates a maintenance risk—ifFEATURE_COUNT_OPTIONSis updated, this list won't automatically reflect the change. Extract numeric presets directly from the options to keep them in sync. Additionally, remove theas numbercast sinceFeatureCountis already typed asnumber, and the'custom'guard is unnecessary (the type definition does not include'custom'as a validFeatureCountvalue).Proposed fix
useEffect(() => { - if (open) { - const presetValues = [20, 50, 100]; + if (!open) return; + + const numericOptions = FEATURE_COUNT_OPTIONS + .map((o) => o.value) + .filter((v): v is number => typeof v === 'number'); + + if (typeof featureCount === 'number') { + const isPreset = numericOptions.includes(featureCount); setIsCustom(!isPreset); - setCustomFeatureCount(isPreset ? '' : String(featureCount)); + setCustomFeatureCount(isPreset ? '' : String(featureCount)); + return; } - }, [open, featureCount]); + }, [open, featureCount]);
🤖 Fix all issues with AI agents
In @apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx:
- Around line 91-124: The Input currently trims on every keystroke via
onChange={(e) => onWorktreeBranchChange(e.target.value.trim())}, which causes
cursor jumps; change onChange to pass the raw value (e.g., onChange={(e) =>
onWorktreeBranchChange(e.target.value)}) and move trimming/validation to an
onBlur handler and the submit path (call
onWorktreeBranchChange(worktreeBranch.trim()) or trim before submit in the
function that handles the dialog submit); keep the existing props
(value={worktreeBranch}, data-testid) and add an onBlur that trims and updates
the same onWorktreeBranchChange to ensure final validation without trimming
during typing.
In @apps/ui/tests/git/delete-worktree-with-features.spec.ts:
- Around line 14-25: Remove the redundant second import of addFeature and
getKanbanColumn and instead import them from the centralized utils import
alongside the other helpers; update the first import statement (the one that
currently brings in waitForNetworkIdle, createTestGitRepo, cleanupTempDir,
createTempDirPath, setupProjectWithPath, waitForBoardView, authenticateForTests,
handleLoginScreenIfPresent) to also include addFeature and getKanbanColumn, then
delete the separate import that references addFeature/getKanbanColumn to keep
imports consistent and avoid duplication.
🧹 Nitpick comments (8)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (2)
44-52: Derive preset values fromFEATURE_COUNT_OPTIONSto avoid duplication.The hardcoded
presetValuesarray duplicates values already defined inFEATURE_COUNT_OPTIONS(fromapps/ui/src/components/views/spec-view/constants.ts). If the preset options change in constants, this code won't update automatically.♻️ Proposed refactor to eliminate duplication
useEffect(() => { if (open) { - const presetValues = [20, 50, 100]; + const presetValues = FEATURE_COUNT_OPTIONS + .filter(o => typeof o.value === 'number') + .map(o => o.value as number); const isPreset = presetValues.includes(featureCount as number); setIsCustom(!isPreset); setCustomFeatureCount(isPreset ? '' : String(featureCount)); } }, [open, featureCount]);
91-124: Consider moving.trim()toonBlurfor better UX.Trimming on every keystroke (line 115) prevents users from typing spaces during input, though this is unlikely to affect branch names. Moving
.trim()toonBlurwould provide a better typing experience while still ensuring clean values.♻️ Optional refactor to improve input UX
<Input type="text" value={worktreeBranch} - onChange={(e) => onWorktreeBranchChange(e.target.value.trim())} + onChange={(e) => onWorktreeBranchChange(e.target.value)} + onBlur={(e) => onWorktreeBranchChange(e.target.value.trim())} placeholder="e.g., test-worktree, feature-new" disabled={isCreatingSpec} className="font-mono text-sm" data-testid="create-worktree-branch-input" />apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (1)
174-207: Feature count selection logic is fine, but consider setting a default when switching to “custom”.
Today clicking “custom” with an empty input keeps the priorfeatureCount, which can be surprising. Optional: seedcustomFeatureCountfrom currentfeatureCount(if numeric) when toggling into custom.apps/ui/tests/git/delete-worktree-with-features.spec.ts (5)
30-60: Reduce flakiness: replace fixed sleeps + keyboard shortcut navigation with state-based waits.
waitForTimeout(500)andpage.keyboard.press('k')make this helper sensitive to timing/shortcuts. Prefer asserting the checkbox becomes checked and navigating back via an explicit UI affordance (or a stable testid).Proposed adjustment (state-based waits)
async function enableWorktrees(page: Page) { await page.click('[data-testid="settings-button"]'); await page.waitForSelector('[data-testid="settings-view"]', { timeout: 5000 }); // Click on Feature Defaults section const featureDefaultsSection = page.getByRole('button', { name: /Feature Defaults/i }); await featureDefaultsSection.click(); - await page.waitForTimeout(500); // Scroll to and enable worktrees checkbox const worktreeCheckbox = page.locator('[data-testid="use-worktrees-checkbox"]'); await worktreeCheckbox.scrollIntoViewIfNeeded(); await worktreeCheckbox.waitFor({ state: 'visible', timeout: 5000 }); const isChecked = await worktreeCheckbox.isChecked(); if (!isChecked) { await worktreeCheckbox.click(); - await page.waitForTimeout(500); + await expect(worktreeCheckbox).toBeChecked({ timeout: 5000 }); } - // Navigate back to board using keyboard shortcut - await page.keyboard.press('k'); + // Prefer explicit navigation to avoid shortcut dependency + // e.g. click a "Back" / "Board" button if available (or add a data-testid for it). await waitForBoardView(page);
63-82:createWorktree: avoid regex injection + replace unconditional 2s sleep with a concrete “worktree ready” signal.
text=/...${branchName}/iwill break ifbranchNamecontains/or other regex delimiters, andwaitForTimeout(2000)is a classic flake source. Preferexpect(...).toBeVisible()on the new worktree tab (or a deterministic API/network wait) instead of sleeping.
84-103: HardengetFeaturesForBranch: fail with clearer errors on malformed JSON (and keep tests debuggable).
A single corruptedfeature.jsonwill currently throw a rawJSON.parseexception without context. Consider wrapping parse with a message that includesfeatureJsonPathfor fast diagnosis.
164-172: Brittle selector:locator('..').locator('button').last()can click the wrong control as UI evolves.
If the container gains another button, these tests will misclick. Prefer a stable selector (data-testid/aria-label) for the worktree “actions” (three-dots) button; if none exists, adding one will pay off quickly.Also applies to: 253-261, 353-357, 398-406, 457-465
394-430: Git assertions: prefer exact-match checks overgit branchsubstring matching.
expect(stdout).toContain('branch-to-delete')can false-positive if similarly named branches exist. Usegit branch --list <name>orgit show-ref --verify --quiet refs/heads/<name>and assert on exit code.Also applies to: 453-494
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.tsapps/ui/tests/utils/views/spec-editor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/tests/utils/views/spec-editor.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/ui/tests/git/delete-worktree-with-features.spec.ts
🧬 Code graph analysis (1)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (3)
apps/ui/src/components/views/spec-view/types.ts (1)
CreateSpecDialogProps(15-36)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (2)
1-17: LGTM!The new imports (
useState,useEffect,Input) are necessary for the worktree branch and custom feature count functionality.
30-33: LGTM!The new worktree branch props are correctly integrated and match the interface defined in
apps/ui/src/components/views/spec-view/types.ts.apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (2)
2-15: Imports look good;Inputaddition is appropriate.
245-249: Good: submit blocks empty worktree branch; also block invalid custom count.Proposed tweak (if you add
isCustomCountValid)disabled={ !projectDefinition.trim() || isDisabled || - (useWorktreeBranch && !worktreeBranch.trim()) + (useWorktreeBranch && !worktreeBranch.trim()) || + !isCustomCountValid }apps/ui/tests/git/delete-worktree-with-features.spec.ts (2)
113-131: No changes needed.cleanupTempDir()is synchronous (usesfs.rmSync()and returnsvoid), so anawaitis not applicable. Additionally,createTempDirPath()already ensures worker uniqueness by incorporatingprocess.pidand a random component into the directory path, so parallel test runs will not collide.Likely an incorrect or invalid review comment.
133-220: Remove hard-coded'main'references; the feature code already handles this dynamically, and tests are correct given their setup.The delete-worktree feature already dynamically detects the base branch via
worktrees.find((w) => w.isMain)?.branch(board-view.tsx:1444), which correctly retrieves whatever branch is the main worktree—not hard-coded to'main'. SincecreateTestGitRepoexplicitly enforcesgit branch -M main(worktree.ts:100), the test's hard-coded'main'references are safe and accurate. The implementation is flexible; the tests are not flaky. No code changes needed.Likely an incorrect or invalid review comment.
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (3)
38-66: Significant code duplication withCreateSpecDialog.The custom feature count state management (lines 38-66) is nearly identical to
CreateSpecDialog. Consider extracting this into a custom hook likeuseCustomFeatureCount:function useCustomFeatureCount(open: boolean, featureCount: FeatureCount, generateFeatures: boolean) { const [customFeatureCount, setCustomFeatureCount] = useState<string>(''); const [isCustom, setIsCustom] = useState(false); useEffect(() => { /* sync logic */ }, [open, featureCount]); const isCustomCountValid = /* validation */; return { customFeatureCount, setCustomFeatureCount, isCustom, setIsCustom, isCustomCountValid }; }This would reduce duplication and ensure consistent behavior across both dialogs.
103-137: Worktree branch UI block is duplicated.This entire UI block (checkbox + conditional input) is identical to
CreateSpecDialog. Consider extracting into a shared component likeWorktreeBranchSection:<WorktreeBranchSection id="regenerate-use-worktree-branch" checked={useWorktreeBranch} onCheckedChange={onUseWorktreeBranchChange} value={worktreeBranch} onChange={onWorktreeBranchChange} disabled={isDisabled} testIdPrefix="regenerate" />
187-220: Feature count selection UI is also duplicated.The feature count buttons and custom input logic mirrors
CreateSpecDialog. This is a third area where extracting aFeatureCountSelectorcomponent would improve maintainability:<FeatureCountSelector featureCount={featureCount} onFeatureCountChange={onFeatureCountChange} disabled={isDisabled} testIdPrefix="regenerate" />For now, the implementation is correct and functional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/tests/git/delete-worktree-with-features.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/tests/git/delete-worktree-with-features.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx
🧬 Code graph analysis (1)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (4)
apps/ui/src/components/views/spec-view/types.ts (1)
RegenerateSpecDialogProps(39-58)apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-19)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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 (6)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (5)
41-68: Custom feature count state management looks correct.The
useEffectproperly syncs local state when the dialog opens. The validation logic at lines 63-68 correctly handles the three cases: features disabled, preset selected, or valid custom count.One minor observation: when
customFeatureCountis an empty string,Number('')returns0, which fails validation as expected. This is correct behavior.
103-137: Worktree branch UI implementation is well-structured.The checkbox-controlled conditional input pattern is clean. The
onBlurhandler trims whitespace, and the input is properly disabled during spec creation.
196-206: Potential state inconsistency when selecting "Custom" with empty input.When clicking the "Custom" button with an empty
customFeatureCount, theparseInt('', 10)call is skipped (line 199-201), butisCustomis set totrue. This leavesfeatureCountat its previous value while the UI shows the custom input as selected.If the previous
featureCountwas a preset (e.g., 20), the validation will pass (isCustomCountValidistruewhenisCustomisfalseor when custom count is valid), but sinceisCustomis nowtrueandcustomFeatureCountis empty,isCustomCountValidbecomesfalse— which is actually correct behavior. The submit button will be disabled until a valid custom count is entered.
229-234: Consider usingvalueAsNumberconsistently for validation.The validation uses
e.currentTarget.valueAsNumberwhich returnsNaNfor empty strings or invalid input. TheNumber.isInteger(NaN)check returnsfalse, so invalid values won't propagate — this is correct.However, a minor edge case: if a user types "1.5",
valueAsNumberreturns1.5, andNumber.isInteger(1.5)isfalse, so it won't update the parent. The displayed value will show "1.5" but the actualfeatureCountprop won't change. The validation message will appear, which is the intended behavior.
270-275: Submit button validation is comprehensive.The disabled logic correctly covers:
- Empty project overview
- Spec creation in progress
- Worktree branch enabled but empty
- Invalid custom feature count
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (1)
265-270: Submit validation logic is correct and consistent.The disabled conditions properly mirror the
CreateSpecDialogvalidation, accounting for:
- Empty project definition
- Regeneration/feature generation in progress
- Required worktree branch when enabled
- Invalid custom feature count
|
@webdevcody the test get this error there no problem at all when I test it locally. |
|
conflicts, also speak with @Shironex as he did some worktree fixes, I'm curious if these are still issues or not. |
2 participants
Enhanced worktree management and spec generation with better UX and
comprehensive test coverage:
Spec Generation Improvements:
Delete Worktree Enhancements:
Test Coverage:
Technical Changes:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.