fix(document-viewer): open document pages at the top - #1782
Conversation
Search and answer links always attach ?chunk=, and the citation-landing effect scrolled #pdf-preview-section into view on every such open. Remove that page-level auto-scroll so the overview stays first; the PDF still targets the cited page inside its canvas. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughDocumentViewer no longer scrolls to the PDF preview when a citation chunk becomes active. A regression test verifies that chunk deep-links preserve the page’s top position. The review ledger records the change and its verification. ChangesDocument viewer scroll behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #9522 (failure). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/document-viewer-shell.dom.test.tsx (1)
188-202: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve that the deep link still targets the cited page.
chunk-1usespage_number: 1, andDocumentVieweralso mounts withinitialPage={1}. If chunk selection or PDF page targeting regresses, this test can still pass because it only checks that page scrolling did not occur. Use a non-default cited page and assert the rendered selected chunk or PDF canvas target.🤖 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 `@tests/document-viewer-shell.dom.test.tsx` around lines 188 - 202, Update the test around DocumentViewer to use a non-default page_number and matching deep-link configuration, then assert that the rendered selected chunk or PDF canvas targets that cited page. Replace the current assertion that only verifies scrolling did not occur, ensuring the test fails if chunk selection or PDF page targeting regresses.
🤖 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 `@tests/document-viewer-shell.dom.test.tsx`:
- Around line 207-210: Replace the asynchronous waitFor negative assertion in
the document viewer test with a direct synchronous assertion after the existing
ready signals. Assert that scrolledIds does not contain "pdf-preview-section" at
that point, preserving the test’s existing setup and readiness flow.
---
Nitpick comments:
In `@tests/document-viewer-shell.dom.test.tsx`:
- Around line 188-202: Update the test around DocumentViewer to use a
non-default page_number and matching deep-link configuration, then assert that
the rendered selected chunk or PDF canvas targets that cited page. Replace the
current assertion that only verifies scrolling did not occur, ensuring the test
fails if chunk selection or PDF page targeting regresses.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1147d6f-c3c7-4861-8d12-1771e0cef9af
📒 Files selected for processing (3)
docs/branch-review-ledger.mdsrc/components/DocumentViewer.tsxtests/document-viewer-shell.dom.test.tsx
💤 Files with no reviewable changes (1)
- src/components/DocumentViewer.tsx
…pen-scroll-e5bf Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Removes automatic PDF-section scrolling so cited document links open at the overview while preserving citation targeting.
Changes:
- Removes chunk-triggered
scrollIntoView. - Adds regression coverage for chunk deep links.
- Records the review in the branch ledger.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/components/DocumentViewer.tsx |
Removes automatic PDF-section scrolling. |
tests/document-viewer-shell.dom.test.tsx |
Tests overview-first citation landing. |
docs/branch-review-ledger.md |
Adds the PR review record. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The new open-at-top DOM test associates Citation proof with document-viewer-shell.dom.test.tsx; the committed adoption manifest drifted and failed both static-pr and unit coverage. Sync the manifest, harden the negative scroll assertion, and teach pre-commit/handoff to regenerate adoption on the same input paths so the drift fails before CI. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
PR #1782 unblock snapshotTip: What blockedRequired CI on prior tip
Root cause: the open-at-top DOM test newly associates Citation proof; the manifest was not regenerated after that test landed / after main syncs. What I fixed
Pushed commits: Required CI statusRe-triggered on Residual risks
No provider-backed gates run. No force-push, rebase, or merge performed. |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Annotate detailPayload as DocumentDetailPayload so chunk deep-link fixtures typecheck. Escalate DsmCompareRemoveLink to location.assign — router.push still left the remove control active with a stuck ?ids= query under Production UI load — and cover the hop with a DOM test. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
PR #1782 CI fix follow-upTip: Failures addressed
Local proof
Required CIRe-triggered on this tip — merge left to you / existing auto-merge once green. |
`Production UI (1)` on run 32473783831 confirmed the review-lens fix — that
step passed — and then went red one test earlier in the same file:
tests/ui-route-coverage.spec.ts:435:7 › DSM comparison renders responsively
and removes a selected diagnosis
TimeoutError: page.waitForURL: Timeout 30000ms exceeded.
waiting for navigation until "domcontentloaded"
This step looked like a plain link and was left alone in the previous commit.
It is not one. `DsmCompareRemoveLink` renders a `next/link` whose `onClick`
calls `preventDefault()` and `stopPropagation()` and then
`window.location.assign(href)`. Before hydration the anchor is a bare
`<a href>`, so a click in that window races two different navigations — the
browser's native one, or React capturing the discrete event and replaying it
once the root hydrates — and neither is guaranteed to leave the URL where the
assertion reads it. Waiting for the handler makes the assign hop the only path
the click can take.
The control's own component doc already records two earlier stalls of this same
hop under Production UI load (`<Link>` soft-nav, then `router.push` on PR
#1782), which is why it uses a full assign at all; this closes the remaining
pre-hydration window rather than changing the hop again.
Not diagnosed from the uploaded trace: artifact download is not available from
this session. If the stall recurs after this, read
`production-ui-diagnostics-<run>-shard1`'s `error-context.md` and `trace.zip`
before changing the navigation itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HoyoNN7nCUVDLaE84k6e6c
Summary
?chunk=(every search/answer open) no longer auto-scroll the page to#pdf-preview-section.scrollIntoViewthe PDF section when a chunk deep-link is present.origin/main(f6b0fac2); only overlapping file was append-onlydocs/branch-review-ledger.md(auto-merged). No product-file conflicts.Verification
node scripts/run-vitest.mjs run tests/document-viewer-shell.dom.test.tsx— 7 passed (including new auto-scroll regression; re-run after main merge)node scripts/run-vitest.mjs run tests/document-section-summary.dom.test.tsx— 8 passednpm run verify:pr-local -- --dry-run --files src/components/DocumentViewer.tsx,tests/document-viewer-shell.dom.test.tsx(selects full lint/typecheck/test/build; not re-run locally — focused DOM coverage is the changed failure class)git merge origin/mainclean;git merge-tree --write-tree origin/main HEADexit 0scrollIntoViewRisk and rollout
Notes
useEffectinDocumentViewertreating anyactiveChunkIdas a reason to scroll to the PDF; search always attacheschunk.Summary by CodeRabbit
Bug Fixes
Tests