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.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.
I don't understand why the tests specifically care about the order, nor did I do extensive research on why I needed to put the requests in a different order than the sources for a successful test. If I'm hiding a problem here, I'd like to know about it but as far as I can tell the order has no meaning outside of the test.
kheiner
left a comment
kheiner
left a comment
There was a problem hiding this comment.
Looking forward to the feedback.
📝 WalkthroughWalkthroughThis pull request implements typed Sharp operation support and keyed transform functionality for the Directus image provider. The core provider implementation adds type-safe Sharp operation definitions, supports mutually-exclusive keyed transforms via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 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
kheiner
changed the title
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. |
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
kheiner
force-pushed
the
feat/add-directus-transform-types
branch
from
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
| :modifiers="{ | ||
| height: '512', | ||
| withoutEnlargement: 'true', | ||
| transforms: [['blur', 4], ['negate']] |
There was a problem hiding this comment.
Use boolean in the withoutEnlargement example, not string.
Line 52 uses 'true' (string), but Line 78 documents this field as boolean. This copy-paste example should use true to prevent incorrect usage.
Suggested doc fix
- withoutEnlargement: 'true',
+ withoutEnlargement: true,📝 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.
| :modifiers="{ | |
| height: '512', | |
| withoutEnlargement: 'true', | |
| transforms: [['blur', 4], ['negate']] | |
| :modifiers="{ | |
| withoutEnlargement: true, | |
| transforms: [['blur', 4], ['negate']] |
🤖 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 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.
| Disable automatically upscaling the image when true. | ||
| :: | ||
|
|
||
| ::field{name="transforms" type="['string', ...any][]"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file around line 82 to see the actual content
cat -n docs/content/3.providers/directus.md | sed -n '75,90p'Repository: nuxt/image
Length of output: 940
🏁 Script executed:
# Also check if there's any TypeScript definition or PR description that clarifies the intended type
fd -t f -e ts -e d.ts | head -20 | xargs grep -l "transforms" 2>/dev/null | head -5Repository: nuxt/image
Length of output: 82
🏁 Script executed:
# Search for any references to the transforms type in the codebase
rg "transforms.*type" --type markdown -A 2 -B 2Repository: nuxt/image
Length of output: 395
🏁 Script executed:
# Search for the transforms type definition in TypeScript source files
rg "transforms\s*:" --type ts -A 3 -B 1 | head -50Repository: nuxt/image
Length of output: 329
🏁 Script executed:
# Look for Directus provider implementation to understand the actual type
fd -t f -name "*directus*" | grep -E "\.(ts|js)$"Repository: nuxt/image
Length of output: 285
🏁 Script executed:
# Check if there's any type definition file that shows the transforms type
rg "transforms.*\[.*\]" --type ts -A 1 | head -30Repository: nuxt/image
Length of output: 465
🏁 Script executed:
# Find where SharpOperation is defined
rg "type SharpOperation|interface SharpOperation" --type ts -A 5Repository: nuxt/image
Length of output: 888
🏁 Script executed:
# Also check the imports and type definitions in the directus provider
cat -n src/runtime/providers/directus.ts | head -40Repository: nuxt/image
Length of output: 2043
🏁 Script executed:
# Get more of the directus.ts file to see the full SharpOperation type definition
cat -n src/runtime/providers/directus.ts | sed -n '40,120p'Repository: nuxt/image
Length of output: 2784
🏁 Script executed:
# Also check if there are any type definitions that match the tuple with rest pattern
rg "\[string.*\]" --type ts -A 1 -B 1 | head -20Repository: nuxt/image
Length of output: 642
🏁 Script executed:
# Let me verify the TypeScript rest element syntax is correct
# Check for similar patterns in the codebase
rg "\.\.\." --type ts -B 1 -A 1 | grep -E "^\s*\[.*\.\.\." | head -10Repository: nuxt/image
Length of output: 36
🏁 Script executed:
# Look at the actual usage in the playground to understand what the transforms value looks like
cat -n playground/app/providers.ts | grep -A 3 -B 3 "transforms"Repository: nuxt/image
Length of output: 549
Fix transforms type notation to valid TypeScript tuple-array syntax.
Line 82 currently shows ['string', ...any][], which uses invalid TypeScript syntax. The quoted string literal 'string' should be the unquoted string type, and the rest element must be written as ...any[]. Use [string, ...any[]][] to accurately represent the tuple structure where each transform is an array starting with an operation name (string) followed by any arguments.
Suggested doc fix
-::field{name="transforms" type="['string', ...any][]"}
+::field{name="transforms" type="[string, ...any[]][]"}📝 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.
| ::field{name="transforms" type="['string', ...any][]"} | |
| ::field{name="transforms" type="[string, ...any[]][]"} |
🤖 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` at 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.
2 participants
🔗 Linked issue
fixes #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.