Remove import-boundary compatibility re-export shims - #1018
Conversation
The boundary-extraction PRs (#1014, #1017) moved shared primitives to neutral #worker/* modules but left pure re-export shims in #app/* to keep their diffs small. Repoint the 30 remaining importers (including four vi.mock specifiers that would otherwise stop applying) at the canonical modules and delete the six shims; #app/username.ts already had zero importers. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe pull request replaces app-layer compatibility imports with direct worker identity, permission, base-URL, audit-log, user-lookup, and account-deletion-state imports, and updates related TypeScript inputs and test mocks. ChangesWorker import boundary consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e2e/playwright-utils.ts imported username.ts through a relative path that the specifier-based sweep missed, and two tsconfig include lists still pointed at app/username.ts, app/reserved-usernames.ts (already gone since #1014), and app/permissions.ts. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-1018.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/tsconfig-client.json (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate identity-permissions include.
./src/identity/permissions.tsis already included at Line 39, so this replacement leaves the same file listed twice. Keep one canonical entry to avoid configuration drift.🤖 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 `@packages/worker/tsconfig-client.json` at line 23, Remove the duplicate "./src/identity/permissions.ts" entry from the tsconfig include list, retaining the existing canonical entry referenced elsewhere. Ensure the file remains included exactly once.
🤖 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.
Nitpick comments:
In `@packages/worker/tsconfig-client.json`:
- Line 23: Remove the duplicate "./src/identity/permissions.ts" entry from the
tsconfig include list, retaining the existing canonical entry referenced
elsewhere. Ensure the file remains included exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 25d9bcaa-e1cc-4c68-8d03-a186e50af895
📒 Files selected for processing (42)
e2e/playwright-utils.tspackages/worker/client/app.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/client/session.tspackages/worker/src/app/account-deletion-state.tspackages/worker/src/app/account-package-invocation-tokens-data.tspackages/worker/src/app/app-base-url.tspackages/worker/src/app/audit-log.tspackages/worker/src/app/handlers/account-jobs.node.test.tspackages/worker/src/app/handlers/account-jobs.tspackages/worker/src/app/handlers/account-mcp-servers.tspackages/worker/src/app/handlers/account-package-invocation-tokens.node.test.tspackages/worker/src/app/handlers/account-profile.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/handlers/blog.tsxpackages/worker/src/app/handlers/community-install.tspackages/worker/src/app/handlers/og-page-image.tspackages/worker/src/app/handlers/package-app.node.test.tspackages/worker/src/app/handlers/package-app.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/onboarding-data.tspackages/worker/src/app/package-app-origin.tspackages/worker/src/app/permissions.tspackages/worker/src/app/ssr-render.tsxpackages/worker/src/app/user-lookup.tspackages/worker/src/app/username.tspackages/worker/src/community/activity-package-subscriptions.tspackages/worker/src/email/package-subscriptions.tspackages/worker/src/execute-maintenance.tspackages/worker/src/index.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth.tspackages/worker/src/oauth-handlers.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/platform-feedback/package-subscriptions.tspackages/worker/src/run-records/package-subscriptions.tspackages/worker/src/webhooks/http.tspackages/worker/src/webhooks/service.tspackages/worker/tsconfig-client.jsontsconfig-tools.json
💤 Files with no reviewable changes (6)
- packages/worker/src/app/permissions.ts
- packages/worker/src/app/audit-log.ts
- packages/worker/src/app/account-deletion-state.ts
- packages/worker/src/app/user-lookup.ts
- packages/worker/src/app/username.ts
- packages/worker/src/app/app-base-url.ts
./src/identity/permissions.ts was already listed further down the include list, so the repointed entry was redundant. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
What
The boundary-extraction work (#1014, #1017) moved shared primitives into neutral
#worker/*modules but left six pure re-export shims underpackages/worker/src/app/to keep those diffs small:app-base-url.ts(23 importers)permissions.ts(4)user-lookup.ts,account-deletion-state.ts,audit-log.ts(1 each)username.ts(1 importer via a relative path ine2e/playwright-utils.ts)This repoints every importer at the canonical
#worker/*module and deletes the shims. Fourvi.mock('#app/app-base-url.ts', ...)specifiers in handler tests are repointed too — after the import repoint they would have silently stopped intercepting the real module.It also fixes two stale tsconfig
includeentries:tsconfig-tools.jsonstill listedapp/username.tsandapp/reserved-usernames.ts(the latter already deleted by #1014 — silently ignored since explicitincludepaths behave like globs), andtsconfig-client.jsonstill listedapp/permissions.ts.Why now
Ten of the shim importers were shared-primitive-layer files (
webhooks/,email/,run-records/,mcp-auth.ts,package-runtime/, ...) importing#app/*— the exact upward-coupling pattern the July architecture audit flagged and #1014 set out to eliminate. With the shims gone, those edges no longer exist, and no new importer can quietly re-adopt the app-layer path.Notes for review
export * from '#worker/...'; no behavior moved.tools/oxlint/import-boundaries.node.test.tsstill asserts the#app/app-base-url.tsspecifier is forbidden from#mcp/*; that assertion checks rule behavior for the specifier and does not require the file to exist.Validation
Full
npm run validateon the VM: format, lint, typecheck, structure checks (primitives,migrations),backup:build, 1507/1507 unit tests, and all 20 Playwright E2E tests pass. The MCP-E2E lane timed out once under the parallel gate (a different test each attempt, both on the 45sstartDevServertimeout) and passes cleanly when run in isolation vianpm run test:mcp— Nx flaggedworker:test-mcpas a flaky task. Known VM contention between the two dev-server-spawning suites, unrelated to this diff.Summary by CodeRabbit