Add code-health PR check (dead-code delta)#2019
Conversation
⚪ Code HealthNo change to the dead-code surface. 60 Unused files
157 Unused exports
103 Unused exported types
14 Unused exported enum members
5 Unused dependencies
3 Unused devDependencies
|
4b402dc to
f34b363
Compare
jshearer
left a comment
There was a problem hiding this comment.
Claude found at least one "structural" problem with the code that it claims will cause false-positive signals.
This (dead code detection) is a genuinely hard problem to get right, and isn't even wholly solvable in a dynamic language like Typescript. I'm not sure that the juice is worth the squeeze here...
Also, what's up with that typing version bump? Maybe a no-op, but seems relevant.
| "version": "20.17.30", | ||
| "resolved": "https://registry.npmjs.org/@types/node/-/node-20.17.30.tgz", | ||
| "integrity": "sha512-7zf4YyHA+jvBNfVrk2Gtvs6x7E8V+YDW05bNfG2XkWDJfYRXrTiP/DsB2zSYTaHX0bGIujTBQdMVAhb+j7mwpg==", | ||
| "version": "26.0.1", |
There was a problem hiding this comment.
This lockfile silently bumps the hoisted @types/node from 20.17.30 to 26.0.1 — a six-major jump in the typings tsc resolves for src/ — in a PR that only adds knip.
package.json doesn't pin @types/node and tsconfig has no types allowlist, so after merge everyone's next npm ci typechecks against Node 26 typings (e.g. NodeJS.Timeout/global signature changes), potentially breaking npm run typecheck on main for an unrelated-looking PR. Suggest pinning/deduping @types/node back to 20.x in the lockfile before merging
| # only changes config (like the one that introduces this check) | ||
| # doesn't manufacture a phantom delta. The ruler is constant; | ||
| # only source differences register. Falls back to base's config. | ||
| git checkout --quiet ${{ github.event.pull_request.head.sha }} -- knip.json 2>/dev/null || true |
There was a problem hiding this comment.
The "constant ruler" pin covers only knip.json, but knip's effective config also includes tsconfig.json and package.json (plugin auto-detection, package.json#knip), which are never pinned — so a PR changing tsconfig paths or dependency lists can still manufacture a phantom delta today. And if config ever moves to knip.ts/package.json#knip, this checkout fails silently via || true and base/head measure with different configs.
Not a today-bug (the check is informational and the fallback is documented), but the failure mode is exactly the one this pin was written to prevent — a red +N on a config-only PR that teaches people to distrust the check. Worth either pinning the wider config surface or at least echoing a warning into the report when the pin path diverges.
718324d to
a4152a1
Compare
|
I'm feeling pretty tolerant of an imperfect tool here - it's only meant to give visibility on existing unused code, and will help us identify code orphaned by future changes. false negatives (orphaned code missed by the check) will just leave dead code in the project, and false positives can be ignored - if we get so many false positives that the check is useless, we can just turn it off. |
a4152a1 to
604416d
Compare
|
Discussed this on a call w/ Joseph - we agreed that presenting the bare Knip output is a stronger first pass, and we can refine/reclassify the categories later, if needed |
Code-health check: dead-code delta on every PR
Adds a GitHub Action that runs Knip on the PR head and its base, then posts a sticky comment with the difference — which unused files / exports / types / dependencies this PR introduces vs. removes.