Repository navigation
refactor(lint): consolidate + relocate findMany rule fixtures (fast-follow to #2129) - #2136
Conversation
…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
|
Warning Review limit reached
Next review available in: 18 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 (9)
✨ 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 |
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.
…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>
Summary
Fast-follow to #2129 ("chore(lint): flag unbounded findMany calls — Phase 8 of task board crash-prevention epic", merged as
5258619c2).#2129 was merged mid-review — after the Codex-review fixes landed (generalized the
findManyreceiver match, scoped thelimitcheck to the top-level options object, replaced the file-widewarnoverride with 80 per-call-site suppressions) but before a proactive/simplifypass (4 parallel review agents: reuse, simplification, efficiency, altitude) had finished and its findings were pushed. That last commit (ac6c92e41) never made it into master. This PR carries just that commit, cherry-picked onto currentmaster(the original branch's diff against current master was full of unrelated noise from other PRs merged in the meantime — this PR's diff is exactly the 9 files this commit touched, verified withgit diff origin/master...HEAD --stat).Reuse and efficiency came back clean in that pass. Two real, actionable findings, both applied here:
apps/web/src/app/api/__fixtures__/) each duplicated ~3 lines of import/header boilerplate. Consolidated into 2 files grouped by scenario —db-handle-findmany-fixture.ts(plaindbhandle: unbounded / limited / findFirst) andalt-handle-and-nested-limit-findmany-fixture.ts(thetx-handle case and the nested-relation-limit case) — each case still its own independently-exercised exported function.src/app/api/__fixtures__/sat inside the real 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 literalroute.tsfilename — incidental, not a structural exclusion, and a future coverage walker that scans by extension instead would trip over it. Moved toapps/web/src/__fixtures__/eslint-unbounded-findmany/— still undersrc/(sonext lint's default scan dirs cover it — verified by temporarily adding an unsuppressed violation there and confirmingbun run lint --filter=webfailed on it, then reverting) but fully outsideapp/api/.Also documented a known limitation directly in the rule's comment (
apps/web/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 generalization in #2129 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.No behavior change to the lint rule itself — same selector, same severity, same 80 pre-existing suppressions (just relocated/consolidated as fixture files, not touched as actual violation sites).
Epic: PageSpace page
j44e35jwzlhr54fbmruk3k4i("Task Board Query & Reorder Safety"), Phase 8.Test plan
bunx eslint src/__fixtures__/eslint-unbounded-findmany/— clean, all 6 fixture cases behave as documentedbun run lint --filter=web— 4/4 successful, nono-restricted-syntaxoutputbun run typecheck --filter=web— greengit diff origin/master...HEAD --stat— confirmed the diff is exactly the 9 files this commit touches, no unrelated driftCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com