Conversation
fix: Update CONTRIBUTING.md i18n: add `package.metrics.wasm` key
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
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 WebAssembly support: introduces a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
CONTRIBUTING.md (1)
463-466: Query: Are these documentation changes within scope?These changes simplify the i18n commands table by removing several entries and shortening descriptions. Whilst this may improve readability, it seems unrelated to the PR's stated objective of adding a WASM label. Were these changes intentional cleanup work, or were they made accidentally?
If intentional, consider moving documentation-focused changes to a separate PR to maintain clear change history and make reviews more focused.
test/unit/shared/utils/package-analysis.spec.ts (1)
126-128: Add an exports-only WASM test case to lock expected behaviour.The new test covers
main, but not packages that declare onlyexportswith a.wasmentrypoint.🧪 Suggested additional test
it('detects WASM from main field', () => { expect(detectModuleFormat({ main: 'main.wasm' })).toBe('wasm') }) + + it('detects WASM from exports-only entrypoint', () => { + expect( + detectModuleFormat({ + exports: { + '.': './main.wasm', + }, + }), + ).toBe('wasm') + })As per coding guidelines:
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997eac39-0654-4a11-b8cb-a98fb06c3f32
📒 Files selected for processing (6)
CONTRIBUTING.mdapp/components/Package/MetricsBadges.vuei18n/locales/en.jsoni18n/schema.jsonshared/utils/package-analysis.tstest/unit/shared/utils/package-analysis.spec.ts
|
@coderabbitai should I add an e2e test? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the existing e2e tests to give you a concrete recommendation. 🏁 Script executed: Length of output: 29914
Why you probably don't need one right now:
Why you might want one anyway:
If you do add one, it would go in // test/e2e/metrics-badges.spec.ts
import { expect, test } from './test-utils'
test.describe('MetricsBadges – WASM', () => {
test('/package/<wasm-pkg> → shows WASM badge', async ({ page, goto }) => {
await goto('/package/<wasm-pkg>', { waitUntil: 'domcontentloaded' })
// WASM badge should be visible
await expect(page.locator('[data-testid="wasm-badge"]')).toBeVisible()
// ESM / CJS badges should NOT be shown alongside WASM
await expect(page.locator('[data-testid="esm-badge"]')).not.toBeVisible()
})
})(You'd need to add Bottom line: skip it for now unless the team has a policy of e2e-testing new badge types. The unit test is the right place for the detection logic, and a component-level Nuxt test ( 🧠 Learnings used |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nuxt/components/Package/MetricsBadges.spec.ts (1)
18-32: Tighten the dual-format assertion set.Consider also asserting that
WASMis not rendered in this case, so the test verifies both presence and exclusion paths.Suggested tweak
const text = wrapper.text() expect(text).toContain('Types') expect(text).toContain('ESM') expect(text).toContain('CJS') + expect(text).not.toContain('WASM')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d29b9cc-86a7-4471-b243-1e1aaa331054
📒 Files selected for processing (5)
test/nuxt/components/Package/LikeCard.spec.tstest/nuxt/components/Package/MetricsBadges.spec.tstest/nuxt/components/Package/Sidebar.spec.tstest/nuxt/components/Package/Versions.spec.tstest/nuxt/components/Package/WeeklyDownloadStats.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/nuxt/components/Package/MetricsBadges.spec.ts (2)
13-16: Consider adding mock cleanup between tests.The
mockUsePackageAnalysismock is not being reset between tests, which could lead to test pollution if additional tests are added. Consider clearing the mock in theafterEachhook.🧹 Suggested improvement for mock cleanup
- afterEach(() => wrapper?.unmount()) + afterEach(() => { + wrapper?.unmount() + vi.clearAllMocks() + })
18-46: Good test coverage for the new WASM feature.The tests effectively validate both the existing badge rendering (Types, ESM, CJS) and the new WASM badge. The test structure is clear and follows Vitest best practices.
For more comprehensive coverage, you could optionally consider adding tests for:
- Loading states (when
statusis'pending'or'error')- Other module formats (
'esm','cjs','none')- The
typesproperty variations (e.g.,kind: 'external',kind: 'none')However, the current tests adequately cover the primary objective of this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf8fca50-d32a-4911-9258-0abad7558174
📒 Files selected for processing (1)
test/nuxt/components/Package/MetricsBadges.spec.ts
🔗 Linked issue
Closes #2117
📚 Description
Adds a tag if a package is WASM only