Bumped vite and vitest in signup-form and comments-ui#27873
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
✅ Files skipped from review due to trivial changes (11)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThis PR migrates SVG icon imports across multiple apps from SVGR's Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
apps/comments-ui/package.json (1)
25-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Playwright reporter env var in e2e script.
test:e2eshould includePLAYWRIGHT_REPORTER=listfor AI-agent parsing consistency.Proposed fix
- "test:e2e": "NODE_OPTIONS='--experimental-specifier-resolution=node --no-warnings' VITE_TEST=true playwright test", + "test:e2e": "NODE_OPTIONS='--experimental-specifier-resolution=node --no-warnings' VITE_TEST=true PLAYWRIGHT_REPORTER=list playwright test",As per coding guidelines, "Set PLAYWRIGHT_REPORTER=list environment variable when running Playwright e2e tests as an AI agent for better parsing".
🤖 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 `@apps/comments-ui/package.json` at line 25, Update the npm script named "test:e2e" to export the PLAYWRIGHT_REPORTER=list environment variable when invoking Playwright; specifically modify the "test:e2e" script entry so it includes PLAYWRIGHT_REPORTER=list alongside NODE_OPTIONS and VITE_TEST so Playwright runs with the list reporter for AI-agent parsing.
🤖 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.
Outside diff comments:
In `@apps/comments-ui/package.json`:
- Line 25: Update the npm script named "test:e2e" to export the
PLAYWRIGHT_REPORTER=list environment variable when invoking Playwright;
specifically modify the "test:e2e" script entry so it includes
PLAYWRIGHT_REPORTER=list alongside NODE_OPTIONS and VITE_TEST so Playwright runs
with the list reporter for AI-agent parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d8a5137-470a-45e4-b8ac-bb84587edeea
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
apps/comments-ui/package.jsonapps/comments-ui/src/components/content/avatar.tsxapps/comments-ui/src/components/content/buttons/like-button.tsxapps/comments-ui/src/components/content/buttons/like-count.tsxapps/comments-ui/src/components/content/buttons/more-button.tsxapps/comments-ui/src/components/content/buttons/reply-button.tsxapps/comments-ui/src/components/content/forms/form.tsxapps/comments-ui/src/components/content/forms/sorting-form.tsxapps/comments-ui/src/components/content/loading.tsxapps/comments-ui/src/components/popups/close-button.tsxapps/comments-ui/src/components/popups/delete-popup.tsxapps/comments-ui/src/components/popups/report-popup.tsxapps/comments-ui/src/setup-tests.tsapps/comments-ui/src/typings.d.tsapps/comments-ui/test/unit/utils/api.test.tsapps/signup-form/package.jsonapps/signup-form/src/components/pages/form-view.tsxapps/signup-form/src/components/pages/success-view.tsxapps/signup-form/src/typings.d.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27873 +/- ##
=======================================
Coverage 73.74% 73.74%
=======================================
Files 1519 1519
Lines 127726 127726
Branches 15291 15291
=======================================
Hits 94186 94186
+ Misses 32613 32589 -24
- Partials 927 951 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9larsons
force-pushed
the
security/vite-bump-step4
branch
from
60276f5 to
5eb6018
Compare
The final two public/UMD apps move from vite@5.4.21 + vitest@1.6.1 to vite@7.3.2 + vitest@4.1.2, finishing the unification across apps/* that started with PR #27731 and continued through #27867 and #27869. Every workspace under apps/ now runs the same vite + vitest pair. @vitest/coverage-v8 in comments-ui jumped from 0.34.6 to 4.1.2 - it had been pinned to a version compatible with vitest 0.x and was years out of date. Substantive changes beyond the bumps: 1. vite-plugin-svgr v3 -> v4 changed the SVG import API (./foo.svg now gives the URL, ./foo.svg?react gives the React component as default export). Migrated 16 imports across: - 14 in comments-ui (12 files) - 2 in signup-form (2 files) Both typings.d.ts files were split into *.svg (URL default) and *.svg?react (component default), using `import type` instead of the legacy `import React = require('react')` pattern. 2. vitest 1 -> 4 had two test-side issues in comments-ui: a. setup-tests.ts: `global.ResizeObserver = vi.fn() .mockImplementation(() => ({...}))` - vitest 4 enforces that mocks called with `new` have constructable implementations, and arrow functions are not constructable. Switched to function expression form. b. test/unit/utils/api.test.ts: three tests each called vi.spyOn(window, 'fetch') with no teardown. The spy persisted across tests, accumulating call counts. Added an afterEach with vi.restoreAllMocks(). 3. vite 5 -> 7 build performance fix in all four public UMD apps (signup-form, comments-ui, plus portal and sodo-search already on vite 7 from #27869). Each app's vite.config used `commonjsOptions.dynamicRequireTargets: SUPPORTED_LOCALES.map( locale => '../../ghost/i18n/locales/' + locale + '/X.json')`, producing an array of ~60 explicit paths. Under vite 7's bundled @rollup/plugin-commonjs, each entry in that array triggers a full directory crawl from `dynamicRequireRoot` (= repo root, including node_modules) at the start of every build. The result was a ~58s delay before rollup's buildStart hook fired, which under `vite build && vite preview` blew playwright's webServer timeout (10s for signup-form, 20s for comments-ui). Replaced the N-entries-per-locale array with a single glob - `['../../ghost/i18n/locales/*/X.json']` - which the plugin resolves with one crawl. Same bundle output (verified byte-for-byte for portal/sodo-search/comments-ui; signup-form bundle size unchanged at 301817 bytes). Build time drops from ~60s to ~2s. Also fixed for portal and sodo-search in this PR even though they're not on the failing branch - they were already shipping the slow pattern on main via #27869 and the savings apply. Patch version bumps on both new packages (CI version-bump check requires this whenever source files change in a monitored app). Verification: - comments-ui: 207/207 unit tests, build (2.7s, was 60s), lint - signup-form: build (2.0s, was 60s), lint - portal: build (3.6s, was 60s+) - bundle byte-identical - sodo-search: build (4.7s, was 60s+) - bundle byte-identical - Local repro: dev:test webServer ready in 4s (was timing out)
9larsons
force-pushed
the
security/vite-bump-step4
branch
from
5eb6018 to
8b6b8ed
Compare
EvanHahn
pushed a commit
that referenced
this pull request
May 13, 2026
no ref Final bump to unify the vite/vitest tooling versions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The final two public/UMD apps move from
vite@5.4.21 + vitest@1.6.1tovite@7.3.2 + vitest@4.1.2, finishing the apps/* unification that started with PR #27731 and continued through #27867 (shade + admin-x-design-system) and #27869 (portal + sodo-search + announcement-bar). Every workspace underapps/now runs the same vite + vitest pair.@vitest/coverage-v8incomments-uijumped from0.34.6→4.1.2— it had been pinned to a vitest 0.x compatible version and was years out of date.Substantive changes beyond the bumps
1. SVG migration (vite-plugin-svgr v3 → v4)
v3 exposed both
default(URL) and namedReactComponenton every.svgimport; v4 splits them —./foo.svgis the URL,./foo.svg?reactis the React component (as default export). Migrated 16 imports:comments-ui(12 files)signup-form(2 files)Both
typings.d.tsfiles split into*.svg(URL default) and*.svg?react(component default), usingimport typeinstead of the legacyimport React = require('react')pattern.2. vitest 1 → 4 test-side fixes (
comments-ui)a. Constructable mock in
setup-tests.ts—global.ResizeObserver = vi.fn().mockImplementation(() => ({...}))used an arrow function. Vitest 4 enforces that mocks called withnewhave constructable implementations. Switched to function expression form.b. Persistent spy in
test/unit/utils/api.test.ts— three tests each calledvi.spyOn(window, 'fetch')with no teardown. Under vitest 4 the spy persists across tests, so the second and third tests saw accumulated call counts. Added anafterEachthat callsvi.restoreAllMocks().3. vite 7 build performance —
commonjsOptions.dynamicRequireTargetsThe first CI run on this branch exposed a vite 7 regression that also affected
portalandsodo-searchfrom PR #27869 (their tests didn't surface it because they have no playwrightwebServer):Each app's vite config had:
That produces ~60 explicit paths (one per locale). Under vite 7's bundled
@rollup/plugin-commonjs, every entry in that array triggers a full directory crawl fromdynamicRequireRoot(= repo root, includingnode_modules) at the start of the build. The result: ~58 seconds of file-walker work before rollup'sbuildStarthook even fires. The bundle itself takes <1s after that.For
signup-formandcomments-ui, the playwrightwebServerrunspnpm dev:test=vite build && vite preview --port XXXX, then waits forhttp://localhost:XXXX/<bundle>.min.js. The build's 58s startup meant the preview server didn't bind before the 10s/20swebServer.timeoutfired.Fix: replace the N-entries-per-locale array with a single glob:
The plugin resolves this with one directory crawl. Bundle output is byte-identical (verified for
portal,sodo-search,comments-ui;signup-formsize unchanged at 301,817 bytes). Build time drops from ~60s to ~2s.Applied the same fix to
portalandsodo-searcheven though their PR is already merged — they were shipping the slow pattern on main and the savings apply.Test plan
pnpm --filter @tryghost/comments-ui test:unit— 207/207 across 12 filespnpm --filter @tryghost/comments-ui build— 2.7s (was 60s); UMD wrapper intact (~860KB)pnpm --filter @tryghost/comments-ui lint— 0 errorspnpm --filter @tryghost/signup-form build— 2.0s (was 60s); UMD wrapper intact (~302KB)pnpm --filter @tryghost/signup-form lint— cleanpnpm --filter @tryghost/portal build— 3.6s (was 60s+); bundle byte-identicalpnpm --filter @tryghost/sodo-search build— 4.7s (was 60s+); bundle byte-identicaldev:testwebServer ready in 4s (was timing out at 10s/20s)Followup
Once this lands, every workspace under
apps/*is on the same five-package vite/vitest pin. The natural follow-up is lifting those into a pnpm catalog so future bumps are a one-line edit — seedocs/pnpm-catalog-support.mdfor the CI publish-step prerequisite.