TDD-driven fixes from comprehensive review: 14 bugs, 2 optimizations - #8
Conversation
Pulls in dClimate/jaxray#review/tdd-followups (typed-array flat read path, CF calendar decoding, zone-less UTC time parsing, where(NaN) semantics) ahead of publishing, so the upcoming bugfix pipelines land against current jaxray behavior. Baseline verified: build clean, lint clean, 111/111 tests passing.
…on tests Follow-ups from grok review and fable adversarial review of ebf6562/2216c9c: - Replace O(n²) Array.includes dedupe with Set (measured ~230ms on a 43200-point axis with the old approach) - Pin descending-grid-order preservation and one-axis-empty behavior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins vectorized per-point selection along a 'point' dim, nearest-grid snapping when snapToGrid=true, and NoDataFoundError for off-grid points when snapToGrid=false — all currently broken because points() passes DataArray selection values jaxray ignores. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the ignored DataArray-valued sel with per-point scalar sel and assembles results along a 'point' dimension with point-aligned lat/lon coords. snapToGrid=true snaps without a distance cap; snapToGrid=false enforces the tolerance and maps misses to NoDataFoundError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins: spatial trim to the circle's bounding rows/cols (dead today since where() fills null and isNaN(null) is false), time-dim preservation (validity scan currently isNaN()'s arrays and returns empty), and the lazy zarr path (where() on lazy operands currently throws). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… trim Bbox pre-filter via isel before any masking (no more whole-globe haversine), trim derived from the mask itself instead of the dead isNaN(null) scan, and where() applied last on the small materialized region. Variables are transposed back after jaxray's condition-first where broadcasting so dim order (e.g. time/lat/lon) is preserved. Also corrects the legacy proper-circle test: its 10 km radius could never span 0.1-degree (~11.12 km) latitude rows — it only passed because the old trim was dead code returning the untrimmed grid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-ups from grok review and fable adversarial review of 630b285 (both reproduced): NaN silently defeated snapToGrid=false via jaxray's findNearestIndex NaN comparisons, undefined/null elements were skipped by sel and resurrected the silent-full-grid symptom, and a latitudeKey/longitudeKey typo returned the entire grid untouched. Validates element finiteness and key existence up front, mirroring circle(). The custom-key test now uses a dataset that actually has both custom dims and asserts the selected value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that loadDataset({autoConcatenate: true}) concatenates part1/part2
variants carrying dclimate:concatPriority instead of throwing
'Multiple variants available' from the variant-less
resolveDatasetFromStac call in the concatenable branch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolveDatasetFromStac was called with the always-undefined request.variant on the concatenable branch, throwing 'Multiple variants available' for variant sets outside the no-variant fallback list. Resolving with the first discovered concatenable variant yields the same collection/organization ids without the crash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that an unprefixed collection name plus organization (era5 + ecmwf, resolving to ecmwf_era5) returns the concatenable items instead of [], which today silently disables autoConcatenate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six confirmed defects fixed: over-the-pole circles now widen to the
full longitude range (and clamp latitude to the pole), longitude bbox
membership uses modular angular difference so 0-360 and dateline grids
keep the wrap side, longitude extent uses asin(sin(r/R)/cos(lat))
instead of the flat cos scaling that under-covers high latitudes, the
bbox shares haversine's 6371 km Earth radius plus an epsilon so it is
a strict superset of the disk, per-variable mask coords avoid jaxray's
Dataset coord-rounding mismatch crash on float32 grids, and
where({keepAttrs: true}) preserves variable attrs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
selectCollectionFromCatalog already resolves unprefixed collection names (era5 + ecmwf -> ecmwf_era5); the item loop now compares item-id prefixes against that resolved id instead of the raw argument, so autoConcatenate no longer silently degrades to a single variant (or crashes) for unprefixed requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-ups from grok and fable adversarial reviews of 3258b60/149500c: variants without an explicit dclimate:concatPriority are excluded from auto-concatenation (the documented DatasetVariantConfig contract — previously any multi-variant dataset merged silently with priority 0), and loadAndConcatenateVariants sorts by concatPriority so metadata.concatenatedVariants/cid reflect concatenation order rather than catalog discovery order; concatDimension is now populated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins xarray-style slice clamping: endpoints outside the axis clamp to the overlapping data instead of throwing InvalidSelectionError, and a range with no overlap raises NoDataFoundError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Endpoints are compared on a parsed timeline and the selection is made by index, so ranges overlapping the axis return the in-range data (xarray label-slice semantics) instead of throwing on exact-match misses; an empty intersection now raises NoDataFoundError. The legacy integration test expecting InvalidSelectionError for a far-future range encoded the old exact-match behavior and now pins NoDataFoundError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up from grok and fable adversarial reviews: item ids are parsed
by stripping the resolved collection id as a prefix (mirroring
resolveDatasetFromStac) instead of split('-'), so collection and
dataset names containing hyphens no longer silently disable
auto-concatenation. Pins the link-level concatPriority fallback and
the single-opted-in-variant case while here.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that a per-call options.gatewayUrl gets its own catalog rather than whatever the client cached from a previous gateway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A catalog cached from one gateway is no longer served for calls targeting a different options.gatewayUrl; a gateway mismatch is a cache miss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins both directions: a rootCid-pinned load must not be served the cached latest catalog, and a pinned load must not poison later latest-version calls for the TTL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-ups from grok and fable adversarial reviews of abb3ee6 (both confirmed with repros): timezone-less datetime coords (jaxray's zarr formatDate output) are now parsed as UTC instead of host-local Date.parse semantics, and numeric time axes convert through the coordinate's CF units via jaxray's cfTimeToDate instead of comparing CF offsets against epoch-milliseconds (which silently returned the whole dataset for out-of-range requests). Numeric axes without units accept only numeric endpoints. Regressions run green under TZ=America/New_York. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pinned rootCid loads no longer receive the cached latest catalog, and a pinned load no longer poisons later latest-version lookups for the TTL — the cache key is now stac:<gateway>:<rootCid|latest>. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up from the fable adversarial review of c00849d: the client slot duplicated loadStacCatalog's module cache (same TTL, same gateway keying) and re-stamping a module-cache hit let catalogs live up to ~2x the TTL. getStacCatalog now delegates caching entirely to the module cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that a requested shardReadMode reaches openDatasetFromCid for every concatenated variant instead of silently reverting to sparse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each concatenated variant's openDatasetFromCid call now receives options.shardReadMode instead of silently reverting to sparse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…otCid Follow-up from grok and fable adversarial reviews of 56c2388: the stored entry.rootCid was write-only, and an empty-string rootCid now keys the same slot as latest, matching loadStacCatalog's rootCid || getRootCatalogCid() resolution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that a variable-less dataset is empty, ensureHasData throws NoDataFoundError for it, and an out-of-range bounds selection rejects like the point path does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A Dataset with no data variables now counts as empty, so empty spatial selections raise NoDataFoundError via ensureHasData like the point path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins concurrent fetching of independent catalog branches (currently maxConcurrency is exactly 1 across ~30 fetches) while also pinning traversal completeness and per-node error tolerance, which the parallelization must preserve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Organizations, collections, and items fan out with Promise.all instead of three nested one-at-a-time awaits (~139 serial fetches on the real catalog; a review prototype measured 3603ms -> 104ms at 25ms RTT). Ordering follows link order via mapped results and per-node error tolerance (warn + skip) is preserved, both pinned by the red test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-ups from grok and fable adversarial reviews of a1a81d5: gateway fetches are limited to 12 in flight (uncapped fan-out fired ~200 simultaneous requests; throttled responses would be warn-skipped into a silently partial catalog cached for the TTL — the limiter guards only the fetch so nested levels cannot deadlock), an org whose collection listing is malformed stays in catalog.organizations with zero collections as the serial walk did, and the regression tests now pin exact link order, thrown (not just non-ok) failures at every level, and the concurrency cap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins overlap-splitting correctness (priority variant wins the overlap, exact boundaries) and a <250ms budget for two 30,000-step variants — currently ~550ms via full index-array isel plus per-step linear date parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The overlap split point is found by binary search over once-parsed timestamps instead of a per-step linear scan (O(n^2) across variants); two 30,000-step variants now concatenate well under the 250ms pin (previously ~550ms here, 710ms in the review's measurement). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins desired behavior for: ClientOptions.rootCid avoiding the hardcoded public discovery endpoint, resolveCidFromStacServer following STAC next links past the first page, haversine rejecting mismatched multi-element array lengths instead of modulo-cycling phantom pairs, and classifyRetrievalError mapping undici 'fetch failed' / browser 'Failed to fetch' to connection_error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- getRootCatalogCid takes an optional discovery endpoint and ClientOptions.rootCid pins the catalog version so private-gateway clients never contact the public discovery endpoint - resolveCidFromStacServer follows STAC rel:next links (GET href, 10-page cap) instead of silently missing items past the first page - haversine throws RangeError on mismatched multi-element array lengths instead of modulo-cycling phantom coordinate pairs - classifyRetrievalError unwraps error.cause (depth-capped) and maps undici 'fetch failed' / browser 'Failed to fetch' to connection_error Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up from grok and fable adversarial reviews of 4cf1a04: the binary-search split assumes ascending coordinates; descending or unsorted axes now throw (cheap first/last probes — full validation would reintroduce the per-step parse cost) instead of silently dropping the variant, and the perf regression asserts output shape and boundary values so a fast-but-wrong split cannot pass on speed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… disables the server path Follow-ups from grok and fable adversarial reviews of 6957bc9 (the serious ones live-verified against api.stac.dclimate.net): the real server paginates with POST next links carrying a token body, so next links are now re-POSTed (honoring method/body/merge) with hrefs resolved against the server URL and null link entries tolerated; a ClientOptions.rootCid pin now disables the STAC server fast path, which only serves the latest catalog and silently bypassed the pin in default config; classifyRetrievalError's fetch-failure match is anchored to undici/browser exact messages and survives throwing cause getters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change revises STAC catalog loading, root-CID handling, dataset time selection, variant concatenation, spatial shape selection, retrieval error classification, and empty-dataset behavior. It adds regression and performance tests covering these updated flows. ChangesCore data flows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CatalogLoader
participant Gateway
participant DatasetOpener
Client->>CatalogLoader: request catalog with optional rootCid
CatalogLoader->>Gateway: fetch root, organizations, collections, and items
CatalogLoader-->>Client: return resolved catalog
Client->>DatasetOpener: open ordered variant CIDs
DatasetOpener-->>Client: return datasets for concatenation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 594382e86b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (start < end) { | ||
| const midpoint = start + Math.floor((end - start) / 2); | ||
| if ( | ||
| getComparableCoord(coords, midpoint, comparableCoords) > afterComparable |
There was a problem hiding this comment.
When a lower-priority variant has a non-monotonic but not descending axis, this binary search can return a later split point and silently drop valid coordinates instead of hitting the unsorted-axis guard; for example with previous coverage ending at 4 and next coordinates [1, 5, 3, 6], the search returns index 3, so the value at 5 is omitted. Since the new path relies on sorted input, it should either fully validate ascending order or fall back to a linear scan when unsorted data is possible.
Useful? React with 👍 / 👎.
| "homepage": "https://github.com/dClimate/dclimate-client-js#readme", | ||
| "dependencies": { | ||
| "@dclimate/jaxray": "^0.6.11", | ||
| "@dclimate/jaxray": "github:dClimate/jaxray#review/tdd-followups", |
There was a problem hiding this comment.
Because package.json is what npm consumers resolve and the lockfile is not shipped with the package, this mutable review branch will be resolved at install time; if the branch moves, is deleted, or GitHub is unavailable, installs of @dclimate/dclimate-client-js can change or fail. Please depend on an immutable published version or pinned commit before publishing this client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR delivers a large set of test-driven correctness fixes and performance improvements across the client’s spatial selection utilities, STAC catalog/server resolution, variant concatenation, and time-range selection logic.
Changes:
- Fixes and expands spatial selection behavior (
points,circle,rectangle) and adds extensive regression coverage undertests/review-fixes/. - Improves STAC behavior (root CID pinning, cache keying, pagination support, and faster catalog walking with bounded concurrency).
- Enhances time-range selection semantics (endpoint clamping, UTC handling, numeric/CF time support) and improves error classification/validation.
Reviewed changes
Copilot reviewed 31 out of 33 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Switches @dclimate/jaxray resolution to a GitHub tarball for the review branch. |
| package.json | Pins @dclimate/jaxray to a GitHub branch ref instead of a registry semver. |
| .gitignore | Ignores .tdd-logs/ artifacts. |
| tests/shapes.test.ts | Updates/extends shape tests (custom coord keys, non-finite inputs, circle radius correction). |
| tests/geotemporal-dataset.test.ts | Updates expectation to NoDataFoundError for out-of-range timeRange. |
| tests/review-fixes/timerange-endpoint-clamping.test.ts | Adds endpoint clamping + UTC/CF/numeric time axis regression coverage. |
| tests/review-fixes/stac-server-pagination.test.ts | Adds pagination coverage including POST+token next links and null link entries. |
| tests/review-fixes/stac-cache-rootcid.test.ts | Adds cache isolation tests for pinned vs latest root CID. |
| tests/review-fixes/root-cid-endpoint.test.ts | Adds tests for custom discovery endpoint and client rootCid behavior. |
| tests/review-fixes/rectangle-cartesian.test.ts | Adds regression tests for avoiding cartesian duplicates + descending grids. |
| tests/review-fixes/points-vectorized.test.ts | Adds regression tests for vectorized point selection semantics. |
| tests/review-fixes/isempty-empty-dataset.test.ts | Adds regression tests for empty Dataset({}) handling. |
| tests/review-fixes/haversine-length-mismatch.test.ts | Adds regression tests for array-length validation in haversine. |
| tests/review-fixes/concat-variants-perf.test.ts | Adds overlap-splitting correctness + performance pin coverage. |
| tests/review-fixes/concat-shard-read-mode.test.ts | Adds regression ensuring shardReadMode is propagated through concat loads. |
| tests/review-fixes/concat-items-resolved-id.test.ts | Adds regression tests for concat item parsing (resolved id, hyphens, opt-in priority). |
| tests/review-fixes/client-cache-gateway.test.ts | Adds regression test for gateway-isolated catalog behavior. |
| tests/review-fixes/classify-retrieval-error.test.ts | Adds coverage for undici/browser fetch-failure classification. |
| tests/review-fixes/circle-real-datasets.test.ts | Adds real-dataset fixtures for circle trimming + lazy support. |
| tests/review-fixes/circle-geometry.test.ts | Adds geometry regression coverage (poles, 0–360 wrap, float32 coords, attrs). |
| tests/review-fixes/catalog-walk-parallel.test.ts | Adds bounded-concurrency + traversal correctness tests for catalog walk. |
| tests/review-fixes/autoconcat-variant-crash.test.ts | Adds regression for auto-concat variant selection/metadata ordering. |
| src/types.ts | Adds ClientOptions.rootCid documentation. |
| src/client.ts | Wires rootCid to catalog load; fixes auto-concat resolution and shardReadMode propagation; orders concat variants by priority. |
| src/geotemporal-dataset.ts | Reworks timeRange selection (clamping, UTC parsing, CF/numeric support) and empty-dataset detection. |
| src/shapes/points.ts | Replaces non-vectorized selection with per-point selection + coordinate validation. |
| src/shapes/circle.ts | Rewrites circle selection to bbox-prefilter + trimmed haversine mask with lazy-safe compute ordering. |
| src/shapes/rectangle.ts | Fixes rectangle selection to avoid cartesian-product duplication and handle empty-axis results. |
| src/stac/stac-server.ts | Adds pagination support via STAC next links (GET and POST+body). |
| src/stac/stac-catalog.ts | Adds rootCid-aware cache keying; parallel catalog walk with bounded concurrency; fixes concatenable item parsing. |
| src/math/haversine.ts | Exports EARTH_RADIUS_KM and adds strict array-length validation. |
| src/instrumentation.ts | Improves connection-error classification by unwrapping cause and anchoring message matches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "homepage": "https://github.com/dClimate/dclimate-client-js#readme", | ||
| "dependencies": { | ||
| "@dclimate/jaxray": "^0.6.11", | ||
| "@dclimate/jaxray": "github:dClimate/jaxray#review/tdd-followups", |
| links?: Array<{ | ||
| rel: string; | ||
| href: string; | ||
| method?: string; | ||
| body?: Record<string, unknown>; | ||
| merge?: boolean; | ||
| }>; |
| if (coordinates.length === 0) { | ||
| throw new Error("Cannot select from an empty coordinate array"); | ||
| } |
| if (!coordinates.every((coordinate) => typeof coordinate === "number")) { | ||
| throw new Error("Point selection requires numeric spatial coordinates"); | ||
| } |
| async timeRange( | ||
| range: TimeRange, | ||
| range: TimeRangeInput, | ||
| dimension = "time", | ||
| ): Promise<GeoTemporalDataset> { |
| const comparable = toComparable(coords[index]); | ||
| cachedCoords[index] = comparable; | ||
| return comparable; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/review-fixes/classify-retrieval-error.test.ts (1)
5-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name implies cause unwrapping but match happens at depth 0.
The top-level message
"fetch failed"matches the anchored regex on the first iteration, so theECONNREFUSEDcause is never actually inspected. The test validates the anchored regex match but not the nested-cause traversal. Consider adding a separate test where the top-level message is non-matching and only a deeper cause contains a connection-error pattern, to exercise thedepth > 0path.🧪 Suggested additional test for nested cause traversal
+ it("classifies an error whose nested cause contains ECONNREFUSED", () => { + const root = new TypeError("some unrelated wrapper error"); + const cause = Object.assign( + new Error("connect ECONNREFUSED 127.0.0.1:1"), + { code: "ECONNREFUSED" }, + ); + (root as Error & { cause?: unknown }).cause = cause; + expect(classifyRetrievalError(root)).toBe("connection_error"); + }); + + it("returns 'error' when a throwing cause getter prevents deeper inspection", () => { + const root = new TypeError("some unrelated wrapper error"); + Object.defineProperty(root, "cause", { + get() { throw new Error("getter exploded"); }, + }); + expect(classifyRetrievalError(root)).toBe("error"); + });🤖 Prompt for 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. In `@tests/review-fixes/classify-retrieval-error.test.ts` around lines 5 - 19, Add a separate test near the existing classifyRetrievalError case where the top-level error message does not match connection-error patterns and only a nested cause contains one. Construct the cause chain to require traversal beyond depth 0, then assert classifyRetrievalError returns "connection_error"; keep the existing undici fetch-failure test unchanged.tests/review-fixes/isempty-empty-dataset.test.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent import extension convention across test files.
This file imports source modules with
.tsextensions (L3-5), whiletests/review-fixes/classify-retrieval-error.test.tsuses.jsextensions for the same kind of import. Pick one convention consistently — typically.jsfor ESM/NodeNext resolution — to avoid confusion and potential resolution issues across build targets.🤖 Prompt for 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. In `@tests/review-fixes/isempty-empty-dataset.test.ts` around lines 1 - 5, Standardize the source-module imports in this test to use the project’s `.js` ESM/NodeNext extension convention, updating the imports for NoDataFoundError, GeoTemporalDataset, and DatasetMetadata while leaving the package imports unchanged..gitignore (1)
5-5: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winIgnore common environment-file variants.
.env.local,.env.production, and similar files remain eligible for commit.Proposed fix
.env +.env.* +!.env.example +!.env.*.example🤖 Prompt for 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. In @.gitignore at line 5, Update the .env ignore rule in .gitignore to cover common environment-file variants, including .env.local, .env.production, and similar suffixed files, while retaining the existing exclusion of .env.tests/review-fixes/root-cid-endpoint.test.ts (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the casts so these tests enforce the new public API types.
The
unknowncasts allow the tests to compile even if the endpoint parameter orClientOptions.rootCiddisappears later. Call and annotate these APIs directly.Proposed fix
- const getRootCid = getRootCatalogCid as unknown as ( - endpoint?: string, - ) => Promise<string>; - const cid = await getRootCid(discoveryEndpoint); + const cid = await getRootCatalogCid(discoveryEndpoint); - const clientOptions = { + const clientOptions: ClientOptions = { gatewayUrl, stacServerUrl: null, rootCid: pinnedRootCid, ipfsElements: {} as IpfsElements, - } as unknown as ClientOptions; + }; const pinnedDefaultServerClient = new DClimateClient({ gatewayUrl, rootCid: pinnedRootCid, ipfsElements: {} as IpfsElements, - } as unknown as ClientOptions); + });Also applies to: 173-178, 203-207
🤖 Prompt for 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. In `@tests/review-fixes/root-cid-endpoint.test.ts` around lines 55 - 58, Remove the unknown/function-type cast around getRootCatalogCid and call it directly with discoveryEndpoint, allowing TypeScript to validate the public endpoint parameter. Apply the same change to the additional locations and remove any equivalent casts around ClientOptions.rootCid, using direct API calls and annotations so the tests enforce the public types.
🤖 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 `@package.json`:
- Line 54: Update the `@dclimate/jaxray` dependency declaration in package.json to
reference an immutable commit SHA or release tag instead of the mutable
review/tdd-followups branch, and synchronize the corresponding yarn.lock entry.
In `@src/actions/concatenate-variants.ts`:
- Around line 78-109: Replace the first/last-coordinate probes in the
concatenation flow with a complete validation that nextCoords is monotonically
ascending before calling findSplitIndex. Reject any out-of-order adjacent
coordinates with the existing descending/unsorted coordinate error, then
preserve the binary-search split behavior only for validated axes.
In `@src/shapes/circle.ts`:
- Around line 101-103: Update the radius validation in the circle selection
logic to reject all non-finite values, including NaN and Infinity, while
continuing to reject zero and negative radii. Use the existing
InvalidSelectionError path and preserve the current behavior for finite positive
radii.
In `@src/stac/stac-catalog.ts`:
- Around line 417-475: Track traversal failures across the item, collection, and
organization loading branches in the catalog-loading flow, including the catch
handlers around item requests, collection processing, and organization
processing. Before calling setCachedCatalog, only cache the catalog when no
branch failed; otherwise return the assembled result without caching, or
propagate the load failure. Preserve the existing filtering and returned partial
data behavior if choosing to skip the cache.
- Around line 354-415: Update the limitedFetch usage in the organization,
collection, and item-loading paths so each limiter task includes both fetch()
and response.json() before releasing its slot. Preserve existing non-OK handling
and error behavior, and add a test using delayed response.json() that verifies
concurrent response-body reads never exceed MAX_CONCURRENT_CATALOG_FETCHES.
In `@src/stac/stac-server.ts`:
- Around line 116-125: Update the pagination loop in the STAC search flow around
nextLink so that finding a next link on MAX_STAC_SEARCH_PAGES - 1 throws a
pagination-limit error instead of breaking and returning incomplete results.
Preserve the existing break behavior when no next link exists, and continue
normally for earlier pages.
---
Nitpick comments:
In @.gitignore:
- Line 5: Update the .env ignore rule in .gitignore to cover common
environment-file variants, including .env.local, .env.production, and similar
suffixed files, while retaining the existing exclusion of .env.
In `@tests/review-fixes/classify-retrieval-error.test.ts`:
- Around line 5-19: Add a separate test near the existing classifyRetrievalError
case where the top-level error message does not match connection-error patterns
and only a nested cause contains one. Construct the cause chain to require
traversal beyond depth 0, then assert classifyRetrievalError returns
"connection_error"; keep the existing undici fetch-failure test unchanged.
In `@tests/review-fixes/isempty-empty-dataset.test.ts`:
- Around line 1-5: Standardize the source-module imports in this test to use the
project’s `.js` ESM/NodeNext extension convention, updating the imports for
NoDataFoundError, GeoTemporalDataset, and DatasetMetadata while leaving the
package imports unchanged.
In `@tests/review-fixes/root-cid-endpoint.test.ts`:
- Around line 55-58: Remove the unknown/function-type cast around
getRootCatalogCid and call it directly with discoveryEndpoint, allowing
TypeScript to validate the public endpoint parameter. Apply the same change to
the additional locations and remove any equivalent casts around
ClientOptions.rootCid, using direct API calls and annotations so the tests
enforce the public types.
🪄 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
Run ID: 4bca3990-7487-42e6-bc1c-5b9f11d0a7df
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
.gitignorepackage.jsonsrc/actions/concatenate-variants.tssrc/client.tssrc/geotemporal-dataset.tssrc/instrumentation.tssrc/math/haversine.tssrc/shapes/circle.tssrc/shapes/points.tssrc/shapes/rectangle.tssrc/stac/stac-catalog.tssrc/stac/stac-server.tssrc/types.tstests/geotemporal-dataset.test.tstests/review-fixes/autoconcat-variant-crash.test.tstests/review-fixes/catalog-walk-parallel.test.tstests/review-fixes/circle-geometry.test.tstests/review-fixes/circle-real-datasets.test.tstests/review-fixes/classify-retrieval-error.test.tstests/review-fixes/client-cache-gateway.test.tstests/review-fixes/concat-items-resolved-id.test.tstests/review-fixes/concat-shard-read-mode.test.tstests/review-fixes/concat-variants-perf.test.tstests/review-fixes/haversine-length-mismatch.test.tstests/review-fixes/isempty-empty-dataset.test.tstests/review-fixes/points-vectorized.test.tstests/review-fixes/rectangle-cartesian.test.tstests/review-fixes/root-cid-endpoint.test.tstests/review-fixes/stac-cache-rootcid.test.tstests/review-fixes/stac-server-pagination.test.tstests/review-fixes/timerange-endpoint-clamping.test.tstests/shapes.test.ts
| if (radiusKm <= 0) { | ||
| throw new InvalidSelectionError("Radius must be a positive number"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite radii.
NaN bypasses this check and returns an empty dataset, while Infinity can select the entire grid.
Proposed fix
- if (radiusKm <= 0) {
+ if (!Number.isFinite(radiusKm) || radiusKm <= 0) {
throw new InvalidSelectionError("Radius must be a positive number");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (radiusKm <= 0) { | |
| throw new InvalidSelectionError("Radius must be a positive number"); | |
| } | |
| if (!Number.isFinite(radiusKm) || radiusKm <= 0) { | |
| throw new InvalidSelectionError("Radius must be a positive number"); | |
| } |
🤖 Prompt for 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.
In `@src/shapes/circle.ts` around lines 101 - 103, Update the radius validation in
the circle selection logic to reject all non-finite values, including NaN and
Infinity, while continuing to reject zero and negative radii. Use the existing
InvalidSelectionError path and preserve the current behavior for finite positive
radii.
| if (!itemResponse.ok) { | ||
| console.warn(`Failed to load item from ${itemLink.href}: ${itemResponse.status}`); | ||
| return undefined; | ||
| } | ||
|
|
||
| return await itemResponse.json() as StacItem; | ||
| } catch (itemError) { | ||
| console.warn(`Error loading item ${itemLink.href}:`, itemError); | ||
| return undefined; | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| collection.items = itemResults.filter((item): item is StacItem => item !== undefined); | ||
| collection.organizationId = orgId; | ||
| collection.organizationTitle = link.title; | ||
| const category = collectionCategories.get(collection.id); | ||
| if (category) { | ||
| collection.category = category; | ||
| } | ||
|
|
||
| if (!itemResponse.ok) { | ||
| console.warn(`Failed to load item from ${itemLink.href}: ${itemResponse.status}`); | ||
| continue; | ||
| const datasetNames = datasetSlugs | ||
| .filter((slug) => slug.startsWith(`${collection.id}/`)) | ||
| .map((slug) => slug.split("/")[1]) | ||
| .filter(Boolean); | ||
| if (datasetNames.length) { | ||
| collection.datasetNames = datasetNames; | ||
| } | ||
|
|
||
| const item: StacItem = await itemResponse.json(); | ||
| items.push(item); | ||
| } catch (itemError) { | ||
| console.warn(`Error loading item ${itemLink.href}:`, itemError); | ||
| return collection; | ||
| } catch (collectionError) { | ||
| console.warn(`Error loading collection ${collectionLink.href}:`, collectionError); | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| collection.items = items; | ||
| collection.organizationId = orgId; | ||
| collection.organizationTitle = link.title; | ||
| const category = collectionCategories.get(collection.id); | ||
| if (category) { | ||
| collection.category = category; | ||
| } | ||
|
|
||
| const datasetNames = datasetSlugs | ||
| .filter((slug) => slug.startsWith(`${collection.id}/`)) | ||
| .map((slug) => slug.split("/")[1]) | ||
| .filter(Boolean); | ||
| if (datasetNames.length) { | ||
| collection.datasetNames = datasetNames; | ||
| } | ||
|
|
||
| collections.push(collection); | ||
| } catch (collectionError) { | ||
| console.warn(`Error loading collection ${collectionLink.href}:`, collectionError); | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| return { | ||
| organization, | ||
| collections: collectionResults.filter( | ||
| (collection): collection is StacCollection => collection !== undefined | ||
| ), | ||
| }; | ||
| } catch (orgError) { | ||
| console.warn(`Error loading organization ${link.href}:`, orgError); | ||
| return { organization, collections: [] }; | ||
| } | ||
| } catch (orgError) { | ||
| console.warn(`Error loading organization ${link.href}:`, orgError); | ||
| } | ||
| } | ||
| }) | ||
| ); | ||
| const loadedOrganizations = organizationResults.filter( | ||
| (result): result is NonNullable<typeof result> => result !== undefined | ||
| ); | ||
| const organizations = loadedOrganizations.map(({ organization }) => organization); | ||
| const collections = loadedOrganizations.flatMap((result) => result.collections); | ||
|
|
||
| catalog.collections = collections; | ||
| catalog.organizations = organizations; | ||
|
|
||
| setCachedCatalog(gatewayUrl, catalog, cid); | ||
| setCachedCatalog(gatewayUrl, rootCid, catalog); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not cache a catalog assembled after traversal failures.
Item, collection, and organization failures are swallowed as undefined, but the resulting partial catalog is still cached for one hour. A transient gateway error can therefore make valid datasets appear absent until TTL expiry.
Track whether any branch failed and skip setCachedCatalog for incomplete walks, or fail the load instead.
🤖 Prompt for 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.
In `@src/stac/stac-catalog.ts` around lines 417 - 475, Track traversal failures
across the item, collection, and organization loading branches in the
catalog-loading flow, including the catch handlers around item requests,
collection processing, and organization processing. Before calling
setCachedCatalog, only cache the catalog when no branch failed; otherwise return
the assembled result without caching, or propagate the load failure. Preserve
the existing filtering and returned partial data behavior if choosing to skip
the cache.
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
|
|
||
| // Find the index in nextDataset where coords start AFTER lastCombinedCoord | ||
| const splitIndex = findSplitIndex(nextCoords, lastCombinedCoord); | ||
| const splitIndex = findSplitIndex( |
There was a problem hiding this comment.
HIGH
The endpoint probes do not prove that nextCoords is sorted, so this binary search can silently drop valid data. For example, with coverage ending at 5 and coordinates [2, 100, 3, 4, 10], the checks pass but the split is 4, discarding 100. Validate full monotonicity or fall back to the linear scan for non-monotonic axes.
| "Numeric time axis contains a non-numeric value", | ||
| ); | ||
| } | ||
| return cfTimeToTimelineValue(coordinate, units, calendar); |
There was a problem hiding this comment.
HIGH
cfTimeToDate returns null for valid non-Gregorian dates that cannot be represented by Date, such as February 30 in a 360_day calendar. Because every coordinate is converted here, one such value makes all timeRange() calls fail, even for ranges that do not include it. Compare CF coordinates on a calendar-native ordinal timeline instead.
| }; | ||
| } else { | ||
| if ( | ||
| typeof range.start !== "number" || |
There was a problem hiding this comment.
MEDIUM
GeoSelectionOptions.timeRange still exposes only Date | string endpoints, so every type-correct select() call on an unannotated numeric axis reaches this error; the required numeric endpoints are rejected by TypeScript. With a simultaneous point selection, the resulting InvalidSelectionError is swallowed and the time filter is silently skipped. Widen the exported TimeRange type or retain endpoint coercion.
| resolvedCollection || request.collection || "", | ||
| request.dataset, | ||
| request.variant, | ||
| concatenableItems[0].variant, |
There was a problem hiding this comment.
MEDIUM
Auto-concatenation still fails for dataset names containing hyphens. getConcatenableItemsFromStac now recognizes IDs such as collection-wind-speed-part1, but this call immediately passes them to resolveDatasetFromStac, which parses the dataset as only wind via split("-")[0]. Use the same prefix-based parsing in the resolver or avoid reparsing the item ID here.
| variant: string; | ||
| } | ||
|
|
||
| const MAX_STAC_SEARCH_PAGES = 10; |
There was a problem hiding this comment.
MEDIUM
Pagination is silently truncated after ten pages even when the tenth response contains a valid next link. A target on any later page is therefore reported as missing. Follow links until exhaustion with cycle detection, or explicitly surface a pagination-limit error so callers can distinguish truncation from absence.
|
|
||
| // Find the index in nextDataset where coords start AFTER lastCombinedCoord | ||
| const splitIndex = findSplitIndex(nextCoords, lastCombinedCoord); | ||
| const splitIndex = findSplitIndex( |
There was a problem hiding this comment.
MEDIUM
The binary search assumes the entire axis is ascending, but only the first and last values are checked. For nextCoords = [4, 1, 5] and a combined endpoint of 3, this returns index 2 and silently drops the valid coordinate 4; the later guard does not fire. Validate adjacent coordinates or fall back to a safe scan for non-monotonic axes.
| // STAC API pagination: next links may be plain GET hrefs, or POST links | ||
| // carrying a token body (the dClimate server's shape) that must be | ||
| // re-POSTed — optionally merged with the original search body. | ||
| const nextUrl = new URL(nextLink.href, serverUrl).toString(); |
There was a problem hiding this comment.
MEDIUM
Relative next links are resolved against serverUrl instead of the current /search request URL. For example, ?token=x with https://host/stac resolves to /stac?token=x, not /stac/search?token=x, so valid STAC pagination can fetch the wrong endpoint. Track the current request URL and resolve each link against it.
| const datasetSlugs = extractDatasetSlugsFromOrgLink(link); | ||
| // The limiter guards only the fetch itself (not whole mapper bodies), so | ||
| // a parent level never holds a slot while awaiting its children. | ||
| const limitedFetch = createFetchLimiter(MAX_CONCURRENT_CATALOG_FETCHES); |
There was a problem hiding this comment.
MEDIUM
This limiter is created per catalog load, so it does not enforce the advertised 12-request cap across concurrent cold-cache calls. Because the cache stores no in-flight promise, N simultaneous loads can run 12 × N gateway fetches, triggering throttling and potentially caching partial catalogs. Deduplicate loads by cache key or share the limiter.
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 `@src/geotemporal-dataset.ts`:
- Around line 258-261: Update endpointToOrdinal to validate numeric
CoordinateValue inputs with a finite-number check before returning them. Reject
NaN and infinities through the existing invalid-selection error path rather than
allowing invalid bounds to reach the query and become NoDataFoundError; preserve
conversion for valid numeric and non-numeric values.
🪄 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
Run ID: fca157d1-3195-4fc7-9987-cf3f1711122c
📒 Files selected for processing (10)
src/actions/concatenate-variants.tssrc/geotemporal-dataset.tssrc/stac/stac-catalog.tssrc/stac/stac-server.tssrc/types.tssrc/utils.tstests/review-fixes/catalog-walk-parallel.test.tstests/review-fixes/concat-variants-monotonicity.test.tstests/review-fixes/stac-server-pagination.test.tstests/review-fixes/timerange-cf-calendar.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/actions/concatenate-variants.ts
- tests/review-fixes/catalog-walk-parallel.test.ts
- src/stac/stac-catalog.ts
- src/stac/stac-server.ts
| const endpointToOrdinal = (value: CoordinateValue): number => { | ||
| if (typeof value === "number") { | ||
| // Already a raw CF coordinate value on the axis's timeline. | ||
| return value; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite CF ordinal endpoints.
NaN or infinity bypasses toTimelineValue, produces invalid bounds, and is misreported as NoDataFoundError instead of an invalid selection.
Proposed fix
if (typeof value === "number") {
+ if (!Number.isFinite(value)) {
+ throw new TypeError("CF time range endpoints must be finite numbers");
+ }
// Already a raw CF coordinate value on the axis's timeline.
return value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const endpointToOrdinal = (value: CoordinateValue): number => { | |
| if (typeof value === "number") { | |
| // Already a raw CF coordinate value on the axis's timeline. | |
| return value; | |
| const endpointToOrdinal = (value: CoordinateValue): number => { | |
| if (typeof value === "number") { | |
| if (!Number.isFinite(value)) { | |
| throw new TypeError("CF time range endpoints must be finite numbers"); | |
| } | |
| // Already a raw CF coordinate value on the axis's timeline. | |
| return value; |
🤖 Prompt for 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.
In `@src/geotemporal-dataset.ts` around lines 258 - 261, Update endpointToOrdinal
to validate numeric CoordinateValue inputs with a finite-number check before
returning them. Reject NaN and infinities through the existing invalid-selection
error path rather than allowing invalid bounds to reach the query and become
NoDataFoundError; preserve conversion for valid numeric and non-numeric values.
| >, | ||
| variantName: string, | ||
| concatDim: string | ||
| ): number { |
There was a problem hiding this comment.
HIGH
String coordinates are sent to binary search without validating interior ordering. With a previous endpoint of Jan 3 and next coordinates [Jan 4, Jan 1, Jan 5], this returns index 2 and silently drops Jan 4; the endpoint check cannot detect it. Validate monotonicity or use the safe scan before slicing.
There was a problem hiding this comment.
this is super edge case and honestly not worth it. i doubt we would ever have this.
| : nextLink.body ?? body; | ||
| response = await fetch(nextUrl, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, |
There was a problem hiding this comment.
MEDIUM
Pagination ignores nextLink.headers and hard-codes only Content-Type. Headers are part of the STAC pagination link contract, so header-based cursors can repeat page one or fail, eventually causing an unnecessary IPFS fallback. Model and forward/merge the link headers for both POST and GET next requests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/review-fixes/stac-server-pagination.test.ts (1)
136-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest title overstates coverage — only GET cursor forwarding is exercised.
The test name says "on both GET and POST," but the mocked
nextlink always usesmethod: "GET"(Line 161), so the POST branch of cursor-header handling inresolveCidFromStacServer(which merges aContent-Typefloor differently than the GET branch) is never exercised here.♻️ Suggested fix: rename or add a POST variant
- it("forwards header-based cursors from the next link on both GET and POST", async () => { + it("forwards header-based cursors from the next link on GET", async () => {Consider adding a companion case with
method: "POST"in thenextlink to actually cover the POST cursor-header path.🤖 Prompt for 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. In `@tests/review-fixes/stac-server-pagination.test.ts` around lines 136 - 181, Update the pagination test around “forwards header-based cursors from the next link” so it no longer claims POST coverage while only using a GET next link, or add a companion case whose next link uses method "POST" and verifies the cursor plus POST-specific Content-Type handling in resolveCidFromStacServer.
🤖 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.
Nitpick comments:
In `@tests/review-fixes/stac-server-pagination.test.ts`:
- Around line 136-181: Update the pagination test around “forwards header-based
cursors from the next link” so it no longer claims POST coverage while only
using a GET next link, or add a companion case whose next link uses method
"POST" and verifies the cursor plus POST-specific Content-Type handling in
resolveCidFromStacServer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68277df5-424c-474b-b126-36157bcaa4e3
📒 Files selected for processing (2)
src/stac/stac-server.tstests/review-fixes/stac-server-pagination.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stac/stac-server.ts
| concatDim: string | ||
| ): number { | ||
| return typeof coords[0] === "string" | ||
| ? findSplitIndexBinary( |
There was a problem hiding this comment.
HIGH
String axes use binary search without validating interior ordering. For a previous endpoint of Jan 3 and [Jan 4, Jan 1, Jan 5], the split is index 2 and Jan 4 is silently dropped. Validate monotonicity or use the safe scan.
| resolvedCollection || request.collection || "", | ||
| request.dataset, | ||
| request.variant, | ||
| concatenableItems[0].variant, |
There was a problem hiding this comment.
MEDIUM
Auto-concatenation still fails for hyphenated dataset names. The updated item matcher accepts collection-wind-speed-part1, but resolveDatasetFromStac() parses the dataset as wind using split("-")[0] and throws here. Use the same prefix-based parsing in the resolver.
| ? normalizedValue | ||
| : normalizedValue instanceof Date | ||
| ? normalizedValue.getTime() | ||
| : Date.parse(normalizedValue); |
There was a problem hiding this comment.
MEDIUM
Date.parse() collapses sub-millisecond ISO timestamps to millisecond precision. Coordinates such as .000001Z and .000002Z compare equal, so a range targeting one silently returns both. Preserve fractional-second precision when building the timeline.
|
|
||
| let organization: StacOrganization; | ||
| try { | ||
| const orgResponse = await limitedFetch(() => fetch(orgUrl)); |
There was a problem hiding this comment.
MEDIUM
The limiter wraps only fetch(), which resolves after headers; response.json() runs after the slot is released. Consequently, arbitrarily many response bodies can still download and parse concurrently, defeating the 12-request cap. Keep fetch, status handling, and body parsing inside the limited task.
| catalog.organizations = organizations; | ||
|
|
||
| setCachedCatalog(gatewayUrl, catalog, cid); | ||
| setCachedCatalog(gatewayUrl, rootCid, catalog); |
There was a problem hiding this comment.
MEDIUM
Organization, collection, and item failures are swallowed as undefined, but the resulting partial catalog is cached for an hour here. A transient gateway failure therefore makes valid datasets appear absent until expiry. Track traversal failures and skip caching incomplete results, or fail the load.
| const endpointToOrdinal = (value: CoordinateValue): number => { | ||
| if (typeof value === "number") { | ||
| // Already a raw CF coordinate value on the axis's timeline. | ||
| return value; |
There was a problem hiding this comment.
LOW
Raw CF ordinal endpoints are returned without checking finiteness. NaN becomes a misleading NoDataFoundError, while infinite bounds can select unintended data. Reject non-finite numbers through the existing invalid-selection path.
Summary
This PR lands the full output of a comprehensive multi-agent review of dclimate-client-js: 14 proven bug fixes and 2 performance optimizations, every one implemented test-first. Three of the bugs disabled the library's main spatial-selection APIs outright; four more started as "worth a human look" suspicions and were proven red before fixing.
Each item went through a four-role TDD pipeline:
src/at the claimed defect (not from test scaffolding), committed before any fix..tdd-logs/<slug>/; committed only after independent re-verification.review:fixup round with regression tests.Gate: 169/169 tests green (28 files), eslint clean,
tscclean for both node and browser configs. Baseline before this PR: 112 tests. The 57 new tests are red-phase reproductions and review-driven regressions undertests/review-fixes/.The review pipeline earned its keep — several items had their first fix rejected by review and repaired in a fixup round, including one where the fix passed its red test but was proven broken against the production STAC server's actual pagination protocol.
Bug fixes
Critical — the spatial-selection APIs were broken
points-vectorizedpoints()passed DataArray objects asselvalues expecting xarray-style vectorized selection, which jaxray silently ignores → the full grid came back with integer indices in place of lat/lon coords, and thesnapToGridbranches were swapped (true did exact match, false did nearest). Now performs real per-point nearest-neighbor selection along apointdimension; tolerance misses raiseNoDataFoundError.circle-real-datasetsisNaN()'d whole arrays and returned an empty dataset; on lazy zarr-backed data (the client's actual load path)where()threw; and the trim was dead code (wherefillsnull,isNaN(null) === false) so even eager 2D calls returned the untrimmed globe. Rewritten: bbox pre-filter →isel→ haversine mask over bbox cells only → mask-derived trim →wherelast (also the review's measured 594x perf win — the old code haversine'd 1.04M cells for a 77-cell circle).rectangle-cartesianrectangle()built a cartesian product of coords (M·N entries per axis) instead of unique values → a 2×2 selection came back 4×4, a 153-cell bbox as 23,409 cells, O((M·N)²) overall.High — client / auto-concatenate
autoconcat-variant-crashloadDataset({autoConcatenate: true})threw"Multiple variants available"for the exact case the feature exists for: after finding concatenable variants it re-resolved with the always-undefinedrequest.variant. Variants likepart1+part2with validdclimate:concatPrioritycrashed.concat-items-resolved-idgetConcatenableItemsFromStacmatched item ids against the raw collection argument, not the resolved id — unprefixed"era5"(→ecmwf_era5) returned[], silently degradingautoConcatenateto a single variant.timerange-endpoint-clampingtimeRange()passed user endpoints straight to jaxray's exact-match slice → "Jan 1–Jan 5" against data starting Jan 2 threwInvalidSelectionErrorinstead of returning the overlap (the xarray semantics this API mirrors). Now clamps to the axis; empty intersections raiseNoDataFoundError. Existing tests missed it because their fakeselwas more forgiving than real jaxray.Medium — caching and plumbing
client-cache-gatewaygatewayUrl— a catalog cached from gateway A was served for calls targeting gateway B. Review follow-up: the client-level cache was then deleted entirely (it duplicated the module cache and let catalogs live ~2x the TTL via re-stamping).stac-cache-rootcidloadStacCatalog's cache ignored an explicitrootCid— pinning a version returned whatever was cached, and a pinned load poisoned later latest-version calls for the TTL. Cache key is nowstac:<gateway>:<rootCid|latest>.concat-shard-read-modeshardReadMode: "full"silently became"sparse"for every variant on the concatenate path.isempty-empty-datasetisEmpty()only checked zero-size dims and missedDataset({})→ empty bounds selections resolved successfully instead of raisingNoDataFoundErrorlike the point path. Now delegates to the existingisDatasetEmpty.Proven suspicions (flagged as unproven in the review; all four proved red)
root-cid-endpointhttps://ipfs-gateway.dclimate.net/stacwith no way for a client to avoid it —getRootCatalogCidnow takes an endpoint andClientOptions.rootCidpins the catalog version (and disables the latest-only STAC server path, which silently bypassed the pin).stac-server-paginationresolveCidFromStacServersentlimit: 100and never readlinks— items past the first page were invisible, silently falling back to the slow IPFS walk. Now follows STACnextlinks, including the production server's POST+token protocol (live-verified againstapi.stac.dclimate.net).haversine-length-mismatchhaversine([5,6],[7,8,99],[0],[0])fabricated a phantom (5, 99) coordinate pair. NowRangeErrorunless lengths are 1 or the common max.classify-retrieval-errorTypeError: fetch failedmatched no classifier substring → essentially every Node connection failure was recorded as generic"error", hollowing out theconnection_errormetric. Now unwrapserror.cause(depth-capped, throw-safe) and matches the exact undici/browser messages.Performance (measured; methodology in commit bodies and the review)
catalog-walk-parallel— the IPFS catalog walk fetched ~139 documents one-at-a-time in three nested await loops; now fans out per level withPromise.allconcat-variants-perf—concatenateVariantsdid a per-step linear scan with repeated date parsing per variant(The circle() globe-mask elimination — 229.9 ms → 0.39 ms in the review — ships as part of the
circle-real-datasetsbug fix above.)What the adversarial gauntlet caught (fixup rounds)
circle(): six confirmed defects in the first green — 0–360/dateline longitude grids lost the wrap side, over-the-pole circles dropped far-side cells, the flat cos scaling under-covered high latitudes, a 111.32-vs-111.195 km/° mismatch dropped boundary cells, a coord-rounding mismatch crashed on float32-decoded grids, andwherestripped variable attrs. All fixed with a haversine-derived geometry (asin(sin(r/R)/cos φ), modular longitude membership, shared Earth radius + epsilon).timeRange(): the first green compared on host-localDate.parsewhile zarr time coords are timezone-less UTC — selection shifted by the machine's UTC offset (red tests were invisible to this because bare dates parse as UTC); and numeric CF axes were compared against epoch-milliseconds, silently returning the entire dataset for out-of-range requests. Reworked on UTC/CF-aware timelines via jaxray'scfTimeToDate; regressions run green underTZ=America/New_York.stac-server-pagination: passed its red test but the red test modeled a GET-href server; the production server paginates with POST+token next links, so the follow degenerated into an unfiltered default-limit crawl. Reworked to honormethod/body/mergeand tolerate null link entries.root-cid-endpoint: therootCidpin was silently bypassed unless the caller also passedstacServerUrl: null; pinning now disables the server fast path.points(): NaN defeatedsnapToGrid: falsevia jaxray's NaN comparisons,undefinedelements resurrected the silent-full-grid symptom, and a coordinate-key typo returned the entire grid — all now validated up front.concatenateVariants: the binary-search split silently dropped non-ascending variants where the old code appended them; now throws with a clear message (verified parity on ascending axes via 500-case differential fuzz).concatenatedVariants/cidfollowed catalog discovery order rather thanconcatPriorityorder, and variants that never opted in (noconcatPriority) were silently merged — both fixed per the documentedDatasetVariantConfigcontract.catalog.organizationswith zero collections, matching the serial walk.Deliberate test corrections
Two legacy tests encoded the buggy behavior and were corrected rather than preserved: the "proper circle" test used a 10 km radius that could never span 0.1° (~11.12 km) latitude rows — it only passed because the trim was dead code (now 12 km); and a far-future
timeRangeexpectedInvalidSelectionErrorwhere "no data in range" isNoDataFoundError.Known API inconsistency (left for a team decision)
GeoTemporalDataset.rectangle()/circle()are documented raw-Datasetpassthroughs and still return (possibly empty) datasets, whileselect({bounds})andpoints()throwNoDataFoundErroron empty results. Changing the documented contract was out of scope here.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes