fix: popover position:fixed, miss/not_found ghost scale-up

[bensonwong]

Summary

• Popover scroll fix (Popover.tsx): Restore position:fixed on the popover wrapper so it stays anchored to the viewport instead of scrolling with the page. With position:fixed, the popover is always in viewport space — the old scroll-container-relative coordinate math (x - containerRect.left + container.scrollLeft) reduces to a direct passthrough. The portal detection logic is simplified from ~35 lines of Radix/scroll-ancestor-walking to a 2-line [data-dc-portal-root] ?? document.body lookup; scroll ancestry is now irrelevant.
• Ghost scale-up fix (viewTransition.ts): buildGhostTargetFromViewport (the miss/not_found fallback) previously set ghostRect = visibleRect, causing the ghost to scale up to match the expanded page image dimensions. Now mirrors buildGhostTarget: keeps ghost at source keyhole dimensions (snapshot.viewportRect) and aligns the image center-of-mass over the visible page center — pure translate, no scale.
• Test update (popoverScrollStability.spec.tsx): Flip scroll-stability assertions from "Y shifts by ~scrollAmount" to "Y delta ≤ 5px". Add a direct toHaveCSS("position", "fixed") assertion to prove the mechanism (not just the symptom — a popover portalled to document.body with position:absolute would also appear viewport-stable in the test's scroll-container setup).
• Comment (Popover.tsx): Document the assumption that [data-dc-portal-root] must be present before mount for the z-index stacking guarantee to hold.

Test plan

• Open a citation popover and scroll the page — popover should stay fixed at its viewport position, not drift with the scroll
• Trigger a miss/not_found page-expand — ghost should slide from keyhole to the page at constant size (no scale-up flash)
• npm run test:ct — popoverScrollStability spec passes with the new position:fixed assertion
• npm run build passes
• npm run lint passes

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

4 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
agui-chat-deepcitation	Ignored	Preview	Apr 12, 2026 1:34am
deepcitation-langchain-rag-chat	Ignored	Preview	Apr 12, 2026 1:34am
mastra-rag-deepcitation	Ignored	Preview	Apr 12, 2026 1:34am
nextjs-ai-sdk-deepcitation	Ignored	Preview	Apr 12, 2026 1:34am

[claude]

Code Review

Overall this is a solid, well-reasoned simplification. Both fixes address real bugs (popover drift on scroll, ghost scale-up on miss/not_found), the comments document the tradeoffs clearly, and the test correctly pivots from testing the symptom to testing the mechanism. A few things worth looking at before merging:

---

Popover.tsx — Breaking change concern

The old behavior was explicitly documented as intentional ("popover scrolls with the content") and the scroll-stability spec was written to verify it. This PR inverts that contract.

Any consumer that relies on the popover tracking document scroll (not viewport) will regress silently — they won't see a compile error and the behavior looks fine until they actually scroll. The PR description says "Restore position:fixed" which implies there was a prior regression, but there's no reference to the commit or PR where position:absolute was introduced. Worth verifying that lineage so reviewers can confirm this is a regression fix and not a UX reversal.

Suggested: Add a brief note to the PR (or a code comment) linking to the commit where position:absolute was introduced, so the history is clear.

---

useLayoutEffect(…, []) — Empty dep array is correct but fragile

}, []); // run once on mount — portal target is stable for the session

This is correct given the stated assumption. The comment documents the tradeoff well. One edge case: if CitationComponent mounts during SSR hydration before [data-dc-portal-root] is in the DOM (e.g., streamed HTML where the portal root appears after the citation), the effect runs once, finds nothing, and falls back to document.body permanently for that instance. Since position:fixed works fine with document.body, this is functionally correct — but the z-index stacking guarantee is quietly lost and there's no warning.

Consider: A console.warn in dev mode when [data-dc-portal-root] is absent but the consumer appears to expect it (e.g., some detectable signal). Low priority, but would help catch misconfigurations early.

---

buildGhostTargetFromViewport — Ghost rect can extend off-screen

const ghostRect = new DOMRect(pageCX - anchorInGhostX, pageCY - anchorInGhostY, srcW, srcH);

If anchorInGhostX > pageCX (keyhole is wider than the visible page area and scrolled right), ghostRect.left goes negative — the ghost starts partially off-screen. This mirrors buildGhostTarget exactly, so the behavior is at least consistent with the hit-annotation path. But in the miss/not_found fallback, where pageCX is the center of the visible page area (potentially a narrow stripe), this is more likely to produce a large negative left.

The ghost is position:fixed so negative left is safe visually (clipped by viewport, not by a scroll container), and the scale/translate math in runPageExpandGhostAnimation handles this. Just worth verifying the animation still looks correct when the ghost origin starts off the left viewport edge.

---

Missing test for the ghost fix

The ghost scale-up fix has no dedicated test. The popover scroll stability spec was updated to reflect the new behavior, but there's no spec for buildGhostTargetFromViewport returning a ghostRect at keyhole dimensions rather than visibleRect dimensions.

Given that the animation math is non-trivial (and the bug was subtle — ghost silently scaled to page dimensions), a unit test for buildGhostTargetFromViewport would be a good safety net:

// expected: ghostRect.width === snapshot.viewportRect.width (not visibleRect.width)
// expected: ghostRect.height === snapshot.viewportRect.height

Not blocking, but this is the kind of regression that's easy to re-introduce.

---

Minor: Old triggerRef.current dep array comment is gone — good catch

The removed comment // triggerRef is a stable ref; open re-runs detection on each popover open was describing [triggerRef.current] as a dep, which is a React anti-pattern (.current mutations don't trigger effects). Removing it along with the complex detection logic is the right call.

---

Test update looks correct

The toHaveCSS("position", "fixed") assertion is the right defensive addition — it tests the mechanism, not just the symptom. Without it, a position:absolute portal to document.body (outside the test's scroll container) would also appear viewport-stable, giving a false pass. This is a good pattern.

The Y-delta tolerance of ≤ 5px for fixed positioning is reasonable — accounts for sub-pixel browser rendering differences.

---

Summary: The core logic of both fixes is correct and the implementation is clean. The main things I'd want addressed before merging are (1) confirming the position:absolute → position:fixed revert has a paper trail, and (2) adding a ghost-rect dimension assertion to catch future regressions on the buildGhostTargetFromViewport fix.

[github-actions]

✅ Playwright Test Report

Status: Tests passed

📊 Download Report & Snapshots (see Artifacts section)

What's in the Visual Snapshots

The gallery includes visual snapshots for:

• 🖥️ Desktop showcase (all variants × all states)
• 📱 Mobile showcase (iPhone SE viewport)
• 📟 Tablet showcase (iPad viewport)
• 🔍 Popover states (verified, partial, not found)
• 🔗 URL citation variants

---

Run ID: 24295968335