feat(lint): run ESLint on PRs, and cover every package (#2923) - #2928
Merged
Conversation
`lint.yml` was `workflow_dispatch`-only, so ESLint had never gated a PR. That mattered more than it looked: eslint.config.js sets three `object-ui/*` rules to `error` *specifically* so a new violation fails CI — ADR-0054 Phase 5's no-synthetic-event-trigger, #2879's no-try-catch-around-hook, and the objectql.ts no-inline-spec-config ratchet. Each author pre-cleaned the existing sites so the rule would "lint clean today". All three were inert, because nothing ran them. Same two-layer hole as #2911's type-check gate, and the second layer bites here too: `turbo run lint` silently skips packages with no `lint` script, and 7 of 45 had none — including `apps/console`, the largest surface in the repo. - lint.yml now runs on pull_request + push, keeping workflow_dispatch. - scripts/check-lint-coverage.mjs: every package must lint or be a declared gap, and the list only shrinks. Runs before install (reads package.json only). - lint scripts added to the 5 packages that were already clean: site, vscode-extension, and the three examples. Coverage 38 -> 43 of 45. - console (14 errors) and runner (3) declared in DEBT, tracked in #2927. These are pre-existing and were merely invisible; this does not turn them red. Two config-level fixes, both generated-or-misparsed rather than code debt — they are why `apps/site` looked like it had 7 errors and console 15: - ignore `**/.source` — fumadocs-mdx codegen for apps/site, already gitignored. Linting generated output only reports on the generator's choices. - ignore `**/tailwind.config.js` — authored in TypeScript despite the `.js` extension, so the base JS parser failed on `import type`. - drop a stale `eslint-disable-next-line @next/next/no-img-element` in apps/site: the Next plugin is not in the flat config, so the directive itself errored as an unknown rule. Replaced with a comment stating why a plain <img> is correct there. `--max-warnings` deliberately not set: the 7,724 warnings repo-wide are 81% no-explicit-any, plus React Compiler rules the config downgrades on purpose. This gate is about errors. Proven before being trusted — planting a hook inside try/catch in a linted package now yields `pnpm lint` exit 1 and `Failed: @object-ui/i18n#lint`, which was impossible before. Coverage guard verified red in all three modes (new scriptless package, DEBT entry outliving its gap, script removed from a covered package). Refs #2911, #2923, #2927 Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2923. Follow-up: #2927 for the errors this declares.
The thing I got wrong in #2923, and why this PR exists
I originally filed #2923 recommending against wiring
lint.yml, on the grounds that it would be a no-op gate:pnpm lintreports 7,724 problems but 0 errors, and ESLint only fails on errors unless--max-warningsis set.The arithmetic was right; I had missed the bottom of
eslint.config.js. It sets threeobject-ui/*rules toerrorspecifically so a new violation fails CI:Three separate deliberate efforts, each written assuming CI enforces it, each with its existing violations pre-cleaned so the rule would "lint clean today". The enforcement they were built against did not exist. Same shape as #2911, one layer up: the rule is authored, nothing runs it.
And the same second layer, again
turbo run lintsilently skips packages with nolintscript — identical to the type-check hole. 7 of 45 packages had none, includingapps/console, the largest surface in the repo. So even once wired, the three ratchets would have been blind to console.Two config fixes, not code debt
Measuring the unlinted packages first turned up 25 "errors" — most of which were not real:
apps/site: 7 errors → 0. Six were inapps/site/.source/, which is fumadocs-mdx generated output and already gitignored (apps/site/.gitignore:5). Linting codegen only reports on the generator's choices. The seventh was a staleeslint-disable-next-line @next/next/no-img-element— the Next plugin isn't in the flat config, so the directive itself errored as an unknown rule. Replaced with a comment saying why a plain<img>is right for third-party badge endpoints.apps/console: 15 → 14. One wastailwind.config.js, authored in TypeScript (import type { Config }) despite the.jsextension; the TS parser block is scoped to.ts/.tsx, so the base JS parser choked on it. Now ignored as build config.This is the second time
apps/sitehas looked like debt purely because of generated artifacts — cf. #2924, where its 7 phantom type errors were missing.next/types.What is genuinely broken, and is not fixed here
Per the plan agreed for this PR, the real errors are declared, not fixed — they touch runtime behaviour and deserve their own review. Tracked in #2927:
apps/console— 14 errors. 8xreact-hooks/purity(Date.now()during render inApprovalsInboxPage), 4xreact-hooks/static-components(components created during render — "will reset", i.e. state loss / focus loss, and three of the four are Settings pages), plus a dead assignment on an auth path and aprefer-const.packages/runner— 3 errors.react-hooks/static-componentsinLayoutRendererat 70/106/168, same class.Note both
purityandstatic-componentsare at error severity becauseeslint.config.jsdowngrades five other React Compiler rules but deliberately not these two.These are pre-existing and were simply invisible. This PR does not turn them red — it records them in
DEBTso they cannot stay invisible.Why no
--max-warningsMeasured composition of the 7,724 warnings:
@typescript-eslint/no-explicit-anyreact-refresh/only-export-components@typescript-eslint/no-unused-varsreact-hooks/set-state-in-effectreact-hooks/exhaustive-depsreact-hooks/refsreact-hooks/*One stylistic rule is 81% of it, and five of the
react-hooks/*rules are downgraded on purpose ("codebase predates these rules"). A blanket cap of 7,724 would read as a health signal while trackingno-explicit-any. This gate is about errors; the warning question is left open in #2923's history rather than answered badly.Verification
The ratchets now actually bite. Planted a hook inside try/catch in
packages/i18n(a package that was already linted):Impossible before this PR, in two independent ways: the workflow never ran, and had it run, console still would not have been linted.
Coverage guard red in all three modes:
lintscriptlintscript removed from a covered packageBaseline restores green after each; restored via
cp, no residual diff.Green state:
No changeset: all five packages that gained a
lintscript areprivate: true, so no published package changes.🤖 Generated with Claude Code