fix(cloudflare): honour fit and position in the image endpoint - #2449
fix(cloudflare): honour fit and position in the image endpoint#2449afonsojramos wants to merge 2 commits into
Conversation
The endpoint parsed only w/h/f/q and built its own transform, so the `fit` Astro asks for never reached the Images binding. A request for a square cover-crop got width and height alone, and the binding fell back to its default fit: the image was scaled down inside the box and letterboxed instead of cropped to fill it. Astro's stock Cloudflare endpoint forwards `fit`, so the same markup cropped correctly on Node and did not on Cloudflare. Parse `fit` and `position` alongside the existing params and map them onto the binding's vocabulary, which only partly overlaps Astro's: `fill` is the binding's `squeeze`, `inside` is `contain`, and sharp's compass names are the edges they describe. Values with no binding equivalent -- `outside`, a compound `left top` -- are dropped rather than swapped for a fit that would crop, leaving the previous behaviour rather than a confidently wrong one. Both params are advisory: an unrecognised value is dropped instead of failing the request, because Astro's ImageFit is open-ended and each backend supports a different subset. Also adds @emdash-cms/cloudflare to test:unit. CI runs only that script, so the package's tests -- including this regression test -- never ran. Closes emdash-cms#2228
🦋 Changeset detectedLatest commit: 6c86e35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes a Cloudflare-specific regression where EmDash’s _image endpoint ignored Astro’s fit and position parameters, causing transforms (notably square cover crops) to letterbox instead of crop when using the Cloudflare Images binding.
Changes:
- Extend shared
parseTransformParams()to parsefitandposition(advisory/allowlisted forfit). - Map Astro
fit/positiononto Cloudflare Images bindingfit/gravityand add Cloudflare adapter regression tests. - Ensure
@emdash-cms/cloudflareunit tests run in the roottest:unitscript; add a changeset for the fix.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/tests/unit/media/image-endpoint.test.ts | Adds unit coverage for parsing fit and position. |
| packages/core/src/media/image-endpoint.ts | Parses fit/position alongside existing transform params. |
| packages/cloudflare/tests/image-endpoint-fit.test.ts | Adds regression tests asserting fit/position forwarding/mapping to the Images binding. |
| packages/cloudflare/src/image-endpoint.ts | Maps Astro fit → binding fit and Astro position → binding gravity. |
| package.json | Includes @emdash-cms/cloudflare in the test:unit filter so its unit tests run in CI. |
| .changeset/cf-image-endpoint-fit.md | Publishes patch release notes for the behavior fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // `fit` and `position` come straight from Astro's image service. Both are | ||
| // advisory: an unrecognised value is dropped rather than failing the | ||
| // request, so a rendition still resolves on a backend that doesn't know it. | ||
| const fitRaw = params.get("fit"); | ||
| const fit = fitRaw !== null && isTransformFit(fitRaw) ? fitRaw : undefined; | ||
|
|
||
| const positionRaw = params.get("position"); | ||
| const position = positionRaw !== null && positionRaw !== "" ? positionRaw : undefined; | ||
|
|
||
| return { ok: true, options: { width, height, format, quality, fit, position } }; |
| "emdash": patch | ||
| --- | ||
|
|
||
| Fixes cropping for EmDash media on Cloudflare. Images asked for a `cover` crop — square avatars, fixed-ratio thumbnails — came back scaled down and letterboxed inside the requested box instead of filling it, because the endpoint never passed the requested fit to the Images binding. `fit` and `position` are now honoured, so a crop crops and its focal side is respected. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped bug fix for the right problem. Moving fit/position through the shared parseTransformParams parser and mapping them to the Cloudflare Images binding in the adapter is the idiomatic split for EmDash: core validates/allowlists, the platform adapter translates backend vocabulary. The tests reproduce the reported letterboxing scenario and cover the mapping edge cases (outside, compound positions, compass names). Including @emdash-cms/cloudflare in test:unit is a sensible fix, since the regression test would otherwise have been skipped in CI.
I checked the changed source, the Node endpoint (packages/core/src/astro/image-endpoint.ts) to confirm it doesn't need the same change—it delegates to Astro's astro/assets/endpoint/generic, which already handles fit/position—and searched for other consumers of parseTransformParams. No logic bugs, security issues, or query-count regressions found.
The only real issues are AGENTS.md comment-discipline violations: the new JSDoc blocks for ALLOWED_TRANSFORM_FITS, FIT_TO_BINDING, and GRAVITY_BY_POSITION contain review-facing justification and rejected-alternative narrative ("rather than ...", "better than ...", "Astro passes sharp's vocabulary straight through"). Those should be trimmed to the concise "why" a future reader needs.
| /** | ||
| * The `fit` values Astro's image service emits. Astro's `ImageFit` is | ||
| * open-ended (`string & {}`), so this is an allowlist: an unrecognised fit is | ||
| * dropped rather than forwarded to a backend that would reject it. | ||
| * | ||
| * `inside` and `outside` are not in `ImageFit` but are accepted by Astro's | ||
| * sharp service, so a site can already use them on Node. `none` is absent on | ||
| * purpose: Astro deletes it before building the URL, so it never reaches an | ||
| * endpoint. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] This JSDoc block explains the allowlist, but most of it is review-facing justification: it argues why inside/outside are included, why none is excluded, and why unknown values are dropped. AGENTS.md says comments are for future readers, not PR reviewers—avoid justifying decisions and narrating rejected alternatives.
| /** | |
| * The `fit` values Astro's image service emits. Astro's `ImageFit` is | |
| * open-ended (`string & {}`), so this is an allowlist: an unrecognised fit is | |
| * dropped rather than forwarded to a backend that would reject it. | |
| * | |
| * `inside` and `outside` are not in `ImageFit` but are accepted by Astro's | |
| * sharp service, so a site can already use them on Node. `none` is absent on | |
| * purpose: Astro deletes it before building the URL, so it never reaches an | |
| * endpoint. | |
| */ | |
| /** | |
| * Fit values Astro's image service may emit. Unknown values are dropped so | |
| * backends keep their default behaviour. | |
| */ |
| /** | ||
| * Astro's `fit` vocabulary mapped onto the Images binding's. | ||
| * | ||
| * Most names line up. Astro's `fill` distorts the image to fill the box, which | ||
| * the binding calls `squeeze`. `inside` (accepted by Astro's sharp service) | ||
| * resizes to fit within the box, which is the binding's `contain`. `outside` | ||
| * has no binding equivalent -- nothing there resizes to *exceed* the box | ||
| * without cropping -- so it is dropped and the dimensions apply on their own, | ||
| * rather than substituting a fit that would crop. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] The block contains a clear justification/rejected-alternative narrative ("rather than substituting a fit that would crop"). The Record type and outside: undefined entry already express that unmapped values fall through to the binding default, so the comment only needs to say what the map does.
| /** | |
| * Astro's `fit` vocabulary mapped onto the Images binding's. | |
| * | |
| * Most names line up. Astro's `fill` distorts the image to fill the box, which | |
| * the binding calls `squeeze`. `inside` (accepted by Astro's sharp service) | |
| * resizes to fit within the box, which is the binding's `contain`. `outside` | |
| * has no binding equivalent -- nothing there resizes to *exceed* the box | |
| * without cropping -- so it is dropped and the dimensions apply on their own, | |
| * rather than substituting a fit that would crop. | |
| */ | |
| /** | |
| * Maps Astro `fit` values to the Cloudflare Images binding's fit vocabulary. | |
| * Unmapped values (e.g. `outside`) become `undefined`, leaving the binding's | |
| * default behaviour unchanged. | |
| */ |
| /** | ||
| * Astro's `position` mapped onto the binding's `gravity`. | ||
| * | ||
| * Astro passes sharp's vocabulary straight through, and the two only partly | ||
| * overlap. The shared keywords pass unchanged; sharp's compass names mean the | ||
| * same edges under different words; `centre` is the same as `center`; and | ||
| * `attention` (sharp's saliency-based crop) is the binding's `auto`. | ||
| * | ||
| * A compound position such as `left top` names a corner, which the binding can | ||
| * only express as coordinates. Rather than guess at them, it is dropped and the | ||
| * binding keeps its own gravity -- the same outcome as before this mapping | ||
| * existed, and better than cropping to a confidently wrong edge. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] This comment is mostly a vocabulary walk-through and a justification ("better than cropping to a confidently wrong edge"). The map below is self-describing; the reader only needs a short pointer to what it maps and what happens to unknown positions.
| /** | |
| * Astro's `position` mapped onto the binding's `gravity`. | |
| * | |
| * Astro passes sharp's vocabulary straight through, and the two only partly | |
| * overlap. The shared keywords pass unchanged; sharp's compass names mean the | |
| * same edges under different words; `centre` is the same as `center`; and | |
| * `attention` (sharp's saliency-based crop) is the binding's `auto`. | |
| * | |
| * A compound position such as `left top` names a corner, which the binding can | |
| * only express as coordinates. Rather than guess at them, it is dropped and the | |
| * binding keeps its own gravity -- the same outcome as before this mapping | |
| * existed, and better than cropping to a confidently wrong edge. | |
| */ | |
| /** | |
| * Maps Astro `position` values to the Cloudflare Images binding's gravity | |
| * vocabulary. Compound or unknown positions are dropped. | |
| */ |
Per review: the new JSDoc argued for the mapping rather than describing it, which belongs in the PR rather than the source. Also normalises a whitespace-only `position` at parse time and fixes the changeset wording.
github-actions
Bot
added
review/needs-rereview
|
Thanks both — pushed in 6c86e35. Comment discipline (@emdashbot, 3 threads): applied all three suggestions. You're right that those blocks argued for the mapping instead of describing it; the rationale belongs in this PR, which is where it now lives. I trimmed the same narrative style out of the Changeset grammar (@copilot): reworded to "An image asked to fill a fixed box — a square avatar, a fixed-ratio thumbnail — came back scaled down and letterboxed inside it". Whitespace-only Optional keys in the returned One unrelated thing worth flagging: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
There was a problem hiding this comment.
This is the right fix in the right place: adding fit and position to the shared parseTransformParams parser and mapping them to Cloudflare Images vocabulary in the adapter keeps platform-specific translation where it belongs. The regression tests cover the reported cover-crop letterboxing bug and the dropped-value edge cases, and adding @emdash-cms/cloudflare to test:unit is necessary for the new test to run in CI.
Most of the prior review's comment-discipline findings have been trimmed out of the constant JSDoc blocks. One slipped through in the parseTransformParams docstring, which still uses "Unlike the others" / "dropped rather than failing" — rejected-alternative/justification prose that AGENTS.md asks us to delete. I also spotted a mock-hoisting style gap in the new test: it references top-level consts from vi.mock factories without vi.hoisted, unlike the sibling cache/kv-timeout.test.ts which explicitly does this for the same reason.
No logic bugs, security issues, query-count regressions, or API-contract breaks found.
| * `fit` and `position` describe how the rendition fills its box. Unlike the | ||
| * others they are advisory: an unrecognised value is dropped rather than | ||
| * failing the request. The platform endpoint maps them onto its backend's |
There was a problem hiding this comment.
[needs fixing] The parseTransformParams JSDoc contains AGENTS.md-prohibited reviewer-facing justification: "Unlike the others ... dropped rather than failing the request" frames a rejected alternative rather than stating the concise "why". This is the same pattern the prior review flagged in the constant JSDoc blocks; trim it here too.
| * `fit` and `position` describe how the rendition fills its box. Unlike the | |
| * others they are advisory: an unrecognised value is dropped rather than | |
| * failing the request. The platform endpoint maps them onto its backend's | |
| * `fit` and `position` describe how the rendition fills its box. Unrecognised | |
| * values are ignored because each backend supports a different subset. The | |
| * platform endpoint maps them onto its backend's |
| const adapterGET = vi.fn(() => new Response("adapter", { status: 200 })); | ||
| vi.mock("@astrojs/cloudflare/image-transform-endpoint", () => ({ GET: adapterGET })); | ||
|
|
||
| /** Records the transform the endpoint hands to the Images binding. */ | ||
| const transform = vi.fn(); | ||
| const output = vi.fn(() => ({ | ||
| response: () => new Response("bytes", { headers: { "Content-Type": "image/webp" } }), | ||
| })); | ||
|
|
||
| const images = { | ||
| input: () => ({ | ||
| transform: (options: unknown) => { | ||
| transform(options); | ||
| return { output }; | ||
| }, | ||
| }), | ||
| }; | ||
|
|
||
| vi.mock("cloudflare:workers", () => ({ env: { IMAGES: images } })); |
There was a problem hiding this comment.
[suggestion] These vi.mock factories reference top-level consts (adapterGET, images, transform, output) that are not hoisted with the mock. The sibling packages/cloudflare/tests/cache/kv-timeout.test.ts already uses vi.hoisted for the same cloudflare:workers mock pattern. Follow that convention so a future import reordering can't hit a TDZ/access-before-initialization issue.
const { adapterGET, images, transform, output } = vi.hoisted(() => ({
adapterGET: vi.fn(() => new Response("adapter", { status: 200 })),
transform: vi.fn(),
output: vi.fn(() => ({ response: () => new Response("bytes", { headers: { "Content-Type": "image/webp" } }) })),
images: { /* ... */ },
}));
github-actions
Bot
added
review/awaiting-author
What does this PR do?
The Cloudflare image endpoint parsed only
w/h/f/qand built its own transform, so thefitAstro asks for never reached the Images binding. A request for a square cover-crop arrived as width and height alone, and the binding fell back to its default fit: the image was scaled down inside the box and letterboxed instead of cropped to fill it. Astro's stock Cloudflare endpoint forwardsfit, so identical markup cropped correctly on Node and silently did not on Cloudflare.fitandpositionare now parsed alongside the existing params and mapped onto the binding's vocabulary, which only partly overlaps Astro's:cover,contain,scale-downfillsqueezeinsidecontainImageFit, but Astro's sharp service accepts itoutsideposition: north/south/east/westtop/bottom/right/leftposition: centre/attentioncenter/autoposition: left topValues with no binding equivalent are dropped rather than swapped for something that would crop, so they keep the behaviour they had before this mapping existed instead of gaining a confidently wrong one. Both params are advisory: an unrecognised value is dropped instead of failing the request, because
ImageFitis open-ended (string & {}) and each backend supports a different subset.Closes #2228
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
The regression test reproduces the bug. Reverting only the adapter mapping and re-running:
With the fix,
pnpm test:unitis green, including 323 tests in@emdash-cms/cloudflare.Two things worth a maintainer's eye
1.
@emdash-cms/cloudflarewas not intest:unit. CI runs only that script, so none of the package's 25 test files have been running — a regression test added here would never have executed. I added the package to the filter; all 323 tests pass. Happy to split that into its own PR if you would rather keep this diff to the endpoint.2. An adjacent divergence I deliberately did not change. Astro only defaults
fittocoverwhen alayoutis set (validateOptions:if (options.layout && options.width && options.height) options.fit ??= "cover"). EmDash's own components passlayout="constrained", so the path this issue describes does emitfitand is fixed here. But a bare<Image src={emdashMedia} width={400} height={400} />withimage.layoutunset emits nofitat all, and then sharp still crops (its own default iscover) while the binding scale-downs. Defaultingfittocoverhere when both dimensions are present would align the two, at the cost of cropping requests that never asked to be cropped. That felt like a behaviour change rather than this bug, so I left it — happy to add it if you'd prefer the platforms match.