feat(agents): pane-level Chat/History/Settings tabs (+ history-deleted-send fix) - #2299
Conversation
…nt config across panes Two changes, on the new pu/agents-pane-tabs branch (created off master after PR #2296 merged), continuing that PR's unaddressed review feedback plus new work toward per-pane Chat/History/Settings tabs: 1. Reject sends to history-deleted conversations (both page-agent and global send paths). When History-delete deactivates a conversation's canonical row (not just its messages — a change from #2296's own review cycle) while it's still open in another pane or browser tab, a send to that conversation previously succeeded silently: the new message persisted, but the row stayed excluded from every open/closed session listing, leaving the transcript unreachable once the stale pane refreshed (review finding — chatgpt-codex-connector on PR #2296). - Page-agent path (apps/web/src/app/api/ai/chat/route.ts): the existing conversation lookup now checks `isActive === false` and 404s before the ownership/page checks. - Global-assistant path: a DIFFERENT mechanism produced the same bug — resolveOrCreateConversation's initial SELECT filters isActive: true, so a history-deleted row reads as "no row" and falls into the insert-then-conflict path; the INSERT collides on the row's own id and is swallowed by onConflictDoNothing, and the fallback "who won the race" SELECT had no isActive filter at all — silently handing back the STALE INACTIVE row mislabeled isNew: true. Added a new ConversationHistoryDeletedError, thrown from that fallback when the winner is inactive, caught by the route the same way ConversationOwnershipError already is (404). 2. useAgentConfig — a new SWR-backed hook replacing AgentPageView's bespoke useEffect+useState fetch for the Settings tab's agent config. SWR's cache is keyed by URL, so this is a genuine step toward one shared source of truth: every consumer of the same pageId (the page's own Settings tab, and — once the pane-tabs work lands — every pane showing that agent's Settings) reads the identical cache entry, and a save from any one of them (PageAgentSettingsTab already PATCHes itself and hands back the fresh value) writes through that same cache with no revalidate, instantly visible everywhere else. Two independent per-instance copies that could silently drift is exactly the gap this closes. useAgentConfig's own fetch/cache-sharing behavior gets a dedicated hook-level test (useAgentConfig.test.ts, real SWR via a scoped SWRConfig provider); AgentPageView.test.tsx now mocks the hook directly (matching this file's existing per-hook mocking pattern) rather than asserting on the raw fetch call, since that's no longer this component's own responsibility. 386 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds transactional conversation activity checks, deleted-history 404 handling, shared agent configuration, session-aware conversation summaries, and tabbed agent panes with history and settings workflows. ChangesConversation activity validation
Shared agent configuration
Tabbed agent pane workflow
Reopen response state
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/app/api/ai/global/`[id]/messages/resolve-or-create-conversation.ts:
- Line 131: Make the active-conversation validation atomic with the first
message write and History deletion: in
apps/web/src/app/api/ai/global/[id]/messages/resolve-or-create-conversation.ts:131
serialize the resolve/create flow with persistence, and in
apps/web/src/app/api/ai/chat/route.ts:550-564 use a row lock, transaction, or
conditional write immediately before saveMessageToDatabase that returns 404 when
the conversation is inactive. Add the race test in
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts:542-561,
covering History deletion after validation but before message persistence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7704a17d-d344-4b43-a0cd-567b1a0808f2
📒 Files selected for processing (12)
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/conversation-id-resolution.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/resolve-or-create-conversation.test.tsapps/web/src/app/api/ai/global/[id]/messages/resolve-or-create-conversation.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/components/agents/AgentPageView.tsxapps/web/src/components/agents/__tests__/AgentPageView.test.tsxapps/web/src/lib/ai/shared/hooks/__tests__/useAgentConfig.test.tsapps/web/src/lib/ai/shared/hooks/useAgentConfig.tsapps/web/src/lib/repositories/global-conversation-repository.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b3852ffaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // deactivates the CANONICAL row (not just its messages) while a | ||
| // conversation can still be open in another pane or browser tab | ||
| // (review finding — chatgpt-codex-connector on PR #2296). | ||
| if (existingConversation.isActive === false) { |
There was a problem hiding this comment.
When History deletion commits after getConversation() but before saveMessageToDatabase() later in this route, this snapshot still reports active and the request persists the user message and starts generation beneath the now-inactive conversation. The transcript is then excluded from session listings—the exact data-loss scenario this guard aims to prevent—so the active check and message persistence need to serialize with deletion, such as through a row lock/transaction or an equivalent conditional write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 10d80c7. The first (user) message's persist is now wrapped in db.transaction(async (tx) => { SELECT ... FOR UPDATE; if (!isActive) throw ConversationHistoryDeletedError; saveMessageToDatabase({..., dbClient: tx}); }) — saveMessageToDatabase gained an optional dbClient override so the insert runs on the SAME locked connection as the check, not a second unlocked one. Postgres's own row locking makes this correct without touching the DELETE side: FOR UPDATE blocks a concurrent UPDATE ... SET isActive = false on the same row (and vice versa), so whichever transaction reaches the row first wins and the other correctly observes the outcome.
Gated on existingConversation (not unconditional): a brand-new conversation's eager conversationRepository.createConversation call above is explicitly best-effort/non-fatal elsewhere in this same route (errors swallowed, "eventually consistent" by design) — an unconditional lock-check here would turn that tolerated absence into a hard rejection.
New regression test: conversation looks active at the early ownership check, mockConversationActiveAtLockTime.current = false simulates a delete committing in the gap before the write — asserts 404 and the lifecycle never starts.
| .limit(1); | ||
|
|
||
| if (!winner) throw new Error(`Failed to resolve conversation ${conversationId}`); | ||
| if (!winner.isActive) throw new ConversationHistoryDeletedError(); |
There was a problem hiding this comment.
When a global History deletion commits after the active-row SELECT in resolveOrCreateConversation() but before saveGlobalAssistantMessageToDatabase() in the route, execution never reaches this inactive-winner check because the resolver already returned the formerly active row. The send consequently appends an unreachable message and may start a billed generation, so resolution must remain atomic with persistence or the write must conditionally require the conversation to still be active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 10d80c7, matching your diagnosis exactly — the initial isActive-filtered SELECT and the concurrent-winner fallback SELECT both now use .for('update'). That alone protects resolveOrCreateConversation's own internal race (a concurrent insert vs. a concurrent delete), but per your point 3 it does NOT cover the gap between resolution returning and the message write further down the route (credit-gate/mention/command work in between). Added a SEPARATE, second atomic re-check immediately adjacent to saveGlobalAssistantMessageToDatabase's call site — a short db.transaction doing SELECT ... FOR UPDATE + the insert, catching exactly the scenario you described (resolve returns active, delete commits before the write). New regression test in this route's stream-socket-events.test.ts reproduces it directly.
… pane Requested during review on PR #2296/#2299: a console pane should be able to reach the same history and settings AgentPageView's own Chat/History/Settings tabs already give a plain page — not a narrower, session-scoped slice of it. Checked what that actually requires (the client-side pane-assignment path, `assignPane`, has never enforced session membership — only the create/close/ reopen cap-accounting paths care about `sessionId` — so a pane CAN already render any conversation the caller has access to; the constraint I'd assumed in an earlier round doesn't exist). - `ChatPane` (apps/web/src/components/agents/panes/AgentPanes.tsx) replaces `ChatPaneIdentity`: still the AISelector, now followed by a compact Chat/History/Settings tab strip (icon-only — a pane bar is 30px tall, no room for AgentPageView's text pills). Owns which tab is active and renders all three bodies, since that state has to reach both the bar (the tab strip) and the body below it, and `renderPane` itself can't hold hooks (a plain function invoked per pane, not a component). - History and Settings are NOT reimplemented per-pane — they reuse `useConversations` and the new `useAgentConfig` (both SWR-keyed, already shared with AgentPageView's own tabs) and the SAME `PageAgentHistoryTab`/ `PageAgentSettingsTab` leaf components AgentPageView renders. A pane and the page view showing the same agent are now two views onto one cache entry, not two parallel implementations that could drift. - Picking a History entry: `handlePickHistoryConversation` (new, in `AgentPanes`, alongside its sibling `handlePickAgent`/`handleSwitchAgent` — the container owns all pane IO, per this file's own doc comment) reopens first ONLY when the conversation is bound to THIS session (the reopen POST is idempotent either way, but unbound/foreign-session conversations were never subject to this session's cap or listing, so no server call is needed at all — straight to `assignPane`, the same client-only mechanism `openConversation`'s focus branch already uses). - "New Conversation" from a pane's History tab reuses `handlePickAgent` directly — it's exactly the existing pane-picker mint flow, just triggered from a different affordance. - `useAgentConfig` widened to accept `pageId: string | null` (the Assistant pane has no page) — disables the SWR key rather than fetching a bogus URL, so a pane can call the hook unconditionally across an agent switch without violating hook-order rules. `AgentPanes.test.tsx` mocks `PageAgentHistoryTab`/`PageAgentSettingsTab` as leaf renderers (matching this file's existing PaneChat/Shell mocking philosophy — their own internals are that component's test file's job, not this container's). 392 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
|
@codex review Pushed 7cafcab: Chat | History | Settings as real tabs inside every console pane ( 392 tests passing, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cafcabc03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| const handlePickHistoryConversation = useCallback( | ||
| async (paneId: string, agentPageId: string | null, conversation: { id: string; title: string | null; sessionId: string | null }) => { | ||
| if (conversation.sessionId === sessionId) { |
There was a problem hiding this comment.
When a pane in session A selects a history entry whose sessionId is null or belongs to session B, this condition skips reopening and the unconditional assignPane below installs that conversation into session A's grid. Chat execution intentionally resolves the sandbox from the conversation row's persisted binding, so subsequent tool calls operate in session B's sandbox—or without session capabilities—while the UI presents the conversation beside session A's panes and shells. Filter these entries, navigate to their owning session, or otherwise avoid assigning them to the current session's workspace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated this in depth rather than applying the suggested fix, since it turned on a factual question I needed to verify: does tool-call sandbox resolution key off the pane grid's own session, or the conversation's own persisted binding?
Traced it fully: acquireSandbox (apps/web/src/lib/ai/tools/sandbox-tools-runtime.ts) resolves via findSessionForConversation(conversationId) — a fresh DB read (packages/lib/src/services/agent-sessions/agent-sessions-store.ts's findByConversation) joined purely on conversations.id = conversationId → conversations.sessionId → agentSessions.id. It takes no HTTP-request context, no route params, and no pane/session-grid signal whatsoever — the conversationId in the send request (which becomes the pane's own targetId via assignPane) is the ONLY input. The module's own doc comment states this as a load-bearing invariant: conversation→session resolution happens through conversations.sessionId, explicitly rejecting per-conversation/lazy sandbox minting as "exactly the conflation the session model removed."
Concretely: a pane in session A's grid displaying conversation X (bound to session B) sends conversationId: X on every message; tool calls for that message ALWAYS resolve session B's sandbox, never session A's — this is provably correct, not a leak, regardless of which grid displays it. A sessionId: null conversation gets a hard, explicit no_session denial (never a silent fallback to whichever session's grid it's opened in).
What IS real, and worth flagging separately as a UX-clarity item rather than a P1: a pane's tool activity streams into "every shell in THIS conversation's own session" (session B's shells) — a user watching session A's terminal tab, where the pane visually sits, would not see it. That's a discoverability surprise, not a correctness or security issue, since the routing itself never crosses session boundaries. Happy to open a follow-up for a visual indicator (e.g. a badge on a pane whose conversation belongs to a different/no session) if that's valuable, but did not treat it as blocking here given the underlying mechanism is sound.
| e.stopPropagation(); | ||
| settingsRef.current?.submitForm(); | ||
| }} | ||
| disabled={isSettingsSaving} |
There was a problem hiding this comment.
On a slow or failed config request, the Settings body shows its loading state but this new pane-level Save button remains enabled. PageAgentSettingsTab registers submitForm before returning that loader, and its form defaults contain an empty prompt/tool list, so clicking Save during this state PATCHes those defaults and can overwrite the agent's existing configuration. Include agentConfig === null in the disabled/loading condition and avoid exposing submission until the initial config exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 659581a — the Save button's disabled condition now includes agentConfig === null, exactly as you described (PageAgentSettingsTab registers submitForm before its own loaded-check returns, and its form defaults would PATCH an empty prompt/tool list over the real config). New regression test drives a pending /api/pages/agent-1/agent-config fetch, asserts the button is disabled while it's in flight, then resolves it and asserts the button becomes enabled.
| currentConversationId={conversationId} | ||
| onSelectConversation={handleSelectHistoryConversation} | ||
| onCreateNew={handleCreateNewFromHistory} | ||
| onDeleteConversation={(id) => void deleteConversation(id)} |
There was a problem hiding this comment.
When History deletes the conversation displayed by this pane—or one displayed by another pane—this callback only invokes deleteConversation; no onConversationDelete callback or workspace update removes the corresponding pane binding. After the DELETE deactivates the canonical row, switching that pane back to Chat leaves it pointing at the deleted transcript, and sends now return the new 404 indefinitely until the user manually selects or creates another conversation. Successful deletion should replace/reset every pane showing that ID and refresh the session-list cache.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 659581a — added a new handleHistoryDeleteConversation in AgentPanes (alongside its sibling handlePickAgent/handlePickHistoryConversation, per this file's own "the container owns ALL the IO" doc comment), wired to every pane's onDeleteConversation regardless of which pane's History tab the delete came from. It resets every pane in the grid still pointing at the deleted id back to the picker — the simplest safe recovery, letting the user explicitly pick what's next rather than guessing a replacement. New regression test: two panes, delete the SECOND pane's conversation from the FIRST pane's History tab, assert the second pane's scope resets to null.
| assignPane(sessionId, paneId, { | ||
| kind: 'chat', | ||
| name: conversation.title || 'Conversation', | ||
| targetId: conversation.id, | ||
| agentPageId, | ||
| }); |
There was a problem hiding this comment.
If the selected History conversation is already displayed in another pane, this direct assignment bypasses the store's deduplicating openConversation path and makes the same conversation appear in two panes. That mounts two independently interactive chat surfaces for one transcript and contradicts the existing focus behavior used by agent switching; use openConversation or explicitly focus the pane that already owns the target ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 659581a — handlePickHistoryConversation now checks for an existing pane in the grid already showing the picked conversationId, and focuses it instead of assigning to the pane History was opened from. One subtlety a regression test caught that inspection alone would have missed: a plain synchronous selectPane call there was getting immediately clobbered — ChatPane's outer group/pane div has its own bubble-phase onClick={onSelectPane} (selects the pane the click originated in), which fires AFTER the inner button's click handler in the SAME synchronous dispatch, re-selecting the wrong pane a moment after my code had already selected the right one. Deferred the focus call via a macrotask (setTimeout(..., 0)) so it runs after the entire bubble phase has finished, which is what actually wins. New regression test: pane B already shows the target conversation, pick it from pane A's History — asserts pane B becomes active and pane A's own binding is untouched.
| onPickHistoryConversation({ id, title: picked?.title ?? null, sessionId: picked?.sessionId ?? null }); | ||
| setActiveTab('chat'); |
There was a problem hiding this comment.
For a closed same-session conversation, onPickHistoryConversation starts the asynchronous reopen but returns immediately, so this line switches to Chat before the POST resolves. If reopening is rejected because the session is full, access changed, or the row was deleted, the parent correctly leaves the old pane binding untouched, but the user is nevertheless moved to the old Chat view after attempting to select a different entry. Return and await the reopen result before changing tabs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 659581a — onPickHistoryConversation now resolves to whether the pick actually landed (false on a failed reopen), and the pane's own tab-switch handler only follows it to Chat when it resolves true. A failed reopen leaves the user on History with the error toast already shown, rather than being bounced to the stale Chat view as if the pick had silently succeeded. New regression test: reopen POST rejects, asserts the pane stays on the History tab (never renders pane-chat).
…eck and the message write Round-2 findings on PR #2299 (chatgpt-codex-connector P1 x2, coderabbitai major): the isActive checks added in the previous commit read the conversation's state once, early — but a concurrent History-delete could still commit in the (long, I/O-heavy) gap between that read and the actual message persist, which happens after credit-gate checks, @mention processing, and command resolution. The snapshot going stale meant the exact scenario the earlier fix was meant to prevent could still happen: a message silently persisted beneath a conversation now excluded from every session listing. Global path additionally had a SECOND, independent way to hit the same bug: `resolveOrCreateConversation`'s initial SELECT filters `isActive: true`, so a history-deleted row reads as "no existing row" and falls into the insert-then-conflict path — the INSERT collides with the inactive row's own id and is swallowed by `onConflictDoNothing`, and the fallback "who won the race" SELECT (added last commit to check `isActive`) only runs when THAT code path is reached at all. If the delete instead committed AFTER `resolveOrCreateConversation` returned but BEFORE the message write, neither existing check ever ran again. Fix: a `SELECT ... FOR UPDATE` immediately adjacent to the write, inside a short transaction that wraps ONLY the check + the insert — never the credit-gate/mention/command work around it, which must not run inside an open transaction (holding a row lock across that much unrelated I/O, including its own DB round-trips, would trade a rare race for a routine contention/pool-starvation risk). Postgres's own row locking makes this correct without touching the DELETE side at all: `FOR UPDATE` blocks a concurrent `UPDATE ... SET isActive = false` on the same row (and vice versa), so whichever transaction reaches the row first wins and the other correctly observes its outcome. - `message-utils.ts`: `saveMessageToDatabase`/`saveGlobalAssistantMessageToDatabase` gain an optional `dbClient` override (defaults to the module `db`) so a caller already holding a transaction can run the insert on that SAME connection rather than a second, unlocked one. - `resolve-or-create-conversation.ts`: both SELECTs (the initial isActive lookup and the concurrent-winner fallback) now lock via `.for('update')` — a no-op standalone, real protection when the caller wraps the call in an explicit transaction. - Both chat routes: the first (user) message's persist is now wrapped in `db.transaction(async (tx) => { SELECT...FOR UPDATE; if (!isActive) throw; save({..., dbClient: tx}); })`. Page-agent route gates this on `existingConversation` — a brand-new conversation's eager-create is explicitly best-effort/non-fatal elsewhere in this same route, and this check must not turn that tolerated absence into a hard rejection. New regression tests in both routes' test suites reproduce the exact race (conversation looks active at the early check, a concurrent delete commits before the write) and assert 404 + the lifecycle never starts. 345 tests passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
…ind on delete Round-3 findings on PR #2299 (chatgpt-codex-connector: 1 more P1 + 3 P2; one additional P1 investigated and confirmed a false alarm): - Disable Save until the agent config has loaded (P1): PageAgentSettingsTab registers submitForm before its own config-loaded check returns, and its form defaults contain an empty prompt/tool list — clicking Save in that window would PATCH those defaults over the agent's real config. The button's disabled condition now includes `agentConfig === null`. - Focus an existing pane instead of duplicating it (P2): picking a History entry already open in another pane bypassed the store's deduplicating `openConversation` path, mounting a second independently interactive surface for the same transcript. `handlePickHistoryConversation` now checks for an existing pane showing that conversationId first and focuses it — deferred via a macrotask, since a synchronous `selectPane` here was getting immediately clobbered by `ChatPane`'s own outer `onClick={onSelectPane}` firing during the SAME click's bubble phase (caught via a failing regression test, not by inspection — the fix wouldn't have been obvious from reading the code alone). - Stay in History until reopening succeeds (P2): `onPickHistoryConversation` now resolves to whether the pick actually landed; the pane's own tab switch only follows it to Chat on success, so a failed reopen (session full, deleted, access changed) leaves the user on History with a toast, not silently bounced to the old Chat view as if nothing happened. - Rebind panes after deleting a conversation from History (P2): a History delete deactivates the CANONICAL row, and previously only called `deleteConversation` — any pane still pointing at that id (in this pane OR another one, whichever pane's History tab the delete came from) was left showing a transcript that now 404s on send with no way out. New `handleHistoryDeleteConversation` (in `AgentPanes`, alongside its sibling `handlePickAgent`/`handlePickHistoryConversation`) resets every pane in the grid still showing the deleted id back to the picker. - Investigated and refuted the sandbox-routing P1 (chatgpt-codex-connector): claimed a pane could execute tool calls against the WRONG session's sandbox when displaying a foreign-session conversation. Traced the actual resolution path (`acquireSandbox` → `findSessionForConversation`, a fresh DB read keyed only on `conversationId`, documented as intentionally ignoring any session-grid/pane context) and confirmed tool calls always resolve to the CONVERSATION's own persisted session, never the pane grid's — the concern is UX clarity (activity streams to a different session's shells than the one visually surrounding the pane) at most, not a functional or security bug. Replied with the full trace, not a code change. 4 new regression tests. 226 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
|
@codex review Two more rounds pushed: 10d80c7 — closed the TOCTOU race in the isActive checks from the previous round: a 659581a — pane-tabs round-3 findings: Save button now disabled until agent config loads (P1), History picks focus an existing pane instead of duplicating it (P2, with a real bug caught via a failing regression test — event-bubbling clobbering a synchronous 571 tests passing across the affected suites ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659581a4dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onDeleteConversation={(id) => { | ||
| void deleteConversation(id); | ||
| onHistoryDeleteConversation(id); |
There was a problem hiding this comment.
When the DELETE fails—most notably with the intentional 409 returned for the last open conversation in a session—deleteConversation catches the failure and resolves without indicating success, but this callback immediately resets every pane showing the conversation anyway. The server therefore retains the live conversation while the client discards its pane bindings and shows pickers; only reset the panes after a confirmed successful deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0df7444 — deleteConversation (apps/web/src/lib/ai/shared/hooks/useConversations.ts, shared with AgentPageView's own History tab) now resolves boolean instead of resolving regardless of outcome, and surfaces the server's own error message on a refusal (previously silent for a non-ok response). AgentPanes only calls onHistoryDeleteConversation when it resolves true. New regression test: a 409 refusal leaves the pane's binding untouched.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/web/src/components/agents/panes/AgentPanes.tsx (3)
1027-1045: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport a failed share toggle to the user.
toggleConversationSharelogs failures to the console only. Every other failing action in this file raises atoast.error. A silent failure leaves the History row showing a share state the server never accepted. Add a toast, and use the sameApiRequestError-aware message shape the other handlers use.♻️ Proposed change to surface the failure
- if (!response.ok) console.error('Failed to update conversation sharing'); + if (!response.ok) { + console.error('Failed to update conversation sharing'); + toast.error('Could not update sharing for this conversation'); + } } catch (error) { console.error('Failed to update conversation sharing:', error); + toast.error('Could not update sharing for this conversation', { + description: error instanceof Error ? error.message : 'Please try again.', + }); }🤖 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/agents/panes/AgentPanes.tsx` around lines 1027 - 1045, Update toggleConversationShare to surface both non-OK responses and caught errors with toast.error instead of console-only logging. Match the ApiRequestError-aware message formatting already used by other handlers in AgentPanes, while preserving the existing request and early-return behavior.
673-699: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRead the workspace from the live store after the awaited reopen.
workspaceis the render-time snapshot captured when this callback was created. Theawait post(...)above can resolve much later. Two effects follow. The dedup lookup can miss a pane that another action bound toconversation.idduring the request. TheassignPanecall can also overwrite a pane that the user repurposed during the request, which the file guards elsewhere withpaneStillShows/paneStillLoading. ReadinguseAgentWorkspaceStore.getState()here matches that existing idiom and removesworkspacefrom the dependency list.♻️ Proposed change to read live pane state
- const existingPane = workspace - ? panesOf(workspace).find( - (p) => p.id !== paneId && p.scope?.kind === 'chat' && p.scope.targetId === conversation.id, - ) - : undefined; + const current = useAgentWorkspaceStore.getState().workspaces[sessionId]; + const existingPane = current + ? panesOf(current).find( + (p) => p.id !== paneId && p.scope?.kind === 'chat' && p.scope.targetId === conversation.id, + ) + : undefined; if (existingPane) { @@ return true; }, - [sessionId, assignPane, workspace, selectPane], + [sessionId, assignPane, selectPane], );🤖 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/agents/panes/AgentPanes.tsx` around lines 673 - 699, After the awaited reopen completes, read the current workspace from useAgentWorkspaceStore.getState() before deduplication or assignment instead of using the render-time workspace snapshot. Use this live state for panesOf lookup and guard assignment against changes made while awaiting, following the existing paneStillShows/paneStillLoading protections, and remove workspace from the callback dependency list.
1192-1228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate each tab with its panel.
The buttons carry
role="tab"inside arole="tablist", but no element carriesrole="tabpanel", and the tabs have noaria-controls. Assistive technology therefore cannot move from a tab to the content it controls. Add an id to the pane body container at Line 1133, give itrole="tabpanel", and point each tab at it witharia-controls. Arrow-key movement between tabs is the other half of the ARIA tabs pattern, and it is worth adding here as well.🤖 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/agents/panes/AgentPanes.tsx` around lines 1192 - 1228, Update PaneChatTabStrip and the pane body container to complete the ARIA tabs relationship: assign the body a stable id and role="tabpanel", add matching aria-controls to each tab button, and implement arrow-key navigation between tabs while preserving the existing selection behavior.apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx (2)
1027-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the pane count to pin the no-duplicate claim.
The test verifies the focus move and the first pane's binding. It does not verify that the grid still holds exactly two panes. A regression that both focuses the existing pane and assigns a duplicate elsewhere would still pass. Add a count assertion.
♻️ Proposed additional assertion
expect(firstPaneAfter?.scope?.targetId).toBe('conv-1'); + const panesAfter = useAgentWorkspaceStore + .getState() + .workspaces['ses-1'].columns.flatMap((c) => c.panes); + expect(panesAfter).toHaveLength(2); + expect(panesAfter.filter((p) => p.scope?.targetId === 'conv-other')).toHaveLength(1); });🤖 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/agents/panes/__tests__/AgentPanes.test.tsx` around lines 1027 - 1036, Update the test around the existing firstPaneAfter assertion to count panes across all columns in workspace ses-1 and assert that exactly two panes remain, preserving the existing focus and binding assertions.
1092-1104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the deleting pane deterministically, and assert both panes reset.
The test title states that the delete comes from another pane's History tab.
historyTabs[0]relies on DOM order, which the preceding test explicitly avoids. If the first match belongs to the pane bound toconv-doomed, the test then verifies that the deleting pane reset itself, which is a weaker case than the title states. Locate the pane bar whose body rendersconv-1, the same way the dedup test does. Also assert that the pane bound toconv-1keeps its binding, so the reset stays scoped to the deleted 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/agents/panes/__tests__/AgentPanes.test.tsx` around lines 1092 - 1104, Update the test to select the History tab from the pane whose body is bound to conv-1, using the pane-specific lookup pattern from the deduplication test instead of historyTabs[0]. After deleting delete-conv-doomed, assert both that the targeted pane’s scope is null and that the conv-1 pane still retains its original binding.apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts (1)
97-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated transactional db mock into a shared test helper.
Three test files each hand-roll the same
makeDbLike()+transaction()mock shape (aselect().from().where().for('update').limit()chain plus a transaction callback that runs against a fresh mock instance). The root cause is the absence of a shared test utility for this pattern. A future change to the real transaction contract requires updating all three copies independently.
apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts#L97-L164: extract this version (it already has table-label detection) into a shared test helper, e.g.apps/web/src/lib/testing/create-transactional-db-mock.ts, parameterized by table-label-to-row mappings.apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts#L103-L157: replace this file'smakeDbLike/transactionwith the shared helper.apps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.ts#L97-L131: replace this file'smakeDbLike/transactionwith the shared helper.🤖 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/app/api/ai/global/`[id]/messages/__tests__/stream-socket-events.test.ts around lines 97 - 164, The transactional database mock is duplicated across three test files; extract the table-label-aware makeDbLike and fresh-client transaction behavior into a shared create-transactional-db-mock helper, parameterized by table-label-to-row mappings. Update apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts:97-164 to use the helper, and replace the corresponding makeDbLike/transaction implementations in apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts:103-157 and apps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.ts:97-131, preserving each test’s existing rows, mocks, and assertions.
🤖 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/components/agents/panes/AgentPanes.tsx`:
- Around line 1154-1157: Update the onDeleteConversation callback in AgentPanes
so onHistoryDeleteConversation runs only after deleteConversation(id) resolves
successfully. Preserve the existing deletion error handling and avoid resetting
the panes when the promise rejects.
- Around line 1161-1181: Reset activeTab to 'chat' whenever the pane’s current
agent changes, including transitions where scope.agentPageId becomes null and
switches to a different agent. Update the effect or state logic controlling
activeTab in AgentPanes, using the agent identity/page identifier as the
dependency, so the Settings tab cannot remain selected across agent changes.
---
Nitpick comments:
In
`@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/stream-socket-events.test.ts:
- Around line 97-164: The transactional database mock is duplicated across three
test files; extract the table-label-aware makeDbLike and fresh-client
transaction behavior into a shared create-transactional-db-mock helper,
parameterized by table-label-to-row mappings. Update
apps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.ts:97-164
to use the helper, and replace the corresponding makeDbLike/transaction
implementations in
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts:103-157 and
apps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.ts:97-131,
preserving each test’s existing rows, mocks, and assertions.
In `@apps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsx`:
- Around line 1027-1036: Update the test around the existing firstPaneAfter
assertion to count panes across all columns in workspace ses-1 and assert that
exactly two panes remain, preserving the existing focus and binding assertions.
- Around line 1092-1104: Update the test to select the History tab from the pane
whose body is bound to conv-1, using the pane-specific lookup pattern from the
deduplication test instead of historyTabs[0]. After deleting delete-conv-doomed,
assert both that the targeted pane’s scope is null and that the conv-1 pane
still retains its original binding.
In `@apps/web/src/components/agents/panes/AgentPanes.tsx`:
- Around line 1027-1045: Update toggleConversationShare to surface both non-OK
responses and caught errors with toast.error instead of console-only logging.
Match the ApiRequestError-aware message formatting already used by other
handlers in AgentPanes, while preserving the existing request and early-return
behavior.
- Around line 673-699: After the awaited reopen completes, read the current
workspace from useAgentWorkspaceStore.getState() before deduplication or
assignment instead of using the render-time workspace snapshot. Use this live
state for panesOf lookup and guard assignment against changes made while
awaiting, following the existing paneStillShows/paneStillLoading protections,
and remove workspace from the callback dependency list.
- Around line 1192-1228: Update PaneChatTabStrip and the pane body container to
complete the ARIA tabs relationship: assign the body a stable id and
role="tabpanel", add matching aria-controls to each tab button, and implement
arrow-key navigation between tabs while preserving the existing selection
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f6a3a92-c0ac-4dc6-a88c-f0121534285d
📒 Files selected for processing (12)
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/resolve-or-create-conversation.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/resolve-or-create-conversation.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsxapps/web/src/lib/ai/core/message-utils.tsapps/web/src/lib/ai/shared/hooks/__tests__/useAgentConfig.test.tsapps/web/src/lib/ai/shared/hooks/useAgentConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/ai/global/[id]/messages/tests/resolve-or-create-conversation.test.ts
… on agent switch Round-4 findings on PR #2299 (chatgpt-codex-connector P1 + coderabbitai major, same underlying bug from two angles; coderabbitai minor): - Wait for History deletion before resetting panes: `deleteConversation` swallowed both a non-ok response (e.g. the never-empty guard's 409 for the session's last open listing) and a network failure, resolving either way with no signal — so the previous round's pane-rebind fix reset every pane showing the conversation regardless of whether the server actually deleted it, discarding a live, working pane binding for a conversation that was NEVER removed. `deleteConversation` (apps/web/src/lib/ai/shared/ hooks/useConversations.ts — shared with AgentPageView's own History tab) now resolves `boolean`, surfaces the server's own error message on a refusal (previously silent), and `AgentPanes` only calls `onHistoryDeleteConversation` when it resolves `true`. - Leave the Settings tab when the pane switches to a different agent (coderabbitai): `showSettings` hid the tab once `agentPageId` became null or changed, but `activeTab` itself stayed `'settings'` — the body then fell through to a spinner branch that could never resolve (the Assistant has no config to load, ever). A pane switched to a different agent would also silently keep showing Settings, presenting a config the user never asked for. `activeTab` now resets to `'chat'` whenever `scope.agentPageId` changes. 3 new regression tests for the delete-gating (success/refused/network- failure, all in useConversations.test.ts directly) plus 2 in AgentPanes.test.tsx (a refused 409 delete leaves panes untouched; switching agents leaves the Settings tab). 234 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0df74440a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .select({ isActive: conversations.isActive }) | ||
| .from(conversations) | ||
| .where(eq(conversations.id, conversationId!)) | ||
| .for('update') |
There was a problem hiding this comment.
When a page-agent History DELETE begins first, softDeleteConversation() updates chatMessages before it updates the conversations row; a concurrent send can therefore acquire this row lock after that message sweep, observe isActive: true, insert a new active message, and commit before the DELETE finally tombstones the conversation. The deleted conversation then retains the newly inserted active message, and generation can continue against an unreachable transcript. Fresh evidence after the prior review is the delete transaction's child-before-parent lock ordering in conversation-repository.ts; make deletion acquire this same conversation lock before sweeping messages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22538c5, matching your diagnosis exactly — reordered softDeleteConversation's transaction to update conversations FIRST (acquiring the row lock immediately) and chatMessages second, so a concurrent send's own FOR UPDATE re-check contends for that lock across the WHOLE transaction, not just from the second statement onward. Existing ordering-assertion test in conversation-repository.test.ts updated to match; the round-2 atomic re-check's own race tests continue to exercise the (now-corrected) interaction.
| assignPane(sessionId, paneId, { | ||
| kind: 'chat', | ||
| name: conversation.title || 'Conversation', | ||
| targetId: conversation.id, | ||
| agentPageId, | ||
| }); |
There was a problem hiding this comment.
When a same-session reopen is slow, the user can select another History row or switch the pane's agent before this request resolves; this completion then unconditionally assigns the older conversation and overwrites the user's newer choice. Two rapid closed-conversation selections can likewise resolve out of order, making network timing determine the visible transcript. Verify that this pane still represents the initiating selection, or use a per-pane request token, before assigning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22538c5 — added a per-pane token (paneAssignTokens, bumped by both handlePickHistoryConversation and handlePickAgent), checked before applying a completion at each point a supersede could have happened. Covers both scenarios you named directly (two rapid History picks on the same pane; a pending reopen raced by a different agent's mint into that pane). One gap I want to flag rather than silently leave: handleSwitchAgent's FOCUS branch (switching to an agent that already has an OPEN conversation, vs. minting a new one) calls the store's openConversation directly, which can target a DIFFERENT pane than the one the switch originated from (it focuses whichever pane already shows that conversation, if any) — wiring that branch into the same token risked invalidating the wrong pane's token rather than the one it actually affects, so I left it out rather than guess. Happy to take a follow-up pass at that specific interaction if it's a real concern; the two explicitly-reported scenarios are fully covered by this fix. New regression test reproduces the same-pane race directly (a fast unbound pick superseding a slow same-session reopen).
| if (!workspace) return; | ||
| const affectedPanes = panesOf(workspace).filter( | ||
| (p) => p.scope?.kind === 'chat' && p.scope.targetId === deletedConversationId, |
There was a problem hiding this comment.
When deletion succeeds after the user has already selected another conversation in an affected pane, this callback still reads the workspace snapshot captured before the DELETE and resets that pane based on its old binding, discarding the newer selection. History remains interactive while the request is pending, so this race is reachable; query useAgentWorkspaceStore.getState() at completion time and reset only panes that still show the deleted ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 22538c5, exactly as suggested — handleHistoryDeleteConversation now reads useAgentWorkspaceStore.getState().workspaces[sessionId] fresh at call time instead of the workspace it closed over (stale by definition, since this always runs after the DELETE's own async round trip). New regression test: reassign the affected pane to something else while the delete is still pending, resolve the delete, assert the pane keeps the newer assignment.
…aces Round-5 findings on PR #2299 (chatgpt-codex-connector, 1 P1 + 2 P2): - Lock the conversation before sweeping page messages (P1): the previous round's atomic re-check (a SELECT ... FOR UPDATE on the conversations row immediately before a message write) assumed softDeleteConversation's own transaction would contend for that SAME lock — but it updated chatMessages FIRST and conversations SECOND, so the lock stayed free for the whole message sweep. A concurrent send's FOR UPDATE could acquire it in that window, observe isActive: true (still true — the conversations UPDATE hadn't run yet), insert a new active message, and commit, all before the delete transaction finally flipped isActive. Reordered to conversations FIRST, then chatMessages — the lock is now held for the transaction's WHOLE duration, closing the window entirely. Existing ordering-assertion test updated to match; the fix is otherwise self- verified by the round-2 atomic re-check's own existing race tests (they now exercise the corrected order). - Ignore stale History reopen completions (P2): a slow same-session reopen completing after the user has since picked something else on the SAME pane (another History row, or a different agent via mint) previously applied unconditionally, letting network timing decide the visible transcript. Added a per-pane token (`paneAssignTokens`, bumped by both `handlePickHistoryConversation` and `handlePickAgent`) — a completion checks it's still current before applying its result, at each of the 3 points a supersede could have happened (post-reopen-error, post-reopen- success, pre-assign). Coverage is for the two explicitly reported scenarios (repeated History picks; a pending reopen raced by an agent mint) — the `handleSwitchAgent` FOCUS branch (an existing-conversation agent switch, not a mint) isn't wired into the same token, since which pane it actually touches isn't statically knowable without risking an incorrect invalidation. - Re-read pane bindings after History deletion (P2): `handleHistoryDeleteConversation` closed over `workspace` from whenever the enclosing callback was created — stale by the time it runs, always after the DELETE's own async round trip. A pane reassigned to something else while the delete was in flight got reset based on its now-irrelevant old binding. Now reads `useAgentWorkspaceStore.getState().workspaces[sessionId]` fresh at call time instead. 2 new regression tests reproduce both AgentPanes races directly (a fast second pick superseding a slow first reopen; a pane reassigned mid-delete staying put). 140 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
|
@codex review Round 5 pushed (22538c5): fixed the P1 lock-ordering bug in softDeleteConversation (conversations row now locked before the chatMessages sweep, closing the window your finding identified), plus two P2 races in AgentPanes (stale reopen completions ignored via a per-pane token; a stale workspace snapshot in the History-delete pane-rebind now reads fresh at completion time). One partial-coverage note on the reopen-token fix explained inline — flagging it rather than silently leaving it. 140 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
PageSpace/apps/web/src/components/agents/panes/AgentPanes.tsx
Lines 783 to 784 in 22538c5
Supersede pending reopens on selector focus
When a slow History reopen is pending for this pane and the user selects an agent whose conversation already exists in the session, this focus branch never advances the pane's assignment token. The older reopen therefore still considers itself current and later assigns its conversation back into the originating pane, despite the intervening selector action. Fresh evidence after the prior stale-reopen finding is this uncovered non-mint branch; invalidate the originating pane's token before calling openConversation.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uiMessage: userMessage, | ||
| dbClient: tx, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
When a History DELETE is permitted for a session-less conversation, or for a session with another open listing, it can acquire the row lock immediately after this transaction commits, mark the conversation and its existing messages inactive, and then return while this request continues into model generation. The later createStreamLifecycle and saveTerminalAssistantMessage calls do not recheck conversations.isActive, so they can persist an active assistant row beneath the now-hidden conversation and bill for generation the user deleted. Fresh evidence after the earlier lock fix is that the lock is released here, well before those unconditional assistant writes; deletion needs to abort the generation or terminal persistence must require an active conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a81ae2. Page-agent route: saveTerminalAssistantMessage — the single existing choke point all 3 terminal-write sites (execute-end, onFinish, outer-catch cleanup) already route through for the mention-notification contract — now wraps its persist in the same locked-check-then-save pattern as the user's own message, so all 3 are covered by one change rather than three separate re-checks. Global route had no equivalent shared wrapper, so I added saveTerminalGlobalAssistantMessage mirroring it and switched its 3 terminal-write call sites over. A deleted conversation now silently drops the reply instead of persisting it orphaned. On the billing half of your finding: the provider call itself is untouched by this — it still gets billed/tracked via the existing trackUsage path regardless (the tokens were already consumed by the time this write would run), this only gates what gets WRITTEN to the now-gone conversation's transcript. Whether a deleted-mid-generation run should also skip billing/refund credits felt like a separate product decision rather than a pure bug, so I left it out — happy to take a follow-up pass if that's wanted.
| }); | ||
| return false; | ||
| } | ||
| if (!isCurrent()) return false; |
There was a problem hiding this comment.
When a same-session reopen succeeds after another History pick or agent mint has superseded it, returning here protects the pane assignment but does not undo the POST's server-side effect: the abandoned conversation now has closedInSessionAt cleared and occupies a session listing/cap slot with no pane showing it. This can make the newer reopen fail as “session full” or leave an invisible open listing. Fresh evidence after the prior stale-completion finding is this success path's early return without a compensating close or other serialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a81ae2 — the stale-completion branch now closes the conversation right back out via cleanupOrphanedConversation (the same helper an abandoned mint already uses for exactly this "undo a successful-but-now-unwanted server-side effect" purpose), freeing the cap slot it consumed. Strengthened the existing stale-reopen regression test to also assert this DELETE fires with the right URL.
| const existingPane = workspace | ||
| ? panesOf(workspace).find( | ||
| (p) => p.id !== paneId && p.scope?.kind === 'chat' && p.scope.targetId === conversation.id, | ||
| ) |
There was a problem hiding this comment.
When two panes select the same closed conversation while their reopen requests are pending, both invocations retain the pre-request workspace snapshot, so each can miss the other pane's completed assignment and assign the conversation twice. That recreates the two independently interactive chat surfaces this check is intended to prevent. Fresh evidence after the earlier deduplication fix is that this lookup still uses the render-time snapshot after an await; read useAgentWorkspaceStore.getState() at completion time before assigning or focusing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a81ae2, exactly as suggested — the dedup lookup now reads useAgentWorkspaceStore.getState().workspaces[sessionId] fresh at the check instead of the workspace this callback closed over.
…g pane races Round-6 findings on PR #2299 (chatgpt-codex-connector, P1 + 2 P2): - Stop a send when deletion lands after the user write (P1): the round-2/5 atomic re-check only covers the USER's message — persisted early, inside a lock held for a moment. The ASSISTANT's reply persists LATER, after model generation (which can run for seconds, during which nothing else in the route blocks a History delete from landing). Without a matching check there, a conversation deleted mid-generation still gets an ACTIVE assistant message written beneath it — a reply to a conversation the user already asked to delete, invisible in every session listing. Page-agent route: `saveTerminalAssistantMessage` (the ALREADY-existing single choke point all 3 terminal-write sites — execute-end, onFinish, outer-catch — route through, added for the mention-notification contract) now wraps its persist in the same locked-check-then-save pattern, so all 3 sites are covered by one change. Global route had no equivalent single choke point, so a new `saveTerminalGlobalAssistantMessage` wrapper was added mirroring it, and its 3 terminal-write call sites switched to use it. A deleted conversation now silently drops the reply rather than persisting it orphaned — the provider call itself is still billed/tracked via the existing trackUsage path, untouched by this change; only what gets WRITTEN to the (now-gone) conversation is affected. - Roll back stale successful reopens (P2): the round-5 stale-completion token correctly stopped a superseded reopen from being APPLIED client-side, but didn't undo its SERVER-SIDE effect — a stale reopen that completed successfully still cleared `closedInSessionAt` and consumed a session cap slot with no pane showing it, which could make a later, legitimate reopen fail as "session full" for no visible reason. The stale branch now closes it right back out via the same `cleanupOrphanedConversation` helper an abandoned mint already uses. - Read live pane state when deduplicating reopens (P2): the "already open in another pane" dedup check read the `workspace` this callback closed over — stale by the time two panes' own async reopens both reach it, so neither could see the other's just-completed assignment and both could independently assign the same conversation. Reads `useAgentWorkspaceStore.getState()` fresh at the check instead. Existing stale-reopen regression test strengthened to also assert the rollback DELETE fires. 464 tests passing across the affected suites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
|
@codex review Round 6 pushed (0a81ae2): the P1 was a real gap — the atomic re-check from rounds 2/5 only covered the user's message, not the assistant's reply written later after generation. Fixed on both routes (page-agent via the existing 464 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a81ae29c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .where(eq(conversations.id, args.conversationId)) | ||
| .for('update') | ||
| .limit(1); | ||
| if (!row?.isActive) return false; |
There was a problem hiding this comment.
When a History deletion commits after the user-message transaction but before the unguarded placeholder insert at route.ts:1510, the delete has already swept existing messages, so this inactive branch silently skips the terminal write while leaving the newly inserted isActive: true, status: 'streaming' placeholder beneath the deleted conversation. Fresh evidence after the prior terminal-write finding is this earlier assistant-row write, which also exists in the global route at messages/route.ts:1251; guard that insert with the same conversation lock/check (or explicitly remove/deactivate the placeholder) so deletion cannot leave an orphaned streaming row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f3fe93. You are right — the stream-start placeholder INSERT (both route.ts:1510 page-agent and messages/route.ts:1251 global) ran unguarded, on the plain db, with no isActive re-check. If a History delete commits in the window between the guarded user-message write and this insert, the row would have been planted as isActive: true, status: 'streaming' under an already-inactive conversation — and since it is a BRAND NEW row, the later terminal-write guard (saveTerminalAssistantMessage/saveTerminalGlobalAssistantMessage) checking isActive at that point would just skip its update, leaving it stuck at 'streaming' permanently. Visible again the moment this PR's reopen feature brings the conversation back.
Both inserts now run inside the same db.transaction + SELECT ... FOR UPDATE pattern already used for the terminal writes: lock the conversation row, check isActive, only insert if still active. Best-effort framing preserved (own try/catch, non-throwing) since a skipped placeholder just means history cannot show the in-flight entry mid-stream — generation still proceeds. Added regression tests on both routes covering the skip-on-race and no-false-positive-on-active cases.
| // full" for no visible reason. Close it right back out, the same | ||
| // way an orphaned mint is cleaned up above (review finding — | ||
| // chatgpt-codex-connector on PR #2299). | ||
| void cleanupOrphanedConversation(conversation.id); |
There was a problem hiding this comment.
When the same closed conversation is selected twice quickly, the newer reopen can finish first and legitimately assign the conversation, after which the older request is stale and this unconditional cleanup closes that now-visible conversation out of the session again. The same occurs if another pane selects it while the stale request is pending. Before rolling back, read the live workspace and only close the conversation if no pane currently displays it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0f3fe93. Good catch — the rollback at AgentPanes.tsx:703 closed unconditionally whenever isCurrent() was false, which did not distinguish 'genuinely orphaned' from 'a newer pick on the same conversation already won and is now showing in a pane.' The same conversation picked twice quickly (or raced by a second pane) can have the newer reopen resolve first and legitimately land, with the older request's reopen resolving second and stale.
Now reads live workspace state across every pane in the grid (not just this pane) before closing — useAgentWorkspaceStore.getState().workspaces[sessionId], same live-read pattern already used a few lines below for the focus-existing-pane dedup check — and only calls cleanupOrphanedConversation when no pane currently shows that conversation. Added a regression test: same conversation picked twice on the same pane, older reopen resolves after the newer one already landed it, asserts no DELETE fires.
…ncurrently-landed reopen Round 7 review findings on PR #2299: - The 'streaming' placeholder assistant row inserted right after createStreamLifecycle resolves (both page-agent and global routes) had its own unguarded window: a History delete committing between the guarded user-message write and this insert could plant an isActive:true row under a now-inactive conversation. Since the row is brand new, the later terminal-write guard's isActive check would just skip its update, leaving it stuck at 'streaming' forever — visible again, permanently "in progress", once the conversation is reopened. Guarded with the same locked isActive re-check pattern as the terminal writes. - handlePickHistoryConversation's stale-reopen rollback closed the conversation unconditionally whenever its own token was superseded. When the SAME conversation is picked twice quickly, the newer pick can win the race and legitimately land in a pane before the older, now- stale reopen resolves — the unconditional close would then rip that just-reopened, currently-visible conversation back out. Now reads live workspace state across the whole grid before closing, and skips it if any pane still shows the conversation. 620 tests passing, tsc/eslint clean.
|
@codex review Round 7 pushed (0f3fe93): 2 findings, both real.
620 tests passing, tsc/eslint clean. |
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 (1)
apps/web/src/app/api/ai/global/[id]/messages/route.ts (1)
1418-1447: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate
assistantMessagePersistedand theconversationstimestamp update on the actual persist result.Both terminal-write sites call
saveTerminalGlobalAssistantMessage(...)without capturing itsPromise<boolean>result, then unconditionally setassistantMessagePersisted = trueand unconditionally rundb.update(conversations).set({ lastMessageAt: new Date(), updatedAt: new Date() }).When the conversation was deleted between generation start and this terminal write — the exact race this PR closes —
saveTerminalGlobalAssistantMessagereturnsfalseand skips the message insert. The two lines that follow it still run, so the "deleted" conversation row getslastMessageAt/updatedAtbumped anyway, andassistantMessagePersistedis markedtrueeven though nothing was saved. This defeats the isolation the new lock is meant to provide: the message content is protected, but the conversation row itself is still touched.The sibling helper in
apps/web/src/app/api/ai/chat/route.ts(saveTerminalAssistantMessage) gates its own post-write side effect the correct way:if (persisted && mentionNotify) mentionNotified = true;. Apply the same pattern here.🐛 Proposed fix for both call sites
try { - await saveTerminalGlobalAssistantMessage({ + const persisted = await saveTerminalGlobalAssistantMessage({ messageId: serverAssistantMessageId, conversationId, userId, role: 'assistant', ...payload, status: aborted ? 'interrupted' : 'complete', }); - assistantMessagePersisted = true; - - // A conversation with newly-written assistant content — even empty-content, - // interrupted content — must still surface as recently active in a list - // sorted/filtered by lastMessageAt. onFinish's own responseMessage branch below - // does the same bump when it gets to refine this write with richer content. - await db - .update(conversations) - .set({ - lastMessageAt: new Date(), - updatedAt: new Date(), - }) - .where(eq(conversations.id, conversationId)); + if (persisted) { + assistantMessagePersisted = true; + + // A conversation with newly-written assistant content — even empty-content, + // interrupted content — must still surface as recently active in a list + // sorted/filtered by lastMessageAt. onFinish's own responseMessage branch below + // does the same bump when it gets to refine this write with richer content. + await db + .update(conversations) + .set({ + lastMessageAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(conversations.id, conversationId)); + } } catch (e) { loggers.api.error('Global Assistant Chat API: execute-end persist failed', e as Error); }Apply the equivalent change in
onFinish(Lines 1488-1508):try { const messageContent = extractMessageContent(responseMessage); ... - await saveTerminalGlobalAssistantMessage({ + const persisted = await saveTerminalGlobalAssistantMessage({ messageId, conversationId, userId, role: 'assistant', content: messageContent, toolCalls: extractedToolCalls.length > 0 ? extractedToolCalls : undefined, toolResults: extractedToolResults.length > 0 ? extractedToolResults : undefined, uiMessage: responseMessage, status: aborted ? 'interrupted' : 'complete', }); - assistantMessagePersisted = true; - - // Update conversation lastMessageAt - await db - .update(conversations) - .set({ - lastMessageAt: new Date(), - updatedAt: new Date(), - }) - .where(eq(conversations.id, conversationId)); - - loggers.api.debug('Global Assistant Chat API: AI response message saved to database', {}); + if (persisted) { + assistantMessagePersisted = true; + + // Update conversation lastMessageAt + await db + .update(conversations) + .set({ + lastMessageAt: new Date(), + updatedAt: new Date(), + }) + .where(eq(conversations.id, conversationId)); + + loggers.api.debug('Global Assistant Chat API: AI response message saved to database', {}); + } } catch (error) { loggers.api.error('Global Assistant Chat API: Failed to save AI response message', error as Error); }Also applies to: 1477-1513
🤖 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/app/api/ai/global/`[id]/messages/route.ts around lines 1418 - 1447, Capture the boolean result returned by saveTerminalGlobalAssistantMessage at both terminal-write sites, including the onFinish path, and only set assistantMessagePersisted to true and update conversations.lastMessageAt/updatedAt when that result is true. Preserve the existing error handling and status behavior while preventing post-write side effects when the persistence is skipped.
🧹 Nitpick comments (1)
apps/web/src/app/api/ai/chat/route.ts (1)
212-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated lock-then-skip transaction helper across the page-agent and global-assistant routes. Both routes implement the same
SELECT ... FOR UPDATEonconversations.isActivefollowed by a skip-on-inactive write, as two separately maintained functions with the same correctness contract.
apps/web/src/app/api/ai/chat/route.ts#L212-L243:saveTerminalAssistantMessageduplicates the lock-and-skip transaction logic; extract it (and the isActive check) into a shared utility that returns whether the write happened.apps/web/src/app/api/ai/global/[id]/messages/route.ts#L277-L309:saveTerminalGlobalAssistantMessageduplicates the same logic; migrate this to the shared utility too.Consolidating this pattern into one tested utility would also make it harder to reintroduce the kind of bug flagged separately on
apps/web/src/app/api/ai/global/[id]/messages/route.ts#L1418-L1447,L1477-L1513, where a caller of the duplicated helper forgot to honor its returned skip/persist flag.🤖 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/app/api/ai/chat/route.ts` around lines 212 - 243, Extract the lock-and-skip transaction, including the conversations.isActive check and boolean persisted result, from saveTerminalAssistantMessage into a shared tested utility. In apps/web/src/app/api/ai/chat/route.ts lines 212-243, replace the duplicated transaction with that utility and preserve mentionNotified handling. In apps/web/src/app/api/ai/global/[id]/messages/route.ts lines 277-309, migrate saveTerminalGlobalAssistantMessage to the same utility and ensure every caller honors its returned write status.
🤖 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/ai/shared/hooks/__tests__/useConversations.test.ts`:
- Around line 154-170: Rename the test case containing deleteConversation with
enabled: false to describe only that a successful DELETE resolves true and
invokes onConversationDelete; remove the unsupported “invalidates the SWR cache”
claim from the test name while leaving its assertions unchanged.
---
Outside diff comments:
In `@apps/web/src/app/api/ai/global/`[id]/messages/route.ts:
- Around line 1418-1447: Capture the boolean result returned by
saveTerminalGlobalAssistantMessage at both terminal-write sites, including the
onFinish path, and only set assistantMessagePersisted to true and update
conversations.lastMessageAt/updatedAt when that result is true. Preserve the
existing error handling and status behavior while preventing post-write side
effects when the persistence is skipped.
---
Nitpick comments:
In `@apps/web/src/app/api/ai/chat/route.ts`:
- Around line 212-243: Extract the lock-and-skip transaction, including the
conversations.isActive check and boolean persisted result, from
saveTerminalAssistantMessage into a shared tested utility. In
apps/web/src/app/api/ai/chat/route.ts lines 212-243, replace the duplicated
transaction with that utility and preserve mentionNotified handling. In
apps/web/src/app/api/ai/global/[id]/messages/route.ts lines 277-309, migrate
saveTerminalGlobalAssistantMessage to the same utility and ensure every caller
honors its returned write status.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba02b2b7-3724-4f10-b118-4f327e982dc7
📒 Files selected for processing (8)
apps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/__tests__/AgentPanes.test.tsxapps/web/src/lib/ai/shared/hooks/__tests__/useConversations.test.tsapps/web/src/lib/ai/shared/hooks/useConversations.tsapps/web/src/lib/repositories/__tests__/conversation-repository.test.tsapps/web/src/lib/repositories/conversation-repository.ts
Round 7 review (coderabbitai): the case is mounted with enabled: false, so swrKey is null and mutate is never called — the title's "invalidates the SWR cache" claim isn't actually exercised. Renamed to describe what it verifies (resolves true, notifies the parent).
|
Round 7b pushed (a966237): fixed the test-name nit (misleading '''invalidates the SWR cache''' claim on a case where |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9662372da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const stillOrphaned = | ||
| !liveWorkspace || | ||
| !panesOf(liveWorkspace).some((p) => p.scope?.kind === 'chat' && p.scope.targetId === conversation.id); | ||
| if (stillOrphaned) { | ||
| void cleanupOrphanedConversation(conversation.id); |
There was a problem hiding this comment.
When the same closed conversation is selected twice and the older reopen finishes first, that request is stale while the newer request is still pending, so no pane displays the conversation yet and this check declares it orphaned. The resulting cleanup DELETE can acquire the session-listing lock after the newer reopen, close the conversation the newer request then assigns, and leave the visible pane outside the session listing. Fresh evidence in the latest code is that orphan detection considers only landed pane bindings, not newer pending assignments for the same conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fef0119. Correct — the round-7 fix only checked whom currently shows the conversation, not whether something ELSE is about to. Added pendingReopenCounts, an in-flight count per conversationId (incremented before the reopen POST, decremented exactly once regardless of exit path). The rollback now closes the conversation only when nothing shows it AND no other reopen for that same conversationId is still pending — so the older request's stale completion defers to the newer one that's still in flight rather than racing it. Regression test: two picks of the same conversation, both left pending, older resolved first — asserts no DELETE fires, then the newer resolving lands it normally.
| const affectedPanes = panesOf(liveWorkspace).filter( | ||
| (p) => p.scope?.kind === 'chat' && p.scope.targetId === deletedConversationId, | ||
| ); | ||
| for (const pane of affectedPanes) { | ||
| resetPane(sessionId, pane.id); |
There was a problem hiding this comment.
When a closed conversation's reopen has committed server-side but its response is delayed, the user can delete that conversation from the still-visible History tab before the completion assigns it. This scan finds no affected pane because the target is only pending, deletion does not invalidate the pane's assignment token, and the delayed reopen completion subsequently assigns the now-history-deleted conversation, producing a pane whose sends 404. Fresh evidence in the current code is that successful deletion resets only presently bound panes without accounting for pending picks of the deleted ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fef0119. Good catch — this one's outside the pane-token mechanism entirely, since no pane is bound to the target while its reopen is still in flight, so handleHistoryDeleteConversation's affected-panes scan finds nothing to reset. Added a second ref, historyDeletedDuringReopen (a Set of conversationIds), unconditionally marked by handleHistoryDeleteConversation regardless of whether any pane is currently affected. The reopen completion now checks-and-consumes that mark (Set.delete returns whether it was present) right before assigning, and bails out with no assignment if it was set — treated the same as any other superseded pick. Regression test: reopen started (pending), same conversation deleted from History mid-flight, delayed reopen resolves — asserts the pane never gets assigned to the deleted id.
Review findings on PR #2299: - The round-7 stale-reopen rollback checked whether any pane currently shows the conversation before closing it, but missed the case where a NEWER pick for the same conversation is still in flight when the OLDER one resolves — at that instant nothing shows it yet, but something is about to. Added an in-flight reopen counter per conversationId; the rollback now also checks that no other reopen for the same conversation is still pending before closing it. - A reopen can commit server-side and then have its own HTTP response delayed long enough for the same conversation to be deleted from History before the completion assigns it — landing a pane on a transcript that already 404s on send. handleHistoryDeleteConversation now marks the deleted id; the reopen completion checks-and-consumes that mark before assigning, and bails out (no assignment) if it was set. 273 tests passing (agents), tsc/eslint clean.
|
@codex review Round 8 pushed (fef0119): 2 more real races in the History-reopen flow.
273 agents tests passing, tsc/eslint clean. |
|
@codex review Round 15 pushed (ea64377): 3 findings, all real.
432 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Invalidate pending History picks on focus switches
When a History reopen is pending and the user switches this pane to an agent whose conversation is already open in the session, the focus branch bypasses handlePickAgent and never advances the pane's assignment token. openConversation can therefore repoint the pane to the selected agent, but the delayed reopen still considers itself current and subsequently overwrites that newer selection with the older History conversation. Supersede the pending assignment before taking the focus path as well.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // opened it, so rolling it back would close a listing still in | ||
| // use for a no-op this request didn't cause (review finding — | ||
| // chatgpt-codex-connector on PR #2299, round 15). | ||
| if (reopenResult.alreadyOpen || isShownSomewhere()) return false; |
There was a problem hiding this comment.
When two reopens for the same closed conversation are both superseded, the first request can genuinely reopen the listing and set deferredReopenSuccess while the second remains pending; the server then reports the second request as alreadyOpen. If the genuine response arrives first, the last no-op response returns here without consuming the deferred marker or closing the orphan, leaving an invisible open listing that occupies a session-cap slot. The last settler still needs to perform any cleanup deferred by the request that actually transitioned the listing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf7173c. Confirmed — my own round-15 fix short-circuited on alreadyOpen BEFORE ever checking whether this was the last settler for the conversationId, so a sibling's deferred cleanup (a genuine transition that deferred because this request was still pending) never got drained if the LAST response happened to be a no-op. Reordered: the last-settler check (remaining <= 0 && deferredReopenSuccess.delete(...)) now runs FIRST, before deciding anything based on THIS request's own alreadyOpen/transition status — draining a sibling's deferred cleanup takes priority over this request's own (lack of) outcome. Regression test: older reopen genuinely transitions and defers (newer still pending), newer — also superseded — resolves reporting alreadyOpen: asserts the older's deferred cleanup still fires.
…er's own outcome is alreadyOpen (round 16) Review finding on PR #2299: the round-15 alreadyOpen early-return skipped the "am I the last settler for this conversationId, do I need to finish a sibling's deferred cleanup" check entirely. Two overlapping reopens for the same conversation, both superseded — the older genuinely transitions the listing and defers its own rollback (a newer sibling is still pending); if that newer sibling is the one to settle last and its own outcome happens to be alreadyOpen, the deferred marker from the older request was never drained, leaking an invisible open listing (and the session cap slot it holds) forever. The last-settler check now runs before deciding what to do with THIS request's own outcome — draining any deferred sibling first regardless of whether this specific reopen was itself a transition or a no-op. 282 tests passing (agents), tsc/eslint clean.
|
@codex review Round 16 pushed (cf7173c): the alreadyOpen fix from round 15 had its own gap — the last-settler-drains-a-sibling's-deferred-cleanup check ran AFTER (and was short-circuited by) the alreadyOpen early-return, so a genuine transition that deferred could leak forever if the LAST response to arrive happened to be a no-op. Reordered so the last-settler drain always runs first, regardless of this specific request's own outcome. 282 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf7173cc15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setConfig: (next) => { | ||
| void mutate(next, { revalidate: false }); |
There was a problem hiding this comment.
When two Settings surfaces for the same agent are mounted and one contains unsaved edits, saving the other calls setConfig here and broadcasts the new object through the shared SWR entry. PageAgentSettingsTab resets its entire form whenever config changes, so the sibling surface silently loses all of its unsaved edits. Defer applying external cache updates while a form is dirty, or merge them without resetting locally edited fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48cbd57. Confirmed and a real regression from the shared-identity design earlier in this PR (the user explicitly asked for one source of truth across panes) — PageAgentSettingsTab's config-reset effect had no dirty-check at all, so a SIBLING surface for the same agent saving would silently wipe unsaved edits here. Reused the useEditingStore dirty-tracking this same file already registers with for a DIFFERENT purpose (blocking SWR revalidation mid-edit): the reset effect now skips while the form is dirty, unless a ref (set right before THIS instance's own onConfigUpdate call) marks that the update is this instance's own save — in which case it still resets to the just-saved values as the new clean baseline, same as before. Note: PageAgentSettingsTab has no pre-existing test file (not something this PR introduced); verified via typecheck, lint, and a manual trace of both the own-save and sibling-update paths rather than a new test harness — happy to add one if you'd like it.
| const hadDeferredSibling = remaining <= 0 && deferredReopenSuccess.current.delete(conversation.id); | ||
| if (shownSomewhere) return false; | ||
| if (hadDeferredSibling) { | ||
| void cleanupOrphanedConversation(conversation.id); |
There was a problem hiding this comment.
When this cleanup DELETE is delayed, the user can select the same conversation again after the shownSomewhere snapshot and after the pending count reached zero; that new reopen may return alreadyOpen and assign the pane before this older DELETE reaches the server, after which the DELETE closes the now-visible listing. Fresh evidence beyond the prior pending-reopen fix is that a reopen beginning after cleanup is launched is absent from both the snapshot and the settled counter. Serialize cleanup with later picks or compensate after it completes if the conversation is now displayed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48cbd57. Good catch — cleanupOrphanedConversation's DELETE had no serialization against a later pick landing the same conversation while it's in flight. Now rechecks isConversationShownSomewhere (hoisted out of handlePickHistoryConversation into a shared callback both call sites use) right after the DELETE resolves, and compensates with a reopen call if the conversation is displayed by then. Regression test: pick superseded and genuinely orphaned (cleanup DELETE pending/delayed), a fresh pick of the same conversation lands it in the pane while the DELETE is still in flight, DELETE then resolves — asserts a compensating reopen call fires.
…a delayed orphan-cleanup DELETE (round 17) Review findings on PR #2299: - PageAgentSettingsTab's config-reset effect fired unconditionally on every config change, including one propagated from a SIBLING Settings surface for the same agent saving through the shared useAgentConfig cache (the "one source of truth" identity the user explicitly asked for earlier in this PR). A form with unsaved edits would have them silently wiped the moment a different pane's Settings tab for the same agent saved. Reused the project's existing useEditingStore dirty- tracking pattern already present in this same file: the reset effect now skips when the form is dirty, UNLESS this instance's own save is what triggered the update (tracked via a ref set right before this instance's own onConfigUpdate call), so a save's own values still become its new clean baseline as before. - cleanupOrphanedConversation's DELETE is fire-and-forget with no serialization against a LATER, independent pick of the exact same conversationId — if that DELETE is delayed, a fresh pick can land the conversation in a pane before the DELETE reaches the server, which then closes a listing that is now visibly displayed. Now rechecks right after its own DELETE resolves and compensates with a reopen call if the conversation is shown somewhere by then. Hoisted the shown-somewhere check (previously local to handlePickHistoryConversation) to a shared callback both call sites now use. 524 tests passing (agents + shared hooks + page-agents), tsc/eslint clean. Note: PageAgentSettingsTab has no pre-existing test file (a gap this PR did not introduce) — the fix follows the project's own established useEditingStore pattern already in the same file; verified by typecheck, lint, and manual trace, not a new dedicated test harness.
|
@codex review Round 17 pushed (48cbd57): 2 findings, both real, in new areas (Settings shared-cache, orphan cleanup).
524 tests passing, tsc/eslint clean. Noted in the Settings fix reply: PageAgentSettingsTab has no pre-existing test file, so that fix is verified by typecheck/lint/manual trace rather than a new dedicated test. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48cbd570ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { | ||
| selectedProvider, | ||
| setSelectedProvider, | ||
| selectedModel, | ||
| setSelectedModel, | ||
| isProviderConfigured, | ||
| } = useProviderSettings(scope.agentPageId ? { pageId: scope.agentPageId } : {}); |
There was a problem hiding this comment.
When two Settings surfaces for the same agent are mounted, each invocation of useProviderSettings owns independent selectedProvider/selectedModel state and only loads it once. If one surface saves a provider change, the shared config cache updates the sibling, but these values remain stale; that sibling's controls still show the old selection, and saving an unrelated setting later overrides the request data with the stale provider/model, reverting the first save. Derive these selections from the shared config or update every mounted provider state when that config changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4a41524. Confirmed — useProviderSettings is a separate, per-mount hook with NO shared cache (unlike useAgentConfig), so its local selectedProvider/selectedModel never learn about a sibling's save. Rather than trying to synchronize that hook across instances (a larger change, and it's used by other surfaces beyond this one per its own file header), fixed it locally in PageAgentSettingsTab: two refs track whether THIS instance's provider/model controls were explicitly touched (wrapped onValueChange handlers set them). onSubmit now prefers the shared config's aiProvider/aiModel (reflecting any sibling's save) when untouched, falling back to the local selection only when the user actually picked something here — so an explicit local choice always wins, but a stale unused snapshot never silently overwrites a sibling's save.
There was a problem hiding this comment.
Following up on my earlier reply — that fix (providerTouchedRef/modelTouchedRef in onSubmit) closed the data-loss half of this: a sibling's saved provider/model can no longer be silently reverted by an untouched instance's stale submit. But you were right that a real gap remained: the <Select value={selectedProvider}>/value={selectedModel} controls still rendered the raw per-mount useProviderSettings state directly, which is never resynced when a sibling saves — so an untouched sibling's dropdowns could keep displaying the wrong provider/model indefinitely (cosmetic, since a subsequent save from that instance already resolved correctly, but a real and confusing gap until then).
Fixed in 9084bb8: added displayedProvider/displayedModel, computed with the exact same touched/untouched resolution onSubmit already used (providerOrModelTouched ? selectedProvider : (config?.aiProvider || selectedProvider)), read at render time. Both Selects, the model list (getCurrentProviderModels), the SelectLabel, and the eager Ollama/LM Studio model-fetch effect now all key off displayedProvider/displayedModel instead of the raw per-mount props — so an untouched sibling's display now follows a saved change from another Settings surface for the same agent, matching what onSubmit would already correctly submit.
Leaving this thread open for verification per our process.
| if (isConversationShownSomewhere(conversationId)) { | ||
| await post( | ||
| `/api/agent-sessions/${encodeURIComponent(sessionId)}/conversations/${encodeURIComponent(conversationId)}/reopen`, | ||
| {}, | ||
| ); |
There was a problem hiding this comment.
When this compensating POST is slow, the pane that caused isConversationShownSomewhere to return true can switch to another conversation before the POST resolves. The POST then successfully reopens the old listing after it has become orphaned, and nothing performs another check or cleanup, so the invisible conversation consumes a session-cap slot. The compensation needs serialization with subsequent pane assignments or a post-reopen orphan check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4a41524. Right — this is the same race the compensating reopen itself was built to close, just one level deeper. Added a re-check after the compensating reopen resolves; if the conversation is orphaned again by then, it recurses into another cleanup pass rather than leaving it unnoticed. This recurses (not loops) so it naturally terminates once the grid stops churning through the same conversation. Regression test: compensating reopen left pending, the pane moves to something else again before it resolves, then it resolves — asserts a SECOND cleanup DELETE fires.
…hot; re-check ownership after compensating reopen (round 18) Review findings on PR #2299: - PageAgentSettingsTab's onSubmit unconditionally sent useProviderSettings's local selectedProvider/selectedModel as part of every save's request body. That hook is a separate, per-mount instance with no shared cache (unlike useAgentConfig) — a sibling Settings surface for the same agent saving a provider change would leave THIS instance's copy stale, and the next unrelated save here would silently revert that sibling's change. Now tracks whether THIS instance's own provider/model controls were explicitly touched; if not, prefers the shared config's value (reflecting any sibling's save) instead of the stale local snapshot. An explicit local choice still always wins. - cleanupOrphanedConversation's round-17 compensating reopen has the same race its own fix was closing: if that reopen itself is slow, the pane that made it necessary can move on to something else again before it lands, leaving the conversation orphaned a second time with nothing left to notice. Now re-checks once more after the compensating reopen resolves and recurses into another cleanup pass if still orphaned. 312 tests passing (agents + page-agents), tsc/eslint clean.
|
@codex review Round 18 pushed (4a41524): 2 findings, both real, one in a new area (provider/model selection state).
312 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
PageSpace/apps/web/src/components/agents/panes/AgentPanes.tsx
Lines 1081 to 1085 in 4a41524
Invalidate pending reopens on focus-only agent switches
When a History reopen is pending and the user switches to an agent whose conversation is already open but currently unshown, this focus branch rebinds the pane through openConversation without advancing its assignment token. The earlier reopen therefore still sees isCurrent() as true when it resolves and overwrites the newer agent selection. Fresh evidence after the prior stale-completion fix is that only the mint branch enters handlePickAgent/beginPaneAssign; advance the token before this focus path as well.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resolvedProvider = providerTouchedRef.current ? selectedProvider : (config?.aiProvider || selectedProvider); | ||
| const resolvedModel = modelTouchedRef.current ? selectedModel : (config?.aiModel || selectedModel); |
There was a problem hiding this comment.
When this surface changes its provider/model and successfully saves once, providerTouchedRef and modelTouchedRef remain true for the component's entire lifetime. If a sibling Settings surface subsequently saves a different provider/model, a later unrelated save here still chooses this surface's now-stale local values on these lines and reverts the sibling's change. Fresh evidence after the prior synchronization fix is that the new touch markers are set to true but never reset after establishing the saved baseline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a950d6. Correct — I set the touch markers but never cleared them, so touching the provider dropdown even ONCE would pin this instance to its local snapshot for its entire lifetime, reintroducing the original round-18 bug for every subsequent save. Cleared both refs at the same point justSavedOwnConfigRef is set (right before onConfigUpdate) — config.aiProvider/aiModel now correctly reflect what THIS instance just saved, so preferring the shared config from that point forward is correct for both this instance's own value and any later sibling's save.
…ve (round 19) Review finding on PR #2299: the round-18 fix set providerTouchedRef/ modelTouchedRef to true on the first local edit but never reset them, so once a user ever touched the provider dropdown in a given Settings surface, onSubmit would pin to that instance's local snapshot FOREVER — reintroducing the original bug for every subsequent save, since a later sibling save would always be reverted by this instance's now-stale value on its next unrelated save. Cleared both refs right alongside justSavedOwnConfigRef, at the same point this instance's own save becomes its new clean baseline — config.aiProvider/aiModel now correctly reflect what was just chosen, so preferring the shared config from this point on is exactly as correct for this instance's own save as for a later sibling's. 312 tests passing (agents + page-agents), tsc/eslint clean.
|
@codex review Round 19 pushed (3a950d6): a follow-up gap in my own round-18 fix — the touch markers were set on first edit but never cleared, so they pinned a Settings surface to its local snapshot forever after the first touch, reverting every later sibling save. Cleared both refs at the point this instance's own save becomes its new clean baseline. 312 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a950d6b33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resolvedProvider = providerTouchedRef.current ? selectedProvider : (config?.aiProvider || selectedProvider); | ||
| const resolvedModel = modelTouchedRef.current ? selectedModel : (config?.aiModel || selectedModel); | ||
| const requestData = { | ||
| ...data, |
There was a problem hiding this comment.
When one Settings surface is dirty and a sibling saves a different field, the reset effect deliberately retains this surface's old form snapshot while its config prop advances. Saving the dirty surface later spreads every stale field from data into the PATCH, silently reverting sibling changes to fields the user never touched here. Fresh evidence after the round-17 dirty guard is this whole-form submission; merge the latest config for untouched fields or submit only dirtyFields plus this surface's edits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e52b8. Confirmed — this surface deliberately keeps its stale form snapshot while dirty (that's the point of the round-17 dirty guard), but onSubmit was still spreading the WHOLE data object into the PATCH, so any OTHER field a sibling had already saved would get silently reverted the moment this surface saved anything. Now merges per-field: react-hook-form's own dirtyFields says which fields THIS instance's form actually has a pending edit for; only those come from data, everything else comes from the freshest shared config.
| const resolvedProvider = providerTouchedRef.current ? selectedProvider : (config?.aiProvider || selectedProvider); | ||
| const resolvedModel = modelTouchedRef.current ? selectedModel : (config?.aiModel || selectedModel); |
There was a problem hiding this comment.
When a sibling changes the provider, this mounted surface's useProviderSettings controls still display the old provider and its models. If the user then changes only the model here, providerTouchedRef is false while modelTouchedRef is true, so these lines combine the sibling's new provider with a model selected from the stale provider; the agent-config route rejects that pair with a 400 and the user's other edits cannot be saved. Fresh evidence after the round-18 fallback is this independently resolved provider/model pair; synchronize the controls or treat the pair as one atomic selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e52b8 — good catch, this was a real gap in my round-18/19 fix resolving provider and model independently. They're now resolved as ONE atomic pair: if EITHER was touched in this instance, BOTH come from this instance's own current selection (internally consistent, since the model list is derived from the currently-selected provider); otherwise BOTH come from the shared config (also consistent). Never mixes one from each source anymore.
| /> | ||
| ) : scope.agentPageId !== null && agent ? ( | ||
| <div className="h-full overflow-auto"> | ||
| <PageAgentSettingsTab |
There was a problem hiding this comment.
Adding another PageAgentSettingsTab mount here allows multiple forms for the same page, but every instance registers the identical page-agent-settings-${pageId} key in useEditingStore. If two surfaces are dirty and either one saves or unmounts, its cleanup deletes the shared Map entry while the other remains dirty; that remaining form is no longer counted by isAnyEditing(), so auth refresh and editing-paused background work can resume prematurely. Include a per-mount identifier in the registration key or make the store reference-count duplicate registrations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e52b8. Correct — added a stable per-mount id via React's useId(), folded into the useEditingStore registration key (page-agent-settings-${pageId}-${mountId}), so each mounted Settings surface for the same agent now registers and cleans up its OWN entry rather than sharing (and potentially deleting out from under) a sibling's.
…unt editing key (round 20) Review findings on PR #2299, all in PageAgentSettingsTab's multi-instance handling — the round-17/18/19 patches kept finding new derived symptoms of the same underlying gap: this component was built assuming exactly one mounted instance per agent, and the shared useAgentConfig cache (earlier in this PR) is what first made multiple simultaneous mounts of the same agent's Settings possible. - onSubmit spread the WHOLE form's data into the PATCH regardless of which fields the user actually edited in THIS instance. A dirty surface's stale snapshot of every OTHER field would silently overwrite a sibling's already-saved change to any field this instance never touched. Now merges: only fields react-hook-form's own dirtyFields marks as actually edited here come from the form; everything else comes from the freshest shared config. - Provider and model were resolved independently (round 18/19's touched- refs, checked separately) — touching only the model while a sibling changed the provider could combine this instance's stale-provider model with the sibling's new provider, an invalid pair the route rejects with a 400. Now resolved as ONE atomic pair: if EITHER was touched here, both come from this instance's own current selection (guaranteed internally consistent); otherwise both come from the shared config (also consistent). - useEditingStore's registration key was `page-agent-settings-${pageId}` — identical across every mounted instance for the same agent. Either instance saving or unmounting deleted the shared map entry, dropping a still-dirty sibling out of isAnyEditing() and letting SWR revalidation resume while it still had unsaved edits. Now keyed by a stable per-mount id (React's useId()) as well. 312 tests passing (agents + page-agents), tsc/eslint clean. Same note as rounds 17-19: PageAgentSettingsTab has no pre-existing test file; verified by typecheck, lint, and manual trace.
|
@codex review Round 20 pushed (d5e52b8): 3 findings, all continuing to surface the same underlying gap in PageAgentSettingsTab's multi-instance handling (rounds 17-19 already patched several derived symptoms of this).
312 tests passing, tsc/eslint clean. Same note as prior rounds: PageAgentSettingsTab has no pre-existing test file. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5e52b83ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // PR #2299, round 20). | ||
| const mergedFields = { | ||
| systemPrompt: dirtyFields.systemPrompt ? data.systemPrompt : (config?.systemPrompt ?? data.systemPrompt), | ||
| enabledTools: dirtyFields.enabledTools ? data.enabledTools : (config?.enabledTools ?? data.enabledTools), |
There was a problem hiding this comment.
When the user clicks Select All or Deselect All, those handlers call setValue('enabledTools', ...) without shouldDirty: true; React Hook Form therefore updates the displayed value but leaves dirtyFields.enabledTools false. This branch then takes config.enabledTools, so Save silently discards the bulk selection. Mark those setValue calls dirty or track bulk changes explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6866f71 — added shouldDirty: true to both setValue calls. Correct, and a direct regression from the round-20 dirty-only merge specifically.
| const requestData = { | ||
| ...data, | ||
| aiProvider: selectedProvider, | ||
| aiModel: selectedModel, | ||
| includeDrivePrompt: data.includeDrivePrompt, | ||
| agentDefinition: data.agentDefinition, | ||
| visibleToGlobalAssistant: data.visibleToGlobalAssistant, | ||
| includePageTree: data.includePageTree, | ||
| pageTreeScope: data.pageTreeScope, | ||
| ...mergedFields, | ||
| aiProvider: resolvedProvider, | ||
| aiModel: resolvedModel, | ||
| }; |
There was a problem hiding this comment.
When two mounted settings surfaces save different edits before either save's SWR update reaches the other, both build mergedFields from the same stale config snapshot and send every field; whichever PATCH finishes last overwrites the first surface's change with its old baseline. The route already supports partial PATCHes, so send only this instance's dirty fields (and the provider/model pair only when touched), or add revision-based conflict handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6866f71. Right — and on reflection this is a strictly better fix than round 20's merge approach, not just a patch on top of it: the agent-config route already applies each field conditionally (if (field !== undefined) updateData.field = ...), so it fully supports sparse PATCHes. onSubmit now sends ONLY the fields this instance's own form has a dirty edit for (plus the provider/model pair, only when touched) — never a value merely copied from a snapshot that could already be stale. This removes the race entirely rather than narrowing its window: two surfaces saving different fields concurrently now literally cannot conflict, since neither PATCH mentions a field the other one touched. Kept the local optimistic-cache merge (display-only, never sent to the server) for immediate UI feedback.
…tale snapshot; mark bulk tool selections dirty (round 21) Review findings on PR #2299, both derived from round 20's fix: - handleSelectAllTools/handleDeselectAllTools called setValue without shouldDirty: true — dirtyFields.enabledTools stayed false even though the displayed selection changed, so round 20's dirty-only merge would silently discard a bulk selection and resubmit the stale config value instead. Added shouldDirty: true to both calls. - Round 20's "merge untouched fields from the freshest config" still raced: two Settings surfaces saving different fields close enough together that neither's SWR update had reached the other yet would both build their "unedited" fields from the SAME stale snapshot, and whichever PATCH landed second would stomp the first's just-saved change. The agent-config route already applies each field only when present in the body (`if (field !== undefined) updateData.field = ...`), so onSubmit now sends a SPARSE PATCH — only fields this instance's own form actually has a dirty edit for (plus the provider/model pair, only when touched) — rather than ever resending a value merely copied from a snapshot that might already be stale. The local optimistic cache update (display-only, never sent to the server) still merges with the freshest config for immediate UI feedback. 312 tests passing (agents + page-agents), tsc/eslint clean. Same note as rounds 17-20: PageAgentSettingsTab has no pre-existing test file.
|
@codex review Round 21 pushed (6866f71): 2 findings, both good.
312 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6866f71497
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // safe — this just makes sure THIS instance's own edits are visible | ||
| // immediately without waiting on a revalidation round trip. | ||
| const updatedConfig = { | ||
| ...config, |
There was a problem hiding this comment.
When two settings surfaces save different fields concurrently, each onSubmit retains the config snapshot from when its own save began. Although the sparse PATCHes now preserve both changes in the database, the request that finishes last spreads that stale snapshot here and publishes it through the shared SWR cache, making the other successful change disappear from every mounted form and potentially allowing a later edit of the displayed stale value to overwrite it server-side. Apply dirtyPatch through a functional update against the current SWR value, or revalidate/use the persisted response instead of rebuilding from this closure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ff31b6. Correct — round 21 made the SERVER-side PATCH fully concurrency-safe, but the LOCAL cache update still built from a config value closed over when onSubmit was created, which could go stale relative to a sibling's meanwhile-arrived change. useAgentConfig's setConfig now also accepts an updater function, passed straight through to SWR's own mutate (which natively supports this form), so it merges against whatever the cache actually holds at apply time rather than a stale closure. PageAgentSettingsTab's onSubmit now uses that form. Added a regression test directly to useAgentConfig (which has existing coverage, unlike PageAgentSettingsTab) verifying two sequential updater calls each see the prior one's result rather than reverting it.
…ue, not a stale closure (round 22) Review finding on PR #2299: round 21 made the server-side PATCH fully concurrency-safe (sparse, field-scoped), but the LOCAL optimistic cache update still built its merged object from `config` — a value closed over when onSubmit was created. Two Settings surfaces for the same agent saving DIFFERENT fields close enough together that neither's SWR update had reached the other yet would each spread their own stale closure; whichever finished last would publish a snapshot missing the other's meanwhile-arrived change, making it disappear from the shared cache (and every mounted consumer) even though both PATCHes persisted correctly server-side. useAgentConfig's setConfig now also accepts an updater function `(current) => next`, passed straight through to SWR's own mutate (which natively supports this form) so it merges against whatever the cache actually holds at the moment it runs. PageAgentSettingsTab's onSubmit now calls onConfigUpdate with an updater instead of a plain merged value. 526 tests passing (agents + shared hooks + page-agents), tsc/eslint clean. New regression test added directly to useAgentConfig (which, unlike PageAgentSettingsTab, has existing coverage) verifying the updater form merges against the live cache value.
|
@codex review Round 22 pushed (7ff31b6): the round-21 sparse-PATCH fix made the server side fully concurrency-safe, but the LOCAL cache update still merged from a closure-captured config value. Fixed by giving useAgentConfig's setConfig a functional-updater form (passed straight through to SWR's own mutate) so it merges against the live cache value instead. Includes a new regression test in useAgentConfig itself. 526 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ff31b657d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // here routes that stale completion into the existing orphan-cleanup | ||
| // path (rounds 8-9) instead (review finding — chatgpt-codex-connector | ||
| // on PR #2299, round 10). | ||
| beginPaneAssign(paneId); |
There was a problem hiding this comment.
When the only pane is mid-mint and the session-conversation listing has not resolved, decideClosePane returns noop, but this call has already invalidated the mint's assignment token. If the mint then succeeds, its stale-completion path deletes the new conversation and returns without clearing the pane's loading scope; if it fails, the token guard likewise prevents restoration, leaving the pane spinning indefinitely even though the close itself did nothing. Only invalidate after a non-noop decision, or explicitly restore the pending pane when close cannot proceed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a33d5f. Correct — my round-10 fix bumped the token unconditionally at the top, before decideClosePane even ran. Moved it to only fire once a decision ACTUALLY commits to altering the pane (close-pane, rebind-pane, closeConversationListing) — not for noop, and not for the end-session ask-first branch either (the user can still cancel that dialog; a CONFIRMED end-session tears down the whole workspace via forgetWorkspace, at which point the existing paneStillLoading shape-check already correctly detects this pane is gone, no token needed). Regression test: a mid-mint, grid-last pane closed while the session-conversation listing hasn't resolved (a genuine noop) — asserts the pane isn't removed AND the mint still lands normally afterward rather than being discarded as superseded.
| onConfigUpdate((current) => ({ | ||
| ...current, | ||
| ...patchThisSave, | ||
| ...providerModelPatch, | ||
| } as AgentConfig)); |
There was a problem hiding this comment.
When two mounted settings surfaces save different values for the same field concurrently, PATCH response order need not match database-write order—for example, the earlier database write can have a slower post-write response. Each completion applies its own submitted value here, so the late response from the earlier write can overwrite the cache with a value that is no longer in the database. Because this mutate explicitly disables revalidation, every mounted form can retain the wrong value indefinitely; serialize same-field saves, use revisions, or revalidate after overlapping saves settle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a33d5f. Right — round 22's live-value merge closes the 'missing the other field entirely' case, but not this one: two PATCHes to the SAME field race on HTTP response arrival order, which need not match DB write order, and no amount of client-side merging can tell which response reflects the true final DB state. Added revalidate() to useAgentConfig (a plain SWR re-fetch, no args) and call it right after the optimistic update in onSubmit, so the cache reconciles to server ground truth once any overlapping sibling save has also landed. Added a regression test directly to useAgentConfig verifying revalidate() replaces an incorrect optimistic value with the server's response.
…nfig revalidation for out-of-order saves (round 23) Review findings on PR #2299: - handleClosePane bumped the closing pane's assignment token BEFORE deciding what the close actually does — including when the decision turns out to be 'noop' (the grid-last pane's session-conversation listing hasn't resolved yet, so decideClosePane can't safely act) or when 'end-session' is only being ASKED about and the user can still cancel. Invalidating a pending mint/reopen's token for a close that never actually happened left its own completion treating itself as superseded, discarding its result without ever moving the pane out of its loading state — stuck spinning forever for nothing. The token is now only bumped once a decision actually commits to altering the pane (close-pane, rebind-pane, closeConversationListing); the end-session ask-first branch doesn't need it at all, since a confirmed end-session tears down the whole workspace and the existing paneStillLoading shape-check already handles that case without a token. - Round 22 made the local config cache update merge against the LIVE SWR value instead of a stale closure, but that still can't fully resolve two sibling surfaces PATCHing the SAME field concurrently: HTTP response order need not match DB-write order (the earlier write can have the slower response), so whichever completion applies LAST here wins the cache regardless of which write actually landed last in the DB. Added a revalidate() method to useAgentConfig (a plain SWR re-fetch) and call it after every save, alongside the optimistic update, so the cache eventually reconciles to server ground truth once any overlapping sibling save has also had a chance to land. 528 tests passing (agents + shared hooks + page-agents), tsc/eslint clean. New regression tests: a no-op-close-doesn't-strand-a-pending-mint test in AgentPanes, and a revalidate()-corrects-an-optimistic-mismatch test in useAgentConfig.
|
@codex review Round 23 pushed (7a33d5f): 2 findings, both real.
528 tests passing, tsc/eslint clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a33d5f916
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /> | ||
| ) : scope.agentPageId !== null && agent ? ( | ||
| <div className="h-full overflow-auto"> | ||
| <PageAgentSettingsTab |
There was a problem hiding this comment.
When two panes show Settings for the same agent, each PageAgentSettingsTab created here owns an independent membership snapshot loaded only on mount. Changing the Drive Membership role in one pane updates only that instance, so the sibling continues displaying the old role indefinitely and can later overwrite the newer role based on stale UI. Share or revalidate the membership state after handleMembershipRoleChange, as is done for the agent config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this was a genuine gap (the only unresolved thread with no prior reply). Drive membership (membership/membershipUserRole/driveRoles) was local useState, fetched once per mount with no shared cache, so a role change in one Settings surface never reached a sibling instance for the same agent.
Fixed in 9084bb8: added useAgentMembership(driveId, pageId) — a shared, SWR-backed hook mirroring useAgentConfig, keyed by driveId so every mounted Settings surface reads the same agentMembers/roles cache entries. updateRole() now does PATCH + mutate() (revalidate) against that shared cache instead of a local setMembership(...) merge, so a role change from any instance is immediately visible in every other mounted instance for the same agent. PageAgentSettingsTab now consumes this hook instead of its own useEffect+useState pair. Covered by new unit tests in useAgentMembership.test.ts, including one asserting a second hook instance for the same agent sees an updated role with no extra fetch/local state.
Leaving this thread open for verification per our process.
… Settings Two Settings surfaces for the same agent (e.g. two panes both open to that agent's Settings tab) previously drifted: - Drive membership (role/custom role) was loaded into per-instance useState with no shared cache, so a role change in one instance never reached a sibling instance. New useAgentMembership hook mirrors useAgentConfig's SWR-shared-cache approach, keyed by driveId, and updateRole() revalidates that shared cache instead of merging locally. - The provider/model Selects rendered the raw per-mount selectedProvider/ selectedModel prop, which is never resynced when a sibling saves a different provider — onSubmit already preferred the shared config in that case (round 18), but the display never caught up, so an untouched sibling could show the wrong provider/model indefinitely. Added displayedProvider/displayedModel, mirroring onSubmit's own resolution logic, for the Selects, the model list, and the eager Ollama/LM Studio model fetch. review findings — chatgpt-codex-connector on PR #2299, round 26/27 (threads r3694897525, r3695144705)
Convergence status after round 24 (commit 9084bb8)CI: all 10 checks green (CodeQL ×2, Secret Scanning, Security Summary, Security Test Suite, Static Security Analysis, Dependency Audit, CodeRabbit, Lint & TypeScript, Unit Tests). Ran two independent adversarial audits over all 47 unresolved threads this round (re-reading each comment against current source, not trusting prior "Fixed in
Why 47 threads remain unresolved despite this: per this loop's own AUTO-RESOLVE SAFETY rule, only threads addressed before the loop started may be auto-resolved — threads fixed during the loop must stay open for human verification. All 47 remaining threads were addressed during this loop's 24 rounds, so none qualify for auto-resolution. This is intentional, not an oversight: the PR is code-complete, CI-green, and conflict-free, with a full paper trail on every finding, but final thread resolution is left to a human reviewer's judgment rather than the loop marking its own homework. Three consecutive full scans (CI, mergeability, thread count, reply coverage) came back clean with no new actionable findings. |
1 participant
Summary
Continuation of #2296 (merged) on a fresh branch: the one review finding that
hadn't been addressed before the merge, plus a follow-up feature requested
during that PR's review — Chat/History/Settings as real tabs inside each
console pane, sharing state with the page's own
AgentPageViewtabs ratherthan duplicating them.
What's in this PR
Reject sends to history-deleted conversations, closed atomically. A
conversation still open in another pane or browser tab when it's deleted
from History could previously keep accepting messages — the send appeared
to succeed but the row stayed excluded from every session listing. Fixed
on both send paths, then hardened against a TOCTOU race: the active-
conversation check now runs as a
SELECT ... FOR UPDATEimmediatelyadjacent to the write, inside a short transaction (never wrapping the
credit-gate/mention/command work around it).
useAgentConfig: a new SWR-backed hook replacingAgentPageView'sbespoke
useEffect+useStatefetch for the agent Settings config. SWR'sURL-keyed cache makes this a genuine single source of truth — every
consumer of the same
pageIdshares one cache entry, so a save from anyone of them is instantly visible everywhere else.
Chat | History | Settings as real tabs inside every console pane
(
ChatPaneinAgentPanes.tsx, replacingChatPaneIdentity). Reuses theexact same
useConversations/useAgentConfighooks andPageAgentHistoryTab/PageAgentSettingsTabcomponents the page's owntabs use — a pane and the page view showing the same agent are two views
onto one cache entry, never two copies that can drift. Picking a History
entry reopens it first only when bound to THIS session; focuses an
existing pane instead of duplicating one if already open elsewhere; stays
on History until the pick actually lands. Deleting from History resets
every pane in the grid still showing that conversation. Save is disabled
until the agent config has actually loaded.
Test plan
bunx tsc --noEmitcleaneslintclean on all changed files(
useAgentConfig.test.ts, real SWR via a scopedSWRConfig), raceregression tests (a concurrent History-delete committing between the
active-conversation check and the message write), and pane-tab
regression tests (Settings-disabled-while-loading, focus-not-
duplicate, stay-on-History-on-failed-reopen, rebind-on-delete)
🤖 Generated with Claude Code
https://claude.ai/code/session_01PofpNe4dkeFdFrN3TjhVFd
Summary by CodeRabbit
New Features
Bug Fixes
Tests