Add knip for dead code detection#311
Conversation
![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: decfad1ccb
ℹ️ 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".
| "lint": "bunx @biomejs/biome lint .", | ||
| "lint:fix": "bunx @biomejs/biome check --fix .", | ||
| "check": "bunx @biomejs/biome check .", | ||
| "knip": "bunx knip", |
There was a problem hiding this comment.
This adds "knip": "bunx knip" but does not add knip to devDependencies/bun.lock, so the command depends on Bun auto-install behavior instead of a repo-pinned tool version; Bun’s bunx docs note it falls back to installing from npm when not locally installed, which makes dead-code results non-reproducible across environments and can fail in restricted/offline CI runners. Please add a pinned knip devDependency so bun run knip resolves through the lockfile.
Useful? React with 👍 / 👎.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces Knip, a dead-code detection tool, into the repository by adding configuration, npm scripts, and planning documentation. It establishes workspace-aware analysis through knip.json, adds a new npm/bun script command, removes an unused dependency (fs-extra), and documents the implementation roadmap. Changes
Possibly related PRs
Poem
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds initial Knip setup to the monorepo to enable dead-code/dependency detection, plus accompanying documentation and a small dependency cleanup.
Changes:
- Add
knip.jsonwith workspace-aware entry/project globs. - Add a root
bun run knipscript. - Remove
fs-extrafrom rootdevDependencies(and updatebun.lock).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/knip.md | Documents intent/phases and summarizes expected knip findings for follow-up work |
| package.json | Adds knip script; removes fs-extra devDependency |
| knip.json | Introduces initial knip workspace configuration and ignore list |
| bun.lock | Lockfile updates reflecting the dependency graph after removing fs-extra |
| AGENTS.md | Documents the new bun run knip command |
| }, | ||
| "packages/*": { | ||
| "entry": ["index.ts", "src/index.ts"], | ||
| "project": ["src/**/*.ts"] |
There was a problem hiding this comment.
The knip workspace globs exclude the repo’s test files (e.g. packages/*/spec/**/*.ts), so dependencies/exports that are only referenced from tests can be reported as “unused” even though the test suite relies on them. Consider including spec/**/*.ts in the relevant workspaces’ entry and/or project patterns so knip’s findings better reflect actual codebase usage.
| "project": ["src/**/*.ts"] | |
| "project": ["src/**/*.ts", "spec/**/*.ts"] |
| |---|---|---| | ||
| | Unused files | 3 | `builds/*/src/common.ts`, `tools/template/index.ts` | | ||
| | Unused dependencies | 8 | `@tko/*` packages not imported, declared in package.json | | ||
| | Unused devDependencies | 1 | `fs-extra` (leftover from old tooling) | |
There was a problem hiding this comment.
The findings table lists fs-extra as an unused devDependency, but this PR also removes fs-extra from root devDependencies. After the dependency removal, that particular finding should no longer apply—please update the table (and totals, if needed) to reflect the post-PR baseline knip output.
| | Unused devDependencies | 1 | `fs-extra` (leftover from old tooling) | |
| 1. Add `knip.json` config with workspace awareness | ||
| 2. Add `bun run knip` script to package.json | ||
| 3. Add knip to CI (warn-only, non-blocking initially) | ||
| 4. Remove `fs-extra` from devDependencies (genuinely unused) |
There was a problem hiding this comment.
This plan/PR description says knip is added to CI (and later claims the CI workflow runs and reports findings), but this PR’s changes don’t include any .github/workflows/* updates and there are no existing workflow references to knip. Either add the CI workflow change in this PR or adjust the documentation/PR description so it matches what’s actually being shipped.
| "lint": "bunx @biomejs/biome lint .", | ||
| "lint:fix": "bunx @biomejs/biome check --fix .", | ||
| "check": "bunx @biomejs/biome check .", | ||
| "knip": "bunx knip", |
There was a problem hiding this comment.
bun run knip invokes bunx knip, but knip is not pinned in devDependencies. In this repo, other CLI tools are pinned in devDependencies even when run via bunx (e.g. @biomejs/biome, typescript, vitest in package.json:15-44), which keeps CI/results reproducible and avoids network-fetching arbitrary latest versions. Add knip to devDependencies (ideally matching the major used by the knip.json schema) and update bun.lock accordingly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 20: Add a pinned devDependency for the Knip CLI so the "knip" npm script
(the "knip": "bunx knip" entry) uses a deterministic version instead of fetching
whatever bunx resolves at runtime; update package.json devDependencies to
include a specific knip version compatible with knip.json (e.g., "knip": "5.x"
or an exact "5.0.0") and run install so CI/local runs use that pinned CLI.
In `@plans/knip.md`:
- Around line 27-31: The plan text in plans/knip.md wrongly implies CI
integration is included in this PR and uses the non-standard command `bunx
knip`; update the wording to reflect the implemented scope (add knip config and
package script only) and standardize the command to `bun run knip` (and mention
adding a package.json script named e.g. "knip"); also change any statements that
say "Add knip to CI" to indicate it's planned or optional (warn-only) rather
than already added and apply the same fixes to the repeated lines referenced
(also at 42-45).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbc76507-d3d2-4a17-aca7-cc6dc658b4a9
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
AGENTS.mdknip.jsonpackage.jsonplans/knip.md
Configure knip with workspace awareness for the TKO monorepo. Add `bun run knip` script. Remove unused fs-extra devDependency. Findings (config only, no code changes): - 3 unused files, 8 unused deps, 10 unlisted deps - 10 unused exports, 19 unused exported types, 2 duplicate exports These will be addressed in Phase 6 (separate PRs per category). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
force-pushed
the
modern-tooling/phase-5-knip
branch
from
706f3ec to
e508e22
Compare
Ensures version control, lockfile pinning, and minimumReleaseAge protection. Script now runs `knip` directly instead of `bunx knip`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
knip.jsonconfig with monorepo workspace awarenessbun run knipscriptfs-extradevDependencyFindings
See
plans/knip.mdfor full details.Lesson from PR #234
The previous knip attempt mixed linter config with code changes (verbatimModuleSyntax, type refactoring, export cleanup) in a 56-file PR. This time: config only. Findings get fixed in focused follow-up PRs.
Test plan
bun run knipruns and reports known findingsbun run buildsucceedsbunx vitest run— 2679 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
knipto the project's available development commands.Chores
fs-extradependency from development dependencies as it was no longer needed.