feat(agents): any page type as a pane in agent sessions - #2298
Conversation
📝 WalkthroughWalkthroughThe PR adds page panes to agent workspaces. It defines the ChangesPage pane flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AI as AI assistant
participant Tool as open_page_pane
participant Chat as SessionChatView
participant Store as Agent workspace store
participant Pane as PagePaneView
AI->>Tool: call with pageId and title
Tool-->>Chat: return pageId and title
Chat->>Store: openPage(sessionId, page scope)
Store->>Pane: assign or focus page pane
Pane->>Pane: fetch and render page
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: 6d306e622a
ℹ️ 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 isReplaceable = (pane: PaneState) => | ||
| pane.scope === null || (pane.scope.kind !== 'terminal' && pane.scope.targetId !== null); | ||
| const replaceable = active && isReplaceable(active) ? active : panes.find(isReplaceable); |
There was a problem hiding this comment.
When open_page_pane completes in the normal single-chat workspace, that chat is the active pane and satisfies this isReplaceable predicate, so openPage assigns the page over the conversation rather than opening it beside the conversation. Because a mounted invoking chat guarantees at least one replaceable non-terminal pane, the tool generally evicts a chat or another page and reaches the split branch only in impossible/all-terminal circumstances; the tool-triggered path needs to preserve its source chat and split unless an explicitly empty picker can be reused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — openPage now takes an optional excludeTargetId; useOpenPagePane passes the invoking conversation's own id so its own pane is never one of the replaceable candidates. Threaded conversationId through SessionChatView/SessionChat/AssistantSessionChat to make that possible. Added a store-level test ("never replaces the pane the caller excluded") and a hook-level test ("never evicts the invoking chat pane — splits beside it instead"). See 3d4eee4.
| if (toolPart.state !== 'output-available' || !toolPart.output?.opened) continue; | ||
| if (handledRef.current.has(toolPart.toolCallId)) continue; | ||
| handledRef.current.add(toolPart.toolCallId); |
There was a problem hiding this comment.
When a conversation is reopened or the chat component remounts and its last persisted assistant message contains a completed open_page_pane call, handledRef starts empty and this effect treats that historical result as newly streamed. The old call therefore mutates the restored grid again on every mount, potentially focusing or replacing the user's current panes; initialize the seen set from loaded history or react only to a transition into output-available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — good catch. useOpenPagePane now seeds a "first sighting" flag: whatever's already output-available on the very first render after mount is marked handled WITHOUT being acted on (that's history, not a new event); only a call that transitions to output-available on a later render is treated as new and opens/focuses the pane. Added a dedicated test ("does NOT act on a completed call already present on the FIRST render (history replay guard)") plus one confirming a genuinely new call on a later render still fires. See 3d4eee4.
| let pageComponent: React.ReactNode; | ||
| if (componentName === 'DocumentView') { | ||
| const DocumentView = componentMap.DocumentView; | ||
| pageComponent = <DocumentView key={`document-${page.id}`} pageId={page.id} driveId={driveId ?? undefined} />; |
There was a problem hiding this comment.
For a global session, the picker intentionally searches across drives while this driveId is null, so every document pane passes undefined here even though the fetched page includes its actual page.driveId. DocumentView consequently disables usePageContentSocket; a drive-bound session opening an accessible cross-drive page similarly joins the wrong drive room, causing collaborator or AI content updates to be missed until a manual refresh. Pass the fetched page's drive ID instead.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed — this was a real bug, not just theoretical: I was passing the SESSION's driveId (null for a global-assistant session) instead of the fetched PAGE's own page.driveId, which CenterPanel.tsx always uses for this exact prop. Switched to page.driveId and dropped the now-unused driveId prop from PagePaneView entirely (nothing else in the component needed it). Added a test asserting DocumentView receives the page's own driveId regardless of the session's. See 3d4eee4.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx (1)
60-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse a globally unique client-only identifier for editing-store keys.
These components use React
useId()values as suffixes for keys in the module-leveluseEditingStoreMap. Verify that all page views share one React root. If separate roots are possible, usecrypto.randomUUID()so cleanup from one view cannot remove another view’s editing session.
apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx#L60-L63: ReplaceuseId()with a per-mount client-only identifier, or verify the single-root invariant.apps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsx#L74-L77: ReplaceuseId()with a per-mount client-only identifier, or verify the single-root invariant.apps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsx#L39-L44: ReplaceuseId()with a per-mount client-only identifier, or verify the single-root invariant.apps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsx#L205-L210: ReplaceuseId()with a per-mount client-only identifier, or verify the single-root invariant.apps/web/src/components/layout/middle-content/page-views/task-list/TaskListView.tsx#L534-L539: ReplaceuseId()with a per-mount client-only identifier, or verify the single-root invariant.Suggested identity source
-const instanceId = useId(); +const [instanceId] = useState(() => crypto.randomUUID());Remove
useIdfrom the affected imports.Based on learnings, use
crypto.randomUUID()for ephemeral client-only identifiers and Zustand store keys. The supplieduseEditingStorecontract stores and removes the exact caller-provided key.🤖 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/middle-content/page-views/canvas/CanvasPageView.tsx` around lines 60 - 63, Replace the useId-based editing-store instance identifiers with crypto.randomUUID() in CanvasPageView.tsx (60-63), CodePageView.tsx (74-77), DocumentView.tsx (39-44), SheetView.tsx (205-210), and TaskListView.tsx (534-539); remove the affected useId imports and preserve each exact per-mount key for store creation and cleanup.Source: Learnings
apps/web/src/components/agents/panes/PagePaneView.tsx (1)
61-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
PagePaneView's render branches.
AgentPanes.tsx,PanePicker.tsx, andpane-surface.tseach have an adjacent test file, but no test evidence surfaced forPagePaneView.tsx. This component has several distinct branches worth covering: loading skeleton, fetch error, non-paneable type, unsupported component, and the pageId-only vs. full-page prop split (lines 100-112).Consider adding a test file mocking
useSWRand thenext/dynamicimports to cover these branches, following the pattern already used for the sibling test files in this directory.🤖 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/PagePaneView.tsx` around lines 61 - 115, Add a colocated test file for PagePaneView that mocks useSWR and next/dynamic/component dependencies, following neighboring pane component test patterns. Cover loading, fetch-error, non-paneable, unsupported-component, and successful rendering branches, including verifying pageId-only components receive pageId/driveId while other components receive the full page object.apps/web/src/components/agents/panes/PanePicker.tsx (1)
236-242: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDo not treat the empty-query fetch as a candidate-only issue.
/api/mentions/searchreturns the latest 20 pages whenqis empty, so/api/mentions/search?q=returns a non-empty page list. Use an inactive SWR key until the typed query has at least one non-whitespace character, or update the empty-state copy to describe the default recent-pages list.🤖 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/PanePicker.tsx` around lines 236 - 242, Update the SWR key used by the PanePicker search flow around searchKey and useSWR so it is inactive when debouncedQuery contains no non-whitespace characters, preventing the empty-query request from populating results. Preserve the existing driveId and cross-drive query behavior once a meaningful search query is entered.
🤖 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/PagePaneView.tsx`:
- Around line 61-115: Update PagePaneView to import ReactNode and ComponentType
from 'react', then use those imported types for pageComponent and the Component
annotation instead of React.ReactNode and React.ComponentType.
In `@apps/web/src/components/agents/panes/PanePicker.tsx`:
- Around line 239-262: Update the SWR state destructuring in the PanePicker
search flow to use isValidating instead of isLoading for the searching
indicator. Rename the state alias to searching if needed, and preserve the
existing query !== debouncedQuery condition and rendered “Searching…” behavior.
In `@apps/web/src/stores/agent-workspace/useAgentWorkspaceStore.ts`:
- Around line 62-68: The openPage flow should guard replacement of a dirty page
pane before applying focusOrAssignScope or assignPane. Reuse the existing
unsaved-edits confirmation/guard used by conversation-pane replacement, and
ensure handlePickPage and agent-driven open_page_pane resolution preserve the
current pane when confirmation is denied while allowing replacement after
confirmation.
---
Nitpick comments:
In `@apps/web/src/components/agents/panes/PagePaneView.tsx`:
- Around line 61-115: Add a colocated test file for PagePaneView that mocks
useSWR and next/dynamic/component dependencies, following neighboring pane
component test patterns. Cover loading, fetch-error, non-paneable,
unsupported-component, and successful rendering branches, including verifying
pageId-only components receive pageId/driveId while other components receive the
full page object.
In `@apps/web/src/components/agents/panes/PanePicker.tsx`:
- Around line 236-242: Update the SWR key used by the PanePicker search flow
around searchKey and useSWR so it is inactive when debouncedQuery contains no
non-whitespace characters, preventing the empty-query request from populating
results. Preserve the existing driveId and cross-drive query behavior once a
meaningful search query is entered.
In
`@apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx`:
- Around line 60-63: Replace the useId-based editing-store instance identifiers
with crypto.randomUUID() in CanvasPageView.tsx (60-63), CodePageView.tsx
(74-77), DocumentView.tsx (39-44), SheetView.tsx (205-210), and TaskListView.tsx
(534-539); remove the affected useId imports and preserve each exact per-mount
key for store creation and cleanup.
🪄 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: b27c1756-fc47-406b-91ee-e3488bc51623
📒 Files selected for processing (24)
README.mdapps/marketing/src/app/docs/getting-started/page.tsxapps/web/src/components/agents/AgentPageView.tsxapps/web/src/components/agents/chat/AssistantSessionChat.tsxapps/web/src/components/agents/chat/SessionChat.tsxapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/PagePaneView.tsxapps/web/src/components/agents/panes/PaneChat.tsxapps/web/src/components/agents/panes/PanePicker.tsxapps/web/src/components/agents/panes/pane-surface.tsapps/web/src/components/ai/shared/chat/tool-calls/OpenPagePaneRenderer.tsxapps/web/src/components/ai/shared/chat/tool-calls/registry.tsxapps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsxapps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsxapps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsxapps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsxapps/web/src/components/layout/middle-content/page-views/task-list/TaskListView.tsxapps/web/src/lib/ai/core/__tests__/ai-tools.test.tsapps/web/src/lib/ai/core/ai-tools.tsapps/web/src/lib/ai/shared/hooks/useOpenPagePane.tsapps/web/src/lib/ai/tools/page-pane-tools.tsapps/web/src/stores/agent-workspace/useAgentWorkspaceStore.tspackages/lib/src/agent-sessions/contract.tspackages/lib/src/content/page-types.config.ts
Adds a `'page'` pane kind alongside chat/terminal so a document, task list, sheet, canvas, code file, channel, or file can be opened (and edited) in an agent session's pane grid — most useful when an agent is actively editing a page and wants to show its work without the user hunting for it in the sidebar. - `PagePaneView`: generic pane renderer, dispatching through the same page-type config CenterPanel uses; view components are lazily loaded via next/dynamic (matches DocumentView's own editor-loading pattern, and keeps pdfjs-dist out of every pane bundle). - `isPaneablePageType` excludes FOLDER (a tree browser, not content) and AI_CHAT (would nest a chat inside a chat pane). - PanePicker gets a debounced "Pages" search section; AgentPanes gains handlePickPage/a 'page' render branch; useAgentWorkspaceStore gains openPage (shares openConversation's focus-or-replace-or-split logic). - useEditingStore registrations in DocumentView/CodePageView/ CanvasPageView/SheetView/TaskListView now include a useId() suffix so the same page open in the main view and a pane can't clobber each other's dirty-state entry. - New open_page_pane tool lets an agent surface a page pane itself: its execute() always acks (a pane grid is client-local state the server can't see), and a new useOpenPagePane hook reacts to the completed result by opening/focusing the pane in the conversation's own session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
CI (both required checks were red): - knip flagged apps/web/src/lib/ai/tools/page-pane-tools.ts's OpenPagePaneInput as a new unused export — deleted it (nothing consumed it; OpenPagePaneOutput, which IS consumed, is untouched). - packages/lib/src/agent-sessions/__tests__/contract.test.ts had a hardcoded PANE_KINDS exhaustiveness assertion that predates the new 'page' kind — updated the expected array and its guard comment. Review findings (Codex + CodeRabbit): - P1 (Codex): a page pane opened via open_page_pane in a single-chat-pane workspace would REPLACE the very conversation that triggered it (that chat pane is the only replaceable candidate). openPage now takes an optional excludeTargetId; useOpenPagePane passes the invoking conversation's own id so it's never evicted — splits beside it instead. Threaded conversationId through SessionChatView/SessionChat/AssistantSessionChat to make this possible. - P2 (Codex): remounting/reopening a conversation whose last message already contains a completed open_page_pane call would replay it — reopening/refocusing a pane the user may have since closed or repurposed. useOpenPagePane now seeds its seen-set from whatever's already on the first render without acting on it; only a call that becomes output-available on a LATER run is treated as new. - P2 (Codex): DocumentView was getting the SESSION's driveId (null for a global-assistant session) instead of the fetched PAGE's own driveId, disabking/misrouting usePageContentSocket for a page that always belongs to a real drive. PagePaneView now passes page.driveId (matching CenterPanel's own usage) and drops the now-unused driveId prop entirely. - Major (CodeRabbit): openPage could silently overwrite a page pane with unsaved edits. focusOrAssignScope's isReplaceable now checks useEditingStore for an active document/form session on that page's id and refuses to replace it — falls through to a split instead. - Major (CodeRabbit): PagePaneView used React.ReactNode/ComponentType without importing React — switched to named `ReactNode`/`ComponentType` imports from 'react'. - Minor (CodeRabbit): PanePicker's "Searching…" indicator used isLoading, which SWR's keepPreviousData keeps false during a background revalidation once any data has loaded — switched to isValidating. Also fixed the empty-query empty-state copy, which claimed "type to search" even though the endpoint already returns recent pages for an empty query. Also added test coverage CodeRabbit flagged as missing: PagePaneView's render branches (loading/error/unsupported/each view's prop convention, including the driveId fix) and useOpenPagePane's new history-replay and invoker-exclusion behavior, plus store-level tests for openPage's dirty-guard and excludeTargetId. Not changed (reviewed, no action): CodeRabbit suggested useId() might collide across multiple React roots and recommended crypto.randomUUID(). This app has exactly one root per page (Next.js App Router), so useId() is already unique across every simultaneous mount. Switching to crypto.randomUUID() would trade that safety for a real regression: DocumentView/CodePageView/CanvasPageView render server-side (unlike their ssr:false pane instances) in CenterPanel's main content area, and crypto.randomUUID() executed in a lazy useState initializer produces a different value on the server than on the client, which is exactly the hydration-mismatch pitfall useId() exists to avoid. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
6d306e6 to
3d4eee4
Compare
|
Pushed two follow-up commits addressing CI and review feedback ( CI (both required checks were red):
Review threads — replied individually on each of the 6 with what changed; leaving them open for your own verification rather than auto-resolving, since I fixed all of them during this pass rather than finding them already addressed. Two were real bugs worth calling out here too:
Two CodeRabbit nitpicks from the initial review body (not separate inline threads, so noting here):
Validation: |
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/agents/panes/PagePaneView.tsx`:
- Line 61: Update the SWR request key in PagePaneView to URL-encode pageId
before interpolating it into the `/api/pages/` path, ensuring values containing
`/`, `?`, or `#` address a single page resource.
In `@apps/web/src/lib/ai/shared/hooks/useOpenPagePane.ts`:
- Around line 56-70: Update the effect in useOpenPagePane to scan all assistant
messages for completed open_page_pane tool outputs, rather than only
messages[messages.length - 1]. Do not mark a completed tool call as handled
before processing it; preserve deduplication through handledRef while allowing
outputs already present during the initial scan to open the pane.
🪄 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: 6c88bc42-b3e6-4f44-aa30-e747f4420fb3
📒 Files selected for processing (28)
README.mdapps/marketing/src/app/docs/getting-started/page.tsxapps/web/src/components/agents/AgentPageView.tsxapps/web/src/components/agents/chat/AssistantSessionChat.tsxapps/web/src/components/agents/chat/SessionChat.tsxapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/PagePaneView.tsxapps/web/src/components/agents/panes/PaneChat.tsxapps/web/src/components/agents/panes/PanePicker.tsxapps/web/src/components/agents/panes/__tests__/PagePaneView.test.tsxapps/web/src/components/agents/panes/pane-surface.tsapps/web/src/components/ai/shared/chat/tool-calls/OpenPagePaneRenderer.tsxapps/web/src/components/ai/shared/chat/tool-calls/registry.tsxapps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsxapps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsxapps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsxapps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsxapps/web/src/components/layout/middle-content/page-views/task-list/TaskListView.tsxapps/web/src/lib/ai/core/__tests__/ai-tools.test.tsapps/web/src/lib/ai/core/ai-tools.tsapps/web/src/lib/ai/shared/hooks/__tests__/useOpenPagePane.test.tsapps/web/src/lib/ai/shared/hooks/useOpenPagePane.tsapps/web/src/lib/ai/tools/page-pane-tools.tsapps/web/src/stores/agent-workspace/__tests__/useAgentWorkspaceStore.test.tsapps/web/src/stores/agent-workspace/useAgentWorkspaceStore.tspackages/lib/src/agent-sessions/__tests__/contract.test.tspackages/lib/src/agent-sessions/contract.tspackages/lib/src/content/page-types.config.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/lib/src/agent-sessions/contract.ts
- apps/marketing/src/app/docs/getting-started/page.tsx
- apps/web/src/components/agents/AgentPageView.tsx
- apps/web/src/components/layout/middle-content/page-views/task-list/TaskListView.tsx
- apps/web/src/lib/ai/core/ai-tools.ts
- apps/web/src/components/layout/middle-content/page-views/sheet/SheetView.tsx
- README.md
- apps/web/src/components/ai/shared/chat/tool-calls/registry.tsx
- packages/lib/src/content/page-types.config.ts
- apps/web/src/lib/ai/core/tests/ai-tools.test.ts
- apps/web/src/components/agents/chat/AssistantSessionChat.tsx
- apps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsx
- apps/web/src/components/layout/middle-content/page-views/document/DocumentView.tsx
- apps/web/src/components/ai/shared/chat/tool-calls/OpenPagePaneRenderer.tsx
- apps/web/src/components/agents/panes/AgentPanes.tsx
- apps/web/src/components/agents/panes/pane-surface.ts
- apps/web/src/components/agents/panes/PaneChat.tsx
- apps/web/src/components/layout/middle-content/page-views/canvas/CanvasPageView.tsx
- apps/web/src/components/agents/panes/PanePicker.tsx
- apps/web/src/components/agents/chat/SessionChat.tsx
…witch Found during a proactive self-review pass (not from an external review comment): SessionChat/AssistantSessionChat are never keyed by conversationId, so the pane-bar agent switcher's handleSwitchAgent → assignPane rebinds the SAME pane's scope to a different conversation WITHOUT remounting SessionChatView. useOpenPagePane's handledRef/ hasSeededRef are useRef state, which survives exactly that kind of prop-only change — so switching a pane from conversation A (already seeded) to a fresh conversation B whose own last message already ends in a completed open_page_pane call would read B's history through A's stale hasSeededRef and wrongly treat it as new, reopening/refocusing a pane the user may have since closed or repurposed. Same failure class as the Codex-flagged remount case, just reachable via a different path. Added seenConversationIdRef: both refs now reset whenever conversationId changes, so a switch-in-place reseeds exactly like a fresh mount would. Two new tests cover it — one confirming a different conversation's own history isn't replayed after a switch, one confirming a genuinely new call after switching still fires. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
|
One more fix, found in a self-review pass rather than from an external comment (`201e1b489`): `SessionChat`/`AssistantSessionChat` are never keyed by `conversationId` — the pane-bar's agent-switcher (`handleSwitchAgent` → `assignPane`) rebinds the same pane's scope to a different conversation without remounting. `useOpenPagePane`'s `handledRef`/`hasSeededRef` are plain `useRef`s, so they survive exactly that prop-only change — switching a pane from a conversation that was already "seeded" to a different, unrelated conversation whose own last message already ends in a completed `open_page_pane` call would read the new conversation's history through the old one's stale ref state and wrongly treat it as live, reopening/refocusing a pane the user may have since closed or repurposed. Same failure class the Codex thread above flagged for remounts, just reachable via a second path I hadn't covered. Added a `seenConversationIdRef` that resets both refs whenever `conversationId` changes, plus two tests (one pinning the non-replay behavior, one confirming a genuinely new call after a switch still fires). Full validation (typecheck/lint/knip/targeted tests) green; pushed as a fast-forward on top of `3d4eee4c3`, no rebase needed this time. |
Applied a CodeRabbit suggestion (r3695743914): unlike every other
/api/pages/${pageId} call site in the app (CenterPanel, DocumentView,
CodePageView, etc. — all sourced from the loaded tree or a URL param),
this one can carry a pageId straight from an agent's open_page_pane
tool call, a free-text string rather than a validated id. A /, ?, or #
in it must not reroute or reshape the request; it should just 404 into
the existing error state.
Not applying CodeRabbit's other suggestion on this same push (scan
every message in useOpenPagePane rather than only the last one) —
replied on the thread with why: it would reintroduce the exact
history-replay bug the last-message check exists to prevent, and this
app's multi-step agentic turns still fold into one client-visible
UIMessage (same reason ask_user's own answerability selector only
checks the last message).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
# Conflicts: # apps/web/src/components/agents/panes/AgentPanes.tsx
|
Sibling PR #2299 ("pane-level Chat/History/Settings tabs") merged into master while this was in flight. Merged `master` in (`4361529e4`) rather than rebasing — one conflict, in `AgentPanes.tsx`'s `renderPane`, since #2299 pulled chat-pane rendering out into a new early-returning `ChatPane` component. Resolution: kept my `'page'` branch (``) in the tail of `renderPane` alongside the picker/loading/terminal cases (unchanged by #2299), and dropped the now-dead inline `'chat'` branch there (unreachable — any chat-kind pane already returns via the new `ChatPane` early return). I also had to thread `sessionId` into `ChatPane`'s own `` call (it wasn't part of the textual conflict — `ChatPane` already receives `sessionId` as a prop for its own History-tab needs, it just wasn't forwarding it to `PaneChat` — needed for `useOpenPagePane`'s remount/switch fixes above to apply inside the new tabbed chat pane too). Full validation after the merge: typecheck/lint/knip clean, |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4361529e4a
ℹ️ 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".
| } else if (componentName === 'CodePageView') { | ||
| const CodePageView = componentMap.CodePageView; | ||
| pageComponent = <CodePageView key={`code-${page.id}`} pageId={page.id} />; |
There was a problem hiding this comment.
When a code page is opened from the global /dashboard/agents surface, CodePageView receives only the page ID and derives driveId from useParams(), where no drive parameter exists. Its usePageTree lookup therefore cannot detect the language from the page title, and—more importantly—its page:content-updated listener never joins the opened page's drive room, so collaborator or AI edits are missed. Pass the fetched page's drive context into the code view and subscribe to that drive rather than relying on the surrounding route.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — confirmed real, not just theoretical for panes specifically: CodePageView never accepted a driveId prop at all, always deriving one from useParams() (absent in the global assistant console, wrong for a cross-drive session), and its raw useSocket() listener never joined the page's own drive room the way DocumentView's usePageContentSocket does. Added an optional driveId prop (falls back to the route param — CenterPanel's existing call site is unaffected) and swapped the raw listener for usePageContentSocket, mirroring DocumentView exactly. PagePaneView now passes page.driveId here too. Added a test pinning it. See 07af1c4.
| <span className="text-muted-foreground"> | ||
| Opened <span className="font-medium text-foreground">{title ?? 'this page'}</span> in a pane | ||
| </span> |
There was a problem hiding this comment.
In a plain page-agent conversation or other surface with sessionId === null, open_page_pane still executes and is rendered here, but useOpenPagePane explicitly no-ops because there is no pane grid. The user consequently sees “Opened … in a pane” even though nothing happened; either suppress/filter the tool outside session conversations or render an acknowledgement that does not assert the pane was opened.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — good catch, this was a real honesty gap. Reworded the card from "Opened X in a pane" to "Requested to open X in a pane": an accurate description of the action taken (a signal was sent) rather than a claim about the outcome (a pane visibly appeared), which the server genuinely cannot know either way. See 07af1c4.
| const pages = results.filter( | ||
| (result) => result.type === 'page' && isPaneablePageType(result.data?.pageType ?? PageType.DOCUMENT), | ||
| ); |
There was a problem hiding this comment.
The mentions endpoint truncates its response to ten suggestions before this client-side filter runs. If those ten matches are folders or AI-chat pages—for example, the most recently updated pages for an empty query—pages becomes empty and the picker reports “No pages found” even when paneable results exist later in the endpoint's candidate set. Exclude the unsupported page types in the server query or otherwise apply the limit after filtering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at the root rather than client-side: added an additive excludePageTypes param to /api/mentions/search (defaults to no exclusion, so every other consumer — MentionPicker, PagePickerPopover, TriggerPagePicker — is unaffected), applied per-row BEFORE the existing final slice(0, 10), and PanePicker now sends it instead of relying on a client-side filter after the fact. Added a test that specifically simulates the top-10 window being dominated by excluded types. See 07af1c4.
Three new P2 findings from a fresh @codex review after the master merge: - CodePageView never accepted a driveId prop — it always derived one from the route's own useParams(), which is absent (or wrong, for a cross-drive page) when hosted in a pane. Its raw useSocket() listener also never joined the page's own drive room at all (unlike DocumentView's usePageContentSocket), so live updates for a code page opened from the global assistant console — or any cross-drive session — were silently missed. Added an optional driveId prop (falls back to the route param, so CenterPanel's existing call site is unaffected) and replaced the raw socket listener with usePageContentSocket, mirroring DocumentView exactly. PagePaneView now passes page.driveId here too. - OpenPagePaneRenderer said "Opened X in a pane" unconditionally, but the tool's execute() always acks (the server can't know if any client actually had a pane grid to act on — see page-pane-tools.ts's doc) — in a plain, session-less conversation this claimed something happened when nothing did. Reworded to "Requested to open X in a pane", an honest description of the action taken rather than a claim about the outcome. - PanePicker's Pages search filtered FOLDER/AI_CHAT client-side, but /api/mentions/search caps its response to 10 suggestions BEFORE that filter ever runs — if the top 10 by relevance/recency happened to be dominated by excluded types, eligible pages ranked just below them would vanish from view entirely. Added excludePageTypes to the route (additive, defaults to no exclusion — every other caller unaffected), applied per-row before the existing final slice(0, 10), and PanePicker now sends it. Added test coverage for all three (CodePageView driveId assertion in PagePaneView.test.tsx, a new excludePageTypes describe block in the mentions-search route's own test file). Full validation: typecheck/ lint/knip clean; the affected surface (mentions search + its other consumers, PagePaneView, PanePicker, AgentPanes, task-list, code view, full @pagespace/lib) all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
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/app/api/mentions/search/route.ts`:
- Around line 146-159: Push excludePageTypesParam into the database query’s
WHERE clause by adding notInArray(pages.type, [...excludePageTypesParam])
alongside the existing pageTypeParam filter. Remove any redundant post-fetch
exclusion while preserving the final suggestions ranking and limit behavior.
In
`@apps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsx`:
- Around line 173-190: Stabilize the callback passed to usePageContentSocket so
typing changes do not repeatedly detach and reattach the page:content-updated
listener. Keep the latest documentState?.content and documentState?.isDirty
accessible through a mutable callback ref, invoke that ref from the socket
handler, and ensure the listener effect depends only on stable connection inputs
such as pageId, driveId, and enabled.
🪄 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: 370d9980-59d9-4fd7-925c-108c696cc0b9
📒 Files selected for processing (11)
apps/web/src/app/api/mentions/search/__tests__/route.test.tsapps/web/src/app/api/mentions/search/route.tsapps/web/src/components/agents/AgentPageView.tsxapps/web/src/components/agents/panes/AgentPanes.tsxapps/web/src/components/agents/panes/PagePaneView.tsxapps/web/src/components/agents/panes/PanePicker.tsxapps/web/src/components/agents/panes/__tests__/PagePaneView.test.tsxapps/web/src/components/ai/shared/chat/tool-calls/OpenPagePaneRenderer.tsxapps/web/src/components/layout/middle-content/page-views/code/CodePageView.tsxapps/web/src/lib/ai/shared/hooks/__tests__/useOpenPagePane.test.tsapps/web/src/lib/ai/shared/hooks/useOpenPagePane.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/components/agents/AgentPageView.tsx
- apps/web/src/components/agents/panes/AgentPanes.tsx
- apps/web/src/components/agents/panes/PanePicker.tsx
- apps/web/src/components/agents/panes/PagePaneView.tsx
CodeRabbit follow-up on 07af1c4: excludePageTypesParam was only checked after pages.limit(50)/limit(20) had already run, so if excluded types dominated THAT initial DB-level fetch window (not just the final slice(0, 10) the previous commit fixed), eligible pages ranked beyond it would still never reach the response. Added notInArray(pages.type, [...excludePageTypesParam]) to the WHERE clause alongside the existing pageTypeParam condition, and removed the now-redundant post-fetch in-memory check. Rewrote the excludePageTypes tests to match: the mocked db.select chain can't observe SQL-level filtering (its .where() is a no-op passthrough, the same limitation every other filter test in this file already has), so they now spy on the real (unmocked) notInArray from @pagespace/db/operators and assert it's called with the right arguments — or not called at all when the param is absent/garbage — rather than asserting on a response shape the mock can't meaningfully vary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
1 participant
Summary
'page'pane kind alongsidechat/terminalso any non-container, non-chat page type (document, task list, sheet, canvas, code, channel, file) can be opened and edited in an agent session's pane grid.PagePaneViewdispatches through the same page-type configCenterPaneluses, lazily loading each view vianext/dynamic(keepspdfjs-distand other heavy editors out of every pane bundle).PanePickergets a debounced "Pages" search section (backed by/api/mentions/search, now with a server-sideexcludePageTypesparam so FOLDER/AI_CHAT never occupy a slot in its capped result window);AgentPanes/useAgentWorkspaceStoregain the wiring (handlePickPage,openPage, sharingopenConversation's focus-or-replace-or-split policy) — extended with a dirty-page guard (never silently replaces a pane with unsaved edits) and anexcludeTargetIdguard (never evicts the conversation that itself asked to open a page).DocumentView/CodePageView/CanvasPageView/SheetView/TaskListViewnow suffix theiruseEditingStoreregistration withuseId(), so the same page open in the main center panel and a pane can no longer clobber each other's dirty-state entry.open_page_panetool lets an agent surface a page pane itself — itsexecute()always acks (the server has no visibility into any browser tab's pane-grid state, which is purely client-local), and a newuseOpenPagePanehook reacts to the completed result by opening/focusing the pane in the conversation's own session, with guards against replaying historical calls on remount, on a pane-bar conversation switch, and against evicting the invoking conversation. Registered as an ordinary workspace tool (77th), with a rich tool-call card (OpenPagePaneRenderer) that describes the action taken ("Requested to open…"), not a claimed outcome.CodePageViewnow accepts an optionaldriveId(falls back to the route param, soCenterPanel's existing call site is unaffected) and joins its page's own drive room viausePageContentSocket, matchingDocumentView— needed because a pane can host a code page outside its own drive's route entirely.Follow-up commits (post-review, in order)
3d4eee4c3— CI fixes (a knip dead-code finding, a stalePANE_KINDSexhaustiveness test) + every first-round Codex/CodeRabbit finding: the invoking-chat eviction bug, the tool-call history-replay bug, a wrongdriveIdpassed toDocumentView, a missing dirty-pane guard, missingReactNode/ComponentTypeimports, anisLoading/isValidatingSWR bug in the picker.201e1b489— a second, self-found history-replay instance: switching a pane's bound conversation in place (no remount) could replay a different conversation's own history.881ef0847— URL-encoded the agent-suppliablepageIdinPagePaneView's fetch.4361529e4— merged master after sibling PR feat(agents): pane-level Chat/History/Settings tabs (+ history-deleted-send fix) #2299 ("pane-level Chat/History/Settings tabs") pulled chat rendering into a newChatPanecomponent; resolved the one conflict and threadedsessionIdintoChatPane's ownPaneChatcall.07af1c482— a second@codex reviewpass found three more real issues, all fixed:CodePageView's missing driveId/socket-room join,OpenPagePaneRendereroverclaiming "Opened" when the tool's ack can't actually know that, and the mentions-search truncation gap inPanePicker's search.99085867a— CodeRabbit follow-up: pushed theexcludePageTypesfix into the actual SQLWHEREclause (my prior commit only fixed it before the final.slice(0,10), not before the earlier DB-level.limit(50/20)fetch).Test plan
bun run typecheck/bun run lint/bun run knip:check(web, @pagespace/lib) — clean throughout@pagespace/libsuite (8682 tests) and the full touchedwebsurface (panes/store/hooks/tool-registry/API routes/mentions-search/code view — 198+ tests in the final sweep alone,AgentPanes.test.tsx's 65 post-merge) — all greentool-registry-docs.test.ts) — updated README.md and marketing docs from 76 → 77 workspace toolsMERGEABLE,CLEAN, all 10 CI checks green as of99085867a@codex review) both re-verified and withdrew/resolved every finding they raisedopen_page_paneafter editing a page and confirm the pane opens without manual interaction🤖 Generated with Claude Code
https://claude.ai/code/session_01FaXkcqLXNPp9AnbJVntWqD
Summary by CodeRabbit