Conversation
…ctus baseURL. Updated the Directus URL with a publicly available example.
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:
📝 WalkthroughWalkthroughReworked the playground providers dataset and related test fixtures: playground/app/providers.ts was extensively reordered and had many provider entries added, removed, or reconfigured (notably caisy, cloudimage, directus, sirv, supabase, imagekit, imgix, san ity, storyblok, twicpics, vercel and others) with updated sample srcs, modifiers, sizing, formats and operations. playground/nuxt.config.ts changed directus.baseURL to https://example.directus.app/assets/. Multiple e2e snapshots (directus, caisy, fastly) and test fixtures were updated. Tests were reworked (test/nuxt/providers.test.ts, test/providers.ts, test/unit/provider-coverage.test.ts) and runtime GitHub sizing logic (src/runtime/providers/github.ts) was adjusted. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2193 +/- ##
=======================================
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:
|
Coderabbit noticed that Strapi5 was not retained in
Strapi5 is not a duplicate, was omitted by accident.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
playground/app/providers.ts (1)
852-978:⚠️ Potential issue | 🟡 MinorAlphabetical ordering issue:
storyblokshould come beforestrapi.The PR objective is to alphabetize providers, but
storyblok(s-t-o) is incorrectly placed afterstrapi(s-t-r). Alphabetically, "o" comes before "r", so the correct order should be:
sirv→storyblok→strapi→strapi5→supabaseSuggested reordering
Move the
storyblokprovider block (lines 912-978) to appear before thestrapiprovider block (line 852).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/app/providers.ts` around lines 852 - 978, The providers are out of alphabetical order: move the entire provider object whose name is 'storyblok' so it appears before the provider object named 'strapi' (keeping 'strapi5' after 'strapi'), i.e., reorder the provider blocks so the sequence is ... 'sirv', 'storyblok', 'strapi', 'strapi5', 'supabase'; ensure you cut and paste the full 'storyblok' samples block (including its sample objects and trailing commas) and place it immediately before the 'strapi' block without altering the inner sample data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@playground/app/providers.ts`:
- Around line 852-978: The providers are out of alphabetical order: move the
entire provider object whose name is 'storyblok' so it appears before the
provider object named 'strapi' (keeping 'strapi5' after 'strapi'), i.e., reorder
the provider blocks so the sequence is ... 'sirv', 'storyblok', 'strapi',
'strapi5', 'supabase'; ensure you cut and paste the full 'storyblok' samples
block (including its sample objects and trailing commas) and place it
immediately before the 'strapi' block without altering the inner sample data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd491017-5729-4108-ba16-86ac05fe6772
📒 Files selected for processing (1)
playground/app/providers.ts
Fixed sort of strap/strapi5/storyblok
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playground/app/providers.ts`:
- Around line 338-368: The Directus provider configuration named 'directus'
contains sample entries under samples with UUIDs in the src fields but the
provider's baseURL is still the placeholder
https://example.directus.app/assets/, so image requests will fail; update the
provider configuration for 'directus' to point baseURL to a real public Directus
instance (or document the need to replace it), or replace the sample src values
with fully-qualified URLs that resolve against your actual Directus baseURL so
the samples in samples (src: '39ff62fd-902e-458c-808f-702460c51309', etc.) will
load correctly.
- Around line 266-273: Update the image src string in
playground/app/providers.ts to remove the unnecessary token query parameter from
the GitBook URL; locate the object literal (the entry with keys src, width,
height, densities, fit, quality, format) and replace the src value so it ends at
.../image.png (no ?alt=media&token=... suffix) to use the public URL without
authentication.
🪄 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: d36c4176-cc88-4edb-9f03-e355dc835df7
📒 Files selected for processing (1)
playground/app/providers.ts
kheiner
changed the title
Add comments inline for providers missing tests. Add and comment imports for missing providers.
|
Converting to draft to increase scope in fixing some of the missing/faked tests. |
kheiner
changed the title
Add todo for other missing tests.
Improves the test case for when prismic uses an Unsplash CDN by simulating the baseURL as the unsplashCDN and checking that the returned uri is valid.
The unit test that was removed testted against existing queries being retained.
Currently both tests fail for different reasons as noted in comments.
|
@coderabbitai come back and do another sweep - I've got this ready for review. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/provider-coverage.test.ts (1)
7-18:⚠️ Potential issue | 🟠 MajorSeparate playground existence checks from matrix coverage exclusions.
missingProviderTestsis now being used for two different things: “providers excluded from the sharedimagesmatrix” and “providers still missing runtime coverage”. That dropsstrapi/strapi5from the exact-once playground assertion, and the inline snapshot at Lines 93-95 is counting exclusions instead of the real TODO runtime tests—even thoughtest/nuxt/providers.test.tsstill leaves Bunny and Caisy asit.todo.🧭 Suggested split
-const missingProviderTests = [ +const matrixCoverageExclusions = [ 'ipxStatic', // build-time-only alias for ipx 'strapi', // covered in a unique test 'strapi5', // covered in a unique test ] +const todoProviderTests = ['bunny', 'caisy'] const providers = files.map(f => f.replace(/\.ts$/, '')) -const withProviderTests = providers.filter(p => !missingProviderTests.includes(p)) +const providersExpectedInPlayground = providers.filter(p => p !== 'ipxStatic') +const providersWithMatrixFixtures = providers.filter(p => !matrixCoverageExclusions.includes(p)) -const matrixCases = withProviderTests.flatMap(provider => +const matrixCases = providersWithMatrixFixtures.flatMap(provider => images.map((image, index) => ({ provider, image, index, })), ) @@ it('each provider should exist exactly once in playground', () => { - for (const provider of withProviderTests) { + for (const provider of providersExpectedInPlayground) { const matches = playgroundProviders.filter(p => p.name === provider) expect( matches.length, `expect exactly 1 match for "${provider}", got ${matches.length}`, ).toBe(1) } }) @@ - const allowedKeys = new Set(['args', ...withProviderTests]) + const allowedKeys = new Set(['args', ...providersWithMatrixFixtures]) @@ - for (const provider of withProviderTests) { + for (const provider of providersWithMatrixFixtures) { const count = images.filter(img => Object.prototype.hasOwnProperty.call(img, provider), ).length @@ - it('expect missingProviderTests to shrink over time', () => + it('expect todo provider tests to shrink over time', () => // TODO: remove this once all providers have tests - expect(missingProviderTests.length).toMatchInlineSnapshot(`3`)) + expect(todoProviderTests).toMatchInlineSnapshot(` + [ + "bunny", + "caisy", + ] + `))Also applies to: 35-41, 93-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/provider-coverage.test.ts` around lines 7 - 18, The current missingProviderTests array is being used for two different purposes (playground/matrix exclusions and runtime-coverage TODOs); split it into two distinct arrays (e.g., playgroundExcludedProviders and runtimeMissingProviders), update usages so providers.filter(...) uses playgroundExcludedProviders to build withProviderTests, and update the exact-once playground assertion and the snapshot/TODO counting to reference runtimeMissingProviders instead; ensure unique symbols referenced are missingProviderTests (to remove), withProviderTests (to keep deriving from playgroundExcludedProviders), and any assertions around the inline snapshot or it.todo checks to use runtimeMissingProviders.
🧹 Nitpick comments (1)
test/nuxt/providers.test.ts (1)
319-325: Add a boundary test for the new GitHub size rules.This block only covers the default and
200pxpaths from the shared matrix. The newly documented cap/fallback behavior—at least thewidth: 512 -> s=460case—can still regress without a dedicated assertion here.➕ Suggested test
it('github', () => { const providerOptions = {} for (const image of images) { const [src, modifiers] = image.args const generated = github().getImage(src, { modifiers, ...providerOptions }, getEmptyContext()) expect(generated).toMatchObject(image.github) } }) + + it('github caps oversized avatars', () => { + const generated = github().getImage('unjs', { modifiers: { width: 512 } }, getEmptyContext()) + expect(generated).toMatchObject({ + url: 'https://avatars.githubusercontent.com/unjs?v=4&s=460', + }) + })🤖 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 319 - 325, The current 'github' test iterates shared images but misses a boundary assertion for the new GitHub size rules; add an explicit test case (either inside the existing it('github') loop or as a new it block) that calls github().getImage with a src/args representing width:512 (use the same images/test helpers) and assert the produced URL or modifiers include the expected cap/fallback (e.g. s=460) to prevent regressions; reference github(), getImage, images and the 'github' test name when locating where to add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/3.providers/github.md`:
- Line 22: The inline explanatory comment placed after the NuxtImg component
(<NuxtImg provider="github" src="unjs" width="512" />) uses JavaScript-style //
which is invalid in a Vue template; move that explanatory note out of the
component tag into surrounding prose or convert it to an HTML comment (<!-- ...
-->) so the example remains valid SFC template code and a reader can copy-paste
without errors.
In `@src/runtime/providers/github.ts`:
- Around line 18-20: The current avatar sizing uses the nullish coalescing (??
1) which still allows zero/negative values to pass through: update the sizing
logic in the GitHub provider where you check modifiers?.width /
modifiers?.height and set size (the block with if (modifiers?.width ||
modifiers?.height) and size = Math.min(...)). Fix by coercing modifiers?.width
and modifiers?.height to numbers, treating non-positive values as absent, taking
the numeric max of width and height, then clamping that result to the inclusive
range 1..460 and assigning it to size.
---
Outside diff comments:
In `@test/unit/provider-coverage.test.ts`:
- Around line 7-18: The current missingProviderTests array is being used for two
different purposes (playground/matrix exclusions and runtime-coverage TODOs);
split it into two distinct arrays (e.g., playgroundExcludedProviders and
runtimeMissingProviders), update usages so providers.filter(...) uses
playgroundExcludedProviders to build withProviderTests, and update the
exact-once playground assertion and the snapshot/TODO counting to reference
runtimeMissingProviders instead; ensure unique symbols referenced are
missingProviderTests (to remove), withProviderTests (to keep deriving from
playgroundExcludedProviders), and any assertions around the inline snapshot or
it.todo checks to use runtimeMissingProviders.
---
Nitpick comments:
In `@test/nuxt/providers.test.ts`:
- Around line 319-325: The current 'github' test iterates shared images but
misses a boundary assertion for the new GitHub size rules; add an explicit test
case (either inside the existing it('github') loop or as a new it block) that
calls github().getImage with a src/args representing width:512 (use the same
images/test helpers) and assert the produced URL or modifiers include the
expected cap/fallback (e.g. s=460) to prevent regressions; reference github(),
getImage, images and the 'github' test name when locating where to add the
assertion.
🪄 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: 24970146-6a0f-4083-aad7-15c032d0caf2
📒 Files selected for processing (8)
docs/content/3.providers/github.mdplayground/app/providers.tssrc/runtime/providers/github.tstest/e2e/__snapshots__/caisy.json5test/e2e/__snapshots__/fastly.json5test/nuxt/providers.test.tstest/providers.tstest/unit/provider-coverage.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/e2e/snapshots/caisy.json5
- test/e2e/snapshots/fastly.json5
Suggested by coderabbit nuxt#2193 (comment)
…alculation Issue caught by coderabbit nuxt#2193 (comment)
Suggested by coderabbit nuxt#2193 (comment)
I think code rabbit is really taking issue with my naming and not with the function of the tests. I'm happy to look into making the changes as it's suggesting but for now I'm hoping someone else can offer an opinion. |
…alphebatize-providers # Conflicts: # playground/app/providers.ts # test/nuxt/providers.test.ts # test/providers.ts
danielroe
changed the title
danielroe
changed the title
|
it looks like this PR was only partially merged into main. Can I get clarification on what the plan is forward? I'm fine with being told the remainder of the changes do not align or aren't accepted; My original PR description did not fully explain what I did (my mistake, hopefully this clears it up): The changes to
There were also quite a few integration tests that did not appropriately consider the 'src' and/or were missing from
if I were to do it all over again I might choose to make commits that separate the movement alphabetically into commented blocks and then delete them so it's easier to understand/follow the changes. Let me know if this helps or if I'm off base. If these changes aren't wanted/needed I'd like to delete the branch and close the PR. |
3 participants
The providers are now organized alphabetically. I also updated the Directus example in the playground to reflect a public dataset and public image src.
🔗 Linked issue
resolves #2192
📚 Description
Providers were not sorted in any way, so finding your provider in the list was not intuitive.
Additional: Previously the Directus provider pointed to a localhost:8055/assets url that was unique to each person, ensuring that the 'src' could not be duplicated for testing.