refactor: migrate old grid.js into a new reusable ResultGrid component

[emrberk]

Refactor: reusable ResultGrid component

Extracts the result grid into a self-contained, framework-native React package and
drives the console's Result scene through it, replacing the legacy imperative grid.

The problem

• The console's grid lived in src/js/console/grid.js — a large, imperative,
  pre-React module that manipulates the DOM directly. It's hard to read, hard to
  test, and can't drop into a React tree as a component.
• Notebooks need a grid too. This grid began as an inline implementation on the
  notebooks branch (feat/notebooks); rather than leave it there as a notebook-only
  grid drifting alongside the console's — a second lineage to maintain — we extracted
  it into a neutral, reusable component. That extraction is this PR.
• There was no clean seam between grid behaviour (selection, keyboard nav, freeze,
  resize) and app concerns (data fetching, persistence, telemetry), so neither
  could evolve independently.

How this resolves it

A new src/components/ResultGrid/ package — one neutral, app-agnostic grid built on
TanStack Table (columns, pinning, ordering) and TanStack Virtual (row and
column virtualization, so both axes stay performant on wide, deep results).

• Self-contained behaviour. The grid owns grid-only concerns: cell/keyboard
  navigation, copy cell & column, freeze-left, move-column-to-front, reset layout,
  debounced column resize, scroll shadows, markdown export. It never imports app
  state — everything app-specific arrives through props.
• Pluggable data via ResultGridDataSource. The grid reads rows through a small
  interface (getRow, rowCount, sampleRows, onVisibleRowsChange) instead of
  owning a dataset. Two implementations ship:
  • inMemoryDataSource — bounded, fully-loaded results (notebooks).
  • usePagedDataSource — server-paged source for the console's unbounded results:
    sparse page cache, debounced fetch windows, outlier-page purging, and a
    generation guard so a slow page from a previous query can never apply to the
    current one.
• Tested in isolation. The pure helpers (inlineGridUtils, resultPageMarkdown,
  virtualRowMapping, columnLayoutStore, nextPageWindow, pageFetchPlan) have
  unit tests; grid.spec.js e2e coverage is expanded.

How it integrates with the console

• ResultGridAdapter implements the legacy IQuestDBGrid interface (setData,
  focus, show/hide, getSQL, addEventListener, clearCustomLayout,
  shuffleFocusedColumnToFront, toggleFreezeLeft, getResultAsMarkdown). The
  Result scene and toolbar keep calling the same surface — toolbar actions,
  selection state, editor focus-yield, and freeze state are bridged through it via
  CustomEvents, so existing call sites are untouched.
• Persistence is preserved. Column layout (sizing / order / pinned) is stored
  via columnLayoutStore, keyed by the result's column set.
• Behind a feature flag. useNewGrid (feature.new.grid, default on)
  selects the new grid; the legacy grid.js path remains available with
  useNewGrid=false (also toggleable via the ?useNewGrid= query param) as a safe
  fallback while the new grid bakes in production.

Next plans

• Notebooks (single source of truth). Land this package on main, merge main
  into feat/notebooks (the grid directory is purely added — minimal conflicts), then
  repoint InlineResultTable.tsx to the package via inMemoryDataSource and delete
  the now-superseded inline grid. See src/components/ResultGrid/NOTEBOOKS_MIGRATION.md
  for the checklist. Optionally persist freeze/order per cell there too.
• Phased removal of the legacy grid. The next release ships both grids — the new
  one on by default, ?useNewGrid=0 as a safeguard to fall back to grid.js. We
  remove grid.js and the useNewGrid flag in the release after that, once the new
  grid has proven itself in production.

Benchmark

The detailed benchmark results between the legacy grid and the new ResultGrid and the steps to reproduce are in #569

[emrberk]

PR #568 Review — refactor: migrate old grid.js into a new reusable ResultGrid component

Scope: Critical issues on the real user path only (per request — not interested in minors or NITs).
Branch reviewed: refactor/result-grid (PR #568) vs main.

---

Issues

No critical issues found on the real user path.

The full real-user flow was traced — run query → scroll/paginate → keyboard nav → copy cell/column → freeze/reorder/resize/reset → layout persistence → re-run/refresh → markdown copy/download. Nothing rises to a blocking, real-path critical bug.

Review coverage: 3 focused subagents (pagination/data-source, frozen-column & virtualization rendering, React state/effects) plus a manual trace of the data-source, rendering, and state machinery.

---

False-positives

Candidates surfaced during review that were verified as not critical (or not real), with reasoning. Listed for transparency so the dispositions can be sanity-checked.

Category	Candidate	Why it's not a critical finding
State management & async	Keyboard-resize commit (200ms debounce) races a same-query re-run: setData reads localStorage before the pending commitSizingDebounced timer fires, so the run restores the pre-resize width. ResultGrid.tsx:599-617, :629-641	Reachability is effectively nil: requires focusing a resizer separator, pressing arrows, then triggering a re-run (Ctrl+Enter in the editor or clicking Refresh) within 200ms of the last arrow press — a focus transition + keypress no human does that fast. Even if hit, the timer still fires and persists the new width correctly; only the current run shows the old width and it self-heals next run. No data loss, cosmetic, niche. Adjacent to the already-accepted "unmount-after-debounce won't fix".
React correctness	1-frame flash: runToken reset (setFocusedCell(null)) is a post-paint useEffect while layout restore is a pre-paint useLayoutEffect, so a query switch can paint one frame with the previous query's cell highlight. ResultGrid.tsx:559-568	Single-frame cosmetic flash that self-corrects on the next tick; no wrong data, no stuck state. Below the critical bar.
Performance / correctness	{count && (...)} renders a stray 0 for empty result sets (falsy-number render). index.tsx:447	Pre-existing on main (identical at main:src/scenes/Result/index.tsx), not introduced by this PR, and only a minor toolbar glyph. Out of scope.
Pagination data-source	Wrong-page data / off-by-one in planPageFetch pair-split or the limit: lo+1,hi math; cross-query stale response landing in a new query; stuck/blank pages after scroll.	Verified correct: limit translation matches QuestDB's 1-based-inclusive contract; pair fetch splits exactly at PAGE_SIZE; the resultGenerationRef guard drops stale in-flight responses; setResult clears the debounce timer; window/purge bookkeeping self-heals. 50k randomized async interleavings + boundary-straddle cases produced zero defects (e2e suite asserts the 2000/3000 boundary).
Rendering / freeze	A cell showing the wrong column's value after freeze col 0 + move col 2 to front, or frozen/center column overlap/gap.	Verified correct: value and column meta always come from the same headers[colIdx] object; visualLeafIds matches the two render loops exactly; center columns start precisely at getLeftTotalSize() (no measureElement drift), so no overlap/gap; designated-timestamp coloring is computed in data-index space and stays reorder-safe.

---

Quality checks

Check	Result
yarn typecheck	✅ pass (exit 0)
yarn test:unit	✅ pass (exit 0)
yarn lint	✅ pass (exit 0)
yarn build	✅ pass (exit 0)

---

Summary

Verdict: Approve (no blocking issues).

• The real user path is correct. Server-paging (generation guard, page-split math, window/purge), frozen-column + dual-axis virtualization, cell→data mapping after reorder/freeze, layout persistence, and keyboard nav all hold up under adversarial tracing.
• Quality checks: typecheck, unit tests, lint, and build all pass.
• 5 draft candidates considered, 0 confirmed critical, 5 downgraded (2 cosmetic/niche edge cases, 1 pre-existing minor, 2 verified-correct false alarms).

Prior dispositions respected (not re-reported)

• No AbortController on page fetches — accepted.
• Leap-tail mapping (middle rows unreachable >333k) — by design.
• Legacy grid.js / useNewGrid=false path — no-fix.
• Toolbar-click clearing cell selection — intended.
• Every column resizer being a tab stop — intended.
• No auto-focus of a cell on query run — intended main-parity.
• Debounced page load after unmount — won't fix.
• Clipboard textarea-fallback on http:// — not reproducible.

[emrberk]

PR #568 Review — refactor: migrate old grid.js into a new reusable ResultGrid component

Review level 3 — full mission-critical pass (all 13 agents incl. fresh-context adversarial, per-finding verification). Branch refactor/result-grid vs main.

PR title & description

Title follows Conventional Commits (refactor: …) ✓. Description is implementation-heavy but acceptable for a refactor; it clearly documents the useNewGrid flag (default on), the ?useNewGrid= override, and the legacy fallback. No issues.

Quality checks

Check	Result
yarn typecheck	✅ pass (exit 0)
yarn lint	✅ pass (exit 0)
yarn test:unit	✅ pass (exit 0)
yarn build	✅ pass (exit 0)

Issues

#	Issue	Category	Severity	Location	Description / Repro / Fix
1	headerSignature string rebuilt every render (incl. vertical scroll)	Performance (Agent 6)	Minor	in-diff ResultGrid.tsx:621	`virtualColumns.map(...).join("
2	Transient page-fetch error leaves ~1000 rows blank until re-scroll	Query execution (Agent 1)	Minor	in-diff usePagedDataSource.ts, index.tsx:183-197	On a paginationFn error the page stays unloaded while the window pointers already advanced, so nextPageWindow returns "unchanged" for small nudges and doesn't re-request until a scroll threshold is crossed. An error toast is shown and it self-heals → Minor. Fix: roll back loPage/hiPage (or mark the page retry-eligible) on failure so the next onVisibleRowsChange re-plans it.
3	Already-loaded seed page evicted on deep jump	Query execution (Agent 1)	Minor	in-diff usePagedDataSource.ts (loadPages → purgeOutlierPages)	loadPages purges pages outside [loPage,hiPage] before fetching, deleting the in-hand seed page 0 on a deep jump; scrolling back re-fetches it (fine online, blank while offline). Fix: exempt/pin page 0 in the cache.
4	Virtualized-grid ARIA index inconsistencies	Accessibility (Agent 10)	Minor	in-diff ResultGrid.tsx	(a) Cell id/aria-activedescendant use the virtual row index while aria-rowindex uses the absolute index — divergent in the leap-tail beyond ~333k rows. (b) aria-activedescendant blanks when the focused cell scrolls out of the virtual window. Fix: drive cell id/focus off the absolute index; keep the focused cell rendered (expand overscan around focusedCell).
5	Resizer separators have no visible focus indicator	Accessibility (Agent 10)	Minor	in-diff styles.ts (ColResizer)	Resizers are tabIndex=0 tab stops but the ::after bar shows only on :hover, not :focus — keyboard users can't see which resizer is focused. Fix: add a :focus/:focus-visible indicator to ColResizer.
6	Minor a11y polish	Accessibility (Agent 10)	Minor	in-diff ResultGrid.tsx, useGridKeyboardNav.ts	No Escape to clear selection; the header copy button announces only "Copy to clipboard" (no column context); copy-success/fetch-error have no aria-live region; confirm the freeze toggle exposes aria-pressed. Fix: add Escape handling, a column-specific aria-label, a visually-hidden aria-live region, and aria-pressed on the toggle.
7	Wide column keyboard-scroll hides left edge under frozen band	Accessibility/UX (Agent 13)	Minor	in-diff useGridKeyboardNav.ts:49-52	For a column wider than clientWidth - frozenWidth, the right-align branch puts the cell's left edge under the frozen band, hiding the start of the value. Fix: left-align into the unfrozen region (scrollLeft = cellLeft - frozenWidth) when the column exceeds the scrollable width.
8	pulseAnim opacity hack + odd hue transition	Styling (Agent 7)	Minor	in-diff styles.ts (pulseAnim)	${theme.color.yellow}00 fakes transparency by string-appending to a hex (breaks if the token becomes rgb()/named); it also transitions cyan→yellow and has no 100% frame so the shadow snaps off. Fix: explicit rgba(...,0) endpoint, single hue, and a 100% keyframe.
9	DQL result shape duplicated 3×; over-broad barrel exports	Code structure (Agent 8)	Minor	in-diff types.ts:13, pagedSource.ts, ResultGridAdapter.tsx:35	The DQL-seed shape exists as DqlQueryResult (exported, zero consumers), SeedResult, and DqlResultInput; the index.ts barrel also re-exports many symbols nothing outside the package consumes. Fix: consolidate onto DqlQueryResult; trim the barrel to the intended public API.
10	Redundant stopRunning() dispatch reads backwards	Async/code clarity (Agents 3, 13)	Minor	in-diff index.tsx:194-196	if (runningRef.current === RunningType.NONE) dispatch(stopRunning()) is a no-op in the branch where it runs and is correctly skipped when a query is in flight. Behavior is correct (it is the intended fix vs main), but the code invites a future re-break. Fix: delete the dead dispatch or add a clarifying comment.

False-positives (verified, not real / not blocking)

Category	Candidate	Why dismissed
React correctness (Agents 2, 4)	Parent's one-time [] effect attaches listeners to a null gridRef (the child adapter populates it).	React 17: the child commits and useImperativeHandle assigns before any parent passive effect runs, and the adapter's handle is outside the hasData guard. Listeners attach to the real handle.
React correctness (Agent 2)	usePagedDataSource callbacks capture a stale paginationFn/helpers.	Every referenced value is stable (useCallback([]); paginationFn is stable and reads runningRef). Nothing goes stale.
React correctness (Agent 2)	visualLeafIds/columnOffsets memo deps omit headers → stale.	Column ids are invariant to sizing; columnOffsets includes columnSizing. Not stale.
State/async (Agent 13)	paginationFn error handler is an inverted/regressed stopRunning.	It is a fix vs main (which unconditionally stopped, killing a fresh foreground query when a background page errored). Listed above only as a Minor clarity item.
Persistence (Agent 5)	columnLayoutStore LRU breaks on integer-like keys; collisions restore the wrong layout.	The k prefix guarantees no array-index-like keys (0 across 200k generated sets); collision among ≤50 entries ≈ 2.85e-7, layout-only, user-resettable; quota/corruption are try/caught.
Cross-context (Agent 12)	Legacy grid.js lacks removeEventListener → listeners leak.	Negligible: the effect has [] deps, there is no StrictMode, so cleanup runs only at scene unmount when the grid is discarded; legacy path is behind useNewGrid=false and slated for removal.
Security (Agent 11)	XSS via unescapeHtml(formatCellValue(...)).	No HTML sinks anywhere in the new grid; all values render as React text children (re-escaped). limit is numeric + encodeURIComponent; no secrets/PII in localStorage or telemetry; the ?useNewGrid= replaceState has no redirect vector.
React/Code (Agents 2, 8)	Two useEffect([result]) both call setCount (redundant render).	Pre-existing on main (both effects exist there); not introduced by this PR.
State (Agent 4)	LocalStorageProvider context value is unmemoized.	Pre-existing; setUseNewGrid fires only on a deliberate settings toggle, not a hot path — the PR does not worsen it.
Performance (Agents 2, 6)	setData's state updates do not batch in React 17 → multiple grid renders per query.	Real but performance-only and user-paced (gated by hasData); not a correctness bug.

Summary

Verdict: Approve. No critical or data-integrity defects. The core machinery — server-paging (generation guard, 1-based limit translation, pair-split at PAGE_SIZE, debounce/purge), dual-axis virtualization, cell→data mapping after reorder/freeze, designated-timestamp coloring, and layout persistence — was traced adversarially and holds up. All four quality gates pass.

• All findings are Minor (performance, accessibility, blank-on-error edge cases, styling, code structure). They are good follow-ups, not blockers — especially with the useNewGrid=false fallback shipping alongside.
• Verification tally: ~30 draft candidates across 13 agents → 10 findings confirmed (all Minor), 10 dismissed as false-positives, plus 2 noted as pre-existing/out-of-scope.
• In-diff vs out-of-diff: 10 in-diff, 0 confirmed out-of-diff. Agent 12 walked every consumer of the changed symbols (ColumnDefinition +dim/elemType, RawDqlResult +timestamp, IQuestDBGrid +removeEventListener?, useLocalStorage +useNewGrid, StoreKey.USE_NEW_GRID, Button.onMouseDown, the legacy markdown path) and confirmed all SAFE — the changes are additive and the adapter faithfully implements the legacy interface, so the zero out-of-diff count is genuine, not an underrun.