fix: alias and namespace import support for import protection#6784
Open
schiller-manuel wants to merge 7 commits intomainfrom
Open
fix: alias and namespace import support for import protection#6784schiller-manuel wants to merge 7 commits intomainfrom
schiller-manuel wants to merge 7 commits intomainfrom
Conversation
Contributor
|
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 a new e2e project with Playwright tests for custom import-protection config and performs a large refactor of the import-protection plugin: AST-driven analysis, extensionless resolution support, self-denial mock generation, pluggable import-locator, new utilities, types, tests, and supporting e2e/test harness updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev/Build Pipeline
participant AST as AST Parser (parseImportProtectionAst)
participant Resolver as Path Resolver (incl. extensionless)
participant Cache as Cache/State
participant MockGen as Mock Generator
Dev->>AST: parse source
AST->>Dev: named exports & import info
Dev->>Resolver: resolve candidates (TS paths / extensionless)
Resolver->>Cache: check/update resolution cache
Cache-->>Resolver: cached resolution
Resolver->>Dev: resolved paths
Dev->>Dev: match resolved paths vs deny rules
alt File-based violation
Dev->>MockGen: generate self-contained mock module (exports)
MockGen->>Dev: mock module code
Dev->>Cache: record deferred or emitted violation
else Specifier/marker violation
Dev->>Dev: emit or defer based on behavior (mock/error)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Comment |
|
View your CI Pipeline Execution ↗ for commit 08014f0
☁️ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/react-start/import-protection/vite.config.ts (1)
4-16:⚠️ Potential issue | 🟡 MinorHarden
tsconfigPathsproject resolution against cwd differences.Using a relative
projectspath breaks alias resolution when the dev server is launched from a different working directory. Use an absolute path viapath.resolve(import.meta.dirname, ...)to ensure consistent resolution regardless of cwd.Proposed diff
plugins: [ - tsconfigPaths({ projects: ['./tsconfig.json'] }), + tsconfigPaths({ + projects: [path.resolve(import.meta.dirname, './tsconfig.json')], + }), tanstackStart({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/vite.config.ts` around lines 4 - 16, The tsconfigPaths plugin uses a relative projects path which can break when the server is started from a different CWD; update the plugin invocation in vite.config.ts to resolve the tsconfig.json to an absolute path by importing/using path.resolve with import.meta.url (or import.meta.dirname) and pass that absolute path to tsconfigPaths({ projects: [resolvedPath] }) so alias resolution is stable regardless of working directory.
🧹 Nitpick comments (7)
e2e/react-start/import-protection-custom-config/package.json (1)
4-4: ConsidersideEffects: truefor this e2e application.
"sideEffects": falseis typically for libraries. For an application with routes and components, this could cause unexpected tree-shaking behavior. However, since this is an e2e test project and Vite handles module resolution differently, the practical impact may be minimal.🔧 Suggested fix
- "sideEffects": false, + "sideEffects": true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/package.json` at line 4, The package.json currently sets "sideEffects": false which is intended for libraries and can cause unexpected tree-shaking for an application; update the package.json to either set "sideEffects": true or remove the "sideEffects" field so the e2e app's routes/components are not inadvertently eliminated—locate the "sideEffects" entry in package.json and change its value to true (or delete the key) and run the e2e build to verify no runtime component/route stripping occurs.e2e/react-start/import-protection-custom-config/vite.config.ts (1)
4-7: Consider validating the behavior value.The type assertion
as 'mock' | 'error'doesn't validate at runtime. If an invalid value is passed, it may cause unexpected behavior.🔧 Optional: Add runtime validation
-const behavior = (process.env.BEHAVIOR ?? 'mock') as 'mock' | 'error' +const behaviorEnv = process.env.BEHAVIOR ?? 'mock' +if (behaviorEnv !== 'mock' && behaviorEnv !== 'error') { + throw new Error(`Invalid BEHAVIOR: ${behaviorEnv}. Expected 'mock' or 'error'.`) +} +const behavior = behaviorEnvThat said, since this is test infrastructure with controlled env var usage from
package.jsonscripts, the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/vite.config.ts` around lines 4 - 7, The code uses a type assertion for the environment variable into 'mock' | 'error' but does no runtime validation; update the logic that sets the behavior variable (the behavior constant in vite.config.ts) to validate process.env.BEHAVIOR against the allowed values ('mock' and 'error') at runtime and fall back to 'mock' (or throw) on invalid input—i.e., read process.env.BEHAVIOR, check if it === 'mock' or === 'error', and assign that value or the default 'mock' (or raise a clear error) so you don’t rely solely on the type assertion.e2e/react-start/import-protection-custom-config/src/routes/__root.tsx (1)
1-8: Sort imports alphabetically to satisfy ESLint.ESLint flags that
HeadContentshould come beforeLinkalphabetically.♻️ Proposed fix
import { createRootRoute, + HeadContent, Link, - HeadContent, linkOptions, Outlet, Scripts, } from '@tanstack/react-router'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/src/routes/__root.tsx` around lines 1 - 8, The import specifiers from '@tanstack/react-router' are not alphabetized (HeadContent should come before Link); reorder the named imports in the import statement so they are sorted alphabetically (e.g., createRootRoute, HeadContent, Link, linkOptions, Outlet, Scripts) to satisfy ESLint and keep the existing import path and usage of createRootRoute/HeadContent/Link/etc. unchanged.e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts (1)
9-52: Consider extracting shared utilities.The
waitForHttpOkandkillChildfunctions are duplicated between this file andviolations.setup.ts. If the test suite grows, consider extracting these to a shared utility file to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts` around lines 9 - 52, Both waitForHttpOk and killChild are duplicated; extract them into a shared test utility module and import it from both setup files. Create a new utility file exporting waitForHttpOk and killChild (preserve their signatures and behavior, including use of AbortSignal.timeout and spawn ReturnType), update this file to export types if needed, then replace the duplicated implementations in error-mode.setup.ts and violations.setup.ts with imports from the new module (update any relative import paths). Run tests to ensure no behavior change.e2e/react-start/import-protection-custom-config/tests/custom-config.spec.ts (1)
112-113: Redundant dynamic imports offsandpathmodules.These modules are already available -
pathis imported at the top of the file (line 1). Consider reusing the existing import and importingfsat the module level for consistency.Suggested fix
At the top of the file, add:
import path from 'node:path' +import fs from 'node:fs' import { expect } from '@playwright/test'Then in the test:
test('build: client JS bundle does not contain real backend secret', async () => { - const fs = await import('node:fs') - const path = await import('node:path') const clientDir = path.resolve(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/custom-config.spec.ts` around lines 112 - 113, Remove the redundant dynamic imports of node:fs and node:path in the test body and instead import fs at module level and reuse the existing top-level path import; specifically, delete the two lines "const fs = await import('node:fs')" and "const path = await import('node:path')" in custom-config.spec.ts and add a top-level import for fs (so tests referencing fs and path use the module-level identifiers) to keep imports consistent and avoid dynamic requires.packages/start-plugin-core/tests/importProtection/utils.test.ts (1)
141-203: Add a boundary test forsrcDirectoryprefix collisions.Please add a case ensuring
/app/src2/...is not treated as inside/app/src.🧪 Suggested test
describe('shouldDeferResolvedViolation', () => { @@ test('does not defer pre-transform or non-src alias', () => { @@ }) + + test('does not treat sibling prefix path as inside srcDirectory', () => { + expect( + shouldDeferResolvedViolation({ + isDevMock: true, + isBuild: false, + source: 'src2/x', + resolvedPath: '/app/src2/x.ts', + srcDirectory: '/app/src', + isPreTransformResolve: false, + }), + ).toBe(false) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/importProtection/utils.test.ts` around lines 141 - 203, Add a boundary test inside the existing describe('shouldDeferResolvedViolation') block that verifies paths under '/app/src2' are not treated as inside '/app/src': call shouldDeferResolvedViolation with isDevMock: true, isBuild: false, source: 'src/x', resolvedPath: '/app/src2/x.ts', srcDirectory: '/app/src', isPreTransformResolve: false and assert the result is false; this ensures the prefix '/app/src' match is not satisfied by '/app/src2'.packages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.ts (1)
175-201: Add regression tests for destructured and string-keyed named exports.The new
collectNamedExportscoverage is good, but these two cases are still uncovered and are high-value for this PR’s export-surface logic.🧪 Suggested tests
describe('collectNamedExports', () => { @@ test('ignores default and type-only exports', () => { @@ expect(collectNamedExports(code)).toEqual([]) }) + + test('collects destructured variable declaration exports', () => { + const code = `export const { alpha: renamed, beta } = obj` + expect(collectNamedExports(code)).toEqual(['beta', 'renamed']) + }) + + test('collects string-literal exported names', () => { + const code = [`const local = 1`, `export { local as "foo-bar" }`].join('\n') + expect(collectNamedExports(code)).toEqual(['foo-bar']) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.ts` around lines 175 - 201, Add two regression tests to the collectNamedExports suite: one that verifies destructured exports are collected (e.g. a snippet using "export const { a, b: renamed } = obj" and asserting collectNamedExports returns ['a','renamed']) and one that verifies string-keyed export specifiers are handled (e.g. a snippet with "const x = 1; export { x as 'str' }" and asserting collectNamedExports returns ['str']). Place these tests alongside the existing cases in the same describe('collectNamedExports') block so they exercise the collectNamedExports function handling of destructured and string-keyed named exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/import-protection-custom-config/tests/violations.utils.ts`:
- Around line 59-67: The current parsing uses non-null assertions like
importerLine.split('Importer')[1]! which can throw if the delimiter is missing;
update the parsing for importer/specifier/resolved to safely handle missing
delimiters by checking for the delimiter or split result length before indexing
(e.g. inspect importerLine/includes('Importer') or const parts =
importerLine.split('Importer'); const importer = parts.length > 1 ?
parts[1].trim() : ''), apply the same pattern for specLine/specifier and
resolvedLine/resolved (and preserve the existing replace for specifier when
present) so the code never assumes split()[1] exists.
In `@e2e/react-start/import-protection/package.json`:
- Around line 21-22: Update the dependency declaration for vite-tsconfig-paths
in package.json from "vite-tsconfig-paths": "^5.1.4" to a Vite 7–compatible
v6.1.x release (e.g. "^6.1.0"); then reinstall/update lockfile (npm install /
yarn install) so the lockfile reflects the new version and run the
import-protection e2e tests; locate the dependency entry by the
"vite-tsconfig-paths" key in package.json to make the change.
In `@e2e/react-start/import-protection/tests/import-protection.spec.ts`:
- Around line 50-57: The empty object destructuring in test.beforeEach causes
lint errors; update the parameter list for test.beforeEach to avoid an unused
destructured object — e.g. replace "({}, testInfo)" with "(_, testInfo)"
(underscore for the unused first fixture) or otherwise accept only the testInfo
position permitted by Playwright; update the test.beforeEach callback signature
so testInfo is still used while the unused first fixture is named "_" to satisfy
linters.
In
`@packages/start-plugin-core/src/import-protection-plugin/extensionlessAbsoluteIdResolver.ts`:
- Around line 91-100: The HMR deps built in buildDepsForKey only add
dir:${dirname(key)}, which misses tracking the directory named by an
extensionless absolute ID (e.g. /src/foo.server) and allows stale cache entries;
modify buildDepsForKey to also add deps.add(`dir:${key}`) when isAbsolute(key)
so both the parent directory and the directory represented by the extensionless
absolute ID are indexed (refer to buildDepsForKey, the existing deps Set,
isAbsolute(key) and dirname(key)).
In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md`:
- Around line 175-177: The fenced code block containing the marker string
"\0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>" is missing a
language identifier (triggering markdownlint MD040); update the opening fence to
include a language (for example use "text") so the block reads like a fenced
code block with a language identifier and re-run linting to confirm the MD040
warning is resolved.
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 13-29: The utils import list violates sort-imports and also pulls
in an unused type (DeferredBuildViolation); reorder the named imports from
'./utils' into alphabetical order (e.g., buildResolutionCandidates,
buildSourceCandidates, canonicalizeResolvedId, clearNormalizeFilePathCache,
debugLog, dedupePatterns, escapeRegExp, extractImportSources, getOrCreate,
isRelativeSpecifier, matchesDebugFilter, normalizeFilePath, relativizePath,
shouldDeferResolveViolation, shouldDeferResolvedViolation, stripQuery) to
satisfy the linter, and remove the unused DeferredBuildViolation import (or
change it to a type-only import "type DeferredBuildViolation" if it will be used
purely as a type elsewhere). Apply the same fix for the other occurrence
mentioned.
In `@packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts`:
- Around line 3-4: Reorder the imports in postCompileUsage.ts so the value
import parseImportProtectionAst comes before the type-only import ParsedAst;
specifically swap the two import lines so parseImportProtectionAst is imported
first and then import type { ParsedAst } from './ast' to satisfy the
import/order rule.
In
`@packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts`:
- Around line 6-8: Combine and reorder the imports so type and value imports are
grouped consistently: replace the separate imports from './ast' with a single
statement that includes the value and the type (e.g. import {
parseImportProtectionAst, type ParsedAst } from './ast') and keep the type-only
import for SourceMapLike as import type { SourceMapLike } from
'./sourceLocation'; this groups exports by module and keeps type imports
explicit to satisfy import/order.
- Around line 145-173: collectNamedExportsFromAst is under-collecting because it
only adds simple Identifier names and skips destructured variable declarators;
update it to (1) extract identifiers recursively from VariableDeclarator.id when
the id is an ObjectPattern or ArrayPattern (walk patterns and add each
Identifier name via the existing add helper), and (2) ensure export specifiers
handle non-Identifier exported nodes (e.g., StringLiteral) by converting
exported.value to a string before calling add; keep using isValidExportName in
add but feed it every discovered stringified export name. Reference: function
collectNamedExportsFromAst, VariableDeclaration -> decl.declarations,
VariableDeclarator.id, ObjectPattern/ArrayPattern cases, and export specifier
handling where s.exported is not an Identifier.
In `@packages/start-plugin-core/src/import-protection-plugin/utils.ts`:
- Around line 1-2: The import order in utils.ts violates the import/order rule:
move the Node builtin import before third-party imports so that "node:path" (the
extname, isAbsolute, resolve as resolvePath imports) appears before the 'vite'
import (normalizePath); update the top of file to import extname, isAbsolute,
resolvePath from 'node:path' first and then import { normalizePath } from 'vite'
to satisfy linting.
- Line 151: The containment check using resolvedPath.startsWith(srcDirectory) is
boundary-unsafe; instead normalize both paths with path.resolve and use
path.relative(srcDirectory, resolvedPath) to determine containment — return true
only when the relative path does not start with '..' and is not an absolute
path. Update the check that currently references resolvedPath and srcDirectory
to use the normalized path and path.relative approach so paths like /app/src2
are not treated as inside /app/src.
---
Outside diff comments:
In `@e2e/react-start/import-protection/vite.config.ts`:
- Around line 4-16: The tsconfigPaths plugin uses a relative projects path which
can break when the server is started from a different CWD; update the plugin
invocation in vite.config.ts to resolve the tsconfig.json to an absolute path by
importing/using path.resolve with import.meta.url (or import.meta.dirname) and
pass that absolute path to tsconfigPaths({ projects: [resolvedPath] }) so alias
resolution is stable regardless of working directory.
---
Nitpick comments:
In `@e2e/react-start/import-protection-custom-config/package.json`:
- Line 4: The package.json currently sets "sideEffects": false which is intended
for libraries and can cause unexpected tree-shaking for an application; update
the package.json to either set "sideEffects": true or remove the "sideEffects"
field so the e2e app's routes/components are not inadvertently eliminated—locate
the "sideEffects" entry in package.json and change its value to true (or delete
the key) and run the e2e build to verify no runtime component/route stripping
occurs.
In `@e2e/react-start/import-protection-custom-config/src/routes/__root.tsx`:
- Around line 1-8: The import specifiers from '@tanstack/react-router' are not
alphabetized (HeadContent should come before Link); reorder the named imports in
the import statement so they are sorted alphabetically (e.g., createRootRoute,
HeadContent, Link, linkOptions, Outlet, Scripts) to satisfy ESLint and keep the
existing import path and usage of createRootRoute/HeadContent/Link/etc.
unchanged.
In `@e2e/react-start/import-protection-custom-config/tests/custom-config.spec.ts`:
- Around line 112-113: Remove the redundant dynamic imports of node:fs and
node:path in the test body and instead import fs at module level and reuse the
existing top-level path import; specifically, delete the two lines "const fs =
await import('node:fs')" and "const path = await import('node:path')" in
custom-config.spec.ts and add a top-level import for fs (so tests referencing fs
and path use the module-level identifiers) to keep imports consistent and avoid
dynamic requires.
In `@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts`:
- Around line 9-52: Both waitForHttpOk and killChild are duplicated; extract
them into a shared test utility module and import it from both setup files.
Create a new utility file exporting waitForHttpOk and killChild (preserve their
signatures and behavior, including use of AbortSignal.timeout and spawn
ReturnType), update this file to export types if needed, then replace the
duplicated implementations in error-mode.setup.ts and violations.setup.ts with
imports from the new module (update any relative import paths). Run tests to
ensure no behavior change.
In `@e2e/react-start/import-protection-custom-config/vite.config.ts`:
- Around line 4-7: The code uses a type assertion for the environment variable
into 'mock' | 'error' but does no runtime validation; update the logic that sets
the behavior variable (the behavior constant in vite.config.ts) to validate
process.env.BEHAVIOR against the allowed values ('mock' and 'error') at runtime
and fall back to 'mock' (or throw) on invalid input—i.e., read
process.env.BEHAVIOR, check if it === 'mock' or === 'error', and assign that
value or the default 'mock' (or raise a clear error) so you don’t rely solely on
the type assertion.
In
`@packages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.ts`:
- Around line 175-201: Add two regression tests to the collectNamedExports
suite: one that verifies destructured exports are collected (e.g. a snippet
using "export const { a, b: renamed } = obj" and asserting collectNamedExports
returns ['a','renamed']) and one that verifies string-keyed export specifiers
are handled (e.g. a snippet with "const x = 1; export { x as 'str' }" and
asserting collectNamedExports returns ['str']). Place these tests alongside the
existing cases in the same describe('collectNamedExports') block so they
exercise the collectNamedExports function handling of destructured and
string-keyed named exports.
In `@packages/start-plugin-core/tests/importProtection/utils.test.ts`:
- Around line 141-203: Add a boundary test inside the existing
describe('shouldDeferResolvedViolation') block that verifies paths under
'/app/src2' are not treated as inside '/app/src': call
shouldDeferResolvedViolation with isDevMock: true, isBuild: false, source:
'src/x', resolvedPath: '/app/src2/x.ts', srcDirectory: '/app/src',
isPreTransformResolve: false and assert the result is false; this ensures the
prefix '/app/src' match is not satisfied by '/app/src2'.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
e2e/react-start/import-protection-custom-config/.gitignoree2e/react-start/import-protection-custom-config/package.jsone2e/react-start/import-protection-custom-config/playwright.config.tse2e/react-start/import-protection-custom-config/src/routeTree.gen.tse2e/react-start/import-protection-custom-config/src/router.tsxe2e/react-start/import-protection-custom-config/src/routes/__root.tsxe2e/react-start/import-protection-custom-config/src/routes/backend-leak.tsxe2e/react-start/import-protection-custom-config/src/routes/frontend-leak.tsxe2e/react-start/import-protection-custom-config/src/routes/index.tsxe2e/react-start/import-protection-custom-config/tests/custom-config.spec.tse2e/react-start/import-protection-custom-config/tests/error-mode.setup.tse2e/react-start/import-protection-custom-config/tests/error-mode.spec.tse2e/react-start/import-protection-custom-config/tests/utils/isErrorMode.tse2e/react-start/import-protection-custom-config/tests/violations.setup.tse2e/react-start/import-protection-custom-config/tests/violations.utils.tse2e/react-start/import-protection-custom-config/tsconfig.jsone2e/react-start/import-protection-custom-config/vite.config.tse2e/react-start/import-protection/package.jsone2e/react-start/import-protection/src/routes/__root.tsxe2e/react-start/import-protection/src/routes/alias-path-leak.tsxe2e/react-start/import-protection/src/routes/alias-path-namespace-leak.tsxe2e/react-start/import-protection/src/routes/non-alias-namespace-leak.tsxe2e/react-start/import-protection/tests/import-protection.spec.tse2e/react-start/import-protection/tests/violations.setup.tse2e/react-start/import-protection/vite.config.tspackages/start-plugin-core/src/constants.tspackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/ast.tspackages/start-plugin-core/src/import-protection-plugin/constants.tspackages/start-plugin-core/src/import-protection-plugin/extensionlessAbsoluteIdResolver.tspackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/import-protection-plugin/postCompileUsage.tspackages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.tspackages/start-plugin-core/src/import-protection-plugin/sourceLocation.tspackages/start-plugin-core/src/import-protection-plugin/types.tspackages/start-plugin-core/src/import-protection-plugin/utils.tspackages/start-plugin-core/src/import-protection-plugin/virtualModules.tspackages/start-plugin-core/src/start-compiler-plugin/plugin.tspackages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.tspackages/start-plugin-core/tests/importProtection/utils.test.tspackages/start-plugin-core/tests/importProtection/virtualModules.test.ts
Comment on lines
+59
to
+67
| const importer = importerLine | ||
| ? importerLine.split('Importer:')[1]!.trim() | ||
| : '' | ||
| const specifier = specLine | ||
| ? specLine.split('Import:')[1]!.trim().replace(/^"|"$/g, '') | ||
| : '' | ||
| const resolved = resolvedLine | ||
| ? resolvedLine.split('Resolved:')[1]!.trim() | ||
| : undefined |
Contributor
There was a problem hiding this comment.
Non-null assertions after split() may throw if delimiter is missing.
If the log format changes or a line doesn't contain the expected delimiter, split()[1] returns undefined, and the ! assertion will cause a runtime error. Consider adding fallback handling.
🛡️ Proposed safer handling
- const importer = importerLine
- ? importerLine.split('Importer:')[1]!.trim()
- : ''
- const specifier = specLine
- ? specLine.split('Import:')[1]!.trim().replace(/^"|"$/g, '')
- : ''
- const resolved = resolvedLine
- ? resolvedLine.split('Resolved:')[1]!.trim()
- : undefined
+ const importer = importerLine?.split('Importer:')[1]?.trim() ?? ''
+ const specifier =
+ specLine?.split('Import:')[1]?.trim().replace(/^"|"$/g, '') ?? ''
+ const resolved = resolvedLine?.split('Resolved:')[1]?.trim()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection-custom-config/tests/violations.utils.ts`
around lines 59 - 67, The current parsing uses non-null assertions like
importerLine.split('Importer')[1]! which can throw if the delimiter is missing;
update the parsing for importer/specifier/resolved to safely handle missing
delimiters by checking for the delimiter or split result length before indexing
(e.g. inspect importerLine/includes('Importer') or const parts =
importerLine.split('Importer'); const importer = parts.length > 1 ?
parts[1].trim() : ''), apply the same pattern for specLine/specifier and
resolvedLine/resolved (and preserve the existing replace for specifier when
present) so the code never assumes split()[1] exists.
Comment on lines
+50
to
+57
| test.beforeEach(({}, testInfo) => { | ||
| const baseURL = testInfo.project.use.baseURL | ||
| if (!baseURL) { | ||
| throw new Error( | ||
| 'Missing Playwright baseURL for import-protection e2e. Run with `pnpm exec playwright test -c e2e/react-start/import-protection/playwright.config.ts` (or configure `use.baseURL`).', | ||
| ) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix empty object pattern flagged by static analysis.
The empty object destructuring ({}, testInfo) triggers both Biome and ESLint errors. Since you're not using fixture parameters, you can remove the empty destructuring.
🐛 Proposed fix
-test.beforeEach(({}, testInfo) => {
+test.beforeEach(({ }, testInfo) => {Actually, since Playwright's beforeEach provides fixtures as the first argument, and you're only using testInfo, consider using the underscore convention or accessing testInfo differently:
-test.beforeEach(({}, testInfo) => {
+test.beforeEach((_, testInfo) => {
const baseURL = testInfo.project.use.baseURLOr alternatively, if Playwright allows:
-test.beforeEach(({}, testInfo) => {
+test.beforeEach(({ baseURL }, testInfo) => {
+ if (!baseURL) {
- const baseURL = testInfo.project.use.baseURL
- if (!baseURL) {📝 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
| test.beforeEach(({}, testInfo) => { | |
| const baseURL = testInfo.project.use.baseURL | |
| if (!baseURL) { | |
| throw new Error( | |
| 'Missing Playwright baseURL for import-protection e2e. Run with `pnpm exec playwright test -c e2e/react-start/import-protection/playwright.config.ts` (or configure `use.baseURL`).', | |
| ) | |
| } | |
| }) | |
| test.beforeEach((_, testInfo) => { | |
| const baseURL = testInfo.project.use.baseURL | |
| if (!baseURL) { | |
| throw new Error( | |
| 'Missing Playwright baseURL for import-protection e2e. Run with `pnpm exec playwright test -c e2e/react-start/import-protection/playwright.config.ts` (or configure `use.baseURL`).', | |
| ) | |
| } | |
| }) |
🧰 Tools
🪛 Biome (2.4.4)
[error] 50-50: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 ESLint
[error] 50-50: Unexpected empty object pattern.
(no-empty-pattern)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/tests/import-protection.spec.ts` around
lines 50 - 57, The empty object destructuring in test.beforeEach causes lint
errors; update the parameter list for test.beforeEach to avoid an unused
destructured object — e.g. replace "({}, testInfo)" with "(_, testInfo)"
(underscore for the unused first fixture) or otherwise accept only the testInfo
position permitted by Playwright; update the test.beforeEach callback signature
so testInfo is still used while the unused first fixture is named "_" to satisfy
linters.
Comment on lines
+91
to
+100
| private buildDepsForKey(key: string, resolvedFile: string | undefined) { | ||
| const deps = new Set<DepKey>() | ||
| deps.add(`file:${key}`) | ||
|
|
||
| if (isAbsolute(key)) { | ||
| deps.add(`dir:${dirname(key)}`) | ||
| } | ||
| if (resolvedFile) { | ||
| deps.add(`file:${resolvedFile}`) | ||
| } |
Contributor
There was a problem hiding this comment.
Track dir:${key} for extensionless absolute IDs to prevent stale HMR cache
Line 96 only records dir:${dirname(key)}. For IDs like /src/foo.server, resolution can depend on /src/foo.server/index.*. Changes under that directory invalidate dir:/src/foo.server, but this key dependency is never indexed, so stale entries can persist.
💡 Suggested fix
- private buildDepsForKey(key: string, resolvedFile: string | undefined) {
+ private buildDepsForKey(
+ key: string,
+ resolvedFile: string | undefined,
+ ): Set<DepKey> {
const deps = new Set<DepKey>()
deps.add(`file:${key}`)
if (isAbsolute(key)) {
deps.add(`dir:${dirname(key)}`)
+ // `./<basename(key)>` may resolve via `<key>/index.*`
+ if (!FILE_RESOLUTION_EXTENSIONS.includes(extname(key))) {
+ deps.add(`dir:${key}`)
+ }
}
if (resolvedFile) {
deps.add(`file:${resolvedFile}`)
}📝 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
| private buildDepsForKey(key: string, resolvedFile: string | undefined) { | |
| const deps = new Set<DepKey>() | |
| deps.add(`file:${key}`) | |
| if (isAbsolute(key)) { | |
| deps.add(`dir:${dirname(key)}`) | |
| } | |
| if (resolvedFile) { | |
| deps.add(`file:${resolvedFile}`) | |
| } | |
| private buildDepsForKey( | |
| key: string, | |
| resolvedFile: string | undefined, | |
| ): Set<DepKey> { | |
| const deps = new Set<DepKey>() | |
| deps.add(`file:${key}`) | |
| if (isAbsolute(key)) { | |
| deps.add(`dir:${dirname(key)}`) | |
| // `./<basename(key)>` may resolve via `<key>/index.*` | |
| if (!FILE_RESOLUTION_EXTENSIONS.includes(extname(key))) { | |
| deps.add(`dir:${key}`) | |
| } | |
| } | |
| if (resolvedFile) { | |
| deps.add(`file:${resolvedFile}`) | |
| } |
🤖 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/extensionlessAbsoluteIdResolver.ts`
around lines 91 - 100, The HMR deps built in buildDepsForKey only add
dir:${dirname(key)}, which misses tracking the directory named by an
extensionless absolute ID (e.g. /src/foo.server) and allows stale cache entries;
modify buildDepsForKey to also add deps.add(`dir:${key}`) when isAbsolute(key)
so both the parent directory and the directory represented by the extensionless
absolute ID are indexed (refer to buildDepsForKey, the existing deps Set,
isAbsolute(key) and dirname(key)).
packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts
Outdated
Show resolved
Hide resolved
Comment on lines
6
to
8
| import type { SourceMapLike } from './sourceLocation' | ||
| import { parseImportProtectionAst } from './ast' | ||
| import type { ParsedAst } from './ast' |
Contributor
There was a problem hiding this comment.
Fix import ordering to satisfy import/order.
The current value/type import order around ./ast and ./sourceLocation triggers lint failure.
🧰 Tools
🪛 ESLint
[error] 7-7: ./ast import should occur before type import of ./sourceLocation
(import/order)
🤖 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/rewriteDeniedImports.ts`
around lines 6 - 8, Combine and reorder the imports so type and value imports
are grouped consistently: replace the separate imports from './ast' with a
single statement that includes the value and the type (e.g. import {
parseImportProtectionAst, type ParsedAst } from './ast') and keep the type-only
import for SourceMapLike as import type { SourceMapLike } from
'./sourceLocation'; this groups exports by module and keeps type imports
explicit to satisfy import/order.
packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts
Show resolved
Hide resolved
packages/start-plugin-core/src/import-protection-plugin/utils.ts
Outdated
Show resolved
Hide resolved
packages/start-plugin-core/src/import-protection-plugin/utils.ts
Outdated
Show resolved
Hide resolved
schiller-manuel
force-pushed
the
fix-6770
branch
from
32101ec to
0761ff6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/start-plugin-core/src/import-protection-plugin/extensionlessAbsoluteIdResolver.ts (1)
91-100:⚠️ Potential issue | 🟠 MajorTrack
dir:${key}for extensionless absolute IDs to prevent stale HMR cache entries.Line 96 only indexes
dir:${dirname(key)}. For a key like/src/foo.server, changes under/src/foo.server/index.*invalidatedir:/src/foo.server, which is not tracked here, so stale entries can persist.💡 Suggested fix
- private buildDepsForKey(key: string, resolvedFile: string | undefined) { + private buildDepsForKey( + key: string, + resolvedFile: string | undefined, + ): Set<DepKey> { const deps = new Set<DepKey>() deps.add(`file:${key}`) if (isAbsolute(key)) { deps.add(`dir:${dirname(key)}`) + // Extensionless absolute IDs may resolve via <key>/index.* + if (!FILE_RESOLUTION_EXTENSIONS.includes(extname(key))) { + deps.add(`dir:${key}`) + } } if (resolvedFile) { deps.add(`file:${resolvedFile}`) }🤖 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/extensionlessAbsoluteIdResolver.ts` around lines 91 - 100, In buildDepsForKey, when handling absolute keys (function buildDepsForKey) also add a dependency for the directory named by the key itself for extensionless absolute IDs: inside the isAbsolute(key) branch, if path.extname(key) === '' (i.e. key has no extension) call deps.add(`dir:${key}`) in addition to deps.add(`dir:${dirname(key)}`) so changes to /src/foo.server/index.* invalidate dir:/src/foo.server; add path.extname import if missing.e2e/react-start/import-protection/tests/import-protection.spec.ts (1)
50-57:⚠️ Potential issue | 🟡 MinorReplace empty object destructuring in
beforeEach.Line 50 still uses an empty object pattern (
({}, testInfo)), which triggers both Biome and ESLintno-empty-pattern.🐛 Proposed fix
-test.beforeEach(({}, testInfo) => { +test.beforeEach((_, testInfo) => { const baseURL = testInfo.project.use.baseURL if (!baseURL) { throw new Error( 'Missing Playwright baseURL for import-protection e2e. Run with `pnpm exec playwright test -c e2e/react-start/import-protection/playwright.config.ts` (or configure `use.baseURL`).', ) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/tests/import-protection.spec.ts` around lines 50 - 57, The beforeEach callback is using an empty object pattern "({}, testInfo)" which triggers no-empty-pattern; change the parameter list to name the unused first arg instead (e.g., "_fixtures" or simply "_" as the first parameter) so it becomes "( _ , testInfo )" or "( _fixtures, testInfo )", and leave the body unchanged (still read baseURL from testInfo). Update the test.beforeEach declaration accordingly to reference testInfo as before.packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts (1)
170-174:⚠️ Potential issue | 🟠 Major
collectNamedExportsFromAststill drops valid string-keyed exports.Line 173 gates on
isValidExportName, so names like"foo-bar"are discarded even though the mock generators already support string-keyed re-exports. That can produce self-denial modules missing real exports.Suggested fix
export function collectNamedExportsFromAst(ast: ParsedAst): Array<string> { const names = new Set<string>() const add = (name: string) => { - if (isValidExportName(name)) names.add(name) + if (name.length > 0 && name !== 'default') names.add(name) }#!/bin/bash # Verify collection-vs-emission mismatch for non-identifier export names. rg -n "collectNamedExportsFromAst|isValidExportName|generateExportLines|filterExportNames" \ packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts \ packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts # Check whether tests currently cover string-keyed exports through collectNamedExportsFromAst. rg -n 'collectNamedExportsFromAst|string-keyed|export \{.*as ".*"\}' \ packages/start-plugin-core/tests/importProtection || true🤖 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/rewriteDeniedImports.ts` around lines 170 - 174, collectNamedExportsFromAst currently filters export names with isValidExportName which drops valid string-keyed exports (e.g., "foo-bar"); remove or bypass that validation inside collectNamedExportsFromAst so it collects raw export keys (including quoted/string keys) and let downstream logic (generateExportLines or filterExportNames) decide how to emit or sanitize identifiers; update collectNamedExportsFromAst to add every export name encountered to the Set and ensure generateExportLines/filterExportNames handle any necessary quoting or renaming.packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md (1)
192-194:⚠️ Potential issue | 🟡 MinorAdd a language identifier to this fenced block.
Line 192 still opens a fence without a language, which trips markdownlint
MD040.Suggested fix
-``` +```text \0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD></details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdaround
lines 192 - 194, The fenced code block containing the literal
"\0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>" lacks a language
identifier (tripping markdownlint MD040); fix it by changing the opening fence
fromtotext so the block becomes a fenced "text" code block (i.e., add
the language identifiertextimmediately after the opening backticks).</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (6)</summary><blockquote> <details> <summary>e2e/react-start/import-protection/src/routes/alias-path-namespace-leak.tsx (1)</summary><blockquote> `2-2`: **Consider documenting this as an intentional violation fixture.** Please add a short inline comment at Line 2 that this server-only import is deliberate for import-protection e2e coverage, to prevent accidental “cleanup” later. Based on learnings, in TanStack/router e2e tests intentionally non-production patterns should stay test-only and be clearly documented as test-only. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/src/routes/alias-path-namespace-leak.tsx` at line 2, Add a short inline comment on the import line that makes clear the server-only import of secretModule (import * as secretModule from '~/violations/secret.server') is an intentional test-only violation for import-protection e2e coverage; update the line to include a brief note referencing that this pattern is deliberate for alias/namespace leak testing so it isn't accidentally removed or "cleaned up" later. ``` </details> </blockquote></details> <details> <summary>e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts (3)</summary><blockquote> `73-77`: **Use type narrowing instead of `any` for error handling.** Using `err: any` bypasses TypeScript's type checking. Consider narrowing the error type for safer property access. <details> <summary>♻️ Proposed type-safe error handling</summary> ```diff - } catch (err: any) { - exitCode = err.status ?? 1 - stdout = err.stdout?.toString() ?? '' - stderr = err.stderr?.toString() ?? '' + } catch (err: unknown) { + if (err && typeof err === 'object') { + exitCode = 'status' in err && typeof err.status === 'number' ? err.status : 1 + stdout = 'stdout' in err && err.stdout ? String(err.stdout) : '' + stderr = 'stderr' in err && err.stderr ? String(err.stderr) : '' + } else { + exitCode = 1 + } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts` around lines 73 - 77, The catch currently types the caught error as `any`; change it to `unknown` and narrow it before accessing `status`, `stdout`, and `stderr`: in the catch block for the code that sets `exitCode`, `stdout`, and `stderr`, declare the parameter as `err: unknown` and then use type guards (e.g. `instanceof Error` and/or checking for specific properties like `status`, `stdout`, `stderr`) or narrow to the NodeJS ExecException-like shape before reading those properties, falling back to safe defaults if checks fail. ``` </details> --- `9-52`: **Consider extracting shared utilities.** The `waitForHttpOk` and `killChild` functions are duplicated across multiple setup files (`violations.setup.ts` in both projects, and `error-mode.setup.ts`). Consider extracting these to a shared utility file to reduce maintenance burden. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts` around lines 9 - 52, Extract the duplicated utilities waitForHttpOk and killChild into a shared test-utilities module and update callers to import them instead of duplicating; create a new module exporting async function waitForHttpOk(url: string, timeoutMs: number) and async function killChild(child: ReturnType<typeof spawn>): Promise<void> with the same logic, then replace the local definitions in error-mode.setup.ts (and the other setup files like violations.setup.ts) with imports from the new module and run tests to ensure nothing breaks. ``` </details> --- `133-141`: **Consider capturing actual dev server exit code.** The result always writes `exitCode: 0` regardless of the actual child process outcome. If the error-mode tests need to verify server behavior, capturing the real exit code could be useful. <details> <summary>♻️ Proposed fix to capture actual exit code</summary> ```diff + let actualExitCode = 0 + child.once('exit', (code) => { + actualExitCode = code ?? 0 + }) + child.stdout?.on('data', (d: Buffer) => logChunks.push(d.toString())) // ... rest of function ... const combined = logChunks.join('') fs.writeFileSync( path.resolve(cwd, 'error-dev-result.json'), JSON.stringify( - { exitCode: 0, stdout: combined, stderr: '', combined }, + { exitCode: actualExitCode, stdout: combined, stderr: '', combined }, null, 2, ), ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts` around lines 133 - 141, The file currently writes exitCode: 0 into the result JSON; change this to record the real dev server exit code by using the actual variable that tracks the child process outcome (e.g., use the exit code captured from the spawned process's 'exit'/'close' handler, commonly named exitCode or serverExitCode) instead of 0; ensure the handler sets that variable before writing the file (refer to logChunks, combined, cwd, and the error-dev-result.json write site) so the JSON contains the real exitCode value. ``` </details> </blockquote></details> <details> <summary>e2e/react-start/import-protection-custom-config/tests/violations.utils.ts (1)</summary><blockquote> `8-11`: **`CodeSnippet` type differs from the source definition.** The local `CodeSnippet` type is missing the `highlightLine` property and makes `location` optional, whereas the source definition in `packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts` (lines 385-392) has `highlightLine: number` as required and `location: string` as required. This inconsistency could cause confusion or type mismatches if the types are expected to align. Consider either importing the type from the source package or aligning the properties if this is intentionally a subset for test purposes. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/violations.utils.ts` around lines 8 - 11, The local CodeSnippet type in violations.utils.ts is inconsistent with the canonical definition (missing required highlightLine:number and marking location optional); either import the CodeSnippet type from packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts or update the local declaration to include highlightLine: number and make location: string required so the shapes match (adjust any tests that relied on optional location accordingly). ``` </details> </blockquote></details> <details> <summary>e2e/react-start/import-protection-custom-config/tests/violations.setup.ts (1)</summary><blockquote> `148-157`: **This setup only captures cold violations.** Unlike `e2e/react-start/import-protection/tests/violations.setup.ts` which captures both cold and warm dev violations, this file only does a single cold pass. This may be intentional for the custom-config tests, but worth verifying if warm cache behavior should also be tested here. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection-custom-config/tests/violations.setup.ts` around lines 148 - 157, The current captureDevViolations function only performs a single cold run via runDevPass and writes that result; update captureDevViolations to also perform a warm/dev cached run (either by invoking runDevPass a second time after the initial pass or by calling the existing warm-run helper if present) and include both results when writing the output (e.g. an object with cold and warm keys or separate files). Locate captureDevViolations and runDevPass in this file, perform the second invocation using the same cwd and port, and persist both outputs so warm-cache behavior is tested alongside cold behavior. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@e2e/react-start/import-protection-custom-config/src/routes/__root.tsx:
- Around line 1-8: The named imports from '@tanstack/react-router' in the import
statement (createRootRoute, HeadContent, Link, linkOptions, Outlet, Scripts) are
not in the order required by ESLint sort-imports; reorder the members to match
the project's alphabetical/sort-imports policy (e.g., alphabetically:
HeadContent, Link, Scripts, createRootRoute, linkOptions, Outlet or whatever
ordering your config enforces) so the import statement for these symbols is
lint-compliant, updating the import clause that references createRootRoute,
HeadContent, Link, linkOptions, Outlet, Scripts.In
@e2e/react-start/import-protection/tests/import-protection.spec.ts:
- Around line 551-552: The key-generation currently strips everything after the
first colon with v.importer.replace(/:./, ''), which collapses Windows drive
letters (e.g., C:) and causes collisions; update the logic that builds the
dedupe key to only remove URL/file scheme prefixes (e.g., strip "file:" or
"file://" or other protocol:// prefixes) while preserving Windows drive
letters—use a targeted replace like removing /^file:/// or a conditional that,
if v.importer startsWith('file:') or contains '://', strips the scheme,
otherwise leaves v.importer intact; reference the use site where the key is
composed alongside normalizeSpec and v.importer and replace the global /:./
replace with the safer scheme-only handling.In
@packages/start-plugin-core/src/import-protection-plugin/plugin.ts:
- Around line 1842-1863: The current check treats importerSurvived as sufficient
for reporting a violation, which can yield false positives when the denied edge
was fully eliminated; change the logic in the block around
mockSurvived/importerSurvived so that importerSurvived alone does not trigger a
report for file/specifier cases — after computing importerVariantIds (from
info.importer and
env.transformResultKeysByFile.get(normalizeFilePath(info.importer))) and
toModuleIdCandidates(checkId), additionally verify that at least one surviving
candidate for checkId is actually referenced by the importer's transform keys
(i.e., an import edge still exists between any importerVariantIds and any
candidate in toModuleIdCandidates(checkId) within survivingModules or the
transform results) before treating importerSurvived as sufficient; keep the
existing mockSurvived check as-is and only report when a real surviving import
edge is confirmed.
Duplicate comments:
In@e2e/react-start/import-protection/tests/import-protection.spec.ts:
- Around line 50-57: The beforeEach callback is using an empty object pattern
"({}, testInfo)" which triggers no-empty-pattern; change the parameter list to
name the unused first arg instead (e.g., "fixtures" or simply "" as the first
parameter) so it becomes "( _ , testInfo )" or "( _fixtures, testInfo )", and
leave the body unchanged (still read baseURL from testInfo). Update the
test.beforeEach declaration accordingly to reference testInfo as before.In
@packages/start-plugin-core/src/import-protection-plugin/extensionlessAbsoluteIdResolver.ts:
- Around line 91-100: In buildDepsForKey, when handling absolute keys (function
buildDepsForKey) also add a dependency for the directory named by the key itself
for extensionless absolute IDs: inside the isAbsolute(key) branch, if
path.extname(key) === '' (i.e. key has no extension) call deps.add(dir:${key})
in addition to deps.add(dir:${dirname(key)}) so changes to
/src/foo.server/index.* invalidate dir:/src/foo.server; add path.extname import
if missing.In
@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md:
- Around line 192-194: The fenced code block containing the literal
"\0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>" lacks a language
identifier (tripping markdownlint MD040); fix it by changing the opening fence
fromtotext so the block becomes a fenced "text" code block (i.e., add
the language identifiertextimmediately after the opening backticks).In
@packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts:
- Around line 170-174: collectNamedExportsFromAst currently filters export names
with isValidExportName which drops valid string-keyed exports (e.g., "foo-bar");
remove or bypass that validation inside collectNamedExportsFromAst so it
collects raw export keys (including quoted/string keys) and let downstream logic
(generateExportLines or filterExportNames) decide how to emit or sanitize
identifiers; update collectNamedExportsFromAst to add every export name
encountered to the Set and ensure generateExportLines/filterExportNames handle
any necessary quoting or renaming.
Nitpick comments:
In@e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts:
- Around line 73-77: The catch currently types the caught error as
any; change
it tounknownand narrow it before accessingstatus,stdout, andstderr:
in the catch block for the code that setsexitCode,stdout, andstderr,
declare the parameter aserr: unknownand then use type guards (e.g.
instanceof Errorand/or checking for specific properties likestatus,
stdout,stderr) or narrow to the NodeJS ExecException-like shape before
reading those properties, falling back to safe defaults if checks fail.- Around line 9-52: Extract the duplicated utilities waitForHttpOk and killChild
into a shared test-utilities module and update callers to import them instead of
duplicating; create a new module exporting async function waitForHttpOk(url:
string, timeoutMs: number) and async function killChild(child: ReturnType): Promise with the same logic, then replace the local definitions
in error-mode.setup.ts (and the other setup files like violations.setup.ts) with
imports from the new module and run tests to ensure nothing breaks.- Around line 133-141: The file currently writes exitCode: 0 into the result
JSON; change this to record the real dev server exit code by using the actual
variable that tracks the child process outcome (e.g., use the exit code captured
from the spawned process's 'exit'/'close' handler, commonly named exitCode or
serverExitCode) instead of 0; ensure the handler sets that variable before
writing the file (refer to logChunks, combined, cwd, and the
error-dev-result.json write site) so the JSON contains the real exitCode value.In
@e2e/react-start/import-protection-custom-config/tests/violations.setup.ts:
- Around line 148-157: The current captureDevViolations function only performs a
single cold run via runDevPass and writes that result; update
captureDevViolations to also perform a warm/dev cached run (either by invoking
runDevPass a second time after the initial pass or by calling the existing
warm-run helper if present) and include both results when writing the output
(e.g. an object with cold and warm keys or separate files). Locate
captureDevViolations and runDevPass in this file, perform the second invocation
using the same cwd and port, and persist both outputs so warm-cache behavior is
tested alongside cold behavior.In
@e2e/react-start/import-protection-custom-config/tests/violations.utils.ts:
- Around line 8-11: The local CodeSnippet type in violations.utils.ts is
inconsistent with the canonical definition (missing required
highlightLine:number and marking location optional); either import the
CodeSnippet type from
packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts or
update the local declaration to include highlightLine: number and make location:
string required so the shapes match (adjust any tests that relied on optional
location accordingly).In
@e2e/react-start/import-protection/src/routes/alias-path-namespace-leak.tsx:
- Line 2: Add a short inline comment on the import line that makes clear the
server-only import of secretModule (import * as secretModule from
'~/violations/secret.server') is an intentional test-only violation for
import-protection e2e coverage; update the line to include a brief note
referencing that this pattern is deliberate for alias/namespace leak testing so
it isn't accidentally removed or "cleaned up" later.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 32101ec71e9d3f26621290d13aff97d3b265906d and 0761ff6d6011581f655950a86e1ee24bebf507a3. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (41)</summary> * `e2e/react-start/import-protection-custom-config/.gitignore` * `e2e/react-start/import-protection-custom-config/package.json` * `e2e/react-start/import-protection-custom-config/playwright.config.ts` * `e2e/react-start/import-protection-custom-config/src/routeTree.gen.ts` * `e2e/react-start/import-protection-custom-config/src/router.tsx` * `e2e/react-start/import-protection-custom-config/src/routes/__root.tsx` * `e2e/react-start/import-protection-custom-config/src/routes/backend-leak.tsx` * `e2e/react-start/import-protection-custom-config/src/routes/frontend-leak.tsx` * `e2e/react-start/import-protection-custom-config/src/routes/index.tsx` * `e2e/react-start/import-protection-custom-config/tests/custom-config.spec.ts` * `e2e/react-start/import-protection-custom-config/tests/error-mode.setup.ts` * `e2e/react-start/import-protection-custom-config/tests/error-mode.spec.ts` * `e2e/react-start/import-protection-custom-config/tests/utils/isErrorMode.ts` * `e2e/react-start/import-protection-custom-config/tests/violations.setup.ts` * `e2e/react-start/import-protection-custom-config/tests/violations.utils.ts` * `e2e/react-start/import-protection-custom-config/tsconfig.json` * `e2e/react-start/import-protection-custom-config/vite.config.ts` * `e2e/react-start/import-protection/package.json` * `e2e/react-start/import-protection/src/routes/__root.tsx` * `e2e/react-start/import-protection/src/routes/alias-path-leak.tsx` * `e2e/react-start/import-protection/src/routes/alias-path-namespace-leak.tsx` * `e2e/react-start/import-protection/src/routes/non-alias-namespace-leak.tsx` * `e2e/react-start/import-protection/tests/import-protection.spec.ts` * `e2e/react-start/import-protection/tests/violations.setup.ts` * `e2e/react-start/import-protection/vite.config.ts` * `packages/start-plugin-core/src/constants.ts` * `packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md` * `packages/start-plugin-core/src/import-protection-plugin/ast.ts` * `packages/start-plugin-core/src/import-protection-plugin/constants.ts` * `packages/start-plugin-core/src/import-protection-plugin/extensionlessAbsoluteIdResolver.ts` * `packages/start-plugin-core/src/import-protection-plugin/plugin.ts` * `packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts` * `packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts` * `packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts` * `packages/start-plugin-core/src/import-protection-plugin/types.ts` * `packages/start-plugin-core/src/import-protection-plugin/utils.ts` * `packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts` * `packages/start-plugin-core/src/start-compiler-plugin/plugin.ts` * `packages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.ts` * `packages/start-plugin-core/tests/importProtection/utils.test.ts` * `packages/start-plugin-core/tests/importProtection/virtualModules.test.ts` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (24)</summary> * e2e/react-start/import-protection-custom-config/tests/utils/isErrorMode.ts * e2e/react-start/import-protection/package.json * e2e/react-start/import-protection-custom-config/.gitignore * packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts * e2e/react-start/import-protection-custom-config/playwright.config.ts * e2e/react-start/import-protection-custom-config/src/routes/backend-leak.tsx * packages/start-plugin-core/src/import-protection-plugin/constants.ts * e2e/react-start/import-protection-custom-config/package.json * e2e/react-start/import-protection/src/routes/non-alias-namespace-leak.tsx * e2e/react-start/import-protection-custom-config/vite.config.ts * packages/start-plugin-core/tests/importProtection/rewriteDeniedImports.test.ts * e2e/react-start/import-protection-custom-config/tests/error-mode.spec.ts * e2e/react-start/import-protection-custom-config/src/routes/frontend-leak.tsx * e2e/react-start/import-protection-custom-config/src/routeTree.gen.ts * packages/start-plugin-core/tests/importProtection/utils.test.ts * e2e/react-start/import-protection/vite.config.ts * packages/start-plugin-core/src/constants.ts * e2e/react-start/import-protection/src/routes/__root.tsx * e2e/react-start/import-protection-custom-config/src/router.tsx * e2e/react-start/import-protection-custom-config/src/routes/index.tsx * packages/start-plugin-core/src/import-protection-plugin/types.ts * packages/start-plugin-core/src/start-compiler-plugin/plugin.ts * e2e/react-start/import-protection-custom-config/tests/custom-config.spec.ts * e2e/react-start/import-protection-custom-config/tsconfig.json </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+1
to
+8
| import { | ||
| createRootRoute, | ||
| HeadContent, | ||
| Link, | ||
| linkOptions, | ||
| Outlet, | ||
| Scripts, | ||
| } from '@tanstack/react-router' |
Contributor
There was a problem hiding this comment.
Fix import member ordering to satisfy ESLint sort-imports.
Line 3 indicates member order is currently not lint-compliant.
Proposed diff
import {
- createRootRoute,
HeadContent,
+ createRootRoute,
Link,
linkOptions,
Outlet,
Scripts,
} from '@tanstack/react-router'🧰 Tools
🪛 ESLint
[error] 3-3: Member 'HeadContent' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection-custom-config/src/routes/__root.tsx` around
lines 1 - 8, The named imports from '@tanstack/react-router' in the import
statement (createRootRoute, HeadContent, Link, linkOptions, Outlet, Scripts) are
not in the order required by ESLint sort-imports; reorder the members to match
the project's alphabetical/sort-imports policy (e.g., alphabetically:
HeadContent, Link, Scripts, createRootRoute, linkOptions, Outlet or whatever
ordering your config enforces) so the import statement for these symbols is
lint-compliant, updating the import clause that references createRootRoute,
HeadContent, Link, linkOptions, Outlet, Scripts.
Comment on lines
+551
to
552
| `${v.envType}|${v.type}|${normalizeSpec(v)}|${v.importer.replace(/:.*/, '')}` | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid colon-stripping that breaks Windows paths in warm/cold dedupe keys.
v.importer.replace(/:.*/, '') can reduce C:\... to C, causing key collisions and incorrect warm-vs-cold comparisons.
🐛 Proposed fix
const normalizeSpec = (v: Violation) =>
(v.resolved ?? v.specifier).replace(/\.[cm]?[tj]sx?$/, '')
+const stripLineCol = (importer: string) => importer.replace(/:\d+:\d+$/, '')
const uniqueKey = (v: Violation) =>
- `${v.envType}|${v.type}|${normalizeSpec(v)}|${v.importer.replace(/:.*/, '')}`
+ `${v.envType}|${v.type}|${normalizeSpec(v)}|${stripLineCol(v.importer)}`📝 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
| `${v.envType}|${v.type}|${normalizeSpec(v)}|${v.importer.replace(/:.*/, '')}` | |
| const normalizeSpec = (v: Violation) => | |
| (v.resolved ?? v.specifier).replace(/\.[cm]?[tj]sx?$/, '') | |
| const stripLineCol = (importer: string) => importer.replace(/:\d+:\d+$/, '') | |
| const uniqueKey = (v: Violation) => | |
| `${v.envType}|${v.type}|${normalizeSpec(v)}|${stripLineCol(v.importer)}` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/tests/import-protection.spec.ts` around
lines 551 - 552, The key-generation currently strips everything after the first
colon with v.importer.replace(/:.*/, ''), which collapses Windows drive letters
(e.g., C:\) and causes collisions; update the logic that builds the dedupe key
to only remove URL/file scheme prefixes (e.g., strip "file:" or "file://" or
other protocol:// prefixes) while preserving Windows drive letters—use a
targeted replace like removing /^file:\/\// or a conditional that, if v.importer
startsWith('file:') or contains '://', strips the scheme, otherwise leaves
v.importer intact; reference the use site where the key is composed alongside
normalizeSpec and v.importer and replace the global /:.*/ replace with the safer
scheme-only handling.
packages/start-plugin-core/src/import-protection-plugin/plugin.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 1871-1886: The branch guarded by "else if (importerSurvived &&
checkModuleId == null)" is currently re-checking module existence (via
toModuleIdCandidates(checkId) and survivingModules) which is redundant with the
earlier mockSurvived check and makes the branch unreachable; change the check to
test the actual surviving import-edge between the importer and each candidate
target instead of just module presence: use the data structure that tracks
surviving import edges (e.g., survivingEdges or the map that records
importer→target relations) to see if any candidate c has a surviving edge from
the importer (use importerId/importerSurvived and checkId/checkModuleId with
that edges map), and only continue when no such surviving edge exists; reference
symbols: importerSurvived, mockSurvived, checkModuleId, checkId,
toModuleIdCandidates, survivingModules, edgeSurvived—replace the
survivingModules-based edgeSurvived test with an import-edge existence check
against the surviving edges collection so file/specifier (barrel) violations can
be reported.
packages/start-plugin-core/src/import-protection-plugin/plugin.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/start-plugin-core/src/import-protection-plugin/plugin.ts (1)
1950-1961:⚠️ Potential issue | 🟠 Major
edgeSurvivedcheck is redundant here and makes the file/specifier branch effectively unreachable.At Line 1950, this branch runs only when
mockSurvivedis already false andcheckModuleId == null(socheckId === mockModuleId).
At Line 1958-1960,edgeSurvivedre-checks the same condition asmockSurvived, so it stays false and alwayscontinues. This can suppress real file/specifier violations.💡 Proposed fix
- } else if (importerSurvived && checkModuleId == null) { - // File/specifier violation: importerSurvived alone is NOT - // sufficient. The importer may survive for unrelated imports - // (barrel pattern) while tree-shaking eliminated the denied - // edge. Verify that the denied target's module candidates - // actually exist in survivingModules (i.e., an import edge - // between the importer and the denied target still exists). - const checkCandidates = toModuleIdCandidates(checkId) - const edgeSurvived = checkCandidates.some((c) => - survivingModules.has(c), - ) - if (!edgeSurvived) continue + } else if (importerSurvived && checkModuleId == null) { + // File/specifier violation: importer survival alone is not enough. + // Confirm at least one importer variant still imports a candidate + // of the denied target after transform. + const targetCandidates = new Set(toModuleIdCandidates(checkId)) + let edgeSurvived = false + + for (const importerId of importerVariantIds) { + const imports = + env.postTransformImports.get(importerId) ?? + env.postTransformImports.get(normalizeFilePath(importerId)) + if (!imports) continue + + for (const importedId of imports) { + if ( + toModuleIdCandidates(importedId).some((c) => + targetCandidates.has(c), + ) + ) { + edgeSurvived = true + break + } + } + if (edgeSurvived) break + } + + if (!edgeSurvived) continue } else {🤖 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/plugin.ts` around lines 1950 - 1961, The edgeSurvived re-check is redundant in the file/specifier branch and makes that branch unreachable; remove the block that computes checkCandidates and edgeSurvived (the call to toModuleIdCandidates(checkId), the survivingModules.has check, and the if (!edgeSurvived) continue) so the logic that handles the file/specifier violation (when importerSurvived is true and checkModuleId == null / checkId === mockModuleId) can run; reference symbols: importerSurvived, checkModuleId, checkId, mockModuleId, toModuleIdCandidates, survivingModules, edgeSurvived.
🤖 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/plugin.ts`:
- Around line 1950-1961: The edgeSurvived re-check is redundant in the
file/specifier branch and makes that branch unreachable; remove the block that
computes checkCandidates and edgeSurvived (the call to
toModuleIdCandidates(checkId), the survivingModules.has check, and the if
(!edgeSurvived) continue) so the logic that handles the file/specifier violation
(when importerSurvived is true and checkModuleId == null / checkId ===
mockModuleId) can run; reference symbols: importerSurvived, checkModuleId,
checkId, mockModuleId, toModuleIdCandidates, survivingModules, edgeSurvived.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/react-start/import-protection/package.jsonpackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/import-protection-plugin/utils.tspackages/start-plugin-core/tests/importProtection/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/react-start/import-protection/package.json
schiller-manuel
force-pushed
the
fix-6770
branch
from
c838b77 to
4e00b36
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md (1)
219-221:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
The fenced code block is missing a language identifier, which triggers markdownlint MD040.
🛠️ Suggested fix
-``` +```text \0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>🤖 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 219 - 221, The fenced code block containing the literal line "\0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>" is missing a language identifier and triggers markdownlint MD040; update the block delimiter to include a language (for example change "```" to "```text") so it reads "```text" above that line and keep the closing "```" unchanged, ensuring the fenced block now has a language identifier.
🤖 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/INTERNALS.md`:
- Around line 219-221: The fenced code block containing the literal line
"\0tanstack-start-import-protection:mock-edge:<BASE64_PAYLOAD>" is missing a
language identifier and triggers markdownlint MD040; update the block delimiter
to include a language (for example change "```" to "```text") so it reads
"```text" above that line and keep the closing "```" unchanged, ensuring the
fenced block now has a language identifier.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/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/utils.tspackages/start-plugin-core/tests/importProtection/utils.test.ts
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.
fixes #6770
Summary by CodeRabbit
New Features
Tests
Refactor
Chores