Conversation
commit: |
|
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:
📝 WalkthroughWalkthroughAdds full Imgproxy support as a new Nuxt Image provider. Introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
🤖 Fix all issues with AI agents
In `@docs/content/3.providers/imgproxy.md`:
- Around line 15-25: The example Nuxt config uses the wrong provider key: change
the image provider object from imageengine to imgproxy so the config under
defineNuxtConfig({ image: { ... } }) targets the correct provider; update the
object key name (image.imageengine -> image.imgproxy) and keep the same baseURL,
key and salt fields under imgproxy to ensure the imgproxy provider is configured
correctly.
- Around line 97-119: Change the incorrect provider attribute and fix the
example numbering in the NuxtImg examples: replace provider="imagick" with
provider="imgproxy" for both NuxtImg components (the blur example and the
presets example) and update the heading text from "Example 2:" to "Example 3:"
for the presets example so numbering is sequential; ensure references to
:modifiers, src, width and height remain unchanged.
- Around line 36-42: The fenced code block showing the Imgproxy provider
modifiers (the block containing resizingType, gravity, enlarge, format) is
missing a language identifier; update the opening triple-backticks to include a
language (e.g., ```ts) so the block is fenced as TypeScript and satisfies
markdownlint MD040.
In `@playground/app/providers.ts`:
- Around line 440-500: The sample inside the 'imgproxy' provider contains an
invalid rotation value (modifiers.rotate: 192) — imgproxy only accepts multiples
of 90; locate the sample object with width: 500, height: 500 and
modifiers.rotate and change the rotate value to a valid multiple of 90 (e.g.,
180 or 270) so the modifiers.rotate property is compliant.
In `@src/runtime/providers/imgproxy.ts`:
- Around line 92-164: The rotate handler in the operationsGenerator valueMap
currently floors the raw value to a multiple of 90 after validating the original
value, which allows inputs like 1–89 to become 0 (producing an invalid rot:0);
change the logic in the rotate function to first normalize the input to the
nearest lower multiple of 90 (e.g., normalized = value - (value % 90)), then
validate the normalized angle (ensure it's a number and not 0, and within
allowed rotation values like 90, 180, 270; reject <=0 or >=360), and finally
return the normalized angle; update the rotate entry in valueMap accordingly to
prevent rot:0 from being emitted.
🧹 Nitpick comments (2)
docs/content/3.providers/imgproxy.md (1)
20-21: Consider using placeholder values instead of real-looking hex keys in documentation.Static analysis (Gitleaks) flagged the key/salt as potential API key exposure. While these are test values, using obvious placeholders like
your-imgproxy-key-hexwould be clearer to readers and avoid false positives in secret scanners.src/runtime/providers/imgproxy.ts (1)
196-217: Avoid mutating caller-provided modifiers.
resolveModifiersdeletesfitand setsresizingTypeon the provided object, which can surprise callers who reuse the same modifiers object. Consider cloning first.♻️ Suggested refactor
-function resolveModifiers(modifiers: Partial<ImgproxyModifiers>): Partial<ImgproxyModifiers> { - if (modifiers?.fit) { - switch (modifiers.fit) { +function resolveModifiers(modifiers: Partial<ImgproxyModifiers>): Partial<ImgproxyModifiers> { + const resolved = { ...modifiers } + if (resolved?.fit) { + switch (resolved.fit) { case 'contain': - modifiers.resizingType = 'fill' + resolved.resizingType = 'fill' break default: - modifiers.resizingType = modifiers.fit + resolved.resizingType = resolved.fit break } - delete modifiers.fit + delete resolved.fit } - return modifiers + return resolved }
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
=======================================
Coverage 32.52% 32.52%
=======================================
Files 7 7
Lines 372 372
Branches 131 131
=======================================
Hits 121 121
Misses 194 194
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/content/3.providers/imgproxy.md`:
- Line 95: The example uses a string for the rotate modifier which conflicts
with the ImgproxyModifiers type (rotate should be a number); update the example
modifier from { rotate: '180' } to use a numeric value ({ rotate: 180 }) in the
:modifiers usage and scan nearby examples to ensure all ImgproxyModifiers fields
(like rotate) use the correct types.
- Around line 32-33: Remove the empty "Imgproxy default options" section or
merge it into the following "Imgproxy Modifiers" section; specifically, delete
the standalone heading "Imgproxy default options" and either move its intended
default-value content (currently described around the "Imgproxy Modifiers" area)
directly under the "Imgproxy Modifiers" heading so defaults and modifiers are
combined, or eliminate the heading entirely if no separate content is needed;
update the document so there are no back-to-back headings with no content
between them.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/runtime/providers/imgproxy.ts`:
- Around line 154-160: The rotate validator (rotate) incorrectly rejects 359 due
to an off-by-one: change the upper-bound check from ">= 359" to ">= 360" so the
condition becomes typeof value !== 'number' || value < 0 || value >= 360,
leaving the existing return value - (value % 90) logic intact to allow valid
rotations like 359 to floor to 270; update the TypeError message if you want but
the core fix is the bound change in rotate.
- Around line 92-132: The keyMap passed into createOperationsGenerator for
operationsGenerator is missing the extend mapping, so the extend modifier is
emitted as "extend" instead of the imgproxy shorthand "ex"; add an entry
"extend: 'ex'" to the keyMap object (alongside keys like resize, size,
extendAspectRatio) so that the extend modifier is correctly translated to "ex"
when operationsGenerator builds the operations string.
🧹 Nitpick comments (2)
src/runtime/providers/imgproxy.ts (2)
174-186:String.fromCharCode(...bytes)may throw on very large inputs.The spread operator is subject to engine-specific argument count limits. While typical image URLs and HMAC digests are well within bounds, consider a chunk-based approach for robustness:
♻️ Safer alternative
- const binaryString = String.fromCharCode(...bytes) + let binaryString = '' + for (let i = 0; i < bytes.length; i++) { + binaryString += String.fromCharCode(bytes[i]) + }
219-231: Support imgproxy's unsigned/insecure mode.The code requires
keyandsaltto be configured and always computes an HMAC signature. Users running imgproxy withoutIMGPROXY_KEY/IMGPROXY_SALT(unsigned mode) cannot use this provider. When signing is disabled, imgproxy expects the signature segment to be a placeholder (e.g.,insecure,unsafe,_) rather than a computed value.Consider detecting when
keyand/orsaltare undefined or empty, and use a placeholder string in place of the computed signature for unsigned mode.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/providers/imgproxy.ts`:
- Around line 243-257: The resolveModifiers function currently assigns
modifiers.resizingType = modifiers.fit in the default branch, allowing
unsupported Nuxt Image fit strings to become invalid imgproxy resizingType
values; update resolveModifiers (and the ImgproxyModifiers handling) to
explicitly map Nuxt fit values to imgproxy resizingType values (e.g., 'contain'
-> 'fill', 'cover' -> 'fill', 'inside' -> 'fit' or 'auto' per desired behavior,
'outside' -> 'fill-down', etc.), and for any unsupported fit value either
convert to a safe default (one of 'fit','fill','fill-down','force','auto') or
remove the fit key and emit a warning/error via the existing logger so invalid
imgproxy parameters are never produced.
🧹 Nitpick comments (4)
docs/content/3.providers/imgproxy.md (1)
121-123: Nit: Remove trailing blank lines at end of file.There are three blank lines at the end of the file. A single newline is sufficient.
src/runtime/providers/imgproxy.ts (3)
188-194:hexToBytessilently producesNaNbytes on invalid hex input.If
keyorsaltcontain non-hex characters or have an odd length,Number.parseIntreturnsNaN, which gets stored in theUint8Arrayas0. This silently corrupts the signing key/salt, producing valid-looking but incorrect signatures — hard to debug.Proposed defensive fix
function hexToBytes(hex: string): Uint8Array { + if (hex.length % 2 !== 0) { + throw new Error('Hex string must have an even length') + } const bytes = new Uint8Array(hex.length / 2) for (let i = 0; i < hex.length; i += 2) { - bytes[i / 2] = Number.parseInt(hex.slice(i, i + 2), 16) + const byte = Number.parseInt(hex.slice(i, i + 2), 16) + if (Number.isNaN(byte)) { + throw new Error(`Invalid hex character at position ${i}`) + } + bytes[i / 2] = byte } return bytes }
201-213:String.fromCharCode(...bytes)can throw on very large byte arrays.The spread operator has an engine-specific argument limit (~65k–125k). For this provider the inputs are HMAC digests (32 bytes) and URL strings (typically short), so this is unlikely to trigger in practice. However, if the provider is ever used with very long source URLs, this could silently fail. A chunked approach would be more robust.
263-275: No validation thatkeyandsaltare provided before signing.If a user forgets to configure
keyorsalt,hexToByteswill be called withundefined, causing a runtime error deep in the signing logic. An early guard with a clear error message would improve DX.Proposed fix
getImage: (src, { modifiers, baseURL, key, salt }) => { + if (!key || !salt) { + throw new Error('[imgproxy] `key` and `salt` are required options for the imgproxy provider') + } const mergeModifiers = resolveModifiers(defu(modifiers, defaultModifiers))
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/3.providers/imgproxy.md`:
- Around line 154-170: The headings "Summary Table" and "Dimension Edge Cases"
are using single-# (h1) but the document uses ## for top-level sections; update
those two headings to the correct level (change "# Summary Table" and "#
Dimension Edge Cases" to "## Summary Table" and "## Dimension Edge Cases" or to
"###" if they should be subsections under the existing fit documentation) so the
heading hierarchy matches the rest of the document and the outline remains
consistent.
🧹 Nitpick comments (6)
src/runtime/providers/imgproxy.ts (4)
9-62: Redundant't'literal inImgproxyBooleanPrimitiveunion.Line 22:
string | number | boolean | 't'— the literal't'is already a subtype ofstring, making it redundant.-export type ImgproxyBooleanPrimitive = string | number | boolean | 't' +export type ImgproxyBooleanPrimitive = string | number | boolean
201-213:String.fromCharCode(...bytes)can throwRangeErroron large inputs.The spread operator has a JS engine-imposed argument limit (~65k–130k depending on engine). For typical image source URLs this is fine, but if someone passes an unusually large URL or binary payload,
urlSafeBase64will throw. Consider chunking for robustness:Safer alternative
- const binaryString = String.fromCharCode(...bytes) + let binaryString = '' + for (let i = 0; i < bytes.length; i++) { + binaryString += String.fromCharCode(bytes[i]) + }
188-194:hexToBytessilently produces garbage for invalid hex input.If a user misconfigures
keyorsaltwith an odd-length string or non-hex characters,Number.parseIntreturnsNaNand the resulting signature will be silently wrong. Consider adding minimal validation.Suggested guard
function hexToBytes(hex: string): Uint8Array { + if (hex.length % 2 !== 0 || !/^[\da-f]+$/i.test(hex)) { + throw new Error('Invalid hex string') + } const bytes = new Uint8Array(hex.length / 2)
278-290: Provider will throw at runtime ifkeyorsaltare not configured.When
keyorsaltareundefined(user forgets to configure them),hexToBytesreceivesundefined, and thehmaccall will fail with an unhelpful error. A guard with a clear message would improve DX.Suggested guard
getImage: (src, { modifiers, baseURL, key, salt }) => { + if (!key || !salt) { + throw new Error('[imgproxy] `key` and `salt` must be configured in the imgproxy provider options.') + } const mergeModifiers = resolveModifiers(defu(modifiers, defaultModifiers))docs/content/3.providers/imgproxy.md (2)
16-26: Documentation example exposes key/salt values — add a note that these are for local development only.While these are clearly example values for a local Docker setup, it would help to add a brief comment or note reminding users to use environment variables or secrets management in production.
273-289: Example 4 may conflict withresolveModifiersbehavior.The advanced example passes
resize: 'fit:100:100:1:1'alongsidewidth: 100andheight: 100(implicitly via the component). Since nofitis specified,resolveModifierswon't setresizingType, and the defaultresizingType: 'auto'fromdefaultModifierswill be applied viadefu. This means the URL will contain bothrt:autoandrs:fit:100:100:1:1, which may be confusing. Consider adding a note thatresizeis a raw passthrough that overrides the normal resizing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.nuxtrc:
- Line 1: The .nuxtrc currently pins setups.@nuxt/test-utils to "3.23.0"; update
that value to "4.0.0" to use the latest release, then run the test suite and
linting to verify compatibility; if tests fail, review breaking changes between
`@nuxt/test-utils` v3 and v4 and adjust test setup/configs referenced in your test
harness (e.g., fixtures, globals, or setup files) accordingly until the suite
passes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/3.providers/imgproxy.md`:
- Around line 188-189: Update the sentence to correct grammar and add a concrete
example showing how to override default imgproxy modifiers in nuxt.config.ts
using defineNuxtConfig; specifically mention the image.imgproxy.modifiers object
and show how to set properties like resizingType, gravity, and format so readers
can copy the pattern into their project (replace placeholder key/salt/baseURL
values).
🧹 Nitpick comments (1)
docs/content/3.providers/imgproxy.md (1)
20-22: Clarify that key and salt are example values.The hex-encoded
keyandsaltvalues should be explicitly noted as examples for local development to prevent users from mistaking them for production credentials or valid keys.📝 Suggested clarification
Add a comment or note after the code block:
})
+> Note: The
keyandsaltvalues shown above are examples for local development. Generate your own secure random values for production use. See the Imgproxy configuration documentation for details on generating keys.</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/3.providers/imgproxy.md`:
- Around line 16-23: The docs currently show realistic-looking secret values for
imgproxy configuration (inside defineNuxtConfig with image.imgproxy.baseURL, key
and salt); replace those literal key/salt strings with obvious placeholders
(e.g. YOUR_IMGPROXY_KEY, YOUR_IMGPROXY_SALT or <IMGPROXY_KEY>, <IMGPROXY_SALT>)
in the code block and any docker-compose examples so readers cannot accidentally
reuse or leak real secrets, keeping baseURL as an example URL like
http://localhost:8080/ or <IMGPROXY_BASE_URL>.
|
Hey, @danielroe. Could you check the PR? Thanks! |
Nks
force-pushed
the
feature/add-imgproxy-support
branch
from
ad4db79 to
d5f27d6
Compare
danielroe
changed the title
…lt fit modifier; Updated documentation;
Nks
force-pushed
the
feature/add-imgproxy-support
branch
from
d5f27d6 to
cfdf8fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/content/3.providers/imgproxy.md (1)
44-46: Optional docs polish: vary repeated sentence openings in “Dimension Handling”.All three bullets begin with “If…”. Rewording one or two improves readability without changing meaning.
✍️ Example wording
-- If both `width` and `height` are positive numbers → **both dimensions are considered defined**. -- If only one dimension is provided → behavior falls back to proportional resizing. -- If neither dimension is provided → resizing defaults to proportional behavior. +- When both `width` and `height` are positive numbers, **both dimensions are considered defined**. +- With only one dimension provided, behavior falls back to proportional resizing. +- Without either dimension, resizing defaults to proportional behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/3.providers/imgproxy.md` around lines 44 - 46, In the "Dimension Handling" section update the three bullets so they don't all begin with "If..." while preserving meaning: rewrite the first to state that providing both width and height means both dimensions are defined, rephrase the second to indicate that supplying only one dimension causes proportional resizing, and adjust the third to say that omitting both dimensions defaults to proportional behavior; keep the technical semantics unchanged and maintain the same ordering and emphasis.test/nuxt/providers.test.ts (1)
270-288: Optional: deduplicate repeated imgproxy provider options constant.The same options object is declared twice; extracting one shared constant will reduce maintenance noise.
♻️ Suggested refactor
+const IMGPROXY_PROVIDER_OPTIONS = { + baseURL: 'http://localhost:8080/', + key: 'ee3b0e07dfc9ec20d5d9588a558753547a8a88c48291ae96171330daf4ce2800', + salt: '8dd0e39bb7b14eeaf02d49e5dc76d2bc0abd9e09d52e7049e791acd3558db68e', +} + it('imgproxy', () => { - const providerOptions = { - baseURL: 'http://localhost:8080/', - key: 'ee3b0e07dfc9ec20d5d9588a558753547a8a88c48291ae96171330daf4ce2800', - salt: '8dd0e39bb7b14eeaf02d49e5dc76d2bc0abd9e09d52e7049e791acd3558db68e', - } + const providerOptions = IMGPROXY_PROVIDER_OPTIONS ... }) it('imgproxy modifiers', () => { - const providerOptions = { - baseURL: 'http://localhost:8080/', - key: 'ee3b0e07dfc9ec20d5d9588a558753547a8a88c48291ae96171330daf4ce2800', - salt: '8dd0e39bb7b14eeaf02d49e5dc76d2bc0abd9e09d52e7049e791acd3558db68e', - } + const providerOptions = IMGPROXY_PROVIDER_OPTIONS ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt/providers.test.ts` around lines 270 - 288, The test duplicates the same imgproxy providerOptions object; extract it into a single shared constant (e.g., IMGPROXY_PROVIDER_OPTIONS) used by both tests to avoid repetition—update occurrences where providerOptions is passed to imgproxy().getImage and any other references, keeping the same property values (baseURL, key, salt) and ensuring imports/fixtures (imgproxy, getEmptyContext) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/providers/imgproxy.ts`:
- Around line 188-193: hexToBytes currently converts any string into bytes
without validating input, which lets odd-length or non-hex characters produce
wrong signatures; update hexToBytes to validate the input (in function
hexToBytes) by checking the string only contains [0-9a-fA-F] and has even length
(optionally enforce expected key/salt lengths if known), and throw a clear Error
(e.g., "Invalid hex string for signing key/salt: must be even-length hex") when
validation fails so invalid inputs fail fast and produce actionable logs.
---
Nitpick comments:
In `@docs/content/3.providers/imgproxy.md`:
- Around line 44-46: In the "Dimension Handling" section update the three
bullets so they don't all begin with "If..." while preserving meaning: rewrite
the first to state that providing both width and height means both dimensions
are defined, rephrase the second to indicate that supplying only one dimension
causes proportional resizing, and adjust the third to say that omitting both
dimensions defaults to proportional behavior; keep the technical semantics
unchanged and maintain the same ordering and emphasis.
In `@test/nuxt/providers.test.ts`:
- Around line 270-288: The test duplicates the same imgproxy providerOptions
object; extract it into a single shared constant (e.g.,
IMGPROXY_PROVIDER_OPTIONS) used by both tests to avoid repetition—update
occurrences where providerOptions is passed to imgproxy().getImage and any other
references, keeping the same property values (baseURL, key, salt) and ensuring
imports/fixtures (imgproxy, getEmptyContext) remain unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.nuxtrcdocs/content/3.providers/imgproxy.mdpackage.jsonplayground/app/providers.tsplayground/nuxt.config.tssrc/provider.tssrc/runtime/providers/imgproxy.tstest/e2e/__snapshots__/imgproxy.json5test/nuxt/providers.test.tstest/providers.ts
✅ Files skipped from review due to trivial changes (1)
- .nuxtrc
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/snapshots/imgproxy.json5
- playground/app/providers.ts
- src/provider.ts
- package.json
| function hexToBytes(hex: string): Uint8Array { | ||
| const bytes = new Uint8Array(hex.length / 2) | ||
| for (let i = 0; i < hex.length; i += 2) { | ||
| bytes[i / 2] = Number.parseInt(hex.slice(i, i + 2), 16) | ||
| } | ||
| return bytes |
There was a problem hiding this comment.
Validate hex input for signing key/salt before conversion.
hexToBytes currently accepts malformed/odd-length hex strings silently, which can produce invalid signatures and hard-to-debug runtime failures.
🔧 Proposed fix
function hexToBytes(hex: string): Uint8Array {
+ if (!hex || hex.length % 2 !== 0 || !/^[\da-f]+$/i.test(hex)) {
+ throw new TypeError('Imgproxy key/salt must be a non-empty even-length hex string')
+ }
+
const bytes = new Uint8Array(hex.length / 2)
for (let i = 0; i < hex.length; i += 2) {
bytes[i / 2] = Number.parseInt(hex.slice(i, i + 2), 16)
}
return bytes
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/providers/imgproxy.ts` around lines 188 - 193, hexToBytes
currently converts any string into bytes without validating input, which lets
odd-length or non-hex characters produce wrong signatures; update hexToBytes to
validate the input (in function hexToBytes) by checking the string only contains
[0-9a-fA-F] and has even length (optionally enforce expected key/salt lengths if
known), and throw a clear Error (e.g., "Invalid hex string for signing key/salt:
must be even-length hex") when validation fails so invalid inputs fail fast and
produce actionable logs.
2 participants
🔗 Linked issue
resolves #378
closes #1337
❓ Type of change
📚 Description
Uncompleted and abandoned PR: #1337
Instead of hash.js for HMAC used https://github.com/paulmillr/noble-hashes, since ohash doesn't support hmac.
Added the ability to use imgproxy with most options (excluding pro parameters, as I cannot verify them).
For local testing, docker compose can be used:
Example from playground:
