Do not flag embedded deck.gl rows as unloadable in the share-readiness check - #1823
Conversation
Two follow-ups to the share-readiness check in #1812, from review comments that landed after that PR merged. A non-GeoJSON deck.gl visualization (arc, heatmap, hexagon built from a CSV) keeps its rows in `source.data` as an array. `isPlainObject` excludes arrays, so `carriesOwnData` missed those layers, they fell through to the reference walk, and a layer whose data travels inside the project file was reported as "no source" or, when `sourcePath` still held the original CSV name, as a local file. An array `data` now counts as embedded, like an inline FeatureCollection. Only a string `data` is a URL. Separately, a rejected ranged GET no longer condemns the host. `Range` is CORS-safelisted only for a simple byte range, and an older webview may preflight it and get no matching `Access-Control-Allow-Headers` back. The HEAD that preceded the retry already proved the host answers and lets this origin read the response, so a rejection there is about the ranged request rather than the host: the probe falls back to the HEAD's verdict instead of reporting "a browser cannot fetch this host" for a host whose plain GET a renderer would fetch fine. Refs #1671.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe share-readiness logic excludes inline array data from external-source checks. It also preserves successful HEAD probe results when ranged GET requests fail, while keeping abort and timeout failures unchecked. Tests cover both behaviors. ChangesShare readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
🔍 Cloudflare PR preview
|
|
Good, all necessary imports already exist. The implementation and tests are consistent and well-reasoned. I've reviewed both hunks of the diff thoroughly against the surrounding source and did not find bugs — the logic changes match their stated rationale and the new tests correctly exercise the new branches. I won't post inline comments since no issues rose to a level worth flagging. Code reviewBugs: None found. Both changes were checked against their surrounding code:
Security: None. Credentials are still withheld ( Performance: None. No new requests are added — the fallback only changes how an existing ranged-GET failure is interpreted. Quality: Minor, low confidence: the fallback treats any non-abort/non-timeout failure ( CLAUDE.md: No violations — no touched files intersect the mirrored-constant or catalog-regeneration rules, and the frontend test suite (per the PR description) was run and passes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/share-readiness.test.ts`:
- Around line 361-364: Update the fallback test fetch mocks and assertions
around the ranged-read cases to record each request method, then assert the
sequence is HEAD followed by GET. For bounded-retry coverage, also assert that
the GET includes the Range header value bytes=0-0, ensuring the tests fail when
the implementation stops after HEAD.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad9f3661-2339-4c1b-8aab-db9f946cf26d
📒 Files selected for processing (2)
apps/geolibre-desktop/src/lib/share-readiness.tstests/share-readiness.test.ts
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
Both fallback tests rejected every non-HEAD request but never checked that a GET was attempted, so they would have kept passing if the retry were dropped entirely: a bare HEAD 405 already reads as reachable and a bare 403 as credentialed. They now record each attempt through a shared helper and assert HEAD followed by GET with `Range: bytes=0-0`. Verified by deleting the retry locally, which takes the suite from 26 passing to 3 failing instead of 1.
|
I have no substantive findings that warrant an inline comment — the change is correct, minimal, and well-tested. No comments to post. Code reviewBugs: None found. The Bugs: The ranged-GET fallback correctly maps only to the HEAD statuses that can trigger the retry (400/403/405/501 via Security: None found. Fetch calls still use Performance: None found. No new requests are introduced; the fallback only changes how an already-issued ranged GET's rejection is interpreted. Quality: Very minor, unrelated to correctness — the pre-existing comment at CLAUDE.md: No violations — the change doesn't touch any of the mirrored-constant or generated-catalog areas called out in CLAUDE.md, and the new tests follow the existing Both new tests are well-constructed: they assert not just the final status but the actual request sequence ( |
Two follow-ups to the share-readiness check from #1812. Both came from review comments that landed after that PR was merged, so they could not be folded into it.
Refs #1671.
Embedded deck.gl rows were reported as unloadable
A non-GeoJSON deck.gl visualization (arc, heatmap, hexagon built from a CSV) keeps its rows in
source.dataas an array (createDeckVizStoreLayerinpackages/plugins/src/plugins/deckgl-viz/store-layer.ts, read back bydeckVizRows).carriesOwnDatatested that field withisPlainObject, which explicitly excludes arrays, andlayer.geojsonis unset for these layers, so neither embedded-data check caught them.The layer then fell through to the reference walk, which finds no string reference (
source.datais skipped bynonEmptyString), and it was reported asno-sourceor, whensourcePathstill held the original CSV name, aslocal-file. A layer whose data travels entirely inside the project file was told to the author as something the recipient cannot load.An array
datanow counts as embedded, alongside an inlineFeatureCollection. Only a stringdatais a URL, and that is a reference like any other.A rejected ranged GET no longer condemns the host
The probe retries with
Range: bytes=0-0when a HEAD comes back 400/403/405/501, so a HEAD-refusing object store is not misread as credential-gated (and so the check never downloads a whole COG). ButRangeis CORS-safelisted only for a simple byte range, and an older webview may preflight it and get no matchingAccess-Control-Allow-Headersback, even where the plain GET a renderer issues would succeed. That rejection was being reported asblocked/cors: "a browser cannot fetch this host", about a host that works.The HEAD that preceded the retry already proved the host answers and lets this origin read the response, so a rejection on the retry is about the ranged request rather than the host. The probe now falls back to the HEAD's own verdict: a 405 stays reachable, a 403 stays credentialed. Aborts and timeouts are still reported as such.
Testing
Three new cases in
tests/share-readiness.test.ts: a row-based deck.gl layer with asourcePathproducing no findings, and the two ranged-GET fallbacks (HEAD 405 → reachable, HEAD 403 → credentialed). Full frontend suite green (5625 passing), build and pre-commit clean.Summary by CodeRabbit
Bug Fixes
Tests