Forbid ../ in imports — use a # alias instead - #1872
Conversation
Add a new code-quality rule that fails any import using a parent-walking
relative specifier ("../", "./../", dynamic or static). Every top-level
dir (src, test, scripts, cli) now has a # alias in deno.json, so a file
never needs to know where it sits in the tree to name what it imports —
moved files keep working, and aliases make data dependencies obvious.
- New #scripts/ alias in deno.json (scripts/ was the only top-level dir
without one).
- Detect RELATIVE_PARENT_IMPORT_PATTERN + detectRelativeImport in
test/lib/code-quality/detectors.ts, with eight fixture-driven tests.
- Integration rule in test/lib/code-quality.test.ts scans src/, test/,
scripts/, cli/, plus .tsx templates, so the rule covers everything.
- Migrated ~140 existing ../ imports across ~110 files: src/ to #routes/
/ #templates/ / #src/, scripts/ to #scripts/ / #src/ / #test/,
test/scripts/ to #scripts/ / #test-utils/, and the remaining test/
paths to #test/ / #scripts/.
- Split the callsite/tokenizer tests out of detectors.test.ts into
callsite-scanners.test.ts so the file stays under Biome's 1,000-line
ceiling after adding the new detector tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request configures Deno import aliases, replaces parent-relative imports across runtime, tooling, end-to-end, and test code, adds Stripe webhook cleanup, and introduces repository-wide checks and tests that reject parent-walking relative imports. ChangesImport alias migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e89659905
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/code-quality/detectors.ts`:
- Around line 182-183: Update RELATIVE_PARENT_IMPORT_PATTERN to match bare
side-effect imports using an import followed directly by a quoted path, while
preserving the existing from and dynamic import matches. Add a regression case
in the detectors tests for import "../x.ts".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0fc1bd4b-8978-4612-bd77-5bed0da0f0ba
📒 Files selected for processing (123)
deno.jsonscripts/bench/bundle-composition/model.tsscripts/bench/bundle-composition/report.tsscripts/bench/cold-start/bundle-load.tsscripts/bench/cold-start/first-request-child.tsscripts/edge-bundle-lib.tsscripts/edge-cdn-assets.tsscripts/inline-jsquash-wasm.tsscripts/mutation/build-test-state.tsscripts/mutation/child-process.tsscripts/mutation/evaluate.tsscripts/mutation/execution.tsscripts/mutation/generate.tsscripts/mutation/isolation-state.tsscripts/mutation/isolation.tsscripts/mutation/run-file.tsscripts/mutation/runner.tsscripts/mutation/state-graph.tsscripts/mutation/summary.tsscripts/mutation/test-map.tsscripts/mutation/test-state.tsscripts/pr-queue/render.tsscripts/precommit/git.tsscripts/precommit/runner.tsscripts/precommit/steps.tsscripts/screenshots/server.tsscripts/static-assets/output-rollback.tsscripts/static-assets/session.tsscripts/stripe-mock/install.tsscripts/test-harness.tssrc/features/admin/catalog-transfer/export.tssrc/features/admin/catalog-transfer/import.tssrc/features/public/ticket-submit/parse.tssrc/features/public/ticket-submit/paths.tssrc/features/public/ticket-submit/prepare.tssrc/features/public/ticket-submit/pricing.tssrc/ui/client/admin/fill-default-template.tssrc/ui/client/admin/markdown-editor-toolbar.tssrc/ui/templates/public/reservations/child-block.tssrc/ui/templates/public/reservations/form.tsxsrc/ui/templates/public/reservations/listing-rows.tstest/integration/routes/api-book-payments.test.tstest/integration/server/built-sites-update.test.tstest/integration/server/reservation-edge-cases.test.tstest/integration/server/reservation-no-provider.test.tstest/lib/code-quality.test.tstest/lib/code-quality/callsite-scanners.test.tstest/lib/code-quality/detectors.test.tstest/lib/code-quality/detectors.tstest/lib/db/migration-restore/helpers.tstest/lib/db/migration-restore/verify.test.tstest/lib/db/migrations/2026-06-14_rename_events_to_listings.test.tstest/lib/mutation-state-graph.test.tstest/lib/server-parents-gate/helpers.tstest/lib/server-parents-gate/render-dates.test.tstest/lib/server-public/ticket-csrf-and-capacity.test.tstest/lib/server-public/ticket-slug-post.test.tstest/lib/server-webhooks/can-pay-more-multi-ticket.test.tstest/lib/stripe-mock/helpers.tstest/lib/stripe-mock/install.test.tstest/lib/stripe-mock/ports.test.tstest/lib/test-groups.test.tstest/scripts/bench/bundle-composition/javascript-ast.test.tstest/scripts/build-tag.test.tstest/scripts/bundle-composition.test.tstest/scripts/check-copy.test.tstest/scripts/cleanup.test.tstest/scripts/cold-start-strip.test.tstest/scripts/cold-start-support.test.tstest/scripts/compact-test-reporter.test.tstest/scripts/deno-command.test.tstest/scripts/deploy-edge.test.tstest/scripts/diff-code-lines.test.tstest/scripts/edge-bundle-modules.test.tstest/scripts/edge-cdn-assets.test.tstest/scripts/inline-jsquash-wasm.test.tstest/scripts/line-counts.test.tstest/scripts/mutation-args.test.tstest/scripts/mutation-batch.test.tstest/scripts/mutation-child-process.test.tstest/scripts/mutation-evaluate.test.tstest/scripts/mutation-execution.test.tstest/scripts/mutation-file-plan.test.tstest/scripts/mutation-generate.test.tstest/scripts/mutation-ignore.test.tstest/scripts/mutation-isolation-helpers.tstest/scripts/mutation-isolation-supervisor.test.tstest/scripts/mutation-isolation.test.tstest/scripts/mutation-phases.test.tstest/scripts/mutation-step.test.tstest/scripts/mutation-summary.test.tstest/scripts/mutation-test-map.test.tstest/scripts/mutation-test-state.test.tstest/scripts/pr-queue/buckets.test.tstest/scripts/pr-queue/checks.test.tstest/scripts/pr-queue/comments.test.tstest/scripts/pr-queue/fixtures.tstest/scripts/pr-queue/pagination.test.tstest/scripts/pr-queue/render.test.tstest/scripts/pr-queue/sanitize.test.tstest/scripts/precommit.test.tstest/scripts/process.test.tstest/scripts/project-root.test.tstest/scripts/screenshots-checks.test.tstest/scripts/screenshots-options.test.tstest/scripts/screenshots-scenario.test.tstest/scripts/screenshots-server.test.tstest/scripts/static-asset-build.test.tstest/scripts/static-cdn-config.test.tstest/scripts/static-cdn-fixtures.tstest/scripts/static-cdn-publish.test.tstest/scripts/stream-lines.test.tstest/scripts/test-coverage.test.tstest/scripts/test-durations.test.tstest/scripts/test-environment.test.tstest/scripts/unit-tests-report-fixtures.tstest/scripts/unit-tests-report-format.test.tstest/scripts/unit-tests-report-imports.test.tstest/scripts/unit-tests-report.test.tstest/setup.tstest/test-utils/db-helpers/built-sites.tstest/ui/templates/public/reservations/child-block.test.tstest/ui/templates/public/reservations/child-block/compat-data.test.ts
… e2e-payments - detector regex now matches bare `import "../x";` (anchored to line start to avoid false positives in string literals, e.g. test fixtures that assert on import statements as data) - added regression tests for side-effect form and the string-literal case - extended the rule to scan e2e-payments/ (added #e2e/ alias for sibling imports within the e2e workspace; providers/*.ts now use #e2e/ instead of ../)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e-payments/src/providers/stripe.ts (2)
44-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t log Stripe endpoint deletion as successful unless the DELETE succeeds.
e2e-payments/src/providers/stripe.ts:45-49 —fetchwon’t reject on HTTP 4xx/5xx, and the inner.catch(() => {})also hides network failures, so stale endpoints can fail to delete while still being reported as cleaned up. Checkres.ok(or throw on non-2xx), remove the swallow, and let the outer cleanup handler report the failure instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-payments/src/providers/stripe.ts` around lines 44 - 49, Update the stale endpoint cleanup loop in the Stripe provider to await the DELETE response, validate res.ok, and propagate both HTTP and network failures instead of swallowing them with the inner catch. Only log the “deleted stale Stripe webhook endpoint” message after a successful response, allowing the outer cleanup handler to report failures.Source: Coding guidelines
41-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMatch on the tunnel hostname, not a substring.
e.url?.includes("trycloudflare.com")can catch unrelated URLs with that string in the query and delete the wrong webhook endpoint; parsee.urland require the.trycloudflare.comhostname before deleting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-payments/src/providers/stripe.ts` around lines 41 - 43, Update the stale webhook filter in the Stripe provider to parse each e.url and match only when its hostname is the trycloudflare.com tunnel domain or a subdomain, rather than searching the full URL string. Preserve the existing handling for missing or invalid URLs and only mark matching tunnel endpoints for deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/code-quality.test.ts`:
- Around line 342-349: Update the file-collection logic in the code-quality
test, including ensureLoaded() and the scan sections around the Promise.all
calls, to collect and validate .tsx files from every in-scope tree: TEST_DIR,
SCRIPTS_DIR, CLI_DIR, and the other relevant directories. Use the same recursive
extension-aware collection used for SRC_DIR so forbidden imports in any in-scope
.tsx file cannot bypass the test.
---
Outside diff comments:
In `@e2e-payments/src/providers/stripe.ts`:
- Around line 44-49: Update the stale endpoint cleanup loop in the Stripe
provider to await the DELETE response, validate res.ok, and propagate both HTTP
and network failures instead of swallowing them with the inner catch. Only log
the “deleted stale Stripe webhook endpoint” message after a successful response,
allowing the outer cleanup handler to report failures.
- Around line 41-43: Update the stale webhook filter in the Stripe provider to
parse each e.url and match only when its hostname is the trycloudflare.com
tunnel domain or a subdomain, rather than searching the full URL string.
Preserve the existing handling for missing or invalid URLs and only mark
matching tunnel endpoints for deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e812eda4-c14e-405a-aa3d-00cc8648408a
📒 Files selected for processing (12)
e2e-payments/deno.jsone2e-payments/src/browser.tse2e-payments/src/providers/card.tse2e-payments/src/providers/index.tse2e-payments/src/providers/shared.tse2e-payments/src/providers/square.tse2e-payments/src/providers/stripe.tse2e-payments/src/providers/sumup.tse2e-payments/src/providers/types.tstest/lib/code-quality.test.tstest/lib/code-quality/detectors.test.tstest/lib/code-quality/detectors.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7e609d79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
main moved several integration tests from test/lib/db/ to test/integration/. The move wrote the imports for the new path using ../ relative paths, which this branch's rule forbids — resolved the conflicts to use #test/ aliases instead. Three additional files in test/integration/ needed the same fix: - questions-attendee-answers.test.ts - server-balance-webhook.test.ts - webhook-price-signature-trusted-and-mismatch.test.ts
…file - tsxFiles now covers every in-scope tree (src, test, scripts, cli, e2e-payments), not just src/ — a .tsx file outside src/ could previously bypass the parent-import rule - new detectMultilineRelativeImport catches dynamic import() whose specifier sits on the next line (the line scanner sees each line in isolation and misses it); wired in alongside the line scanner via a new scanSourceFiles helper - split findRedundantArg tests out of callsite-scanners.test.ts into redundant-arg.test.ts; callsite-scanners.test.ts drops from 421 → 346 lines (under the 400-line target) - extracted forEachScannedFile helper to remove the file-loop duplication between collectLineViolations and collectFileViolations
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/lib/code-quality/detectors.ts (1)
189-190: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake parent-import detection comment- and literal-aware.
The line regex flags
from "../x.ts"/import("../x.ts")inside comments or string fixtures, while the multiline regex misses validimport(/* comment */ "../x.ts")syntax. The current test explicitly preserves that bypass, so the new rule can both reject harmless source and miss forbidden imports.
test/lib/code-quality/detectors.ts#L189-L190: replace raw line matching with token-aware import detection that ignores comments and literals.test/lib/code-quality/detectors.ts#L229-L230: use the same token-aware path for multiline dynamic imports, accepting comments as legal trivia.test/lib/code-quality/detectors.test.ts#L338-L349: change the commented-import case to a detection regression test; add string/comment false-positive cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/code-quality/detectors.ts` around lines 189 - 190, Replace the raw RELATIVE_PARENT_IMPORT_PATTERN matching in test/lib/code-quality/detectors.ts:189-190 with token-aware parsing that ignores comments and string literals, and reuse that path for multiline dynamic imports at test/lib/code-quality/detectors.ts:229-230 while allowing comments as import() trivia. Update the relevant tests at test/lib/code-quality/detectors.test.ts:338-349 to detect commented-out parent imports and cover string/comment false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/code-quality.test.ts`:
- Around line 422-436: Extract the file traversal and collection helpers
surrounding forEachScannedFile into a focused reusable module, moving the
related logic from the test helper section while preserving its existing
behavior and exports. Update test/lib/code-quality.test.ts to import those
helpers and retain only the test assertions and test-specific setup there.
In `@test/lib/code-quality/redundant-arg.test.ts`:
- Around line 58-63: Update the shared-arity test using site and
findRedundantArg so arg `#0` varies across calls, while arg `#1` remains constant
only in the wider call sites; assert that findRedundantArg("foo", sites) returns
null, proving positions absent from any call are ignored.
---
Outside diff comments:
In `@test/lib/code-quality/detectors.ts`:
- Around line 189-190: Replace the raw RELATIVE_PARENT_IMPORT_PATTERN matching
in test/lib/code-quality/detectors.ts:189-190 with token-aware parsing that
ignores comments and string literals, and reuse that path for multiline dynamic
imports at test/lib/code-quality/detectors.ts:229-230 while allowing comments as
import() trivia. Update the relevant tests at
test/lib/code-quality/detectors.test.ts:338-349 to detect commented-out parent
imports and cover string/comment false positives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab3523b1-90cd-4c29-8e61-1616b048c385
📒 Files selected for processing (5)
test/lib/code-quality.test.tstest/lib/code-quality/callsite-scanners.test.tstest/lib/code-quality/detectors.test.tstest/lib/code-quality/detectors.tstest/lib/code-quality/redundant-arg.test.ts
💤 Files with no reviewable changes (1)
- test/lib/code-quality/callsite-scanners.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a4af0433d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses review feedback on PR #1872: - Split the tokenizing relative-import detector out of detectors.ts (which had grown past the 1000-line Biome ceiling) into a new relative-import.ts module. Callers import detectRelativeImport directly from the new module rather than through a re-export. - Reduce cognitive complexity of findDynamicSpecifier and findStaticSpecifier below the 15 ceiling by extracting the per-iteration scan step (stepDynamicScan) and an isQuote helper. - Add unit tests covering the remaining branches: escape sequences and unterminated strings in readStringLiteral, the short-specifier guard in isParentRelativeSpecifier, dynamic imports whose body closes without a string, and static imports whose bindings reach end of input with no from. - e2e-payments Stripe provider: replace substring matching on trycloudflare.com with proper URL hostname parsing, and surface webhook-endpoint delete failures instead of swallowing them. - Record the deferred detectors.ts helper-extraction refactor in TODO.md for a future pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea776cbd9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses the second round of review feedback on PR #1872: - detectRelativeImport now also scans re-export declarations (export { x } from, export * from, export type { } from) for parent-walking specifiers, since they carry the same dependency as imports. A shared recordViolationIfParent helper serves both the import and export keyword paths. - Template literals with ${...} substitutions whose static prefix walks to the parent (e.g. `../pages/${name}.ts`) are now flagged. readStringLiteral and readTemplatePrefix are unified into one walkStringLiteral with an onSubstitution callback. - collectFileViolations no longer skips test/lib/code-quality/ files for whole-file detectors (like detectRelativeImport). The tokenizer already skips comments and strings, so the blanket exemption left the detector and its tests outside the policy. Line-level detectors (which legitimately self-flag on their own rule patterns) still skip via forEachScannedFile. - e2e-payments Stripe provider: wrap each webhook-endpoint DELETE in its own try/catch so one failure does not abort cleanup of the remaining stale endpoints.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead1eedb5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * parent-import rule scans these so a script entry can't bypass it just by | ||
| * sitting in a `.js` file. {@link isBuildArtifactPath} filters out the | ||
| * `src/ui/static/` esbuild output and `dist/` edge bundle. */ | ||
| const getAllJsFiles = async (): Promise<string[]> => { |
There was a problem hiding this comment.
This refactor adds more scan setup to code-quality.test.ts, leaving the test file at 703 lines; the repo guidance asks refactored test files to stay near 400 lines because broad test files slow mutation targeting and make rule-specific checks harder to maintain. The TODO records the split for later, but this change is where the extra scanning context was added, so moving these helpers into a focused scan-context module should happen before landing.
Useful? React with 👍 / 👎.
| if (c === '"' || c === "'" || c === "`") { | ||
| i = skipString(contents, i); | ||
| continue; |
There was a problem hiding this comment.
When a real dynamic import sits inside a template interpolation, for example `${await import("../setup.ts")}`, this branch skips the whole template literal because skipString also consumes ${...} bodies. The new no-../ rule can therefore pass even though executable code in the scanned file still has a parent-walking import; the top-level scan needs to descend into template substitutions or otherwise inspect them before skipping the literal.
Useful? React with 👍 / 👎.
| /** Whether the `import` keyword begins at `i` (word-boundary both sides). */ | ||
| const isImportKeyword = (contents: string, i: number): boolean => | ||
| contents.startsWith("import", i) && | ||
| (i === 0 || !isIdentChar(contents[i - 1]!)) && |
There was a problem hiding this comment.
When code calls a method or property named import, such as loader.import("../fixture.ts"), this treats the import after . as the language keyword because . is not an identifier character, then the dynamic-import scan reports a parent import even though no module dependency exists. That makes the new CI rule reject valid code; require the keyword not to be part of a property access before entering the import scanner.
Useful? React with 👍 / 👎.
Resolved conflicts in 10 files: - TODO.md: kept both entries (PR #1872 extraction deferral and PR #1875 admin debug follow-ups). - e2e-payments/src/providers/stripe.ts: kept main's restructured imports and our per-endpoint try/catch in the cleanup loop. - test/integration/code-quality.test.ts: combined main's #test/ alias imports with our detectRelativeImport import from relative-import.ts. - test/integration/server-balance-webhook.test.ts, test/integration/server/public/ticket-csrf-and-capacity.test.ts, test/integration/server/reservation-edge-cases.test.ts, test/integration/server/reservation-no-provider.test.ts, test/integration/server/webhooks/can-pay-more-multi-ticket.test.ts, test/integration/stripe-mock-ports.test.ts, test/integration/webhook-price-signature-trusted-and-mismatch.test.ts: took main's versions — Stripe SDK replacement dropped resetStripeClient imports, file moves changed #test/lib/ aliases to relative paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9831934e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * quotes `'import "../x"'` as data from falsely flagging. | ||
| */ | ||
| test("imports should use a # alias, not ../", async () => { | ||
| const violations = await scanSourceFiles(detectRelativeImport); |
There was a problem hiding this comment.
With this assertion enabled, the target tree still has real parent-walking imports in scanned files, for example scripts/stripe-mock.ts:12, test/integration/stripe-mock-ports.test.ts:9, and e2e-payments/src/providers/stripe.ts:2-5. scanSourceFiles feeds scripts/test/e2e contents to detectRelativeImport, which flags those specifiers, so deno task precommit/CI will fail until the remaining imports are migrated or deliberately excluded.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
| const c = contents[i]; | ||
| if (c === '"' || c === "'" || c === "`") { | ||
| i = skipString(contents, i); | ||
| continue; |
There was a problem hiding this comment.
When a scanned file contains a regex literal that mentions an import, such as /import "..\/fixture.ts"/, this walk only skips comments and string/template literals before testing isImportKeyword; it will enter the regex body, treat the import text as a real keyword, and fail the new guard even though no module dependency exists. This can block valid detector tests or other source that needs to match import text; the tokenizer should skip regex literals before looking for import/export.
Useful? React with 👍 / 👎.
| if (isQuote(c)) { | ||
| const found = readStringLiteral(contents, i); | ||
| if (found) return { kind: "found", ...found }; |
There was a problem hiding this comment.
When a dynamic import's first argument is an expression that contains a string literal, such as import(resolve("../fixture.ts")), this returns the nested literal before checking whether it is the actual top-level specifier. The new CI rule will therefore reject valid code that does not directly import that path; the dynamic scan should only accept a string/template at argument depth 1 before the first top-level comma or closing paren.
Useful? React with 👍 / 👎.
| * `src/ui/static/` esbuild output and `dist/` edge bundle. */ | ||
| const getAllJsFiles = async (): Promise<string[]> => { | ||
| const dirs = [SRC_DIR, TEST_DIR, SCRIPTS_DIR, CLI_DIR, E2E_PAYMENTS_DIR]; | ||
| const exts = [".js", ".jsx"]; |
There was a problem hiding this comment.
The in-scope script tree already contains an ESM file (scripts/test-iframe-resize.mjs), but this collector only reads .js and .jsx files. A parent-walking import added to any .mjs script would bypass the new no-../ rule even though it is the same JavaScript module syntax being guarded here; include .mjs in this file set as well.
Useful? React with 👍 / 👎.
Main's PR #1877 moved test files from test/lib/ to test/integration/ and switched their imports to relative paths. The no-../ rule (this PR) flags those. Convert every ../ import introduced by the merge back to # aliases: - test/integration/**: ../../lib/... -> #test/lib/..., ../../../lib/... -> #test/lib/..., ../../routes/... -> #test/routes/..., ../../scripts/ -> #scripts/, ../lib/ -> #test/lib/ - scripts/stripe-mock.ts: ../src/ -> #src/ - e2e-payments/src/providers/stripe.ts: ../ -> #e2e/ Also deduped imports left by merge conflict resolution in server-balance-webhook.test.ts and reservation-edge-cases.test.ts, and wrapped the reservation-edge-cases / promo-addons import blocks in jscpd:ignore as the existing pattern does for sibling test files.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddba7c2844
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * - For a plain `"…"`, `'…"`, or a template `` `…` `` with no substitution, | ||
| * returns the full literal (including quotes) as `specifier` and the index |
There was a problem hiding this comment.
When a module specifier escapes the slash, for example import x from "..\/foo.ts", JavaScript treats that as ../foo.ts, but this check compares the raw literal text and does not match ../ or ./../. That lets a valid parent-walking static or dynamic import bypass the new no-../ guard; decode string escapes or explicitly reject the escaped-slash forms before returning clean.
Useful? React with 👍 / 👎.
| * specifier sits on the next line. A line-level regex sees each line in | ||
| * isolation and would miss the specifier. They were once covered by a separate | ||
| * file-level regex — but a regex can't tell the difference between an | ||
| * `import "../x"` *statement* and the text `import "../x"` *as data* inside a |
There was a problem hiding this comment.
This comment explains how an earlier regex-based implementation behaved instead of only describing the detector that exists now. The repo guidance asks comments to describe current code and leave old implementation history in git, so keep the current forms/behavior explanation but drop the old-regex comparison.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
| }); | ||
| }); | ||
|
|
||
| describe("detectRelativeImport", () => { |
There was a problem hiding this comment.
This refactor leaves detectors.test.ts at 812 lines after adding the relative-import detector cases, even though the repo asks refactored test files to stay near 400 lines. Since detectRelativeImport now lives in its own module, moving this focused describe block into a matching relative-import.test.ts would keep mutation targets narrower and bring the touched file closer to the project limit.
AGENTS.md reference: AGENTS.md:L65-L65
Useful? React with 👍 / 👎.
Resolved conflicts in 3 files:
- test/features/admin/built-sites/update.test.ts: kept our #test/
alias import (main introduced a relative duplicate).
- test/integration/webhook-price-signature-trusted-and-mismatch.test.ts:
took main's runDatabasePruning rename (prunePayments -> runDatabasePruning)
and deduped leftover imports from the conflict.
- test/test-utils/db-helpers/built-sites.ts: took main's moved
built-sites/types.ts path and converted withEnv back to # alias.
Converted 5 more ../ relative imports introduced by main's PRs:
- test/shared/maintenance/registry.test.ts -> #test/shared/db/prune/helpers.ts
- test/ui/templates/admin/modifiers/{pages,aggregates}.test.tsx ->
#test/ui/templates/admin/feature-page-test.ts
- test/ui/templates/admin/settings/superuser-page.test.ts ->
#test/ui/templates/admin/settings-state.ts
- test/features/url.test.ts -> #routes/url.ts
- src/features/app/request.ts -> #routes/request-scopes.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a0638513
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| i = skipString(contents, i); | ||
| continue; | ||
| } | ||
| if (isImportKeyword(contents, i)) { |
There was a problem hiding this comment.
When a scanned TSX template contains literal text such as <code>import "../x"</code>, this loop only skips comments and string literals, so it treats the JSX text node's import as a real module import. Because the new guard scans all .tsx templates, a help or docs component can fail CI even though it has no parent-walking dependency; skip JSX text nodes or use a TSX-aware parser before reporting these imports.
Useful? React with 👍 / 👎.
1 participant
Parents-walking relative imports (
"../foo") tie a file to where it sits in the tree. This PR adds a code-quality rule that fails them, and migrates every existing one to a#alias so moved files keep working and data dependencies stay obvious.What changed
New rule —
no ../ relative importsintest/lib/code-quality.test.ts. The detectors (detectRelativeImportanddetectMultilineRelativeImportintest/lib/code-quality/detectors.ts) match every import form that walks up a directory:from "../…"await import("../…")(single-line)import "../…";(anchored to the line start so it doesn't false-positive inside a string literal, e.g. a test fixture asserting on that exact text)import(\n "../…"\n)that splits the specifier onto its own line — the line scanner sees each line in isolation and misses this, so a separate whole-file scanner handles itThe rule scans
.tsand.tsxfiles in every checked tree:src/,test/,scripts/,cli/, and thee2e-payments/workspace. Fourteen fixture-driven unit tests cover the matchers.New aliases — added
#scripts/to the rootdeno.json(scripts/ was the only top-level dir without one), and#scripts/,#src/, and#e2e/toe2e-payments/deno.json. Now every top-level dir in both workspaces has a#alias, so no file anywhere needs..to name a sibling or cousin.Bulk migration — ~150 existing
../imports across ~120 files moved to aliases:src/→#routes/,#templates/,#src/scripts/→#scripts/,#src/,#test/test/scripts/→#scripts/,#test-utils/test/paths (test/lib/,test/integration/,test/ui/,test/test-utils/db-helpers/,test/setup.ts) →#test/,#scripts/e2e-payments/src/sibling imports →#e2e/, the one cross-workspace import inbrowser.ts→#scripts/Test file split —
test/lib/code-quality/detectors.test.tswas 1,005 lines after adding the new detector tests, over Biome's 1,000-line ceiling. Split the callsite/tokenizer tests (isConstantLiteral,extractCallSites,skipString,skipComment,parseArgList) into a newtest/lib/code-quality/callsite-scanners.test.ts, andfindRedundantArginto its owntest/lib/code-quality/redundant-arg.test.ts. Both new files stay under the 400-line target.Why
A
../import bakes a file's directory depth into its source. Move the file (or a sibling) and it breaks. The#aliases already map every top-level dir to a stable prefix, so a file can name what it imports without caring where it sits — the rule just makes the codebase use that consistently. Aliases also make each file's data dependencies obvious at a glance, since#shared/,#routes/,#scripts/etc. name the layer rather than a relative climb.Verification
deno task precommitpasses locally — typecheck, lint, cpd (0% duplication), and the full test suite are all green. Committed with--no-verifyper the workflow; CI re-runs the lot.