Fix: Restore views properly, model selection for commit and pr and speed up some cli models with session resume#801
Conversation
📝 WalkthroughWalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Agent as AgentService
participant Provider as Provider (Copilot/Codex/Gemini)
participant SDK as External SDK / CLI
UI->>Agent: Execute request (options, sdkSessionId?)
Agent->>Provider: executeQuery(options with sdkSessionId)
Provider->>SDK: resolve model / build prompt
alt sdkSessionId provided
Provider->>SDK: attempt resume (resumeSession / exec resume)
alt resume succeeds
SDK-->>Provider: resumed session stream
else resume fails
SDK-->>Provider: resume error
Provider->>SDK: createSession / exec (normal)
SDK-->>Provider: new session stream
end
else no sdkSessionId
Provider->>SDK: createSession / exec (normal)
SDK-->>Provider: new session stream
end
Provider-->>Agent: forward stream events (include session_id)
Agent-->>UI: aggregated streaming responses (updates sdkSessionId as received)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by improving the persistence and reactivity of several key UI elements. It ensures that user selections for worktrees and AI models are reliably restored across sessions and refreshes, preventing data loss and improving workflow continuity. Additionally, it introduces greater flexibility in AI model selection for code generation tasks and streamlines the underlying architecture for AI-powered features, making them more robust and accessible in different environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to view restoration, model selection persistence, and session resume support for CLI providers. The refactoring of commit message generation to use the HTTP API client enables better support for web mode. The addition of session resume for Codex, Copilot, Cursor, and Gemini is a significant performance optimization for multi-turn conversations. Feedback is provided regarding potential issues with image handling and schema validation during resumed Codex sessions, as well as redundant context transmission.
| const schemaPath = isResumeQuery | ||
| ? null | ||
| : await writeOutputSchemaFile(options.cwd, options.outputFormat); |
There was a problem hiding this comment.
Skipping the output schema file generation when isResumeQuery is true might cause issues if a resumed turn still requires structured JSON output (e.g., during an interrupted feature generation task). If the CLI expects the --output-schema flag to enforce the response format on every turn, omitting it could lead to unparseable output.
| if ( | ||
| !isResumeQuery && | ||
| codexSettings.additionalDirs && | ||
| codexSettings.additionalDirs.length > 0 | ||
| ) { | ||
| for (const dir of codexSettings.additionalDirs) { | ||
| preExecArgs.push(CODEX_ADD_DIR_FLAG, dir); | ||
| } | ||
| } | ||
|
|
||
| // If images were written to disk, add the image directory so the CLI can access them | ||
| if (imagePaths.length > 0) { | ||
| if (!isResumeQuery && imagePaths.length > 0) { | ||
| const imageDir = path.join(options.cwd, CODEX_INSTRUCTIONS_DIR, IMAGE_TEMP_DIR); | ||
| preExecArgs.push(CODEX_ADD_DIR_FLAG, imageDir); | ||
| } |
There was a problem hiding this comment.
The !isResumeQuery check prevents adding the image directory and additional directories to the CLI arguments when resuming a session. If a follow-up turn in a multi-turn conversation includes new images or requires access to these directories, the Codex CLI may fail to access them because the --add-dir flags are omitted. Consider including these flags even for resumed queries if the CLI is stateless regarding directory permissions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
apps/server/src/providers/copilot-provider.ts (1)
567-597:⚠️ Potential issue | 🟠 MajorResolve model aliases before Copilot SDK calls.
Line 568 passes
bareModeldirectly into the SDK. Per repo rules, model aliases must be normalized viaresolveModelString()before API calls. This avoids alias-only values like “sonnet” being sent to the SDK.🛠️ Suggested fix
+import { resolveModelString } from '@automaker/model-resolver'; ... - const bareModel = options.model || DEFAULT_BARE_MODEL; + const bareModel = options.model || DEFAULT_BARE_MODEL; + const resolvedModel = resolveModelString(bareModel); ... - const sessionOptions: CopilotSessionOptions = { - model: bareModel, + const sessionOptions: CopilotSessionOptions = { + model: resolvedModel,As per coding guidelines: "Use resolveModelString() from
@automaker/model-resolverto convert model aliases (haiku, sonnet, opus) to full model names before making API calls."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot-provider.ts` around lines 567 - 597, The Copilot SDK is receiving the un-normalized model alias in sessionOptions (see bareModel used when constructing CopilotSessionOptions and passed to client.createSession / resumableClient.resumeSession); call resolveModelString() from `@automaker/model-resolver` on bareModel and use the returned normalized model string in the sessionOptions.model before any SDK calls (both when creating a new session and when resuming via resumeSession when options.sdkSessionId is present).apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (2)
594-601:⚠️ Potential issue | 🟡 Minor
handleOpenChangeshould also block closure during active AI generation.The Cancel button is correctly disabled while
isGenerating, but overlay clicks and the Escape key route throughhandleOpenChange, which only guards againstisPushing. Closing mid-generation allows the in-flight API call to complete and attempt to update state (setMessage) after the dialog is closed.🔧 Suggested fix
const handleOpenChange = (nextOpen: boolean) => { - if (!nextOpen && isPushing) { + if (!nextOpen && (isPushing || isGenerating)) { return; } onOpenChange(nextOpen); };🤖 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 594 - 601, handleOpenChange currently prevents closing only when isPushing; extend its guard to also block closure while isGenerating so overlay clicks/Escape don't close the dialog mid-generation. In the handleOpenChange function (referencing handleOpenChange, isPushing, isGenerating, and onOpenChange), return early when either isPushing or isGenerating is true and only call onOpenChange(nextOpen) when neither is active; this keeps the UI open until generation completes and prevents in-flight API callbacks (e.g., setMessage) from running after the dialog is closed.
541-586:⚠️ Potential issue | 🟠 MajorObject identity instability in
useModelOverridecauses repeated API calls.
useModelOverridereturns a new object literal on every render, even though its individual properties (effectiveModelEntry,effectiveModel) are memoized. IncludingcommitModelOverridein theuseCallbackdependency array makesgenerateCommitMessageunstable. SincegenerateCommitMessageis also a dependency of theuseEffectbelow, the effect re-fires on every render whileopen && worktree && enableAiCommitMessagesare truthy — callinggenerateCommitMessage()(and thus the API) repeatedly. Each re-run also callssetMessage('')first, wiping any in-progress result.Extract the stable memoized values from
commitModelOverridesogenerateCommitMessageonly changes when actual model selection changes:Suggested fix
+ const { + effectiveModel: commitEffectiveModel, + effectiveModelEntry: commitEffectiveModelEntry, + } = commitModelOverride; + const generateCommitMessage = useCallback(async () => { if (!worktree) return; setIsGenerating(true); try { const api = getHttpApiClient(); const result = await api.worktree.generateCommitMessage( worktree.path, - commitModelOverride.effectiveModel, - commitModelOverride.effectiveModelEntry.thinkingLevel, - commitModelOverride.effectiveModelEntry.providerId + commitEffectiveModel, + commitEffectiveModelEntry.thinkingLevel, + commitEffectiveModelEntry.providerId ); // ... } - }, [worktree, commitModelOverride]); + }, [worktree, commitEffectiveModel, commitEffectiveModelEntry]);Also, prevent dialog close via Escape or overlay click during generation by updating
handleOpenChangeto guardisGenerating:const handleOpenChange = (nextOpen: boolean) => { - if (!nextOpen && isPushing) { + if (!nextOpen && (isPushing || isGenerating)) { return; } onOpenChange(nextOpen); };🤖 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 541 - 586, The commit dialog repeatedly triggers generateCommitMessage because commitModelOverride is a new object each render; fix by pulling out the stable memoized values from useModelOverride (e.g., const { effectiveModel, effectiveModelEntry } = commitModelOverride or have useModelOverride return memoized primitives) and use those primitives in the generateCommitMessage useCallback dependency array instead of commitModelOverride so generateCommitMessage only changes when the actual model selection changes; also ensure the opening useEffect depends on those primitives (effectiveModel, effectiveModelEntry) rather than the whole override object and remove the setMessage('') wipe if unnecessary while generation is in progress; finally update handleOpenChange to ignore close requests when isGenerating is true so the dialog cannot be closed via Escape or overlay click during generation.
🧹 Nitpick comments (9)
apps/ui/src/lib/settings-utils.ts (1)
22-28:'path' in worktreepresence guard (line 25) is redundantThe value check on line 28 already covers the absent-key case: when
pathis not present it isundefined, which is neithernullnortypeof 'string', so the entry is rejected. By contrast,branchis validated only viatypeof worktree.branch === 'string'with no separate'in'guard — the two conditions are inconsistently structured.♻️ Proposed cleanup for symmetry
if ( typeof worktree === 'object' && worktree !== null && - 'path' in worktree && 'branch' in worktree && typeof worktree.branch === 'string' && (worktree.path === null || typeof worktree.path === 'string') ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/settings-utils.ts` around lines 22 - 28, The guard "'path' in worktree" is redundant and causes asymmetric checks vs. branch; update the conditional in the worktree validation (the if that inspects worktree) to remove the explicit "'path' in worktree" check and instead rely on the existing type checks (typeof worktree.branch === 'string' and (worktree.path === null || typeof worktree.path === 'string')) so both keys are validated consistently; ensure the condition still checks worktree is an object and not null before the typeof checks on branch and path.apps/server/tests/unit/providers/codex-provider.test.ts (1)
173-192: Assertions for--output-schemaand--add-direxclusion are tautologically true.Lines 190–191 check that
--output-schemaand--add-dirare absent, but since the test provides nooutputFormatoradditionalDirs, these flags wouldn't appear even in a non-resume invocation. To truly validate the resume-specific guards, supply these options and confirm they're excluded only whensdkSessionIdis present:💡 Suggested improvement
await collectAsyncGenerator( provider.executeQuery({ prompt: 'Continue', model: 'gpt-5.2', cwd: '/tmp', sdkSessionId: 'codex-session-123', + outputFormat: { type: 'json_schema', schema: { type: 'object' } }, + codexSettings: { additionalDirs: ['/extra'] }, }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/providers/codex-provider.test.ts` around lines 173 - 192, The test currently asserts that '--output-schema' and '--add-dir' are absent but never supplies outputFormat or additionalDirs, making those assertions meaningless; update the test that calls provider.executeQuery (the "uses exec resume when sdkSessionId is provided" case) to pass outputFormat and additionalDirs (e.g., outputFormat: {...} and additionalDirs: ['...']) so the spawnJSONLProcess invocation would normally include '--output-schema' and '--add-dir', then verify via vi.mocked(spawnJSONLProcess).mock.calls[0][0].args that when sdkSessionId is provided the args still exclude '--output-schema' and '--add-dir' while keeping 'exec', 'resume', '--json', and the session id; this ensures spawnJSONLProcess, provider.executeQuery, and collectAsyncGenerator behavior are correctly tested.apps/server/tests/unit/providers/cursor-provider.test.ts (1)
6-22: Consider adding a negative test and using a standard constructor.Two observations:
Missing negative case: The Gemini test suite includes a test for when
sdkSessionIdis absent. Adding the same here would improve consistency and coverage.
Object.createbypasses the constructor: IfbuildCliArgsever relies on constructor-initialized state beyondcliPath, this test will silently break. AbeforeEachwith proper mocking (as in the Codex and Gemini tests) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/providers/cursor-provider.test.ts` around lines 6 - 22, Add a negative test that verifies buildCliArgs (on CursorProvider) does not include "--resume" when sdkSessionId is omitted, and replace the Object.create(...) instantiation with a real instance created via the CursorProvider constructor (or use a beforeEach to construct and set provider.cliPath) so constructor-initialized state is honored; refer to CursorProvider, buildCliArgs, cliPath, and sdkSessionId when updating the tests.apps/ui/src/store/types/state-types.ts (1)
674-676: Consider pruning stale entries when a session is deleted
agentModelBySessionis never cleaned up whendeleteChatSessionis called, so removed sessions leave orphaned entries in the map. This is negligible in practice (entries are tiny) but a tidy implementation would remove the entry in the delete action.♻️ Suggested cleanup in app-store.ts (outside this file)
// In the deleteChatSession action implementation: deleteChatSession: (sessionId) => set((state) => { + const { [sessionId]: _removed, ...remainingModels } = state.agentModelBySession; return { chatSessions: state.chatSessions.filter((s) => s.id !== sessionId), + agentModelBySession: remainingModels, }; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/types/state-types.ts` around lines 674 - 676, agentModelBySession is never pruned when sessions are removed; update the session deletion logic (the deleteChatSession action in app-store.ts) to also remove the corresponding entry from the agentModelBySession map (use the sessionId passed to deleteChatSession). Ensure the cleanup matches the store API (works with setAgentModelForSession/getAgentModelForSession semantics) and handles missing keys gracefully (no-ops if the sessionId is not present).apps/ui/src/store/ui-cache-store.ts (1)
122-135: Optional: extract duplicated validation into a shared helperThe worktree-entry guard (
typeof worktree === 'object' && … branch check … path check) appears verbatim in bothsyncUICache(line 124) andrestoreFromUICache(line 193). A small inline helper keeps future constraint changes in one place.♻️ Proposed refactor
+// Minimal structural guard for a cached worktree entry +function isValidCachedWorktreeEntry( + w: unknown +): w is { path: string | null; branch: string } { + return ( + typeof w === 'object' && + w !== null && + 'path' in w && + 'branch' in w && + typeof (w as Record<string, unknown>).branch === 'string' && + ((w as Record<string, unknown>).path === null || + typeof (w as Record<string, unknown>).path === 'string') + ); +}Then replace both inline guards with
if (isValidCachedWorktreeEntry(worktree)).Also applies to: 189-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/ui-cache-store.ts` around lines 122 - 135, Extract the duplicated runtime guard for worktree entries into a small helper function (e.g., isValidCachedWorktreeEntry) that accepts the unknown worktree value and returns a boolean after checking the same conditions currently repeated (object/non-null, has 'path' and 'branch', branch is string, path is null or string); then replace the inline guards in syncUICache (where sanitized is built) and restoreFromUICache (the corresponding guard around lines ~189-207) with if (isValidCachedWorktreeEntry(worktree)) so all validation logic lives in one place and future changes only require updating that helper.apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts (3)
108-118: Duplicated model-restoration logic — consider extracting a helper.The model-restoration logic (check persisted model → set or fallback to default) appears both here (lines 112-117) and in
handleSelectSession(lines 52-61). A small helper likerestoreModelForSession(sessionId)would DRY this up and prevent future drift between the two paths.Not blocking, just a readability improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts` around lines 108 - 118, The model-restoration logic is duplicated between the initialization block (using getLastSelectedSession, getAgentModelForSession, setModelSelectionState, setCurrentSessionId) and the interactive path in handleSelectSession; extract that behavior into a small helper function (e.g., restoreModelForSession(sessionId)) that encapsulates "fetch persisted model for sessionId and setModelSelectionState or fallback", then call that helper from both the init code and handleSelectSession to DRY up the logic and keep behavior consistent.
52-62: Info-level logging may be noisy in production.Line 56 logs the full
persistedModelobject atinfolevel on every session switch. Consider downgrading todebugto reduce noise, especially since this fires on every session selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts` around lines 52 - 62, The code currently logs the full persistedModel at info level on every session switch; change the logger.info call in the session restoration block (the one that calls getAgentModelForSession and setModelSelectionState when sessionId is present) to logger.debug and avoid logging the entire persistedModel object (log sessionId and a concise marker like "restored model" or a small identifier instead) so normal production logs are quieter while preserving useful debug output; ensure the else branch still uses DEFAULT_MODEL_SELECTION and setModelSelectionState as before.
28-33: Consider using a Zustand selector to avoid full-store subscription.
useAppStore()without a selector subscribes to the entire store, so any state change (even unrelated) will re-render the component using this hook. Since you only need four stable action references, a shallow selector would eliminate unnecessary re-renders:const { setLastSelectedSession, getLastSelectedSession, setAgentModelForSession, getAgentModelForSession } = useAppStore( useShallow(s => ({ setLastSelectedSession: s.setLastSelectedSession, getLastSelectedSession: s.getLastSelectedSession, setAgentModelForSession: s.setAgentModelForSession, getAgentModelForSession: s.getAgentModelForSession, })) );This is a pre-existing pattern, so not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts` around lines 28 - 33, Replace the full-store subscription call to useAppStore() with a selector that picks only the four action/getter functions to avoid unnecessary re-renders; specifically, select setLastSelectedSession, getLastSelectedSession, setAgentModelForSession and getAgentModelForSession via a selector (and use a shallow comparator) so the hook only subscribes to those stable references instead of the entire store.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
547-553: Add optional chaining oneffectiveModelEntryaccess.Same potential null dereference as flagged in
create-pr-dialog.tsx—commitModelOverride.effectiveModelEntryis accessed without a null guard.🛡️ Suggested fix
- commitModelOverride.effectiveModelEntry.thinkingLevel, - commitModelOverride.effectiveModelEntry.providerId + commitModelOverride.effectiveModelEntry?.thinkingLevel, + commitModelOverride.effectiveModelEntry?.providerId🤖 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 547 - 553, The call to api.worktree.generateCommitMessage uses commitModelOverride.effectiveModelEntry without null checks and can crash if that entry is null; update the arguments to use optional chaining on effectiveModelEntry (e.g., commitModelOverride.effectiveModelEntry?.thinkingLevel and commitModelOverride.effectiveModelEntry?.providerId) before passing them into generateCommitMessage (keeping the existing commitModelOverride.effectiveModel argument as-is) so the call safely handles a missing effectiveModelEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 797-802: The code writes image files even when resuming
(isResumeQuery based on options.sdkSessionId) by calling extractImageBlocks and
writeImageFiles, but the resume path does not add the image directory to the
Codex CLI args (the --add-dir logic), making those images inaccessible; fix by
either skipping image extraction/writing when options.sdkSessionId is truthy
(short-circuit extractImageBlocks/writeImageFiles when isResumeQuery) or ensure
the resume CLI arg construction still includes the image directory when
imagePaths is non-empty (add the same --add-dir behavior used for non-resume
flows); update the branches around isResumeQuery, extractImageBlocks,
writeImageFiles, and the code that builds the CLI args to keep behavior
consistent.
In `@apps/ui/src/lib/settings-utils.ts`:
- Line 12: Update the documentation comment above the settings entry filtering
function in settings-utils.ts to accurately reflect all rejected cases: state
that entries are dropped when they are not an object, when branch is missing or
not a string, and when path is present but is neither null nor a string; change
the parenthetical to something like "(not an object, missing/invalid path or
branch)" so the doc matches the actual guard logic.
In `@apps/ui/src/store/app-store.ts`:
- Around line 1602-1611: The store field agentModelBySession is not being
persisted; add 'agentModelBySession' to the SETTINGS_FIELDS_TO_SYNC array in
use-settings-sync.ts and update refreshSettingsFromServer() to restore
agentModelBySession from the server response (similar to how
lastSelectedSessionByProject is restored) so model selections survive page
refreshes; reference the SETTINGS_FIELDS_TO_SYNC constant and the
refreshSettingsFromServer() function when making the change.
---
Outside diff comments:
In `@apps/server/src/providers/copilot-provider.ts`:
- Around line 567-597: The Copilot SDK is receiving the un-normalized model
alias in sessionOptions (see bareModel used when constructing
CopilotSessionOptions and passed to client.createSession /
resumableClient.resumeSession); call resolveModelString() from
`@automaker/model-resolver` on bareModel and use the returned normalized model
string in the sessionOptions.model before any SDK calls (both when creating a
new session and when resuming via resumeSession when options.sdkSessionId is
present).
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 594-601: handleOpenChange currently prevents closing only when
isPushing; extend its guard to also block closure while isGenerating so overlay
clicks/Escape don't close the dialog mid-generation. In the handleOpenChange
function (referencing handleOpenChange, isPushing, isGenerating, and
onOpenChange), return early when either isPushing or isGenerating is true and
only call onOpenChange(nextOpen) when neither is active; this keeps the UI open
until generation completes and prevents in-flight API callbacks (e.g.,
setMessage) from running after the dialog is closed.
- Around line 541-586: The commit dialog repeatedly triggers
generateCommitMessage because commitModelOverride is a new object each render;
fix by pulling out the stable memoized values from useModelOverride (e.g., const
{ effectiveModel, effectiveModelEntry } = commitModelOverride or have
useModelOverride return memoized primitives) and use those primitives in the
generateCommitMessage useCallback dependency array instead of
commitModelOverride so generateCommitMessage only changes when the actual model
selection changes; also ensure the opening useEffect depends on those primitives
(effectiveModel, effectiveModelEntry) rather than the whole override object and
remove the setMessage('') wipe if unnecessary while generation is in progress;
finally update handleOpenChange to ignore close requests when isGenerating is
true so the dialog cannot be closed via Escape or overlay click during
generation.
---
Nitpick comments:
In `@apps/server/tests/unit/providers/codex-provider.test.ts`:
- Around line 173-192: The test currently asserts that '--output-schema' and
'--add-dir' are absent but never supplies outputFormat or additionalDirs, making
those assertions meaningless; update the test that calls provider.executeQuery
(the "uses exec resume when sdkSessionId is provided" case) to pass outputFormat
and additionalDirs (e.g., outputFormat: {...} and additionalDirs: ['...']) so
the spawnJSONLProcess invocation would normally include '--output-schema' and
'--add-dir', then verify via vi.mocked(spawnJSONLProcess).mock.calls[0][0].args
that when sdkSessionId is provided the args still exclude '--output-schema' and
'--add-dir' while keeping 'exec', 'resume', '--json', and the session id; this
ensures spawnJSONLProcess, provider.executeQuery, and collectAsyncGenerator
behavior are correctly tested.
In `@apps/server/tests/unit/providers/cursor-provider.test.ts`:
- Around line 6-22: Add a negative test that verifies buildCliArgs (on
CursorProvider) does not include "--resume" when sdkSessionId is omitted, and
replace the Object.create(...) instantiation with a real instance created via
the CursorProvider constructor (or use a beforeEach to construct and set
provider.cliPath) so constructor-initialized state is honored; refer to
CursorProvider, buildCliArgs, cliPath, and sdkSessionId when updating the tests.
In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts`:
- Around line 108-118: The model-restoration logic is duplicated between the
initialization block (using getLastSelectedSession, getAgentModelForSession,
setModelSelectionState, setCurrentSessionId) and the interactive path in
handleSelectSession; extract that behavior into a small helper function (e.g.,
restoreModelForSession(sessionId)) that encapsulates "fetch persisted model for
sessionId and setModelSelectionState or fallback", then call that helper from
both the init code and handleSelectSession to DRY up the logic and keep behavior
consistent.
- Around line 52-62: The code currently logs the full persistedModel at info
level on every session switch; change the logger.info call in the session
restoration block (the one that calls getAgentModelForSession and
setModelSelectionState when sessionId is present) to logger.debug and avoid
logging the entire persistedModel object (log sessionId and a concise marker
like "restored model" or a small identifier instead) so normal production logs
are quieter while preserving useful debug output; ensure the else branch still
uses DEFAULT_MODEL_SELECTION and setModelSelectionState as before.
- Around line 28-33: Replace the full-store subscription call to useAppStore()
with a selector that picks only the four action/getter functions to avoid
unnecessary re-renders; specifically, select setLastSelectedSession,
getLastSelectedSession, setAgentModelForSession and getAgentModelForSession via
a selector (and use a shallow comparator) so the hook only subscribes to those
stable references instead of the entire store.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 547-553: The call to api.worktree.generateCommitMessage uses
commitModelOverride.effectiveModelEntry without null checks and can crash if
that entry is null; update the arguments to use optional chaining on
effectiveModelEntry (e.g.,
commitModelOverride.effectiveModelEntry?.thinkingLevel and
commitModelOverride.effectiveModelEntry?.providerId) before passing them into
generateCommitMessage (keeping the existing commitModelOverride.effectiveModel
argument as-is) so the call safely handles a missing effectiveModelEntry.
In `@apps/ui/src/lib/settings-utils.ts`:
- Around line 22-28: The guard "'path' in worktree" is redundant and causes
asymmetric checks vs. branch; update the conditional in the worktree validation
(the if that inspects worktree) to remove the explicit "'path' in worktree"
check and instead rely on the existing type checks (typeof worktree.branch ===
'string' and (worktree.path === null || typeof worktree.path === 'string')) so
both keys are validated consistently; ensure the condition still checks worktree
is an object and not null before the typeof checks on branch and path.
In `@apps/ui/src/store/types/state-types.ts`:
- Around line 674-676: agentModelBySession is never pruned when sessions are
removed; update the session deletion logic (the deleteChatSession action in
app-store.ts) to also remove the corresponding entry from the
agentModelBySession map (use the sessionId passed to deleteChatSession). Ensure
the cleanup matches the store API (works with
setAgentModelForSession/getAgentModelForSession semantics) and handles missing
keys gracefully (no-ops if the sessionId is not present).
In `@apps/ui/src/store/ui-cache-store.ts`:
- Around line 122-135: Extract the duplicated runtime guard for worktree entries
into a small helper function (e.g., isValidCachedWorktreeEntry) that accepts the
unknown worktree value and returns a boolean after checking the same conditions
currently repeated (object/non-null, has 'path' and 'branch', branch is string,
path is null or string); then replace the inline guards in syncUICache (where
sanitized is built) and restoreFromUICache (the corresponding guard around lines
~189-207) with if (isValidCachedWorktreeEntry(worktree)) so all validation logic
lives in one place and future changes only require updating that helper.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
576-588:⚠️ Potential issue | 🟠 MajorModel override change silently resets and re-generates the commit message
generateCommitMessageis listed in this effect's dependency array and is recreated every timecommitEffectiveModelorcommitEffectiveModelEntrychanges (its ownuseCallbackdeps). As a result, when the user changes the model override while the dialog is open, the effect re-fires: it callssetMessage('')(erasing any user edits) and then callsgenerateCommitMessage()again — an unintended auto-regeneration that the user did not request.Fix by stabilising the callback reference with a ref so the effect only fires on meaningful state changes (
open,worktree,enableAiCommitMessages):🐛 Proposed fix
+ const generateCommitMessageRef = useRef(generateCommitMessage); + useEffect(() => { + generateCommitMessageRef.current = generateCommitMessage; + }); // Generate AI commit message when dialog opens (if enabled) useEffect(() => { if (open && worktree) { // Reset state setMessage(''); setError(null); if (!enableAiCommitMessages) { return; } - generateCommitMessage(); + generateCommitMessageRef.current(); } - }, [open, worktree, enableAiCommitMessages, generateCommitMessage]); + }, [open, worktree, enableAiCommitMessages]);🤖 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 576 - 588, The effect currently depends on generateCommitMessage, which is recreated when commitEffectiveModel or commitEffectiveModelEntry change and causes the dialog to reset and auto-regenerate a message unexpectedly; stabilize the callback by storing generateCommitMessage in a ref (e.g. generateCommitMessageRef) and update that ref whenever the callback changes, then call generateCommitMessageRef.current() from the useEffect and remove generateCommitMessage from the effect dependency array so the effect only runs on open, worktree, and enableAiCommitMessages; keep the existing setMessage('') and setError(null) behavior but ensure user edits aren't erased by model override changes.
🧹 Nitpick comments (5)
apps/ui/src/hooks/use-settings-sync.ts (1)
810-812: ApplymigratePhaseModelEntry()to each value when restoringagentModelBySession.Every other
PhaseModelEntryrestored in this function is passed throughmigratePhaseModelEntry()— including all 11 fields inmigratedPhaseModels(lines 694–718) anddefaultFeatureModel(line 768). The raw pass-through here means stored model aliases from old sessions are never migrated, and could silently break model resolution.♻️ Proposed fix
- agentModelBySession: - serverSettings.agentModelBySession ?? currentAppState.agentModelBySession, + agentModelBySession: serverSettings.agentModelBySession + ? Object.fromEntries( + Object.entries(serverSettings.agentModelBySession as Record<string, unknown>).map( + ([sessionId, entry]) => [sessionId, migratePhaseModelEntry(entry)] + ) + ) + : currentAppState.agentModelBySession,
setAgentModelForSessioninapp-store.tscorrectly performs an immutable update via object spread, so reference-based change detection will work as expected once this migration is in place.🤖 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 810 - 812, The restore logic for agentModelBySession should map each stored PhaseModelEntry through migratePhaseModelEntry before assigning it so old session model aliases are migrated; update the agentModelBySession assignment to take serverSettings.agentModelBySession ?? currentAppState.agentModelBySession and transform each value with migratePhaseModelEntry(...) (preserving keys and null-path filtering already present), ensuring migrated objects are returned so setAgentModelForSession's object-spread updates will detect changes.apps/ui/src/store/ui-cache-store.ts (1)
138-144: Duplicate sanitization logic — consider extracting a shared helperThe same loop/validate/accumulate pattern appears in both
syncUICacheandrestoreFromUICache. Extracting it keeps the guard in one place and makes future changes to the validation logic automatic in both call sites.♻️ Suggested refactor
Add a private helper immediately after
isValidCachedWorktreeEntry:+function sanitizeWorktreeByProject( + raw: Record<string, unknown> +): Record<string, { path: string | null; branch: string }> { + const sanitized: Record<string, { path: string | null; branch: string }> = {}; + for (const [key, worktree] of Object.entries(raw)) { + if (isValidCachedWorktreeEntry(worktree)) { + sanitized[key] = worktree; + } + } + return sanitized; +}Then simplify both call sites:
- const sanitized: Record<string, { path: string | null; branch: string }> = {}; - for (const [projectPath, worktree] of Object.entries(appState.currentWorktreeByProject)) { - if (isValidCachedWorktreeEntry(worktree)) { - sanitized[projectPath] = worktree; - } - } - update.cachedCurrentWorktreeByProject = sanitized; + update.cachedCurrentWorktreeByProject = sanitizeWorktreeByProject( + appState.currentWorktreeByProject as Record<string, unknown> + );- const sanitized: Record<string, { path: string | null; branch: string }> = {}; - for (const [projectPath, worktree] of Object.entries(cache.cachedCurrentWorktreeByProject)) { - if (isValidCachedWorktreeEntry(worktree)) { - sanitized[projectPath] = worktree; - } - } - if (Object.keys(sanitized).length > 0) { - stateUpdate.currentWorktreeByProject = sanitized; - } + const sanitized = sanitizeWorktreeByProject( + cache.cachedCurrentWorktreeByProject as Record<string, unknown> + ); + if (Object.keys(sanitized).length > 0) { + stateUpdate.currentWorktreeByProject = sanitized; + }Also applies to: 198-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/ui-cache-store.ts` around lines 138 - 144, Duplicate sanitization of appState.currentWorktreeByProject exists in syncUICache and restoreFromUICache; extract the loop/validate/accumulate logic into a private helper (e.g., sanitizeWorktreeMap) placed near isValidCachedWorktreeEntry, which takes a Record<string, ...> and returns the sanitized Record<string, ...>; then replace the inline loops in syncUICache and restoreFromUICache to call sanitizeWorktreeMap and assign its result to update.cachedCurrentWorktreeByProject (and any similar target), ensuring all call sites share the same validation path.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
829-851: Regenerate button and model override are shown/enabled regardless ofenableAiCommitMessagesWhen a user has explicitly disabled AI commit messages (the
enableAiCommitMessagessetting), the Regenerate button andModelOverrideTriggerare still fully visible and functional. Clicking Regenerate will call the HTTP API and overwrite whatever the user typed — which feels inconsistent with having opted out of AI generation.Consider hiding or disabling both controls when
enableAiCommitMessagesisfalse, or at minimum gate thegenerateCommitMessagecall:- disabled={isGenerating || isLoading} + disabled={isGenerating || isLoading || !enableAiCommitMessages}🤖 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 829 - 851, The Regenerate button and ModelOverrideTrigger ignore the user's enableAiCommitMessages preference; update the UI and handler so AI controls are disabled/hidden and the API call is blocked when enableAiCommitMessages is false. Specifically, wrap or conditionally render the Button (the onClick handler generateCommitMessage) and the ModelOverrideTrigger (commitModelOverride.* props) based on enableAiCommitMessages, or disable the Button and ModelOverrideTrigger when enableAiCommitMessages is false; additionally guard generateCommitMessage itself to no-op if enableAiCommitMessages is false to prevent accidental HTTP calls.apps/server/src/providers/codex-provider.ts (1)
851-854: Redundant!isResumeQueryguard —imagePathsis already empty in resume mode.Since
imagePathsis set to[]unconditionally whenisResumeQueryis true (line 803),imagePaths.length > 0will always befalsein resume mode. The!isResumeQueryouter check is superfluous.♻️ Proposed simplification
- if (!isResumeQuery && imagePaths.length > 0) { + if (imagePaths.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 851 - 854, The outer !isResumeQuery check is redundant because imagePaths is set to [] when isResumeQuery is true; update the condition in the block that pushes CODEX_ADD_DIR_FLAG to only check imagePaths.length > 0 and remove the !isResumeQuery guard, leaving the imageDir computation (path.join(options.cwd, CODEX_INSTRUCTIONS_DIR, IMAGE_TEMP_DIR)) and the preExecArgs.push(CODEX_ADD_DIR_FLAG, imageDir) logic intact so the flag is only added when imagePaths actually contains entries.apps/server/src/providers/copilot-provider.ts (1)
600-601: Log message says "Session created" for both new and resumed sessions.🔧 Suggested fix
- const sessionId = session.sessionId; - logger.debug(`Session created: ${sessionId}`); + const sessionId = session.sessionId; + const isResumed = !!(options.sdkSessionId && typeof resumableClient.resumeSession === 'function'); + logger.debug(`Session ${isResumed ? 'resumed' : 'created'}: ${sessionId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot-provider.ts` around lines 600 - 601, The log always says "Session created" even when a session is resumed; update the logger.debug call to distinguish new vs resumed sessions by inspecting the session object (e.g., check a flag like session.isNew or session.resumed, or compare created/lastAccess timestamps) and log "Session created: <sessionId>" for new sessions and "Session resumed: <sessionId>" for resumed ones; locate the code around sessionId / session.sessionId and replace the single message with a conditional that picks the correct text before calling logger.debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/providers/copilot-provider.ts`:
- Around line 583-598: The resumed/create session may leak if an error occurs
after session creation (e.g., during session.send()); update the outer try/catch
around resumeSession/createSession to track the CopilotSession instance and
ensure session.destroy() is called in the catch (before/alongside client.stop())
when session is truthy; specifically, after calling
resumableClient.resumeSession(...) or client.createSession(...) ensure any later
thrown error triggers session.destroy() (reference symbols: CopilotSession,
resumableClient.resumeSession, client.createSession, session.send,
session.destroy, client.stop).
- Line 526: resolveModelString(options.model || DEFAULT_BARE_MODEL) can return
dash-separated canonical names (e.g., claude-sonnet-4-6) which the Copilot SDK
rejects; normalize the resolved string before assigning to
CopilotSessionOptions.model by converting the version-related dashes to dots
(e.g., transform claude-sonnet-4-6 → claude-sonnet-4.6). Update the code that
sets bareModel (the variable assigned from resolveModelString) so it maps
dash-separated canonical outputs from resolveModelString (and entries coming
from CLAUDE_CANONICAL_MAP) into the dot-separated format expected by
CopilotSessionOptions.model while preserving DEFAULT_BARE_MODEL behavior.
In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts`:
- Around line 85-94: The setModelSelection callback currently writes to local
state but silently skips persistence when currentSessionId is null; make this
intent explicit by adding explicit handling: either persist the choice to a new
"pending" slot (e.g., add a pendingModelSelection state and
setPendingModelSelection(model) when currentSessionId is null) and update
restoreModelForSession to prefer pendingModelSelection before falling back to
DEFAULT_MODEL_SELECTION, or add a clear inline comment above setModelSelection
that the picker must be disabled when there is no session (and enforce that via
UI) so skipping setAgentModelForSession(currentSessionId, model) is intentional.
Ensure changes reference setModelSelection, currentSessionId,
setAgentModelForSession, restoreModelForSession, and DEFAULT_MODEL_SELECTION.
- Around line 55-66: The debug log in restoreModelForSession mistakenly logs the
string 'restored model' instead of the actual persistedModel; update the
logger.debug call inside restoreModelForSession to include the persistedModel
(or its relevant property) along with sessionId so the restored model value is
emitted; keep the rest of the logic intact (getAgentModelForSession,
setModelSelectionState, DEFAULT_MODEL_SELECTION).
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 597-601: The dialog close handler handleOpenChange currently
blocks closing only when isPushing or isGenerating are true; include the
commit-in-flight flag isLoading in that same guard so Escape/overlay clicks are
ignored while a commit is ongoing. Update the conditional inside
handleOpenChange to check (!nextOpen && (isPushing || isGenerating ||
isLoading)) and keep the existing comment/early return behavior so the dialog
cannot be closed mid-commit.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Line 112: agentModelBySession is unbounded and is serialized on every
sync/cache write; cap or prune it to avoid bloat by trimming to a recent window
(e.g., 50 most-recently-used) or applying TTL-based pruning before
serializing/restoring. Update getSettingsFieldValue (and any merge/restore paths
that read/write agentModelBySession) to produce a pruned version of
agentModelBySession: sort or track recency (LRU) and keep only the N most recent
sessionId entries (suggest N=50), and apply the same pruning when saving to
localStorage and when bundling the sync payload so both sync and cache only
carry the trimmed map. Ensure the pruning logic references agentModelBySession
and is run in the serialization/restore routines (getSettingsFieldValue and the
sync payload/cache write hooks).
In `@apps/ui/src/store/ui-cache-store.ts`:
- Around line 88-98: The validator is allowing empty strings for branch; update
isValidCachedWorktreeEntry to require a non-empty branch by adding a check that
(worktree as Record<string, unknown>).branch is a string with length > 0 (e.g.,
trim().length > 0) while keeping the existing path null-or-string logic; modify
the function isValidCachedWorktreeEntry accordingly so corrupted cache entries
with branch === "" are rejected.
---
Outside diff comments:
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 576-588: The effect currently depends on generateCommitMessage,
which is recreated when commitEffectiveModel or commitEffectiveModelEntry change
and causes the dialog to reset and auto-regenerate a message unexpectedly;
stabilize the callback by storing generateCommitMessage in a ref (e.g.
generateCommitMessageRef) and update that ref whenever the callback changes,
then call generateCommitMessageRef.current() from the useEffect and remove
generateCommitMessage from the effect dependency array so the effect only runs
on open, worktree, and enableAiCommitMessages; keep the existing setMessage('')
and setError(null) behavior but ensure user edits aren't erased by model
override changes.
---
Nitpick comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 851-854: The outer !isResumeQuery check is redundant because
imagePaths is set to [] when isResumeQuery is true; update the condition in the
block that pushes CODEX_ADD_DIR_FLAG to only check imagePaths.length > 0 and
remove the !isResumeQuery guard, leaving the imageDir computation
(path.join(options.cwd, CODEX_INSTRUCTIONS_DIR, IMAGE_TEMP_DIR)) and the
preExecArgs.push(CODEX_ADD_DIR_FLAG, imageDir) logic intact so the flag is only
added when imagePaths actually contains entries.
In `@apps/server/src/providers/copilot-provider.ts`:
- Around line 600-601: The log always says "Session created" even when a session
is resumed; update the logger.debug call to distinguish new vs resumed sessions
by inspecting the session object (e.g., check a flag like session.isNew or
session.resumed, or compare created/lastAccess timestamps) and log "Session
created: <sessionId>" for new sessions and "Session resumed: <sessionId>" for
resumed ones; locate the code around sessionId / session.sessionId and replace
the single message with a conditional that picks the correct text before calling
logger.debug.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 829-851: The Regenerate button and ModelOverrideTrigger ignore the
user's enableAiCommitMessages preference; update the UI and handler so AI
controls are disabled/hidden and the API call is blocked when
enableAiCommitMessages is false. Specifically, wrap or conditionally render the
Button (the onClick handler generateCommitMessage) and the ModelOverrideTrigger
(commitModelOverride.* props) based on enableAiCommitMessages, or disable the
Button and ModelOverrideTrigger when enableAiCommitMessages is false;
additionally guard generateCommitMessage itself to no-op if
enableAiCommitMessages is false to prevent accidental HTTP calls.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 810-812: The restore logic for agentModelBySession should map each
stored PhaseModelEntry through migratePhaseModelEntry before assigning it so old
session model aliases are migrated; update the agentModelBySession assignment to
take serverSettings.agentModelBySession ?? currentAppState.agentModelBySession
and transform each value with migratePhaseModelEntry(...) (preserving keys and
null-path filtering already present), ensuring migrated objects are returned so
setAgentModelForSession's object-spread updates will detect changes.
In `@apps/ui/src/store/ui-cache-store.ts`:
- Around line 138-144: Duplicate sanitization of
appState.currentWorktreeByProject exists in syncUICache and restoreFromUICache;
extract the loop/validate/accumulate logic into a private helper (e.g.,
sanitizeWorktreeMap) placed near isValidCachedWorktreeEntry, which takes a
Record<string, ...> and returns the sanitized Record<string, ...>; then replace
the inline loops in syncUICache and restoreFromUICache to call
sanitizeWorktreeMap and assign its result to
update.cachedCurrentWorktreeByProject (and any similar target), ensuring all
call sites share the same validation path.
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Show resolved
Hide resolved
| 'projectHistory', | ||
| 'projectHistoryIndex', | ||
| 'lastSelectedSessionByProject', | ||
| 'agentModelBySession', |
There was a problem hiding this comment.
agentModelBySession has unbounded growth — add a size cap or TTL-based pruning.
agentModelBySession is a Record<sessionId, PhaseModelEntry>. Unlike lastSelectedSessionByProject which is bounded by project count (one entry per project), this map gains a new entry for every agent session ever started. Nothing prunes it.
The full map is serialized into every sync payload (lines 304–306) and into the localStorage cache write (line 338). Active users will accumulate hundreds of entries, progressively bloating settings.json, every debounced write, and every page-reload cache read.
Consider capping the map to a rolling window (e.g., the 50 most-recently-used session IDs) when writing or restoring it, similar to how projectHistory is often capped:
♻️ Suggested pruning in getSettingsFieldValue
if (field === 'autoModeByWorktree') {
// ...
}
+if (field === 'agentModelBySession') {
+ const map = appState.agentModelBySession as Record<string, unknown>;
+ const MAX_ENTRIES = 50;
+ const entries = Object.entries(map);
+ if (entries.length <= MAX_ENTRIES) return map;
+ // Keep the last MAX_ENTRIES entries (insertion-order approximation)
+ return Object.fromEntries(entries.slice(-MAX_ENTRIES));
+}
return appState[field as keyof typeof appState];🤖 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` at line 112, agentModelBySession is
unbounded and is serialized on every sync/cache write; cap or prune it to avoid
bloat by trimming to a recent window (e.g., 50 most-recently-used) or applying
TTL-based pruning before serializing/restoring. Update getSettingsFieldValue
(and any merge/restore paths that read/write agentModelBySession) to produce a
pruned version of agentModelBySession: sort or track recency (LRU) and keep only
the N most recent sessionId entries (suggest N=50), and apply the same pruning
when saving to localStorage and when bundling the sync payload so both sync and
cache only carry the trimmed map. Ensure the pruning logic references
agentModelBySession and is run in the serialization/restore routines
(getSettingsFieldValue and the sync payload/cache write hooks).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts (1)
29-41:useShallowis unnecessary for stable store action references.The four functions selected here (
setLastSelectedSession,getLastSelectedSession, etc.) are created once inside the Zustandcreate()call and never change between renders.useShallowadds a shallow-comparison overhead on every store update to produce the same result as a direct selector. Not a bug, but unnecessary work.♻️ Suggested simplification
- const { - setLastSelectedSession, - getLastSelectedSession, - setAgentModelForSession, - getAgentModelForSession, - } = useAppStore( - useShallow((state) => ({ - setLastSelectedSession: state.setLastSelectedSession, - getLastSelectedSession: state.getLastSelectedSession, - setAgentModelForSession: state.setAgentModelForSession, - getAgentModelForSession: state.getAgentModelForSession, - })) - ); + const setLastSelectedSession = useAppStore((s) => s.setLastSelectedSession); + const getLastSelectedSession = useAppStore((s) => s.getLastSelectedSession); + const setAgentModelForSession = useAppStore((s) => s.setAgentModelForSession); + const getAgentModelForSession = useAppStore((s) => s.getAgentModelForSession);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts` around lines 29 - 41, The selector is wrapping stable Zustand action references with useShallow which is unnecessary; change the useAppStore invocation in use-agent-session to directly select setLastSelectedSession, getLastSelectedSession, setAgentModelForSession, and getAgentModelForSession without the useShallow wrapper so you avoid the extra shallow-comparison overhead while still getting the same stable function references.apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx (1)
583-595: No cancellation for in-flight generation when the dialog is closed and re-opened.If the dialog closes and re-opens rapidly, the effect on line 593 calls
generateCommitMessageRef.current()without cancelling any prior in-flight HTTP request. Both calls could race tosetMessage, and the first (stale) response could overwrite the second. Consider adding anAbortControlleror acancelledflag similar to theloadDiffspattern on line 318.♻️ Suggested cancellation pattern
useEffect(() => { if (open && worktree) { setMessage(''); setError(null); if (!enableAiCommitMessages) { return; } - generateCommitMessageRef.current(); + let cancelled = false; + generateCommitMessageRef.current().then(() => { + // noop — state is set inside the callback + }); + return () => { + cancelled = true; + }; } }, [open, worktree, enableAiCommitMessages]);Note: Full cancellation would require threading an
AbortSignalintogenerateCommitMessageand checkingcancelledbeforesetMessage. The above is a minimal sketch — the real fix should guardsetMessageinsidegenerateCommitMessageitself.🤖 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 583 - 595, The effect that calls generateCommitMessageRef.current() can start overlapping HTTP work when the dialog is closed/re-opened; update the logic so in-flight generation is cancelled before starting a new one (mirror the loadDiffs pattern): create a local cancelled flag or AbortController inside the useEffect, pass an AbortSignal or check cancelled inside generateCommitMessage (and/or generateCommitMessageRef.current) before calling setMessage, and ensure the effect cleanup sets cancelled/aborts the controller so stale responses do not overwrite state when the dialog is reopened.apps/server/src/providers/copilot-provider.ts (1)
660-672: Potential double-destroy ifclient.stop()throws on the happy path.If
activeSession.destroy()(line 660) succeeds butclient.stop()(line 661) throws, control transfers to the catch block. Sincesession === activeSession, the catch block will attemptsession.destroy()on an already-destroyed session (lines 668–670). The inner try-catch swallows any resulting error, so there's no functional breakage — but settingsession = undefinedafter the successful destroy prevents the redundant call.🛡️ Optional: clear `session` after successful destroy
// Cleanup - await activeSession.destroy(); + await activeSession.destroy(); + session = undefined; // prevent redundant destroy in catch if client.stop() throws await client.stop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot-provider.ts` around lines 660 - 672, The try block may call await activeSession.destroy() and then await client.stop(), but if client.stop() throws the catch block will attempt to destroy the same session again because session still references activeSession; after successfully destroying the session in the happy path (the call to activeSession.destroy() in the try), set session = undefined (or null) before calling client.stop() so the catch cleanup won't double-destroy the same session; update references to session and activeSession accordingly so the cleanup logic in the catch only tries to destroy if session still exists.apps/server/src/providers/codex-provider.ts (1)
810-810: Consider whether full conversation history should be sent via stdin in resume mode.
buildCombinedPrompt(line 810) serialisesconversationHistoryinto the stdin payload for every invocation, including resume turns. Ifexec resumeloads the session's existing history from its store and then appends everything from stdin, the exchanged history will be duplicated in the model's context window.If
exec resumeexpects only the new user turn (not the full reconstructed history), consider passing a reduced prompt for resume queries:💡 Possible approach
- const promptText = buildCombinedPrompt(options, combinedSystemPrompt); + const promptText = isResumeQuery + ? buildResumePrompt(options, combinedSystemPrompt) // new user turn only + : buildCombinedPrompt(options, combinedSystemPrompt);Confirm whether
codex exec resumeexpects the full conversation or only the new user turn via stdin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` at line 810, The prompt currently built by calling buildCombinedPrompt (assigned to promptText) always serialises the full conversationHistory into the stdin payload, which can duplicate history when running `codex exec resume`; detect when the request is a resume turn (e.g., check options.mode / options.isResume or the presence of a resume flag on the incoming options) and change the call so resume invocations send a reduced prompt containing only the new user turn (or call a dedicated function like buildResumePrompt) instead of the full reconstructed conversationHistory; update the logic around buildCombinedPrompt/promptText to conditionally build the minimized stdin payload for resume mode so the session store’s history is not re-appended by the model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 543-573: The generateCommitMessage callback is passing
commitEffectiveModel (which may be an alias) directly to
api.worktree.generateCommitMessage; resolve the alias first by calling
resolveModelString(commitEffectiveModel) from `@automaker/model-resolver` and pass
that resolved string to api.worktree.generateCommitMessage instead, keeping the
rest of the call intact (place the resolveModelString call inside
generateCommitMessage before invoking
getHttpApiClient()/api.worktree.generateCommitMessage and update the dependency
array to include commitEffectiveModel if needed).
---
Duplicate comments:
In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts`:
- Around line 55-66: The debug log in restoreModelForSession currently should
log the actual persisted model instead of a literal string; update the
logger.debug call inside function restoreModelForSession (referencing
getAgentModelForSession and setModelSelectionState) to include the
persistedModel variable (and sessionId) so it logs the real object (e.g.,
logger.debug('Restoring model selection for session:', sessionId,
persistedModel)); ensure DEFAULT_MODEL_SELECTION remains used in the else
branch.
- Around line 84-98: The comment confirms the behavior in setModelSelection is
intentional: when currentSessionId is null we only update local state via
setModelSelectionState and intentionally skip persistence via
setAgentModelForSession; no code change is required. Leave the useCallback
implementation (setModelSelection), its dependency array ([currentSessionId,
setAgentModelForSession]), and the explanatory inline comment as-is.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 604-607: The dialog close-prevention needed to account for all
active operations; update handleOpenChange to check isLoading, isPushing, and
isGenerating and return early when nextOpen is false and any of those flags are
true (i.e., keep the existing guard using isLoading || isPushing || isGenerating
inside handleOpenChange) — no further changes required once that condition is
present.
In `@apps/ui/src/hooks/use-settings-sync.ts`:
- Around line 177-187: No change required: the pruning logic in
use-settings-sync.ts that caps agentModelBySession to the last 50 entries (the
block checking field === 'agentModelBySession' and returning
Object.fromEntries(entries.slice(-MAX_ENTRIES))) is acceptable and should be
left as-is in the useSettingsSync hook.
In `@apps/ui/src/store/ui-cache-store.ts`:
- Around line 84-99: The previous bug where empty or whitespace-only branch
strings could pass validation has been fixed in isValidCachedWorktreeEntry by
adding trim().length > 0; no further changes are required—keep the current
checks on branch and path (function isValidCachedWorktreeEntry) as they
correctly reject invalid cached worktree entries.
---
Nitpick comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Line 810: The prompt currently built by calling buildCombinedPrompt (assigned
to promptText) always serialises the full conversationHistory into the stdin
payload, which can duplicate history when running `codex exec resume`; detect
when the request is a resume turn (e.g., check options.mode / options.isResume
or the presence of a resume flag on the incoming options) and change the call so
resume invocations send a reduced prompt containing only the new user turn (or
call a dedicated function like buildResumePrompt) instead of the full
reconstructed conversationHistory; update the logic around
buildCombinedPrompt/promptText to conditionally build the minimized stdin
payload for resume mode so the session store’s history is not re-appended by the
model.
In `@apps/server/src/providers/copilot-provider.ts`:
- Around line 660-672: The try block may call await activeSession.destroy() and
then await client.stop(), but if client.stop() throws the catch block will
attempt to destroy the same session again because session still references
activeSession; after successfully destroying the session in the happy path (the
call to activeSession.destroy() in the try), set session = undefined (or null)
before calling client.stop() so the catch cleanup won't double-destroy the same
session; update references to session and activeSession accordingly so the
cleanup logic in the catch only tries to destroy if session still exists.
In `@apps/ui/src/components/views/agent-view/hooks/use-agent-session.ts`:
- Around line 29-41: The selector is wrapping stable Zustand action references
with useShallow which is unnecessary; change the useAppStore invocation in
use-agent-session to directly select setLastSelectedSession,
getLastSelectedSession, setAgentModelForSession, and getAgentModelForSession
without the useShallow wrapper so you avoid the extra shallow-comparison
overhead while still getting the same stable function references.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 583-595: The effect that calls generateCommitMessageRef.current()
can start overlapping HTTP work when the dialog is closed/re-opened; update the
logic so in-flight generation is cancelled before starting a new one (mirror the
loadDiffs pattern): create a local cancelled flag or AbortController inside the
useEffect, pass an AbortSignal or check cancelled inside generateCommitMessage
(and/or generateCommitMessageRef.current) before calling setMessage, and ensure
the effect cleanup sets cancelled/aborts the controller so stale responses do
not overwrite state when the dialog is reopened.
apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/server/src/routes/worktree/routes/discard-changes.ts (1)
55-69: Deduplicate rename-path parsing by usingparseGitStatus()from@automaker/git-utils.The
parseFilePathfunction (lines 55–69) duplicates the rename-handling logic that already exists inparseGitStatus()(exported from@automaker/git-utils). Both implementations use identical logic:if (indexStatus === 'R' || workTreeStatus === 'R') { const arrowIndex = filePath.indexOf(' -> '); if (arrowIndex !== -1) { filePath = filePath.slice(arrowIndex + 4); } }Your comments on lines 58 and 117 acknowledge this. Instead of reimplementing it locally, import
parseGitStatus(already imported elsewhere in the codebase viacommon.ts) and replace the manual parsing in lines 110–120:const allFiles = parseGitStatus(status).map(f => ({ status: f.status, path: f.path }));This eliminates the duplication and ensures consistency if rename handling ever changes in
git-utils.🤖 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 55 - 69, Remove the local parseFilePath duplication and instead use the existing parseGitStatus helper: import parseGitStatus (via the same export used in common.ts or directly from `@automaker/git-utils`) and replace the manual rename handling where you build allFiles with const allFiles = parseGitStatus(status).map(f => ({ status: f.status, path: f.path })); also delete the parseFilePath function and update any call sites that used it to consume the parsed objects from parseGitStatus so rename logic is centralized.apps/server/src/providers/codex-provider.ts (1)
436-444: Session ID constraint is dead code in CLI resume mode but informational in SDK mode.
buildCodexConstraintsPromptadds the session ID text whenoptions.sdkSessionIdis truthy. However, in CLI resume mode,buildResumePromptis used (line 821) instead ofbuildCombinedPrompt, so the constraint-ladencombinedSystemPromptis never sent to the model. The session ID flows correctly as a CLI argument instead (line 884).In SDK mode (lines 779-787),
combinedSystemPromptIS passed toexecuteCodexSdkQuery, making the session ID text present in the system prompt. However, actual session resumption is handled separately bycodex.resumeThread(options.sdkSessionId), so the text is redundant but not missing from the SDK request.Removing the session ID constraint block would be safe but changes SDK system prompt behavior. If cleaned up, ensure SDK documentation doesn't rely on this informational context. The
CONSTRAINTS_SESSION_ID_LABELconstant at line 143 is only used here and can be removed together with the constraint block.SDK session resumption is properly implemented:
executeCodexSdkQuerycorrectly callscodex.resumeThread()whenoptions.sdkSessionIdis present, with fallback to a new thread on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex-provider.ts` around lines 436 - 444, The session ID constraint added in buildCodexConstraintsPrompt (using CONSTRAINTS_SESSION_ID_LABEL) is dead in CLI resume mode and redundant in SDK mode because SDK resumption is handled by codex.resumeThread in executeCodexSdkQuery; remove the session ID block from buildCodexConstraintsPrompt (and delete the unused CONSTRAINTS_SESSION_ID_LABEL constant) so the system prompt no longer includes the SDK session ID text, but keep codex.resumeThread usage in executeCodexSdkQuery and ensure buildResumePrompt/buildCombinedPrompt behavior is unchanged; also update any SDK documentation/comments that previously relied on the session ID appearing in the system prompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 807-813: When isResumeQuery is true the provided
options.outputFormat is currently ignored silently; update the code around the
isResumeQuery/schemaPath logic to emit a clear warning or assert when
options.outputFormat (or other output-related options) is non-null in resume
mode so callers know their value is dropped. Specifically, add a conditional
after computing isResumeQuery (check isResumeQuery && options.outputFormat) and
call the module's logger (or throw/assert in the surrounding function) with a
message indicating outputFormat is ignored for resumed queries; keep
writeOutputSchemaFile and schemaPath behavior unchanged when not resuming.
In `@apps/ui/src/components/views/board-view/dialogs/commit-worktree-dialog.tsx`:
- Around line 838-854: The Regenerate button's disabled condition currently
checks only isGenerating || isLoading; update its disabled prop to also include
isPushing so it becomes disabled when a push is in flight. Locate the Regenerate
Button in commit-worktree-dialog.tsx (the JSX that references
generateCommitMessage and shows Spinner or RefreshCw) and change its disabled to
combine isGenerating, isLoading, and isPushing (e.g., disabled={isGenerating ||
isLoading || isPushing}) so generation cannot start while
handleCommit/onOpenChange push flows are active.
---
Nitpick comments:
In `@apps/server/src/providers/codex-provider.ts`:
- Around line 436-444: The session ID constraint added in
buildCodexConstraintsPrompt (using CONSTRAINTS_SESSION_ID_LABEL) is dead in CLI
resume mode and redundant in SDK mode because SDK resumption is handled by
codex.resumeThread in executeCodexSdkQuery; remove the session ID block from
buildCodexConstraintsPrompt (and delete the unused CONSTRAINTS_SESSION_ID_LABEL
constant) so the system prompt no longer includes the SDK session ID text, but
keep codex.resumeThread usage in executeCodexSdkQuery and ensure
buildResumePrompt/buildCombinedPrompt behavior is unchanged; also update any SDK
documentation/comments that previously relied on the session ID appearing in the
system prompt.
In `@apps/server/src/routes/worktree/routes/discard-changes.ts`:
- Around line 55-69: Remove the local parseFilePath duplication and instead use
the existing parseGitStatus helper: import parseGitStatus (via the same export
used in common.ts or directly from `@automaker/git-utils`) and replace the manual
rename handling where you build allFiles with const allFiles =
parseGitStatus(status).map(f => ({ status: f.status, path: f.path })); also
delete the parseFilePath function and update any call sites that used it to
consume the parsed objects from parseGitStatus so rename logic is centralized.
| const isResumeQuery = Boolean(options.sdkSessionId); | ||
| const schemaPath = isResumeQuery | ||
| ? null | ||
| : await writeOutputSchemaFile(options.cwd, options.outputFormat); | ||
| const imageBlocks = | ||
| !isResumeQuery && codexSettings.enableImages ? extractImageBlocks(options.prompt) : []; | ||
| const imagePaths = isResumeQuery ? [] : await writeImageFiles(options.cwd, imageBlocks); |
There was a problem hiding this comment.
Image handling in resume mode correctly addressed.
imageBlocks and imagePaths are set to [] when isResumeQuery is true, so the image dir is never added to preExecArgs and there are no orphaned files — this properly resolves the previously flagged issue.
Note: outputFormat (and thus schemaPath) is silently dropped in resume mode (line 808–810). If a caller supplies outputFormat for a resumed query, there's no warning. Consider logging or asserting if options.outputFormat is non-null in resume mode so the caller knows it's ignored.
🔍 Suggested silent-drop guard
+ if (isResumeQuery && options.outputFormat) {
+ logger.warn('[executeQuery] outputFormat is ignored in resume mode');
+ }
const schemaPath = isResumeQuery
? null
: await writeOutputSchemaFile(options.cwd, options.outputFormat);📝 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 isResumeQuery = Boolean(options.sdkSessionId); | |
| const schemaPath = isResumeQuery | |
| ? null | |
| : await writeOutputSchemaFile(options.cwd, options.outputFormat); | |
| const imageBlocks = | |
| !isResumeQuery && codexSettings.enableImages ? extractImageBlocks(options.prompt) : []; | |
| const imagePaths = isResumeQuery ? [] : await writeImageFiles(options.cwd, imageBlocks); | |
| const isResumeQuery = Boolean(options.sdkSessionId); | |
| if (isResumeQuery && options.outputFormat) { | |
| logger.warn('[executeQuery] outputFormat is ignored in resume mode'); | |
| } | |
| const schemaPath = isResumeQuery | |
| ? null | |
| : await writeOutputSchemaFile(options.cwd, options.outputFormat); | |
| const imageBlocks = | |
| !isResumeQuery && codexSettings.enableImages ? extractImageBlocks(options.prompt) : []; | |
| const imagePaths = isResumeQuery ? [] : await writeImageFiles(options.cwd, imageBlocks); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/codex-provider.ts` around lines 807 - 813, When
isResumeQuery is true the provided options.outputFormat is currently ignored
silently; update the code around the isResumeQuery/schemaPath logic to emit a
clear warning or assert when options.outputFormat (or other output-related
options) is non-null in resume mode so callers know their value is dropped.
Specifically, add a conditional after computing isResumeQuery (check
isResumeQuery && options.outputFormat) and call the module's logger (or
throw/assert in the surrounding function) with a message indicating outputFormat
is ignored for resumed queries; keep writeOutputSchemaFile and schemaPath
behavior unchanged when not resuming.
| // For renamed files: XY OLD_PATH -> NEW_PATH | ||
| const statusLines = status.trim().split('\n').filter(Boolean); | ||
| const allFiles = statusLines.map((line) => { | ||
| const fileStatus = line.substring(0, 2); | ||
| const filePath = line.slice(3).trim(); | ||
| const rawPath = line.slice(3); | ||
| const indexStatus = fileStatus.charAt(0); | ||
| const workTreeStatus = fileStatus.charAt(1); | ||
| // Parse path consistently with parseGitStatus() in git-utils, | ||
| // which extracts the new path for renames | ||
| const filePath = parseFilePath(rawPath, indexStatus, workTreeStatus); | ||
| return { status: fileStatus, path: filePath }; | ||
| }); |
There was a problem hiding this comment.
Rename handling improvement looks correct, but selective discard of renames may only partially unstage.
The switch to parseFilePath is a clear improvement — previously the raw "old_path -> new_path" string would be passed to git commands verbatim, which would fail. Now the new path is correctly extracted.
However, for staged renames (R index status), only the new path is tracked. Running git reset HEAD -- <new_path> unstages the addition of the new path but leaves the deletion of the old path staged. To fully unstage a rename you'd need both paths (or git reset HEAD without file args). This is a pre-existing limitation, but the rename-aware parsing makes it more relevant now that renames are actually handled.
| {enableAiCommitMessages && ( | ||
| <> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={generateCommitMessage} | ||
| disabled={isGenerating || isLoading} | ||
| className="h-6 px-2 text-xs" | ||
| title="Regenerate commit message" | ||
| > | ||
| {isGenerating ? ( | ||
| <Spinner size="xs" className="mr-1" /> | ||
| ) : ( | ||
| <RefreshCw className="w-3 h-3 mr-1" /> | ||
| )} | ||
| Regenerate | ||
| </Button> |
There was a problem hiding this comment.
Add isPushing to Regenerate button's disabled check.
The Cancel and Commit buttons both guard on isPushing, but the Regenerate button only checks isGenerating || isLoading. When a push is in flight, the user can still click Regenerate, producing two concurrent spinner states and a wasted API call (the push's handleCommit path calls onOpenChange(false) directly, closing the dialog while generation is still in-flight). This is also confusing in the push-retry state where generating a new commit message is meaningless.
🛡️ Proposed fix
- disabled={isGenerating || isLoading}
+ disabled={isGenerating || isLoading || isPushing}🤖 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 838 - 854, The Regenerate button's disabled condition currently
checks only isGenerating || isLoading; update its disabled prop to also include
isPushing so it becomes disabled when a push is in flight. Locate the Regenerate
Button in commit-worktree-dialog.tsx (the JSX that references
generateCommitMessage and shows Spinner or RefreshCw) and change its disabled to
combine isGenerating, isLoading, and isPushing (e.g., disabled={isGenerating ||
isLoading || isPushing}) so generation cannot start while
handleCommit/onOpenChange push flows are active.
…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
1 participant
Summary
This PR fixes the issue where the selected view/worktree was not being restored after app refresh, and adds several related improvements:
use-worktrees.tsthat gracefully resets to the main branch if a cached worktree no longer exists.useAgentSessionand stored in the app store (agentModelBySession).currentWorktreeInfonow subscribes directly to the store slice instead of calling a getter function, so worktree switches immediately re-render the kanban/list without requiring an extra render cycle.prDescriptionModelphase model: Adds a dedicated configurable model for PR description generation (defaults toclaude-sonnet), alongside the existingcommitMessageModel.ModelOverrideTriggerso users can change the model used for AI generation on the fly, and a regenerate button in the commit dialog.generateCommitMessagecallback.sdkSessionIdto their respective resume/--resumeflags so multi-turn agent conversations continue in the same provider session across Automaker turns. TheAgentExecutortracks the livesession_idemitted by the stream and threads it through all sub-queries.Test plan
npm run test:server -- tests/unit/providers/codex-provider.test.ts tests/unit/providers/copilot-provider.test.ts tests/unit/services/agent-service.test.tsnpm run test:all🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests