From bf49003fc3ebc826fc5aeb209b8626f4579c3884 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Wed, 13 May 2026 16:46:55 -0700 Subject: [PATCH 1/3] =?UTF-8?q?explorer:=20table=20v2=20=E2=80=94=20server?= =?UTF-8?q?-side=20pagination=20+=20stale-while-loading?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the up-front "load up to N rows; paginate in JS" model with true server-side pagination via DuckDB LIMIT/OFFSET. Architecture: - One DuckDB query per page (LIMIT TABLE_PAGE_SIZE OFFSET p*size). - One COUNT(*) query per filter change (cached until filters change again — page navigation does not refire it). - ORDER BY pid so "Page N is always the same N rows" is actually true. - Stale-while-loading: existing rendered rows stay visible (dimmed to 60% opacity via #tableContainer.is-loading) while the new page or count is fetched in the DuckDB Web Worker. aria-busy="true" on the container signals to screen readers. - A small CSS-only spinner appears in #tableMeta during loads. Honors prefers-reduced-motion (animation: none). - pageGen counter invalidates in-flight callbacks when a newer load starts, similar to the freshSelectionToken pattern. UI removed: - #tableControls + #maxSamples input. Capping at 25K-100K rows was a relic of the toggle-mode UI where the table was an opt-in surface; with server-side paging the user pages through whatever the filter set is, no client-side memory pressure. - TABLE_DEFAULT_MAX, TABLE_MIN_MAX, TABLE_MAX_MAX constants and getTableMaxSamples() / clampTableMaxSamples() helpers — all unused. Pager UX: - Prev/Next buttons disabled during in-flight loads to prevent the user queueing a second click while a query is in flight. - Meta line shows "Loading samples matching filters..." then "{N} samples match the current filters." once count returns. - If page-data returns before count (smaller query, usually does), pager shows "Page N" without total; updates to "Page N of M (start-end of total)" when count lands. Co-Authored-By: Claude Opus 4.7 (1M context) --- explorer.qmd | 285 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 180 insertions(+), 105 deletions(-) diff --git a/explorer.qmd b/explorer.qmd index cb6e3ac..75a311a 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -258,24 +258,32 @@ format: overflow-y: auto; line-height: 1.35; } - .table-controls { - display: flex; - align-items: center; - gap: 8px; - font-size: 12px; - color: #555; - margin-bottom: 8px; - } - .table-controls input { - width: 92px; - padding: 5px 8px; - border: 1px solid #b8c7d9; - border-radius: 4px; - font-size: 13px; - } #tableContainer { margin-bottom: 16px; } + /* Stale-while-loading: dim the existing table to ~60% opacity while + a new page or filter result is being fetched, so the user can see + that the visible rows are not yet current. The CSS-animated spinner + in #tableMeta provides the explicit "working" affordance. */ + #tableContainer.is-loading .samples-table { + opacity: 0.6; + } + .samples-table { transition: opacity 0.15s ease; } + .table-loading-spinner { + display: inline-block; + width: 12px; + height: 12px; + border: 2px solid #cfd8e3; + border-top-color: #1565c0; + border-radius: 50%; + animation: tableSpin 0.8s linear infinite; + vertical-align: -2px; + margin-right: 6px; + } + @keyframes tableSpin { to { transform: rotate(360deg); } } + @media (prefers-reduced-motion: reduce) { + .table-loading-spinner { animation: none; border-top-color: #1565c0; } + } .samples-section-heading { font-size: 15px; font-weight: 600; @@ -477,11 +485,7 @@ Loading H3 global overview...

Samples table

-
- - -
-
+
Loading samples matching the current filters...
@@ -944,10 +948,12 @@ function updateSamples(samples) { } // === Samples table (permanent below globe; M-5) === +// Server-side pagination via DuckDB LIMIT/OFFSET (table v2, #200 follow-up). +// Previously the table loaded up to 25,000-100,000 rows up-front and sliced +// client-side; now each page is a fresh DuckDB query, and a COUNT(*) is +// issued on filter change so the pager knows the total. The deterministic +// ORDER BY pid makes "Page N is the same N rows" actually true. TABLE_PAGE_SIZE = 100 -TABLE_DEFAULT_MAX = 25000 -TABLE_MIN_MAX = 1000 -TABLE_MAX_MAX = 100000 function escapeHtml(value) { return String(value ?? '') @@ -958,19 +964,6 @@ function escapeHtml(value) { .replace(/'/g, '''); } -function clampTableMaxSamples(value) { - const n = parseInt(value, 10); - if (!Number.isFinite(n)) return TABLE_DEFAULT_MAX; - return Math.min(TABLE_MAX_MAX, Math.max(TABLE_MIN_MAX, n)); -} - -function getTableMaxSamples() { - const el = document.getElementById('maxSamples'); - const value = clampTableMaxSamples(el ? el.value : TABLE_DEFAULT_MAX); - if (el && String(value) !== String(el.value)) el.value = value; - return value; -} - ``` ```{ojs} @@ -1306,28 +1299,69 @@ facetFilters = { //| echo: false //| output: false -// === Table view: paginated sample rows matching current filters === +// === Table view: server-side paginated sample rows (table v2) === +// +// Architecture: +// - One DuckDB query per page (LIMIT TABLE_PAGE_SIZE OFFSET page*size). +// - One COUNT(*) query per filter change (cached until filters change again). +// - ORDER BY pid for deterministic page contents. +// - Stale-while-loading: old rows stay visible (dimmed via .is-loading class) +// while the new page/count is fetched in the DuckDB Web Worker. aria-busy +// on #tableContainer signals to screen readers. +// - pageGen invalidates in-flight queries when a newer one starts. tableView = { if (!facetFilters) return; - let rows = []; - let rowsByPid = new Map(); - let page = 0; - let requestId = 0; - let loadedMax = 0; - let hitHardCap = false; + let totalRows = null; // null = "unknown / fetching"; set by loadCount() + let pageRows = []; // rows visible right now + let pageRowsByPid = new Map(); // pid → row lookup for row-click handler + let currentPage = 0; + let pageGen = 0; // bumped on any new load; in-flight callbacks check this - const maxInput = document.getElementById('maxSamples'); const prevBtn = document.getElementById('tablePrev'); const nextBtn = document.getElementById('tableNext'); const metaEl = document.getElementById('tableMeta'); const pageInfoEl = document.getElementById('tablePageInfo'); const tableEl = document.getElementById('samplesTable'); + const containerEl = document.getElementById('tableContainer'); + + function totalPagesFor(total) { + return Math.max(1, Math.ceil(total / TABLE_PAGE_SIZE)); + } + + function setLoading(loading) { + if (containerEl) { + containerEl.classList.toggle('is-loading', loading); + containerEl.setAttribute('aria-busy', String(loading)); + } + // Disable pager during load so a user can't queue a second click while + // a query is in flight. updatePagerEdges() re-enables based on edge. + if (loading) { + if (prevBtn) prevBtn.disabled = true; + if (nextBtn) nextBtn.disabled = true; + } else { + updatePagerEdges(); + } + } + + function updatePagerEdges() { + if (prevBtn) prevBtn.disabled = currentPage <= 0; + if (nextBtn) { + const atEnd = totalRows != null && currentPage >= totalPagesFor(totalRows) - 1; + nextBtn.disabled = atEnd; + } + } - function setMeta(text, loading) { + function setMeta(text, isError) { if (!metaEl) return; metaEl.textContent = text; - metaEl.style.color = loading ? '#1565c0' : '#555'; + metaEl.style.color = isError ? '#c62828' : '#555'; + } + + function setMetaLoading(text) { + if (!metaEl) return; + metaEl.innerHTML = `${escapeHtml(text)}`; + metaEl.style.color = '#1565c0'; } function tableSourceBadge(source) { @@ -1337,18 +1371,14 @@ tableView = { } function renderTable() { - const totalPages = Math.max(1, Math.ceil(rows.length / TABLE_PAGE_SIZE)); - page = Math.min(page, totalPages - 1); - const start = page * TABLE_PAGE_SIZE; - const visible = rows.slice(start, start + TABLE_PAGE_SIZE); const selectedPid = (typeof viewer !== 'undefined' && viewer._globeState) ? viewer._globeState.selectedPid : null; if (!tableEl) return; - if (visible.length === 0) { + if (pageRows.length === 0 && totalRows === 0) { tableEl.innerHTML = '
No samples match the current filters.
'; - } else { - const body = visible.map(r => { + } else if (pageRows.length > 0) { + const body = pageRows.map(r => { const placeParts = r.place_name; const place = Array.isArray(placeParts) && placeParts.length > 0 ? placeParts.filter(Boolean).join(' › ') @@ -1387,7 +1417,7 @@ tableView = { tr.addEventListener('click', async (e) => { if (e.target.tagName === 'A') return; // let label links work const pid = tr.dataset.pid; - const sample = pid ? rowsByPid.get(pid) : null; + const sample = pid ? pageRowsByPid.get(pid) : null; if (!sample || sample.latitude == null || sample.longitude == null) return; if (typeof viewer === 'undefined') return; @@ -1410,9 +1440,7 @@ tableView = { // Write the #pid hash directly here rather than relying on // zoomWatcher's listener. zoomWatcher may not be wired yet // at very-early-boot click time, and even when it is, the - // _suppressHashWrite gate could swallow the write. Calling - // buildHash() unconditionally keeps the URL canonical with - // the selection regardless of init ordering. + // _suppressHashWrite gate could swallow the write. history.replaceState(null, '', buildHash(viewer)); // Repaint to apply .selected class without a full refresh. tableEl.querySelectorAll('tbody tr.selected').forEach(el => el.classList.remove('selected')); @@ -1437,76 +1465,123 @@ tableView = { }); } + // Pager text. While totalRows is null (count still in flight after a + // filter change) we still know the current page index from the + // page-data query, so render that without the total. if (pageInfoEl) { - const first = rows.length === 0 ? 0 : start + 1; - const last = Math.min(rows.length, start + visible.length); - pageInfoEl.textContent = rows.length === 0 - ? 'Page 0 of 0' - : `Page ${page + 1} of ${totalPages} (${first.toLocaleString()}-${last.toLocaleString()} of ${rows.length.toLocaleString()})`; + if (totalRows === 0) { + pageInfoEl.textContent = ''; + } else if (totalRows == null) { + pageInfoEl.textContent = pageRows.length > 0 + ? `Page ${currentPage + 1}` + : ''; + } else { + const totalPages = totalPagesFor(totalRows); + const first = currentPage * TABLE_PAGE_SIZE + 1; + const last = Math.min(totalRows, first + pageRows.length - 1); + pageInfoEl.textContent = `Page ${currentPage + 1} of ${totalPages} (${first.toLocaleString()}-${last.toLocaleString()} of ${totalRows.toLocaleString()})`; + } } - if (prevBtn) prevBtn.disabled = page <= 0; - if (nextBtn) nextBtn.disabled = page >= totalPages - 1; } - async function refreshTable() { - const myReq = ++requestId; - loadedMax = getTableMaxSamples(); - page = 0; - setMeta(`Loading up to ${loadedMax.toLocaleString()} samples matching filters...`, true); + async function loadCount(genAtStart) { + try { + const data = await db.query(` + SELECT COUNT(*) AS n + FROM read_parquet('${lite_url}') + WHERE 1=1 + ${sourceFilterSQL('source')} + ${facetFilterSQL()} + `); + if (genAtStart !== pageGen) return; + // DuckDB-WASM returns BigInt for COUNT(*); coerce safely. + const arr = Array.from(data); + totalRows = arr.length > 0 ? Number(arr[0].n) : 0; + renderTable(); + } catch (err) { + if (genAtStart !== pageGen) return; + console.error('Table count query failed:', err); + // Leave totalRows null; the page query may still succeed and + // render rows without a total. The meta line surfaces the error. + } + } + async function loadPage(p, genAtStart) { try { + const offset = p * TABLE_PAGE_SIZE; const data = await db.query(` SELECT pid, label, source, latitude, longitude, place_name, result_time FROM read_parquet('${lite_url}') WHERE 1=1 ${sourceFilterSQL('source')} ${facetFilterSQL()} - LIMIT ${loadedMax} + ORDER BY pid + LIMIT ${TABLE_PAGE_SIZE} OFFSET ${offset} `); - if (myReq !== requestId) return; + if (genAtStart !== pageGen) return; const arr = Array.from(data); - hitHardCap = arr.length === loadedMax; - rows = arr; - rowsByPid = new Map(arr.map(r => [r.pid, r])); + pageRows = arr; + pageRowsByPid = new Map(arr.map(r => [r.pid, r])); + currentPage = p; renderTable(); - const capText = hitHardCap - ? (loadedMax < TABLE_MAX_MAX - ? ` Max samples cap reached; raise it to inspect more rows.` - : ` Maximum table cap reached.`) - : ''; - setMeta(`Loaded ${rows.length.toLocaleString()} sample rows.${capText}`, false); } catch (err) { - if (myReq !== requestId) return; - console.error('Table query failed:', err); - rows = []; - rowsByPid = new Map(); + if (genAtStart !== pageGen) return; + console.error('Table page query failed:', err); + pageRows = []; + pageRowsByPid = new Map(); + setMeta('Table query failed; adjust filters and try again.', true); renderTable(); - setMeta('Table query failed; adjust filters and try again.', false); } } - if (prevBtn) prevBtn.addEventListener('click', () => { page = Math.max(0, page - 1); renderTable(); }); - if (nextBtn) nextBtn.addEventListener('click', () => { page += 1; renderTable(); }); - if (maxInput) { - maxInput.addEventListener('change', () => { - maxInput.value = getTableMaxSamples(); - refreshTable(); - }); - } - - function handleTableFilterChange() { - refreshTable(); - } + function summaryText() { + if (totalRows == null) return 'Counting samples...'; + if (totalRows === 0) return 'No samples match the current filters.'; + const total = totalRows.toLocaleString(); + return `${total} sample${totalRows === 1 ? '' : 's'} match the current filters.`; + } + + async function refreshAll() { + const gen = ++pageGen; + currentPage = 0; + totalRows = null; + setLoading(true); + setMetaLoading('Loading samples matching filters...'); + // Stale-while-loading: leave existing pageRows rendered (dimmed by + // .is-loading) so the table doesn't blink to empty. + await Promise.all([loadCount(gen), loadPage(0, gen)]); + if (gen !== pageGen) return; + setLoading(false); + setMeta(summaryText()); + } + + async function refreshPage(newPage) { + const gen = ++pageGen; + setLoading(true); + setMetaLoading(`Loading page ${newPage + 1}...`); + await loadPage(newPage, gen); + if (gen !== pageGen) return; + setLoading(false); + setMeta(summaryText()); + } + + if (prevBtn) prevBtn.addEventListener('click', () => { + if (currentPage <= 0) return; + refreshPage(currentPage - 1); + }); + if (nextBtn) nextBtn.addEventListener('click', () => { + if (totalRows == null) return; + if (currentPage >= totalPagesFor(totalRows) - 1) return; + refreshPage(currentPage + 1); + }); - document.getElementById('sourceFilter')?.addEventListener('change', handleTableFilterChange); - document.getElementById('materialFilterBody')?.addEventListener('change', handleTableFilterChange); - document.getElementById('contextFilterBody')?.addEventListener('change', handleTableFilterChange); - document.getElementById('objectTypeFilterBody')?.addEventListener('change', handleTableFilterChange); + document.getElementById('sourceFilter')?.addEventListener('change', refreshAll); + document.getElementById('materialFilterBody')?.addEventListener('change', refreshAll); + document.getElementById('contextFilterBody')?.addEventListener('change', refreshAll); + document.getElementById('objectTypeFilterBody')?.addEventListener('change', refreshAll); - window.refreshSamplesTable = refreshTable; - // Table is permanent now — load on boot instead of waiting for a - // setView('table') call that no longer exists (issue #200 / M-5). - refreshTable(); + window.refreshSamplesTable = refreshAll; + refreshAll(); return "active"; } From 5c2f052949876c811dc3946d8b8d35812b0e5021 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Wed, 13 May 2026 16:51:47 -0700 Subject: [PATCH 2/3] table v2: address Codex round-1 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged two blockers + recommendations on bf49003: 1. Stale tests in test_globe.py — both test_has_max_samples_input and test_has_view_toggle expected DOM elements that no longer exist (#maxSamples removed in table v2; view toggle removed in PR #200). Replaced with a test_has_permanent_table assertion that confirms the new contract: tableContainer + samplesTable + pager present, no #maxSamples, no view-toggle buttons. 2. loadPage() error path was swallowed — the inner setMeta error message was being overwritten by setMeta(summaryText()) in the orchestrator. Reworked: loadCount and loadPage now return true/false; refreshAll/refreshPage only call setMeta(summary) on success, and surface specific error meta otherwise. Three distinct error meta lines now: page-failed, count-failed-only, page-failed-during-page-change. Plus Codex recommendations folded in: - AND pid IS NOT NULL guard on the page query — belt-and-suspenders for the ORDER BY pid determinism contract. - setMetaLoading() now clears #tablePageInfo too, so a filter change doesn't leave stale "Page 3 of 12 (200-300 of 1,200)" text visible while the new filter set is being counted. - EXPLORER_STATE.md §3 updated to reflect rowsByPid → pageRowsByPid (now scoped per page); #maxSamples removed from the input-source paragraph; new "Table v2 addendum" added to §6 covering pagination contract, determinism, stale-while-loading, race protection, and error handling. - Two new Playwright tests in explorer-map-overlay.spec.js: - server-side pagination: Next button advances to Page 2 with different rows, no #maxSamples element - filter change: aria-busy flips true→false, pager text updates to reflect the new filtered total Co-Authored-By: Claude Opus 4.7 (1M context) --- EXPLORER_STATE.md | 61 ++++++++++++++++--- explorer.qmd | 45 +++++++++++--- tests/playwright/explorer-map-overlay.spec.js | 57 +++++++++++++++++ tests/test_globe.py | 22 ++++--- 4 files changed, 158 insertions(+), 27 deletions(-) diff --git a/EXPLORER_STATE.md b/EXPLORER_STATE.md index 177df95..7adb87f 100644 --- a/EXPLORER_STATE.md +++ b/EXPLORER_STATE.md @@ -92,18 +92,22 @@ predates the store-on-`viewer` pattern and isn't worth migrating. | ~~`document.body.classList['table-view-active']`~~ | _removed in mockup-v1 (#200)_ | — | — | The view marker class is gone with the Globe/Table toggle. The samples table is permanent below the globe; `isTableViewActive()` and `setView()` were deleted | | `data-facet`, `data-value` on `.facet-row` and `.facet-count` | facet selectors for in-place count mutation | rendered in `renderFilter` and the static source legend | `applyFacetCounts()` | rebuilding the HTML would lose mid-interaction selections; `data-*` attrs are why we mutate counts in place | | `data-lat`, `data-lng`, `data-pid` on `.sample-row` | click-to-fly payload for search/nearby results | rendered in `doSearch()` | search-row click handler | the nearby-samples list does not have data-* today; click-to-fly only works from the search list | -| `data-pid` on `.samples-table tbody tr` | click-to-select payload for the permanent table | rendered in `tableView` `renderTable()` | table-row click handler (same ceremony as the search-row click — see §6 mockup-v1 addendum) | per-PID lookup uses `rowsByPid: Map` cached at `refreshTable()` time; not the DOM | -| `tr.selected` on `.samples-table` | "this row is the current sample selection" visual marker | table-row click handler (`add`); next click on a different row (`remove`); next `renderTable()` reflects current `viewer._globeState.selectedPid` | CSS only | derived; do not read. The globe → table direction is *not* live (only repaints on the next refresh) | +| `data-pid` on `.samples-table tbody tr` | click-to-select payload for the permanent table | rendered in `tableView` `renderTable()` | table-row click handler (same ceremony as the search-row click — see §6 mockup-v1 addendum) | per-PID lookup uses `pageRowsByPid: Map` cached per page at `loadPage()` time (table v2); only the current page is in memory | +| `tr.selected` on `.samples-table` | "this row is the current sample selection" visual marker | table-row click handler (`add`); next click on a different row (`remove`); next `renderTable()` reflects current `viewer._globeState.selectedPid` | CSS only | derived; do not read. The globe → table direction is *not* live (only repaints on the next page load) | +| `#tableContainer.is-loading` + `aria-busy="true"` | "table query in flight" marker | `setLoading(true/false)` in `tableView` (table v2) | CSS dim (`.samples-table` opacity 0.6); screen readers | added in table v2 follow-up to PR #200 for stale-while-loading UX | | `.recomputing` on `.facet-count` | transient "loading" styling during cross-filter recompute | `markFacetCountsRecomputing()` (`:570`) | `applyFacetCounts()` clears it (`:562`) | UI-only; not state in the persistence sense | | `.zero` on `.facet-row` | "value has zero count under current filters" styling | `applyFacetCounts()` (`:565`) | CSS only | derived; do not read | | `.disabled` on `#sourceFilter .legend-item` | unchecked source visual | `updateSourceLegendState()` (`:395-400`) | CSS only | derived from checkbox `checked`; do not read | DOM input elements (the four facet checkbox bodies + `#sampleSearch` + -`#sampleSearchSidebar` + `#maxSamples`) are the **source of truth** for -`getActiveSources()`, `getCheckedValues()`, `getTableMaxSamples()`, and -the search input. SQL builders read `#sampleSearch` directly each call. -`#sampleSearchSidebar` is kept in lock-step with `#sampleSearch` via a -two-way `input`-event mirror (see §6 mockup-v1 addendum). +`#sampleSearchSidebar`) are the **source of truth** for +`getActiveSources()`, `getCheckedValues()`, and the search input. SQL +builders read `#sampleSearch` directly each call. `#sampleSearchSidebar` +is kept in lock-step with `#sampleSearch` via a two-way `input`-event +mirror (see §6 mockup-v1 addendum). The `#maxSamples` input and the +`getTableMaxSamples()` / `clampTableMaxSamples()` helpers were removed +in the table v2 follow-up — the samples table now paginates server-side +via DuckDB `LIMIT/OFFSET` instead of fetching up to 25K rows up-front. --- @@ -454,6 +458,49 @@ The biggest delta to this contract: struck), and §5 (OJS cell graph: `tableView` no longer writes `view` to URL; `tableView` does write `#pid` to hash via `buildHash`). +### Table v2 addendum: server-side pagination ([#218](https://github.com/isamplesorg/isamplesorg.github.io/issues/218), 2026-05) + +Follow-up to the mockup-v1 PR. The samples table no longer fetches up +to 25,000-100,000 rows up-front and paginates client-side. Each page +is now its own DuckDB `LIMIT TABLE_PAGE_SIZE OFFSET page*size` query, +plus one `COUNT(*)` query per filter change. Removes the `#maxSamples` +input + `getTableMaxSamples()` / `clampTableMaxSamples()` helpers +entirely. + +**Determinism.** `ORDER BY pid` plus `WHERE pid IS NOT NULL` makes +"Page N is the same N rows" actually true. Defensive null filter even +though `pid` is the canonical identifier and should never be null — +ORDER BY a column that contains nulls is only deterministic by +accident on a read-only parquet snapshot. + +**Stale-while-loading.** When filters change or the user pages, the +existing rendered rows stay visible (dimmed to 60% opacity via +`#tableContainer.is-loading .samples-table`) while the new page+count +queries run in the DuckDB Web Worker. A CSS-only spinner appears in +`#tableMeta`. `#tableContainer[aria-busy="true"]` exposes the state +to screen readers. The pager-info text is cleared during load to +avoid showing stale "Page 3 of 12 (200-300 of 1,200)" against an +incoming filter set. `prefers-reduced-motion` is honored. + +**Race protection.** A `pageGen` integer is bumped on every refresh. +Inner queries (`loadCount`, `loadPage`) compare `gen === pageGen` +BEFORE mutating `pageRows`, `pageRowsByPid`, `totalRows`, or +`currentPage`. `refreshAll` / `refreshPage` re-check the same gen +before clearing the loading state, so a faster newer load can win +the visible UI even if an older load resolves last. + +**Error handling.** `loadCount` and `loadPage` both return +`true`/`false` to the orchestrator. If page load fails, the meta line +shows "Page query failed; adjust filters and try again." If count +fails but page succeeds, the meta line says so explicitly. This +replaces the round-1 codex finding where the error meta was being +overwritten by the success summary. + +**Click handler unchanged.** Table-row click uses +`pageRowsByPid: Map` (renamed from `rowsByPid`) which is now scoped +to the current page only — sufficient since only the visible page has +clickable rows. + --- ## 7. Facet-count contract diff --git a/explorer.qmd b/explorer.qmd index 75a311a..d918a16 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -1362,6 +1362,10 @@ tableView = { if (!metaEl) return; metaEl.innerHTML = `${escapeHtml(text)}`; metaEl.style.color = '#1565c0'; + // Clear the pager-info text while loading so it doesn't show + // stale "Page 3 of 12 (200-300 of 1,200)" against an incoming + // filter change. renderTable() repopulates it after data arrives. + if (pageInfoEl) pageInfoEl.textContent = ''; } function tableSourceBadge(source) { @@ -1493,44 +1497,52 @@ tableView = { ${sourceFilterSQL('source')} ${facetFilterSQL()} `); - if (genAtStart !== pageGen) return; + if (genAtStart !== pageGen) return true; // DuckDB-WASM returns BigInt for COUNT(*); coerce safely. const arr = Array.from(data); totalRows = arr.length > 0 ? Number(arr[0].n) : 0; renderTable(); + return true; } catch (err) { - if (genAtStart !== pageGen) return; + if (genAtStart !== pageGen) return false; console.error('Table count query failed:', err); // Leave totalRows null; the page query may still succeed and // render rows without a total. The meta line surfaces the error. + return false; } } async function loadPage(p, genAtStart) { try { const offset = p * TABLE_PAGE_SIZE; + // AND pid IS NOT NULL: belt-and-suspenders. pid is the primary + // identifier and should never be null, but ORDER BY a column + // that contains nulls is only deterministic by accident on a + // read-only parquet snapshot. Filter them defensively so the + // page contract holds even if a future parquet rev allows nulls. const data = await db.query(` SELECT pid, label, source, latitude, longitude, place_name, result_time FROM read_parquet('${lite_url}') - WHERE 1=1 + WHERE pid IS NOT NULL ${sourceFilterSQL('source')} ${facetFilterSQL()} ORDER BY pid LIMIT ${TABLE_PAGE_SIZE} OFFSET ${offset} `); - if (genAtStart !== pageGen) return; + if (genAtStart !== pageGen) return true; const arr = Array.from(data); pageRows = arr; pageRowsByPid = new Map(arr.map(r => [r.pid, r])); currentPage = p; renderTable(); + return true; } catch (err) { - if (genAtStart !== pageGen) return; + if (genAtStart !== pageGen) return false; console.error('Table page query failed:', err); pageRows = []; pageRowsByPid = new Map(); - setMeta('Table query failed; adjust filters and try again.', true); renderTable(); + return false; } } @@ -1549,20 +1561,33 @@ tableView = { setMetaLoading('Loading samples matching filters...'); // Stale-while-loading: leave existing pageRows rendered (dimmed by // .is-loading) so the table doesn't blink to empty. - await Promise.all([loadCount(gen), loadPage(0, gen)]); + const [countOk, pageOk] = await Promise.all([loadCount(gen), loadPage(0, gen)]); if (gen !== pageGen) return; setLoading(false); - setMeta(summaryText()); + if (pageOk && countOk) { + setMeta(summaryText()); + } else if (pageOk) { + // Count failed but page succeeded: show row totals from the page + // alone (caller can still navigate; Next is enabled only if + // totalRows is known). + setMeta('Page loaded, but the total-row count failed; try again to recompute.', true); + } else { + setMeta('Table query failed; adjust filters and try again.', true); + } } async function refreshPage(newPage) { const gen = ++pageGen; setLoading(true); setMetaLoading(`Loading page ${newPage + 1}...`); - await loadPage(newPage, gen); + const ok = await loadPage(newPage, gen); if (gen !== pageGen) return; setLoading(false); - setMeta(summaryText()); + if (ok) { + setMeta(summaryText()); + } else { + setMeta('Page query failed; adjust filters and try again.', true); + } } if (prevBtn) prevBtn.addEventListener('click', () => { diff --git a/tests/playwright/explorer-map-overlay.spec.js b/tests/playwright/explorer-map-overlay.spec.js index b7d91d7..e67feca 100644 --- a/tests/playwright/explorer-map-overlay.spec.js +++ b/tests/playwright/explorer-map-overlay.spec.js @@ -150,6 +150,63 @@ test.describe('Map search overlay — Cesium toolbar coexistence (#200 / M-1A)', } }); + test('table v2: pagination is server-side, pager shows Page X of Y, Next loads new rows', async ({ page }) => { + await page.setViewportSize({ width: 1280, height: 900 }); + await page.goto(`${BASE_URL}${EXPLORER_PATH}#v=1&lat=20&lng=0&alt=10000000`, { + waitUntil: 'domcontentloaded', + timeout: 60000, + }); + await page.waitForSelector('#cesiumContainer', { timeout: 30000 }); + await waitForBootReady(page); + + // First page should be visible with rows. + await expect(page.locator('.samples-table tbody tr[data-pid]').first()).toBeVisible({ timeout: 60000 }); + + // After COUNT returns, pager text matches "Page 1 of N (1-100 of TOTAL)". + const pagerLocator = page.locator('#tablePageInfo'); + await expect(pagerLocator).toContainText(/Page 1 of \d+/, { timeout: 60000 }); + + // Capture first page's first 3 pids. + const page1Pids = await page.locator('.samples-table tbody tr[data-pid]') + .evaluateAll(rows => rows.slice(0, 3).map(r => r.getAttribute('data-pid'))); + expect(page1Pids.length).toBe(3); + + // Click Next; pager should update to Page 2 and rows should differ. + await page.locator('#tableNext').click(); + await expect(pagerLocator).toContainText(/Page 2 of \d+/, { timeout: 30000 }); + + const page2Pids = await page.locator('.samples-table tbody tr[data-pid]') + .evaluateAll(rows => rows.slice(0, 3).map(r => r.getAttribute('data-pid'))); + expect(page2Pids).not.toEqual(page1Pids); + + // No #maxSamples element exists. + await expect(page.locator('#maxSamples')).toHaveCount(0); + }); + + test('table v2: filter change clears pager text and re-fetches count', async ({ page }) => { + await page.setViewportSize({ width: 1280, height: 900 }); + await page.goto(`${BASE_URL}${EXPLORER_PATH}#v=1&lat=20&lng=0&alt=10000000`, { + waitUntil: 'domcontentloaded', + timeout: 60000, + }); + await page.waitForSelector('#cesiumContainer', { timeout: 30000 }); + await waitForBootReady(page); + + await expect(page.locator('#tablePageInfo')).toContainText(/Page 1 of \d+/, { timeout: 60000 }); + const totalAll = await page.locator('#tablePageInfo').textContent(); + + // Toggle off one source — total should change. + await page.locator('#sourceFilter input[value="OPENCONTEXT"]').uncheck(); + // aria-busy goes true during the refetch. + await expect(page.locator('#tableContainer')).toHaveAttribute('aria-busy', 'true', { timeout: 5000 }); + // Then back to false once both COUNT and page queries return. + await expect(page.locator('#tableContainer')).toHaveAttribute('aria-busy', 'false', { timeout: 60000 }); + + await expect(page.locator('#tablePageInfo')).toContainText(/Page 1 of \d+/, { timeout: 60000 }); + const totalFiltered = await page.locator('#tablePageInfo').textContent(); + expect(totalFiltered).not.toBe(totalAll); + }); + test('clicking a table row selects the sample, updates #pid hash, and marks the row selected', async ({ page }) => { await page.setViewportSize({ width: 1280, height: 900 }); await page.goto(`${BASE_URL}${EXPLORER_PATH}#v=1&lat=20&lng=0&alt=10000000`, { diff --git a/tests/test_globe.py b/tests/test_globe.py index 934b82e..ec2cdda 100644 --- a/tests/test_globe.py +++ b/tests/test_globe.py @@ -40,16 +40,18 @@ def test_has_sampled_feature_filter(self, explorer_page): def test_has_specimen_type_filter(self, explorer_page): assert explorer_page.locator("#objectTypeFilter").count() == 1 - def test_has_max_samples_input(self, explorer_page): - # Phase 4: number input bounded 1K-100K, default 25K. Lives in table view controls. - max_input = explorer_page.locator("#maxSamples") - max_input.wait_for(state="attached", timeout=15000) - assert max_input.count() == 1 - - def test_has_view_toggle(self, explorer_page): - # Phase 4: binary Globe/Table toggle (no List). - assert explorer_page.locator("#globeViewBtn").count() == 1 - assert explorer_page.locator("#tableViewBtn").count() == 1 + def test_has_permanent_table(self, explorer_page): + # PR #200 made the samples table a permanent surface below the globe; + # table v2 (#218) replaced the #maxSamples cap with server-side + # pagination, so neither #maxSamples nor the old Globe/Table view + # toggle (#globeViewBtn / #tableViewBtn) exists anymore. + assert explorer_page.locator("#tableContainer").count() == 1 + assert explorer_page.locator("#samplesTable").count() == 1 + assert explorer_page.locator("#tablePrev").count() == 1 + assert explorer_page.locator("#tableNext").count() == 1 + assert explorer_page.locator("#maxSamples").count() == 0 + assert explorer_page.locator("#globeViewBtn").count() == 0 + assert explorer_page.locator("#tableViewBtn").count() == 0 class TestExplorerFacetCounts: From 7519a9c2579481b18ddd2dce96cda7decc759591 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Wed, 13 May 2026 16:55:20 -0700 Subject: [PATCH 3/3] table v2: address Codex round-2 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blockers from round 2: 1. Page-query failure left the OLD DOM visible while pageRowsByPid was empty — rows looked clickable but did nothing. Added a lastPageFailed flag; renderTable() swaps the table body for an explicit sentinel row ("Page query failed. Adjust filters or click Previous/Next to retry.") when the flag is on. Pager text is also cleared in the failure branch. Reset on the next successful loadPage so a recovery returns to normal rendering. 2. Next button stayed enabled when totalRows == null (count failure) even though the click handler bailed immediately — confusing UX. updatePagerEdges() now disables Next whenever totalRows == null, regardless of currentPage. 3. COUNT(*) query didn't have the WHERE pid IS NOT NULL guard. With ORDER BY pid + IS NOT NULL on the page query only, totalRows could overcount and the pager could enable Next past the last non-null page. Added the matching predicate to the count query for consistency. Plus updates to EXPLORER_STATE.md table v2 addendum: the determinism note now mentions BOTH queries carry the null filter; the error handling section now describes all three failure modes (page-failed, count-failed-only, both-failed) and the explicit sentinel table state. Co-Authored-By: Claude Opus 4.7 (1M context) --- EXPLORER_STATE.md | 34 ++++++++++++++++++++++++---------- explorer.qmd | 32 +++++++++++++++++++++++++++----- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/EXPLORER_STATE.md b/EXPLORER_STATE.md index 7adb87f..874d66a 100644 --- a/EXPLORER_STATE.md +++ b/EXPLORER_STATE.md @@ -467,11 +467,14 @@ plus one `COUNT(*)` query per filter change. Removes the `#maxSamples` input + `getTableMaxSamples()` / `clampTableMaxSamples()` helpers entirely. -**Determinism.** `ORDER BY pid` plus `WHERE pid IS NOT NULL` makes -"Page N is the same N rows" actually true. Defensive null filter even -though `pid` is the canonical identifier and should never be null — -ORDER BY a column that contains nulls is only deterministic by -accident on a read-only parquet snapshot. +**Determinism.** `ORDER BY pid` plus `WHERE pid IS NOT NULL` on +**both** the page query and the `COUNT(*)` query makes "Page N is +the same N rows" actually true, and keeps the count consistent with +what's pageable. Defensive null filter even though `pid` is the +canonical identifier and should never be null — ORDER BY a column +that contains nulls is only deterministic by accident on a read-only +parquet snapshot, and an unfiltered count could over-enable +pagination past the last non-null page. **Stale-while-loading.** When filters change or the user pages, the existing rendered rows stay visible (dimmed to 60% opacity via @@ -490,11 +493,22 @@ before clearing the loading state, so a faster newer load can win the visible UI even if an older load resolves last. **Error handling.** `loadCount` and `loadPage` both return -`true`/`false` to the orchestrator. If page load fails, the meta line -shows "Page query failed; adjust filters and try again." If count -fails but page succeeds, the meta line says so explicitly. This -replaces the round-1 codex finding where the error meta was being -overwritten by the success summary. +`true`/`false` to the orchestrator. Three distinct error surfaces: + +- **Page load failed:** meta shows the error, `lastPageFailed` flag + flips on, and `renderTable()` swaps the table body for an explicit + "Page query failed. Adjust filters or click Previous/Next to retry." + sentinel row (rather than leaving the old, now-inert rows visible + with a cleared `pageRowsByPid`). Pager text is cleared. +- **Count failed but page succeeded:** rows render, but `totalRows` + stays `null`. Pager text shows "Page N" without the total. The + Next button is disabled while `totalRows == null` (so a user can't + click it into a no-op handler). +- **Both failed:** generic error meta; sentinel table state. + +This replaces the round-1 codex finding where the error meta was +being overwritten by the success summary, and the round-2 finding +where a failed page left old DOM visible but pageRowsByPid empty. **Click handler unchanged.** Table-row click uses `pageRowsByPid: Map` (renamed from `rowsByPid`) which is now scoped diff --git a/explorer.qmd b/explorer.qmd index d918a16..db1be7d 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -1317,6 +1317,7 @@ tableView = { let pageRowsByPid = new Map(); // pid → row lookup for row-click handler let currentPage = 0; let pageGen = 0; // bumped on any new load; in-flight callbacks check this + let lastPageFailed = false; // surfaces a sentinel table state when loadPage errors const prevBtn = document.getElementById('tablePrev'); const nextBtn = document.getElementById('tableNext'); @@ -1347,8 +1348,15 @@ tableView = { function updatePagerEdges() { if (prevBtn) prevBtn.disabled = currentPage <= 0; if (nextBtn) { - const atEnd = totalRows != null && currentPage >= totalPagesFor(totalRows) - 1; - nextBtn.disabled = atEnd; + // Next is disabled when totalRows is unknown (count query + // failed or hasn't returned) — otherwise the button looks + // active but its click handler bails, which is confusing. + // It's also disabled once we're at the last page. + if (totalRows == null) { + nextBtn.disabled = true; + } else { + nextBtn.disabled = currentPage >= totalPagesFor(totalRows) - 1; + } } } @@ -1379,7 +1387,13 @@ tableView = { ? viewer._globeState.selectedPid : null; if (!tableEl) return; - if (pageRows.length === 0 && totalRows === 0) { + if (lastPageFailed) { + // Page query failed — show an explicit sentinel rather than + // leaving the old, now-inert rows visible (they'd look + // clickable but pageRowsByPid is empty, so clicks would + // silently no-op). + tableEl.innerHTML = '
Page query failed. Adjust filters or click Previous/Next to retry.
'; + } else if (pageRows.length === 0 && totalRows === 0) { tableEl.innerHTML = '
No samples match the current filters.
'; } else if (pageRows.length > 0) { const body = pageRows.map(r => { @@ -1473,7 +1487,9 @@ tableView = { // filter change) we still know the current page index from the // page-data query, so render that without the total. if (pageInfoEl) { - if (totalRows === 0) { + if (lastPageFailed) { + pageInfoEl.textContent = ''; + } else if (totalRows === 0) { pageInfoEl.textContent = ''; } else if (totalRows == null) { pageInfoEl.textContent = pageRows.length > 0 @@ -1490,10 +1506,14 @@ tableView = { async function loadCount(genAtStart) { try { + // pid IS NOT NULL mirrors the page query so the total counts only + // rows that are actually pageable. Without this, totalRows could + // overcount and the pager could enable Next past the last + // non-null page. const data = await db.query(` SELECT COUNT(*) AS n FROM read_parquet('${lite_url}') - WHERE 1=1 + WHERE pid IS NOT NULL ${sourceFilterSQL('source')} ${facetFilterSQL()} `); @@ -1534,6 +1554,7 @@ tableView = { pageRows = arr; pageRowsByPid = new Map(arr.map(r => [r.pid, r])); currentPage = p; + lastPageFailed = false; renderTable(); return true; } catch (err) { @@ -1541,6 +1562,7 @@ tableView = { console.error('Table page query failed:', err); pageRows = []; pageRowsByPid = new Map(); + lastPageFailed = true; renderTable(); return false; }