ci(ui): frontend-lint job enforcing prettier + eslint on changed files#29633
ci(ui): frontend-lint job enforcing prettier + eslint on changed files#29633ryan-crabbe-berri wants to merge 13 commits into
Conversation
…d files Lints only the files a PR adds or modifies under ui/litellm-dashboard, so new and touched code must be prettier-clean and eslint-clean while the existing tree is grandfathered. Skips cleanly when a PR touches no lintable UI files. This lets us adopt the formatters incrementally without a repo-wide reformat
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR introduces a
Confidence Score: 5/5Safe to merge — purely additive CI infrastructure with no effect on runtime behaviour. All changes are confined to the CI workflow and ESLint/Prettier configuration files. Path-handling logic, exit-code accumulation, and suppression/budget mechanisms are correct. The one observation only affects CI diagnostic clarity, not correctness. No files require special attention;
|
| Filename | Overview |
|---|---|
| .github/workflows/test-litellm-ui-build.yml | Adds the frontend-lint job: collects changed files with git diff --relative, routes them to prettier/eslint by extension, skips install entirely when no UI files change, and runs the budget checker unconditionally on !cancelled(). Logic is correct. |
| ui/litellm-dashboard/scripts/check-lint-budgets.mjs | Reads the full-repo eslint JSON report and checks warning counts for budgeted rules; correct logic but lacks defensive error handling around file reads and argument validation. |
| ui/litellm-dashboard/eslint.config.mjs | Adds new rules (react/no-danger, no-var, no-self-assign, fetch ban, tremor import ban, complexity/depth/param thresholds). Config is consistent with the described enforcement strategy. |
| ui/litellm-dashboard/eslint-budgets.json | Defines max/target ceilings for no-explicit-any, complexity, and max-depth; values match the PR description and leave reasonable headroom. |
| ui/litellm-dashboard/.prettierignore | Adds eslint-suppressions.json to .prettierignore so the large generated baseline file is not subject to prettier formatting. |
| ui/litellm-dashboard/eslint-suppressions.json | Generated baseline capturing 963 error-level ESLint findings; grandfathers existing violations so only newly introduced ones block CI. |
Reviews (3): Last reviewed commit: "docs(ui): note pruning the eslint suppre..." | Re-trigger Greptile
Keep the prettier/eslint changed-file lists out of the checkout dir so they cannot collide with a future source file of the same name
|
@greptileai re review |
Capture the current error-level eslint findings (318 across 183 files) in a committed suppressions baseline via eslint --suppress-all. Every rule stays at its error severity, so any newly introduced violation fails the frontend-lint gate, while the existing tree is grandfathered; touching a legacy file never forces fixing its pre-existing issues. CI runs eslint with --pass-on-unpruned-suppressions so that fixing a baselined issue does not fail on a now-stale suppression, and the generated baseline is prettier-ignored since eslint owns its format. Burn the baseline down over time with eslint --prune-suppressions
ca16462 to
c8bb6e5
Compare
Make @typescript-eslint/no-explicit-any a warning and cap the total instead of hard-blocking each new one. A frontend-lint step counts the repo-wide explicit any and fails only when it exceeds the committed budget in eslint-any-budget.json. max starts at 2031, ten above the current 2021, so the next ten land as warnings and the build fails once that headroom is gone. Lower max over time toward target to ratchet the count down. New anys still surface as warnings on changed files via the normal eslint step
fa66aba to
e387111
Compare
These have no existing violations, so they need no baseline; turning them on purely blocks new instances. react/no-danger guards against new dangerouslySetInnerHTML (XSS), no-var enforces let/const, and no-self-assign catches self-assignment typos. no-debugger is already enforced by the recommended preset
Enable complexity:20, max-depth:4, max-params:4, max-nested-callbacks:4, with thresholds set near the codebase p99 so only genuine outliers are flagged. The 272 existing over-threshold functions are grandfathered in the suppressions baseline; new over-threshold functions block. Lower the thresholds over time to ratchet complexity down. max-lines-per-function is intentionally left off since React components are legitimately long
Add a no-restricted-syntax rule flagging bare fetch() calls, pointing contributors at React Query (@tanstack/react-query). The rule is not exempted anywhere, including the already-bloated networking.tsx, so all 331 existing fetch calls are grandfathered but no new ones can be added there or elsewhere. New data access goes through React Query, and the networking layer can be migrated out and pruned from the baseline over time
e3ac98f to
e5e74ac
Compare
Add a no-restricted-imports rule flagging imports from @tremor/react so tremor is phased out rather than spread further. The 232 existing tremor imports are grandfathered in the baseline; new ones block and point at antd. Migrate components off tremor and prune the baseline over time
Raise max from 2031 to 2040, giving ~19 of slack over the current 2021 instead of 10
The frontend-lint gate flagged its own config file. Format it so the prettier check on this PR's changed files passes
These two are smell metrics with arbitrary thresholds where a legit new function can trip them, so make them advisory rather than hard-blocking. They drop out of the baseline (now 963). max-params, max-nested-callbacks, and the react-hooks rules stay strict since those are clear-cut
Generalize the explicit-any budget into a shared lint-budget mechanism:
eslint-budgets.json maps a rule to {max, target} and check-lint-budgets.mjs
counts each across the repo and fails when a count exceeds its max.
complexity (129, max 140) and max-depth (61, max 70) now use the same
slack-plus-counter model as explicit-any (2021, max 2040): they warn
per-file and the build only fails if the repo-wide total crosses the
ceiling. Lower each max toward its target over time
fcbbefc to
fd65a48
Compare
|
@greptileai re review |
Relevant issues
Linear ticket
Resolves LIT-3556
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
The UI has prettier and (on flat config) eslint set up, but neither runs in CI. A one-shot repo-wide reformat is too large a diff to land, so this adopts both incrementally: a
frontend-lintjob that checks only the files a PR adds or modifies underui/litellm-dashboard. New and touched code has to be clean; the rest of the tree is grandfathered and converges over time as files get edited.The job's shell logic was validated locally against committed diffs. Prettier gate, on an unformatted file:
ESLint gate, on a brand-new violation against the baseline (a new file with an unused import):
A PR that changes no lintable UI files short-circuits before install and passes.
How enforcement works
Two tools, two scopes:
npm run format. Prettier has no changed-lines mode, so this is whole-file by design.The baseline is
eslint-suppressions.json, generated byeslint --suppress-all; it captures 963 current error-level findings. Because the rules stay at error rather than being downgraded, a new violation of any rule (including the react-hooks / React Compiler rules from eslint-config-next 16) blocks, while none of the existing ones force a PR author to clean up a legacy file they happen to touch. CI runs eslint with--pass-on-unpruned-suppressionsso that fixing a baselined issue, which leaves a now-stale suppression, does not fail the build. The baseline is added to.prettierignoresince eslint owns its formatting, and it is burned down over time witheslint --prune-suppressions.Explicit
anyis handled with a count budget rather than the baseline.@typescript-eslint/no-explicit-anyis a warning, so each newanyis visible but does not by itself fail a PR; a separate frontend-lint step counts the repo-wide total and fails when it exceeds the committed budget ineslint-budgets.json.maxstarts at 2031, ten above the current 2021, so there is a little headroom and the build only breaks once that is used up.maxis lowered over time towardtargetto drive the count down, and the step printsexplicit any: N | budget max: M | target: Teach run.Three zero-cost rules are also enabled, all with no existing violations so no baseline is involved:
react/no-danger(blocks newdangerouslySetInnerHTML, an XSS vector),no-var(enforceslet/const), andno-self-assign(catches self-assignment typos).no-debuggeris already enforced by the recommended preset.Baselined complexity rules are included as well:
complexity20,max-depth4,max-params4, andmax-nested-callbacks4, with thresholds set near the codebase p99 so only genuine outliers are flagged. The 272 existing over-threshold functions are grandfathered; new ones block, and the thresholds can be lowered over time.max-lines-per-functionis intentionally left off since React components are legitimately long.Raw
fetch()is banned via ano-restricted-syntaxrule that points contributors at React Query (@tanstack/react-query, already in the app). The rule is not exempted anywhere, including the already-bloatednetworking.tsx, so all 331 existingfetchcalls are grandfathered but no new ones can be added there or elsewhere; data access standardizes on React Query and the networking layer can be migrated out over time.useEffectis intentionally not rule-restricted: a blanket rule cannot tell a legitimate effect from a misused one, and the concrete misuse patterns are already covered by the baselined react-hooks rules.For context, across all 1,164 UI files, 84% currently have zero eslint errors, and of the rest the median is a single finding; the heaviest is one rule, react-hooks/set-state-in-effect, at 163 of the 318.
Summary: rules and baselines
The
frontend-lintjob runs three checks on each PR:eslint-suppressions.json(963)eslint-budgets.json(max 2040)anyexceeds the budgetError rules, existing violations grandfathered in the baseline so only new ones block:
no-restricted-syntax(raw fetch)fetch(); use React Queryno-restricted-imports(tremor)@tremor/reactimports; use antdreact-hooks/*(React Compiler)complexity(max 20)max-depth(max 4)max-params(max 4)unused-imports/no-unused-importsmax-nested-callbacks(max 4)react/display-name@typescript-eslint/no-require-importsrequire()importsreact/no-unescaped-entities>in JSX@typescript-eslint/no-this-aliasconst self = thisError rules with no existing violations (pure regression guards, no baseline):
react/no-danger(newdangerouslySetInnerHTML, an XSS vector),no-var,no-self-assign, andno-debugger(from the recommended preset).Explicit
anyis a warning under a count budget:max2040 (current 2021 plus ~19 of headroom),target1500. Newanys warn until the total crossesmax, then the build fails; lowermaxover time to drive the count down.complexityandmax-depthuse the same count-budget pattern as explicitany: the rule is a warning (visible, never blocks per file), and afrontend-lintstep counts the repo-wide total and fails only if it crosses a committed ceiling ineslint-budgets.json. Current ceilings:complexity129 of 140,max-depth61 of 70, explicitany2021 of 2040, each with atargetto ratchet down toward.max-params,max-nested-callbacks, and the react-hooks rules stay strict (hard-block new violations).Disabled (not enforced):
no-unused-vars,no-unused-expressions,ban-ts-comment,prefer-const,no-empty,no-prototype-builtins,no-useless-catch,no-useless-escape.Type
🚄 Infrastructure
Changes
Adds a
frontend-lintjob totest-litellm-ui-build.yml, parallel to the existing build job. It checks out with full history, computes the PR's added/modified files viagit diff --diff-filter=ACMR --relative <base>...HEADscoped toui/litellm-dashboard, splits them by extension (JS/TS to both prettier and eslint; json/css/md/yaml/html to prettier only), and fails if either tool reports an issue. Both tools run so a contributor sees all findings at once, intermediate file lists are written to$RUNNER_TEMP, and the job skipsnpm cientirely when no lintable UI files changed.Also adds the
eslint-suppressions.jsonbaseline so the eslint gate only blocks new violations. This does not reformat any existing files and does not changeeslint.config.mjs; enforcement is scoped to changed files only.