feat(workspaces): reveal PDF citations at cited pages - #685
Conversation
Keep PDF page locators and compact-row metadata visible when primary labels are long. Preserve full citation labels for hover and assistive technology.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWorkspace navigation now returns view instance ids, propagates them through workspace viewers and editors, and supports request-driven PDF page reveals. Citation labels include page locators, while workspace and chat presentation styles are updated. ChangesWorkspace navigation and rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceCitation
participant WorkspaceLocationProvider
participant WorkspaceLayout
participant useWorkspaceNavigation
participant WorkspacePdfDocumentLoader
WorkspaceCitation->>WorkspaceLocationProvider: reveal PDF page location
WorkspaceLocationProvider->>WorkspaceLayout: navigate(location)
WorkspaceLayout->>useWorkspaceNavigation: revealWorkspaceLocation(location)
useWorkspaceNavigation-->>WorkspaceLayout: return view instance id
WorkspaceLayout-->>WorkspaceLocationProvider: provide view instance id
WorkspaceLocationProvider->>WorkspacePdfDocumentLoader: store reveal request
WorkspacePdfDocumentLoader->>WorkspacePdfDocumentLoader: scroll to page and consume request
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
![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: cedc887670
ℹ️ 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".
|
|
||
| let handled = false; | ||
|
|
||
| return scrollCapability.onLayoutReady((event) => { |
There was a problem hiding this comment.
When a citation targets a PDF that is already open and laid out, this effect subscribes to onLayoutReady only after the click, so the existing readiness event has already passed and no immediate call to scrollToPage occurs. The request then remains pending until another layout event happens, meaning citations for the currently viewed PDF—the common case—do not reveal their cited page; consult the current scroll state or retain layout readiness and scroll immediately when it is already available.
Useful? React with 👍 / 👎.
| if (activeTab?.viewItemId === item.id) { | ||
| return true; | ||
| return activeTab.id; |
There was a problem hiding this comment.
When presentation.mode is split and a citation targets a PDF rendered in one of its panes, this returns a standard tab ID, while WorkspacePaneRenderer passes pane.id as the viewer's viewInstanceId. The location context consequently tags the request with an ID no mounted split-pane viewer accepts, so the cited page never scrolls and any tab activation occurs behind the still-visible split presentation; navigation needs to resolve and return the displayed pane's ID in this mode.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| setRevealRequest( | ||
| viewInstanceId && location.kind === "pdf-page" ? { location, viewInstanceId } : null, | ||
| ); |
There was a problem hiding this comment.
When a PDF citation is opened in maximized or split-pane presentation, this request stores the tab ID returned by navigation, while the mounted PDF viewer subscribes using its pane ID. The identifiers never match, so the citation opens the PDF without revealing the cited page.
Artifacts
Repro: focused React harness exercising the real pane renderer and reveal context
- Evidence file captured while the check ran.
Repro: verbose Vitest output showing the pane and tab identifier mismatch and missed page-37 request
- The full command output behind this check.
Greptile SummaryThis PR adds page-specific PDF citation navigation and adjusts compact metadata layouts.
Confidence Score: 3/5The PR should not merge until PDF citations can target viewers in maximized and split-pane presentations. Page reveal requests are keyed with tab IDs, while pane-rendered PDF viewers subscribe with independent pane IDs, so those viewers never receive the requested page. Files Needing Attention: src/features/workspaces/locations/workspace-location-context.tsx, src/features/workspaces/components/WorkspacePaneRenderer.tsx, and src/features/workspaces/navigation/useWorkspaceNavigation.ts
T-Rex LogsWhat T-Rex did
|
| Filename | Overview |
|---|---|
| src/features/workspaces/locations/workspace-location-context.tsx | Adds page-reveal request state, but records tab IDs that cannot target pane-identified viewers. |
| src/features/workspaces/navigation/useWorkspaceNavigation.ts | Returns selected tab IDs from location navigation for downstream reveal targeting. |
| src/features/workspaces/components/WorkspacePdfViewer.tsx | Consumes matching page requests after PDF layout readiness and scrolls to valid pages. |
| src/features/workspaces/components/WorkspacePaneRenderer.tsx | Identifies pane-hosted viewers with pane IDs, exposing the mismatch with tab-targeted reveal requests. |
| src/features/workspaces/components/ai-chat/WorkspaceCitation.tsx | Separates truncating citation labels from fixed page locators and preserves the full accessible label. |
Reviews (1): Last reviewed commit: "fix(workspaces): preserve trailing metad..." | Re-trigger Greptile
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/workspaces/components/ai-chat/AiChatMessageResponse.test.tsx (1)
1-8: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftCI blocker: suite fails to import due to
katex/contrib/copy-texreferencingdocument.Pipeline logs show the whole suite fails at import time (
ReferenceError: document is not defined), traced toAiChatMessageResponse.tsx'simport "katex/contrib/copy-tex"executing in a non-DOM vitest environment. None of the assertions in this file can run until this is fixed — either by configuring a DOM environment (jsdom) for this test file/suite, or by mocking/guarding the KaTeX copy-tex import inAiChatMessageResponse.tsx.🤖 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 `@src/features/workspaces/components/ai-chat/AiChatMessageResponse.test.tsx` around lines 1 - 8, Fix the import-time failure affecting the AiChatMessageResponse test suite by making the katex/contrib/copy-tex dependency safe in the non-DOM Vitest environment. Prefer configuring this test file or suite to use a DOM environment such as jsdom; otherwise guard or mock the copy-tex import in AiChatMessageResponse without changing normal browser behavior.Source: Pipeline failures
🤖 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 `@src/features/workspaces/components/WorkspacePdfViewer.tsx`:
- Around line 197-209: Update the PDF reveal effect in the component containing
useWorkspacePdfPageRevealRequest to synchronously check whether
scrollCapability’s layout is already ready before subscribing to its lifecycle
event. Immediately consume or process the pending request when ready, while
retaining the onLayoutAfter subscription for layouts that are not yet ready so
reused inactive tabs navigate correctly.
In `@src/features/workspaces/navigation/useWorkspaceNavigation.ts`:
- Around line 217-240: Update revealWorkspaceLocation to detect non-standard
presentation modes, resolve the active WorkspacePane for the target item, and
return its pane.id instead of the session tab id; handle split and maximized
modes consistently with WorkspacePaneRenderer’s viewInstanceId. Preserve the
existing tab activation, replacement, and new-tab behavior for standard
presentation.
---
Outside diff comments:
In `@src/features/workspaces/components/ai-chat/AiChatMessageResponse.test.tsx`:
- Around line 1-8: Fix the import-time failure affecting the
AiChatMessageResponse test suite by making the katex/contrib/copy-tex dependency
safe in the non-DOM Vitest environment. Prefer configuring this test file or
suite to use a DOM environment such as jsdom; otherwise guard or mock the
copy-tex import in AiChatMessageResponse without changing normal browser
behavior.
🪄 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: 90f0cd71-faca-4315-a9a2-3a9557d549d9
📒 Files selected for processing (14)
src/features/workspaces/components/WorkspaceContent.tsxsrc/features/workspaces/components/WorkspaceFileViewer.tsxsrc/features/workspaces/components/WorkspaceImageViewer.tsxsrc/features/workspaces/components/WorkspaceLayout.tsxsrc/features/workspaces/components/WorkspaceMobileBreadcrumbOverflow.tsxsrc/features/workspaces/components/WorkspacePaneRenderer.tsxsrc/features/workspaces/components/WorkspacePdfViewer.tsxsrc/features/workspaces/components/WorkspaceStandardTabPanes.tsxsrc/features/workspaces/components/ai-chat/AiChatMessageResponse.test.tsxsrc/features/workspaces/components/ai-chat/WorkspaceCitation.tsxsrc/features/workspaces/components/document-editor/DocumentEditorSurface.tsxsrc/features/workspaces/components/workspace-card-meta-row.tsxsrc/features/workspaces/locations/workspace-location-context.tsxsrc/features/workspaces/navigation/useWorkspaceNavigation.ts
| viewInstanceId: string; | ||
| workspaceId: string; | ||
| }) { | ||
| const { provides: documentManager } = useDocumentManagerCapability(); | ||
| const { provides: scrollCapability } = useScrollCapability(); | ||
| const paneRuntime = useWorkspacePaneRuntime(); | ||
| const { consume, request } = useWorkspacePdfPageRevealRequest(viewInstanceId); | ||
| const [openError, setOpenError] = useState<{ | ||
| documentId: string; | ||
| message: string; | ||
| } | null>(null); | ||
| const currentOpenError = openError?.documentId === documentId ? openError.message : null; | ||
| const isActive = paneRuntime?.isActive ?? true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether `@embedpdf/plugin-scroll` exposes a synchronous "current layout/ready" query
# that can be checked immediately (avoiding reliance on a fresh onLayoutReady firing).
fd -HI 'plugin-scroll' node_modules/@embedpdf 2>/dev/null
rg -n 'isInitial|onLayoutReady|getState|totalPages' node_modules/@embedpdf/plugin-scroll/dist 2>/dev/null | head -50Repository: ThinkEx-OSS/thinkex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files of interest:"
git ls-files | rg '(^|/)WorkspacePdfViewer\.tsx$|WorkspaceStandardTabPanes|package\.json|yarn\.lock|pnpm-lock\.yaml|package-lock\.json'
echo
echo "Dependency versions:"
if [ -f package.json ]; then jq '.dependencies, .devDependencies' package.json | rg -n 'embedpdf|react-pdf|pdf|plugin-scroll' -C 2; fi
echo
echo "Inspect WorkspacePdfViewer outline and relevant sections:"
ast-grep outline src/features/workspaces/components/WorkspacePdfViewer.tsx 2>/dev/null || true
wc -l src/features/workspaces/components/WorkspacePdfViewer.tsx
sed -n '1,340p' src/features/workspaces/components/WorkspacePdfViewer.tsx
echo
echo "Inspect WorkspaceStandardTabPanes:"
fd -HI 'WorkspaceStandardTabPanes' . -x sh -c 'echo "FILE: $1"; wc -l "$1"; sed -n "1,220p" "$1"' sh {}
echo
echo "Search revealWorkspaceLocation and related types:"
rg -n "revealWorkspaceLocation|WorkspacePdfPage|useWorkspacePdfPageRevealRequest|onLayoutReady|viewInstanceId|handleScroll" src package.jsonRepository: ThinkEx-OSS/thinkex
Length of output: 20236
🌐 Web query:
embedpdf plugin-scroll onLayoutReady isInitial documentation
💡 Result:
In the EmbedPDF viewer, the onLayoutReady event is part of the Scroll plugin and is used to perform actions—such as programmatic scrolling—once a document's layout has been fully initialized and is ready for interaction [1][2][3]. The isInitial property is a boolean flag included in the payload of the onLayoutReady event [1][4]. It serves to distinguish between different layout stages: - isInitial is true only on the first layout immediately following the initial document load [1][5]. - isInitial is false on subsequent layouts, such as when switching between tabs or re-rendering the layout [1][5]. Because documents load asynchronously, developers should use the onLayoutReady event rather than attempting to call navigation methods like scrollToPage immediately after initialization [2][6][3]. Checking that isInitial is true ensures that your logic (e.g., jumping to a specific page on load) only executes when the document is first ready [2][4][5]. This pattern was introduced as part of a update that removed the global initialPage configuration option in favor of more flexible, programmatic control [4][5].
Citations:
- 1: https://www.embedpdf.com/docs/react/headless/plugins/plugin-scroll
- 2: https://www.embedpdf.com/docs/react/viewer/plugins/plugin-scroll
- 3: https://www.embedpdf.com/docs/vue/viewer/plugins/plugin-scroll
- 4: https://github.com/embedpdf/embed-pdf-viewer/releases/tag/v2.0.0-next.1
- 5: https://github.com/embedpdf/embed-pdf-viewer/blob/main/packages/plugin-scroll/CHANGELOG.md
- 6: https://www.embedpdf.com/docs/svelte/viewer/plugins/plugin-scroll
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect workspace navigation / revealWorkspaceLocation:"
wc -l src/features/workspaces/navigation/useWorkspaceNavigation.ts
sed -n '180,290p' src/features/workspaces/navigation/useWorkspaceNavigation.ts
echo
echo "Inspect workspace location context:"
wc -l src/features/workspaces/locations/workspace-location-context.tsx
sed -n '1,120p' src/features/workspaces/locations/workspace-location-context.tsx
echo
echo "Inspect workspace content around pdf viewer path:"
sed -n '380,530p' src/features/workspaces/components/WorkspaceContent.tsx
echo
echo "Search pdfPage references:"
rg -n "pdf-page|pdfPage|pdf-page|locationPage|pageNumber|revealWorkspaceLocation|matchingTab|activeItem" src -C 2Repository: ThinkEx-OSS/thinkex
Length of output: 50375
🌐 Web query:
embedpdf plugin-scroll source onLayoutReady handler isInitial
💡 Result:
In the EmbedPDF viewer, the onLayoutReady event handler within the scroll plugin is used to handle scrolling after a document has loaded [1][2]. Because document loading is asynchronous, this event ensures that the document structure is calculated and the viewer is ready for interaction before attempting to navigate to a specific position [1][3]. The onLayoutReady event provides a payload containing documentId and isInitial properties [4][5]. The isInitial boolean is a key feature used to differentiate between the first layout following a document's initial load (where isInitial is true) and subsequent layout events, such as those triggered by switching tabs or document re-renders (where isInitial is false) [6][7]. This mechanism was introduced as part of a move to remove the global initialPage configuration option [6][7]. By checking if isInitial is true, developers can safely trigger actions—like scrolling to a specific page—only once during the initial load cycle [1][2]. Example usage in React: const unsubscribe = scrollCapability.onLayoutReady((event) => { if (event.documentId === documentId && event.isInitial) { scrollCapability.forDocument(documentId).scrollToPage({ pageNumber: initialPage, behavior: 'instant', }); } });
Citations:
- 1: https://www.embedpdf.com/docs/react/viewer/plugins/plugin-scroll
- 2: https://www.embedpdf.com/docs/vue/viewer/plugins/plugin-scroll
- 3: https://www.embedpdf.com/docs/snippet/plugins/plugin-scroll
- 4: https://www.embedpdf.com/docs/react/headless/plugins/plugin-scroll
- 5: https://www.embedpdf.com/docs/svelte/headless/plugins/plugin-scroll
- 6: https://github.com/embedpdf/embed-pdf-viewer/blob/main/packages/plugin-scroll/CHANGELOG.md
- 7: https://github.com/embedpdf/embed-pdf-viewer/releases/tag/v2.0.0-next.1
🌐 Web query:
github embedpdf embed-pdf-viewer plugin-scroll source onLayoutReady isInitial
💡 Result:
The terms onLayoutReady and isInitial refer to the API provided by the @embedpdf/plugin-scroll package within the embedpdf/embed-pdf-viewer repository [1][2][3]. The onLayoutReady event is used to detect when a document's layout is fully calculated and ready for interaction, which is necessary because PDF documents load asynchronously [1][3]. The isInitial flag is a boolean included in the event payload that allows developers to distinguish between the first layout after an initial document load (true) and subsequent layout events, such as those occurring when switching between tabs (false) [3][4][5]. This mechanism was introduced to replace the deprecated initialPage configuration option, as the latter did not support multi-document scenarios effectively [4][5]. Developers typically use this to perform actions like scrolling to a specific page immediately after the viewer initializes [1][4]. Example usage (React): const unsubscribe = scrollCapability.onLayoutReady((event) => { if (event.documentId === 'my-doc' && event.isInitial) { scrollCapability.forDocument('my-doc').scrollToPage({ pageNumber: 3, behavior: 'instant' }); } });
Citations:
- 1: https://www.embedpdf.com/docs/react/viewer/plugins/plugin-scroll
- 2: https://github.com/embedpdf/embed-pdf-viewer
- 3: https://www.embedpdf.com/docs/react/headless/plugins/plugin-scroll
- 4: https://github.com/embedpdf/embed-pdf-viewer/blob/main/packages/plugin-scroll/CHANGELOG.md
- 5: https://github.com/embedpdf/embed-pdf-viewer/releases/tag/v2.0.0-next.1
🌐 Web query:
embedpdf plugin-scroll @embedpdf/plugin-scroll source onLayoutReady isInitial getLayout
💡 Result:
In the @embedpdf/plugin-scroll package, the onLayoutReady event is a lifecycle event provided by the scroll capability that triggers when a document's layout has been calculated and the viewer is ready for navigation [1][2][3]. The event provides a payload containing the documentId and a boolean isInitial property [1][2]. The isInitial property is true only during the first layout calculation immediately following the initial loading of the document [1][2]. It becomes false on subsequent layout calculations, such as those that occur when switching tabs or changing viewer configurations [1][2][4]. This event is essential for programmatic navigation, such as calling scrollToPage, because it ensures the document structure is available before scroll operations are attempted [3]. While the provided information focuses on onLayoutReady, the getLayout function is not typically cited as a direct top-level public API in the standard scroll plugin documentation [1][2][3]. Interaction with layout state is generally handled via the provided scroll capability hooks (like useScroll) and events (like onLayoutReady or onScroll) which abstract the underlying layout calculations [1][3].
Citations:
- 1: https://www.embedpdf.com/docs/react/headless/plugins/plugin-scroll
- 2: https://www.embedpdf.com/docs/vue/headless/plugins/plugin-scroll
- 3: https://www.embedpdf.com/docs/vue/viewer/plugins/plugin-scroll
- 4: https://www.embedpdf.com/docs/svelte/headless/plugins/plugin-scroll
Check EmbedPDF layout readiness synchronously before relying on onLayoutReady.
For reused inactive PDF tabs, revealWorkspaceLocation reactivates an already-mounted view, and this effect may subscribe to scrollCapability.onLayoutAfter after that document’s layout has already fired. Since onLayoutReady is modeled as a lifecycle event, a late subscriber can be left waiting and request will never be consumed, making citation-aware PDF navigation no-op on the standard tab-reuse path. Handle already-ready state before subscribing if a synchronous capability/query is available.
🤖 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 `@src/features/workspaces/components/WorkspacePdfViewer.tsx` around lines 197 -
209, Update the PDF reveal effect in the component containing
useWorkspacePdfPageRevealRequest to synchronously check whether
scrollCapability’s layout is already ready before subscribing to its lifecycle
event. Immediately consume or process the pending request when ready, while
retaining the onLayoutAfter subscription for layouts that are not yet ready so
reused inactive tabs navigate correctly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/components/WorkspacePaneRenderer.tsx">
<violation number="1" location="src/features/workspaces/components/WorkspacePaneRenderer.tsx:18">
P2: Citations opened while the workspace is in split presentation will not reveal the page in the target pane: this line identifies the PDF viewer with `pane.id`, but the navigation callback stores a workspace tab ID in the reveal request. The request consequently matches no mounted `WorkspacePdfViewer`. The navigation/reveal flow would need to resolve and return the actual rendered pane instance (or otherwise map the tab to that pane) for split/maximized presentations.</violation>
</file>
<file name="src/features/workspaces/components/WorkspacePdfViewer.tsx">
<violation number="1" location="src/features/workspaces/components/WorkspacePdfViewer.tsx:287">
P3: The `consume` function returned by `useWorkspacePdfPageRevealRequest` gets a new reference on every render of `WorkspaceLocationProvider` because it's defined inline without `useCallback`. Since the new effect depends on `consume`, this causes the effect to tear down and re-register the `onLayoutReady` listener whenever the provider re-renders — even when nothing about the reveal request has actually changed. Consider stabilizing the reference with `useCallback` in the provider to avoid wasteful listener churn.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| return ( | ||
| <WorkspaceContent | ||
| viewInstanceId={pane.id} |
There was a problem hiding this comment.
P2: Citations opened while the workspace is in split presentation will not reveal the page in the target pane: this line identifies the PDF viewer with pane.id, but the navigation callback stores a workspace tab ID in the reveal request. The request consequently matches no mounted WorkspacePdfViewer. The navigation/reveal flow would need to resolve and return the actual rendered pane instance (or otherwise map the tab to that pane) for split/maximized presentations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/components/WorkspacePaneRenderer.tsx, line 18:
<comment>Citations opened while the workspace is in split presentation will not reveal the page in the target pane: this line identifies the PDF viewer with `pane.id`, but the navigation callback stores a workspace tab ID in the reveal request. The request consequently matches no mounted `WorkspacePdfViewer`. The navigation/reveal flow would need to resolve and return the actual rendered pane instance (or otherwise map the tab to that pane) for split/maximized presentations.</comment>
<file context>
@@ -15,6 +15,7 @@ export default function WorkspacePaneRenderer({
return (
<WorkspaceContent
+ viewInstanceId={pane.id}
workspace={workspace}
items={scopedItems}
</file context>
| }); | ||
| }, [ | ||
| activeDocumentId, | ||
| consume, |
There was a problem hiding this comment.
P3: The consume function returned by useWorkspacePdfPageRevealRequest gets a new reference on every render of WorkspaceLocationProvider because it's defined inline without useCallback. Since the new effect depends on consume, this causes the effect to tear down and re-register the onLayoutReady listener whenever the provider re-renders — even when nothing about the reveal request has actually changed. Consider stabilizing the reference with useCallback in the provider to avoid wasteful listener churn.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/components/WorkspacePdfViewer.tsx, line 287:
<comment>The `consume` function returned by `useWorkspacePdfPageRevealRequest` gets a new reference on every render of `WorkspaceLocationProvider` because it's defined inline without `useCallback`. Since the new effect depends on `consume`, this causes the effect to tear down and re-register the `onLayoutReady` listener whenever the provider re-renders — even when nothing about the reveal request has actually changed. Consider stabilizing the reference with `useCallback` in the provider to avoid wasteful listener churn.</comment>
<file context>
@@ -240,6 +252,46 @@ function WorkspacePdfDocumentLoader({
+ });
+ }, [
+ activeDocumentId,
+ consume,
+ currentOpenError,
+ documentId,
</file context>
Tool activity row was rendering at the same weight and near-full text color as the surrounding chat response, so it competed with the model's output. Drops it to text-xs + muted-foreground with medium weight so it reads as a status label one hierarchy level below prose. Also shrinks the leading icon slot to match. Model picker trigger swaps the ad-hoc class string for the shared WorkspaceToolbarTextButton primitive. Removes an unused rounded-full class from the dictation button. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Send page requests to the mounted split or maximized PDF viewer. Load KaTeX copy support only in the browser for node tests and SSR.
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/navigation/useWorkspaceNavigation.ts">
<violation number="1" location="src/features/workspaces/navigation/useWorkspaceNavigation.ts:242">
P2: On mobile, citations can target an unmounted pane and never reach `WorkspacePdfViewer`: the navigation helper returns the desktop split/maximized `pane.id`, while mobile renders the item through its tab ID. The view-instance lookup should be bypassed or normalized to standard-tab navigation when the mobile layout is active.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return undefined; | ||
| } | ||
|
|
||
| const presentedViewInstanceId = findPresentedItemViewInstanceId(presentation, item.id); |
There was a problem hiding this comment.
P2: On mobile, citations can target an unmounted pane and never reach WorkspacePdfViewer: the navigation helper returns the desktop split/maximized pane.id, while mobile renders the item through its tab ID. The view-instance lookup should be bypassed or normalized to standard-tab navigation when the mobile layout is active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/navigation/useWorkspaceNavigation.ts, line 242:
<comment>On mobile, citations can target an unmounted pane and never reach `WorkspacePdfViewer`: the navigation helper returns the desktop split/maximized `pane.id`, while mobile renders the item through its tab ID. The view-instance lookup should be bypassed or normalized to standard-tab navigation when the mobile layout is active.</comment>
<file context>
@@ -220,6 +239,11 @@ export function useWorkspaceNavigation({
return undefined;
}
+ const presentedViewInstanceId = findPresentedItemViewInstanceId(presentation, item.id);
+ if (presentedViewInstanceId) {
+ return presentedViewInstanceId;
</file context>
1 participant
Summary
Why
PDF citations already identified a durable page location, but opening a citation did not consistently carry that location through navigation and into the mounted PDF viewer. Long item names could also truncate the page number from the citation pill.
Changes
Testing
pnpm exec vp checkon the five focused citation/layout files.pnpm exec vitest run --environment jsdom src/features/workspaces/components/ai-chat/AiChatMessageResponse.test.tsx src/features/workspaces/locations/workspace-location.test.tsReview Notes
Start with
workspace-location-context.tsx,useWorkspaceNavigation.ts, andWorkspacePdfViewer.tsxfor the reveal flow. The compact-row changes are intentionally local; no generic layout wrapper was introduced.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests