feat(handlers): add opacity and the missing sharp operation modifiers - #322
Conversation
…difiers Closes the gaps between the modifiers and sharp's operation API, and adds the two operations sharp does not provide directly. New modifiers: - `opacity` (#240), composited since sharp has no opacity operation. The image is made transparent, or blended into the `background` colour when one is set so that formats without an alpha channel work too. - `round` (#187), rounds the corners with a given radius, or as round as the image can take without one (a circle when square, a pill otherwise). The mask has to match the output dimensions, which are only known once the pipeline has run, so it is applied last. Cut off corners are made transparent, or filled with the `background` colour when one is set. - `brightness` (#240), `saturation`, `hue` and `lightness`, each mapping to the matching `modulate` option. sharp merges the calls, so combining them stays a single operation. - `autoOrient`, `dilate`, `erode`, `clahe` and `linear`. - `autoorient`, `normalise` and `greyscale` aliases. Existing modifiers now take the arguments they were missing: `sharpen` (`x1`, `y2`, `y3`), `blur` (`precision`, `minAmplitude`), `negate` (`alpha`), `normalize` (`lower`, `upper`) and `threshold` (`greyscale`). Ranges keep mirroring the ones sharp enforces, so invalid arguments are still rejected with a `400` rather than surfacing as a `500`. `dilate` and `erode` are capped below sharp's maximum, in line with `median`, since the mask grows with the width. `affine`, `convolve`, `recomb` and `boolean` are deliberately left out: the first three take a matrix, which fits a URL modifier poorly and is unbounded in cost, and `boolean` takes a second image by path or buffer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModifier support now includes additional Sharp operations, boolean argument parsing, opacity compositing, eager background validation, normalized modifier errors, expanded public types, updated documentation, and broader unit and integration coverage. ChangesImage modifier expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant IPX
participant Handler
participant opacityOverlay
participant Sharp
Request->>IPX: Submit image modifiers
IPX->>Handler: Apply modifier arguments
Handler->>opacityOverlay: Build opacity overlay
opacityOverlay->>Sharp: Composite overlay
Sharp->>IPX: Return image buffer or modifier error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
src/handlers/handlers.ts (1)
434-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
normalizepasses explicitundefinedkeys while the neighbouring handlers deliberately omit them.
blur,negateandthresholdall strip undefined args because "sharp validates withkey in options".normalizedoes the opposite. It currently works (tests pass), but the inconsistency invites a future regression if sharp tightens validation here.♻️ Optional consistency tweak
apply: (_context, pipe, lower, upper) => { - return pipe.normalize({ lower, upper }); + return pipe.normalize({ + ...(lower === undefined ? {} : { lower }), + ...(upper === undefined ? {} : { upper }), + }); },🤖 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/handlers/handlers.ts` around lines 434 - 444, Update the normalize handler’s apply function to omit lower or upper when their arguments are undefined, matching the established behavior in blur, negate, and threshold while preserving provided values.
🤖 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/ipx.ts`:
- Around line 69-99: Update the flag modifier types in the relevant options
definition, including autoOrient, autoorient, flip, flop, greyscale, animated,
a, enlarge, flatten, and unflatten, to accept the empty string alongside true
and "true". Define and reuse a shared FlagModifier alias for consistency across
the entire flag family.
---
Nitpick comments:
In `@src/handlers/handlers.ts`:
- Around line 434-444: Update the normalize handler’s apply function to omit
lower or upper when their arguments are undefined, matching the established
behavior in blur, negate, and threshold while preserving provided 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 Plus
Run ID: 1feae65e-302e-4c77-a4e7-159a611b57e6
📒 Files selected for processing (7)
README.mdsrc/handlers/handlers.tssrc/handlers/utils.tssrc/ipx.tssrc/types.tstest/handlers/handlers.test.tstest/index.test.ts
Follow-up to the review of the modifiers added in the previous commit.
- `round` had to be bounded. It materializes the whole output as raw
pixels, unlike the rest of the pipeline, which libvips streams, and
`maxOutputDimension` only bounds a single page: an animated image
stacks every frame into one tall image, so `/a,enlarge,s_4096x4096,
round_20/x.gif` allocated 1.5GB and then failed with a `500` because
the mask overlay busted the sharp input limit. The output size is now
bounded from the source dimensions and page count before anything is
allocated (`400 IPX_OUTPUT_TOO_LARGE`, 1.6GB peak down to nothing),
the overlay opts out of the input limit, and a second check on the
measured size covers the rest.
- `round` dropped the frame delays and loop count of an animated image,
since they do not survive the raw round-trip. They are carried over to
the output when it is a gif or webp.
- `clahe` accepted a window larger than the image, which libvips rejects
once it runs ("window too large") -- an unhandled `500`. It is now
clamped to the source, and any other late libvips failure is turned
into a `400`, which also covers pre-existing ones such as an `extract`
outside the image.
- `clahe`, `dilate` and `erode` are capped at `100` rather than sharp's
`65536`: the cost grows with the window, and `dilate_1000` alone took
minutes.
- An unknown `background` colour silently rendered as black in the
`round` and `opacity` overlays instead of being rejected. Colours are
now parsed up front, so they fail the same way everywhere.
- `linear` and `brightness` / `saturation` no longer reject values sharp
accepts.
- `IPXModifiers` flag modifiers admit the `""` they are parsed from, so
`{ animated: "" }` type checks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review pass on 27b08ea — every finding was reproducible and is fixed.
Cost caps. Unknown colours rendered as black. Ranges. Types. Not changed: the tiled per-page overlay looked like a cheaper alternative for animated masks, but 378 passing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ipx.ts (1)
88-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
FlagModifier | stringcollapses totrue | string.The bare
stringswallows the""/"true"literals, losing the editor hints that(string & {})preserves elsewhere in this interface (seekernel,fit).♻️ Suggested tweak
- negate: FlagModifier | string; - normalize: FlagModifier | string; + negate: FlagModifier | (string & {}); + normalize: FlagModifier | (string & {}); // alias for normalize - normalise: FlagModifier | string; + normalise: FlagModifier | (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/ipx.ts` around lines 88 - 91, Update the FlagModifier fields negate, normalize, and normalise to use the same string-literal-preserving type pattern as kernel and fit, such as a string intersection that retains editor hints, instead of a bare string union that collapses to true | string.test/index.test.ts (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
round: ""may not exercise the rounding branch.
src/ipx.tsgates onhandlerContext.round !== undefined. If a bare flag maps toundefined, this case only asserts that a buffer comes back. Consider asserting a transparent corner pixel here (or confirming the default radius) so the no-arg form is genuinely covered.🤖 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/index.test.ts` at line 110, Update the test case using round: { round: "", format: "png" } to verify the rounding behavior rather than only successful output. Assert a transparent corner pixel or the expected default radius, ensuring the bare round flag maps to the branch guarded by handlerContext.round !== undefined.
🤖 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/handlers/utils.ts`:
- Around line 317-320: Update the pre-allocation estimate around the output
destructuring and width/height calculations so arbitrary-angle rotate operations
are treated as canGrow, or otherwise account for their rotated bounding-box
dimensions. Ensure animated inputs cannot materialize a raw buffer whose
estimated area exceeds max * max before applyRoundedCorners performs its
measured check.
---
Nitpick comments:
In `@src/ipx.ts`:
- Around line 88-91: Update the FlagModifier fields negate, normalize, and
normalise to use the same string-literal-preserving type pattern as kernel and
fit, such as a string intersection that retains editor hints, instead of a bare
string union that collapses to true | string.
In `@test/index.test.ts`:
- Line 110: Update the test case using round: { round: "", format: "png" } to
verify the rounding behavior rather than only successful output. Assert a
transparent corner pixel or the expected default radius, ensuring the bare round
flag maps to the branch guarded by handlerContext.round !== undefined.
🪄 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 Plus
Run ID: 23961a10-8a17-4ec3-a119-52ba65a7dbc1
📒 Files selected for processing (6)
README.mdsrc/handlers/handlers.tssrc/handlers/utils.tssrc/ipx.tstest/handlers/handlers.test.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/handlers/handlers.test.ts
- src/handlers/handlers.ts
`round` is the only modifier here that cannot be expressed as operations on the pipeline: sharp has no rounded corners operation, and the mask it has to be composited from must match the *output* dimensions, which are only known once the pipeline has run. That forced a raw round-trip and a second sharp pipeline, and with it a set of problems the rest of the modifiers do not have -- an unstreamed full-size buffer to bound, frame timing to carry over by hand, and an extra decode per request. Removed until it can be done in one pass. #187 stays open. Everything the review turned up that is not specific to it stays: the `clahe` window clamp and cost caps, late libvips failures surfacing as a `400`, eager colour parsing (`opacity` composites an SVG overlay too, so an unknown name would otherwise render as black), the `linear` and `brightness` / `saturation` ranges, and the `IPXModifiers` flag types. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pi0x
changed the title
opacity, round and the missing sharp operation modifiersopacity and the missing sharp operation modifiers
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ipx.ts (1)
461-477: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
backgroundwith the same parser used for the SVG overlay.
opacityOverlay()interpolatesbackgrounddirectly into an SVGfillattribute and passes it through Sharp’s SVG loader, while this validation only uses Sharp’screate.backgroundparser. Reject any color accepted byVColor()that Sharp can later fail to parse when rasterizing the SVG, or validate atVColor()with the SVG/Sharp rasterization parser.🤖 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/ipx.ts` around lines 461 - 477, Update the background validation in the handler flow around opacityOverlay and VColor to use the same SVG/Sharp rasterization parsing path that ultimately processes the interpolated fill value, rather than only Sharp’s create.background parser. Ensure colors accepted by VColor cannot later fail during SVG rasterization, while preserving the existing asModifierError conversion for validation failures.
🧹 Nitpick comments (1)
src/handlers/utils.ts (1)
300-324: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCompositing math checks out.
The
over+fill-opacitybranch and thedest-in+alpha branch both correctly implementimage·opacity + background·(1-opacity)and "scale existing alpha by opacity" respectively.One nit:
opacityOverlaydoesn't clamp/validateopacityitself (e.g.1 - opacitycould go negative if a caller passes an out-of-range value directly, since this is an exported helper). If range validation is guaranteed to happen in the calling handler before this is invoked, this is fine to leave as-is.🤖 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/handlers/utils.ts` around lines 300 - 324, Update the exported opacityOverlay function to validate or clamp opacity to the supported range before using it in fill-opacity or overlay alpha calculations, ensuring direct callers cannot produce out-of-range values. Preserve the existing background and dest-in compositing behavior for valid opacity values.
🤖 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.
Outside diff comments:
In `@src/ipx.ts`:
- Around line 461-477: Update the background validation in the handler flow
around opacityOverlay and VColor to use the same SVG/Sharp rasterization parsing
path that ultimately processes the interpolated fill value, rather than only
Sharp’s create.background parser. Ensure colors accepted by VColor cannot later
fail during SVG rasterization, while preserving the existing asModifierError
conversion for validation failures.
---
Nitpick comments:
In `@src/handlers/utils.ts`:
- Around line 300-324: Update the exported opacityOverlay function to validate
or clamp opacity to the supported range before using it in fill-opacity or
overlay alpha calculations, ensuring direct callers cannot produce out-of-range
values. Preserve the existing background and dest-in compositing behavior for
valid opacity values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 220c12cf-c78b-4386-a3a4-c592368a6f58
📒 Files selected for processing (6)
README.mdsrc/handlers/handlers.tssrc/handlers/utils.tssrc/ipx.tstest/handlers/handlers.test.tstest/index.test.ts
💤 Files with no reviewable changes (2)
- test/handlers/handlers.test.ts
- src/handlers/handlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
`autoorient`, `normalise` and `greyscale` were aliases carrying their own weight in `IPXModifiers` and the docs while adding nothing: one spelling per modifier is enough. `autoOrient`, `normalize` and `grayscale` are the names kept. The `threshold` boolean argument is renamed `grayscale` to match. sharp accepts either spelling for it, so the behaviour is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/handlers.ts (1)
422-466: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the no-argument
normalizeform.
normalizerequires bothlowerandupper, so a bare modifier path fails before Sharp runs. Sharp’s normalize API accepts omitted options with default bounds, so accept the empty form and only validate the supplied range.🤖 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/handlers/handlers.ts` around lines 422 - 466, Update the normalize handler’s argument definition and apply logic to accept a no-argument form, invoking Sharp’s default normalize behavior when both lower and upper are omitted. Validate and pass the supplied lower/upper range when provided, while preserving the existing invalid-range rejection behavior.Source: MCP tools
🧹 Nitpick comments (1)
test/index.test.ts (1)
176-185: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the opaque-output opacity path.
This test hard-codes
format: "png", so it does not exercise the advertised background compositing behavior for an opaque output. Add ajpg/jpegcase or parameterize the test.🤖 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/index.test.ts` around lines 176 - 185, Add coverage for the opaque-output background compositing path in the test describing blending into the background: use a JPEG format, or parameterize the existing PNG case to include JPEG, while preserving the transparent PNG assertion and verifying the advertised green background result for the opaque output.
🤖 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 `@README.md`:
- Line 269: Update the threshold documentation row to hyphenate “single-channel
grayscale” in the description, leaving the surrounding wording and examples
unchanged.
---
Outside diff comments:
In `@src/handlers/handlers.ts`:
- Around line 422-466: Update the normalize handler’s argument definition and
apply logic to accept a no-argument form, invoking Sharp’s default normalize
behavior when both lower and upper are omitted. Validate and pass the supplied
lower/upper range when provided, while preserving the existing invalid-range
rejection behavior.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 176-185: Add coverage for the opaque-output background compositing
path in the test describing blending into the background: use a JPEG format, or
parameterize the existing PNG case to include JPEG, while preserving the
transparent PNG assertion and verifying the advertised green background result
for the opaque output.
🪄 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 Plus
Run ID: 32852e55-ff4a-4ea3-be1f-fc7569d0c271
📒 Files selected for processing (5)
README.mdsrc/handlers/handlers.tssrc/ipx.tstest/handlers/handlers.test.tstest/index.test.ts
💤 Files with no reviewable changes (1)
- src/ipx.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/handlers/handlers.test.ts
Modifier names are matched case sensitively, and `autoOrient` was the only one that was not a single lowercase word, so it was the only one where the casing in a URL mattered. Renamed to match the rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`clahe` took ~4.3s on the full 3840x2160 source locally and tipped over the 5s default timeout on CI, which is slower and runs under coverage instrumentation. `opacity` was next in line at ~3.8s, spent encoding a full size png. Both are in the table that exercises argument handling against real sharp, so the source size is incidental: resizing first takes them to ~40ms without changing what they cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 91.33% 91.97% +0.64%
==========================================
Files 9 9
Lines 669 735 +66
Branches 196 218 +22
==========================================
+ Hits 611 676 +65
- Misses 49 50 +1
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Closes the gaps between the modifiers and sharp's operation API, and adds
opacity, which sharp does not provide directly.Resolves #240 (
opacity/brightness, plusbackgroundso the output is not limited to PNG). #187 (rounded edges) is not addressed here — see the note at the bottom.New modifiers
opacity_0.75dest-in(which multiplies the alpha channel). Required argument between0and1.brightness,saturation,hue,lightnessmodulateoption. sharp merges the calls, so/brightness_2,hue_90/stays a singlemodulateoperation. Addressing one option positionally (/modulate___90/) was awkward.autoorient,dilate,erode,clahe,linearNo aliases: one spelling per modifier, the American one where sharp offers both. Every modifier name is a single lowercase word, so URL casing never matters.
Existing modifiers
They now take the arguments they were missing:
sharpen—x1,y2,y3blur—precision,minAmplitudenegate—alphanormalize—lower,upperthreshold—grayscalebackground/bgained one more consumer.opacityotherwise produces transparency, which needs an output format with an alpha channel; with a background set it blends the image into it instead (/opacity_0.75,b_fff,f_jpeg/), so the output stays opaque — this is the "avoid limiting the output to PNG" half of #240.Implementation notes
400 IPX_INVALID_MODIFIER_ARGinstead of surfacing as a500. Two deliberate deviations, both documented:dilate,erodeandclaheare capped at100rather than sharp's65536(the cost grows with the window —dilate_1000alone takes minutes on a 4K image), and theclahewindow is clamped to the source, since libvips rejects one larger than the image.400. Some arguments are only validated once the pipeline runs, after every handler has been applied, so they escapedapplyHandler's existing wrapping. This also covers pre-existing cases such as/extract_0_0_99999_99999/, which was a500onmain.opacitycomposites its overlay through an SVG, where an unknown name would silently render as black rather than being rejected the way/flatten,b_notacolour/is.VColoronly lets a hex, functional or named colour through, none of which can contain a quote or an angle bracket.negate,threshold) accepttrue/falseand the shorter1/0.IPXModifiersflag modifiers now admit the""they are actually parsed from, so{ animated: "" }type checks.Deliberately left out
affine,convolveandrecombtake a matrix, which fits a URL modifier poorly and is unbounded in cost.booleantakes a second image by path or buffer.On rounded corners (#187)
roundwas implemented here and then reverted (62f46b1). It is the only modifier in this set that cannot be expressed as operations on the pipeline: sharp has no rounded corners operation, and the mask has to match the output dimensions, which are only known once the pipeline has run. That forced a raw round-trip and a second sharp pipeline, and with it a set of problems none of the other modifiers have — an unstreamed full-size buffer to bound (an animated image stacks every frame, somaxOutputDimensiondoes not cover it), frame delays and loop count to carry over by hand, and an extra decode per request. Left for a follow-up that can do it in one pass; #187 stays open.Tests
opacityis composited rather than mapped to a sharp operation, so it is asserted on the pixels themselves (the alpha is halved; with a background the result is the blend and stays opaque). Every other new modifier and argument is exercised end-to-end against real sharp — mocking it hides its own validation — plus the usual valid/invalid argument tables.361 passing,
eslintandprettierclean.tsc --noEmitreports only the three pre-existingexamples/*.tserrors that needpnpm buildfirst.🤖 Generated with Claude Code
Summary by CodeRabbit
autoOrient,dilate,erode,clahe,linear, and per-channelbrightness,saturation,hue,lightness.sharpen,blur, andopacitycapabilities.opacitycompositing (including proper background blending and safer alpha handling).negate,normalize,threshold, andbackground/tintparsing (now acceptstrue/falsein addition to1/0).