feat(directus): add Sharp transforms & key modifier.#2207
Conversation
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
=======================================
Coverage 32.61% 32.61%
=======================================
Files 7 7
Lines 371 371
Branches 131 131
=======================================
Hits 121 121
Misses 194 194
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kheiner
left a comment
There was a problem hiding this comment.
Looking forward to the feedback.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds typed Sharp operation support and a keyed modifier to the Directus image provider. DirectusOptions gains top-level image fields (width, height, quality, format, fit) and modifiers become either a keyed form ({key}) or operation-list form. getImage now emits ?key=... for keyed modifiers or serializes Sharp operations for other modifiers. Documentation, playground samples, tests, and snapshots were updated to use the sandbox Directus instance and to cover the new keyed behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/runtime/providers/directus.ts (1)
102-114: Transforms deduplication approach works but has a minor inefficiency.The JSON stringify/parse approach for deduplication is functional but performs redundant parsing. Consider using a
Mapor filtering duplicates in a single pass for better performance, though this is a minor concern given typical transform array sizes.♻️ Alternative deduplication approach
const operationsGenerator = createOperationsGenerator({ valueMap: { transforms(value: SharpOperation[]) { - return value.length > 0 - ? JSON.stringify( - Array.from(new Set(value.map(v => JSON.stringify(v)))) - .map(v => JSON.parse(v)), - ) - : undefined + if (value.length === 0) return undefined + const seen = new Set<string>() + const unique = value.filter((v) => { + const key = JSON.stringify(v) + if (seen.has(key)) return false + seen.add(key) + return true + }) + return JSON.stringify(unique) }, }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/providers/directus.ts` around lines 102 - 114, The transforms valueMap currently deduplicates by JSON.stringify/parse which works but does extra parsing; update the transforms function inside createOperationsGenerator (the operationsGenerator/valueMap/transforms handler for SharpOperation[]) to deduplicate in a single pass (e.g., use a Map or Set of serialized entries to filter duplicates while building the resulting array) and then JSON.stringify that deduplicated array (return undefined for empty), keeping the same return shape and types as before.docs/content/3.providers/directus.md (1)
41-41: Minor: Use hyphen for compound adjective.For grammatical correctness, "Directus specific" should be "Directus-specific" when used as a compound adjective.
📝 Proposed fix
-The `modifiers` object is used for Directus specific features. All modifiers are optional. +The `modifiers` object is used for Directus-specific features. All modifiers are optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/3.providers/directus.md` at line 41, Update the sentence describing the "modifiers" object to use a hyphenated compound adjective: change "Directus specific features" to "Directus-specific features" so the phrase correctly reads "The `modifiers` object is used for Directus-specific features. All modifiers are optional." Ensure the `modifiers` symbol remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/content/3.providers/directus.md`:
- Line 41: Update the sentence describing the "modifiers" object to use a
hyphenated compound adjective: change "Directus specific features" to
"Directus-specific features" so the phrase correctly reads "The `modifiers`
object is used for Directus-specific features. All modifiers are optional."
Ensure the `modifiers` symbol remains unchanged.
In `@src/runtime/providers/directus.ts`:
- Around line 102-114: The transforms valueMap currently deduplicates by
JSON.stringify/parse which works but does extra parsing; update the transforms
function inside createOperationsGenerator (the
operationsGenerator/valueMap/transforms handler for SharpOperation[]) to
deduplicate in a single pass (e.g., use a Map or Set of serialized entries to
filter duplicates while building the resulting array) and then JSON.stringify
that deduplicated array (return undefined for empty), keeping the same return
shape and types as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c8abb98-16cf-45a3-8906-78b5a8da1ec5
📒 Files selected for processing (7)
docs/content/3.providers/directus.mdplayground/app/providers.tsplayground/nuxt.config.tssrc/runtime/providers/directus.tstest/e2e/__snapshots__/directus.json5test/nuxt/providers.test.tstest/providers.ts
key modifier.
|
Really hoping that I can get this added in to v2.1.0 #2028 let me know if there's anything needed on my end. |
ef4a3f1 to
fb2c1ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/content/3.providers/directus.md (1)
119-120: ⚡ Quick winDocument that
keyis mutually exclusive with other modifiers.The
keyfield description should explicitly state it cannot be combined with other image modifiers, since that’s a core behavior of this feature.Suggested doc wording
-::field{name="key" type="string"} -Sets a unique identifier for allowing faster and easier image transformation requests. +::field{name="key" type="string"} +Sets a unique identifier for a pre-configured Directus transform. When `key` is provided, do not combine it with other modifiers.🤖 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 `@docs/content/3.providers/directus.md` around lines 119 - 120, Update the ::field{name="key" type="string"} description to explicitly state that the key is mutually exclusive with other image modifiers: mention that when a key is provided all other image transformation/modifier query parameters or settings are ignored and cannot be combined with the key, and provide a short suggested sentence such as "This unique key cannot be used together with any other image modifiers; when a key is present, other transformation parameters are ignored."
🤖 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 `@docs/content/3.providers/directus.md`:
- Around line 51-53: The example object uses a string for the withoutEnlargement
property (currently 'true') while the field is documented as a boolean; update
the example in the :modifiers object to use the boolean true (without quotes)
for withoutEnlargement so it matches the documented type and prevents copy-paste
errors when using the withoutEnlargement option.
- Line 82: The docs use an invalid TypeScript tuple-array for the "transforms"
field: change the type in the ::field{name="transforms"} declaration from
"['string', ...any][]" to the correct tuple-array syntax "[string, ...any[]][]"
so the first element is a string type and the rest are any[]; update the
declaration for the transforms field to use [string, ...any[]][] accordingly.
---
Nitpick comments:
In `@docs/content/3.providers/directus.md`:
- Around line 119-120: Update the ::field{name="key" type="string"} description
to explicitly state that the key is mutually exclusive with other image
modifiers: mention that when a key is provided all other image
transformation/modifier query parameters or settings are ignored and cannot be
combined with the key, and provide a short suggested sentence such as "This
unique key cannot be used together with any other image modifiers; when a key is
present, other transformation parameters are ignored."
🪄 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: 83639579-91c9-49de-b1c1-330932c0b9d2
📒 Files selected for processing (7)
docs/content/3.providers/directus.mdplayground/app/providers.tsplayground/nuxt.config.tssrc/runtime/providers/directus.tstest/e2e/__snapshots__/directus.json5test/nuxt/providers.test.tstest/providers.ts
✅ Files skipped from review due to trivial changes (1)
- test/e2e/snapshots/directus.json5
🚧 Files skipped from review as they are similar to previous changes (5)
- playground/nuxt.config.ts
- test/nuxt/providers.test.ts
- test/providers.ts
- playground/app/providers.ts
- src/runtime/providers/directus.ts
|
@danielroe Are you able to review/merge this? Open to feedback and fixes but I'm really proud of this work and it solves a real problem for me. Thanks in advance! |
|
my apologies for the delay! will get this in for that release 🙏 |
Intentionally omitting Resizing operations at the moment due to coverage in Nuxt/Image and abstraction available to Directus. TODO: Fix duplication of transform object.
Updated Directus playground/e2e_snapshots to use the Directus Sandbox.
…orms Fixes nuxt#2194 * Implements the ability to use a `key` to use a preconfigured Directus Transform set. * Adds Types for Sharp Transforms [intentionally omitted resizing since the basic functionality is abstracted in the nuxt/image module] * Fixes issue of incorrect typing for transforms to allow for `[string, ...any[]]` (was incorrectly typed as `string[]`)
Whitespace was causing tests to fail;
* Sort import for `providers.test.ts` (NOTE: PR nuxt#2193 greatly improves test and readability) * Add E2E snapshot for `key` modifier. * Fix missing test in `providers.test.ts` * Add `key` test for `providers.test.ts`
Updated Docs to include: * `key` :modifier * Explain Sharp Transforms and provide examples.
Remove unnecessary comments
7157525 to
f6f0e6a
Compare
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 `@docs/content/3.providers/directus.md`:
- Line 26: The sentence claiming "The NuxtImg Props map cleanly to the Directus
Transforms" is too broad; change it to state that only a subset of NuxtImg props
map directly (e.g., width, height, quality, format, fit) and clarify that other
Directus-specific behavior must be expressed via the modifiers option. Update
the sentence to explicitly list or refer to the supported props
(width/height/quality/format/fit) and add a short clause pointing readers to use
modifiers for additional Directus transform functionality.
🪄 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: 53fbcce2-717e-40f3-abb8-5a99547da2fb
📒 Files selected for processing (7)
docs/content/3.providers/directus.mdplayground/app/providers.tsplayground/nuxt.config.tssrc/runtime/providers/directus.tstest/e2e/__snapshots__/directus.json5test/nuxt/providers.test.tstest/providers.ts
✅ Files skipped from review due to trivial changes (2)
- playground/nuxt.config.ts
- test/e2e/snapshots/directus.json5
🚧 Files skipped from review as they are similar to previous changes (4)
- test/nuxt/providers.test.ts
- playground/app/providers.ts
- test/providers.ts
- src/runtime/providers/directus.ts
Clarifies the NuxtImg Props mapping to Directus Transforms
|
Coderabbit is going to start suggesting changes to the code recommendations itself made. I'll keep rebasing as necessary, hoping for a merge soon :). |
🔗 Linked issue
keyed transforms #2194📚 Description
keystring, stripping all other transforms supported by Nuxt/Imagekeyif present is not paired with other modifiers.keys in the public space for better explanation to developers.string[]which is not valid. Transforms should fallback to a tuple of type[string, ...any[]][]to prevent errors with using transforms not typed explicitly. (Future proofing for new transforms added to Sharp & supported by Directus.)keymodifier.