fix: validate required registry fields after runtime merge#762
Conversation
|
@tsukiyama-3 is attempting to deploy a commit to the Nuxt Team on Vercel. A member of the Team first needs to authorize it. |
commit: |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change modifies required-field validation for auto-loading registry scripts in the Nuxt scripts module. Instead of checking the raw registry input, the validation now derives an "effective input" from the runtime configuration's public scripts entry (excluding scriptOptions), and falls back to the normalized registry input when unavailable. A new exported helper findMissingRequiredFields implements this logic and unit tests were added to cover its behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Pull the post-merge required-field check out of setup() into a pure helper so it's unit-testable, and cover the runtimeConfig/env merge case from nuxt#761.
![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)
test/unit/setup.test.ts (1)
168-170: ⚡ Quick winAdd test case for
scriptOptionsalongside valid required fields.The current test verifies that
scriptOptionsalone doesn't satisfy requirements, but a more realistic scenario would includescriptOptionsmixed with valid fields. This would confirm the filtering logic correctly ignoresscriptOptionswhile recognizing required fields.✅ Suggested additional test
it('ignores `scriptOptions` carried on the merged entry', () => { expect(findMissingRequiredFields(['id'], {}, { scriptOptions: { bundle: true } })).toEqual(['id']) }) + + it('correctly filters scriptOptions while recognizing valid fields in merged entry', () => { + expect(findMissingRequiredFields(['id'], {}, { id: 'G-123', scriptOptions: { bundle: true } })).toEqual([]) + })🤖 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/unit/setup.test.ts` around lines 168 - 170, Add a new unit test that ensures findMissingRequiredFields correctly ignores scriptOptions when valid required fields are present: call findMissingRequiredFields with required key ['id'] and a merged entry object that contains both id (e.g. '123') and scriptOptions (e.g. { bundle: true }) and assert the result is an empty array; this verifies the function recognizes the valid id while filtering out scriptOptions.
🤖 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 `@test/unit/setup.test.ts`:
- Around line 176-178: The test duplicates an earlier case; remove or revise it:
either delete the redundant it(...) that calls findMissingRequiredFields(['id'],
{ id: 'G-123' }, undefined), or change it to a meaningful fallback scenario (for
example call findMissingRequiredFields(['id'], { id: 'G-123' }, { }) or { id:
undefined } to assert that raw input is used when merged config exists but lacks
the field). Update the test description to reflect the new intent and keep
references to findMissingRequiredFields and the specific inputs in the revised
assertion.
---
Nitpick comments:
In `@test/unit/setup.test.ts`:
- Around line 168-170: Add a new unit test that ensures
findMissingRequiredFields correctly ignores scriptOptions when valid required
fields are present: call findMissingRequiredFields with required key ['id'] and
a merged entry object that contains both id (e.g. '123') and scriptOptions (e.g.
{ bundle: true }) and assert the result is an empty array; this verifies the
function recognizes the valid id while filtering out scriptOptions.
🪄 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: edb849a4-6fea-4a67-a6dc-40a159c43a31
📒 Files selected for processing (2)
packages/script/src/module.tstest/unit/setup.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/script/src/module.ts
harlan-zw
changed the title
|
Thanks! |
2 participants
🔗 Linked issue
#761
📚 Description
Required-field validation during module setup compared the Valibot schema to the normalized
[input]fromscripts.registryonly.IDs and similar fields are often supplied only through
runtimeConfig.public.scriptsand/orNUXT_PUBLIC_SCRIPTS_*, merged intopublic.scriptsearlier in this module (registryWithDefaults+defu). After that merge, the effective config already includesid, but the check still complained.The WARN now inspects each script’s merged object in
runtimeConfig.public.scripts, droppingscriptOptionskeys that aren’t part of script input schema. Behaviour at runtime is unchanged; this only aligns dev warnings with documented env/runtime configuration.