fix: import protection robustness#6746
Conversation
|
View your CI Pipeline Execution ↗ for commit 755a654
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds build-time deferred violation handling to import-protection: the plugin emits per-violation build mock modules, records deferredBuildViolations, and verifies tree-shaking in generateBundle to report only real (non-tree-shaken) violations. Also adds e2e tests, fixtures, docs, types, virtual-module constants, and async-capable onViolation handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as "Dev/Build Runner"
participant Resolver as "plugin.resolveId/transform"
participant Loader as "plugin.load/transform-cache"
participant Bundler as "Vite Bundler"
participant Generator as "plugin.generateBundle"
participant Reporter as "Violation Reporter"
Runner->>Resolver: encounter import (specifier, importer)
Resolver->>Resolver: detect potential import-protection violation
alt Build mode
Resolver->>Resolver: create DeferredBuildViolation with unique mockModuleId
Resolver->>Loader: return resolved mockModuleId to satisfy import
Runner->>Bundler: include mock module in bundle graph
Note over Resolver,Generator: deferredBuildViolations recorded in EnvState
else Dev mode
Resolver->>Reporter: report or defer violation immediately (dev path)
Resolver->>Loader: return dev-time mock or original behavior
end
Bundler->>Generator: after bundle generation, invoke generateBundle
Generator->>Generator: collect emitted module IDs and map mockModuleIds
alt Mock survived (real violation)
Generator->>Reporter: emit build-time warning/error for real violation
else Mock tree-shaken (false positive)
Generator->>Reporter: no report for this deferred violation
end
Reporter-->>Runner: final build status (warnings/errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
1904b5c to
7c58156
Compare
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)
packages/start-plugin-core/src/import-protection-plugin/plugin.ts (1)
561-587:⚠️ Potential issue | 🟠 MajorClear deferred/pending violation state (and reset the counter) at build start.
deferredBuildViolations(andpendingViolations) are never cleared, so watch builds can accumulate stale entries and grow unbounded. Reset them inbuildStart, and reset the build mock counter so IDs don’t grow forever.🛠️ Suggested fix (in buildStart)
for (const envState of envStates.values()) { envState.resolveCache.clear() envState.resolveCacheByFile.clear() envState.importLocCache.clear() envState.seenViolations.clear() envState.transformResultCache.clear() envState.transformResultKeysByFile.clear() envState.postTransformImports.clear() envState.serverFnLookupModules.clear() envState.graph.clear() envState.deniedSources.clear() envState.deniedEdges.clear() envState.mockExportsByImporter.clear() + envState.pendingViolations.clear() + envState.deferredBuildViolations.length = 0 } + + buildViolationCounter = 0🤖 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 561 - 587, At build start (inside the buildStart function) clear accumulated violation state to avoid unbounded growth by resetting envState.deferredBuildViolations = [] and envState.pendingViolations.clear(); also reset the build mock id counter (buildMockCounter = 0) so generated mock IDs don’t grow forever; locate envState and the buildStart implementation and add these resets there (and ensure envState is defined before you clear these fields).
🤖 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/barrel-false-positive.tsx`:
- Line 8: The import mixes value and type imports which violates
import/consistent-type-specifier-style and sort-imports; split into two imports
from '../violations/barrel-reexport': keep the value imports "getUsers" and
"userColumns" together (sorted) in one statement and add a separate type-only
import "import type { User } from '../violations/barrel-reexport'"; ensure the
value import appears before the type-only import to satisfy ordering.
In
`@e2e/react-start/import-protection/src/violations/barrel-reexport/db.server.ts`:
- Around line 7-10: The current side-effect log prints the full DATABASE_URL
(variable DATABASE_URL) which may contain credentials; change the console.log
call in this module to avoid exposing secrets by either logging a redacted
connection string (mask username/password from DATABASE_URL before logging) or
simply logging a static/redacted message (e.g., "[db] connecting (redacted)") so
the module load remains detectable without emitting sensitive data; update the
console.log usage accordingly and keep the DATABASE_URL value assignment
unchanged.
In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md`:
- Around line 53-60: The fenced code blocks in INTERNALS.md are missing language
identifiers (MD040); update the three examples so the first code fence around
the db/index.ts / route.tsx snippet uses a language like "text" and the second
fence around the shouldDefer example uses "ts". Locate the blocks containing the
db/index.ts → export { getUsers } / route.tsx → import { getUsers, userColumns }
snippet and the block with "shouldDefer = (isDevMock && !isPreTransformResolve)
|| isBuild" and change their opening backticks to ```text and ```ts respectively
so markdownlint stops flagging missing languages.
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 1450-1494: Deferred build violations skip the configured
onViolation hook; update generateBundle to invoke config.onViolation(info) for
each real violation and only proceed to this.error or this.warn if the hook does
not indicate suppression. Specifically, inside generateBundle where
realViolations is built, iterate realViolations and call
config.onViolation(info) (or await if it's async) and filter out suppressed
entries before: (a) in error mode, calling this.error(formatViolation(...)) on
the first non-suppressed violation; and (b) in mock/warn mode, deduplicating via
dedupeKey and calling this.warn(formatViolation(...)) only for non-suppressed,
unseen violations; keep existing references to env.deferredBuildViolations,
formatViolation, dedupeKey, this.error and this.warn.
---
Outside diff comments:
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 561-587: At build start (inside the buildStart function) clear
accumulated violation state to avoid unbounded growth by resetting
envState.deferredBuildViolations = [] and envState.pendingViolations.clear();
also reset the build mock id counter (buildMockCounter = 0) so generated mock
IDs don’t grow forever; locate envState and the buildStart implementation and
add these resets there (and ensure envState is defined before you clear these
fields).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/start/framework/react/guide/import-protection.mde2e/react-start/import-protection/src/routes/__root.tsxe2e/react-start/import-protection/src/routes/barrel-false-positive.tsxe2e/react-start/import-protection/src/routes/component-server-leak.tsxe2e/react-start/import-protection/src/violations/barrel-reexport/db.server.tse2e/react-start/import-protection/src/violations/barrel-reexport/foo.tse2e/react-start/import-protection/src/violations/barrel-reexport/index.tse2e/react-start/import-protection/src/violations/barrel-reexport/shared.tse2e/react-start/import-protection/src/violations/db-credentials.server.tse2e/react-start/import-protection/tests/error-mode.setup.tse2e/react-start/import-protection/tests/import-protection.spec.tse2e/react-start/import-protection/tests/violations.setup.tspackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/import-protection-plugin/virtualModules.ts
| // `userColumns` (from ./shared) is a plain object used in JSX — not server-only. | ||
| // Tree-shaking should eliminate the ./db.server dependency entirely from the | ||
| // client bundle, so no import-protection violation should fire for it. | ||
| import { getUsers, userColumns, type User } from '../violations/barrel-reexport' |
There was a problem hiding this comment.
Split the type-only import and keep value imports sorted.
ESLint flags inline type specifiers and ordering here. Separate User into a type-only import to satisfy import/consistent-type-specifier-style and sort-imports.
🧹 Suggested fix
-import { getUsers, userColumns, type User } from '../violations/barrel-reexport'
+import { getUsers, userColumns } from '../violations/barrel-reexport'
+import type { User } from '../violations/barrel-reexport'🧰 Tools
🪛 ESLint
[error] 8-8: Member 'User' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 8-8: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
🤖 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/barrel-false-positive.tsx` at
line 8, The import mixes value and type imports which violates
import/consistent-type-specifier-style and sort-imports; split into two imports
from '../violations/barrel-reexport': keep the value imports "getUsers" and
"userColumns" together (sorted) in one statement and add a separate type-only
import "import type { User } from '../violations/barrel-reexport'"; ensure the
value import appears before the type-only import to satisfy ordering.
| const DATABASE_URL = | ||
| process.env.DATABASE_URL ?? 'postgres://admin:s3cret@localhost:5432/myapp' | ||
|
|
||
| console.log(`[db] connecting to ${DATABASE_URL}`) |
There was a problem hiding this comment.
Avoid logging credentials — use a redacted connection string in the side-effect log.
The default fallback on line 8 embeds plaintext credentials (admin:s3cret), and line 10 immediately logs the full DATABASE_URL (credentials and all). While these are fake credentials, this fixture is documentation-level example code. The pattern is problematic in two ways:
- If
process.env.DATABASE_URLis populated with real credentials in CI, they would be emitted to build/test logs. - It models the anti-pattern of logging raw connection strings to readers who may copy this pattern.
Since the only requirement of the side-effect is that it be detectable (i.e., proves the module was loaded), logging a redacted or static string is sufficient.
🔒 Proposed fix
-console.log(`[db] connecting to ${DATABASE_URL}`)
+// Log a redacted URL so credentials are never emitted even if the env var is real.
+console.log(`[db] connecting to ${DATABASE_URL.replace(/:\/\/[^@]+@/, '://<redacted>@')}`)📝 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.
| const DATABASE_URL = | |
| process.env.DATABASE_URL ?? 'postgres://admin:s3cret@localhost:5432/myapp' | |
| console.log(`[db] connecting to ${DATABASE_URL}`) | |
| const DATABASE_URL = | |
| process.env.DATABASE_URL ?? 'postgres://admin:s3cret@localhost:5432/myapp' | |
| // Log a redacted URL so credentials are never emitted even if the env var is real. | |
| console.log(`[db] connecting to ${DATABASE_URL.replace(/:\/\/[^@]+@/, '://<redacted>@')}`) |
🤖 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/barrel-reexport/db.server.ts`
around lines 7 - 10, The current side-effect log prints the full DATABASE_URL
(variable DATABASE_URL) which may contain credentials; change the console.log
call in this module to avoid exposing secrets by either logging a redacted
connection string (mask username/password from DATABASE_URL before logging) or
simply logging a static/redacted message (e.g., "[db] connecting (redacted)") so
the module load remains detectable without emitting sensitive data; update the
console.log usage accordingly and keep the DATABASE_URL value assignment
unchanged.
packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md
Outdated
Show resolved
Hide resolved
packages/start-plugin-core/src/import-protection-plugin/plugin.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e/react-start/import-protection/src/routes/barrel-false-positive.tsx (1)
1-8: Split type-only import to satisfy lint rules.
Line 8 matches a prior review note about separatingtype Userand sorting members.🤖 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/barrel-false-positive.tsx` around lines 1 - 8, Split the mixed import so the runtime values and the type are imported separately: keep the runtime import for getUsers and userColumns as a plain import (import { getUsers, userColumns } from '.../barrel-reexport') and add a separate type-only import for User using the TypeScript syntax (import type { User } from '.../barrel-reexport'); also sort the named imports alphabetically in each statement if your lint requires deterministic ordering.packages/start-plugin-core/src/import-protection-plugin/plugin.ts (1)
1471-1489: HonoronViolationin build-mode reportingBuild-mode
generateBundlestill reports every surviving violation without callingconfig.onViolation, so suppression/custom handling doesn’t work in build. Please apply the hook before erroring/warning.🤖 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 1471 - 1489, The build-phase reporting currently skips calling config.onViolation; update the generateBundle logic so that for every surviving violation in realViolations you invoke config.onViolation(info) (or await it if it can be async) before deciding to fail or warn: in the 'error' branch call config.onViolation(realViolations[0]) before this.error(formatViolation(...)), and in the mock/warn branch call config.onViolation(info) for each deduped info (before this.warn(formatViolation(...))); use the existing symbols realViolations, config.effectiveBehavior, config.onViolation, dedupeKey, formatViolation, this.error and this.warn to locate and modify the code.packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md (1)
53-60: Add language identifiers to fenced code blocks (MD040)The two fenced blocks are missing language tags. Use
textfor the path diagram andtsfor theshouldDefersnippet.Also applies to: 89-91
🤖 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 53 - 60, Add language identifiers to the markdown fenced code blocks: for the path/exports diagram that shows "db/index.ts → export { getUsers } ..." and "route.tsx → import { getUsers, userColumns } ..." add the text language tag (```text), and for the code snippet demonstrating shouldDefer (the TypeScript example later in the file) add the ts language tag (```ts); also update the other similar fenced block at lines ~89-91 to use the appropriate language tag so all fenced blocks include language identifiers.
🤖 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 585-586: The deferredBuildViolations array (declared as
deferredBuildViolations) is not cleared during rebuilds, causing stale entries
to persist; update the buildStart handler (the buildStart method/function that
currently clears per-environment caches and pendingViolations) to also reset
deferredBuildViolations = [] whenever you clear per-env caches so
deferredBuildViolations is fresh on each build/watch iteration.
---
Duplicate comments:
In `@e2e/react-start/import-protection/src/routes/barrel-false-positive.tsx`:
- Around line 1-8: Split the mixed import so the runtime values and the type are
imported separately: keep the runtime import for getUsers and userColumns as a
plain import (import { getUsers, userColumns } from '.../barrel-reexport') and
add a separate type-only import for User using the TypeScript syntax (import
type { User } from '.../barrel-reexport'); also sort the named imports
alphabetically in each statement if your lint requires deterministic ordering.
In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md`:
- Around line 53-60: Add language identifiers to the markdown fenced code
blocks: for the path/exports diagram that shows "db/index.ts → export { getUsers
} ..." and "route.tsx → import { getUsers, userColumns } ..." add the text
language tag (```text), and for the code snippet demonstrating shouldDefer (the
TypeScript example later in the file) add the ts language tag (```ts); also
update the other similar fenced block at lines ~89-91 to use the appropriate
language tag so all fenced blocks include language identifiers.
In `@packages/start-plugin-core/src/import-protection-plugin/plugin.ts`:
- Around line 1471-1489: The build-phase reporting currently skips calling
config.onViolation; update the generateBundle logic so that for every surviving
violation in realViolations you invoke config.onViolation(info) (or await it if
it can be async) before deciding to fail or warn: in the 'error' branch call
config.onViolation(realViolations[0]) before this.error(formatViolation(...)),
and in the mock/warn branch call config.onViolation(info) for each deduped info
(before this.warn(formatViolation(...))); use the existing symbols
realViolations, config.effectiveBehavior, config.onViolation, dedupeKey,
formatViolation, this.error and this.warn to locate and modify the code.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/start/framework/react/guide/import-protection.mde2e/react-start/import-protection/src/routes/__root.tsxe2e/react-start/import-protection/src/routes/barrel-false-positive.tsxe2e/react-start/import-protection/src/routes/component-server-leak.tsxe2e/react-start/import-protection/src/violations/barrel-reexport/db.server.tse2e/react-start/import-protection/src/violations/barrel-reexport/foo.tse2e/react-start/import-protection/src/violations/barrel-reexport/index.tse2e/react-start/import-protection/src/violations/barrel-reexport/shared.tse2e/react-start/import-protection/src/violations/db-credentials.server.tse2e/react-start/import-protection/tests/error-mode.setup.tse2e/react-start/import-protection/tests/import-protection.spec.tse2e/react-start/import-protection/tests/violations.setup.tspackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/import-protection-plugin/virtualModules.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/react-start/import-protection/tests/error-mode.setup.ts
- e2e/react-start/import-protection/src/violations/barrel-reexport/db.server.ts
- e2e/react-start/import-protection/tests/import-protection.spec.ts
- e2e/react-start/import-protection/src/violations/barrel-reexport/foo.ts
- packages/start-plugin-core/src/import-protection-plugin/virtualModules.ts
- e2e/react-start/import-protection/src/violations/barrel-reexport/shared.ts
- docs/start/framework/react/guide/import-protection.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/start-plugin-core/src/import-protection-plugin/plugin.ts (1)
1065-1086:deferredBuildViolationsandbuildViolationCounternot reset between builds.The
buildStartloop clears every per-env cache exceptdeferredBuildViolations(line 1086). In watch/rebuild scenarios, stale entries accumulate. BecausebuildViolationCounteris also never reset, the old mock IDs (from prior builds) won't appear in the new bundle's surviving modules — so there's no false-positive/negative — but the array grows as a memory leak.Both should be reset in
buildStart:🧹 Suggested fix
for (const envState of envStates.values()) { envState.resolveCache.clear() envState.resolveCacheByFile.clear() envState.importLocCache.clear() envState.seenViolations.clear() envState.transformResultCache.clear() envState.transformResultKeysByFile.clear() envState.postTransformImports.clear() envState.serverFnLookupModules.clear() envState.graph.clear() envState.deniedSources.clear() envState.deniedEdges.clear() envState.mockExportsByImporter.clear() + envState.deferredBuildViolations.length = 0 }And reset the counter alongside (line 1065 scope):
buildStart() { if (!config.enabled) return + buildViolationCounter = 0 // Clear memoization caches ...🤖 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 1065 - 1086, Reset the stale per-build state by clearing deferredBuildViolations and resetting buildViolationCounter at the start of each build: inside buildStart (alongside where you clear per-env caches and the envStates loop), ensure deferredBuildViolations is cleared (or assigned a fresh empty array) and buildViolationCounter is set back to 0 so previous-build entries do not accumulate and leak memory.
🤖 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 1065-1086: Reset the stale per-build state by clearing
deferredBuildViolations and resetting buildViolationCounter at the start of each
build: inside buildStart (alongside where you clear per-env caches and the
envStates loop), ensure deferredBuildViolations is cleared (or assigned a fresh
empty array) and buildViolationCounter is set back to 0 so previous-build
entries do not accumulate and leak memory.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/start/framework/react/guide/import-protection.mdpackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/plugin.tspackages/start-plugin-core/src/schema.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md (1)
1-290: LGTM — well-structured and technically accurate.The document clearly covers all key design axes, the false-positive problem and its mitigations, per-environment state management, and the build "mock-first, verify-later" pattern. The previously flagged MD040 fenced-code-language issues are resolved.
One minor optional nit on lines 125–126: the LanguageTool style hint flags three consecutive sentences beginning with "If" (
If reachable → confirm. If unreachable → discard. If unknown → keep pending.). The parallel structure is deliberate and idiomatic for exhaustive-condition documentation, but it could be reformatted as a nested bullet list for slightly better readability if desired.🧹 Optional: reformat as a nested list
- `processPendingViolations()` checks graph reachability from entry points - using only post-transform edges. If the violating importer is reachable - → confirm (warn). If unreachable → discard. If unknown → keep pending. + `processPendingViolations()` checks graph reachability from entry points + using only post-transform edges: + - Reachable → confirm (warn). + - Unreachable → discard. + - Unknown → keep pending.🤖 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 1 - 290, The three consecutive "If" sentences in the "Dev + Mock" section ("If reachable → confirm. If unreachable → discard. If unknown → keep pending.") are flagged for stylistic repetition; reformat that fragment into a nested bullet list under the "Dev + Mock" heading (or under processPendingViolations() discussion) so each condition is a separate bullet with the outcome, preserving the exact meaning and brevity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md`:
- Around line 1-290: The three consecutive "If" sentences in the "Dev + Mock"
section ("If reachable → confirm. If unreachable → discard. If unknown → keep
pending.") are flagged for stylistic repetition; reformat that fragment into a
nested bullet list under the "Dev + Mock" heading (or under
processPendingViolations() discussion) so each condition is a separate bullet
with the outcome, preserving the exact meaning and brevity.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/plugin.ts
Summary by CodeRabbit
Documentation
New Features
Improvements
Tests