Feature: Worktree View Customization#804
Feature: Worktree View Customization#804gsxdsm wants to merge 15 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
📝 WalkthroughWalkthroughVersion bump to 0.15.0; adds Claude prompt controls and tool presets, provider session resume (sdkSessionId), agent/pipeline threading for prompt/settings/reasoning, new push/sync/set-tracking services and routes, event-hook feature completion/status handling, worktree pinned UI and sync actions, inline & CodeMirror diffs, feature templates/quick-add, many store/type additions, and assorted UI/UX refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Browser UI
participant Store as App Store
participant Server as Backend
participant Hooks as Event Hook Service
User->>UI: Trigger feature update or completion
UI->>Server: POST /features/update (includes executionMode?)
Server->>Server: persist change, compute status
Server->>Hooks: emit "feature:completed" or "feature_status_changed"
Hooks->>Hooks: dedupe, resolve featureName (may call FeatureLoader)
Hooks->>Server: trigger configured hooks (feature_success / feature_error)
Server->>UI: respond with updated feature
UI->>Store: invalidate/refresh queries, re-render
sequenceDiagram
participant User
participant UI as Worktree Panel
participant Store as App Store
participant Server as Backend
participant GitSvc as Sync/Push Service
User->>UI: Request Sync / Set Tracking on worktree
UI->>Server: POST /api/worktree/sync or /set-tracking
Server->>GitSvc: performSync (pull then push) / setTracking (git branch --set-upstream-to)
GitSvc->>Server: return structured SyncResult / tracking result
Server->>UI: return status (pulled, pushed, diverged, conflicts)
UI->>Store: update isSyncing, remotesWithBranch, show toasts
(Note: rectangles/colors omitted per sequence simplicity requirements.) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by introducing customizable worktree views, improving the file editor with inline Git diffs and direct feature creation from code selections, and upgrading the diff viewer to a more robust CodeMirror implementation. It also refines the AI enhancement workflow to better distinguish between content rewriting and additive suggestions, alongside crucial server-side updates for event handling and expanded AI tooling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new features and improvements across the application. Key changes include adding new AI agent tools (MultiEdit, LS, Task, Skill), refining event handling for feature completions to distinguish between 'auto' and 'manual' execution modes, and a major overhaul of the UI's worktree management to support 'pinned worktrees' as tabs with a 'swap worktree' functionality. The file editor now features inline Git diff highlighting and the ability to create new features directly from selected code. Additionally, AI enhancement modes for feature descriptions are categorized into 'rewrite' and 'additive' types, with additive modes appending content to the original description. The CodeMirror editor received updates for better performance in diff rendering and selection change detection, as noted in review comments regarding efficient iteration for decorations and preventing redundant re-renders for selection state changes.
| for (let i = 1; i <= doc.lines; i++) { | ||
| if (addedLines.has(i)) { | ||
| const linePos = doc.line(i).from; | ||
| builder.add(linePos, linePos, addedLineDecoration); | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterating over every line in the document to apply decorations can be a performance bottleneck for large files, especially since this callback runs on every view update (scrolling, typing, etc.). Since addedLines is a Set and is already populated in order during parsing, it is much more efficient to iterate over the set of added lines directly.
| for (let i = 1; i <= doc.lines; i++) { | |
| if (addedLines.has(i)) { | |
| const linePos = doc.line(i).from; | |
| builder.add(linePos, linePos, addedLineDecoration); | |
| } | |
| } | |
| for (const lineNum of addedLines) { | |
| if (lineNum <= doc.lines) { | |
| const linePos = doc.line(lineNum).from; | |
| builder.add(linePos, linePos, addedLineDecoration); | |
| } | |
| } |
| const { from, to } = update.state.selection.main; | ||
| onSelectionChangeRef.current(from !== to); |
There was a problem hiding this comment.
The onSelectionChange callback is currently triggered on every selection update, even if the 'has selection' state hasn't changed. This can cause redundant re-renders in the parent FileEditorView. It's better to track the previous state and only notify the parent when the selection state actually toggles between empty and non-empty.
if (onSelectionChangeRef.current) {
const { from, to } = update.state.selection.main;
const hasSelection = from !== to;
if (hasSelection !== lastHasSelectionRef.current) {
lastHasSelectionRef.current = hasSelection;
onSelectionChangeRef.current(hasSelection);
}
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/board-view/shared/enhancement/enhancement-constants.ts (1)
2-2: 🛠️ Refactor suggestion | 🟠 MajorRemove the local
EnhancementModetype definition and import it from@automaker/types.This file redefines
EnhancementModelocally instead of importing from the shared@automaker/typespackage, where the canonical definition exists. Other files likelibs/prompts/src/enhancement.tscorrectly import this type from the shared package. Importing from shared packages ensures consistency and prevents sync issues if the type is updated.Current code
export type EnhancementMode = 'improve' | 'technical' | 'simplify' | 'acceptance' | 'ux-reviewer';Replace with an import from
@automaker/typesand keep only the local constants (ENHANCEMENT_MODE_LABELS).🤖 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/shared/enhancement/enhancement-constants.ts` at line 2, Remove the local EnhancementMode type declaration and import EnhancementMode from `@automaker/types` instead; update the top of the file to import { EnhancementMode } from '@automaker/types', delete the line defining export type EnhancementMode = 'improve' | 'technical' | 'simplify' | 'acceptance' | 'ux-reviewer'; and leave the existing ENHANCEMENT_MODE_LABELS constant (and any other local constants) unchanged so they use the imported EnhancementMode type.apps/server/src/lib/sdk-options.ts (1)
132-163:⚠️ Potential issue | 🟡 MinorTask and Skill tools are now always available in chat mode, making the conditional gating logic in agent-service.ts (lines 491–510) a no-op.
The test at
sdk-options.test.ts:36explicitly expectsTOOL_PRESETS.chatto equalTOOL_PRESETS.fullAccess, confirming this is intentional. However, this means that with Task and Skill now inTOOL_PRESETS.chat, the conditions at lines 494 and 498 inagent-service.ts(!allowedTools.includes('Skill')and!allowedTools.includes('Task')) will always be false. The conditional additions cannot execute because the tools are already present from the preset.Clarify whether this is the intended behavior: if Task and Skill should always be available in chat (matching auto-mode's fullAccess), this is fine and the conditional logic can be simplified. If they should be gated behind settings, they should remain outside
TOOL_PRESETS.chatand only be added conditionally as currently attempted inagent-service.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/sdk-options.ts` around lines 132 - 163, TOOL_PRESETS.chat currently contains 'Task' and 'Skill', which makes the checks !allowedTools.includes('Skill') and !allowedTools.includes('Task') in the agent-service conditional logic redundant; decide which behavior you want and implement it: either remove 'Task' and 'Skill' from TOOL_PRESETS.chat (so the agent-service code that conditionally pushes Task/Skill into allowedTools will actually gate them) or remove the redundant conditional additions in agent-service (and simplify the logic since TOOL_PRESETS.chat already matches TOOL_PRESETS.fullAccess); update the related test expectations in sdk-options.test.ts accordingly to reflect the chosen behavior.
♻️ Duplicate comments (1)
libs/prompts/src/enhancement.ts (1)
131-132: DuplicateADDITIVE_MODESdefinition — already flagged.This is the second copy of the
['technical', 'acceptance', 'ux-reviewer']list (the UI-side copy is inenhancement-constants.ts). See the earlier comment on that file for the consolidation suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prompts/src/enhancement.ts` around lines 131 - 132, ADDITIVE_MODES is duplicated; remove the hardcoded array here and instead import and re-export (or reference) the single source of truth from the existing enhancement-constants module so both UI and libs use the same constant; update the current file to import the array (ensuring its type is EnhancementMode[]) and delete the local ADDITIVE_MODES declaration to avoid divergence.
🧹 Nitpick comments (12)
apps/ui/src/components/views/board-view/shared/enhancement/enhance-with-ai.tsx (1)
88-97: Repeated additive enhancements will stack content — is this intentional?If a user applies an additive mode (e.g., "Add Technical Details"), the result is
original + AI content. If they then apply another additive mode (e.g., "Add Acceptance Criteria"),valuenow includes the prior AI output, so the new result becomesoriginal + prior AI content + new AI content. Over multiple enhancements the description grows monotonically.This may be the desired behavior (cumulative enrichment), but it could also surprise users. The
onHistoryAddcallback (Line 97) stores the pre-enhancementoriginalText, so undo is possible. Just flagging 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/board-view/shared/enhancement/enhance-with-ai.tsx` around lines 88 - 97, The current additive branch in enhance-with-ai.tsx (isAdditiveMode + composing enhancedText from value and result.enhancedText) causes repeated additive enhancements to stack AI outputs; decide whether to prevent stacking or keep it and implement accordingly. To prevent stacking, change the composition for isAdditiveMode to use a preserved baseline/original source (e.g., a new baseText prop or an initialValue captured when the component mounts) instead of the mutable value variable so enhancedText = `${baseText.trim()}\n\n${result.enhancedText.trim()}`; alternatively, add an explicit option/flag (e.g., applyToLatest boolean) to control stacking behavior and respect it when computing enhancedText. Update the logic around isAdditiveMode, enhancementMode, onChange, and onHistoryAdd to use the chosen source (baseText vs value) when building and storing originalText so undo/history remain consistent.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx (1)
1178-1207: Swap Worktree submenu: placement after destructive actions.The "Swap Worktree" submenu is rendered after the "Delete Worktree" destructive item (line 1170). Conventionally, non-destructive navigation/swap actions appear before destructive ones. Consider moving this block above the destructive section (before line 1148) to follow standard dropdown ordering and reduce risk of accidental deletion when the user intended to swap.
🤖 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 1178 - 1207, Move the "Swap Worktree" DropdownMenuSub block so it appears before the destructive menu items (the Delete Worktree item) instead of after them; keep the existing conditional checks (worktree.isMain, availableWorktreesForSwap, slotIndex, onSwapWorktree) and preserve the mapping/filtering logic, keys (wt.path), onSelect handler (onSwapWorktree(slotIndex, wt.branch)), and pinnedBranches check — only change the placement in the JSX so navigation/swap actions render above destructive actions.apps/ui/src/components/ui/codemirror-diff-view.tsx (1)
20-46:syntaxColorsis duplicated between this file andcode-editor.tsx.This identical
HighlightStyle.define(...)block also exists incode-editor.tsx(lines 368–394). Consider extracting it to a shared module (e.g.,@/lib/codemirror-theme.ts) alongside thesyntaxHighlightingcall, so both the editor and diff viewer stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/codemirror-diff-view.tsx` around lines 20 - 46, The syntaxColors HighlightStyle.define(...) block is duplicated (used in this file and in code-editor.tsx); extract it and the associated syntaxHighlighting call into a shared module (e.g., a new codemirror theme export) and replace the local const syntaxColors in both codemirror-diff-view.tsx and code-editor.tsx with an import from that module; ensure the shared module exports the HighlightStyle instance (syntaxColors) and any helper like syntaxHighlighting so both components import and use the same symbol to stay in sync.apps/ui/src/lib/diff-utils.ts (1)
134-208: Good extraction of shared diff utilities.The
reconstructFilesFromDifffunction handles single-file diffs well. One thing to note: for diffs with multiple non-adjacent hunks, lines between hunks (the gap) are not reconstructed — botholdContentandnewContentwill be missing those lines. This is acceptable for theunifiedMergeViewuse case since both sides lose the same context, and the merge view'scollapseUnchangedhides them anyway, but worth documenting for future callers who might expect full file reconstruction.📝 Suggested doc addition
/** * Reconstruct old (original) and new (modified) file content from a single-file * unified diff string. Used by the CodeMirror merge diff viewer which needs * both document versions to compute inline highlighting. * * For new files (entire content is additions), oldContent will be empty. * For deleted files (entire content is deletions), newContent will be empty. + * + * Note: only hunk-covered lines are included; lines between non-adjacent + * hunks are omitted from both sides. This is fine for diff visualisation + * but callers should not assume a full file reconstruction. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/diff-utils.ts` around lines 134 - 208, The function reconstructFilesFromDiff does not reconstruct gaps between multiple non-adjacent hunks (lines between hunks are omitted from both oldContent and newContent); update the function's JSDoc comment (above reconstructFilesFromDiff) to explicitly document this limitation, stating that non-adjacent-hunk gaps are not preserved, that this is acceptable for unifiedMergeView which uses collapseUnchanged, and advising callers that if they need full-file reconstruction they must use a different parser or provide the full original/new files; reference the existing function name reconstructFilesFromDiff and keep the note concise and visible to future callers.apps/ui/src/components/ui/git-diff-panel.tsx (1)
339-343: Verify thatrawDiffis always available when a file has parsed hunks.The guard
isExpanded && rawDiffsilently hides the diff content if thefileDiffMaplookup returnsundefined. SinceparseDiffandsplitDiffByFileuse the same regex for file path extraction, the keys should always match, but if they ever diverge (e.g., a renamed file path mismatch), an expanded file would show an empty body with no user-visible indication.Consider adding a small fallback message when expanded but
rawDiffis missing:📝 Optional: add fallback
{isExpanded && rawDiff && ( <div className="bg-background border-t border-border"> <CodeMirrorDiffView fileDiff={rawDiff} filePath={fileDiff.filePath} maxHeight="400px" /> </div> )} + {isExpanded && !rawDiff && ( + <div className="px-4 py-3 text-sm text-muted-foreground bg-background border-t border-border"> + Diff content not available for rendering + </div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/git-diff-panel.tsx` around lines 339 - 343, The expanded file view currently hides its body when fileDiffMap lookup returns undefined (using the guard isExpanded && rawDiff), which can leave users with an empty panel if parseDiff/splitDiffByFile keys diverge; update the render logic around isExpanded/rawDiff (the conditional that currently renders CodeMirrorDiffView with fileDiff/filePath) to show a small fallback UI (e.g., "Diff unavailable for this file" or "No diff found for filePath") when isExpanded is true but rawDiff is missing, and optionally emit a console.warn or logger entry referencing the fileDiff.filePath to aid debugging; keep CodeMirrorDiffView rendering unchanged when rawDiff exists.apps/ui/src/components/views/file-editor-view/components/code-editor.tsx (1)
286-303: Added-line decorations use static line numbers; will drift if the user edits the document.
EditorView.decorations.of((view) => {...})rebuilds on every update, butaddedLinesis a closed-overSet<number>from the original parse. If the user inserts or deletes lines above a decorated line, the decoration will attach to the wrong line number.This is acceptable if
diffContentis only provided when the editor is read-only or the decorations are considered transient. If it's expected to be used on an editable buffer, consider either:
- Forcing
readOnlywhendiffContentis set, or- Documenting that decorations are best-effort after edits.
🤖 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/code-editor.tsx` around lines 286 - 303, The decorations use static line numbers from addedLines in EditorView.decorations.of, so they will drift when the document is edited; to fix, either make the editor read-only whenever diffContent is present (e.g., add the EditorState.readOnly extension or pass readOnly prop when diffContent exists) so the line-number-based decorations remain valid, or implement persistent mappings (convert addedLines numbers into document positions stored and remapped via a StateField/StateEffect that updates positions on document changes); reference addedLines, EditorView.decorations.of, addedLineDecoration and diffContent to locate where to apply the change.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (2)
674-752: Feature creation from editor:as anycast on the API call.Line 726 casts the feature payload with
as any. This bypasses type checking on thefeatures.createcall. If the API shape changes, this won't surface at compile time. The eslint-disable is noted, but consider defining a properCreateFeaturePayloadtype that matches the API contract to avoid silent breakage on API changes.🤖 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 674 - 752, The code is casting the feature payload to any when calling api.features.create inside handleAddFeatureFromEditor, which disables type checking; define a proper CreateFeaturePayload (or reuse the existing API type) matching the API contract and replace the as any cast by typing the feature variable (or the api.features.create parameter) to CreateFeaturePayload so TypeScript validates the payload shape before the call to api.features.create; update imports/types and remove the eslint-disable comment after ensuring the payload matches the API.
649-668: Truncation marker// ...is JavaScript-specific but applied to all file types.When the selection exceeds 200 lines, the truncation indicator is
'\n// ...'(Line 654). For non-JS/TS files (Python, Ruby, shell, etc.), this would be syntactically incorrect in the code fence. Consider using the language's comment syntax or a language-neutral indicator like…or[truncated].Example: language-neutral truncation
- const codeText = truncated ? lines.slice(0, 200).join('\n') + '\n// ...' : selection.text; + const codeText = truncated ? lines.slice(0, 200).join('\n') + '\n[...]' : selection.text;🤖 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 649 - 668, The truncation marker currently uses a hardcoded JavaScript comment ('\n// ...') regardless of file type; update the truncation logic in the block that computes codeText (referencing fenceLang, langMap, and selection) to emit either a language-appropriate comment prefix based on fenceLang (e.g., '#' for Python/shell, '//' for C-like languages, '—' or other mapping) or switch to a language-neutral marker like '\n…' or '\n[truncated]' so the marker is syntactically safe inside the fenced code block; implement a small helper (e.g., getTruncationMarker(fenceLang)) and use it when building codeText and the trailing truncated message.apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx (1)
111-181: Scroll arrows are always visible even when tabs don't overflow.The left/right chevron buttons are unconditionally rendered regardless of whether the tab container is scrollable. On a small number of tabs, they consume space without function. Consider hiding them (or reducing opacity) when the scroll container doesn't overflow, using a resize/scroll observer to track overflow state. This is a minor polish item.
🤖 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 111 - 181, The left/right chevron buttons (rendered near data-testid="editor-tabs") are always visible even when the scroll container (ref: scrollRef) doesn't overflow; add an overflow state (e.g., isOverflowing) and a ResizeObserver/scroll handler in the component to compute whether scrollRef.current.scrollWidth > scrollRef.current.clientWidth (update when size or content changes and when activeTabRef changes), then conditionally render or apply reduced opacity/hidden classes to the chevron buttons (the ones that call scrollBy('left'|'right')). Ensure the observer is wired up in a useEffect, updates the state, and is cleaned up on unmount.apps/server/src/routes/features/routes/update.ts (1)
60-70: Consider merging the duplicate condition check.Lines 60 and 73 evaluate the identical condition (
newStatus && SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus). While the two blocks perform different actions, a singleifblock would reduce duplication and ensure both actions always stay in sync.♻️ Suggested consolidation
- if (newStatus && SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus) { - events?.emit('feature:completed', { - featureId, - featureName: updated.title, - projectPath, - passes: true, - message: - newStatus === 'verified' ? 'Feature verified manually' : 'Feature completed manually', - executionMode: 'manual', - }); - } - - // Trigger sync to app_spec.txt when status changes to verified or completed - if (newStatus && SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus) { + // Emit completion event and sync to app_spec.txt when status transitions to verified/completed + if (newStatus && SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus) { + events?.emit('feature:completed', { + featureId, + featureName: updated.title, + projectPath, + passes: true, + message: + newStatus === 'verified' ? 'Feature verified manually' : 'Feature completed manually', + executionMode: 'manual', + }); + try {Also applies to: 73-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/features/routes/update.ts` around lines 60 - 70, The condition newStatus && SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus is duplicated; consolidate by evaluating it once and executing both side-effects (the events?.emit('feature:completed', {...}) call and the other action currently inside the second identical if block at 73-85) inside that single branch, or extract the predicate into a helper like shouldTriggerSync(newStatus, previousStatus) and call it from one if, then run both the emit and the second block’s logic so they remain in sync.apps/server/src/services/event-hook-service.ts (1)
218-228: Extract duplicated feature-name loading into a private helper.The
featureLoader.get()try/catch block inhandleFeatureCompletedEventis essentially identical to the one inhandleAutoModeEvent(lines 178–187). Extracting it eliminates the duplication and keeps the two handlers consistent.♻️ Proposed extraction
+ private async loadFeatureName(projectPath: string, featureId: string): Promise<string | undefined> { + if (!this.featureLoader) return undefined; + try { + const feature = await this.featureLoader.get(projectPath, featureId); + return feature?.title ?? undefined; + } catch (error) { + logger.warn(`Failed to load feature ${featureId} for event hook:`, error); + return undefined; + } + }Then replace both loading blocks:
- let featureName: string | undefined = undefined; - if (payload.featureId && payload.projectPath && this.featureLoader) { - try { - const feature = await this.featureLoader.get(payload.projectPath, payload.featureId); - if (feature?.title) { - featureName = feature.title; - } - } catch (error) { - logger.warn(`Failed to load feature ${payload.featureId} for event hook:`, error); - } - } + const featureName = await this.loadFeatureName(payload.projectPath, payload.featureId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/event-hook-service.ts` around lines 218 - 228, Extract the duplicated try/catch block that loads a feature title into a private helper method (e.g., a private async loadFeatureName(projectPath: string | undefined, featureId: string | undefined): Promise<string | undefined>) and use it from both handleFeatureCompletedEvent and handleAutoModeEvent; the helper should check this.featureLoader, call this.featureLoader.get(projectPath, featureId) inside the same try/catch, set/return the feature title or undefined, and log failures with the same logger.warn message that currently references payload.featureId so the handlers simply await loadFeatureName(...) and assign its result to featureName.libs/types/src/settings.ts (1)
272-281: Prefer capability lookup over string matching for adaptive defaults.
Now that the default is derived from model capabilities, consider sourcing “adaptive thinking” from per-model capability flags in model definitions instead of string heuristics. Based on learnings: “When modeling AI capabilities, add per-model flags to model definitions … and check capabilities by model ID rather than assuming all models from a provider share the same features.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/settings.ts` around lines 272 - 281, The current getDefaultThinkingLevel uses string heuristics via isAdaptiveThinkingModel; instead, update it to consult per-model capability flags on the model definitions (e.g., a hasAdaptiveThinking or capabilities.adaptive boolean stored with each model entry) by looking up the model by ID and returning 'adaptive' when that capability is true, otherwise 'none'. Locate getDefaultThinkingLevel and replace the isAdaptiveThinkingModel check with a capabilities lookup against the model registry (or models map) and fall back to 'none' if the model entry or flag is missing; ensure you reference the model ID parameter when performing the lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ui/src/components/views/board-view/shared/enhancement/enhancement-constants.ts`:
- Around line 22-31: Replace the locally defined EnhancementMode with the
canonical type from `@automaker/types` by importing EnhancementMode and removing
the local type declaration, then update REWRITE_MODES and isAdditiveMode to use
that imported type; next, move the ADDITIVE_MODES constant out of this module
into the shared package (e.g., export it from `@automaker/types` or a shared
constants package) and import ADDITIVE_MODES in both this file and
libs/prompts/src/enhancement.ts so the constant is defined in one place and
reused.
In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx`:
- Line 1448: Pinned-tab rendering uses handleStartDevServer instead of
handleStartDevServerAndShowLogs, causing logs not to open for pinned tabs;
update the pinned-tabs JSX where onStartDevServer is passed (in worktree-panel
rendering) to use handleStartDevServerAndShowLogs so it matches the dropdown and
main-only tab paths, ensuring the logs panel auto-opens when starting the dev
server from a pinned tab.
- Around line 197-217: The handler handleSwapWorktreeSlot contains unused
"padding" logic that builds currentPinned from
pinnedWorktreeBranches/pinnedWorktreesCount/worktrees but never uses it; remove
the whole block that constructs/pads currentPinned (including references to
pinnedWorktreeBranches, pinnedWorktreesCount, worktrees, mainWt, otherWts,
usedBranches, available) and leave the function to simply call
swapPinnedWorktreeBranch(projectPath, slotIndex, newBranch). Also update the
useCallback dependency array to include only projectPath and
swapPinnedWorktreeBranch (remove pinnedWorktreeBranches, pinnedWorktreesCount,
worktrees). Ensure the adjacent useEffect that initializes pinned arrays remains
unchanged.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 143-148: The file color dot is invisible because the non-dirty
span uses text-* classes from getFileColor (no text content), so change the
indicator to use background color classes: update getFileColor (or add
getFileBgColor) to return bg-* class names and apply that value to the non-dirty
span in editor-tabs.tsx (and update the dirty case that currently uses Circle to
use the same bg-* variant if desired), or map the returned text-* token to its
bg-* counterpart inline when rendering the span; ensure you reference
getFileColor and the tab.isDirty branch in editor-tabs.tsx when making the
change.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 68-74: The exported DiffLine interface is unused and should be
removed from the store; delete the DiffLine declaration (the interface named
DiffLine with fields type, content, oldLineNumber, newLineNumber) from
use-file-editor-store.ts and ensure any other exported types (e.g.,
MarkdownViewMode, GitFileDetailsInfo, EditorTab) remain unchanged so consumers
are unaffected.
In `@apps/ui/src/store/app-store.ts`:
- Line 2551: The getters getPinnedWorktreesCount and
getAlwaysUseWorktreeDropdown return fallbacks that conflict with the documented
defaults in state-types.ts; locate getPinnedWorktreesCount (uses
pinnedWorktreesCountByProject) and getAlwaysUseWorktreeDropdown (uses
alwaysUseWorktreeDropdownByProject) and change their fallback values to match
the documented defaults: return 1 for getPinnedWorktreesCount when no project
entry exists, and return false for getAlwaysUseWorktreeDropdown when no project
entry exists (or alternatively update the state-types.ts comments if the code is
correct—ensure code and comment agree).
- Around line 2561-2577: swapPinnedWorktreeBranch currently writes empty strings
and leaves sparse holes when slotIndex >= current.length; make current dense and
avoid writing '' by expanding current to length slotIndex+1 (e.g., clone current
and push nulls until length > slotIndex) before any assignments, capture
oldBranch = current[slotIndex] (which will be null after expansion) and only use
that value when swapping into existingIndex (assign existingIndex = oldBranch
even if null, but do not substitute ''), then set current[slotIndex] = newBranch
and write the dense array back into state.pinnedWorktreeBranchesByProject;
reference the swapPinnedWorktreeBranch function and the
current/pinnedWorktreeBranchesByProject variables when applying the change.
---
Outside diff comments:
In `@apps/server/src/lib/sdk-options.ts`:
- Around line 132-163: TOOL_PRESETS.chat currently contains 'Task' and 'Skill',
which makes the checks !allowedTools.includes('Skill') and
!allowedTools.includes('Task') in the agent-service conditional logic redundant;
decide which behavior you want and implement it: either remove 'Task' and
'Skill' from TOOL_PRESETS.chat (so the agent-service code that conditionally
pushes Task/Skill into allowedTools will actually gate them) or remove the
redundant conditional additions in agent-service (and simplify the logic since
TOOL_PRESETS.chat already matches TOOL_PRESETS.fullAccess); update the related
test expectations in sdk-options.test.ts accordingly to reflect the chosen
behavior.
In
`@apps/ui/src/components/views/board-view/shared/enhancement/enhancement-constants.ts`:
- Line 2: Remove the local EnhancementMode type declaration and import
EnhancementMode from `@automaker/types` instead; update the top of the file to
import { EnhancementMode } from '@automaker/types', delete the line defining
export type EnhancementMode = 'improve' | 'technical' | 'simplify' |
'acceptance' | 'ux-reviewer'; and leave the existing ENHANCEMENT_MODE_LABELS
constant (and any other local constants) unchanged so they use the imported
EnhancementMode type.
---
Duplicate comments:
In `@libs/prompts/src/enhancement.ts`:
- Around line 131-132: ADDITIVE_MODES is duplicated; remove the hardcoded array
here and instead import and re-export (or reference) the single source of truth
from the existing enhancement-constants module so both UI and libs use the same
constant; update the current file to import the array (ensuring its type is
EnhancementMode[]) and delete the local ADDITIVE_MODES declaration to avoid
divergence.
---
Nitpick comments:
In `@apps/server/src/routes/features/routes/update.ts`:
- Around line 60-70: The condition newStatus &&
SYNC_TRIGGER_STATUSES.includes(newStatus) && previousStatus !== newStatus is
duplicated; consolidate by evaluating it once and executing both side-effects
(the events?.emit('feature:completed', {...}) call and the other action
currently inside the second identical if block at 73-85) inside that single
branch, or extract the predicate into a helper like shouldTriggerSync(newStatus,
previousStatus) and call it from one if, then run both the emit and the second
block’s logic so they remain in sync.
In `@apps/server/src/services/event-hook-service.ts`:
- Around line 218-228: Extract the duplicated try/catch block that loads a
feature title into a private helper method (e.g., a private async
loadFeatureName(projectPath: string | undefined, featureId: string | undefined):
Promise<string | undefined>) and use it from both handleFeatureCompletedEvent
and handleAutoModeEvent; the helper should check this.featureLoader, call
this.featureLoader.get(projectPath, featureId) inside the same try/catch,
set/return the feature title or undefined, and log failures with the same
logger.warn message that currently references payload.featureId so the handlers
simply await loadFeatureName(...) and assign its result to featureName.
In `@apps/ui/src/components/ui/codemirror-diff-view.tsx`:
- Around line 20-46: The syntaxColors HighlightStyle.define(...) block is
duplicated (used in this file and in code-editor.tsx); extract it and the
associated syntaxHighlighting call into a shared module (e.g., a new codemirror
theme export) and replace the local const syntaxColors in both
codemirror-diff-view.tsx and code-editor.tsx with an import from that module;
ensure the shared module exports the HighlightStyle instance (syntaxColors) and
any helper like syntaxHighlighting so both components import and use the same
symbol to stay in sync.
In `@apps/ui/src/components/ui/git-diff-panel.tsx`:
- Around line 339-343: The expanded file view currently hides its body when
fileDiffMap lookup returns undefined (using the guard isExpanded && rawDiff),
which can leave users with an empty panel if parseDiff/splitDiffByFile keys
diverge; update the render logic around isExpanded/rawDiff (the conditional that
currently renders CodeMirrorDiffView with fileDiff/filePath) to show a small
fallback UI (e.g., "Diff unavailable for this file" or "No diff found for
filePath") when isExpanded is true but rawDiff is missing, and optionally emit a
console.warn or logger entry referencing the fileDiff.filePath to aid debugging;
keep CodeMirrorDiffView rendering unchanged when rawDiff exists.
In
`@apps/ui/src/components/views/board-view/shared/enhancement/enhance-with-ai.tsx`:
- Around line 88-97: The current additive branch in enhance-with-ai.tsx
(isAdditiveMode + composing enhancedText from value and result.enhancedText)
causes repeated additive enhancements to stack AI outputs; decide whether to
prevent stacking or keep it and implement accordingly. To prevent stacking,
change the composition for isAdditiveMode to use a preserved baseline/original
source (e.g., a new baseText prop or an initialValue captured when the component
mounts) instead of the mutable value variable so enhancedText =
`${baseText.trim()}\n\n${result.enhancedText.trim()}`; alternatively, add an
explicit option/flag (e.g., applyToLatest boolean) to control stacking behavior
and respect it when computing enhancedText. Update the logic around
isAdditiveMode, enhancementMode, onChange, and onHistoryAdd to use the chosen
source (baseText vs value) when building and storing originalText so
undo/history remain consistent.
In
`@apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx`:
- Around line 1178-1207: Move the "Swap Worktree" DropdownMenuSub block so it
appears before the destructive menu items (the Delete Worktree item) instead of
after them; keep the existing conditional checks (worktree.isMain,
availableWorktreesForSwap, slotIndex, onSwapWorktree) and preserve the
mapping/filtering logic, keys (wt.path), onSelect handler
(onSwapWorktree(slotIndex, wt.branch)), and pinnedBranches check — only change
the placement in the JSX so navigation/swap actions render above destructive
actions.
In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx`:
- Around line 286-303: The decorations use static line numbers from addedLines
in EditorView.decorations.of, so they will drift when the document is edited; to
fix, either make the editor read-only whenever diffContent is present (e.g., add
the EditorState.readOnly extension or pass readOnly prop when diffContent
exists) so the line-number-based decorations remain valid, or implement
persistent mappings (convert addedLines numbers into document positions stored
and remapped via a StateField/StateEffect that updates positions on document
changes); reference addedLines, EditorView.decorations.of, addedLineDecoration
and diffContent to locate where to apply the change.
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 111-181: The left/right chevron buttons (rendered near
data-testid="editor-tabs") are always visible even when the scroll container
(ref: scrollRef) doesn't overflow; add an overflow state (e.g., isOverflowing)
and a ResizeObserver/scroll handler in the component to compute whether
scrollRef.current.scrollWidth > scrollRef.current.clientWidth (update when size
or content changes and when activeTabRef changes), then conditionally render or
apply reduced opacity/hidden classes to the chevron buttons (the ones that call
scrollBy('left'|'right')). Ensure the observer is wired up in a useEffect,
updates the state, and is cleaned up on unmount.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 674-752: The code is casting the feature payload to any when
calling api.features.create inside handleAddFeatureFromEditor, which disables
type checking; define a proper CreateFeaturePayload (or reuse the existing API
type) matching the API contract and replace the as any cast by typing the
feature variable (or the api.features.create parameter) to CreateFeaturePayload
so TypeScript validates the payload shape before the call to
api.features.create; update imports/types and remove the eslint-disable comment
after ensuring the payload matches the API.
- Around line 649-668: The truncation marker currently uses a hardcoded
JavaScript comment ('\n// ...') regardless of file type; update the truncation
logic in the block that computes codeText (referencing fenceLang, langMap, and
selection) to emit either a language-appropriate comment prefix based on
fenceLang (e.g., '#' for Python/shell, '//' for C-like languages, '—' or other
mapping) or switch to a language-neutral marker like '\n…' or '\n[truncated]' so
the marker is syntactically safe inside the fenced code block; implement a small
helper (e.g., getTruncationMarker(fenceLang)) and use it when building codeText
and the trailing truncated message.
In `@apps/ui/src/lib/diff-utils.ts`:
- Around line 134-208: The function reconstructFilesFromDiff does not
reconstruct gaps between multiple non-adjacent hunks (lines between hunks are
omitted from both oldContent and newContent); update the function's JSDoc
comment (above reconstructFilesFromDiff) to explicitly document this limitation,
stating that non-adjacent-hunk gaps are not preserved, that this is acceptable
for unifiedMergeView which uses collapseUnchanged, and advising callers that if
they need full-file reconstruction they must use a different parser or provide
the full original/new files; reference the existing function name
reconstructFilesFromDiff and keep the note concise and visible to future
callers.
In `@libs/types/src/settings.ts`:
- Around line 272-281: The current getDefaultThinkingLevel uses string
heuristics via isAdaptiveThinkingModel; instead, update it to consult per-model
capability flags on the model definitions (e.g., a hasAdaptiveThinking or
capabilities.adaptive boolean stored with each model entry) by looking up the
model by ID and returning 'adaptive' when that capability is true, otherwise
'none'. Locate getDefaultThinkingLevel and replace the isAdaptiveThinkingModel
check with a capabilities lookup against the model registry (or models map) and
fall back to 'none' if the model entry or flag is missing; ensure you reference
the model ID parameter when performing the lookup.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
apps/server/package.jsonapps/server/src/lib/sdk-options.tsapps/server/src/providers/codex-provider.tsapps/server/src/routes/features/index.tsapps/server/src/routes/features/routes/update.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/event-hook-service.tsapps/server/src/services/execution-service.tsapps/server/src/services/pipeline-orchestrator.tsapps/server/tests/unit/services/event-hook-service.test.tsapps/ui/package.jsonapps/ui/src/components/ui/codemirror-diff-view.tsxapps/ui/src/components/ui/git-diff-panel.tsxapps/ui/src/components/views/board-view/shared/enhancement/enhance-with-ai.tsxapps/ui/src/components/views/board-view/shared/enhancement/enhancement-constants.tsapps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-dropdown.tsxapps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsxapps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsxapps/ui/src/components/views/file-editor-view/components/code-editor.tsxapps/ui/src/components/views/file-editor-view/components/editor-tabs.tsxapps/ui/src/components/views/file-editor-view/components/file-tree.tsxapps/ui/src/components/views/file-editor-view/file-editor-view.tsxapps/ui/src/components/views/file-editor-view/use-file-editor-store.tsapps/ui/src/components/views/project-settings-view/worktree-preferences-section.tsxapps/ui/src/hooks/use-project-settings-loader.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/codemirror-languages.tsapps/ui/src/lib/diff-utils.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tsapps/ui/src/store/types/state-types.tslibs/prompts/src/enhancement-modes/acceptance.tslibs/prompts/src/enhancement-modes/technical.tslibs/prompts/src/enhancement-modes/ux-reviewer.tslibs/prompts/src/enhancement.tslibs/types/src/settings.tspackage.json
| /** Modes that rewrite/replace the entire description */ | ||
| export const REWRITE_MODES: EnhancementMode[] = ['improve', 'simplify']; | ||
|
|
||
| /** Modes that append additional content below the original description */ | ||
| export const ADDITIVE_MODES: EnhancementMode[] = ['technical', 'acceptance', 'ux-reviewer']; | ||
|
|
||
| /** Check if a mode appends content rather than replacing */ | ||
| export function isAdditiveMode(mode: EnhancementMode): boolean { | ||
| return ADDITIVE_MODES.includes(mode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, verify the file structure and find the mentioned files
git ls-files | grep -E "(enhancement-constants|enhancement\.ts|types)" | head -20Repository: AutoMaker-Org/automaker
Length of output: 1030
🏁 Script executed:
# Check ADDITIVE_MODES occurrences across the codebase
rg -n "ADDITIVE_MODES" --type=tsRepository: AutoMaker-Org/automaker
Length of output: 812
🏁 Script executed:
# Check the specific file mentioned in the review
cat -n apps/ui/src/components/views/board-view/shared/enhancement/enhancement-constants.ts | head -40Repository: AutoMaker-Org/automaker
Length of output: 1685
🏁 Script executed:
# Check the prompts library file
cat -n libs/prompts/src/enhancement.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 5740
🏁 Script executed:
# Check if `@automaker/types` exports EnhancementMode
fd "types" --type=d | xargs -I {} find {} -name "*.ts" -o -name "*.d.ts" 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 739
🏁 Script executed:
# Search for EnhancementMode type definition/export
rg -n "EnhancementMode" --type=ts -B2 -A2Repository: AutoMaker-Org/automaker
Length of output: 30722
Import EnhancementMode from @automaker/types instead of defining it locally.
The UI currently defines its own EnhancementMode type at line 2, violating the coding guideline that requires importing from shared packages (@automaker/*). The canonical EnhancementMode type is already exported from @automaker/types and correctly used by @automaker/prompts.
Additionally, ADDITIVE_MODES is duplicated between apps/ui/src/components/views/board-view/shared/enhancement/enhancement-constants.ts (line 26) and libs/prompts/src/enhancement.ts (line 132). Extract this constant to a shared location (e.g., @automaker/types or a shared constants package) and import it in both places to avoid the maintenance burden of keeping them in sync.
🤖 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/shared/enhancement/enhancement-constants.ts`
around lines 22 - 31, Replace the locally defined EnhancementMode with the
canonical type from `@automaker/types` by importing EnhancementMode and removing
the local type declaration, then update REWRITE_MODES and isAdditiveMode to use
that imported type; next, move the ADDITIVE_MODES constant out of this module
into the shared package (e.g., export it from `@automaker/types` or a shared
constants package) and import ADDITIVE_MODES in both this file and
libs/prompts/src/enhancement.ts so the constant is defined in one place and
reused.
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx
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/use-file-editor-store.ts
Outdated
Show resolved
Hide resolved
| swapPinnedWorktreeBranch: (projectPath, slotIndex, newBranch) => | ||
| set((state) => { | ||
| const current = [...(state.pinnedWorktreeBranchesByProject[projectPath] ?? [])]; | ||
| // If the new branch is already in another slot, swap them | ||
| const existingIndex = current.indexOf(newBranch); | ||
| if (existingIndex !== -1 && existingIndex !== slotIndex) { | ||
| // Swap: put the old branch from this slot into the other slot | ||
| current[existingIndex] = current[slotIndex] ?? ''; | ||
| } | ||
| current[slotIndex] = newBranch; | ||
| return { | ||
| pinnedWorktreeBranchesByProject: { | ||
| ...state.pinnedWorktreeBranchesByProject, | ||
| [projectPath]: current, | ||
| }, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
swapPinnedWorktreeBranch creates sparse arrays and stores empty strings as branch names
Two related bugs when slotIndex >= current.length:
- Line 2568:
current[slotIndex]isundefined, so?? ''stores an empty string intocurrent[existingIndex]. An empty string is not a valid branch name and will silently break any downstream worktree lookup that uses this list. - Line 2570: Directly assigning to an out-of-bounds index leaves
undefinedholes between existing entries and the new slot (e.g., setting index 4 on a 2-element array creates['a', 'b', undefined, undefined, newBranch]).
🐛 Proposed fix — pre-fill the array to keep it dense
- const current = [...(state.pinnedWorktreeBranchesByProject[projectPath] ?? [])];
+ const src = state.pinnedWorktreeBranchesByProject[projectPath] ?? [];
+ // Pre-fill up to slotIndex to prevent sparse holes
+ const current: string[] = Array.from(
+ { length: Math.max(src.length, slotIndex + 1) },
+ (_, i) => src[i] ?? ''
+ );
const existingIndex = current.indexOf(newBranch);
if (existingIndex !== -1 && existingIndex !== slotIndex) {
- current[existingIndex] = current[slotIndex] ?? '';
+ current[existingIndex] = current[slotIndex]; // always defined after pre-fill
}
current[slotIndex] = newBranch;
📝 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.
| swapPinnedWorktreeBranch: (projectPath, slotIndex, newBranch) => | |
| set((state) => { | |
| const current = [...(state.pinnedWorktreeBranchesByProject[projectPath] ?? [])]; | |
| // If the new branch is already in another slot, swap them | |
| const existingIndex = current.indexOf(newBranch); | |
| if (existingIndex !== -1 && existingIndex !== slotIndex) { | |
| // Swap: put the old branch from this slot into the other slot | |
| current[existingIndex] = current[slotIndex] ?? ''; | |
| } | |
| current[slotIndex] = newBranch; | |
| return { | |
| pinnedWorktreeBranchesByProject: { | |
| ...state.pinnedWorktreeBranchesByProject, | |
| [projectPath]: current, | |
| }, | |
| }; | |
| }), | |
| swapPinnedWorktreeBranch: (projectPath, slotIndex, newBranch) => | |
| set((state) => { | |
| const src = state.pinnedWorktreeBranchesByProject[projectPath] ?? []; | |
| // Pre-fill up to slotIndex to prevent sparse holes | |
| const current: string[] = Array.from( | |
| { length: Math.max(src.length, slotIndex + 1) }, | |
| (_, i) => src[i] ?? '' | |
| ); | |
| // If the new branch is already in another slot, swap them | |
| const existingIndex = current.indexOf(newBranch); | |
| if (existingIndex !== -1 && existingIndex !== slotIndex) { | |
| // Swap: put the old branch from this slot into the other slot | |
| current[existingIndex] = current[slotIndex]; // always defined after pre-fill | |
| } | |
| current[slotIndex] = newBranch; | |
| return { | |
| pinnedWorktreeBranchesByProject: { | |
| ...state.pinnedWorktreeBranchesByProject, | |
| [projectPath]: current, | |
| }, | |
| }; | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/store/app-store.ts` around lines 2561 - 2577,
swapPinnedWorktreeBranch currently writes empty strings and leaves sparse holes
when slotIndex >= current.length; make current dense and avoid writing '' by
expanding current to length slotIndex+1 (e.g., clone current and push nulls
until length > slotIndex) before any assignments, capture oldBranch =
current[slotIndex] (which will be null after expansion) and only use that value
when swapping into existingIndex (assign existingIndex = oldBranch even if null,
but do not substitute ''), then set current[slotIndex] = newBranch and write the
dense array back into state.pinnedWorktreeBranchesByProject; reference the
swapPinnedWorktreeBranch function and the
current/pinnedWorktreeBranchesByProject variables when applying the change.
…#793) * Changes from fix/docker-compose-cors-error * Update apps/server/src/index.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Fix: Delete Worktree Crash + PR Comments + Dev Server UX Improvements (AutoMaker-Org#792) * Changes from fix/delete-worktree-hotifx * fix: Improve bot detection and prevent UI overflow issues - Include GitHub app-initiated comments in bot detection - Wrap handleQuickCreateSession with useCallback to fix dependency issues - Truncate long branch names in agent header to prevent layout overflow * feat: Support GitHub App comments in PR review and fix session filtering * feat: Return invalidation result from delete session handler * fix: Improve CORS origin validation to handle wildcard correctly * fix: Correct IPv6 localhost parsing and improve responsive UI layouts * Changes from fix/pwa-cache-fix (AutoMaker-Org#794) * fix: Add type checking to prevent crashes from malformed cache entries --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…g#797) * Changes from fix/codex-cli-timeout * test: Clarify timeout values and multipliers in codex-provider tests * refactor: Rename useWorktreesEnabled to worktreesEnabled for clarity
* Changes from fix/improve-pull-request-prompt * Update apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Changes from fix/event-hook-persistence * feat: Add explicit permission escape hatch for clearing eventHooks and improve error handling in UI
* Changes from fix/deleting-worktree * fix: Improve worktree deletion safety and branch cleanup logic * fix: Improve error handling and async operations across auto-mode and worktree services * Update apps/server/src/routes/auto-mode/routes/analyze-project.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…eed up some cli models with session resume (AutoMaker-Org#801) * Changes from fix/restoring-view * feat: Add resume query safety checks and optimize store selectors * feat: Improve session management and model normalization * refactor: Extract prompt building logic and handle file path parsing for renames
* Changes from feature/quick-add * feat: Clarify system prompt and improve error handling across services. Address PR Feedback * feat: Improve PR description parsing and refactor event handling * feat: Add context options to pipeline orchestrator initialization * fix: Deduplicate React and handle CJS interop for use-sync-external-store Resolve "Cannot read properties of null (reading 'useState')" errors by deduplicating React/react-dom and ensuring use-sync-external-store is bundled together with React to prevent CJS packages from resolving to different React instances.
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
584-597:⚠️ Potential issue | 🟡 MinorNo cancellation guard for the in-flight AI generation request.
If the effect re-runs (e.g.,
worktreereference changes while a generation is already in progress),generateCommitMessageRef.current()fires again without aborting the previous request. The first response will callsetMessage, then the second will overwrite it — andisGeneratingwill flip tofalseon the first completion even though the second call is still in flight.Consider tracking a cancellation flag (similar to the
cancelledpattern already used in the diffs-loading effect on line 319) so stale responses are discarded:Suggested fix
useEffect(() => { if (open && worktree) { // Reset state setMessage(''); setError(null); if (!enableAiCommitMessages) { return; } - generateCommitMessageRef.current(); + let cancelled = false; + generateCommitMessageRef.current().then(() => { + // If superseded by a newer run, the result has already been + // applied — but at least we won't stack multiple in-flight calls + // once an AbortController is threaded through. + }); + return () => { + cancelled = true; + }; } }, [open, worktree, enableAiCommitMessages]);For a more robust solution, pass an
AbortSignalintogenerateCommitMessageso the HTTP request itself is cancelled on cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx` around lines 584 - 597, The AI commit-generation effect calls generateCommitMessageRef.current() without any cancellation guard, so overlapping invocations can race and flip isGenerating or overwrite setMessage/setError; add a cancellation mechanism like the existing "cancelled" flag used in the diffs-loading effect and/or pass an AbortSignal into the generateCommitMessage implementation, set cancelled = false at start, set it to true in the effect cleanup, and in the async generator response handling check cancelled before calling setMessage, setError or toggling isGenerating; also if generateCommitMessage supports AbortController, create one per effect run and call controller.abort() in cleanup so in-flight HTTP requests are actually cancelled.apps/server/src/routes/worktree/routes/discard-changes.ts (2)
124-124:⚠️ Potential issue | 🟡 Minor
isSelectiveDiscardsilently promotes to an all-discard when every changed file is selected.The condition
files.length < allFiles.lengthisfalsewhen the caller passes exactly the same count of files that exist inallFiles. The all-discard path then runsgit checkout .andgit clean -ffdwithout any path restrictions, which will also discard any additional files that appeared in the worktree between when the status snapshot was taken and when this request executes — files the caller never selected.If the intent is to treat "all selected = same as all discard" as an optimisation, that should at minimum be documented. Otherwise the safer fix is to remove the count-based shortcut:
🛠️ Proposed fix
- const isSelectiveDiscard = files && files.length > 0 && files.length < allFiles.length; + const isSelectiveDiscard = files && files.length > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` at line 124, The current isSelectiveDiscard check (const isSelectiveDiscard = files && files.length > 0 && files.length < allFiles.length) can mistakenly trigger the full-discard branch when the caller selects exactly every file by count; update the logic to only treat the operation as a full discard if the selected file set truly equals the entire allFiles set or, safer, remove the length-based shortcut and instead compute membership: consider isSelectiveDiscard = files && files.length > 0 && files.every(f => allFiles.includes(f)) ? false : true (or simply set isSelectiveDiscard = files && files.length > 0 and then ensure all git commands are invoked with explicit path restrictions when files is provided), referencing the isSelectiveDiscard variable and the files/allFiles arrays so the discard path never runs git checkout . / git clean -ffd without path arguments unless the selection genuinely covers the whole worktree.
24-24:⚠️ Potential issue | 🟡 MinorChange import of
execGitCommandto use@automaker/git-utils.Line 23 imports from
'../../../lib/git.js', a relative cross-directory import. Per coding guidelines, this file should import from the shared package@automaker/git-utilsinstead, which exportsexecGitCommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` at line 24, The file imports execGitCommand via a relative path; replace that import to use the shared package export instead. Update the import in discard-changes.ts to import execGitCommand from '@automaker/git-utils' (instead of '../../../lib/git.js') so the route continues to call execGitCommand as before but via the shared package export.apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts (1)
157-157:⚠️ Potential issue | 🟡 MinorDead-code ternary: both branches return
targetBranch.
worktreeData.isMain ? targetBranch : targetBranchevaluates totargetBranchregardless. This looks like an oversight — perhaps the main branch display was intended to differ (e.g., showing"main"or a different label)?Suggested fix (if both should truly be the same)
- const branchDisplay = worktreeData.isMain ? targetBranch : targetBranch; + const branchDisplay = targetBranch;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts` at line 157, The ternary assigning branchDisplay is dead code (worktreeData.isMain ? targetBranch : targetBranch) — replace it with a single value or the intended alternate; either simplify to const branchDisplay = targetBranch; or, if main should display differently, change the true branch to the correct value (e.g., const branchDisplay = worktreeData.isMain ? 'main' : targetBranch or use worktreeData.branchName) and update any references to branchDisplay accordingly.
♻️ Duplicate comments (2)
apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx (1)
143-150: ✅ Invisible-dot fix correctly applied.The
fileColor.replace('text-', 'bg-')approach resolves the previously reported invisible indicator, including thetext-muted-foreground → bg-muted-foregroundfallback for extension-less files.🤖 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 143 - 150, The invisible-dot bug is fixed by converting the file color utility class from a text color to a background color for the non-dirty indicator: update the span rendering in editor-tabs.tsx to use fileColor.replace('text-', 'bg-') so the dot uses a background class (ensure this is applied where tab.isDirty is false and that fileColor falls back to "text-muted-foreground" so the replace produces "bg-muted-foreground" for extension-less files); verify usage around the tab.isDirty conditional and the fileColor variable in the editor-tabs component.apps/ui/src/store/app-store.ts (1)
2662-2719: Past review feedback addressed: default values corrected and sparse-array fix applied.
getPinnedWorktreesCountnow defaults to1(was0).getAlwaysUseWorktreeDropdownnow defaults tofalse(wastrue).swapPinnedWorktreeBranchpre-fills the array to avoid sparse holes, adopting the previously suggested fix.The remaining empty-string-as-placeholder concern in
swapPinnedWorktreeBranch(Line 2686, Line 2692) was already raised — empty strings will be stored for unfilled slots, which could confuse downstream worktree lookups that expect valid branch names. Consumers ofgetPinnedWorktreeBranchesshould filter out empty entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/app-store.ts` around lines 2662 - 2719, The swapPinnedWorktreeBranch implementation currently pre-fills missing slots with empty strings which then get persisted and can confuse consumers; change it to avoid storing empty-string placeholders by either (a) pre-fill with undefined/null and when writing back to state filter out falsy entries, or (b) build a compact array that only includes real branch names before calling setPinnedWorktreeBranches / updating pinnedWorktreeBranchesByProject; update getPinnedWorktreeBranches if needed to return a filtered list so consumers never see "" placeholders.
🟡 Minor comments (21)
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts-467-471 (1)
467-471:⚠️ Potential issue | 🟡 MinorMissing fallback description leaves users without actionable guidance for non-
Errorexceptions.When
erroris not anErrorinstance (e.g. a rejected promise with a plain object or a thrown string),descriptionbecomesundefinedand the toast shows only"Failed to start dev server"with no hint of where to look — a regression from the previous"Check the dev server logs panel for details."message. This is also inconsistent with theresult.success === falsebranch immediately above (Lines 463–465), which still carries that helpful fallback.🛠️ Proposed fix
} catch (error) { logger.error('Start dev server failed:', error); toast.error('Failed to start dev server', { - description: error instanceof Error ? error.message : undefined, + description: error instanceof Error ? error.message : 'Check the dev server logs panel for details.', }); }🤖 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 467 - 471, The catch block in use-dev-servers.ts currently calls logger.error and toast.error but leaves toast.description undefined for non-Error throws; update the catch to mirror the earlier result.success === false branch by passing a fallback description (e.g., "Check the dev server logs panel for details.") when error is not an Error, or alternatively include a safe stringified version of the thrown value; update the toast.error call (and keep logger.error) so toast.description uses error instanceof Error ? error.message : fallbackDescription and ensure any stringification is guarded to avoid throwing.apps/ui/src/components/views/board-view/dialogs/delete-worktree-dialog.tsx-82-87 (1)
82-87:⚠️ Potential issue | 🟡 MinorSilent
catchleaves UI stale ifonDeletedthrowsWhen
onDeletedthrows, the parent's board state never updates — the deleted worktree stays visible in the panel, feature-branch assignments aren't cleared, and the worktree list isn't refreshed. The user sees a success toast and a closed dialog, but has no idea the UI is stale and no path to recovery short of a full page refresh.At minimum, surface a user-visible error in the catch block so they know to refresh:
🛡️ Proposed fix
try { onDeleted(worktree, deleteBranch); } catch (error) { // Prevent errors in onDeleted from propagating to the error boundary console.error('onDeleted callback failed:', error); + toast.error('Worktree deleted, but the view may be out of sync. Please refresh.'); }🤖 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/delete-worktree-dialog.tsx` around lines 82 - 87, The catch is currently swallowing errors from the onDeleted callback causing the UI to remain stale; update the delete-worktree-dialog component so the success path (success toast, closing dialog) only runs after onDeleted(worktree, deleteBranch) completes without throwing, and in the catch for onDeleted surface a user-visible error (e.g., use the same toast/snackbar mechanism or an inline error message) that explains the delete failed and suggests refreshing or retrying; reference the onDeleted callback, worktree and deleteBranch variables and ensure the error is logged to console as well as shown to the user so there is both developer and user-facing feedback.apps/server/src/routes/worktree/routes/generate-pr-description.ts-198-203 (1)
198-203:⚠️ Potential issue | 🟡 MinorRename-match regex is ambiguous for paths containing
b/.The greedy
.+in the rename fallback regexa\/(.+) b\/(.+)$will split on the last occurrence ofb/in the line, which produces incorrect captures when either the source or destination path literally containsb/(e.g.,src/b/foo.ts). The backref pattern correctly handles the non-rename case, but renames involving such paths would be mis-keyed in the map.In practice git quotes these problematic paths, so this is unlikely to trigger, but worth a defensive note.
🤖 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-pr-description.ts` around lines 198 - 203, The rename regex in generate-pr-description.ts uses greedy captures (a\/(.+) b\/(.+)$) which can split on the last " b/" inside a path; change the renameMatch pattern to use non-greedy captures (e.g., a\/(.+?) b\/(.+?)$) or otherwise restrict the capture (e.g., use [^ ]+ for path segments) so the split occurs at the intended " b/" delimiter; update the renameMatch assignment and keep the existing logic that prefers backrefMatch but uses match[2] for renames (the variables backrefMatch, renameMatch and filePath should be updated accordingly).apps/server/src/routes/worktree/routes/set-tracking.ts-18-22 (1)
18-22:⚠️ Potential issue | 🟡 MinorNo format validation on
remoteandbranchinputs.While
execGitCommanduses array-based execution (safe from shell injection), theremoteandbranchvalues from the request body are not validated beyond presence checks. Whitespace-only strings like" "would pass the truthy check but produce confusing git errors. Consider trimming inputs and optionally validating against a pattern (e.g.,isValidBranchNamewhich is already used indelete.tsfor branch names).Suggested trimming
- const { worktreePath, remote, branch } = req.body as { + const { worktreePath, remote, branch: rawBranch } = req.body as { worktreePath: string; remote: string; branch?: string; }; + const branch = rawBranch?.trim(); - if (!worktreePath) { + if (!worktreePath?.trim()) { res.status(400).json({ success: false, error: 'worktreePath required' }); return; } - if (!remote) { + if (!remote?.trim()) { res.status(400).json({ success: false, error: 'remote required' }); return; }Also applies to: 34-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/set-tracking.ts` around lines 18 - 22, Trim the request values for worktreePath, remote, and branch (the destructured variables in the set-tracking route handler) and reject whitespace-only strings for remote/branch with a 400; if branch is provided validate it using the existing isValidBranchName helper (same validator used in delete.ts) before calling execGitCommand, and similarly ensure remote matches a sane pattern (or at least non-empty after trim) so you don't pass whitespace-only values into execGitCommand; return a clear 4xx error when validation fails.apps/server/src/routes/worktree/routes/delete.ts-77-81 (1)
77-81:⚠️ Potential issue | 🟡 MinorPath comparison may fail on trailing-slash or symlink differences.
registeredPath === worktreePathuses strict string equality. If the request-suppliedworktreePathhas a trailing slash, differs in case (on case-insensitive file systems), or the porcelain output resolves symlinks differently, this check will produce a false negative and the prune path will incorrectly re-throw.Consider normalizing both sides before comparison (e.g.
path.resolve):Proposed fix
+import path from 'path'; ... - .some((registeredPath) => registeredPath === worktreePath); + .some((registeredPath) => path.resolve(registeredPath) === path.resolve(worktreePath));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/delete.ts` around lines 77 - 81, The equality check that computes stillRegistered can falsely fail due to trailing slashes, symlinks or case differences; normalize both sides before comparing by resolving canonical paths: for each registeredPath and the input worktreePath call path.resolve and preferably fs.realpathSync (or async realpath) to resolve symlinks, then normalize (path.normalize) and, if you want case-insensitive comparisons, compare toLowerCase() on both; update the .map/.some pipeline that derives stillRegistered to use the canonicalized forms of registeredPath and worktreePath so comparisons are robust.apps/server/src/routes/worktree/routes/discard-changes.ts-60-68 (1)
60-68:⚠️ Potential issue | 🟡 Minor
parseFilePathmay misparse filenames that literally contain->.
trimmedPath.indexOf(' -> ')finds the first occurrence. If the old path in a rename entry itself contains->(e.g.,foo -> bar.ts -> new_name.ts),slice(arrowIndex + 4)yields the wrong segment. Git porcelain v1 uses->as a fixed separator, so any path segment containing that substring will confuse this heuristic.Consider switching to
lastIndexOf(' -> ')to always take the last arrow as the delimiter (git will only ever emit one->per rename entry, and the new path is always to the right of the final arrow):🛠️ Proposed fix
- const arrowIndex = trimmedPath.indexOf(' -> '); + const arrowIndex = trimmedPath.lastIndexOf(' -> ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` around lines 60 - 68, parseFilePath currently uses trimmedPath.indexOf(' -> ') which picks the first arrow and can misparse rename entries when the old path contains ' -> '; change the search to use trimmedPath.lastIndexOf(' -> ') so the delimiter is the final arrow and then return trimmedPath.slice(arrowIndex + 4) as before (keep the existing indexStatus/workTreeStatus checks and trimmedPath handling in the parseFilePath function).apps/server/src/routes/worktree/routes/discard-changes.ts-179-187 (1)
179-187:⚠️ Potential issue | 🟡 MinorUnmatched-file fallback can silently skip tracked-but-modified files.
When a requested path is not found in
allFiles(e.g., due to a path-format normalisation difference between the UI and git output), it is pushed directly intountrackedFilesand passed togit clean -ffd.git cleanonly removes untracked files, so if the real problem is a path-format mismatch for a tracked modified file, the change remains un-discarded and no error or warning is emitted. The response will still report success.Consider at least pushing a warning into
warningsfor unmatched paths so the caller can surface the discrepancy:🛠️ Proposed fix
for (const requestedFile of files) { if (!matchedFiles.has(requestedFile)) { + warnings.push(`File not found in current git status, attempting clean: ${requestedFile}`); untrackedFiles.push(requestedFile); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/discard-changes.ts` around lines 179 - 187, The current unmatched-file fallback quietly treats any requested path not found in allFiles as untracked by pushing it into untrackedFiles (using variables files, matchedFiles, untrackedFiles), which can silently skip tracked-but-modified files; modify the loop that processes requestedFile so that when a requestedFile is not in matchedFiles you also push a descriptive warning into the existing warnings collection (e.g., warnings.push(`Requested path "${requestedFile}" did not match repo entries; it may be a tracked file with path-normalization differences`))—this ensures callers are informed before running git clean -ffd and can surface the discrepancy to the UI.apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx-531-531 (1)
531-531:⚠️ Potential issue | 🟡 MinorUser is locked out of free-form input when a remote is selected but its branch list is empty.
allowCreateisfalsewheneverselectedRemote !== 'local'and there's no fetch error. If the selected remote has no branches yet (or the server returned an empty list),branchNamesis[]and theBranchAutocompleterenders with nothing to pick and no way to type a custom ref.🐛 Proposed fix
- allowCreate={!!branchFetchError || selectedRemote === 'local'} + allowCreate={!!branchFetchError || selectedRemote === 'local' || branchNames.length === 0}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx` at line 531, The UI disables free-form branch input whenever a non-local remote is selected and branchFetchError is false; update the allowCreate logic passed to BranchAutocomplete in create-worktree-dialog.tsx so that free-form entry is allowed if branchNames is empty (i.e., when the remote has no branches) in addition to the existing conditions. Locate the BranchAutocomplete usage (prop allowCreate with selectedRemote and branchFetchError) and extend the condition to also permit create when branchNames.length === 0 so users can type a new ref for an empty remote.apps/ui/src/components/views/settings-view/model-defaults/phase-model-selector.tsx-2240-2240 (1)
2240-2240:⚠️ Potential issue | 🟡 MinorUse underscore-encoded spaces inside
calc()for spec compliance.The CSS spec requires whitespace on both sides of
+and-operators incalc()expressions. The current code omits these spaces:calc(100vw-2rem)is technically non-compliant.In Tailwind v4 arbitrary values, encode required spaces as underscores—Tailwind automatically converts
_to spaces at build time. Apply this to the entire expression inside the brackets:🛠️ Proposed fix
- className="w-[min(calc(100vw-2rem),320px)] p-0" + className="w-[min(calc(100vw_-_2rem),320px)] p-0"Modern browsers tolerate the missing spaces, but this ensures spec compliance and aligns with Tailwind v4 best practices.
🤖 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/model-defaults/phase-model-selector.tsx` at line 2240, Update the Tailwind arbitrary width string in phase-model-selector's JSX: the className currently uses "w-[min(calc(100vw-2rem),320px)]" which is non-compliant because calc() needs spaces around the minus; replace the calc expression by encoding spaces as underscores per Tailwind v4 arbitrary-value rules (e.g., use calc(100vw_-_2rem) inside the w-[min(...)] value) so Tailwind will convert underscores to real spaces at build time; locate the className on the element in phase-model-selector.tsx and update the arbitrary value accordingly.apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx-113-119 (1)
113-119:⚠️ Potential issue | 🟡 MinorIcon-only scroll buttons are missing
aria-label.
titleprovides a mouse tooltip but is not reliably announced by screen readers. Icon-only<button>elements need an explicit accessible label.♿ Proposed fix
<button onClick={() => scrollBy('left')} className="shrink-0 p-1 text-muted-foreground hover:text-foreground hover:bg-muted/50 transition-colors" title="Scroll tabs left" + aria-label="Scroll tabs left" ><button onClick={() => scrollBy('right')} className="shrink-0 p-1 text-muted-foreground hover:text-foreground hover:bg-muted/50 transition-colors" title="Scroll tabs right" + aria-label="Scroll tabs right" >Also applies to: 177-183
🤖 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 113 - 119, The icon-only scroll buttons in editor-tabs.tsx (the button rendering ChevronLeft with onClick={() => scrollBy('left')} and the corresponding ChevronRight button using scrollBy('right')) lack accessible labels; add an explicit aria-label (e.g., aria-label="Scroll tabs left" and aria-label="Scroll tabs right") to each button element so screen readers announce their purpose while keeping the existing title and classes unchanged.apps/ui/src/components/views/file-editor-view/components/code-editor.tsx-287-303 (1)
287-303:⚠️ Potential issue | 🟡 Minor
addedLinesiterated without sorting — inconsistent withdeletedGroupsand fragile againstRangeSetBuilderordering requirement.
RangeSetBuilder.add()requires ranges in ascending position order.deletedGroupsexplicitly sorts its keys at Line 314, butaddedLinesrelies onSetinsertion order. For well-formed unified diffs this holds, but any edge case (duplicate hunk ranges, tool-generated diffs) that inserts a lower line number after a higher one will throw atbuilder.finish().Additionally, using
EditorView.decorations.of(a compute function called on every view update — scroll, cursor move, etc.) for added lines while using the more performantStateField(only updates ondocChanged) for deleted lines is inconsistent. Converting to aStateFieldalso makes explicit sorting natural:🛠️ Proposed fix: sort + StateField for added lines
if (addedLines.size > 0) { + const sortedAddedLines = [...addedLines].sort((a, b) => a - b); + extensions.push( - EditorView.decorations.of((view) => { - const builder = new RangeSetBuilder<Decoration>(); - const doc = view.state.doc; - - for (const lineNum of addedLines) { - if (lineNum <= doc.lines) { - const linePos = doc.line(lineNum).from; - builder.add(linePos, linePos, addedLineDecoration); - } - } - - return builder.finish(); - }) + StateField.define({ + create(state) { + const builder = new RangeSetBuilder<Decoration>(); + const doc = state.doc; + for (const lineNum of sortedAddedLines) { + if (lineNum <= doc.lines) { + builder.add(doc.line(lineNum).from, doc.line(lineNum).from, addedLineDecoration); + } + } + return builder.finish(); + }, + update(decorations, tr) { + return tr.docChanged ? decorations.map(tr.changes) : decorations; + }, + provide: (f) => EditorView.decorations.from(f), + }) ); }🤖 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/code-editor.tsx` around lines 287 - 303, The addedLines decoration creation iterates the Set in insertion order inside EditorView.decorations.of which is called on every view update and can violate RangeSetBuilder's requirement that ranges are added in ascending document order (causing builder.finish() to throw); convert the added-lines logic to a StateField (like deletedGroups) so it only recomputes on doc changes and ensure you iterate a sorted array of line numbers (e.g. Array.from(addedLines).sort((a,b)=>a-b)) before calling RangeSetBuilder.add with addedLineDecoration, referencing addedLines, deletedGroups, RangeSetBuilder, addedLineDecoration, EditorView.decorations.of, StateField and docChanged in your changes.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx-699-744 (1)
699-744:⚠️ Potential issue | 🟡 MinorSilent no-op when
api.features?.createis undefined.If the Electron API doesn't expose
features.create, the handler exits without giving any feedback. Thetoast.error('No project selected')guard earlier sets the expectation that errors are surfaced, so a missing capability should also be reported.🛡️ Proposed fix
- if (api.features?.create) { - const feature = { ... }; - const result = await api.features.create(...); - ... - } + if (!api.features?.create) { + toast.error('Feature creation is not available'); + return; + } + const feature = { ... }; + const result = await api.features.create(...); + ...🤖 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 699 - 744, The handler silently no-ops if getElectronAPI().features?.create is undefined; update the create flow in file-editor-view.tsx to check for api.features?.create and if missing call toast.error with a clear message (e.g., "Feature creation not available") and return early; locate the block using getElectronAPI, api.features.create, and the feature creation logic (including the result handling, queryClient.invalidateQueries, and toast.success) and add the guard before constructing or calling api.features.create so users get feedback when the capability is absent.apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx-33-66 (1)
33-66:⚠️ Potential issue | 🟡 MinorRemove redundant error handling—
setEventHookscatches and suppresses errors internally, socatchblocks in these handlers will never fire.
setEventHooksis async and awaits the HTTP persistence call, but it catches errors internally (line 1449-1454 inapp-store.ts) and logs them without re-throwing. The Promise returned bysetEventHookswill always resolve successfully, making thecatchblocks inhandleDeleteHook,handleToggleHook, andhandleSaveHookunreachable. The error toasts ('Failed to delete/toggle/save event hook') will never display.Either:
- Remove the redundant try/catch blocks here, or
- Have
setEventHooksre-throw errors so handlers can respond with user-facing feedback🤖 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/event-hooks/event-hooks-section.tsx` around lines 33 - 66, The handlers handleDeleteHook, handleToggleHook, and handleSaveHook contain redundant try/catch blocks because setEventHooks (in app-store.ts) swallows errors and never re-throws; remove the try/catch wrappers in these three functions and simply await setEventHooks(...) then proceed with the success-only logic (e.g., setDialogOpen(false), setEditingHook(null)) so the code is not misleading; alternatively, if you prefer user-facing error toasts, modify setEventHooks to re-throw errors instead—choose one approach and apply it consistently across handleDeleteHook, handleToggleHook, and handleSaveHook.apps/ui/src/components/views/board-view/components/add-feature-button.tsx-86-96 (1)
86-96:⚠️ Potential issue | 🟡 MinorMinor accessibility inconsistency: compact mode template trigger missing
aria-label.The full-mode template dropdown trigger (line 159) has
aria-label="Templates", but the compact-mode trigger here only hastitle="Templates". For consistency and screen-reader accessibility, addaria-labelhere too.♿ Add aria-label to compact dropdown trigger
<Button variant="default" size="sm" className="h-6 w-4 p-0 rounded-l-none border-l border-primary-foreground/20" title="Templates" + aria-label="Templates" data-testid={`${testIdPrefix}-dropdown-trigger`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/add-feature-button.tsx` around lines 86 - 96, The compact-mode dropdown trigger (the Button inside DropdownMenuTrigger in add-feature-button.tsx) is missing an aria-label for screen readers; update the Button within the compact-mode block (the Button with title="Templates" and data-testid={`${testIdPrefix}-dropdown-trigger`}) to include aria-label="Templates" to match the full-mode trigger and restore accessibility consistency.apps/server/src/services/event-hook-service.ts-81-125 (1)
81-125:⚠️ Potential issue | 🟡 MinorDuplicate
FeatureCompletedPayloadinterface declaration.
FeatureCompletedPayloadis declared identically at lines 84–91 and again at lines 118–125. TypeScript merges duplicate interface declarations so this doesn't break anything, but the second copy is dead code and likely a copy-paste oversight.🧹 Remove the duplicate declaration (lines 115–125)
-/** - * Feature completed event payload structure - */ -interface FeatureCompletedPayload { - featureId: string; - featureName?: string; - projectPath: string; - passes?: boolean; - message?: string; - executionMode?: 'auto' | 'manual'; -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/event-hook-service.ts` around lines 81 - 125, There is a duplicate declaration of the interface FeatureCompletedPayload in the file; remove the redundant second declaration (the duplicate block after isFeatureStatusChangedPayload) so only a single FeatureCompletedPayload definition remains, leaving the type guard isFeatureStatusChangedPayload and FeatureStatusChangedPayload intact; simply delete the repeated interface block to eliminate the dead/copy-paste code.apps/ui/src/routes/__root.tsx-601-611 (1)
601-611:⚠️ Potential issue | 🟡 Minor
serverHooks.length > 0guard silently skips full-deletion sync.If a user removes all event hooks on another device,
serverHookswill be[], so the condition is false and the local store keeps the stale hooks indefinitely on warm restarts (fast-hydrate path). The cold-start path (hydrateStoreFromSettings) handles this correctly, but warm-restart users won't see the deletion propagate until their next cold start.If the intent is only to avoid overwriting fresh-install state where the server hasn't populated hooks yet, consider checking server settings availability more explicitly (e.g., the absence of the
eventHookskey rather than an empty array), or document the known limitation.💡 Possible fix to handle intentional empty-hook state
- const serverHooks = (finalSettings as GlobalSettings).eventHooks ?? []; + const rawHooks = (finalSettings as GlobalSettings).eventHooks; + // undefined means the key was absent (legacy/unset); [] means explicitly empty + if (rawHooks === undefined) return; // don't reconcile if not present + const serverHooks = rawHooks; const currentHooks = useAppStore.getState().eventHooks; if ( - JSON.stringify(serverHooks) !== JSON.stringify(currentHooks) && - serverHooks.length > 0 + JSON.stringify(serverHooks) !== JSON.stringify(currentHooks) ) {🤖 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 601 - 611, The current guard `serverHooks.length > 0` prevents syncing an intentional empty array from the server; change the condition so we reconcile whenever the server explicitly provides the `eventHooks` key (even if it's `[]`). Replace the `serverHooks.length > 0` check with a presence check against `finalSettings` (e.g., use Object.prototype.hasOwnProperty.call(finalSettings, 'eventHooks') or check `finalSettings.eventHooks !== undefined`) while keeping the existing JSON.stringify comparison and the `useAppStore.setState({ eventHooks: serverHooks })` update; this ensures deletions from the server propagate during the fast-hydrate path (symbols: serverHooks, currentHooks, finalSettings as GlobalSettings, useAppStore, hydrateStoreFromSettings).apps/ui/src/components/views/board-view/dialogs/quick-add-dialog.tsx-42-48 (1)
42-48:⚠️ Potential issue | 🟡 Minor
effectiveDefaultFeatureModelin deps may reset user input mid-editing.If the effective default model changes while the dialog is open (e.g., project settings updated in another tab), this effect will clear the description and reset the model selection. Consider guarding the reset to only fire when
opentransitions fromfalsetotrue:Proposed fix
+ const prevOpenRef = useRef(false); useEffect(() => { - if (open) { + if (open && !prevOpenRef.current) { setDescription(''); setDescriptionError(false); setModelEntry(effectiveDefaultFeatureModel || { model: 'claude-opus' }); } - }, [open, effectiveDefaultFeatureModel]); + prevOpenRef.current = open; + }, [open, effectiveDefaultFeatureModel]);🤖 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/quick-add-dialog.tsx` around lines 42 - 48, The effect that resets description and model (useEffect watching [open, effectiveDefaultFeatureModel]) can clobber user edits when effectiveDefaultFeatureModel changes while the dialog is open; change the logic so the reset runs only when open transitions from false to true (not on every effectiveDefaultFeatureModel update) — e.g., track previous open state with a ref or usePrevious and only call setDescription(''), setDescriptionError(false), and setModelEntry(...) inside the effect when open is true and prevOpen was false, keeping effectiveDefaultFeatureModel available for initial set but avoiding mid-edit resets.apps/ui/src/components/views/settings-view/templates/templates-section.tsx-211-260 (1)
211-260:⚠️ Potential issue | 🟡 MinorSurface template mutation failures to avoid silent errors.
onUpdateTemplate/onAddTemplate/onDeleteTemplate/onReorderTemplatesare awaited without error handling; a rejection will be silent and may leave the dialog state ambiguous. Consider wrapping these calls and showing a failure toast.🛠️ Suggested pattern (handleSave)
const handleSave = async () => { // Validate let hasError = false; if (!formData.name.trim()) { setNameError(true); hasError = true; } if (!formData.prompt.trim()) { setPromptError(true); hasError = true; } if (hasError) return; - if (editingTemplate) { - // Update existing - await onUpdateTemplate(editingTemplate.id, { - name: formData.name.trim(), - prompt: formData.prompt.trim(), - model: formData.model, - }); - toast.success('Template updated'); - } else { - // Create new - const newTemplate: FeatureTemplate = { - id: generateId(), - name: formData.name.trim(), - prompt: formData.prompt.trim(), - model: formData.model, - isBuiltIn: false, - enabled: true, - order: Math.max(...templates.map((t) => t.order ?? 0), -1) + 1, - }; - await onAddTemplate(newTemplate); - toast.success('Template created'); - } - setDialogOpen(false); + try { + if (editingTemplate) { + // Update existing + await onUpdateTemplate(editingTemplate.id, { + name: formData.name.trim(), + prompt: formData.prompt.trim(), + model: formData.model, + }); + toast.success('Template updated'); + } else { + // Create new + const newTemplate: FeatureTemplate = { + id: generateId(), + name: formData.name.trim(), + prompt: formData.prompt.trim(), + model: formData.model, + isBuiltIn: false, + enabled: true, + order: Math.max(...templates.map((t) => t.order ?? 0), -1) + 1, + }; + await onAddTemplate(newTemplate); + toast.success('Template created'); + } + setDialogOpen(false); + } catch (error) { + toast.error('Failed to save template', { + description: error instanceof Error ? error.message : 'Please try again.', + }); + } };🤖 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/templates/templates-section.tsx` around lines 211 - 260, Wrap all awaited template mutations (onUpdateTemplate in handleToggleEnabled and handleSave, onAddTemplate in handleSave, onDeleteTemplate in handleDelete, and any onReorderTemplates usage) in try/catch blocks, show a failure toast with the error message on catch, and only update local UI state (e.g., close dialog via setDialogOpen(false) or assume success) after the awaited call succeeds; for updates ensure you rethrow or handle errors so the dialog isn't closed on failure and use descriptive messages like "Failed to update template" / "Failed to create template" / "Failed to delete template" to surface mutation failures.apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx-1243-1313 (1)
1243-1313:⚠️ Potential issue | 🟡 MinorSync actions aren’t wired in the WorktreeDropdown path.
Line 1243+: the desktop dropdown variant omits
isSyncing/onSync/onSyncWithRemote/onSetTracking/remotesWithBranch, so sync and tracking actions won’t be available there even though they are in other render paths.🧩 Suggested fix
onPushWithRemote={handlePushWithSpecificRemote} remotesCache={remotesCache} + isSyncing={isSyncing} + onSync={handleSyncWithRemoteSelection} + onSyncWithRemote={handleSyncWithSpecificRemote} + onSetTracking={handleSetTrackingForRemote} + remotesWithBranch={remotesWithBranch} onOpenInEditor={handleOpenInEditor}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx` around lines 1243 - 1313, The desktop WorktreeDropdown render is missing the sync/tracking props so sync features aren't available; add the sync-related props to the WorktreeDropdown JSX: pass isSyncing, onSync, onSyncWithRemote, onSetTracking and remotesWithBranch and wire them to the existing local state/handlers (the module's isSyncing flag and the corresponding sync handlers — e.g. the local onSync/handleSync, onSyncWithRemote/handleSyncWithSpecificRemote, onSetTracking/handleSetTracking, and remotesWithBranch variable) so the dropdown receives the same sync/tracking API as the other render paths.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx-580-593 (1)
580-593:⚠️ Potential issue | 🟡 MinorDuplicate props passed to
WorktreeActionsDropdown.Lines 589–592 duplicate
availableWorktreesForSwap,slotIndex,onSwapWorktree, andpinnedBrancheswhich are already passed on lines 580–583. This is a copy-paste error. While React uses the last occurrence so behavior is unchanged, the duplicates should be removed.🧹 Remove duplicate props
onEditScripts={onEditScripts} availableWorktreesForSwap={availableWorktreesForSwap} slotIndex={slotIndex} onSwapWorktree={onSwapWorktree} pinnedBranches={pinnedBranches} isSyncing={isSyncing} onSync={onSync} onSyncWithRemote={onSyncWithRemote} onSetTracking={onSetTracking} remotesWithBranch={remotesWithBranch} - availableWorktreesForSwap={availableWorktreesForSwap} - slotIndex={slotIndex} - onSwapWorktree={onSwapWorktree} - pinnedBranches={pinnedBranches} />🤖 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-tab.tsx` around lines 580 - 593, Remove the duplicated prop entries passed into the WorktreeActionsDropdown component: availableWorktreesForSwap, slotIndex, onSwapWorktree, and pinnedBranches are passed twice (first at the block with other props and again later); keep the first occurrences and delete the redundant duplicate props so each prop is passed only once from the parent component in worktree-tab.tsx.apps/ui/src/hooks/use-settings-sync.ts-840-844 (1)
840-844:⚠️ Potential issue | 🟡 MinorAlign the sanitization comment with actual behavior.
The sanitizer preserves non-null paths; the comment states only null paths are restored. Either update the comment or change the sanitizer to match the intent.✏️ Suggested comment fix
- // Sanitize: only restore entries with path === null (main branch). - // Non-null paths may reference deleted worktrees, causing crash loops. + // Sanitize structure only; runtime worktree validation handles deleted paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 840 - 844, The comment above the currentWorktreeByProject assignment is inaccurate: sanitizeWorktreeByProject actually preserves non-null paths instead of restoring only null paths. Update the comment to reflect actual behavior (e.g., “Sanitize: preserve non-null worktree paths; only drop entries pointing to deleted worktrees”) or, if the original intent was to restore only null (main branch) entries, modify sanitizeWorktreeByProject to filter out non-null paths accordingly; reference sanitizeWorktreeByProject and currentWorktreeByProject when making the change.
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/file-editor-view/file-editor-view.tsx
Outdated
Show resolved
Hide resolved
- Update DEFAULT_MAX_TURNS from 1000 to 10000 in settings-helpers.ts and agent-executor.ts - Update MAX_ALLOWED_TURNS from 2000 to 10000 in settings-helpers.ts - Update UI clamping logic from 2000 to 10000 in app-store.ts - Update fallback values from 1000 to 10000 in use-settings-sync.ts - Update default value from 1000 to 10000 in DEFAULT_GLOBAL_SETTINGS - Update documentation to reflect new range: 1-10000 Allows agents to perform up to 10000 turns for complex feature execution. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
apps/server/src/lib/settings-helpers.ts (2)
37-40: Operational risk: 10× increase in default/max turns may spike API costs for unconfigured installations.Both
DEFAULT_MAX_TURNSandMAX_ALLOWED_TURNSare now10000. Any deployment that has not explicitly configureddefaultMaxTurnswill silently jump from 1 000 to 10 000 turns per run. A misbehaving or looping agent that previously self-limited at 1 000 iterations will now run 10× longer before the ceiling kicks in, with corresponding API cost exposure.Additionally, setting
DEFAULT_MAX_TURNS === MAX_ALLOWED_TURNSeliminates the conceptual headroom between "typical default" and "hard ceiling". Consider whether a lower default (e.g., 1 000 or 2 000) paired with a higher ceiling (10 000) better reflects the intent — the default is what every project gets out of the box, while the ceiling is the opt-in maximum.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/settings-helpers.ts` around lines 37 - 40, DEFAULT_MAX_TURNS was changed to equal MAX_ALLOWED_TURNS, causing a 10× jump in the default run limit and removing the headroom between typical default and the hard ceiling; revert or lower the default to a safer value (e.g., 1000 or 2000) while keeping MAX_ALLOWED_TURNS at 10000 so the hard cap remains higher than the out-of-the-box default. Locate the constants DEFAULT_MAX_TURNS and MAX_ALLOWED_TURNS in settings-helpers.ts and set DEFAULT_MAX_TURNS to the chosen lower default (and leave MAX_ALLOWED_TURNS at 10000), adding a short comment that explains the rationale (default vs ceiling) to prevent accidental parity in future edits.
83-124:getUseClaudeCodeSystemPromptSettingis a near-exact copy ofgetAutoLoadClaudeMdSetting.The two functions share identical structure: same parameter signature, same project-then-global resolution, same
truedefault, and same error re-throw. A single generic helper would eliminate the duplication:♻️ Proposed generic helper
+/** + * Generic boolean setting resolver: project settings first, then global, then defaultValue. + * Throws on load errors. Returns defaultValue when settingsService is unavailable. + */ +async function resolveBooleanSetting( + projectPath: string, + settingsService: SettingsService | null | undefined, + projectKey: string, + globalKey: string, + logPrefix: string, + defaultValue = true +): Promise<boolean> { + if (!settingsService) { + logger.info(`${logPrefix} SettingsService not available, ${projectKey} defaulting to ${defaultValue}`); + return defaultValue; + } + try { + const projectSettings = await settingsService.getProjectSettings(projectPath); + const projectVal = (projectSettings as Record<string, unknown>)[projectKey]; + if (projectVal !== undefined) { + logger.info(`${logPrefix} ${projectKey} from project settings: ${projectVal}`); + return projectVal as boolean; + } + const globalSettings = await settingsService.getGlobalSettings(); + const result = ((globalSettings as Record<string, unknown>)[globalKey] ?? defaultValue) as boolean; + logger.info(`${logPrefix} ${globalKey} from global settings: ${result}`); + return result; + } catch (error) { + logger.error(`${logPrefix} Failed to load ${projectKey} setting:`, error); + throw error; + } +} export async function getAutoLoadClaudeMdSetting( projectPath: string, settingsService?: SettingsService | null, logPrefix = '[SettingsHelper]' ): Promise<boolean> { - if (!settingsService) { ... } - try { ... } catch { ... } + return resolveBooleanSetting(projectPath, settingsService, 'autoLoadClaudeMd', 'autoLoadClaudeMd', logPrefix); } export async function getUseClaudeCodeSystemPromptSetting( projectPath: string, settingsService?: SettingsService | null, logPrefix = '[SettingsHelper]' ): Promise<boolean> { - if (!settingsService) { ... } - try { ... } catch { ... } + return resolveBooleanSetting(projectPath, settingsService, 'useClaudeCodeSystemPrompt', 'useClaudeCodeSystemPrompt', logPrefix); }Alternatively, if type safety over
Record<string, unknown>indexing is preferred, you can keep both functions as-is and simply document the pattern for future settings of the same shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/settings-helpers.ts` around lines 83 - 124, The two near-identical functions getUseClaudeCodeSystemPromptSetting and getAutoLoadClaudeMdSetting should be consolidated by adding a generic helper (e.g., getBooleanSettingWithProjectFallback) that accepts (projectPath: string, settingsService?: SettingsService|null, logPrefix: string, projectKey: keyof ProjectSettings, globalKey: keyof GlobalSettings, defaultValue = true) and implements the current project-then-global lookup, logging, and error handling; replace both existing functions with thin wrappers that call this helper (passing the appropriate keys and default) so code is centralized, type-safe (use appropriate keyof types or a typed generic), and preserves current logging/error behavior via the same logger and parameter names.apps/server/src/services/agent-executor.ts (2)
41-41: ImportDEFAULT_MAX_TURNSfromsettings-helpers.tsinstead of redeclaring it.
settings-helpers.tsalready exportsDEFAULT_MAX_TURNS = 10000(and is already imported on line 22). Redeclaring an identical local shadow means a future value change in the canonical source would silently diverge here — used across every call path (lines 105, 381, 609, 732).♻️ Proposed fix
-import { getPromptCustomization } from '../lib/settings-helpers.js'; +import { getPromptCustomization, DEFAULT_MAX_TURNS } from '../lib/settings-helpers.js';-const DEFAULT_MAX_TURNS = 10000; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` at line 41, Remove the local redeclaration of DEFAULT_MAX_TURNS in agent-executor.ts and use the exported constant from settings-helpers.ts instead; replace the local const DEFAULT_MAX_TURNS = 10000 with the imported symbol (the file already imports settings-helpers) so all usages inside functions/methods in this module (references to DEFAULT_MAX_TURNS used across agent execution paths) rely on the single canonical export to avoid shadowing and future drift.
223-225: Avoid mutating the caller-suppliedoptionsobject as a session-ID side-channel.The same pattern appears in all four streaming paths (lines 223–225, 394–396, 611–613, 734–736). Writing back to
options.sdkSessionIdworks within a single execution (becausebuildExecOptsre-readso.sdkSessionIdon every call), but it silently mutates the caller's object, making the side-effect invisible from the function signature and fragile ifoptionsis ever reused or shared.A simple local mutable variable eliminates the mutation without changing behavior:
♻️ Proposed refactor (all four call sites)
Introduce a local variable right after the initial destructure:
sdkSessionId, sdkOptions, } = options; + let currentSdkSessionId = sdkSessionId;Replace every mutation block:
- if (msg.session_id && msg.session_id !== options.sdkSessionId) { - options.sdkSessionId = msg.session_id; - } + if (msg.session_id && msg.session_id !== currentSdkSessionId) { + currentSdkSessionId = msg.session_id; + }Pass
currentSdkSessionIdexplicitly where needed (e.g., thread it intobuildExecOptsor pass it tohandleSpecGenerated/executeTasksLoopinstead of relying on the mutated field). This keeps all session-ID tracking within the local scope ofexecute().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 223 - 225, The code currently mutates caller-supplied options.sdkSessionId inside execute(), which is fragile; instead introduce a local mutable variable (e.g., currentSdkSessionId) initialized from options.sdkSessionId, update that local variable when msg.session_id is present, and pass currentSdkSessionId explicitly into buildExecOpts and into downstream functions like handleSpecGenerated and executeTasksLoop rather than writing back to options; do this for all four streaming sites that touch options.sdkSessionId to remove the side-effect while preserving behavior.libs/prompts/tests/enhancement.test.ts (2)
332-349:ux-reviewermissing from theall modestest array.The type annotation at line 334 only includes 4 modes and omits
ux-reviewer, sobuildUserPrompt('ux-reviewer', ...)is not exercised here even though it is a valid, PR-introduced mode.♻️ Suggested fix
- const modes: Array<'improve' | 'technical' | 'simplify' | 'acceptance'> = [ + const modes: Array<'improve' | 'technical' | 'simplify' | 'acceptance' | 'ux-reviewer'> = [ 'improve', 'technical', 'simplify', 'acceptance', + 'ux-reviewer', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prompts/tests/enhancement.test.ts` around lines 332 - 349, The test "all modes" omits the new 'ux-reviewer' mode; update the modes declaration in the test (the Array<'improve' | 'technical' | 'simplify' | 'acceptance'> and the array value in the describe block) to include 'ux-reviewer' so buildUserPrompt('ux-reviewer', testText) is exercised; either extend the union to Array<'improve' | 'technical' | 'simplify' | 'acceptance' | 'ux-reviewer'> or relax the type to include the new mode and add 'ux-reviewer' to the modes array used in the forEach.
1-17: New public exportsisAdditiveMode,ADDITIVE_MODES,REWRITE_MODESare untested.The PR explicitly exports these three symbols from
libs/prompts/src/enhancement.ts(PR objectives), but neither test file imports or asserts anything about them. Adding basic coverage ensures regressions in the additive/rewrite split are caught.♻️ Suggested additions
import { getEnhancementPrompt, getSystemPrompt, getExamples, buildUserPrompt, isValidEnhancementMode, getAvailableEnhancementModes, IMPROVE_SYSTEM_PROMPT, TECHNICAL_SYSTEM_PROMPT, SIMPLIFY_SYSTEM_PROMPT, ACCEPTANCE_SYSTEM_PROMPT, IMPROVE_EXAMPLES, TECHNICAL_EXAMPLES, SIMPLIFY_EXAMPLES, ACCEPTANCE_EXAMPLES, + isAdditiveMode, + ADDITIVE_MODES, + REWRITE_MODES, } from '../src/enhancement.js';Then add a
describeblock:describe('isAdditiveMode / mode categories', () => { it('ADDITIVE_MODES should contain technical, acceptance, ux-reviewer', () => { expect(ADDITIVE_MODES).toContain('technical'); expect(ADDITIVE_MODES).toContain('acceptance'); expect(ADDITIVE_MODES).toContain('ux-reviewer'); expect(ADDITIVE_MODES).not.toContain('improve'); expect(ADDITIVE_MODES).not.toContain('simplify'); }); it('REWRITE_MODES should contain improve and simplify', () => { expect(REWRITE_MODES).toContain('improve'); expect(REWRITE_MODES).toContain('simplify'); }); it('isAdditiveMode returns true for additive modes', () => { ADDITIVE_MODES.forEach((mode) => expect(isAdditiveMode(mode)).toBe(true)); }); it('isAdditiveMode returns false for rewrite modes', () => { REWRITE_MODES.forEach((mode) => expect(isAdditiveMode(mode)).toBe(false)); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/prompts/tests/enhancement.test.ts` around lines 1 - 17, Add tests asserting the new public exports isAdditiveMode, ADDITIVE_MODES, and REWRITE_MODES: create a new describe block that checks ADDITIVE_MODES contains 'technical', 'acceptance', 'ux-reviewer' and does not contain 'improve' or 'simplify'; checks REWRITE_MODES contains 'improve' and 'simplify'; iterates ADDITIVE_MODES to assert isAdditiveMode(mode) === true and iterates REWRITE_MODES to assert isAdditiveMode(mode) === false. Reference the exported symbols isAdditiveMode, ADDITIVE_MODES, and REWRITE_MODES from the enhancement tests and add these assertions alongside the existing enhancement test suite.apps/server/tests/unit/lib/enhancement-prompts.test.ts (1)
194-200: Missing coverage for additive-mode prompt instruction.The loop asserts only that each prompt contains the input text and is non-trivially long. For additive modes (
technical,acceptance,ux-reviewer) the instruction phrase changes to'Generate ONLY the additional details…', but that is never asserted here, leaving the core Issue#803fix untested in this file.♻️ Suggested additions
it('should work with all enhancement modes', () => { ENHANCEMENT_MODES.forEach((mode) => { const prompt = buildUserPrompt(mode, testText); expect(prompt).toContain(testText); expect(prompt.length).toBeGreaterThan(100); + // Additive modes must not rewrite — they use a different instruction + const additiveModes = ['technical', 'acceptance', 'ux-reviewer']; + if (additiveModes.includes(mode)) { + expect(prompt).toContain('Generate ONLY the additional details'); + expect(prompt).not.toContain('Please enhance the following task description:'); + } else { + expect(prompt).toContain('Please enhance the following task description:'); + } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/lib/enhancement-prompts.test.ts` around lines 194 - 200, The test currently checks only that each prompt contains the input text and is long enough; add an assertion that for additive enhancement modes ('technical', 'acceptance', 'ux-reviewer') the prompt returned by buildUserPrompt(mode, testText) includes the additive instruction phrase "Generate ONLY the additional details" (or the exact phrase used in the prompt templates) so the change for Issue `#803` is actually covered; update the it block iterating ENHANCEMENT_MODES to detect those specific mode values and assert prompt.includes('Generate ONLY the additional details') for them while keeping existing assertions for all modes.apps/ui/src/hooks/use-settings-sync.ts (1)
833-839:agentModelBySessionentries are not capped to 50 on restore — inconsistent with save path
getSettingsFieldValue(lines 182–192) trimsagentModelBySessionto the 50 most-recent entries before every sync to the server. The restore path here does not apply the same cap, so a settings.json written before this PR (or by another client) could populate in-memory state with an unbounded number of entries until the next outbound sync.This self-corrects on the first successful sync, but for consistency consider applying the same cap during restore.
♻️ Apply the cap on restore
agentModelBySession: serverSettings.agentModelBySession ? Object.fromEntries( - Object.entries(serverSettings.agentModelBySession as Record<string, unknown>).map( - ([sessionId, entry]) => [sessionId, migratePhaseModelEntry(entry)] - ) + Object.entries(serverSettings.agentModelBySession as Record<string, unknown>) + .slice(-50) + .map(([sessionId, entry]) => [sessionId, migratePhaseModelEntry(entry)]) ) : currentAppState.agentModelBySession,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 833 - 839, Restore path currently assigns serverSettings.agentModelBySession without capping, allowing unbounded entries; mirror the save/sync behavior by trimming to the 50 most-recent entries during restore. Update the restore logic that sets agentModelBySession (the block using serverSettings.agentModelBySession and migratePhaseModelEntry) to apply the same cap as getSettingsFieldValue (i.e., limit to 50 entries) — either by calling the same trimming helper used by getSettingsFieldValue or by taking Object.entries(serverSettings.agentModelBySession), preserving migratePhaseModelEntry mapping, and slicing to the last 50 entries before converting back with Object.fromEntries so agentModelBySession and currentAppState stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 840-844: Update the misleading comment above the
currentWorktreeByProject restoration to accurately state that
sanitizeWorktreeByProject performs structural validation only and preserves both
null and non-null paths; it does not strip stale/deleted worktree paths —
runtime validation (e.g., in useWorktrees or similar) handles removal of deleted
worktrees. Reference the sanitizeWorktreeByProject function and
currentWorktreeByProject usage in use-settings-sync.ts when replacing the
two-comment lines so readers understand where deletion checks actually occur.
- Around line 605-615: The optimistic localStorage write in
forceSyncSettingsToServer (setItem('automaker-settings-cache',
JSON.stringify(updates))) happens before the server call
(api.settings.updateGlobal) and is asymmetric with syncToServer; to fix, move
the setItem call to after the await and only write when result.success is true
(mirror the logic in syncToServer), or if optimistic write is deliberate keep it
but add an explicit comment in forceSyncSettingsToServer explaining the
intentional divergence from syncToServer and why it’s required for race
conditions; update the code around forceSyncSettingsToServer,
api.settings.updateGlobal, and setItem accordingly so the invariant is
consistent or documented.
In `@libs/git-utils/src/status.ts`:
- Around line 174-176: The current merge detection treats any .git/rebase-apply
as 'rebase' causing false positives for `git am`; update the logic in
libs/git-utils/src/status.ts (the code that sets mergeOperationType / returns
MergeStateInfo) to distinguish 'am' by checking for the presence of
.git/rebase-apply/applying (e.g., use fs.access or equivalent to test
path.join(gitDir, 'rebase-apply', 'applying')); when present return
mergeOperationType 'am' (or add 'am' to the MergeStateInfo union and return it),
otherwise keep 'rebase' as before and preserve existing behavior for other
states like MERGE_HEAD and CHERRY_PICK_HEAD.
---
Nitpick comments:
In `@apps/server/src/lib/settings-helpers.ts`:
- Around line 37-40: DEFAULT_MAX_TURNS was changed to equal MAX_ALLOWED_TURNS,
causing a 10× jump in the default run limit and removing the headroom between
typical default and the hard ceiling; revert or lower the default to a safer
value (e.g., 1000 or 2000) while keeping MAX_ALLOWED_TURNS at 10000 so the hard
cap remains higher than the out-of-the-box default. Locate the constants
DEFAULT_MAX_TURNS and MAX_ALLOWED_TURNS in settings-helpers.ts and set
DEFAULT_MAX_TURNS to the chosen lower default (and leave MAX_ALLOWED_TURNS at
10000), adding a short comment that explains the rationale (default vs ceiling)
to prevent accidental parity in future edits.
- Around line 83-124: The two near-identical functions
getUseClaudeCodeSystemPromptSetting and getAutoLoadClaudeMdSetting should be
consolidated by adding a generic helper (e.g.,
getBooleanSettingWithProjectFallback) that accepts (projectPath: string,
settingsService?: SettingsService|null, logPrefix: string, projectKey: keyof
ProjectSettings, globalKey: keyof GlobalSettings, defaultValue = true) and
implements the current project-then-global lookup, logging, and error handling;
replace both existing functions with thin wrappers that call this helper
(passing the appropriate keys and default) so code is centralized, type-safe
(use appropriate keyof types or a typed generic), and preserves current
logging/error behavior via the same logger and parameter names.
In `@apps/server/src/services/agent-executor.ts`:
- Line 41: Remove the local redeclaration of DEFAULT_MAX_TURNS in
agent-executor.ts and use the exported constant from settings-helpers.ts
instead; replace the local const DEFAULT_MAX_TURNS = 10000 with the imported
symbol (the file already imports settings-helpers) so all usages inside
functions/methods in this module (references to DEFAULT_MAX_TURNS used across
agent execution paths) rely on the single canonical export to avoid shadowing
and future drift.
- Around line 223-225: The code currently mutates caller-supplied
options.sdkSessionId inside execute(), which is fragile; instead introduce a
local mutable variable (e.g., currentSdkSessionId) initialized from
options.sdkSessionId, update that local variable when msg.session_id is present,
and pass currentSdkSessionId explicitly into buildExecOpts and into downstream
functions like handleSpecGenerated and executeTasksLoop rather than writing back
to options; do this for all four streaming sites that touch options.sdkSessionId
to remove the side-effect while preserving behavior.
In `@apps/server/tests/unit/lib/enhancement-prompts.test.ts`:
- Around line 194-200: The test currently checks only that each prompt contains
the input text and is long enough; add an assertion that for additive
enhancement modes ('technical', 'acceptance', 'ux-reviewer') the prompt returned
by buildUserPrompt(mode, testText) includes the additive instruction phrase
"Generate ONLY the additional details" (or the exact phrase used in the prompt
templates) so the change for Issue `#803` is actually covered; update the it block
iterating ENHANCEMENT_MODES to detect those specific mode values and assert
prompt.includes('Generate ONLY the additional details') for them while keeping
existing assertions for all modes.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 833-839: Restore path currently assigns
serverSettings.agentModelBySession without capping, allowing unbounded entries;
mirror the save/sync behavior by trimming to the 50 most-recent entries during
restore. Update the restore logic that sets agentModelBySession (the block using
serverSettings.agentModelBySession and migratePhaseModelEntry) to apply the same
cap as getSettingsFieldValue (i.e., limit to 50 entries) — either by calling the
same trimming helper used by getSettingsFieldValue or by taking
Object.entries(serverSettings.agentModelBySession), preserving
migratePhaseModelEntry mapping, and slicing to the last 50 entries before
converting back with Object.fromEntries so agentModelBySession and
currentAppState stay consistent.
In `@libs/prompts/tests/enhancement.test.ts`:
- Around line 332-349: The test "all modes" omits the new 'ux-reviewer' mode;
update the modes declaration in the test (the Array<'improve' | 'technical' |
'simplify' | 'acceptance'> and the array value in the describe block) to include
'ux-reviewer' so buildUserPrompt('ux-reviewer', testText) is exercised; either
extend the union to Array<'improve' | 'technical' | 'simplify' | 'acceptance' |
'ux-reviewer'> or relax the type to include the new mode and add 'ux-reviewer'
to the modes array used in the forEach.
- Around line 1-17: Add tests asserting the new public exports isAdditiveMode,
ADDITIVE_MODES, and REWRITE_MODES: create a new describe block that checks
ADDITIVE_MODES contains 'technical', 'acceptance', 'ux-reviewer' and does not
contain 'improve' or 'simplify'; checks REWRITE_MODES contains 'improve' and
'simplify'; iterates ADDITIVE_MODES to assert isAdditiveMode(mode) === true and
iterates REWRITE_MODES to assert isAdditiveMode(mode) === false. Reference the
exported symbols isAdditiveMode, ADDITIVE_MODES, and REWRITE_MODES from the
enhancement tests and add these assertions alongside the existing enhancement
test suite.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/server/src/lib/settings-helpers.tsapps/server/src/services/agent-executor.tsapps/server/tests/unit/lib/enhancement-prompts.test.tsapps/ui/src/hooks/use-settings-sync.tsapps/ui/src/store/app-store.tslibs/git-utils/src/status.tslibs/prompts/tests/enhancement.test.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/src/store/app-store.ts
- libs/types/src/settings.ts
| // Update localStorage cache immediately so a page reload before the | ||
| // server response arrives still sees the latest state (e.g. after | ||
| // deleting a worktree, the stale worktree path won't survive in cache). | ||
| try { | ||
| setItem('automaker-settings-cache', JSON.stringify(updates)); | ||
| } catch (storageError) { | ||
| logger.warn('Failed to update localStorage cache during force sync:', storageError); | ||
| } | ||
|
|
||
| const result = await api.settings.updateGlobal(updates); | ||
| return result.success; |
There was a problem hiding this comment.
forceSyncSettingsToServer writes localStorage cache before server confirmation — asymmetric with syncToServer
The localStorage cache is updated optimistically at line 609 before the server call at line 614. In syncToServer (lines 351–358), the cache is only written after result.success. If the server call fails here, the localStorage cache reflects state the server never accepted. On the next app start, if the server is reachable, the server's (older) state overrides the cache — but during the window where the server is unavailable and the cache is used as a fallback (e.g., Electron offline mode), the cache and server diverge.
The intent is valid (preserve "latest" state across a reload that races the server response), but the behaviour should be explicit, and the two sync paths ideally share the same invariant.
💡 Option: write cache only on success (consistent with `syncToServer`)
- // Update localStorage cache immediately so a page reload before the
- // server response arrives still sees the latest state (e.g. after
- // deleting a worktree, the stale worktree path won't survive in cache).
- try {
- setItem('automaker-settings-cache', JSON.stringify(updates));
- } catch (storageError) {
- logger.warn('Failed to update localStorage cache during force sync:', storageError);
- }
-
const result = await api.settings.updateGlobal(updates);
+ if (result.success) {
+ try {
+ setItem('automaker-settings-cache', JSON.stringify(updates));
+ } catch (storageError) {
+ logger.warn('Failed to update localStorage cache during force sync:', storageError);
+ }
+ }
return result.success;If the optimistic write is intentional (to protect against a reload racing the server response for destructive ops like worktree deletion), add a comment that explicitly documents the deliberate inconsistency with syncToServer.
📝 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.
| // Update localStorage cache immediately so a page reload before the | |
| // server response arrives still sees the latest state (e.g. after | |
| // deleting a worktree, the stale worktree path won't survive in cache). | |
| try { | |
| setItem('automaker-settings-cache', JSON.stringify(updates)); | |
| } catch (storageError) { | |
| logger.warn('Failed to update localStorage cache during force sync:', storageError); | |
| } | |
| const result = await api.settings.updateGlobal(updates); | |
| return result.success; | |
| const result = await api.settings.updateGlobal(updates); | |
| if (result.success) { | |
| try { | |
| setItem('automaker-settings-cache', JSON.stringify(updates)); | |
| } catch (storageError) { | |
| logger.warn('Failed to update localStorage cache during force sync:', storageError); | |
| } | |
| } | |
| return result.success; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 605 - 615, The
optimistic localStorage write in forceSyncSettingsToServer
(setItem('automaker-settings-cache', JSON.stringify(updates))) happens before
the server call (api.settings.updateGlobal) and is asymmetric with syncToServer;
to fix, move the setItem call to after the await and only write when
result.success is true (mirror the logic in syncToServer), or if optimistic
write is deliberate keep it but add an explicit comment in
forceSyncSettingsToServer explaining the intentional divergence from
syncToServer and why it’s required for race conditions; update the code around
forceSyncSettingsToServer, api.settings.updateGlobal, and setItem accordingly so
the invariant is consistent or documented.
| // Sanitize: only restore entries with path === null (main branch). | ||
| // Non-null paths may reference deleted worktrees, causing crash loops. | ||
| currentWorktreeByProject: sanitizeWorktreeByProject( | ||
| serverSettings.currentWorktreeByProject ?? currentAppState.currentWorktreeByProject | ||
| ), |
There was a problem hiding this comment.
Misleading comment — sanitizeWorktreeByProject does NOT filter non-null paths
Lines 840–841 state:
// Sanitize: only restore entries with path === null (main branch).
// Non-null paths may reference deleted worktrees, causing crash loops.
However, sanitizeWorktreeByProject in apps/ui/src/lib/settings-utils.ts (lines 14–32) explicitly preserves non-null paths — it performs structural validation only, and its own comment reads "keep both null (main) and non-null (non-main) worktrees; Runtime validation in use-worktrees.ts handles deleted worktrees." The comment here creates a false safety guarantee: a reader will believe that stale/deleted worktree paths are stripped at this layer when they are not.
📝 Corrected comment
- // Sanitize: only restore entries with path === null (main branch).
- // Non-null paths may reference deleted worktrees, causing crash loops.
- currentWorktreeByProject: sanitizeWorktreeByProject(
+ // Structurally validate entries (both main/null and non-main/non-null paths
+ // are preserved). Runtime validation in use-worktrees.ts handles stale/deleted
+ // worktree paths.
+ currentWorktreeByProject: sanitizeWorktreeByProject(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/hooks/use-settings-sync.ts` around lines 840 - 844, Update the
misleading comment above the currentWorktreeByProject restoration to accurately
state that sanitizeWorktreeByProject performs structural validation only and
preserves both null and non-null paths; it does not strip stale/deleted worktree
paths — runtime validation (e.g., in useWorktrees or similar) handles removal of
deleted worktrees. Reference the sanitizeWorktreeByProject function and
currentWorktreeByProject usage in use-settings-sync.ts when replacing the
two-comment lines so readers understand where deletion checks actually occur.
| * Checks for .git/MERGE_HEAD, .git/rebase-merge, .git/rebase-apply, | ||
| * and .git/CHERRY_PICK_HEAD to determine if a merge/rebase/cherry-pick | ||
| * is in progress. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code in the file
cd /tmp && git clone https://github.com/automaker-ai/automaker.git 2>/dev/null || true
cat -n libs/git-utils/src/status.ts | head -210 | tail -80Repository: AutoMaker-Org/automaker
Length of output: 127
🌐 Web query:
git am state directory .git/rebase-apply documentation
💡 Result:
git am keeps its in-progress “apply” state in the rebase-apply directory under Git’s directory, i.e. effectively:
.git/rebase-apply/(or, more generally,$(git rev-parse --git-path rebase-apply))
That’s why commands like git rebase will refuse to start if they detect rebase-apply exists: the rebase script explicitly checks for an “am in progress” state directory. [2]
Relevant upstream docs/background:
git rev-parse --git-path <path>is the documented way to resolve paths likerebase-applyrelative to$GIT_DIR(and it correctly handles worktrees / relocated git dirs). [3]git rebase’s apply backend works by generating patches and then applying them by callinggit aminternally—this is the backend that uses the “am/apply” state machinery. [1]- When
git amstops mid-series (e.g., conflict), the documented recovery isgit am --continue,--skip, or--abort(and it won’t start a new mailbox until the current operation is finished). [4]
Sources:
[1] git-rebase(1) manual (apply backend calls git am) (kernel.org)
[2] git-rebase.sh (checks $apply_dir / rebase-apply to detect git am in progress) (chromium.googlesource.com)
[3] git-rev-parse(1) manual (--git-path resolves $GIT_DIR/<path>) (kernel.org)
[4] git-am(1) manual (continue/skip/abort behavior; refuses new mailboxes until finished) (git-scm.com)
Citations:
- 1: https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html?utm_source=openai
- 2: https://chromium.googlesource.com/external/github.com/git/git/%2B/85eb0f162c5e47d37221496030dc49d10f3554a2/git-rebase.sh?utm_source=openai
- 3: https://www.kernel.org/pub/software/scm/git/docs/git-rev-parse.html?utm_source=openai
- 4: https://git-scm.com/docs/git-am.html?utm_source=openai
🌐 Web query:
git rebase --apply state directory differences git rebase -i
💡 Result:
git rebase has two “backends” that keep their in-progress state in different .git/ directories:
- Apply backend (
--apply): replays commits by generating/applying patches (internally viagit am). During the rebase it stores state under.git/rebase-apply/(including the patch series). (kernel.org) - Merge backend (default for many cases, and what interactive uses): replays commits using merge machinery and a todo list. During the rebase it stores state under
.git/rebase-merge/(including the interactivegit-rebase-todolist). (kernel.org)
Key implication for git rebase -i:
-i/--interactiveis incompatible with--apply(you can’t force interactive rebase to use the apply backend via--apply). (kernel.org)
Practical check: if you’re “stuck in a rebase”, the presence of .git/rebase-apply/ vs .git/rebase-merge/ tells you which backend is mid-flight. (stackoverflow.com)
Citations:
- 1: https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html
- 2: https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html
- 3: https://www.kernel.org/pub/software/scm/git/docs/git-rebase.html?utm_source=openai
- 4: https://stackoverflow.com/questions/47308609/low-level-difference-between-git-rebase-git-rebase-i-and-git-merge?utm_source=openai
🌐 Web query:
git rebase-apply directory structure files distinguish git am
💡 Result:
rebase-apply exists because both git rebase (when using the apply backend) and git am use the same “apply a series of patches” machinery, and they store their in-progress state under $GIT_DIR/rebase-apply/ (often .git/rebase-apply/). Git also has a separate merge backend that stores state under rebase-merge/. (git-scm.com)
How to distinguish git am vs git rebase when you see rebase-apply/
Presence of rebase-apply/ alone is not enough.
A common/robust distinction is:
git amin progress:rebase-apply/andrebase-apply/applyingexistsgit rebase(apply-backend) in progress:rebase-apply/exists butrebase-apply/applyingdoes not existgit rebase(merge-backend, incl. many interactive cases):rebase-merge/exists (stackoverflow.com)
Example check (handles non-standard .git locations/worktrees):
gitdir=$(git rev-parse --git-path .)
test -f "$(git rev-parse --git-path rebase-apply/applying)" && echo "git am in progress"
test -d "$(git rev-parse --git-path rebase-apply)" && echo "apply-backend (am-style) operation in progress"
test -d "$(git rev-parse --git-path rebase-merge)" && echo "merge-backend rebase in progress"Why rebase-apply/ vs rebase-merge/ happens
git rebase has two backends:
- apply backend → patch-application workflow (historically associated with “am-style”) → uses
rebase-apply/(git-scm.com) - merge backend → cherry-pick/merge machinery → uses
rebase-merge/(git-scm.com)
Citations:
- 1: https://git-scm.com/docs/git-rebase/2.40.0?utm_source=openai
- 2: https://stackoverflow.com/questions/3921409/how-to-know-if-there-is-a-git-rebase-in-progress?utm_source=openai
- 3: https://git-scm.com/docs/git-rebase/2.40.0?utm_source=openai
- 4: https://git-scm.com/docs/git-rebase/2.40.0?utm_source=openai
🏁 Script executed:
# Try to find and inspect the status.ts file
find . -name "status.ts" -path "*/git-utils/*" 2>/dev/null | head -5Repository: AutoMaker-Org/automaker
Length of output: 97
🏁 Script executed:
# List the directory structure
ls -la libs/ 2>/dev/null || ls -la src/ 2>/dev/null || find . -type d -name "git-utils" 2>/dev/null | head -3Repository: AutoMaker-Org/automaker
Length of output: 723
🏁 Script executed:
# Search for the detectMergeState function in any TypeScript files
rg "detectMergeState" --type ts --type tsx -A 15 2>/dev/null | head -80Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Look for the checks array that was mentioned in the review
rg "MERGE_HEAD|rebase-merge|rebase-apply|CHERRY_PICK_HEAD" --type ts -B 2 -A 2 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 6526
rebase-apply is shared by both git am and git rebase --apply, causing incorrect 'rebase' classification when git am is in progress
git am uses .git/rebase-apply/ as its state directory, identical to the apply-backend of git rebase. The current implementation will incorrectly report mergeOperationType: 'rebase' when a git am is interrupted by a conflict. Any UI surfacing this label (e.g. "Rebase in progress") would mislead the user.
Distinguish git am from git rebase --apply by checking for .git/rebase-apply/applying, which exists only for git am:
💡 Suggested distinguishing heuristic
// After detecting rebase-apply exists:
// git-am sets .git/rebase-apply/applying; git rebase --apply does not
let mergeOperationType: 'rebase' | 'am' = 'rebase';
try {
await fs.access(path.join(gitDir, 'rebase-apply', 'applying'));
mergeOperationType = 'am';
} catch {
mergeOperationType = 'rebase';
}Or add 'am' as a variant to MergeStateInfo type and return it when detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/git-utils/src/status.ts` around lines 174 - 176, The current merge
detection treats any .git/rebase-apply as 'rebase' causing false positives for
`git am`; update the logic in libs/git-utils/src/status.ts (the code that sets
mergeOperationType / returns MergeStateInfo) to distinguish 'am' by checking for
the presence of .git/rebase-apply/applying (e.g., use fs.access or equivalent to
test path.join(gitDir, 'rebase-apply', 'applying')); when present return
mergeOperationType 'am' (or add 'am' to the MergeStateInfo union and return it),
otherwise keep 'rebase' as before and preserve existing behavior for other
states like MERGE_HEAD and CHERRY_PICK_HEAD.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/tests/projects/overview-dashboard.spec.ts (2)
319-367:⚠️ Potential issue | 🟡 MinorTest body does not match its title — navigation is not exercised.
The test only asserts that the project card is visible, which is already verified in
should display project status cards. The actual click + board navigation path is not tested, meaning a regression in the card's click handler would go undetected.The comment on line 364 acknowledges this, but the title still implies coverage it doesn't provide. At minimum, rename the test to reflect its actual scope (
should display project card for navigation), or extend it to simulate the click and assert the board route.Would you like me to draft the extended version of this test that mocks the project-switch and board APIs, performs the card click, and asserts navigation to
/board?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/overview-dashboard.spec.ts` around lines 319 - 367, The test titled "should navigate to board when clicking on a project card" does not perform the click or assert navigation; update the test to either (A) rename the test to "should display project card for navigation" to reflect current behavior, or (B) extend it to actually exercise the navigation by: mocking the project-switch and board APIs used by the app (the same mock setup pattern used to create overviewMock), using page.locator('[data-testid="project-status-card-test-project-1"]') to click the card, waiting for network responses from the project-switch/board endpoints, and asserting the URL includes '/board' (or that a board-specific locator is visible); reference functions/helpers in the file such as handleLoginScreenIfPresent and makeOverviewResponse to reuse login and mock setup.
149-153:⚠️ Potential issue | 🟡 MinorReplace
waitForTimeoutwith a condition-based wait.
page.waitForTimeout(300)is a bare time delay — it's both slower than necessary and potentially insufficient on loaded CI. Since the Dashboard button is checked for visibility immediately after (line 157), thewaitForTimeoutis redundant; theexpect(...).toBeVisible()call already handles the async delay.🛠️ Proposed fix
if (await expandSidebarButton.isVisible()) { await expandSidebarButton.click(); - await page.waitForTimeout(300); } // Click on the Dashboard link in the sidebar (navigates to /overview) const overviewLink = page.getByRole('button', { name: 'Dashboard' }); await expect(overviewLink).toBeVisible({ timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/tests/projects/overview-dashboard.spec.ts` around lines 149 - 153, The explicit sleep after clicking the expandSidebarButton is unnecessary and flaky; remove the await page.waitForTimeout(300) and replace it with a condition-based wait that ensures the sidebar has expanded — e.g. after await expandSidebarButton.click() add a wait for the target element to appear such as await page.waitForSelector('button:has-text("Dashboard")', { state: 'visible' }) or use await expect(page.locator('button:has-text("Dashboard")')).toBeVisible(); this uses the expandSidebarButton and the Dashboard button locator referenced in the test.
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view.tsx (1)
1016-1096:⚠️ Potential issue | 🟠 Major
providerIdstill not forwarded through Quick Add and template handlersThe
modelEntryparameter type in bothhandleQuickAddandhandleQuickAddAndStartstill omitsproviderId. WhenhandleTemplateSelectpassestemplate.model(which may carryproviderIdfor provider-specific templates), the field exists at runtime but is structurally invisible and is not forwarded tohandleAddFeature. Provider-specific model selections from templates are silently dropped.🛠️ Proposed fix
- async ( - description: string, - modelEntry: { model: string; thinkingLevel?: string; reasoningEffort?: string } - ) => { + async ( + description: string, + modelEntry: { + model: string; + thinkingLevel?: string; + reasoningEffort?: string; + providerId?: string; + } + ) => {Apply the same change to
handleQuickAddAndStart, and forwardproviderIdin both payload objects:+ providerId: modelEntry.providerId, branchName: addFeatureUseSelectedWorktreeBranch ? selectedWorktreeBranch : undefined,🤖 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 1016 - 1096, The modelEntry parameter and payloads in handleQuickAdd and handleQuickAddAndStart drop providerId; update the modelEntry parameter type for both handlers to include providerId and forward modelEntry.providerId into the objects passed to handleAddFeature and handleAddAndStartFeature (and ensure handleTemplateSelect continues to pass template.model unchanged) so provider-specific templates keep their providerId through handleQuickAdd, handleQuickAddAndStart, handleTemplateSelect, handleAddFeature and handleAddAndStartFeature.
🧹 Nitpick comments (8)
apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx (1)
626-633: Consider disablingModelOverrideTriggerwhile generation is in-flight.The sibling "Generate with AI" button is disabled during generation, but
ModelOverrideTriggerremains interactive. Changing the model mid-flight is harmless (the in-flight call already captured the model at click time), but the asymmetry is mildly confusing to the user.✨ Optional: sync disabled state
<ModelOverrideTrigger currentModelEntry={prDescriptionModelOverride.effectiveModelEntry} onModelChange={prDescriptionModelOverride.setOverride} phase="prDescriptionModel" isOverridden={prDescriptionModelOverride.isOverridden} size="sm" variant="icon" + disabled={isGeneratingDescription} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx` around lines 626 - 633, Disable the ModelOverrideTrigger while AI generation is in-flight by wiring its disabled (or isDisabled) prop to the same generation state used by the "Generate with AI" button (e.g., isGenerating / isGeneratingDescription or the existing generation flag that controls that button), so the component becomes non-interactive during generation; update the <ModelOverrideTrigger ... /> instance that uses prDescriptionModelOverride.{effectiveModelEntry,setOverride,isOverridden} to receive disabled={isGenerating} (or the matching state variable) so the UI state remains consistent.apps/ui/src/hooks/use-settings-migration.ts (2)
762-762:phaseModelsdefault-value drift: persisted values will shadow future DEFAULT_PHASE_MODELS updates to existing keys.The spread
{ ...DEFAULT_PHASE_MODELS, ...(settings.phaseModels ?? current.phaseModels) }is correct for new keys (they get defaults until the user saves). However, sincebuildSettingsUpdateFromStoreimmediately persistsstate.phaseModels(the merged object) back to the server, every existing key in DEFAULT_PHASE_MODELS gets locked into the persisted settings on the next sync. If a future release changes the default value for an existingDEFAULT_PHASE_MODELSkey, already-migrated users won't receive it — their server-persisted value silently wins.This is an accepted trade-off (user customization survives upgrades) but worth documenting in the comment above this line so future maintainers don't add "just change the default" and wonder why it has no effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-migration.ts` at line 762, The merge for phaseModels in buildSettingsUpdateFromStore (phaseModels: { ...DEFAULT_PHASE_MODELS, ...(settings.phaseModels ?? current.phaseModels) }) persists the merged object back to the server which causes existing DEFAULT_PHASE_MODELS keys to be locked to the persisted values and prevents future default-value changes from taking effect; add a concise comment directly above this line in use-settings-migration.ts explaining that this merge intentionally prefers persisted/user values over future default changes (i.e., a deliberate trade-off: user customizations survive upgrades), so maintainers understand why changing DEFAULT_PHASE_MODELS alone won’t update already-migrated users.
188-188:useClaudeCodeSystemPromptparsed from localStorage but not preserved inmergeSettings.
enableAiCommitMessages,enableSkills,enableSubagents, and siblings all have explicit preservation clauses inmergeSettings(lines 380–397) so a legacyfalsevalue survives the first migration.useClaudeCodeSystemPromptis parsed (line 188) but has no corresponding clause, so during migration the server-sideundefinedis overridden totrueby the hydration default regardless of what was in localStorage.Since this appears to be a new field (no legacy Zustand data expected), the practical impact is negligible today. Adding the clause now keeps the pattern consistent and protects against edge cases:
♻️ Proposed addition in
mergeSettings(after line 396)+ if (localSettings.useClaudeCodeSystemPrompt != null && merged.useClaudeCodeSystemPrompt == null) { + merged.useClaudeCodeSystemPrompt = localSettings.useClaudeCodeSystemPrompt; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-settings-migration.ts` at line 188, The parsed setting useClaudeCodeSystemPrompt is read from localStorage but missing the preservation clause in mergeSettings, so legacy false values get overridden by the hydration default; open the mergeSettings function and add a preservation line for useClaudeCodeSystemPrompt (mirroring existing clauses like enableAiCommitMessages/enableSkills/enableSubagents) so that when state.useClaudeCodeSystemPrompt exists it is preserved during migration instead of being replaced by the default/hydrated value.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (4)
677-693: Large inline type forfeatureData— consider aligning withAddFeatureDialog's submission typeThe anonymous inline type duplicates (and must manually stay in sync with) the shape that
AddFeatureDialogpasses to itsonAddprop. IfAddFeatureDialogexports its callback type, importing it here eliminates the duplication and guarantees the signatures stay aligned.🤖 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 677 - 693, The anonymous inline type for the featureData parameter duplicates the submission shape used by AddFeatureDialog; import the exported callback/submission type from AddFeatureDialog (e.g., AddFeatureDialogProps or AddFeatureSubmission) and replace the inline type in the async handler signature with that imported type (use the exact exported type name from AddFeatureDialog). Update the parameter annotation in the async function that currently accepts featureData to use the imported type and remove the inline type block so the types stay in sync with AddFeatureDialog's onAdd callback.
657-668: No-op.filter((line) => line !== undefined)— consider filtering empty trailing strings insteadEvery element in the array is a string (the ternary produces
'', notundefined), so the filter never removes anything. If the goal is to suppress the blank line when the selection is not truncated, filter online !== ''or handle the conditional element differently.♻️ Proposed fix
const description = [ `**File:** \`${relativePath}\` (Lines ${selection.fromLine}-${selection.toLine})`, '', `\`\`\`${fenceLang}`, codeText, '```', - truncated ? `\n*Selection truncated (${lines.length} lines total)*` : '', + ...(truncated ? [`\n*Selection truncated (${lines.length} lines total)*`] : []), '', '---', '', ] - .filter((line) => line !== undefined) .join('\n');🤖 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 657 - 668, The array building for description uses .filter((line) => line !== undefined) which is a no-op because all elements are strings; change the conditional truncated element to only include the truncated message when needed (e.g., spread a conditional array like ...(truncated ? [ `\n*Selection truncated (${lines.length} lines total)*` ] : []) ) so you don't rely on filtering, and then join the array into the final string; update the code around the description variable (references: description, truncated, lines, fenceLang, selection) to remove the ineffective filter and correctly omit the blank line when not truncated.
1665-1679: Mobile diff-toggle uses a raw<button>while all other actions inHeaderActionsPaneluse<Button>Every other action in this panel (search, undo/redo, save, create-feature) uses
<Button variant="outline">. Swapping for the shared component also picks up focus-ring, disabled-state, and keyboard-navigation styles for free.♻️ Proposed fix
- <button - onClick={() => setShowInlineDiff(!showInlineDiff)} - className={cn( - 'flex items-center gap-2 w-full p-2 rounded-lg border transition-colors text-sm', - showInlineDiff - ? 'bg-primary/10 border-primary/30 text-primary' - : 'bg-muted/30 border-border text-muted-foreground hover:text-foreground' - )} - > - <Diff className="w-4 h-4" /> - <span>{showInlineDiff ? 'Hide Git Diff' : 'Show Git Diff'}</span> - </button> + <Button + variant={showInlineDiff ? 'default' : 'outline'} + className="w-full justify-start" + onClick={() => setShowInlineDiff(!showInlineDiff)} + > + <Diff className="w-4 h-4 mr-2" /> + {showInlineDiff ? 'Hide Git Diff' : 'Show Git Diff'} + </Button>🤖 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 1665 - 1679, Replace the raw <button> used for the inline diff toggle with the shared Button component (use variant="outline") so it inherits focus-ring, disabled-state and keyboard styles; keep the existing conditional rendering (activeTab && !activeTab.isBinary && !activeTab.isTooLarge), preserve the onClick handler (setShowInlineDiff(!showInlineDiff)), the Diff icon (<Diff />) and the label (showInlineDiff ? 'Hide Git Diff' : 'Show Git Diff'), and replicate the current className-based visual states (using the cn expression) by mapping them into the Button's className prop so behavior and appearance remain the same.
499-511:handleSavecapturesshowInlineDiffvia closure — inconsistent withsaveTabByIdand resets the auto-save timer on diff toggle
handleSavelistsshowInlineDiffas a dependency (line 511), so toggling the diff view recreateshandleSave, which in turn causes the auto-save timer effect (line 587) to re-run and reset its countdown — cancelling any in-progress auto-save.saveTabByIdalready solves the exact same problem by reading fromgetState()(line 528). Apply the same pattern here:♻️ Proposed fix
if (result.success) { markTabSaved(activeTab.id, activeTab.content); // Refresh git status and inline diff after save loadGitStatus(); - if (showInlineDiff) { + const { showInlineDiff: currentShowInlineDiff } = useFileEditorStore.getState(); + if (currentShowInlineDiff) { loadFileDiff(activeTab.filePath); } } ... - }, [activeTab, markTabSaved, loadGitStatus, showInlineDiff, loadFileDiff]); + }, [activeTab, markTabSaved, loadGitStatus, loadFileDiff]);🤖 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 499 - 511, handleSave currently closes over showInlineDiff which forces handleSave to be recreated when the diff toggle changes and resets the auto-save timer; change handleSave to read the live flag from the store like saveTabById does: inside handleSave call getState() (or the same selector used in saveTabById) to retrieve the current showInlineDiff value before calling loadFileDiff, then remove showInlineDiff from the handleSave dependency array so handleSave remains stable and does not trigger the auto-save effect to reset; keep other dependencies (activeTab, markTabSaved, loadGitStatus, loadFileDiff) as needed.apps/ui/src/components/views/board-view.tsx (1)
2155-2159: Hard-coded 1500 mssetTimeoutis fragile on slow connectionsThe 1500 ms window before
invalidateQueriesis intended to letcancelQuerieswin the race against in-flight polling responses. On slow or flaky networks a pending polling response can arrive after 1500 ms and overwrite the optimistic update. Consider tying the delay to the actual polling interval (e.g., the refetch interval forqueryKeys.worktrees.all), or replacing the timeout with a controlled refetch that honours query cancellation.🤖 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 2155 - 2159, Replace the fragile hard-coded setTimeout(1500) used before calling queryClient.invalidateQueries with a deterministic synchronization: call and await queryClient.cancelQueries(queryKeys.worktrees.all(projectPathForRefetch)) (or read the actual refetch/polling interval used for that query and use it instead) and only then call queryClient.invalidateQueries(...). In other words, remove the arbitrary 1500ms delay and either await cancelQueries for queryKeys.worktrees.all(projectPathForRefetch) before invalidating, or derive the timeout from the worktrees query's refetch interval so the invalidateQueries call won't lose to a late polling response. Ensure you update the code around setTimeout, cancelQueries, and queryClient.invalidateQueries accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 2169-2175: The call to forceSyncSettingsToServer() is missing
error handling and can produce an unhandled rejection; update the invocation in
the board-view component so the returned promise is caught (either by appending
a .catch(...) that logs the error via logger.warn/error with context, or by
using await inside an async function with try/catch) and keep the existing
success .then(...) behavior (referencing forceSyncSettingsToServer and
logger.warn in your fix).
- Around line 428-444: The batchResetBranchFeatures callback sets updates to {
branchName: undefined } which is stripped by JSON.stringify and thus not
persisted; change the updates payload to use { branchName: null } so the server
records the cleared field, and replace console.error in the persistFeatureUpdate
catch with the module logger.error for consistent logging; update references
inside batchResetBranchFeatures (the updates variable and the catch handler
calling persistFeatureUpdate) accordingly.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 639-650: The current fallback for fenceLang uses || which treats
the explicit empty-string mapping for 'plain text' as falsy and falls back to
langName; change the logic so that presence in langMap is checked explicitly
(e.g., use "langName in langMap" or
Object.prototype.hasOwnProperty.call(langMap, langName)) and if present use
langMap[langName] (even if it's ''), otherwise use langName; update the
assignment for fenceLang to use this explicit membership check referencing
langMap, fenceLang, and langName.
---
Outside diff comments:
In `@apps/ui/tests/projects/overview-dashboard.spec.ts`:
- Around line 319-367: The test titled "should navigate to board when clicking
on a project card" does not perform the click or assert navigation; update the
test to either (A) rename the test to "should display project card for
navigation" to reflect current behavior, or (B) extend it to actually exercise
the navigation by: mocking the project-switch and board APIs used by the app
(the same mock setup pattern used to create overviewMock), using
page.locator('[data-testid="project-status-card-test-project-1"]') to click the
card, waiting for network responses from the project-switch/board endpoints, and
asserting the URL includes '/board' (or that a board-specific locator is
visible); reference functions/helpers in the file such as
handleLoginScreenIfPresent and makeOverviewResponse to reuse login and mock
setup.
- Around line 149-153: The explicit sleep after clicking the expandSidebarButton
is unnecessary and flaky; remove the await page.waitForTimeout(300) and replace
it with a condition-based wait that ensures the sidebar has expanded — e.g.
after await expandSidebarButton.click() add a wait for the target element to
appear such as await page.waitForSelector('button:has-text("Dashboard")', {
state: 'visible' }) or use await
expect(page.locator('button:has-text("Dashboard")')).toBeVisible(); this uses
the expandSidebarButton and the Dashboard button locator referenced in the test.
---
Duplicate comments:
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 1016-1096: The modelEntry parameter and payloads in handleQuickAdd
and handleQuickAddAndStart drop providerId; update the modelEntry parameter type
for both handlers to include providerId and forward modelEntry.providerId into
the objects passed to handleAddFeature and handleAddAndStartFeature (and ensure
handleTemplateSelect continues to pass template.model unchanged) so
provider-specific templates keep their providerId through handleQuickAdd,
handleQuickAddAndStart, handleTemplateSelect, handleAddFeature and
handleAddAndStartFeature.
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 2155-2159: Replace the fragile hard-coded setTimeout(1500) used
before calling queryClient.invalidateQueries with a deterministic
synchronization: call and await
queryClient.cancelQueries(queryKeys.worktrees.all(projectPathForRefetch)) (or
read the actual refetch/polling interval used for that query and use it instead)
and only then call queryClient.invalidateQueries(...). In other words, remove
the arbitrary 1500ms delay and either await cancelQueries for
queryKeys.worktrees.all(projectPathForRefetch) before invalidating, or derive
the timeout from the worktrees query's refetch interval so the invalidateQueries
call won't lose to a late polling response. Ensure you update the code around
setTimeout, cancelQueries, and queryClient.invalidateQueries accordingly.
In `@apps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsx`:
- Around line 626-633: Disable the ModelOverrideTrigger while AI generation is
in-flight by wiring its disabled (or isDisabled) prop to the same generation
state used by the "Generate with AI" button (e.g., isGenerating /
isGeneratingDescription or the existing generation flag that controls that
button), so the component becomes non-interactive during generation; update the
<ModelOverrideTrigger ... /> instance that uses
prDescriptionModelOverride.{effectiveModelEntry,setOverride,isOverridden} to
receive disabled={isGenerating} (or the matching state variable) so the UI state
remains consistent.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 677-693: The anonymous inline type for the featureData parameter
duplicates the submission shape used by AddFeatureDialog; import the exported
callback/submission type from AddFeatureDialog (e.g., AddFeatureDialogProps or
AddFeatureSubmission) and replace the inline type in the async handler signature
with that imported type (use the exact exported type name from
AddFeatureDialog). Update the parameter annotation in the async function that
currently accepts featureData to use the imported type and remove the inline
type block so the types stay in sync with AddFeatureDialog's onAdd callback.
- Around line 657-668: The array building for description uses .filter((line) =>
line !== undefined) which is a no-op because all elements are strings; change
the conditional truncated element to only include the truncated message when
needed (e.g., spread a conditional array like ...(truncated ? [ `\n*Selection
truncated (${lines.length} lines total)*` ] : []) ) so you don't rely on
filtering, and then join the array into the final string; update the code around
the description variable (references: description, truncated, lines, fenceLang,
selection) to remove the ineffective filter and correctly omit the blank line
when not truncated.
- Around line 1665-1679: Replace the raw <button> used for the inline diff
toggle with the shared Button component (use variant="outline") so it inherits
focus-ring, disabled-state and keyboard styles; keep the existing conditional
rendering (activeTab && !activeTab.isBinary && !activeTab.isTooLarge), preserve
the onClick handler (setShowInlineDiff(!showInlineDiff)), the Diff icon (<Diff
/>) and the label (showInlineDiff ? 'Hide Git Diff' : 'Show Git Diff'), and
replicate the current className-based visual states (using the cn expression) by
mapping them into the Button's className prop so behavior and appearance remain
the same.
- Around line 499-511: handleSave currently closes over showInlineDiff which
forces handleSave to be recreated when the diff toggle changes and resets the
auto-save timer; change handleSave to read the live flag from the store like
saveTabById does: inside handleSave call getState() (or the same selector used
in saveTabById) to retrieve the current showInlineDiff value before calling
loadFileDiff, then remove showInlineDiff from the handleSave dependency array so
handleSave remains stable and does not trigger the auto-save effect to reset;
keep other dependencies (activeTab, markTabSaved, loadGitStatus, loadFileDiff)
as needed.
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Line 762: The merge for phaseModels in buildSettingsUpdateFromStore
(phaseModels: { ...DEFAULT_PHASE_MODELS, ...(settings.phaseModels ??
current.phaseModels) }) persists the merged object back to the server which
causes existing DEFAULT_PHASE_MODELS keys to be locked to the persisted values
and prevents future default-value changes from taking effect; add a concise
comment directly above this line in use-settings-migration.ts explaining that
this merge intentionally prefers persisted/user values over future default
changes (i.e., a deliberate trade-off: user customizations survive upgrades), so
maintainers understand why changing DEFAULT_PHASE_MODELS alone won’t update
already-migrated users.
- Line 188: The parsed setting useClaudeCodeSystemPrompt is read from
localStorage but missing the preservation clause in mergeSettings, so legacy
false values get overridden by the hydration default; open the mergeSettings
function and add a preservation line for useClaudeCodeSystemPrompt (mirroring
existing clauses like enableAiCommitMessages/enableSkills/enableSubagents) so
that when state.useClaudeCodeSystemPrompt exists it is preserved during
migration instead of being replaced by the default/hydrated value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/server/src/services/worktree-service.tsapps/ui/nginx.confapps/ui/src/components/session-manager.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/dialogs/create-pr-dialog.tsxapps/ui/src/components/views/file-editor-view/file-editor-view.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/tests/projects/overview-dashboard.spec.tsapps/ui/tests/settings/settings-startup-sync-race.spec.tsapps/ui/tests/utils/navigation/views.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/nginx.conf
- apps/server/src/services/worktree-service.ts
| const batchResetBranchFeatures = useCallback( | ||
| (branchName: string) => { | ||
| const affectedIds = hookFeatures.filter((f) => f.branchName === branchName).map((f) => f.id); | ||
| if (affectedIds.length === 0) return; | ||
| const updates: Partial<Feature> = { branchName: undefined }; | ||
| batchUpdateFeatures(affectedIds, updates); | ||
| for (const id of affectedIds) { | ||
| persistFeatureUpdate(id, updates).catch((err: unknown) => { | ||
| console.error( | ||
| `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`, | ||
| err | ||
| ); | ||
| }); | ||
| } | ||
| }, | ||
| [hookFeatures, batchUpdateFeatures, persistFeatureUpdate] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find useBoardPersistence hook and persistFeatureUpdate implementation
fd -type f -name '*persistence*' '*.ts' '*.tsx' | head -20Repository: AutoMaker-Org/automaker
Length of output: 237
🏁 Script executed:
# Search for persistFeatureUpdate usage and definition
rg 'persistFeatureUpdate' -A 3 -B 3 | head -100Repository: AutoMaker-Org/automaker
Length of output: 7604
🏁 Script executed:
# Find the useBoardPersistence or board persistence hook
rg 'useBoardPersistence|const persistFeatureUpdate' -A 10 | head -150Repository: AutoMaker-Org/automaker
Length of output: 7172
🏁 Script executed:
# Check imports in board-view.tsx to see where persistFeatureUpdate comes from
head -80 apps/ui/src/components/views/board-view.tsx | grep -E 'import|from'Repository: AutoMaker-Org/automaker
Length of output: 1838
🏁 Script executed:
# Read the persistFeatureUpdate implementation
cat -n apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts | head -200Repository: AutoMaker-Org/automaker
Length of output: 9491
🏁 Script executed:
# Get more of the file to see the full persistFeatureUpdate implementation
wc -l apps/ui/src/components/views/board-view/hooks/use-board-persistence.tsRepository: AutoMaker-Org/automaker
Length of output: 141
🏁 Script executed:
# Find getElectronAPI implementation
rg 'getElectronAPI|features\.update' -A 5 apps/ui/src/lib/ | head -80Repository: AutoMaker-Org/automaker
Length of output: 3261
🏁 Script executed:
# Look for the Electron API implementation/preload
fd -name '*electron*' -o -name '*preload*' | grep -v node_modules | head -20Repository: AutoMaker-Org/automaker
Length of output: 297
🏁 Script executed:
# Find where features API is defined
rg 'features.*update' -B 5 -A 10 | grep -E '(function|const|export).*update|features.*:' | head -40Repository: AutoMaker-Org/automaker
Length of output: 2887
🏁 Script executed:
# Read the server-side features update handler
cat -n apps/server/src/routes/features/routes/update.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 3856
🏁 Script executed:
# Check if there's JSON serialization or filtering logic
rg 'JSON.stringify|undefined|null' apps/server/src/routes/features/routes/update.ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 404
🏁 Script executed:
# Find FeatureLoader implementation
rg 'class FeatureLoader|update\(' apps/server/src/services/feature-loader.ts -A 15 | head -100Repository: AutoMaker-Org/automaker
Length of output: 1009
🏁 Script executed:
# Get the file size first
wc -l apps/server/src/services/feature-loader.tsRepository: AutoMaker-Org/automaker
Length of output: 113
🏁 Script executed:
# Read the update method in detail
sed -n '/async update(/,/^ }/p' apps/server/src/services/feature-loader.ts | head -150Repository: AutoMaker-Org/automaker
Length of output: 3184
🏁 Script executed:
# Also check how feature objects are saved/persisted
rg 'save|writeFile|JSON.stringify' apps/server/src/services/feature-loader.ts -B 2 -A 2 | head -80Repository: AutoMaker-Org/automaker
Length of output: 679
🏁 Script executed:
# Find atomicWriteJson implementation
rg 'atomicWriteJson|export.*atomicWriteJson' -B 3 -A 10 | head -60Repository: AutoMaker-Org/automaker
Length of output: 3995
🏁 Script executed:
# Search for the implementation of atomicWriteJson
fd -name '*.ts' -type f | xargs grep -l 'atomicWriteJson' | grep -v node_modulesRepository: AutoMaker-Org/automaker
Length of output: 297
🏁 Script executed:
# Read the atomicWriteJson implementation
cat -n libs/utils/src/atomic-writer.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 3788
🏁 Script executed:
# Check specifically how JSON.stringify is used
rg 'JSON.stringify' libs/utils/src/atomic-writer.ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 134
🏁 Script executed:
# Check if logger is imported in board-view.tsx
head -50 apps/ui/src/components/views/board-view.tsx | grep -i loggerRepository: AutoMaker-Org/automaker
Length of output: 122
🏁 Script executed:
# Verify JSON.stringify behavior with undefined (sanity check)
node -e "console.log(JSON.stringify({ branchName: undefined }))"Repository: AutoMaker-Org/automaker
Length of output: 69
🏁 Script executed:
# Also check if there are other uses of undefined for field clearing in the codebase
rg 'undefined.*persist|persist.*undefined' --type ts --type tsx | head -20Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Verify JSON.stringify behavior with null
node -e "console.log(JSON.stringify({ branchName: null }))"Repository: AutoMaker-Org/automaker
Length of output: 86
🏁 Script executed:
# Check if there are any existing uses of null for field clearing in updates
rg 'null.*persist|{ [^}]*:\s*null' apps/ui/src/components/views/board-view.tsx -A 2 -B 2 | head -30Repository: AutoMaker-Org/automaker
Length of output: 185
🏁 Script executed:
# Check how logger is used in board-view.tsx (any existing logger usage)
rg 'logger\.' apps/ui/src/components/views/board-view.tsx | head -10Repository: AutoMaker-Org/automaker
Length of output: 599
🏁 Script executed:
# Verify the imports at the top of board-view to see if logger is available
sed -n '1,80p' apps/ui/src/components/views/board-view.tsx | grep -E 'createLogger|logger'Repository: AutoMaker-Org/automaker
Length of output: 122
{ branchName: undefined } silently fails to clear the field server-side
When persistFeatureUpdate sends { branchName: undefined }, the value is stripped by JSON.stringify before reaching storage. The merged feature object becomes { ...feature, branchName: undefined }, but upon serialization for disk persistence, JSON.stringify removes the undefined property entirely. This leaves branchName intact on the server. On the next reload, features re-acquire their deleted worktree's branch and vanish from the UI.
The fix is to use null instead of undefined—it serializes as {"branchName":null} and persists correctly. Also replace console.error with the available logger.error for consistency:
🐛 Proposed fix
- const updates: Partial<Feature> = { branchName: undefined };
+ const updates: Partial<Feature> = { branchName: null };
batchUpdateFeatures(affectedIds, updates);
for (const id of affectedIds) {
- persistFeatureUpdate(id, updates).catch((err: unknown) => {
- console.error(
- `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`,
- err
- );
+ persistFeatureUpdate(id, updates).catch((err: unknown) => {
+ logger.error(
+ `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`,
+ err
+ );
});
}📝 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.
| const batchResetBranchFeatures = useCallback( | |
| (branchName: string) => { | |
| const affectedIds = hookFeatures.filter((f) => f.branchName === branchName).map((f) => f.id); | |
| if (affectedIds.length === 0) return; | |
| const updates: Partial<Feature> = { branchName: undefined }; | |
| batchUpdateFeatures(affectedIds, updates); | |
| for (const id of affectedIds) { | |
| persistFeatureUpdate(id, updates).catch((err: unknown) => { | |
| console.error( | |
| `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`, | |
| err | |
| ); | |
| }); | |
| } | |
| }, | |
| [hookFeatures, batchUpdateFeatures, persistFeatureUpdate] | |
| ); | |
| const batchResetBranchFeatures = useCallback( | |
| (branchName: string) => { | |
| const affectedIds = hookFeatures.filter((f) => f.branchName === branchName).map((f) => f.id); | |
| if (affectedIds.length === 0) return; | |
| const updates: Partial<Feature> = { branchName: null }; | |
| batchUpdateFeatures(affectedIds, updates); | |
| for (const id of affectedIds) { | |
| persistFeatureUpdate(id, updates).catch((err: unknown) => { | |
| logger.error( | |
| `[batchResetBranchFeatures] Failed to persist update for feature ${id}:`, | |
| err | |
| ); | |
| }); | |
| } | |
| }, | |
| [hookFeatures, batchUpdateFeatures, persistFeatureUpdate] | |
| ); |
🤖 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 428 - 444, The
batchResetBranchFeatures callback sets updates to { branchName: undefined }
which is stripped by JSON.stringify and thus not persisted; change the updates
payload to use { branchName: null } so the server records the cleared field, and
replace console.error in the persistFeatureUpdate catch with the module
logger.error for consistent logging; update references inside
batchResetBranchFeatures (the updates variable and the catch handler calling
persistFeatureUpdate) accordingly.
| forceSyncSettingsToServer().then((ok) => { | ||
| if (!ok) { | ||
| logger.warn( | ||
| 'forceSyncSettingsToServer failed after worktree deletion; stale path may reappear on reload' | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing .catch() on forceSyncSettingsToServer — unhandled rejection risk
If forceSyncSettingsToServer() rejects, the .then() handler creates a dangling rejected promise that is never caught. In environments with strict unhandled-rejection policies this will surface as an uncaught error.
🛠️ Proposed fix
- forceSyncSettingsToServer().then((ok) => {
- if (!ok) {
- logger.warn(
- 'forceSyncSettingsToServer failed after worktree deletion; stale path may reappear on reload'
- );
- }
- });
+ forceSyncSettingsToServer()
+ .then((ok) => {
+ if (!ok) {
+ logger.warn(
+ 'forceSyncSettingsToServer failed after worktree deletion; stale path may reappear on reload'
+ );
+ }
+ })
+ .catch((err: unknown) => {
+ logger.error('forceSyncSettingsToServer threw after worktree deletion:', err);
+ });🤖 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 2169 - 2175, The
call to forceSyncSettingsToServer() is missing error handling and can produce an
unhandled rejection; update the invocation in the board-view component so the
returned promise is caught (either by appending a .catch(...) that logs the
error via logger.warn/error with context, or by using await inside an async
function with try/catch) and keep the existing success .then(...) behavior
(referencing forceSyncSettingsToServer and logger.warn in your fix).
| const langMap: Record<string, string> = { | ||
| javascript: 'js', | ||
| jsx: 'jsx', | ||
| typescript: 'ts', | ||
| tsx: 'tsx', | ||
| python: 'py', | ||
| ruby: 'rb', | ||
| shell: 'sh', | ||
| 'c++': 'cpp', | ||
| 'plain text': '', | ||
| }; | ||
| const fenceLang = langMap[langName] || langName; |
There was a problem hiding this comment.
fenceLang fallback silently ignores the intentional empty-string mapping for 'plain text'
langMap['plain text'] = '' is meant to produce no fence language, but '' || langName is falsy, so fenceLang becomes 'plain text' — an invalid code-fence identifier. Use an explicit in check instead:
🐛 Proposed fix
- const fenceLang = langMap[langName] || langName;
+ const fenceLang = langName in langMap ? langMap[langName] : langName;🤖 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 639 - 650, The current fallback for fenceLang uses || which treats the
explicit empty-string mapping for 'plain text' as falsy and falls back to
langName; change the logic so that presence in langMap is checked explicitly
(e.g., use "langName in langMap" or
Object.prototype.hasOwnProperty.call(langMap, langName)) and if present use
langMap[langName] (even if it's ''), otherwise use langName; update the
assignment for fenceLang to use this explicit membership check referencing
langMap, fenceLang, and langName.
1 participant
Summary
This PR introduces per-project worktree view customization, allowing users to control how worktrees are displayed in the board view panel. It also ships several quality-of-life improvements across the file editor, diff viewer, and AI enhancement workflow.
Worktree Panel — Pinned Tabs & Swap
WorktreeDropdownwith an updated trigger label (+N more) and ahighlightTriggerprop to distinguish primary vs. secondary dropdowns.pinnedWorktreesCountis persisted to project settings via the server API.Project Settings — Display Settings Section
WorktreePreferencesSectionwith a labelled slider (0–25) to configure the number of pinned worktree tabs.onValueCommit) and persisted to the server.libs/types—ProjectSettingsadditionspinnedWorktreesCountandworktreeDropdownThresholdtoProjectSettings.getDefaultThinkingLevel()now returns'adaptive'for Opus/adaptive-thinking models instead of hardcoded'none'.File Editor Improvements
showInlineDiff/activeFileDiffstate; the editor can now render inline diff decorations via CodeMirrorStateFieldandDecoration.CodeEditorHandlegains agetSelection()method returning{ text, fromLine, toLine }for use by downstream actions (e.g. "Add Feature from selection").AddFeatureDialogintegration allows creating a feature card directly from a code selection in the file editor.getLanguageExtension()moved to a sharedcodemirror-languages.tsmodule to keepcode-editor.tsxfocused.FolderKanbanicon).Diff Viewer — CodeMirror-based Diff
parseDiff()andsplitDiffByFile()extracted fromgit-diff-panel.tsxinto a reusablediff-utils.tsmodule (exportingParsedFileDifftype).CodeMirrorDiffViewcomponent replaces the hand-rolled HTML diff renderer for richer syntax-highlighted diffs.git-diff-panel.tsxsignificantly slimmed down (~250 lines removed).AI Enhancement — Additive vs. Rewrite Modes
improve,simplify): replaces the description.technical,acceptance,ux-reviewer): appends AI-generated content below the original description.isAdditiveMode()helper andREWRITE_MODES/ADDITIVE_MODESarrays exported fromenhancement-constants.ts.Server — Event Hook & Feature Completion
EventHookServicenow handlesfeature:completedevents (emitted for manual feature runs) and maps them tofeature_success/feature_errorhooks.auto_mode_feature_completeevents are now guarded to only fire hooks whenexecutionMode === 'auto', preventing double-firing.AutoModeEventPayloadand a newFeatureCompletedPayloadinterface captureexecutionModecontext.event-hook-service.test.tsupdated to cover the new event handling paths.Other
sdk-options.ts: minor additions.codex-provider.ts: minor additions.features/update.tsroute: minor adjustments.defaultThinkingLevelinitial state changed from'none'to'adaptive'.Summary by CodeRabbit
New Features
UI Improvements
Chores