-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(ci): walk every route entry the workspace app composes #7026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,19 +46,33 @@ const APP = join(ROOT, 'apps/sim') | |
| const FORBIDDEN = join(APP, 'tools/registry.ts') | ||
|
|
||
| /** | ||
| * Root the guard walks: every `page.tsx` and `layout.tsx` under the workspace app. | ||
| * Root the guard walks: every route entry Next.js composes under the workspace app. | ||
| * | ||
| * Discovered rather than listed. A hardcoded list goes stale silently — the | ||
| * first version of this guard named `app/workspace/layout.tsx` as "the shared | ||
| * shell", but that file only wraps `SocketProvider`; the real shell is | ||
| * `app/workspace/[workspaceId]/layout.tsx`, which was never checked. | ||
| * | ||
| * Layouts must be enumerated separately because Next.js composes them by | ||
| * convention — a page does not `import` its layout, so walking pages alone never | ||
| * reaches layout modules even though every route pays for them. | ||
| * Every filename here is composed by convention rather than imported, so each | ||
| * must be enumerated: a page does not `import` its layout, its error boundary, | ||
| * or its loading state, yet the route pays for all of them. `error.tsx` in | ||
| * particular is always a Client Component — Next requires it — so a registry | ||
| * edge there lands in the browser bundle as surely as one from a page. | ||
| * | ||
| * The root stays at `app/workspace`. Widening it to `app` reports | ||
| * `(interfaces)/resume/[workflowId]/[executionId]/page.tsx`, which is a Server | ||
| * Component (`runtime = 'nodejs'`, `force-dynamic`) whose `PauseResumeManager` | ||
| * import resolves server-side and never reaches a client bundle. This guard | ||
| * cannot tell the two apart, so it stays where the premise holds. | ||
| */ | ||
| const ENTRY_ROOT = 'app/workspace' | ||
| const ENTRY_FILENAMES = new Set(['page.tsx', 'layout.tsx']) | ||
| const ENTRY_FILENAMES = new Set([ | ||
| 'page.tsx', | ||
| 'layout.tsx', | ||
| 'error.tsx', | ||
| 'loading.tsx', | ||
| 'not-found.tsx', | ||
| ]) | ||
|
Comment on lines
+69
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The expanded allowlist still excludes convention-composed entries such as |
||
|
|
||
| function collectEntries(dir: string, found: string[] = []): string[] { | ||
| for (const entry of readdirSync(dir, { withFileTypes: true })) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-route error.tsx treated as entry
Medium Severity
Adding
error.tsxtoENTRY_FILENAMESmakescollectEntriespick upapp/workspace/[workspaceId]/components/error/error.tsx, a sharedErrorStatehelper with no default export, not a Next-composed route boundary. That file is one of the claimed 26 new roots and already sits on realerror.tsxgraphs via the components barrel, so the guard now walks a non-route entry and will baseline it as one.Additional Locations (1)
scripts/check-tool-registry-boundary.ts#L76-L84Reviewed by Cursor Bugbot for commit f2aa705. Configure here.