Repository navigation
fix(agents): dashboard sidebar lists every drive — roster-first session groups - #2279
Conversation
📝 WalkthroughWalkthroughThe sidebar now derives global-mode session groups from the drive roster, includes empty drives, filters trashed drives where applicable, preserves sessions for missing drives, and applies deterministic ordering. Tests cover roster-driven rendering, permissions, session creation, and grouping behavior. ChangesRoster-based session grouping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentsSidebar
participant DriveStore
participant SessionList
participant buildSessionGroups
AgentsSidebar->>DriveStore: read drive roster
AgentsSidebar->>SessionList: pass drives and sessions
SessionList->>buildSessionGroups: build roster and orphan groups
buildSessionGroups-->>SessionList: return ordered groups
SessionList-->>AgentsSidebar: render sidebar groups
🚥 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 |
![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: 36c0071b00
ℹ️ 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".
| [agentsByDrive], | ||
| ); | ||
| if (driveId) return sessions.length > 0 || notice === null ? [{ driveId, driveName: null, sessions }] : []; | ||
| return buildSessionGroups(sessions, { assistant: canSpawn, drives: roster }); |
There was a problem hiding this comment.
When a non-admin opens /dashboard/agents with drives in the persisted store, canSpawn only suppresses the Assistant group; every roster drive still becomes a group, and the render below unconditionally adds a NewSessionRow. The administrator-refusal notice is therefore shown alongside “New session” controls that expand into empty choosers because usePageAgents is disabled. Gate the roster groups or their spawn rows with canSpawn so this remains a refusal-only surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e8f419d. The roster is now gated on canSpawn (isAdmin && !authLoading) — empty for a non-admin, so no roster drive groups or spawn rows render, only the refusal notice. Added a regression test: shows no roster groups for a non-admin — refusal-only, not a dead spawn chooser in AgentsSidebar.test.tsx.
| // `agentsByDrive`: that fetch's drive enumeration is an implementation | ||
| // detail of the multi-drive agents feature, and coupling the sidebar's | ||
| // drive list to it would regress silently if that feature's shape changes. | ||
| const roster = useMemo(() => drives.map((d) => ({ driveId: d.id, driveName: d.name })), [drives]); |
There was a problem hiding this comment.
When useGlobalDriveSocket has populated useDriveStore via fetchDrives(true, true), the store intentionally contains trashed drives. Mapping every entry here makes /dashboard/agents render those deleted drives with “New session” rows, unlike DriveSwitcher and the multi-drive agents API, which both exclude trashed drives; the resulting chooser has no agents and is a dead entry. Build this roster from active drives only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e8f419d. The roster now filters !d.isTrashed, matching DriveSwitcher and the multi-drive agents API's active-drives-only behavior — useGlobalDriveSocket does indeed refetch with includeTrash: true on drive events, so useDriveStore.drives can hold trashed entries. Added a regression test: excludes trashed drives from the roster, matching DriveSwitcher and the multi-drive agents API.
| [agentsByDrive], | ||
| ); | ||
| if (driveId) return sessions.length > 0 || notice === null ? [{ driveId, driveName: null, sessions }] : []; | ||
| return buildSessionGroups(sessions, { assistant: canSpawn, drives: roster }); |
There was a problem hiding this comment.
When an admin enters the global console with a cached drive roster while /api/agent-sessions is still pending, this immediately renders each drive's spawn row under the loading notice. If the user opens a chooser and the response then adds a session for that drive, React inserts SessionRow elements before the unkeyed NewSessionRow, remounting it and closing the chooser mid-interaction—the drive-scoped empty-state branch explicitly avoids this by checking !isLoading. Apply the same cold-load guard to global roster groups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated but did not reproduce, so no code change here. I wrote an isolated React Testing Library repro of exactly this shape — a .map() of keyed elements followed by a static unkeyed sibling inside the same parent — and grew the mapped array from 0 → 1 → 2 items across rerenders while tracking the sibling's mount count via useState(() => ++mountCount). The sibling's mount count stayed at 1 throughout; it was never remounted. This matches React's reconciliation model: each {expr} in JSX is its own positional slot in the parent's children, and a nested array occupies exactly one of those slots (effectively wrapped as its own keyed list) — growing that array does not shift the position of a following sibling slot. So NewSessionRow after group.sessions.map(...) is not at risk of the remount-losing-open-chooser failure mode described, either during the roster's cold-load window or from a later session arriving via poll.
If you have a concrete repro where this does remount (e.g. a different React version behavior, or something about ScrollArea/Radix primitives intercepting children), I'd like to see it — happy to revisit. For now, applying the suggested !isLoading guard would reintroduce the exact "flash of missing content" the roster-first redesign was meant to fix (roster drives would disappear during every session-list load/poll), so I'm not adding it without evidence the bug is real.
…on groups Reshape session-groups.ts into buildSessionGroups: groups are the union of the drive roster (useDriveStore.drives) and any drive a returned session names, not just drives with a live session. Fixes /dashboard/agents showing only the Assistant group when the admin has zero active drive sessions, and the chicken-and-egg where a drive's first session couldn't be spawned from the dashboard console because the spawn row only existed inside a group. Ordering: Assistant first (requested, or an assistant session already exists), roster drives name-ascending (ties on driveId), orphan session-drives last. AgentsSidebar reads the roster from useDriveStore, not agentsByDrive — that fetch's drive enumeration is a different feature's implementation detail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvHJimQTUKGE5Em4co6iKV
… drives Two real gaps a Codex review caught on PR #2279: - A non-admin with drives in the persisted useDriveStore saw every roster drive's header and "+ New session" row alongside the admin-refusal notice — only the Assistant group's visibility was gated on canSpawn. The roster itself is now empty unless canSpawn. - useDriveStore can legitimately hold trashed drives (useGlobalDriveSocket refetches with includeTrash: true on drive events), so the roster now filters isTrashed out, matching DriveSwitcher and the multi-drive agents API — both active-drives-only. A third finding (claiming the mapped SessionRow list could remount the trailing NewSessionRow as sessions arrive) does not reproduce: verified with an isolated React Testing Library repro that a static sibling after a growing keyed .map() list is not remounted. No change made for that one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvHJimQTUKGE5Em4co6iKV
36c0071 to
e8f419d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/layout/left-sidebar/AgentsSidebar.tsx (1)
257-265: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOrphan groups (e.g. trashed drives with lingering sessions) still get a "+ New session" affordance.
NewSessionRowrenders unconditionally for every group, including orphan groups. Since the roster now deliberately excludes trashed drives (per the comment above at lines 230-234), a trashed drive with an existing session becomes an orphan group — and that orphan group still exposes "+ New session", letting an admin spawn a new sandbox session into a drive that was explicitly excluded from the roster for being trashed. This render logic predates this PR (unchanged here), but this PR is what introduces trash-awareness, so it's worth closing the gap now rather than leaving a latent inconsistency between "roster drives" (trash-protected) and "orphan drives" (not).Consider passing the full (unfiltered)
driveslist alongside the roster so orphan groups whosedriveIdmaps to a trashed drive can suppressNewSessionRow, while still preserving/surfacing the existing session.♻️ Sketch of a possible guard
+ const trashedDriveIds = useMemo( + () => new Set(drives.filter((d) => d.isTrashed).map((d) => d.id)), + [drives], + ); + {groups.map((group) => ( <div key={group.driveId}> ... - <NewSessionRow - driveId={group.driveId === ASSISTANT_GROUP_KEY ? null : group.driveId} - agentsByDrive={agentsByDrive} - onSpawned={onChanged} - /> + {!trashedDriveIds.has(group.driveId) && ( + <NewSessionRow + driveId={group.driveId === ASSISTANT_GROUP_KEY ? null : group.driveId} + agentsByDrive={agentsByDrive} + onSpawned={onChanged} + /> + )}Since backend validation of
driveIdfor trashed/inaccessible drives isn't visible in this file set, it would help to confirm whether/api/agent-sessionsalready rejects such spawns server-side.🤖 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/layout/left-sidebar/AgentsSidebar.tsx` around lines 257 - 265, Update the group rendering around AgentsSidebar’s NewSessionRow to distinguish roster-backed drives from orphan groups: use the full drives list to identify when group.driveId maps to a trashed drive, suppress NewSessionRow for those groups, and continue rendering existing SessionRow entries. Preserve the current behavior for the assistant group and valid non-trashed drives, and verify that the agent-session spawn endpoint also rejects trashed or inaccessible drive IDs server-side.
🤖 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.
Nitpick comments:
In `@apps/web/src/components/layout/left-sidebar/AgentsSidebar.tsx`:
- Around line 257-265: Update the group rendering around AgentsSidebar’s
NewSessionRow to distinguish roster-backed drives from orphan groups: use the
full drives list to identify when group.driveId maps to a trashed drive,
suppress NewSessionRow for those groups, and continue rendering existing
SessionRow entries. Preserve the current behavior for the assistant group and
valid non-trashed drives, and verify that the agent-session spawn endpoint also
rejects trashed or inaccessible drive IDs server-side.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7b82603-6506-462a-8c3e-9f6ca0aadf64
📒 Files selected for processing (4)
apps/web/src/components/layout/left-sidebar/AgentsSidebar.tsxapps/web/src/components/layout/left-sidebar/__tests__/AgentsSidebar.test.tsxapps/web/src/components/layout/left-sidebar/__tests__/session-groups.test.tsapps/web/src/components/layout/left-sidebar/session-groups.ts
CodeRabbit caught the gap left by the trashed-drive roster filter (previous commit): a trashed drive with a lingering session surfaces as an orphan group (session-groups.ts never drops a session), and that orphan group still rendered "+ New session" — letting an admin spawn a brand-new sandbox session into a drive explicitly excluded from the roster for being trashed. The existing session stays visible (no data loss); only the spawn affordance is suppressed for orphan groups whose driveId is trashed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvHJimQTUKGE5Em4co6iKV
|
Addressed CodeRabbit's nitpick in e615118: orphan groups (a session in a drive the roster excludes — e.g. a trashed drive) now suppress the "+ New session" row while still surfacing the existing session. Added a regression test ( Re: whether |
Summary
/dashboard/agentsshowed only the Assistant group when the admin had no active drive sessions, because the sidebar grouped sessions by drive and never rendered a drive with zero sessions.session-groups.tsis reshaped intobuildSessionGroups({ assistant, drives, sessions }): groups are the union of the drive roster (useDriveStore.drives— populated byDriveSwitcher'sfetchDrives()) and any drive a returned session names. A drive with zero live sessions still gets a group, with its own "+ New session" spawn affordance — fixing the chicken-and-egg where a drive's first session couldn't be started from the dashboard console.driveId), orphan session-drives last.AgentsSidebar.tsxnow reads the roster fromuseDriveStore, notagentsByDrive— that fetch's drive enumeration is an implementation detail of the multi-drive agents feature and shouldn't be coupled to for the sidebar's drive list.agentsByDrivekeeps its real job: the spawn chooser's per-drive agent list.canSpawn(isAdmin && !authLoading) — a non-admin sees only the refusal notice, never roster drive groups (caught by automated review, fixed).!d.isTrashed) —useGlobalDriveSocketcan populateuseDriveStorewith trashed drives viafetchDrives(true, true); the roster now matchesDriveSwitcherand the multi-drive agents API's active-drives-only behavior (caught by automated review, fixed).GET /api/agent-sessionsalready returns sessions across all drives.This is a follow-up from the six-slice adversarial audit of the "Dev → Agents Rebuild" epic (no tracked GitHub issue number for this specific item).
Test plan
session-groups.test.tsrewritten forbuildSessionGroups: roster drives render as groups even with zero sessions; Assistant-first then name-ascending ordering; roster ∪ session union merges without duplicating a group; an orphan session-drive not in the roster still surfaces, ordered last; Assistant omitted when not requested and no assistant session exists (but kept if one already exists); name ties break ondriveId.AgentsSidebar.test.tsxglobal-mode describe: seedsuseDriveStorewith a roster fixture; asserts a roster drive with zero sessions renders its header + "+ New session", and spawning from it POSTs{ driveId, agentPageId }; verifies every roster drive gets its own spawn affordance; a non-admin sees no roster groups; a trashed drive is excluded from the roster; a trashed drive's lingering session stays visible without its own spawn row.bun run typecheck && bun run lint && bun run test:unit && bun run knip:check— all pass. One pre-existing failure (grouping.test.tsmidnight-boundary test) is a known TZ-sensitive test in a file this branch never touches; unrelated to this change./dashboard/agentsshows Assistant first, then every drive by name, each with "+ New session"; picking an agent spawns into that drive; a zero-agent drive shows "No agents in this drive yet";/dashboard/{driveId}/agentsbehavior unchanged.Review notes
Two automated reviewers weighed in; all findings replied to inline or via comment:
Codex raised 3 findings:
canSpawn.useGlobalDriveSocket'sincludeTrash: truerefetch. Roster now filtersisTrashed..map()ofSessionRows remounts the trailingNewSessionRowsibling does not hold up; verified with an isolated React Testing Library repro (a static sibling after a growing keyed list is not remounted, consistent with React's per-slot reconciliation). No change made; explained with the repro in the thread reply.CodeRabbit raised 1 nitpick, building on fix #2 above:
4. Fixed — a trashed drive's lingering session (an orphan group, since the drive is excluded from the roster) still exposed "+ New session", letting an admin spawn a brand-new session into a trashed drive. The row is now suppressed for such orphan groups while the existing session stays visible.
https://claude.ai/code/session_01KvHJimQTUKGE5Em4co6iKV