Skip to content

chore(ui): make knip trustworthy and enforce dead-code in CI - #32727

Merged
ryan-crabbe-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_knip_confidence
Jul 10, 2026
Merged

chore(ui): make knip trustworthy and enforce dead-code in CI#32727
ryan-crabbe-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_knip_confidence

Conversation

@ryan-crabbe-berri

@ryan-crabbe-berri ryan-crabbe-berri commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Screenshots / Proof of Fix

knip was already in the repo but never run in CI, so it had quietly rotted. Running it exposed two failure modes that made it untrustworthy: it silently degrades on a partial install (a fresh worktree that had not run npm install flagged all 436 test files as unused), and even on a complete tree its config had blind spots that surfaced as false positives.

Before, at base 27cf064556 (full npm ci tree, previous config):

$ npm run knip
Unused files (2)
e2e_tests/serverRootPath.config.ts     <- FALSE POSITIVE; invoked via --config in test_server_root_path.yml
src/components/ui/select.tsx           <- FALSE POSITIVE; shadcn primitive, added to the palette ahead of use
Unlisted dependencies (198)
@ant-design/icons  ...                 <- all 198 are the same missing package.json entry
Unused exports (118)
Unused exported types (158)
...
$ echo $?
1

After, at 012afc0c4d (this branch):

$ npm run knip
Unused exports (100)
Unused exported types (158)
Unused exported enum members (6)
Duplicate exports (23)
$ echo $?
0

Both false-positive files and the 198 phantom "unlisted" entries are gone, and the exit code is 0 because the remaining findings are staged as non-blocking warnings. The CI job installs with npm ci, so it always runs against a complete tree and can never silently degrade

Type

🚄 Infrastructure

Changes

The goal is to make a green knip run actually mean something, then enforce it, rather than blindly deleting everything it lists (some of which is wrong)

