Fix large-PDF tree and XREF rendering performance#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves UI responsiveness on very large PDFs by deferring XREF fetching until the XREF tab is activated, virtualizing the XREF row list to avoid rendering tens of thousands of DOM rows, and reducing react-arborist tree rebuilds caused by unstable prop identities.
Changes:
- Defer XREF table fetch until first XREF tab activation and cache results per document.
- Virtualize XREF table rows with fixed-height windowing and overscan spacers.
- Memoize TreePanel selection and node renderer props to avoid unnecessary react-arborist model rebuilds.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/components/XRefTableView.tsx | Gate XREF fetch on activation and virtualize row rendering to keep DOM size bounded. |
| frontend/src/components/XRefTableView.test.tsx | Update tests for deferred fetch behavior and add a virtualization guard test. |
| frontend/src/components/TreePanel.tsx | Stabilize react-arborist props (selection, node renderer) to reduce O(N) rebuilds on rerender. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…can't eager-fetch
5b73f88 to
d43c943
Compare
unidoc-alip
left a comment
There was a problem hiding this comment.
Solid perf work: the activation-gated fetch and the windowed row list both do
what the description claims, the react-arborist prop stabilization is correct
(node.id and data.id are the same value under the default idAccessor), and the
new tests pin the important invariants. One gap in the activation latch:
latchedTabId is never cleared on a document switch, so switching BACK to a
document whose XREF tab was previously opened re-fires an eager background
fetch while the user is on the Object tree - the exact class of work this PR
set out to defer. Also, the description's "known issue" about arrow keys
stopping at the window edge looks stale; the code and 9.11-UNIT-018 implement
cross-window keyboard nav.
Findings
- [nit]
frontend/src/components/XRefTableView.tsx- The PR description lists "arrow-key row navigation now stops at the edge of the visible window" as a known limitation, but the shipped code walks the absolute index, scrolls the target into view, and defers focus (handleRowKeyDown + the pendingFocusRef effect), and 9.11-UNIT-018 asserts ArrowDown crosses the window and focuses the newly rendered row. The description paragraph looks stale from an earlier revision - worth updating so reviewers/QA don't chase a limitation that no longer exists.
|
|
||
| // Derived activation gate: true only for the tabId the user opened XREF on. | ||
| // False in the same render as a tabId change -- no stale-true window. | ||
| const everActive = latchedTabId === tabId; |
There was a problem hiding this comment.
[should-fix] latchedTabId is never cleared when the document changes, so everActive becomes true again as soon as the user switches BACK to a document whose XREF tab they previously opened. The reset effect above has already cleared dataRef, so the fetch effect (deps [tabId, everActive]) fires an eager background fetch of the full xref table while the user is sitting on the Object tab - the invariant this PR establishes is "no xref work until the tab is activated on this document".
Repro: open doc A, open its XREF tab (latches A), switch to doc B, switch back to doc A -> GetXRefTable(A) fires immediately with active=false. On the 129k-entry file that is a ~12 MB fetch + JSON.parse on the main thread on the Object tree.
Suggested fix: clear the latch in the tabId reset effect (setLatchedTabId(null) alongside the dataRef/inFlightRef resets) AND keep the eager same-pass fetch out by additionally gating the fetch on the render-time value, e.g. derive everActive = latchedTabId !== null && latchedTabId === tabId where the reset effect nulls the latch; since the fetch effect closure for that first pass still sees the stale everActive=true, the cleanest fix is to compare against tabIdRef inside the fetch effect: if (latchedTabIdRef.current !== tabId) return; with the latch mirrored to a ref that the reset effect clears synchronously. A unit test mirroring the existing doc-switch test but switching back to the original tabId would lock this in.
There was a problem hiding this comment.
Fixed in f4ca3d6. You're right - the derived gate never cleared latchedTabId, so returning to a document you'd previously opened XREF on made everActive true again and the fetch effect eager-fetched the full table on the Object tab. Rather than clearing it in the reset effect (a render late - the same stale-window class as the previous two rounds), I reset the latch render-phase via React's reset-state-on-prop-change pattern: when tabId changes, setLatchedTabId(null) runs in the SAME render as the switch, so everActive is false both on a switch and on a return visit, with no stale-true window for the fetch effect. Added a regression test (open A + XREF, switch to B, switch back to A on the Object tab, assert GetXRefTable is not called again) and verified it fails without the reset. This also removes the tabId-keyed reliance that caused the earlier iterations - the render-phase reset is the root-cause fix for the whole 'effect-delayed reset leaves a stale window' class.
…ment can't eager-fetch
Two rendering performance fixes that only really bite on very large PDFs. I hit them with the 750-page PDF 32000 spec (~130k xref entries).
Opening a big document froze the whole UI for a while - the tree wouldn't click or even hover. The XREF pane is force-mounted, so on every open it was fetching the full xref table (~12 MB of JSON on that file) and rendering every single row, even while you're sitting on the Object tab. Now the fetch is gated on the tab actually being active, so opening a document does no xref work until you switch to that tab, and the row list is windowed (only the visible slice is in the DOM) the same way the font mapping table already does it. The XREF tab opens and scrolls smoothly instead of hanging.
The tree had a separate problem: react-arborist rebuilds its entire node model whenever any of its props change identity, and the child render function was a fresh closure on every render, so it rebuilt on every render rather than only when the data changed. Memoized the render prop and the selection id and pulled a pure helper out of the component.
Frontend tests all pass, including a new guard that fails if the xref table ever commits more than a bounded window of rows to the DOM. tsc and eslint are clean.
One minor known thing: arrow-key row navigation in the xref table now stops at the edge of the visible window (mouse scrolling is unaffected). Easy to revisit later if anyone leans on keyboard nav there.