Fix: Dev server detection bug fixes. Settings sync bug fixes. Cli provider fixes. Terminal background/foreground colors#791
Merged
gsxdsm merged 6 commits intoAutoMaker-Org:v0.15.0rcfrom Feb 21, 2026
Conversation
|
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:
📝 WalkthroughWalkthroughAdds guarded auto-mode event emissions, pre-create fast‑forward sync for worktree creation (syncResult/baseCommitHash in responses), terminal color customization (UI, store, types), dev-server URL-detection timeout/reconciliation, expanded/maxTurns configurability and runtime logging, and supporting tests and types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Worktree API Handler
participant Git as Git Repo
participant FS as Local Filesystem
participant Response as Response
Client->>API: POST /worktree/create (baseBranch or HEAD)
API->>API: determine effectiveBase
alt effectiveBase ≠ HEAD
API->>Git: getTrackingBranch(projectPath, effectiveBase)
Git-->>API: remote & remoteBranch or null
API->>Git: isBranchCheckedOut(projectPath, effectiveBase)
Git-->>API: isCheckedOut
API->>Git: syncBaseBranch(projectPath, effectiveBase) (fetch + fast-forward-only)
alt tracking branch exists
Git->>Git: fetch remote
Git->>Git: attempt fast-forward
Git-->>API: synced=true or synced=false + diverged/message
else no tracking branch
Git-->>API: synced=false (local-only)
end
else effectiveBase == HEAD
API->>Git: determine current HEAD branch and optionally sync
end
API->>FS: create worktree at resolved commit (baseCommitHash)
FS-->>API: worktree created
API->>Response: return worktree + baseCommitHash + syncResult
Response-->>Client: 200 OK (payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
gsxdsm
changed the title
Contributor
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 delivers significant improvements across several core areas of the application, focusing on stability, user experience, and configuration flexibility. It resolves critical UI re-rendering issues, enhances the reliability of dev server detection and worktree creation, and introduces highly requested terminal customization options. Additionally, it refines event emission logic for auto-mode features and ensures comprehensive settings synchronization, leading to a more robust and predictable user environment. 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}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces significant improvements to dev server detection, React performance, and git worktree management. The fixes for spurious auto-mode events and the React re-render loop (Error #185) are particularly valuable for application stability. The addition of base branch synchronization during worktree creation ensures a more reliable development workflow. The dev server robustness improvements, including process pruning and timeout fallbacks, enhance the developer experience. However, the heuristic used to identify remote branches in the synchronization logic is too broad and may skip valid local branches that contain slashes in their names.
Comment on lines
131
to
156
| if (branchName.includes('/')) { | ||
| // Get the commit hash of the remote ref for logging | ||
| try { | ||
| const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); | ||
| return { | ||
| attempted: false, | ||
| synced: true, | ||
| commitHash: commitHash.trim(), | ||
| message: `Using remote ref ${branchName} (already fetched)`, | ||
| }; | ||
| } catch { | ||
| return { attempted: false, synced: false, message: `Remote ref ${branchName} not found` }; | ||
| } | ||
| } | ||
|
|
||
| // Check if the branch exists locally | ||
| try { | ||
| await execGitCommand(['rev-parse', '--verify', branchName], projectPath); | ||
| } catch { | ||
| // Branch doesn't exist locally — nothing to sync | ||
| return { | ||
| attempted: false, | ||
| synced: false, | ||
| message: `Local branch '${branchName}' does not exist`, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
The heuristic branchName.includes('/') used to identify remote refs is too broad. It incorrectly identifies local branches that follow common naming conventions (e.g., feature/abc or fix/issue-123) as remote refs, causing them to skip the synchronization process. A more robust approach is to verify if the branch exists in refs/heads/ to confirm it is a local branch before attempting to sync with its tracking branch.
// Check if the branch exists locally
let existsLocally = false;
try {
await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], projectPath);
existsLocally = true;
} catch {
existsLocally = false;
}
if (!existsLocally) {
// If not a local branch, check if it's a valid ref (remote ref, tag, or hash)
try {
const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath);
return {
attempted: false,
synced: true,
commitHash: commitHash.trim(),
message: `Using ref ${branchName} (not a local branch)`,
};
} catch {
return { attempted: false, synced: false, message: `Ref '${branchName}' not found` };
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
apps/server/src/services/dev-server-service.ts (2)
956-958:isRunningdoesn't account for stale processes — could returntruefor exited servers.Unlike
listDevServerswhich now prunes stale entries,isRunning(andgetServerInfo) still only checks map membership. Between reconciliation cycles, these could report a dead server as running.♻️ Optional: inline a staleness check
isRunning(worktreePath: string): boolean { - return this.runningServers.has(worktreePath); + const server = this.runningServers.get(worktreePath); + if (!server) return false; + if (server.process && typeof server.process.exitCode === 'number') { + // Process exited — prune stale entry + if (server.flushTimeout) clearTimeout(server.flushTimeout); + if (server.urlDetectionTimeout) clearTimeout(server.urlDetectionTimeout); + this.allocatedPorts.delete(server.port); + this.runningServers.delete(worktreePath); + return false; + } + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 956 - 958, The isRunning method currently only checks this.runningServers.has(worktreePath) and can return true for stale/exited servers; update isRunning (and similarly getServerInfo) to fetch the ServerInfo from this.runningServers (use the same shape used by listDevServers), perform the same liveness check used in listDevServers (e.g., verify the stored process/child handle or PID is still alive), remove the entry from this.runningServers if stale, and return false for dead processes; reference functions/objects: isRunning, getServerInfo, listDevServers, and this.runningServers/ServerInfo so the check mirrors the reconciliation logic.
912-937: Pruned stale servers don't emitdev-server:stopped— consider emitting for consistency.The other cleanup paths (
cleanupAndEmitStopat line 721,stopDevServerat line 869) all emitdev-server:stoppedwhen a server is removed. The pruning here silently removes entries, relying on the frontend's 5s polling reconciliation to notice. Emitting the event would give the frontend an immediate notification and keep all removal paths consistent.♻️ Proposed change: emit stop event for pruned servers
for (const stalePath of stalePaths) { const server = this.runningServers.get(stalePath); if (server) { // Clean up timers if (server.flushTimeout) { clearTimeout(server.flushTimeout); } if (server.urlDetectionTimeout) { clearTimeout(server.urlDetectionTimeout); } + // Emit stopped event so frontend gets immediate notification + if (this.emitter) { + this.emitter.emit('dev-server:stopped', { + worktreePath: stalePath, + port: server.port, + exitCode: server.process?.exitCode ?? null, + timestamp: new Date().toISOString(), + }); + } this.allocatedPorts.delete(server.port); } this.runningServers.delete(stalePath); }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/dev-server-service.ts` around lines 912 - 937, The pruning loop removes entries from this.runningServers without emitting the "dev-server:stopped" event, causing inconsistency with cleanupAndEmitStop and stopDevServer; update the prune path to emit the same stop event (using the same payload/shape those functions use) before deleting the server and clearing timers — either call the existing cleanupAndEmitStop(stalePath, server) helper if available or invoke the module's event emitter to emit "dev-server:stopped" with worktreePath/port (and any other fields the other paths include), then proceed to clear flushTimeout/urlDetectionTimeout and allocatedPorts and delete the runningServers entry.apps/server/src/routes/worktree/routes/create.ts (1)
125-363: Consider extracting the repeated commit-hash-in-catch pattern.The pattern of getting the short commit hash and building a
BaseBranchSyncResultwithcanProceedWithStale: trueappears ~6 times with a nested try/catch fallback. A small helper would reduce the repetition:♻️ Example helper extraction
+async function buildStaleResult( + projectPath: string, + branchName: string, + remote: string | undefined, + message: string, + extra?: Partial<BaseBranchSyncResult> +): Promise<BaseBranchSyncResult> { + let commitHash: string | undefined; + try { + const hash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + commitHash = hash.trim(); + } catch { /* ignore */ } + return { + attempted: true, + synced: false, + remote, + commitHash, + message, + canProceedWithStale: true, + ...extra, + }; +}🤖 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.ts` around lines 125 - 363, The syncBaseBranch function repeats the pattern of calling execGitCommand(['rev-parse','--short', branchName], projectPath) inside catch blocks to build a BaseBranchSyncResult with canProceedWithStale: true; extract that logic into a small helper (e.g., buildStaleResult or getStaleSyncResult) that accepts parameters like projectPath, branchName, attempted/synced/remote/message and returns the properly populated BaseBranchSyncResult after trying to fetch the short commit hash with a fallback if rev-parse fails; replace each repeated nested try/catch in syncBaseBranch with calls to this helper (also use it for diverged/failed-fetch/merge/update-ref/unexpected-error cases) to remove duplication and make intent clearer.apps/server/src/services/agent-executor.ts (1)
102-113: Falsy check onresolvedMaxTurnsconflates0withundefined/null.
!resolvedMaxTurnsis true for0,undefined, andnull. While0isn't a practical maxTurns value, the warning message says "maxTurns is not set (undefined)" which would be misleading if it were ever0. Consider a stricter check for consistency with the codex-provider pattern:♻️ Suggested stricter check
const resolvedMaxTurns = sdkOptions?.maxTurns; - if (!resolvedMaxTurns) { + if (resolvedMaxTurns == null) { logger.warn( `[execute] Feature ${featureId}: sdkOptions.maxTurns is not set (undefined). ` +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 102 - 113, The current falsy check (!) on resolvedMaxTurns conflates 0 with undefined/null; change the condition in agent-executor.ts to a strict nullish check (e.g., if (resolvedMaxTurns == null) or if (resolvedMaxTurns === undefined)) so only missing values trigger the warning, and keep the existing logger.warn message (or adjust wording to match the chosen check) — update the block around resolvedMaxTurns / sdkOptions and ensure logger.info still runs for set numeric values (use resolvedMaxTurns, featureId, effectiveBareModel to locate the code).
🤖 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/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts`:
- Around line 232-291: The updater passed to setRunningDevServers (the
functional updater starting where setRunningDevServers is called) performs side
effects (logger.info, showUrlDetectedToast, clearPortDetectionTimer, and
mutations to toastShownForRef) which makes the updater impure; instead, make the
updater only compute the new Map and a list of side-effect actions: inside the
updater (referencing backendServers and existing/next logic) collect side
effects into a local array (e.g. sideEffects) rather than executing them, return
the new state, then immediately after calling setRunningDevServers iterate and
execute those collected actions (calling showUrlDetectedToast,
clearPortDetectionTimer, logger.info and mutating toastShownForRef outside the
updater) so the state updater remains pure and side effects run only once.
- Around line 239-250: Reconciliation adds newly discovered servers to state but
doesn't start the port-detection timer when server.urlDetected === false; update
the reconciliation branch that handles !existing (the code that calls
next.set(key, { ...server, urlDetected: server.urlDetected ?? true })) to call
startPortDetectionTimer(key) when server.urlDetected === false (and ensure
toastShownForRef.current behavior remains unchanged), and also keep the existing
clearPortDetectionTimer(key) call for detected servers; finally add
startPortDetectionTimer to the effect's dependency array so the hook uses the
latest timer start function.
- Around line 92-103: The timeout callback in use-dev-servers currently performs
side-effects (logger.warn and backend reconciliation) inside the
setRunningDevServers updater and relies on its return value to decide whether to
continue; lift the “is server still undetected” check out of the updater so the
updater remains a pure reader: first read the current server from the
runningDevServers state (or a synced ref) to determine if server exists and
urlDetected is false, and only if that check passes call setRunningDevServers
(with a pure updater that updates state) and perform the logger.warn and backend
reconciliation/retry logic; reference the
portDetectionTimers.current.delete(key) logic, the setRunningDevServers updater,
and the async timeout callback in your changes.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1957-1959: The overlay text color is still taken from
currentTerminalTheme.foreground so custom foreground overrides are ignored; add
a terminalForegroundColor variable (e.g., const terminalForegroundColor =
customForegroundColor ?? currentTerminalTheme.foreground) next to
terminalBackgroundColor and replace usages of currentTerminalTheme.foreground in
the select-mode overlay styles with terminalForegroundColor (update the overlay
style object/render where select-mode overlay is created, and also apply the
same change in the other occurrence around lines 2488-2494).
In `@apps/ui/src/hooks/use-project-settings-loader.ts`:
- Around line 134-143: The consolidated setState bypasses setCurrentProject and
therefore skips the persistEffectiveThemeForProject side-effect; either
(preferred) after computing updatedProjects and calling useAppStore.setState({
currentProject: updatedProjectData, projects: updatedProjects }), detect if the
project's theme changed (compare updatedProjectData.theme to previous
currentProject.theme from storeState) and, if so, call
persistEffectiveThemeForProject(updatedProjectData) to persist to localStorage,
or (alternative) add a concise comment next to this useAppStore.setState
explaining that theme persistence is intentionally skipped and why (safe because
only activeClaudeApiProfileId and phaseModelOverrides are modified here).
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 731-735: parseLocalStorageSettings and mergeSettings currently
omit the new keys (enableAiCommitMessages, enableSkills, skillsSources,
enableSubagents, subagentsSources) so first-time migrations reset them to
defaults; update the legacy parsing in parseLocalStorageSettings and the
fallback merge logic in mergeSettings to read and preserve these keys from
localStorage (use nullish-coalescing or explicit checks to accept stored falsey
values) and ensure the merged result includes enableAiCommitMessages,
enableSkills, skillsSources, enableSubagents, and subagentsSources so existing
user prefs are not dropped during migration.
In `@apps/ui/src/routes/__root.tsx`:
- Around line 186-213: The root issue is that getEffectiveTheme() depends on
previewTheme but the component never subscribes to previewTheme, so preview
updates don't trigger re-renders; fix by adding a useAppStore selector for
previewTheme (e.g., const previewTheme = useAppStore(s => s.previewTheme)) or
refactor getEffectiveTheme into the selector (e.g., const effectiveTheme =
useAppStore(s => s.getEffectiveTheme()) ) so the component subscribes to the
previewTheme-dependent value and re-renders when previews change; update
references in the component to use the new selector (getEffectiveTheme or
effectiveTheme) accordingly.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 125-363: The syncBaseBranch function repeats the pattern of
calling execGitCommand(['rev-parse','--short', branchName], projectPath) inside
catch blocks to build a BaseBranchSyncResult with canProceedWithStale: true;
extract that logic into a small helper (e.g., buildStaleResult or
getStaleSyncResult) that accepts parameters like projectPath, branchName,
attempted/synced/remote/message and returns the properly populated
BaseBranchSyncResult after trying to fetch the short commit hash with a fallback
if rev-parse fails; replace each repeated nested try/catch in syncBaseBranch
with calls to this helper (also use it for
diverged/failed-fetch/merge/update-ref/unexpected-error cases) to remove
duplication and make intent clearer.
In `@apps/server/src/services/agent-executor.ts`:
- Around line 102-113: The current falsy check (!) on resolvedMaxTurns conflates
0 with undefined/null; change the condition in agent-executor.ts to a strict
nullish check (e.g., if (resolvedMaxTurns == null) or if (resolvedMaxTurns ===
undefined)) so only missing values trigger the warning, and keep the existing
logger.warn message (or adjust wording to match the chosen check) — update the
block around resolvedMaxTurns / sdkOptions and ensure logger.info still runs for
set numeric values (use resolvedMaxTurns, featureId, effectiveBareModel to
locate the code).
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 956-958: The isRunning method currently only checks
this.runningServers.has(worktreePath) and can return true for stale/exited
servers; update isRunning (and similarly getServerInfo) to fetch the ServerInfo
from this.runningServers (use the same shape used by listDevServers), perform
the same liveness check used in listDevServers (e.g., verify the stored
process/child handle or PID is still alive), remove the entry from
this.runningServers if stale, and return false for dead processes; reference
functions/objects: isRunning, getServerInfo, listDevServers, and
this.runningServers/ServerInfo so the check mirrors the reconciliation logic.
- Around line 912-937: The pruning loop removes entries from this.runningServers
without emitting the "dev-server:stopped" event, causing inconsistency with
cleanupAndEmitStop and stopDevServer; update the prune path to emit the same
stop event (using the same payload/shape those functions use) before deleting
the server and clearing timers — either call the existing
cleanupAndEmitStop(stalePath, server) helper if available or invoke the module's
event emitter to emit "dev-server:stopped" with worktreePath/port (and any other
fields the other paths include), then proceed to clear
flushTimeout/urlDetectionTimeout and allocatedPorts and delete the
runningServers entry.
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
apps/ui/src/hooks/use-project-settings-loader.ts (1)
134-148: Past review concern addressed —persistEffectiveThemeForProjectbypass is now documented.The NOTE comment at lines 140–144 precisely explains why skipping
setCurrentProject(and thuspersistEffectiveThemeForProject) is safe here. This resolves the previous flag.The consolidated
useAppStore.setStateis the correct atomic write and aligns with Zustand's imperative-access pattern (getState()at line 117 gives the freshest snapshot without polluting the dependency array).One minor naming nit at line 118:
updatedProjectis the current store snapshot — it isn't updated yet at that point. Renaming it tocurrentStoredProjectorsnapshotProjectwould reduce the cognitive gap between this variable and theupdatedProjectDatacomputed just below it.📝 Suggested rename
- const updatedProject = storeState.currentProject; - if (updatedProject && updatedProject.path === projectPath) { + const currentStoredProject = storeState.currentProject; + if (currentStoredProject && currentStoredProject.path === projectPath) { const needsUpdate = (activeClaudeApiProfileId !== undefined && - updatedProject.activeClaudeApiProfileId !== activeClaudeApiProfileId) || + currentStoredProject.activeClaudeApiProfileId !== activeClaudeApiProfileId) || (phaseModelOverrides !== undefined && - JSON.stringify(updatedProject.phaseModelOverrides) !== + JSON.stringify(currentStoredProject.phaseModelOverrides) !== JSON.stringify(phaseModelOverrides)); if (needsUpdate) { const updatedProjectData = { - ...updatedProject, + ...currentStoredProject, ...(activeClaudeApiProfileId !== undefined && { activeClaudeApiProfileId }), ...(phaseModelOverrides !== undefined && { phaseModelOverrides }), }; const updatedProjects = storeState.projects.map((p) => - p.id === updatedProject.id ? updatedProjectData : p + p.id === currentStoredProject.id ? updatedProjectData : p );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-project-settings-loader.ts` around lines 134 - 148, Rename the variable currently named updatedProject (the snapshot from the store) to a clearer name like snapshotProject or currentStoredProject to reduce confusion with updatedProjectData; update all references within use-project-settings-loader.ts (including where snapshot is used to compute updatedProjectData and when building updatedProjects) so the rename is consistent, keep the existing logic and comments unchanged, and run tests/TS checks to ensure no remaining references to the old identifier.apps/server/src/routes/worktree/routes/create.ts (3)
39-358: Business logic should live in a service, not a route handler.~300 lines of new sync logic (
getTrackingBranch,isBranchCheckedOut,buildStaleResult,syncBaseBranch) are added directly inside a route file. Consider extracting these into a dedicated service (e.g.,services/branch-sync-service.ts) and having the route handler delegate to it. This would improve testability, reusability, and align with the project's architecture.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".
🤖 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.ts` around lines 39 - 358, Move the ~300 lines of sync logic out of the route into a new service module (e.g., services/branch-sync-service.ts): extract and export the BaseBranchSyncResult type and the functions getTrackingBranch, isBranchCheckedOut, buildStaleResult, and syncBaseBranch into that file, and ensure the service imports or accepts (via parameters) the dependencies used in the implementation (execGitCommand, logger, FETCH_TIMEOUT_MS, getErrorMessage, AbortController). Then update the route to delegate to the service by importing syncBaseBranch (and any exported types if needed) and calling it from the route handler; keep all behavior unchanged but remove the implementations from the route file so the route only orchestrates request/response while business logic lives in the new service.
214-240: Double fetch:fetch --allalready ran beforesyncBaseBranchis called.Lines 504-509 already do
git fetch --all, so the per-branch fetch here is redundant in the normal flow. If you extract this into a service (per the refactor above), consider accepting an optionalskipFetchparameter to avoid the duplicate network round-trip.🤖 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.ts` around lines 214 - 240, The per-branch fetch in syncBaseBranch is redundant when a prior git fetch --all already ran; add an optional boolean parameter (e.g., skipFetch) to the syncBaseBranch function and, before the AbortController/fetch block that calls execGitCommand(['fetch', tracking.remote, tracking.remoteBranch, '--quiet'], ...), short-circuit that block when skipFetch is true. Ensure callers of syncBaseBranch that run after a global fetch pass skipFetch=true (update call sites), while defaulting skipFetch to false so existing behavior remains unchanged; keep the existing timeout logic (FETCH_TIMEOUT_MS), error handling (getErrorMessage, logger.warn) and buildStaleResult path unchanged for the non-skip case.
169-186:synced: trueis misleading when no sync was performed.When the branch isn't local but resolves as a valid ref (remote ref, tag, commit hash), the code returns
{ attempted: false, synced: true }. No sync actually happened — the ref merely exists. Considersynced: falsewith a separate flag (e.g.,usable: true) or renamingsyncedto something likebaseResolvedto avoid confusion for future maintainers.🤖 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.ts` around lines 169 - 186, The returned object when a non-local ref resolves should not claim synced: true; update the return value in the block that uses existsLocally and execGitCommand (the branchName resolution path) to set synced: false and add a new explicit flag (e.g., baseResolved: true or usable: true) so callers can distinguish "ref exists but no sync performed"; ensure the same symbol names (attempted, commitHash, message) are preserved and only add the new flag to the returned object.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts (1)
220-319: Reconciliation effect polls every 5 seconds — consider gating on visibility.The 5-second polling interval runs unconditionally, even when the browser tab is backgrounded or the panel is not visible. For a dev tool this is acceptable, but you could reduce unnecessary API calls by pausing when
document.hidden === trueor when the worktree panel is not mounted.🤖 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/hooks/use-dev-servers.ts` around lines 220 - 319, The reconcile polling currently started in the useEffect (inside use-dev-servers.ts) runs unconditionally via setInterval(reconcile, STATE_RECONCILE_INTERVAL_MS); change it to pause when the panel/tab is not visible by checking document.hidden (and optionally a prop/state that indicates the worktree panel is mounted/visible) before calling the API: add a visibilitycheck wrapper around reconcile (or early-return inside reconcile) and subscribe to the visibilitychange event to start/stop or skip the interval work; ensure you still clearInterval on cleanup and keep the same symbols: reconcile, STATE_RECONCILE_INTERVAL_MS, setInterval, clearInterval, and the useEffect dependency list.apps/server/src/lib/settings-helpers.ts (1)
65-68: Consider aligning the no-SettingsService fallback with the new default.With the global default now true, returning false when SettingsService is unavailable disables CLAUDE.md auto-loading in early-boot/test paths. If that’s not intentional, consider defaulting to true here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/settings-helpers.ts` around lines 65 - 68, The fallback when SettingsService is unavailable currently treats autoLoadClaudeMd as false; update the logic where settingsService.getGlobalSettings() is called so that the fallback default matches the new global default (true) — specifically change how globalSettings.autoLoadClaudeMd is resolved in the block that computes result (and logged with logger.info) so it uses true when settingsService or globalSettings is missing, ensuring autoLoadClaudeMd defaults to true during early-boot/test paths.apps/server/src/lib/sdk-options.ts (1)
371-374: Consider clamping maxTurns inside sdk-options.Callers outside settings helpers can still pass values outside 1–2000; a small clamp helper here would enforce the documented range consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/sdk-options.ts` around lines 371 - 374, Add a small clamp helper in sdk-options to enforce the documented 1–2000 range for the optional maxTurns field: implement a function (e.g., clampMaxTurns or clampRange) that takes a number and returns Math.min(2000, Math.max(1, value || defaultValue)), and use it wherever the maxTurns value from the SDK options is read or applied (references: the maxTurns property in the relevant options interface and any functions that consume options to derive MAX_TURNS); ensure the helper is exported/used so callers passing out-of-range values are normalized consistently.
🤖 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/create.ts`:
- Around line 96-109: isBranchCheckedOut currently only checks the main worktree
(function isBranchCheckedOut) so update-ref may desync branches checked out in
other worktrees; modify isBranchCheckedOut to run "git worktree list
--porcelain" (via execGitCommand, following the findExistingWorktreeForBranch
pattern) and return true if any listed worktree has HEAD on branchName, and then
in the merge/update-ref flow use that result to either skip the direct
update-ref or perform the fast-forward/merge from the identified worktree path
instead of updating refs blindly.
In `@apps/server/src/services/agent-executor.ts`:
- Around line 102-118: The code currently leaves resolvedMaxTurns undefined when
sdkOptions?.maxTurns is not set, causing execute() to behave differently than
executeTasksLoop() which defaults to 1000; update the logic in execute() so
resolvedMaxTurns falls back to the same default (1000) when sdkOptions?.maxTurns
is null/undefined, adjust the logger messages to reflect the applied default,
and ensure the ExecuteOptions object (executeOptions.maxTurns) receives this
fallback value so behavior is consistent with executeTasksLoop().
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 968-979: Extract a private helper (e.g., pruneStaleServer) that
encapsulates the stale-entry removal logic currently duplicated in isRunning,
getServerInfo, and listDevServers: check server.process && typeof
server.process.exitCode === 'number', clear server.flushTimeout and
server.urlDetectionTimeout if present, remove server.port from
this.allocatedPorts and remove the entry from this.runningServers, and crucially
emit the "dev-server:stopped" event with the removed server info before
returning. Replace the stale-removal blocks inside isRunning and getServerInfo
(and the corresponding block in listDevServers) to call this helper so all paths
consistently emit the stopped event when pruning entries.
In
`@apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx`:
- Around line 135-141: The current code uses parseInt on maxTurnsInput which
silently truncates decimals (e.g., "1.5" -> 1); change the validation to
explicitly reject non-integers: parse the value with parseFloat or
Number(maxTurnsInput), check Number.isInteger(value) and that value is between 1
and 2000 before calling onDefaultMaxTurnsChange(value); otherwise call
setMaxTurnsInput(String(defaultMaxTurns)) to reset. Update references in this
block (maxTurnsInput, parsed/parseInt, onDefaultMaxTurnsChange,
setMaxTurnsInput, defaultMaxTurns) accordingly; optionally add step={1} to the
input element to prevent decimal entry in the UI.
- Line 69: The local state maxTurnsInput (set by
useState(String(defaultMaxTurns))) is only initialized once and will stay stale
if defaultMaxTurns changes after mount; add a useEffect that watches
defaultMaxTurns and calls setMaxTurnsInput(String(defaultMaxTurns)) to keep the
displayed input in sync with prop updates (ensure you reference maxTurnsInput,
setMaxTurnsInput and defaultMaxTurns in the effect and convert the value to a
string).
In `@apps/ui/src/store/app-store.ts`:
- Around line 2372-2383: In setDefaultMaxTurns ensure maxTurns is a finite
number before flooring and clamping: validate with Number.isFinite(maxTurns) (or
Number.isFinite(Number(maxTurns))) and if not finite fallback to a safe default
(e.g., 1), then apply Math.floor and clamp to the 1–2000 range, assign to
clamped and use that value for set({ defaultMaxTurns: clamped }) and the
subsequent httpApi.put; this prevents NaN/Infinity from propagating into state
or the server call.
---
Nitpick comments:
In `@apps/server/src/lib/sdk-options.ts`:
- Around line 371-374: Add a small clamp helper in sdk-options to enforce the
documented 1–2000 range for the optional maxTurns field: implement a function
(e.g., clampMaxTurns or clampRange) that takes a number and returns
Math.min(2000, Math.max(1, value || defaultValue)), and use it wherever the
maxTurns value from the SDK options is read or applied (references: the maxTurns
property in the relevant options interface and any functions that consume
options to derive MAX_TURNS); ensure the helper is exported/used so callers
passing out-of-range values are normalized consistently.
In `@apps/server/src/lib/settings-helpers.ts`:
- Around line 65-68: The fallback when SettingsService is unavailable currently
treats autoLoadClaudeMd as false; update the logic where
settingsService.getGlobalSettings() is called so that the fallback default
matches the new global default (true) — specifically change how
globalSettings.autoLoadClaudeMd is resolved in the block that computes result
(and logged with logger.info) so it uses true when settingsService or
globalSettings is missing, ensuring autoLoadClaudeMd defaults to true during
early-boot/test paths.
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 39-358: Move the ~300 lines of sync logic out of the route into a
new service module (e.g., services/branch-sync-service.ts): extract and export
the BaseBranchSyncResult type and the functions getTrackingBranch,
isBranchCheckedOut, buildStaleResult, and syncBaseBranch into that file, and
ensure the service imports or accepts (via parameters) the dependencies used in
the implementation (execGitCommand, logger, FETCH_TIMEOUT_MS, getErrorMessage,
AbortController). Then update the route to delegate to the service by importing
syncBaseBranch (and any exported types if needed) and calling it from the route
handler; keep all behavior unchanged but remove the implementations from the
route file so the route only orchestrates request/response while business logic
lives in the new service.
- Around line 214-240: The per-branch fetch in syncBaseBranch is redundant when
a prior git fetch --all already ran; add an optional boolean parameter (e.g.,
skipFetch) to the syncBaseBranch function and, before the AbortController/fetch
block that calls execGitCommand(['fetch', tracking.remote,
tracking.remoteBranch, '--quiet'], ...), short-circuit that block when skipFetch
is true. Ensure callers of syncBaseBranch that run after a global fetch pass
skipFetch=true (update call sites), while defaulting skipFetch to false so
existing behavior remains unchanged; keep the existing timeout logic
(FETCH_TIMEOUT_MS), error handling (getErrorMessage, logger.warn) and
buildStaleResult path unchanged for the non-skip case.
- Around line 169-186: The returned object when a non-local ref resolves should
not claim synced: true; update the return value in the block that uses
existsLocally and execGitCommand (the branchName resolution path) to set synced:
false and add a new explicit flag (e.g., baseResolved: true or usable: true) so
callers can distinguish "ref exists but no sync performed"; ensure the same
symbol names (attempted, commitHash, message) are preserved and only add the new
flag to the returned object.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts`:
- Around line 220-319: The reconcile polling currently started in the useEffect
(inside use-dev-servers.ts) runs unconditionally via setInterval(reconcile,
STATE_RECONCILE_INTERVAL_MS); change it to pause when the panel/tab is not
visible by checking document.hidden (and optionally a prop/state that indicates
the worktree panel is mounted/visible) before calling the API: add a
visibilitycheck wrapper around reconcile (or early-return inside reconcile) and
subscribe to the visibilitychange event to start/stop or skip the interval work;
ensure you still clearInterval on cleanup and keep the same symbols: reconcile,
STATE_RECONCILE_INTERVAL_MS, setInterval, clearInterval, and the useEffect
dependency list.
In `@apps/ui/src/hooks/use-project-settings-loader.ts`:
- Around line 134-148: Rename the variable currently named updatedProject (the
snapshot from the store) to a clearer name like snapshotProject or
currentStoredProject to reduce confusion with updatedProjectData; update all
references within use-project-settings-loader.ts (including where snapshot is
used to compute updatedProjectData and when building updatedProjects) so the
rename is consistent, keep the existing logic and comments unchanged, and run
tests/TS checks to ensure no remaining references to the old identifier.
apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ui/src/store/app-store.ts (1)
1379-1392:⚠️ Potential issue | 🟠 Major
httpApi.put('/api/settings', ...)pattern in most settings actions targets a non-existent server endpoint.
setAutoLoadClaudeMdandsetSkipSandboxWarningcorrectly usehttpApi.settings.updateGlobal()which routes toPUT /api/settings/global, but all other async settings actions (20+ calls includingsetDefaultMaxTurns,setSkipVerificationInAutoMode,setEnableAiCommitMessages, etc.) callhttpApi.put('/api/settings', ...).The server defines no
PUT /api/settingsendpoint—onlyPUT /api/settings/globalexists. This means the legacy calls are likely failing silently or being misrouted. Migrate all settings sync calls in this file to usehttpApi.settings.updateGlobal()consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/app-store.ts` around lines 1379 - 1392, Multiple settings actions in app-store.ts call httpApi.put('/api/settings', ...) which targets a non-existent endpoint; update them to use httpApi.settings.updateGlobal() like the already-correct setAutoLoadClaudeMd and setSkipSandboxWarning. For each async setter (e.g., setDefaultMaxTurns, setSkipVerificationInAutoMode, setEnableAiCommitMessages, and the other ~20 actions), replace the httpApi.put('/api/settings', payload) call with const httpApi = getHttpApiClient(); await httpApi.settings.updateGlobal(payload) and preserve the existing try/catch and logger.error usage so errors are logged consistently. Ensure the payload remains an object matching the existing key name(s) (e.g., { defaultMaxTurns: value }) so the server receives the same shape.apps/server/src/lib/settings-helpers.ts (1)
36-52:⚠️ Potential issue | 🟡 MinorUpdate the stale JSDoc comment after the
false→truedefault change.Line 38 still reads
"Returns false if settings service is not available."but the function now returnstruein that path (lines 51–52) and also defaults the global-settings fallback totrue(line 67).📝 Proposed fix
/** * Get the autoLoadClaudeMd setting, with project settings taking precedence over global. - * Returns false if settings service is not available. + * Returns true if settings service is not available (defaults to enabling CLAUDE.md auto-loading). *🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/settings-helpers.ts` around lines 36 - 52, Update the stale JSDoc for getAutoLoadClaudeMdSetting to reflect that when SettingsService is not provided (or global fallback is used) the function now defaults to true rather than false; edit the description line that currently says "Returns false if settings service is not available." to indicate it returns true, and ensure any mention of the global-settings fallback default also states true so the comment matches the behavior in getAutoLoadClaudeMdSetting and the logger message that defaults to true.apps/server/src/services/agent-executor.ts (1)
715-717:⚠️ Potential issue | 🟡 Minor
executeSingleAgentContinuationis the only call site missing the?? 1000fallback.Every other
buildExecOptsinvocation in this file applies an explicit?? 1000default (executeline 102,executeTasksLoopline 373, plan-revision line 598). Here,options.sdkOptions?.maxTurnsis passed without a fallback, so ifsdkOptionsis absent the provider falls back to its own internal default — relying on an implicit coupling that breaks silently if the provider default ever changes.♻️ Proposed fix
for await (const msg of provider.executeQuery( - this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns) + this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns ?? 1000) )) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 715 - 717, The call in executeSingleAgentContinuation passes options.sdkOptions?.maxTurns to buildExecOpts without the explicit fallback used elsewhere; update the invocation inside provider.executeQuery in executeSingleAgentContinuation to pass options.sdkOptions?.maxTurns ?? 1000 so buildExecOpts receives a consistent default, mirroring other call sites (see buildExecOpts and other uses in execute, executeTasksLoop, plan-revision) and preventing reliance on the provider's internal default.
🧹 Nitpick comments (6)
apps/server/src/routes/worktree/routes/create.ts (4)
104-127:isBranchCheckedOutusesexecAsync(string command) instead ofexecGitCommand(array args).Every other git invocation in this file uses
execGitCommandwith array arguments, which is the established pattern for preventing command injection and consistent error handling. This helper should follow the same pattern. WhilebranchNameis validated upstream, keeping a consistent execution pattern reduces the risk of future regressions.Use execGitCommand for consistency
async function isBranchCheckedOut(projectPath: string, branchName: string): Promise<boolean> { try { - const { stdout } = await execAsync('git worktree list --porcelain', { cwd: projectPath }); - const lines = stdout.split('\n'); + const output = await execGitCommand(['worktree', 'list', '--porcelain'], projectPath); + const lines = output.split('\n');🤖 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.ts` around lines 104 - 127, The isBranchCheckedOut function currently calls execAsync with a string command; change it to use execGitCommand with array args (e.g., ['worktree','list','--porcelain']) and the same cwd option for consistency and to avoid injection risks, then read stdout from the execGitCommand result exactly as before and preserve the try/catch and return-false on error behavior; update references to execAsync -> execGitCommand inside isBranchCheckedOut and keep branchName handling unchanged.
232-246: Double fetch:fetch --all(line 527) followed by a targeted fetch insidesyncBaseBranch(line 238).The pre-existing
fetch --allon line 527 already fetches all remotes, andsyncBaseBranchperforms a second targeted fetch for the same remote+branch. The comment on line 232 acknowledges this. The overhead is generally small, but for repos with large objects or slow remotes it doubles the network round-trips.Consider passing a
skipFetchflag intosyncBaseBranchwhen the caller has already performed afetch --all, or removing thefetch --alland lettingsyncBaseBranchhandle its own targeted fetch.Also applies to: 523-534
🤖 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.ts` around lines 232 - 246, There are duplicate fetches: callers sometimes run a global "fetch --all" then syncBaseBranch performs another targeted fetch; add an optional parameter (e.g., skipFetch: boolean = false) to syncBaseBranch and check it before running the execGitCommand fetch block (the AbortController/fetch timer using FETCH_TIMEOUT_MS and execGitCommand([... 'fetch', tracking.remote, tracking.remoteBranch, '--quiet'], projectPath, undefined, fetchController) should only run when skipFetch is false), and update the callers that already perform "fetch --all" to call syncBaseBranch(..., { skipFetch: true }) or equivalent to avoid the redundant network round-trip.
39-376: ~340 lines of sync business logic lives in a route file.The
syncBaseBranch,getTrackingBranch,isBranchCheckedOut, andbuildStaleResulthelpers are pure git/sync logic with no HTTP or request/response coupling. Per the project's coding guidelines, server business logic should be organized into services inservices/, with route handlers inroutes/that delegate to services. Consider extracting these into a dedicated service (e.g.,branch-sync-service.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."
🤖 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.ts` around lines 39 - 376, Large sync-related logic (syncBaseBranch, getTrackingBranch, isBranchCheckedOut, buildStaleResult and BaseBranchSyncResult) must be moved out of the route into a service module; create a new service (e.g., branch-sync-service) that exports BaseBranchSyncResult, getTrackingBranch, isBranchCheckedOut, buildStaleResult, and syncBaseBranch and copy the git logic and helper types there, keeping the implementation identical but decoupled from HTTP; then modify the route to import syncBaseBranch (and any needed types) from the new service and make the route handler delegate to syncBaseBranch, keeping the route file only for request/response handling and error mapping; finally update any imports/tests to reference the new exported symbols.
80-88: Remote names containing/will be misparsed.
indexOf('/')splits on the first slash, so a remote namedmy/remotetrackingmy/remote/mainwould yieldremote="my"andremoteBranch="remote/main". While uncommon, this is valid in git. A safer approach would be to query the remote name separately (e.g.,git config branch.<name>.remote) and strip it as a known prefix.🤖 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.ts` around lines 80 - 88, The current split uses indexOf('/') on the upstream string (variables upstream, trimmed, slashIndex) which misparses remotes that themselves contain slashes; change the logic to first attempt to determine the remote name via git config branch.<branch>.remote (or another explicit git query) and use that explicit remote as a known prefix to strip from upstream to get the remoteBranch; if that config lookup fails, fall back to splitting on the last slash (use lastIndexOf) rather than the first so remotes with slashes are preserved when producing remote and remoteBranch.apps/server/src/services/agent-executor.ts (1)
373-379:taskMaxTurnsis loop-invariant — hoist before the loop.
sdkOptionsis destructured once before the loop (lines 327–334) and never mutated. ComputingtaskMaxTurnsinside the loop on every iteration is redundant and slightly misleads the reader into thinking it may vary per task.♻️ Proposed refactor
+ const taskMaxTurns = sdkOptions?.maxTurns ?? 1000; + logger.info( + `[executeTasksLoop] Feature ${featureId}: maxTurns=${taskMaxTurns}` + + (sdkOptions?.maxTurns == null ? ' (defaulted)' : '') + ); for (let taskIndex = 0; taskIndex < tasks.length; taskIndex++) { const task = tasks[taskIndex]; // ... - const taskMaxTurns = sdkOptions?.maxTurns ?? 1000; - logger.info( - `[executeTasksLoop] Feature ${featureId}, task ${task.id} (${taskIndex + 1}/${tasks.length}): ` + - `maxTurns=${taskMaxTurns} (sdkOptions.maxTurns=${sdkOptions?.maxTurns ?? 'undefined'})` - ); + logger.info(`[executeTasksLoop] Feature ${featureId}, task ${task.id} (${taskIndex + 1}/${tasks.length})`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 373 - 379, Hoist the loop-invariant computation of taskMaxTurns out of the loop in executeTasksLoop: compute const taskMaxTurns = sdkOptions?.maxTurns ?? 1000 once before iterating over tasks (sdkOptions is already destructured earlier) and then use that variable inside the loop and when calling this.buildExecOpts(options, taskPrompt, taskMaxTurns) and in the logger message; remove the per-iteration assignment so readers don't think taskMaxTurns can change per task.apps/server/src/services/dev-server-service.ts (1)
924-963:listDevServersstale-pruning andstartedAtfieldThe two-pass approach (collect stale keys → prune) correctly avoids mutating the map during iteration.
Minor note:
startedAtis now included in the response (line 962), but the client-sideDevServerInfointerface (apps/ui/src/components/views/board-view/worktree-panel/types.ts, lines 32–38) does not declare this field. The field is silently included in runtime objects but invisible to the TypeScript type system, which prevents consumers from accessing it in a type-safe way. Consider addingstartedAt?: stringto the UI type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 924 - 963, The server's listDevServers() now returns a startedAt timestamp for each server but the client-side DevServerInfo interface (DevServerInfo) doesn't declare it; update the DevServerInfo type to include startedAt?: string so the UI can access the timestamp in a type-safe way, ensuring all consumers of DevServerInfo recognize the new optional field.
🤖 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/create.ts`:
- Around line 186-204: The code path where !existsLocally currently returns
attempted: false and synced: true is incorrect because no sync occurred; update
the return value in the execGitCommand success branch to reflect that the ref
was only resolved, not synchronized—e.g., set synced: false and add a new flag
resolved: true (or replace synced with resolved), and keep attempted: false,
include the commitHash and a clarifying message using branchName so downstream
consumers can distinguish a resolved ref from an actually synced branch; locate
this change around the !existsLocally check and the execGitCommand(['rev-parse',
'--short', branchName], projectPath) handling.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 142-155: pruneStaleServer currently deletes
this.allocatedPorts.delete(server.port) but detectUrlFromOutput may have mutated
server.port, causing the originally reserved port to leak; add an immutable
field on DevServerInfo (e.g., allocatedPort or initialPort) and set it in
startDevServer when you call findAvailablePort(), leave detectUrlFromOutput to
continue updating server.port, and change pruneStaleServer to delete
this.allocatedPorts.delete(server.allocatedPort) (or .initialPort) and emit the
event using server.port for external reporting while using the new immutable
field to free the reserved port.
---
Outside diff comments:
In `@apps/server/src/lib/settings-helpers.ts`:
- Around line 36-52: Update the stale JSDoc for getAutoLoadClaudeMdSetting to
reflect that when SettingsService is not provided (or global fallback is used)
the function now defaults to true rather than false; edit the description line
that currently says "Returns false if settings service is not available." to
indicate it returns true, and ensure any mention of the global-settings fallback
default also states true so the comment matches the behavior in
getAutoLoadClaudeMdSetting and the logger message that defaults to true.
In `@apps/server/src/services/agent-executor.ts`:
- Around line 715-717: The call in executeSingleAgentContinuation passes
options.sdkOptions?.maxTurns to buildExecOpts without the explicit fallback used
elsewhere; update the invocation inside provider.executeQuery in
executeSingleAgentContinuation to pass options.sdkOptions?.maxTurns ?? 1000 so
buildExecOpts receives a consistent default, mirroring other call sites (see
buildExecOpts and other uses in execute, executeTasksLoop, plan-revision) and
preventing reliance on the provider's internal default.
In `@apps/ui/src/store/app-store.ts`:
- Around line 1379-1392: Multiple settings actions in app-store.ts call
httpApi.put('/api/settings', ...) which targets a non-existent endpoint; update
them to use httpApi.settings.updateGlobal() like the already-correct
setAutoLoadClaudeMd and setSkipSandboxWarning. For each async setter (e.g.,
setDefaultMaxTurns, setSkipVerificationInAutoMode, setEnableAiCommitMessages,
and the other ~20 actions), replace the httpApi.put('/api/settings', payload)
call with const httpApi = getHttpApiClient(); await
httpApi.settings.updateGlobal(payload) and preserve the existing try/catch and
logger.error usage so errors are logged consistently. Ensure the payload remains
an object matching the existing key name(s) (e.g., { defaultMaxTurns: value })
so the server receives the same shape.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 317-353: isBranchCheckedOut can return true for a linked worktree,
so instead call findExistingWorktreeForBranch(projectPath, branchName) and if it
returns a worktreeInfo run execGitCommand(['merge','--ff-only', remoteRef],
worktreeInfo.path) (preserving the existing try/catch and error handling using
getErrorMessage, logger and buildStaleResult), otherwise fall back to the
current update-ref logic that runs rev-parse and update-ref against projectPath;
ensure you reference execGitCommand, branchName, remoteRef, projectPath and
buildStaleResult exactly as in the diff.
In `@apps/ui/src/store/app-store.ts`:
- Around line 2372-2385: The guard in setDefaultMaxTurns is correct—keep the
Number.isFinite(safeValue) fallback, Math.floor and clamp to [1,2000], call
set({ defaultMaxTurns: clamped }) and sync with httpApi.put('/api/settings', {
defaultMaxTurns: clamped }); no code changes required; just finalize the
approval and remove the duplicate review comment marker ([duplicate_comment])
from the PR metadata.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 104-127: The isBranchCheckedOut function currently calls execAsync
with a string command; change it to use execGitCommand with array args (e.g.,
['worktree','list','--porcelain']) and the same cwd option for consistency and
to avoid injection risks, then read stdout from the execGitCommand result
exactly as before and preserve the try/catch and return-false on error behavior;
update references to execAsync -> execGitCommand inside isBranchCheckedOut and
keep branchName handling unchanged.
- Around line 232-246: There are duplicate fetches: callers sometimes run a
global "fetch --all" then syncBaseBranch performs another targeted fetch; add an
optional parameter (e.g., skipFetch: boolean = false) to syncBaseBranch and
check it before running the execGitCommand fetch block (the
AbortController/fetch timer using FETCH_TIMEOUT_MS and execGitCommand([...
'fetch', tracking.remote, tracking.remoteBranch, '--quiet'], projectPath,
undefined, fetchController) should only run when skipFetch is false), and update
the callers that already perform "fetch --all" to call syncBaseBranch(..., {
skipFetch: true }) or equivalent to avoid the redundant network round-trip.
- Around line 39-376: Large sync-related logic (syncBaseBranch,
getTrackingBranch, isBranchCheckedOut, buildStaleResult and
BaseBranchSyncResult) must be moved out of the route into a service module;
create a new service (e.g., branch-sync-service) that exports
BaseBranchSyncResult, getTrackingBranch, isBranchCheckedOut, buildStaleResult,
and syncBaseBranch and copy the git logic and helper types there, keeping the
implementation identical but decoupled from HTTP; then modify the route to
import syncBaseBranch (and any needed types) from the new service and make the
route handler delegate to syncBaseBranch, keeping the route file only for
request/response handling and error mapping; finally update any imports/tests to
reference the new exported symbols.
- Around line 80-88: The current split uses indexOf('/') on the upstream string
(variables upstream, trimmed, slashIndex) which misparses remotes that
themselves contain slashes; change the logic to first attempt to determine the
remote name via git config branch.<branch>.remote (or another explicit git
query) and use that explicit remote as a known prefix to strip from upstream to
get the remoteBranch; if that config lookup fails, fall back to splitting on the
last slash (use lastIndexOf) rather than the first so remotes with slashes are
preserved when producing remote and remoteBranch.
In `@apps/server/src/services/agent-executor.ts`:
- Around line 373-379: Hoist the loop-invariant computation of taskMaxTurns out
of the loop in executeTasksLoop: compute const taskMaxTurns =
sdkOptions?.maxTurns ?? 1000 once before iterating over tasks (sdkOptions is
already destructured earlier) and then use that variable inside the loop and
when calling this.buildExecOpts(options, taskPrompt, taskMaxTurns) and in the
logger message; remove the per-iteration assignment so readers don't think
taskMaxTurns can change per task.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 924-963: The server's listDevServers() now returns a startedAt
timestamp for each server but the client-side DevServerInfo interface
(DevServerInfo) doesn't declare it; update the DevServerInfo type to include
startedAt?: string so the UI can access the timestamp in a type-safe way,
ensuring all consumers of DevServerInfo recognize the new optional field.
…c bug. Address pr comments
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/server/src/lib/settings-helpers.ts (1)
87-107: Consider extracting the magic numbers into named constants.
1000(default) and2000(upper clamp) each appear multiple times inside the function and should stay in sync with the UI store and other layers mentioned in the PR. Named constants make future adjustments safer and document intent.♻️ Proposed refactor
+const DEFAULT_MAX_TURNS = 1000; +const MAX_TURNS_UPPER_BOUND = 2000; +const MAX_TURNS_LOWER_BOUND = 1; + export async function getDefaultMaxTurnsSetting( settingsService?: SettingsService | null, logPrefix = '[SettingsHelper]' ): Promise<number> { if (!settingsService) { - logger.info(`${logPrefix} SettingsService not available, using default maxTurns=1000`); - return 1000; + logger.info(`${logPrefix} SettingsService not available, using default maxTurns=${DEFAULT_MAX_TURNS}`); + return DEFAULT_MAX_TURNS; } try { const globalSettings = await settingsService.getGlobalSettings(); - const result = globalSettings.defaultMaxTurns ?? 1000; - // Clamp to valid range - const clamped = Math.max(1, Math.min(2000, Math.floor(result))); + const raw = globalSettings.defaultMaxTurns; + const result = typeof raw === 'number' && Number.isFinite(raw) ? raw : DEFAULT_MAX_TURNS; + // Clamp to valid range [MAX_TURNS_LOWER_BOUND, MAX_TURNS_UPPER_BOUND] + const clamped = Math.max(MAX_TURNS_LOWER_BOUND, Math.min(MAX_TURNS_UPPER_BOUND, Math.floor(result))); logger.debug(`${logPrefix} defaultMaxTurns from global settings: ${clamped}`); return clamped; } catch (error) { logger.error(`${logPrefix} Failed to load defaultMaxTurns setting:`, error); - return 1000; + return DEFAULT_MAX_TURNS; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/settings-helpers.ts` around lines 87 - 107, Extract the magic numbers in getDefaultMaxTurnsSetting into named constants (e.g., DEFAULT_MAX_TURNS and MAX_ALLOWED_TURNS) and use those constants everywhere the function currently uses 1000 and 2000: return default when settingsService is missing, fallback in result calculation, clamping Math.max/Math.min, and log messages; define the constants near the top of the module (and export them if needed for reuse by the UI/store) so all layers stay in sync and the intent is documented.apps/server/src/services/dev-server-service.ts (2)
758-762: Nit:cleanupAndEmitStopstill uses the closureportvariable forallocatedPorts.delete.Line 760 uses the closure-captured
portwhilestopDevServer(line 915) andpruneStaleServer(line 149) both useserver.allocatedPort. They're always the same value, so this is correct, but usingserverInfo.allocatedPorthere would make the intent consistent across all cleanup paths.♻️ Optional consistency fix
- this.allocatedPorts.delete(port); + this.allocatedPorts.delete(serverInfo.allocatedPort);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 758 - 762, cleanupAndEmitStop currently calls this.allocatedPorts.delete(port) using the closure variable; change it to use the serverInfo object for consistency with other cleanup paths. Specifically, inside cleanupAndEmitStop replace the closure-captured port usage with this.allocatedPorts.delete(serverInfo.allocatedPort) and keep this.runningServers.delete(worktreePath) as-is so all cleanup paths (stopDevServer, pruneStaleServer, cleanupAndEmitStop) consistently reference serverInfo.allocatedPort.
1013-1043:getServerLogsdoesn't prune stale entries, unlike the other accessors.
isRunning,getServerInfo, andlistDevServersall prune stale process entries now, butgetServerLogsstill returns data for a server whose process has exited without triggering cleanup or the stopped event. This is pre-existing and low-risk since other code paths will catch it, but for consistency you could add the same stale check here.♻️ Optional consistency fix
getServerLogs(worktreePath: string): { // ... } { const server = this.runningServers.get(worktreePath); - if (!server) { + if (!server) { + return { + success: false, + error: `No dev server running for worktree: ${worktreePath}`, + }; + } + + // Prune stale entry if the process has exited + if (server.process && typeof server.process.exitCode === 'number') { + this.pruneStaleServer(worktreePath, server); return { success: false, error: `No dev server running for worktree: ${worktreePath}`, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 1013 - 1043, getServerLogs currently returns entries for servers whose child process has already exited; mirror the stale-entry pruning used by isRunning/getServerInfo/listDevServers by checking the server.process (e.g., server.process?.killed || server.process?.exitCode != null) after retrieving server from this.runningServers, and if the process is stale remove the entry from this.runningServers and return the same failure shape (success: false, error: `No dev server running for worktree: ${worktreePath}`); otherwise continue to return the logs (server.scrollbackBuffer) and startedAt as before.apps/server/src/services/agent-executor.ts (2)
295-299: Stream-end log is skipped on all exception paths.The block is inside the
trybut outsidestreamLoop, so any exception thrown during the loop (abort, auth error, sanitized provider error) bypasses it entirely — the elapsed-time/responseLength diagnostic is lost precisely when it would be most useful.♻️ Suggested move to `finally`
- } finally { + } finally { + const streamElapsedMs = Date.now() - streamStartTime; + logger.info( + `[execute] Stream ended for feature ${featureId} after ${Math.round(streamElapsedMs / 1000)}s. ` + + `aborted=${aborted}, specDetected=${specDetected}, responseLength=${responseText.length}` + ); clearInterval(streamHeartbeat);Then remove lines 295-299 from inside the
try.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 295 - 299, The stream-end diagnostic log (using streamStartTime/streamElapsedMs and logger.info in execute around the streamLoop) is currently inside the try and thus skipped on exceptions; move that logging block into a finally block that always runs after the try/catch so elapsed time, aborted/specDetected/responseLength are always logged, and remove the original logger.info lines from inside the try; ensure the finally executes after any catch paths that handle abort/auth/sanitized-provider errors so the same variables (streamStartTime, responseText, aborted, specDetected) are still available or captured before the try ends.
102-102: Extract the repeated1000default into a named constant.The fallback value appears at four independent call sites (lines 102, 373, 598, 716). A class-level or module-level constant documents intent and makes future changes to the default atomic.
♻️ Suggested refactor
export class AgentExecutor { private static readonly WRITE_DEBOUNCE_MS = 500; private static readonly STREAM_HEARTBEAT_MS = 15_000; + private static readonly DEFAULT_MAX_TURNS = 1000;Then replace each occurrence:
- const resolvedMaxTurns = sdkOptions?.maxTurns ?? 1000; + const resolvedMaxTurns = sdkOptions?.maxTurns ?? AgentExecutor.DEFAULT_MAX_TURNS;- const taskMaxTurns = sdkOptions?.maxTurns ?? 1000; + const taskMaxTurns = sdkOptions?.maxTurns ?? AgentExecutor.DEFAULT_MAX_TURNS;- this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? 1000) + this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? AgentExecutor.DEFAULT_MAX_TURNS)- this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns ?? 1000) + this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns ?? AgentExecutor.DEFAULT_MAX_TURNS)Also applies to: 373-373, 598-598, 716-716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` at line 102, Introduce a module-level named constant (e.g., DEFAULT_MAX_TURNS = 1000) and replace the literal fallback 1000 wherever it is used as the max-turns default; specifically update the expression that computes resolvedMaxTurns (currently using sdkOptions?.maxTurns ?? 1000) to use the named constant, and replace the other three occurrences that fall back to 1000 with the same constant so the default is documented and changed atomically.apps/server/src/routes/worktree/routes/create.ts (1)
188-243: Double fetch:fetch --all(line 192) followed by per-remote fetch insidesyncBaseBranch.The
fetch --allon line 192 already updates all remote refs. ThensyncBaseBranchperforms a second, targeted fetch for the specific remote/branch. The service acknowledges this ("may have already been fetched"), but for large repos with many remotes the combined cost is non-trivial.Consider either:
- Skipping the
--allfetch here and relying onsyncBaseBranch's targeted fetch, or- Passing an option to
syncBaseBranchto skip its internal fetch when the caller has already fetched.🤖 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.ts` around lines 188 - 243, The code currently runs a global fetch via execGitCommand(['fetch','--all','--quiet'], ...) then calls syncBaseBranch which does its own targeted fetch, causing redundant work; either remove the global fetch block (the try around execGitCommand with FETCH_TIMEOUT_MS) and rely on syncBaseBranch's targeted fetch, or add a parameter to syncBaseBranch (e.g., skipFetch: boolean) and pass skipFetch=true when you keep the top-level fetch; update calls to syncBaseBranch in create.ts (and any other callers) and adjust syncBaseBranch to conditionally skip its internal fetch logic when skipFetch is true, preserving existing logging and error-handling behavior.
🤖 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/lib/settings-helpers.ts`:
- Around line 98-100: The current assignment of result from
globalSettings.defaultMaxTurns uses the nullish coalescing operator but doesn't
guard against NaN, so NaN can flow into Math.floor and produce NaN for clamped;
update the logic around result/clamped (where globalSettings.defaultMaxTurns is
read) to validate the numeric value (e.g., via Number.isFinite) and fallback to
the safe default (1000) when defaultMaxTurns is not a finite number before
applying Math.floor and the Math.max/Math.min clamping; ensure you keep the
clamp bounds (1..2000) and the variable names result and clamped intact so
callers still receive a finite integer maxTurns.
In `@apps/server/src/services/branch-sync-service.ts`:
- Around line 97-107: The fallback that splits an upstream string uses
trimmed.lastIndexOf('/') which incorrectly treats slashed branch names as part
of the remote; update that fallback to use trimmed.indexOf('/') (first slash)
instead so remote = trimmed.substring(0, slashIndex) and remoteBranch =
trimmed.substring(slashIndex + 1) produce remote="origin" and
remoteBranch="feature/foo" for inputs like "origin/feature/foo"; change the
variable slashIndex accordingly (keep the existing guard like if (slashIndex >
0)) and update the surrounding comment to reflect that this fallback favors the
common case of single-name remotes and slash-containing branch names.
- Around line 345-381: isBranchCheckedOut currently only reports a boolean but
callers (like the code in syncBaseBranch using execGitCommand) need the actual
worktree path; change isBranchCheckedOut to return the worktree path string when
checked out (or null otherwise), update its callers, and in this
branch-sync-service code call const worktreePath = await
isBranchCheckedOut(projectPath, branchName) and, if worktreePath is non-null,
run execGitCommand(['merge','--ff-only', remoteRef], worktreePath) (keep using
projectPath for the update-ref path case); ensure all call sites of
isBranchCheckedOut handle the new return type and preserve existing error
handling/logging around buildStaleResult and execGitCommand.
---
Duplicate comments:
In `@apps/server/src/services/agent-executor.ts`:
- Around line 102-112: The log for the "not set" case currently uses logger.info
which may hide a noteworthy missing configuration; change the logger call in the
branch that checks sdkOptions?.maxTurns == null to logger.warn so the absence of
sdkOptions.maxTurns is surfaced (keep the message text and variables like
resolvedMaxTurns, featureId, effectiveBareModel and the surrounding code in
execute intact).
In `@apps/ui/src/store/app-store.ts`:
- Around line 2372-2385: setDefaultMaxTurns now guards against NaN/Infinity by
falling back to 1 and clamping before setting and syncing; no functional change
required, but if you prefer using the previously suggested global default
instead of hardcoded 1, replace the fallback expression in setDefaultMaxTurns
from "Number.isFinite(maxTurns) ? maxTurns : 1" to use
"DEFAULT_GLOBAL_SETTINGS.defaultMaxTurns ?? 1000" (and keep the subsequent
Math.floor and clamp) so the fallback comes from the shared default constant.
---
Nitpick comments:
In `@apps/server/src/lib/settings-helpers.ts`:
- Around line 87-107: Extract the magic numbers in getDefaultMaxTurnsSetting
into named constants (e.g., DEFAULT_MAX_TURNS and MAX_ALLOWED_TURNS) and use
those constants everywhere the function currently uses 1000 and 2000: return
default when settingsService is missing, fallback in result calculation,
clamping Math.max/Math.min, and log messages; define the constants near the top
of the module (and export them if needed for reuse by the UI/store) so all
layers stay in sync and the intent is documented.
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 188-243: The code currently runs a global fetch via
execGitCommand(['fetch','--all','--quiet'], ...) then calls syncBaseBranch which
does its own targeted fetch, causing redundant work; either remove the global
fetch block (the try around execGitCommand with FETCH_TIMEOUT_MS) and rely on
syncBaseBranch's targeted fetch, or add a parameter to syncBaseBranch (e.g.,
skipFetch: boolean) and pass skipFetch=true when you keep the top-level fetch;
update calls to syncBaseBranch in create.ts (and any other callers) and adjust
syncBaseBranch to conditionally skip its internal fetch logic when skipFetch is
true, preserving existing logging and error-handling behavior.
In `@apps/server/src/services/agent-executor.ts`:
- Around line 295-299: The stream-end diagnostic log (using
streamStartTime/streamElapsedMs and logger.info in execute around the
streamLoop) is currently inside the try and thus skipped on exceptions; move
that logging block into a finally block that always runs after the try/catch so
elapsed time, aborted/specDetected/responseLength are always logged, and remove
the original logger.info lines from inside the try; ensure the finally executes
after any catch paths that handle abort/auth/sanitized-provider errors so the
same variables (streamStartTime, responseText, aborted, specDetected) are still
available or captured before the try ends.
- Line 102: Introduce a module-level named constant (e.g., DEFAULT_MAX_TURNS =
1000) and replace the literal fallback 1000 wherever it is used as the max-turns
default; specifically update the expression that computes resolvedMaxTurns
(currently using sdkOptions?.maxTurns ?? 1000) to use the named constant, and
replace the other three occurrences that fall back to 1000 with the same
constant so the default is documented and changed atomically.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 758-762: cleanupAndEmitStop currently calls
this.allocatedPorts.delete(port) using the closure variable; change it to use
the serverInfo object for consistency with other cleanup paths. Specifically,
inside cleanupAndEmitStop replace the closure-captured port usage with
this.allocatedPorts.delete(serverInfo.allocatedPort) and keep
this.runningServers.delete(worktreePath) as-is so all cleanup paths
(stopDevServer, pruneStaleServer, cleanupAndEmitStop) consistently reference
serverInfo.allocatedPort.
- Around line 1013-1043: getServerLogs currently returns entries for servers
whose child process has already exited; mirror the stale-entry pruning used by
isRunning/getServerInfo/listDevServers by checking the server.process (e.g.,
server.process?.killed || server.process?.exitCode != null) after retrieving
server from this.runningServers, and if the process is stale remove the entry
from this.runningServers and return the same failure shape (success: false,
error: `No dev server running for worktree: ${worktreePath}`); otherwise
continue to return the logs (server.scrollbackBuffer) and startedAt as before.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… tracking logic - Add DEFAULT_MAX_TURNS (1000) and MAX_ALLOWED_TURNS (2000) constants to settings-helpers - Replace hardcoded 1000 values with DEFAULT_MAX_TURNS constant throughout codebase - Improve max turns validation with explicit Number.isFinite check - Update getTrackingBranch to split on first slash instead of last for better remote parsing - Change isBranchCheckedOut return type from boolean to string|null to return worktree path - Add comments explaining skipFetch parameter in worktree creation - Fix cleanup order in AgentExecutor finally block to run before logging ```
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/dev-server-service.ts (1)
749-758:⚠️ Potential issue | 🟡 Minor
cleanupAndEmitStopreports the wrong port indev-server:stoppedwhen URL detection has changedserver.port
porthere is the closure variable bound to the allocated port. OncedetectUrlFromOutputmutatesserverInfo.portto the actual detected port (e.g., 3001 allocated → 3000 detected), a natural process exit fires the stop event with the stale allocated value (3001), whilestopDevServerandpruneStaleServerboth deliberately useserver.portto report the externally-visible detected port (note the comment at line 154:// Report the externally-visible (detected) port).🐛 Proposed fix
if (this.emitter && !serverInfo.stopping) { this.emitter.emit('dev-server:stopped', { worktreePath, - port, + port: serverInfo.port, // Report externally-visible (detected) port, consistent with stopDevServer/pruneStaleServer exitCode, error: errorMessage, timestamp: new Date().toISOString(), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 749 - 758, cleanupAndEmitStop emits the stale allocated port (the closure variable port) instead of the externally-visible detected port after detectUrlFromOutput may have mutated serverInfo.port; update the payload passed to this.emitter.emit('dev-server:stopped') to use serverInfo.port (or server.port where serverInfo is the referenced ServerInfo) for the port field so the emitted event matches stopDevServer/pruneStaleServer and reports the detected port.
🧹 Nitpick comments (7)
apps/server/src/services/agent-executor.ts (2)
41-41: ImportDEFAULT_MAX_TURNSfromsettings-helpersinstead of redefining it locally.
settings-helpers.tsalready exports this constant. Keeping a local copy creates two sources of truth; if one drifts they'll silently diverge.♻️ Proposed fix
-import { getPromptCustomization } from '../lib/settings-helpers.js'; +import { getPromptCustomization, DEFAULT_MAX_TURNS } from '../lib/settings-helpers.js';-const DEFAULT_MAX_TURNS = 1000; - export class AgentExecutor {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` at line 41, Replace the locally defined DEFAULT_MAX_TURNS in agent-executor.ts with the shared constant exported from settings-helpers: remove the local "const DEFAULT_MAX_TURNS = 1000;" and add an import for DEFAULT_MAX_TURNS from settings-helpers at the top of the file, leaving all existing usages of DEFAULT_MAX_TURNS in functions/methods unchanged so the module now relies on the single shared source of truth.
686-702:maxTurnsinbuildExecOptsshould be required, not optional.All three call sites now always pass a concrete value via
?? DEFAULT_MAX_TURNS, making the?annotation vestigial. A future caller omitting the argument would silently passundefinedto the provider — the exact regression the maxTurns work aims to prevent.♻️ Proposed fix
- private buildExecOpts(o: AgentExecutionOptions, prompt: string, maxTurns?: number) { + private buildExecOpts(o: AgentExecutionOptions, prompt: string, maxTurns: number) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 686 - 702, The buildExecOpts function currently declares maxTurns as optional but callers always pass a concrete value (via ?? DEFAULT_MAX_TURNS), so change the signature of buildExecOpts to require maxTurns (remove the ?), ensure the returned object sets maxTurns as a number, and update any typings that reference buildExecOpts to reflect the required parameter (e.g., AgentExecutionOptions usages that call buildExecOpts can continue to pass their computed value); this prevents accidental undefined being forwarded to providers.apps/server/src/services/dev-server-service.ts (1)
95-96: Consider markingallocatedPortasreadonlyto enforce its documented immutabilityThe JSDoc already says "never mutated after startDevServer sets it", but there's no compiler-level enforcement.
readonlyprevents accidental mutation in any future code path without changing runtime behavior.♻️ Proposed change
- /** The port originally reserved by findAvailablePort() – never mutated after startDevServer sets it */ - allocatedPort: number; + /** The port originally reserved by findAvailablePort() – never mutated after startDevServer sets it */ + readonly allocatedPort: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/dev-server-service.ts` around lines 95 - 96, The field allocatedPort in the DevServerService (documented as "never mutated after startDevServer sets it") should be declared readonly to enforce immutability at compile time; update the class field declaration for allocatedPort to include the readonly modifier and ensure startDevServer initializes it only once (e.g., in constructor or within startDevServer before any consumers) so TypeScript enforces no further assignments to allocatedPort.apps/server/src/services/branch-sync-service.ts (3)
15-16:FETCH_TIMEOUT_MSis duplicated increate.ts(Line 39).Both modules independently define the same 30-second constant. Extract it to a shared location (e.g., a
constants.tsinlib/) and import from there to keep the timeout in sync across callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/branch-sync-service.ts` around lines 15 - 16, The constant FETCH_TIMEOUT_MS is duplicated across modules (defined in branch-sync-service and create.ts); move the shared value into a single exported constant in a new lib/constants.ts (e.g., export const FETCH_TIMEOUT_MS = 30_000) and update both modules to import FETCH_TIMEOUT_MS from that file (replace local definitions in branch-sync-service and create.ts with the imported symbol) so the timeout is maintained in one place.
347-358:synced: truefor the "ahead" case may mislead consumers.When the local branch is ahead of remote, no sync occurred yet
attempted: true, synced: trueis returned — identical to a successful fast-forward. A consumer that interpretssyncedas "we pulled new commits" will silently misread this state. Consider returningsynced: false(withcanProceedWithStale: true) or a dedicated field likealreadyAhead: trueto distinguish "branch is ahead of remote and no action taken" from "branch was successfully fast-forwarded."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/branch-sync-service.ts` around lines 347 - 358, The current return in the ahead-only branch (inside the if (ahead > 0 && behind === 0) block) incorrectly marks synced: true even though no pull/merge occurred; update the returned object from this branch (where you call execGitCommand(['rev-parse', '--short', branchName], projectPath) and reference tracking.remote and commitHash) to clearly indicate no sync action was performed—for example set synced: false and add either canProceedWithStale: true or alreadyAhead: true (pick one consistent flag used elsewhere) and adjust the message to reflect “local is ahead, no action taken”; ensure consumers still get attempted: true, remote: tracking.remote, and commitHash.trim() unchanged.
213-217:syncBaseBranchdoesn't emit WebSocket events — potential coding guideline violation.
branch-sync-service.tsperforms server-side git operations but never accepts anEventEmitteror emits events to stream progress to the frontend. The route'seventsemitter (available increateCreateHandler) is not passed down. If a sync takes several seconds (e.g., slow remote), the client has no real-time feedback until the HTTP response arrives.Consider accepting an optional
EventEmitterparameter and emitting coarse-grained events (e.g.,sync:start,sync:complete,sync:warn) so the frontend can show live progress.As per coding guidelines: "All server operations should emit events using
createEventEmitter()fromlib/events.tsthat 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/branch-sync-service.ts` around lines 213 - 217, syncBaseBranch currently performs long-running git work without emitting progress events; update its signature to accept an optional EventEmitter (typed to the app's createEventEmitter) and thread the route's events emitter (from createCreateHandler) into calls that invoke syncBaseBranch; inside syncBaseBranch (and helper functions like any fetch/pull wrappers) emit coarse-grained events such as "sync:start" at start, "sync:progress" with brief status messages during long steps (fetch, merge, resolve), "sync:warn" for recoverable issues, "sync:error" on failures, and "sync:complete" on success using the provided emitter; ensure you import and use createEventEmitter's type from lib/events.ts and do null-safe checks before emitting so existing callers without an emitter still work.apps/server/src/routes/worktree/routes/create.ts (1)
205-205:effectiveBaseandbasecompute the same expression — deduplicate.Line 205 declares
effectiveBase = baseBranch || 'HEAD'for the sync path; Line 262 redeclaresbase = baseBranch || 'HEAD'for the worktree add command. They'll silently diverge if the default ever changes.♻️ Proposed fix
- const base = baseBranch || 'HEAD'; - await execGitCommand( - ['worktree', 'add', '-b', branchName, worktreePath, base], + await execGitCommand( + ['worktree', 'add', '-b', branchName, worktreePath, effectiveBase], projectPath );Also applies to: 262-262
🤖 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.ts` at line 205, Rename or consolidate the duplicated base computation so there's a single source of truth: compute const effectiveBase = baseBranch || 'HEAD' once (where currently declared) and remove the later const base = baseBranch || 'HEAD'; then update the worktree add invocation to use effectiveBase instead of base; ensure no other references expect the old name and run tests to confirm behavior remains identical.
🤖 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/services/dev-server-service.ts`:
- Around line 1033-1040: The stale-check in getServerLogs should match the other
methods (isRunning, getServerInfo, listDevServers): remove the extra
server.process.killed branch and only treat a server as stale when the child
process has a numeric exitCode (e.g., typeof server.process.exitCode ===
'number'); update the conditional in getServerLogs (referencing server.process
and worktreePath and calling this.pruneStaleServer(worktreePath, server)) so it
prunes and returns the error only when exitCode is a number, keeping behavior
consistent across all four methods.
---
Outside diff comments:
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 749-758: cleanupAndEmitStop emits the stale allocated port (the
closure variable port) instead of the externally-visible detected port after
detectUrlFromOutput may have mutated serverInfo.port; update the payload passed
to this.emitter.emit('dev-server:stopped') to use serverInfo.port (or
server.port where serverInfo is the referenced ServerInfo) for the port field so
the emitted event matches stopDevServer/pruneStaleServer and reports the
detected port.
---
Duplicate comments:
In `@apps/server/src/services/branch-sync-service.ts`:
- Around line 129-163: The isBranchCheckedOut function has already been
correctly refactored to return the worktree path (string | null) and the
post-loop check in isBranchCheckedOut handles outputs that don’t end with a
blank line; no code changes required—simply remove the duplicate review
tag/comment ([duplicate_comment]) from the PR/review so the approval stands and
callers can use the returned worktree path for git merge --ff-only targeting the
correct worktree.
- Around line 97-109: The fallback parsing now correctly uses
trimmed.indexOf('/') and checks slashIndex > 0 to split on the first slash
(variables: trimmed, slashIndex) so ensure this logic remains and add tests:
verify "origin/feature/foo" yields remote="origin" and
remoteBranch="feature/foo", verify strings with no slash and strings starting
with "/" are handled as expected (i.e., previous git-config lookup behavior or
error branch), and confirm the code returns remote via trimmed.substring(0,
slashIndex) and remoteBranch via trimmed.substring(slashIndex + 1).
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 135-159: pruneStaleServer should fully clean up a stale
DevServerInfo by clearing any pending timers, releasing the reserved slot using
the immutable server.allocatedPort (not server.port), removing the entry from
runningServers, and emitting the "dev-server:stopped" event with worktreePath,
the externally-visible server.port (which may have been mutated by
detectUrlFromOutput), exitCode (server.process?.exitCode ?? null), and a
timestamp; update the implementation of pruneStaleServer to clear
server.flushTimeout and server.urlDetectionTimeout, call
this.allocatedPorts.delete(server.allocatedPort), call
this.runningServers.delete(worktreePath), and if this.emitter exists emit the
described payload so all call sites get a consistent stopped notification.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Line 205: Rename or consolidate the duplicated base computation so there's a
single source of truth: compute const effectiveBase = baseBranch || 'HEAD' once
(where currently declared) and remove the later const base = baseBranch ||
'HEAD'; then update the worktree add invocation to use effectiveBase instead of
base; ensure no other references expect the old name and run tests to confirm
behavior remains identical.
In `@apps/server/src/services/agent-executor.ts`:
- Line 41: Replace the locally defined DEFAULT_MAX_TURNS in agent-executor.ts
with the shared constant exported from settings-helpers: remove the local "const
DEFAULT_MAX_TURNS = 1000;" and add an import for DEFAULT_MAX_TURNS from
settings-helpers at the top of the file, leaving all existing usages of
DEFAULT_MAX_TURNS in functions/methods unchanged so the module now relies on the
single shared source of truth.
- Around line 686-702: The buildExecOpts function currently declares maxTurns as
optional but callers always pass a concrete value (via ?? DEFAULT_MAX_TURNS), so
change the signature of buildExecOpts to require maxTurns (remove the ?), ensure
the returned object sets maxTurns as a number, and update any typings that
reference buildExecOpts to reflect the required parameter (e.g.,
AgentExecutionOptions usages that call buildExecOpts can continue to pass their
computed value); this prevents accidental undefined being forwarded to
providers.
In `@apps/server/src/services/branch-sync-service.ts`:
- Around line 15-16: The constant FETCH_TIMEOUT_MS is duplicated across modules
(defined in branch-sync-service and create.ts); move the shared value into a
single exported constant in a new lib/constants.ts (e.g., export const
FETCH_TIMEOUT_MS = 30_000) and update both modules to import FETCH_TIMEOUT_MS
from that file (replace local definitions in branch-sync-service and create.ts
with the imported symbol) so the timeout is maintained in one place.
- Around line 347-358: The current return in the ahead-only branch (inside the
if (ahead > 0 && behind === 0) block) incorrectly marks synced: true even though
no pull/merge occurred; update the returned object from this branch (where you
call execGitCommand(['rev-parse', '--short', branchName], projectPath) and
reference tracking.remote and commitHash) to clearly indicate no sync action was
performed—for example set synced: false and add either canProceedWithStale: true
or alreadyAhead: true (pick one consistent flag used elsewhere) and adjust the
message to reflect “local is ahead, no action taken”; ensure consumers still get
attempted: true, remote: tracking.remote, and commitHash.trim() unchanged.
- Around line 213-217: syncBaseBranch currently performs long-running git work
without emitting progress events; update its signature to accept an optional
EventEmitter (typed to the app's createEventEmitter) and thread the route's
events emitter (from createCreateHandler) into calls that invoke syncBaseBranch;
inside syncBaseBranch (and helper functions like any fetch/pull wrappers) emit
coarse-grained events such as "sync:start" at start, "sync:progress" with brief
status messages during long steps (fetch, merge, resolve), "sync:warn" for
recoverable issues, "sync:error" on failures, and "sync:complete" on success
using the provided emitter; ensure you import and use createEventEmitter's type
from lib/events.ts and do null-safe checks before emitting so existing callers
without an emitter still work.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 95-96: The field allocatedPort in the DevServerService (documented
as "never mutated after startDevServer sets it") should be declared readonly to
enforce immutability at compile time; update the class field declaration for
allocatedPort to include the readonly modifier and ensure startDevServer
initializes it only once (e.g., in constructor or within startDevServer before
any consumers) so TypeScript enforces no further assignments to allocatedPort.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx (1)
858-867: Addaria-labelto the icon-only refresh button for screen-reader accessibility.The button contains only an SVG icon.
titlegives a hover tooltip but isn't reliably announced by assistive technology.♿ Suggested fix
<Button variant="ghost" size="sm" className="h-7 w-7 p-0 shrink-0" onClick={() => refetch()} disabled={refreshing} title="Refresh comments" + aria-label="Refresh comments" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 858 - 867, The refresh Button (the Button element rendering RefreshCw with onClick={() => refetch()} and disabled={refreshing}) is icon-only and needs an accessible name: add an aria-label (for example aria-label="Refresh comments" or "Refresh PR comments") to the Button so screen readers announce its purpose; keep the existing title if desired but ensure the aria-label string describes the action and matches the RefreshCw usage and the refreshing state.apps/server/src/services/agent-executor.ts (1)
41-41: ImportDEFAULT_MAX_TURNSfromsettings-helpersinstead of redefining it.
apps/server/src/lib/settings-helpers.tsalready exportsDEFAULT_MAX_TURNS = 1000. Redefining it locally creates two sources of truth — if the value is updated insettings-helpers.ts, this file silently diverges.♻️ Proposed fix
-import { getPromptCustomization } from '../lib/settings-helpers.js'; +import { getPromptCustomization, DEFAULT_MAX_TURNS } from '../lib/settings-helpers.js';-const DEFAULT_MAX_TURNS = 1000; - export class AgentExecutor {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` at line 41, The file defines a local constant DEFAULT_MAX_TURNS which duplicates the value exported from settings-helpers; remove the local definition and import DEFAULT_MAX_TURNS from apps/server/src/lib/settings-helpers instead so there is a single source of truth — update the top-of-file imports to include DEFAULT_MAX_TURNS and delete the const DEFAULT_MAX_TURNS = 1000; declaration in agent-executor.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 1033-1040: The stale-process check in getServerLogs uses
"server.process.killed || server.process.exitCode != null", which is
inconsistent with the other methods (isRunning, getServerInfo, listDevServers)
that treat staleness as "typeof server.process.exitCode === 'number'"; update
getServerLogs to use the same predicate (typeof server.process.exitCode ===
'number') when deciding to call pruneStaleServer(worktreePath, server) and
return the error, so all methods treat exitCode-based staleness identically and
avoid transient inconsistencies.
---
Nitpick comments:
In `@apps/server/src/services/agent-executor.ts`:
- Line 41: The file defines a local constant DEFAULT_MAX_TURNS which duplicates
the value exported from settings-helpers; remove the local definition and import
DEFAULT_MAX_TURNS from apps/server/src/lib/settings-helpers instead so there is
a single source of truth — update the top-of-file imports to include
DEFAULT_MAX_TURNS and delete the const DEFAULT_MAX_TURNS = 1000; declaration in
agent-executor.ts.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 858-867: The refresh Button (the Button element rendering
RefreshCw with onClick={() => refetch()} and disabled={refreshing}) is icon-only
and needs an accessible name: add an aria-label (for example aria-label="Refresh
comments" or "Refresh PR comments") to the Button so screen readers announce its
purpose; keep the existing title if desired but ensure the aria-label string
describes the action and matches the RefreshCw usage and the refreshing state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Bug Fixes
Auto-mode event emission (
execution-service.ts,pipeline-orchestrator.ts,auto-mode/facade.ts)auto_mode_feature_completeevents were unconditionally emitted even for non-auto-mode executions, causing spurious UI state updatesisAutoModeflag (fromconcurrencyManager.getRunningFeature()) before emitting at 6+ call sites across execution and pipeline orchestrationDev server state desync (
dev-server-service.ts,use-dev-servers.ts)urlDetectionTimeoutfallback that emits the allocated port URL if no URL pattern is matched from output; stale process entries are now pruned inlistDevServers()by checkingprocess.exitCodeReact re-render loop (
__root.tsx,board-view.tsx,use-auto-mode.ts,use-project-settings-loader.ts,use-worktrees.ts)useAppStore()subscriptions with targeted selectors in__root.tsxandboard-view.tsxsetState()calls into single atomic update inuse-project-settings-loader.tsbranchNameuseMemo dependencies inuse-auto-mode.tsto use primitives instead of object referencesuse-worktrees.tsto skip store updates when worktree data is structurally unchangedEvent hook context leaking (
event-hook-service.ts)erroranderrorTypefields were being populated in hook contexts for non-error triggersfeature_errorandauto_mode_errortriggersGitHub PRs view API namespace (
github-prs-view.tsx)api.features?.run()→api.autoMode?.runFeature()to match correct API namespaceNew Features
Base branch sync on worktree creation (
routes/worktree/routes/create.ts,create-worktree-dialog.tsx)git update-reffor non-checked-out,git merge --ff-onlyfor checked-out)syncResultin response withsynced,remote,message,divergedfieldsTerminal color customization (
terminal-section.tsx,terminal-panel.tsx,terminal-themes.ts,terminal-theme-colors.ts)#RRGGBB) with color picker and reset-to-default buttongetTerminalThemeWithOverride()applies overrides onto the base theme; background color also applied to containerbackgroundColorstylecustomBackgroundColorandcustomForegroundColorfields inTerminalState; defaults tonull(uses theme default)maxTurns observability (
agent-executor.ts,codex-provider.ts)maxTurnsvalue at execution time; warns if undefined (provider will use its own internal default)Settings Sync Fixes (
use-settings-sync.ts,use-settings-migration.ts)Several settings were stored in the Zustand store but never persisted to or hydrated from the server:
claudeCompatibleProvidersSETTINGS_FIELDS_TO_SYNCenableAiCommitMessagesSETTINGS_FIELDS_TO_SYNC,hydrateStoreFromSettings,buildSettingsUpdateFromStoreenableSkills/skillsSourcesenableSubagents/subagentsSourcesAll five fields now round-trip correctly: loaded from server on app startup and included in debounced auto-sync.
Tests
execution-service.test.ts: Added tests verifyingfeature_completeis not emitted whenisAutoMode=false; updated mockacquire()to returnisAutoModepipeline-orchestrator.test.ts: Added tests for each pipeline step verifying no auto-mode event emission whenisAutoMode=false; updated existing tests with mock setup forgetRunningFeature()Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Performance
Tests