Bumped vite and vitest in shade and admin-x-design-system#27867
Merged
Conversation
The two admin-lane stragglers move from vite@5.4.21 + vitest@1.6.1 to vite@7.3.2 + vitest@4.1.2, aligning with the rest of the admin lane (admin, admin-x-framework, admin-x-settings, activitypub, posts, stats). Two breaking changes had to be addressed: 1. vite-plugin-svgr v3 -> v4 changed the SVG import API. v3 exposed both `default` (URL) and named `ReactComponent` on every .svg import; v4 splits them - ./foo.svg is the URL, ./foo.svg?react is the React component (as default export). v3's dual-export came from its `transform` hook merging with Vite's URL handling; v4's `load` hook bypasses Vite's asset pipeline entirely, so the dual export cannot be preserved by configuration. Migrated 12 explicit imports and 2 import.meta.glob patterns across both packages, and split the *.svg ambient module declarations into *.svg (URL) and *.svg?react (component). 2. vitest 1 -> 4: ReturnType<typeof vi.fn> now resolves to Mock<Procedure | Constructable>, which TS won't dispatch as either without a function-shape generic. One test file affected (shade's filters.test.tsx); typed prop callbacks updated to use the function-shape generic ReturnType<typeof vi.fn<(value: string) => void>>. Verification: - shade: 220/220 tests, build, lint - admin-x-design-system: 23/23 tests, build - Downstream regression check all green: admin-x-framework 350/350, admin-x-settings 156/156, activitypub 93/93, posts 173/173, stats 234/234
Contributor
WalkthroughThis PR migrates SVG imports across the admin-x-design-system and shade packages from using named 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/shade/src/components/ui/icon.ts (1)
25-28: 💤 Low valueRefine the glob generic type for better type safety.
The generic type
{default: React.FC<IconProps>}is slightly inaccurate. The actual SVG modules imported via?reactexport components typed asReact.FC<React.SVGProps<SVGSVGElement>>(per the updated typings.d.ts), notReact.FC<IconProps>. TheIconPropsinterface includes thesizevariant prop, which the raw SVG components don't understand—your wrapper component (lines 34-41) correctly handles this by extractingsizeand passing only the remaining props to the underlying SVG.While this works at runtime and compiles successfully due to TypeScript's structural typing, using the more accurate type improves type safety and clarity.
♻️ Suggested type refinement
-const iconModules = import.meta.glob<{default: React.FC<IconProps>}>( +const iconModules = import.meta.glob<{default: React.FC<React.SVGProps<SVGSVGElement>>}>( '../../assets/icons/*.svg', {eager: true, query: '?react'} );🤖 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/shade/src/components/ui/icon.ts` around lines 25 - 28, The glob's generic currently types SVG modules as {default: React.FC<IconProps>} which is inaccurate; change the import.meta.glob generic for iconModules to {default: React.FC<React.SVGProps<SVGSVGElement>>} so the raw SVG components are typed correctly, and keep the wrapper component that extracts size from IconProps and forwards the remaining props to the underlying SVG (the wrapper that receives size and spreads ...props) unchanged.
🤖 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 `@apps/admin-x-design-system/src/global/icon.tsx`:
- Line 31: The destructuring on line with "const {default: SvgComponent} =
icons[`../assets/icons/${name}.svg`]" can throw if icons[...] is undefined;
update the Icon lookup (in this module/file where SvgComponent and icons are
referenced) to first retrieve the lookup result into a variable (e.g., const
IconModule = icons[`../assets/icons/${name}.svg`]) and guard it (if falsy return
null) before destructuring/using SvgComponent so the intended fallback is
reached safely.
---
Nitpick comments:
In `@apps/shade/src/components/ui/icon.ts`:
- Around line 25-28: The glob's generic currently types SVG modules as {default:
React.FC<IconProps>} which is inaccurate; change the import.meta.glob generic
for iconModules to {default: React.FC<React.SVGProps<SVGSVGElement>>} so the raw
SVG components are typed correctly, and keep the wrapper component that extracts
size from IconProps and forwards the remaining props to the underlying SVG (the
wrapper that receives size and spreads ...props) unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce61894c-9e05-4ceb-8926-f07582bb9f09
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/admin-x-design-system/package.jsonapps/admin-x-design-system/src/global/avatar.tsxapps/admin-x-design-system/src/global/form/color-picker.tsxapps/admin-x-design-system/src/global/icon.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-design-system/src/typings.d.tsapps/shade/package.jsonapps/shade/src/components.tsapps/shade/src/components/ui/icon.tsapps/shade/src/typings.d.tsapps/shade/test/unit/components/patterns/filters.test.tsx
| */ | ||
| const Icon: React.FC<IconProps> = ({name, size = 'md', colorClass = '', className = ''}) => { | ||
| const {ReactComponent: SvgComponent} = icons[`../assets/icons/${name}.svg`]; | ||
| const {default: SvgComponent} = icons[`../assets/icons/${name}.svg`]; |
Contributor
There was a problem hiding this comment.
Guard icon lookup before destructuring
Line 31 can throw when name doesn’t map to a known icon key (icons[...] is undefined), so the intended return null fallback is never reached.
Proposed fix
- const {default: SvgComponent} = icons[`../assets/icons/${name}.svg`];
+ const iconModule = icons[`../assets/icons/${name}.svg`];
+ const SvgComponent = iconModule?.default;📝 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.
Suggested change
| const {default: SvgComponent} = icons[`../assets/icons/${name}.svg`]; | |
| const iconModule = icons[`../assets/icons/${name}.svg`]; | |
| const SvgComponent = iconModule?.default; |
🤖 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/admin-x-design-system/src/global/icon.tsx` at line 31, The destructuring
on line with "const {default: SvgComponent} =
icons[`../assets/icons/${name}.svg`]" can throw if icons[...] is undefined;
update the Icon lookup (in this module/file where SvgComponent and icons are
referenced) to first retrieve the lookup result into a variable (e.g., const
IconModule = icons[`../assets/icons/${name}.svg`]) and guard it (if falsy return
null) before destructuring/using SvgComponent so the intended fallback is
reached safely.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27867 +/- ##
==========================================
- Coverage 73.76% 73.76% -0.01%
==========================================
Files 1515 1515
Lines 127517 127517
Branches 15257 15257
==========================================
- Hits 94069 94067 -2
- Misses 32522 32523 +1
- Partials 926 927 +1
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:
|
This was referenced May 13, 2026
9larsons
added a commit
that referenced
this pull request
May 13, 2026
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
added a commit
that referenced
this pull request
May 13, 2026
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)
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 two admin-lane stragglers —
apps/shadeandapps/admin-x-design-system— move fromvite@5.4.21 + vitest@1.6.1tovite@7.3.2 + vitest@4.1.2, aligning with the rest of the admin lane (admin,admin-x-framework,admin-x-settings,activitypub,posts,stats).Continues the audit-flagged dependency cleanup from PR #27731 (framework + settings + activitypub) and commit
ccf3d7368f(posts + stats). PR #27866 was the prerequisite lint-config cleanup that lets this PR land without any band-aideslint-disablecomments.Substantive changes (beyond the bumps)
Two breaking changes hit:
1.
vite-plugin-svgrv3 → v4 — SVG imports require?reactsuffixv3 exposed both
default(URL) and namedReactComponenton every.svgimport; v4 splits them —./foo.svgis the URL,./foo.svg?reactis the React component (as default export). v3's dual-export came from itstransformhook merging with Vite's URL handling; v4'sloadhook bypasses Vite's asset pipeline entirely, so the dual export cannot be preserved by configuration. Migrated:shade/src/components.ts,admin-x-design-system/src/index.ts, plus icon imports inadmin-x-design-system/src/global/avatar.tsxandcolor-picker.tsx)import.meta.globpatterns (shade/src/components/ui/icon.tsandadmin-x-design-system/src/global/icon.tsx) — added{eager: true, query: '?react'}, callers switched frommodule.ReactComponenttomodule.default*.svgambient module declarations in bothtypings.d.tsfiles — split into*.svg(URL default) and*.svg?react(component default), usingimport typeinstead of legacyimport React = require('react')so no@typescript-eslint/no-require-importsdisable is needed2. vitest 1 → 4 —
ReturnType<typeof vi.fn>not directly callableIn vitest 4 it resolves to
Mock<Procedure | Constructable>, which TS won't dispatch as either without a function-shape generic. One file affected:shade/test/unit/components/patterns/filters.test.tsx—DateFiltersPropscallback types moved toReturnType<typeof vi.fn<(value: string) => void>>. This is the same idiom already used inapps/stats/test/unit/utils/content-helpers.test.ts.No other vitest 1→4 patterns from PR #27731 (e.g.
global.fetch,MockInstance, constructable mock implementations) appear in these packages.Test plan
pnpm --filter @tryghost/shade test— 220/220 across 22 filespnpm --filter @tryghost/shade build— exit 0pnpm --filter @tryghost/shade lint— exit 0 (only the pre-existingreact-hooks/exhaustive-depswarning insrc/components/patterns/filters.tsxremains, unchanged by this PR)pnpm --filter @tryghost/admin-x-design-system test— 23/23 across 3 filespnpm --filter @tryghost/admin-x-design-system build— exit 0admin-x-framework350/350,admin-x-settings156/156,activitypub93/93,posts173/173,stats234/234