Conversation
github-actions
bot
added
documentation
Contributor
📝 WalkthroughWalkthroughAdds a full Import Protection feature: plugin implementation, schemas, defaults, utilities, virtual/mock/marker modules, source-map and compiler improvements, e2e Playwright fixture and tests, framework marker exports (React/Solid/Vue), docs, and .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Vite as Vite/Dev/Build
participant Plugin as ImportProtectionPlugin
participant Graph as ImportGraph
participant Compiler as StartCompiler/Transform
participant Reporter as Violation Handler
Vite->>Plugin: resolveId(importer, specifier, ctx)
Plugin->>Plugin: compile matchers & check rules
alt Denied import
Plugin->>Graph: addEdge(resolvedId, importer, specifier)
Plugin->>Compiler: request transform & sourcemap for importer/resolved
Compiler-->>Plugin: transform result + sourcemap (with sourcesContent)
Plugin->>Plugin: buildTrace(graph, startNode)
Plugin->>Reporter: create ViolationInfo (trace + snippet)
alt behavior = error
Reporter-->>Vite: emit error (fail build)
else behavior = mock
Reporter-->>Vite: return virtual/mock module id
Vite->>Plugin: load virtual mock/runtime module
end
else Allowed
Plugin-->>Vite: normal resolution
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 8m 50s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 3s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-02-18 00:14:37 UTC
schiller-manuel
force-pushed
the
import-protection
branch
from
b6a890c to
9d363c4
Compare
![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: 12
🧹 Nitpick comments (5)
e2e/react-start/import-protection/src/violations/marked-server-only-edge.ts (1)
3-4: Add an explicit return type for the wrapper export.This makes the API surface explicit and aligns with strict typing.
🔧 Proposed change
-export function getServerOnlyDataViaEdge() { +export function getServerOnlyDataViaEdge(): ReturnType<typeof getServerOnlyData> { return getServerOnlyData() }As per coding guidelines, “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/src/violations/marked-server-only-edge.ts` around lines 3 - 4, The exported wrapper getServerOnlyDataViaEdge currently lacks an explicit return type; update its signature to declare the return type explicitly (for example use ReturnType<typeof getServerOnlyData> or the concrete type returned by getServerOnlyData) so the API surface is explicit and compatible with strict TypeScript. Modify the function declaration export function getServerOnlyDataViaEdge(): ReturnType<typeof getServerOnlyData> { return getServerOnlyData() } (or the concrete type) to satisfy strict typing and alignment with getServerOnlyData.e2e/react-start/import-protection/src/violations/marked-server-only.ts (1)
9-10: Consider adding an explicit return type for consistency with type safety practices.TypeScript strict mode is enabled in this package. While the function's return type can be inferred correctly by TypeScript, other similar exported functions in this package (
getSecret(),getClientOnlyData(), etc.) similarly omit explicit return types. If you want to improve type safety alignment across the module, add an explicit return type; however, this is optional and not required since TypeScript strict mode already handles inference properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/src/violations/marked-server-only.ts` around lines 9 - 10, Add an explicit return type to the exported function getServerOnlyData to match other exported helpers (e.g., getSecret(), getClientOnlyData()) and improve type consistency; locate the getServerOnlyData() function and annotate its signature with the appropriate type (for example the concrete type of SERVER_ONLY_DATA or typeof SERVER_ONLY_DATA) so the function signature is explicit while leaving the implementation unchanged.packages/start-plugin-core/src/start-compiler-plugin/plugin.ts (1)
198-198: Consider removing the unused_commandvariable.The
_commandvariable is declared and assigned in multipleconfigResolvedhooks (lines 232, 426) but is never read. If this variable is not needed, consider removing it entirely rather than keeping it as dead code with an underscore prefix.If it's intended for future use, a brief comment explaining the planned purpose would help maintainability.
♻️ If the variable is truly unused, remove it
- let _command: 'build' | 'serve' = 'build'And remove the assignments in
configResolvedhooks:configResolved(config) { root = config.root - _command = config.command },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/start-compiler-plugin/plugin.ts` at line 198, The declared variable `_command: 'build' | 'serve' = 'build'` is never read; remove the dead declaration and all assignments to `_command` inside the `configResolved` hooks (the hooks that set `_command` at two places) to eliminate unused code, or if you intend to keep it for future use add a one-line comment above the declaration explaining the planned purpose and leave it as-is; update all references accordingly (i.e., delete the `_command` symbol and its assignments in the `configResolved` handlers if removing).packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts (1)
197-210: Potential variable name collision with multiple re-exports from different denied sources.The internal variable
__tss_reexport_${localName}uses only thelocalNamefrom the specifier. If a file has multipleexport { x } from 'denied1'andexport { x } from 'denied2'statements, both would generate__tss_reexport_x, causing a collision.Consider incorporating the
mockCounterto ensure uniqueness:♻️ Suggested fix
- const internalVar = `__tss_reexport_${localName}` + const internalVar = `__tss_reexport_${mockCounter}_${localName}`🤖 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 197 - 210, The internal temp name __tss_reexport_${localName} can collide across multiple re-exports; update the generator to include the unique mock counter (mockCounter) when creating internalVar (e.g., __tss_reexport_${localName}_${mockCounter}) and then increment mockCounter so each created variable for t.variableDeclarator and the corresponding entry pushed to exportSpecifiers is unique; ensure you reference mockVar, localName, exportedName as before but use the counter-enhanced internalVar when pushing to replacements and exportSpecifiers.e2e/react-start/import-protection/tests/violations.utils.ts (1)
1-22: Tighten violation/env types to explicit unions.
Line 14-16 currently usestring, which allows invalid values and weakens strict typing. Consider narrowing to literal unions forenvTypeandtype.🔧 Suggested type tightening
-export type Violation = { - envType?: string - type: string +export type Violation = { + envType?: 'client' | 'server' + type: 'marker' | 'specifier' | 'file' | 'unknown'As per coding guidelines:
**/*.{ts,tsx}: 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.utils.ts` around lines 1 - 22, The Violation type's envType and type fields are overly broad strings; update the Violation declaration to replace envType?: string and type: string with explicit literal union types (e.g., envType?: 'node' | 'browser' | 'universal' or whatever allowed envs are, and type: 'forbidden' | 'deprecated' | 'internal' etc. matching your app's valid violation kinds) so that consumers of Violation get strict typing; modify the Violation type in violations.utils.ts accordingly and update any callers of Violation/creation sites to use the new literals.
🤖 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/src/routes/__root.tsx`:
- Around line 1-7: Reorder the named imports in the import from
'@tanstack/react-router' so they are alphabetically sorted; adjust the list
containing createRootRoute, HeadContent, Link, Outlet, Scripts to the correct
alphabetical order (ensure HeadContent is placed according to that ordering) to
satisfy the sort-imports ESLint rule.
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Around line 43-47: Change the catch parameter from any to unknown and add
explicit type-narrowing before accessing error properties: in the catch block
for execSync (the catch currently using err: any), use a type guard (e.g., check
typeof err === 'object' && err !== null and 'status' in err, 'stdout' in err,
'stderr' in err) to safely read err.status, err.stdout, and err.stderr and fall
back to your defaults (exitCode = 1, stdout = '', stderr = '') when properties
are missing or of unexpected types; ensure you update the assignments to
exitCode, stdout, and stderr accordingly so no direct property access occurs on
an unknown-typed value.
In `@e2e/react-start/import-protection/tests/error-mode.spec.ts`:
- Around line 20-22: Replace the ad-hoc "as any" cast on the dynamic import
options by extracting the options object into a const (e.g. const importOptions
= { with: { type: 'json' } }) and annotate it using the TypeScript "satisfies
ImportCallOptions" pattern, then pass that const to import(resultPath,
importOptions); update the same pattern in import-protection.spec.ts as well so
both locations remove the unsafe cast and use a properly typed ImportCallOptions
object.
In `@e2e/react-start/import-protection/tests/import-protection.spec.ts`:
- Around line 27-36: The JSON dynamic import in readViolations currently uses a
loose cast ("as any"); change the cast to a strict module shape like "as {
default: Array<Violation> }" for the result of the import(violationsPath, {
with: { type: 'json' }) call and then return mod.default ?? [] without an
additional cast so the function signature Promise<Array<Violation>> is preserved
(update the import call site in readViolations to use the typed module shape).
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 1-8: The import order violates the ESLint import/order rule: move
the type-only import "import type { FullConfig } from '@playwright/test'" so it
appears after the local import "import { extractViolationsFromLog } from
'./violations.utils'"; reorder the top-of-file imports accordingly so external
packages (fs, path, child_process, getTestServerPort, packageJson) come first,
then local utilities (extractViolationsFromLog), and then the type-only
Playwright import to satisfy the lint rule.
- Around line 123-134: The teardown Promise can hang if the child process
already exited; before attaching the 'exit' listener in the block that
constructs the Promise for child shutdown, check child.exitCode (or
child.killed) and resolve immediately if it already exited, add a
child.on('error', ...) handler to reject/resolve on unexpected errors, and wrap
the initial child.kill('SIGTERM') call in a try-catch so kill failures don't
throw; keep the fallback SIGKILL timeout but also guard its kill() in a
try-catch and ensure the resolve() (done) is only called once (reuse the
existing done function) when exit or error occurs.
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 43-46: Import specifiers in the type-only import from
sourceLocation are out of alphabetical order; reorder the members so they are
sorted alphabetically (e.g., ensure TransformResult, TransformResultProvider,
SourceMapLike appear in ascending name order) to satisfy the sort-imports rule;
update the import list where these symbols are declared to be alphabetized.
In `@packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts`:
- Around line 131-203: The var bindings are being recorded into the current
(possibly block) scope in walk() which causes incorrect shadowing; change the
t.isVariableDeclarator(astNode) handling so that when the declarator's parent is
a VariableDeclaration with kind === 'var' you register bindings into the nearest
enclosing function/program scope instead of scopes[scopes.length-1]. Implement
this by inspecting astNode.parent (or passing the parent) to check
declaration.kind, and if 'var' walk the scopes array backwards to find the first
function/program scope index (using the same isScopeNode logic or a helper) and
call collectPatternBindings(astNode.id, scopes[thatIndex]) ; otherwise keep the
current behavior of collectPatternBindings(astNode.id, scopes[scopes.length-1]).
In `@packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts`:
- Line 1: Split the mixed value/type import on SourceMapConsumer and
RawSourceMap into two imports: keep the runtime import for SourceMapConsumer as
"import { SourceMapConsumer } from 'source-map'" and add a separate type-only
import "import type { RawSourceMap } from 'source-map'"; update the import at
the top of the file to use these two statements to match the project's
import-style pattern.
In `@packages/start-plugin-core/src/import-protection-plugin/trace.ts`:
- Around line 124-139: The code incorrectly treats nodes with no reverse-edge
map as non-entry by using importers?.size === 0; change the isEntry check in the
block that sets root to explicitly treat missing maps as entries (e.g., const
importers = graph.reverseEdges.get(node); const isEntry =
graph.entries.has(node) || !importers || importers.size === 0) and remove or
reuse the subsequent duplicate graph.reverseEdges.get(node) fetch to avoid
shadowing the importers variable; ensure the later logic that checks
depth/maxDepth uses that same importers reference.
In `@packages/start-plugin-core/tests/importProtection/dedupePatterns.test.ts`:
- Around line 54-56: Replace the unsafe cast "map as any" when calling
pickOriginalCodeFromSourcesContent by typing the fixture map as SourceMapLike
from sourceLocation; locate the test file references to the variable named map
and the calls to pickOriginalCodeFromSourcesContent (including the second
occurrence around lines 72-74) and change the fixture declarations or the
call-site types to use SourceMapLike so the objects keep strict typing instead
of using any.
In `@packages/start-plugin-core/tests/importProtection/transform.test.ts`:
- Around line 1-4: One of the top-level imports in transform.test.ts is unused
and failing lint; remove the unused identifier from the import list (e.g., drop
the unused symbol from "import { describe, expect, test } from 'vitest'" or
remove the unused "RESOLVED_MOCK_MODULE_ID" import) and update the import
statement to only include the symbols actually referenced by the file (ensure
rewriteDeniedImports and any used vitest helpers remain imported).
---
Nitpick comments:
In `@e2e/react-start/import-protection/src/violations/marked-server-only-edge.ts`:
- Around line 3-4: The exported wrapper getServerOnlyDataViaEdge currently lacks
an explicit return type; update its signature to declare the return type
explicitly (for example use ReturnType<typeof getServerOnlyData> or the concrete
type returned by getServerOnlyData) so the API surface is explicit and
compatible with strict TypeScript. Modify the function declaration export
function getServerOnlyDataViaEdge(): ReturnType<typeof getServerOnlyData> {
return getServerOnlyData() } (or the concrete type) to satisfy strict typing and
alignment with getServerOnlyData.
In `@e2e/react-start/import-protection/src/violations/marked-server-only.ts`:
- Around line 9-10: Add an explicit return type to the exported function
getServerOnlyData to match other exported helpers (e.g., getSecret(),
getClientOnlyData()) and improve type consistency; locate the
getServerOnlyData() function and annotate its signature with the appropriate
type (for example the concrete type of SERVER_ONLY_DATA or typeof
SERVER_ONLY_DATA) so the function signature is explicit while leaving the
implementation unchanged.
In `@e2e/react-start/import-protection/tests/violations.utils.ts`:
- Around line 1-22: The Violation type's envType and type fields are overly
broad strings; update the Violation declaration to replace envType?: string and
type: string with explicit literal union types (e.g., envType?: 'node' |
'browser' | 'universal' or whatever allowed envs are, and type: 'forbidden' |
'deprecated' | 'internal' etc. matching your app's valid violation kinds) so
that consumers of Violation get strict typing; modify the Violation type in
violations.utils.ts accordingly and update any callers of Violation/creation
sites to use the new literals.
In
`@packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts`:
- Around line 197-210: The internal temp name __tss_reexport_${localName} can
collide across multiple re-exports; update the generator to include the unique
mock counter (mockCounter) when creating internalVar (e.g.,
__tss_reexport_${localName}_${mockCounter}) and then increment mockCounter so
each created variable for t.variableDeclarator and the corresponding entry
pushed to exportSpecifiers is unique; ensure you reference mockVar, localName,
exportedName as before but use the counter-enhanced internalVar when pushing to
replacements and exportSpecifiers.
In `@packages/start-plugin-core/src/start-compiler-plugin/plugin.ts`:
- Line 198: The declared variable `_command: 'build' | 'serve' = 'build'` is
never read; remove the dead declaration and all assignments to `_command` inside
the `configResolved` hooks (the hooks that set `_command` at two places) to
eliminate unused code, or if you intend to keep it for future use add a one-line
comment above the declaration explaining the planned purpose and leave it as-is;
update all references accordingly (i.e., delete the `_command` symbol and its
assignments in the `configResolved` handlers if removing).
Comment on lines
+1
to
+7
| import { | ||
| createRootRoute, | ||
| HeadContent, | ||
| Link, | ||
| Outlet, | ||
| Scripts, | ||
| } from '@tanstack/react-router' |
Contributor
There was a problem hiding this comment.
Fix ESLint import member ordering.
sort-imports reports HeadContent is out of order. Please re-order the named imports to satisfy the rule.
🔧 Suggested change
import {
- createRootRoute,
- HeadContent,
+ HeadContent,
Link,
Outlet,
Scripts,
+ createRootRoute,
} from '@tanstack/react-router'📝 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
| import { | |
| createRootRoute, | |
| HeadContent, | |
| Link, | |
| Outlet, | |
| Scripts, | |
| } from '@tanstack/react-router' | |
| import { | |
| HeadContent, | |
| Link, | |
| Outlet, | |
| Scripts, | |
| createRootRoute, | |
| } 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/src/routes/__root.tsx` around lines 1 - 7,
Reorder the named imports in the import from '@tanstack/react-router' so they
are alphabetically sorted; adjust the list containing createRootRoute,
HeadContent, Link, Outlet, Scripts to the correct alphabetical order (ensure
HeadContent is placed according to that ordering) to satisfy the sort-imports
ESLint rule.
Comment on lines
+43
to
+47
| } catch (err: any) { | ||
| // execSync throws on non-zero exit code — this is the *expected* path. | ||
| exitCode = err.status ?? 1 | ||
| stdout = err.stdout ?? '' | ||
| stderr = err.stderr ?? '' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and view the relevant lines
cat -n e2e/react-start/import-protection/tests/error-mode.setup.ts | head -60 | tail -30Repository: TanStack/router
Length of output: 958
🏁 Script executed:
# Also check the tsconfig to verify strict mode settings
find . -name "tsconfig.json" -type f | head -5Repository: TanStack/router
Length of output: 314
🏁 Script executed:
# Check the root tsconfig
cat tsconfig.json 2>/dev/null | jq '.compilerOptions.strict' || grep -A 5 '"strict"' tsconfig.json 2>/dev/null || echo "No root tsconfig or strict setting found"Repository: TanStack/router
Length of output: 63
🏁 Script executed:
# Check for similar execSync usage patterns in the codebase
rg "execSync" -A 3 -B 3 --type ts --type tsx | head -100Repository: TanStack/router
Length of output: 86
🏁 Script executed:
# Let's check the Node.js types for execSync by looking at dependencies
cat package.json | grep -A 2 -B 2 "@types/node"Repository: TanStack/router
Length of output: 428
🏁 Script executed:
# Check what properties are actually accessed on the error object
rg "err\.(status|stdout|stderr)" --type ts --type tsxRepository: TanStack/router
Length of output: 86
🏁 Script executed:
# Search for execSync patterns
rg "execSync" -A 5 -B 2 | head -150Repository: TanStack/router
Length of output: 2057
🏁 Script executed:
# Search for catch blocks with error handling
rg "catch.*err.*any" -B 2 -A 3Repository: TanStack/router
Length of output: 7943
🏁 Script executed:
# Check the structure of what execSync throws - look for other error handling
rg "\\.status.*\\.stdout.*\\.stderr" -B 5Repository: TanStack/router
Length of output: 41
Replace any with unknown and add type narrowing for error properties.
TypeScript strict mode requires proper error typing. When execSync throws, it includes status, stdout, and stderr properties that need explicit type handling rather than using any.
🔧 Suggested change
- } catch (err: any) {
+ } catch (err: unknown) {
+ const e = err as NodeJS.ErrnoException & {
+ status?: number | null
+ stdout?: string | Buffer
+ stderr?: string | Buffer
+ }
// execSync throws on non-zero exit code — this is the *expected* path.
- exitCode = err.status ?? 1
- stdout = err.stdout ?? ''
- stderr = err.stderr ?? ''
+ exitCode = e.status ?? 1
+ stdout = typeof e.stdout === 'string' ? e.stdout : e.stdout?.toString() ?? ''
+ stderr = typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString() ?? ''
}📝 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
| } catch (err: any) { | |
| // execSync throws on non-zero exit code — this is the *expected* path. | |
| exitCode = err.status ?? 1 | |
| stdout = err.stdout ?? '' | |
| stderr = err.stderr ?? '' | |
| } catch (err: unknown) { | |
| const e = err as NodeJS.ErrnoException & { | |
| status?: number | null | |
| stdout?: string | Buffer | |
| stderr?: string | Buffer | |
| } | |
| // execSync throws on non-zero exit code — this is the *expected* path. | |
| exitCode = e.status ?? 1 | |
| stdout = typeof e.stdout === 'string' ? e.stdout : e.stdout?.toString() ?? '' | |
| stderr = typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString() ?? '' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts` around lines 43
- 47, Change the catch parameter from any to unknown and add explicit
type-narrowing before accessing error properties: in the catch block for
execSync (the catch currently using err: any), use a type guard (e.g., check
typeof err === 'object' && err !== null and 'status' in err, 'stdout' in err,
'stderr' in err) to safely read err.status, err.stdout, and err.stderr and fall
back to your defaults (exitCode = 1, stdout = '', stderr = '') when properties
are missing or of unexpected types; ensure you update the assignments to
exitCode, stdout, and stderr accordingly so no direct property access occurs on
an unknown-typed value.
Comment on lines
+20
to
+22
| const mod = await import(resultPath, { | ||
| with: { type: 'json' }, | ||
| } as any) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file to confirm the snippet
find . -name "error-mode.spec.ts" -path "*/e2e/react-start/import-protection/tests/*" | head -5Repository: TanStack/router
Length of output: 119
🏁 Script executed:
# Get the file size to determine how to read it
wc -l e2e/react-start/import-protection/tests/error-mode.spec.ts 2>/dev/null || echo "File not found"Repository: TanStack/router
Length of output: 120
🏁 Script executed:
# Read the relevant lines around 20-22
sed -n '15,30p' e2e/react-start/import-protection/tests/error-mode.spec.tsRepository: TanStack/router
Length of output: 552
🏁 Script executed:
# Check tsconfig.json to understand TypeScript configuration
find . -name "tsconfig.json" -type f | xargs grep -l "strict" 2>/dev/null | head -3Repository: TanStack/router
Length of output: 234
🏁 Script executed:
# Search for how dynamic imports are used elsewhere in the codebase to understand patterns
rg "await import\(" --type ts --type tsx -B2 -A2 | head -40Repository: TanStack/router
Length of output: 86
🏁 Script executed:
# Check if ImportCallOptions is used elsewhere or if there are type definitions for import options
rg "ImportCallOptions" --type ts --type tsxRepository: TanStack/router
Length of output: 86
🏁 Script executed:
# Check TypeScript configuration for strict mode
cat e2e/react-start/import-protection/tests/tsconfig.json 2>/dev/null || echo "No local tsconfig, checking parent"Repository: TanStack/router
Length of output: 93
🏁 Script executed:
# Find root tsconfig
find . -maxdepth 2 -name "tsconfig.json" | head -1 | xargs cat | head -30Repository: TanStack/router
Length of output: 846
🏁 Script executed:
# Search for import patterns without type flag issues
grep -r "await import" e2e/react-start/import-protection/tests/ 2>/dev/nullRepository: TanStack/router
Length of output: 269
🏁 Script executed:
# Check if satisfies keyword is used in the codebase
grep -r "satisfies" --include="*.ts" --include="*.tsx" e2e/ 2>/dev/null | head -10Repository: TanStack/router
Length of output: 990
🏁 Script executed:
# Check what the ImportCallOptions type is in TypeScript definitions
python3 << 'EOF'
# ImportCallOptions is actually not a standard TypeScript built-in type
# The second parameter to import() should be ImportCallOptions from TS 5.3+
# Let's check TypeScript version in the project
import json
try:
with open('package.json', 'r') as f:
pkg = json.load(f)
ts_version = pkg.get('devDependencies', {}).get('typescript', 'not found')
print(f"TypeScript version: {ts_version}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: TanStack/router
Length of output: 85
🏁 Script executed:
# Verify the current type annotation is actually incorrect by checking what TS expects
# Let's also check if there are any other uses of import() with type assertions
grep -r "as any" e2e/react-start/import-protection/ --include="*.ts" 2>/dev/nullRepository: TanStack/router
Length of output: 207
🏁 Script executed:
# Check if ImportCallOptions is actually available in the codebase or needs to be imported
grep -r "ImportCallOptions" . --include="*.ts" --include="*.tsx" 2>/dev/nullRepository: TanStack/router
Length of output: 1094
🏁 Script executed:
# Check TypeScript lib definitions to see what types are available for import()
# Let's also check what types are available for the import assertion
python3 << 'EOF'
import json
import re
# Read package.json to understand setup
with open('package.json', 'r') as f:
pkg = json.load(f)
print("TypeScript version:", pkg.get('devDependencies', {}).get('typescript'))
# Check if there are type definition files
import os
for root, dirs, files in os.walk('.'):
if 'node_modules' in root or '.git' in root:
continue
for f in files:
if 'types.ts' in f or 'types.d.ts' in f:
filepath = os.path.join(root, f)
print(f"Found types file: {filepath}")
if os.path.getsize(filepath) < 5000:
with open(filepath, 'r') as tf:
content = tf.read()
if 'import' in content.lower():
print(f" Contains import-related types")
EOFRepository: TanStack/router
Length of output: 929
🏁 Script executed:
# Let's check the exact line context around 20-22 to see the function signature
sed -n '10,35p' e2e/react-start/import-protection/tests/error-mode.spec.tsRepository: TanStack/router
Length of output: 917
🏁 Script executed:
# Check if there are any other similar patterns in the codebase that handle type safety for import()
rg "import\(.*with.*type" --type ts -B2 -A2Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if the type for import options can be inferred without 'as any'
# by looking at what types are used in other test files
find e2e -name "*.spec.ts" -exec grep -l "import(" {} \; | head -5Repository: TanStack/router
Length of output: 255
🏁 Script executed:
# Let's check the actual ImportCallOptions interface definition
sed -n '1,/interface ImportCallOptions/p' ./node_modules/.pnpm/typescript@5.9.3/node_modules/typescript/lib/lib.es5.d.ts | tail -20Repository: TanStack/router
Length of output: 565
🏁 Script executed:
# Get more context around ImportCallOptions
grep -A10 "interface ImportCallOptions" ./node_modules/.pnpm/typescript@5.9.3/node_modules/typescript/lib/lib.es5.d.ts | head -20Repository: TanStack/router
Length of output: 333
🏁 Script executed:
# Check if there are any other test files that handle import assertions properly
find . -name "*.spec.ts" -exec grep -l "with.*type" {} \; | head -5 | xargs grep -B2 -A2 "with.*type" | head -30Repository: TanStack/router
Length of output: 2800
🏁 Script executed:
# Verify the suggested fix would actually work by checking syntax
python3 << 'EOF'
# Create a test to verify the suggested syntax is valid
test_code = '''
// Test if satisfies can be used this way
const importOptions = {
with: { type: 'json' },
} satisfies ImportCallOptions
// This should work since ImportCallOptions is the correct type
'''
print("The suggested fix pattern is valid TypeScript syntax")
print("ImportCallOptions is available in TypeScript 5.9.3")
print("The satisfies keyword enforces type constraints without changing inferred type")
EOFRepository: TanStack/router
Length of output: 241
🏁 Script executed:
# Check the other import protection test file to see if it has the same issue
sed -n '1,50p' e2e/react-start/import-protection/tests/import-protection.spec.tsRepository: TanStack/router
Length of output: 1344
🏁 Script executed:
# Check if there are other instances of this pattern in the same file or directory
grep -n "as any" e2e/react-start/import-protection/tests/*.spec.tsRepository: TanStack/router
Length of output: 213
Remove as any type assertion and use typed import options.
The as any cast defeats TypeScript strict mode. Extract the import options to a variable and use the satisfies ImportCallOptions pattern to maintain type safety while allowing the natural object structure.
🔧 Suggested change
- const mod = await import(resultPath, {
- with: { type: 'json' },
- } as any)
+ const importOptions = {
+ with: { type: 'json' },
+ } satisfies ImportCallOptions
+ const mod = await import(resultPath, importOptions)Note: The same pattern exists in import-protection.spec.ts at line 34 and should be fixed consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mod = await import(resultPath, { | |
| with: { type: 'json' }, | |
| } as any) | |
| const importOptions = { | |
| with: { type: 'json' }, | |
| } satisfies ImportCallOptions | |
| const mod = await import(resultPath, importOptions) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/tests/error-mode.spec.ts` around lines 20 -
22, Replace the ad-hoc "as any" cast on the dynamic import options by extracting
the options object into a const (e.g. const importOptions = { with: { type:
'json' } }) and annotate it using the TypeScript "satisfies ImportCallOptions"
pattern, then pass that const to import(resultPath, importOptions); update the
same pattern in import-protection.spec.ts as well so both locations remove the
unsafe cast and use a properly typed ImportCallOptions object.
Comment on lines
+27
to
+36
| async function readViolations( | ||
| type: 'build' | 'dev', | ||
| ): Promise<Array<Violation>> { | ||
| const filename = `violations.${type}.json` | ||
| const violationsPath = path.resolve(import.meta.dirname, '..', filename) | ||
| const mod = await import(violationsPath, { | ||
| with: { type: 'json' }, | ||
| } as any) | ||
| return (mod.default ?? []) as Array<Violation> | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "e2e/react-start/import-protection/tests/import-protection.spec.ts" | sed -n '20,45p'Repository: TanStack/router
Length of output: 986
🏁 Script executed:
fd -t f "tsconfig" | head -20Repository: TanStack/router
Length of output: 1138
🏁 Script executed:
rg "type Violation|interface Violation" e2e/react-start/import-protection/tests/Repository: TanStack/router
Length of output: 232
🏁 Script executed:
cat -n "e2e/react-start/import-protection/tests/import-protection.spec.ts" | sed -n '1,26p'Repository: TanStack/router
Length of output: 695
🏁 Script executed:
cat -n "e2e/react-start/import-protection/tests/violations.utils.ts" | grep -A 10 "export type Violation"Repository: TanStack/router
Length of output: 326
🏁 Script executed:
find "e2e/react-start/import-protection" -name "tsconfig.json" -exec cat {} \;Repository: TanStack/router
Length of output: 568
🏁 Script executed:
cat -n "e2e/react-start/import-protection/tsconfig.json" 2>/dev/null || echo "No tsconfig at root level, checking parent"Repository: TanStack/router
Length of output: 722
Avoid as any in JSON import to keep strict typing.
Cast the module shape directly: as { default: Array<Violation> } instead of using as any, then return mod.default ?? [] without the additional cast.
Example fix
- const mod = await import(violationsPath, {
- with: { type: 'json' },
- } as any)
- return (mod.default ?? []) as Array<Violation>
+ const mod = (await import(violationsPath, {
+ with: { type: 'json' },
+ })) as { default: Array<Violation> }
+ return mod.default ?? []🤖 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 27 - 36, The JSON dynamic import in readViolations currently uses a loose
cast ("as any"); change the cast to a strict module shape like "as { default:
Array<Violation> }" for the result of the import(violationsPath, { with: { type:
'json' }) call and then return mod.default ?? [] without an additional cast so
the function signature Promise<Array<Violation>> is preserved (update the import
call site in readViolations to use the typed module shape).
Comment on lines
+1
to
+8
| import fs from 'node:fs' | ||
| import path from 'node:path' | ||
| import { spawn } from 'node:child_process' | ||
| import type { FullConfig } from '@playwright/test' | ||
| import { getTestServerPort } from '@tanstack/router-e2e-utils' | ||
| import packageJson from '../package.json' with { type: 'json' } | ||
|
|
||
| import { extractViolationsFromLog } from './violations.utils' |
Contributor
There was a problem hiding this comment.
Fix import order to satisfy lint rule.
ESLint import/order expects the type-only @playwright/test import after the local utils import.
🧹 Suggested order
-import type { FullConfig } from '@playwright/test'
import { getTestServerPort } from '@tanstack/router-e2e-utils'
import packageJson from '../package.json' with { type: 'json' }
import { extractViolationsFromLog } from './violations.utils'
+import type { FullConfig } from '@playwright/test'📝 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
| import fs from 'node:fs' | |
| import path from 'node:path' | |
| import { spawn } from 'node:child_process' | |
| import type { FullConfig } from '@playwright/test' | |
| import { getTestServerPort } from '@tanstack/router-e2e-utils' | |
| import packageJson from '../package.json' with { type: 'json' } | |
| import { extractViolationsFromLog } from './violations.utils' | |
| import fs from 'node:fs' | |
| import path from 'node:path' | |
| import { spawn } from 'node:child_process' | |
| import { getTestServerPort } from '@tanstack/router-e2e-utils' | |
| import packageJson from '../package.json' with { type: 'json' } | |
| import { extractViolationsFromLog } from './violations.utils' | |
| import type { FullConfig } from '@playwright/test' |
🧰 Tools
🪛 ESLint
[error] 4-4: @playwright/test type import should occur after import of ./violations.utils
(import/order)
🤖 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 1 -
8, The import order violates the ESLint import/order rule: move the type-only
import "import type { FullConfig } from '@playwright/test'" so it appears after
the local import "import { extractViolationsFromLog } from
'./violations.utils'"; reorder the top-of-file imports accordingly so external
packages (fs, path, child_process, getTestServerPort, packageJson) come first,
then local utilities (extractViolationsFromLog), and then the type-only
Playwright import to satisfy the lint rule.
packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts
Outdated
Show resolved
Hide resolved
packages/start-plugin-core/src/import-protection-plugin/sourceLocation.ts
Outdated
Show resolved
Hide resolved
packages/start-plugin-core/tests/importProtection/dedupePatterns.test.ts
Show resolved
Hide resolved
Comment on lines
+1
to
+4
| import { describe, expect, test } from 'vitest' | ||
| import { rewriteDeniedImports } from '../../src/import-protection-plugin/rewriteDeniedImports' | ||
| import { RESOLVED_MOCK_MODULE_ID } from '../../src/import-protection-plugin/virtualModules' | ||
|
|
Contributor
There was a problem hiding this comment.
Remove unused import to satisfy lint.
🧹 Cleanup
-import { RESOLVED_MOCK_MODULE_ID } from '../../src/import-protection-plugin/virtualModules'📝 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
| import { describe, expect, test } from 'vitest' | |
| import { rewriteDeniedImports } from '../../src/import-protection-plugin/rewriteDeniedImports' | |
| import { RESOLVED_MOCK_MODULE_ID } from '../../src/import-protection-plugin/virtualModules' | |
| import { describe, expect, test } from 'vitest' | |
| import { rewriteDeniedImports } from '../../src/import-protection-plugin/rewriteDeniedImports' | |
🧰 Tools
🪛 ESLint
[error] 3-3: 'RESOLVED_MOCK_MODULE_ID' is defined but never used.
(unused-imports/no-unused-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/tests/importProtection/transform.test.ts` around
lines 1 - 4, One of the top-level imports in transform.test.ts is unused and
failing lint; remove the unused identifier from the import list (e.g., drop the
unused symbol from "import { describe, expect, test } from 'vitest'" or remove
the unused "RESOLVED_MOCK_MODULE_ID" import) and update the import statement to
only include the symbols actually referenced by the file (ensure
rewriteDeniedImports and any used vitest helpers remain imported).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/start-plugin-core/src/import-protection-plugin/defaults.ts (1)
16-20: Consider extracting the duplicatedframeworksarray.The
frameworksarray is defined identically in both functions. Extracting it to a module-level constant would reduce duplication and ensure consistency.♻️ Suggested refactor
+const SUPPORTED_FRAMEWORKS: Array<CompileStartFrameworkOptions> = [ + 'react', + 'solid', + 'vue', +] + export function getDefaultImportProtectionRules( _framework: CompileStartFrameworkOptions, ): DefaultImportProtectionRules { - const frameworks: Array<CompileStartFrameworkOptions> = [ - 'react', - 'solid', - 'vue', - ] - // Deny client importing server-specific entrypoints - const clientSpecifiers: Array<Pattern> = frameworks.map( + const clientSpecifiers: Array<Pattern> = SUPPORTED_FRAMEWORKS.map( (fw) => `@tanstack/${fw}-start/server`, ) // ... rest of function } export function getMarkerSpecifiers(_framework: CompileStartFrameworkOptions): { // ... } { - const frameworks: Array<CompileStartFrameworkOptions> = [ - 'react', - 'solid', - 'vue', - ] - return { - serverOnly: frameworks.map((fw) => `@tanstack/${fw}-start/server-only`), - clientOnly: frameworks.map((fw) => `@tanstack/${fw}-start/client-only`), + serverOnly: SUPPORTED_FRAMEWORKS.map((fw) => `@tanstack/${fw}-start/server-only`), + clientOnly: SUPPORTED_FRAMEWORKS.map((fw) => `@tanstack/${fw}-start/client-only`), } }Also applies to: 46-50
🤖 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/defaults.ts` around lines 16 - 20, The duplicated frameworks array (Array<CompileStartFrameworkOptions> containing 'react','solid','vue') appears in two places; extract it as a single module-level constant (e.g., AVAILABLE_FRAMEWORKS or FRAMEWORKS) and replace both local arrays with references to that constant so both functions use the same source of truth (update any uses in defaults.ts that currently declare frameworks at lines referencing the local variable).packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts (1)
174-181: Minor: Inconsistent indentation in generated code.The generated code string has inconsistent leading whitespace which, while functional, differs from the cleaner formatting in
loadSilentMockModule.🧹 Suggested fix
return { code: ` - import mock from ${JSON.stringify(runtimeId)}; - ${exportLines.join('\n')} - export default mock; - `, +import mock from ${JSON.stringify(runtimeId)}; +${exportLines.join('\n')} +export default mock; +`, }🤖 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 174 - 181, The generated module string contains inconsistent leading whitespace around the import and exports (see runtimeId and exportLines usage); update the template in the return { code: `...` } so it matches the cleaner formatting used by loadSilentMockModule — remove the extra leading spaces and align lines as: import mock from ${JSON.stringify(runtimeId)}\n${exportLines.join('\n')}\nexport default mock; ensuring consistent indentation and no stray leading spaces.
🤖 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 156-183: Update the two fenced-code blocks in
docs/start/framework/react/guide/import-protection.md that render the diagnostic
examples so they include a language identifier (use "text"); locate the examples
showing "[import-protection] Import denied..." (the blocks around the first
diagnostic and the second diagnostic later in the file) and change the opening
fences from ``` to ```text so the linter MD040 is satisfied and the output is
explicitly marked as plain text.
---
Duplicate comments:
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 1-8: ESLint import/order is violated: the type-only import
FullConfig from '@playwright/test' must come after local imports; reorder the
import statements so that external/third-party imports (fs, path, child_process,
getTestServerPort, packageJson) remain before the local test helper import, but
move "import type { FullConfig } from '@playwright/test'" to immediately after
those local imports or right before "import { extractViolationsFromLog }" so the
type-only import (FullConfig) appears after local imports; keep the "import
type" form and preserve all existing module names.
---
Nitpick comments:
In `@packages/start-plugin-core/src/import-protection-plugin/defaults.ts`:
- Around line 16-20: The duplicated frameworks array
(Array<CompileStartFrameworkOptions> containing 'react','solid','vue') appears
in two places; extract it as a single module-level constant (e.g.,
AVAILABLE_FRAMEWORKS or FRAMEWORKS) and replace both local arrays with
references to that constant so both functions use the same source of truth
(update any uses in defaults.ts that currently declare frameworks at lines
referencing the local variable).
In `@packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts`:
- Around line 174-181: The generated module string contains inconsistent leading
whitespace around the import and exports (see runtimeId and exportLines usage);
update the template in the return { code: `...` } so it matches the cleaner
formatting used by loadSilentMockModule — remove the extra leading spaces and
align lines as: import mock from
${JSON.stringify(runtimeId)}\n${exportLines.join('\n')}\nexport default mock;
ensuring consistent indentation and no stray leading spaces.
9a6c98e to
3a3a831
Compare
schiller-manuel
force-pushed
the
import-protection
branch
from
0226aaa to
a36638d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/violations.setup.ts`:
- Around line 223-236: The test currently calls out.end() then immediately reads
logFile, which can miss buffered writes; modify the teardown to wait for the
output stream to fully finish before reading the log. After calling out.end()
(in the finally block where killChild(child) and out.end() are used), await the
stream's completion (e.g., use stream.finished/promisify or listen for the
'finish' event) before checking fs.existsSync(logFile) and before calling
extractViolationsFromLog(text); ensure you still call killChild(child) first and
keep using logFile and extractViolationsFromLog as the identifiers.
In `@packages/start-plugin-core/tests/importProtection/dedupePatterns.test.ts`:
- Around line 7-8: The import order is wrong: the type-only import SourceMapLike
comes before the value import pickOriginalCodeFromSourcesContent which violates
the lint rule; swap the two import statements so the value import
(pickOriginalCodeFromSourcesContent) appears before the type import
(SourceMapLike), or combine them into a single import with "type" specifier for
SourceMapLike, ensuring pickOriginalCodeFromSourcesContent is a value import and
SourceMapLike remains type-only.
---
Duplicate comments:
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 1-8: The Playwright type-only import (FullConfig) is in the wrong
place for ESLint import/order; move the type-only import after the local utils
import so imports read in the correct groups—keep "import type { FullConfig }
from '@playwright/test'" and place it immediately after "import {
extractViolationsFromLog } from './violations.utils'"; ensure the rest of the
imports (fs, path, spawn, getTestServerPort, packageJson) remain in their
original positions.
Comment on lines
+223
to
+236
| } finally { | ||
| await killChild(child) | ||
| out.end() | ||
| } | ||
|
|
||
| if (!fs.existsSync(logFile)) { | ||
| fs.writeFileSync(path.resolve(cwd, 'violations.dev.json'), '[]') | ||
| return | ||
| } | ||
|
|
||
| const text = fs.readFileSync(logFile, 'utf-8') | ||
| const violations = extractViolationsFromLog(text) | ||
| fs.writeFileSync( | ||
| path.resolve(cwd, 'violations.dev.json'), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n e2e/react-start/import-protection/tests/violations.setup.ts | head -250 | tail -100Repository: TanStack/router
Length of output: 3884
🏁 Script executed:
# Check the TypeScript types for WriteStream.end()
cd e2e/react-start/import-protection/tests && \
cat -n violations.setup.ts | sed -n '159,175p'Repository: TanStack/router
Length of output: 954
🏁 Script executed:
# Verify the Node.js fs.WriteStream API and its TypeScript definition
python3 << 'EOF'
import subprocess
result = subprocess.run(['node', '--version'], capture_output=True, text=True)
print("Node version:", result.stdout.strip())
# Check TypeScript definition of WriteStream.end()
import_check = subprocess.run(
['grep', '-n', 'import.*WriteStream\|from.*fs', 'e2e/react-start/import-protection/tests/violations.setup.ts'],
capture_output=True,
text=True
)
print("Imports:")
print(import_check.stdout)
EOFRepository: TanStack/router
Length of output: 117
🏁 Script executed:
# Check the tsconfig and TypeScript setup
cat -n e2e/react-start/import-protection/tests/violations.setup.ts | head -10Repository: TanStack/router
Length of output: 545
🏁 Script executed:
# Verify the exact WriteStream API and its TypeScript signature
python3 << 'EOF'
# The Node.js fs.WriteStream has two overloads for end():
# 1. end(): WriteStream (returns immediately)
# 2. end(callback?: (error?: Error | null) => void): WriteStream (waits for finish)
# 3. end(data: string | Buffer, callback?: (error?: Error | null) => void): WriteStream
# The suggested fix uses: out.end(resolve)
# where resolve has type (value: void | PromiseLike<void>) => void
# This should work because the callback signature matches (error?: Error | null) => void
# when error is undefined
# Let's verify this is standard Node.js behavior
print("""
WriteStream.end() method signatures in Node.js:
1. end(): WriteStream - returns immediately
2. end(callback): WriteStream - calls callback when stream finishes/closes
3. end(data, callback): WriteStream - writes data then calls callback
The suggested fix: out.end(resolve)
- Passes a Promise resolver as the callback
- When stream finishes, resolve() is called
- This correctly waits for the stream to flush
""")
EOFRepository: TanStack/router
Length of output: 469
🏁 Script executed:
# Check if there's any custom error handling expected
grep -n "out\." e2e/react-start/import-protection/tests/violations.setup.ts | head -20Repository: TanStack/router
Length of output: 196
🏁 Script executed:
# Check if there are any test failures or issues related to this in recent commits
git log --oneline -20 e2e/react-start/import-protection/tests/violations.setup.ts 2>/dev/null | head -10Repository: TanStack/router
Length of output: 94
🏁 Script executed:
# Verify the exact TypeScript context - check if file is in strict mode
head -50 e2e/react-start/import-protection/tests/violations.setup.ts | grep -E "^import|^export|strict"Repository: TanStack/router
Length of output: 395
Wait for log stream to finish flushing before reading violations.
out.end() returns immediately without flushing buffered data. Reading the file immediately after can lose buffered logs, causing violations to be missed and producing flaky results. Wait for the stream to finish before readFileSync.
🛠️ Suggested fix
} finally {
await killChild(child)
- out.end()
+ await new Promise<void>((resolve, reject) => {
+ out.once('error', reject)
+ out.end(resolve)
+ })
}
if (!fs.existsSync(logFile)) {📝 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
| } finally { | |
| await killChild(child) | |
| out.end() | |
| } | |
| if (!fs.existsSync(logFile)) { | |
| fs.writeFileSync(path.resolve(cwd, 'violations.dev.json'), '[]') | |
| return | |
| } | |
| const text = fs.readFileSync(logFile, 'utf-8') | |
| const violations = extractViolationsFromLog(text) | |
| fs.writeFileSync( | |
| path.resolve(cwd, 'violations.dev.json'), | |
| } finally { | |
| await killChild(child) | |
| await new Promise<void>((resolve, reject) => { | |
| out.once('error', reject) | |
| out.end(resolve) | |
| }) | |
| } | |
| if (!fs.existsSync(logFile)) { | |
| fs.writeFileSync(path.resolve(cwd, 'violations.dev.json'), '[]') | |
| return | |
| } | |
| const text = fs.readFileSync(logFile, 'utf-8') | |
| const violations = extractViolationsFromLog(text) | |
| fs.writeFileSync( | |
| path.resolve(cwd, 'violations.dev.json'), |
🤖 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 223
- 236, The test currently calls out.end() then immediately reads logFile, which
can miss buffered writes; modify the teardown to wait for the output stream to
fully finish before reading the log. After calling out.end() (in the finally
block where killChild(child) and out.end() are used), await the stream's
completion (e.g., use stream.finished/promisify or listen for the 'finish'
event) before checking fs.existsSync(logFile) and before calling
extractViolationsFromLog(text); ensure you still call killChild(child) first and
keep using logFile and extractViolationsFromLog as the identifiers.
Comment on lines
+7
to
+8
| import type { SourceMapLike } from '../../src/import-protection-plugin/sourceLocation' | ||
| import { pickOriginalCodeFromSourcesContent } from '../../src/import-protection-plugin/sourceLocation' |
Contributor
There was a problem hiding this comment.
Fix import order for sourceLocation per lint rule.
ESLint flags value import after the type-only import from the same module. Swap them to satisfy import/order.
Suggested fix
-import type { SourceMapLike } from '../../src/import-protection-plugin/sourceLocation'
-import { pickOriginalCodeFromSourcesContent } from '../../src/import-protection-plugin/sourceLocation'
+import { pickOriginalCodeFromSourcesContent } from '../../src/import-protection-plugin/sourceLocation'
+import type { SourceMapLike } from '../../src/import-protection-plugin/sourceLocation'📝 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
| import type { SourceMapLike } from '../../src/import-protection-plugin/sourceLocation' | |
| import { pickOriginalCodeFromSourcesContent } from '../../src/import-protection-plugin/sourceLocation' | |
| import { pickOriginalCodeFromSourcesContent } from '../../src/import-protection-plugin/sourceLocation' | |
| import type { SourceMapLike } from '../../src/import-protection-plugin/sourceLocation' |
🧰 Tools
🪛 ESLint
[error] 8-8: ../../src/import-protection-plugin/sourceLocation import should occur before type import of ../../src/import-protection-plugin/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/tests/importProtection/dedupePatterns.test.ts`
around lines 7 - 8, The import order is wrong: the type-only import
SourceMapLike comes before the value import pickOriginalCodeFromSourcesContent
which violates the lint rule; swap the two import statements so the value import
(pickOriginalCodeFromSourcesContent) appears before the type import
(SourceMapLike), or combine them into a single import with "type" specifier for
SourceMapLike, ensuring pickOriginalCodeFromSourcesContent is a value import and
SourceMapLike remains type-only.
e43b107 to
20bc19a
Compare
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 1-8: The ESLint import/order rule requires the type-only import of
FullConfig from '@playwright/test' to come after the local import
extractViolationsFromLog; fix by reordering imports in violations.setup.ts so
that the line "import type { FullConfig } from '@playwright/test'" appears below
"import { extractViolationsFromLog } from './violations.utils'" (keep the import
as a type-only import).
- Around line 223-238: The code calls out.end() and immediately reads logFile
which can miss buffered data; after calling out.end() (and after await
killChild(child)) wait for the stream to fully flush by awaiting the stream
"finish" event (e.g., use stream/promises.finished(out) or await an event once
on out) before checking fs.existsSync(logFile) and reading it; update the block
around killChild(child), out.end(), and the subsequent fs.readFileSync/log
handling to await the stream finish and then proceed to
extractViolationsFromLog.
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.
closes #5469
Summary by CodeRabbit