fix: don't rely on syntheticNamedExports in import protection#6758
Merged
schiller-manuel merged 6 commits intomainfrom Feb 25, 2026
Merged
fix: don't rely on syntheticNamedExports in import protection#6758schiller-manuel merged 6 commits intomainfrom
schiller-manuel merged 6 commits intomainfrom
Conversation
this is not supported by rolldown so we need to rewrite mock imports
github-actions
bot
added
documentation
Contributor
📝 WalkthroughWalkthroughCentralized virtual-module resolution APIs and refactored import-protection to emit per-violation mock-edge modules with explicit exports; documentation and tests updated; dev diagnostic artifact removed; small e2e navigation/wait adjustments and .gitignore additions. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit f0eda8f
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/start/framework/react/guide/import-protection.md (1)
34-39:⚠️ Potential issue | 🟡 MinorStale "file checks" wording in the Default Rules bullets (lines 34 and 39).
The PR updates line 41 and line 139 to say "excluded from resolved-target deny checks" (covering both file-pattern and marker checks), but the two bullet points directly above line 41 still read "Excluded from file checks". This leaves contradictory terminology within the same section.
📝 Suggested wording update for lines 34 and 39
**Client environment denials:** - Files matching `**/*.server.*` - The specifier `@tanstack/react-start/server` -- Excluded from file checks: `**/node_modules/**` +- Excluded from resolved-target deny checks: `**/node_modules/**` **Server environment denials:** - Files matching `**/*.client.*` -- Excluded from file checks: `**/node_modules/**` +- Excluded from resolved-target deny checks: `**/node_modules/**`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/start/framework/react/guide/import-protection.md` around lines 34 - 39, Update the two bullets that currently read "Excluded from file checks: `**/node_modules/**`" to use the same terminology as the rest of the section by changing them to "Excluded from resolved-target deny checks: `**/node_modules/**`"; locate and edit the bullets near the "Default Rules" / "Server environment denials" headings so the phrasing matches the updated lines that reference "resolved-target deny checks" (ensure both occurrences that previously said "file checks" are updated).packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md (1)
34-37:⚠️ Potential issue | 🟡 MinorDocumentation says "dev only" but the plugin now runs in all environments.
Line 34 describes the mock-rewrite plugin as
(enforce: 'pre', dev only), but the updatedapplyToEnvironmentinplugin.ts(lines 1621-1632) now enables it for all environments (environmentNames.has(env.name)), not just dev+mock. The comment in the code explicitly states it's needed in build too because Rolldown doesn't supportsyntheticNamedExports`.Proposed fix
-### 3. `tanstack-start-core:import-protection-mock-rewrite` (enforce: `'pre'`, dev only) +### 3. `tanstack-start-core:import-protection-mock-rewrite` (enforce: `'pre'`) -Records expected named exports per importer so that dev mock-edge modules can -provide explicit ESM named exports. Only active in dev + mock mode. +Records expected named exports per importer so that mock-edge modules can +provide explicit ESM named exports. Active whenever import protection is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md` around lines 34 - 37, The docs are out of sync: the plugin `tanstack-start-core:import-protection-mock-rewrite` is described as "dev only" but `applyToEnvironment` in `plugin.ts` now enables it for all environments; update INTERNALS.md to reflect the current behavior (remove "dev only" and note it runs in build too because Rollup lacks syntheticNamedExports), or alternatively revert `applyToEnvironment` in `plugin.ts` to only enable the plugin for dev+mock if you prefer the original behavior; reference `tanstack-start-core:import-protection-mock-rewrite` and the `applyToEnvironment` logic in `plugin.ts` when making the fix.
🤖 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/start/framework/react/guide/import-protection.md`:
- Around line 448-453: Update the documentation so the merge-semantics labels
for client.specifiers and server.specifiers are consistent: either change the
server.specifiers parenthetical from "(replaces defaults)" to "(additive with
defaults)" to match client.specifiers, or if the implementation truly replaces
server defaults, add a short clarifying note next to server.specifiers stating
that the default is [] so "replaces" has no practical effect; reference the
symbols client.specifiers and server.specifiers when making the edit.
In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts`:
- Around line 40-54: Remove the duplicated JSDoc comment above
getResolvedVirtualModuleMatchers(): keep a single copy of the block that
explains resolved vs unresolved virtual ids and delete the repeated duplicate;
update the comment immediately preceding the export function
getResolvedVirtualModuleMatchers() and ensure the function still returns
RESOLVED_VIRTUAL_MODULE_MATCHERS unchanged.
---
Outside diff comments:
In `@docs/start/framework/react/guide/import-protection.md`:
- Around line 34-39: Update the two bullets that currently read "Excluded from
file checks: `**/node_modules/**`" to use the same terminology as the rest of
the section by changing them to "Excluded from resolved-target deny checks:
`**/node_modules/**`"; locate and edit the bullets near the "Default Rules" /
"Server environment denials" headings so the phrasing matches the updated lines
that reference "resolved-target deny checks" (ensure both occurrences that
previously said "file checks" are updated).
In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md`:
- Around line 34-37: The docs are out of sync: the plugin
`tanstack-start-core:import-protection-mock-rewrite` is described as "dev only"
but `applyToEnvironment` in `plugin.ts` now enables it for all environments;
update INTERNALS.md to reflect the current behavior (remove "dev only" and note
it runs in build too because Rollup lacks syntheticNamedExports), or
alternatively revert `applyToEnvironment` in `plugin.ts` to only enable the
plugin for dev+mock if you prefer the original behavior; reference
`tanstack-start-core:import-protection-mock-rewrite` and the
`applyToEnvironment` logic in `plugin.ts` when making the fix.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/start/framework/react/guide/import-protection.mde2e/react-start/import-protection/.gitignoree2e/react-start/import-protection/error-dev-result.jsonpackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/import-protection-plugin/virtualModules.tspackages/start-plugin-core/tests/importProtection/transform.test.tspackages/start-plugin-core/tests/importProtection/virtualModules.test.ts
💤 Files with no reviewable changes (2)
- packages/start-plugin-core/tests/importProtection/transform.test.ts
- e2e/react-start/import-protection/error-dev-result.json
Comment on lines
40
to
54
| /** | ||
| * Convenience list for plugin `load` filters/handlers. | ||
| * | ||
| * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). | ||
| * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). | ||
| */ | ||
| /** | ||
| * Convenience list for plugin `load` filters/handlers. | ||
| * | ||
| * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). | ||
| * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). | ||
| */ | ||
| export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> { | ||
| return RESOLVED_VIRTUAL_MODULE_MATCHERS | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate JSDoc comment block.
The JSDoc comment for getResolvedVirtualModuleMatchers() is duplicated — lines 40-44 and 46-51 contain the exact same text.
Proposed fix
-/**
- * Convenience list for plugin `load` filters/handlers.
- *
- * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`).
- * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`).
- */
/**
* Convenience list for plugin `load` filters/handlers.
*
* Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`).
* `resolveId(source)` sees the *unresolved* id/prefix (without `\0`).
*/
export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> {📝 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
| /** | |
| * Convenience list for plugin `load` filters/handlers. | |
| * | |
| * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). | |
| * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). | |
| */ | |
| /** | |
| * Convenience list for plugin `load` filters/handlers. | |
| * | |
| * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). | |
| * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). | |
| */ | |
| export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> { | |
| return RESOLVED_VIRTUAL_MODULE_MATCHERS | |
| } | |
| /** | |
| * Convenience list for plugin `load` filters/handlers. | |
| * | |
| * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). | |
| * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). | |
| */ | |
| export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> { | |
| return RESOLVED_VIRTUAL_MODULE_MATCHERS | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts`
around lines 40 - 54, Remove the duplicated JSDoc comment above
getResolvedVirtualModuleMatchers(): keep a single copy of the block that
explains resolved vs unresolved virtual ids and delete the repeated duplicate;
update the comment immediately preceding the export function
getResolvedVirtualModuleMatchers() and ensure the function still returns
RESOLVED_VIRTUAL_MODULE_MATCHERS unchanged.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts (1)
40-48: Duplicate JSDoc resolved.The previously flagged duplicate JSDoc block above
getResolvedVirtualModuleMatchershas been removed. Only a single documentation block remains (lines 40–45).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts` around lines 40 - 48, Remove the duplicate JSDoc so only one documentation block documents the function: keep a single JSDoc for getResolvedVirtualModuleMatchers and ensure it describes the returned RESOLVED_VIRTUAL_MODULE_MATCHERS; delete any extra/duplicate comment blocks above or below getResolvedVirtualModuleMatchers to avoid redundancy.
🧹 Nitpick comments (2)
packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts (1)
40-56:getResolvedVirtualModuleMatchersreferencesRESOLVED_VIRTUAL_MODULE_MATCHERSbefore it is declared.
getResolvedVirtualModuleMatchers(a hoistedfunctiondeclaration) refers toRESOLVED_VIRTUAL_MODULE_MATCHERS, aconstthat is initialised two lines later (line 50). While this is safe at runtime (theconstis fully initialised before any external caller can invoke the function), it creates a confusing inversion that could trip up future readers and tools. Reordering eliminates any doubt.♻️ Suggested reorder
-/** - * Convenience list for plugin `load` filters/handlers. - * - * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). - * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). - */ -export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> { - return RESOLVED_VIRTUAL_MODULE_MATCHERS -} - const RESOLVED_VIRTUAL_MODULE_MATCHERS = [ RESOLVED_MOCK_MODULE_ID, RESOLVED_MOCK_BUILD_PREFIX, RESOLVED_MOCK_EDGE_PREFIX, RESOLVED_MOCK_RUNTIME_PREFIX, RESOLVED_MARKER_PREFIX, ] as const + +/** + * Convenience list for plugin `load` filters/handlers. + * + * Vite/Rollup call `load(id)` with the *resolved* virtual id (prefixed by `\0`). + * `resolveId(source)` sees the *unresolved* id/prefix (without `\0`). + */ +export function getResolvedVirtualModuleMatchers(): ReadonlyArray<string> { + return RESOLVED_VIRTUAL_MODULE_MATCHERS +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts` around lines 40 - 56, Move the RESOLVED_VIRTUAL_MODULE_MATCHERS const so it is declared before the getResolvedVirtualModuleMatchers function to avoid referencing a const before its declaration; specifically, place the RESOLVED_VIRTUAL_MODULE_MATCHERS array (which contains RESOLVED_MOCK_MODULE_ID, RESOLVED_MOCK_BUILD_PREFIX, RESOLVED_MOCK_EDGE_PREFIX, RESOLVED_MOCK_RUNTIME_PREFIX, RESOLVED_MARKER_PREFIX) above the getResolvedVirtualModuleMatchers() function so the function returns an already-declared constant.e2e/react-start/import-protection/tests/violations.setup.ts (1)
86-94:routeReadyTestIdstype could be narrowed for future-safety.
Record<string, string>meansrouteReadyTestIds[route]is typed asstring(notstring | undefined) withoutnoUncheckedIndexedAccess. If a new route is added toroutesbut omitted fromrouteReadyTestIds, TypeScript won't flag it and theif (testId)guard on line 120 will be bypassed silently at runtime (the undefined value will be falsy but the type says otherwise).A tighter type locks the allowed keys to those in
routes:♻️ Suggested narrower typing
-const routeReadyTestIds: Record<string, string> = { +type AppRoute = (typeof routes)[number] +const routeReadyTestIds: Partial<Record<AppRoute, string>> = { '/': 'heading', '/leaky-server-import': 'leaky-heading', '/client-only-violations': 'client-only-heading', '/client-only-jsx': 'client-only-jsx-heading', '/beforeload-leak': 'beforeload-leak-heading', '/component-server-leak': 'component-leak-heading', '/barrel-false-positive': 'barrel-heading', }This keeps the
if (testId)guard meaningful and ensures any new entry inroutesthat lacks atestIdmapping will be caught by TypeScript. As per coding guidelines,**/*.tsfiles should use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/tests/violations.setup.ts` around lines 86 - 94, Narrow the type of routeReadyTestIds so its keys are the exact routes defined in routes: make routes a readonly/as const array (so its elements form a string literal union) and type routeReadyTestIds as Record<typeof routes[number], string>; update the routeReadyTestIds declaration to use that type (instead of Record<string,string>) so TypeScript will error if a route in routes is missing from routeReadyTestIds and the runtime guard (if (testId)) remains meaningful.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts`:
- Around line 40-48: Remove the duplicate JSDoc so only one documentation block
documents the function: keep a single JSDoc for getResolvedVirtualModuleMatchers
and ensure it describes the returned RESOLVED_VIRTUAL_MODULE_MATCHERS; delete
any extra/duplicate comment blocks above or below
getResolvedVirtualModuleMatchers to avoid redundancy.
---
Nitpick comments:
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 86-94: Narrow the type of routeReadyTestIds so its keys are the
exact routes defined in routes: make routes a readonly/as const array (so its
elements form a string literal union) and type routeReadyTestIds as
Record<typeof routes[number], string>; update the routeReadyTestIds declaration
to use that type (instead of Record<string,string>) so TypeScript will error if
a route in routes is missing from routeReadyTestIds and the runtime guard (if
(testId)) remains meaningful.
In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts`:
- Around line 40-56: Move the RESOLVED_VIRTUAL_MODULE_MATCHERS const so it is
declared before the getResolvedVirtualModuleMatchers function to avoid
referencing a const before its declaration; specifically, place the
RESOLVED_VIRTUAL_MODULE_MATCHERS array (which contains RESOLVED_MOCK_MODULE_ID,
RESOLVED_MOCK_BUILD_PREFIX, RESOLVED_MOCK_EDGE_PREFIX,
RESOLVED_MOCK_RUNTIME_PREFIX, RESOLVED_MARKER_PREFIX) above the
getResolvedVirtualModuleMatchers() function so the function returns an
already-declared constant.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/start/framework/react/guide/import-protection.mde2e/react-start/import-protection/tests/error-mode.setup.tse2e/react-start/import-protection/tests/violations.setup.tspackages/start-plugin-core/src/import-protection-plugin/virtualModules.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/start/framework/react/guide/import-protection.md
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.
Summary by CodeRabbit
Documentation
Tests
E2E / Stability
Chores