Explorer fixes for Eric (#295) and Andrea (#311, #312)#318
Merged
Conversation
quarto.js's findAndActivateCategories() unconditionally fetches /listings.json on every page whose frontmatter declares `categories:` (explorer.qmd does, for the tag chips under the title). Quarto only ever GENERATES that file for sites with an actual `listing:` page — this project has none, so the fetch 404s on every load. Harmless (the JS no-ops on a non-200 response) but a real, reported console error (Eric, isamplesorg#295) and one of the three sources behind isamplesorg#297's "console errors" report. Shipping a static empty-array listings.json via project `resources:` is exactly what Quarto itself would emit for a site with zero listings: same shape (`[]`), zero behavior change, just a 200 instead of a 404. Verified: reproduced the 404 live on isamples.org via Playwright, then confirmed 0 console errors against a local render with this fix.
…plingSite traversal Andrea reported the Samples table's Place and Date columns are "mostly (possibly entirely) blank" (isamplesorg#311). Verified against production wide.parquet: place_name and result_time are declared on MaterialSampleRecord's own schema but are 100% NULL there (0 of 6,680,932 rows) — a dead read, not a display bug. The real values live one or two hops away in the standard iSamples graph: Sample -produced_by-> SamplingEvent [-sampling_site-> SamplingSite]. Verified live: joining wide.p__produced_by[1] -> SamplingEvent.result_time recovers result_time for 94.7% of located samples; a further -> SamplingEvent.p__sampling_site[1] -> SamplingSite.place_name recovers place_name for 37.4%. build_frontend_derived.py's `samp` CTE now joins SamplingEvent/SamplingSite (same row_id-join pattern as the existing mat/ctx/obj joins) and pulls place_name/result_time from there, COALESCEd with the direct MSR-row value so this stays backward-compatible with any future wide build that DOES populate these fields directly on the sample row. New test (test_place_name_and_result_time_via_graph_traversal) covers the full-chain, event-only, and no-produced-by-link cases. All 37 existing tests in this file plus the 56 in test_ingest_oc_records.py/ test_oc_concept_enrichment.py still pass unchanged (the two new p__produced_by/p__sampling_site columns are NULL-default additions to the shared fixtures). Ships the code + tests only. Taking effect requires a samples_map_lite rebuild from a wide.parquet + an R2 upload (same manual deploy gate as isamplesorg#317) — flagged as a follow-up, not blocking this PR.
…/Sampled feature columns + CSV export isamplesorg#311 (Andrea): the Samples table only showed Source/Label/Place/Date/Lat/ Lon. Adds Material, Object type, and Sampled feature (the isamplesorg#291 naming for the `context` facet dimension) columns, joined from sample_facets_v3.parquet (scalar per-pid material/context/object_type URI columns) keyed by pid, resolved to human labels client-side via the existing window.conceptLabelForUri() rather than three more SQL joins per page. The join happens in a CTE AFTER the LIMIT/OFFSET page slice, so it only ever touches TABLE_PAGE_SIZE rows, not a full facets scan. isamplesorg#312 (Andrea): adds a "Download CSV" button that exports the CURRENT FILTERED result set (viewport + source/facet/search filters), not just the visible page — same WHERE-clause builders as loadPage()/loadCount(), without the LIMIT/OFFSET. Capped at 50,000 rows so an unfiltered world-zoom export can't hang the tab; the status line says so honestly ("first 50,000 of N — narrow your filters") rather than truncating silently, matching this table's existing "honesty" convention for loading/stale states. Verified live against production data.isamples.org (local render, real parquet fetches): table renders real labels ("Mineral", "Rock", "Earth interior", ...) in the three new columns; CSV button produces a well-formed 50,001-line file with the correct honest-cap message; 0 console errors either way.
Codex review of this branch (before opening upstream) found 3 real issues, 2 of them bugs: 1. scripts/build_frontend_derived.py: COALESCE(s.place_name, site.place_name) let an EMPTY array win over a populated traversal value — DuckDB's COALESCE treats []::VARCHAR[] as non-NULL, so COALESCE([], ['site']) = [], not ['site']. Same issue for result_time with ''. Switched both to a CASE that explicitly checks for NULL-or-empty on the direct value before falling through to the traversal. Also cast s.result_time to VARCHAR before TRIM (its declared type varies — VARCHAR in production wide, TIMESTAMP in the test fixtures — and TRIM only accepts VARCHAR). New fixture case (t-empty-direct) + assertions prove the fallthrough. 2. explorer.qmd csvField(): the quoting regex missed bare \r (RFC 4180 requires quoting CR same as LF/comma/quote). Added \r to the test. Also added formula-injection hardening (a cell starting with =, +, -, or @ gets a leading single-quote) since this is public data landing in a file users will likely open in Excel/Sheets. Had to explicitly skip the guard for typeof v === 'number' — latitude/longitude are negative roughly half the time, and "-130.0136" naively matches the same leading-character check as a formula; verified with node that negative coordinates now pass through unmodified while string fields still get the safety prefix. 3. (documented, not changed) The ev/site join reduces multiple SamplingEvents to p__produced_by[1], same limitation the existing mat/ctx/obj[1]-pick joins already have. Verified against production 202608 wide: 0 of 6.68M located samples currently have more than one p__produced_by entry, so left as-is with a comment (matching the existing accepted tradeoff for context/object_type) rather than a larger unnest-with-ordinality rewrite for a currently-empty case. Re-verified after these fixes: 40/40 pipeline tests pass, quarto render clean, 0 console errors on a fresh local boot against live production data, CSV re-downloaded and spot-checked — negative longitudes intact, no corruption.
rdhyee
added a commit
that referenced
this pull request
Jul 3, 2026
… place_name Array.isArray bug Deploys the #311 pipeline fix (SamplingEvent/SamplingSite traversal for place_name/result_time, merged in #318) to production data: rebuilt samples_map_lite from the SAME wide.parquet currently live on R2 (verified byte-identical: exact row counts + per-source min/max pid match against https://data.isamples.org/isamples_202608_wide.parquet before rebuilding), uploaded to R2 as isamples_202608_samples_map_lite_v3.parquet (a NEW filename — the live isamples_202608_samples_map_lite_v2.parquet is served Cache-Control: immutable, max-age=31536000, so overwriting it would leave stale/broken copies at CDN edges and in visitors' browsers for up to a year; same convention _v2 itself already used over the unsuffixed name). Rebuild results (verified before deploying): place_name now resolves for 2,263,648/6,026,242 (37.6%) located samples, result_time for 5,937,692/6,026,242 (98.5%) — both were 0% before this fix. ## A second, latent bug this surfaced Testing the corrected data against the real page (not just raw parquet) found a genuine frontend bug that was invisible until now: the four places in explorer.qmd that render place_name (`Array.isArray(placeParts) && placeParts.length > 0 ? placeParts.filter(Boolean).join(' › ') : ''`) all checked `Array.isArray()`. Observable's DuckDBClient returns Arrow LIST columns (place_name is VARCHAR[]) as an Arrow `Vector` — iterable, has `.length`, but is NOT a plain JS Array, so `Array.isArray()` is FALSE on it. Every non-null place_name was silently rendering as blank. This was masked in every deploy up to today because place_name was 100% NULL in production before the samples_map_lite rebuild above — the bug had no observable symptom until real data started flowing. Extracted the shared logic to `formatPlaceName()` in assets/js/explorer-utils.js (matching this file's existing extracted- pure-helper convention, unit-tested under Node) and replaced all 4 call sites. The new unit test reproduces the bug directly with a fake non-Array iterable shaped like an Arrow Vector, asserting `Array.isArray(vector) === false` before asserting `formatPlaceName` still handles it correctly — so a regression back to a bare `Array.isArray` check would be caught even without live parquet data. Verified end-to-end against the real corrected R2 file (fresh browser origin, to rule out ES-module cache artifacts from repeated same-session testing): table Place column shows real values ("Axial Seamount summit caldera" for IGSN:321000001), Date column shows real values ("2013-12-20"), CSV export matches. 0 console errors. 48/48 JS unit tests and 40/40 pipeline tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Addresses open issues filed by Eric Kansa and Andrea Thomer.
#295 (Eric) — listings.json 404
quarto.js'sfindAndActivateCategories()unconditionally fetches/listings.jsonon every page whose frontmatter declarescategories:(explorer.qmddoes, for the tag chips under the title). Quarto only generates that file for sites with an actuallisting:page — this project has none, so the fetch 404s on every load. Harmless (the JS no-ops on a non-200) but a real, reported console error, and one of the three sources behind #297's "console errors" report.Fix: ship a static empty-array
listings.jsonvia projectresources:— exactly what Quarto itself would emit for a site with zero listings. Same shape, zero behavior change, just a 200 instead of a 404.Verified: reproduced the 404 live on isamples.org via Playwright, then confirmed 0 console errors with this fix (locally, on the rdhyee fork's Pages deploy, and now here).
#311 (Andrea) — Samples table detail
Two parts:
Material / Object type / Sampled feature columns — the table only showed Source/Label/Place/Date/Lat/Lon. Adds the three facet dimensions, joined from
sample_facets_v3.parquet(scalar per-pid URI columns) keyed by pid, resolved to human labels client-side via the existingwindow.conceptLabelForUri(). The join happens in a CTE after the LIMIT/OFFSET page slice, so it only ever touches page-size rows.Place/Date were always blank — root-caused, not just patched. Verified against production
wide.parquet:place_name/result_timeare declared onMaterialSampleRecord's own schema but are 100% NULL there (0 of 6,680,932 rows). The real values live onSamplingEvent(result_time, ~95% populated) andSamplingSite(place_name, viaSamplingEvent.p__sampling_site, ~37%), reached via the standardSample -produced_by-> SamplingEvent [-sampling_site-> SamplingSite]graph traversal.build_frontend_derived.py'ssampCTE now joins these (mirroring the existingmat/ctx/objrow_id-join pattern), falling through to the traversal value whenever the direct value is NULL or empty (not just NULL — see Codex review note below).samples_map_literebuild from a wide.parquet + an R2 upload — same manual deploy gate as #313 P1+P3: replace the boot-time facet-index full scan with a tiny trusted manifest #317. Flagged as a follow-up, not blocking this PR; the frontend columns (part 1) work today against the live data regardless.#312 (Andrea) — CSV download
Adds a "Download CSV" button that exports the current filtered result set (viewport + source/facet/search filters), not just the visible page. Capped at 50,000 rows with an honest "first 50,000 of N — narrow your filters" message rather than a silent truncation or a hung tab on an unfiltered world-zoom export.
Codex review
Two rounds. First round found 3 issues, addressed here:
COALESCE(s.place_name, site.place_name)let an empty array win over a populated traversal value (DuckDB treats[]as non-NULL). Switched to an explicit NULL-or-empty CASE. Same fix forresult_time, plus aCAST(... AS VARCHAR)beforeTRIM(the column's declared type varies between production VARCHAR and the TIMESTAMP test fixtures). New fixture case + assertions cover the regression directly.\r(RFC 4180 requires quoting it like\n); also added formula-injection hardening for cells starting with=+-@(public data landing in a file users will open in Excel/Sheets) — had to explicitly skip that guard fortypeof v === 'number'since a negative coordinate like-130.0136would otherwise be misidentified as a formula. Verified with a standalone Node script.SamplingEvent/SamplingSitejoin reduces multiple events top__produced_by[1], same limitation the existingmat/ctx/obj[1]-pick joins already have. Verified against production 202608 wide: 0 of 6.68M located samples currently have more than onep__produced_byentry.Second round, after the fix commit: explicit LGTM, no blocking issues.
Verification
tests/test_frontend_derived.py) pass, including 3 new tests.quarto render.listings.jsonreturns 200, table renders real resolved labels ("Mineral", "Rock", "Earth interior", ...) in the three new columns, CSV button downloads a well-formed 50,001-line file (spot-checked negative-longitude rows for the formula-guard fix — clean, no corruption).🤖 Generated with Claude Code