Repository navigation
chore(lint): flag unbounded findMany calls — Phase 8 of task board crash-prevention epic - #2129
Merged
Merged
Conversation
…ash-prevention epic Add a no-restricted-syntax rule to apps/web/eslint.config.mjs that errors on any db.query.<table>.findMany(...) call whose options object has no `limit` key. findFirst() is exempt (inherently single-row). No new ESLint plugin dependency — uses an inline AST selector, per flat-config support for rule objects. Production Postgres OOM-crashed on 2026-07-18 because a task-list route called findMany() with no limit on an unbounded table. Other phases of this epic fix that specific call site; this phase adds the structural backstop so a NEW unbounded findMany can't ship the same way again. A full repo lint pass found 74 pre-existing call sites (41 files) with no limit. Fixing them is out of scope for this phase — they're catalogued in apps/web/eslint-unbounded-findmany-baseline.json and downgraded to `warn` via a files-scoped override, so the rule is `error` for new code without failing CI on existing debt today. TDD: added three fixtures under apps/web/src/app/api/__fixtures__/ — unbounded (confirmed lint passed before the rule existed, fails after — suppressed with a documented inline eslint-disable since it's a permanent demonstration fixture, not real debt), limited (passes), and findFirst (passes, proving no over-firing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AmaDMiZasBmANeANB94m2G
Contributor
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (51)
✨ 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 |
![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: b721e7042f
ℹ️ 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".
| // eslint-unbounded-findmany-baseline.json; fixing these is a follow-up, not this change. | ||
| files: unboundedFindManyBaselineGlobs, | ||
| rules: { | ||
| "no-restricted-syntax": ["warn", unboundedFindManyRule], |
There was a problem hiding this comment.
This file-scoped override downgrades every matching call in each of the 41 listed files, not just the 74 pre-existing call sites. If another unbounded findMany() is added to a frequently modified file such as src/app/api/tasks/route.ts, it will therefore be reported only as a warning and can pass the stated lint gate, defeating the protection for a substantial part of the API; baseline the exact existing nodes instead of changing severity for whole files.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed — see 39532da. The file-scoped warn override is gone entirely. Replaced with a per-call-site eslint-disable-next-line no-restricted-syntax at each of the (now 80, after the receiver-generalization fix below also widened the scan) exact pre-existing violation lines. A new unbounded findMany() added anywhere in src/app/api/tasks/route.ts — or any other previously-flagged file — now errors, exactly as you flagged. Thanks for catching this.
|
|
||
| const unboundedFindManyRule = { | ||
| selector: | ||
| "CallExpression[callee.object.object.object.name='db'][callee.object.object.property.name='query'][callee.property.name='findMany']:not(:has(Property[key.name='limit']))", |
There was a problem hiding this comment.
The selector hard-codes the receiver identifier to db, so the same unbounded relational query is not diagnosed when invoked through another supported Drizzle handle. This repository already uses tx.query.pages.findMany(...) in src/app/api/upload/complete/route.ts:86 and database.query.pages.findMany(...) in src/lib/workflows/agent-trigger-shared.ts:89; new calls in either form can still fetch an unbounded result set without any lint finding.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed — see 39532da. Dropped the [callee.object.object.object.name=db] constraint entirely; the selector now matches <anything>.query.<table>.findMany(...) regardless of the receiver. Verified this actually catches tx.query.pages.findMany (upload/complete/route.ts:87, bulk-copy/route.ts, bulk-delete, bulk-move) and database.query.pages.findMany (agent-trigger-shared.ts:90) — a full rescan now finds 80 pre-existing sites (up from 74), including the 6 you called out. Added alt-handle-findmany-fixture.ts / alt-handle-limited-findmany-fixture.ts as a regression lock (tx handle, no-limit vs. with-limit).
|
|
||
| const unboundedFindManyRule = { | ||
| selector: | ||
| "CallExpression[callee.object.object.object.name='db'][callee.object.object.property.name='query'][callee.property.name='findMany']:not(:has(Property[key.name='limit']))", |
There was a problem hiding this comment.
The :has(Property[key.name='limit']) predicate searches every descendant of the call, rather than only the top-level options object. A relational query such as findMany({ with: { children: { limit: 10 } } }) therefore escapes the rule even though only the nested relation is limited and the root result remains unbounded; constrain the predicate to a direct property of the first argument.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed — see 39532da. Replaced :has(Property[key.name='limit']) with :has(ObjectExpression.arguments > Property[key.name='limit']). .arguments is esquery's field-position qualifier, so it anchors the ObjectExpression to the call's own options-argument position and requires the limit Property to be a direct child of that specific object — a limit nested under with: { children: { limit } } } no longer satisfies it. Verified against exactly the findMany({ with: { children: { limit: 10 } } }) shape you described (both via a standalone esquery unit check and the real ESLint run), and added nested-limit-findmany-fixture.ts as a permanent regression fixture for this case.
…t check, per-site suppression Three real findings from the automated Codex review on PR #2129, all fixed: 1. (P1) The selector hard-coded the receiver to a literal `db` identifier, so `tx.query.<table>.findMany` and `database.query.<table>.findMany` — both real patterns in this codebase (db.transaction callbacks, injected handles) — were invisible to the rule. Generalized to match any `<handle>.query.<table>.findMany` chain. This surfaced 6 additional real unbounded call sites (bulk-copy, bulk-delete, bulk-move, upload/complete, home-drive, agent-trigger-shared). 2. (P2) `:has(Property[key.name='limit'])` searched the entire call subtree, so a `limit` nested inside `with: { children: { limit } } }` (bounding only that relation) falsely satisfied the root query's own boundedness. Anchored the check to `ObjectExpression.arguments` (esquery's field-position qualifier) so only a `limit` that is a direct key of the call's own options object counts. 3. (P1) The file-scoped `warn` override downgraded EVERY findMany call in each of the 41 baseline files, not just the specific pre-existing ones — a new unbounded findMany added to e.g. src/app/api/tasks/route.ts would have passed lint as a mere warning. Replaced with a per-call-site `eslint-disable-next-line` at each exact pre-existing violation, so the rule stays `error` for any new call in the same file. eslint-unbounded-findmany- baseline.json is removed (no longer read by config; would have drifted from the real suppression sites). Corrected full-repo scan: 80 pre-existing call sites across 44 files (up from the initial 74/41 — the 6 newly-caught tx/database sites plus the 2 nested-limit test fixtures added below). Added two regression fixtures locking in the fixes: alt-handle-findmany-fixture.ts (tx handle, no limit → flagged) / alt-handle-limited-findmany-fixture.ts (tx handle, with limit → passes), and nested-limit-findmany-fixture.ts (nested relation limit → root still flagged). Verified: bun run typecheck, bun run lint --filter=web both green (0 errors, only pre-existing unrelated warnings). bun run test:unit shows the same 4 pre-existing failures as master (missing local `test` Postgres role, unrelated SDK/grouping issues) — reproduced identically without this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AmaDMiZasBmANeANB94m2G
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
Cross-checked this PR against sibling Phase 8 (PR #2129, open in parallel), which adds an ESLint rule requiring every db.query.<table>.findMany() call to carry a direct `limit` key. My phase-2 hydrate call was bounded only indirectly (inArray scoped to the already-limited boundedTaskIds array), with no literal `limit` key — so once both phases land it would get flagged as a new unbounded findMany. Add `limit: boundedTaskIds.length` as defense-in-depth: redundant with the existing invariant today, but keeps this call self-evidently bounded even if that invariant is ever broken upstream, and satisfies the upcoming lint rule without depending on its unmerged PR. Locked in with a test assertion on the findMany call's options. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
…eads
Ran a 4-angle /simplify pass (reuse, simplification, efficiency, altitude)
over the diff. Applied the two safe, in-scope findings:
- route.ts: the childPageIds/boundedTaskIds early-return branches built the
identical { taskList, tasks: [], statusConfigs } response object verbatim
twice — extracted into a local emptyTasksResponse() closure.
- query-spec.ts: parseTaskQuerySpec built its return object via three
repeated `...(x ? { x } : {})` conditional spreads to omit falsy keys, when
`x: x || undefined` does the same job (toEqual treats undefined-valued and
absent keys as equivalent) with one fewer allocation and less repetition.
Confirmed against the 4-agent findings and skipped as false positives /
out of scope:
- The `limit: boundedTaskIds.length` on the phase-2 findMany was flagged as
dead code by one pass, but cross-checked against the actual (unmerged)
sibling PR #2129 ESLint rule it anticipates — the rule requires a literal
`limit` key on every findMany, so this earns its keep as more than
defense-in-depth.
- escapeLikePattern duplicates 3 existing local implementations elsewhere
in apps/web (tasks/route.ts, search/route.ts, mentions/search/route.ts).
Real, but centralizing it means touching 3 files outside this phase's
declared scope (route.ts + query-spec.ts only) — logged as a follow-up
task instead of expanding this PR.
- The pre-existing tasks.sort() JS comparator was flagged as duplicating
phase-1's SQL ordering rule — skipped because the epic's Phase 2 spec
explicitly requires preserving that exact sort semantic unchanged, and two
existing tests assert on it directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf
Merged
8 tasks done
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
…ing gap Proactive /simplify pass on PR #2129 (4 parallel review agents: reuse, simplification, efficiency, altitude). Reuse and efficiency came back clean. Two real, actionable findings applied: - Simplification: 6 single-purpose fixture files had ~3 lines of duplicated import/header boilerplate each and some redundant in-file comments. Consolidated into 2 files grouped by scenario (db-handle-findmany-fixture.ts: unbounded/limited/findFirst on a plain `db` handle; alt-handle-and-nested- limit-findmany-fixture.ts: the `tx` handle case and the nested-limit case), each still independently exercising every case as its own exported function. - Altitude: `src/app/api/__fixtures__/` sat inside the actual Next.js route tree. It happened to be silently skipped by two coverage-audit tests (drive-member-gate-coverage.test.ts, security-audit-coverage.test.ts) that filter by literal `route.ts` filename — but that's incidental, not a structural exclusion, and a future coverage walker that scans by extension instead would trip over it. Moved to src/__fixtures__/eslint-unbounded- findmany/ — still under `src/` (Next's default `next lint` scan dirs cover it, verified by temporarily adding an unsuppressed violation there and confirming `bun run lint --filter=web` failed on it, then reverting), but fully outside `app/api/`. Also documented a known limitation directly in the rule's comment: it's a syntactic AST pattern, not type-aware, so aliasing the query object first (`const q = db.query.taskItems; q.findMany(...)`) evades it — the same class of gap the tx/database fix (39532da) just closed for receiver identifiers. Noted the durable fix would be a typed wrapper in packages/db, not another AST special case, if this pattern shows up in practice. Verified: bun run lint --filter=web and bun run typecheck --filter=web both green after the move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AmaDMiZasBmANeANB94m2G
4 tasks done
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
…rd crash-prevention epic (#2128) * fix(tasks): bound the OOM-crash GET tasks query — Phase 2 of task board crash-prevention epic Production Postgres OOM-crashed on 2026-07-18 (17:30-17:45 UTC). Root cause: the GET /api/pages/[pageId]/tasks handler ran an unbounded relational query (5 joined relations, no limit/offset) over every task under a page, then sorted and search-filtered in JS after the full fetch. This is the route responsible for that crash. - Add parseTaskQuerySpec (query-spec.ts), a pure parser that clamps limit/offset via parseBoundedIntParam (limit 1-200, default 100; offset >= 0, default 0) and falls back to sortOrder 'asc' for unrecognized values. - Rewrite the GET handler to run two bounded queries instead of one unbounded one: a lightweight join over taskItems+pages resolves the ordered (by page.position, falling back to task.position — same sort semantic as before), filtered, limited/offset set of task ids, then the existing relational findMany hydrates only those ids' 5 relations. - Response shape is unchanged; TaskListView/TaskKanbanView need no changes. Closes Phase 2 of the Task Board Query & Reorder Safety epic (PageSpace page j44e35jwzlhr54fbmruk3k4i). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf * fix(tasks): remove dead in-memory search filter, keep sort The 'Apply search filter in memory' block was dead code — the phase-1 SQL query already applies ilike(pages.title, ...) before boundedTaskIds is derived, so every hydrated task already matches the search term. tasks.sort() is still needed since findMany(where: inArray(...)) doesn't guarantee row order, so it's kept. Updated the 'filters tasks by search query' test to mock the phase-1 bounded select and a findMany that filters by the ids it's given, proving the narrowing happens via the SQL query rather than in JS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf * fix(tasks): escape ILIKE metacharacters in the search term A search term containing a literal %, _, or backslash was passed unescaped into ilike(pages.title, `%${search}%`), so Postgres read it as pattern syntax instead of literal text (e.g. searching "100% done" would match against a % wildcard, not the literal percent sign) — over/under-matching the bounded phase-1 query. Add escapeLikePattern, a pure helper in query-spec.ts, and apply it at the ilike() call site in route.ts. Covered by 5 unit tests on the escaping function plus a route-level regression test asserting the exact escaped pattern reaches ilike(). Addresses a P2 finding from automated PR review on #2128. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf * fix(tasks): add explicit limit to the phase-2 hydrate findMany Cross-checked this PR against sibling Phase 8 (PR #2129, open in parallel), which adds an ESLint rule requiring every db.query.<table>.findMany() call to carry a direct `limit` key. My phase-2 hydrate call was bounded only indirectly (inArray scoped to the already-limited boundedTaskIds array), with no literal `limit` key — so once both phases land it would get flagged as a new unbounded findMany. Add `limit: boundedTaskIds.length` as defense-in-depth: redundant with the existing invariant today, but keeps this call self-evidently bounded even if that invariant is ever broken upstream, and satisfies the upcoming lint rule without depending on its unmerged PR. Locked in with a test assertion on the findMany call's options. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf * refactor(tasks): dedupe empty-response block, flatten conditional spreads Ran a 4-angle /simplify pass (reuse, simplification, efficiency, altitude) over the diff. Applied the two safe, in-scope findings: - route.ts: the childPageIds/boundedTaskIds early-return branches built the identical { taskList, tasks: [], statusConfigs } response object verbatim twice — extracted into a local emptyTasksResponse() closure. - query-spec.ts: parseTaskQuerySpec built its return object via three repeated `...(x ? { x } : {})` conditional spreads to omit falsy keys, when `x: x || undefined` does the same job (toEqual treats undefined-valued and absent keys as equivalent) with one fewer allocation and less repetition. Confirmed against the 4-agent findings and skipped as false positives / out of scope: - The `limit: boundedTaskIds.length` on the phase-2 findMany was flagged as dead code by one pass, but cross-checked against the actual (unmerged) sibling PR #2129 ESLint rule it anticipates — the rule requires a literal `limit` key on every findMany, so this earns its keep as more than defense-in-depth. - escapeLikePattern duplicates 3 existing local implementations elsewhere in apps/web (tasks/route.ts, search/route.ts, mentions/search/route.ts). Real, but centralizing it means touching 3 files outside this phase's declared scope (route.ts + query-spec.ts only) — logged as a follow-up task instead of expanding this PR. - The pre-existing tasks.sort() JS comparator was flagged as duplicating phase-1's SQL ordering rule — skipped because the epic's Phase 2 spec explicitly requires preserving that exact sort semantic unchanged, and two existing tests assert on it directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9HNEnE2gFPs1c7UrjJGcf --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
…ing gap (#2136) Proactive /simplify pass on PR #2129 (4 parallel review agents: reuse, simplification, efficiency, altitude). Reuse and efficiency came back clean. Two real, actionable findings applied: - Simplification: 6 single-purpose fixture files had ~3 lines of duplicated import/header boilerplate each and some redundant in-file comments. Consolidated into 2 files grouped by scenario (db-handle-findmany-fixture.ts: unbounded/limited/findFirst on a plain `db` handle; alt-handle-and-nested- limit-findmany-fixture.ts: the `tx` handle case and the nested-limit case), each still independently exercising every case as its own exported function. - Altitude: `src/app/api/__fixtures__/` sat inside the actual Next.js route tree. It happened to be silently skipped by two coverage-audit tests (drive-member-gate-coverage.test.ts, security-audit-coverage.test.ts) that filter by literal `route.ts` filename — but that's incidental, not a structural exclusion, and a future coverage walker that scans by extension instead would trip over it. Moved to src/__fixtures__/eslint-unbounded- findmany/ — still under `src/` (Next's default `next lint` scan dirs cover it, verified by temporarily adding an unsuppressed violation there and confirming `bun run lint --filter=web` failed on it, then reverting), but fully outside `app/api/`. Also documented a known limitation directly in the rule's comment: it's a syntactic AST pattern, not type-aware, so aliasing the query object first (`const q = db.query.taskItems; q.findMany(...)`) evades it — the same class of gap the tx/database fix (39532da) just closed for receiver identifiers. Noted the durable fix would be a typed wrapper in packages/db, not another AST special case, if this pattern shows up in practice. Verified: bun run lint --filter=web and bun run typecheck --filter=web both green after the move. Claude-Session: https://claude.ai/code/session_01AmaDMiZasBmANeANB94m2G Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
Merged 13 more master commits (task-board crash-prevention epic phases 1-8, Sentry crash reporting, payment receipt emails). Two real conflicts: - apps/control-plane/package.json, apps/realtime/package.json: the Sentry commit (#2131) re-added dotenv/cookie next to its new @sentry/node dependency in both files — same class of parallel-PR collision as the earlier admin/broadcast one. Verified via grep: still zero real consumers of dotenv (control-plane) or cookie (realtime) post-merge. Kept @sentry/node, dropped both. - knip flagged 2 new "unused files": apps/web/src/__fixtures__/ eslint-unbounded-findmany/*.ts. These are intentional TDD fixtures for a new no-restricted-syntax ESLint rule (#2129, relocated by #2136) — files meant to be *linted*, not imported; their pass/fail lint state is the test itself. Added a knip.json ignore rather than treating them as dead code. Baseline unchanged at 5. Full uncached typecheck (16/16) + lint (14/14) clean.
2witstudios
added a commit
that referenced
this pull request
Jul 19, 2026
…aselined issues (#2135) * feat(ci): make knip a blocking CI gate with a monotonic baseline Wires knip into the lint job as a blocking check via a baseline/ratchet script (scripts/knip-ratchet.mjs), modeled on scripts/coverage-ratchet.mjs. knip:check fails only on issues not already in knip-baseline.json; knip:ratchet regenerates the baseline after cleanup, refusing to let it grow. Also fixes real knip.json config drift found while generating the baseline: - packages/lib and packages/db entry globs pointed at deleted barrel files (src/index.ts, src/server.ts, src/client.ts) instead of the packages' actual ~320/70 subpath exports, badly inflating unused-exports/types counts - apps/android and apps/ios Capacitor plugins were false-positive "unused deps" (auto-linked natively, never imported in TS) - packages/cli's bin.ts/bin-pagespace-mcp.ts (package.json "bin" field) had no matching knip entry, so they showed as unused files - 14 stale ignoreDependencies entries and 4 redundant entry patterns already covered by knip's built-in plugins Baseline lands at 408 real issues (down from a raw 587 before the config fixes). Follow-up PRs against this branch burn that down to zero. * chore(web): delete orphaned version-history UI and dead versions/compare route (#2116) The VersionHistoryPanel/VersionHistoryItem components and their barrel were knip-baselined as unused; the /api/pages/[pageId]/versions/compare route's only caller was that UI (knip can't flag route.ts files — manual audit finding). RollbackConfirmDialog.tsx stays: it's live via ActivityItem and SidebarActivityTab. Removes the route's now-stale AUDIT_EXEMPT_ROUTES entry and ratchets the knip baseline down by the 3 UI files. Claude-Session: https://claude.ai/code/session_01ABo3G5XFj1jDWgx4jEhF8D Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(web): delete dead ToolBuilder cluster (knip PR 4) (#2115) Removes ToolBuilder.tsx, JsonSchemaBuilder.tsx, and OpenAPIImportDialog.tsx — an unreferenced closed cluster (they import only each other and shared UI primitives) — and drops their three entries from the knip baseline. Claude-Session: https://claude.ai/code/session_01X9MVgoceVrAtKxpzM6BdBc Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(web): delete 26 unused AI Elements components from ai/ui (#2117) The vendored "AI Elements" kit under apps/web/src/components/ai/ui/ was mostly dead: 26 of its 30 components had zero imports anywhere in the repo (verified by knip and a full-repo grep including dynamic imports). The four live files — code-block.tsx, conversation.tsx, tool.tsx, web-preview.tsx — are kept, along with their test. Baseline shrinks by 26 entries via knip:ratchet. Claude-Session: https://claude.ai/code/session_01XdBnRUGsKhJnkq7DjheNmg Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(deps): knip baseline burn-down — dependency hygiene sweep (non-marketing) (#2119) Removes 15 unused dependencies, adds 4 missing (unlisted) ones, removes 3 orphaned package.json exports subpaths, and encodes 7 knip false positives as ignoreDependencies. Baseline shrinks 405 -> 361. Removed deps: - apps/web: @ai-sdk/anthropic, @ai-sdk/google, @ai-sdk/openai, @ai-sdk/xai (abandoned AI SDK 7 migration remnants), pg, recharts, @xyflow/react, @radix-ui/react-accordion, embla-carousel-react - apps/control-plane: dotenv - apps/processor: @aws-sdk/s3-request-presigner, ts-node - apps/realtime: cookie - packages/lib: mammoth, pdf-parse-debugging-disabled Added (were unlisted): - root: drizzle-orm, pg (scripts/ imports) - apps/web: server-only - apps/admin: postcss Orphaned exports removed (source deleted in earlier refactors): - packages/lib ./services/sandbox/lifecycle (14f50df) - packages/lib ./services/rate-limit-cache (e5659ae) - packages/db ./schema/sandbox-sessions (88b10b4) plus lib's matching dangling typesVersions entries. knip.json ignoreDependencies (false positives, mirroring apps/web's existing pattern): - apps/admin eslint stack (used by `next lint` + eslint.config.mjs) - apps/android + apps/ios `web` (turbo build-order dep for cap sync) - packages/lib react-dom (peer-dep host for @react-email/render) Left alone: apps/web tokenlens + @radix-ui/react-hover-card — their only importers are knip-dead but still-typechecked files, and nothing else declares them, so removal breaks typecheck until a dead-file deletion PR lands. Claude-Session: https://claude.ai/code/session_014q2Rfkf5PFkjiacLKLGXoD Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(marketing): delete 45 unused shadcn ui components + 22 unused deps (#2118) Pure deletion PR in the knip-baseline burn-down series. Every removed file was flagged unused by knip and verified to have no references anywhere in apps/marketing (including dynamic imports and cross-imports — the only importers of these files were other files in the same deletion set). Each removed dependency was imported only by the deleted files. Marketing build passes; knip baseline shrinks 405 → 338. Claude-Session: https://claude.ai/code/session_011RdhxswpnTxfygoeSm46gz Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * chore(web): delete dead shadcn accordion/carousel files apps/web/src/components/ui/{accordion,carousel}.tsx had zero real consumers (carousel.tsx's only consumer, ai/ui/inline-citation.tsx, was deleted in the AI Elements cleanup). They were invisible to knip (src/components/ui/** is knip-ignored) and only kept typechecking because apps/marketing still declared @radix-ui/react-accordion/embla-carousel-react and bun's hoisting gave web a free resolution path — a path removed by the marketing + deps-sweep cleanup PRs. Deleting them removes the fragile cross-workspace hoisting dependency entirely. * chore(knip): close remaining config gaps + orphaned deps sweep - apps/marketing: add ignoreDependencies for its eslint stack (same FlatCompat-resolved-but-not-imported pattern already ignored for apps/web/apps/admin); add postcss/postcss-load-config as explicit devDeps instead of relying on hoisting from another workspace - apps/admin: add src/components/ui/** to knip ignore, matching apps/web/apps/marketing convention for vendored shadcn kits — was producing 13 false-positive "unused export" entries - apps/web: remove tokenlens + @radix-ui/react-hover-card (only consumers were the already-deleted ai/ui/* files); delete components/ui/hover-card.tsx (zero real consumers, same knip-ignored-but-dead pattern as the accordion/carousel cleanup) Baseline: 262 -> 241. * chore(knip): resolve all 4 remaining unresolved-import issues - vitest.workspace.ts: 'setupFiles: ./src/test/setup.ts' resolves fine at runtime (vitest remaps it relative to the web project's root: './apps/web'), but knip resolves it relative to vitest.workspace.ts's own location instead — false positive, added ignoreUnresolved. - scripts/__tests__/changelog-shell-safety.test.ts: tested a module tree (scripts/changelog/*) deleted wholesale in 5c47614 ("remove stale docs/, prune cross-references") — the test was never cleaned up alongside it. Deleted. Note: root package.json's "changelog:generate" script still points at the deleted scripts/changelog/index.ts and is therefore broken — left as-is, this is a pre-existing, already-known issue (not a dead-code/knip finding) and out of scope here. Baseline: 241 -> 237. * chore(knip): PR8a — clear the long tail of 16 remaining unused-file flags (#2122) Deleted 13 genuinely dead files (zero references anywhere in the repo, confirmed via repo-wide grep for imports, dynamic require()/import(), and string-keyed lookups): - apps/control-plane/src/repositories/index.ts (dead barrel; createTenantRepository is imported directly from ./tenant-repository everywhere) - apps/web/src/components/layout/left-sidebar/DriveList.tsx - apps/web/src/components/layout/left-sidebar/workspace-selector.tsx - apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarSettingsTab.tsx - apps/web/src/components/shared/AuthButtons.tsx (superseded by OAuthButtons) - apps/web/src/components/shared/ContactForm.tsx (superseded by marketing's own ContactForm) - apps/web/src/components/tasks/TaskMobileCard.tsx - apps/web/src/components/tasks/task-hooks.ts - apps/web/src/hooks/useDirectUpload.ts (superseded by useAttachmentUpload + lib/upload/*) - apps/web/src/lib/ai/types/global-prompt.ts (superseded by admin's local copy of the type) - apps/web/src/lib/repositories/index.ts (dead barrel; individual repository files are imported directly everywhere) - apps/web/src/lib/tabs/index.ts (dead barrel; individual tab modules are imported directly) - packages/lib/src/email-templates/TenantProvisioningCompleteEmail.tsx Kept 3 files that are genuinely reachable only through mechanisms knip's static analysis can't trace, and reclassified them in knip.json instead of deleting: - apps/web/src/test/next-server-stub.ts — resolved via vitest.config.ts's resolve.alias substitute for 'next/server'; added to apps/web's ignore list. - apps/e2e/support/mock-server-main.ts — invoked as a shell command (`bun run support/mock-server-main.ts`) by Playwright's webServer config; added a new apps/e2e workspace block with it in ignore. - packages/db/src/migrate-pending-invites.ts — standalone CLI script invoked via the package.json `migrate-pending-invites` script; added to packages/db's entry list alongside its sibling scripts (migrate.ts, migrate-admin.ts, provision-admin-users.ts). Ran `bun run knip:ratchet` to lock in the shrink (237 -> 221 baseline issues). Verified with a full `bunx turbo run typecheck --force` and `bunx turbo run lint --force` across the whole repo (not just affected workspaces). Claude-Session: https://claude.ai/code/session_01HX9rxrXVhWCJVdU3haUciU Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * chore(knip): PR8b — resolve remaining unused exports/types/duplicate-exports (#2132) * chore(knip): PR8b — resolve remaining unused exports/types/duplicate-exports Removed unused export keywords, dead symbols, and duplicate export forms (named + default) flagged by knip across web, admin, marketing, desktop, control-plane, realtime, e2e, and the lib/sdk packages. Deleted six fully dead component files and VoiceModeOverlay.tsx (zero importers repo-wide; prior pass had stripped its exports but left the file in place). Every removed symbol was cross-checked with a repo-wide ripgrep sweep for remaining consumers before deletion. * chore(knip): fix cascade breakage from PR8b + ratchet baseline Merging pu/knip-ci's tip exposed 4 new knip flags plus a handful of typecheck/lint errors caused by symbols becoming dead only after PR8b's exports were stripped (a removed component/type left its sibling constant, prop type, or union member with zero remaining consumers). - apps/desktop/fetch-proxy-types.ts: FetchProxyResponse union was itself dead (verified: no repo-wide consumer), which cascaded to its four member interfaces once removed — deleted all five. - apps/web/lib/mentions/mentionConfig.ts: MentionConfigManager's removal in PR8b left `globalConfig`/DEFAULT_GLOBAL_CONFIG/GlobalMentionConfig with no reader — deleted. - apps/web/lib/utils/formatters.ts: LANGUAGE_EXTENSION_MAP was only used by a function already removed in PR8b — deleted. - apps/web/lib/monitoring/monitoring-types.ts: last remaining interface (MonitoringWidgetProps) had zero consumers after DetailedWidgetProps was removed; the admin app has its own separate copy that's very much alive, this web-only copy was dead — deleted the whole file. - apps/web/components/ai/ui/tool.tsx: ToolInputProps/ToolOutputProps types were only used by the ToolInput/ToolOutput components PR8b removed — deleted. - apps/web/lib/editor/pagination: getPageSize had zero importers outside its own dead barrel re-export — deleted. - apps/admin/lib/auth/auth-fetch.ts: fetchJSON had zero callers anywhere in the admin app — deleted (unrelated to the merge, pre-existing). Ratcheted knip-baseline.json: 221 -> 8 issues. The remaining 8 are deliberately left alone (not oversights): - capacitor-bridge.ts (isAndroid, getInjectedPlatform, callNative): exercised by a dedicated test suite via dynamic import; removing would break the tests, and this reads as a stable platform-detection surface not yet called from every code path. - page-agent-repository.ts PageAgentConfig: explicitly documented in its own comment as a forward-looking contract for sibling Terminal PRs to build against. - replay-dedupe.ts MAX_ANCHOR_BYTES/MAX_DISCARDABLE_GIVEUP_BYTES: semantically distinct constants that are intentionally equal by design (documented in a comment), not an accidental alias. - cli keys/list.ts and keys/revoke.ts handler/Handler pairs: explicitly documented as split so tests can call the plain function without going through the router. - device-auth-helpers.ts createDeviceToken/createWebDeviceToken: @deprecated alias with an active migration still in progress across real call sites (device/register, google/callback, apple/callback) — a behavior migration, not an export-hygiene fix. Verified with `rm -rf apps/*/.next && bunx turbo run typecheck --force` and `bunx turbo run lint --force` across the whole repo (both clean), plus the affected unit test suites. * chore(knip): remove dead capacitor-bridge exports; accept final 5 apps/web/src/lib/capacitor-bridge.ts: isAndroid, getInjectedPlatform, callNative had zero real callers in application code — only exercised by their own dedicated unit tests (test-only usage doesn't count per this repo's knip config). Removed the functions and their test blocks; 23/23 tests in capacitor-bridge.test.ts still pass. Baseline: 8 -> 5. The remaining 5 are deliberately kept, not oversights: - 4 "duplicate exports" (tokensList/tokensListHandler, tokensRevoke/tokensRevokeHandler in packages/cli, and createDeviceToken/createWebDeviceToken in apps/web) follow an intentional two-tier naming convention — bare name is the reusable business-logic function (part of @pagespace/cli's published library API), Handler-suffixed alias is what the CLI router imports. A generic duplicate-export detector can't distinguish that from an accidental leftover alias; consolidating risks breaking external consumers of a published package. - 1 "unused type" (PageAgentConfig in page-agent-repository.ts) is explicitly documented as "the contract sibling Terminal PRs (tool-group, settings UI, session routing) build against" — active in-flight design work, not dead code. knip:check / typecheck / lint all clean, full repo, uncached. * fix(admin): restore broadcast exports orphaned by cross-PR merge Merging master's broadcast admin UI feature (#2107) into pu/knip-ci surfaced real breakage: PR8b had removed post/fetchJSON from auth-fetch.ts and AudienceDefinitionInput/BroadcastActionInput from broadcasts/schema.ts because they had zero consumers *in PR8b's branch* — the broadcast admin UI didn't exist there yet. Two PRs landing in parallel, neither could see the other's need for these. Restored exactly what master's broadcast-composer.tsx/composer-form.ts actually import; left `del` and `TemplateCreateInput` removed since nothing currently uses either (verified via repo-wide grep — the one `del(` hit was `stripe.customers.del`, unrelated). Full repo typecheck (16/16) + lint (14/14) clean; 28/28 broadcast component tests pass. * chore(knip): reconcile second wave of master drift Merged 13 more master commits (task-board crash-prevention epic phases 1-8, Sentry crash reporting, payment receipt emails). Two real conflicts: - apps/control-plane/package.json, apps/realtime/package.json: the Sentry commit (#2131) re-added dotenv/cookie next to its new @sentry/node dependency in both files — same class of parallel-PR collision as the earlier admin/broadcast one. Verified via grep: still zero real consumers of dotenv (control-plane) or cookie (realtime) post-merge. Kept @sentry/node, dropped both. - knip flagged 2 new "unused files": apps/web/src/__fixtures__/ eslint-unbounded-findmany/*.ts. These are intentional TDD fixtures for a new no-restricted-syntax ESLint rule (#2129, relocated by #2136) — files meant to be *linted*, not imported; their pass/fail lint state is the test itself. Added a knip.json ignore rather than treating them as dead code. Baseline unchanged at 5. Full uncached typecheck (16/16) + lint (14/14) clean. * fix(knip-ratchet): fix duplicate fingerprinting + enforce shrink Two real bugs flagged by automated PR review on #2135: 1. knip's `duplicates` entries are arrays of group members (each group is 2+ symbols that are the same export under different names), not flat {name} objects like every other category. The old code did `item?.name` on the group array itself, which is always undefined, so every duplicate group in a file collapsed onto the identical `duplicates::file::` fingerprint. A file with two distinct duplicate groups would only ever register one — the second would silently match the already-baselined (empty-name) fingerprint. Fixed by fingerprinting the whole group as its sorted, comma-joined member names. 2. Check mode printed a "you could shrink the baseline" suggestion but still exited 0. A stale fingerprint for now-fixed dead code remains permitted indefinitely — reintroducing that exact dead code later would silently pass CI. Check mode now fails when the baseline can shrink, same severity as finding a new issue; verified with a scenario test (injected a stale fingerprint, confirmed check mode fails with a clear message, confirmed it passes clean once removed). Regenerated knip-baseline.json with the corrected duplicate fingerprints (same 5 issues, now uniquely identified). knip:check / full typecheck / full lint / scripts lint all clean. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
no-restricted-syntaxESLint rule toapps/web/eslint.config.mjsthat errors on any<handle>.query.<table>.findMany(...)call whose options object has no directlimitkey (findFirst()is exempt — inherently single-row;<handle>matchesdb,tx,database, or any other object exposing Drizzle's relational query API — see review history below). No new plugin dependency: an inline AST selector, per flat-config's support for rule objects.findMany()with nolimiton an unbounded table. Other phases of this epic fix that specific call site; this phase (8/8) adds the structural backstop so a new unboundedfindManycan't ship the same way again.limit. Fixing them is out of scope for this phase — each is suppressed with a per-call-siteeslint-disable-next-line no-restricted-syntax(not a file- or severity-level override), so the rule stayserrorfor any new unbounded call, including ones added to an already-flagged file.j44e35jwzlhr54fbmruk3k4i("Task Board Query & Reorder Safety").Review history (addressed)
Automated review (
chatgpt-codex-connector) caught three real issues in the first version of this rule, all fixed in the second commit:db. The original selector only matched a literaldbidentifier, missingtx.query.<table>.findMany(used insidedb.transactioncallbacks) anddatabase.query.<table>.findMany(an injected handle) — both real patterns already in this codebase. Generalized the selector to match any<handle>.query.<table>.findManychain regardless of the receiver's name. This surfaced 6 additional real call sites:bulk-copy,bulk-delete,bulk-move,upload/complete,home-drive,agent-trigger-shared.:has(Property[key.name='limit'])matched alimitanywhere in the call, sofindMany({ with: { children: { limit: 10 } } } })— which only bounds the nested relation, not the root query — falsely passed. Anchored the check toObjectExpression.arguments(esquery's field-position qualifier), so only alimitthat is a direct key of the call's own options object counts.warnoverride. The original fix downgraded the rule towarnfor everyfindManycall in each of the 41 baseline files, not just the specific pre-existing lines — a brand-new unboundedfindManyadded to e.g.src/app/api/tasks/route.tswould have shipped as a mere warning. Replaced with aneslint-disable-next-lineat each of the 80 exact violation lines, so the rule iserroreverywhere except those specific pre-existing calls.Two regression fixtures were added to lock these fixes in: the
txhandle case (prove it's caught, and that addinglimitclears it) and the nested-limit case (proves a nested-relationlimitdoes NOT satisfy the root query) — see fixture layout below.TDD (RED → GREEN)
Fixtures live under
apps/web/src/__fixtures__/eslint-unbounded-findmany/(moved out ofsrc/app/api/__fixtures__/in a later commit — see "Proactive simplify pass" below), consolidated into 2 files by scenario, each exported function independently proving one case:db-handle-findmany-fixture.ts(plaindbhandle):getAllTaskItemsUnbounded— confirmedbunx eslintpassed on this before the rule existed (RED). Fails as intended after the rule (GREEN); kept as a permanent demonstration case, suppressed with a documented inlineeslint-disable-next-line(not "fixed" with a limit — that would defeat its purpose).getTaskItemsWithLimit— same shape, withlimit: 50. Passes.getOneTaskItem—findFirst(), no limit. Passes — proves no over-firing on single-row reads.alt-handle-and-nested-limit-findmany-fixture.ts:getChildPagesUnboundedViaTx/getChildPagesLimitedViaTx—tx.query.pages.findMany(insidedb.transaction) without/withlimit. Fails / passes.getPagesWithOnlyNestedLimit— top-level nolimit, but a nested relation has one. Fails, as it should.Verified the relocation didn't silently drop these from CI's lint scope: temporarily added an unsuppressed unbounded
findManyto the new location and confirmedbun run lint --filter=webfailed on it, then reverted.Follow-up inventory (80 call sites / 44 files, suppressed with
eslint-disable-next-line)Full list
src/app/api/account/drives-status/route.ts— line 22src/app/api/admin/global-prompt/route.ts— line 90src/app/api/calendar/events/[eventId]/attendees/route.ts— lines 126, 265src/app/api/calendar/events/route.ts— lines 295, 427src/app/api/canvas/file-view-tokens/route.ts— line 49src/app/api/channels/[pageId]/messages/route.ts— line 48src/app/api/commands/resolve/route.ts— line 89src/app/api/commands/route.ts— lines 69, 81, 110src/app/api/cron/calendar-sync/route.ts— line 34src/app/api/drives/[driveId]/pages/route.ts— lines 83, 133, 164src/app/api/drives/[driveId]/trash/route.ts— line 80src/app/api/drives/route.ts— line 45src/app/api/integrations/zoom/triggers/route.ts— line 39src/app/api/mcp/documents/route.ts— lines 267, 402src/app/api/pages/[pageId]/children/route.ts— line 30src/app/api/pages/[pageId]/tasks/[taskId]/route.ts— lines 86, 244src/app/api/pages/[pageId]/tasks/route.ts— lines 55, 115, 151, 420src/app/api/pages/[pageId]/tasks/statuses/route.ts— lines 67, 263, 367src/app/api/pages/bulk-copy/route.ts— lines 113, 282src/app/api/pages/bulk-delete/route.ts— lines 43, 180src/app/api/pages/bulk-move/route.ts— lines 113, 290src/app/api/pages/tree/route.ts— line 85src/app/api/storage/info/route.ts— line 66src/app/api/tasks/route.ts— lines 160, 198, 211src/app/api/upload/complete/route.ts— line 87src/app/api/user/favorites/reorder/route.ts— line 28src/app/api/user/favorites/route.ts— line 39src/lib/ai/tools/calendar-read-tools.ts— lines 304, 446, 763src/lib/ai/tools/channel-tools.ts— line 195src/lib/ai/tools/command-tools.ts— line 348src/lib/ai/tools/page-read-tools.ts— lines 414, 577, 1194src/lib/ai/tools/task-helpers.ts— lines 177, 444src/lib/ai/tools/task-management-tools.ts— lines 147, 567, 589, 596, 644src/lib/canvas/asset-pipeline.ts— lines 322, 425src/lib/canvas/custom-domain-mirror.ts— line 388src/lib/canvas/publish-page.ts— lines 519, 607, 785src/lib/channels/agent-mention-responder.ts— lines 194, 289src/lib/commands/available-commands.ts— lines 83, 89src/lib/integrations/zoom/webhook-trigger-queries.ts— line 47src/lib/machines/machine-list-runtime.ts— line 30src/lib/onboarding/home-drive.ts— line 39src/lib/repositories/session-repository.ts— lines 93, 105src/lib/workflows/agent-trigger-shared.ts— line 90src/services/api/page-service.ts— lines 392, 396, 502, 506TOTAL FILES: 44 TOTAL CALL SITES: 80
Proactive simplify pass
Ran a 4-agent parallel review (reuse / simplification / efficiency / altitude) against the diff before calling this done. Reuse and efficiency came back clean. Two real findings applied (third commit):
src/app/api/__fixtures__/sat inside the real Next.js route tree — it happened to be silently skipped by two coverage-audit tests that filter by literalroute.tsfilename, but that's incidental, not structural. Moved tosrc/__fixtures__/eslint-unbounded-findmany/, still undersrc/(sonext lint's default scan still covers it) but fully outsideapp/api/.Also documented a known limitation directly in the rule's comment (
eslint.config.mjs): it's a syntactic AST pattern, not type-aware, so aliasing the query object first (const q = db.query.taskItems; q.findMany(...)) would evade it — the same class of gap the tx/database fix above just closed for receiver identifiers. If that pattern shows up in practice, the durable fix is a typed wrapper inpackages/dbmakinglimita required parameter, not another AST special case.Test plan
bunx eslint src/__fixtures__/eslint-unbounded-findmany/— clean (all 6 fixture cases behave as documented above)bun run lint --filter=web— 0 errors, only pre-existing unrelated warnings (nono-restricted-syntaxoutput at all — all 80 pre-existing sites are suppressed inline)bun run typecheck— greenbun run test:unit— same 4 pre-existing failures reproduce identically on unmodifiedmaster(missing localtestPostgres role, unrelated SDK/grouping test issues) — not introduced by this changeCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com