Repository navigation
fix(agents): keep session/conversation selection alive across sidebar nav - #2319
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConsole-created conversations now update global agent selection. The sidebar preserves matching session, conversation, and agent parameters for the current drive while using drive-aware active-path matching. ChangesAgent selection navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
6cf45f3 to
0647092
Compare
![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: 6cf45f33d5
ℹ️ 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".
| // longer shows it — reverting the swap or, once the eviction guard | ||
| // treats this pane as protected, splitting a second pane open for it | ||
| // instead of recognizing the swap as already done (caught in review). | ||
| useAgentSurfaceStore.getState().selectConversation({ sessionId, conversationId, agentId: agentPageId }); |
There was a problem hiding this comment.
When AgentPanes is embedded in an AI_CHAT page through AgentPageView, clicking “Start a new conversation” also reaches this call. selectConversation always constructs an Agents-console URL and invokes history.pushState, which Next folds into router state, so successfully creating a conversation unexpectedly navigates the user away from the page to /dashboard/agents (or the store's possibly stale drive-scoped Agents route). The console selection should be updated only when this grid is hosted by AgentsSurface, rather than from this shared component unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3dfed54 — good catch. AgentPanes is indeed also embedded via AgentPageView with chatContext="page", whose initialConversation comes from its own local current state, not useAgentSurfaceStore. The unconditional call would have both done nothing useful there and silently pushed a /dashboard/agents URL, navigating a page-embedded chat's user away.
Gated the selectConversation(...) call on chatContext === 'console' (the one context — AgentsSurface's default — where this store is actually this component's source of truth for what should be showing). Left this thread open for verification rather than resolving it myself.
… nav Two bugs on the Agents console, both surfacing as "the thing I just created isn't where I left it after navigating away and back via the sidebar": - The left sidebar's "Agents" link was a static href with no query string, while the surface's whole selection lives in the URL query (useAgentSurfaceStore). Navigating away and clicking "Agents" again wiped the selection even though the in-memory store still had it. Fixed by deriving the link's href from the live store selection, scoped to the correct drive. - The in-pane "+" (start a new conversation in the current session) swapped the pane's content but never told useAgentSurfaceStore about it, unlike useSpawnSession which does the equivalent update when spawning a new session. On a later remount, the seeding effect tried to reopen the stale old conversation against a pane that no longer showed it, tripping the eviction guard's split fallback instead of recognizing the swap as already done — producing a phantom second pane. Fixed by calling selectConversation right after the swap. Verified live: stood up a local instance (isolated Postgres, seeded admin session, production build) and drove it with Playwright to reproduce both bugs and confirm the fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxRVM6D5xXQz6urBaeSdjs
Self-review cleanup: every other useAgentWorkspaceStore action in AgentPanes.tsx is obtained via the reactive useAgentWorkspaceStore((s) => s.xxx) selector hook, not an imperative .getState() call. Match that convention for useAgentSurfaceStore's selectConversation too — functionally identical (a single-action selector is a stable reference either way) but consistent with the file's existing style. Also normalize PrimaryNavigation.tsx's agentsBasePathForDrive to derive from the already-computed agentsTargetDriveId rather than the raw driveId prop directly — same behavior (agentsBasePath treats undefined/null identically), just one fewer way to spell "this nav's drive scope." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxRVM6D5xXQz6urBaeSdjs
Review finding (chatgpt-codex-connector, P1): AgentPanes is also embedded in a regular page's chat tab via AgentPageView (chatContext="page"), whose initialConversation is driven by its own local `current` state, not useAgentSurfaceStore. The unconditional selectConversation() call added in 0647092 would, for that embedding, both do nothing useful (its seeding effect never reads this store) and push a /dashboard/agents URL via history.pushState — silently navigating a page-embedded chat's user away to the Agents console the instant they started a new conversation there. Gate the call on chatContext === 'console' (AgentsSurface's default, matching the one context where useAgentSurfaceStore's selection is actually this component's source of truth for what should be showing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxRVM6D5xXQz6urBaeSdjs
Proactive self-review pass (/simplify: reuse, simplification, efficiency,
altitude angles run in parallel):
- AgentPanes.tsx: extracted the chatContext-gated selectConversation call
into a single syncConsoleSelection(conversationId, agentPageId) helper
instead of inlining the same guard at every call site that reassigns a
pane's targetId to a different conversation. Applied it to the one
additional call site with the identical bug: handleSwitchAgent's
focus-existing branch already treats the pane's outgoing conversation as
"replaced" (it calls closeReplacedConversation, same as the mint path)
but was never telling the console's selection about the incoming one —
same staleness risk (revert-or-split on next remount), reachable simply
by switching an agent in an existing pane, not just minting a new
conversation.
Deliberately NOT extended to handleClosePane's rebind-pane branch: that
branch only ever repoints a pane that held NO conversation of its own
(terminal/picker/still-minting), so selectedConversationId was never
referring to what's being displaced there. Force-syncing it would risk a
DIFFERENT bug — silently reassigning the console's "selected conversation"
to whatever gets rebound into an unrelated empty pane, based on a close
the user made elsewhere in the grid.
- PrimaryNavigation.tsx: dropped the separate `matchPath` field (and the
`agentsBasePathForDrive` value it was hand-computed from) in favor of
deriving the active-match path from `item.href.split("?")[0]` at render
time — the Agents entry's href is the only one that ever carries a query
string, so this removes a whole "did these two stay in sync" axis rather
than just documenting it. Also replaced the three-way
`agentsMatchesDrive ? ... : null` field-by-field ternary with a single
selection swap against the already-exported `EMPTY_AGENT_SELECTION`
constant (lib/agents/agent-selection.ts), and split the drive-id
comparison into its own primitive selector so the remaining shallow-compared
object exactly matches `AgentSelection`'s shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PxRVM6D5xXQz6urBaeSdjs
0abe91e to
c46e718
Compare
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
useAgentSurfaceStore). Navigating away and back via that link reset the selection to nothing, even though the in-memory store still had it — fixed by deriving the link's href from the live store selection, scoped to the correct drive.useAgentSurfaceStoreabout it, unlikeuseSpawnSession's equivalent update when spawning a new session. On a later remount, the seeding effect tried to reopen the stale old conversation against a pane that no longer showed it, tripping the eviction guard's split fallback instead of recognizing the swap as done — producing a phantom second pane. Fixed by callingselectConversationright after the swap, scoped tochatContext === 'console'(review finding:AgentPanesis also embedded in a regular page's chat tab viaAgentPageView,chatContext="page", whose selection isn't sourced from this store — an unconditional call there would silently navigate that user to/dashboard/agents).handleSwitchAgent's "focus-existing" branch (switching an agent already open elsewhere into the current pane) — it has the identical shape (treats the pane's outgoing conversation as replaced, just like the mint path) and was equally vulnerable to the same staleness bug, just reachable via a different everyday action. Consolidated both call sites behind onesyncConsoleSelectionhelper instead of duplicating thechatContextguard.Test plan
bun run typecheck,eslint, andknip:checkpass on all changed filesAgentPanes's only other rendering context (AgentPageView, page-embedded chat) is unaffected by the console-selection sync (gated onchatContext)handleSwitchAgentcoverage extension above. Deliberately did not extend tohandleClosePane's rebind-pane branch — that branch only ever repoints a pane that held no conversation of its own, so there's nothing stale to sync there, and forcing it would risk hijacking the console's selection based on an unrelated pane close elsewhere in the grid.🤖 Generated with Claude Code
https://claude.ai/code/session_01PxRVM6D5xXQz6urBaeSdjs