Zarr coverage, verification and unit signals - #278
Merged
Conversation
parseZarrIndex now returns a {format_version: 1 | 3} discriminated union;
v3 adds discovered_count, pending[], failure detail, and per-store
units_report/channels_tsv_read_error while every existing consumer keeps
working unchanged. Adds the pure zarrCoverage() and unitsNoticeText()
helpers for the dataset-page coverage panel and viewer notice (website#277).
Fixtures: a real production v1 index (on008083, 2 stores/36 failures) and
a schema-valid v3 sample with pending, captured/built against
epic-zarr-serving's zarr-index.schema.json.
The comment above discoverViewLevels described the probe as the only path; declaredViewLevels has preferred the producer-declared shape for a while, and biosigio 1.2.6+ writes it on every group, so probing is now the legacy fallback. declaredViewLevels also returned null for an explicit but EMPTY view_levels array, which reads as "not declared" and sends a very short recording through a full probe batch to discover the same zero the attrs already stated. An empty (or zero-count) declaration is now kept as a real answer: zero levels, no probe. Verified against a reverted copy of the fix that the new tests fail on the old behavior (website#276).
Biome flags the ??= grouping idiom; rewrite as explicit if-checks. Also re-formats the test file additions to match biome's style.
FilterSidebar gains has_zarr and has_zarr_verified checkboxes, round-tripped through the URL and sent server-side like has_hed. Sent even though production's /datasets doesn't understand them yet (an unrecognized query param is ignored, not an error) -- documented at the call site in filters.ts. Dataset.zarr_verify_status drives a small badge (verified/failed/ unverifiable, one-sentence tooltip) on DatasetCard.astro and the dataset page header; null renders nothing.
Adds bidsRowId(path), a deterministic DOM-safe id keyed by BIDS path. render-dir-listing.ts stamps it on file and directory-recording rows; the client-side upgradeDirRecordingRow path (BTi dirs recognized only after the Zarr index resolves) stamps the same id for parity. Groundwork for the coverage panel's "link each recording to its row" (website#277 decision 2) landing next.
Pure renderer over zarrCoverage(): "N of M recordings viewable" (M = discovered_count on v3, stores+failures on v1 with a note that pending isn't reported by that index version), failures grouped by code with the viewer-safe reason and a detail disclosure, pending grouped by reason with attempt counts. Each recording links to its BIDS row via bidsRowId. Returns "" when the dataset has no Zarr data at all. Tested against both real fixtures end-to-end (not just zarrCoverage in isolation) plus edge cases (empty index, full coverage, an unrecognized pending reason).
Coverage panel: an SSR placeholder slot sits just above the BIDS tree, populated by renderZarrCoveragePanel() inside the tree's existing index.json fetch callback -- no new request. Each failed/pending recording's link resolves via bidsRowId; a click delegate walks the path's ancestor directories (opening each <details>, revealing further "Show next" chunks as needed) before scrolling the row into view and flashing it, since a plain #anchor only works when the row already happens to be rendered. Units notice: applyUnitsNotice appends unitsNoticeText()'s one-line notice under a just-mounted viewer, called from all three mountEegViewer success paths (inline open, dialog open, in-dialog navigate) with the current recording's ZarrIndexStore. Styling for .zcov, .tree__row--flash, and .eegv-units-notice added to BidsTree.astro's existing is:global block (dynamically-rendered tree/viewer markup lives there, not in scoped component styles). Verified: bun run lint, bun run typecheck, bun run test (1456 tests), and bun run build all pass.
Documents the --test Hallu instance (nemar-cli#1180) so contributors know zarr-test can exercise a producer change (chunk geometry, index v3 fields) against a real re-conversion of the exemplar fleet before it reaches production, not just serve bytes copied from prod.
Deploying nemar-website with
|
| Latest commit: |
b2f1676
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://79b837bc.nemar-website.pages.dev |
| Branch Preview URL: | https://feature-zarr-coverage-v3.nemar-website.pages.dev |
This was referenced Sep 2, 2026
The declared branch used Promise.all, so one rejected level (403, transient 5xx, a declared-but-missing level) discarded every level that fetched successfully -- unlike the sibling probing branch, which already tolerated this. Mirrors it: Promise.allSettled, keep fulfilled levels, throw ViewLevelDiscoveryError with the partial list on a non-404 failure so the caller flags degradation the same way. A clean 404 on a declared level is dropped silently, matching the probing branch's "ended" case. openViewLevel's return type never actually included null (it either resolves a ViewLevel or its openNode() call rejects), so the type is now Promise<ViewLevel> and the vestigial null-check filter is gone from both call sites, not just the declared one. declaredViewLevels also now distinguishes a genuinely empty `view_levels: []` (declared zero levels) from a non-empty array that filters down to zero valid entries (malformed producer data): the latter warns and falls back to probing instead of being read as a deliberate zero. Adds regression tests for all three: the declared-branch partial failure (verified to fail against the pre-fix code before restoring the fix), the numeric n_view_levels/view_level_count branch at a positive count and at zero, and the malformed-vs-genuinely-empty array distinction.
annotateZarrRows only checked state.paths (viewable) and state.failureReasons (failed) when deciding whether an ordinary-looking <details> directory is actually a recording to upgrade into a signal-viewer row. A pending 4D/BTi directory recording (no name-derived extension, recognized only via the zarr index) fell through: it never got upgraded, so it never got a bidsRowId, and the coverage panel's jump link for it resolved to nothing. Adds zarrPendingPaths(index) (mirrors zarrAvailablePaths / zarrFailureReasonByPath's shape) and threads a pendingPaths set through the per-section zarr state so the same recognition check covers all three buckets.
"failed" means the sweep RAN and found a real channel-count mismatch; "unverifiable" means no check could run at all (private dataset, infra error). Both rendered as identical neutral "Zarr unverified"/"Zarr unverifiable" tags, which read as the same thing. Relabels failed as "Zarr fidelity issue" and adds two new Tag kinds, positive and warning, reusing the existing --color-success / --color-warning palette tokens the StatusBadge-family components already use for pass/fail states (not new tag colors). verified now renders positive (green), failed renders warning (amber), unverifiable stays neutral -- it isn't a flagged issue, there is nothing to warn about. Documented in the Tag color bible (.rules/design-language.md).
ZarrIndexPending.last_error was parsed but never rendered -- a pending entry gave no way to see WHY it was stuck, unlike a failure's detail. Adds the same escaped <details>/<pre> disclosure treatment, labeled "Last error", shown only when the entry actually carries one. Adds a dedicated escaping test suite: a single string carrying all five HTML-significant characters run through every field the renderer interpolates raw text into (failure code/reason/detail/path, pending reason/path/last_error), asserting the escaped form appears and the raw form never does -- including inside the data-jump-path attribute, and a structural check that no bare '<' survives outside the renderer's own tags.
…rlap parsePending: a null entry is skipped and a non-string reason defaults to "unknown" -- was implemented but untested. unitsNoticeText: pins which branch wins when kept_importer_unit > 0 and channels_tsv_read_error are both true (a real case -- the sidecar was read, some channels kept the importer's unit, but it also could not be read for a different reason). kept_importer_unit's exact count wins over the coarser channels_tsv_read_error fallback.
A present discovered_count was taken as-is: a too-small value could render "5 of 3 recordings viewable", a raw 0 alongside nonempty stores/failures/pending hid the whole panel (?? does not fall through on 0), and a negative value rendered as-is. discovered_count is now always stores.length + failures.length + pending.length -- the producer's own documented invariant, enforced rather than trusted. A present value that disagrees logs one warning naming both numbers; a matching or absent value stays silent.
The hybrid search endpoint's reduced SearchResult projection carries
no zarr fields, and applyClientFilters never reads hasZarr/
hasZarrVerified, so checking either box while a search query is
active silently did nothing -- the checkboxes stayed enabled with no
indication they weren't working, unlike license/density/electrode-
system, which already disable themselves in search mode.
Adds disabled={searchActive} to both checkboxes and the same
filters__note idiom the license chips use. A dedicated .flag--disabled
class (not the existing .flag--soon, which means "not shipped yet")
keeps the "temporarily unavailable while searching" reading distinct
from "phase 3, not built". Search-side zarr filtering (extending
SearchResult) is a follow-up issue, not in scope here.
…core
'_' doubled as the escape marker for every other disallowed character
('.' -> '_2e', '/' -> '_2f', ...) but was itself left unescaped, so
"a_2e" (literal underscore + "2e") and "a." (escaped) both produced
"rec-a_2e". Escaping '_' too (-> '_5f') means '_' never appears in the
output except as part of an escape sequence, which makes the mapping
injective again.
byFailureCode/byPendingReason were plain {} literals indexed by
producer-supplied strings (a failure code, a pending reason). A key
literally equal to "__proto__" hits the special accessor an ordinary
object has for that name: the init check reads a truthy
Object.prototype instead of undefined, skips allocating an array, and
the next .push throws. Verified against a reverted copy that both new
tests actually fail with that TypeError before restoring the fix.
Switches both accumulators to Object.create(null), which has no such
accessor, so "__proto__" behaves like any other string key. Object.
entries/keys/values (what render-zarr-coverage.ts already calls) work
unchanged on a null-prototype object.
The single .catch after fetchZarrIndex().then(...) caught both an actual fetch/parse failure AND any exception thrown while applying an already-successfully-fetched index (annotateZarrRows, renderZarrCoveragePanel, ...) under the same "index fetch threw" console message -- a rendering bug against known-good data pointed a developer at the wrong stage. Wraps the apply step in its own try/catch with a distinct "applying the fetched index threw" label (console.error, since it is a bug against data that already parsed fine, not an expected network hiccup); the outer .catch keeps its original label and warn level for the fetch/parse stage.
Non-2xx, a network failure, and a malformed JSON body all collapsed into an identical silent null, indistinguishable from the (also-null) happy path a caller can't tell apart from an outage. Mirrors api.ts's resolveCanonical convention: a 404 stays quiet (the common "not converted yet" case), anything else non-ok logs the status, and a thrown error (network failure or bad JSON) logs the caught error -- all before returning null, never after. Tested with a stubbed globalThis.fetch (same transport-boundary pattern the existing prefetchZarrStoreMetadata tests already use): 404 silent, 503 warns with status, a rejected fetch warns with the error, invalid JSON warns, and a successful response stays silent.
revealRecordingRow's top-level chunk-reveal loop could synthesize up to REVEAL_MAX_CHUNK_CLICKS (500) .click() calls back-to-back with no yield, each triggering a DOM insert + re-annotate pass -- on a pathological dataset that blocks the main thread solid instead of letting the browser paint between chunks. Adds one requestAnimationFrame yield per click; REVEAL_MAX_CHUNK_CLICKS is unchanged. Inline page script, so no unit test is possible for this one -- noted in the PR body.
Both were defined only in the light :root block; Tag.astro's new positive/warning kinds (PR #278) use them as text color, so "Zarr verified" and "Zarr fidelity issue" rendered at roughly 3.7:1 on the dark background. Adds dark overrides in both dark blocks (prefers-color-scheme and [data-theme="dark"]), following the --color-maintenance pattern in the same file. Reuses the exact hexes already dark-lifted on --license-public / --license-sharealike (#4ade80 / #fbbf24) rather than inventing new lift pairs -- the light values are identical (#15803d / #b45309), so these are the same color under two names, not two different colors that happen to coincide. This also fixes the same pre-existing contrast gap in DoiBadge.astro, BidsTree.astro, and UploadProgress.astro, which already consumed --color-success/--color-warning as text before this PR touched tokens.css at all.
This was referenced Sep 2, 2026
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.
Summary
Website half of nemarOrg/nemar-cli#1181 (zarr serving next phase) and nemarOrg/nemar-cli#1197 (per-dataset coverage). Folds in #276.
parseZarrIndexnow returns a discriminated{ format_version: 1 | 3 }union. v3 addsdiscovered_count,pending[], failuredetail, per-storeunits_report/channels_tsv_read_error, and top-level provenance (doi/license/citation/hed_version). Every existing consumer keeps working unchanged against both a v1 and a v3 index; unknown fields are ignored; malformed documents still return null. A purezarrCoverage(index)computes{ viewable, failed, pending, discovered, byFailureCode, byPendingReason, unknownPending }.discovered_counton v3; on v1, M = stores + failures with a note that pending isn't reported by that index version), then failures grouped by code with the viewer-safe reason and adetaildisclosure, then pending grouped by reason with attempt counts and alast_errordisclosure. Populated client-side from the tree's existingindex.jsonfetch — no new API call. Each recording links to its row in the file tree via a newbidsRowIdanchor; the link click auto-expands collapsed ancestor directories (including revealing further "Show next N" chunks, now yielding a frame between each) before scrolling the row into view.has_zarr/has_zarr_verifiedcheckboxes in the FilterSidebar, round-tripped through the URL and sent server-side likehas_hed(production ignores the unrecognized params harmlessly until the backend release catches up — documented at the call site); disabled while a search query is active, since the hybrid search endpoint can't enforce them yet. Azarr_verify_statusbadge (verified = positive/green, failed = "Zarr fidelity issue" in warning/amber, unverifiable = neutral) onDatasetCard.astroand the dataset page header;nullrenders nothing.units_report.kept_importer_unit > 0, orchannels_tsv_read_erroris set, one line renders under the viewer explaining the sidecar unit couldn't be adopted, with the affected channel count. Absent for v1 stores (nounits_reportat all).store.ts(probing has been the legacy fallback sincedeclaredViewLevelsstarted preferring the producer-declared shape); an empty declaredview_levelsarray is now treated as "declared, zero levels" instead of "not declared," so a very short recording no longer triggers a full probe batch. Verified against a reverted copy of the fix that the new regression test actually fails on the old behavior.zarr-test.nemar.orgcarries real nightly re-conversions of the exemplar fleet (not copies of prod's stores) and can exercise a producer change before it reaches production.Review response
Two review passes landed after the initial PR (code/tests review, then a silent-failure-focused delta). All 16 items addressed; decisions below.
Must fix
discoverViewLevels's declared branch usedPromise.all, so one rejected level (403, transient 5xx) discarded every level that fetched. Fixed to mirror the probing branch:Promise.allSettled, keep fulfilled levels, throwViewLevelDiscoveryErrorwith the partial list on a non-404 failure. Also correctedopenViewLevel's return type (never actually resolved null) so both call sites lost their vestigial null-check. New test verified to fail against the pre-fix code first.bidsRowId.annotateZarrRowsonly recognizedstate.paths/state.failureReasons; addedzarrPendingPaths(index)and apendingPathsset to the per-section state so the coverage panel's jump link resolves for a still-converting directory recording too.failedas "Zarr fidelity issue" (the sweep ran and found a mismatch) and added two newTagkinds,positive/warning, reusing the existing--color-success/--color-warningpalette tokens (documented in the Tag color bible,.rules/design-language.md) rather than inventing new tag colors.verified→ positive,failed→ warning,unverifiable→ neutral (no check ran at all, nothing to flag).last_errorwas parsed but never rendered. Now gets the same escaped<details>/<pre>disclosure treatment as a failure'sdetail.code/reason/detail/path, pendingreason/path/last_error), asserting the escaped form appears, the raw form never does, and no bare<survives outside the renderer's own tags.n_view_levels/view_level_countat a positive count and at zero (zero probes either way), mirroring the array-branch coverage.parsePendingmalformed-entry coverage. Added a test: anullentry is skipped, a non-stringreasondefaults to"unknown".unitsNoticeTextoverlap pinned. Whenkept_importer_unit > 0andchannels_tsv_read_errorare both true,kept_importer_unit's exact count wins over the coarser channel-count fallback — now asserted explicitly.discovered_counttrusted verbatim. A too-small value could under-report the panel total, a raw0alongside nonempty arrays hid the whole panel (??doesn't fall through on 0), and a negative value rendered as-is.discovered_countis now alwaysstores.length + failures.length + pending.length; a disagreeing raw value logs one warning naming both numbers, a matching or absent one stays silent. Tests: too-small, zero-with-nonempty, negative, matching-stays-silent.SearchResultprojection carries no zarr fields andapplyClientFiltersdoesn't readhasZarr/hasZarrVerified, so the checkboxes did nothing while searching with no indication. Addeddisabled={searchActive}and the samefilters__noteidiom the license chips use (a dedicated.flag--disabledclass, distinct from the already-existing.flag--soon"not shipped yet" state). Search-side zarr filtering (extendingSearchResult) is an explicit follow-up, not in this PR.bidsRowIdcollision._doubled as the escape marker for other characters but was left unescaped itself, so"a_2e"and"a."both produced"rec-a_2e". Now_is escaped too (_5f); the one fixture-pinned expected id inbids-tree.test.tsis updated, plus a dedicated non-collision test.zarrCoverage__proto__crash.byFailureCode/byPendingReasonwere plain{}literals; a failurecodeor pendingreasonliterally equal to"__proto__"hits the special own-prototype accessor an ordinary object has for that key, so the init check sees a truthyObject.prototypeand the next.pushthrows. Switched both toObject.create(null). Verified against a reverted copy that both new tests fail with that exactTypeErrorfirst..catchafterfetchZarrIndex().then(...)labeled both an actual fetch/parse failure AND any exception from applying an already-successfully-fetched index (a rendering bug against known-good data) as "index fetch threw". Split into a try/catch around the apply step with its own "applying the fetched index threw" label.fetchZarrIndexsilently swallowed failures. Non-2xx, a network failure, and bad JSON all collapsed into an identical silentnull. Now logs before returning null — mirrorsapi.ts'sresolveCanonicalconvention: a 404 stays quiet (the common "not converted yet" case), anything else non-ok logs the status, a thrown error logs itself. Tested with a stubbedglobalThis.fetch(same transport-boundary pattern the existing prefetch tests already use).Should fix
declaredViewLevelsconflated malformed with declared-empty. A non-emptyview_levelsarray that filters down to zero valid entries (every element garbage) is producer data corruption, not a deliberate "zero levels" declaration — now warns and falls back to probing; a genuine[]is unaffected. Tested both.revealRecordingRowcould block the main thread. Up toREVEAL_MAX_CHUNK_CLICKS(500) synthesized "Show next" clicks ran back-to-back with no yield. Added onerequestAnimationFrameyield per click; the bound is unchanged. Inline page script, so no unit test is possible for this one.Delta re-review (after the above 16, all 12 commits verified)
positive/warningTag kinds failed dark-mode contrast.--color-success/--color-warning(src/styles/tokens.css) were defined only in the light:rootblock;Tag.astrouses them as text color for the badge, so "Zarr verified"/"Zarr fidelity issue" rendered at roughly 3.7:1 on the dark background. Added dark-lift overrides in both dark blocks (prefers-color-schemeand[data-theme="dark"]), following the--color-maintenancepattern in the same file, reusing the exact hexes already dark-lifted on--license-public/--license-sharealike(#4ade80/#fbbf24) since the light values are identical. This also fixes the same pre-existing gap inDoiBadge.astro,BidsTree.astro, andUploadProgress.astro, which already consumed these tokens as text before this PR touchedtokens.css.Screenshots / manual verification
Verified live against
bun run devwithPUBLIC_ZARR_BASE_URLpointed at a same-origin static fixture (a schema-valid v3index.json, served for the realon008083dataset so the rest of the page — landing/metadata/tree — came from production):not_continuous/retry_exhausted, with a workingDetaildisclosure showing the sanitized exception text), "3 recordings pending" (grouped byinfra_failure/memory_budget/not_attempted, each with its attempt count).sub-011/→ses-01/→eeg/) it had never rendered before, then scrolled the target row into view and updated the URL hash — confirmed via the realon008083file tree.has_zarr/has_zarr_verifiedcheckboxes present, submit via Apply round-trips through?has_zarr=1&has_zarr_verified=1, confirmed via the renderedcheckedattribute on reload; confirmeddisabled+ the new note appear only when?q=is set.Not independently verified live (both from before the review passes, still true): the units notice under a mounted viewer (needs a real dataset with a real v3-format S3 store — production still serves v1 everywhere today) and the
zarr_verify_statusbadge (no real dataset has a non-null verdict yet, since the fidelity sweep hasn't shipped to production). Both paths are covered by real unit tests instead (unitsNoticeText,isZarrVerifyStatus, and the conditional-render guards inDatasetCard.astro/ the dataset page). Also not independently verified live: item 16'srequestAnimationFrameyield (inline script, no unit test possible, and exercising 500 synthesized clicks against a real huge dataset wasn't practical here) — verified only by reading the change against the reveal loop's structure.Test plan
bun run lint— cleanbun run typecheck— 0 errorsbun run test— 1486 tests / 61 files passing, including:src/lib/zarr-index.test.ts(51 tests) — v1/v3 discrimination,zarrCoverageon both the realon008083v1 fixture and a schema-valid v3 fixture with pending,unitsNoticeText,discovered_countvalidation,__proto__safety,fetchZarrIndexloggingsrc/lib/render-zarr-coverage.test.ts(22 tests) — the coverage panel's HTML end-to-end against both fixtures, plus the escaping suitesrc/lib/eeg-viewer/store.test.ts(56 tests) — declared-levels partial-failure degradation, numeric branch, malformed-vs-empty array, all verified to fail against pre-fix code firstsrc/lib/bids-tree.test.ts(19 tests) —bidsRowId, including the injectivity fixsrc/lib/render-dir-listing.test.ts(30 tests) — recording-row anchor idssrc/lib/filters.test.ts(56 tests) /src/lib/tags.test.ts(26 tests) —has_zarr/has_zarr_verifiedround trip + API query, badge label/kind mappingbun run build— Cloudflare adapter build succeedsbun run devwith a local same-origin v3 fixture (see above)Closes #277
Closes #276
Part of nemarOrg/nemar-cli#1181