Agents means "my conversations": land on the list, and give a session a way out - #2383
Conversation
…ssion has a way out
`AgentsSurface` renders the conversation list ONLY while nothing is selected,
and the three things that clear a selection all require the session to be
destroyed first (`onSessionEnded`, the sidebar's End Session, and the GC that
fires when the server says the workspace is gone). The Agents nav item then
carried the live selection forward, so it reopened the last session instead of
leaving it. Together that made the surface a room with a door in and no door
out: a user parked in a session could not reach their own history without
ending the work they were in the middle of.
Two changes, one root cause.
**The nav item stops carrying a selection.** `agentsHref` is now built from
`EMPTY_AGENT_SELECTION` unconditionally. This changes what the NAV ITEM means,
not the URL-is-the-state design it was written to serve: `hydrateFromSearch` is
untouched, and deep links, refresh, `popstate` and the sidebar's own session
rows still carry and restore a full selection through the same grammar. The
link is a real route-level navigation to a URL that differs from the current
one only in its query string, which Next's router treats as a navigation —
`useSearchParams()` updates, the hydrate effect re-runs against an empty
search, and the selection clears. Verified in a browser, not inferred.
With no "?" left in any href, `isActive` compares `pathname` to `item.href`
directly; the `split("?")` that existed for this one entry is gone, and Agents
stays highlighted across the whole surface via its existing `exact: false`.
**A session gets a persistent way out.** "All conversations" above the grid
calls `selectSession(null)` — the selection drops, the workspace does not.
Browsing history should never require destroying a live session, and the nav
fix alone would still have left the only in-session exit being a destructive
one. It lives in `AgentsSurface`, not `AgentPanes`, because it is the console's
control: the same grid also mounts inside `AgentPageView`, where there is no
conversation list to go back to. The grid moves into a `min-h-0 flex-1` track
so the header is subtracted by the flex layout rather than overflowing
`SessionPanes`' own `h-full`.
Tests: a new `PrimaryNavigation.test.tsx` pins the href carrying no selection
in drive and global mode, and the active highlight surviving it; two new
`AgentsSurface` tests pin the back control clearing the selection and the URL
while issuing no DELETE and leaving the workspace in the store, and its absence
when nothing is selected. All four mutation-checked. The first draft of the
highlight test passed against a deliberately broken `isActive` because
`toContain('bg-accent')` matched the inactive state's own `hover:bg-accent`; it
compares class tokens now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DMesaDDBUUcxXYS29uDUp7
📝 WalkthroughWalkthroughAgents navigation now opens the conversation list without restoring selections. An active session shows an “All conversations” control that clears only the session selection. Tests cover URL, workspace, network, and active-link behavior. ChangesAgents navigation and session flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant PrimaryNavigation
participant AgentsSurface
participant URL
User->>PrimaryNavigation: Open Agents
PrimaryNavigation->>URL: Build Agents URL without selections
URL-->>AgentsSurface: Load conversation list
User->>AgentsSurface: Select a session
AgentsSurface->>URL: Set session selection
User->>AgentsSurface: Choose “All conversations”
AgentsSurface->>URL: Clear session selection
URL-->>AgentsSurface: Show conversation list
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: b864d25674
ℹ️ 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".
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 gap-1 px-2 text-muted-foreground hover:text-foreground" | ||
| onClick={() => selectSession(null)} |
There was a problem hiding this comment.
When the selected session contains a terminal, this click clears selectedSessionId, so the outer conditional replaces the entire AgentPanes tree with the conversation list rather than merely hiding it. That unmounts XtermTerminal, whose teardown emits shell:disconnect; if this was the last viewer, the realtime service detaches the PTY and arms its 30-minute idle reap, while SessionPanes explicitly documents that such unmounts can lose final output/exit state and cause a cold-started PTY on return. The supposedly nondestructive escape therefore disrupts or eventually kills live terminal work; keep the session panes mounted (for example, in an invisible keep-alive container) while displaying the list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct on the mechanism, and I checked it rather than taking it on trust: XtermTerminal's cleanup does emit shell:disconnect (XtermTerminal.tsx:390), and SessionPanes really does use invisible rather than unmounting for exactly this reason.
Where I land differently is on whether this is a NEW cost.
AgentPanes is key={selectedSessionId}. Switching to a different session therefore already unmounts the previous session's terminals and pays precisely this price — disconnect, viewer removed, idle reap armed if last, cold PTY on return. That is today's accepted behaviour for every click between sessions in the sidebar. "Return to the list" costs what "switch to another session" costs; it does not open a new class of harm, it reuses an existing one.
The larger point against fixing it here: the requested behaviour is that clicking Agents in the nav lands on the conversation list, and that path unmounts the grid too — it is a query-string change on the same route, so AgentsSurface re-renders with a null selection and the panes come down the same way. Making the back button uniquely non-destructive would leave the two doors out of a session behaving differently, and the nav one is the one the owner actually asked for.
A keep-alive host would cover both, and it is feasible — but it is the MachineKeepAliveHost pattern this surface's own docblock says "has no successor here because it has no problem to solve", so re-introducing it is a design change with its own review, not a footnote on a nav-href fix. Recorded as a stated trade-off in the description rather than silently accepted; if terminals-survive-the-list is wanted, that is worth doing deliberately and would also make session switching non-destructive, which is the bigger win.
There was a problem hiding this comment.
Tracked as #2385 rather than left as a stated trade-off, and the issue records what I verified in the handler — because the summary above overstates the cost in two ways worth correcting for whoever picks it up.
Detaching removes a viewer, not the session. disconnectConnection → removeViewer (shell-handler.ts:1529-1537), the PTY keeps running, scrollback is buffered and persisted (:188-203), and reattach is a built-in fast path that hands back the scrollback and cancels the pending reap (:1552-1561). So navigation does not lose terminal data.
And the reap is quiet-based, not detachment-based: agent input into a viewer-less session re-arms it, with armIdleReap's docblock giving the reason — "an agent driving a headless shell is activity, and reaping mid-command thirty minutes after it started would kill work in progress" (:481-500).
The genuine exposure is narrower than "disrupts or eventually kills live terminal work": a human-started long-running command, with no agent driving it, left unattended past DETACHED_IDLE_MS (30 min, terminal-session-map.ts:5) — plus a missed shell:closed exit status if a command finishes with nobody attached.
Where you are right, and it is the part that makes this worth tracking: the frequency changes. AgentPanes is key={selectedSessionId}, so session switching already paid this exact price — but leaving a session used to be nearly impossible, and is now the default landing behaviour. Same mechanism, moved onto the happy path.
Keeping the thread open for your read on the severity correction.
Both branches were adding to the top of Unreleased/Fixed, which conflicts on whichever merges second. Same entries, anchored after the node-tree cluster instead, so the two hunks do not overlap.
1 participant
The problem, as reported
The product owner could not reach their conversation history at all, on mobile, in production.
It is not a data problem.
listAllConversationsPaginated(apps/web/src/lib/agent-workspaces/workspace-conversations-runtime.ts) alreadyleftJoins the membership node, so conversations with no node still list. The list was simply unreachable.AgentsSurfaceis a ternary:selectedSessionIdset →<AgentPanes>, nothing selected → the list. So the list renders only while nothing is selected — and the only three things that clear a selection all require the session to be destroyed first:AgentsSurfaceonSessionEndedAgentsSidebarEnd SessionAgentsSurface's GC effectAnd the nav item carried the live selection forward, so clicking Agents reopened the last session rather than leaving it.
Combined: a room with a door in and no door out. Browsing your own history meant ending the work you were in the middle of.
What changed
1. The Agents nav item stops carrying a selection.
agentsHrefis built fromEMPTY_AGENT_SELECTIONunconditionally.This changes what the nav item means, not the "the URL is the state" design the old behaviour was written to serve.
hydrateFromSearchis untouched. Deep links, refresh,popstate, the tab bar's own saved search, and the sidebar's session rows all still carry and restore a full selection through the same grammar — they just aren't spelled by this one link any more.Three things I checked rather than assumed:
history.pushState, which Next folds into its router state, so the router's canonical URL genuinely differs from the link's and<Link>performs a real navigation —useSearchParams()updates, the hydrate effect re-runs against an empty search, the selection clears. Confirmed in a browser (below), not reasoned about.?left in any href,isActivecomparespathnamedirectly; thesplit("?")that existed for this one entry is now dead and removed. Agents stays lit across the whole surface via its existingexact: false. Visible in the screenshot below.useTabSyncalready syncs onpathname/searchchange, so a Link navigation updates the tab bar's record without help.2. A session gets a persistent way out — an "All conversations" control above the grid.
The landing-page behaviour is the stated requirement, but it is the narrower fix for this root cause: without a back control, the only in-session exit is still a destructive one.
selectSession(null)drops the selection and nothing else — the workspace, its panes, its PTYs and any streaming reply are untouched, and the sidebar row reselects it.It lives in
AgentsSurface, notAgentPanes, because it is the console's control: the same grid also mounts insideAgentPageView, where there is no conversation list to go back to. The grid moves into amin-h-0 flex-1track so the header is subtracted by the flex layout rather than overflowingSessionPanes' ownh-full.Verification
Gates (monorepo-wide, from a bootstrapped worktree with
packages/dbandpackages/librebuilt):A note on that test run: the suite first showed 25 failures, all in
*.integration.test.tsand all42P01 relation does not exist— the sharedpagespace_testdatabase on 5433 has no tables. Migrating a fresh database and pointingDATABASE_URLat it turns the whole suite green, so those were an unmigrated local DB, not this branch. (Runningbun run db:migratefrom the repo root silently migrates the wrong database — the root.envwins over the passed variable. Run it frompackages/db.)Mutation-checked, each fix broken and the named test watched go red, then restored:
agentsHrefre-attachesselectedSessionIdisActiveforced tofalseonClick→ no-op"All conversations" drops the selection and shows the listOne test was wrong on the first pass and is worth naming: the highlight assertion
toContain('bg-accent')passed against a deliberately brokenisActive, because the inactive class string containshover:bg-accent. It compares class tokens now.Manual, in a real browser, at 390×844 — production build (
next start;bun run devcan't be used for this, the CSP has no dev exemption for Next'sunsafe-eval), real Postgres, seeded user and drive, driven with Playwright:/dashboard/{drive}/agents→ conversation list.?workspace=…&c=…, the pane grid renders, and the "All conversations" control is visible above it with the composer still correctly pinned to the bottom of the viewport (no overflow from the added header)./dashboard/{drive}/agents, the list renders, and the session is still listed in the sidebar (screenshot below shows both sessions alive).?workspace=…&c=…. The history entry is real./dashboard/{drive}/agents, list renders. This is the reported bug, fixed. The nav item is highlighted in that same screenshot with no selection in its href.Known trade-off: returning to the list unmounts the grid
Raised in review (P1). Accurate on the mechanism —
XtermTerminal's cleanup emitsshell:disconnect(XtermTerminal.tsx:390), which removes the viewer and, if it was the last one, arms the PTY's idle reap;SessionPanesusesinvisiblerather than unmounting for exactly this reason.It is not a new cost.
AgentPanesiskey={selectedSessionId}, so switching to a different session already unmounts the previous session's terminals and pays the same price on every sidebar click. And the behaviour actually being asked for — Agents lands on the conversation list — unmounts the grid too, since it is a query-string change on the same route. Making the back button uniquely non-destructive would leave the two doors out of a session behaving differently.A keep-alive host would cover both and is feasible, but it is the
MachineKeepAliveHostpattern this surface's docblock says "has no successor here because it has no problem to solve" — worth doing deliberately (it would also make session switching non-destructive, the bigger win) rather than as a footnote on a nav-href fix.Found, not fixed
?c=param can appear on the bare agents URL before any session is picked (/dashboard/{drive}/agents?c=<id>on first load) — something upstream of this surface seeds a global-assistant conversation id into the URL. Harmless here (noworkspace=, so the list still renders) and out of scope for this fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01DMesaDDBUUcxXYS29uDUp7
Summary by CodeRabbit
New Features
Bug Fixes