fix(start): prevent false positives for transitively resolved imports…#6722
Merged
schiller-manuel merged 4 commits intomainfrom Feb 21, 2026
Merged
fix(start): prevent false positives for transitively resolved imports…#6722schiller-manuel merged 4 commits intomainfrom
schiller-manuel merged 4 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds import-protection features and tests: new violation routes and helpers, expanded e2e build/dev capture with warm/cold passes, major import-protection plugin refactor (tracing, deferral, caching), improved source-location and mock generation, and extensive unit test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Browser
participant DevServer
participant Plugin
participant TransformCache
Tester->>DevServer: Start dev server (BEHAVIOR=error)
DevServer->>Plugin: register entries / buildStart
Plugin->>TransformCache: init state (entryFiles, serverFnLookupModules,...)
Browser->>DevServer: Navigate to routes (/, /beforeload-leak, ...)
DevServer->>Plugin: resolveId / load / transform hooks
Plugin->>Plugin: buildTraceFromModuleGraph()
Plugin->>TransformCache: store postTransformImports & transform results
Plugin->>Plugin: processPendingViolations() / checkPostTransformReachability()
alt Violation found
Plugin->>Plugin: buildViolationInfo()
Plugin->>Plugin: defer or emit violation (pendingViolations)
Plugin->>DevServer: emit runtime diagnostic or mock module
end
Browser->>Tester: Logs collected
Tester->>DevServer: Repeat warm pass (reuse cache)
DevServer->>Plugin: faster resolution using cached traces
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit f868ef4
☁️ Nx Cloud last updated this comment at |
![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: 4
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/tests/error-mode.setup.ts (1)
67-98:⚠️ Potential issue | 🟡 MinorAvoid
anyin exec error handling.This bypasses strict typing and can hide shape mismatches. Prefer
unknownwith a narrow cast.💡 Suggested tweak
- } catch (err: any) { - exitCode = err.status ?? 1 - stdout = err.stdout ?? '' - stderr = err.stderr ?? '' - } + } catch (err: unknown) { + const e = err as { status?: number; stdout?: string; stderr?: string } + exitCode = e.status ?? 1 + stdout = e.stdout ?? '' + stderr = e.stderr ?? '' + }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/error-mode.setup.ts` around lines 67 - 98, In captureBuild, change the catch from "err: any" to "err: unknown" and narrow it before reading status/stdout/stderr; e.g., import or reference the Node type ExecException from 'child_process' (or perform a type guard like typeof err === 'object' && err !== null && 'status' in err) then cast to ExecException to set exitCode, stdout, stderr, falling back to safe defaults if the shape doesn't match—update the catch block to use this narrowed value instead of any to keep strict typing.
🧹 Nitpick comments (1)
packages/start-plugin-core/src/import-protection-plugin/utils.ts (1)
71-78: Verify boundary condition logic inrelativizePath.The check
!Number.isNaN(ch)appears intended to handle the case wherep.length === root.length(whencharCodeAtreturnsNaN). However,charCodeAtreturnsNaNonly when the index is out of bounds, andNaN !== 47will always be true. The current logic correctly handles:
p === root→ returnsp.slice(root.length)(empty string)p === root + "/"→ returnsp.slice(root.length + 1)(empty string)p === root + "-extra/..."→ returns unchanged (correct rejection)The logic works correctly, though the
!Number.isNaN(ch)check is a bit subtle. Consider adding a brief inline comment clarifying thatNaNoccurs whenp.length === root.length.📝 Optional: Add clarifying comment
export function relativizePath(p: string, root: string): string { if (!p.startsWith(root)) return p const ch = p.charCodeAt(root.length) - // Must be followed by a separator or end-of-string to be a true child + // Must be followed by a separator (/) or end-of-string (NaN when index OOB) to be a true child if (ch !== 47 && !Number.isNaN(ch)) return p return ch === 47 ? p.slice(root.length + 1) : p.slice(root.length) }🤖 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/utils.ts` around lines 71 - 78, The boundary check in relativizePath uses charCodeAt(root.length) and relies on NaN when p.length === root.length, which is subtle; update the function comment or add a one-line inline comment next to the charCodeAt call (inside relativizePath) clarifying that charCodeAt returns NaN when the index equals p.length and that the subsequent logic treats NaN as "end-of-string" so we correctly return an empty relative path for p === root; keep the current condition behavior but add that explanatory note referencing relativizePath and the charCodeAt(root.length) usage.
🤖 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/error-mode.setup.ts`:
- Around line 120-167: The test currently calls out.end() then immediately reads
logFile which can race because the write stream 'out' may not have flushed;
change the teardown to wait for the stream to finish before reading/writing
results: after killChild(child) invoke out.end() and await the stream's 'finish'
event (or await a Promise that resolves in out.end callback) so that the
combined = fs.readFileSync(logFile, 'utf-8') occurs only after out has fully
flushed; reference the existing symbols out, child, killChild, logFile, outFile
and ensure the await happens in the finally block before reading and writing the
JSON.
In `@e2e/react-start/import-protection/tests/error-mode.spec.ts`:
- Around line 7-17: The dynamic JSON import in readResult currently uses an
unsafe cast ("as any"); instead declare a typed import shape (e.g., an interface
like ErrorResultModule { default: ErrorResult }) and use that type for the
imported module or the import assertion promise so mod is strongly typed, then
return mod.default as ErrorResult; update the import call (and any local typing)
to remove "as any" and rely on the declared ErrorResultModule type to satisfy
TypeScript strict mode.
In `@e2e/react-start/import-protection/tests/violations.setup.ts`:
- Around line 106-131: The runDevPass function currently declares its return
type as Array<any>; change it to the concrete type returned by
extractViolationsFromLog to preserve type safety (e.g. use ReturnType<typeof
extractViolationsFromLog> or Array<Violation>). Update the signature of
runDevPass to return that precise type and ensure any callers still align with
the tighter type; locate the function definition named runDevPass and the call
to extractViolationsFromLog(text) to make the replacement.
In `@packages/start-plugin-core/tests/importProtection/virtualModules.test.ts`:
- Around line 2-13: The import list in virtualModules.test.ts includes an unused
symbol RESOLVED_MOCK_MODULE_ID and the members are not alphabetized; remove
RESOLVED_MOCK_MODULE_ID from the import statement and reorder the remaining
imported names (e.g., MOCK_EDGE_PREFIX, MOCK_MODULE_ID, MOCK_RUNTIME_PREFIX,
loadMarkerModule, loadMockEdgeModule, loadMockRuntimeModule,
loadSilentMockModule, makeMockEdgeModule, mockRuntimeModuleIdFromViolation) into
alphabetical order to satisfy the linter and eliminate the unused import.
---
Outside diff comments:
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Around line 67-98: In captureBuild, change the catch from "err: any" to "err:
unknown" and narrow it before reading status/stdout/stderr; e.g., import or
reference the Node type ExecException from 'child_process' (or perform a type
guard like typeof err === 'object' && err !== null && 'status' in err) then cast
to ExecException to set exitCode, stdout, stderr, falling back to safe defaults
if the shape doesn't match—update the catch block to use this narrowed value
instead of any to keep strict typing.
---
Nitpick comments:
In `@packages/start-plugin-core/src/import-protection-plugin/utils.ts`:
- Around line 71-78: The boundary check in relativizePath uses
charCodeAt(root.length) and relies on NaN when p.length === root.length, which
is subtle; update the function comment or add a one-line inline comment next to
the charCodeAt call (inside relativizePath) clarifying that charCodeAt returns
NaN when the index equals p.length and that the subsequent logic treats NaN as
"end-of-string" so we correctly return an empty relative path for p === root;
keep the current condition behavior but add that explanatory note referencing
relativizePath and the charCodeAt(root.length) usage.
packages/start-plugin-core/tests/importProtection/virtualModules.test.ts
Show resolved
Hide resolved
schiller-manuel
force-pushed
the
fix-import-protection-false-pos
branch
from
1f7c6a0 to
2c4da32
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/start-plugin-core/src/import-protection-plugin/plugin.ts (1)
1477-1498: Consider performance impact of resolve calls in transform hook.The transform hook now resolves all import sources to populate
postTransformImports. While individual resolves are cached at the Vite level, this adds N resolve calls per transformed file. For large codebases, this could impact HMR latency.The try-catch handling is appropriate for making these resolves non-fatal.
🤖 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 1477 - 1498, The transform hook is calling this.resolve for every import source which can add N resolve calls per file; restrict resolves to likely-local imports only (e.g., only when src starts with "./", "../" or "/") before invoking this.resolve to avoid unnecessary work for bare/external imports, keep the existing try/catch, continue to call normalizeFilePath and envState.graph.addEdge for resolved results, and still set envState.postTransformImports(cacheKey) and file when done and call processPendingViolations as before.
🤖 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/error-mode.setup.ts`:
- Around line 87-90: The catch currently types the error as any; change the
catch parameter to unknown and narrow it to ExecSyncException (or check for the
presence of stdout/stderr) before assigning exitCode, stdout, and stderr: use
"catch (err: unknown)" then if (isExecSyncException(err)) or check typeof (err
=== 'object' && err !== null && 'status' in err) to extract status, stdout,
stderr safely, converting Buffer values to strings (e.g., Buffer.isBuffer(value)
? value.toString() : String(value)) and defaulting to 1/'' when properties are
absent; reference the catch variable err and the assignment targets exitCode,
stdout, stderr and the ExecSyncException shape when implementing the narrowing.
---
Duplicate comments:
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Around line 160-167: The teardown closes the dev log stream with out.end() and
immediately reads logFile, which can race because out.end() is asynchronous;
update the teardown around killChild(child) / out.end() so you wait for the
stream to fully finish before reading/writing logs (e.g., await stream finished
via stream.finished or wrap out.end() in a Promise) and then read logFile and
write outFile; keep references to killChild, out.end, logFile, outFile and
ensure the stream is resolved before fs.readFileSync/fs.writeFileSync.
---
Nitpick comments:
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 1477-1498: The transform hook is calling this.resolve for every
import source which can add N resolve calls per file; restrict resolves to
likely-local imports only (e.g., only when src starts with "./", "../" or "/")
before invoking this.resolve to avoid unnecessary work for bare/external
imports, keep the existing try/catch, continue to call normalizeFilePath and
envState.graph.addEdge for resolved results, and still set
envState.postTransformImports(cacheKey) and file when done and call
processPendingViolations as before.
Comment on lines
87
to
90
| } 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:
#!/bin/bash
# Check tsconfig for strict mode
echo "=== Checking tsconfig files for strict mode ==="
find . -name 'tsconfig*.json' -type f | head -20 | while read f; do
echo "File: $f"
cat "$f" | jq '.compilerOptions.strict' 2>/dev/null || grep -i '"strict"' "$f" || echo "Not found in $f"
done
echo ""
echo "=== Checking eslintrc for no-explicit-any ==="
find . -name '.eslintrc*' -o -name 'eslint.config.*' | head -10 | while read f; do
if [ -f "$f" ]; then
echo "File: $f"
cat "$f" | grep -i "no-explicit-any" || echo "no-explicit-any rule not found in $f"
fi
doneRepository: TanStack/router
Length of output: 2594
🏁 Script executed:
#!/bin/bash
# Read the actual file to understand context
echo "=== Reading error-mode.setup.ts around lines 87-90 ==="
if [ -f "e2e/react-start/import-protection/tests/error-mode.setup.ts" ]; then
wc -l "e2e/react-start/import-protection/tests/error-mode.setup.ts"
echo ""
sed -n '70,100p' "e2e/react-start/import-protection/tests/error-mode.setup.ts"
else
find . -name "error-mode.setup.ts" -type f
fiRepository: TanStack/router
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Find tsconfig for react-start project
echo "=== Looking for tsconfig in react-start ==="
find e2e/react-start -name 'tsconfig*.json' -type f
echo ""
echo "=== Looking for eslintrc in react-start ==="
find e2e/react-start -name '.eslintrc*' -o -name 'eslint.config.*'
echo ""
echo "=== Check parent/root tsconfig ==="
if [ -f "tsconfig.json" ]; then
echo "Root tsconfig.json:"
cat tsconfig.json | jq '.compilerOptions.strict' 2>/dev/null || grep -A5 '"compilerOptions"' tsconfig.json | head -20
fiRepository: TanStack/router
Length of output: 1400
🏁 Script executed:
#!/bin/bash
# Verify ExecSyncException type - check Node.js types
echo "=== Checking Node version and available types ==="
node --version
echo ""
echo "=== Verify execSync throws ExecSyncException when encoding is specified ==="
# Create a simple test to understand the type
python3 << 'EOF'
import json
# From Node.js documentation, when execSync fails:
# - It throws an Error-like object with properties: status, signal, stdout, stderr, pid
# - With encoding: 'utf-8', stdout and stderr are strings
# - Without encoding, they are Buffers
# Let's verify the suggested fix makes sense
exec_sync_exception = {
"extends": "Error",
"properties": {
"status": "number | null",
"signal": "string | null",
"stdout": "string | Buffer", # Can be either depending on encoding
"stderr": "string | Buffer" # Can be either depending on encoding
}
}
print("ExecSyncException properties:")
print(json.dumps(exec_sync_exception, indent=2))
print("\nIn this code, encoding is 'utf-8', so stdout/stderr should be strings")
print("However, when catching as unknown/any, we should handle both types for safety")
EOFRepository: TanStack/router
Length of output: 547
🏁 Script executed:
#!/bin/bash
# Look for the actual type definitions or usage patterns in the repo
echo "=== Checking if ExecSyncException is imported/used elsewhere ==="
rg -n 'ExecSyncException|execSync' --type ts --type tsx | head -20
echo ""
echo "=== Check the import statement for child_process ==="
sed -n '1,30p' e2e/react-start/import-protection/tests/error-mode.setup.tsRepository: TanStack/router
Length of output: 1453
🏁 Script executed:
#!/bin/bash
# Check if ExecSyncException is available in Node.js types
echo "=== Checking Node.js child_process types ==="
node -e "
const cp = require('child_process');
console.log('child_process methods:', Object.keys(cp).slice(0, 15));
"
echo ""
echo "=== Check `@types/node` for ExecSyncException ==="
find . -path '*/node_modules/@types/node*' -name '*.d.ts' 2>/dev/null | head -5 | while read f; do
if grep -l 'ExecSyncException' "$f" 2>/dev/null; then
echo "Found ExecSyncException in: $f"
grep -A8 'interface ExecSyncException\|class ExecSyncException' "$f"
fi
done
echo ""
echo "=== Alternative: Check what the proper narrowing should be ==="
echo "Since ExecSyncException may not be directly exported, let's see available options:"
rg -n 'ExecException|ExecFileException|SpawnException' . --type ts 2>/dev/null | head -10Repository: TanStack/router
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Verify the actual type handling needed
echo "=== Checking how stdout/stderr are actually assigned with encoding ==="
echo "The code has: encoding: 'utf-8'"
echo "This means stdout should be string when successful"
echo "When execSync throws, err.stdout and err.stderr are Buffer | string"
echo ""
echo "The suggested fix attempts to handle both Buffer and string:"
echo "- Type narrowing approach is correct"
echo "- Using 'unknown' in catch is correct for TypeScript strictness"
echo "- Need to verify the exact type import/usage"
echo ""
echo "=== Check if there's a pattern in the codebase for error handling ==="
rg -B3 -A5 'catch.*err.*unknown' . --type ts | head -30Repository: TanStack/router
Length of output: 2190
🏁 Script executed:
#!/bin/bash
# Final check - verify the best way to reference ExecSyncException
echo "=== How to properly type the error ==="
python3 << 'EOF'
# In TypeScript with `@types/node`, ExecSyncException is in the child_process module
# The import syntax used in the suggestion is valid:
# import('node:child_process').ExecSyncException
# Or alternatively:
# 1. Direct import at top: import type { ExecSyncException } from 'node:child_process'
# 2. Or cast as Error and access properties
# The suggested approach is valid but let's verify it works
print("Suggested approach: import('node:child_process').ExecSyncException")
print("This is the inline import type syntax, which is valid in TypeScript")
print("")
print("The suggested fix is correct:")
print("1. Catch as 'unknown' for strictness")
print("2. Cast/narrow to ExecSyncException")
print("3. Type-check stdout/stderr before using (since they could be Buffer | string)")
EOFRepository: TanStack/router
Length of output: 415
Replace any in the catch block with proper type narrowing to match strict mode requirements.
With strict: true enabled in tsconfig, the catch parameter should use unknown and be narrowed to the appropriate error type. ExecSyncException has stdout and stderr as Buffer | string, so they need type checking before use.
♻️ Suggested refactor
- } catch (err: any) {
- exitCode = err.status ?? 1
- stdout = err.stdout ?? ''
- stderr = err.stderr ?? ''
- }
+ } catch (err: unknown) {
+ const execErr = err as import('node:child_process').ExecSyncException
+ exitCode = execErr.status ?? 1
+ stdout =
+ typeof execErr.stdout === 'string'
+ ? execErr.stdout
+ : execErr.stdout?.toString() ?? ''
+ stderr =
+ typeof execErr.stderr === 'string'
+ ? execErr.stderr
+ : execErr.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 execErr = err as import('node:child_process').ExecSyncException | |
| exitCode = execErr.status ?? 1 | |
| stdout = | |
| typeof execErr.stdout === 'string' | |
| ? execErr.stdout | |
| : execErr.stdout?.toString() ?? '' | |
| stderr = | |
| typeof execErr.stderr === 'string' | |
| ? execErr.stderr | |
| : execErr.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 87
- 90, The catch currently types the error as any; change the catch parameter to
unknown and narrow it to ExecSyncException (or check for the presence of
stdout/stderr) before assigning exitCode, stdout, and stderr: use "catch (err:
unknown)" then if (isExecSyncException(err)) or check typeof (err === 'object'
&& err !== null && 'status' in err) to extract status, stdout, stderr safely,
converting Buffer values to strings (e.g., Buffer.isBuffer(value) ?
value.toString() : String(value)) and defaulting to 1/'' when properties are
absent; reference the catch variable err and the assignment targets exitCode,
stdout, stderr and the ExecSyncException shape when implementing the narrowing.
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/react-start/import-protection/tests/error-mode.setup.ts (1)
22-65: Consider extracting shared utilities to reduce duplication.The
waitForHttpOkandkillChildfunctions are duplicated between this file andviolations.setup.ts. Consider extracting them into a shared utility module (e.g.,test-helpers.ts) to improve maintainability.🤖 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 22 - 65, Extract the duplicated waitForHttpOk and killChild functions into a shared test helper module (e.g., create test-helpers.ts), move both implementations there, export them, and update this file and violations.setup.ts to import { waitForHttpOk, killChild } from that module; ensure the helper exports correct types (use Node's ChildProcess/ReturnType<typeof spawn> typing or re-export the spawn type if needed) and remove the duplicate implementations from both files so they reference the single shared utility.e2e/react-start/import-protection/tests/error-mode.spec.ts (1)
71-77: Consider adding a runtime guard for stricter type safety.While the
expect(violations.length).toBeGreaterThanOrEqual(1)assertion ensuresviolations[0]exists at runtime, TypeScript's strict mode doesn't narrow the type after anexpect()call. Adding a guard would make the code more explicit and satisfy strict array access checks.♻️ Optional improvement
expect(violations.length).toBeGreaterThanOrEqual(1) const v = violations[0] + if (!v) throw new Error('Expected at least one violation') expect(v.envType).toMatch(/^(client|server)$/)🤖 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 71 - 77, Add an explicit runtime guard before indexing into violations to satisfy TypeScript strict checks: after the existing expect for length, check violations.length and throw or call fail if it is 0 so that the subsequent const v = violations[0] (used in the test block) is type-safe; update the test around the violations variable and the v constant to include this guard (e.g., if (violations.length === 0) throw new Error('expected at least one violation')) so TS knows violations[0] is non-empty at the point of use.packages/start-plugin-core/tests/importProtection/virtualModules.test.ts (1)
129-210: Add explicit payload types when decoding JSON.
JSON.parsereturnsany, which weakens strict type safety in these tests. Consider casting to explicit payload types for runtime and edge payloads so accidental typos don’t slip through.♻️ Suggested type-safe decoding
describe('mockRuntimeModuleIdFromViolation', () => { + type RuntimePayload = { + env: string + mode: string + specifier?: string + trace?: string[] + } const baseViolation: ViolationInfo = { @@ - const payload = JSON.parse( - Buffer.from(encoded, 'base64url').toString('utf8'), - ) + const payload = JSON.parse( + Buffer.from(encoded, 'base64url').toString('utf8'), + ) as RuntimePayload @@ - const payload = JSON.parse( - Buffer.from(encoded, 'base64url').toString('utf8'), - ) + const payload = JSON.parse( + Buffer.from(encoded, 'base64url').toString('utf8'), + ) as RuntimePayload @@ - const payload = JSON.parse( - Buffer.from(encoded, 'base64url').toString('utf8'), - ) + const payload = JSON.parse( + Buffer.from(encoded, 'base64url').toString('utf8'), + ) as RuntimePayload }) describe('makeMockEdgeModuleId', () => { + type EdgePayload = { source: string; exports: string[]; runtimeId: string } @@ - const payload = JSON.parse( - Buffer.from(encoded, 'base64url').toString('utf8'), - ) + const payload = JSON.parse( + Buffer.from(encoded, 'base64url').toString('utf8'), + ) as EdgePayload @@ - const payload = JSON.parse( - Buffer.from(encoded, 'base64url').toString('utf8'), - ) + const payload = JSON.parse( + Buffer.from(encoded, 'base64url').toString('utf8'), + ) as EdgePayload })As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 214-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/importProtection/virtualModules.test.ts` around lines 129 - 210, The tests decode a base64url JSON payload from mockRuntimeModuleIdFromViolation and assign it to untyped variables via JSON.parse, weakening strict typing; define explicit payload interfaces (e.g., RuntimePayload/EdgePayload including env, mode, specifier, trace, etc.) and cast the decoded JSON to those types when parsing the encoded string (use JSON.parse(... ) as RuntimePayload or a type-safe parse via unknown first) for every place where payload is derived from MOCK_RUNTIME_PREFIX (references: mockRuntimeModuleIdFromViolation and MOCK_RUNTIME_PREFIX, and the test assertions that read payload.env, payload.mode, payload.specifier, payload.trace) so the tests enforce correct property names and types.
🤖 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/error-mode.setup.ts`:
- Around line 87-91: The catch currently types err as any; narrow it to the
child_process ExecSyncException type so strict mode is satisfied: import or
reference ExecSyncException and use a type guard (e.g., isExecSyncException) or
an inline discriminant check on err.status/err.stdout/err.stderr to assert the
ExecSyncException shape, then assign exitCode = err.status ?? 1, stdout =
err.stdout ?? '', stderr = err.stderr ?? ''; ensure the guard falls back to a
safe default for unknown errors so no any is used in the catch block.
---
Nitpick comments:
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Around line 22-65: Extract the duplicated waitForHttpOk and killChild
functions into a shared test helper module (e.g., create test-helpers.ts), move
both implementations there, export them, and update this file and
violations.setup.ts to import { waitForHttpOk, killChild } from that module;
ensure the helper exports correct types (use Node's
ChildProcess/ReturnType<typeof spawn> typing or re-export the spawn type if
needed) and remove the duplicate implementations from both files so they
reference the single shared utility.
In `@e2e/react-start/import-protection/tests/error-mode.spec.ts`:
- Around line 71-77: Add an explicit runtime guard before indexing into
violations to satisfy TypeScript strict checks: after the existing expect for
length, check violations.length and throw or call fail if it is 0 so that the
subsequent const v = violations[0] (used in the test block) is type-safe; update
the test around the violations variable and the v constant to include this guard
(e.g., if (violations.length === 0) throw new Error('expected at least one
violation')) so TS knows violations[0] is non-empty at the point of use.
In `@packages/start-plugin-core/tests/importProtection/virtualModules.test.ts`:
- Around line 129-210: The tests decode a base64url JSON payload from
mockRuntimeModuleIdFromViolation and assign it to untyped variables via
JSON.parse, weakening strict typing; define explicit payload interfaces (e.g.,
RuntimePayload/EdgePayload including env, mode, specifier, trace, etc.) and cast
the decoded JSON to those types when parsing the encoded string (use
JSON.parse(... ) as RuntimePayload or a type-safe parse via unknown first) for
every place where payload is derived from MOCK_RUNTIME_PREFIX (references:
mockRuntimeModuleIdFromViolation and MOCK_RUNTIME_PREFIX, and the test
assertions that read payload.env, payload.mode, payload.specifier,
payload.trace) so the tests enforce correct property names and types.
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.
… in dev mode
Summary by CodeRabbit
New Features
Tests
Chores