Repository navigation
fix(agents): show global-assistant sessions in every drive's sidebar - #2325
Conversation
Clicking a past conversation from a drive's Agents page opened the pane correctly but never populated that drive's left sidebar, even after a refresh — because the conversation lived in a global-assistant session (agent_sessions.driveId IS NULL), and the sidebar's list() query filtered drive-scoped requests with a plain `driveId = X` equality. NULL never equals X in Postgres, so global sessions were unconditionally excluded. Global-assistant sessions belong to the user, not to any drive, so every drive's sidebar now shows them unconditionally in their own "Global Assistant" section (mirroring the dashboard sidebar's existing Assistant group) rather than trying to infer drive membership from a global session's conversation activity, which would create cross-drive ambiguity about what "belongs" to a drive. - agent-sessions-store.ts: list()'s driveId filter now ORs in driveId IS NULL, still gated by the ownerId condition so it can't leak another user's global sessions. - AgentsSidebar.tsx: drive-mode splits into the drive's own session group (unchanged) plus a distinct Global Assistant group. - Adds integration test coverage for inclusion + owner scoping.
|
Warning Review limit reached
Next review available in: 16 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 (6)
📝 WalkthroughWalkthroughDrive-scoped session listings now include the current owner’s global sessions. The sidebar displays these sessions in a separate “Global Assistant” group and supports creating new global sessions with ChangesDrive-scoped global assistant sessions
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AgentsSidebar
participant listAgentSessions
participant AgentSessionStore
participant Database
AgentsSidebar->>listAgentSessions: Request sessions for the current drive
listAgentSessions->>AgentSessionStore: Apply drive and owner filters
AgentSessionStore->>Database: Query drive sessions and matching owner global sessions
Database-->>AgentSessionStore: Return filtered sessions
AgentSessionStore-->>listAgentSessions: Return session list
listAgentSessions-->>AgentsSidebar: Render drive and Global Assistant groups
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 |
![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: eaee6e6f89
ℹ️ 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".
| // ASSISTANT_GROUP_KEY). The ownerId condition above already scopes | ||
| // this to the caller when supplied, so this can't surface another | ||
| // user's global sessions. | ||
| conditions.push(or(eq(agentSessions.driveId, filter.driveId), isNull(agentSessions.driveId))!); |
There was a problem hiding this comment.
When a caller uses the supported ownerless { driveId } filter, this OR returns every active global session because the preceding owner condition is absent. AgentSessionListFilter explicitly permits this shape, and the existing listAgentSessions tests exercise it as a drive-wide listing, so a caller authorized to enumerate one drive can now receive unrelated users' global sessions. Only include driveId IS NULL when ownerId is present, or introduce a separate owner-required filter for this sidebar behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3ffc6b3. Only OR in driveId IS NULL when the filter also names an ownerId; an ownerless { driveId } listing (the admin-facing "every session in this drive, any owner" shape) now keeps the original strict equality, so it can't return other users' global sessions. Added a unit test for both the owned and ownerless shapes in agent-sessions.test.ts, updated the in-memory fake to mirror the same owner-gated union, and added an integration-test regression case against real Postgres for this exact scenario. Leaving this thread open for verification.
… P1)
Codex correctly flagged that the previous fix unioned in driveId-IS-NULL
sessions unconditionally: AgentSessionListFilter allows an ownerless
{ driveId } shape (the admin "every session in this drive, any owner"
listing agent-sessions.test.ts's listAgentSessions already exercises),
and without an owner to scope the union, it would return every user's
global-assistant sessions across the whole deployment, not just ones
relevant to the drive.
- agent-sessions-store.ts: only OR in driveId IS NULL when the filter
also names an ownerId; an ownerless driveId filter keeps the old
strict equality.
- fakes.ts: the in-memory list() now mirrors that same owner-gated
union so unit tests against the fake match production semantics.
- Adds unit coverage (owned vs ownerless driveId filter) and an
integration-test regression case against real Postgres for the
exact leak Codex described.
…group An adversarial review pass surfaced three real problems with the previous commit's client change: 1. It broke two pre-existing tests (AgentsSidebar.test.tsx: the new-session palette's "Global Assistant" agent pick, and the null-agentPageId conversation-label fallback) via a text collision — the new group header rendered unconditionally whenever canSpawn was true (mirroring global mode's always-show Assistant group), even with zero global sessions to show. 2. That same unconditional render sits right next to the drive palette's own, unrelated "Global Assistant" agent-picker entry (spawns a DRIVE-scoped session with the assistant as its first agent) — same label, different scope, easy to conflate. 3. Group order contradicted session-groups.ts's documented "Assistant first" rule (drive mode appended it last). Fix: the drive-mode Assistant group now renders ONLY when the caller actually has a global session to show (no canSpawn escape hatch), and is ordered first, matching the dashboard's convention. Adds direct coverage in AgentsSidebar.test.tsx for the null-driveId session's separate section, the escape-hatch removal (regression pin for the collision), and spawning from the group posting driveId: null.
…d filter type A /simplify pass (reuse, simplification, efficiency, altitude angles, run in parallel) on the last two commits' diff converged on: - AgentsSidebar.tsx's drive-mode grouping hand-rolled exactly what session-groups.ts's buildSessionGroups already does (bucket by drive, Assistant group only when non-empty, Assistant first). Now calls it with a single-drive roster instead of duplicating the bucketing/ordering logic inline — also removes a double .filter() pass over the session list and an unused DriveGroup import. - The header-suppression check's `!driveId ||` disjunct was dead: `group.driveId` is always a real string (never undefined), so it can never equal an undefined `driveId` regardless — `group.driveId !== driveId` alone already covers both modes. - AgentSessionListFilter's drive-scoped shape had `ownerId` as merely optional, making "owned" vs "ownerless" (a real behavioral fork in list() — exactly what Codex's P1 was about) a runtime presence-check standing in for what should be a type-level distinction, leaving room for a future caller to pass `ownerId: possiblyUndefined` and land in either behavior. Split into three explicit variants; list() and the in-memory fake now narrow with `'ownerId' in filter` instead of `!== undefined` to match. No behavior change intended — full lib unit suite (8700 tests) and the web sidebar/session-groups suites still pass, typecheck clean.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/layout/left-sidebar/__tests__/AgentsSidebar.test.tsx`:
- Around line 1309-1317: Update the test around the nameInput interaction to
type a non-empty session name before submitting with Enter, then change the
mockPost expectation to assert that exact name in the request body instead of an
empty string.
In `@packages/lib/src/services/agent-sessions/agent-sessions-store.ts`:
- Around line 68-81: Normalize an undefined ownerId to the ownerless
drive-scoped path before selecting branches in agent-sessions-store.ts (lines
68-81 and 415-425), so only a defined ownerId adds global sessions and applies
the owner filter. Update the related fake in
packages/lib/src/services/agent-sessions/__tests__/fakes.ts (lines 130-143) as
needed and add a regression test in
packages/lib/src/services/agent-sessions/__tests__/agent-sessions.test.ts (lines
458-480) for { driveId, ownerId: undefined }, verifying it behaves as an
ownerless drive listing.
🪄 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: c6b1bb0d-bac0-4b6b-9e32-20ff7df05d10
📒 Files selected for processing (6)
apps/web/src/components/layout/left-sidebar/AgentsSidebar.tsxapps/web/src/components/layout/left-sidebar/__tests__/AgentsSidebar.test.tsxpackages/lib/src/services/agent-sessions/__tests__/agent-sessions-store.integration.test.tspackages/lib/src/services/agent-sessions/__tests__/agent-sessions.test.tspackages/lib/src/services/agent-sessions/__tests__/fakes.tspackages/lib/src/services/agent-sessions/agent-sessions-store.ts
…odeRabbit) CodeRabbit found a gap in the P1 fix from 3ffc6b3: 'ownerId' in filter is true even for { driveId, ownerId: undefined } (the key exists, it's just unset), so a caller constructing the filter that way would both take the owner-scoped global-session union AND push a never-matching eq(ownerId, undefined) condition — silently returning zero rows rather than the intended ownerless drive-wide listing, for a shape the stricter three-variant AgentSessionListFilter type no longer names but doesn't fully guard against at the value level. - agent-sessions-store.ts's list() and the in-memory fake now normalize to `'ownerId' in filter ? filter.ownerId : undefined` and branch on that VALUE (not just the key's presence), so an explicitly-undefined ownerId is treated identically to an absent one. - Added regression coverage at both the unit (fake store) and integration (real Postgres) layers for { driveId, ownerId: undefined } — reached via a deliberate cast, since the type itself no longer permits constructing that shape directly, documented as pinning the runtime guard rather than describing a shape callers should use. - Also addressed a minor test-quality nit CodeRabbit raised on the new "spawning from the Assistant group" test: it typed Enter on an empty name and only asserted name: '' — now types a real name and asserts that exact value, strictly widening what the test proves (the empty-name path stays covered by the pre-existing dashboard-mode test that exercises it on purpose).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3446607a10
ℹ️ 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".
| // encodes "no canSpawn escape hatch" above (`hasAssistantSessions` | ||
| // still shows the group once one exists). | ||
| const driveName = drives.find((d) => d.id === driveId)?.name ?? driveId; | ||
| return buildSessionGroups(filteredSessions, { assistant: false, drives: [{ driveId, driveName }] }); |
There was a problem hiding this comment.
When a global session contains a conversation with an agent from another drive, this newly rendered group cannot label that agent correctly in drive mode. AgentsSidebar still calls usePageAgents(driveId), whose agentsByDrive result is filtered to the current drive, while global sessions explicitly support agents from every accessible drive; consequently conversationLabel falls back to the generic Agent for every out-of-drive agent. Use an unfiltered agent-name lookup for global-session rows while retaining the drive-filtered list for the drive spawn palette.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9992f9a — usePageAgents now called unfiltered (undefined instead of driveId) in both modes, so agentNamesById covers every accessible agent, not just this drive's. The spawn palette's drive-scoping is unaffected since useSpawnSession already does its own per-drive .find() lookup on the full list. Added a regression test with a driveId-aware mock (closer to the real hook's filtering than the file's existing fixture) and verified it by temporarily reverting the fix and confirming the test fails as expected. Leaving this thread open for verification.
…ve (review: Codex P2) Codex's re-review caught a real gap: AgentsSidebar still called usePageAgents(driveId), whose agentsByDrive result is filtered to the current drive. A global-assistant session can legitimately host a conversation with an agent from ANY accessible drive (that's the whole premise of surfacing them in every drive's sidebar), so once that session's conversations render here, an out-of-drive agent's id was never in agentNamesById and conversationLabel silently fell back to the generic "Agent" instead of the real name. Fix: call usePageAgents(undefined, ...) — unfiltered — in both modes now, not just global mode. The spawn palette's drive-scoping is unaffected: useSpawnSession already does its own `agentsByDrive.find(entry => entry.driveId === spawnTarget.driveId)` lookup, so handing it the full list changes nothing about which agents a drive's "+" offers. Added a regression test with a driveId-aware mock (a closer simulation of the real hook's filtering than the file's existing driveId-agnostic fixture) asserting a global session's out-of-drive agent renders its real name. Verified end-to-end by temporarily reverting the fix and confirming the new test fails exactly as expected before restoring it.
Summary
agent_sessions.driveId IS NULL), and the sidebar'slist()query filtered with a plaindriveId = Xequality, which never matchesNULLin Postgres.agent-sessions-store.ts:list()'sdriveIdfilter now ORs indriveId IS NULL, still gated by theownerIdcondition so it can't leak another user's global sessions across drives.AgentsSidebar.tsx: drive-mode now splits into the drive's own session group (unchanged, headerless) plus a distinct Global Assistant group, reusing the existingASSISTANT_GROUP_KEY/spawn affordance.Test plan
bun run typecheckpasses for@pagespace/libandwebbun run --filter '@pagespace/lib' test:db -- src/services/agent-sessions/__tests__/agent-sessions-store.integration.test.tsagainst a real Postgres instance (couldn't run in this worktree — no local DB role configured)🤖 Generated with Claude Code
https://claude.ai/code/session_015WjrwseJCNQTQPNrQ6nZ2S
Summary by CodeRabbit
New Features
Bug Fixes
Tests