fix(lib): fix error message and name the codec when a dataset cannot be decoded - #210
fix(lib): fix error message and name the codec when a dataset cannot be decoded#210eeholmes wants to merge 2 commits into
Conversation
A dataset whose chunks use a codec gridlook has no decoder for fails in a way that is indistinguishable from a slow or unreachable host: the store opens, every variable is listed, and only the first chunk read fails — under a "Could not fetch data" heading that sends the reader to the network, which is not where the problem is. zarrita names the codec precisely at that point, but nothing carried the name through to the user. src/lib/data/codecErrors.ts recognises three cases and rewrites each into a message naming the culprit: - an unregistered codec -> "Unsupported codec: numcodecs.quantize" - a codec that rejected a chunk -> the codec, plus the reason it threw - a data type the browser lacks -> the type, plus the browsers that have it Behaviour is unchanged and no decoder is added: an unsupported dataset still fails, it just says why — and says it precisely enough to report. Errors are matched both structurally and by message text, because the grid data worker flattens them to a plain string before posting back, which loses the error class and its fields. That flattening also dropped the reason a codec threw, which zarrita puts on `cause`, so gridData.worker.ts now appends it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #10 / PR #11 is merged into main, so the handoff now carries the BASE_URL rule it leaves behind: files in public/ are referenced from script through import.meta.env.BASE_URL, never a leading slash. The single-branch note about fix/codec-error-messages becomes a table of two, now that fix/colormap-swatch-base-url exists alongside it. Both are open on d70-t/gridlook (d70-t#210 and d70-t#211), both must stay byte-identical to their copies on main, and neither is merged here as a branch — but the swatch fix reached main on its own PR, so merging d70-t#211 upstream is a no-op on those lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FniDZANGZVtrQgN1eacurN
Karinon
left a comment
There was a problem hiding this comment.
Thanks for the PR and I think the idea is neat. I tried it by commenting out our custom logBins-codec and loading one Zarr File from Chalmers Ice Climatology ( https://chalmerscloudiceclimatology.s3.amazonaws.com/record/cpcir/2000/ccic_cpcir_200005100200.zarr ).
The formatting is a bit hard to read, but this is an issue with our Toast component and should be tackled separately.
Other than that I have only a few nitpicks.
| const isFloat16 = dataType.startsWith("float16"); | ||
| return { | ||
| heading: `Unsupported data type: ${dataType}`, | ||
| detail: isFloat16 |
There was a problem hiding this comment.
I would be really interested in seeing such a file. I haven't seen datatype issues like this in a long time, but I haven't checked older browsers.
There was a problem hiding this comment.
See comment. Below. Happy to drop if you prefer. It is just an issue for older browsers.
…lper
Two naming points from review:
- `explainDataError` claimed more than it does, so it is now
`explainCodecError` (and `TDataErrorExplanation` follows it to
`TCodecErrorExplanation`).
- `flattenErrorMessage` is not codec-specific at all — it moves to
`src/utils/errorHandling.ts` next to `getErrorMessage` and
`toNormalizedError`, with its unit tests alongside it in
`tests/unit/utils/errorHandling.test.ts`.
No behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0162gtw32ckX3EFQFPqs74dN
|
Thanks for the review — and for going to the trouble of reproducing it by commenting out Both naming points are done in
On the float16 branch — that it isn't a live problem. zarrita maps So I'm happy to drop it. That would also settle your first point: with the dtype case gone the file explains nothing but codec errors, and |
refactor(lib): mirror the PR d70-t#210 review changes onto main
Two more files joined the byte-identical set shared between `main` and `fix/codec-error-messages`, because upstream review asked for `flattenErrorMessage` to live in `src/utils/errorHandling.ts` rather than in the codec-specific file. Note that, and the one point still open on `d70-t#210`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0162gtw32ckX3EFQFPqs74dN
A dataset whose chunks use a codec gridlook has no decoder for fails in a way that is indistinguishable from a slow or unreachable host: the store opens, every variable is listed, and only the first chunk read fails — and gives the user a "Could not fetch data" error but that is not the problem and it interferes with debugging. zarrita names the codec precisely, but nothing carried the name through to the user.
This PR makes the error surface into the message that appears for the user. I tried to make minimal changes to the upstream code. Just enough to surface the error.
Note, I don't have a url to demo the error message. I will post a comment once I do.
With this change to
src/lib/data/codecErrors.ts, it now recognises three cases and rewrites each into a message naming the culprit:Behaviour is unchanged and no decoder is added: an unsupported dataset still fails, it just says why — and says it precisely enough to report.
Errors are matched both structurally and by message text, because the grid data worker flattens them to a plain string before posting back, which loses the error class and its fields. That flattening also dropped the reason a codec threw, which zarrita puts on
cause, so gridData.worker.ts now appends it.