Feature: Add PR review comments and resolution, improve AI prompt handling#790
Merged
gsxdsm merged 8 commits intoAutoMaker-Org:v0.15.0rcfrom Feb 21, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end PR review comment resolution: server routes and services to fetch/resolve review threads, UI dialog and hooks for listing/resolving comments, React Query keys/mutations, and related UI wiring plus assorted terminal-scripts, commands-and-scripts, file-ops, and UX updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as PR Comment Dialog
participant Hooks as Query/Mutation Hooks
participant Client as HttpApiClient
participant Server as Server Route
participant Service as PR Review Service
User->>UI: open dialog / select comment(s)
UI->>Hooks: call useResolveReviewThread(threadId, resolve)
Hooks->>Client: POST /api/github/resolve-pr-comment
Client->>Server: HTTP request
Server->>Service: executeReviewThreadMutation(projectPath, threadId, resolve)
Service->>Service: build GraphQL mutation & run gh CLI (stdin/stdout)
Service-->>Server: return { isResolved }
Server-->>Client: response { success, isResolved }
Client-->>Hooks: mutation result
Hooks->>Hooks: invalidate queryKeys.github.prReviewComments(...)
Hooks-->>UI: mutation result
UI->>User: show success / error state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 significantly enhances the application's integration with GitHub by introducing a full PR review comment management system, allowing users to efficiently address feedback. It also streamlines project configuration with a new unified 'Commands & Scripts' settings area and refines AI prompt handling for more precise output. Various UI/UX improvements have been made across the board and worktree panels, alongside better integration for creating features from GitHub issues, aiming to provide a more cohesive and productive developer experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces a comprehensive PR review comment workflow, a new commands and scripts settings section, and several UX improvements. The backend additions for fetching and resolving comments are well-structured, and the new PRCommentResolutionDialog is a very well-designed and feature-rich component. The improvements to AI prompt handling are also excellent, making the generated content more reliable. I've found one minor area for improvement in how outdated comments are detected, but overall this is a very strong contribution.
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts (1)
109-111:⚠️ Potential issue | 🟡 MinorUnhandled promise from async
selectHighlightedProject()in keydown handler.On Line 111,
selectHighlightedProject()is now async but the returned promise isn't awaited or.catch()-ed. If the upstreamsetCurrentProjectever rejects (e.g., due to an unexpected error ininitializeProjectthat escapes the try/catch), this becomes an unhandled promise rejection. Currently the callers guard with try/catch so the risk is low, but adding a.catch()here is a cheap safeguard.🛡️ Proposed fix
} else if (event.key === 'Enter') { event.preventDefault(); - selectHighlightedProject(); + selectHighlightedProject().catch(() => { + // Error already logged upstream + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/hooks/use-project-picker.ts` around lines 109 - 111, The keydown handler calls the now-async selectHighlightedProject() without handling its returned promise, risking unhandled rejections; update the handler (the block where event.key === 'Enter') to handle the promise by either awaiting selectHighlightedProject() if you make the handler async, or append .catch(...) to selectHighlightedProject() to log or swallow errors; ensure any errors from downstream functions like setCurrentProject or initializeProject are caught so they don't become unhandled promise rejections.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts (1)
78-116:⚠️ Potential issue | 🟡 MinorMinor:
url-detectedevent is silently dropped if the server isn't in the map yet.On line 84, if the server key doesn't exist in the map (e.g., a
url-detectedevent arrives before thedev-server:startedevent or beforefetchDevServerscompletes), the update is silently skipped. If event ordering from the backend guaranteesstartedalways precedesurl-detected, this is fine. Otherwise, you may want to upsert the entry onurl-detectedas a fallback.Possible defensive upsert
setRunningDevServers((prev) => { const existing = prev.get(key); - if (!existing) return prev; // Avoid updating if already detected with same url/port - if (existing.urlDetected && existing.url === url && existing.port === port) return prev; + if (existing?.urlDetected && existing.url === url && existing.port === port) return prev; const next = new Map(prev); next.set(key, { - ...existing, + ...existing, // spreads undefined safely if no prior entry + worktreePath, url, port, 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 78 - 116, The handler for event.type === 'dev-server:url-detected' currently returns early when no existing entry is found in setRunningDevServers (via normalizePath and existing), silently dropping the event; change the update to upsert instead: inside setRunningDevServers (the closure used for state updates) create a new Map(prev) and if existing is missing create a new entry for key that at minimum sets url, port, urlDetected: true (and any default fields like running: true or other expected shape) or merges with defaults if present, otherwise merge with existing as before; ensure didUpdate is set when you insert a new entry so the logger.info and toastShownForRef behavior still runs, and use buildDevServerBrowserUrl and toastShownForRef as currently done for toasts.apps/server/src/routes/worktree/routes/generate-commit-message.ts (1)
32-51: 🛠️ Refactor suggestion | 🟠 Major
withTimeoutdoesn't clear the timer — unlike the fixed version ingenerate-pr-description.ts.The
generate-pr-description.tsversion ofwithTimeout(lines 61-93) properly wraps the loop intry/finallyand callsclearTimeout(timerId). This version leaks the timer: if the generator finishes before 30s, thesetTimeoutcallback still fires and callsreject()on an unobserved promise, which can surface as an unhandled-rejection warning in Node.js.Consider aligning with the
generate-pr-description.tsimplementation:♻️ Proposed fix
async function* withTimeout<T>( generator: AsyncIterable<T>, timeoutMs: number ): AsyncGenerator<T, void, unknown> { - const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout(() => reject(new Error(`AI provider timed out after ${timeoutMs}ms`)), timeoutMs); - }); + let timerId: ReturnType<typeof setTimeout> | undefined; + + const timeoutPromise = new Promise<never>((_, reject) => { + timerId = setTimeout( + () => reject(new Error(`AI provider timed out after ${timeoutMs}ms`)), + timeoutMs + ); + }); const iterator = generator[Symbol.asyncIterator](); let done = false; - while (!done) { - const result = await Promise.race([iterator.next(), timeoutPromise]); - if (result.done) { - done = true; - } else { - yield result.value; + try { + while (!done) { + const result = await Promise.race([iterator.next(), timeoutPromise]).catch(async (err) => { + await iterator.return?.(); + throw err; + }); + if (result.done) { + done = true; + } else { + yield result.value; + } } + } finally { + clearTimeout(timerId); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 32 - 51, The withTimeout generator leaks its setTimeout because it never clears the timer; change its implementation to mirror generate-pr-description.ts by storing the timerId returned from setTimeout, wrapping the while-loop in try/finally, and calling clearTimeout(timerId) in the finally block so the timer is cancelled if the generator completes normally (use the existing withTimeout function name and iterator logic, but create the timeout promise using the timerId and ensure clearTimeout(timerId) is executed in finally).apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx (1)
78-193:⚠️ Potential issue | 🟠 MajorAdd accessible labels for icon-only buttons on mobile.
When
isMobilehides text, several buttons become icon-only (Back, Validate/Re-validate, View Result/View (stale), Open in GitHub). These lose an accessible name, which blocks screen-reader users.🔧 Suggested pattern (apply to each icon-only button)
- {isMobile && ( - <Button variant="ghost" size="sm" onClick={onClose} className="shrink-0 -ml-1"> - <ArrowLeft className="h-4 w-4" /> - </Button> - )} + {isMobile && ( + <Button + variant="ghost" + size="sm" + onClick={onClose} + className="shrink-0 -ml-1" + aria-label="Back" + title="Back" + > + <ArrowLeft className="h-4 w-4" /> + <span className="sr-only">Back</span> + </Button> + )}- <Button variant="outline" size="sm" onClick={() => onOpenInGitHub(issue.url)}> - <ExternalLink className="h-4 w-4" /> - {!isMobile && <span className="ml-1">Open in GitHub</span>} - </Button> + <Button + variant="outline" + size="sm" + onClick={() => onOpenInGitHub(issue.url)} + aria-label="Open in GitHub" + title="Open in GitHub" + > + <ExternalLink className="h-4 w-4" /> + {!isMobile && <span className="ml-1">Open in GitHub</span>} + {isMobile && <span className="sr-only">Open in GitHub</span>} + </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view/components/issue-detail-panel.tsx` around lines 78 - 193, Several buttons become icon-only on mobile and lack accessible names; update each Button and any icon-only controls (e.g., the Back Button using ArrowLeft with onClose, the Validate/Re-validate Buttons that call onValidateIssue or onShowRevalidateConfirm, the View Result/View (stale) Buttons that call onViewCachedValidation, and the Open in GitHub Button that calls onOpenInGitHub) to provide an accessible label when isMobile is true by adding an aria-label (and optionally a title) that describes the action (e.g., "Back", "Validate issue", "Re-validate", "View validation result", "Open in GitHub"); ensure the label logic is applied where text is conditionally hidden (!isMobile) so screen readers get the name while visual UI remains unchanged.apps/ui/src/components/views/github-issues-view.tsx (1)
260-272:⚠️ Potential issue | 🟡 MinorModel alias
'opus'should be resolved viaresolveModelString().
handleConvertToTaskpasses the raw string'opus'as the model (Line 268). Per coding guidelines, model aliases must be converted to full model names usingresolveModelString()before making API calls. Other handlers in this PR (e.g.,handleAutoAddressPRCommentsinboard-view.tsx) correctly callresolveModelString('opus').Proposed fix
Add the import (if not already present):
+import { resolveModelString } from '@automaker/model-resolver';Then update the feature object:
- model: 'opus', + model: resolveModelString('opus'),Based on learnings: "Use resolveModelString() from
@automaker/model-resolverto convert model aliases (haiku, sonnet, opus) to full model names before making API calls."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view.tsx` around lines 260 - 272, The feature object is setting model to the raw alias 'opus' (used in handleConvertToTask) instead of resolving it; import resolveModelString from '@automaker/model-resolver' and replace the model assignment (e.g., in the feature literal constructed in handleConvertToTask) with model: resolveModelString('opus') so the alias is mapped to the full model name before any API calls; apply the same pattern anywhere else in this file that uses raw model aliases.
🧹 Nitpick comments (19)
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
90-103: LGTM — consider skipping invalidation on no-op re-selection (optional)The invalidation correctly flushes stale feature data on worktree switches, and adding
queryClientto the dep array is the right call since it's now used inside the callback.One minor opportunity:
invalidateQueriescurrently fires even when the user clicks the already-active worktree (no-op selection), triggering a redundant features refetch. A cheap guard avoids it:♻️ Proposed guard against redundant invalidation
const handleSelectWorktree = useCallback( (worktree: WorktreeInfo) => { + const isSameWorktree = + worktree.isMain + ? currentWorktreePath === null + : pathsEqual(worktree.path, currentWorktreePath ?? ''); + setCurrentWorktree(projectPath, worktree.isMain ? null : worktree.path, worktree.branch); - // Invalidate feature queries when switching worktrees to ensure fresh data. - // Without this, feature cards that remount after the worktree switch may have stale - // or missing planSpec/task data, causing todo lists to appear empty until the next - // polling cycle or user interaction triggers a re-render. - queryClient.invalidateQueries({ - queryKey: queryKeys.features.all(projectPath), - }); + if (!isSameWorktree) { + // Invalidate feature queries when switching worktrees to ensure fresh data. + // Without this, feature cards that remount after the worktree switch may have stale + // or missing planSpec/task data, causing todo lists to appear empty until the next + // polling cycle or user interaction triggers a re-render. + queryClient.invalidateQueries({ + queryKey: queryKeys.features.all(projectPath), + }); + } }, - [projectPath, setCurrentWorktree, queryClient] + [projectPath, currentWorktreePath, setCurrentWorktree, queryClient] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts` around lines 90 - 103, Avoid redundant invalidation when re-selecting the active worktree by having handleSelectWorktree first compare the clicked worktree to the currently-selected worktree (use the same state source used with setCurrentWorktree, e.g., currentWorktree or a selector/getter in this hook); if the path/branch (and main/null) match the current selection, return early and skip calling setCurrentWorktree and queryClient.invalidateQueries. Keep references to setCurrentWorktree, handleSelectWorktree, queryClient.invalidateQueries, and queryKeys.features.all when implementing the guard so the equality check and early return are clearly located before the invalidateQueries call.apps/ui/src/components/layout/project-switcher/project-switcher.tsx (1)
105-120: Duplicated initialization wrapper — consider extracting a shared helper.The
try { await initializeProject(path) } catch { /* continue */ }pattern is repeated nearly identically inproject-selector-with-options.tsx(setCurrentProjectWithInit). Extracting a small shared helper (e.g.,ensureProjectInitialized(path: string): Promise<void>) would keep both call sites in sync and avoid divergence.♻️ Example shared helper
// e.g., in apps/ui/src/lib/project-init.ts export async function ensureProjectInitialized(projectPath: string): Promise<void> { try { await initializeProject(projectPath); } catch (error) { console.error('Failed to initialize project during switch:', error); // Non-fatal — project may already be initialized } }Then in both
project-switcher.tsxandproject-selector-with-options.tsx:- try { - await initializeProject(project.path); - } catch (error) { - console.error('Failed to initialize project during switch:', error); - } + await ensureProjectInitialized(project.path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/project-switcher/project-switcher.tsx` around lines 105 - 120, Extract the repeated try/await initializeProject(...) catch(...) block into a shared helper (e.g., ensureProjectInitialized(projectPath: string): Promise<void>) and use it from both handleProjectClick in project-switcher.tsx and setCurrentProjectWithInit in project-selector-with-options.tsx; specifically, implement ensureProjectInitialized to call initializeProject(projectPath) and log the caught error with the same message used today ('Failed to initialize project during switch:'), then replace the inline try/catch in handleProjectClick and setCurrentProjectWithInit with await ensureProjectInitialized(project.path) to keep behavior consistent and avoid duplication.apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (1)
229-232: Consider??over||for semantic precision.Since the props are typed
string | undefined, using??(nullish coalescing) more precisely communicates "fall back only when the value is absent" rather than also treating an explicitly passed empty string as absent — though both produce identical output here.🔧 Optional: swap `||` for `??`
- setTitle(prefilledTitle || ''); - setDescription(prefilledDescription || ''); - setCategory(prefilledCategory || ''); + setTitle(prefilledTitle ?? ''); + setDescription(prefilledDescription ?? ''); + setCategory(prefilledCategory ?? '');🤖 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/add-feature-dialog.tsx` around lines 229 - 232, Replace the logical-OR fallbacks with nullish coalescing to preserve intentional empty strings: in the initialization where setTitle, setDescription, and setCategory are called with prefilledTitle, prefilledDescription, and prefilledCategory, use the ?? operator instead of || so only null/undefined trigger the default ''.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts (1)
152-158: Nit:getWorktreeKeycan returnundefined, used as a Map key.When
worktree.pathis falsy on a non-main worktree,getWorktreeKeyreturnsundefined. WhileMap.get(undefined)works (returnsundefined), multiple worktrees with missing paths would silently collide on the same key. If this is expected to be unreachable, a guard or assertion would make the intent clearer.🤖 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 152 - 158, getWorktreeKey can return undefined for non-main worktrees with falsy path which lets multiple worktrees collide when used as a Map key; update the getWorktreeKey (used in use-dev-servers hook) to guarantee a stable, non-undefined key by either asserting/throwing when a non-main WorktreeInfo has no path or returning a distinct placeholder (e.g. throw new Error or return `__missing_path__:${worktree.id}`) so callers of getWorktreeKey/Map usage don’t silently collide — locate the getWorktreeKey callback and add the chosen guard/placeholder handling referencing WorktreeInfo and projectPath.apps/server/src/routes/github/routes/list-pr-review-comments.ts (2)
94-115: GraphQL query limited tofirst: 100— large PRs may silently lose review thread data.The query fetches at most 100 review threads (line 102) and 100 comments per thread (line 106). PRs with extensive code review could exceed these limits. Since the resolved status is best-effort (line 165), this won't break anything, but consider logging when pagination might be needed (e.g., checking
pageInfo.hasNextPage).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/github/routes/list-pr-review-comments.ts` around lines 94 - 115, The GraphQL query GetPRReviewThreads is hard-limited to reviewThreads(first: 100) and comments(first: 100) so large PRs can lose data; update the query to support pagination (add pageInfo { hasNextPage endCursor } for reviewThreads and comments) and implement cursor-based loops in the code that calls the query (the logic around reviewThreads and comments retrieval) to fetch subsequent pages using after/endCursor until hasNextPage is false, and emit a log (e.g., via the existing logger) whenever pagination occurs or pageInfo.hasNextPage is true to surface when extra pages were required.
289-333: Consider extracting the business logic into a service module.The route handler currently contains all the comment-fetching, GraphQL enrichment, and merging logic. As per coding guidelines, server business logic should be in
services/with route handlers inroutes/delegating to services. This would also facilitate unit testing the fetch/merge logic independently. 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/github/routes/list-pr-review-comments.ts` around lines 289 - 333, Extract the business logic in createListPRReviewCommentsHandler into a new service function (e.g., listPRReviewComments) under services: move the remote-checking and comment-fetching/merge steps (calls to checkGitHubRemote and fetchPRReviewComments and any GraphQL enrichment/merging) into that service so it accepts (projectPath, prNumber) and returns { comments, totalCount } or throws; then simplify createListPRReviewCommentsHandler to only validate inputs, call the new listPRReviewComments service, send the JSON response with success/comments/totalCount, and preserve existing error logging (logError/getErrorMessage) for errors thrown by the service.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown-item.tsx (1)
147-155: Minor: Comment doesn't precisely match the condition.The comment says "only shown when port is confirmed detected" but the condition
devServerInfo?.urlDetected !== falsealso shows the indicator whenurlDetectedisundefined(i.e., not yet confirmed). A more accurate comment would be "hidden when URL detection explicitly failed."Not blocking — the logic itself is sound.
🤖 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-dropdown-item.tsx` around lines 147 - 155, Update the inline comment above the dev-server indicator in WorktreeDropdownItem to accurately describe the condition: instead of "only shown when port is confirmed detected" change to something like "hidden when URL detection explicitly failed" (the JSX condition uses devServerRunning && devServerInfo?.urlDetected !== false), so the comment matches the semantics of devServerRunning and devServerInfo?.urlDetected !== false.apps/ui/src/components/views/terminal-view.tsx (2)
1043-1048:createTerminaldoesn't wire per-session command overrides (unlikecreateTerminalInNewTab).
createTerminalonly setsnewSessionIdswhendefaultRunScriptis present but has nocommandparameter and never populatessessionCommandOverrides. This is asymmetric withcreateTerminalInNewTab(lines 1080-1117). Currently callers don't need per-session overrides on splits, but the inconsistency could bite if a future caller tries to pass a command throughcreateTerminal.Not blocking — just flagging the asymmetry for awareness.
🤖 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 1043 - 1048, In createTerminal, mirror createTerminalInNewTab's behavior by wiring per-session command overrides: if defaultRunScript includes a command, add an entry for the new session id to sessionCommandOverrides (the same map/state used by createTerminalInNewTab) when data.success is true (alongside addTerminalToLayout and setNewSessionIds); use the same updater pattern used elsewhere to set sessionCommandOverrides for data.data.id so split-created terminals support per-session command overrides the same way as new-tab terminals.
585-638:initialCommandnot included in the deduplication key — may silently drop a command.The
cwdKeyat line 591 is built frominitialCwd,initialMode, andnonce, butinitialCommandis not part of it. If a caller navigates to/terminalwith the same cwd/mode/nonce but a different command, theinitialCwdHandledRefguard will short-circuit and the new command will be ignored.In practice the
nonceprop should vary per invocation, so this is likely fine — but it's worth noting in a comment or includinginitialCommandin the key for safety.🔧 Include initialCommand in the dedup key
- const cwdKey = `${initialCwd}:${initialMode || 'default'}:${nonce || 0}`; + const cwdKey = `${initialCwd}:${initialMode || 'default'}:${nonce || 0}:${initialCommand || ''}`;🤖 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 585 - 638, The deduplication key cwdKey (used with initialCwdHandledRef) omits initialCommand which can cause a new command to be ignored; update the key generation to include initialCommand (e.g., append a safe representation of initialCommand or an empty marker) so that createTerminalWithCwd() runs for different commands, and add a brief comment near cwdKey explaining why initialCommand is included; reference symbols: cwdKey, initialCwdHandledRef, initialCommand, createTerminalWithCwd.apps/ui/src/routes/project-settings.tsx (1)
11-15: Unsafe type assertion bypasses runtime validation ofsectionsearch param.The
as ProjectSettingsViewIdcast trusts user input without validation, unlike the terminal route which uses a zod schema. If an invalidsectionvalue is passed in the URL, it silently becomes an invalidProjectSettingsViewId. The downstream switch'sdefaultbranch prevents a crash, but a zod-based approach would be more consistent and correct.♻️ Suggested: Use zod validation consistent with terminal.tsx
+import { z } from 'zod'; + +const projectSettingsSearchSchema = z.object({ + section: z.enum([ + 'identity', 'theme', 'worktrees', 'commands', 'scripts', + 'commands-scripts', 'claude', 'data', 'danger', + ]).optional(), +}); + export const Route = createFileRoute('/project-settings')({ component: ProjectSettingsView, - validateSearch: (search: Record<string, unknown>): ProjectSettingsSearchParams => { - return { - section: search.section as ProjectSettingsViewId | undefined, - }; - }, + validateSearch: projectSettingsSearchSchema, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/routes/project-settings.tsx` around lines 11 - 15, The validateSearch implementation castingly trusts the incoming section string (validateSearch, ProjectSettingsSearchParams, ProjectSettingsViewId); replace the unsafe "as" assertion with zod validation similar to the terminal route: define or reuse a zod schema that enumerates the allowed ProjectSettingsViewId values, parse/parseOptional the search.section, and return section only when the schema succeeds (otherwise return undefined) so runtime input is validated instead of asserted.apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx (3)
105-127: Potential stale sync: commands effect doesn't depend onproject.path.The commands sync effect (lines 105-114) depends only on
[projectSettings]. IfprojectSettingsis cached for the old project and React Query serves it briefly before the new project's query resolves, the commands could temporarily sync from the wrong project's settings. The scripts effect (lines 117-127) correctly includesproject.pathin its dependency array.Consider adding
project.pathto the commands effect deps for consistency, or note that the reset effect (lines 95-102) adequately clears state before the new data arrives.♻️ Add project.path to commands sync deps for consistency
useEffect(() => { if (projectSettings) { const dev = projectSettings.devCommand || ''; const test = projectSettings.testCommand || ''; setDevCommand(dev); setOriginalDevCommand(dev); setTestCommand(test); setOriginalTestCommand(test); } - }, [projectSettings]); + }, [projectSettings, project.path]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 105 - 127, The commands-sync useEffect that sets dev/test state from projectSettings can race with cached data for a different project; update its dependency array to include project.path so the effect re-runs when the active project changes (i.e., ensure the useEffect that reads projectSettings and calls setDevCommand, setOriginalDevCommand, setTestCommand, setOriginalTestCommand also depends on project.path), mirroring the scripts effect and preventing temporary stale values.
464-516: Drag-and-drop handlers are bound per-item without memoization.Each script row creates inline closures for
onDragStart,onDragOver,onDrop, andonDragEnd. For a small list this is fine, but if the list grows, consideruseCallbackwith item index or a data attribute approach to avoid N closures per render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 464 - 516, The drag handlers are recreated per-row causing N closures; memoize or consolidate them: wrap handleDragStart, handleDragOver, handleDrop, and handleDragEnd with useCallback so they are stable across renders (accepting an index or event and reading a data-index), or replace per-item inline handlers with single shared handlers that read a data-index attribute on the dragged element (set data-index on the row and use e.currentTarget.dataset.index or e.dataTransfer). Update the Input/Button JSX to pass either a stable callback (e.g., onDragStart={handleDragStart} with data-index) or memoized inline wrappers created via useCallback to avoid recreating closures each render.
129-134:JSON.stringifycomparison on every render for change detection.
hasScriptChanges(line 133) serializes bothscriptsandoriginalScriptson every render. This is fine for small lists but could be wrapped inuseMemoif the script list grows or the component re-renders frequently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 129 - 134, The JSON.stringify comparison for hasScriptChanges is being run on every render; wrap this comparison in a React useMemo to avoid unnecessary serialization work—compute hasScriptChanges with useMemo(() => JSON.stringify(scripts) !== JSON.stringify(originalScripts), [scripts, originalScripts]) so the expensive stringify runs only when scripts or originalScripts change; update the component that defines hasScriptChanges (in commands-and-scripts-section.tsx) to import and use useMemo and keep the rest of change-detection logic unchanged.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
1055-1095: Duplicate PRInfo construction — extract a helper.The identical PRInfo object is constructed twice (Lines 1059–1068 and Lines 1078–1087) for the two different menu actions. Consider extracting a small helper to reduce duplication.
Proposed refactor
{showPRInfo && worktree.pr && ( + (() => { + const prInfo: PRInfo = { + number: worktree.pr!.number, + title: worktree.pr!.title, + url: worktree.pr!.url, + state: worktree.pr!.state, + author: '', + body: '', + comments: [], + reviewComments: [], + }; + return ( <DropdownMenuSub> ... <DropdownMenuItem onClick={() => { - const prInfo: PRInfo = { ... }; onAddressPRComments(worktree, prInfo); }} ... <DropdownMenuItem onClick={() => { - const prInfo: PRInfo = { ... }; onAutoAddressPRComments(worktree, prInfo); }} ... </DropdownMenuSub> + ); + })() )}Or better yet, compute
prInfoonce viauseMemoat the top of the component and pass it directly.🤖 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 1055 - 1095, Duplicate construction of the PRInfo object is present before calling onAddressPRComments and onAutoAddressPRComments; extract that logic into a single helper or compute it once with useMemo (referencing PRInfo and worktree) and reuse it in both DropdownMenuItem handlers so both onAddressPRComments(worktree, prInfo) and onAutoAddressPRComments(worktree, prInfo) receive the same deduplicated object.apps/ui/src/components/views/board-view.tsx (2)
1027-1041: Pre-existing:model: 'opus' as constbypassesresolveModelString().This isn't a new change in this PR, but
handleCreateMergeConflictResolutionFeatureat Line 1034 uses a raw model alias. Other similar handlers in the same file (lines 983, 1077, 1103, 1142) correctly callresolveModelString('opus'). Worth fixing for consistency.Proposed fix
- model: 'opus' as const, + model: resolveModelString('opus'),Based on learnings: "Use resolveModelString() from
@automaker/model-resolverto convert model aliases (haiku, sonnet, opus) to full model names before making API calls."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 1027 - 1041, handleCreateMergeConflictResolutionFeature is setting model: 'opus' as const which bypasses resolveModelString() used elsewhere; change the featureData construction in handleCreateMergeConflictResolutionFeature to call resolveModelString('opus') (or resolveModelString(modelAlias) where modelAlias is used) and assign its return value to the model field so the API receives the full resolved model name; ensure you import/use resolveModelString from `@automaker/model-resolver` consistently with other handlers.
37-40: Consider importing from the barrel export instead of a direct path.
PRCommentResolutionDialogis re-exported from@/components/dialogs/index.ts. For consistency with the intra-app barrel-import pattern used elsewhere, consider:-import { - PRCommentResolutionDialog, - type PRCommentResolutionPRInfo, -} from '@/components/dialogs/pr-comment-resolution-dialog'; +import { + PRCommentResolutionDialog, + type PRCommentResolutionPRInfo, +} from '@/components/dialogs';That said, other imports in this file (lines 63–68) also use direct paths, so this is a low-priority consistency nit.
Based on learnings: "In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 37 - 40, Replace the direct file import of PRCommentResolutionDialog and the PRCommentResolutionPRInfo type with the barrel export; specifically, change the import that currently references '@/components/dialogs/pr-comment-resolution-dialog' to import both symbols from the dialogs barrel (re-export) such as '@/components/dialogs' (or the app-level components barrel if preferred) so the component and its type are consumed from the barrel export for consistency with the project's intra-app import pattern; leave other direct-path imports alone for now as they're lower priority.apps/ui/src/components/views/github-prs-view.tsx (2)
64-64:useCreateFeaturereceives an empty string when no project is selected.
useCreateFeature(currentProject?.path ?? '')passes''as the project path if no project is selected. If the mutation is triggered before a project is selected (e.g., during a race), it could create a feature against an empty path. The guard at Line 68 checkscurrentProject?.pathbefore proceeding, which mitigates this — but the hook itself may perform setup work with the invalid path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-prs-view.tsx` at line 64, The hook is being initialized with an empty-string path which can lead to invalid setup; change the call from useCreateFeature(currentProject?.path ?? '') to useCreateFeature(currentProject?.path ?? undefined) (or simply useCreateFeature(currentProject?.path)) and update the useCreateFeature implementation to accept an optional projectPath and treat undefined as "not ready" (no automatic setup or mutation), ensuring createFeature only becomes usable once a real currentProject?.path exists.
405-413: PRCommentResolutionDialog import could use the barrel export.Same as noted in
board-view.tsx— Line 28 imports from the direct path. A barrel re-export exists at@/components/dialogs. Minor consistency nit.Based on learnings: "In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-prs-view.tsx` around lines 405 - 413, The PRCommentResolutionDialog is imported via a direct path instead of the components barrel; update the import to use the barrel re-export (from the components dialogs index) so imports are consistent—locate the import for PRCommentResolutionDialog in apps/ui/src/components/views/github-prs-view.tsx (used in the JSX block that renders PRCommentResolutionDialog with props open, onOpenChange, pr) and replace the direct path import with the barrel export from "@/components/dialogs" (matching how board-view.tsx does it).apps/ui/src/components/views/github-issues-view.tsx (1)
1-1: Remove@ts-nocheckand useresolveModelString()for model aliasesThe
@ts-nocheckdirective suppresses all TypeScript checks in this file, masking potential issues in the new feature-creation logic. Fix any type errors instead of suppressing them.Additionally, in
handleConvertToTask(line 268), replace the hardcodedmodel: 'opus'withresolveModelString('opus')to comply with the model resolution guideline used throughout the codebase (see board-view.tsx for the pattern).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view.tsx` at line 1, Remove the file-level "@ts-nocheck" and address the TypeScript errors in this component rather than suppressing them: fix any incorrect prop types, missing null checks, and explicit any usages in apps/ui/src/components/views/github-issues-view.tsx so the file typechecks cleanly; in handleConvertToTask replace the hardcoded model: 'opus' with model: resolveModelString('opus') and import resolveModelString if not already present (follow the pattern used in board-view.tsx), and add any necessary type annotations for the function parameters/returned task payload to satisfy the compiler.
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
* feat: Add file management feature * feat: Add auto-save functionality to file editor * fix: Replace HardDriveDownload icon with Save icon for consistency * fix: Prevent recursive copy/move and improve shell injection prevention * refactor: Extract editor settings form into separate component
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
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/layout/sidebar/hooks/use-navigation.ts (1)
146-164:⚠️ Potential issue | 🟡 MinorInaccurate comment:
file-editoris always included, not conditional.Line 146 says "Terminal and File Editor are conditionally included", but
file-editoris unconditionally pushed intoprojectItems— there is no guard analogous tohideTerminal. Only Terminal is truly conditional. The comment should reflect this.📝 Proposed fix
- // Build project items - Terminal and File Editor are conditionally included + // Build project items - Terminal is conditionally included const projectItems: NavItem[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts` around lines 146 - 164, Comment in use-navigation.ts incorrectly states "Terminal and File Editor are conditionally included" while only the Terminal is guarded; update the comment to accurately reflect that Terminal is conditional (via hideTerminal) and File Editor ('file-editor' in the projectItems array) is always included, e.g., change wording to "Terminal is conditionally included; File Editor is always included" so the comment matches the logic around projectItems and the hideTerminal guard.
🧹 Nitpick comments (18)
apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts (1)
160-164:Foldericon is semantically misleading for a "File Editor" item.
Folderconventionally signals a directory browser. For an editor surface, considerFileEdit,FileCode, orCode2fromlucide-reactto better communicate intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts` around lines 160 - 164, The sidebar item with id 'file-editor' and label 'File Editor' currently uses the Folder icon (icon: Folder), which is misleading; replace Folder with a more appropriate lucide-react icon such as FileEdit, FileCode, or Code2 and update the import to include the chosen icon from 'lucide-react' (e.g., import { FileEdit } from 'lucide-react'), then set icon: FileEdit for the 'file-editor' entry so the icon semantically matches the editor action.apps/server/src/routes/git/routes/details.ts (1)
42-46: Inconsistent use ofexecvsexecFilewithin this file.Line 44 uses
execAsync(shell-basedexec) forgit rev-parse, while every other git command usesexecFileAsync. UseexecFileAsynchere too for consistency and to avoid the shell layer.Proposed fix
- const { stdout: branchRaw } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd: projectPath, - }); + const { stdout: branchRaw } = await execFileAsync( + 'git', + ['rev-parse', '--abbrev-ref', 'HEAD'], + { cwd: projectPath } + );With this change,
execandexecAsyncimports can be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/details.ts` around lines 42 - 46, Replace the shell-based execAsync call used to get the current branch with the file-based execFileAsync: call execFileAsync('git', ['rev-parse','--abbrev-ref','HEAD'], { cwd: projectPath }) and extract stdout into branchRaw as before; update any surrounding code that expects the same stdout shape. Also remove the now-unused exec/execAsync imports from this module so all git commands consistently use execFileAsync (refer to execAsync and execFileAsync in this file to locate the change).apps/server/src/routes/git/routes/enhanced-status.ts (4)
61-176: All business logic is inline in the route handler — coding guidelines require service delegation.Per the project coding guidelines, server business logic should be organized into services in the
services/directory, with route handlers inroutes/delegating to those services. Additionally, server operations should emit events viacreateEventEmitter(). This handler does neither — all git operations and parsing live directly inside the route handler, and no events are emitted.This is noted for awareness; refactoring to a service can be deferred if the team prefers to batch it with the
details.tshandler (same pattern).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 AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 61 - 176, The route handler createEnhancedStatusHandler contains all git business logic inline; extract this into a new service function (e.g., services/gitService.createEnhancedStatus(projectPath: string): Promise<{branch:string,files:EnhancedFileStatus[]}>) that performs the git commands, parsing (numstat, status, renames), and returns the branch and files, and update the route to simply call that service; ensure errors still use logError/getErrorMessage and that the service emits events via createEventEmitter() (e.g., emit progress/results events named like "git:enhanced-status") while performing operations so the route only delegates and returns the service result.
7-11: PreferexecFileoverexecfor hardcoded git commands.This file uses
exec(which spawns a shell) for all git commands, while the siblingdetails.tsusesexecFilefor most of the same operations.execFileavoids the shell layer, eliminating shell-metacharacter injection risks and reducing overhead. Since none of these commands require shell features (pipes, globs),execFileis the better choice here for consistency and security.♻️ Suggested refactor
-import { exec } from 'child_process'; +import { execFile } from 'child_process'; import { promisify } from 'util'; import { getErrorMessage, logError } from '../common.js'; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile);Then replace call sites, e.g.:
-const { stdout: branchRaw } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd: projectPath, -}); +const { stdout: branchRaw } = await execFileAsync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: projectPath, +});Apply similarly for
git status --porcelain,git diff --numstat, andgit diff --numstat --cached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 7 - 11, Replace use of promisified exec with promisified execFile to avoid spawning a shell: import execFile from 'child_process' and create execFileAsync = promisify(execFile), then update all execAsync call sites in this file (the places invoking git commands such as the calls that run "git status --porcelain", "git diff --numstat", and "git diff --numstat --cached") to call execFileAsync with the command 'git' and an args array (e.g., ['status','--porcelain'] or ['diff','--numstat','--cached']) instead of a single shell string; keep error handling via getErrorMessage/logError unchanged.
25-59: Duplicate status-label logic withdetails.ts.
getStatusLabel()here (lines 25–59) and the inline status-label derivation indetails.ts(lines 155–202) implement the same conflict detection / labeling logic. This is a clear DRY violation that will diverge over time.Extract it into a shared utility in
../common.ts(or a newstatus-utils.ts) and import from both handlers.Also applies to: 121-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 25 - 59, The getStatusLabel function duplicates logic present in details.ts; extract this logic into a single shared utility (e.g., export a function named getStatusLabel from a new status-utils.ts or add it to ../common.ts) and replace the inline implementation in both enhanced-status.ts (currently the getStatusLabel function) and details.ts (the inline status-label derivation) to import and call that shared getStatusLabel; ensure the exported function signature matches existing usage (indexStatus: string, workTreeStatus: string) and preserve exact return values so callers (enhanced-status.ts and details.ts) continue to behave the same.
86-97: Binary files emit-in--numstat—parseIntsilently returnsNaN, then|| 0saves it, but the intent is ambiguous.For binary files,
git diff --numstatoutputs-\t-\tfilename.parseInt('-', 10)returnsNaN, and|| 0coerces to0. This works by accident but obscures that the file is binary. If you ever need to distinguish "no diff data" from "binary," consider explicitly checking for-:const added = parts[0] === '-' ? 0 : parseInt(parts[0], 10) || 0;This is minor since the fallback is correct today.
Also applies to: 109-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 86 - 97, The parsing for git --numstat output in the workTreeStats loop treats '-' (binary files) the same as numeric values because parseInt('-') returns NaN and the code falls back to 0; update the logic in the loop that processes numstatRaw (and the other similar parsing block that sets added/removed) to explicitly check parts[0] and parts[1] for '-' before calling parseInt and only parse when not '-', otherwise set added/removed to 0 (retain the existing fallback but make the binary case explicit) so the intent is clear and binary files are handled deterministically when populating workTreeStats (and the equivalent staged/index stats variables).apps/server/src/routes/fs/routes/move.ts (1)
65-66:renamefails across filesystem boundaries (EXDEV).
secureFs.renamewrapsfs.rename, which doesn't work across mount points/volumes. OnEXDEV, the user gets a generic 500 error. Since this is a dev tool where projects could span volumes, consider falling back to copy-then-delete onEXDEV, or at least returning a descriptive error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/move.ts` around lines 65 - 66, secureFs.rename in the move handler can throw EXDEV when source and destination are on different filesystems; update the move route (where secureFs.rename is called) to catch the error, detect err.code === 'EXDEV' and perform a safe fallback: copy the file/directory from sourcePath to destinationPath (preserving permissions/metadata where relevant) then delete the original (use secureFs.copyFile or a recursive copy for directories, then secureFs.unlink/rmdir), and only return success after the delete completes; if implementing the fallback is too invasive, instead catch EXDEV and return a clear 4xx/5xx response indicating cross-filesystem move is unsupported and advising copy-then-delete, referencing secureFs.rename, sourcePath and destinationPath in the handler.apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx (1)
90-98: Tab elements are not keyboard-accessible.The tabs use
<div onClick>instead of<button>or elements withrole="tab"andtabIndex. They won't be focusable or operable via keyboard. For an IDE-like editor, keyboard navigation between tabs is a common expectation. Consider using<button>elements or addingrole="tab",tabIndex={0}, andonKeyDownhandlers, ideally wrapped in arole="tablist"container.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx` around lines 90 - 98, The tab elements are currently non-keyboard-accessible because they render as a <div> with onClick; change the clickable tab node in editor-tabs.tsx to be a semantic, focusable element (e.g., a <button>) or add role="tab", tabIndex={0} and keyboard handlers on the element that currently uses onClick (the element keyed by tab.id that calls onTabSelect(tab.id)); also ensure the container uses role="tablist" and that the active tab toggles aria-selected appropriately (update isActive handling to set aria-selected="true"/"false") and add an onKeyDown handler to call onTabSelect for Enter/Space to match mouse click behavior.apps/ui/package.json (1)
45-62: Inconsistent version pinning across@codemirrorpackages.New
@codemirrorpackages use caret ranges (e.g.,"^6.0.3") while existing ones like@codemirror/lang-xmland@codemirror/theme-one-darkuse exact versions ("6.1.0","6.1.3"). Mixing strategies within the same dependency family can lead to surprising version drift. Consider aligning all@codemirror/*packages to the same pinning strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/package.json` around lines 45 - 62, The `@codemirror` dependencies are inconsistently pinned: most use caret ranges (e.g., "@codemirror/lang-javascript": "^6.2.4") while "@codemirror/lang-xml" and "@codemirror/theme-one-dark" are exact ("6.1.0", "6.1.3"); pick one pinning strategy (prefer consistent caret ranges or exact pins across the family), and update the entries for "@codemirror/lang-xml" and "@codemirror/theme-one-dark" to match the chosen strategy so all "@codemirror/*" versions follow the same format.apps/ui/src/components/views/file-editor-view/components/markdown-preview.tsx (1)
84-88:isMarkdownFilesplits only on/, may miss Windows-style paths.If
filePathever contains backslashes (e.g., from Electron'sdialog.showOpenDialogon Windows),.split('/').pop()returns the entire path including backslashes, and the extension extraction will still work sincelastIndexOf('.')operates on the full string. So this is functionally correct for extension detection, but the intermediatefileNamevariable is misleading. A simpler approach:♻️ Suggested simplification
export function isMarkdownFile(filePath: string): boolean { - const fileName = filePath.split('/').pop() || ''; - const dotIndex = fileName.lastIndexOf('.'); - const ext = dotIndex > 0 ? fileName.slice(dotIndex + 1).toLowerCase() : ''; + const ext = filePath.split('.').pop()?.toLowerCase() ?? ''; return ['md', 'mdx', 'markdown'].includes(ext); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/markdown-preview.tsx` around lines 84 - 88, The helper isMarkdownFile currently derives fileName via filePath.split('/').pop(), which is misleading for Windows paths; update it to get the basename in a path-separator-agnostic way: compute fileName using Node's path.basename(filePath) (import path from 'path') or use a regex like filePath.replace(/^.*[\\/]/, '') to strip any trailing slash/backslash, then keep the existing dotIndex/ext logic and return check; reference symbols: isMarkdownFile, filePath, fileName, dotIndex, ext.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (3)
168-231:DestinationPickerrenders inline — consider portal rendering to avoid z-index stacking issues.This fixed-position overlay is rendered inside
TreeNode, which is inside nesteddivs. Whilefixed inset-0 z-50works in many cases, if any ancestor creates a new stacking context (e.g., viatransform,opacityanimations, orwill-change), the overlay could be clipped or layered incorrectly. A React portal todocument.bodywould be more robust.This is a nice-to-have rather than a current bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 168 - 231, The DestinationPicker overlay should be rendered via a React portal to avoid stacking-context clipping; change DestinationPicker to import and use createPortal (from 'react-dom') and return the modal DOM into document.body instead of rendering inline inside TreeNode, keeping the same structure/props (path state, inputRef, key handlers, onSubmit/onCancel). Ensure the portal mount still runs the existing useEffect focus/select logic (it will after portal mount) and preserve accessibility/focus behavior and the existing classNames and buttons; no changes needed to TreeNode other than where DestinationPicker is invoked.
353-368: Potential stale state in rapid drag events due to spread of store snapshot.Line 367 spreads the current
dragStatecaptured at render time:setDragState({ ...dragState, dropTargetPath: node.path });Similarly on line 374. If multiple
dragOverevents fire within the same render frame, each one uses the same staledragStatesnapshot. Consider reading from the store directly or using a functional update if the store supports it.This is a minor issue since
dragOverevents fire frequently but onlydropTargetPathchanges here, anddraggedPathsis stable during a drag. Noting for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 353 - 368, The drag handlers use a render-time snapshot of dragState when calling setDragState (e.g., in handleDragOver) which can lead to stale updates during rapid events; change the update to use a functional updater that reads the latest state (e.g., setDragState(prev => ({ ...prev, dropTargetPath: node.path }))) or read the store directly before updating, and apply the same change to the other drag handlers that spread dragState so only dropTargetPath is replaced without relying on a stale snapshot.
233-760:TreeNodeis large (~530 lines) with many concerns — consider extracting sub-components in a follow-up.It handles rendering, drag-and-drop, inline editing, context menus, git status display, and recursive child rendering. Extracting the context menu into a separate component (e.g.,
TreeNodeContextMenu) and the drag handlers into a custom hook would improve readability and testability without changing behavior.Not blocking — this is a first implementation and works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 233 - 760, TreeNode is doing too many things; extract the context menu and drag logic: create a new TreeNodeContextMenu component that receives node, isExpanded, isRenaming setters, menuOpen state, and callbacks used in the dropdown (onCopyPath, onCopyItem, onMoveItem, onDownloadItem, onRenameItem, handleDelete via onDeleteItem) and replace the inline DropdownMenu block in TreeNode with <TreeNodeContextMenu .../>; also move drag handlers (handleDragStart, handleDragOver, handleDragLeave, handleDrop, handleDragEnd) and drag-related state accesses to a custom hook useTreeNodeDrag that accepts node, isSelected, selectedPaths, dragState, setDragState, and onDragDropMove and returns the handler functions and booleans (isDragging, isDropTarget), then use that hook inside TreeNode instead of the inline handlers to keep behavior identical but separate concerns.apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx (1)
1-95: Clean, well-structured settings form.Minor nit: the default font size
13appears as a magic number on lines 53–54. Consider extracting it as a constant (e.g.,DEFAULT_EDITOR_FONT_SIZE) for clarity — especially if it's also used elsewhere in the store defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx` around lines 1 - 95, The reset/default font size is hard-coded as 13 in the EditorSettingsForm (see the onClick handler calling setEditorFontSize(13) and the disabled check editorFontSize === 13); extract this into a named constant like DEFAULT_EDITOR_FONT_SIZE (either defined in this file or imported from the store/defaults) and replace both occurrences with that constant so the default is single-sourced and clear.apps/ui/src/routes/file-editor.lazy.tsx (1)
1-2: Prefer theFileEditorViewbarrel export for intra-app imports.This keeps the import stable and leverages the new index barrel in
components/views/file-editor-view.♻️ Proposed import tweak
-import { FileEditorView } from '@/components/views/file-editor-view/file-editor-view'; +import { FileEditorView } from '@/components/views/file-editor-view';Based on learnings: In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/routes/file-editor.lazy.tsx` around lines 1 - 2, Change the direct path import of FileEditorView to use the components barrel export: replace the import that references '@/components/views/file-editor-view/file-editor-view' with the barrel export from '@/components' so FileEditorView is imported via the components index; update the top of file-editor.lazy.tsx where FileEditorView is referenced (alongside createLazyFileRoute and useSearch) to use the barrel-import style for intra-app stability and consistency.apps/ui/src/components/views/settings-view/editor/editor-section.tsx (1)
28-36: Limit re-renders by selecting only editor state.
useAppStore()without a selector subscribes this component to the entire store. Selecting only the needed fields reduces unrelated re-renders.♻️ Suggested refactor
- const { - editorFontSize, - editorFontFamily, - editorAutoSave, - setEditorFontSize, - setEditorFontFamily, - setEditorAutoSave, - } = useAppStore(); + const editorFontSize = useAppStore((s) => s.editorFontSize); + const editorFontFamily = useAppStore((s) => s.editorFontFamily); + const editorAutoSave = useAppStore((s) => s.editorAutoSave); + const setEditorFontSize = useAppStore((s) => s.setEditorFontSize); + const setEditorFontFamily = useAppStore((s) => s.setEditorFontFamily); + const setEditorAutoSave = useAppStore((s) => s.setEditorAutoSave);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/editor/editor-section.tsx` around lines 28 - 36, The EditorSection component currently calls useAppStore() without a selector, subscribing to the whole store; change the call to select only the needed pieces (editorFontSize, editorFontFamily, editorAutoSave, setEditorFontSize, setEditorFontFamily, setEditorAutoSave) by passing a selector function to useAppStore so the component only re-renders when those specific values change; locate the useAppStore invocation inside the EditorSection function and replace it with a selector that returns an object or tuple containing only those six identifiers.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (1)
113-117: Prefer a selector to avoid full-store subscriptions.
useAppStore()without a selector will re-render this view on any store change.♻️ Suggested refactor
- const { currentProject } = useAppStore(); + const currentProject = useAppStore((s) => s.currentProject);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 113 - 117, The component currently calls useAppStore() with no selector which subscribes to the entire store; change it to selector-based hooks to avoid full-store re-renders by replacing the top-level useAppStore() with a selector that reads only currentProject (e.g. useAppStore(s => s.currentProject)), and keep currentWorktree derived via a second selector that reads currentWorktreeByProject using currentProject?.path (i.e. call useAppStore(s => currentProject?.path ? s.currentWorktreeByProject[currentProject.path] ?? null : null)); update references to the symbols FileEditorView, useAppStore, currentProject and currentWorktreeByProject accordingly.apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts (1)
146-164:reset()reuses mutableinitialStatereferences — consider shallow-cloning.
set(initialState)makes the store share the sameSetandMapinstances frominitialState. Current actions are safe (they copy before mutating), but if any future code or consumer mutates these collections in-place after a reset, the sharedinitialStateconstant becomes corrupted for subsequent resets.🛡️ Defensive fix: clone mutable collections on reset
- reset: () => set(initialState), + reset: () => + set({ + ...initialState, + expandedFolders: new Set<string>(), + selectedPaths: new Set<string>(), + gitStatusMap: new Map<string, string>(), + enhancedGitStatusMap: new Map<string, EnhancedGitFileStatus>(), + }),Also applies to: 299-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts` around lines 146 - 164, The reset() currently calls set(initialState) which shares mutable references; instead shallow-clone all mutable collections when resetting: call set with a new object that spreads initialState but replaces expandedFolders and selectedPaths with new Set(initialState.expandedFolders/selectedPaths), gitStatusMap and enhancedGitStatusMap with new Map(initialState.gitStatusMap/enhancedGitStatusMap), tabs with [...initialState.tabs], and dragState with a shallow copy that clones draggedPaths (e.g., dragState: {...initialState.dragState, draggedPaths: [...initialState.dragState.draggedPaths]}), so reset creates fresh Set/Map/array instances while preserving the initial values.
🤖 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/fs/routes/copy.ts`:
- Around line 15-29: copyDirectoryRecursive can recurse infinitely on symlink
cycles because entry.isDirectory() follows symlinks; to fix, avoid following
directory symlinks and detect cycles by using secureFs.lstat(srcPath) and
tracking visited inodes (device+ino) in a Set; if lstat shows a symlink, either
copy the symlink as a link or treat it as a file instead of recursing; also add
an optional maxDepth parameter to copyDirectoryRecursive and increment on each
recursive call to provide a hard stop. Use the symbols copyDirectoryRecursive,
secureFs.readdir, entry.isDirectory(), entry.isSymbolicLink(), secureFs.lstat,
and a visitedInodes Set / maxDepth param to implement the checks.
- Around line 58-74: The current try/catch around secureFs.stat(destinationPath)
swallows all errors; change it to only treat ENOENT as "doesn't exist" and
rethrow or return a 500 for other errors. Specifically, in the block using
secureFs.stat(destinationPath), inspect the caught error's code (e.g., err.code
=== 'ENOENT'); if ENOENT, continue as before, but for any other error (EACCES,
ELOOP, etc.) either call next(err) or send a 5xx JSON response describing the
filesystem error instead of proceeding to secureFs.rm or the copy operation;
keep the existing behavior for overwrite handling when stat succeeds.
In `@apps/server/src/routes/fs/routes/download.ts`:
- Around line 79-80: The Content-Disposition header uses unsanitized filenames
(zipFileName and the single-file fileName derived via path.basename) which can
contain quotes or control/non-ASCII chars; update the download route to RFC
5987-encode filenames for Content-Disposition (or at minimum remove/escape
double quotes and strip control characters and newlines) before setting
res.setHeader('Content-Disposition', ...) and apply the same fix to the
single-file download path (the code that builds fileName on line ~119); locate
usages of path.basename(...) and replace with a sanitized/encoded filename
helper used for both zipFileName and fileName.
- Around line 70-75: The current directory-download code calls
execFileAsync('zip', ...) (see execFileAsync, tmpZipPath, fileName, filePath)
which will fail on Windows; replace that platform-dependent call with a
Node-native zipping solution (e.g., use the archiver or yazl library to create
tmpZipPath by streaming the directory at path.dirname(filePath) into a zip) or,
if you prefer a quicker change, add a platform guard (process.platform) before
the execFileAsync call and return a clear 400/501 error when running on
unsupported platforms; ensure tmpZipPath is created and cleaned up as before and
keep the same response behavior for successful zip creation.
- Around line 21-36: getDirectorySize currently recurses into directories using
entry.isDirectory() which can follow symlinks and cause infinite recursion on
symlink loops; update getDirectorySize to detect and skip already-seen
filesystem nodes by resolving each entry to a stable identity (call
secureFs.realpath(entryPath) or use secureFs.lstat/stat and combine
stats.dev+stats.ino) and maintain a Set of visited identifiers (e.g.,
visitedPaths or visitedInodes) so if the resolved path or inode is already
present you skip recursing; additionally consider enforcing a max recursion
depth parameter on getDirectorySize to further guard against pathological cases.
- Around line 84-106: The temp zip cleanup currently only runs on stream 'end'
and 'error', which can miss client aborts; add a single unified cleanup handler
that listens for res.on('close') (or res.once('close')) and in that handler
calls secureFs.rm(tmpZipPath) (with try/catch to ignore removal errors) and also
remove the existing duplicate cleanup code in stream 'end'/'error' (or delegate
to the same cleanup function) so tmpZipPath is always removed when the response
is closed; reference the existing tmpZipPath, stream, secureFs.rm, and res
objects when implementing this change.
In `@apps/server/src/routes/fs/routes/move.ts`:
- Around line 44-60: The current try/catch around secureFs.stat(destinationPath)
swallows all exceptions; change the catch to inspect the caught error (e.g.,
catch (err)) and re-throw unless err.code === 'ENOENT' so real errors
(permission denied, EACCES, etc.) are not ignored; keep the existing logic that
returns 409 when destination exists (and uses overwrite to call secureFs.rm) but
only proceed to rm or continue when stat succeeded or the error is ENOENT.
Reference symbols: secureFs.stat, destinationPath, overwrite, secureFs.rm.
In `@apps/server/src/routes/git/routes/details.ts`:
- Around line 224-241: The inner catch in routes/details.ts currently returns
res.json({ success: true, details: { ...empty defaults }}), which hides the
failure; change the response to indicate failure by returning res.json({
success: false, error: innerError?.message || 'Git details unavailable',
details: null }) (or include both success:false and an error field) and keep the
existing logError(innerError, 'Git details failed'); ensure you update the block
that references innerError, logError, and res.json so callers can distinguish an
error from empty data.
- Around line 64-77: Replace the fragile pipe delimiter in the git log format
with a NUL separator and update parsing: change the execFileAsync git --format
string to use %x00 between fields (e.g. %H%x00%s%x00%an%x00%aI), split logOutput
on the NUL character '\0', and then safely map parts[0..3] to lastCommitHash,
lastCommitMessage, lastCommitAuthor, lastCommitTimestamp (using empty-string
fallbacks and only the first four parts to avoid overflow); update the code
around the execFileAsync call and the variables
lastCommitHash/lastCommitMessage/lastCommitAuthor/lastCommitTimestamp
accordingly.
- Around line 107-131: The code double-counts staged changes because `git diff
--numstat HEAD -- filePath` includes staged changes; update the two
execFileAsync calls to follow the unstaged+staged pattern: call
execFileAsync('git', ['diff', '--numstat', '--', filePath], { cwd: projectPath
}) to get unstaged (index→worktree) into diffStatRaw, and keep
execFileAsync('git', ['diff', '--numstat', '--cached', '--', filePath], { cwd:
projectPath }) for staged into stagedDiffStatRaw; then parse and sum into
linesAdded/linesRemoved as you already do (variables: diffStatRaw,
stagedDiffStatRaw, linesAdded, linesRemoved, execFileAsync, filePath,
projectPath).
- Around line 94-106: The block that handles untracked files uses
secureFs.readFile(filePath, 'utf-8') but filePath is relative to the git process
cwd (projectPath), so secureFs resolves it against the server CWD and may read
the wrong file; update the code that reads the file in the statusLine handling
to join projectPath and filePath (e.g., use path.join or path.resolve with
projectPath and filePath) before calling secureFs.readFile, keeping the same
error handling and assignment to linesAdded so untracked files are read from the
repository directory used for the git commands.
In `@apps/server/src/routes/git/routes/enhanced-status.ts`:
- Around line 167-170: The catch block in the enhanced-status handler currently
logs innerError via logError('Git enhanced status failed') then returns
res.json({ success: true, branch: '', files: [] }), which masks failures; update
the catch to return an error response (e.g., res.status(500).json({ success:
false, error: innerError?.message || 'Git enhanced status failed' })) or include
an error field so callers can distinguish failures from empty/no-changes, while
still logging via logError(innerError, 'Git enhanced status failed').
- Around line 73-81: The two execAsync calls that run git ('git rev-parse
--abbrev-ref HEAD' and 'git status --porcelain') are missing an increased
maxBuffer, which can cause ERR_CHILD_PROCESS_STDIO_MAXBUFFER for large repos;
update both execAsync invocations (the one assigning branchRaw and the one
assigning statusOutput) to include the same option used elsewhere: pass { cwd:
projectPath, maxBuffer: 10 * 1024 * 1024 } so both commands use the larger
stdout buffer.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 789-798: handleRootDragOver currently overwrites draggedPaths to
an empty array which clears drag visual feedback; change the update to preserve
existing draggedPaths by reading the current dragState and only updating
dropTargetPath instead of resetting draggedPaths (e.g., include dragState from
the store and call setDragState with the existing draggedPaths and new
dropTargetPath or use a functional update that spreads previous state), keeping
effectivePath usage and function name handleRootDragOver unchanged.
In
`@apps/ui/src/components/views/file-editor-view/components/worktree-directory-dropdown.tsx`:
- Around line 124-139: When rendering the change-count label in the
otherWorktrees map, avoid showing an empty number when wt.hasChanges is true but
wt.changedFilesCount is undefined; update the DropdownMenuItem rendering logic
(inside the otherWorktrees.map callback) to check wt.changedFilesCount and, if
it's undefined, render a fallback label such as "Changes" (otherwise render "{n}
change(s)" with proper singular/plural). Modify the conditional around
wt.hasChanges / wt.changedFilesCount and adjust the string interpolation so
wt.changedFilesCount is only used when defined; locate this code near the map
over otherWorktrees and the handleSelectWorktree usage to make the change.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 305-349: Persisting full EditorTab content causes localStorage
quota failures; update the custom storage adapter (storage.setItem and
storage.getItem used by the partialize for FileEditorState) to serialize only
tab metadata (e.g., filePath, id, cursor/selection, isDirty flag) and explicitly
strip heavy fields like content and originalContent before JSON.stringify, and
on hydration (storage.getItem / when reconstructing tabs) mark tabs as needing
content reload so the editor fetches file contents from the server using the
persisted filePath; also change the setItem catch to surface/log the error
(including QuotaExceededError) instead of silently swallowing it so failures are
visible and can be handled.
In `@apps/ui/src/lib/http-api-client.ts`:
- Around line 1270-1315: The downloadItem function skips the shared request
setup and rebuilds auth headers itself; update downloadItem to await
waitForApiKeyInit() before the fetch and use the centralized getHeaders()
(instead of manually constructing headers and calling
getApiKey/getSessionToken), preserve credentials: 'include', and keep the same
endpoint and body; also improve Content-Disposition parsing in downloadItem to
fallback to unquoted filename and filename* (UTF-8'') forms when the quoted
regex fails so filenames like filename=foo.txt or filename*=UTF-8''foo.txt are
handled.
In `@apps/ui/src/styles/global.css`:
- Around line 953-958: The gutter min-width for .cm-lineNumbers
.cm-gutterElement is too small for 4-digit line numbers and prevents CodeMirror
from expanding; update the rule to either increase the cap to 2rem and add
overflow: hidden (to prevent visual bleed) or keep 1.75rem but add overflow:
hidden so digits are clipped instead of overlapping the code area; modify the
CSS selector .cm-lineNumbers .cm-gutterElement to include the chosen min-width
and overflow property and remove the !important only if safe to allow CodeMirror
to adjust dynamically.
---
Outside diff comments:
In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts`:
- Around line 146-164: Comment in use-navigation.ts incorrectly states "Terminal
and File Editor are conditionally included" while only the Terminal is guarded;
update the comment to accurately reflect that Terminal is conditional (via
hideTerminal) and File Editor ('file-editor' in the projectItems array) is
always included, e.g., change wording to "Terminal is conditionally included;
File Editor is always included" so the comment matches the logic around
projectItems and the hideTerminal guard.
---
Duplicate comments:
In `@apps/server/src/routes/git/routes/details.ts`:
- Around line 155-202: The logic computing isConflicted/isStaged/isUnstaged and
mapping statusChar to human labels is duplicated; extract it into a shared
helper (e.g., getStatusLabel) and replace the block using
indexStatus/workTreeStatus/statusLabel in this file with a call to that helper;
ensure the helper signature accepts (indexStatus: string, workTreeStatus:
string) and returns { statusLabel: string, isConflicted: boolean, isStaged:
boolean, isUnstaged: boolean } so callers in this file and enhanced-status.ts
can both consume it without duplicating logic.
---
Nitpick comments:
In `@apps/server/src/routes/fs/routes/move.ts`:
- Around line 65-66: secureFs.rename in the move handler can throw EXDEV when
source and destination are on different filesystems; update the move route
(where secureFs.rename is called) to catch the error, detect err.code ===
'EXDEV' and perform a safe fallback: copy the file/directory from sourcePath to
destinationPath (preserving permissions/metadata where relevant) then delete the
original (use secureFs.copyFile or a recursive copy for directories, then
secureFs.unlink/rmdir), and only return success after the delete completes; if
implementing the fallback is too invasive, instead catch EXDEV and return a
clear 4xx/5xx response indicating cross-filesystem move is unsupported and
advising copy-then-delete, referencing secureFs.rename, sourcePath and
destinationPath in the handler.
In `@apps/server/src/routes/git/routes/details.ts`:
- Around line 42-46: Replace the shell-based execAsync call used to get the
current branch with the file-based execFileAsync: call execFileAsync('git',
['rev-parse','--abbrev-ref','HEAD'], { cwd: projectPath }) and extract stdout
into branchRaw as before; update any surrounding code that expects the same
stdout shape. Also remove the now-unused exec/execAsync imports from this module
so all git commands consistently use execFileAsync (refer to execAsync and
execFileAsync in this file to locate the change).
In `@apps/server/src/routes/git/routes/enhanced-status.ts`:
- Around line 61-176: The route handler createEnhancedStatusHandler contains all
git business logic inline; extract this into a new service function (e.g.,
services/gitService.createEnhancedStatus(projectPath: string):
Promise<{branch:string,files:EnhancedFileStatus[]}>) that performs the git
commands, parsing (numstat, status, renames), and returns the branch and files,
and update the route to simply call that service; ensure errors still use
logError/getErrorMessage and that the service emits events via
createEventEmitter() (e.g., emit progress/results events named like
"git:enhanced-status") while performing operations so the route only delegates
and returns the service result.
- Around line 7-11: Replace use of promisified exec with promisified execFile to
avoid spawning a shell: import execFile from 'child_process' and create
execFileAsync = promisify(execFile), then update all execAsync call sites in
this file (the places invoking git commands such as the calls that run "git
status --porcelain", "git diff --numstat", and "git diff --numstat --cached") to
call execFileAsync with the command 'git' and an args array (e.g.,
['status','--porcelain'] or ['diff','--numstat','--cached']) instead of a single
shell string; keep error handling via getErrorMessage/logError unchanged.
- Around line 25-59: The getStatusLabel function duplicates logic present in
details.ts; extract this logic into a single shared utility (e.g., export a
function named getStatusLabel from a new status-utils.ts or add it to
../common.ts) and replace the inline implementation in both enhanced-status.ts
(currently the getStatusLabel function) and details.ts (the inline status-label
derivation) to import and call that shared getStatusLabel; ensure the exported
function signature matches existing usage (indexStatus: string, workTreeStatus:
string) and preserve exact return values so callers (enhanced-status.ts and
details.ts) continue to behave the same.
- Around line 86-97: The parsing for git --numstat output in the workTreeStats
loop treats '-' (binary files) the same as numeric values because parseInt('-')
returns NaN and the code falls back to 0; update the logic in the loop that
processes numstatRaw (and the other similar parsing block that sets
added/removed) to explicitly check parts[0] and parts[1] for '-' before calling
parseInt and only parse when not '-', otherwise set added/removed to 0 (retain
the existing fallback but make the binary case explicit) so the intent is clear
and binary files are handled deterministically when populating workTreeStats
(and the equivalent staged/index stats variables).
In `@apps/ui/package.json`:
- Around line 45-62: The `@codemirror` dependencies are inconsistently pinned:
most use caret ranges (e.g., "@codemirror/lang-javascript": "^6.2.4") while
"@codemirror/lang-xml" and "@codemirror/theme-one-dark" are exact ("6.1.0",
"6.1.3"); pick one pinning strategy (prefer consistent caret ranges or exact
pins across the family), and update the entries for "@codemirror/lang-xml" and
"@codemirror/theme-one-dark" to match the chosen strategy so all "@codemirror/*"
versions follow the same format.
In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts`:
- Around line 160-164: The sidebar item with id 'file-editor' and label 'File
Editor' currently uses the Folder icon (icon: Folder), which is misleading;
replace Folder with a more appropriate lucide-react icon such as FileEdit,
FileCode, or Code2 and update the import to include the chosen icon from
'lucide-react' (e.g., import { FileEdit } from 'lucide-react'), then set icon:
FileEdit for the 'file-editor' entry so the icon semantically matches the editor
action.
In
`@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx`:
- Around line 1-95: The reset/default font size is hard-coded as 13 in the
EditorSettingsForm (see the onClick handler calling setEditorFontSize(13) and
the disabled check editorFontSize === 13); extract this into a named constant
like DEFAULT_EDITOR_FONT_SIZE (either defined in this file or imported from the
store/defaults) and replace both occurrences with that constant so the default
is single-sourced and clear.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 90-98: The tab elements are currently non-keyboard-accessible
because they render as a <div> with onClick; change the clickable tab node in
editor-tabs.tsx to be a semantic, focusable element (e.g., a <button>) or add
role="tab", tabIndex={0} and keyboard handlers on the element that currently
uses onClick (the element keyed by tab.id that calls onTabSelect(tab.id)); also
ensure the container uses role="tablist" and that the active tab toggles
aria-selected appropriately (update isActive handling to set
aria-selected="true"/"false") and add an onKeyDown handler to call onTabSelect
for Enter/Space to match mouse click behavior.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 168-231: The DestinationPicker overlay should be rendered via a
React portal to avoid stacking-context clipping; change DestinationPicker to
import and use createPortal (from 'react-dom') and return the modal DOM into
document.body instead of rendering inline inside TreeNode, keeping the same
structure/props (path state, inputRef, key handlers, onSubmit/onCancel). Ensure
the portal mount still runs the existing useEffect focus/select logic (it will
after portal mount) and preserve accessibility/focus behavior and the existing
classNames and buttons; no changes needed to TreeNode other than where
DestinationPicker is invoked.
- Around line 353-368: The drag handlers use a render-time snapshot of dragState
when calling setDragState (e.g., in handleDragOver) which can lead to stale
updates during rapid events; change the update to use a functional updater that
reads the latest state (e.g., setDragState(prev => ({ ...prev, dropTargetPath:
node.path }))) or read the store directly before updating, and apply the same
change to the other drag handlers that spread dragState so only dropTargetPath
is replaced without relying on a stale snapshot.
- Around line 233-760: TreeNode is doing too many things; extract the context
menu and drag logic: create a new TreeNodeContextMenu component that receives
node, isExpanded, isRenaming setters, menuOpen state, and callbacks used in the
dropdown (onCopyPath, onCopyItem, onMoveItem, onDownloadItem, onRenameItem,
handleDelete via onDeleteItem) and replace the inline DropdownMenu block in
TreeNode with <TreeNodeContextMenu .../>; also move drag handlers
(handleDragStart, handleDragOver, handleDragLeave, handleDrop, handleDragEnd)
and drag-related state accesses to a custom hook useTreeNodeDrag that accepts
node, isSelected, selectedPaths, dragState, setDragState, and onDragDropMove and
returns the handler functions and booleans (isDragging, isDropTarget), then use
that hook inside TreeNode instead of the inline handlers to keep behavior
identical but separate concerns.
In
`@apps/ui/src/components/views/file-editor-view/components/markdown-preview.tsx`:
- Around line 84-88: The helper isMarkdownFile currently derives fileName via
filePath.split('/').pop(), which is misleading for Windows paths; update it to
get the basename in a path-separator-agnostic way: compute fileName using Node's
path.basename(filePath) (import path from 'path') or use a regex like
filePath.replace(/^.*[\\/]/, '') to strip any trailing slash/backslash, then
keep the existing dotIndex/ext logic and return check; reference symbols:
isMarkdownFile, filePath, fileName, dotIndex, ext.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 113-117: The component currently calls useAppStore() with no
selector which subscribes to the entire store; change it to selector-based hooks
to avoid full-store re-renders by replacing the top-level useAppStore() with a
selector that reads only currentProject (e.g. useAppStore(s =>
s.currentProject)), and keep currentWorktree derived via a second selector that
reads currentWorktreeByProject using currentProject?.path (i.e. call
useAppStore(s => currentProject?.path ?
s.currentWorktreeByProject[currentProject.path] ?? null : null)); update
references to the symbols FileEditorView, useAppStore, currentProject and
currentWorktreeByProject accordingly.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 146-164: The reset() currently calls set(initialState) which
shares mutable references; instead shallow-clone all mutable collections when
resetting: call set with a new object that spreads initialState but replaces
expandedFolders and selectedPaths with new
Set(initialState.expandedFolders/selectedPaths), gitStatusMap and
enhancedGitStatusMap with new
Map(initialState.gitStatusMap/enhancedGitStatusMap), tabs with
[...initialState.tabs], and dragState with a shallow copy that clones
draggedPaths (e.g., dragState: {...initialState.dragState, draggedPaths:
[...initialState.dragState.draggedPaths]}), so reset creates fresh Set/Map/array
instances while preserving the initial values.
In `@apps/ui/src/components/views/settings-view/editor/editor-section.tsx`:
- Around line 28-36: The EditorSection component currently calls useAppStore()
without a selector, subscribing to the whole store; change the call to select
only the needed pieces (editorFontSize, editorFontFamily, editorAutoSave,
setEditorFontSize, setEditorFontFamily, setEditorAutoSave) by passing a selector
function to useAppStore so the component only re-renders when those specific
values change; locate the useAppStore invocation inside the EditorSection
function and replace it with a selector that returns an object or tuple
containing only those six identifiers.
In `@apps/ui/src/routes/file-editor.lazy.tsx`:
- Around line 1-2: Change the direct path import of FileEditorView to use the
components barrel export: replace the import that references
'@/components/views/file-editor-view/file-editor-view' with the barrel export
from '@/components' so FileEditorView is imported via the components index;
update the top of file-editor.lazy.tsx where FileEditorView is referenced
(alongside createLazyFileRoute and useSearch) to use the barrel-import style for
intra-app stability and consistency.
apps/ui/src/components/views/file-editor-view/components/worktree-directory-dropdown.tsx
Show resolved
Hide resolved
fix: Improve error handling and stabilize async operations - Add error event handlers to GraphQL process spawns to prevent unhandled rejections - Replace execAsync with execFile for safer command execution and better control - Fix timeout cleanup in withTimeout generator to prevent memory leaks - Improve outdated comment detection logic by removing redundant condition - Use resolveModelString for consistent model string handling - Replace || with ?? for proper falsy value handling in dialog initialization - Add comments clarifying branch name resolution logic for local branches with slashes - Add catch handler for project selection to handle async errors gracefully ```
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/terminal-view.tsx (1)
1985-2030:⚠️ Potential issue | 🟡 MinorMaximized
TerminalPanelis missingrunCommandOnConnectandonCommandRan, causing commands to silently not fire if a terminal is maximized before it connects.When
terminalState.maximizedSessionIdis set, React unmounts the regularTerminalPanel(keysessionId) and mounts a new one (keymaximized-${sessionId}). If the user maximizes immediately after creating a session with a command, the maximized panel establishes its WebSocket connection withoutrunCommandOnConnect, so the command is never sent. BecauseonCommandRanis also absent,handleCommandRanis never called, leaving the session stuck innewSessionIds/sessionCommandOverridesuntil the user un-maximizes. The command will eventually fire on un-maximize (since the session remains innewSessionIdsfor the re-mounted regular panel), but the experience is inconsistent.🐛 Proposed fix – propagate run-command props to the maximized panel
<TerminalPanel key={`maximized-${terminalState.maximizedSessionId}`} sessionId={terminalState.maximizedSessionId} authToken={terminalState.authToken} isActive={true} onFocus={() => setActiveTerminalSession(terminalState.maximizedSessionId!)} onClose={() => killTerminal(terminalState.maximizedSessionId!)} ... onRunCommandInNewTab={(command: string) => { const { cwd, branchName: branch } = getActiveSessionWorktreeInfo(); createTerminalInNewTab(cwd, branch, command); }} + runCommandOnConnect={(() => { + const sessionId = terminalState.maximizedSessionId!; + const isNew = newSessionIds.has(sessionId); + const sessionCommand = sessionCommandOverrides.get(sessionId); + return isNew ? sessionCommand || defaultRunScript : undefined; + })()} + onCommandRan={() => handleCommandRan(terminalState.maximizedSessionId!)} onSessionInvalid={...} ... />🤖 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 1985 - 2030, The maximized TerminalPanel rendered when terminalState.maximizedSessionId is set is missing the runCommandOnConnect and onCommandRan props, so propagate the same handlers the regular panel uses: add runCommandOnConnect={runCommandOnConnect} and onCommandRan={handleCommandRan} (or the equivalent names used in your file) to the TerminalPanel inside the TerminalErrorBoundary for the maximized branch so commands queued on session creation fire correctly and handleCommandRan runs to clear newSessionIds/sessionCommandOverrides.
🧹 Nitpick comments (10)
apps/server/src/routes/worktree/routes/generate-commit-message.ts (2)
9-9: Consider switchingexec→execFilefor consistency with the PR's stated convention.The PR commit message states it "replaces execAsync with execFile for safer command execution," but this file still uses
promisify(exec). While there's no injection risk here (commands are hardcoded), switching toexecFilewith explicit args would be consistent and more defensive:♻️ Suggested change
-import { exec } from 'child_process'; -import { promisify } from 'util'; +import { execFile } from 'child_process'; +import { promisify } from 'util'; ... -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); ... - const { stdout: stagedDiff } = await execAsync('git diff --cached', { + const { stdout: stagedDiff } = await execFileAsync('git', ['diff', '--cached'], { cwd: worktreePath, maxBuffer: 1024 * 1024 * 5, }); ... - const { stdout: unstagedDiff } = await execAsync('git diff', { + const { stdout: unstagedDiff } = await execFileAsync('git', ['diff'], { cwd: worktreePath, maxBuffer: 1024 * 1024 * 5, });Also applies to: 132-135, 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` at line 9, The file still imports and uses promisify(exec) (symbol: exec and promisify) instead of execFile; replace the exec import with execFile and create a promisified execFile (e.g., const execFileAsync = promisify(execFile)), then update any calls that currently pass a full command string (e.g., usages of execAsync or direct exec calls inside the generateCommitMessage flow) to call execFileAsync with the command as the first arg and an explicit args array as the second argument so arguments are not shell-interpolated; also update the other occurrences noted (around the blocks previously at 132-135 and 139-142) to use execFileAsync similarly.
36-62: Good improvement to timeout handling with proper cleanup.The explicit
timerIdtracking withclearTimeoutinfinallyand iterator cleanup viaiterator.return()on timeout are solid improvements that prevent leaks and hangs.One minor concern: if
iterator.return()itself throws (e.g., the underlying stream errors during teardown), the original timeout error is lost. Consider wrapping it defensively:🛡️ Defensive cleanup suggestion
const result = await Promise.race([iterator.next(), timeoutPromise]).catch(async (err) => { - await iterator.return?.(); + try { + await iterator.return?.(); + } catch { + // Ignore cleanup errors; propagate the original timeout error + } throw err; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 36 - 62, The timeout cleanup can lose the original timeout error if iterator.return() throws; update the Promise.race catch path to capture the original error, then call iterator.return()?. If iterator.return() throws, catch and ignore or log that secondary error, but rethrow the original error so the timeout error from timeoutPromise (and not the teardown error) is preserved—apply this change around the existing iterator, iterator.return, timerId and timeoutPromise logic used with Promise.race.apps/ui/src/components/views/terminal-view.tsx (1)
299-302: ConsiderRecord<string, string>instead ofMap<string, string>for React state.
Mapin React state is unconventional. A plain object has simpler spread/copy semantics ({ ...prev, [id]: command }/omit) and is serializable. The functional-update pattern used here (new Map(prev).set(...)) is correctly immutable, so this is purely an ergonomics suggestion.♻️ Proposed refactor
-const [sessionCommandOverrides, setSessionCommandOverrides] = useState<Map<string, string>>( - new Map() -); +const [sessionCommandOverrides, setSessionCommandOverrides] = useState<Record<string, string>>({});Then update all usages:
-setSessionCommandOverrides((prev) => new Map(prev).set(data.data.id, initialCommand)); +setSessionCommandOverrides((prev) => ({ ...prev, [data.data.id]: initialCommand })); -const sessionCommand = sessionCommandOverrides.get(content.sessionId); +const sessionCommand = sessionCommandOverrides[content.sessionId]; -setSessionCommandOverrides((prev) => { - const next = new Map(prev); - next.delete(sessionId); - return next; -}); +setSessionCommandOverrides(({ [sessionId]: _, ...rest }) => rest);🤖 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 299 - 302, Replace the React state type Map<string,string> with a plain Record<string,string> for sessionCommandOverrides and update setSessionCommandOverrides usage: initialize sessionCommandOverrides as an empty object, change any immutable updates that currently do new Map(prev).set(id, cmd) to functional updates using object spread (e.g., prev => ({ ...prev, [id]: command })) and change deletions that clone the Map then delete to creating a shallow copy and deleting the key (or using object rest to omit the key); update all usages of sessionCommandOverrides, setSessionCommandOverrides, and any helper logic that relied on Map methods to use plain object access, keys() via Object.keys, and typing accordingly so the state remains serializable and easier to copy.apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx (2)
105-127: Consider merging the two siblinguseEffectblocks into one.Both effects share the identical dependency array
[projectSettings, project.path]and the sameif (projectSettings)guard, so they always fire together. Merging them eliminates a redundant reconcile pass and makes the sync logic easier to follow in one place.♻️ Proposed refactor
- // Sync commands state when project settings load - useEffect(() => { - if (projectSettings) { - const dev = projectSettings.devCommand || ''; - const test = projectSettings.testCommand || ''; - setDevCommand(dev); - setOriginalDevCommand(dev); - setTestCommand(test); - setOriginalTestCommand(test); - } - }, [projectSettings, project.path]); - - // Sync scripts state when project settings load - useEffect(() => { - if (projectSettings) { - const configured = projectSettings.terminalScripts; - const scriptList = - configured && configured.length > 0 - ? configured.map((s) => ({ id: s.id, name: s.name, command: s.command })) - : DEFAULT_TERMINAL_SCRIPTS.map((s) => ({ ...s })); - setScripts(scriptList); - setOriginalScripts(JSON.parse(JSON.stringify(scriptList))); - } - }, [projectSettings, project.path]); + // Sync all settings state when project settings load + useEffect(() => { + if (projectSettings) { + const dev = projectSettings.devCommand || ''; + const test = projectSettings.testCommand || ''; + setDevCommand(dev); + setOriginalDevCommand(dev); + setTestCommand(test); + setOriginalTestCommand(test); + + const configured = projectSettings.terminalScripts; + const scriptList = + configured && configured.length > 0 + ? configured.map((s) => ({ id: s.id, name: s.name, command: s.command })) + : DEFAULT_TERMINAL_SCRIPTS.map((s) => ({ ...s })); + setScripts(scriptList); + setOriginalScripts(structuredClone(scriptList)); + } + }, [projectSettings, project.path]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 105 - 127, Merge the two sibling useEffect blocks that both depend on [projectSettings, project.path] into a single useEffect which first guards if (projectSettings) and then performs both groups of state updates: setDevCommand and setOriginalDevCommand using projectSettings.devCommand/testCommand, and construct scriptList from projectSettings.terminalScripts or DEFAULT_TERMINAL_SCRIPTS and call setScripts plus setOriginalScripts (preserving the deep-copy via JSON.parse(JSON.stringify(...))). Keep the same dependency array and ensure you reference the existing identifiers (useEffect, projectSettings, setDevCommand, setOriginalDevCommand, setTestCommand, setOriginalTestCommand, setScripts, setOriginalScripts, DEFAULT_TERMINAL_SCRIPTS) so functionality and immutability are preserved.
125-125: PreferstructuredClone()overJSON.parse(JSON.stringify(...))for deep cloning.
structuredCloneis built into modern runtimes (including Electron/Chromium), is faster, and avoids theJSONround-trip caveats.♻️ Proposed refactor (apply to all three sites)
- setOriginalScripts(JSON.parse(JSON.stringify(scriptList))); + setOriginalScripts(structuredClone(scriptList));- setOriginalScripts(JSON.parse(JSON.stringify(normalizedScripts))); + setOriginalScripts(structuredClone(normalizedScripts));- setScripts(JSON.parse(JSON.stringify(originalScripts))); + setScripts(structuredClone(originalScripts));Also applies to: 164-164, 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` at line 125, Replace the JSON round-trip deep clone usage with structuredClone: where you call setOriginalScripts(JSON.parse(JSON.stringify(scriptList))) (and the similar occurrences for original scripts at the other sites), use setOriginalScripts(structuredClone(scriptList)) instead; locate the calls to setOriginalScripts and any JSON.parse(JSON.stringify(...)) clones in commands-and-scripts-section.tsx and switch them to structuredClone(scriptList) to leverage the built-in deep clone (apply the same change to the other two similar occurrences mentioned).apps/server/src/routes/github/routes/list-pr-review-comments.ts (2)
13-14:constdeclaration interleaved with import statements.
const execFileAsync = promisify(execFile)is placed between import statements. Imports are hoisted so this is harmless at runtime, but it's non-idiomatic and ESLint'simport/firstrule flags this pattern. Move theconstafter all imports.♻️ Proposed fix
import { spawn, execFile } from 'child_process'; import { promisify } from 'util'; import type { Request, Response } from 'express'; import { execEnv, getErrorMessage, logError } from './common.js'; - -const execFileAsync = promisify(execFile); import { checkGitHubRemote } from './check-github-remote.js'; +const execFileAsync = promisify(execFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/github/routes/list-pr-review-comments.ts` around lines 13 - 14, The const declaration "const execFileAsync = promisify(execFile)" is interleaved with import statements and triggers the import/first lint rule; move that declaration below all import lines so the top of the module only contains imports, then retain "execFileAsync" (and its references to promisify and execFile) in its current module scope after the imports to restore idiomatic ordering and satisfy ESLint.
266-266: Prefer??over||forc.linefallback.The PR commit explicitly replaces
||with??for proper falsy-value handling. Whilec.line = 0is not a valid GitHub line number, using||is inconsistent with that stated intent and would swallow a hypothetical0response.♻️ Proposed fix
- line: c.line || c.original_line, + line: c.line ?? c.original_line,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/github/routes/list-pr-review-comments.ts` at line 266, Replace the fallback using logical OR with the nullish coalescing operator for the comment line value: change the expression that sets line from "c.line || c.original_line" to use "c.line ?? c.original_line" so the mapping that constructs the PR review comment object (the code handling variable c and its line/original_line properties in the list-pr-review-comments logic) preserves defined zero-like values while falling back only on null/undefined.apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx (2)
77-80: Use generateUUID for feature IDs to reduce collision risk.🔁 Proposed refactor
-import { cn, modelSupportsThinking } from '@/lib/utils'; +import { cn, modelSupportsThinking, generateUUID } from '@/lib/utils'; ... -function generateFeatureId(): string { - return `feature-${Date.now()}-${Math.random().toString(36).slice(2)}`; -} +function generateFeatureId(): string { + return `feature-${generateUUID()}`; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 77 - 80, Replace the ad-hoc generateFeatureId implementation with a UUID-based generator to avoid collisions: update the generateFeatureId function to return generateUUID() (or generateUUID().toString()) and add the appropriate import for generateUUID; keep the function name generateFeatureId so callers don't change, ensure the return type remains string, and remove the Date.now()/Math.random logic in the current function.
49-50: Prefer components barrel export for PhaseModelSelector.If it’s an internal UI component, import it from the components index barrel for stability.
Based on learnings: "In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports. The guidance about importing from automaker/* packages is only for cross-module imports from shared libraries; do not apply that rule to intra-app imports. This improves import stability and refactorability within the app."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 49 - 50, The import of PhaseModelSelector should use the app's components barrel export instead of the direct internal path; update the import for PhaseModelSelector in pr-comment-resolution-dialog.tsx to import PhaseModelSelector from the components index barrel (the top-level components export) rather than '@/components/views/settings-view/model-defaults/phase-model-selector' so intra-app refactors remain stable and predictable.apps/ui/src/components/views/board-view.tsx (1)
37-40: Prefer components barrel export for PRCommentResolutionDialog.Based on learnings: "In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports. The guidance about importing from automaker/* packages is only for cross-module imports from shared libraries; do not apply that rule to intra-app imports. This improves import stability and refactorability within the app."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 37 - 40, Replace the direct path import of PRCommentResolutionDialog and PRCommentResolutionPRInfo with the app's components barrel export; locate the import that currently references '@/components/dialogs/pr-comment-resolution-dialog' and change it to import { PRCommentResolutionDialog, type PRCommentResolutionPRInfo } from the components index barrel (the central components export) so the two symbols are imported via the barrel export instead of the dialog file path.
🤖 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/github/routes/list-pr-review-comments.ts`:
- Around line 89-301: Extract the domain logic in
fetchReviewThreadResolvedStatus and fetchPRReviewComments out of the route file
into a new service module (e.g., export both functions from
apps/server/src/services/pr-review-comments.service.ts) and update
createListPRReviewCommentsHandler to import and call these service functions;
specifically, move all GraphQL/gh CLI calls, parsing, enrichment, and sorting
into the service, keep only request/response plumbing and error handling in the
route, and ensure function signatures (projectPath, owner, repo, prNumber) and
exported names remain unchanged so existing callers continue to work.
- Around line 105-115: The GraphQL query using reviewThreads(first: 100) can
silently truncate data; update the reviewThreads selection to include pageInfo {
hasNextPage } and after executing the query (in the list-pr-review-comments
handler where the query result is processed) check
reviewThreads.pageInfo.hasNextPage and emit a warning log when true so
truncation is surfaced; leave a TODO comment noting that full fix requires
cursor-based pagination (iterating with reviewThreads.nodes pageInfo.endCursor
across spawn calls).
In `@apps/server/src/routes/github/routes/resolve-pr-comment.ts`:
- Around line 43-113: The function executeReviewThreadMutation is business logic
and should be moved out of the route into a service module (e.g.,
githubPRCommentService) so the route delegates to it; create and export
executeReviewThreadMutation from the new service with the exact signature
(projectPath: string, threadId: string, resolve: boolean): Promise<{ isResolved:
boolean }>, copy all internals (spawn usage, timeout handling, response parsing
and errors) unchanged, ensure it imports any shared constants/types it uses
(execEnv, GITHUB_API_TIMEOUT_MS, GraphQLMutationResponse), then replace the
in-route implementation with an import of executeReviewThreadMutation and call
it from the route handler.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 588-631: The script menu items can be clicked but do nothing when
their optional callbacks are missing; update the DropdownMenuItem props to
disable the items when their handlers are not provided: for the init script item
(function onRunInitScript) set disabled if !hasInitScript || !onRunInitScript,
for each terminal script item (onRunTerminalScript) set disabled if
!onRunTerminalScript, and for the "Edit Commands & Scripts" item (onEditScripts)
set disabled if !onEditScripts; keep the existing onClick handlers but rely on
the disabled prop so clicks are visibly and functionally prevented.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 169-231: The handler handleAddFeatureFromIssue is dropping
excludedPipelineSteps because the inline featureData type and the feature object
sent to api.features.create do not include excludedPipelineSteps; update the
featureData parameter to include excludedPipelineSteps (e.g.,
excludedPipelineSteps: string[] or appropriate type) and add
excludedPipelineSteps to the constructed feature object before calling
api.features.create (the same place where id, title, description, etc. are set)
so the created feature persists pipeline exclusions.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 469-518: The drag handle (GripVertical) is not
keyboard-accessible; make it focusable and operable via keyboard by adding
tabIndex={0}, role="button", and an onKeyDown handler that listens for
ArrowUp/ArrowDown (and Home/End if desired) to call the component's reorder
logic (reuse or call into handleDragStart/handleDrop wrappers or a new
moveUp/moveDown helper to change script order); alternatively or additionally
add small focusable move-up/move-down Button elements next to GripVertical that
invoke the same reorder helpers so users can reorder scripts with keyboard
only—update the focusable element and handlers in the same component where
GripVertical is rendered and ensure any helpers (moveUp/moveDown) update state
consistently with handleUpdateScript/handleRemoveScript and trigger the same
persistence/side effects as the drag handlers.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 299-302: Stale entries are left in the sessionCommandOverrides Map
when a session is killed before its initial command runs; update killTerminal
(and inside the sessionIds.map(...) loop in killTerminalTab) to remove the
killed session's key from sessionCommandOverrides and also remove any
corresponding id from newSessionIds by calling setSessionCommandOverrides(prev
=> { const next = new Map(prev); next.delete(sessionId); return next; }) (and
analogously update the newSessionIds state), ensuring you reference the existing
sessionId variable being killed and the state setters sessionCommandOverrides /
setSessionCommandOverrides and newSessionIds / setNewSessionIds so the map no
longer grows with entries for killed-before-ran sessions.
---
Outside diff comments:
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 1985-2030: The maximized TerminalPanel rendered when
terminalState.maximizedSessionId is set is missing the runCommandOnConnect and
onCommandRan props, so propagate the same handlers the regular panel uses: add
runCommandOnConnect={runCommandOnConnect} and onCommandRan={handleCommandRan}
(or the equivalent names used in your file) to the TerminalPanel inside the
TerminalErrorBoundary for the maximized branch so commands queued on session
creation fire correctly and handleCommandRan runs to clear
newSessionIds/sessionCommandOverrides.
---
Duplicate comments:
In `@apps/server/src/routes/github/routes/resolve-pr-comment.ts`:
- Around line 63-98: The gh.on('error', ...) handler references timeoutId before
its const declaration, which can create TDZ confusion; to fix, declare timeoutId
(e.g., let timeoutId: NodeJS.Timeout | undefined) before registering
gh.on('error') and assign the setTimeout result to it afterward (or move the
setTimeout block above the gh.on('error') registration) so the error handler can
safely call clearTimeout(timeoutId); keep the rest of the Promise logic
(spawn('gh', ...), gh.stdout/stderr handlers, gh.on('close'), gh.stdin writes)
unchanged.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 244-269: handleConvertToTask currently builds `description` as an
array that includes intentional empty-string separators but then calls
`.filter(Boolean)`, which removes those blank lines; update
`handleConvertToTask` so it mirrors `buildIssueDescription`'s pattern: create a
`parts` array (or keep `description` as an array) and use conditional
`parts.push('', someValue)` calls for optional sections (e.g.,
`validation.suggestedFix`, `validation.relatedFiles`) instead of filtering;
ensure you do not call `.filter(Boolean)` before `.join('\n')` so the deliberate
blank lines remain, and keep identifiers like `id:
\`issue-${issue.number}-${generateUUID()}\``, `title`, `category`, and `status`
unchanged.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 90-99: The success toast is shown even when api.features?.run is
undefined (a silent no-op); update the block around createFeature.mutateAsync,
getElectronAPI(), and api.features?.run to explicitly guard for api.features
before calling run (or reuse the board-view start pattern), await the run call
and surface errors (throw or show toast.error) if the feature run cannot start,
and only call toast.success('Feature created and started', ...) after confirming
the run actually started for currentProject.path and featureId.
---
Nitpick comments:
In `@apps/server/src/routes/github/routes/list-pr-review-comments.ts`:
- Around line 13-14: The const declaration "const execFileAsync =
promisify(execFile)" is interleaved with import statements and triggers the
import/first lint rule; move that declaration below all import lines so the top
of the module only contains imports, then retain "execFileAsync" (and its
references to promisify and execFile) in its current module scope after the
imports to restore idiomatic ordering and satisfy ESLint.
- Line 266: Replace the fallback using logical OR with the nullish coalescing
operator for the comment line value: change the expression that sets line from
"c.line || c.original_line" to use "c.line ?? c.original_line" so the mapping
that constructs the PR review comment object (the code handling variable c and
its line/original_line properties in the list-pr-review-comments logic)
preserves defined zero-like values while falling back only on null/undefined.
In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts`:
- Line 9: The file still imports and uses promisify(exec) (symbol: exec and
promisify) instead of execFile; replace the exec import with execFile and create
a promisified execFile (e.g., const execFileAsync = promisify(execFile)), then
update any calls that currently pass a full command string (e.g., usages of
execAsync or direct exec calls inside the generateCommitMessage flow) to call
execFileAsync with the command as the first arg and an explicit args array as
the second argument so arguments are not shell-interpolated; also update the
other occurrences noted (around the blocks previously at 132-135 and 139-142) to
use execFileAsync similarly.
- Around line 36-62: The timeout cleanup can lose the original timeout error if
iterator.return() throws; update the Promise.race catch path to capture the
original error, then call iterator.return()?. If iterator.return() throws, catch
and ignore or log that secondary error, but rethrow the original error so the
timeout error from timeoutPromise (and not the teardown error) is
preserved—apply this change around the existing iterator, iterator.return,
timerId and timeoutPromise logic used with Promise.race.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 77-80: Replace the ad-hoc generateFeatureId implementation with a
UUID-based generator to avoid collisions: update the generateFeatureId function
to return generateUUID() (or generateUUID().toString()) and add the appropriate
import for generateUUID; keep the function name generateFeatureId so callers
don't change, ensure the return type remains string, and remove the
Date.now()/Math.random logic in the current function.
- Around line 49-50: The import of PhaseModelSelector should use the app's
components barrel export instead of the direct internal path; update the import
for PhaseModelSelector in pr-comment-resolution-dialog.tsx to import
PhaseModelSelector from the components index barrel (the top-level components
export) rather than
'@/components/views/settings-view/model-defaults/phase-model-selector' so
intra-app refactors remain stable and predictable.
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 37-40: Replace the direct path import of PRCommentResolutionDialog
and PRCommentResolutionPRInfo with the app's components barrel export; locate
the import that currently references
'@/components/dialogs/pr-comment-resolution-dialog' and change it to import {
PRCommentResolutionDialog, type PRCommentResolutionPRInfo } from the components
index barrel (the central components export) so the two symbols are imported via
the barrel export instead of the dialog file path.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 105-127: Merge the two sibling useEffect blocks that both depend
on [projectSettings, project.path] into a single useEffect which first guards if
(projectSettings) and then performs both groups of state updates: setDevCommand
and setOriginalDevCommand using projectSettings.devCommand/testCommand, and
construct scriptList from projectSettings.terminalScripts or
DEFAULT_TERMINAL_SCRIPTS and call setScripts plus setOriginalScripts (preserving
the deep-copy via JSON.parse(JSON.stringify(...))). Keep the same dependency
array and ensure you reference the existing identifiers (useEffect,
projectSettings, setDevCommand, setOriginalDevCommand, setTestCommand,
setOriginalTestCommand, setScripts, setOriginalScripts,
DEFAULT_TERMINAL_SCRIPTS) so functionality and immutability are preserved.
- Line 125: Replace the JSON round-trip deep clone usage with structuredClone:
where you call setOriginalScripts(JSON.parse(JSON.stringify(scriptList))) (and
the similar occurrences for original scripts at the other sites), use
setOriginalScripts(structuredClone(scriptList)) instead; locate the calls to
setOriginalScripts and any JSON.parse(JSON.stringify(...)) clones in
commands-and-scripts-section.tsx and switch them to structuredClone(scriptList)
to leverage the built-in deep clone (apply the same change to the other two
similar occurrences mentioned).
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 299-302: Replace the React state type Map<string,string> with a
plain Record<string,string> for sessionCommandOverrides and update
setSessionCommandOverrides usage: initialize sessionCommandOverrides as an empty
object, change any immutable updates that currently do new Map(prev).set(id,
cmd) to functional updates using object spread (e.g., prev => ({ ...prev, [id]:
command })) and change deletions that clone the Map then delete to creating a
shallow copy and deleting the key (or using object rest to omit the key); update
all usages of sessionCommandOverrides, setSessionCommandOverrides, and any
helper logic that relied on Map methods to use plain object access, keys() via
Object.keys, and typing accordingly so the state remains serializable and easier
to copy.
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
apps/server/src/routes/github/routes/list-pr-review-comments.ts
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx
Show resolved
Hide resolved
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/terminal-view.tsx (1)
1185-1190:⚠️ Potential issue | 🟡 Minor
catchblock omits the samesessionCommandOverrides/newSessionIdscleanup.When
apiDeleteRawthrows a network error,removeTerminalFromLayoutis called but the two map/set cleanups added to thetryblock (lines 1165-1175) are skipped. The killed session's entries become permanently stale because the layout no longer contains the session sohandleCommandRanwill never fire for it.🐛 Proposed fix
} catch (err) { logger.error('Kill session error:', err); // Still remove from UI on network error - better UX than leaving broken terminal removeTerminalFromLayout(sessionId); + setSessionCommandOverrides((prev) => { + const next = new Map(prev); + next.delete(sessionId); + return next; + }); + setNewSessionIds((prev) => { + const next = new Set(prev); + next.delete(sessionId); + return next; + }); }🤖 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 1185 - 1190, The catch block for the apiDeleteRaw call currently only logs and calls removeTerminalFromLayout, but it must also perform the same cleanup as the try block: remove the killed session's entries from sessionCommandOverrides and newSessionIds so they don't remain stale; update the catch in the function that calls apiDeleteRaw (the same scope where sessionCommandOverrides and newSessionIds are used) to delete sessionCommandOverrides.delete(sessionId) and newSessionIds.delete(sessionId) (or the equivalent removal logic used in the try block) before or after calling removeTerminalFromLayout, preserving the existing logging behavior.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (1)
575-608:⚠️ Potential issue | 🟡 MinorAsync
onClickhandlers swallow errors — wrap in try/catch.Both the "Copy To..." (line 575) and "Move To..." (line 597) handlers are
asyncbut have no error handling.DropdownMenuItem.onClickexpects a synchronousvoidreturn, so the returnedPromiseis discarded. Any rejection fromopenFileBrowseroronCopyItem/onMoveItemsilently becomes an unhandled promise rejection with no user feedback.🛡️ Proposed fix
- onClick={async (e) => { + onClick={(e) => { e.stopPropagation(); const parentPath = node.path.substring(0, node.path.lastIndexOf('/')) || '/'; - const destPath = await openFileBrowser({ - title: `Copy "${node.name}" To...`, - description: 'Select the destination folder for the copy operation', - initialPath: parentPath, - }); - if (destPath) { - await onCopyItem(node.path, destPath); - } + openFileBrowser({ + title: `Copy "${node.name}" To...`, + description: 'Select the destination folder for the copy operation', + initialPath: parentPath, + }).then((destPath) => { + if (destPath) return onCopyItem(node.path, destPath); + }).catch((err) => { + console.error('Copy operation failed', err); + }); }}Apply the same pattern to the "Move To..." handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 575 - 608, Wrap the bodies of the inline onClick handlers on the "Copy To..." and "Move To..." DropdownMenuItem entries (the anonymous async functions referencing node.path, node.name, openFileBrowser, onCopyItem and onMoveItem) in try/catch so no rejection bubbles out to DropdownMenuItem (which expects void). Specifically, ensure you call e.stopPropagation(), then run the async work inside try { const destPath = await openFileBrowser(...); if (destPath) await onCopyItem(...) / await onMoveItem(...); } catch (err) { handle the error (e.g. console.error or show a user toast) } so the handler never returns a rejected promise.
🧹 Nitpick comments (13)
apps/ui/src/components/ui/app-error-boundary.tsx (2)
90-93: Addtype="button"to the reload button.Without an explicit
type, browsers default totype="submit". While there's no parent<form>here today, it's defensive best practice to be explicit.♻️ Proposed fix
<button + type="button" onClick={this.handleReload} className="inline-flex items-center gap-2 rounded-md border border-border bg-background px-4 py-2 text-sm font-medium text-foreground shadow-sm transition-colors hover:bg-muted focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/app-error-boundary.tsx` around lines 90 - 93, The reload button rendered with onClick={this.handleReload} lacks an explicit type, so add type="button" to that <button> element (the one with className and onClick={this.handleReload}) to prevent browsers treating it as a submit button; update the JSX for the button element to include type="button" while leaving the existing onClick and className intact.
118-120: Consider including the stack trace in the technical details section.Currently only
error.messageis shown. In an Electron context, users can share the collapsible section contents with support;error.stackis far more actionable than the bare message alone, and there are no confidentiality concerns with stack traces in this desktop app.♻️ Proposed fix
<pre className="mt-2 p-3 bg-muted/50 rounded-md text-left overflow-auto max-h-32 border border-border"> - {this.state.error.message} + {this.state.error.stack ?? this.state.error.message} </pre>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/app-error-boundary.tsx` around lines 118 - 120, The technical details section currently renders only this.state.error.message; update the AppErrorBoundary (app-error-boundary.tsx) render block that shows the <pre> to include the full stack trace (this.state.error.stack) instead of or in addition to the message, falling back to the message or String(this.state.error) if stack is missing, and ensure you safely access error (check this.state.error) to avoid undefined access before rendering.apps/server/src/services/github-pr-comment.service.ts (2)
1-11: Service extraction looks good; minor dependency direction concern.The service is correctly placed under
services/per project conventions. However,execEnvis imported from a route-layer module (../routes/github/routes/common.js), which inverts the typical dependency direction (services shouldn't depend on routes). Consider extracting shared utilities likeexecEnvinto a common location (e.g.,lib/orutils/) that both routes and services can import from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/github-pr-comment.service.ts` around lines 1 - 11, The service github-pr-comment.service.ts wrongly imports execEnv from a route-layer module, creating an inverted dependency; move execEnv into a shared module (e.g., lib or utils) and update imports in both the service and the route to reference the new shared location so services no longer depend on routes—specifically, extract the execEnv function used in github-pr-comment.service.ts (and any other callers) into a new shared file and replace the import from ../routes/github/routes/common.js with the new shared path.
53-88: Duplicate spawn-based GraphQL execution pattern — consider extracting a shared helper.This spawn + timeout + JSON-parse pattern is duplicated almost identically in
pr-review-comments.service.ts(fetchReviewThreadResolvedStatus, lines 137–172). Both handle timeout, stderr collection, close-event parsing, and error rejection in the same way. A shared helper (e.g.,executeGhGraphQL(projectPath, requestBody): Promise<T>) would reduce duplication and centralize error/timeout handling.♻️ Sketch of a shared helper
// e.g., in lib/gh-graphql.ts export async function executeGhGraphQL<T>( projectPath: string, requestBody: string, timeoutMs = 30000 ): Promise<T> { let timeoutId: NodeJS.Timeout | undefined; return new Promise<T>((resolve, reject) => { const gh = spawn('gh', ['api', 'graphql', '--input', '-'], { cwd: projectPath, env: execEnv, }); gh.on('error', (err) => { clearTimeout(timeoutId); reject(err); }); timeoutId = setTimeout(() => { gh.kill(); reject(new Error('GitHub GraphQL API request timed out')); }, timeoutMs); let stdout = ''; let stderr = ''; gh.stdout.on('data', (data: Buffer) => (stdout += data.toString())); gh.stderr.on('data', (data: Buffer) => (stderr += data.toString())); gh.on('close', (code) => { clearTimeout(timeoutId); if (code !== 0) { return reject(new Error(`gh process exited with code ${code}: ${stderr}`)); } try { resolve(JSON.parse(stdout)); } catch (e) { reject(e); } }); gh.stdin.write(requestBody); gh.stdin.end(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/github-pr-comment.service.ts` around lines 53 - 88, The spawn+timeout+JSON-parse logic in github-pr-comment.service.ts (the Promise block creating `gh`, collecting stdout/stderr, handling `gh.on('error')`, timeout via `GITHUB_API_TIMEOUT_MS`, and parsing JSON into `GraphQLMutationResponse`) is duplicated in pr-review-comments.service.ts (`fetchReviewThreadResolvedStatus`). Extract this into a shared helper like `executeGhGraphQL<T>(projectPath: string, requestBody: string, timeoutMs?: number): Promise<T>` (e.g., lib/gh-graphql.ts) that accepts `execEnv`/timeout, centralizes error/timeout handling, returns parsed JSON, and then replace the Promise blocks in `github-pr-comment.service.ts` and `pr-review-comments.service.ts` to call this helper (preserve types such as `GraphQLMutationResponse` and `fetchReviewThreadResolvedStatus` return types).apps/server/src/services/pr-review-comments.service.ts (1)
86-89: Import statement placed after type/constant declarations.The
import { createLogger }on line 88 is separated from the other imports (lines 9–11) by ~75 lines of type declarations and constants. Move it to the top with the other imports for conventional grouping.♻️ Move import to the top
import { spawn, execFile } from 'child_process'; import { promisify } from 'util'; import { execEnv, logError } from '../routes/github/routes/common.js'; +import { createLogger } from '@automaker/utils'; const execFileAsync = promisify(execFile); +const logger = createLogger('PRReviewCommentsService');Then remove lines 86–89.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-review-comments.service.ts` around lines 86 - 89, The import for createLogger and the logger instantiation (import { createLogger } and const logger = createLogger('PRReviewCommentsService')) are placed after type/constants instead of with the other module imports; move the import statement for createLogger up into the top import block alongside the other imports and keep the const logger = createLogger('PRReviewCommentsService') near the top (or immediately after imports), then remove the duplicate import/instantiation currently located around PRReviewCommentsService to maintain conventional grouping and avoid duplicate declarations.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
1042-1054: IIFE in JSX adds unnecessary complexity; consider extracting.The immediately-invoked function expression used to build
prInfoand return JSX is harder to read and prevents React from optimizing the block. A simpleuseMemoor a local variable above the return would be cleaner:♻️ Suggested refactor
Compute
prInfoonce near the other derived values (e.g., around line 247):const prInfo: PRInfo | null = worktree.pr ? { number: worktree.pr.number, title: worktree.pr.title, url: worktree.pr.url, state: worktree.pr.state, author: '', body: '', comments: [], reviewComments: [], } : null;Then in JSX, replace the IIFE with a simple conditional:
{showPRInfo && prInfo && ( <DropdownMenuSub> ... </DropdownMenuSub> )}🤖 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 1042 - 1054, Extract the IIFE that builds prInfo into a stable derived value (either a local variable near other derived values or wrapped in useMemo) so JSX becomes a simple conditional render; specifically compute prInfo from worktree.pr (returning null when absent) and then replace the (() => { ... })() block with {showPRInfo && prInfo && (<DropdownMenuSub>...)}; update any handlers that expect PRInfo to accept the possibly-null prInfo (or guard before calling) so symbols to change are prInfo, worktree.pr, showPRInfo, and the JSX subtree using DropdownMenuSub.apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx (2)
138-165: Silent removal of incomplete scripts on save — consider user feedback.Line 141 silently filters out scripts where
nameorcommandis empty. After save,setScripts(normalizedScripts)(line 160) removes those entries from the UI with no notification. A user who partially filled a script row may be surprised it vanished.Consider showing a brief toast or visual indicator when incomplete scripts are dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 138 - 165, handleSave currently filters out incomplete script entries (scripts.filter(...)) and then calls setScripts(normalizedScripts), which silently removes user-entered but incomplete rows; update the handler to detect when any scripts were dropped (compare scripts.length vs normalizedScripts.length) and, when drops occurred, call the app's notification/toast API (e.g., useToast, enqueueSnackbar, or existing notify util) to show a brief message like "Empty script entries were discarded" before or after calling updateSettingsMutation.mutate; keep the normalization and state updates (setScripts, setOriginalScripts, setDevCommand, setTestCommand) as-is, but ensure the toast is triggered in the same onSuccess callback so the user sees feedback only when the save succeeded.
94-124: Potential stale-data flash when switching projects.The reset effect (line 95) and the sync effect (line 105) both fire when
project.pathchanges. If TanStack Query returns stale/cached data for the old project before the new query resolves (e.g., viaplaceholderDataorkeepPreviousData), effect 2 will immediately overwrite the reset with the previous project's settings.Consolidating into a single effect that checks whether the loaded data belongs to the current project—or guarding the sync effect against stale data—would be more robust:
♻️ Suggested consolidation
- // Reset local state when project changes - useEffect(() => { - setDevCommand(''); - setOriginalDevCommand(''); - setTestCommand(''); - setOriginalTestCommand(''); - setScripts([]); - setOriginalScripts([]); - }, [project.path]); - - // Sync commands and scripts state when project settings load - useEffect(() => { - if (projectSettings) { + // Sync commands and scripts state when project settings load; + // reset to empty when settings are not yet available for the current project. + useEffect(() => { + if (!projectSettings) { + setDevCommand(''); + setOriginalDevCommand(''); + setTestCommand(''); + setOriginalTestCommand(''); + setScripts([]); + setOriginalScripts([]); + } else { // Commands const dev = projectSettings.devCommand || ''; ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 94 - 124, The two useEffect hooks cause a race where the sync effect can apply stale projectSettings for the previous project when project.path changes; replace both with a single effect that (1) always clears local state when project.path changes and (2) only applies projectSettings if it actually belongs to the current project—e.g., check an identifier on projectSettings (projectSettings.projectPath or projectSettings.projectId) against project.path (or project.id) before calling setDevCommand, setOriginalDevCommand, setTestCommand, setOriginalTestCommand, setScripts, and setOriginalScripts (preserving DEFAULT_TERMINAL_SCRIPTS and structuredClone usage); this ensures switching projects never briefly flashes the previous project's settings.apps/server/src/routes/worktree/routes/generate-commit-message.ts (2)
242-244: Nit: redundant inner.trim()onmessage
messageis already the result ofresponseText.trim()(line 242), and the!messagecheck already handles the empty-string case (since''is falsy). The inner.trim()on line 244 is a no-op.🔧 Suggested cleanup
- if (!message || message.trim().length === 0) { + if (!message) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 242 - 244, The check after creating const message = responseText.trim() redundantly calls message.trim() again; update the condition to simply if (!message) (or if (message.length === 0)) to detect an empty string and remove the inner .trim() call—locate the variable message in the generate-commit-message route handler and replace the `if (!message || message.trim().length === 0)` with `if (!message)`.
45-46: Floatingiterator.next()promise after timeoutWhen
timeoutPromisewins the race,iterator.return()is correctly called to signal the generator to stop. However, theiterator.next()call already submitted toPromise.raceremains pending and is abandoned. If the underlying AI provider backs the async iterable with an open network connection, that connection may not be torn down until it resolves or the server process eventually GCs it.This is low-severity for a one-shot HTTP handler (the connection naturally closes when the response is sent), but worth noting if the provider implementation holds resources.
Also applies to: 50-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 45 - 46, The Promise.race call leaves the pending iterator.next() promise unresolved when timeoutPromise wins, potentially leaving provider resources open; fix by storing the next promise (e.g., const nextPromise = iterator.next()) before racing, and when the timeout branch runs call iterator.return() and then await nextPromise.catch(() => {}) to let that pending promise settle/cleanup (or otherwise cancel via an AbortController if the provider supports it); update uses around iterator.next(), iterator.return(), timeoutPromise and Promise.race to ensure the pending nextPromise is awaited or cancelled after timeout.apps/ui/src/components/views/github-issues-view.tsx (1)
463-465: MemoizeprefilledDescriptioninstead of computing it on every render.
buildIssueDescription(createFeatureIssue)is invoked inline in JSX, so it runs on every re-render while the dialog is open (e.g., user interaction triggering parent state changes).♻️ Suggested refactor
+ const prefilledDescription = useMemo( + () => (createFeatureIssue ? buildIssueDescription(createFeatureIssue) : undefined), + [createFeatureIssue, buildIssueDescription] + );Then in JSX:
- prefilledDescription={ - createFeatureIssue ? buildIssueDescription(createFeatureIssue) : undefined - } + prefilledDescription={prefilledDescription}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view.tsx` around lines 463 - 465, The JSX currently calls buildIssueDescription(createFeatureIssue) inline for the prefilledDescription prop causing it to recompute on every render; wrap that call in a memo (useMemo) so the description is only built when createFeatureIssue changes (e.g., const prefilledDescription = useMemo(() => createFeatureIssue ? buildIssueDescription(createFeatureIssue) : undefined, [createFeatureIssue])) and pass that memoized prefilledDescription into the prefilledDescription prop; update references in the same component (github-issues-view.tsx) where prefilledDescription, buildIssueDescription, and createFeatureIssue are used.apps/ui/src/routes/__root.tsx (1)
401-415: Optional:automaker:logged-outhandler still mixes directnavigate()with state mutation.This handler (pre-existing, not changed here) calls both
setAuthState()andnavigate({ to: '/logged-out' }), which means the routing effect will also fire and attempt anothernavigate({ to: '/logged-out' })— the exact pattern that was fixed forinitAuth. TanStack Router handles same-destination duplicates without throwing error#185, and thelocation.pathname !== '/logged-out'guard at line 790 provides a second safety net, so this is not currently broken. But for consistency with the new approach, you could consider removing the inline navigate call here and relying on the routing effect exclusively.♻️ Suggested refactor for consistency
const handleLoggedOut = () => { logger.warn('automaker:logged-out event received!'); useAuthStore.getState().setAuthState({ isAuthenticated: false, authChecked: true }); - - if (location.pathname !== '/logged-out') { - logger.warn('Navigating to /logged-out due to logged-out event'); - navigate({ to: '/logged-out' }); - } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/routes/__root.tsx` around lines 401 - 415, The automaker:logged-out handler mixes direct navigation with state mutation; update handleLoggedOut to only call useAuthStore.getState().setAuthState({ isAuthenticated: false, authChecked: true }) and remove the inline navigate({ to: '/logged-out' }) and its location.pathname !== '/logged-out' guard so the centralized routing effect can handle navigation consistently; keep the window.addEventListener/removeEventListener and the logger.warn calls intact.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (1)
156-169: No visual feedback when an invalid name is entered — consider an error state.When the user types
foo/barand presses Enter,handleSubmitsilently returns and the editor stays open (line 163–165). On blur with the same input,onCancel()is called and the editor disappears (line 191). In both cases there is no indication of why the input was rejected — the user is left guessing.Consider tracking an error boolean in
InlineInputand rendering a brief inline message or red border when validation fails:♻️ Suggested approach
function InlineInput({ defaultValue, onSubmit, onCancel, placeholder }) { const [value, setValue] = useState(defaultValue || ''); + const [hasError, setHasError] = useState(false); const inputRef = useRef<HTMLInputElement>(null); const submittedRef = useRef(false); ... const handleSubmit = useCallback(() => { if (submittedRef.current) return; const trimmed = value.trim(); if (!trimmed) { onCancel(); return; } if (!isValidFileName(trimmed)) { + setHasError(true); return; } + setHasError(false); submittedRef.current = true; onSubmit(trimmed); }, [value, onSubmit, onCancel]); ... return ( <input ... - className="text-sm bg-muted border border-border rounded px-1 py-0.5 w-full outline-none focus:border-primary" + className={cn( + 'text-sm bg-muted border rounded px-1 py-0.5 w-full outline-none', + hasError ? 'border-destructive focus:border-destructive' : 'border-border focus:border-primary', + )} /> ); }Also applies to: 183-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 156 - 169, handleSubmit currently silently returns when isValidFileName(trimmed) fails, leaving the editor open with no feedback; add a local error state (e.g., const [nameError, setNameError] = useState<string | null>(null)) in the same component that owns handleSubmit/InlineInput, set nameError with a short message when isValidFileName(trimmed) is false inside handleSubmit (and likewise on blur handling where onCancel() is called), prevent onCancel from closing the editor when nameError is set, pass an error prop/message and an error boolean to InlineInput so it can render a red border/inline message, and clear nameError on input change or when a successful onSubmit(trimmed) occurs; reference handleSubmit, isValidFileName, submittedRef, onCancel, onSubmit, and InlineInput when applying these changes.
🤖 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/github-pr-comment.service.ts`:
- Around line 30-34: The mutation function executeReviewThreadMutation currently
changes GitHub PR review thread state but no event is emitted; update the flow
so an event is emitted to the server event emitter after a successful mutation:
either add an EventEmitter parameter to executeReviewThreadMutation(projectPath,
threadId, resolve, eventEmitter) and emit a descriptive event (e.g.,
"github:reviewThread:updated" with payload { projectPath, threadId, isResolved
}) inside executeReviewThreadMutation, or keep the signature and emit from the
route handler in resolve-pr-comment.ts immediately after calling
executeReviewThreadMutation, using the same emitter/event name pattern used by
validate-issue.ts so the frontend receives the update via WebSocket.
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 220-227: Both execFileAsync invocations that call the GitHub CLI
(the one returning commentsOutput and the one returning inlineCommentsOutput)
lack a timeout and can hang indefinitely; update both calls to pass a timeout
option (e.g., 30000 ms) in their options object alongside cwd and env so the
promise will reject if the gh CLI stalls, keeping the rest of the existing error
handling intact.
- Around line 113-129: The inner comments list lacks pagination info so threads
with >100 comments are silently truncated; update the GraphQL query to include
comments { pageInfo { hasNextPage } } inside reviewThreads and then, in the loop
that builds the resolved map (the code that iterates reviewThreads.nodes to
populate the resolved map), check each thread.comments.pageInfo.hasNextPage and
emit a warning (use the same logger used elsewhere, e.g., processLogger.warn or
logger.warn) noting the thread id and that comments were truncated;
alternatively document the limitation if you choose not to support pagination.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 325-347: The current global isResolvingThread state disables/spins
every row; change this to track the active thread id (e.g., resolvingThreadId)
and use a derived boolean like isResolvingThisThread = comment.threadId ===
resolvingThreadId to drive disabled, spinner, and CSS classes; update the
resolve handler (handleResolveClick) to set resolvingThreadId = comment.threadId
before starting the async resolve and clear it in finally; apply the same
per-thread boolean replacement wherever resolveThread.isPending or
isResolvingThread is used (including the other resolve/unresolve button block)
so only the targeted thread shows the pending state.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1039-1092: The PR state badge currently uses hard-coded green
classes causing merged/closed PRs to appear green; update the rendering where
worktree.pr is used (the DropdownMenuItem showing "PR #{worktree.pr!.number}"
and the span with the badge) to compute classes based on worktree.pr!.state (or
the derived prInfo.state) and apply different class sets for OPEN (green),
MERGED (purple/indigo), and CLOSED (gray/red) so the background and text colors
reflect the state; ensure the conditional mapping is used where the span's
className is set so the badge color changes according to the PR state.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 143-156: The Save button rendered when showSaveButton and onSave
are truthy lacks an explicit accessible name; add an aria-label (for example
aria-label="Save file") to the button element that uses props showSaveButton,
onSave, isDirty and the Save icon component so screen readers announce the
control consistently in addition to the existing title attribute.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 65-113: The feature is currently created with status 'in_progress'
and can remain stuck if api.features.run is unavailable or throws; change
createFeature.mutateAsync to create the feature with status 'backlog' (modify
the local feature object’s status before calling createFeature.mutateAsync),
then attempt to start via getElectronAPI().features.run(currentProject.path,
featureId); only after a successful run update the feature status to
'in_progress' (either via an API call like api.features.update or a second
createFeature.mutateAsync/update call that sets feature.status = 'in_progress');
ensure any run errors are caught and reported (toast.error) without leaving the
feature marked as in_progress.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Line 103: refreshSettingsFromServer currently writes back many per-project
fields but omits restoring serverSettings.currentWorktreeByProject, so add logic
inside the useAppStore.setState block in refreshSettingsFromServer to merge
serverSettings.currentWorktreeByProject into the store (same place where
lastSelectedSessionByProject, projectHistory and projectHistoryIndex are
restored); specifically, set the store's currentWorktreeByProject to
serverSettings.currentWorktreeByProject when present (falling back to existing
in-memory value if undefined) so the field persisted by SETTINGS_FIELDS_TO_SYNC
is actually rehydrated on refresh/login.
---
Outside diff comments:
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 575-608: Wrap the bodies of the inline onClick handlers on the
"Copy To..." and "Move To..." DropdownMenuItem entries (the anonymous async
functions referencing node.path, node.name, openFileBrowser, onCopyItem and
onMoveItem) in try/catch so no rejection bubbles out to DropdownMenuItem (which
expects void). Specifically, ensure you call e.stopPropagation(), then run the
async work inside try { const destPath = await openFileBrowser(...); if
(destPath) await onCopyItem(...) / await onMoveItem(...); } catch (err) { handle
the error (e.g. console.error or show a user toast) } so the handler never
returns a rejected promise.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 1185-1190: The catch block for the apiDeleteRaw call currently
only logs and calls removeTerminalFromLayout, but it must also perform the same
cleanup as the try block: remove the killed session's entries from
sessionCommandOverrides and newSessionIds so they don't remain stale; update the
catch in the function that calls apiDeleteRaw (the same scope where
sessionCommandOverrides and newSessionIds are used) to delete
sessionCommandOverrides.delete(sessionId) and newSessionIds.delete(sessionId)
(or the equivalent removal logic used in the try block) before or after calling
removeTerminalFromLayout, preserving the existing logging behavior.
---
Duplicate comments:
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 588-638: Scripts submenu items must be disabled when their
handlers or config are absent; ensure the DropdownMenuItem for init script uses
disabled={ !hasInitScript || !onRunInitScript } (reference onRunInitScript and
hasInitScript), the terminal script items use disabled={ !onRunTerminalScript }
and call onRunTerminalScript(worktree, script.command), and the "Edit Commands &
Scripts" item uses disabled={ !onEditScripts } and calls onEditScripts(); update
those props in worktree-actions-dropdown.tsx (functions: onRunInitScript,
onRunTerminalScript, onEditScripts, and variable hasInitScript) so the UI
correctly prevents actions when handlers/config are missing.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 747-756: handleRootDragOver currently calls setDragState with
draggedPaths: [] which clears active dragged paths and breaks drag visuals;
change the update to preserve existing draggedPaths by using the functional
state updater and spreading the previous dragState (e.g., setDragState(prev =>
({ ...prev, dropTargetPath: effectivePath })) ) so only dropTargetPath is
updated while draggedPaths and other fields remain intact; update the handler in
handleRootDragOver to use this pattern and reference the
dragState/draggedPaths/dropTargetPath fields used by isDragging.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 261-289: The keyboard-reorder accessibility issue is resolved:
handleDragHandleKeyDown correctly intercepts ArrowUp/ArrowDown/Home/End,
prevents default, and calls moveScript which updates the list via setScripts
with bounds checks; no code changes required for moveScript,
handleDragHandleKeyDown, or setScripts.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts`:
- Around line 242-244: The check after creating const message =
responseText.trim() redundantly calls message.trim() again; update the condition
to simply if (!message) (or if (message.length === 0)) to detect an empty string
and remove the inner .trim() call—locate the variable message in the
generate-commit-message route handler and replace the `if (!message ||
message.trim().length === 0)` with `if (!message)`.
- Around line 45-46: The Promise.race call leaves the pending iterator.next()
promise unresolved when timeoutPromise wins, potentially leaving provider
resources open; fix by storing the next promise (e.g., const nextPromise =
iterator.next()) before racing, and when the timeout branch runs call
iterator.return() and then await nextPromise.catch(() => {}) to let that pending
promise settle/cleanup (or otherwise cancel via an AbortController if the
provider supports it); update uses around iterator.next(), iterator.return(),
timeoutPromise and Promise.race to ensure the pending nextPromise is awaited or
cancelled after timeout.
In `@apps/server/src/services/github-pr-comment.service.ts`:
- Around line 1-11: The service github-pr-comment.service.ts wrongly imports
execEnv from a route-layer module, creating an inverted dependency; move execEnv
into a shared module (e.g., lib or utils) and update imports in both the service
and the route to reference the new shared location so services no longer depend
on routes—specifically, extract the execEnv function used in
github-pr-comment.service.ts (and any other callers) into a new shared file and
replace the import from ../routes/github/routes/common.js with the new shared
path.
- Around line 53-88: The spawn+timeout+JSON-parse logic in
github-pr-comment.service.ts (the Promise block creating `gh`, collecting
stdout/stderr, handling `gh.on('error')`, timeout via `GITHUB_API_TIMEOUT_MS`,
and parsing JSON into `GraphQLMutationResponse`) is duplicated in
pr-review-comments.service.ts (`fetchReviewThreadResolvedStatus`). Extract this
into a shared helper like `executeGhGraphQL<T>(projectPath: string, requestBody:
string, timeoutMs?: number): Promise<T>` (e.g., lib/gh-graphql.ts) that accepts
`execEnv`/timeout, centralizes error/timeout handling, returns parsed JSON, and
then replace the Promise blocks in `github-pr-comment.service.ts` and
`pr-review-comments.service.ts` to call this helper (preserve types such as
`GraphQLMutationResponse` and `fetchReviewThreadResolvedStatus` return types).
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 86-89: The import for createLogger and the logger instantiation
(import { createLogger } and const logger =
createLogger('PRReviewCommentsService')) are placed after type/constants instead
of with the other module imports; move the import statement for createLogger up
into the top import block alongside the other imports and keep the const logger
= createLogger('PRReviewCommentsService') near the top (or immediately after
imports), then remove the duplicate import/instantiation currently located
around PRReviewCommentsService to maintain conventional grouping and avoid
duplicate declarations.
In `@apps/ui/src/components/ui/app-error-boundary.tsx`:
- Around line 90-93: The reload button rendered with onClick={this.handleReload}
lacks an explicit type, so add type="button" to that <button> element (the one
with className and onClick={this.handleReload}) to prevent browsers treating it
as a submit button; update the JSX for the button element to include
type="button" while leaving the existing onClick and className intact.
- Around line 118-120: The technical details section currently renders only
this.state.error.message; update the AppErrorBoundary (app-error-boundary.tsx)
render block that shows the <pre> to include the full stack trace
(this.state.error.stack) instead of or in addition to the message, falling back
to the message or String(this.state.error) if stack is missing, and ensure you
safely access error (check this.state.error) to avoid undefined access before
rendering.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1042-1054: Extract the IIFE that builds prInfo into a stable
derived value (either a local variable near other derived values or wrapped in
useMemo) so JSX becomes a simple conditional render; specifically compute prInfo
from worktree.pr (returning null when absent) and then replace the (() => { ...
})() block with {showPRInfo && prInfo && (<DropdownMenuSub>...)}; update any
handlers that expect PRInfo to accept the possibly-null prInfo (or guard before
calling) so symbols to change are prInfo, worktree.pr, showPRInfo, and the JSX
subtree using DropdownMenuSub.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 156-169: handleSubmit currently silently returns when
isValidFileName(trimmed) fails, leaving the editor open with no feedback; add a
local error state (e.g., const [nameError, setNameError] = useState<string |
null>(null)) in the same component that owns handleSubmit/InlineInput, set
nameError with a short message when isValidFileName(trimmed) is false inside
handleSubmit (and likewise on blur handling where onCancel() is called), prevent
onCancel from closing the editor when nameError is set, pass an error
prop/message and an error boolean to InlineInput so it can render a red
border/inline message, and clear nameError on input change or when a successful
onSubmit(trimmed) occurs; reference handleSubmit, isValidFileName, submittedRef,
onCancel, onSubmit, and InlineInput when applying these changes.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 463-465: The JSX currently calls
buildIssueDescription(createFeatureIssue) inline for the prefilledDescription
prop causing it to recompute on every render; wrap that call in a memo (useMemo)
so the description is only built when createFeatureIssue changes (e.g., const
prefilledDescription = useMemo(() => createFeatureIssue ?
buildIssueDescription(createFeatureIssue) : undefined, [createFeatureIssue]))
and pass that memoized prefilledDescription into the prefilledDescription prop;
update references in the same component (github-issues-view.tsx) where
prefilledDescription, buildIssueDescription, and createFeatureIssue are used.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 138-165: handleSave currently filters out incomplete script
entries (scripts.filter(...)) and then calls setScripts(normalizedScripts),
which silently removes user-entered but incomplete rows; update the handler to
detect when any scripts were dropped (compare scripts.length vs
normalizedScripts.length) and, when drops occurred, call the app's
notification/toast API (e.g., useToast, enqueueSnackbar, or existing notify
util) to show a brief message like "Empty script entries were discarded" before
or after calling updateSettingsMutation.mutate; keep the normalization and state
updates (setScripts, setOriginalScripts, setDevCommand, setTestCommand) as-is,
but ensure the toast is triggered in the same onSuccess callback so the user
sees feedback only when the save succeeded.
- Around line 94-124: The two useEffect hooks cause a race where the sync effect
can apply stale projectSettings for the previous project when project.path
changes; replace both with a single effect that (1) always clears local state
when project.path changes and (2) only applies projectSettings if it actually
belongs to the current project—e.g., check an identifier on projectSettings
(projectSettings.projectPath or projectSettings.projectId) against project.path
(or project.id) before calling setDevCommand, setOriginalDevCommand,
setTestCommand, setOriginalTestCommand, setScripts, and setOriginalScripts
(preserving DEFAULT_TERMINAL_SCRIPTS and structuredClone usage); this ensures
switching projects never briefly flashes the previous project's settings.
In `@apps/ui/src/routes/__root.tsx`:
- Around line 401-415: The automaker:logged-out handler mixes direct navigation
with state mutation; update handleLoggedOut to only call
useAuthStore.getState().setAuthState({ isAuthenticated: false, authChecked: true
}) and remove the inline navigate({ to: '/logged-out' }) and its
location.pathname !== '/logged-out' guard so the centralized routing effect can
handle navigation consistently; keep the
window.addEventListener/removeEventListener and the logger.warn calls intact.
Comment on lines
+30
to
+34
| export async function executeReviewThreadMutation( | ||
| projectPath: string, | ||
| threadId: string, | ||
| resolve: boolean | ||
| ): Promise<{ isResolved: boolean }> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the route handler that calls executeReviewThreadMutation
rg -n "executeReviewThreadMutation" apps/server/src/routes/ -B 5 -A 10Repository: AutoMaker-Org/automaker
Length of output: 2941
🏁 Script executed:
# Also check the structure of routes directory to understand the file organization
fd . apps/server/src/routes/ --type f -name "*.ts" -o -name "*.tsx"Repository: AutoMaker-Org/automaker
Length of output: 297
🏁 Script executed:
# Check for event emission in github-related routes
rg -n "createEventEmitter\|eventEmitter\|\.emit(" apps/server/src/routes/ -C 2Repository: AutoMaker-Org/automaker
Length of output: 199
🏁 Script executed:
# Get the complete resolve-pr-comment.ts file to see full context
cat -n apps/server/src/routes/github/routes/resolve-pr-comment.tsRepository: AutoMaker-Org/automaker
Length of output: 2492
🏁 Script executed:
# Search for event emission patterns in github routes (fixed regex)
rg "createEventEmitter|eventEmitter|\.emit" apps/server/src/routes/ -C 2Repository: AutoMaker-Org/automaker
Length of output: 30016
🏁 Script executed:
# Check what event patterns exist in other services to understand the pattern
rg "createEventEmitter\(" apps/server/src/ -B 2 -A 5 | head -60Repository: AutoMaker-Org/automaker
Length of output: 2931
Missing event emission for GitHub state mutation.
The route handler in apps/server/src/routes/github/routes/resolve-pr-comment.ts calls executeReviewThreadMutation() to mutate GitHub PR review thread state but does not emit any events. Per coding guidelines, all server operations that mutate state should emit events using the event emitter that streams to the frontend via WebSocket. Other GitHub routes in the codebase (e.g., validate-issue.ts) follow this pattern consistently.
Add event emission to the route handler after the mutation succeeds, or pass the event emitter to the service and emit from there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/github-pr-comment.service.ts` around lines 30 - 34,
The mutation function executeReviewThreadMutation currently changes GitHub PR
review thread state but no event is emitted; update the flow so an event is
emitted to the server event emitter after a successful mutation: either add an
EventEmitter parameter to executeReviewThreadMutation(projectPath, threadId,
resolve, eventEmitter) and emit a descriptive event (e.g.,
"github:reviewThread:updated" with payload { projectPath, threadId, isResolved
}) inside executeReviewThreadMutation, or keep the signature and emit from the
route handler in resolve-pr-comment.ts immediately after calling
executeReviewThreadMutation, using the same emitter/event name pattern used by
validate-issue.ts so the frontend receives the update via WebSocket.
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx
Show resolved
Hide resolved
Comment on lines
65
to
113
| const createFeature = useCreateFeature(currentProject?.path ?? ''); | ||
|
|
||
| const handleAutoAddressComments = useCallback( | ||
| async (pr: GitHubPR) => { | ||
| if (!pr.number || !currentProject?.path) { | ||
| toast.error('Cannot address PR comments', { | ||
| description: 'No PR number or project available.', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const featureId = `pr-${pr.number}-${generateUUID()}`; | ||
| const feature: Feature = { | ||
| id: featureId, | ||
| title: `Address PR #${pr.number} Review Comments`, | ||
| category: 'bug-fix', | ||
| description: `Read the review requests on PR #${pr.number} and address any feedback the best you can.`, | ||
| steps: [], | ||
| status: 'in_progress', | ||
| model: resolveModelString('opus'), | ||
| thinkingLevel: 'none', | ||
| planningMode: 'skip', | ||
| ...(pr.headRefName ? { branchName: pr.headRefName } : {}), | ||
| }; | ||
|
|
||
| try { | ||
| await createFeature.mutateAsync(feature); | ||
|
|
||
| // Start the feature immediately after creation | ||
| const api = getElectronAPI(); | ||
| if (api.features?.run) { | ||
| await api.features.run(currentProject.path, featureId); | ||
| toast.success('Feature created and started', { | ||
| description: `Addressing review comments on PR #${pr.number}`, | ||
| }); | ||
| } else { | ||
| toast.error('Cannot start feature', { | ||
| description: | ||
| 'Feature API is not available. The feature was created but could not be started.', | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| toast.error('Failed to create feature', { | ||
| description: error instanceof Error ? error.message : 'An error occurred', | ||
| }); | ||
| } | ||
| }, | ||
| [currentProject, createFeature] | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid leaving features stuck in in_progress when auto-start can’t run.
If api.features.run is unavailable or throws, the feature stays in_progress even though no agent is running. Consider creating it as backlog and promoting only after a successful start, or reverting the status on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/github-prs-view.tsx` around lines 65 - 113, The
feature is currently created with status 'in_progress' and can remain stuck if
api.features.run is unavailable or throws; change createFeature.mutateAsync to
create the feature with status 'backlog' (modify the local feature object’s
status before calling createFeature.mutateAsync), then attempt to start via
getElectronAPI().features.run(currentProject.path, featureId); only after a
successful run update the feature status to 'in_progress' (either via an API
call like api.features.update or a second createFeature.mutateAsync/update call
that sets feature.status = 'in_progress'); ensure any run errors are caught and
reported (toast.error) without leaving the feature marked as in_progress.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (17)
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx (1)
153-153: Tooltip Ctrl+S hint is misleading for a mobile-only button.The tooltip advertises a keyboard shortcut that doesn't apply on touch devices. Consider dropping the shortcut from the title or making it conditional on desktop.
✏️ Suggested change
- title="Save file (Ctrl+S)" + title="Save file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx` at line 153, The tooltip text "Save file (Ctrl+S)" in the Save button within EditorTabs is misleading on touch-only devices; update the save button's title prop in editor-tabs.tsx (the Save button JSX inside the EditorTabs component) to omit the "(Ctrl+S)" hint on mobile — either remove the shortcut from the static title or make it conditional using a platform or touch detection helper (e.g., isMobile/isTouch or navigator.maxTouchPoints) so the shortcut is shown only on desktop/keyboard-capable environments.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (1)
575-615: Copy/Move handlers are nearly identical — consider extracting a helper.The Copy To and Move To click handlers (lines 575–589 and 601–615) share the same structure: compute
parentPath, open file browser, and call the operation callback. A small shared helper would reduce duplication.♻️ Example extraction
+ const handleFileBrowserOperation = useCallback( + async ( + operationName: string, + operationFn: (source: string, dest: string) => Promise<void> + ) => { + try { + const parentPath = node.path.substring(0, node.path.lastIndexOf('/')) || '/'; + const destPath = await openFileBrowser({ + title: `${operationName} "${node.name}" To...`, + description: `Select the destination folder for the ${operationName.toLowerCase()} operation`, + initialPath: parentPath, + }); + if (destPath) { + await operationFn(node.path, destPath); + } + } catch (err) { + console.error(`${operationName} operation failed:`, err); + } + }, + [node.path, node.name, openFileBrowser] + );Then in the menu items:
- onClick={async (e) => { - e.stopPropagation(); - try { - const parentPath = node.path.substring(0, node.path.lastIndexOf('/')) || '/'; - const destPath = await openFileBrowser({ - title: `Copy "${node.name}" To...`, - ... - }); - ... - } catch (err) { ... } - }} + onClick={(e) => { + e.stopPropagation(); + handleFileBrowserOperation('Copy', onCopyItem!); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 575 - 615, Extract a small shared async helper (e.g., chooseDestinationAndRun) that accepts the node and an operation callback and performs the shared logic: compute parentPath from node.path (using substring/lastIndexOf with fallback '/'), call openFileBrowser with title/description/initialPath, and if a destPath is returned invoke the provided callback. Replace the inline handlers on the DropdownMenuItem for onCopyItem and onMoveItem to call this helper with the appropriate operation (onCopyItem or onMoveItem) and keep the try/catch around the helper call to log errors as before.apps/server/src/routes/worktree/routes/generate-commit-message.ts (4)
254-259: Nit:messageis already trimmed on line 242 — redundant.trim()calls.Lines 254 and 258 call
.trim()again on a value that was already trimmed.✏️ Suggested cleanup
- logger.info(`Generated commit message: ${message.trim().substring(0, 100)}...`); + logger.info(`Generated commit message: ${message.substring(0, 100)}...`); const response: GenerateCommitMessageSuccessResponse = { success: true, - message: message.trim(), + message, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 254 - 259, The variable message is already trimmed earlier, so remove the redundant .trim() calls where it is used later; update logger.info to log the generated message using message.substring(0, 100) (no .trim()) and set response.message to message (no .trim()) so GenerateCommitMessageSuccessResponse and logger.info reference the already-trimmed message value directly.
48-66:iterator.return()in the timeout handler has no timeout itself — cleanup could block indefinitely.If the AI provider stream is stuck (which is precisely when the timeout fires),
await iterator.return?.()on line 55 may also hang, defeating the purpose of the timeout. Consider wrapping the cleanup in its own short timeout:♻️ Suggested improvement
try { - await iterator.return?.(); + await Promise.race([ + iterator.return?.(), + new Promise((resolve) => setTimeout(resolve, 2_000)), + ]); } catch (teardownErr) { logger.warn('Error during iterator cleanup after timeout:', teardownErr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 48 - 66, Wrap the cleanup call await iterator.return?.() inside its own short timeout (e.g., a Promise.race with a small teardown timeout) so iterator.return cannot block indefinitely; if the teardown promise times out or rejects, log via logger.warn (preserving the original teardownErr variable name) and continue to rethrow the original error caught by the outer .catch handler that currently references err and timeoutPromise; update the catch block in the async loop around iterator.next() to use this bounded cleanup pattern and ensure the original err is rethrown after attempting (and possibly timing out) the iterator.return cleanup.
96-269: Route handler contains substantial business logic — consider extracting to a service.Per coding guidelines, route handlers should delegate to services. This handler directly orchestrates git diff retrieval, AI provider streaming, prompt composition, and response parsing. Consider extracting the core logic (lines ~136–240) into a service method (e.g.,
CommitMessageService.generate(worktreePath)) and keeping the handler thin. This is a pre-existing pattern, so deferring is fine. 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/generate-commit-message.ts` around lines 96 - 269, The handler createGenerateCommitMessageHandler contains heavy business logic (git diff retrieval, prompt composition, AI provider streaming and response parsing) — extract that core flow into a new service method like CommitMessageService.generate(worktreePath: string, settingsService?: SettingsService): Promise<GenerateCommitMessageSuccessResponse|GenerateCommitMessageErrorResponse>, moving code that uses execFileAsync/git diff, getPhaseModelWithOverrides, resolvePhaseModel, getSystemPrompt, ProviderFactory.getProviderForModel/stripProviderPrefix/isCursorModel, withTimeout/AI_TIMEOUT_MS and the response assembly into the service; then simplify the route to validate input/existence and call CommitMessageService.generate, returning its result and mapping errors/status codes, preserving existing helpers logError/getErrorMessage and maintaining same signatures and behavior.
234-239: Reasonable guard against result truncation, but note the assumption.The longest-content-wins heuristic assumes length correlates with completeness. This works well for the truncation bug described in the PR objectives. Be aware that if a provider ever returns a cleaned/deduplicated
resultthat is intentionally shorter than raw accumulated stream chunks, this would discard it. A comment documenting when/why this was needed would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 234 - 239, Add a short clarifying comment above the longest-content-wins heuristic in generate-commit-message.ts (the block comparing msg.result.length and responseText and assigning responseText = msg.result) explaining that this is a defensive workaround for a known streaming truncation bug (mirrors simpleQuery behavior), that it assumes longer text implies a more complete result, and noting the caveat that a provider could intentionally return a shorter, cleaned result which this heuristic would override; keep the heuristic as-is but document the rationale and the edge-case so future maintainers understand why it exists and when it might need revisiting.apps/ui/src/hooks/queries/use-features.ts (1)
191-193: Minor: redundant localStorage write fromqueryFn.The new cache subscription (lines 164–179) already catches every successful query update — including the one triggered by this
queryFn— and writes to localStorage. This explicitwritePersistedFeaturescall inqueryFnis therefore a double-write for the same data on every fetch.You could remove this line and let the subscription be the single path for persisting features, simplifying the write contract. Not urgent — the duplication is harmless.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/queries/use-features.ts` around lines 191 - 193, Remove the redundant localStorage write inside the query function: in use-features.ts, eliminate the explicit call to writePersistedFeatures from the queryFn (the block where result.features is processed and returned) and rely on the existing cache subscription (lines handling queryCache.subscribe/queryCache.onSuccess) to perform persistence; keep returning features from the queryFn as-is and ensure no other code path depends on that immediate write.apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx (2)
503-514:draggableon the entire row may interfere with input text selection.The
draggableattribute is on the container<div>(line 510), which wraps the<Input>fields. On some browsers, this can make text selection within the inputs inconsistent — attempting to click-and-drag to select text may inadvertently start a drag operation instead.A more robust approach is to make only the grip handle
draggableand use the drag events on the handle to drive reordering, or suppress drag initiation from inputs viaonDragStartguards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 503 - 514, The container div currently has draggable and drag event handlers which causes text-selection issues inside inputs; update the implementation so only the drag-grip (the UI element users grab to reorder) is draggable and wires handleDragStart, handleDragOver, handleDrop, handleDragEnd on that grip, or alternatively keep handlers on the row but guard onDragStart to ignore events originating from inputs/buttons (e.g., check event.target.tagName or class and return early). Locate the div using draggedIndex/dragOverIndex and the handlers handleDragStart, handleDragOver, handleDrop, handleDragEnd to make this change.
155-171: No user-facing feedback on save failure.The mutation's error state (
updateSettingsMutation.isError) is never checked or surfaced in the UI. If the save fails, the button silently re-enables and the user has no indication that their changes weren't persisted. Consider adding an error toast inonErroror displayingupdateSettingsMutation.isErrornear the save button.♻️ Minimal onError addition
updateSettingsMutation.mutate( { devCommand: normalizedDevCommand || null, testCommand: normalizedTestCommand || null, terminalScripts: normalizedScripts, }, { onSuccess: () => { setDevCommand(normalizedDevCommand); setOriginalDevCommand(normalizedDevCommand); setTestCommand(normalizedTestCommand); setOriginalTestCommand(normalizedTestCommand); setScripts(normalizedScripts); setOriginalScripts(structuredClone(normalizedScripts)); }, + onError: () => { + // Surface error via your toast/notification system + // e.g. toast.error('Failed to save settings'); + }, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 155 - 171, The save mutation currently only handles onSuccess and never surfaces failures; add an onError handler to the updateSettingsMutation.mutate call (or display updateSettingsMutation.isError near the save button) to surface failures to the user. In the mutate options, add onError: (error) => { show a user-facing toast or set a local error state } and ensure any optimistic UI changes are rolled back or the form remains editable (the same state setters like setDevCommand, setOriginalDevCommand, setTestCommand, setOriginalTestCommand, setScripts, setOriginalScripts should only be applied on success). Also use updateSettingsMutation.isError to conditionally render an inline error message by the save button if you prefer persistent UI feedback.apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx (2)
640-644:allSelectedcheck compares set size rather than set contents.
selectedIds.size === comments.lengthcan be true even when the IDs don't match the currentcommentslist (e.g., after a data refetch removes/changes a comment while stale IDs remain selected). This would show the "Deselect all" label incorrectly.A more robust check:
- const allSelected = comments.length > 0 && selectedIds.size === comments.length; + const allSelected = comments.length > 0 && comments.every((c) => selectedIds.has(c.id));The performance difference is negligible for the expected comment count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 640 - 644, The allSelected boolean currently uses selectedIds.size === comments.length which can be true even if the selectedIds refer to stale/other IDs; replace its calculation to verify that every current comment's id is present in selectedIds (e.g., comments.length > 0 && comments.every(c => selectedIds.has(c.id))). Update any downstream logic that relies on allSelected (symbols: allSelected, selectedIds, comments, someSelected, noneSelected) accordingly so the "Deselect all" label only appears when the current comments list is fully selected.
756-780: Sequential feature creation is fine for correctness but will be slow at scale.The
for...ofloop withawait createFeature.mutateAsync(...)creates features one at a time. If a user selects many comments in "individually" mode, this could feel sluggish.Promise.allSettledwould parallelize, but sequential is safer if the backend or store doesn't handle concurrent writes well — so this is a judgment call depending on your backend's behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 756 - 780, The current sequential loop over selectedComments awaiting createFeature.mutateAsync for each comment is slow at scale; replace it by mapping selectedComments to a list of promises (for each comment build the Feature using generateFeatureId, generateSingleCommentTitle, generateSingleCommentDescription and the same branchName logic) and run Promise.allSettled on that array, then iterate the results to increment successCount for fulfilled promises and push errors (associating the original comment) for rejected ones; keep using createFeature.mutateAsync for each item so behavior remains unchanged but parallelized, and ensure you preserve the existing error shape (err instanceof Error ? err.message : 'Unknown error').apps/ui/src/components/views/github-prs-view.tsx (1)
260-418:getReviewStatus(selectedPR)is called three times in a row without memoization.Lines 332, 336, and 340 all call
getReviewStatus(selectedPR)separately. While the function is lightweight, it's cleaner to call once and reuse:Suggested refactor
{(() => { + const status = getReviewStatus(selectedPR); + return status ? ( - {getReviewStatus(selectedPR) && ( - <span - className={cn( - 'px-2 py-0.5 rounded-full text-xs font-medium', - getReviewStatus(selectedPR)!.bg, - getReviewStatus(selectedPR)!.color - )} - > - {getReviewStatus(selectedPR)!.label} - </span> - )} + <span + className={cn( + 'px-2 py-0.5 rounded-full text-xs font-medium', + status.bg, + status.color + )} + > + {status.label} + </span> + ) : null; + })()}This eliminates the non-null assertions (
!) and the redundant calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-prs-view.tsx` around lines 260 - 418, getReviewStatus(selectedPR) is invoked multiple times in the JSX causing redundant calls and non-null assertions; capture its result once in a local variable (e.g., const reviewStatus = getReviewStatus(selectedPR)) near where selectedPR is used, then replace the three calls (and the getReviewStatus(selectedPR)! usages) with reviewStatus and guard rendering with reviewStatus truthiness (e.g., {reviewStatus && (...)}) so you reuse the computed value and remove the non-null assertions.apps/server/src/services/pr-review-comments.service.ts (2)
325-333: Redundant guard and defensive optional chain.
- Line 325:
if (resolvedMap.size > 0)is unnecessary — iterating an emptyMapis a no-op, and removing the guard reduces nesting.- Line 329: after confirming
resolvedMap.has(comment.id)on line 327,resolvedMap.get()is guaranteed non-undefined;info?.isResolved ?? falseshould beinfo!.isResolved(or an explicit non-null assertion pattern).♻️ Proposed simplification
- if (resolvedMap.size > 0) { - for (const comment of allComments) { - if (comment.isReviewComment && resolvedMap.has(comment.id)) { - const info = resolvedMap.get(comment.id); - comment.isResolved = info?.isResolved ?? false; - comment.threadId = info?.threadId; - } - } - } + for (const comment of allComments) { + if (comment.isReviewComment) { + const info = resolvedMap.get(comment.id); + if (info) { + comment.isResolved = info.isResolved; + comment.threadId = info.threadId; + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-review-comments.service.ts` around lines 325 - 333, Remove the unnecessary outer guard and the defensive optional chaining: iterate allComments unconditionally and, inside the loop where you already check resolvedMap.has(comment.id), call const info = resolvedMap.get(comment.id) and use non-null access (e.g., info!.isResolved and info!.threadId) to assign comment.isResolved and comment.threadId; this removes the redundant resolvedMap.size > 0 check and the unnecessary info? optional chains.
100-338: No WebSocket event emission — violates server operations guideline.Neither
fetchReviewThreadResolvedStatusnorfetchPRReviewCommentsemits any events viacreateEventEmitter(). Given that the GraphQL andghCLI calls can be slow (30 s timeout), surfacing progress to the frontend via WebSocket would also improve perceived performance.As per coding guidelines: "All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-review-comments.service.ts` around lines 100 - 338, Import and use createEventEmitter() from lib/events.ts in both fetchReviewThreadResolvedStatus and fetchPRReviewComments, creating an emitter at the start of each function and emitting progress events (e.g., "start", "graphql:started", "graphql:finished", "comments:started", "comments:finished", "completed", and "error") at key steps: before/after spawning the gh graphql process in fetchReviewThreadResolvedStatus (and when pageInfo.hasNextPage warnings occur), and before/after execFileAsync calls for regular comments and inline review comments in fetchPRReviewComments; include contextual metadata (owner, repo, prNumber, threadId or comment id when relevant) in each event, ensure emits are best-effort (don’t throw if emitter fails), and emit a final "completed" or "failed" event before returning so the frontend can stream progress.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
58-58: Prefer barrel export over deep direct import for intra-app type.
TerminalScriptis imported via its implementation file path rather than through the component barrel index, which makes this import brittle to file renames/moves.-import type { TerminalScript } from '@/components/views/project-settings-view/terminal-scripts-constants'; +import type { TerminalScript } from '@/components/views/project-settings-view';Based on learnings from PR
#656: "when importing UI components within the same app, prefer barrel exports from the components index barrel rather than direct path imports."🤖 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` at line 58, The import of the TerminalScript type in worktree-actions-dropdown.tsx uses a deep path to the implementation; replace it with the barrel export from the components index so the type is imported from the components' public barrel export (use the existing components index export for TerminalScript instead of the direct path), updating the import statement that references TerminalScript to point at the component barrel export name.apps/ui/src/components/views/terminal-view.tsx (2)
2067-2072: InlinerunCommandOnConnectduplicates the extracted logic inrenderPanelContent.The ternary at lines 2067-2072 mirrors the
runCommandcomputation at lines 1461-1463. Extracting a shared helper avoids drift if override priority rules change.♻️ Proposed refactor
+ // Shared helper — per-session override takes priority over defaultRunScript + const getRunCommandForSession = (sessionId: string): string | undefined => { + if (!newSessionIds.has(sessionId)) return undefined; + return sessionCommandOverrides.get(sessionId) || defaultRunScript; + };Then in
renderPanelContent(lines 1461-1463):- const isNewSession = newSessionIds.has(content.sessionId); - const sessionCommand = sessionCommandOverrides.get(content.sessionId); - const runCommand = isNewSession ? sessionCommand || defaultRunScript : undefined; + const runCommand = getRunCommandForSession(content.sessionId);And for the maximized terminal (lines 2067-2072):
- runCommandOnConnect={ - newSessionIds.has(terminalState.maximizedSessionId) - ? sessionCommandOverrides.get(terminalState.maximizedSessionId) || - defaultRunScript - : undefined - } + runCommandOnConnect={getRunCommandForSession(terminalState.maximizedSessionId!)}🤖 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 2067 - 2072, The inline ternary used for runCommandOnConnect duplicates the runCommand computation in renderPanelContent; extract a shared helper (e.g., getRunCommandForSession(sessionId: string | undefined): string | undefined) that encapsulates the priority logic using newSessionIds, terminalState.maximizedSessionId, sessionCommandOverrides and defaultRunScript, and replace both the computation in renderPanelContent and the runCommandOnConnect prop with calls to this helper so the override priority stays in one place.
1152-1200: Consolidate duplicated cleanup into afinallyblock.
removeTerminalFromLayout,setSessionCommandOverrides, andsetNewSessionIdsare called identically in both thetryandcatchbranches — a maintenance hazard since a future change to one copy may miss the other.fetchServerSettingsstays intryonly (correct — no count refresh on network error).♻️ Proposed refactor
const killTerminal = async (sessionId: string) => { try { const headers: Record<string, string> = {}; if (terminalState.authToken) { headers['X-Terminal-Token'] = terminalState.authToken; } const response = await apiDeleteRaw(`/api/terminal/sessions/${sessionId}`, { headers }); - // Always remove from UI - even if server says 404 (session may have already exited) - removeTerminalFromLayout(sessionId); - - // Clean up stale entries for killed sessions - setSessionCommandOverrides((prev) => { - const next = new Map(prev); - next.delete(sessionId); - return next; - }); - setNewSessionIds((prev) => { - const next = new Set(prev); - next.delete(sessionId); - return next; - }); - if (!response.ok && response.status !== 404) { const data = await response.json().catch(() => ({})); logger.error('Server failed to kill session:', data.error || response.statusText); } - - // Refresh session count fetchServerSettings(); } catch (err) { logger.error('Kill session error:', err); - // Still remove from UI on network error - better UX than leaving broken terminal - removeTerminalFromLayout(sessionId); - // Clean up stale entries for killed sessions (same cleanup as try block) - setSessionCommandOverrides((prev) => { - const next = new Map(prev); - next.delete(sessionId); - return next; - }); - setNewSessionIds((prev) => { - const next = new Set(prev); - next.delete(sessionId); - return next; - }); + } finally { + // Always remove from UI and clean up state regardless of server response + removeTerminalFromLayout(sessionId); + setSessionCommandOverrides((prev) => { + const next = new Map(prev); + next.delete(sessionId); + return next; + }); + setNewSessionIds((prev) => { + const next = new Set(prev); + next.delete(sessionId); + return next; + }); } };🤖 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 1152 - 1200, The cleanup calls removeTerminalFromLayout, setSessionCommandOverrides, and setNewSessionIds are duplicated in try and catch; move these identical cleanup steps into a finally block inside killTerminal to run unconditionally, removing them from both try and catch, while keeping fetchServerSettings() inside the try (so it only runs on successful request handling) and preserving existing error logging in both branches (logger.error, response handling, etc.); update killTerminal to call removeTerminalFromLayout(sessionId), setSessionCommandOverrides(...) and setNewSessionIds(...) only once in finally and remove their copies from try/catch.
🤖 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/pr-review-comments.service.ts`:
- Around line 232-240: The execFileAsync call that fetches regular PR comments
(the one assigning stdout to commentsOutput) is missing a maxBuffer option and
can exceed the 1MB default; update that execFileAsync invocation to include
maxBuffer: 1024 * 1024 * 10 (same as the inline-comments call), while keeping
the existing cwd: projectPath, env: execEnv, and timeout: GITHUB_API_TIMEOUT_MS
so stdout won't overflow for large PRs; locate the call by the
variables/commentsOutput, execFileAsync, prNumber, owner, and repo to modify the
options object.
- Around line 9-12: The service imports execEnv and logError from the routes
layer, inverting the dependency; move these utilities into a shared server lib
(e.g., create lib/exec-utils.ts or a shared `@automaker/`* package) and export
execEnv and logError from there, then update pr-review-comments.service.ts to
import execEnv and logError from that new lib path (and update
routes/github/routes/common.ts to re-export or import from the lib so route
consumers remain unchanged); ensure function names stay the same so references
inside PrReviewCommentsService (and any functions/methods that call
execEnv/logError) need only their import path changed.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 569-570: The per-thread spinner logic should track multiple
in-flight thread IDs instead of a single resolvingThreadId: change
resolvingThreadId state to a Set (e.g., resolvingThreads) and replace
setResolvingThreadId calls with functional updates that add the thread id when a
resolve/unresolve starts and remove that id in onSettled; update any checks
(previously comparing resolvingThreadId === threadId) to
resolvingThreads.has(threadId) so a request finishing only removes its own id
and does not clear other in-flight indicators; use the existing onSettled
handler name and the same resolve/unresolve start handlers but switch their
adds/removes to operate on the Set via the setter to avoid races.
In `@apps/ui/src/components/ui/app-error-boundary.tsx`:
- Around line 95-109: The reload button's decorative SVG in the AppErrorBoundary
component is missing aria-hidden="true" so screen readers may read path data;
update the SVG element inside the reload button (the small icon with
className="h-4 w-4" used for "Reload Page") to include aria-hidden="true"
(matching the logo SVG pattern) so the icon is ignored by assistive tech.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 374-383: The "Open in Browser" item can render with an undefined
port; tighten the render guard so the menu item only appears when devServerInfo
exists and has a valid port. Update the conditional that renders
DropdownMenuItem (referencing devServerInfo, devServerInfo.port, and
onOpenDevServerUrl) to check devServerInfo is non-null and devServerInfo.port is
not undefined/null (e.g., devServerInfo != null && devServerInfo.port != null &&
devServerInfo.urlDetected !== false), and ensure the aria-label uses that
confirmed port value.
- Around line 947-1000: The chevron sub-trigger (DropdownMenuSubTrigger) is
shown even when the submenu would be empty or only duplicate the primary action
(case: !worktree.hasChanges && onViewStashes); change the render logic so you
only render the DropdownMenuSub wrapper (DropdownMenuSub,
DropdownMenuSubTrigger, DropdownMenuSubContent) when the submenu actually
provides non-duplicate actions (e.g., worktree.hasChanges || onStashChanges
present), and otherwise render a single plain DropdownMenuItem for the stashes
action (use onViewStashes directly) so the chevron is hidden; update the branch
that currently checks (worktree.hasChanges || onViewStashes) to instead detect a
non-trivial submenu and fall back to the plain DropdownMenuItem when
appropriate.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 156-169: handleSubmit currently returns silently when
isValidFileName(trimmed) is false, providing no user feedback; update the
component to surface an error state when validation fails (e.g., add a local
state variable like invalidName or errorMessage and a setter used by
handleSubmit) and set that state instead of silently returning, then ensure the
JSX for this component uses that state to render a visual cue (red border,
inline error text or tooltip) and optionally trigger a brief animation or focus
on the input; keep submittedRef, onSubmit, onCancel and value logic unchanged
except to set and clear the new error state (clear it on successful submit, on
change of value, or onCancel).
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 175-239: The handler handleAddFeatureFromIssue currently drops
attachments because the inline featureData type and the constructed feature
object omit imagePaths and textFilePaths; update the featureData usage (or its
inline type) to include imagePaths: DescriptionImagePath[] and textFilePaths:
DescriptionTextFilePath[] and forward those fields into the created feature
object before calling api.features.create (or, if you prefer the quick runtime
fix, read featureData.imagePaths and featureData.textFilePaths directly and
include them in the feature passed to api.features.create).
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 98-131: The useEffect tied to projectSettings currently overwrites
local edits on every background refetch; change it to only sync settings on
initial load for the current project or when there are no unsaved edits: add an
isInitializedRef (reset when prevProjectPathRef changes) and guard the sync so
it runs if isInitializedRef is false or if the local state is not dirty (compare
current devCommand/testCommand/scripts to
originalDevCommand/originalTestCommand/originalScripts), otherwise skip
overwriting; update prevProjectPathRef and set isInitializedRef=true after the
initial sync and reset isInitializedRef on project switch so functions/state
like useEffect, prevProjectPathRef, projectSettings, setDevCommand,
setOriginalDevCommand, setTestCommand, setOriginalTestCommand, setScripts, and
setOriginalScripts are only applied when appropriate.
---
Duplicate comments:
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 712-813: The feature objects created in handleSubmit include an
unused steps: [] property that isn't part of the Feature type; remove the steps:
[] entry from both feature definitions (the one in the addressMode ===
'together' branch and the one inside the per-comment loop) so
createFeature.mutateAsync is passed only valid Feature fields (id, title,
category, description, status, model, thinkingLevel, reasoningEffort, and
optional branchName).
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 596-608: The disabled check on the "Re-run Init Script"
DropdownMenuItem incorrectly includes a dead guard (!onRunInitScript) because
onRunInitScript is non-optional; remove the redundant part and use only the
meaningful condition (e.g., disabled={!hasInitScript}) so the button state
reflects whether an init script exists; update the DropdownMenuItem in the
worktree-actions-dropdown component (the element using worktree,
onRunInitScript, and hasInitScript) accordingly.
- Around line 1039-1101: The JSX currently creates prInfo inside an IIFE—remove
that IIFE and instead compute prInfo as a local variable (preferably via
useMemo) when showPRInfo and worktree.pr are truthy, eliminate the redundant
`worktree.pr &&` guard, and pass that prInfo to the existing handlers
(onAddressPRComments, onAutoAddressPRComments) and the DropdownMenuSub children;
locate references to prInfo, worktree.pr, showPRInfo, onAddressPRComments, and
onAutoAddressPRComments in the WorktreeActionsDropdown component and replace the
inline IIFE with the memoized/local prInfo binding before the JSX return.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 143-157: The accessibility issue is already fixed by adding
aria-label="Save file" on the Save button in the EditorTabs component, so no
code change is required; keep the button with onClick={onSave},
disabled={!isDirty}, title="Save file (Ctrl+S)" and aria-label="Save file" in
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx and
mark the change approved, and remove any duplicate/obsolete review comments
referencing this accessibility concern.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 762-771: The onDragOver handler handleRootDragOver currently
always calls setDragState with a new object even when dragState.dropTargetPath
already equals effectivePath; add a guard at the top of handleRootDragOver
(inside the useCallback) to compare dragState.dropTargetPath to effectivePath
and skip calling setDragState when they are equal, preserving the existing
dropTargetPath and avoiding redundant state updates while keeping
e.preventDefault() and setting e.dataTransfer.dropEffect.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 65-122: The feature object created in handleAutoAddressComments
includes a non-existent/dead property steps: [] which is not defined on the
Feature interface; remove the steps key from the feature literal (the block
where `const feature: Feature = { ... }`) so the created object only contains
valid properties (id, title, category, description, status, model,
thinkingLevel, planningMode, and optional branchName) before calling
createFeature.mutateAsync; update any related references if other code expects
steps to avoid unused property warnings.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts`:
- Around line 254-259: The variable message is already trimmed earlier, so
remove the redundant .trim() calls where it is used later; update logger.info to
log the generated message using message.substring(0, 100) (no .trim()) and set
response.message to message (no .trim()) so GenerateCommitMessageSuccessResponse
and logger.info reference the already-trimmed message value directly.
- Around line 48-66: Wrap the cleanup call await iterator.return?.() inside its
own short timeout (e.g., a Promise.race with a small teardown timeout) so
iterator.return cannot block indefinitely; if the teardown promise times out or
rejects, log via logger.warn (preserving the original teardownErr variable name)
and continue to rethrow the original error caught by the outer .catch handler
that currently references err and timeoutPromise; update the catch block in the
async loop around iterator.next() to use this bounded cleanup pattern and ensure
the original err is rethrown after attempting (and possibly timing out) the
iterator.return cleanup.
- Around line 96-269: The handler createGenerateCommitMessageHandler contains
heavy business logic (git diff retrieval, prompt composition, AI provider
streaming and response parsing) — extract that core flow into a new service
method like CommitMessageService.generate(worktreePath: string,
settingsService?: SettingsService):
Promise<GenerateCommitMessageSuccessResponse|GenerateCommitMessageErrorResponse>,
moving code that uses execFileAsync/git diff, getPhaseModelWithOverrides,
resolvePhaseModel, getSystemPrompt,
ProviderFactory.getProviderForModel/stripProviderPrefix/isCursorModel,
withTimeout/AI_TIMEOUT_MS and the response assembly into the service; then
simplify the route to validate input/existence and call
CommitMessageService.generate, returning its result and mapping errors/status
codes, preserving existing helpers logError/getErrorMessage and maintaining same
signatures and behavior.
- Around line 234-239: Add a short clarifying comment above the
longest-content-wins heuristic in generate-commit-message.ts (the block
comparing msg.result.length and responseText and assigning responseText =
msg.result) explaining that this is a defensive workaround for a known streaming
truncation bug (mirrors simpleQuery behavior), that it assumes longer text
implies a more complete result, and noting the caveat that a provider could
intentionally return a shorter, cleaned result which this heuristic would
override; keep the heuristic as-is but document the rationale and the edge-case
so future maintainers understand why it exists and when it might need
revisiting.
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 325-333: Remove the unnecessary outer guard and the defensive
optional chaining: iterate allComments unconditionally and, inside the loop
where you already check resolvedMap.has(comment.id), call const info =
resolvedMap.get(comment.id) and use non-null access (e.g., info!.isResolved and
info!.threadId) to assign comment.isResolved and comment.threadId; this removes
the redundant resolvedMap.size > 0 check and the unnecessary info? optional
chains.
- Around line 100-338: Import and use createEventEmitter() from lib/events.ts in
both fetchReviewThreadResolvedStatus and fetchPRReviewComments, creating an
emitter at the start of each function and emitting progress events (e.g.,
"start", "graphql:started", "graphql:finished", "comments:started",
"comments:finished", "completed", and "error") at key steps: before/after
spawning the gh graphql process in fetchReviewThreadResolvedStatus (and when
pageInfo.hasNextPage warnings occur), and before/after execFileAsync calls for
regular comments and inline review comments in fetchPRReviewComments; include
contextual metadata (owner, repo, prNumber, threadId or comment id when
relevant) in each event, ensure emits are best-effort (don’t throw if emitter
fails), and emit a final "completed" or "failed" event before returning so the
frontend can stream progress.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 640-644: The allSelected boolean currently uses selectedIds.size
=== comments.length which can be true even if the selectedIds refer to
stale/other IDs; replace its calculation to verify that every current comment's
id is present in selectedIds (e.g., comments.length > 0 && comments.every(c =>
selectedIds.has(c.id))). Update any downstream logic that relies on allSelected
(symbols: allSelected, selectedIds, comments, someSelected, noneSelected)
accordingly so the "Deselect all" label only appears when the current comments
list is fully selected.
- Around line 756-780: The current sequential loop over selectedComments
awaiting createFeature.mutateAsync for each comment is slow at scale; replace it
by mapping selectedComments to a list of promises (for each comment build the
Feature using generateFeatureId, generateSingleCommentTitle,
generateSingleCommentDescription and the same branchName logic) and run
Promise.allSettled on that array, then iterate the results to increment
successCount for fulfilled promises and push errors (associating the original
comment) for rejected ones; keep using createFeature.mutateAsync for each item
so behavior remains unchanged but parallelized, and ensure you preserve the
existing error shape (err instanceof Error ? err.message : 'Unknown error').
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Line 58: The import of the TerminalScript type in
worktree-actions-dropdown.tsx uses a deep path to the implementation; replace it
with the barrel export from the components index so the type is imported from
the components' public barrel export (use the existing components index export
for TerminalScript instead of the direct path), updating the import statement
that references TerminalScript to point at the component barrel export name.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Line 153: The tooltip text "Save file (Ctrl+S)" in the Save button within
EditorTabs is misleading on touch-only devices; update the save button's title
prop in editor-tabs.tsx (the Save button JSX inside the EditorTabs component) to
omit the "(Ctrl+S)" hint on mobile — either remove the shortcut from the static
title or make it conditional using a platform or touch detection helper (e.g.,
isMobile/isTouch or navigator.maxTouchPoints) so the shortcut is shown only on
desktop/keyboard-capable environments.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 575-615: Extract a small shared async helper (e.g.,
chooseDestinationAndRun) that accepts the node and an operation callback and
performs the shared logic: compute parentPath from node.path (using
substring/lastIndexOf with fallback '/'), call openFileBrowser with
title/description/initialPath, and if a destPath is returned invoke the provided
callback. Replace the inline handlers on the DropdownMenuItem for onCopyItem and
onMoveItem to call this helper with the appropriate operation (onCopyItem or
onMoveItem) and keep the try/catch around the helper call to log errors as
before.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 260-418: getReviewStatus(selectedPR) is invoked multiple times in
the JSX causing redundant calls and non-null assertions; capture its result once
in a local variable (e.g., const reviewStatus = getReviewStatus(selectedPR))
near where selectedPR is used, then replace the three calls (and the
getReviewStatus(selectedPR)! usages) with reviewStatus and guard rendering with
reviewStatus truthiness (e.g., {reviewStatus && (...)}) so you reuse the
computed value and remove the non-null assertions.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 503-514: The container div currently has draggable and drag event
handlers which causes text-selection issues inside inputs; update the
implementation so only the drag-grip (the UI element users grab to reorder) is
draggable and wires handleDragStart, handleDragOver, handleDrop, handleDragEnd
on that grip, or alternatively keep handlers on the row but guard onDragStart to
ignore events originating from inputs/buttons (e.g., check event.target.tagName
or class and return early). Locate the div using draggedIndex/dragOverIndex and
the handlers handleDragStart, handleDragOver, handleDrop, handleDragEnd to make
this change.
- Around line 155-171: The save mutation currently only handles onSuccess and
never surfaces failures; add an onError handler to the
updateSettingsMutation.mutate call (or display updateSettingsMutation.isError
near the save button) to surface failures to the user. In the mutate options,
add onError: (error) => { show a user-facing toast or set a local error state }
and ensure any optimistic UI changes are rolled back or the form remains
editable (the same state setters like setDevCommand, setOriginalDevCommand,
setTestCommand, setOriginalTestCommand, setScripts, setOriginalScripts should
only be applied on success). Also use updateSettingsMutation.isError to
conditionally render an inline error message by the save button if you prefer
persistent UI feedback.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 2067-2072: The inline ternary used for runCommandOnConnect
duplicates the runCommand computation in renderPanelContent; extract a shared
helper (e.g., getRunCommandForSession(sessionId: string | undefined): string |
undefined) that encapsulates the priority logic using newSessionIds,
terminalState.maximizedSessionId, sessionCommandOverrides and defaultRunScript,
and replace both the computation in renderPanelContent and the
runCommandOnConnect prop with calls to this helper so the override priority
stays in one place.
- Around line 1152-1200: The cleanup calls removeTerminalFromLayout,
setSessionCommandOverrides, and setNewSessionIds are duplicated in try and
catch; move these identical cleanup steps into a finally block inside
killTerminal to run unconditionally, removing them from both try and catch,
while keeping fetchServerSettings() inside the try (so it only runs on
successful request handling) and preserving existing error logging in both
branches (logger.error, response handling, etc.); update killTerminal to call
removeTerminalFromLayout(sessionId), setSessionCommandOverrides(...) and
setNewSessionIds(...) only once in finally and remove their copies from
try/catch.
In `@apps/ui/src/hooks/queries/use-features.ts`:
- Around line 191-193: Remove the redundant localStorage write inside the query
function: in use-features.ts, eliminate the explicit call to
writePersistedFeatures from the queryFn (the block where result.features is
processed and returned) and rely on the existing cache subscription (lines
handling queryCache.subscribe/queryCache.onSuccess) to perform persistence; keep
returning features from the queryFn as-is and ensure no other code path depends
on that immediate write.
apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx (1)
147-152:cursor-not-allowedon a native<button disabled>is silently ignored.Disabled
<button>elements havepointer-events: nonein Chromium and WebKit, so the cursor style is never painted. Use Tailwind'sdisabled:cursor-not-allowedvariant instead (which targets the element while still letting:disabledapply pointer-events through its own cascade layer).Since this is described as a mobile-only button, touch-device cursor feedback is moot, so this is purely a nitpick for correctness.
♻️ Proposed fix
className={cn( 'p-1 rounded transition-colors', isDirty ? 'text-primary hover:text-primary hover:bg-muted/50' - : 'text-muted-foreground/40 cursor-not-allowed' + : 'text-muted-foreground/40 disabled:cursor-not-allowed' )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx` around lines 147 - 152, The rendered button uses a plain "cursor-not-allowed" class which is ignored for native disabled buttons; update the class switch in editor-tabs.tsx (the JSX that builds the className around isDirty) to use Tailwind's disabled variant instead — replace the non-disabled cursor class with "disabled:cursor-not-allowed" in the branch that represents the disabled state and ensure the button element uses the disabled attribute when isDirty is false so the disabled: variant can take effect.apps/server/src/services/pr-review-comments.service.ts (1)
228-321: Regular and inline comment fetches are sequential; parallelise them to halve worst-case latency.
resolvedStatusPromisealready runs in parallel, but the twoexecFileAsynccalls (lines 232 and 272) are awaited in series. Since they are fully independent, they can be fetched concurrently withPromise.allSettled.♻️ Proposed refactor
- // Fetch review thread resolved status in parallel with comment fetching - const resolvedStatusPromise = fetchReviewThreadResolvedStatus(projectPath, owner, repo, prNumber); - - // 1. Fetch regular PR comments (issue-level comments) - try { - const { stdout: commentsOutput } = await execFileAsync( ... ); - ... - allComments.push(...regularComments); - } catch (error) { - logError(error, 'Failed to fetch regular PR comments'); - } - - // 2. Fetch inline review comments (code-level comments with file/line info) - try { - const { stdout: reviewsOutput } = await execFileAsync( ... ); - ... - allComments.push(...reviewComments); - } catch (error) { - logError(error, 'Failed to fetch inline review comments'); - } - - // Wait for resolved status and apply to inline review comments - const resolvedMap = await resolvedStatusPromise; + // Run all three I/O calls concurrently + const [regularResult, inlineResult, resolvedMap] = await Promise.all([ + fetchRegularComments(projectPath, owner, repo, prNumber), + fetchInlineComments(projectPath, owner, repo, prNumber), + fetchReviewThreadResolvedStatus(projectPath, owner, repo, prNumber), + ]); + allComments.push(...regularResult, ...inlineResult);Extract the current try/catch bodies into small private helper functions (
fetchRegularComments,fetchInlineComments) that returnPRReviewComment[]and internally swallow errors withlogError, matching the existing best-effort semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pr-review-comments.service.ts` around lines 228 - 321, Parallelise the two independent GH calls by extracting their try/catch bodies into two small helpers named fetchRegularComments and fetchInlineComments that each return PRReviewComment[] and internally catch/log errors via logError (preserving the current best-effort semantics and using execFileAsync and JSON.parse as before); then call them concurrently (alongside resolvedStatusPromise if needed) with Promise.allSettled, merge the settled results into allComments, and ensure existing fields (isReviewComment, isOutdated, isResolved, avatarUrl, diffHunk, commitId, etc.) are preserved exactly as in the original mapping.apps/ui/src/components/views/terminal-view.tsx (1)
1152-1200: Deduplicate the state cleanup with afinallyblock.
removeTerminalFromLayout,setSessionCommandOverrides, andsetNewSessionIdsare copy-pasted identically in both thetryandcatchbranches. OnlyfetchServerSettings()and theresponse.okguard belong exclusively in the success path.♻️ Proposed refactor
const killTerminal = async (sessionId: string) => { try { const headers: Record<string, string> = {}; if (terminalState.authToken) { headers['X-Terminal-Token'] = terminalState.authToken; } const response = await apiDeleteRaw(`/api/terminal/sessions/${sessionId}`, { headers }); - // Always remove from UI - even if server says 404 (session may have already exited) - removeTerminalFromLayout(sessionId); - - // Clean up stale entries for killed sessions - setSessionCommandOverrides((prev) => { - const next = new Map(prev); - next.delete(sessionId); - return next; - }); - setNewSessionIds((prev) => { - const next = new Set(prev); - next.delete(sessionId); - return next; - }); - if (!response.ok && response.status !== 404) { - // Log non-404 errors but still proceed with UI cleanup const data = await response.json().catch(() => ({})); logger.error('Server failed to kill session:', data.error || response.statusText); } - // Refresh session count fetchServerSettings(); } catch (err) { logger.error('Kill session error:', err); - // Still remove from UI on network error - better UX than leaving broken terminal - removeTerminalFromLayout(sessionId); - // Clean up stale entries for killed sessions (same cleanup as try block) - setSessionCommandOverrides((prev) => { - const next = new Map(prev); - next.delete(sessionId); - return next; - }); - setNewSessionIds((prev) => { - const next = new Set(prev); - next.delete(sessionId); - return next; - }); + } finally { + // Always remove from UI and clean up state, even on network error + removeTerminalFromLayout(sessionId); + setSessionCommandOverrides((prev) => { + const next = new Map(prev); + next.delete(sessionId); + return next; + }); + setNewSessionIds((prev) => { + const next = new Set(prev); + next.delete(sessionId); + return next; + }); } };🤖 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 1152 - 1200, The duplicated UI/state cleanup in killTerminal should be moved into a finally block: keep the API call and response handling (including the response.ok check and fetchServerSettings()) in the try block, handle errors/logging in the catch block, and perform removeTerminalFromLayout(sessionId), setSessionCommandOverrides(... delete sessionId ...), and setNewSessionIds(... delete sessionId ...) once in a finally block so cleanup always runs; reference the killTerminal function and the existing helpers removeTerminalFromLayout, setSessionCommandOverrides, setNewSessionIds, and fetchServerSettings when making the change.apps/server/src/routes/worktree/routes/generate-commit-message.ts (2)
48-69: Underlying iterator not closed infinallyon non-timeout early exit
iterator.return?.()is only invoked in the.catchhandler (the timeout path). If the outerfor await...ofexits early for any other reason (exception thrown from the loop body, futurebreak, cancellation), the generator'sfinallyblock fires and clears the timer — but the underlying AI-streamiteratoris left open, leaking the connection.Adding an explicit
return()call tofinally(guarded by!doneto skip the normal-completion case) closes the iterator regardless of the exit path:♻️ Proposed fix
} finally { clearTimeout(timerId); + if (!done) { + try { + await iterator.return?.(); + } catch { + // ignore teardown errors + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts` around lines 48 - 69, The finally block currently only clears timerId and doesn't close the underlying AI-stream iterator on non-timeout early exits; update the finally block to, when !done, call and await iterator.return?.() to ensure the iterator is closed on any early exit (e.g., exceptions or break), catching and logging any teardown error (logger.warn) but not rethrowing so cleanup proceeds, and keep the existing clearTimeout(timerId) behavior; reference iterator.return, done, timerId, and the existing timeoutPromise/Promise.race flow when making this change.
76-80:getSystemPromptbelongs in a service, not the route fileThis newly added function contains business logic (fetching settings, merging prompts) that should live in the
services/directory per the project's server-layer organization guidelines. The route handler should delegate to it just as it already delegates toSettingsService.♻️ Suggested placement
Move to e.g.
apps/server/src/services/commit-message.service.tsand expose it from there, then import it in the route handler.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/generate-commit-message.ts` around lines 76 - 80, The helper getSystemPrompt now contains business logic and should be moved out of the route into a service; create a new service file (e.g., commit-message.service.ts) that exports an async function (e.g., getSystemPrompt) which accepts a SettingsService, calls settingsService.getGlobalSettings(), invokes mergeCommitMessagePrompts(settings?.promptCustomization?.commitMessage) and returns prompts.systemPrompt, then update the route to import and call that service function instead of defining it inline; ensure the service is exported and the route delegates to it, keeping SettingsService usage unchanged.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (1)
575-590: Copy/Move failures are only logged; no user-facing error is surfaced.Both the "Copy To…" and "Move To…" paths swallow errors into
console.error, so ifonCopyItem/onMoveItemrejects the user sees nothing. A brief toast or inline error would make failures observable.Also applies to: 601-616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 575 - 590, The onClick handlers in file-tree.tsx that call openFileBrowser and then await onCopyItem/onMoveItem currently catch errors and only call console.error, leaving users unaware of failures; update these catch blocks to surface failures via the app's user notification system (e.g., calling the existing toast/notification helper or dispatching an error banner) and include the error message/context (e.g., `Copy "name" failed: ${err.message}`) so the user sees a clear message; ensure you update both the onCopy path (handler referencing openFileBrowser and onCopyItem) and the onMove path (handler referencing onMoveItem) to use the same notification API and keep console.error for debug logging if desired.apps/ui/src/hooks/queries/use-features.ts (2)
174-175: Double write to localStorage on every successfulqueryFnexecution.
writePersistedFeaturesis called both inqueryFn(line 192) and here in the subscription callback. ThequeryFnwrite is a useful safety net that covers the race between the first fetch completing and theuseEffectsubscription being registered. The subscription is the right home for subsequent writes (optimistic,setQueryData, etc.). The overlap on ordinary fetches triggers two serializations and twoevictStaleFeaturesCache()scans per call.The simplest way to eliminate the redundant scan is to remove the call from
queryFnonce the subscription is considered reliable for all post-mount paths:♻️ Proposed refactor (remove redundant write from `queryFn`)
const features = (result.features ?? []) as Feature[]; - writePersistedFeatures(projectPath, features); return features;If the initial-render race window is a real concern (fetch resolves before
useEffectruns), keep thequeryFnwrite and document that the subscription write is intentionally redundant for that narrow window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/queries/use-features.ts` around lines 174 - 175, Remove the redundant localStorage write by deleting the call to writePersistedFeatures(...) inside the queryFn implementation and keep the single write in the useEffect subscription callback (which already writes when features and projectPathRef.current are present); if you are worried about the initial-render race where the fetch resolves before the subscription registers, either leave a short comment in the queryFn explaining the tradeoff or retain the queryFn write but document it as a deliberate safety net—ensure all references to writePersistedFeatures are consistent and only invoked from the subscription path (or are explicitly documented if left in queryFn).
164-180: UsingJSON.stringifyfor hash comparison is brittle; use TanStack Query'shashKeydirectly for consistency and future-proofing.The current code works correctly because
queryKeys.features.all(projectPath)returns a pure-primitive array['features', projectPath], whereJSON.stringifyand TanStack Query's defaulthashKeyfunction produce identical output. However, if the query key structure changes to include objects (asqueryKeys.sessions.alldoes with{ includeArchived }), the two serializations could diverge and silently break the subscription filter.No custom
queryHashFnis currently configured, so there's no immediate risk today. But for robustness and explicit parity with TanStack Query internals, derive the hash directly:Recommended fix
-import { useQuery, useQueryClient } from '@tanstack/react-query'; +import { useQuery, useQueryClient } from '@tanstack/react-query'; +import { hashKey } from '@tanstack/query-core';-const targetQueryHash = JSON.stringify(queryKeys.features.all(projectPath)); +const targetQueryHash = hashKey(queryKeys.features.all(projectPath));Also note:
writePersistedFeaturesis called twice on successful fetch—once inqueryFn(line 192) and once via the subscription (line 175). This is harmless but worth consolidating to a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/queries/use-features.ts` around lines 164 - 180, The subscription currently compares event.query.queryHash to JSON.stringify(target) which is brittle; replace the JSON.stringify usage in the useEffect (the targetQueryHash variable) with TanStack Query's canonical hash function (e.g. import and call hashQueryKey from '@tanstack/query-core') applied to queryKeys.features.all(projectPath) so the computed targetQueryHash matches TanStack's internal serialization; update the reference in the subscription to use that hash and keep the rest of the logic (including writePersistedFeatures and projectPathRef) unchanged, and optionally consider consolidating the duplicate writePersistedFeatures calls mentioned in the comment.apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx (1)
78-80:generateFeatureIdis a pointless thin wrapper — callgenerateUUID()directly.♻️ Proposed fix
-/** Generate a feature ID */ -function generateFeatureId(): string { - return generateUUID(); -}Then replace every call site:
- id: generateFeatureId(), + id: generateUUID(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx` around lines 78 - 80, The generateFeatureId() function is a redundant thin wrapper around generateUUID(); remove the generateFeatureId function declaration and update all call sites (references to generateFeatureId) to call generateUUID() directly (ensure any necessary import for generateUUID is present where you replace the calls).apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
1039-1101: Replace the IIFE in JSX with a plain variable derived before thereturn.IIFEs inside JSX (
{(() => { ... })()}) are non-idiomatic and harder to read.prInfocan simply be computed before the component'sreturnstatement:♻️ Proposed refactor
Above the
return:+const prInfo: PRInfo | null = worktree.pr + ? { + number: worktree.pr.number, + title: worktree.pr.title, + url: worktree.pr.url, + state: worktree.pr.state, + author: '', + body: '', + comments: [], + reviewComments: [], + } + : null;In the JSX:
-{showPRInfo && - worktree.pr && - (() => { - const prInfo: PRInfo = { ... }; - return ( - <DropdownMenuSub> - ... - </DropdownMenuSub> - ); - })()} +{showPRInfo && prInfo && ( + <DropdownMenuSub> + <div className="flex items-center"> + <DropdownMenuItem + onClick={() => { window.open(worktree.pr!.url, '_blank', 'noopener,noreferrer'); }} + className="text-xs flex-1 pr-0 rounded-r-none" + > + ... + </DropdownMenuItem> + <DropdownMenuSubTrigger className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" /> + </div> + <DropdownMenuSubContent> + ... + </DropdownMenuSubContent> + </DropdownMenuSub> +)}🤖 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 1039 - 1101, There's an IIFE used inside JSX to build prInfo and render the PR submenu; move that logic out of the JSX by computing prInfo (derived from worktree.pr) and the conditional showPR block above the component's return, then render the JSX using those variables instead of the (() => { ... })() expression; update references to worktree.pr, prInfo, and the handlers onAddressPRComments/onAutoAddressPRComments to use the precomputed prInfo.
🤖 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/generate-commit-message.ts`:
- Around line 139-149: The git subprocess calls using execFileAsync in
generate-commit-message (the calls that assign stagedDiff and unstagedDiff) have
no timeout and can hang; update those execFileAsync invocations to pass a
timeout option (e.g., { cwd: worktreePath, maxBuffer: ..., timeout: <reasonable
ms> }) so the Promise rejects when git stalls, and handle the rejection by
returning an error or falling back (ensure behavior stays consistent with how
AI_TIMEOUT_MS is used downstream); locate the execFileAsync calls and add a
timeout value and appropriate error handling around stagedDiff and unstagedDiff
retrieval.
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 149-151: Add an 'error' listener on gh.stdin to swallow or forward
spawn-level stream errors (e.g., EPIPE) so they don't become uncaught
exceptions; specifically, in the block that creates the gh process where you
already have gh.on('error', ...) and use timeoutId/reject, attach
gh.stdin.on('error', (err) => { clearTimeout(timeoutId); reject(err); }) or
otherwise handle EPIPE silently as appropriate; repeat the same fix for the
other gh invocation around the second occurrence (the one referenced at lines
176-177) so both gh.stdin streams are protected.
- Around line 180-182: The current error handling only throws
response.errors[0].message and drops remaining GraphQL errors; update the throw
to include all errors from response.errors (e.g., join all .message values or
include JSON.stringify(response.errors)) so callers see the full set of GraphQL
errors, and optionally attach the original errors array to the thrown Error
(e.g., error.details = response.errors) to preserve structured data; locate the
check using the response variable and replace the single-index throw with the
aggregated/attached-error approach.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 662-676: The mutation callback in handleResolveComment leaves
resolved thread IDs in selectedIds, causing an incorrect selection count; update
the onSettled handler of resolveThread.mutate (inside handleResolveComment) to
remove comment.threadId from selectedIds (using setSelectedIds) after the
mutation settles — e.g., compute a new array filtering out the settled threadId
and call setSelectedIds(newArray) so stale IDs are pruned when comments are
hidden by the refetch.
- Around line 732-744: The constructed Feature object wrongly includes a
non-existent steps property; remove "steps: []" from the feature literal built
where Feature is created (the object assigned to the const feature using
generateFeatureId(), generateGroupTitle(pr, selectedComments),
generateGroupDescription(pr, selectedComments), selectedModel,
normalizedThinking, normalizedReasoning and the branchName spread for
pr.headRefName) and also remove the same steps property in the other
feature-construction location referenced near line 764 so the object matches the
Feature interface and only includes supported fields.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1059-1063: Replace the window.open call inside the
DropdownMenuItem click handler so it uses the Electron bridge: call
getElectronAPI().openExternalLink(worktree.pr!.url) instead of window.open(...)
to ensure the link opens in the system browser; update the onClick in the
DropdownMenuItem component where worktree.pr!.url is referenced (keep the same
className and handler shape) and import/get getElectronAPI if not already
available in the file.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Line 153: The Save button in the EditorTabs component currently sets
title="Save file (Ctrl+S)" which is misleading for the mobile-only rendering;
update the element in editor-tabs.tsx (the Save button in the EditorTabs
component) to either remove the shortcut hint or make the title match the
aria-label by changing title to "Save file" (or remove the title attribute
entirely) so the title and aria-label are consistent for touch devices.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 183-192: The onBlur handler currently cancels the edit when the
filename is invalid, which conflicts with handleSubmit's behavior of keeping the
input open for correction; update the onBlur logic (in the component using
submittedRef, value, isValidFileName, onSubmit, onCancel) so it mirrors
handleSubmit: if submittedRef.current return; compute trimmed = value.trim(); if
trimmed && isValidFileName(trimmed) set submittedRef.current = true and call
onSubmit(trimmed); otherwise DO NOT call onCancel (so the input stays open for
the user to correct); optionally re-focus the input if needed to keep editing,
but do not discard the user's change on blur for invalid names.
---
Duplicate comments:
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 232-240: The execFileAsync invocation that fetches PR comments
(the call returning { stdout: commentsOutput } in pr-review-comments.service.ts)
lacks a maxBuffer option so large outputs can trigger "stdout maxBuffer
exceeded"; update that execFileAsync call to include a larger maxBuffer value
(e.g., 10 * 1024 * 1024) in the options object alongside cwd, env, and timeout
so the full comments JSON is not truncated or the process does not throw.
- Around line 9-12: The service currently imports execEnv and logError from the
routes layer; move these utilities into a shared lib module and update the
service import to use that lib instead: create or relocate execEnv and logError
into a shared lib (e.g., lib/githubCommon or similar), export them, then change
the imports in pr-review-comments.service.ts to import execEnv and logError from
the new shared lib and remove the route-layer import; ensure any other modules
that used the old route path are updated to the shared lib to preserve a
unidirectional dependency from routes -> services.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 569-570: resolvingThreadId currently holds only one in-flight
thread id so concurrent resolves clear the spinner incorrectly; change the state
to a Set<string> (e.g., const [resolvingThreadIds, setResolvingThreadIds] =
useState<Set<string>>(new Set())) and update all places that add/remove the id
(use setResolvingThreadIds(prev => { const next = new Set(prev);
next.add(threadId); return next; }) and similarly remove on settled) including
the place in the onSettled callback so it removes only the settled thread id
instead of setting the whole state to null, and update any UI checks that read
resolvingThreadId to use resolvingThreadIds.has(threadId).
In `@apps/ui/src/components/ui/app-error-boundary.tsx`:
- Around line 95-109: The reload icon SVG in app-error-boundary.tsx is missing
the accessibility attribute; update the <svg> element used for the reload icon
(the inline SVG block containing the four <path> elements) to include
aria-hidden="true" so screen readers ignore the decorative graphic, matching the
logo SVG's behavior.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 947-1001: The submenu currently renders a DropdownMenuSub with a
chevron trigger even when worktree.hasChanges is false, causing a redundant
single-item submenu; update the render logic in the worktree-actions-dropdown
component so that when !worktree.hasChanges && onViewStashes is true you do not
render DropdownMenuSub/DropdownMenuSubTrigger/DropdownMenuSubContent—instead
render a plain DropdownMenuItem that calls onViewStashes(worktree); keep the
existing branch that uses DropdownMenuSub only when worktree.hasChanges is true
(so DropdownMenuSubTrigger and the stash options remain for the multi-item
case).
- Around line 374-383: The condition showing the DropdownMenuItem allows
devServerInfo to be undefined because "devServerInfo?.urlDetected !== false"
evaluates true when devServerInfo is undefined; update the render condition to
require devServerInfo (and ideally a defined port) e.g. check "devServerInfo !=
null && devServerInfo.urlDetected !== false" or "devServerInfo?.port != null &&
devServerInfo.urlDetected !== false" so the item isn't rendered for undefined
devServerInfo, and also make the aria-label in the DropdownMenuItem (and the
onClick call to onOpenDevServerUrl(worktree)) only interpolate the port when
devServerInfo?.port is defined to avoid "undefined" in the label.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 143-157: No change required: the Save button in editor-tabs.tsx
already includes an accessible aria-label ("Save file") alongside the title
attribute; leave the button in the render block that uses onSave, isDirty, and
the Save icon unchanged and ensure the aria-label is preserved if refactoring
the onClick handler or className logic in the future.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 156-169: The handler handleSubmit currently returns silently when
isValidFileName(trimmed) is false; instead update the component to provide
visible feedback (e.g., set a local validation error state like validationError
via useState or call an existing showToast/showError prop) and ensure the input
is focused so the user can correct it; do not set submittedRef.current or call
onSubmit/onCancel in this branch. Update handleSubmit to set the error message
(and clear it on input change), and include any new state in the hook
dependencies.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 175-239: The inline featureData type in handleAddFeatureFromIssue
is missing imagePaths and textFilePaths and the constructed feature object
doesn’t forward those attachments to api.features.create; update the parameter
type for handleAddFeatureFromIssue to include imagePaths: string[] and
textFilePaths: string[] (or optional) and add imagePaths: featureData.imagePaths
and textFilePaths: featureData.textFilePaths to the feature object before
calling api.features.create (ensure sensible defaults like [] if undefined) so
attachments from AddFeatureDialog are preserved.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 77-88: The feature object is including a non-existent/ignored
property steps (dead data); remove the "steps: []" entry from the Feature
literal in the feature creation (the const feature in github-prs-view.tsx) so
the object only contains fields declared on the Feature interface (id, title,
category, description, status, model via resolveModelString('opus'),
thinkingLevel, planningMode, and optional branchName from pr.headRefName);
ensure no other dead properties are added to feature in this block.
---
Nitpick comments:
In `@apps/server/src/routes/worktree/routes/generate-commit-message.ts`:
- Around line 48-69: The finally block currently only clears timerId and doesn't
close the underlying AI-stream iterator on non-timeout early exits; update the
finally block to, when !done, call and await iterator.return?.() to ensure the
iterator is closed on any early exit (e.g., exceptions or break), catching and
logging any teardown error (logger.warn) but not rethrowing so cleanup proceeds,
and keep the existing clearTimeout(timerId) behavior; reference iterator.return,
done, timerId, and the existing timeoutPromise/Promise.race flow when making
this change.
- Around line 76-80: The helper getSystemPrompt now contains business logic and
should be moved out of the route into a service; create a new service file
(e.g., commit-message.service.ts) that exports an async function (e.g.,
getSystemPrompt) which accepts a SettingsService, calls
settingsService.getGlobalSettings(), invokes
mergeCommitMessagePrompts(settings?.promptCustomization?.commitMessage) and
returns prompts.systemPrompt, then update the route to import and call that
service function instead of defining it inline; ensure the service is exported
and the route delegates to it, keeping SettingsService usage unchanged.
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 228-321: Parallelise the two independent GH calls by extracting
their try/catch bodies into two small helpers named fetchRegularComments and
fetchInlineComments that each return PRReviewComment[] and internally catch/log
errors via logError (preserving the current best-effort semantics and using
execFileAsync and JSON.parse as before); then call them concurrently (alongside
resolvedStatusPromise if needed) with Promise.allSettled, merge the settled
results into allComments, and ensure existing fields (isReviewComment,
isOutdated, isResolved, avatarUrl, diffHunk, commitId, etc.) are preserved
exactly as in the original mapping.
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 78-80: The generateFeatureId() function is a redundant thin
wrapper around generateUUID(); remove the generateFeatureId function declaration
and update all call sites (references to generateFeatureId) to call
generateUUID() directly (ensure any necessary import for generateUUID is present
where you replace the calls).
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1039-1101: There's an IIFE used inside JSX to build prInfo and
render the PR submenu; move that logic out of the JSX by computing prInfo
(derived from worktree.pr) and the conditional showPR block above the
component's return, then render the JSX using those variables instead of the (()
=> { ... })() expression; update references to worktree.pr, prInfo, and the
handlers onAddressPRComments/onAutoAddressPRComments to use the precomputed
prInfo.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 147-152: The rendered button uses a plain "cursor-not-allowed"
class which is ignored for native disabled buttons; update the class switch in
editor-tabs.tsx (the JSX that builds the className around isDirty) to use
Tailwind's disabled variant instead — replace the non-disabled cursor class with
"disabled:cursor-not-allowed" in the branch that represents the disabled state
and ensure the button element uses the disabled attribute when isDirty is false
so the disabled: variant can take effect.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 575-590: The onClick handlers in file-tree.tsx that call
openFileBrowser and then await onCopyItem/onMoveItem currently catch errors and
only call console.error, leaving users unaware of failures; update these catch
blocks to surface failures via the app's user notification system (e.g., calling
the existing toast/notification helper or dispatching an error banner) and
include the error message/context (e.g., `Copy "name" failed: ${err.message}`)
so the user sees a clear message; ensure you update both the onCopy path
(handler referencing openFileBrowser and onCopyItem) and the onMove path
(handler referencing onMoveItem) to use the same notification API and keep
console.error for debug logging if desired.
In `@apps/ui/src/components/views/terminal-view.tsx`:
- Around line 1152-1200: The duplicated UI/state cleanup in killTerminal should
be moved into a finally block: keep the API call and response handling
(including the response.ok check and fetchServerSettings()) in the try block,
handle errors/logging in the catch block, and perform
removeTerminalFromLayout(sessionId), setSessionCommandOverrides(... delete
sessionId ...), and setNewSessionIds(... delete sessionId ...) once in a finally
block so cleanup always runs; reference the killTerminal function and the
existing helpers removeTerminalFromLayout, setSessionCommandOverrides,
setNewSessionIds, and fetchServerSettings when making the change.
In `@apps/ui/src/hooks/queries/use-features.ts`:
- Around line 174-175: Remove the redundant localStorage write by deleting the
call to writePersistedFeatures(...) inside the queryFn implementation and keep
the single write in the useEffect subscription callback (which already writes
when features and projectPathRef.current are present); if you are worried about
the initial-render race where the fetch resolves before the subscription
registers, either leave a short comment in the queryFn explaining the tradeoff
or retain the queryFn write but document it as a deliberate safety net—ensure
all references to writePersistedFeatures are consistent and only invoked from
the subscription path (or are explicitly documented if left in queryFn).
- Around line 164-180: The subscription currently compares event.query.queryHash
to JSON.stringify(target) which is brittle; replace the JSON.stringify usage in
the useEffect (the targetQueryHash variable) with TanStack Query's canonical
hash function (e.g. import and call hashQueryKey from '@tanstack/query-core')
applied to queryKeys.features.all(projectPath) so the computed targetQueryHash
matches TanStack's internal serialization; update the reference in the
subscription to use that hash and keep the rest of the logic (including
writePersistedFeatures and projectPathRef) unchanged, and optionally consider
consolidating the duplicate writePersistedFeatures calls mentioned in the
comment.
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/file-editor-view/components/file-tree.tsx
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (4)
1059-1059: Redundantworktree.prguard —prInfoalready encodes the null case.
prInfoisnullwheneverworktree.pris falsy (memo explicitly guards on it), so the extraworktree.pr &&is unnecessary.-{prInfo && worktree.pr && ( +{prInfo && (🤖 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` at line 1059, Remove the redundant guard on worktree.pr in the JSX conditional: the memo that produces prInfo already returns null when worktree.pr is falsy, so replace the compound condition using both prInfo and worktree.pr with a single prInfo check (i.e., change the JSX expression that currently reads {prInfo && worktree.pr && (...)} to only use {prInfo && (...)}), updating the component rendering block in worktree-actions-dropdown.tsx where prInfo is referenced.
59-59: Prefer barrel export over deep direct-path import for intra-app type.
TerminalScriptis imported via a direct path intoterminal-scripts-constants. Per the codebase convention, intra-app imports should go through the component index barrel rather than reaching into file internals.-import type { TerminalScript } from '@/components/views/project-settings-view/terminal-scripts-constants'; +import type { TerminalScript } from '@/components/views/project-settings-view';(Only if
TerminalScriptis re-exported from the view's barrel index; otherwise the barrel should be updated to include it.)Based on learnings: "when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports."
🤖 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` at line 59, Replace the deep import of the TerminalScript type from terminal-scripts-constants with the app's components barrel export: update the import in worktree-actions-dropdown.tsx to import TerminalScript from the view's/components' index barrel instead of '@/components/views/project-settings-view/terminal-scripts-constants'; if TerminalScript is not currently re-exported from the barrel, add it to the appropriate components index (re-export TerminalScript from terminal-scripts-constants) and then update the import in worktree-actions-dropdown.tsx to use that barrel.
606-612: Scripts submenu is unconditionally rendered — consider hiding when empty.For main worktrees with no
terminalScriptsand noonEditScriptshandler, the submenu body renders only a disabled "No scripts configured" placeholder and a disabled "Edit Commands & Scripts" item — all non-actionable. Showing a submenu trigger for this state is misleading UX.Consider gating the entire
DropdownMenuSubon whether it has at least one actionable item:+{/* Only render Scripts submenu when there is something actionable */} +{(!worktree.isMain || (terminalScripts && terminalScripts.length > 0) || onEditScripts) && ( <DropdownMenuSub> <DropdownMenuSubTrigger className="text-xs"> <ScrollText className="w-3.5 h-3.5 mr-2" /> Scripts </DropdownMenuSubTrigger> ... </DropdownMenuSub> +)}🤖 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 606 - 612, The Scripts submenu (DropdownMenuSub and its DropdownMenuSubTrigger) should be hidden when it has no actionable items; change the render to only include DropdownMenuSub when there is at least one actionable script (e.g., terminalScripts?.length > 0) or an edit handler exists (onEditScripts) or an init script is present. Locate the DropdownMenuSub / DropdownMenuSubTrigger block and gate its rendering on a condition like (terminalScripts?.length > 0 || !!onEditScripts || !!initScript) so the submenu trigger is not shown when only non-actionable disabled items (like the "No scripts configured" placeholder and the disabled "Edit Commands & Scripts" entry) would appear.
960-965: Stale duplicate comment block — clean up.The comment at lines 960–964 is a leftover of the old description and is immediately followed by the updated one at line 965. The first block should be removed.
- {/* View Changes split button - main action views changes directly, chevron reveals stash options. - Only render when at least one action is meaningful: - - worktree.hasChanges: View Changes action is available - - (worktree.hasChanges && onStashChanges): Create Stash action is possible - - onViewStashes: viewing existing stashes is possible */} {/* View Changes split button - show submenu only when there are non-duplicate sub-actions */}🤖 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 960 - 965, Remove the duplicated/stale comment block describing the "View Changes split button" that precedes the updated comment; specifically delete the first comment paragraph that references worktree.hasChanges, onStashChanges, and onViewStashes so only the single, current comment remains near the WorktreeActionsDropdown component (look for the "View Changes split button" comment and references to worktree.hasChanges/onStashChanges/onViewStashes to locate it).apps/ui/src/components/views/github-issues-view.tsx (1)
258-278: Optional: extract shared description header into a helper to reduce duplication.The issue-header fragment (
**From GitHub Issue #…**, blank line,issue.body) is copy-pasted frombuildIssueDescription.handleConvertToTaskonly differs in that it always appends validation and omits labels/linked PRs. A small shared helper (e.g.,buildIssueHeader(issue)) would keep the two paths in sync if the format ever changes.♻️ Suggested refactor
+ // Shared issue header builder (DRY) + const buildIssueHeader = (issue: GitHubIssue) => [ + `**From GitHub Issue #${issue.number}**`, + '', + issue.body || 'No description provided.', + ]; const buildIssueDescription = useCallback( (issue: GitHubIssue) => { - const parts = [ - `**From GitHub Issue #${issue.number}**`, - '', - issue.body || 'No description provided.', - ]; + const parts = buildIssueHeader(issue); // ... rest unchanged // Inside handleConvertToTask: - const parts = [ - `**From GitHub Issue #${issue.number}**`, - '', - issue.body || 'No description provided.', - '', - '---', - '', - '**AI Validation Analysis:**', - validation.reasoning, - ]; + const parts = [ + ...buildIssueHeader(issue), + '', + '---', + '', + '**AI Validation Analysis:**', + validation.reasoning, + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-issues-view.tsx` around lines 258 - 278, The issue is duplicated issue-header formatting between buildIssueDescription and handleConvertToTask; extract the common header into a helper like buildIssueHeader(issue) that returns the initial array or string (containing `**From GitHub Issue #${issue.number}**`, the blank line and `issue.body || 'No description provided.'`) and then reuse it in both buildIssueDescription and handleConvertToTask before appending their respective extra sections (labels/linked PRs or validation), updating references to validation.suggestedFix and validation.relatedFiles only in handleConvertToTask.apps/server/src/services/github-pr-comment.service.ts (1)
90-92: Only the first GraphQL error is surfaced; remaining errors are silently dropped.
response.errorscan contain multiple entries. Joining them aids debugging, especially for complex mutations.💡 Suggested improvement
if (response.errors && response.errors.length > 0) { - throw new Error(response.errors[0].message); + throw new Error(response.errors.map((e) => e.message).join('; ')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/github-pr-comment.service.ts` around lines 90 - 92, The code currently throws only the first GraphQL error (response.errors[0].message); update the error handling where the GraphQL response is processed (the response variable in github-pr-comment.service.ts) to aggregate all messages from response.errors (e.g., map over response.errors to extract .message and join with a separator) and throw a single Error containing the joined messages so no errors are silently dropped.apps/ui/src/components/views/github-prs-view.tsx (1)
261-431: Consider extracting the IIFE into a named component.The PR detail panel is rendered via an immediately-invoked function expression to scope the
reviewStatusconst. This is functional but unidiomatic for React — it creates a new function on every render and bypasses React's reconciliation for the inner tree. Extracting to a<PRDetailPanel>component (or computingreviewStatusviauseMemoand using inline JSX) would be cleaner and allow React to optimize re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/github-prs-view.tsx` around lines 261 - 431, The current IIFE rendering the PR detail panel should be replaced by a named React component to avoid creating a new function each render; create a PRDetailPanel component that accepts props (selectedPR, isMobile, setSelectedPR, setCommentDialogPR, handleOpenInGitHub, handleAutoAddressComments) and move the getReviewStatus(selectedPR) call and all JSX inside it (or memoize reviewStatus with useMemo inside PRDetailPanel), then replace the IIFE block with <PRDetailPanel ...props /> so React can better reconcile and optimize re-renders.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (1)
592-633: Silent failure on Copy/Move — consider surfacing an error to the user.Both handlers catch errors from
openFileBrowser/onCopyItem/onMoveItemand log them viaconsole.error, but the user sees nothing when an operation fails. If the parent callbacks don't already emit their own error toasts, the operation will appear to silently succeed (or just not happen).Consider either propagating the error up so the parent can display feedback, or using whatever toast/notification primitive the app already exposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 592 - 633, The Copy/Move handlers swallow errors (caught in the onClick async blocks around openFileBrowser/onCopyItem/onMoveItem) and only call console.error so users get no feedback; update those handlers (the DropdownMenuItem onClick blocks that reference node.path, openFileBrowser, onCopyItem and onMoveItem) to surface failures by either rethrowing the caught error or invoking the app's notification/toast API with a clear message (e.g., include node.name and err.message) before/after logging, so the parent or UI shows an error toast instead of silently failing.apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx (1)
162-165: Incomplete scripts are silently discarded on save without user feedback.
validScriptsfilters out any entry wherenameorcommandis blank (line 165), andsetScripts(normalizedScripts)on success (line 184) removes those rows from the UI. A user who added a row but only partially filled it in will see it disappear after saving with no explanation.Consider either surfacing a warning before saving, or at minimum a toast noting how many entries were skipped:
💡 Example: toast on filtered entries
const validScripts = scripts.filter((s) => s.name.trim() && s.command.trim()); +const skippedCount = scripts.length - validScripts.length; const normalizedScripts = validScripts.map((s) => ({ ... })); updateSettingsMutation.mutate(..., { onSuccess: () => { + if (skippedCount > 0) { + toast.warning(`${skippedCount} incomplete script${skippedCount > 1 ? 's' : ''} were not saved.`); + } setDevCommand(normalizedDevCommand); ... }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx` around lines 162 - 165, The save flow in handleSave currently drops incomplete entries by computing validScripts = scripts.filter(...) and later calling setScripts(normalizedScripts) without user feedback; modify handleSave to detect skippedCount = scripts.length - validScripts.length and if skippedCount > 0 show a user-visible notification (toast or inline warning) that "X incomplete scripts were skipped" (or surface a confirmation dialog asking to proceed), then proceed to setScripts(normalizedScripts) and complete the save; reference handleSave, validScripts, normalizedScripts and setScripts when making the change so the feedback is triggered right before persisting the filtered scripts.
🤖 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/github-pr-comment.service.ts`:
- Around line 86-88: The gh.stdin write/close sequence
(gh.stdin.write(requestBody); gh.stdin.end();) lacks an 'error' handler which
can cause unhandled EPIPE exceptions; add a gh.stdin.on('error', handler) before
writing that logs or safely ignores EPIPEs (and rethrows other errors) and
remove the handler after the stream finishes (e.g., in gh.stdin.on('finish' or
in the callback after end) or via once) so gh.stdin errors are handled; use the
exact gh.stdin, gh.stdin.write, gh.stdin.end, and requestBody symbols to locate
where to attach and clean up the listener.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 369-377: Add accessible labels tied to the inputs with
id="dev-command" and id="test-command": create <label htmlFor="dev-command"> and
<label htmlFor="test-command"> (use the same visible h3 text or a
visually-hidden/sr-only variant if you want the heading to remain purely visual)
and place them immediately before each <Input> so screen readers have a
persistent label instead of relying on placeholders; update the JSX around the
devCommand and testCommand inputs (the Input with id="dev-command" and the Input
with id="test-command") to include these labels following the existing
visual/spacing pattern used elsewhere in the component.
- Around line 601-611: The preset buttons are showing preset.command instead of
the human-readable label; update the JSX in the SCRIPT_PRESETS map so the Button
displays preset.name (or preset.label if you prefer consistency with
DEV_SERVER_PRESETS/TEST_PRESETS) while keeping the key as preset.command and
keeping onClick calling handleAddPreset(preset); change the Button child from
{preset.command} to {preset.name} so the quick-add buttons are easier to scan.
- Around line 551-566: The Input fields for script.name and script.command in
the commands-and-scripts-section lack accessible labels; update the component
rendering where the Input for script.name and script.command is used (the
handlers handleUpdateScript and handleKeyDown are nearby) to provide accessible
labeling by either adding explicit aria-label attributes (e.g.,
aria-label={`Script name for ${script.id || index}`} and aria-label={`Script
command for ${script.id || index}`}) or by rendering a visually-hidden <label>
element linked via id to each Input; ensure ids are unique per row (use index or
script.id) and apply the labels to both the name and command Inputs so screen
readers announce them reliably.
---
Duplicate comments:
In `@apps/server/src/services/github-pr-comment.service.ts`:
- Around line 30-34: The executeReviewThreadMutation function mutates GitHub
state but never emits a websocket event; after the state change completes (and
before returning), call createEventEmitter() (import it if missing) and emit a
descriptive event (e.g., "reviewThread.updated" or "reviewThread.resolved") with
a payload containing projectPath, threadId and the new isResolved boolean so the
frontend can react; ensure the emission occurs only after the mutation succeeds
and include the same isResolved value returned by executeReviewThreadMutation.
In `@apps/server/src/services/pr-review-comments.service.ts`:
- Around line 176-177: Add an 'error' listener to gh.stdin before calling
gh.stdin.write(requestBody) / gh.stdin.end() to prevent an uncaught EPIPE from
crashing; specifically, attach gh.stdin.on('error', ...) that handles or logs
EPIPE (and other errors) and then remove the listener after the stream finishes
(or use once) so you don't leak handlers; ensure the handler is registered in
the same scope where gh.stdin.write and gh.stdin.end are invoked.
- Around line 180-182: The current check in the response handling only throws
the first GraphQL error (response.errors[0].message); change it to aggregate all
error messages from response.errors (e.g., map over response.errors to extract
message and join with a separator) and throw a single Error containing the
combined messages so callers see every GraphQL error; update the code around the
response.errors check in pr-review-comments.service (the block that currently
throws new Error(response.errors[0].message)) to build and include the joined
messages (and optionally include the original errors array or JSON string for
more detail).
In `@apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx`:
- Around line 661-684: When resolving a thread remove its ID from the
selectedIds set so the selection badge stays correct: in handleResolveComment
(and its onSettled handler) call the state updater for selectedIds (e.g.,
setSelectedIds) to produce a new Set that deletes the resolved threadId; also
add an effect that runs when comments or the "hide resolved" filter changes to
intersect selectedIds with the current visible comments' IDs (derived from
comments) so any IDs that disappeared due to hiding resolved threads are cleaned
up automatically.
- Line 745: The Feature objects in pr-comment-resolution-dialog.tsx include a
dead "steps: []" property that isn't defined on the Feature interface; remove
the unnecessary "steps: []" entries from the feature object literals (or
alternatively add an optional steps?: YourStepType[] to the Feature interface if
those steps are intended to be used) so the data matches the Feature type; look
for the feature array/object literals referenced in pr-comment-resolution-dialog
(where Feature is used/constructed) and either delete the "steps" key from those
objects or add an optional steps property to the Feature interface declaration
to make it intentional.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1063-1066: Replace the direct window.open call in
DropdownMenuItem’s onClick (the usage referencing worktree.pr!.url) with the
established Electron helper so the link opens in the system browser; locate the
onClick in worktree-actions-dropdown.tsx and call
getElectronAPI().openExternalLink(worktree.pr!.url) instead of window.open(...),
removing the noopener/noreferrer window.open pattern to match other components
like github-prs-view.tsx and use-worktree-mutations.ts.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 779-791: handleRootDragOver correctly preserves draggedPaths and
avoids redundant updates; ensure the handler continues to set dropTargetPath
with setDragState({ ...dragState, dropTargetPath: effectivePath }) only when
dragState.dropTargetPath !== effectivePath, and keep the useCallback dependency
array including effectivePath, dragState, and setDragState so the closure is
up-to-date (function name: handleRootDragOver; state: dragState; setter:
setDragState).
- Around line 191-204: The onBlur handler currently keeps the input focused when
the trimmed value is empty, which diverges from handleSubmit (which calls
onCancel on empty) and makes the comment "// mirrors handleSubmit" misleading;
update the onBlur logic in the file-tree component so that if trimmed is empty
it calls onCancel() (and does not call inputRef.current?.focus()), while
preserving the existing submittedRef and valid-name branch behavior (use the
same checks of submittedRef.current, isValidFileName, onSubmit and submittedRef
manipulation as in the onBlur block) so blur and Enter behave consistently; also
update or remove the misleading comment near onBlur to reflect the corrected
empty-string behavior.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Line 82: Remove the dead "steps: []" property from the Feature object in
github-prs-view.tsx (the same unused property noted in the dialog file); locate
the Feature definition/instantiation that includes steps and delete that key (or
replace it with the correct, used field if intended), and ensure no other code
references Feature.steps—if references exist, either implement the intended
behavior or update callers to the actual field name.
---
Nitpick comments:
In `@apps/server/src/services/github-pr-comment.service.ts`:
- Around line 90-92: The code currently throws only the first GraphQL error
(response.errors[0].message); update the error handling where the GraphQL
response is processed (the response variable in github-pr-comment.service.ts) to
aggregate all messages from response.errors (e.g., map over response.errors to
extract .message and join with a separator) and throw a single Error containing
the joined messages so no errors are silently dropped.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Line 1059: Remove the redundant guard on worktree.pr in the JSX conditional:
the memo that produces prInfo already returns null when worktree.pr is falsy, so
replace the compound condition using both prInfo and worktree.pr with a single
prInfo check (i.e., change the JSX expression that currently reads {prInfo &&
worktree.pr && (...)} to only use {prInfo && (...)}), updating the component
rendering block in worktree-actions-dropdown.tsx where prInfo is referenced.
- Line 59: Replace the deep import of the TerminalScript type from
terminal-scripts-constants with the app's components barrel export: update the
import in worktree-actions-dropdown.tsx to import TerminalScript from the
view's/components' index barrel instead of
'@/components/views/project-settings-view/terminal-scripts-constants'; if
TerminalScript is not currently re-exported from the barrel, add it to the
appropriate components index (re-export TerminalScript from
terminal-scripts-constants) and then update the import in
worktree-actions-dropdown.tsx to use that barrel.
- Around line 606-612: The Scripts submenu (DropdownMenuSub and its
DropdownMenuSubTrigger) should be hidden when it has no actionable items; change
the render to only include DropdownMenuSub when there is at least one actionable
script (e.g., terminalScripts?.length > 0) or an edit handler exists
(onEditScripts) or an init script is present. Locate the DropdownMenuSub /
DropdownMenuSubTrigger block and gate its rendering on a condition like
(terminalScripts?.length > 0 || !!onEditScripts || !!initScript) so the submenu
trigger is not shown when only non-actionable disabled items (like the "No
scripts configured" placeholder and the disabled "Edit Commands & Scripts"
entry) would appear.
- Around line 960-965: Remove the duplicated/stale comment block describing the
"View Changes split button" that precedes the updated comment; specifically
delete the first comment paragraph that references worktree.hasChanges,
onStashChanges, and onViewStashes so only the single, current comment remains
near the WorktreeActionsDropdown component (look for the "View Changes split
button" comment and references to
worktree.hasChanges/onStashChanges/onViewStashes to locate it).
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 592-633: The Copy/Move handlers swallow errors (caught in the
onClick async blocks around openFileBrowser/onCopyItem/onMoveItem) and only call
console.error so users get no feedback; update those handlers (the
DropdownMenuItem onClick blocks that reference node.path, openFileBrowser,
onCopyItem and onMoveItem) to surface failures by either rethrowing the caught
error or invoking the app's notification/toast API with a clear message (e.g.,
include node.name and err.message) before/after logging, so the parent or UI
shows an error toast instead of silently failing.
In `@apps/ui/src/components/views/github-issues-view.tsx`:
- Around line 258-278: The issue is duplicated issue-header formatting between
buildIssueDescription and handleConvertToTask; extract the common header into a
helper like buildIssueHeader(issue) that returns the initial array or string
(containing `**From GitHub Issue #${issue.number}**`, the blank line and
`issue.body || 'No description provided.'`) and then reuse it in both
buildIssueDescription and handleConvertToTask before appending their respective
extra sections (labels/linked PRs or validation), updating references to
validation.suggestedFix and validation.relatedFiles only in handleConvertToTask.
In `@apps/ui/src/components/views/github-prs-view.tsx`:
- Around line 261-431: The current IIFE rendering the PR detail panel should be
replaced by a named React component to avoid creating a new function each
render; create a PRDetailPanel component that accepts props (selectedPR,
isMobile, setSelectedPR, setCommentDialogPR, handleOpenInGitHub,
handleAutoAddressComments) and move the getReviewStatus(selectedPR) call and all
JSX inside it (or memoize reviewStatus with useMemo inside PRDetailPanel), then
replace the IIFE block with <PRDetailPanel ...props /> so React can better
reconcile and optimize re-renders.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`:
- Around line 162-165: The save flow in handleSave currently drops incomplete
entries by computing validScripts = scripts.filter(...) and later calling
setScripts(normalizedScripts) without user feedback; modify handleSave to detect
skippedCount = scripts.length - validScripts.length and if skippedCount > 0 show
a user-visible notification (toast or inline warning) that "X incomplete scripts
were skipped" (or surface a confirmation dialog asking to proceed), then proceed
to setScripts(normalizedScripts) and complete the save; reference handleSave,
validScripts, normalizedScripts and setScripts when making the change so the
feedback is triggered right before persisting the filtered scripts.
Comment on lines
+86
to
+88
| gh.stdin.write(requestBody); | ||
| gh.stdin.end(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing gh.stdin error handler — potential unhandled EPIPE crash.
If the gh process exits before consuming stdin, an 'error' event (typically EPIPE) is emitted on gh.stdin with no listener, which Node.js converts to an uncaught exception. The same pattern issue was identified in pr-review-comments.service.ts.
🛡️ Proposed fix
+ gh.stdin.on('error', (err) => {
+ // Suppress EPIPE — the process close/error handler will reject
+ if ((err as NodeJS.ErrnoException).code !== 'EPIPE') {
+ clearTimeout(timeoutId);
+ rej(err);
+ }
+ });
+
gh.stdin.write(requestBody);
gh.stdin.end();📝 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
| gh.stdin.write(requestBody); | |
| gh.stdin.end(); | |
| }); | |
| gh.stdin.on('error', (err) => { | |
| // Suppress EPIPE — the process close/error handler will reject | |
| if ((err as NodeJS.ErrnoException).code !== 'EPIPE') { | |
| clearTimeout(timeoutId); | |
| rej(err); | |
| } | |
| }); | |
| gh.stdin.write(requestBody); | |
| gh.stdin.end(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/github-pr-comment.service.ts` around lines 86 - 88,
The gh.stdin write/close sequence (gh.stdin.write(requestBody); gh.stdin.end();)
lacks an 'error' handler which can cause unhandled EPIPE exceptions; add a
gh.stdin.on('error', handler) before writing that logs or safely ignores EPIPEs
(and rethrows other errors) and remove the handler after the stream finishes
(e.g., in gh.stdin.on('finish' or in the callback after end) or via once) so
gh.stdin errors are handled; use the exact gh.stdin, gh.stdin.write,
gh.stdin.end, and requestBody symbols to locate where to attach and clean up the
listener.
Comment on lines
+369
to
+377
| <Input | ||
| id="dev-command" | ||
| value={devCommand} | ||
| onChange={(e) => setDevCommand(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder="e.g., npm run dev, yarn dev, cargo watch" | ||
| className="font-mono text-sm pr-8" | ||
| data-testid="dev-command-input" | ||
| /> |
Contributor
There was a problem hiding this comment.
id-linked <label> elements are missing for both command inputs.
id="dev-command" and id="test-command" are set on the inputs, but no <label htmlFor="..."> is associated with them. The <h3> text above is purely visual; screen readers will fall back to the placeholder, which disappears once content is typed.
♿ Proposed fix (same pattern for both inputs)
<div className="flex items-center gap-2">
<Play className="w-4 h-4 text-brand-500" />
- <h3 className="text-base font-medium text-foreground">Dev Server</h3>
+ <label
+ htmlFor="dev-command"
+ className="text-base font-medium text-foreground cursor-pointer"
+ >
+ Dev Server
+ </label> <div className="flex items-center gap-2">
<FlaskConical className="w-4 h-4 text-brand-500" />
- <h3 className="text-base font-medium text-foreground">Test Runner</h3>
+ <label
+ htmlFor="test-command"
+ className="text-base font-medium text-foreground cursor-pointer"
+ >
+ Test Runner
+ </label>Also applies to: 425-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`
around lines 369 - 377, Add accessible labels tied to the inputs with
id="dev-command" and id="test-command": create <label htmlFor="dev-command"> and
<label htmlFor="test-command"> (use the same visible h3 text or a
visually-hidden/sr-only variant if you want the heading to remain purely visual)
and place them immediately before each <Input> so screen readers have a
persistent label instead of relying on placeholders; update the JSX around the
devCommand and testCommand inputs (the Input with id="dev-command" and the Input
with id="test-command") to include these labels following the existing
visual/spacing pattern used elsewhere in the component.
Comment on lines
+551
to
+566
| <Input | ||
| value={script.name} | ||
| onChange={(e) => handleUpdateScript(index, 'name', e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder="Script name" | ||
| className="h-8 text-sm flex-[0.4] min-w-0" | ||
| /> | ||
|
|
||
| {/* Script command */} | ||
| <Input | ||
| value={script.command} | ||
| onChange={(e) => handleUpdateScript(index, 'command', e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder="Command to run" | ||
| className="h-8 text-sm font-mono flex-[0.6] min-w-0" | ||
| /> |
Contributor
There was a problem hiding this comment.
Script row inputs have no accessible labels.
The name and command Input fields in each script row have neither aria-label attributes nor linked <label> elements. Placeholder text alone is insufficient for screen readers (it disappears on input and is not reliably announced).
♿ Proposed fix
<Input
value={script.name}
onChange={(e) => handleUpdateScript(index, 'name', e.target.value)}
onKeyDown={handleKeyDown}
placeholder="Script name"
+ aria-label={`Script ${index + 1} name`}
className="h-8 text-sm flex-[0.4] min-w-0"
/>
<Input
value={script.command}
onChange={(e) => handleUpdateScript(index, 'command', e.target.value)}
onKeyDown={handleKeyDown}
placeholder="Command to run"
+ aria-label={`Script ${index + 1} command`}
className="h-8 text-sm font-mono flex-[0.6] min-w-0"
/>📝 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
| <Input | |
| value={script.name} | |
| onChange={(e) => handleUpdateScript(index, 'name', e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Script name" | |
| className="h-8 text-sm flex-[0.4] min-w-0" | |
| /> | |
| {/* Script command */} | |
| <Input | |
| value={script.command} | |
| onChange={(e) => handleUpdateScript(index, 'command', e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Command to run" | |
| className="h-8 text-sm font-mono flex-[0.6] min-w-0" | |
| /> | |
| <Input | |
| value={script.name} | |
| onChange={(e) => handleUpdateScript(index, 'name', e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Script name" | |
| aria-label={`Script ${index + 1} name`} | |
| className="h-8 text-sm flex-[0.4] min-w-0" | |
| /> | |
| {/* Script command */} | |
| <Input | |
| value={script.command} | |
| onChange={(e) => handleUpdateScript(index, 'command', e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Command to run" | |
| aria-label={`Script ${index + 1} command`} | |
| className="h-8 text-sm font-mono flex-[0.6] min-w-0" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`
around lines 551 - 566, The Input fields for script.name and script.command in
the commands-and-scripts-section lack accessible labels; update the component
rendering where the Input for script.name and script.command is used (the
handlers handleUpdateScript and handleKeyDown are nearby) to provide accessible
labeling by either adding explicit aria-label attributes (e.g.,
aria-label={`Script name for ${script.id || index}`} and aria-label={`Script
command for ${script.id || index}`}) or by rendering a visually-hidden <label>
element linked via id to each Input; ensure ids are unique per row (use index or
script.id) and apply the labels to both the name and command Inputs so screen
readers announce them reliably.
Comment on lines
+601
to
+611
| {SCRIPT_PRESETS.map((preset) => ( | ||
| <Button | ||
| key={preset.command} | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => handleAddPreset(preset)} | ||
| className="text-xs font-mono h-7 px-2" | ||
| > | ||
| {preset.command} | ||
| </Button> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Preset button renders command instead of name.
SCRIPT_PRESETS buttons display {preset.command} (e.g., npm run dev) while the analogous DEV_SERVER_PRESETS and TEST_PRESETS buttons display {preset.label}. In the presets section the human-readable preset.name field (e.g., Dev Server, Build) is ignored, making the quick-add buttons harder to scan.
🐛 Proposed fix
- {preset.command}
+ {preset.name}📝 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
| {SCRIPT_PRESETS.map((preset) => ( | |
| <Button | |
| key={preset.command} | |
| variant="outline" | |
| size="sm" | |
| onClick={() => handleAddPreset(preset)} | |
| className="text-xs font-mono h-7 px-2" | |
| > | |
| {preset.command} | |
| </Button> | |
| ))} | |
| {SCRIPT_PRESETS.map((preset) => ( | |
| <Button | |
| key={preset.command} | |
| variant="outline" | |
| size="sm" | |
| onClick={() => handleAddPreset(preset)} | |
| className="text-xs font-mono h-7 px-2" | |
| > | |
| {preset.name} | |
| </Button> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx`
around lines 601 - 611, The preset buttons are showing preset.command instead of
the human-readable label; update the JSX in the SCRIPT_PRESETS map so the Button
displays preset.name (or preset.label if you prefer consistency with
DEV_SERVER_PRESETS/TEST_PRESETS) while keeping the key as preset.command and
keeping onClick calling handleAddPreset(preset); change the Button child from
{preset.command} to {preset.name} so the quick-add buttons are easier to scan.
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
This PR introduces a full PR review comment workflow — users can now view, manage, and resolve GitHub PR review comments directly within Automaker, and even dispatch AI agents to address them. Additionally, this PR ships a new Commands & Scripts settings section, several UX improvements to the board and worktree panels, and fixes to AI prompt handling to prevent model preamble leaking into generated commit messages and PR descriptions.
What's New
🔍 PR Review Comments (
PRCommentResolutionDialog)POST /github/pr-review-comments— fetches both inline code review comments and general PR comments, enriched with GitHub GraphQL data (thread resolution status, diff hunk context, outdated state)POST /github/resolve-pr-comment— resolves or unresolves a PR review thread via GitHub GraphQL mutationPRCommentResolutionDialog(1,095 lines) with:useGitHubPRReviewCommentsanduseResolveReviewThread⚙️ Commands & Scripts Settings Section
commands-and-scripts-section.tsx(594 lines) in Project Settings🗂️ Worktree Actions Dropdown Improvements
onRunTerminalScriptcallback navigates to the terminal view with a pre-set command📋 Board View Improvements
🐙 GitHub Issues View Improvements
AddFeatureDialogprefilled with issue data (title, body, labels, linked PRs, AI validation analysis)AddFeatureDialogacceptsprefilledTitle,prefilledDescription, andprefilledCategoryprops🤖 AI Prompt Handling Fixes
---TITLE---marker anywhere in the response (not just at the start); improved fallback logic to skip AI preamble lines ("I'll analyze...","Here is...", etc.)resultmessage overwriting longer accumulated text from streaming🔀 Project Switcher
.automakerdirectory structure is initialized before switching projectsFiles Changed (46 files, ~3,651 insertions / ~310 deletions)
apps/server/src/routes/github/list-pr-review-comments.ts,resolve-pr-comment.ts, route registrationapps/server/src/routes/worktree/routes/generate-pr-description.ts,generate-commit-message.tsapps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx(1,095 lines)apps/ui/src/components/views/project-settings-view/commands-and-scripts-section.tsx, navigation configapps/ui/src/components/views/github-prs-view.tsxapps/ui/src/components/views/github-issues-view.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/hooks/queries/use-github.tsuseGitHubPRReviewCommentshookapps/ui/src/hooks/mutations/use-github-mutations.tsuseResolveReviewThreadmutationapps/ui/src/lib/electron.tsPRReviewCommenttype,getPRReviewComments,resolveReviewThreadAPIapps/ui/src/lib/query-keys.tsprReviewCommentsquery keyTest Plan
PRCommentResolutionDialogloads comments with correct inline/general classificationSummary by CodeRabbit
New Features
Improvements
UI/UX