Fix concurrency limits and remote branch fetching issues#788
Merged
gsxdsm merged 14 commits intoAutoMaker-Org:v0.15.0rcfrom Feb 20, 2026
Merged
Fix concurrency limits and remote branch fetching issues#788gsxdsm merged 14 commits intoAutoMaker-Org:v0.15.0rcfrom
gsxdsm merged 14 commits intoAutoMaker-Org:v0.15.0rcfrom
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:
📝 WalkthroughWalkthroughManual feature starts no longer perform per-worktree capacity checks; running counts are unified with an optional auto-only filter. Git remotes are pre-fetched with a 30s timeout before create/checkout/switch operations. Dev-server URL detection and a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RunFeatureRoute as RunFeatureRoute
participant Background as BackgroundExecutor
participant AutoLoop as AutoLoopCoordinator
participant Concurrency as ConcurrencyManager
Client->>RunFeatureRoute: POST /auto-mode/run-feature (manual start)
RunFeatureRoute->>Background: start background execution (no per-worktree capacity check)
RunFeatureRoute-->>Client: 200 OK
Background->>AutoLoop: register running feature
AutoLoop->>Concurrency: getRunningCountForWorktree(projectPath, branch, {autoModeOnly: true})
Concurrency-->>AutoLoop: return counts
AutoLoop->>AutoLoop: regulate future auto dispatches based on counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 refines the system's handling of concurrent operations and remote Git interactions, improving both reliability and user experience. It ensures that manual tasks are not unnecessarily blocked by concurrency limits while maintaining overall system stability. Additionally, it strengthens Git operations by guaranteeing up-to-date remote branch information, and enhances the UI's responsiveness to the real-time state of running tasks, providing a more consistent and informative display. 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
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 effectively addresses concurrency limits for manual feature starts and improves remote branch fetching logic. The changes ensure that manual tasks are not blocked by auto-mode limits while still contributing to overall system load calculations. Remote fetching before branch operations enhances accuracy, and the updated auto-mode failure tracking now accounts for all active worktrees. The UI changes provide better visual feedback for running tasks, including those with stale statuses, and the expanded URL detection in the dev server service is a valuable enhancement. The new E2E test for running task card display is a good addition to ensure UI consistency.
| projectPath: string, | ||
| branchName: string | null | ||
| branchName: string | null, | ||
| options?: { autoModeOnly?: boolean } |
Contributor
There was a problem hiding this comment.
The options parameter is passed directly to concurrencyManager.getRunningCountForWorktree. However, the options object is defined as { autoModeOnly?: boolean } in the getRunningCountForWorktree method signature, but the comment for autoModeOnly states "The auto-loop coordinator now counts ALL running features (not just auto-mode) to ensure total system load is respected." This creates a contradiction. If the coordinator is always counting all features, then passing options with autoModeOnly: true would lead to incorrect behavior or the option is unused by the coordinator itself. It should either be removed from the coordinator's call or clarified why it's passed if it's not intended to filter.
return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName);
Comment on lines
29
to
32
| // Note: No concurrency limit check here. Manual feature starts always run | ||
| // immediately and bypass the concurrency limit. However, their presence IS | ||
| // counted by the auto-loop coordinator when deciding whether to dispatch | ||
| // new auto-mode tasks, ensuring total system load is respected. |
Contributor
There was a problem hiding this comment.
The comment accurately describes the new behavior, but it could be slightly more concise. The phrase "ensuring total system load is respected" is a bit redundant given the previous sentence. It's also good practice to keep comments focused on what the code does, rather than why (which should be in the PR description or design docs).
Suggested change
| // Note: No concurrency limit check here. Manual feature starts always run | |
| // immediately and bypass the concurrency limit. However, their presence IS | |
| // counted by the auto-loop coordinator when deciding whether to dispatch | |
| // new auto-mode tasks, ensuring total system load is respected. | |
| // Note: No concurrency limit check here. Manual feature starts always run | |
| // immediately and bypass the concurrency limit. Their presence IS counted | |
| // by the auto-loop coordinator when deciding whether to dispatch new auto-mode tasks. |
Comment on lines
42
to
47
| } catch (error) { | ||
| if (error instanceof Error && error.message === 'Process aborted') { | ||
| logger.warn( | ||
| `fetchRemotes timed out after ${FETCH_TIMEOUT_MS}ms - continuing without latest remote refs` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
The error message for a timed-out fetch operation is quite verbose. It could be shortened to improve readability in logs while still conveying the essential information.
if (error instanceof Error && error.message === 'Process aborted') {
logger.warn(`fetchRemotes timed out after ${FETCH_TIMEOUT_MS}ms. Continuing with local refs.`);
}| logger.info('Fetching from all remotes before creating worktree'); | ||
| try { | ||
| const controller = new AbortController(); | ||
| const timerId = setTimeout(() => controller.abort(), 30_000); |
Contributor
There was a problem hiding this comment.
The hardcoded timeout of 30_000 milliseconds (30 seconds) for the git fetch operation is duplicated here. It would be better to define this as a constant, perhaps at the top of the file or in a shared utility, to improve maintainability and consistency.
Suggested change
| const timerId = setTimeout(() => controller.abort(), 30_000); | |
| const timerId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); |
| private stripAnsi(str: string): string { | ||
| // Matches ANSI escape sequences: CSI sequences, OSC sequences, and simple escapes | ||
| // eslint-disable-next-line no-control-regex | ||
| return str.replace(/\x1B(?:\[[0-9;]*[a-zA-Z]|\].*?(?:\x07|\x1B\\)|\[[\?]?[0-9;]*[hl])/g, ''); |
Contributor
There was a problem hiding this comment.
The regular expression for stripping ANSI escape codes (/\x1B(?:...)/g) is missing the u flag for Unicode support. While it might work for common ANSI sequences, it's best practice to include the u flag when dealing with strings that might contain Unicode characters to ensure correct behavior across all possible inputs.
Suggested change
| return str.replace(/\x1B(?:\[[0-9;]*[a-zA-Z]|\].*?(?:\x07|\x1B\\)|\[[\?]?[0-9;]*[hl])/g, ''); | |
| return str.replace(/\x1B(?:\[[0-9;]*[a-zA-Z]|\]\S*?(?:\x07|\x1B\\)|\(\[0-9;]*[hl])/gu, ''); |
| // Localhost URL with port (conservative - must be localhost/127.0.0.1/[::]/0.0.0.0) | ||
| // This catches anything that looks like a dev server URL | ||
| { | ||
| pattern: /(https?:\/\/(?:localhost|127\.0\.0\.1|\[::\]|0\.0\.0\.0):\d+\S*)/i, |
Contributor
There was a problem hiding this comment.
The regular expression /(https?:\/\/(?:localhost|127\.0\.0\.1|\[::\]|0\.0\.0\.0):\d+\S*)/i is quite broad and might capture unintended parts of a URL if there are special characters immediately following the port number. While \S* matches non-whitespace characters, it could include punctuation that is not part of the URL. It would be safer to explicitly exclude common trailing punctuation marks or use a more precise character set.
Suggested change
| pattern: /(https?:\/\/(?:localhost|127\.0\.0\.1|\[::\]|0\.0\.0\.0):\d+\S*)/i, | |
| pattern: /(https?:\/\/(?:localhost|127\.0\.0\.1|\[::\]|0\.0\.0\.0):\d+[^\s,)]*)/i, |
Comment on lines
263
to
265
| pattern: /(?:^|\s)port[:\s]+(\d{4,5})(?:\s|$|[.,;])/im, | ||
| description: '"port:" format', | ||
| }, |
Contributor
There was a problem hiding this comment.
The regular expression /(?:^|\s)port[:\s]+(\d{4,5})(?:\s|$|[.,;])/im for detecting port numbers is generally good, but the (?:\s|$|[.,;]) part at the end could be simplified. The [.,;] part is already covered by \s if it's followed by a space, and if it's at the end of the line, $ covers it. A more concise and equally effective pattern would be (?:\s|$|[.,;]) to (?:\s|$|[,.;]) or even (?:\s|$|\W) if the intent is to match any non-word character as a delimiter.
Suggested change
| pattern: /(?:^|\s)port[:\s]+(\d{4,5})(?:\s|$|[.,;])/im, | |
| description: '"port:" format', | |
| }, | |
| pattern: /(?:^|\s)port[:\s]+(\d{4,5})(?:\s|$|[,.;])/im, |
Comment on lines
+380
to
+390
| const colId = String(collision.id); | ||
| // Direct column ID match (e.g. 'backlog', 'in_progress') | ||
| if (COLUMNS.some((col) => col.id === colId)) return true; | ||
| // Column header droppable (e.g. 'column-header-backlog') | ||
| if (colId.startsWith('column-header-')) { | ||
| const baseId = colId.replace('column-header-', ''); | ||
| return COLUMNS.some((col) => col.id === baseId) || baseId.startsWith('pipeline_'); | ||
| } | ||
| // Pipeline column IDs (e.g. 'pipeline_tests') | ||
| if (colId.startsWith('pipeline_')) return true; | ||
| return false; |
Contributor
There was a problem hiding this comment.
The logic for identifying column collisions is a bit verbose. It can be simplified by creating a helper function or a more streamlined conditional check to reduce redundancy and improve readability.
const columnCollisions = pointerCollisions.filter((collision: Collision) => {
const colId = String(collision.id);
const isColumn = COLUMNS.some((col) => col.id === colId);
const isPipeline = colId.startsWith('pipeline_');
const isHeader = colId.startsWith('column-header-');
if (isColumn || isPipeline) return true;
if (isHeader) {
const baseId = colId.replace('column-header-', '');
return COLUMNS.some((col) => col.id === baseId) || baseId.startsWith('pipeline_');
}
return false;
});
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/server/src/routes/worktree/routes/checkout-branch.ts (1)
32-52: Consider moving fetchRemotes into a service/shared git helper.This keeps the route thin and avoids git business logic living in routes.
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/checkout-branch.ts` around lines 32 - 52, Move the fetchRemotes logic out of the route and into a reusable git service/helper: create a function (e.g., fetchRemotes) in a new or existing service under services/ (or services/gitHelper) that accepts cwd and uses AbortController, FETCH_TIMEOUT_MS and execGitCommand exactly as implemented; then update the route handler to import and call that service function (keeping the route thin and only handling request/response concerns). Ensure the service exports the async fetchRemotes(cwd: string): Promise<void> and preserve the same timeout, logging behavior (logger.warn), and swallowed-error semantics so existing behavior is unchanged.apps/server/src/routes/worktree/routes/create.ts (1)
174-191: Move remote-fetch orchestration into a service and emit an event.This adds git-fetch behavior directly in the route; consider delegating to a service (or a shared fetch helper) and emitting a fetch event so the UI can observe the operation. As per coding guidelines: “Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services” and “All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket”.
🤖 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/checkout-branch.ts`:
- Around line 40-48: The current catch for execGitCommand(['fetch'...]) only
logs a timeout case and swallows other errors; update the catch in
checkout-branch.ts so that if error is a timeout (error.message === 'Process
aborted') you keep the existing logger.warn about FETCH_TIMEOUT_MS, otherwise
log the error (include error.message/stack) with logger.error or logger.warn to
surface auth/remote failures; reference execGitCommand, FETCH_TIMEOUT_MS and
logger.warn/logger.error in your change and ensure non-timeout errors are not
swallowed while preserving the timeout behavior.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts`:
- Around line 49-67: The handler registered with
api.worktree.onDevServerLogEvent currently calls toast.success before confirming
the event applies to a known worktree; move or gate the toast so it only fires
when the Map update actually happens. Inside the onDevServerLogEvent callback
(the function passed to api.worktree.onDevServerLogEvent), compute key =
normalizePath(worktreePath), check whether setRunningDevServers' prev.get(key)
(or existing) is truthy, and only call toast.success after you’ve
confirmed/updated the Map in setRunningDevServers (or after an early check that
existing is present). This change touches the onDevServerLogEvent callback,
setRunningDevServers usage and normalizePath/ toast.success invocation.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/checkout-branch.ts`:
- Around line 32-52: Move the fetchRemotes logic out of the route and into a
reusable git service/helper: create a function (e.g., fetchRemotes) in a new or
existing service under services/ (or services/gitHelper) that accepts cwd and
uses AbortController, FETCH_TIMEOUT_MS and execGitCommand exactly as
implemented; then update the route handler to import and call that service
function (keeping the route thin and only handling request/response concerns).
Ensure the service exports the async fetchRemotes(cwd: string): Promise<void>
and preserve the same timeout, logging behavior (logger.warn), and
swallowed-error semantics so existing behavior is unchanged.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
361-372:⚠️ Potential issue | 🟡 Minor"Open in Browser" should be disabled while
urlDetected === false.The label correctly says "Dev Server Starting..." but the menu item remains enabled. When
urlDetectedis false the stored URL/port may be a placeholder (consistent with the "Dev server started, detecting port..." toast), so clicking it would open a broken page in the browser.🛡️ Proposed fix
<DropdownMenuItem onClick={() => onOpenDevServerUrl(worktree)} className="text-xs" + disabled={devServerInfo?.urlDetected === false} aria-label={ devServerInfo?.urlDetected === false ? 'Open dev server in browser' : `Open dev server on port ${devServerInfo?.port} in browser` } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx` around lines 361 - 372, The "Open in Browser" DropdownMenuItem remains clickable even when devServerInfo?.urlDetected === false; disable it and prevent clicks until a real URL is detected. Update the DropdownMenuItem (the element rendering "Open in Browser") to set disabled={devServerInfo?.urlDetected === false} and conditionally avoid calling onOpenDevServerUrl(worktree) when devServerInfo?.urlDetected === false (e.g., change the onClick to a noop or guard it before invoking onOpenDevServerUrl). Ensure the aria-label and visual state still reflect the "Dev Server Starting..." status by relying on devServerInfo?.urlDetected for both disabled and label logic.
🧹 Nitpick comments (9)
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (1)
97-99: Remove unused destructures fromuseAppStore()
getAutoModeStateandgetMaxConcurrencyForWorktreeare destructured but never referenced in this file.Suggested cleanup
const { addFeature, updateFeature, removeFeature, moveFeature, useWorktrees, enableDependencyBlocking, skipVerificationInAutoMode, isPrimaryWorktreeBranch, getPrimaryWorktreeBranch, - getAutoModeState, - getMaxConcurrencyForWorktree, } = useAppStore();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts` around lines 97 - 99, The destructured values getAutoModeState and getMaxConcurrencyForWorktree are unused in use-board-actions.ts; remove them from the call-site where useAppStore() is destructured to eliminate dead variables (leave the other destructured symbols as-is), ensuring no references to getAutoModeState or getMaxConcurrencyForWorktree remain in the file after the change.apps/server/src/services/worktree-branch-service.ts (1)
64-83: Usecontroller.signal.abortedinstead of string-matching to detect timeouts.The current code checks
error.message === 'Process aborted'to identify timeout errors. While this works today becausespawnProcessexplicitly rejects with that message, it's fragile—if the error message inlibs/platform/src/subprocess.tsever changes, the timeout will silently fall through to the generic error path without specific logging.The
AbortControllersignal is marked aborted synchronously whencontroller.abort()is called, so by the time the error is caught,controller.signal.abortedwill be true. This approach is both more resilient and aligns with the pattern already used throughout the codebase (e.g.,auto-loop-coordinator.ts,pipeline-orchestrator.ts,execution-service.ts).Suggested change
- if (error instanceof Error && error.message === 'Process aborted') { + if (controller.signal.aborted) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/worktree-branch-service.ts` around lines 64 - 83, The fetchRemotes function relies on brittle string-matching of error.message === 'Process aborted' to detect timeouts; instead, detect the timeout by checking the AbortController signal (controller.signal.aborted) in the catch block and log the timeout path when true (include FETCH_TIMEOUT_MS in the message), otherwise ignore other fetch errors as before; modify the catch in fetchRemotes (which calls execGitCommand) to use controller.signal.aborted rather than comparing error.message.apps/server/tests/unit/services/dev-server-service.test.ts (1)
890-899: Consider adding apidproperty to the mock process.Some
ChildProcessconsumers check.pidto verify process liveness. If any future code path inDevServerServicecheckspid, these tests could break silently.Proposed enhancement
function createMockProcess() { const mockProcess = new EventEmitter() as any; mockProcess.stdout = new EventEmitter(); mockProcess.stderr = new EventEmitter(); mockProcess.kill = vi.fn(); mockProcess.killed = false; + mockProcess.pid = 12345; // Don't exit immediately - let the test control the lifecycle return mockProcess; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/dev-server-service.test.ts` around lines 890 - 899, The mock process created by createMockProcess is missing a pid property which some ChildProcess consumers (e.g., DevServerService code paths) may check to verify liveness; add a numeric pid field to the returned mockProcess (e.g., mockProcess.pid = 12345 or make it configurable) and ensure any tests that assert kill/killed behavior account for pid presence so future checks against process.pid won't break silently.apps/server/src/services/dev-server-service.ts (1)
220-223: Normalization regexes require an explicit port —http://0.0.0.0/would not be normalized.The patterns
/\/\/0\.0\.0\.0(:\d+)/and/\/\/\[::\](:\d+)/both require:\d+to match. A URL likehttp://0.0.0.0/(no port) would slip through un-normalized. This is acceptable since dev servers virtually always specify a port, but adding an optional port group would be more robust.Optional: handle port-less URLs
- detectedUrl = detectedUrl.replace(/\/\/0\.0\.0\.0(:\d+)/, '//localhost$1'); - detectedUrl = detectedUrl.replace(/\/\/\[::\](:\d+)/, '//localhost$1'); + detectedUrl = detectedUrl.replace(/\/\/0\.0\.0\.0(:\d+)?/, '//localhost$1'); + detectedUrl = detectedUrl.replace(/\/\/\[::\](:\d+)?/, '//localhost$1');Note: When
(:\d+)?doesn't match,$1isundefined, whichString.replacerenders as the literal string"undefined". Use a function replacement instead:+ detectedUrl = detectedUrl.replace(/\/\/0\.0\.0\.0(:\d+)?/, (_, port) => `//localhost${port || ''}`); + detectedUrl = detectedUrl.replace(/\/\/\[::\](:\d+)?/, (_, port) => `//localhost${port || ''}`);🤖 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 220 - 223, The two normalization replaces on detectedUrl currently require an explicit port and, if you make the port optional, a missing capture will become "undefined" in the replacement; update both calls that reference detectedUrl.replace(/\/\/0\.0\.0\.0(:\d+)/, ...) and detectedUrl.replace(/\/\/\[::\](:\d+)/, ...) to accept an optional port (e.g. (:\d+)? ) and use a function replacer instead of a replacement string so you can return `//localhost` + (port || '') — this ensures URLs like http://0.0.0.0/ and http://[::]/ are normalized correctly without injecting "undefined".apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (1)
346-394: Stale-status running task handling correctly prevents Make button flash.The new block correctly intercepts the
backlog/interrupted/readystatuses whenisRunningTaskis true, showing Logs/Stop instead of Make. The logic correctly gates on!isCurrentAutoTaskto avoid conflicts with the actively-running path.However, the Logs/Stop button JSX (lines 354-392) is nearly identical to the
isCurrentAutoTaskblock (lines 83-121). Consider extracting a sharedRunningTaskActionscomponent to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx` around lines 346 - 394, Extract the duplicated JSX for Logs/Stop into a new presentational component (e.g., RunningTaskActions) and reuse it in both the isCurrentAutoTask and !isCurrentAutoTask && isRunningTask branches; the new component should accept props used in the JSX (onViewOutput, onForceStop, shortcutKey, feature.id, className variants) and preserve the existing event handlers (stopPropagation for onClick and onPointerDown) and data-testid attributes (view-output-<id>, force-stop-<id>, shortcut-key-<id>) so the UI and tests remain unchanged, then replace the duplicated blocks in card-actions.tsx with <RunningTaskActions ... /> calls.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts (1)
26-32:fetchDevServersuses un-normalized path as map key; all other operations normalize.
handleStartDevServer(line 99),handleStopDevServer(line 136), the new URL-detected handler (line 54), and both lookup helpers (isDevServerRunning/getDevServerInfoviagetWorktreeKey) all callnormalizePath. OnlyfetchDevServersdoes not. On Windows (case-folding, backslash vs forward-slash) or when symlinks are involved, servers populated byfetchDevServersbecome invisible to every lookup, soisDevServerRunningwould returnfalsefor an already-running server on startup.♻️ Proposed fix
- serversMap.set(server.worktreePath, { + serversMap.set(normalizePath(server.worktreePath), { ...server, urlDetected: server.urlDetected ?? true, });🤖 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 26 - 32, fetchDevServers populates serversMap using the raw server.worktreePath which is inconsistent with other code that calls normalizePath (e.g., getWorktreeKey, isDevServerRunning, getDevServerInfo, handleStartDevServer, handleStopDevServer); update the loop in fetchDevServers to call normalizePath(server.worktreePath) when creating the Map key (and keep the stored DevServerInfo unchanged except for urlDetected defaulting) so lookups use the same normalized keys everywhere.apps/ui/src/lib/http-api-client.ts (1)
580-606:DevServerUrlDetectedEventis structurally identical toDevServerStartedEvent— consider a shared base.Both interfaces carry the same four fields (
worktreePath,url,port,timestamp). If the shape drifts (e.g. adding aprojectPath), the two will need parallel updates.♻️ Optional: extract a shared base interface
+interface DevServerUrlEvent { + worktreePath: string; + url: string; + port: number; + timestamp: string; +} + -export interface DevServerStartedEvent { - worktreePath: string; - port: number; - url: string; - timestamp: string; -} +export interface DevServerStartedEvent extends DevServerUrlEvent {} // ... -export interface DevServerUrlDetectedEvent { - worktreePath: string; - url: string; - port: number; - timestamp: string; -} +export interface DevServerUrlDetectedEvent extends DevServerUrlEvent {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/http-api-client.ts` around lines 580 - 606, DevServerStartedEvent and DevServerUrlDetectedEvent share the same fields; extract a shared base interface (e.g., DevServerEventBase with worktreePath, url, port, timestamp) and have both DevServerStartedEvent and DevServerUrlDetectedEvent extend that base; update any references/usages of those interfaces to rely on the specific names where needed and ensure imports/exports are adjusted so consumers still get the specific types.apps/server/src/services/auto-mode/facade.ts (1)
360-408: Consider extracting the repeated "active-worktrees → filter → fallback → iterate" pattern.All three callbacks (lines 360–384, 385–395, 396–408) share the same boilerplate:
const activeWorktrees = autoLoopCoordinator.getActiveWorktrees(); const projectWorktrees = activeWorktrees.filter((w) => w.projectPath === projectPath); if (projectWorktrees.length === 0) { /* fallback to null */ } for (const w of projectWorktrees) { /* call per-worktree fn */ }Extracting a helper keeps each callback focused on its specific method call and reduces the surface area for copy-paste bugs (like the
.some()issue above).♻️ Suggested helper
/** * Iterate all active worktrees for this project, falling back to the main * worktree (null) when none are active. */ const forEachProjectWorktree = ( fn: (branchName: string | null) => void ): void => { const projectWorktrees = autoLoopCoordinator .getActiveWorktrees() .filter((w) => w.projectPath === projectPath); if (projectWorktrees.length === 0) { fn(null); } else { for (const w of projectWorktrees) { fn(w.branchName); } } }; // Then the three callbacks become: (errorInfo) => { let shouldPause = false; forEachProjectWorktree((branchName) => { if (autoLoopCoordinator.trackFailureAndCheckPauseForProject(projectPath, branchName, errorInfo)) { shouldPause = true; } }); return shouldPause; }, (errorInfo) => { forEachProjectWorktree((branchName) => autoLoopCoordinator.signalShouldPauseForProject(projectPath, branchName, errorInfo) ); }, () => { forEachProjectWorktree((branchName) => autoLoopCoordinator.recordSuccessForProject(projectPath, branchName) ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 360 - 408, Extract the repeated "getActiveWorktrees → filter by projectPath → fallback to null → iterate" logic into a helper (e.g. forEachProjectWorktree) and replace the three inline callbacks that call autoLoopCoordinator.trackFailureAndCheckPauseForProject, autoLoopCoordinator.signalShouldPauseForProject, and autoLoopCoordinator.recordSuccessForProject with calls into that helper; the helper should accept a callback (branchName: string | null) and invoke it for each branchName or once with null when no active worktrees exist, and the first callback should use a local flag to accumulate any true return from trackFailureAndCheckPauseForProject and return that value.apps/server/src/routes/worktree/routes/create.ts (1)
174-191: Extract the fetch-with-timeout pattern to a shared utility.The same
AbortController+execGitCommand(['fetch', '--all', '--quiet'])+ 30 s timeout pattern is now implemented in three places:
create.ts— inline (lines 180–191)checkout-branch.ts—fetchRemotes()functioncheckout-branch-service.ts—fetchRemotes()functionThis is also a good candidate for
@automaker/git-utilsper the project's git-operations guideline. Additionally, the magic literal30_000on line 182 should align with the namedFETCH_TIMEOUT_MSconstant used by the other two files.♻️ Suggested approach
Add to
@automaker/git-utils(or a shared internal module):// packages/git-utils/src/fetch-remotes.ts export const FETCH_TIMEOUT_MS = 30_000; export async function fetchAllRemotes( cwd: string, logger?: { warn: (msg: string) => void } ): Promise<void> { const controller = new AbortController(); const timerId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); try { await execGitCommand(['fetch', '--all', '--quiet'], cwd, undefined, controller); } catch (err) { logger?.warn(`fetchAllRemotes failed: ${getErrorMessage(err)}`); } finally { clearTimeout(timerId); } }Then replace the three inline/local implementations with a single import.
Based on learnings: "Git operations for feature execution should use automaker/git-utils to create and manage isolated git worktrees for each feature."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/create.ts` around lines 174 - 191, The fetch-with-timeout AbortController pattern is duplicated; extract it into a shared utility (e.g. add FETCH_TIMEOUT_MS and fetchAllRemotes to `@automaker/git-utils`) that calls execGitCommand(['fetch','--all','--quiet'], cwd, undefined, controller) with a 30_000ms timeout and centralized error handling/logging; then replace the inline pattern in create.ts (the try/AbortController block), the fetchRemotes functions in checkout-branch.ts and checkout-branch-service.ts to call the new fetchAllRemotes(cwd, logger) and remove the local FETCH_TIMEOUT_MS magic literal so all usages reference the shared FETCH_TIMEOUT_MS and fetchAllRemotes helper.
🤖 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/auto-mode/facade.ts`:
- Around line 374-383: The use of Array.prototype.some causes
trackFailureAndCheckPauseForProject to short-circuit and skip recording failures
for later worktrees; change the logic in the block using projectWorktrees and
autoLoopCoordinator.trackFailureAndCheckPauseForProject so that you iterate over
all projectWorktrees (e.g., with for...of), call
trackFailureAndCheckPauseForProject(projectPath, w.branchName, errorInfo) for
every worktree to ensure failures are recorded, and accumulate a boolean
(shouldPause) that becomes true if any call returns true, then return that
boolean—matching the pattern used by signalShouldPauseForProject and
recordSuccessForProject.
In `@apps/server/src/services/checkout-branch-service.ts`:
- Around line 54-64: The catch currently swallows all fetch errors except
timeouts; update the fetchRemotes error handling so non-timeout failures are
logged (including the error object/message and context) before continuing. In
the catch that checks error instanceof Error and error.message === 'Process
aborted' keep the existing timeout warn using FETCH_TIMEOUT_MS, but for other
errors call logger.error (or logger.warn if preferred) with a descriptive
message including the error (e.g., from the caught error), the operation name
(fetchRemotes) and any relevant identifiers so operators can see
authentication/connectivity issues while retaining the original behavior of
proceeding with local refs.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 118-135: extractPortFromUrl currently returns default ports
(80/443) when no explicit port is present, which causes unintended overwrites of
server.port; change extractPortFromUrl to return null unless parsed.port is
explicitly set (remove the default protocol fallbacks) and ensure the caller
that uses extractPortFromUrl only assigns to server.port when the returned value
is not null (i.e., guard the assignment to server.port with a null-check).
In `@apps/server/tests/unit/services/dev-server-service.test.ts`:
- Around line 571-592: The Django "Starting development server at" message isn't
matched because the detection regex
/(?:started|listening|running|available|serving|accessible)\s+(?:at|on)\s+/i
(used in the dev server detection logic inside getDevServerService /
startDevServer / getServerInfo) lacks the present-participle "starting"; update
that regex to include "starting" (or add a separate alternation for present
participles) so framework-specific detection captures Django output, and update
the JSDoc that lists supported formats to mention "Starting" for clarity.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 811-817: When start() throws after a successful stop, the catch
block clears transition flags but doesn't revert the UI running state; update
the catch in use-auto-mode.ts to also set the running state to stopped (e.g.,
setIsRunning(false) or isRunningRef.current = false depending on how isRunning
is stored) so the UI reflects the backend stop immediately; keep existing resets
of isTransitioningRef.current and isRestartTransitionRef.current and log the
error via logger.error('Error restarting auto mode:', error) as before.
- Around line 160-182: The restartSafetyTimeoutRef timeout isn't cleared on
component unmount, which can leak timers and emit misleading logger warnings
after unmount; add a useEffect cleanup that on unmount clears
restartSafetyTimeoutRef.current (if present) and resets
isRestartTransitionRef.current to false, mirroring the intended safety-timeout
teardown used with restartSafetyTimeoutRef, so place this cleanup effect near
the other ref declarations (referencing restartSafetyTimeoutRef and
isRestartTransitionRef) to ensure the safety timer is cleared and no stale refs
are used after unmount.
---
Outside diff comments:
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 361-372: The "Open in Browser" DropdownMenuItem remains clickable
even when devServerInfo?.urlDetected === false; disable it and prevent clicks
until a real URL is detected. Update the DropdownMenuItem (the element rendering
"Open in Browser") to set disabled={devServerInfo?.urlDetected === false} and
conditionally avoid calling onOpenDevServerUrl(worktree) when
devServerInfo?.urlDetected === false (e.g., change the onClick to a noop or
guard it before invoking onOpenDevServerUrl). Ensure the aria-label and visual
state still reflect the "Dev Server Starting..." status by relying on
devServerInfo?.urlDetected for both disabled and label logic.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/routes/checkout-branch.ts`:
- Around line 42-48: The catch currently only logs when error.message ===
'Process aborted' and silently swallows all other fetch failures; update the
catch block in checkout-branch.ts (the try that calls fetchRemotes) to handle
non-timeout errors by logging them (e.g., logger.error or logger.warn) with the
error object and context including FETCH_TIMEOUT_MS and that fetchRemotes
failed, while still treating these as non-fatal and continuing with local refs;
ensure you reference the same error variable and preserve the existing
timeout-specific log.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts`:
- Around line 44-71: The toast is shown unconditionally even when the worktree
key isn't present in runningDevServers; change the handler passed to
api.worktree.onDevServerLogEvent so it computes const key =
normalizePath(worktreePath) and only calls toast.success(...) if
runningDevServers actually contains that key and you update it—i.e., inside the
setRunningDevServers updater confirm prev.get(key) exists, create the new Map
and set the updated entry, and ensure the toast is only invoked when an existing
entry was found/updated (do not call toast when the updater short-circuits);
reference functions/vars: getElectronAPI, api.worktree.onDevServerLogEvent,
setRunningDevServers, normalizePath, runningDevServers, existing.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/create.ts`:
- Around line 174-191: The fetch-with-timeout AbortController pattern is
duplicated; extract it into a shared utility (e.g. add FETCH_TIMEOUT_MS and
fetchAllRemotes to `@automaker/git-utils`) that calls
execGitCommand(['fetch','--all','--quiet'], cwd, undefined, controller) with a
30_000ms timeout and centralized error handling/logging; then replace the inline
pattern in create.ts (the try/AbortController block), the fetchRemotes functions
in checkout-branch.ts and checkout-branch-service.ts to call the new
fetchAllRemotes(cwd, logger) and remove the local FETCH_TIMEOUT_MS magic literal
so all usages reference the shared FETCH_TIMEOUT_MS and fetchAllRemotes helper.
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 360-408: Extract the repeated "getActiveWorktrees → filter by
projectPath → fallback to null → iterate" logic into a helper (e.g.
forEachProjectWorktree) and replace the three inline callbacks that call
autoLoopCoordinator.trackFailureAndCheckPauseForProject,
autoLoopCoordinator.signalShouldPauseForProject, and
autoLoopCoordinator.recordSuccessForProject with calls into that helper; the
helper should accept a callback (branchName: string | null) and invoke it for
each branchName or once with null when no active worktrees exist, and the first
callback should use a local flag to accumulate any true return from
trackFailureAndCheckPauseForProject and return that value.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 220-223: The two normalization replaces on detectedUrl currently
require an explicit port and, if you make the port optional, a missing capture
will become "undefined" in the replacement; update both calls that reference
detectedUrl.replace(/\/\/0\.0\.0\.0(:\d+)/, ...) and
detectedUrl.replace(/\/\/\[::\](:\d+)/, ...) to accept an optional port (e.g.
(:\d+)? ) and use a function replacer instead of a replacement string so you can
return `//localhost` + (port || '') — this ensures URLs like http://0.0.0.0/ and
http://[::]/ are normalized correctly without injecting "undefined".
In `@apps/server/src/services/worktree-branch-service.ts`:
- Around line 64-83: The fetchRemotes function relies on brittle string-matching
of error.message === 'Process aborted' to detect timeouts; instead, detect the
timeout by checking the AbortController signal (controller.signal.aborted) in
the catch block and log the timeout path when true (include FETCH_TIMEOUT_MS in
the message), otherwise ignore other fetch errors as before; modify the catch in
fetchRemotes (which calls execGitCommand) to use controller.signal.aborted
rather than comparing error.message.
In `@apps/server/tests/unit/services/dev-server-service.test.ts`:
- Around line 890-899: The mock process created by createMockProcess is missing
a pid property which some ChildProcess consumers (e.g., DevServerService code
paths) may check to verify liveness; add a numeric pid field to the returned
mockProcess (e.g., mockProcess.pid = 12345 or make it configurable) and ensure
any tests that assert kill/killed behavior account for pid presence so future
checks against process.pid won't break silently.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx`:
- Around line 346-394: Extract the duplicated JSX for Logs/Stop into a new
presentational component (e.g., RunningTaskActions) and reuse it in both the
isCurrentAutoTask and !isCurrentAutoTask && isRunningTask branches; the new
component should accept props used in the JSX (onViewOutput, onForceStop,
shortcutKey, feature.id, className variants) and preserve the existing event
handlers (stopPropagation for onClick and onPointerDown) and data-testid
attributes (view-output-<id>, force-stop-<id>, shortcut-key-<id>) so the UI and
tests remain unchanged, then replace the duplicated blocks in card-actions.tsx
with <RunningTaskActions ... /> calls.
In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts`:
- Around line 97-99: The destructured values getAutoModeState and
getMaxConcurrencyForWorktree are unused in use-board-actions.ts; remove them
from the call-site where useAppStore() is destructured to eliminate dead
variables (leave the other destructured symbols as-is), ensuring no references
to getAutoModeState or getMaxConcurrencyForWorktree remain in the file after the
change.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts`:
- Around line 26-32: fetchDevServers populates serversMap using the raw
server.worktreePath which is inconsistent with other code that calls
normalizePath (e.g., getWorktreeKey, isDevServerRunning, getDevServerInfo,
handleStartDevServer, handleStopDevServer); update the loop in fetchDevServers
to call normalizePath(server.worktreePath) when creating the Map key (and keep
the stored DevServerInfo unchanged except for urlDetected defaulting) so lookups
use the same normalized keys everywhere.
In `@apps/ui/src/lib/http-api-client.ts`:
- Around line 580-606: DevServerStartedEvent and DevServerUrlDetectedEvent share
the same fields; extract a shared base interface (e.g., DevServerEventBase with
worktreePath, url, port, timestamp) and have both DevServerStartedEvent and
DevServerUrlDetectedEvent extend that base; update any references/usages of
those interfaces to rely on the specific names where needed and ensure
imports/exports are adjusted so consumers still get the specific types.
Comment on lines
118
to
135
| /** | ||
| * Extract port number from a URL string | ||
| * Returns the port if present, or the default port for the protocol | ||
| */ | ||
| private extractPortFromUrl(url: string): number | null { | ||
| try { | ||
| const parsed = new URL(url); | ||
| if (parsed.port) { | ||
| return parseInt(parsed.port, 10); | ||
| } | ||
| // Default ports | ||
| if (parsed.protocol === 'https:') return 443; | ||
| if (parsed.protocol === 'http:') return 80; | ||
| return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
extractPortFromUrl — returning default ports (80/443) could cause unintended port overwrites.
If a dev server's detected URL lacks an explicit port (e.g., https://localhost/), this returns 443. Line 230-234 would then overwrite server.port from the allocated port (e.g., 3001) to 443, which is almost certainly wrong for a local dev server. Consider returning null instead of default ports, since dev server URLs without explicit ports are likely malformed detections.
Proposed fix
private extractPortFromUrl(url: string): number | null {
try {
const parsed = new URL(url);
if (parsed.port) {
return parseInt(parsed.port, 10);
}
- // Default ports
- if (parsed.protocol === 'https:') return 443;
- if (parsed.protocol === 'http:') return 80;
return null;
} catch {
return null;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Extract port number from a URL string | |
| * Returns the port if present, or the default port for the protocol | |
| */ | |
| private extractPortFromUrl(url: string): number | null { | |
| try { | |
| const parsed = new URL(url); | |
| if (parsed.port) { | |
| return parseInt(parsed.port, 10); | |
| } | |
| // Default ports | |
| if (parsed.protocol === 'https:') return 443; | |
| if (parsed.protocol === 'http:') return 80; | |
| return null; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| /** | |
| * Extract port number from a URL string | |
| * Returns the port if present, or the default port for the protocol | |
| */ | |
| private extractPortFromUrl(url: string): number | null { | |
| try { | |
| const parsed = new URL(url); | |
| if (parsed.port) { | |
| return parseInt(parsed.port, 10); | |
| } | |
| return null; | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 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 118 - 135,
extractPortFromUrl currently returns default ports (80/443) when no explicit
port is present, which causes unintended overwrites of server.port; change
extractPortFromUrl to return null unless parsed.port is explicitly set (remove
the default protocol fallbacks) and ensure the caller that uses
extractPortFromUrl only assigns to server.port when the returned value is not
null (i.e., guard the assignment to server.port with a null-check).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/worktree-branch-service.ts (1)
354-405:⚠️ Potential issue | 🟠 Major
popStashin error recovery is unguarded — an unexpected throw masks the originalcheckoutError.At line 357,
await popStash(worktreePath)is called inside thecatch (checkoutError)block without its owntry/catch. IfpopStashthrows (e.g., a subprocess error or unexpected git failure), the exception propagates unhandled, the originalcheckoutErroris lost, the stash may remain, and thethrow checkoutErrorat line 404 is never reached. The caller receives a confusing, unrelated error.This exact scenario is handled correctly in
checkout-branch-service.ts(same PR, lines 355–374) via a dedicatedcatch (popError)that builds a combined message and returns a structured failure result.🔧 Proposed fix — mirror the checkout-branch-service.ts recovery pattern
} catch (checkoutError) { // 9. Error recovery: if checkout failed and we stashed, try to restore the stash if (didStash) { - const popResult = await popStash(worktreePath); - if (popResult.hasConflicts) { - const checkoutErrorMsg = getErrorMessage(checkoutError); - events?.emit('switch:error', { - worktreePath, - branchName, - error: checkoutErrorMsg, - stashPopConflicts: true, - }); - return { - success: false, - error: checkoutErrorMsg, - stashPopConflicts: true, - stashPopConflictMessage: - 'Stash pop resulted in conflicts: your stashed changes were partially reapplied ' + - 'but produced merge conflicts. Please resolve the conflicts before retrying the branch switch.', - }; - } else if (!popResult.success) { - const checkoutErrorMsg = getErrorMessage(checkoutError); - const combinedMessage = - `${checkoutErrorMsg}. Additionally, restoring your stashed changes failed: ` + - `${popResult.error ?? 'unknown error'} — your changes are still saved in the stash.`; - events?.emit('switch:error', { - worktreePath, - branchName, - error: combinedMessage, - }); - return { - success: false, - error: combinedMessage, - stashPopConflicts: false, - }; - } - // popResult.success === true: stash was cleanly restored, re-throw the checkout error + try { + const popResult = await popStash(worktreePath); + if (popResult.hasConflicts) { + const checkoutErrorMsg = getErrorMessage(checkoutError); + events?.emit('switch:error', { + worktreePath, + branchName, + error: checkoutErrorMsg, + stashPopConflicts: true, + }); + return { + success: false, + error: checkoutErrorMsg, + stashPopConflicts: true, + stashPopConflictMessage: + 'Stash pop resulted in conflicts: your stashed changes were partially reapplied ' + + 'but produced merge conflicts. Please resolve the conflicts before retrying the branch switch.', + }; + } else if (!popResult.success) { + const checkoutErrorMsg = getErrorMessage(checkoutError); + const combinedMessage = + `${checkoutErrorMsg}. Additionally, restoring your stashed changes failed: ` + + `${popResult.error ?? 'unknown error'} — your changes are still saved in the stash.`; + events?.emit('switch:error', { worktreePath, branchName, error: combinedMessage }); + return { success: false, error: combinedMessage, stashPopConflicts: false }; + } + // popResult.success === true: stash was cleanly restored, re-throw the checkout error + } catch (popError) { + const checkoutErrorMsg = getErrorMessage(checkoutError); + const popErrorMsg = getErrorMessage(popError); + const combinedMessage = + `${checkoutErrorMsg}. Additionally, an error occurred while attempting to restore ` + + `your stashed changes: ${popErrorMsg} — your changes may still be saved in the stash.`; + events?.emit('switch:error', { worktreePath, branchName, error: combinedMessage }); + return { + success: false, + error: combinedMessage, + stashPopConflicts: false, + stashPopConflictMessage: combinedMessage, + }; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/worktree-branch-service.ts` around lines 354 - 405, The popStash call inside the catch(block) for checkoutError is unguarded and may throw, masking the original checkoutError; wrap the await popStash(worktreePath) in its own try/catch (mirroring the pattern in checkout-branch-service) so any popError is caught, build a combined error message using getErrorMessage(checkoutError) and getErrorMessage(popError), emit the 'switch:error' event with that combined message, and return a structured failure result (including stash still intact) instead of letting the popError propagate; only rethrow checkoutError when popResult.success === true (stash restored) as currently intended.
🧹 Nitpick comments (5)
apps/server/src/services/checkout-branch-service.ts (1)
30-65:fetchRemotesandFETCH_TIMEOUT_MSare duplicated verbatim across three files.The same helper and constant appear in
checkout-branch-service.ts(lines 30–65),checkout-branch.ts(lines 29–54), andworktree-branch-service.ts(lines 51–83). Any future change to the timeout value or error-handling logic must be applied in all three places. Extract toapps/server/src/lib/git.ts(already used by all three files forexecGitCommand) so there is a single source of truth.♻️ Proposed refactor — add to `apps/server/src/lib/git.ts`
// In apps/server/src/lib/git.ts +import { createLogger, getErrorMessage } from '@automaker/utils'; + +const gitLogger = createLogger('GitUtils'); + +/** Timeout for git fetch operations (30 seconds) */ +export const FETCH_TIMEOUT_MS = 30_000; + +/** + * Fetch latest from all remotes (silently, with timeout). + * Non-fatal: fetch errors are logged and swallowed so the workflow continues. + */ +export async function fetchRemotes(cwd: string): Promise<void> { + const controller = new AbortController(); + const timerId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + try { + await execGitCommand(['fetch', '--all', '--quiet'], cwd, undefined, controller); + } catch (error) { + if (controller.signal.aborted) { + gitLogger.warn( + `fetchRemotes timed out after ${FETCH_TIMEOUT_MS}ms - continuing without latest remote refs` + ); + } else { + gitLogger.warn(`fetchRemotes failed: ${getErrorMessage(error)} - continuing with local refs`); + } + } finally { + clearTimeout(timerId); + } +}Each consuming file can then be simplified to:
-const FETCH_TIMEOUT_MS = 30_000; -async function fetchRemotes(cwd: string): Promise<void> { ... } +import { fetchRemotes } from '../lib/git.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/checkout-branch-service.ts` around lines 30 - 65, Extract the duplicated FETCH_TIMEOUT_MS constant and fetchRemotes function into the shared git utility module that already exports execGitCommand; implement the same logic there (create AbortController, setTimeout to abort after FETCH_TIMEOUT_MS, call execGitCommand(['fetch','--all','--quiet'], cwd, undefined, controller), catch errors and call logger.warn with the same messages using getErrorMessage, and always clear the timer) and export FETCH_TIMEOUT_MS and fetchRemotes; then update checkoutBranchService (and the other two files) to remove their local FETCH_TIMEOUT_MS/fetchRemotes and import the shared FETCH_TIMEOUT_MS and fetchRemotes instead so all three consumers use the single source of truth.apps/ui/src/hooks/use-auto-mode.ts (2)
633-645:setMaxConcurrencyForWorktreeis in the event handler dep array but never called within the effect.Scanning the entire handler body (lines 271–631),
setMaxConcurrencyForWorktreeis not invoked —setAutoModeRunningalready receivesmaxConcurrencyas its fourth argument. Since Zustand setters are stable references this has no runtime cost, but it misleads readers and adds noise to the dependency list.♻️ Suggested cleanup
}, [ projectId, addRunningTask, removeRunningTask, addAutoModeActivity, getProjectIdFromPath, setPendingPlanApproval, setAutoModeRunning, currentProject?.path, getMaxConcurrencyForWorktree, - setMaxConcurrencyForWorktree, isPrimaryWorktreeBranch, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-auto-mode.ts` around lines 633 - 645, The dependency list for the effect includes setMaxConcurrencyForWorktree even though the effect body (the handler in use-auto-mode) never calls setMaxConcurrencyForWorktree; remove setMaxConcurrencyForWorktree from the effect's dependency array so it no longer appears in the deps for the effect that references projectId, addRunningTask, removeRunningTask, addAutoModeActivity, getProjectIdFromPath, setPendingPlanApproval, setAutoModeRunning, currentProject?.path, getMaxConcurrencyForWorktree, isPrimaryWorktreeBranch, ensuring no behavioral change because Zustand setters (like setMaxConcurrencyForWorktree) are stable references.
685-698:refreshStatusis missing fromstart()(andstop()) dependency arrays.
refreshStatusis called viasetTimeoutinside bothstartandstop, but neither lists it as a dependency. In practice this doesn't produce a stale-closure bug today becauserefreshStatus's own deps (branchName,currentProject,setAutoModeRunning) are a strict subset of both callbacks' deps — sorefreshStatusis always recreated wheneverstart/stopare. However, thereact-hooks/exhaustive-depslinter will flag this, and any future addition of a dependency torefreshStatusthat isn't also instart/stopwould silently introduce a stale closure.The idiomatic fix is to add
refreshStatusto both dep arrays. Alternatively, since React 19.2 shipsuseEffectEventas stable, thesetTimeoutcallbacks could use auseEffectEvent-wrapped version ofrefreshStatusto read the latest state without needing to be in the dep array at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-auto-mode.ts` around lines 685 - 698, start() and stop() call refreshStatus via setTimeout but do not include refreshStatus in their dependency arrays, which can trigger the react-hooks/exhaustive-deps linter and risk stale closures. Fix by adding refreshStatus to the dependency arrays of the start and stop callbacks (the useCallback that defines start and stop), or alternatively wrap refreshStatus with useEffectEvent and call that stable wrapper from the setTimeout handlers; update the useCallback([...]) deps for start/stop to include refreshStatus so the timeout always uses the latest closure.apps/server/src/services/dev-server-service.ts (1)
164-305:urlPatternsandportPatternsarrays are recreated on every call before URL detection completes.Both arrays are defined as local
constinsidedetectUrlFromOutput. They're heap-allocated on every invocation — which happens for every stdout/stderr chunk emitted by the dev server — untilurlDetectedbecomestrue. Hoisting them to module-level (or class-level) constants avoids repeated allocation during the startup burst.♻️ Proposed refactor (module-level constants)
+const URL_PATTERNS: Array<{ pattern: RegExp; description: string }> = [ + { pattern: /(?:Local|Network|External):\s+(https?:\/\/[^\s]+)/i, description: 'Vite/Nuxt/SvelteKit/Astro/Angular format' }, + { pattern: /(?:ready|started server).*?(?:url:\s*)?(https?:\/\/[^\s,]+)/i, description: 'Next.js format' }, + // ... remaining patterns +]; + +const PORT_PATTERNS: Array<{ pattern: RegExp; description: string }> = [ + { pattern: /(?:listening|running|started|serving|available)\s+on\s+port\s+(\d+)/i, description: '"listening on port" format' }, + { pattern: /(?:^|\s)port[:\s]+(\d{4,5})(?:\s|$|[.,;])/im, description: '"port:" format' }, +]; private detectUrlFromOutput(server: DevServerInfo, content: string): void { if (server.urlDetected) return; const cleanContent = this.stripAnsi(content); - const urlPatterns: Array<{ pattern: RegExp; description: string }> = [ /* ... */ ]; - const portPatterns: Array<{ pattern: RegExp; description: string }> = [ /* ... */ ]; + for (const { pattern, description } of URL_PATTERNS) { + // ... + for (const { pattern, description } of PORT_PATTERNS) {🤖 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 164 - 305, The urlPatterns and portPatterns arrays are being reallocated on every call to detectUrlFromOutput; move them out of the function and define them once as module-level (or class-level static) constants so they are allocated once and reused until server.urlDetected becomes true. Update detectUrlFromOutput to reference the relocated urlPatterns and portPatterns (leave pattern contents and matching logic unchanged), and ensure any references to description or pattern still work; keep server.urlDetected logic intact so behavior is identical. Also run a quick lint/type check to ensure no unused-variable or import issues after hoisting.apps/server/tests/unit/services/dev-server-service.test.ts (1)
394-520: Consider replacingsetTimeout-based synchronization withvi.waitFor()for more robust async testing.The
await new Promise(resolve => setTimeout(resolve, 50))pattern throughout the URL detection tests is timing-dependent and may be flaky on slow CI runners. Vitest'svi.waitFor()polls until the assertion passes, making tests more resilient.♻️ Example refactor
-await new Promise((resolve) => setTimeout(resolve, 50)); -const serverInfo = service.getServerInfo(testDir); -expect(serverInfo?.url).toBe('http://localhost:5173/'); +await vi.waitFor(() => { + expect(service.getServerInfo(testDir)?.url).toBe('http://localhost:5173/'); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/services/dev-server-service.test.ts` around lines 394 - 520, Replace fragile setTimeout-based waits in the URL detection tests with vitest's vi.waitFor to make assertions resilient: where the tests call await new Promise(resolve => setTimeout(resolve, 50)) after emitting mockProcess.stdout data (in tests that use createMockProcess(), spawn mock, startDevServer() and then call getServerInfo()), swap that pause for vi.waitFor(() => { expect(service.getServerInfo(testDir)?.url...).toBe(...) }) or vi.waitFor(() => service.getServerInfo(testDir)?.urlDetected === true) so the test polls until the server info is updated rather than relying on a fixed delay.
🤖 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/auto-loop-coordinator.ts`:
- Around line 310-316: The method getRunningCountForWorktree has a branchName
parameter missing the class-wide default of = null; update its signature to
declare branchName: string | null = null so it matches other public methods
(e.g., startAutoLoopForProject, stopAutoLoopForProject,
isAutoLoopRunningForProject, getAutoLoopConfigForProject) and keeps the API
consistent — no further logic changes needed since callers already pass
branchName explicitly.
In `@apps/server/src/services/checkout-branch-service.ts`:
- Around line 54-60: Replace the fragile string check for timeout in the
fetchRemotes error handler by testing the AbortSignal instead: inside the block
where you handle errors from fetchRemotes (the catch that currently checks
error.message === 'Process aborted'), use controller.signal.aborted to detect a
timeout and log the timeout-specific warning, otherwise fall through to the
generic logger.warn with getErrorMessage(error); update the branch in
checkout-branch-service.ts to mirror the robust pattern used in
worktree-branch-service.ts (checking controller.signal.aborted) so timeouts are
detected reliably regardless of the thrown error message.
In `@apps/ui/src/components/ui/test-logs-panel.tsx`:
- Line 218: TestLogsPanelContent's header is applying the mobile-only
`dialog-compact-header-mobile` class even when rendered inside the bottom Sheet;
add a boolean prop (e.g., applyCompactHeaderMobile or compactHeaderMobile) to
TestLogsPanelContent (update its props interface) and use it to conditionally
include `dialog-compact-header-mobile` on the DialogHeader JSX where the class
is currently hard-coded; when rendering TestLogsPanelContent from the desktop
Dialog pass the prop true (or explicitly set it), and render it from the mobile
Sheet without the prop (or set it false) so the Sheet header keeps the intended
`py-3` padding without the safe-area inset.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 327-335: The event handler is currently clearing and checking
instance-level refs without verifying the event's worktree, which lets events
for worktree B incorrectly affect worktree A; update the handler to only clear
isRestartTransitionRef.current and isTransitioningRef.current (and clear
restartSafetyTimeoutRef) when eventBranchName === branchName, and similarly only
suppress auto_mode_stopped when eventBranchName === branchName and
isTransitioningRef.current is true; also add branchName to the useEffect
dependency array that registers the handler so the closure always reads the
current branchName.
- Around line 800-831: The failure branch inside the restart flow is doing
duplicate cleanup and logging before throwing; remove the manual resets and
logging from the if (!startResult.success) block (remove
isTransitioningRef.current/isRestartTransitionRef.current assignments,
setAutoModeSessionForWorktree, setAutoModeRunning, and logger.error there) and
simply throw the Error(startResult.error || 'Failed to restart auto mode') so
the catch handler can perform the single consolidated cleanup and logging (see
isTransitioningRef, isRestartTransitionRef, setAutoModeSessionForWorktree,
setAutoModeRunning, logger.error, and restartSafetyTimeoutRef to locate the
related code).
---
Outside diff comments:
In `@apps/server/src/services/worktree-branch-service.ts`:
- Around line 354-405: The popStash call inside the catch(block) for
checkoutError is unguarded and may throw, masking the original checkoutError;
wrap the await popStash(worktreePath) in its own try/catch (mirroring the
pattern in checkout-branch-service) so any popError is caught, build a combined
error message using getErrorMessage(checkoutError) and
getErrorMessage(popError), emit the 'switch:error' event with that combined
message, and return a structured failure result (including stash still intact)
instead of letting the popError propagate; only rethrow checkoutError when
popResult.success === true (stash restored) as currently intended.
---
Duplicate comments:
In `@apps/server/src/routes/worktree/routes/checkout-branch.ts`:
- Around line 25-54: fetchRemotes and FETCH_TIMEOUT_MS are duplicated across
files and the timeout detection is brittle: replace the string comparison of
error.message === 'Process aborted' with a check against
controller.signal.aborted to reliably detect an abort, and consolidate the
duplicated fetchRemotes/FETCH_TIMEOUT_MS logic into a single shared utility
(e.g., move the helper into a common module used by fetchRemotes in
checkout-branch.ts, checkout-branch-service.ts, and worktree-branch-service.ts);
ensure the shared helper uses AbortController, calls
execGitCommand(['fetch','--all','--quiet'], ...), logs via
createLogger('CheckoutBranchRoute')/appropriate logger, and preserves the
non-fatal swallow-on-error behavior while checking controller.signal.aborted for
timeout-specific logging.
In `@apps/server/src/services/checkout-branch-service.ts`:
- Around line 47-65: fetchRemotes now properly logs non-timeout failures using
getErrorMessage in the else branch, so no further changes are required; leave
the fetchRemotes function (which uses execGitCommand, AbortController,
FETCH_TIMEOUT_MS, logger.warn and getErrorMessage) as-is since it now handles
both timeout and non-timeout errors and continues gracefully with local refs.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 118-134: The extractPortFromUrl function already correctly returns
null for URLs without an explicit port (avoiding default protocol ports 80/443)
so no code change is needed; keep the existing implementation of
extractPortFromUrl(url: string): number | null as-is and add/ensure unit tests
that cover cases "https://localhost/", "http://example.com:8080", and invalid
URLs to validate returning null for protocol-default and invalid URLs and a
parsed integer for explicit ports.
In `@apps/server/tests/unit/services/dev-server-service.test.ts`:
- Around line 571-592: Update the Django URL detection pattern in the dev-server
output parser so it matches present-participle phrasing like "Starting
development server at ..."; specifically modify the regex used in the
dev-server-service.ts detection logic (the function used by
getDevServerService/startDevServer that parses process stdout and sets
getServerInfo) to accept "Starting" (case-insensitive) or a more general pattern
such as "(?:Starting|Starting up|Starting).*at\\s+(https?://\\S+)" so the
emitted line "Starting development server at http://127.0.0.1:8000/" is captured
and getServerInfo(...).url and .urlDetected are set accordingly.
---
Nitpick comments:
In `@apps/server/src/services/checkout-branch-service.ts`:
- Around line 30-65: Extract the duplicated FETCH_TIMEOUT_MS constant and
fetchRemotes function into the shared git utility module that already exports
execGitCommand; implement the same logic there (create AbortController,
setTimeout to abort after FETCH_TIMEOUT_MS, call
execGitCommand(['fetch','--all','--quiet'], cwd, undefined, controller), catch
errors and call logger.warn with the same messages using getErrorMessage, and
always clear the timer) and export FETCH_TIMEOUT_MS and fetchRemotes; then
update checkoutBranchService (and the other two files) to remove their local
FETCH_TIMEOUT_MS/fetchRemotes and import the shared FETCH_TIMEOUT_MS and
fetchRemotes instead so all three consumers use the single source of truth.
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 164-305: The urlPatterns and portPatterns arrays are being
reallocated on every call to detectUrlFromOutput; move them out of the function
and define them once as module-level (or class-level static) constants so they
are allocated once and reused until server.urlDetected becomes true. Update
detectUrlFromOutput to reference the relocated urlPatterns and portPatterns
(leave pattern contents and matching logic unchanged), and ensure any references
to description or pattern still work; keep server.urlDetected logic intact so
behavior is identical. Also run a quick lint/type check to ensure no
unused-variable or import issues after hoisting.
In `@apps/server/tests/unit/services/dev-server-service.test.ts`:
- Around line 394-520: Replace fragile setTimeout-based waits in the URL
detection tests with vitest's vi.waitFor to make assertions resilient: where the
tests call await new Promise(resolve => setTimeout(resolve, 50)) after emitting
mockProcess.stdout data (in tests that use createMockProcess(), spawn mock,
startDevServer() and then call getServerInfo()), swap that pause for
vi.waitFor(() => { expect(service.getServerInfo(testDir)?.url...).toBe(...) })
or vi.waitFor(() => service.getServerInfo(testDir)?.urlDetected === true) so the
test polls until the server info is updated rather than relying on a fixed
delay.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 633-645: The dependency list for the effect includes
setMaxConcurrencyForWorktree even though the effect body (the handler in
use-auto-mode) never calls setMaxConcurrencyForWorktree; remove
setMaxConcurrencyForWorktree from the effect's dependency array so it no longer
appears in the deps for the effect that references projectId, addRunningTask,
removeRunningTask, addAutoModeActivity, getProjectIdFromPath,
setPendingPlanApproval, setAutoModeRunning, currentProject?.path,
getMaxConcurrencyForWorktree, isPrimaryWorktreeBranch, ensuring no behavioral
change because Zustand setters (like setMaxConcurrencyForWorktree) are stable
references.
- Around line 685-698: start() and stop() call refreshStatus via setTimeout but
do not include refreshStatus in their dependency arrays, which can trigger the
react-hooks/exhaustive-deps linter and risk stale closures. Fix by adding
refreshStatus to the dependency arrays of the start and stop callbacks (the
useCallback that defines start and stop), or alternatively wrap refreshStatus
with useEffectEvent and call that stable wrapper from the setTimeout handlers;
update the useCallback([...]) deps for start/stop to include refreshStatus so
the timeout always uses the latest closure.
Comment on lines
310
to
316
| async getRunningCountForWorktree( | ||
| projectPath: string, | ||
| branchName: string | null | ||
| branchName: string | null, | ||
| options?: { autoModeOnly?: boolean } | ||
| ): Promise<number> { | ||
| return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName); | ||
| return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName, options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file and the specific lines mentioned
wc -l apps/server/src/services/auto-loop-coordinator.tsRepository: AutoMaker-Org/automaker
Length of output: 120
🏁 Script executed:
# Read the file to check the method signatures at the referenced lines
cat -n apps/server/src/services/auto-loop-coordinator.ts | sed -n '90,100p;240,280p;310,320p'Repository: AutoMaker-Org/automaker
Length of output: 2892
🏁 Script executed:
# Search for all invocations of getRunningCountForWorktree
rg -n --type=ts 'getRunningCountForWorktree' -B 1 -A 2Repository: AutoMaker-Org/automaker
Length of output: 19906
🏁 Script executed:
# Check if there are any calls to getRunningCountForWorktree that might omit the branchName parameter
rg -n --type=ts 'getRunningCountForWorktree\s*\(\s*[^,)]+\s*\)' -v 'branchName'Repository: AutoMaker-Org/automaker
Length of output: 117
🏁 Script executed:
# Also search the entire codebase (including route handlers and other sources) that might call this method
rg -n 'getRunningCountForWorktree\(' | head -50Repository: AutoMaker-Org/automaker
Length of output: 3523
🏁 Script executed:
# Check the auto-mode/facade.ts call context
cat -n apps/server/src/services/auto-mode/facade.ts | sed -n '888,896p'Repository: AutoMaker-Org/automaker
Length of output: 323
🏁 Script executed:
# Search for any route handlers that might call getRunningCountForWorktree
rg -n 'getRunningCountForWorktree' apps/server/src/routes/ 2>/dev/null || echo "No matches in routes/"Repository: AutoMaker-Org/automaker
Length of output: 88
🏁 Script executed:
# Verify the method signature in ConcurrencyManager for comparison
cat -n apps/server/src/services/concurrency-manager.ts | sed -n '179,185p'Repository: AutoMaker-Org/automaker
Length of output: 335
Add = null default to branchName parameter for API consistency.
The branchName parameter on getRunningCountForWorktree lacks the = null default present on every other public method in this class (startAutoLoopForProject at line 95, stopAutoLoopForProject at line 242, isAutoLoopRunningForProject at line 261, getAutoLoopConfigForProject at line 274). All current callers pass branchName explicitly, but this inconsistency makes the API surface harder to use and maintains parity with the class's other methods.
🔧 Proposed fix
async getRunningCountForWorktree(
projectPath: string,
- branchName: string | null,
+ branchName: string | null = null,
options?: { autoModeOnly?: boolean }
): Promise<number> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async getRunningCountForWorktree( | |
| projectPath: string, | |
| branchName: string | null | |
| branchName: string | null, | |
| options?: { autoModeOnly?: boolean } | |
| ): Promise<number> { | |
| return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName); | |
| return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName, options); | |
| } | |
| async getRunningCountForWorktree( | |
| projectPath: string, | |
| branchName: string | null = null, | |
| options?: { autoModeOnly?: boolean } | |
| ): Promise<number> { | |
| return this.concurrencyManager.getRunningCountForWorktree(projectPath, branchName, options); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 310 - 316,
The method getRunningCountForWorktree has a branchName parameter missing the
class-wide default of = null; update its signature to declare branchName: string
| null = null so it matches other public methods (e.g., startAutoLoopForProject,
stopAutoLoopForProject, isAutoLoopRunningForProject,
getAutoLoopConfigForProject) and keeps the API consistent — no further logic
changes needed since callers already pass branchName explicitly.
| <> | ||
| {/* Header */} | ||
| <DialogHeader className="shrink-0 px-4 py-3 border-b border-border/50 pr-12"> | ||
| <DialogHeader className="shrink-0 px-4 py-3 border-b border-border/50 pr-12 dialog-compact-header-mobile"> |
There was a problem hiding this comment.
dialog-compact-header-mobile applies safe-area-inset-top in the mobile Sheet path where it isn't needed.
TestLogsPanelContent is shared between the desktop Dialog and the mobile Sheet (bottom drawer). On mobile, dialog-compact-header-mobile adds padding-top: calc(env(safe-area-inset-top, 0px) + 0.75rem). Because env(safe-area-inset-top) is device-global (~44–59 px on notched iPhones), the Sheet header gets ~56–71 px of top padding instead of the intended 12 px (py-3), producing a noticeably oversized gap on notched devices.
The class is designed for full-screen dialogs whose top edge is near the Dynamic Island. The bottom Sheet starts ~20 % from the top of the screen and doesn't need this compensation.
🛠 Suggested fix — pass a prop to opt out of the class in the Sheet context
interface TestLogsPanelContentProps {
worktreePath: string | null;
branch?: string;
sessionId?: string;
onStopTests?: () => void;
+ compactHeaderMobile?: boolean;
}
function TestLogsPanelContent({
worktreePath,
branch,
sessionId,
onStopTests,
+ compactHeaderMobile = false,
}: TestLogsPanelContentProps) {
// ...
- <DialogHeader className="shrink-0 px-4 py-3 border-b border-border/50 pr-12 dialog-compact-header-mobile">
+ <DialogHeader className={cn("shrink-0 px-4 py-3 border-b border-border/50 pr-12", compactHeaderMobile && "dialog-compact-header-mobile")}>Then in the Dialog (desktop) path:
- <TestLogsPanelContent worktreePath={worktreePath} branch={branch} sessionId={sessionId} onStopTests={onStopTests} />
+ <TestLogsPanelContent worktreePath={worktreePath} branch={branch} sessionId={sessionId} onStopTests={onStopTests} compactHeaderMobile />The Sheet (mobile) path continues to omit the prop, defaulting to false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/ui/test-logs-panel.tsx` at line 218,
TestLogsPanelContent's header is applying the mobile-only
`dialog-compact-header-mobile` class even when rendered inside the bottom Sheet;
add a boolean prop (e.g., applyCompactHeaderMobile or compactHeaderMobile) to
TestLogsPanelContent (update its props interface) and use it to conditionally
include `dialog-compact-header-mobile` on the DialogHeader JSX where the class
is currently hard-coded; when rendering TestLogsPanelContent from the desktop
Dialog pass the prop true (or explicitly set it), and render it from the mobile
Sheet without the prop (or set it false) so the Sheet header keeps the intended
`py-3` padding without the safe-area inset.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/ui/src/hooks/use-auto-mode.ts (1)
693-693: Fire-and-forgetsetTimeouttimers are not tracked for unmount cleanup.The
setTimeout(() => void refreshStatus(), 500)calls at lines 693 and 741 aren't stored for cancellation. If the component unmounts within 500 ms, the callback fires against stale Zustand state. In practice this is safe (Zustand actions don't cause unmounted-component warnings, andrefreshStatusexits early on!currentProject), but storing and clearing the IDs would be cleaner.Also applies to: 741-741
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-auto-mode.ts` at line 693, The setTimeout calls in the useAutoMode hook (the setTimeout(() => void refreshStatus(), 500) invocations near where refreshStatus is scheduled) are fire-and-forget and should be tracked and cleared on unmount or when rescheduling; change them to store the returned timer ID (e.g., const id = window.setTimeout(...)) into a ref or array (e.g., timersRef) and call clearTimeout(id) when cleaning up the hook (in the useEffect cleanup) or before scheduling a new timer, ensuring all pending timeouts created by refreshStatus are cancelled on unmount to avoid callbacks running against stale state.apps/server/src/services/dev-server-service.ts (3)
747-755: ReturnserverInfo.urlandserverInfo.portinstead of re-deriving the placeholderDuring the 500ms startup wait at Line 718,
detectUrlFromOutputmay already have updatedserverInfo.urlandserverInfo.portwith the real detected URL. The current return value hard-codes the initial template, so callers receive a stale placeholder even when the URL is already known. This is also inconsistent with the already-running branch (Line 554) which correctly returnsexisting.url.♻️ Proposed fix
return { success: true, result: { worktreePath, - port, - url: `http://${hostname}:${port}`, + port: serverInfo.port, + url: serverInfo.url, message: `Dev server started on port ${port}`, }, };🤖 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 747 - 755, The returned object hard-codes `url` and `port` from the initial hostname/port placeholders instead of using the possibly-updated values on `serverInfo` (which `detectUrlFromOutput` may have populated); update the startup-return block to use `serverInfo.url` and `serverInfo.port` (and keep `worktreePath` from `serverInfo` if present) so callers receive the detected URL/port—mirror the behavior used in the already-running branch that returns `existing.url`.
238-246:[::1](IPv6 loopback) is not normalized tolocalhostThe normalization covers
0.0.0.0and[::]but misses[::1], the IPv6 loopback. A server binding to::1would emithttp://[::1]:3000in the event payload — valid in modern browsers, but inconsistent with the treatment of the other loopback addresses.♻️ Proposed fix
// Normalize [::] to localhost for browser accessibility detectedUrl = detectedUrl.replace( /\/\/\[::\](:\d+)?/, (_, port) => `//localhost${port || ''}` ); + // Normalize [::1] to localhost for browser accessibility + detectedUrl = detectedUrl.replace( + /\/\/\[::1\](:\d+)?/, + (_, port) => `//localhost${port || ''}` + );🤖 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 238 - 246, The code in dev-server-service.ts normalizes detectedUrl for 0.0.0.0 and [::] but misses the IPv6 loopback [::1]; update the normalization logic that operates on the detectedUrl variable (the existing detectedUrl.replace calls) to also replace /\/\/\[::1\](:\d+)?/ with //localhost${port || ''} (same form as the other replacements) so URLs like http://[::1]:3000 are normalized to http://localhost:3000; ensure you add this new replace call alongside the existing replacements in the same function where detectedUrl is adjusted.
175-179: Preferutil.stripVTControlCharacters()(Node.js built-in) over a custom ANSI regexThe custom regex misses single-byte Fe escape sequences (
ESC c,ESC M,ESC 7/8,ESC =/>, etc.) that real terminal output can contain. Node.js now hasstripVTControlCharactersbuilt-in, with the benefit of consistent behavior across Node.js versions and faster improvements. This avoids both the coverage gap and any dependency concern —strip-ansiv7 (current) is ESM-only, which can conflict with a CommonJS server build.♻️ Proposed refactor using the Node.js built-in
Add
utilto the import block at the top of the file:+import { stripVTControlCharacters } from 'util';Replace
stripAnsi:- /** - * Strip ANSI escape codes from a string - * Dev server output often contains color codes that can interfere with URL detection - */ - private stripAnsi(str: string): string { - // Matches ANSI escape sequences: CSI sequences, OSC sequences, and simple escapes - // eslint-disable-next-line no-control-regex - return str.replace(/\x1B(?:\[[0-9;]*[a-zA-Z]|\].*?(?:\x07|\x1B\\)|\[[?]?[0-9;]*[hl])/g, ''); - }And at the call site:
- const cleanContent = this.stripAnsi(content); + const cleanContent = stripVTControlCharacters(content);Verify the project's minimum Node.js version supports this API (added in v16.11.0):
#!/bin/bash # Check declared Node.js engine constraint(s) in package.json files fd -g 'package.json' --exclude node_modules --exec grep -l '"engines"' {} \; | xargs grep -h '"node"' 2>/dev/null🤖 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 175 - 179, Replace the custom ANSI-regex implementation in the stripAnsi function with Node's built-in util.stripVTControlCharacters: import util (or the stripVTControlCharacters export) and have stripAnsi call util.stripVTControlCharacters(str); update any call sites that reference stripAnsi to continue using it unchanged; also verify the project's minimum Node.js engine supports stripVTControlCharacters (>= v16.11.0) in package.json and if not, add a conditional fallback or bump the engine constraint.
🤖 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/ui/src/hooks/use-auto-mode.ts`:
- Around line 329-337: The restart-transition clear and the stop-suppression
logic must also verify the event belongs to the same project; update the
conditional guards to include eventProjectId === projectId (in addition to
eventBranchName === branchName) so that isRestartTransitionRef.current,
isTransitioningRef.current and restartSafetyTimeoutRef.current are only cleared
for matching project events, and apply the same project-id guard where the
auto_mode_stopped suppression check runs (references: isRestartTransitionRef,
isTransitioningRef, restartSafetyTimeoutRef, eventBranchName, branchName,
eventProjectId, projectId).
- Around line 805-838: The if-block that handles !startResult.success currently
duplicates cleanup and logging that the catch block already performs; remove the
manual clearing of isTransitioningRef.current, isRestartTransitionRef.current,
calls to setAutoModeSessionForWorktree and setAutoModeRunning, the logger.error
call, and simply throw the error so the existing catch block handles cleanup and
logging (use throw new Error(startResult.error || 'Failed to restart auto mode')
or rethrow startResult.error). References: startResult, isTransitioningRef,
isRestartTransitionRef, setAutoModeSessionForWorktree, setAutoModeRunning,
restartSafetyTimeoutRef, and logger.error.
---
Nitpick comments:
In `@apps/server/src/services/dev-server-service.ts`:
- Around line 747-755: The returned object hard-codes `url` and `port` from the
initial hostname/port placeholders instead of using the possibly-updated values
on `serverInfo` (which `detectUrlFromOutput` may have populated); update the
startup-return block to use `serverInfo.url` and `serverInfo.port` (and keep
`worktreePath` from `serverInfo` if present) so callers receive the detected
URL/port—mirror the behavior used in the already-running branch that returns
`existing.url`.
- Around line 238-246: The code in dev-server-service.ts normalizes detectedUrl
for 0.0.0.0 and [::] but misses the IPv6 loopback [::1]; update the
normalization logic that operates on the detectedUrl variable (the existing
detectedUrl.replace calls) to also replace /\/\/\[::1\](:\d+)?/ with
//localhost${port || ''} (same form as the other replacements) so URLs like
http://[::1]:3000 are normalized to http://localhost:3000; ensure you add this
new replace call alongside the existing replacements in the same function where
detectedUrl is adjusted.
- Around line 175-179: Replace the custom ANSI-regex implementation in the
stripAnsi function with Node's built-in util.stripVTControlCharacters: import
util (or the stripVTControlCharacters export) and have stripAnsi call
util.stripVTControlCharacters(str); update any call sites that reference
stripAnsi to continue using it unchanged; also verify the project's minimum
Node.js engine supports stripVTControlCharacters (>= v16.11.0) in package.json
and if not, add a conditional fallback or bump the engine constraint.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Line 693: The setTimeout calls in the useAutoMode hook (the setTimeout(() =>
void refreshStatus(), 500) invocations near where refreshStatus is scheduled)
are fire-and-forget and should be tracked and cleared on unmount or when
rescheduling; change them to store the returned timer ID (e.g., const id =
window.setTimeout(...)) into a ref or array (e.g., timersRef) and call
clearTimeout(id) when cleaning up the hook (in the useEffect cleanup) or before
scheduling a new timer, ensuring all pending timeouts created by refreshStatus
are cancelled on unmount to avoid callbacks running against stale state.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (3)
479-491:stripAnsiis missing fromtoggleSelectMode'suseCallbackdependency array
stripAnsiis a non-memoized function created inside the component and used intoggleSelectMode, but it is not listed in theuseCallbackdeps. React's exhaustive-deps rule would flag this. SincestripAnsiis pure (no closure over mutable state), this is functionally safe, but either memoizestripAnsiwithuseCallbackor move it to module scope to silence the lint warning.♻️ Proposed fix (move stripAnsi to module scope, outside the component)
Move the
stripAnsideclaration (currently at line 309) above the component function definition, making it a module-level utility. TheuseCallbackdeps then don't need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 479 - 491, toggleSelectMode's useCallback uses stripAnsi which is defined inside the component but not included in the dependency array; move the stripAnsi function declaration out of the component to module scope (above the component function) so it becomes a stable, pure utility and remove it from the need to be a dependency, then keep toggleSelectMode as currently defined (referencing toggleSelectMode, stripAnsi, getTerminalBufferText, setSelectModeText, setIsSelectMode) to silence the exhaustive-deps lint; alternatively, if you prefer to keep stripAnsi inside the component, wrap it with useCallback to memoize it and add it to the toggleSelectMode dependency array.
1666-1716: Duplicate context menu position boundary check; extract to a shared helperThe boundary-clamping logic (clamp
x/yusingmenuWidth=160,menuHeight=152,padding=8) is copy-pasted verbatim fromhandleContextMenu(lines 1635–1659). A small inline helper eliminates the duplication:♻️ Proposed refactor
+ const clampMenuPosition = useCallback((x: number, y: number) => { + const menuWidth = 160; + const menuHeight = 152; + const padding = 8; + if (x + menuWidth + padding > window.innerWidth) x = window.innerWidth - menuWidth - padding; + if (y + menuHeight + padding > window.innerHeight) y = window.innerHeight - menuHeight - padding; + return { x: Math.max(padding, x), y: Math.max(padding, y) }; + }, []); + const handleContextMenu = useCallback((e: React.MouseEvent) => { e.preventDefault(); e.stopPropagation(); - const menuWidth = 160; - const menuHeight = 152; - const padding = 8; - let x = e.clientX; - let y = e.clientY; - if (x + menuWidth + padding > window.innerWidth) { x = window.innerWidth - menuWidth - padding; } - if (y + menuHeight + padding > window.innerHeight) { y = window.innerHeight - menuHeight - padding; } - x = Math.max(padding, x); - y = Math.max(padding, y); - setContextMenu({ x, y }); + setContextMenu(clampMenuPosition(e.clientX, e.clientY)); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 1666 - 1716, The touch handler duplicates the boundary-clamping logic also used in handleContextMenu; extract that logic into a shared helper (e.g., computeClampedContextMenuPosition or clampContextMenuPosition) that accepts the desired x,y and optional menuWidth/menuHeight/padding and returns the clamped { x, y } position, then replace the inline clamping in both handleTouchStart and handleContextMenu to call this helper and pass its result to setContextMenu; ensure the helper has correct types and is imported/defined in the same module so both handleContextMenu and handleTouchStart reference it.
1663-1664: MoveLONG_PRESS_DURATIONandLONG_PRESS_MOVE_THRESHOLDto module scopeThese pure numeric constants are recreated on every render. Define them alongside the other module-level constants (lines 64–79).
♻️ Proposed fix
+// Long-press mobile context menu constants +const LONG_PRESS_DURATION = 500; // ms +const LONG_PRESS_MOVE_THRESHOLD = 10; // px - cancel if finger moves more than this + // Long-press handlers for mobile context menu // On mobile, there's no right-click, so we trigger the context menu on long-press (500ms hold) -const LONG_PRESS_DURATION = 500; // ms -const LONG_PRESS_MOVE_THRESHOLD = 10; // px - cancel if finger moves more than this - const handleTouchStart = useCallback(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 1663 - 1664, The two numeric constants LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD are being recreated on every render; move their declarations out of the component render scope to the module-level constants alongside the other top-level constants so they are defined once. Locate the current in-component definitions of LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD in TerminalPanel (terminal-panel.tsx) and cut them into the module scope with the other constants, keeping the same values and names, then remove the local declarations so the component references the module-level LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD.apps/ui/src/lib/clipboard-utils.ts (1)
118-134: Simplify the try/throw/catch/re-throw pattern inreadFromClipboardLegacyThe function throws a custom
Errorinside thetryblock which is immediately caught by the adjacentcatch, which then inspects the message to decide whether to re-throw or create an identical error. Any error originating fromdocument.execCommand('paste')itself (rare, but possible) is silently replaced by the same boilerplate message. A cleaner pattern:♻️ Proposed refactor
function readFromClipboardLegacy(): string { const textarea = document.createElement('textarea'); // ...position styles... document.body.appendChild(textarea); textarea.focus(); try { const success = document.execCommand('paste'); if (success && textarea.value) { return textarea.value; } - throw new Error( - 'Clipboard paste is not supported in this browser on non-HTTPS sites. ' + - 'Please use HTTPS or paste manually with keyboard shortcuts.' - ); - } catch (err) { - if (err instanceof Error && err.message.includes('Clipboard paste is not supported')) { - throw err; - } - throw new Error( - 'Clipboard paste is not supported in this browser on non-HTTPS sites. ' + - 'Please use HTTPS or paste manually with keyboard shortcuts.' - ); + } catch { + // execCommand threw — fall through to the error below } finally { document.body.removeChild(textarea); } + throw new Error( + 'Clipboard paste is not supported in this browser on non-HTTPS sites. ' + + 'Please use HTTPS or paste manually with keyboard shortcuts.' + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/clipboard-utils.ts` around lines 118 - 134, The try/catch in readFromClipboardLegacy currently throws a custom Error inside the try which is immediately caught and can mask real errors from document.execCommand('paste'); change the flow to call document.execCommand('paste') inside try, store the boolean result, and only return textarea.value on success; if not successful, move the creation/throwing of the custom Error outside of the try block (after the try) so it isn't caught, and in the catch block rethrow the original error (i.e., throw err) instead of replacing it; this references readFromClipboardLegacy, document.execCommand('paste'), and textarea.value.apps/ui/src/components/views/terminal-view/mobile-terminal-shortcuts.tsx (1)
389-422: ImportComponentTypedirectly from React to avoid fragility withisolatedModulesLine 396 uses
React.ComponentTypewithout importing it. WithisolatedModules: truein tsconfig, relying on theReactnamespace from@types/reactglobals is fragile. Use the explicit type import instead:-import { useCallback, useRef, useEffect, useState } from 'react'; +import { useCallback, useRef, useEffect, useState, type ComponentType } from 'react';Then update the prop:
- icon: React.ComponentType<{ className?: string }>; + icon: ComponentType<{ className?: string }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/mobile-terminal-shortcuts.tsx` around lines 389 - 422, The IconShortcutButton component currently references React.ComponentType without importing ComponentType, which breaks with isolatedModules enabled; import ComponentType from 'react' at the top of the file and update the prop type signature for icon to use ComponentType<{ className?: string }> (i.e., change icon: React.ComponentType<...> to icon: ComponentType<...>) so the type is explicit and resilient to isolatedModules.
🤖 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/dialogs/commit-worktree-dialog.tsx`:
- Around line 813-820: The rendered remote.url may contain embedded credentials;
add a sanitizer and use it when rendering the SelectItem description: implement
a helper named sanitizeRemoteUrl(url: string): string that attempts to parse the
URL (using URL) and redacts parsed.username and parsed.password (e.g., replace
with '****') returning parsed.toString(), falling back to returning the original
url on parse failure (so SSH-style URLs are unchanged), then replace direct uses
of remote.url in the SelectItem description with sanitizeRemoteUrl(remote.url).
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 1534-1547: The button opens terminals without passing the branch
name so add an optional branchName parameter to createTerminal and thread it
into addTerminalToLayout (the same place initialCwd path passes initialBranch)
so the terminal panel header receives the branch; update createTerminal's
signature and its internal call to addTerminalToLayout (and any intermediate
helpers) and then change the worktree button onClick (and the other call sites
around the welcome-screen area noted) to call createTerminal(customCwd,
undefined, currentWorktreePath, currentWorktreeBranch) so the branch preview is
forwarded to the terminal layout.
In `@apps/ui/src/components/views/terminal-view/mobile-terminal-shortcuts.tsx`:
- Around line 216-218: The trailing separator div is rendered unconditionally
causing an orphaned separator when no optional clipboard callbacks are provided;
update the render logic in mobile-terminal-shortcuts.tsx to only render the
separator (the <div className="w-px h-6 bg-border shrink-0" /> between the
sticky modifier keys group and the special keys group) if at least one of the
callbacks onToggleSelectMode, onSelectAll, onCopy, or onPaste is present; i.e.,
wrap that separator in a conditional check for any of those props so it is
omitted when none are passed.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 2381-2444: When viewport flips from mobile to non-mobile the
overlay guarded by (isSelectMode && isMobile) disappears but isSelectMode
remains true with no UI to clear it; add a React useEffect in terminal-panel.tsx
that watches isMobile and, when it becomes false, calls setIsSelectMode(false)
and clears selectModeText via setSelectModeText('') to ensure state is reset;
reference the existing state hooks isSelectMode, isMobile, setIsSelectMode,
selectModeText, and setSelectModeText and run the cleanup effect whenever
isMobile changes.
In `@apps/ui/src/lib/clipboard-utils.ts`:
- Around line 59-70: The try/catch in readFromClipboard swallows permission
errors from navigator.clipboard.readText() and incorrectly falls back to
readFromClipboardLegacy(); change the catch to inspect the thrown error (e.g.,
err.name === 'NotAllowedError' or err.name === 'NotReadableError' or err.code if
present) and rethrow permission-related errors so they propagate to the caller,
while only falling back to readFromClipboardLegacy() for other failure modes;
keep references to the functions readFromClipboard and readFromClipboardLegacy
and ensure the thrown permission error is not converted into the legacy "not
supported" message.
- Around line 20-22: The current secure-context check uses
"window.isSecureContext !== false", which incorrectly passes when
isSecureContext is undefined; update the condition in
apps/ui/src/lib/clipboard-utils.ts to require an explicit true (e.g., use
window.isSecureContext === true or Boolean(window.isSecureContext)) so only
browsers reporting a secure context pass; locate the expression referencing
window.isSecureContext in the clipboard availability check (near the
navigator.clipboard guard) and replace the !== false check with an explicit
true-coercion.
---
Nitpick comments:
In `@apps/ui/src/components/views/terminal-view/mobile-terminal-shortcuts.tsx`:
- Around line 389-422: The IconShortcutButton component currently references
React.ComponentType without importing ComponentType, which breaks with
isolatedModules enabled; import ComponentType from 'react' at the top of the
file and update the prop type signature for icon to use ComponentType<{
className?: string }> (i.e., change icon: React.ComponentType<...> to icon:
ComponentType<...>) so the type is explicit and resilient to isolatedModules.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 479-491: toggleSelectMode's useCallback uses stripAnsi which is
defined inside the component but not included in the dependency array; move the
stripAnsi function declaration out of the component to module scope (above the
component function) so it becomes a stable, pure utility and remove it from the
need to be a dependency, then keep toggleSelectMode as currently defined
(referencing toggleSelectMode, stripAnsi, getTerminalBufferText,
setSelectModeText, setIsSelectMode) to silence the exhaustive-deps lint;
alternatively, if you prefer to keep stripAnsi inside the component, wrap it
with useCallback to memoize it and add it to the toggleSelectMode dependency
array.
- Around line 1666-1716: The touch handler duplicates the boundary-clamping
logic also used in handleContextMenu; extract that logic into a shared helper
(e.g., computeClampedContextMenuPosition or clampContextMenuPosition) that
accepts the desired x,y and optional menuWidth/menuHeight/padding and returns
the clamped { x, y } position, then replace the inline clamping in both
handleTouchStart and handleContextMenu to call this helper and pass its result
to setContextMenu; ensure the helper has correct types and is imported/defined
in the same module so both handleContextMenu and handleTouchStart reference it.
- Around line 1663-1664: The two numeric constants LONG_PRESS_DURATION and
LONG_PRESS_MOVE_THRESHOLD are being recreated on every render; move their
declarations out of the component render scope to the module-level constants
alongside the other top-level constants so they are defined once. Locate the
current in-component definitions of LONG_PRESS_DURATION and
LONG_PRESS_MOVE_THRESHOLD in TerminalPanel (terminal-panel.tsx) and cut them
into the module scope with the other constants, keeping the same values and
names, then remove the local declarations so the component references the
module-level LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD.
In `@apps/ui/src/lib/clipboard-utils.ts`:
- Around line 118-134: The try/catch in readFromClipboardLegacy currently throws
a custom Error inside the try which is immediately caught and can mask real
errors from document.execCommand('paste'); change the flow to call
document.execCommand('paste') inside try, store the boolean result, and only
return textarea.value on success; if not successful, move the creation/throwing
of the custom Error outside of the try block (after the try) so it isn't caught,
and in the catch block rethrow the original error (i.e., throw err) instead of
replacing it; this references readFromClipboardLegacy,
document.execCommand('paste'), and textarea.value.
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/terminal-view/mobile-terminal-shortcuts.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/store/app-store.ts`:
- Around line 2318-2338: The two setters setDefaultThinkingLevel and
setDefaultReasoningEffort call httpApi.put without awaiting, so move the sync
into an async flow: mark the functions (setDefaultThinkingLevel and
setDefaultReasoningEffort) as async (or create an inner async IIFE) and await
the getHttpApiClient().put('/api/settings', ...) inside the try block so the
try/catch can catch rejections and log via logger.error; keep the state update
via set(...) synchronous before awaiting the network call to preserve UI
responsiveness.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… security issues from audit
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
692-741:⚠️ Potential issue | 🟡 MinorUse
mergeState.mergeAffectedFilesas a fallback for merge highlighting.
isMergeFilecurrently depends only onfile.isMergeAffected. If the backend only providesmergeState.mergeAffectedFiles, merge-affected rows won’t show the merge styling/badge even though the banner indicates a merge.✅ Suggested fix
- const isMergeFile = file.isMergeAffected; + const isMergeFile = + file.isMergeAffected ?? + (mergeState?.mergeAffectedFiles?.includes(file.path) ?? false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx` around lines 692 - 741, The merge highlighting only checks file.isMergeAffected so rows miss merge styling when the backend supplies mergeState.mergeAffectedFiles; update the isMergeFile calculation (used where isMergeFile is declared/used in the commit-worktree-dialog rendering) to fallback to checking mergeState.mergeAffectedFiles.includes(file.path) when file.isMergeAffected is falsy (e.g. isMergeFile = file.isMergeAffected || mergeState?.mergeAffectedFiles?.includes(file.path)); ensure null/undefined guards around mergeState and that getMergeTypeLabel(file.mergeType) and any badge rendering still work when only the fallback is true.
🧹 Nitpick comments (10)
apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx (1)
57-59: Reset transient dialog state on open.
rememberChoiceandshowFilespersist across dialog reopens. If the dialog stays mounted, this can surprise users on subsequent merges. Consider clearing them whenopenbecomestrue.💡 Suggested change
-import { useState, useCallback } from 'react'; +import { useState, useCallback, useEffect } from 'react'; const [rememberChoice, setRememberChoice] = useState(false); const [showFiles, setShowFiles] = useState(false); + useEffect(() => { + if (open) { + setRememberChoice(false); + setShowFiles(false); + } + }, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx` around lines 57 - 59, The transient state rememberChoice and showFiles should be cleared each time the dialog is opened; add an effect in the PostMergePromptDialog component that watches the dialog's open prop and when open becomes true calls setRememberChoice(false) and setShowFiles(false) to reset both flags on every reopen. This ensures rememberChoice and showFiles don't persist across mounts/reopens and restores the dialog to its default state whenever open is true.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
455-475: Cap select-mode overlay content to avoid huge DOM renders.
getTerminalBufferText()dumps the entire scrollback into a DOM overlay. On large scrollback settings this can be very heavy on mobile. Consider limiting to the last N lines (or a max char budget).🛠️ Suggested change
- for (let i = 0; i < buffer.length; i++) { + const MAX_SELECT_LINES = 2000; + const start = Math.max(0, buffer.length - MAX_SELECT_LINES); + for (let i = start; i < buffer.length; i++) { const line = buffer.getLine(i); if (line) { lines.push(line.translateToString(true)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 455 - 475, getTerminalBufferText() currently extracts the entire active terminal buffer (via xtermRef.current.buffer.active) which can blow up the DOM on mobile; modify getTerminalBufferText to cap output to a configurable budget by only collecting the last N lines (or enforcing a max character limit) from buffer.active instead of iterating the whole buffer: add a MAX_OVERLAY_LINES (or MAX_OVERLAY_CHARS) constant/prop, compute start index = Math.max(0, buffer.length - MAX_OVERLAY_LINES), iterate from start to buffer.length and build lines (or stop once char budget reached), keep the existing trailing-empty-line trim logic, and reference xtermRef/current buffer and getLine/translateToString as before.apps/ui/src/components/views/terminal-view.tsx (1)
1717-1756: Consider extracting the worktree dropdown items into a small component.The IIFE
(() => {...})()inside JSX works but is uncommon and slightly harder to scan. A small extracted component (e.g.,WorktreeMenuItems) would improve readability. This is purely a readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view.tsx` around lines 1717 - 1756, Extract the IIFE block rendering worktree items into a small React component (e.g., WorktreeMenuItems) to improve readability: create a WorktreeMenuItems component that accepts props currentProject, worktreesByProject, and createTerminalInNewTab, move the logic that computes projectWorktrees, mainWorktree, and featureWorktrees and the JSX that renders DropdownMenuSeparator, DropdownMenuLabel, and the DropdownMenuItem elements (including the mainWorktree item and featureWorktrees.map) into it, and replace the IIFE in terminal-view.tsx with <WorktreeMenuItems {...props} /> while preserving use of FolderGit, GitBranch, and className/handlers.apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx (1)
506-513: Consider using the project's<Checkbox>component instead of a native<input type="checkbox">.The rest of this dialog (and the codebase) uses the
<Checkbox>component from@/components/ui/checkboxfor consistent styling and accessibility. The native checkbox here will look visually different.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx` around lines 506 - 513, Replace the native <input type="checkbox"> used with the rememberChoice state in the GitPullDialog with the project's Checkbox component from "@/components/ui/checkbox": import Checkbox, swap the <input> in the block that references rememberChoice and setRememberChoice to the Checkbox, wire its checked state and use its onCheckedChange handler (which delivers a boolean) to call setRememberChoice, and keep any existing className/aria props to maintain styling and accessibility.libs/git-utils/src/types.ts (2)
79-80: Consider using a string union type formergeType.The JSDoc documents seven possible values, but the field is typed as
string. A union type would provide compile-time safety and autocomplete.♻️ Suggested improvement
- /** Type of merge involvement: 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' | 'deleted-by-them' | 'both-added' | 'both-deleted' */ - mergeType?: string; + /** Type of merge involvement */ + mergeType?: 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' | 'deleted-by-them' | 'both-added' | 'both-deleted' | 'unmerged' | 'merged';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/types.ts` around lines 79 - 80, The mergeType field is currently typed as a plain string despite JSDoc listing seven specific values; update the type to a string literal union (or introduce a named type alias like MergeType and use it) that includes 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' | 'deleted-by-them' | 'both-added' | 'both-deleted' and replace mergeType?: string with mergeType?: MergeType (or the literal union) in the types.ts declaration so callers get compile-time checks and autocomplete.
86-99: ConsolidateMergeStateInfoto@automaker/typespackage.
MergeStateInfois defined identically in bothlibs/git-utils/src/types.tsandapps/ui/src/types/electron.d.ts, violating the single-source-of-truth principle. This core type definition with no external dependencies should live in@automaker/typesper project guidelines. Update@automaker/git-utilsto exportMergeStateInfofrom@automaker/types, and remove the duplicate fromapps/ui/src/types/electron.d.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/types.ts` around lines 86 - 99, Remove the local MergeStateInfo declaration and make the package re-use the single source from `@automaker/types`: delete the MergeStateInfo interface in the git-utils types file, add an import/export so `@automaker/git-utils` re-exports MergeStateInfo from `@automaker/types` (e.g., export type { MergeStateInfo } from '@automaker/types'), and remove the duplicate MergeStateInfo declaration from the UI's electron.d.ts so all consumers use the centralized `@automaker/types` definition.libs/git-utils/src/diff.ts (2)
280-283:git status --porcelainis called twice — once here (line 273) and again insidedetectMergeState.
detectMergeStateinternally runsgit status --porcelain(status.ts:236) to determine merge-affected files. SincegetGitRepositoryDiffsalready has the parsed status output infiles, consider passing the pre-parsed data into the merge-state detection to avoid the redundant call, which could be expensive in large repos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/diff.ts` around lines 280 - 283, getGitRepositoryDiffs currently parses git status into `files` and then calls `detectMergeState(repoPath)` which re-runs `git status --porcelain`; change `detectMergeState` to accept an optional pre-parsed status argument (e.g., `detectMergeState(repoPath, statusFiles?)`) and update its implementation to use `statusFiles` when provided instead of executing `git status` internally, then pass the existing `files` variable from `getGitRepositoryDiffs` into `detectMergeState(repoPath, files)`; keep the original behavior when the second arg is omitted to preserve backwards compatibility.
298-319: O(n·m) lookup for marking existing files as merge-affected.
files.find(f => f.path === filePath)on line 313 runs inside a loop overmergeAffectedFiles. While typically modest in size, building aMap<string, FileStatus>fromfilesonce would make this O(n+m).♻️ Suggested improvement
// Add merge-affected files to the file list (avoid duplicates with working tree changes) const existingPaths = new Set(files.map((f) => f.path)); + const filesByPath = new Map(files.map((f) => [f.path, f])); for (const filePath of mergeCommitInfo.mergeAffectedFiles) { if (!existingPaths.has(filePath)) { files.push({ status: 'M', path: filePath, statusText: 'Merged', indexStatus: ' ', workTreeStatus: ' ', isMergeAffected: true, mergeType: 'merged', }); } else { // Mark existing file as also merge-affected - const existing = files.find((f) => f.path === filePath); + const existing = filesByPath.get(filePath); if (existing) { existing.isMergeAffected = true; existing.mergeType = 'merged'; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/diff.ts` around lines 298 - 319, The current loop over mergeCommitInfo.mergeAffectedFiles does an O(n·m) search using files.find; replace that with a single prebuilt Map keyed by path to make lookups O(1): create a Map (e.g., fileByPath) from the files array (mapping path -> file object), then iterate mergeCommitInfo.mergeAffectedFiles and for each filePath check fileByPath.get(filePath) to mark isMergeAffected=true and set mergeType='merged' or, if missing, push a new FileStatus entry into files and also set it in the map; keep the existingPaths Set logic for avoiding duplicates when pushing new entries.libs/git-utils/src/status.ts (1)
233-245: Broad filter may include files staged before the merge began.Line 241 includes any file with
indexStatus !== ' ' && indexStatus !== '?', which captures files that were independently staged before the merge started — not just merge-affected files. This could overcountmergeAffectedFiles. If this is intentional (showing all "dirty" files during a merge), a comment clarifying the rationale would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/status.ts` around lines 233 - 245, The current filter that populates mergeAffectedFiles is too broad and may include files staged before the merge because it includes any entry with indexStatus !== ' ' && indexStatus !== '?'; narrow it to only include explicit merge-affected entries (use the existing isMergeAffected flag) and, if you need staged results of a merge, only include index statuses that indicate an unmerged/merge state (e.g., indexStatus === 'U' or other merge-specific codes from parseGitStatus) rather than all non-space/non-? statuses; update the filter on mergeAffectedFiles to something like files.filter(f => f.isMergeAffected || f.indexStatus === 'U').map(f => f.path) and add a brief comment explaining the intent (or keep the broader behavior but add a comment clarifying it).apps/ui/src/components/ui/git-diff-panel.tsx (1)
1066-1153: Fallback rendering duplicates merge-aware styling fromFileDiffSection.The merge-related border, background, and icon logic (lines 1074-1091) mirrors what
FileDiffSectionalready does. Consider extracting the shared file-entry chrome (border color, background, icon selection) into a small helper or wrapping component to reduce duplication as the merge feature evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/git-diff-panel.tsx` around lines 1066 - 1153, The fallback block duplicates the merge-aware chrome (border, bg, icon selection) already implemented in FileDiffSection; extract that shared UI into a small reusable component (e.g., FileEntryChrome) that accepts the file object (uses file.path, file.status, file.isMergeAffected, file.mergeType), children for the right-side controls (StagingBadge, status badge, stage/unstage buttons), and props for handlers (handleStageFile, handleUnstageFile, stagingInProgress, enableStaging), then replace the duplicated markup in this file and FileDiffSection to render the new component and keep only the specific preview/footer content here (New/Deleted/No-diff). Ensure the helper delegates icon selection (getFileIcon), MergeBadge rendering, and the isFileMerge className logic (border/bg) so styling and behavior remain 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/pull-service.ts`:
- Around line 315-318: Summary: Merge detection using
pullOutput.includes('Merge') is fragile; instead check whether the new HEAD is a
merge commit (has two parents) after the pull. After performing the pull in
pull-service.ts (where isFastForward and isMerge are set), if not
alreadyUpToDate and not isFastForward, run a git check to see if HEAD has two
parents (e.g., attempt to verify HEAD^2 or run git show -s --pretty=%P HEAD and
count parent hashes) and set isMerge = true only if two parents are present;
otherwise set isMerge = false. Update the logic around the existing
isMerge/isFastForward variables so merge detection relies on commit parent count
rather than localized pull output.
In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx`:
- Around line 505-525: The reset button is never reachable because the dialog's
"merge-complete" phase (set in handleSuccessfulPull) only renders when
mergePostAction is falsy, but the button is currently gated by mergePostAction
being truthy; change the render condition so the Reset preference button is
always available (or at least when rememberChoice is true) by removing the
truthy-only guard around mergePostAction and rendering the button
unconditionally (or conditional on rememberChoice), and optionally show the
current preference text using mergePostAction || 'auto-commit' to make its
purpose clear; update the JSX around the Reset preference button in the
git-pull-dialog component (the rememberChoice / mergePostAction block)
accordingly.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Line 61: The settings field mergePostAction is saved to the server but not
restored in refreshSettingsFromServer; update the restoration by adding
mergePostAction to the useAppStore.setState({...}) call inside
refreshSettingsFromServer so the value from the server payload is applied to the
store (follow the same pattern used for other fields like theme or autoSave),
ensuring you read the server response property and pass it into
useAppStore.setState under the mergePostAction key.
In `@apps/ui/src/store/app-store.ts`:
- Around line 1123-1125: The setter setMergePostAction currently only updates
memory state (mergePostAction); change it to persist the value to the settings
endpoint the same way other settings are synced: after set({ mergePostAction:
action }) call the async settings sync used elsewhere (POST/PATCH to
/api/settings) to save the new mergePostAction value and handle errors, and
ensure initial hydration of mergePostAction reads from the loaded settings; also
confirm the backend settings schema includes the mergePostAction field so the
persisted update succeeds.
In `@libs/git-utils/src/status.ts`:
- Around line 139-170: The calls in detectMergeCommit use unquoted git refs
which break on Windows because cmd.exe treats ^ and ~ specially; change the
execAsync invocations to quote the refs (e.g., use "HEAD^2" in the rev-parse
call and quote the refs in the diff call such as "HEAD~1" and "HEAD") so the
shell receives the intended ref strings, keeping the same execAsync usage and
cwd option and preserving existing try/catch behavior around those calls.
---
Outside diff comments:
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 692-741: The merge highlighting only checks file.isMergeAffected
so rows miss merge styling when the backend supplies
mergeState.mergeAffectedFiles; update the isMergeFile calculation (used where
isMergeFile is declared/used in the commit-worktree-dialog rendering) to
fallback to checking mergeState.mergeAffectedFiles.includes(file.path) when
file.isMergeAffected is falsy (e.g. isMergeFile = file.isMergeAffected ||
mergeState?.mergeAffectedFiles?.includes(file.path)); ensure null/undefined
guards around mergeState and that getMergeTypeLabel(file.mergeType) and any
badge rendering still work when only the fallback is true.
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 897-904: The SelectItem is currently rendering remote.url verbatim
(value tied to remote.name and description showing {remote.url}), which may leak
credentials; add a small sanitizer function (e.g., maskRemoteUrl or
sanitizeRemoteUrl) in commit-worktree-dialog.tsx that parses the URL and strips
or masks the userinfo portion (anything before '@' in the authority, or the
URL.username/URL.password fields), then use that sanitized string in the
SelectItem description instead of remote.url to prevent exposing tokens or
passwords in the UI.
---
Nitpick comments:
In `@apps/ui/src/components/ui/git-diff-panel.tsx`:
- Around line 1066-1153: The fallback block duplicates the merge-aware chrome
(border, bg, icon selection) already implemented in FileDiffSection; extract
that shared UI into a small reusable component (e.g., FileEntryChrome) that
accepts the file object (uses file.path, file.status, file.isMergeAffected,
file.mergeType), children for the right-side controls (StagingBadge, status
badge, stage/unstage buttons), and props for handlers (handleStageFile,
handleUnstageFile, stagingInProgress, enableStaging), then replace the
duplicated markup in this file and FileDiffSection to render the new component
and keep only the specific preview/footer content here (New/Deleted/No-diff).
Ensure the helper delegates icon selection (getFileIcon), MergeBadge rendering,
and the isFileMerge className logic (border/bg) so styling and behavior remain
identical.
In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx`:
- Around line 506-513: Replace the native <input type="checkbox"> used with the
rememberChoice state in the GitPullDialog with the project's Checkbox component
from "@/components/ui/checkbox": import Checkbox, swap the <input> in the block
that references rememberChoice and setRememberChoice to the Checkbox, wire its
checked state and use its onCheckedChange handler (which delivers a boolean) to
call setRememberChoice, and keep any existing className/aria props to maintain
styling and accessibility.
In
`@apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx`:
- Around line 57-59: The transient state rememberChoice and showFiles should be
cleared each time the dialog is opened; add an effect in the
PostMergePromptDialog component that watches the dialog's open prop and when
open becomes true calls setRememberChoice(false) and setShowFiles(false) to
reset both flags on every reopen. This ensures rememberChoice and showFiles
don't persist across mounts/reopens and restores the dialog to its default state
whenever open is true.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 1717-1756: Extract the IIFE block rendering worktree items into a
small React component (e.g., WorktreeMenuItems) to improve readability: create a
WorktreeMenuItems component that accepts props currentProject,
worktreesByProject, and createTerminalInNewTab, move the logic that computes
projectWorktrees, mainWorktree, and featureWorktrees and the JSX that renders
DropdownMenuSeparator, DropdownMenuLabel, and the DropdownMenuItem elements
(including the mainWorktree item and featureWorktrees.map) into it, and replace
the IIFE in terminal-view.tsx with <WorktreeMenuItems {...props} /> while
preserving use of FolderGit, GitBranch, and className/handlers.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 455-475: getTerminalBufferText() currently extracts the entire
active terminal buffer (via xtermRef.current.buffer.active) which can blow up
the DOM on mobile; modify getTerminalBufferText to cap output to a configurable
budget by only collecting the last N lines (or enforcing a max character limit)
from buffer.active instead of iterating the whole buffer: add a
MAX_OVERLAY_LINES (or MAX_OVERLAY_CHARS) constant/prop, compute start index =
Math.max(0, buffer.length - MAX_OVERLAY_LINES), iterate from start to
buffer.length and build lines (or stop once char budget reached), keep the
existing trailing-empty-line trim logic, and reference xtermRef/current buffer
and getLine/translateToString as before.
In `@libs/git-utils/src/diff.ts`:
- Around line 280-283: getGitRepositoryDiffs currently parses git status into
`files` and then calls `detectMergeState(repoPath)` which re-runs `git status
--porcelain`; change `detectMergeState` to accept an optional pre-parsed status
argument (e.g., `detectMergeState(repoPath, statusFiles?)`) and update its
implementation to use `statusFiles` when provided instead of executing `git
status` internally, then pass the existing `files` variable from
`getGitRepositoryDiffs` into `detectMergeState(repoPath, files)`; keep the
original behavior when the second arg is omitted to preserve backwards
compatibility.
- Around line 298-319: The current loop over mergeCommitInfo.mergeAffectedFiles
does an O(n·m) search using files.find; replace that with a single prebuilt Map
keyed by path to make lookups O(1): create a Map (e.g., fileByPath) from the
files array (mapping path -> file object), then iterate
mergeCommitInfo.mergeAffectedFiles and for each filePath check
fileByPath.get(filePath) to mark isMergeAffected=true and set mergeType='merged'
or, if missing, push a new FileStatus entry into files and also set it in the
map; keep the existingPaths Set logic for avoiding duplicates when pushing new
entries.
In `@libs/git-utils/src/status.ts`:
- Around line 233-245: The current filter that populates mergeAffectedFiles is
too broad and may include files staged before the merge because it includes any
entry with indexStatus !== ' ' && indexStatus !== '?'; narrow it to only include
explicit merge-affected entries (use the existing isMergeAffected flag) and, if
you need staged results of a merge, only include index statuses that indicate an
unmerged/merge state (e.g., indexStatus === 'U' or other merge-specific codes
from parseGitStatus) rather than all non-space/non-? statuses; update the filter
on mergeAffectedFiles to something like files.filter(f => f.isMergeAffected ||
f.indexStatus === 'U').map(f => f.path) and add a brief comment explaining the
intent (or keep the broader behavior but add a comment clarifying it).
In `@libs/git-utils/src/types.ts`:
- Around line 79-80: The mergeType field is currently typed as a plain string
despite JSDoc listing seven specific values; update the type to a string literal
union (or introduce a named type alias like MergeType and use it) that includes
'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' |
'deleted-by-them' | 'both-added' | 'both-deleted' and replace mergeType?: string
with mergeType?: MergeType (or the literal union) in the types.ts declaration so
callers get compile-time checks and autocomplete.
- Around line 86-99: Remove the local MergeStateInfo declaration and make the
package re-use the single source from `@automaker/types`: delete the
MergeStateInfo interface in the git-utils types file, add an import/export so
`@automaker/git-utils` re-exports MergeStateInfo from `@automaker/types` (e.g.,
export type { MergeStateInfo } from '@automaker/types'), and remove the
duplicate MergeStateInfo declaration from the UI's electron.d.ts so all
consumers use the centralized `@automaker/types` definition.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/pull-service.ts (1)
366-377:⚠️ Potential issue | 🟠 MajorMerge/fast-forward info is lost when stash is involved.
The variables
isMerge,isFastForward, andmergeAffectedFilesare scoped to thetryblock (lines 341–363). WhendidStashistrue, execution falls through to line 432, which delegates toreapplyStash()— butreapplyStash()never receives or returns these fields. This means the HTTP response will silently omit merge info wheneverstashIfNeededwas used.Hoist the merge-detection variables above the
tryblock and pass them intoreapplyStash()(or merge them into its result).Proposed fix
Declare the variables before the try block and thread them through:
// 8. Pull latest changes + let isFastForward = false; + let isMerge = false; + let mergeAffectedFiles: string[] = []; const pullArgs = upstreamStatus === 'tracking' ? ['pull'] : ['pull', targetRemote, branchName]; let pullConflict = false; let pullConflictFiles: string[] = []; try { const pullOutput = await execGitCommand(pullArgs, worktreePath); const alreadyUpToDate = pullOutput.includes('Already up to date'); - const isFastForward = + isFastForward = pullOutput.includes('Fast-forward') || pullOutput.includes('fast-forward'); - const isMerge = !alreadyUpToDate && !isFastForward ? await isMergeCommit(worktreePath) : false; + isMerge = !alreadyUpToDate && !isFastForward ? await isMergeCommit(worktreePath) : false; - let mergeAffectedFiles: string[] = []; if (isMerge) { // ... existing code ... }Then in
reapplyStash(or after it returns), spread the merge info into the result:if (didStash) { - return await reapplyStash(worktreePath, branchName); + const stashResult = await reapplyStash(worktreePath, branchName); + return { + ...stashResult, + ...(isMerge ? { isMerge: true, mergeAffectedFiles } : {}), + ...(isFastForward ? { isFastForward: true } : {}), + }; }Also applies to: 432-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pull-service.ts` around lines 366 - 377, The merge detection variables isMerge, isFastForward, and mergeAffectedFiles are declared inside the try block and thus lost when didStash is true; hoist/declare these variables (e.g., let isMerge = false, let isFastForward = false, let mergeAffectedFiles: string[] = []) before the try block in pull-service.ts, assign them inside the try where merge detection occurs, and then thread them into the stash path by passing them as arguments to reapplyStash() or by merging/spreading them into the object returned from reapplyStash() so the final response includes the merge/fast-forward info when stashIfNeeded was used.
🧹 Nitpick comments (7)
apps/server/src/services/pull-service.ts (1)
340-345: Locale-sensitiveisFastForwarddetection via string matching.
isFastForwardstill relies on matching"Fast-forward"/"fast-forward"in the pull output, which is locale-dependent (e.g., a German Git locale would print"Vorspulen"). SinceisMergewas moved to the more robust parent-count approach, consider applying a similar strategy forisFastForward— for instance, comparing the pre-pull and post-pull HEAD SHAs against the merge-base, or checking if the new HEAD is an ancestor of the fetched ref.That said, since this is informational and the old code had the same pattern, it's not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pull-service.ts` around lines 340 - 345, Replace the locale-sensitive string match used to set isFastForward with a commit-ancestry check: record the HEAD SHA before the pull and the HEAD SHA after the pull (use existing worktreePath context), then set isFastForward = true if the pre-pull HEAD is an ancestor of the post-pull HEAD (e.g., via a new helper like isAncestorCommit(worktreePath, preHead, postHead) or by using git merge-base --is-ancestor). Update the block that currently computes isFastForward (which reads pullOutput) to use this pre/post-HEAD ancestry test and keep the existing isMerge logic that calls isMergeCommit.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
319-329: In-place sort mutates thefileListarray.
fileListreferencesresult.files ?? []. If the API returns the same array by reference, this mutates it. Since the sorted result is immediately consumed and set into state, this is unlikely to cause issues in practice, but creating a copy first would be more defensive.♻️ Defensive copy before sorting
- const fileList = result.files ?? []; - // Sort merge-affected files first when a merge is in progress - if (result.mergeState?.isMerging) { - const mergeSet = new Set(result.mergeState.mergeAffectedFiles); - fileList.sort((a, b) => { + const fileList = [...(result.files ?? [])]; + // Sort merge-affected files first when a merge is in progress + if (result.mergeState?.isMerging) { + const mergeSet = new Set(result.mergeState.mergeAffectedFiles); + fileList.sort((a, b) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx` around lines 319 - 329, The in-place sort mutates result.files via fileList (in CommitWorktreeDialog), so make a defensive copy before sorting: when assigning fileList from result.files ?? [], create a shallow copy (e.g., via spread or slice) and operate on that copy prior to the sort that references result.mergeState?.isMerging and result.mergeState.mergeAffectedFiles; update the state/consumer with the copied-and-sorted array instead of mutating the original result.files array.apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx (1)
102-132:handleSuccessfulPullauto-commit path silently falls back to plain success whenonCommitMergeis missing.When
mergePostAction === 'commit'butonCommitMergeis not provided, the dialog shows the generic "Pull Complete" success phase with no indication a merge occurred. Consider either falling through to the merge-prompt ('merge-complete'phase) when the commit callback isn't available, or logging a warning.♻️ Proposed fallback to merge prompt
if (mergePostAction === 'commit') { - // User preference: auto-commit - setPhase('success'); - onPulled?.(); - // Auto-trigger commit dialog if (worktree && onCommitMerge) { + // User preference: auto-commit + setPhase('success'); + onPulled?.(); onCommitMerge(worktree); onOpenChange(false); + } else { + // Commit callback unavailable — fall back to merge prompt + setPhase('merge-complete'); + onPulled?.(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx` around lines 102 - 132, handleSuccessfulPull currently assumes auto-commit will run when mergePostAction === 'commit' even if onCommitMerge is undefined, causing a silent fallback to plain success; update the branch in handleSuccessfulPull to first check that onCommitMerge and worktree exist before treating it as an auto-commit: if both exist, keep the current behavior (setPhase('success'), call onPulled, call onCommitMerge(worktree) and onOpenChange(false)); otherwise, fall back to setPhase('merge-complete') and call onPulled and additionally emit a warning (e.g., console.warn) indicating mergePostAction was 'commit' but onCommitMerge was not provided so the UI shows the merge prompt instead.apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx (1)
154-158: Prefer the project's<Checkbox>component over native<input type="checkbox">.The sibling dialogs (
git-pull-dialog.tsx,commit-worktree-dialog.tsx) use the<Checkbox>component from@/components/ui/checkboxfor the same "remember choice" pattern. Using a native<input>here creates a visual inconsistency and bypasses any shared styling/accessibility behavior.♻️ Proposed fix
+import { Checkbox } from '@/components/ui/checkbox';- <label className="flex items-center gap-2 text-xs text-muted-foreground cursor-pointer"> - <input - type="checkbox" - checked={rememberChoice} - onChange={(e) => setRememberChoice(e.target.checked)} - className="rounded border-border" - /> + <label className="flex items-center gap-2 text-xs text-muted-foreground cursor-pointer"> + <Checkbox + checked={rememberChoice} + onCheckedChange={(checked) => setRememberChoice(checked === true)} + className="rounded border-border" + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx` around lines 154 - 158, Replace the native checkbox input used for the "remember choice" state with the project's shared Checkbox component: swap the <input type="checkbox"> element (the one wired to checked={rememberChoice} and onChange={e => setRememberChoice(e.target.checked)}) for the imported Checkbox from "@/components/ui/checkbox", pass the same checked and onCheckedChange (or onChange) handlers to maintain state updates via rememberChoice and setRememberChoice, and ensure you keep the same className/label text so styling and accessibility match sibling dialogs (git-pull-dialog.tsx, commit-worktree-dialog.tsx).libs/git-utils/src/diff.ts (1)
282-296: Redundantgit diffinvocation —detectMergeCommitalready fetches affected file names.
detectMergeCommit(instatus.ts) runsgit diff --name-only "HEAD~1" "HEAD"to populatemergeAffectedFiles. Then, if a merge commit is detected,diff.tsimmediately runs a secondgit diff HEAD~1 HEADat line 293 for the full diff content. The two calls can't be merged trivially given current API boundaries, but consider extendingdetectMergeCommitto optionally return the full diff to avoid spawning two processes against the same commit range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/diff.ts` around lines 282 - 296, The code redundantly invokes git diff in diff.ts after detectMergeCommit already ran git diff --name-only; update detectMergeCommit (in status.ts) to optionally return the full diff (e.g., add a parameter includeDiff or include a mergeDiff property on its result) and have diff.ts use mergeCommitInfo.mergeDiff instead of running execAsync('git diff HEAD~1 HEAD') again; modify detectMergeCommit to run git diff HEAD~1 HEAD only when requested and propagate that diff string back to the caller so mergeCommitInfo.isMergeCommit, mergeCommitInfo.mergeAffectedFiles and mergeCommitInfo.mergeDiff can be used by the existing merge-handling logic in diff.ts.libs/git-utils/src/types.ts (1)
80-83:mergeType?: stringshould be a union literal type.The JSDoc documents specific values, but the field accepts any
string. In practice, two additional undocumented values are written —'unmerged'(instatus.ts) and'merged'(indiff.ts) — that would be caught as type errors with a proper union. Tightening the type here makes the contract explicit and prevents drift.♻️ Proposed fix
- /** Type of merge involvement: 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' | 'deleted-by-them' | 'both-added' | 'both-deleted' */ - mergeType?: string; + /** Type of merge involvement */ + mergeType?: + | 'both-modified' + | 'added-by-us' + | 'added-by-them' + | 'deleted-by-us' + | 'deleted-by-them' + | 'both-added' + | 'both-deleted' + | 'unmerged' + | 'merged';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/git-utils/src/types.ts` around lines 80 - 83, The mergeType property currently typed as string should be changed to a union literal type to match the documented values and existing uses; update the declaration of mergeType (in libs/git-utils/src/types.ts) to a union of 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' | 'deleted-by-them' | 'both-added' | 'both-deleted' and include the two additional values seen in the codebase: 'unmerged' and 'merged', and adjust the JSDoc to reflect the exact allowed literals so the compiler enforces the contract.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
483-495:stripAnsiis missing fromtoggleSelectMode'suseCallbackdependency array
toggleSelectModecloses overstripAnsi(line 491), butstripAnsiis defined inside the component as a plain arrow function (recreated each render) and is absent from theuseCallbackdeps ([isSelectMode, getTerminalBufferText]).react-hooks/exhaustive-depswould flag this. SincestripAnsiis a pure utility with no closure dependencies on component state or refs, hoisting it to module scope is the cleanest fix and also aligns with whereMAX_OVERLAY_LINESalready lives.
LONG_PRESS_DURATIONandLONG_PRESS_MOVE_THRESHOLD(Lines 1681–1682) are similarly pure constants safe to move to module scope.♻️ Proposed refactor
+// Module-level ANSI-stripping utility — pure function, no component deps +const stripAnsi = (text: string): string => + text.replace( + // eslint-disable-next-line no-control-regex + /\x1b\[[0-9;]*[a-zA-Z]|\x1b\][^\x07]*\x07|\x1b[()][AB012]|\x1b[>=<]|\x1b[78HM]|\x1b#[0-9]|\x1b./g, + '' + ); + +const LONG_PRESS_DURATION = 500; +const LONG_PRESS_MOVE_THRESHOLD = 10; const logger = createLogger('Terminal'); ...Then remove the inline definitions of
stripAnsi(line 312) and the twoLONG_PRESS_*constants (lines 1681–1682) from inside the component body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 483 - 495, The toggleSelectMode useCallback closes over stripAnsi which is defined inside the component and therefore recreated each render; hoist stripAnsi to module scope (next to MAX_OVERLAY_LINES) and remove the inline stripAnsi definition from the component, then update toggleSelectMode's dependency array (or leave as [isSelectMode, getTerminalBufferText] if stripAnsi is module-scoped) so linting stops flagging it; likewise move the pure constants LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD out of the component to module scope and remove their inline definitions so they are stable across renders.
🤖 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/dialogs/git-pull-dialog.tsx`:
- Around line 509-511: The onCheckedChange handler for the Radix Checkbox is
passing a value of type boolean | 'indeterminate' directly to setRememberChoice
(rememberChoice / setRememberChoice), causing a type mismatch; update the
handler to coerce the incoming value to a boolean before calling
setRememberChoice (e.g., treat 'indeterminate' as false or true per desired UX)
or narrow the type with an explicit check (typeof checked === 'boolean') and
only call setRememberChoice with a boolean, ensuring the Checkbox
onCheckedChange callback handles both boolean and 'indeterminate' branches.
- Around line 519-526: The Current label falls back to 'auto-commit' when
mergePostAction is null, which misleads users who haven't saved a preference;
update the fallback for mergePostAction in the git-pull-dialog.tsx rendering
(the span showing "Current:") to explicitly show a neutral message like "no
preference" or "not set" when mergePostAction is null/undefined, keeping the
existing branches for mergePostAction === 'commit' -> 'auto-commit' and ===
'manual' -> 'manual review'; ensure this change is visible when rememberChoice
is true so the UI doesn't imply a saved choice.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1684-1719: handleTouchStart can leave an orphaned timeout when
multiple touchstart events occur; before creating a new timeout, clear any
existing timer in longPressTimerRef (use clearTimeout) and then create the new
timeout and store its id in longPressTimerRef.current. Also capture the initial
touch coordinates into a local immutable object (e.g., const startPos = { x:
touch.clientX, y: touch.clientY }) and have the timeout closure use that local
startPos instead of reading longPressTouchStartRef.current, then null out
longPressTouchStartRef.current when cancelling or after firing; reference:
handleTouchStart, longPressTimerRef, longPressTouchStartRef, setContextMenu,
LONG_PRESS_DURATION.
In `@libs/git-utils/src/diff.ts`:
- Around line 303-323: The code assigns mergeType = 'merged' in places like the
newFile object and when updating existing (see newFile, existing, fileByPath,
files, existingPaths), but FileStatus.mergeType's documented union in types.ts
doesn't include 'merged' — tighten the FileStatus.mergeType type to a string
union that includes 'merged' (and update the JSDoc comment accordingly) so these
assignments are valid; alternatively, if 'merged' was unintended, change those
assignments to one of the existing allowed union values consistently across the
newFile creation and the existing.mergeType update.
- Around line 355-374: When the git diff command (execAsync('git diff HEAD~1
HEAD', ...)) fails the catch currently logs and continues but still returns
mergeState.mergeAffectedFiles from mergeCommitInfo while files[] remains empty;
to keep them consistent, update the catch block to either clear
mergeCommitInfo.mergeAffectedFiles or (preferable) iterate
mergeCommitInfo.mergeAffectedFiles and push corresponding minimal file entries
into files[] (matching the shape used elsewhere and setting the isMergeAffected
flag) so that the returned files array contains entries for every path in
mergeState.mergeAffectedFiles; adjust any related flags like hasChanges or
isCleanMerge accordingly before returning.
---
Outside diff comments:
In `@apps/server/src/services/pull-service.ts`:
- Around line 366-377: The merge detection variables isMerge, isFastForward, and
mergeAffectedFiles are declared inside the try block and thus lost when didStash
is true; hoist/declare these variables (e.g., let isMerge = false, let
isFastForward = false, let mergeAffectedFiles: string[] = []) before the try
block in pull-service.ts, assign them inside the try where merge detection
occurs, and then thread them into the stash path by passing them as arguments to
reapplyStash() or by merging/spreading them into the object returned from
reapplyStash() so the final response includes the merge/fast-forward info when
stashIfNeeded was used.
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 899-906: The SelectItem is rendering remote.url verbatim which can
leak embedded credentials; create and call a sanitizer (e.g., sanitizeRemoteUrl
or sanitizeUrl) when rendering the description for the SelectItem in
CommitWorktreeDialog (or the component containing SelectItem) to strip any
userinfo (username:password@) before display—implement sanitization using the
URL parser when possible (new URL(remote.url) and omit username/password) with a
regex fallback to remove "userinfo@" for non-HTTP schemes, and replace usages of
remote.url in the SelectItem description with the sanitized value.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1637-1646: No change required: the useEffect that resets
isSelectMode and setSelectModeText when isMobile becomes false is correct; leave
the effect as-is (useEffect with dependency [isMobile]) and keep the
eslint-disable-line suppression and explanatory comment to document why
isSelectMode was omitted, referencing the useEffect, isSelectMode,
setIsSelectMode, and setSelectModeText identifiers.
---
Nitpick comments:
In `@apps/server/src/services/pull-service.ts`:
- Around line 340-345: Replace the locale-sensitive string match used to set
isFastForward with a commit-ancestry check: record the HEAD SHA before the pull
and the HEAD SHA after the pull (use existing worktreePath context), then set
isFastForward = true if the pre-pull HEAD is an ancestor of the post-pull HEAD
(e.g., via a new helper like isAncestorCommit(worktreePath, preHead, postHead)
or by using git merge-base --is-ancestor). Update the block that currently
computes isFastForward (which reads pullOutput) to use this pre/post-HEAD
ancestry test and keep the existing isMerge logic that calls isMergeCommit.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 319-329: The in-place sort mutates result.files via fileList (in
CommitWorktreeDialog), so make a defensive copy before sorting: when assigning
fileList from result.files ?? [], create a shallow copy (e.g., via spread or
slice) and operate on that copy prior to the sort that references
result.mergeState?.isMerging and result.mergeState.mergeAffectedFiles; update
the state/consumer with the copied-and-sorted array instead of mutating the
original result.files array.
In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx`:
- Around line 102-132: handleSuccessfulPull currently assumes auto-commit will
run when mergePostAction === 'commit' even if onCommitMerge is undefined,
causing a silent fallback to plain success; update the branch in
handleSuccessfulPull to first check that onCommitMerge and worktree exist before
treating it as an auto-commit: if both exist, keep the current behavior
(setPhase('success'), call onPulled, call onCommitMerge(worktree) and
onOpenChange(false)); otherwise, fall back to setPhase('merge-complete') and
call onPulled and additionally emit a warning (e.g., console.warn) indicating
mergePostAction was 'commit' but onCommitMerge was not provided so the UI shows
the merge prompt instead.
In
`@apps/ui/src/components/views/board-view/dialogs/post-merge-prompt-dialog.tsx`:
- Around line 154-158: Replace the native checkbox input used for the "remember
choice" state with the project's shared Checkbox component: swap the <input
type="checkbox"> element (the one wired to checked={rememberChoice} and
onChange={e => setRememberChoice(e.target.checked)}) for the imported Checkbox
from "@/components/ui/checkbox", pass the same checked and onCheckedChange (or
onChange) handlers to maintain state updates via rememberChoice and
setRememberChoice, and ensure you keep the same className/label text so styling
and accessibility match sibling dialogs (git-pull-dialog.tsx,
commit-worktree-dialog.tsx).
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 483-495: The toggleSelectMode useCallback closes over stripAnsi
which is defined inside the component and therefore recreated each render; hoist
stripAnsi to module scope (next to MAX_OVERLAY_LINES) and remove the inline
stripAnsi definition from the component, then update toggleSelectMode's
dependency array (or leave as [isSelectMode, getTerminalBufferText] if stripAnsi
is module-scoped) so linting stops flagging it; likewise move the pure constants
LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD out of the component to module
scope and remove their inline definitions so they are stable across renders.
In `@libs/git-utils/src/diff.ts`:
- Around line 282-296: The code redundantly invokes git diff in diff.ts after
detectMergeCommit already ran git diff --name-only; update detectMergeCommit (in
status.ts) to optionally return the full diff (e.g., add a parameter includeDiff
or include a mergeDiff property on its result) and have diff.ts use
mergeCommitInfo.mergeDiff instead of running execAsync('git diff HEAD~1 HEAD')
again; modify detectMergeCommit to run git diff HEAD~1 HEAD only when requested
and propagate that diff string back to the caller so
mergeCommitInfo.isMergeCommit, mergeCommitInfo.mergeAffectedFiles and
mergeCommitInfo.mergeDiff can be used by the existing merge-handling logic in
diff.ts.
In `@libs/git-utils/src/types.ts`:
- Around line 80-83: The mergeType property currently typed as string should be
changed to a union literal type to match the documented values and existing
uses; update the declaration of mergeType (in libs/git-utils/src/types.ts) to a
union of 'both-modified' | 'added-by-us' | 'added-by-them' | 'deleted-by-us' |
'deleted-by-them' | 'both-added' | 'both-deleted' and include the two additional
values seen in the codebase: 'unmerged' and 'merged', and adjust the JSDoc to
reflect the exact allowed literals so the compiler enforces the contract.
apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx
Outdated
Show resolved
Hide resolved
…g.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…g.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…e state consistently
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx (2)
452-459: Plain<button>elements missingtype="button".Both the "Show/Hide affected files" toggle (line 452) and the "Reset preference" button (line 527) omit the
typeattribute, making them implicitlytype="submit". This is a minor best-practice issue.🔧 Proposed fix
- onClick={() => setShowMergeFiles(!showMergeFiles)} + type="button" + onClick={() => setShowMergeFiles(!showMergeFiles)}- onClick={() => { + type="button" + onClick={() => { setMergePostAction(null);Also applies to: 527-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx` around lines 452 - 459, The toggle and reset buttons in the GitPullDialog are plain <button> elements without an explicit type, making them default to type="submit"; update the two buttons (the "Show/Hide affected files" button in the block using setShowMergeFiles and the "Reset preference" button around lines handling reset preference) to include type="button" to prevent accidental form submission—locate the button with onClick={() => setShowMergeFiles(!showMergeFiles)} and the Reset preference button and add type="button" to each.
106-116: RedundantsetPhase('success')in the auto-commit path.When both
worktreeandonCommitMergeare present,setPhase('success')at line 110 queues a state update that is immediately superseded byonOpenChange(false)at line 115 — the success UI never renders. ThesetPhase('success')call is only meaningful as a fallback whenonCommitMergeis absent, so consider restructuring to avoid the spurious update on the happy path.🔧 Proposed refactor
if (mergePostAction === 'commit') { - setPhase('success'); onPulled?.(); if (worktree && onCommitMerge) { onCommitMerge(worktree); onOpenChange(false); + } else { + setPhase('success'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx` around lines 106 - 116, The call to setPhase('success') is redundant when auto-committing because onCommitMerge(worktree) followed by onOpenChange(false) immediately closes the dialog and the success state never renders; update the logic in the git-pull dialog so setPhase('success') is only executed when mergePostAction === 'commit' AND there is no onCommitMerge/worktree handler (i.e., move or remove the setPhase('success') before invoking onCommitMerge and onOpenChange, or place it in the else branch where onCommitMerge is absent), while preserving the existing calls to onPulled, onCommitMerge(worktree) and onOpenChange(false); refer to setPhase, onCommitMerge, onPulled, onOpenChange, worktree and mergePostAction to locate and adjust the code paths.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
1681-1682: HoistLONG_PRESS_DURATIONandLONG_PRESS_MOVE_THRESHOLDto module scope.Every other threshold/timing constant in this file (
RESIZE_DEBOUNCE_MS,MAX_RECONNECT_ATTEMPTS,INITIAL_RECONNECT_DELAY,MAX_OVERLAY_LINES, etc.) lives at module level. These two primitive constants are re-declared on every render even though they never change.♻️ Proposed change
+// Long-press constants for mobile context menu +const LONG_PRESS_DURATION = 500; // ms +const LONG_PRESS_MOVE_THRESHOLD = 10; // px - cancel if finger moves more than this + // Mobile overlay buffer cap - limit lines read from terminal buffer to avoid DOM blow-up on mobile const MAX_OVERLAY_LINES = 1000;Then remove the two inline
constdeclarations at lines 1681-1682 inside the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx` around lines 1681 - 1682, Hoist the two timing constants LONG_PRESS_DURATION and LONG_PRESS_MOVE_THRESHOLD out of the React component to module scope (so they live alongside other module-level constants like RESIZE_DEBOUNCE_MS), then remove the inline const declarations inside the TerminalPanel component (or the function where they currently appear). Ensure they remain unchanged in name and value and are imported/used directly from module scope by any handlers (e.g., long-press logic) that referenced the previous inline constants.
🤖 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/dialogs/git-pull-dialog.tsx`:
- Around line 102-132: In handleSuccessfulPull, do not invoke onPulled
immediately when entering the 'merge-complete' branch; instead remove the
onPulled?.() call from the merge-complete path and call onPulled only from the
user-action handlers (e.g., handleCommitMerge and handleMergeManually) after the
user chooses an action so behavior matches the conflict phase; update those
handlers to invoke onPulled (and close the dialog if appropriate) and ensure any
related state updates (setPhase, worktree usage) remain correct and the
handleSuccessfulPull dependency list still includes symbols it reads.
- Around line 256-264: The Commit Merge button currently calls handleCommitMerge
which sets setMergePostAction('commit'), closes the dialog and does nothing
visible if onCommitMerge is undefined; update the UI/handler so users get
feedback: either (A) hide or disable the Commit Merge button when the prop
onCommitMerge is not provided (adjust the render condition where the button is
created to check onCommitMerge and/or worktree), or (B) keep the button but add
a fallback in handleCommitMerge that shows a toast/error message when
onCommitMerge is undefined instead of silently closing (do not persist the
rememberChoice when no handler exists), referencing the handleCommitMerge
function and the
rememberChoice/setMergePostAction/onCommitMerge/worktree/onOpenChange symbols to
implement the chosen fix.
---
Duplicate comments:
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1637-1646: The reviewer noted this is correct and the duplicate
review comment is unnecessary; remove the duplicate reviewer comment and keep
the useEffect as-is (the effect using isMobile, isSelectMode, setIsSelectMode,
setSelectModeText) including the intentional eslint-disable-line
react-hooks/exhaustive-deps so the cleanup logic remains unchanged and
documented.
- Around line 1684-1725: Duplicate review comment detected; remove the duplicate
marker and keep the updated implementation. Delete the redundant
"[duplicate_comment]" text in the PR comment and ensure the implemented logic
inside handleTouchStart (using longPressTimerRef, longPressTouchStartRef and
setContextMenu) remains as shown—clearing any existing timer, capturing startPos
locally, and using startPos inside the setTimeout closure. Confirm no other
duplicate review fragments remain.
In `@libs/git-utils/src/diff.ts`:
- Around line 303-323: The code sets mergeType: 'merged' on the newFile object
and existing.mergeType but 'merged' is not part of the documented
FileStatus.mergeType union; update this to keep the implementation and
docs/types consistent by either (A) replacing the literal 'merged' in the
newFile initializer and the existing.mergeType assignment with the correct
documented union member that represents a merged file (e.g., 'both-modified' or
the appropriate existing value), or (B) add 'merged' to the FileStatus.mergeType
JSDoc/union so the docs/types and code match; locate the fixes around the
newFile creation (newFile), the existing := fileByPath.get(filePath) branch
(existing.mergeType), and any duplicate occurrences reported later and apply the
same change.
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx`:
- Around line 452-459: The toggle and reset buttons in the GitPullDialog are
plain <button> elements without an explicit type, making them default to
type="submit"; update the two buttons (the "Show/Hide affected files" button in
the block using setShowMergeFiles and the "Reset preference" button around lines
handling reset preference) to include type="button" to prevent accidental form
submission—locate the button with onClick={() =>
setShowMergeFiles(!showMergeFiles)} and the Reset preference button and add
type="button" to each.
- Around line 106-116: The call to setPhase('success') is redundant when
auto-committing because onCommitMerge(worktree) followed by onOpenChange(false)
immediately closes the dialog and the success state never renders; update the
logic in the git-pull dialog so setPhase('success') is only executed when
mergePostAction === 'commit' AND there is no onCommitMerge/worktree handler
(i.e., move or remove the setPhase('success') before invoking onCommitMerge and
onOpenChange, or place it in the else branch where onCommitMerge is absent),
while preserving the existing calls to onPulled, onCommitMerge(worktree) and
onOpenChange(false); refer to setPhase, onCommitMerge, onPulled, onOpenChange,
worktree and mergePostAction to locate and adjust the code paths.
In `@apps/ui/src/components/views/terminal-view/terminal-panel.tsx`:
- Around line 1681-1682: Hoist the two timing constants LONG_PRESS_DURATION and
LONG_PRESS_MOVE_THRESHOLD out of the React component to module scope (so they
live alongside other module-level constants like RESIZE_DEBOUNCE_MS), then
remove the inline const declarations inside the TerminalPanel component (or the
function where they currently appear). Ensure they remain unchanged in name and
value and are imported/used directly from module scope by any handlers (e.g.,
long-press logic) that referenced the previous inline constants.
apps/ui/src/components/views/board-view/dialogs/git-pull-dialog.tsx
Outdated
Show resolved
Hide resolved
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
Concurrency Limit Changes
checkWorktreeCapacityvalidation that was blocking manual feature starts. Manual features now bypass the concurrency limit but their presence is counted by the auto-loop coordinator when deciding whether to dispatch new auto-mode tasks, ensuring total system load is still respected.Remote Fetching Improvements
fetchRemotes()helper function with 30-second timeout to fetch latest from all remotes before branch checkout. This ensures remote refs are up-to-date when validating remote references like "origin/main". Fetch failures are logged but non-fatal to allow workflow continuation.detectRemoteBranch()logic with a consolidatedfetch --allapproach before worktree creation. This ensures all remote refs are current regardless of which base branch is specified.fetchRemotes()helper with timeout and comprehensive documentation explaining the fetch operation's role in the workflow. Fetch operations are non-fatal and logged at warn level on timeout.Auto-Mode Failure Tracking
autoModeOnlyparameter togetRunningCountForWorktree()for future flexibility.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests