Apply upstream roadmap improvements#49
Merged
Merged
Conversation
added 23 commits
June 2, 2026 12:08
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request centralizes auth HTTP route metadata, adds Codex launch-args and provider-scoped discovery, introduces typed voice-transcription auth-expired errors and propagation, implements a plan-review annotation UI and hook, adds terminal/group inline rename and state for title overrides, persists per-turn changed-files expansion state, and ships multiple supporting infra and tests (git Duration types, keybinding migration, socket shutdown cleanup, schema encoders, theme/platform logic, markdown link regex updates). ChangesHTTP Route Centralization
Codex Launch Arguments
Voice Transcription Error Handling
Plan Review and Annotations
Terminal and Group Title Customization
Changed Files Tree Directory Expansion Persistence
Supporting Infrastructure and Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
github-actions
Bot
added
size:XXL
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/appSettings.ts (1)
297-304:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCoalesce optional
launchArgsat the server-settings boundary.
settings.providers.codex.launchArgsis optional in the shared contract, butAppSettings.codexLaunchArgsis a required string. Passing it through verbatim can leave this fieldundefined, which makes the install row look dirty and can flip the input between controlled and uncontrolled when the server omits the property.Suggested fix
export function serverSettingsToAppSettings(settings: ServerSettings): Partial<AppSettings> { return { addProjectBaseDirectory: settings.addProjectBaseDirectory, claudeBinaryPath: settings.providers.claudeAgent.binaryPath, codexBinaryPath: settings.providers.codex.binaryPath, codexHomePath: settings.providers.codex.homePath, - codexLaunchArgs: settings.providers.codex.launchArgs, + codexLaunchArgs: settings.providers.codex.launchArgs ?? "", cursorApiEndpoint: settings.providers.cursor.apiEndpoint,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/appSettings.ts` around lines 297 - 304, serverSettingsToAppSettings currently copies settings.providers.codex.launchArgs (which is optional) directly into the required AppSettings.codexLaunchArgs, risking undefined and uncontrolled inputs; update serverSettingsToAppSettings (the function) to coalesce settings.providers.codex.launchArgs to a safe default (e.g., empty string) when undefined before returning codexLaunchArgs so the AppSettings field is always a string.apps/web/src/appSettings.test.ts (1)
265-306: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd the omitted-field regression for
codex.launchArgs.This suite exercises the populated case, but the shared contract also allows
providers.codex.launchArgsto be absent. Please add a case that omits the property entirely and asserts the mapper normalizes it tocodexLaunchArgs: "", so the settings UI stays clean when the server leaves it out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/appSettings.test.ts` around lines 265 - 306, The test currently only covers the populated case for providers.codex.launchArgs; add a new unit test that omits providers.codex.launchArgs from the input to serverSettingsToAppSettings and assert the mapper returns codexLaunchArgs: "" (i.e., the mapper normalizes a missing providers.codex.launchArgs to an empty string). Locate the existing describe("server-backed app settings") block and add a new it(...) case that constructs server settings without the codex.launchArgs property and uses expect(serverSettingsToAppSettings(...)).toMatchObject({ codexLaunchArgs: "" }); to validate the regression is fixed.apps/web/src/components/WorkspaceView.tsx (1)
317-393:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire rename callbacks and fix missing dependency in WorkspaceView.
Two issues:
Missing rename callbacks: The component passes
groupTitleOverridesById(line 331) but doesn't provideonRenameTerminaloronRenameGroupcallbacks. Users can see override titles but cannot rename terminals or groups via double-click in WorkspaceView. Follow the pattern fromChatViewto add:
onRenameTerminalcallback usingsetTerminalTitleOverrideonRenameGroupcallback usingsetGroupTitleOverrideMissing useMemo dependency:
terminalState.groupTitleOverridesByIdis used in the memoized object (line 331) but not listed in the dependencies array (lines 366-393). Add it to prevent stale values.Suggested fix
+ const setTerminalTitleOverride = useTerminalStateStore((state) => state.setTerminalTitleOverride); + const setGroupTitleOverride = useTerminalStateStore((state) => state.setGroupTitleOverride); const terminalDrawerProps = useMemo( () => ({ // ... existing props groupTitleOverridesById: terminalState.groupTitleOverridesById, + onRenameTerminal: (terminalId: string, name: string) => { + setTerminalTitleOverride(threadId, terminalId, name); + }, + onRenameGroup: (groupId: string, name: string) => { + setGroupTitleOverride(threadId, groupId, name); + }, // ... rest of props }), [ // ... existing dependencies + setTerminalTitleOverride, + setGroupTitleOverride, + terminalState.groupTitleOverridesById, threadId, ], );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/WorkspaceView.tsx` around lines 317 - 393, The terminal drawer props are missing rename callbacks and a useMemo dependency: inside WorkspaceView where terminalDrawerProps is built, add onRenameTerminal that calls setTerminalTitleOverride(threadId, terminalId, title) and onRenameGroup that calls setGroupTitleOverride(threadId, groupId, title) (follow ChatView pattern), and add terminalState.groupTitleOverridesById to the useMemo dependency array so the memo updates when group title overrides change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/codexAppServerManager.ts`:
- Around line 964-970: getOrCreateDiscoverySession currently hardcodes
spawn("codex", ["app-server"]) and therefore ignores configured codexBinaryPath,
codexHomePath and codexLaunchArgs; update getOrCreateDiscoverySession to call
spawn with codexBinaryPath and buildCodexAppServerArgs(codexLaunchArgs) and pass
the same spawn options used when creating normal sessions (cwd/resolvedCwd,
include homePath when present, and any env/launch arg handling). Ensure you
reference and reuse the same helpers/values (codexBinaryPath, codexHomePath,
codexLaunchArgs, buildCodexAppServerArgs) so discovery sessions run against the
configured Codex runtime.
- Around line 402-405: The parser currently unconditionally treats '\' as an
escape prefix (setting escaping = true) and discards it; update the launch-arg
parsing logic in codexAppServerManager.ts (the block using the escaping variable
where it checks if char === "\\") so that a backslash is only consumed as an
escape when it is followed by a valid escapable character (e.g., backslash,
quote, n, r, t, b, f, v, 0); otherwise append the literal '\' to the current
token and continue parsing normally. Concretely, look at the code that sets
escaping = true and change it to peek the next char: if nextChar is in the
escapables list then consume the backslash and set escaping mode, else push a
'\' into the current buffer and do not set escaping. Ensure the rest of the
parser handles escaped sequences as before.
In `@apps/web/src/components/chat/ChangedFilesTree.tsx`:
- Around line 46-68: The initializer currently hardcodes
buildDirectoryExpansionState(directoryPaths, true) so persisted/collapsed state
is never restored; change that call to use the allDirectoriesExpanded flag
(buildDirectoryExpansionState(directoryPaths, allDirectoriesExpanded)) and keep
the loop that sets state[path] = true only for paths in persistedSet so
persisted expanded paths are applied when the default is collapsed. Also update
the useEffect merge logic that builds next from allDirectoryExpansionState so it
does not blindly preserve current true values that are already represented in
allDirectoryExpansionState: set next = { ...allDirectoryExpansionState } and
only copy current true entries into next when the path is not present in
allDirectoryExpansionState (i.e., if current[path] && !(path in next) then
next[path] = true) to allow collapsing when the global default changes.
- Around line 70-88: The setExpandedDirectories updater in toggleDirectory is
impure because it calls readChangedFilesUiState() and
persistChangedFilesUiState(...) inside the functional updater; move all
localStorage persistence out of the updater so the updater only computes and
returns next state. Concretely: have toggleDirectory compute next via
setExpandedDirectories(current => next) (only computing the new map and
returning it), then after state is set persist expandedPaths by deriving them
from the newest expandedDirectories (or use a useEffect keyed on
[expandedDirectories, turnId]) and call
setExpandedDirectoryPathsForTurn/readChangedFilesUiState/persistChangedFilesUiState
there; reference functions: toggleDirectory, setExpandedDirectories,
readChangedFilesUiState, persistChangedFilesUiState,
setExpandedDirectoryPathsForTurn.
In `@apps/web/src/components/chat/ProposedPlanActions.tsx`:
- Line 23: In ProposedPlanActions.tsx remove the redundant "| undefined" from
the optional prop type declarations (e.g., change "onReview?: (() => void) |
undefined" to "onReview?: () => void") and apply the same cleanup for the other
occurrences noted (around the other prop declarations at the locations
referenced, including the block spanning lines ~157-166); update the component's
Prop type/interface and any similar optional prop typings so optional properties
use the "?:" form without "| undefined".
In `@apps/web/src/components/ChatView.tsx`:
- Line 3367: terminalDrawerProps uses terminalState.groupTitleOverridesById but
the memo dependency array omitted it, causing ThreadTerminalDrawer to show stale
titles after onRenameGroup updates the store; include
terminalState.groupTitleOverridesById in the useMemo (or equivalent memo hook)
dependency array wherever terminalDrawerProps is computed (references around
terminalDrawerProps, ThreadTerminalDrawer, and onRenameGroup) so the memo
invalidates and re-renders when groupTitleOverridesById changes.
- Around line 964-965: The plan-review state (planReviewOpen, setPlanReviewOpen,
and usePlanReview()) must be keyed to the active plan so it can’t bleed across
thread/plan switches; update ChatView so the plan review hook and open flag are
tied to the current sidebarProposedPlan (for example by deriving/resetting state
when sidebarProposedPlan.id changes or by storing a per-plan map keyed by plan
id) and ensure you reset/close the panel and re-init the usePlanReview()
instance whenever sidebarProposedPlan changes so annotations and exported review
content cannot persist into a different plan.
In `@apps/web/src/components/PlanSidebar.tsx`:
- Line 50: Remove the redundant "| undefined" from the optional prop type
annotations in the PlanSidebar props interface/props declarations (e.g., change
"onReview?: (() => void) | undefined" and the other occurrences at the same file
to simply "onReview?: () => void" and likewise for the other mentioned optional
props); update the type annotations in the PlanSidebar component (and any
related prop interfaces) so optional props use ?: without the explicit "|
undefined".
In `@apps/web/src/components/terminal/TerminalViewportPane.tsx`:
- Around line 83-126: Extract the duplicated InlineRenameField component into a
single shared module (e.g., InlineRenameField.tsx) and replace the local
definitions in both TerminalViewportPane and TerminalChrome with imports;
preserve the component signature (props: initialValue, onCommit, onCancel,
className), hooks (useState, useRef, useEffect to select), event handlers
(handleKeyDown, handleBlur) and JSX (input attributes including autoFocus and cn
className usage), export it (named or default) from the new module, update both
files to import that exported InlineRenameField, and remove the duplicate local
component definitions so both files use the shared component.
In `@apps/web/src/hooks/usePlanReview.ts`:
- Around line 20-23: Replace the ad-hoc ID generation in addAnnotation with a
UUID from the Web Crypto API: use crypto.randomUUID() to create the id passed
into createPlanAnnotation and setAnnotations; update the addAnnotation callback
(the function named addAnnotation that calls createPlanAnnotation and
setAnnotations) to use crypto.randomUUID() (optionally add a small fallback if
your target runtimes lack crypto.randomUUID).
In `@apps/web/src/planReview.ts`:
- Around line 13-26: The createPlanAnnotation function currently accepts any
string for comment; add a validation that comment.trim().length > 0 and throw a
clear Error (e.g., "comment must be non-empty") if it fails so callers cannot
create annotations with empty comments; keep the existing return shape (id,
createdAt, updatedAt, quote, comment) and reuse the existing quote trimming
logic, and update any tests/usages of createPlanAnnotation accordingly.
- Around line 28-43: The updatePlanAnnotation function must validate provided
comment updates just like createPlanAnnotation does: if updates.comment is
provided, trim it and if the result is an empty string throw a validation error
(e.g., "Comment cannot be empty") instead of accepting it; otherwise use the
trimmed value. Keep existing behavior for undefined fields (leave
annotation.comment untouched), preserve the handling of updates.quote and
updatedAt, and apply this validation inside updatePlanAnnotation to mirror
createPlanAnnotation's logic.
In `@apps/web/src/terminalStateStore.test.ts`:
- Around line 365-401: Add parallel tests for group title overrides mirroring
the terminal tests: use useTerminalStateStore.getState() and call
setGroupTitleOverride(THREAD_ID, groupId, "Title") then assert
selectThreadTerminalState(...).groupTitleOverridesById contains the value; test
removal by setting an empty string and assert the groupTitleOverridesById is
empty; test clearing by creating terminals in a group with
newTerminal(THREAD_ID, terminalId) tied to a group, setGroupTitleOverride, then
close all terminals in that group with closeTerminal(...) and assert
groupTitleOverridesById is cleared; finally add a test for
normalizeGroupTitleOverrides by seeding stale overrides, calling
normalizeGroupTitleOverrides(THREAD_ID) (or triggering the normalization flow)
and asserting only existing group IDs remain. Ensure tests reference
setGroupTitleOverride, normalizeGroupTitleOverrides, newTerminal, closeTerminal,
selectThreadTerminalState, and useTerminalStateStore to locate code.
In `@packages/contracts/src/auth.test.ts`:
- Around line 82-97: The test currently asserts only 4 route entries; extend the
assertion to include all remaining AuthHttpRoutes entries (pairingToken,
pairingLinks, revokePairingLink, clients, revokeClient, revokeOtherClients) by
adding them to the expected object alongside the existing keys, using each
route’s .method and .pathname (e.g., AuthHttpRoutes.pairingToken.method,
AuthHttpRoutes.pairingToken.pathname) so the compare covers all 10 routes and
ensures shared metadata completeness.
---
Outside diff comments:
In `@apps/web/src/appSettings.test.ts`:
- Around line 265-306: The test currently only covers the populated case for
providers.codex.launchArgs; add a new unit test that omits
providers.codex.launchArgs from the input to serverSettingsToAppSettings and
assert the mapper returns codexLaunchArgs: "" (i.e., the mapper normalizes a
missing providers.codex.launchArgs to an empty string). Locate the existing
describe("server-backed app settings") block and add a new it(...) case that
constructs server settings without the codex.launchArgs property and uses
expect(serverSettingsToAppSettings(...)).toMatchObject({ codexLaunchArgs: "" });
to validate the regression is fixed.
In `@apps/web/src/appSettings.ts`:
- Around line 297-304: serverSettingsToAppSettings currently copies
settings.providers.codex.launchArgs (which is optional) directly into the
required AppSettings.codexLaunchArgs, risking undefined and uncontrolled inputs;
update serverSettingsToAppSettings (the function) to coalesce
settings.providers.codex.launchArgs to a safe default (e.g., empty string) when
undefined before returning codexLaunchArgs so the AppSettings field is always a
string.
In `@apps/web/src/components/WorkspaceView.tsx`:
- Around line 317-393: The terminal drawer props are missing rename callbacks
and a useMemo dependency: inside WorkspaceView where terminalDrawerProps is
built, add onRenameTerminal that calls setTerminalTitleOverride(threadId,
terminalId, title) and onRenameGroup that calls setGroupTitleOverride(threadId,
groupId, title) (follow ChatView pattern), and add
terminalState.groupTitleOverridesById to the useMemo dependency array so the
memo updates when group title overrides change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 920d4b59-c1a1-44d8-b9e7-7aebe0da730e
📒 Files selected for processing (64)
apps/desktop/src/desktopServerExposure.test.tsapps/server/src/auth/http.tsapps/server/src/auth/utils.test.tsapps/server/src/auth/utils.tsapps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/effectServer.test.tsapps/server/src/effectServer.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Services/GitCore.tsapps/server/src/keybindings.test.tsapps/server/src/keybindings.tsapps/server/src/provider/providerStatusCache.test.tsapps/server/src/provider/providerStatusCache.tsapps/server/src/serverRuntimeState.tsapps/server/src/voiceTranscription.test.tsapps/server/src/voiceTranscription.tsapps/server/src/wsRpc.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.logic.test.tsapps/web/src/components/ChatView.logic.tsapps/web/src/components/ChatView.tsxapps/web/src/components/PlanReviewPanel.tsxapps/web/src/components/PlanSidebar.tsxapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/components/WorkspaceView.tsxapps/web/src/components/chat/ChangedFilesTree.tsxapps/web/src/components/chat/ChangedFilesTree.uiState.test.tsapps/web/src/components/chat/ChangedFilesTree.uiState.tsapps/web/src/components/chat/ProposedPlanActions.tsxapps/web/src/components/chat/useComposerVoiceController.tsapps/web/src/components/terminal/TerminalChrome.tsxapps/web/src/components/terminal/TerminalViewportPane.tsxapps/web/src/connection/remoteAuthApi.tsapps/web/src/hooks/usePlanReview.tsapps/web/src/hooks/useTheme.tsapps/web/src/lib/desktopProjectRecovery.test.tsapps/web/src/lib/desktopProjectRecovery.tsapps/web/src/markdown-links.test.tsapps/web/src/markdown-links.tsapps/web/src/planReview.test.tsapps/web/src/planReview.tsapps/web/src/routes/-_chat.settings.install.test.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.settings.tsxapps/web/src/routes/pair.tsxapps/web/src/terminalStateStore.test.tsapps/web/src/terminalStateStore.tsapps/web/src/theme/theme.logic.test.tsapps/web/src/theme/theme.logic.tsapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tspackages/contracts/src/auth.test.tspackages/contracts/src/auth.tspackages/contracts/src/orchestration.test.tspackages/contracts/src/orchestration.tspackages/contracts/src/rpc.test.tspackages/contracts/src/rpc.tspackages/contracts/src/server.tspackages/contracts/src/settings.tspackages/shared/src/terminalThreads.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/terminal/TerminalChrome.tsx (1)
162-171: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding
autoFocusto InlineRenameField for consistent rename UX.
TerminalViewportPanepassesautoFocus={true}toInlineRenameFieldwhen renaming terminal tabs (line 99 in TerminalViewportPane.tsx), but this component doesn't passautoFocuswhen renaming group tabs. SinceInlineRenameFieldalways selects the text on mount (viauseEffect), omittingautoFocusmeans the text is selected but the input isn't focused—the user must click before typing. AddingautoFocuswould let users immediately type after double-clicking, matching the terminal tab rename experience.Suggested enhancement
<InlineRenameField initialValue={displayTitle} onCommit={(value) => { props.onRenameGroup?.(terminalGroup.id, value); setEditingGroupId(null); }} onCancel={() => setEditingGroupId(null)} className="min-w-0 flex-1" + autoFocus />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/terminal/TerminalChrome.tsx` around lines 162 - 171, When entering group-rename mode in TerminalChrome, InlineRenameField is rendered without passing autoFocus so the input text is selected but not focused; update the JSX that renders InlineRenameField (inside the isEditing && props.onRenameGroup branch) to forward autoFocus={true} along with the existing props (initialValue, onCommit calling props.onRenameGroup(terminalGroup.id, value) and setEditingGroupId(null), onCancel calling setEditingGroupId(null), className) so the field receives focus immediately and matches the rename behavior from TerminalViewportPane.apps/server/src/codexAppServerManager.ts (1)
2377-2381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete discovery sessions by their composite cache key.
Lines 2377-2381 still remove discovery sessions with
context.session.cwd, butgetOrCreateDiscoverySession()now stores them underbuildCodexDiscoverySessionKey(...). After an unexpected Codex exit, the dead discovery context stays cached and can be reused for later discovery calls with the same provider options until the idle timer fires.Suggested fix
this.emitLifecycleEvent(context, "session/exited", message); if (context.discovery) { - const discoveryKey = context.session.cwd ?? ""; - if (discoveryKey) { - this.discoverySessions.delete(discoveryKey); + for (const [discoveryKey, session] of this.discoverySessions.entries()) { + if (session === context) { + this.discoverySessions.delete(discoveryKey); + break; + } } } else { this.sessions.delete(context.session.threadId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/codexAppServerManager.ts` around lines 2377 - 2381, The code deletes discovery sessions using context.session.cwd but getOrCreateDiscoverySession() caches sessions under a composite key from buildCodexDiscoverySessionKey(...), so change the deletion to compute that same key and remove by it; specifically, when context.discovery is truthy, call buildCodexDiscoverySessionKey(context) (or the same key-construction used in getOrCreateDiscoverySession) and pass that to this.discoverySessions.delete(...) instead of using context.session.cwd so dead/disconnected Codex contexts aren't left cached.
♻️ Duplicate comments (2)
apps/web/src/lib/providerDiscoveryReactQuery.ts (1)
48-61: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate helper – see comment in
gitReactQuery.ts.This function is identical to
codexProviderOptionsKeyinapps/web/src/lib/gitReactQuery.ts. Refactor both to use a shared utility as noted in that file's review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/providerDiscoveryReactQuery.ts` around lines 48 - 61, The two identical serializers codexDiscoveryProviderOptionsKey and codexProviderOptionsKey should be consolidated into a single shared utility (e.g., serializeCodexProviderOptions or codexProviderOptionsKey in a new/shared util module) and both places should import and call that utility; ensure the shared function preserves current behavior (accepts ProviderStartOptions | null | undefined, returns string | null, reads codex, and JSON.stringify({ binaryPath: codex.binaryPath ?? null, homePath: codex.homePath ?? null, launchArgs: codex.launchArgs ?? null })). Replace the local implementations in the modules that define codexDiscoveryProviderOptionsKey and codexProviderOptionsKey with imports to this shared function.apps/web/src/components/ChatView.tsx (1)
1922-1930:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso reset the review panel open state on plan/thread changes.
This clears annotations, but
planReviewOpenstill survives thread switches. If the user changes threads while the review panel is open and the next thread also has a plan,PlanReviewPanelstays open against the new plan unexpectedly.♻️ Proposed fix
- const planReviewPlanIdRef = useRef<string | null>(null); + const planReviewPlanKeyRef = useRef<string | null>(null); useEffect(() => { - const planId = sidebarProposedPlan?.id ?? null; - if (planReviewPlanIdRef.current === planId) { + const planKey = + activeThread?.id && sidebarProposedPlan?.id + ? `${activeThread.id}:${sidebarProposedPlan.id}` + : null; + if (planReviewPlanKeyRef.current === planKey) { return; } - planReviewPlanIdRef.current = planId; + planReviewPlanKeyRef.current = planKey; + setPlanReviewOpen(false); planReview.resetAnnotations(); - }, [planReview.resetAnnotations, sidebarProposedPlan?.id]); + }, [activeThread?.id, planReview.resetAnnotations, sidebarProposedPlan?.id]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/ChatView.tsx` around lines 1922 - 1930, When the tracked plan id changes you currently reset annotations but not the panel open state, so keep planReviewPlanIdRef logic but also reset the panel visibility: detect plan id change (same place using planReviewPlanIdRef and sidebarProposedPlan?.id) and call the routine that closes the review panel (e.g. setPlanReviewOpen(false) or the PlanReviewPanel close method) immediately after resetting annotations so PlanReviewPanel does not remain open for the new thread/plan.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/lib/gitReactQuery.ts`:
- Around line 17-30: Extract the duplicated JSON-serialization logic into a
single shared helper (e.g., buildCodexProviderOptionsKey) and replace both
codexProviderOptionsKey and codexDiscoveryProviderOptionsKey to call that
helper; specifically, move the logic that reads providerOptions?.codex and
returns JSON.stringify({ binaryPath: codexOptions.binaryPath ?? null, homePath:
codexOptions.homePath ?? null, launchArgs: codexOptions.launchArgs ?? null })
into the new buildCodexProviderOptionsKey(providerOptions) function, export it
from a common module, then import and use it from the modules that currently
define codexProviderOptionsKey and codexDiscoveryProviderOptionsKey so the
duplicate implementations are removed.
---
Outside diff comments:
In `@apps/server/src/codexAppServerManager.ts`:
- Around line 2377-2381: The code deletes discovery sessions using
context.session.cwd but getOrCreateDiscoverySession() caches sessions under a
composite key from buildCodexDiscoverySessionKey(...), so change the deletion to
compute that same key and remove by it; specifically, when context.discovery is
truthy, call buildCodexDiscoverySessionKey(context) (or the same
key-construction used in getOrCreateDiscoverySession) and pass that to
this.discoverySessions.delete(...) instead of using context.session.cwd so
dead/disconnected Codex contexts aren't left cached.
In `@apps/web/src/components/terminal/TerminalChrome.tsx`:
- Around line 162-171: When entering group-rename mode in TerminalChrome,
InlineRenameField is rendered without passing autoFocus so the input text is
selected but not focused; update the JSX that renders InlineRenameField (inside
the isEditing && props.onRenameGroup branch) to forward autoFocus={true} along
with the existing props (initialValue, onCommit calling
props.onRenameGroup(terminalGroup.id, value) and setEditingGroupId(null),
onCancel calling setEditingGroupId(null), className) so the field receives focus
immediately and matches the rename behavior from TerminalViewportPane.
---
Duplicate comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1922-1930: When the tracked plan id changes you currently reset
annotations but not the panel open state, so keep planReviewPlanIdRef logic but
also reset the panel visibility: detect plan id change (same place using
planReviewPlanIdRef and sidebarProposedPlan?.id) and call the routine that
closes the review panel (e.g. setPlanReviewOpen(false) or the PlanReviewPanel
close method) immediately after resetting annotations so PlanReviewPanel does
not remain open for the new thread/plan.
In `@apps/web/src/lib/providerDiscoveryReactQuery.ts`:
- Around line 48-61: The two identical serializers
codexDiscoveryProviderOptionsKey and codexProviderOptionsKey should be
consolidated into a single shared utility (e.g., serializeCodexProviderOptions
or codexProviderOptionsKey in a new/shared util module) and both places should
import and call that utility; ensure the shared function preserves current
behavior (accepts ProviderStartOptions | null | undefined, returns string |
null, reads codex, and JSON.stringify({ binaryPath: codex.binaryPath ?? null,
homePath: codex.homePath ?? null, launchArgs: codex.launchArgs ?? null })).
Replace the local implementations in the modules that define
codexDiscoveryProviderOptionsKey and codexProviderOptionsKey with imports to
this shared function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9e2f4b7-b58d-44e9-84dd-01c502d66256
📒 Files selected for processing (30)
apps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.tsxapps/web/src/components/PlanSidebar.tsxapps/web/src/components/PluginLibrary.tsxapps/web/src/components/SkillLibrarySettingsPanel.tsxapps/web/src/components/WorkspaceView.tsxapps/web/src/components/chat/ChangedFilesTree.browser.tsxapps/web/src/components/chat/ChangedFilesTree.tsxapps/web/src/components/chat/ProposedPlanActions.tsxapps/web/src/components/terminal/InlineRenameField.browser.tsxapps/web/src/components/terminal/InlineRenameField.tsxapps/web/src/components/terminal/TerminalChrome.tsxapps/web/src/components/terminal/TerminalViewportPane.tsxapps/web/src/hooks/usePlanReview.tsapps/web/src/lib/gitReactQuery.test.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/providerDiscoveryReactQuery.test.tsapps/web/src/lib/providerDiscoveryReactQuery.tsapps/web/src/planReview.test.tsapps/web/src/planReview.tsapps/web/src/terminalStateStore.test.tspackages/contracts/src/auth.test.tspackages/contracts/src/git.tspackages/contracts/src/orchestration.tspackages/contracts/src/provider.tspackages/contracts/src/providerDiscovery.ts
Comment on lines
+17
to
+30
| function codexProviderOptionsKey( | ||
| providerOptions: ProviderStartOptions | null | undefined, | ||
| ): string | null { | ||
| const codexOptions = providerOptions?.codex; | ||
| if (!codexOptions) { | ||
| return null; | ||
| } | ||
|
|
||
| return JSON.stringify({ | ||
| binaryPath: codexOptions.binaryPath ?? null, | ||
| homePath: codexOptions.homePath ?? null, | ||
| launchArgs: codexOptions.launchArgs ?? null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider extracting the duplicated helper to a shared module.
This helper function is identical to codexDiscoveryProviderOptionsKey in providerDiscoveryReactQuery.ts. Both extract and serialize the same three codex fields. Extracting to a shared utility (e.g., apps/web/src/lib/providerOptions.ts) would eliminate duplication and provide a single source of truth for codex options keying.
♻️ Suggested refactor
Create a shared helper module:
// apps/web/src/lib/providerOptions.ts
import type { ProviderStartOptions } from "`@jcode/contracts`";
export function buildCodexProviderOptionsKey(
providerOptions: ProviderStartOptions | null | undefined,
): string | null {
const codexOptions = providerOptions?.codex;
if (!codexOptions) {
return null;
}
return JSON.stringify({
binaryPath: codexOptions.binaryPath ?? null,
homePath: codexOptions.homePath ?? null,
launchArgs: codexOptions.launchArgs ?? null,
});
}Then import and use in both files:
+import { buildCodexProviderOptionsKey } from "./providerOptions";
+
-function codexProviderOptionsKey(
- providerOptions: ProviderStartOptions | null | undefined,
-): string | null {
- const codexOptions = providerOptions?.codex;
- if (!codexOptions) {
- return null;
- }
-
- return JSON.stringify({
- binaryPath: codexOptions.binaryPath ?? null,
- homePath: codexOptions.homePath ?? null,
- launchArgs: codexOptions.launchArgs ?? null,
- });
-}
export function gitSummarizeDiffQueryOptions(input: {
// ...
}) {
const normalizedPatch = input.patch?.trim() ?? null;
const patchKey = /*...*/;
- const providerOptionsKey = codexProviderOptionsKey(input.providerOptions);
+ const providerOptionsKey = buildCodexProviderOptionsKey(input.providerOptions);
return queryOptions({
// ...
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/lib/gitReactQuery.ts` around lines 17 - 30, Extract the
duplicated JSON-serialization logic into a single shared helper (e.g.,
buildCodexProviderOptionsKey) and replace both codexProviderOptionsKey and
codexDiscoveryProviderOptionsKey to call that helper; specifically, move the
logic that reads providerOptions?.codex and returns JSON.stringify({ binaryPath:
codexOptions.binaryPath ?? null, homePath: codexOptions.homePath ?? null,
launchArgs: codexOptions.launchArgs ?? null }) into the new
buildCodexProviderOptionsKey(providerOptions) function, export it from a common
module, then import and use it from the modules that currently define
codexProviderOptionsKey and codexDiscoveryProviderOptionsKey so the duplicate
implementations are removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Refactor