Config correctness, so findings are real:

  • Register every playwright config (serverRootPath.config.ts and migration.serverRootPath.config.ts), not just the main one. serverRootPath.config.ts is invoked via --config in test_server_root_path.yml, which knip cannot statically see, so it was wrongly reported as an unused file
  • Treat src/components/ui/** as entry points. These are shadcn design-system primitives; some are intentionally added to the palette before every one has a consumer, and each exports its full sub-component family. knip was flagging the not-yet-used ones (e.g. select.tsx) as dead files and their exports as unused. Registering the directory as the design-system surface is the correct fix. The tradeoff is that knip will no longer flag a genuinely-abandoned ui/ primitive, which is an acceptable call for a vendored component kit
  • Declare @ant-design/icons as a direct dependency. It is imported in ~198 files but only resolved through antd's hoisting, so each import showed up as an "unlisted dependency". This is a real fragility fix, not just noise suppression
  • Add an explicit vitest plugin block so test-file classification no longer depends on auto-detection (the exact thing that broke on partial installs)

Governance, so it ratchets like the existing lint budgets:

  • Stage severities via rules: gate the categories that are clean today (files, dependencies, unlisted, unresolved) as errors, and keep exports/types/duplicates as warnings so they are visible without blocking. Follow-ups can flip each category to error as it is driven to zero
  • Run npm run knip in the frontend-lint CI job
  • Move frontend-lint (prettier + eslint + budgets + knip) out of test-litellm-ui-build.yml and into its own test-litellm-ui-lint.yml named "UI Lint". It was sharing the build workflow's file, so its check surfaced as the misleading "UI Build Check / frontend-lint" despite doing no building; it now reads "UI Lint / frontend-lint". Whoever manages branch protection needs to update the required-check name from the old context to the new one so PRs don't strand

No code is deleted. Every file knip flagged turned out to be a false positive traceable to a config blind spot, which is the whole point: fix the tool so its output is trustworthy, rather than delete real code to satisfy a mis-tuned linter

The larger Unused exports (100) / Unused exported types (158) lists are left for follow-up PRs; they need per-item verification (knip's static analysis has the same blind spots that produced the serverRootPath false positive here), so blind-deleting them is exactly what this PR is trying to avoid

@ryan-crabbe-berri
ryan-crabbe-berri requested a review from a team July 10, 2026 05:29
@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes the existing knip dead-code scanner trustworthy by fixing config blind spots that caused false positives, then enforces it in CI so a green run actually means something. No production code is deleted.

  • Config correctness: registers all three Playwright configs, treats src/components/ui/** as design-system entry points (so un-consumed shadcn primitives aren't mis-flagged), and adds an explicit vitest plugin block to prevent partial-install degradation.
  • Explicit dependency declaration: promotes @ant-design/icons from an implicit transitive hoist to a direct package.json entry, eliminating the 198 phantom "unlisted dependency" warnings.
  • Ratcheted enforcement: a new rules block in knip.json gates the now-clean categories (files, dependencies, unlisted, unresolved, etc.) as errors and keeps noisier categories (exports, types, duplicates) as warnings; the new CI step runs npm run knip inside the frontend-lint job against a full npm ci tree.

Confidence Score: 5/5

Safe to merge — changes are limited to CI configuration, a linter config file, and a package.json dependency declaration; no production source code is touched.

The diff is purely infrastructure: a CI step that runs knip, a knip config that removes documented false positives, and a package.json entry that formalises an already-used transitive dependency. The PR description includes before/after output confirming exit code 0, and the config choices are clearly reasoned. Nothing here affects runtime behaviour.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/test-litellm-ui-build.yml Adds a knip dead-code check step to the frontend-lint job; correctly inherits the job-level working-directory: ui/litellm-dashboard and uses the same has_files guard as the existing lint steps.
ui/litellm-dashboard/knip.json Fixes three false-positive sources (shadcn ui/ entry points, multiple playwright configs, explicit vitest plugin) and introduces a rules block that gates clean categories as errors and keeps noisy ones as warnings.
ui/litellm-dashboard/package.json Promotes @ant-design/icons from an implicit transitive hoist to an explicit direct dependency at 5.6.1, eliminating 198 false "unlisted dependency" warnings from knip.
ui/litellm-dashboard/package-lock.json Lockfile updated to record @ant-design/icons 5.6.1 as a direct dependency entry; no other substantive changes.

Reviews (2): Last reviewed commit: "chore(ui): make knip trustworthy and enf..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

knip was producing garbage locally and was never wired into CI, so nobody
trusted it. Two structural problems: it silently degrades when deps are
missing (a partial worktree install flagged all 436 test files as unused),
and its config had blind spots that surfaced as false positives.

Fixes so a knip run means something:

- Register every playwright config (serverRootPath + migration variants), not
  just the main one. serverRootPath.config.ts is invoked via --config in
  test_server_root_path.yml, which knip can't see; it was falsely flagged as
  an unused file
- Treat src/components/ui/** as entry points. These are shadcn design-system
  primitives, intentionally part of the palette before every one is consumed;
  knip was flagging not-yet-used ones (e.g. select.tsx) as dead files and
  their sub-exports as unused. Marking the directory as the design-system
  surface is the correct fix, not deleting components someone is about to use
- Declare @ant-design/icons as a direct dependency. It was imported in ~198
  files but only resolved via antd hoisting, so every one showed up as an
  "unlisted dependency"
- Add an explicit vitest plugin block so test-file classification no longer
  rides on auto-detection
- Stage severities via rules: gate the now-clean categories (files,
  dependencies, unlisted, unresolved) as errors and keep exports/types/
  duplicates as warnings, so CI enforces what's at zero today while the
  remaining findings ratchet down in follow-ups
- Run npm run knip in the frontend-lint CI job, which installs with npm ci so
  it never sees a partial tree

knip now exits 0 with the gated categories clean
@ryan-crabbe-berri
ryan-crabbe-berri force-pushed the litellm_knip_confidence branch from a581eac to 012afc0 Compare July 10, 2026 05:43
@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_knip_confidence (47c3280) with litellm_internal_staging (bf02a4a)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (190ea08) during the generation of this report, so bf02a4a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@mateo-berri mateo-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks!

@ryan-crabbe-berri
ryan-crabbe-berri merged commit e2eee36 into litellm_internal_staging Jul 10, 2026
128 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_knip_confidence branch July 10, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants