diff --git a/frontend/src/components/TreePanel.tsx b/frontend/src/components/TreePanel.tsx index 9374362..2d375b5 100644 --- a/frontend/src/components/TreePanel.tsx +++ b/frontend/src/components/TreePanel.tsx @@ -2,13 +2,26 @@ * @file PDF object-tree panel. Renders the hierarchical document structure * using react-arborist with lazy child loading and cross-reference navigation. */ -import { useState, useRef, useEffect, useCallback } from 'react'; +import { useState, useRef, useEffect, useCallback, useMemo, createContext, useContext } from 'react'; import { useLatest } from '../hooks/useLatest'; import { Tree, type TreeApi, type NodeRendererProps } from 'react-arborist'; import { BookOpen, FolderTree, FileText, FileCode, Image as ImageIcon, Type, type LucideIcon } from 'lucide-react'; import { GetChildren, GetAncestorPath } from '../../bindings/unidoc-pdf-debugger/internal/pdfservice/pdfservice.js'; import { useAppState, useAppDispatch, type TreeNode } from '../hooks/useDocumentState'; +/** + * Per-row transient state (which node is mid-load, which is flashing) delivered + * to NodeRenderer via context rather than props. react-arborist memoizes its + * rows and rebuilds its entire O(N) node model whenever any Tree prop identity + * changes; passing these through the render-prop would force that rebuild on + * every load/flash toggle. A context change instead re-renders only the row + * consumers, leaving the Tree props (and the model) untouched. + */ +const RowStateContext = createContext<{ loadingNodeId: string | null; flashNodeId: string | null }>({ + loadingNodeId: null, + flashNodeId: null, +}); + /** * Map a backend iconHint to a lucide-react icon component. Returns null for * "default" and unknown hints so untyped scalars/arrays render without @@ -116,6 +129,18 @@ function buildInitialData(rootNode: TreeNode, rootChildren: TreeNode[] | null): return [root]; } +/** Map a backend node id to its react-arborist display id by walking the tree. */ +function findDisplayId(data: TreeNodeData[], backendId: string): string | undefined { + for (const n of data) { + if (n.backendId === backendId) return n.id; + if (n.children) { + const found = findDisplayId(n.children, backendId); + if (found) return found; + } + } + return undefined; +} + /** Recursively find a node by ID and replace its children immutably. */ function updateNodeChildren( data: TreeNodeData[], @@ -134,12 +159,14 @@ function updateNodeChildren( } /** Custom row renderer for tree nodes. Handles selection, flash, and error styling. */ -function NodeRenderer({ node, style, dragHandle, isLoading, flashNodeIdRef }: NodeRendererProps & { isLoading?: boolean; flashNodeIdRef?: React.RefObject }) { +function NodeRenderer({ node, style, dragHandle }: NodeRendererProps) { + const { loadingNodeId, flashNodeId } = useContext(RowStateContext); const data = node.data; const isError = data.error !== ''; const isSelected = node.isSelected; const isInternal = node.isInternal; - const isFlashing = data.id === flashNodeIdRef?.current; + const isLoading = data.id === loadingNodeId; + const isFlashing = data.id === flashNodeId; // Hide rawKey when the inline objectRef suffix is rendered. PDFBox-style // rows show "Pages [2 0 R]" rather than "Pages /Pages [2 0 R]" -- the @@ -257,8 +284,7 @@ export function TreePanel() { const selectedNodeIdRef = useLatest(selectedNodeId); const treeDataRef = useLatest(treeData); const treeRef = useRef | undefined>(undefined); - const [, setFlashNodeId] = useState(null); - const flashNodeIdRef = useRef(null); + const [flashNodeId, setFlashNodeId] = useState(null); // Container sizing for react-arborist const containerRef = useRef(null); @@ -433,11 +459,9 @@ export function TreePanel() { payload: { nodeId: targetNode.backendId, label: targetNode.name, rawKey: targetNode.rawKey, iconHint: targetNode.iconHint }, }); - // Flash effect - flashNodeIdRef.current = targetNode.id; + // Flash effect (delivered to rows via RowStateContext) setFlashNodeId(targetNode.id); setTimeout(() => { - flashNodeIdRef.current = null; setFlashNodeId(null); }, 100); @@ -530,18 +554,25 @@ export function TreePanel() { // selectedNodeIdRef is a stable useLatest ref; listed to satisfy exhaustive-deps. }, [dispatch, selectedNodeIdRef]); - // Map backendId to display id for react-arborist's selection prop - function findDisplayId(data: TreeNodeData[], backendId: string): string | undefined { - for (const n of data) { - if (n.backendId === backendId) return n.id; - if (n.children) { - const found = findDisplayId(n.children, backendId); - if (found) return found; - } - } - return undefined; - } - const selectionDisplayId = selectedNodeId ? findDisplayId(treeData, selectedNodeId) : undefined; + // Memoized so the `selection` prop keeps a stable identity across renders that + // change neither the selection nor the tree. react-arborist rebuilds its entire + // O(N) node model whenever ANY prop identity in treeProps changes (see its + // provider's `api.update(treeProps)` memo), so an unstable prop forces a full + // rebuild on every render -- pathological on large trees. + const selectionDisplayId = useMemo( + () => (selectedNodeId ? findDisplayId(treeData, selectedNodeId) : undefined), + [selectedNodeId, treeData], + ); + + // Fully stable child render-prop: an inline arrow (or one keyed on loadingNodeId) + // is a new function whenever it changes, and react-arborist rebuilds its whole + // O(N) model on any Tree prop identity change. Per-row loading/flash now flow + // through RowStateContext instead, so this never needs to change. + const renderNode = useCallback( + (props: NodeRendererProps) => , + [], + ); + const rowState = useMemo(() => ({ loadingNodeId, flashNodeId }), [loadingNodeId, flashNodeId]); return (
@@ -550,6 +581,7 @@ export function TreePanel() {
{dimensions.width > 0 && dimensions.height > 0 && treeData.length > 0 && ( + key={activeTabId ?? ''} ref={treeRef} @@ -569,10 +601,9 @@ export function TreePanel() { width={dimensions.width} height={dimensions.height} > - {(props: NodeRendererProps) => ( - - )} + {renderNode} + )} {navError && (
{ }); // --------------------------------------------------------------------------- -// 9.11-UNIT-015 [P1] AC#2: eager fetch on mount so the parent can populate -// the "XREF (N)" tab label without waiting for first tab activation. The -// `active` prop no longer gates the fetch (revised after 9-12). +// 9.11-UNIT-015 [P1]: the fetch is DEFERRED until the XREF tab is first +// activated. The payload can be very large (a 129k-entry PDF serializes ~12 MB); +// because the pane is force-mounted, an unconditional fetch would JSON.parse +// ~12 MB and render all rows on the main thread on EVERY document open, freezing +// the UI while the user is still on the Object tree. active=false must NOT fetch; +// activation triggers a single fetch. (Supersedes the pre-perf "eager fetch on +// mount regardless of active" behavior.) // --------------------------------------------------------------------------- -describe('9.11-UNIT-015: eager fetch on mount', () => { +describe('9.11-UNIT-015: fetch deferred until activation', () => { beforeEach(() => { vi.clearAllMocks(); mockGetXRefTable.mockResolvedValue(xrefBasic); }); - test('active=false still fetches eagerly', async () => { + test('active=false does not fetch', () => { render(); + // render() flushes mount effects under act; with active=false the fetch + // effect exits synchronously, so no fetch is scheduled -- assert immediately. + expect(mockGetXRefTable).not.toHaveBeenCalled(); + }); + + test('activation after an inactive mount triggers a single fetch', async () => { + const { rerender } = render( + , + ); + expect(mockGetXRefTable).not.toHaveBeenCalled(); + rerender(); await waitFor(() => { expect(mockGetXRefTable).toHaveBeenCalledWith('tab-1'); }); @@ -448,6 +463,131 @@ describe('9.11-UNIT-015: eager fetch on mount', () => { }); expect(mockGetXRefTable).toHaveBeenCalledTimes(1); }); + + test('switching documents while XREF is inactive does NOT eagerly fetch; re-activating fetches the new doc', async () => { + // Distinct data per document so re-activation can be asserted by rendered rows. + mockGetXRefTable.mockImplementation((id: string) => + Promise.resolve(id === 'tab-2' ? xrefSingleInUse : xrefBasic), + ); + const { rerender } = render( + , + ); + await screen.findByTestId('xref-row-objnum-1'); // tab-1 fetched + rendered + expect(mockGetXRefTable).toHaveBeenCalledTimes(1); + + // Switch documents with the XREF tab INACTIVE (parent lands on Object). A + // stale activation must NOT eagerly fetch the new, unopened document. + rerender(); + await new Promise((r) => setTimeout(r, 20)); + expect(mockGetXRefTable).not.toHaveBeenCalledWith('tab-2'); + expect(mockGetXRefTable).toHaveBeenCalledTimes(1); + + // Re-activating XREF on the new document fetches and renders it. + rerender(); + expect(await screen.findByTestId('xref-row-objnum-7')).toBeInTheDocument(); + expect(mockGetXRefTable).toHaveBeenCalledWith('tab-2'); + }); + + test('returning to a previously-opened document on the Object tab does NOT eagerly re-fetch', async () => { + mockGetXRefTable.mockResolvedValue(xrefBasic); + // Doc A: open XREF (latches A). + const { rerender } = render( + , + ); + await screen.findByTestId('xref-row-objnum-1'); + expect(mockGetXRefTable).toHaveBeenCalledTimes(1); + // Switch to doc B on the Object tab (inactive). + rerender(); + // Switch BACK to doc A, still on the Object tab (inactive). The latch must + // have been cleared on the first switch, so returning to A must NOT re-fetch. + rerender(); + await new Promise((r) => setTimeout(r, 20)); + expect(mockGetXRefTable).toHaveBeenCalledTimes(1); // still just the original doc-A fetch + }); +}); + +// --------------------------------------------------------------------------- +// 9.11-UNIT-017 [P1] Perf: the row list is viewport-virtualized -- a large xref +// (a 750-page PDF can carry ~129k entries) must render only a bounded window of +// rows, not one per entry, or the main thread freezes on render. +// --------------------------------------------------------------------------- + +describe('9.11-UNIT-017: row list is virtualized', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + test('a large table renders only a bounded window of rows', async () => { + const big: XRefTableFixture = { + tabId: 'tab-1', + entries: Array.from({ length: 3000 }, (_, i) => ({ + objNum: i + 1, + gen: 0, + status: 'in-use' as const, + offset: (i + 1) * 16, + hostObjStm: 0, + nodeID: `obj:0:${i + 1}`, + })), + }; + mockGetXRefTable.mockResolvedValue(big); + const { container } = render( + , + ); + await waitFor(() => { + expect(container.querySelectorAll('[data-testid^="xref-row-objnum-"]').length).toBeGreaterThan(0); + }); + // Only a small window commits to the DOM (jsdom viewport fallback ~= 36 + // rows), never all 3000. A spacer reserves the off-window scroll height. + const rendered = container.querySelectorAll('[data-testid^="xref-row-objnum-"]').length; + expect(rendered).toBeGreaterThan(0); + expect(rendered).toBeLessThan(200); + expect(container.querySelectorAll('tr[aria-hidden="true"]').length).toBeGreaterThan(0); + }); +}); + +// --------------------------------------------------------------------------- +// 9.11-UNIT-018 [P1] AC#4 + virtualization: ArrowDown past the rendered window +// scrolls the next row into view and focuses it. DOM-sibling focus cannot work +// here because off-window rows are unmounted; the handler walks the index. +// --------------------------------------------------------------------------- + +describe('9.11-UNIT-018: keyboard nav crosses the virtualization window', () => { + beforeEach(() => { + vi.clearAllMocks(); + const big: XRefTableFixture = { + tabId: 'tab-1', + entries: Array.from({ length: 3000 }, (_, i) => ({ + objNum: i + 1, + gen: 0, + status: 'in-use' as const, + offset: (i + 1) * 16, + hostObjStm: 0, + nodeID: `obj:0:${i + 1}`, + })), + }; + mockGetXRefTable.mockResolvedValue(big); + }); + + test('ArrowDown from the last in-window row focuses the next (initially unrendered) row', async () => { + render(); + // Row 36 is at the edge of the initial ~36-row window; row 37 is not rendered. + const row36 = await screen.findByTestId('xref-row-36'); + expect(screen.queryByTestId('xref-row-37')).toBeNull(); + row36.focus(); + fireEvent.keyDown(row36, { key: 'ArrowDown' }); + // Row 37 must scroll in AND receive focus. + const row37 = await screen.findByTestId('xref-row-37'); + await waitFor(() => expect(document.activeElement).toBe(row37)); + }); + + test('ArrowUp at the top row does not wrap', async () => { + render(); + const row1 = await screen.findByTestId('xref-row-1'); + row1.focus(); + fireEvent.keyDown(row1, { key: 'ArrowUp' }); + // No wrap: focus stays on row 1. + expect(document.activeElement).toBe(row1); + }); }); // --------------------------------------------------------------------------- diff --git a/frontend/src/components/XRefTableView.tsx b/frontend/src/components/XRefTableView.tsx index 8634d0c..44a86f8 100644 --- a/frontend/src/components/XRefTableView.tsx +++ b/frontend/src/components/XRefTableView.tsx @@ -36,21 +36,57 @@ export interface XRefTableViewProps { onLoaded: (count: number) => void; } +/** Fixed row height (px) for windowing math; each rendered row is pinned to this + * height so the spacer geometry matches the true scroll height. */ +const XREF_ROW_HEIGHT = 28; +/** Rows rendered above/below the viewport for smooth scrolling. */ +const XREF_OVERSCAN = 12; + /** * Document-level XREF table view. Lazy-fetches on first activation; data is * cached in component state for the lifetime of the document (tabId change - * resets state). + * resets state). The row list is viewport-virtualized (only the visible slice + * is committed to the DOM) so a large xref -- a 750-page PDF can carry ~129k + * entries -- does not freeze the UI on render. Mirrors the FontMappingTable / + * PlainTextView windowing pattern. */ -export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: XRefTableViewProps) { +export function XRefTableView({ tabId, active, onNavigate, onLoaded }: XRefTableViewProps) { const [data, setData] = useState(null); const [loading, setLoading] = useState(false); const [showLoading, setShowLoading] = useState(false); const [error, setError] = useState(null); - // Cache flags as refs so the fetch effect only re-runs when tabId / active + // latchedTabId records the tabId the user actually activated the XREF tab on. + // The fetch gates on the DERIVED `everActive = latchedTabId === tabId` (below), + // NOT a boolean+reset: a boolean reset in an effect is applied a render late, + // so the fetch effect could observe a stale `everActive=true` on the first + // render after a document switch and eagerly fetch a document the user never + // opened XREF on. The derived value is false in the same render as the switch. + const [latchedTabId, setLatchedTabId] = useState(null); + // Mirrors the tabId this component last rendered for, so the latch can be reset + // render-phase the instant the document changes (below). + const [seenTabId, setSeenTabId] = useState(tabId); + // Cache flags as refs so the fetch effect only re-runs when tabId / everActive // change. Including data/loading in deps creates a stale-fetch race where // the cleanup from the loading-state re-render cancels the in-flight call. const dataRef = useRef(null); const inFlightRef = useRef(false); + // Current tabId mirrored to a ref so the activation latch can read it WITHOUT + // taking tabId as a dependency -- otherwise the one-render stale `active=true` + // after a document switch (the parent resets its detail tab a render late) + // would re-latch the new tabId and trigger an eager fetch. + const tabIdRef = useRef(tabId); + tabIdRef.current = tabId; + + // Render-phase reset (React's "reset state when a prop changes" pattern): the + // moment tabId changes, forget the previous activation. This runs in the SAME + // render as the switch -- unlike an effect, which lands a render late -- so a + // document the user previously opened XREF on can never return with a stale + // latch and eager-fetch its ~12 MB table while sitting on the Object tab. + if (seenTabId !== tabId) { + setSeenTabId(tabId); + setLatchedTabId(null); + } + const onLoadedRef = useRef(onLoaded); useEffect(() => { onLoadedRef.current = onLoaded; }, [onLoaded]); @@ -65,12 +101,34 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: inFlightRef.current = false; }, [tabId]); - // Eager fetch on tabId change so the parent can render the "XREF (N)" - // tab label without waiting for first activation. Stale-fetch guard via - // `cancelled`. The `active` prop is intentionally NOT a gate -- the xref is - // cheap to extract from pdfcpu's already-parsed state. + // Latch the current tabId on a genuine activation (active false->true). Keyed + // on `active` only (tabId via ref): a document switch does not re-run this, so + // the transient stale `active=true` that lingers one render after a switch + // cannot latch the new tabId or trigger an eager fetch. A real re-activation + // of the XREF tab on the new document latches it and lets the fetch proceed. + useEffect(() => { + if (active) setLatchedTabId(tabIdRef.current); + }, [active]); + + // Derived activation gate: true only for the tabId the user opened XREF on. + // The render-phase reset above nulls the latch on any tabId change, so this is + // false in the same render as a switch AND on a return visit to a + // previously-opened document -- no stale-true window for the fetch effect. + const everActive = latchedTabId === tabId; + + // Fetch on FIRST activation of the XREF tab, then cache for the document's + // lifetime. Gated on `everActive` (not `active`): the payload can be very + // large (a 750-page, 129k-entry PDF serializes ~12 MB of JSON), and this pane + // is force-mounted, so an unconditional fetch would JSON.parse ~12 MB + render + // 129k rows on the main thread on EVERY document open -- freezing the UI while + // the user is still on the Object tree. Deferring to activation keeps open + // instant; the "XREF (N)" count label populates on first XREF open instead of + // on load. Because `active` is not a dependency, toggling the tab off/on + // mid-flight cannot start a duplicate fetch; `cancelled` only guards a tabId + // (document) change. useEffect(() => { if (!tabId) return; + if (!everActive) return; if (dataRef.current !== null) return; if (inFlightRef.current) return; inFlightRef.current = true; @@ -94,13 +152,13 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: inFlightRef.current = false; }); return () => { - // Clear inFlightRef in the cleanup too: the .then/.catch branches return - // early when cancelled, so without this the slot stays "in flight" - // forever and a re-activation under the same tabId would be blocked. + // Clear inFlightRef so a StrictMode remount or a tabId reset can fetch + // again; the .then/.catch branches return early when cancelled. An + // active-toggle never reaches here because `active` is not a dependency. cancelled = true; inFlightRef.current = false; }; - }, [tabId]); + }, [tabId, everActive]); // 200ms loading debounce -- mirrors the showContentStreamLoading pattern in // DetailPanel.tsx. @@ -113,6 +171,57 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: return () => clearTimeout(timer); }, [loading]); + // --- Viewport virtualization (see class doc). The scroll container mounts + // only once `data` is present (after the early returns below), so the measure + // and reset effects key on `data` rather than `[]`. --- + const scrollRef = useRef(null); + const [scrollTop, setScrollTop] = useState(0); + const [viewportHeight, setViewportHeight] = useState(0); + // Row index to focus after the window updates. Because virtualization unmounts + // off-window rows, arrow-key navigation cannot walk DOM siblings; instead it + // scrolls the target index into view and focuses it here once it is rendered. + const pendingFocusRef = useRef(null); + const [focusTick, setFocusTick] = useState(0); + + useEffect(() => { + const el = scrollRef.current; + if (!el) return; + setViewportHeight(el.clientHeight); + if (typeof ResizeObserver === 'undefined') return; + const ro = new ResizeObserver(() => setViewportHeight(el.clientHeight)); + ro.observe(el); + return () => ro.disconnect(); + }, [data]); + + const handleScroll = useCallback((e: React.UIEvent) => { + setScrollTop(e.currentTarget.scrollTop); + }, []); + + // Reset the window to the top when the document (row set) changes, so a stale + // scrollTop never slices past the end of a shorter table. + useEffect(() => { + if (scrollRef.current) scrollRef.current.scrollTop = 0; + setScrollTop(0); + }, [data]); + + // Focus the pending arrow-key target once the window has updated to include it. + // Runs on focusTick (a key press) and scrollTop (the window shifted); if the + // row is not yet rendered, the pending index survives to the next run. + useEffect(() => { + const target = pendingFocusRef.current; + if (target === null) return; + const entry = dataRef.current?.entries?.[target]; + if (!entry) { + pendingFocusRef.current = null; + return; + } + const row = scrollRef.current?.querySelector(`[data-testid="xref-row-${entry.objNum}"]`); + if (row) { + row.focus(); + pendingFocusRef.current = null; + } + }, [focusTick, scrollTop]); + /** Click handler: free rows are no-ops; in-use / in-objstm dispatch navigation. */ const handleRowClick = useCallback( (entry: XRefEntryData) => { @@ -125,20 +234,30 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: /** * Keyboard handler per AC4: ArrowDown / ArrowUp move row focus (no wrap); - * Enter / Space activate non-free rows. + * Enter / Space activate non-free rows. `index` is the row's absolute position + * in the full entry list. Because the table is virtualized, moving focus walks + * the index (not DOM siblings, which would hit spacer rows or unmounted rows): + * it scrolls the target into view and defers the focus to the effect above. */ const handleRowKeyDown = useCallback( - (e: React.KeyboardEvent, entry: XRefEntryData) => { - if (e.key === 'ArrowDown') { + (e: React.KeyboardEvent, index: number, entry: XRefEntryData) => { + if (e.key === 'ArrowDown' || e.key === 'ArrowUp') { e.preventDefault(); - const next = e.currentTarget.nextElementSibling as HTMLTableRowElement | null; - if (next) next.focus(); - return; - } - if (e.key === 'ArrowUp') { - e.preventDefault(); - const prev = e.currentTarget.previousElementSibling as HTMLTableRowElement | null; - if (prev) prev.focus(); + const total = dataRef.current?.entries?.length ?? 0; + const target = e.key === 'ArrowDown' ? index + 1 : index - 1; + if (target < 0 || target >= total) return; // no wrap + const el = scrollRef.current; + if (el) { + // Ensure the target row is inside the scroll viewport so it renders in + // the next window; the effect then focuses it. + const rowTop = target * XREF_ROW_HEIGHT; + const rowBottom = rowTop + XREF_ROW_HEIGHT; + if (rowTop < el.scrollTop) el.scrollTop = rowTop; + else if (rowBottom > el.scrollTop + el.clientHeight) el.scrollTop = rowBottom - el.clientHeight; + setScrollTop(el.scrollTop); + } + pendingFocusRef.current = target; + setFocusTick((t) => t + 1); return; } if (e.key === 'Enter' || e.key === ' ') { @@ -182,8 +301,22 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: return
; } + const entries = data.entries ?? []; + const totalRows = entries.length; + const firstVisible = Math.max(0, Math.floor(scrollTop / XREF_ROW_HEIGHT) - XREF_OVERSCAN); + // A zero clientHeight (jsdom / pre-measure) falls back to a bounded default so + // the window stays small rather than rendering every row. + const visibleCount = Math.ceil((viewportHeight || 320) / XREF_ROW_HEIGHT) + XREF_OVERSCAN * 2; + const lastVisible = Math.min(totalRows, firstVisible + visibleCount); + const rowsToRender = entries.slice(firstVisible, lastVisible); + return ( -
+
{/* `position: sticky` on is unreliable on WebKit2GTK and older Safari/WebKit; apply sticky on each - {(data.entries ?? []).map((entry) => { + {/* Top spacer reserves the scroll height of the rows above the window. */} + {firstVisible > 0 && ( + + + )} + {rowsToRender.map((entry, i) => { + const index = firstVisible + i; const isFree = entry.status === 'free'; const isInObjStm = entry.status === 'in-objstm'; const rowClass = [ @@ -217,11 +357,12 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: handleRowClick(entry)} - onKeyDown={(e) => handleRowKeyDown(e, entry)} + onKeyDown={(e) => handleRowKeyDown(e, index, entry)} > ); })} + {/* Bottom spacer reserves the scroll height of the rows below. */} + {lastVisible < totalRows && ( + + + )}
so the header sticks on every @@ -199,7 +332,14 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }:
{entry.objNum} @@ -245,6 +386,12 @@ export function XRefTableView({ tabId, active: _active, onNavigate, onLoaded }: