feat(sanity): support absolute URLs with project and dataset extraction#2274
feat(sanity): support absolute URLs with project and dataset extraction#2274hacknug wants to merge 4 commits into
Conversation
Absolute URLs from Sanity's CDN can now be passed directly as the image source; `projectId` and `dataset` are extracted from the URL and may still be overridden via options.
commit: |
There was a problem hiding this comment.
Pull request overview
Adds support in the Sanity provider for using absolute Sanity CDN URLs as src, extracting projectId/dataset from the URL path while preserving existing behavior for non-URL inputs.
Changes:
- Sanity provider now detects absolute URLs and extracts
projectId,dataset, and filename from the URL path. - Adds provider tests for absolute Sanity CDN URL handling and option override/fallback behavior (including falsy options).
- Updates bundle-size snapshot and bumps
@nuxt/test-utilspatch version in.nuxtrc.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/runtime/providers/sanity.ts |
Adds absolute-URL parsing and fallback chain for project/dataset when building Sanity image URLs. |
test/nuxt/providers.test.ts |
Adds test coverage for absolute Sanity CDN URLs and override/fallback cases. |
test/unit/bundle.test.ts |
Updates bundle-size inline snapshot due to runtime code changes. |
.nuxtrc |
Bumps @nuxt/test-utils setup version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughThis PR updates the Sanity image provider to detect absolute CDN URLs, extract project and dataset segments, and rebuild the final image URL with merged query parameters. It adds tests for absolute URL override behavior and existing query strings. Separately, it bumps the Nuxt test-utils version and updates one bundle-size snapshot. Estimated code review effort: 3 (Moderate) | ~20 minutes Changes
Sequence Diagram(s)Not applicable. Related issues: Fixes broken image path generation when passing an absolute Sanity CDN URL to Suggested labels: bug, provider:sanity Suggested reviewers: danielroe, farnabaz 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/runtime/providers/sanity.ts (1)
107-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDead computation and potential double
?for protocol inputs.
parts/format(lines 107-108) are computed unconditionally by splitting the entire absolute URL on-, but are unused wheneverhasProtocol(src)is true — wasted work with no functional impact.Separately,
_srcfor the protocol branch is derived viasrc.split('/').slice(-3), which does not strip any existing query string from the last path segment. If an absolutesrcalready carries a query (e.g....file.png?dl),filenameAndQuerieswould produce a malformed double-?URL (file.png?dl?h=10...). Not exercised by the new tests, but worth guarding against if absolute URLs with pre-existing queries are a realistic input.🤖 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/runtime/providers/sanity.ts` around lines 107 - 114, In the URL builder inside sanity.ts, avoid splitting the full src into parts/format unless it is actually needed for the non-protocol branch, and make sure the protocol branch strips any existing query string from _src before appending operations. Update the filenameAndQueries construction in the hasProtocol(src) path so joinURL never receives a value that already contains a query, preventing double-? when src includes an existing query string.test/nuxt/providers.test.ts (1)
492-511: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertions match the reviewed provider logic; consider adding crop/hotspot coverage.
All four assertions (baseline extraction,
projectIdoverride,datasetoverride, empty-string inherit) correctly match the fallback chain insanity.ts. However, none of the cases exercisemodifiers.crop/hotspot, which is where thegetMetadataregex mismatch (flagged insrc/runtime/providers/sanity.tslines 80-82) actually breaks for absolute URLs. Adding a case with a percentagecropagainst an absolute URL would catch that regression.🤖 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 `@test/nuxt/providers.test.ts` around lines 492 - 511, Add a regression case in the sanity provider tests that exercises `modifiers.crop` (and optionally `hotspot`) with an absolute URL, since the current `sanity().getImage` assertions only cover height/blur/quality and miss the `getMetadata` regex path in `src/runtime/providers/sanity.ts`. Extend the existing `sanity` test block to include an absolute-image URL with percentage-based crop data so the provider behavior is verified when metadata parsing is involved, especially around the `getMetadata` and `getImage` flow.
🤖 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/runtime/providers/sanity.ts`:
- Around line 80-82: `getMetadata` in `src/runtime/providers/sanity.ts` only
recognizes legacy hyphen-delimited filenames, so absolute CDN URLs like
`...-1552x982.png` are parsed as invalid and skip crop/hotspot handling. Update
`getMetadata` to support both format suffix styles (hyphen and dot) and ensure
`getImage` still receives valid `sourceWidth`/`sourceHeight` for absolute URLs
so `modifiers.crop` is converted before `operationsGenerator` runs. Add a
regression test around `getImage`/`getMetadata` using an absolute Sanity URL
with crop or hotspot modifiers.
In `@test/unit/bundle.test.ts`:
- Line 24: The inline snapshot in the bundle size assertion is out of sync with
the current measured output. Update the expectation in the bundle size test by
regenerating the snapshot or changing the value in the roundToKilobytes
assertion to match the current bundle delta reported by CI, using the existing
withImage and withoutImage test setup as the reference point.
---
Nitpick comments:
In `@src/runtime/providers/sanity.ts`:
- Around line 107-114: In the URL builder inside sanity.ts, avoid splitting the
full src into parts/format unless it is actually needed for the non-protocol
branch, and make sure the protocol branch strips any existing query string from
_src before appending operations. Update the filenameAndQueries construction in
the hasProtocol(src) path so joinURL never receives a value that already
contains a query, preventing double-? when src includes an existing query
string.
In `@test/nuxt/providers.test.ts`:
- Around line 492-511: Add a regression case in the sanity provider tests that
exercises `modifiers.crop` (and optionally `hotspot`) with an absolute URL,
since the current `sanity().getImage` assertions only cover height/blur/quality
and miss the `getMetadata` regex path in `src/runtime/providers/sanity.ts`.
Extend the existing `sanity` test block to include an absolute-image URL with
percentage-based crop data so the provider behavior is verified when metadata
parsing is involved, especially around the `getMetadata` and `getImage` flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d57287a-d8d7-46cc-a39f-0b75a37ed756
📒 Files selected for processing (4)
.nuxtrcsrc/runtime/providers/sanity.tstest/nuxt/providers.test.tstest/unit/bundle.test.ts
| getImage: (src, { modifiers, projectId, dataset, baseURL = sanityCDN }) => { | ||
| const [_projectId, _dataset, _src] = hasProtocol(src) ? src.split('/').slice(-3) : [projectId, dataset, src] | ||
| const { height: sourceHeight, width: sourceWidth } = getMetadata(_src) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
getMetadata regex never matches absolute-URL filenames, silently breaking crop/hotspot for the new feature.
getMetadata (line 45) matches /-(?<width>\d*)x(?<height>\d*)-(?<format>.*)$/, which requires a second hyphen before the format token (matches the legacy asset-ID format like image-<hash>-300x450-jpg). Real Sanity CDN filenames extracted from an absolute URL use a dot before the extension instead (e.g. 228cc6aa...-1552x982.png — the PR's own example). That string has exactly one hyphen, so the regex never matches, getMetadata(_src) always returns { width: undefined, height: undefined, format: undefined }, and a spurious "invalid image asset ID" dev warning fires on every absolute-URL request.
Consequences:
- The crop-conversion block (lines 83-90) is guarded by
sourceWidth && sourceHeight, so it's silently skipped —modifiers.cropsurvives as an object and gets passed tooperationsGenerator, which will stringify it (String(value)) into a brokencrop=[object Object]query param. - Percentage-based crop/hotspot transforms are effectively non-functional for the primary new use case (absolute CDN URLs), with no error surfaced to the caller besides a misleading dev console warning.
🐛 Proposed fix: accept both hyphen- and dot-delimited format suffixes
const getMetadata = (id: string) => {
- const result = id.match(/-(?<width>\d*)x(?<height>\d*)-(?<format>.*)$/)
+ const result = id.match(/-(?<width>\d*)x(?<height>\d*)[-.](?<format>.*)$/)Consider also adding a test with modifiers.crop/hotspot against an absolute URL to catch this regression.
📝 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.
| getImage: (src, { modifiers, projectId, dataset, baseURL = sanityCDN }) => { | |
| const [_projectId, _dataset, _src] = hasProtocol(src) ? src.split('/').slice(-3) : [projectId, dataset, src] | |
| const { height: sourceHeight, width: sourceWidth } = getMetadata(_src) | |
| const getMetadata = (id: string) => { | |
| const result = id.match(/-(?<width>\d*)x(?<height>\d*)[-.](?<format>.*)$/) |
🤖 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/runtime/providers/sanity.ts` around lines 80 - 82, `getMetadata` in
`src/runtime/providers/sanity.ts` only recognizes legacy hyphen-delimited
filenames, so absolute CDN URLs like `...-1552x982.png` are parsed as invalid
and skip crop/hotspot handling. Update `getMetadata` to support both format
suffix styles (hyphen and dot) and ensure `getImage` still receives valid
`sourceWidth`/`sourceHeight` for absolute URLs so `modifiers.crop` is converted
before `operationsGenerator` runs. Add a regression test around
`getImage`/`getMetadata` using an absolute Sanity URL with crop or hotspot
modifiers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/nuxt/providers.test.ts (1)
516-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel absolute Sanity URLs in the provider type instead of suppressing the calls. These cases need
@ts-expect-errorbecauseSanityOptions.projectIdis still required. If absolute URLs are meant to inferprojectId/dataset, encode that in the provider options type so callers don’t need a type suppression at each site.🤖 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 `@test/nuxt/providers.test.ts` around lines 516 - 533, Update the Sanity provider typing so absolute image URLs can be passed to getImage without suppressing type errors in the tests. The issue is that SanityOptions still requires projectId even when the URL already contains the project/dataset, so callers must use `@ts-expect-error`. Adjust the provider option types and any related getImage/SanityOptions definitions to model absolute URLs as a valid case that infers or bypasses projectId/dataset requirements, then remove the suppressions from the sanity().getImage test cases.src/runtime/providers/sanity.ts (1)
64-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making
projectIdoptional now that URL-derived values are supported.The interface still requires
projectId: string, but the new fallback logic (projectId || _projectId) is explicitly designed to let a falsy/emptyprojectIddefer to the value extracted from an absolute URL. Marking itprojectId?: stringwould better reflect the new supported usage and avoid forcing consumers to pass an empty-string placeholder.♻️ Suggested change
interface SanityOptions { - projectId: string + projectId?: string dataset?: string🤖 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/runtime/providers/sanity.ts` around lines 64 - 67, The Sanity options type still requires projectId even though the fallback logic in the Sanity provider is designed to use a URL-derived value when projectId is falsy. Update the SanityOptions interface to make projectId optional, and make sure the provider code that reads projectId || _projectId in the Sanity runtime still handles both direct and URL-derived cases cleanly without forcing callers to pass an empty placeholder.
🤖 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/runtime/providers/sanity.ts`:
- Around line 82-84: The destructured `_id` in `src/runtime/providers/sanity.ts`
can be `string | undefined`, but `getMetadata` requires a definite string.
Update the `isAbsolute` parsing around the `_segment`/`_id` assignment and the
`getMetadata(_id)` call so the code narrows or validates the value before use,
handling malformed absolute URLs safely instead of passing a possibly undefined
value.
---
Nitpick comments:
In `@src/runtime/providers/sanity.ts`:
- Around line 64-67: The Sanity options type still requires projectId even
though the fallback logic in the Sanity provider is designed to use a
URL-derived value when projectId is falsy. Update the SanityOptions interface to
make projectId optional, and make sure the provider code that reads projectId ||
_projectId in the Sanity runtime still handles both direct and URL-derived cases
cleanly without forcing callers to pass an empty placeholder.
In `@test/nuxt/providers.test.ts`:
- Around line 516-533: Update the Sanity provider typing so absolute image URLs
can be passed to getImage without suppressing type errors in the tests. The
issue is that SanityOptions still requires projectId even when the URL already
contains the project/dataset, so callers must use `@ts-expect-error`. Adjust the
provider option types and any related getImage/SanityOptions definitions to
model absolute URLs as a valid case that infers or bypasses projectId/dataset
requirements, then remove the suppressions from the sanity().getImage test
cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33e65ce0-e300-4b0c-8f1c-06376b6de86e
📒 Files selected for processing (2)
src/runtime/providers/sanity.tstest/nuxt/providers.test.ts
|
@danielroe ready for review. I'm having issues running the e2e tests on my machine (can't run them at all) which is why I probably didn't catch the issue with Seems unrelated to these changes since I only touched the |
🔗 Linked issue
Closes #1297
📚 Description
Summary
Absolute URLs from Sanity's CDN can now be passed directly as the image source. The provider will now extract
projectIdanddatasetfrom the URL but they can still be overridden via options if necessary (not sure why but accounted for).Changes
projectId,datasetandfilenamefrom the URL path whensrcis an absolute URLprojectId: '') correctly, falling back to the URL value with test to prevent future regresisonsBackwards-compatible change. Existing configurations with projectId and asset IDs will work exactly as before.
Notes
@ts-expect-errorcomments on the tests in order to not change the type signature ofSanityOptions.nuxtrcbut I can revert or delete it from the repo (precedent 8341b16)I'm really sorry for the extra bytes 🙏 .Hope someone from e18n can shave at least twice as many from any of our deps 🤞Seems like there was a missmatch between my machine and CI so it wasn't necessary to make any changes to the snapshot and I technically didn't make things worse 😈