zarrextra: runtime type guards for tree nodes, docs, and build/test packaging fixes - #105
Merged
Merged
Conversation
`ZarrTree` admits a group or a `LazyZarrArray` at every key and shipped no way to tell them apart, so consumers hand-rolled the check — and the obvious `typeof node === 'object'` test is wrong, because a lazy array is an object too and its own properties (`get`) then read as child keys. That had been open-coded in four places. `zarrextra` now exports the discrimination itself, in `treeNodes.ts`: `isLazyZarrArray` / `isZarrGroup`, the `getChildNode` / `getChildGroup` / `getChildArray` accessors most call sites actually want, and `getNodeAttrs` / `getArrayMetadata` for the symbol-keyed payloads. `getArrayDtype` folds v2's numpy typestrings and v3's names into one vocabulary — zarrita's own `DataType`, not a bare string — so a check made against tree metadata and the same check made against an opened array cannot disagree. `isTextDataType` is that shared definition of "does this need decoding to strings", covering v3 `string`, v2 fixed-width unicode/bytes and `v2:object`; zarrita's `isDataType(dtype, 'string')` excludes the last, and testing for one spelling without the other is what makes a reader hand back raw integer codes where labels were expected. `LazyZarrArray`'s `ZARRAY_KEY` payload is typed as `ZarrArrayMetadata` instead of an untyped record, so `dtype` and `data_type` can no longer be read without narrowing. The union keeps an unrecognised-record member on purpose: this is unvalidated JSON, and zarr v3 permits extension dtypes written as objects, so a strict two-member union would either lie or have to fail the store open. In `@spatialdata/core`, `parsed` is narrowed to a group once in `AbstractElement` so no element subclass sees the union, and `classifyObsColumnNode`, `getObsGroup` and `loadElements` drop their casts. `AnnDataSource.hasTextValues` now asks `isTextDataType` about an opened array's dtype, so the kind a UI sees before loading a column and the decoding it gets on load come from one definition. `readNullableArray`, `isNullableEncoding` and `NULLABLE_ENCODING_KINDS` are public too: guards make "is this group a categorical or a nullable column?" expressible, but only the semantics make it answerable without every consumer re-deriving AnnData's on-disk layout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The package had a README covering codecs and workers, and nothing anywhere covering the thing `@spatialdata/core` actually consumes it for: the consolidated-metadata tree. Adds a docs section for it. `overview` sets out what the package is for and why it is published outside the `@spatialdata/*` namespace, then covers opening a store, the two halves of `ConsolidatedStore` and which one to reach for, the store extras, and `Result`. Codecs and workers are linked rather than restated — they are already documented in the README, the codec-fixtures page and the worker-bundling pattern, and a second copy would drift. `tree-nodes` covers the node model in depth: why attributes and array metadata hang off symbol keys, why `typeof node === 'object'` is the wrong test and what it costs, the guards and child accessors, and reading a data type from metadata alone. The data-type section explains why the answer is in zarrita's vocabulary rather than a bare string, and calls out `v2:object` — excluded from zarrita's own `isDataType(dtype, 'string')`, and the reason a reader can hand back raw integer codes where labels were expected without raising anything. It ends where zarr stops and AnnData starts: a categorical and a nullable column are both groups, so discriminating group from array is necessary but not sufficient, and the semantics for those live in core. Also corrects the core internals page, which pointed at a `zarrUtils.ts` module and a `tryConsolidated` function that no longer exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zarrita is deliberately minimal and zarrextra is not, so the overview now says where that lands before someone depends on it. The main entry is small (15.4 kB, 5.2 kB gzipped) and the WASM codec packages are optional dependencies injected by the caller, so importing the package does not drag them in — the weight is the codec worker at 3.2 MB, and the fact that it is one bundle carrying every codec, so enabling worker decode for JP2K also ships OpenJPH. Most stores need neither. Also notes zod as a hard dependency reached only by an internal schema module, and flags the packaging rather than the APIs as the part most likely to be redesigned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe change adds typed Zarr tree-node guards, accessors, metadata and dtype helpers, and text detection. ChangesZarr tree-node helpers and core adoption
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ZarrStore
participant openExtraConsolidated
participant serializeZarrTree
participant CoreElementLoader
ZarrStore->>openExtraConsolidated: Load consolidated metadata
openExtraConsolidated->>serializeZarrTree: Build Zarr tree
serializeZarrTree->>serializeZarrTree: Read attributes and array metadata
serializeZarrTree-->>CoreElementLoader: Return serialized tree
CoreElementLoader->>CoreElementLoader: Resolve groups and classify dtypes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zarrextra/src/treeNodes.ts`:
- Around line 120-188: Guard the V3_DTYPE_NAMES and V2_DTYPE_NAMES lookups in
normalizeDtype with Object.hasOwn before returning matched values, so inherited
Object.prototype names are treated as unsupported and return undefined. Add
regression coverage for normalizeDtype('constructor') and equivalent
prototype-property inputs if the existing test structure supports it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84715576-c91b-4528-a438-fc54ce394294
📒 Files selected for processing (14)
.changeset/zarr-tree-node-guards.mddocs/docs/core/internals.mdxdocs/docs/zarrextra/_category_.jsondocs/docs/zarrextra/overview.mdxdocs/docs/zarrextra/tree-nodes.mdxpackages/core/src/index.tspackages/core/src/models/VAnnDataSource.tspackages/core/src/models/index.tspackages/core/src/types.tspackages/zarrextra/README.mdpackages/zarrextra/src/index.tspackages/zarrextra/src/treeNodes.tspackages/zarrextra/src/types.tspackages/zarrextra/tests/treeNodes.spec.ts
Both tables are object literals indexed with a name that came out of a store, so `constructor` is as possible an input as `float64`. Unguarded, the lookup answers with `Object` — truthy, so it escapes as if it were a real value. For `normalizeDtype` that is worse than a wrong answer: `getArrayDtype` hands back a function, and the next thing core does with it is `isTextDataType`, which throws `TypeError: dtype.startsWith is not a function`. Classifying a column reports "unknown" for every dtype it does not model; it should not crash for this one. `OBS_KIND_BY_ENCODING` in core has the same shape and returns `Object` as a `TableColumnKind`. `Object.hasOwn` on all three lookups, matching what `getChildNode` already does for the same reason one function up. Regression tests cover both packages and fail without the fix with exactly those two symptoms. Also balances the docs note on zod: array metadata is read with a bare JSON.parse and typed as an unvalidated record, so validating more at that boundary is at least as live an option as dropping the dependency. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`vitest.config.ts` lists the packages as projects so each runs under its own `vite.config.ts`, and only the integration project aliased workspace packages to their sources. Every package project therefore resolved them the normal way: through the workspace link, to `dist`. So a test in `core` importing `zarrextra` was testing whatever was last built. Editing `packages/zarrextra/src` changed nothing until a rebuild, and the suite went on passing — or failing — against the previous build with no hint that the source under the cursor was not the source under test. That is exactly how the guard added in 79d68cd appeared not to work: the fix was in `src`, the assertion was running against `dist`, and the stack trace pointed at `src` anyway because the bundle carries a sourcemap. `vis` already avoided this with `createWorkspaceSourceAliases`. Applies the same helper in `core` and `layers`, which are the other two packages whose tests import a sibling by name, and points the integration project at the helper too rather than a second hand-maintained list that was already missing entries. Harmless for `build`: rollup consults `external` with the unresolved specifier, so the alias never applies there. Verified after the change — `core`'s bundle still imports `from "zarrextra"` and inlines none of it, and `layers` still imports `from "@spatialdata/core"`. Verified the gap is actually closed by breaking `zarrextra/src` with `dist` left intact: `core`'s test now fails, where before it passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the previous commit. Neither package imports a sibling from a test today, so this changes nothing now — the point is that the first test which does should not be the one that has to discover the trap. The reason these two were held back was that the alias sits in `resolve` and so applies to `build` as well, and both are built by `dev` (`vite build --watch`). That turns out to be safe for the same reason it was in `core` and `layers`: each externalizes exactly the sibling specifier it imports — `@spatialdata/core` for react, `zarrextra` for avivatorish — and rollup consults `external` with the unresolved specifier, so the alias never fires. Confirmed by checksum: both bundles are byte-identical before and after. Confirmed the aliases do take effect where they are meant to, by adding a probe export to `zarrextra/src` and `core/src` that no `dist` contained and asserting on it from a test in each package. Caveat for later: a string entry in `external` matches the exact specifier only, so importing a *subpath* — `zarrextra/workers` — without adding it there would let the alias inline that source into the bundle. Neither package does today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes the caveat noted in the previous commit. A string entry in `external` matches the exact specifier only, so `zarrextra/workers` would have been left to resolve — and with the workspace source alias now in this config, resolving it means inlining that source into the bundle. The regex form matches what `vis` already uses. Verified both halves rather than assuming: with a `zarrextra/workers` import added to the package's source, the regex keeps it external and inlines none of it, and reverting to the string entry inlines it. The bundle is byte-identical before and after the change as things stand, since nothing imports a subpath yet. Also takes biome's import-order fix while here — pre-existing, and unflagged because the `lint:biome` gate only covers `packages/*/src`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same change as the previous commit, for the one config that externalizes by function rather than by list. `rollupExternals.has(id)` matched the exact specifier only, so a subpath entry point — `zarrextra/workers`, `zod/v4` — would have been resolved and bundled, and with the workspace source aliases now in this config, resolving `zarrextra/workers` means inlining a sibling's source. Spelled as a predicate rather than an array of regexes because the surrounding `external` is already a function; the matching is what `/^name(?:\/.*)?$/` means. Byte-neutral today. `apache-arrow/vector` is the only subpath core imports and both imports are `import type`, so it is erased before rollup sees it — the bundle is identical before and after. Verified the change bites by making that import a value import: the predicate keeps the specifier external, while the exact-match form resolves it into a chunk. Leaves alone, as a separate question: `@math.gl/core`, `earcut` and `ol` are declared runtime dependencies that this config does not externalize at all, so they are bundled into `dist` today. That may well be deliberate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`@math.gl/core`, `earcut` and `ol` were declared `dependencies` that the build did not externalize, so they were bundled into `dist`. That is the arrangement that puts two copies of a library in one application: `@math.gl/core` is also a direct dependency of `layers`, `vis` and `avivatorish`, and `Matrix4` instances have to survive being passed between them. All three are ordinary `dependencies`, so consumers install them transitively and need to do nothing. `ol` is reached only as `ol/format/WKB.js`, so it is external only by way of the subpath matching added in the previous commit — a plain entry would not have matched it. The bundle gets meaningfully smaller: `index.js` 174.6 kB -> 149.5 kB, and the tessellation chunk 86.2 kB -> 6.7 kB, which was almost entirely earcut and the OpenLayers WKB parser. Checked the worker entry first, since it is the one output where this could break at runtime: `points-worker.js` is loaded with `new Worker(new URL(...))` straight from disk, with no consumer bundler in the path, so a bare specifier there would be unresolvable. Its dependency graph does not reach any of the three — it still imports nothing, and is byte-identical. Verified end to end with `tests/production/browser`, which builds a consumer app against `dist` and renders polygon shapes — the exact path through earcut and the WKB decoder. It builds and passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
changed the title
Merged
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.
Closes #97.
The problem
ZarrTreeadmits a group or aLazyZarrArrayat every key and shipped no way to tell them apart, so consumers hand-rolled the check — and the obvioustypeof node === 'object'test is wrong, because a lazy array is an object too and its own properties (get) then read as child keys. That had been open-coded in four places.What landed
zarrextra/treeNodes.ts— the discrimination itself:isLazyZarrArray/isZarrGroup, discriminating onZARRAY_KEY.getChildNode/getChildGroup/getChildArray— "the node at this path, if it is the kind I need", which is the shape most call sites actually want. Own properties only, sogetChildGroup(tree, '__proto__')cannot resolve toObject.prototypeand pass for a group.getNodeAttrs/getArrayMetadatafor the symbol-keyed payloads.getArrayDtype/normalizeDtype— the wrinkle from the first issue comment. v2's numpy typestrings and v3's names fold into one vocabulary:zarrita's ownDataType, not a bare string, so a check made against tree metadata and the same check made against an opened array cannot disagree. A test asserts that directly against a real store.isTextDataType— one definition of "does this need decoding to strings", covering v3string, v2 fixed-width unicode/bytes andv2:object.zarrita'sisDataType(dtype, 'string')excludes the last; testing for one spelling without the other is what makes a reader hand back raw integer codes where labels were expected.AnnDataSource.hasTextValuesnow calls it instead ofarr.is('string') || arr.is('object'), so the kind a UI sees before loading and the decoding the column gets on load come from one place.Typing.
LazyZarrArray'sZARRAY_KEYpayload isZarrArrayMetadatainstead of an untyped record, sodtypeanddata_typecan no longer be read without narrowing.Sweep in
@spatialdata/core.parsedis narrowed to a group once inAbstractElement, so no element subclass sees the union;classifyObsColumnNode,getObsGroupandloadElementsdrop their casts.readNullableArray,isNullableEncodingandNULLABLE_ENCODING_KINDSare now public — the second issue comment's point that guards make "is this group a categorical or a nullable column?" expressible but only the semantics make it answerable.Docs. A
zarrextrasection covering the store, the tree model and the node API, plus an honest note on the package's weight relative tozarrita. Also corrects the core internals page, which pointed at azarrUtils.tsmodule and atryConsolidatedfunction that no longer exist.Decisions on the issue's open questions
types.tsstays type-only.ZarrTreebecome a discriminated union? No — kept the guards, but typed theZARRAY_KEYpayload asZarrV2ArrayNode | ZarrV3ArrayNode | ZAttrsAny. That buys the compile error that matters without the breaking change to the tree shape. The third member is deliberate: this is unvalidated JSON, and zarr v3 permits extension dtypes written as objects, so a strict two-member union would either lie or have to fail the whole store open over a node nobody asked about.AbstractElement.parsedbe narrowed at construction? Yes.Not in scope
python/v0.8.0/env dir plus fixture regeneration. Worth flagging that the current net is genuinely blind here: the guards are covered against v2 and v3 array nodes and categorical groups, but nothing intest-fixtures/contains a nullable group.packages/zarrextra/src/zarrSchema.tshas no callers outside its own spec. Left alone; worth its own issue.One judgement call
getArrayDtypereturnsZarrDataType = zarr.DataType | 'float16'.zarritaadmitsfloat16only when the type environment declaresFloat16Array, which the repo'sES2022lib does not — without the widening, float16 columns would have silently regressed fromnumericto unclassified. WhereFloat16Arrayis declared the union is exactlyzarr.DataType.Also in here: packaging and test resolution
The last five commits are a separate thread that grew out of the review, kept
here rather than split off. They touch build and test config only — no library
code — and each was verified against build output rather than assumed.
1660bd2dist, not source, so editingpackages/zarrextra/srcchanged nothing until a rebuild. This is why the guard in79d68cdappeared not to work.visalready avoided it;coreandlayersnow use the same helper, and the integration project shares it instead of a second, incomplete list.8caa2a9reactandavivatorish. Nothing there imports a sibling from a test yet — the point is that the first test which does should not have to rediscover the trap.fb79fddavivatorishexternalizedzarrextraas a string, which matches the exact specifier only. With the source alias now in that config, azarrextra/workersimport would have been inlined into the bundle. Regex form, asvisalready uses.d2524a5core, which externalizes by function rather than by list.2c40e98@math.gl/core,earcutandolwere declareddependenciesthat the build did not externalize, so they were bundled.@math.gl/coreis also a direct dependency oflayers,visandavivatorish— twoMatrix4implementations in one app.index.js174.6 kB → 149.5 kB; the tessellation chunk 86.2 kB → 6.7 kB.Every bundle that should not have changed is byte-identical, and the ones that
should are accounted for above.
points-worker.jswas checked separately: it isloaded with
new Worker(new URL(...))straight from disk with no consumerbundler in the path, so a bare specifier there would be unresolvable — its graph
reaches none of the externalized packages and it is unchanged.
Verification
pnpm test:unit— 811 passing (15 new, inpackages/zarrextra/tests/treeNodes.spec.ts).pnpm test:integration— 25 passing across the v0.5.0 / v0.6.1 / v0.7.2 fixtures, exercising both zarr generations through the real reader.pnpm build,pnpm lint:biome,pnpm lint:react— clean.pnpm -F docs build— clean underonBrokenLinks: 'throw'.Separately measured while reviewing
hasTextValues: it issues no store reads on either fixture —zarritaStoreis wrapped bywithConsolidatedMetadata, which answers metadata keys from memory. The one metadata read observed on the v2 fixture comes fromgetJson, which strips to a directory and opens without akind, so zarrita tries.zarrayfirst and takes a 404 on a node that is a group. Over HTTP that is a real round-trip per categorical column — left alone here, but worth a look.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
zarrextraAPIs.Bug Fixes