Restore frontend linting with ESLint 10 - #9
Conversation
Linting was dropped in the 3.0 monorepo consolidation; both 2.x repos carried ESLint. This restores the gate with a modern flat-config stack: ESLint 10 plus @eslint-react, which supersedes eslint-plugin-react and bundles its own hooks rules. exhaustive-deps is error-level in assets/client and assets/shared (the screen client is the production-bug surface) and warn elsewhere pending burn-down. typescript is bumped to ^5.9 (required by @eslint-react's TS API usage) and @types/react is pinned to ^18 with an overrides entry, so the React types resolve deterministically against the react@18 runtime.
The directives reference airbnb-era rules (react/prop-types, jsx-a11y/*, import/*, no-restricted-syntax, ...) that the flat config does not carry, so ESLint reports them as unused directives or missing rule definitions. Comment removals only.
Drops dead imports (React under the automatic JSX runtime, JSX imported only for a JSDoc reference, unused Playwright fixtures), dead destructured parameters, and dead initialisers, and switches two unused catch bindings to the optional catch binding. Also sets no-unused-vars' ignoreRestSiblings back to true: ESLint 10 flipped the default, and destructuring a prop out before spreading the rest is the omit-a-prop pattern used throughout the admin HOCs.
Adds keys to mapped lists in the poster selector, the dropzone rejection list and the screen-layout template builder. In the react-table wrapper the key already came from getHeaderGroupProps()/getRowProps()/ getCellProps() via prop spreading; it is now destructured out and passed explicitly, which is also what React 18.3 wants.
…nt errors Drops a redundant optional chain in the news-feed template (the branch is already guarded by an instanceof check), swaps hasOwnProperty for Object.hasOwn in enhanced-api, and adds justified suppressions where the rules do not fit the library APIs: createRef builds one ref per item for react-transition-group's nodeRef, and react-table's Cell is a column renderer, not a nested component.
The column helpers call useTranslation/useContext, so they are custom hooks; the get-prefix hid that from rules-of-hooks. They are already called unconditionally at the top level of the column HOCs and list components, so this is a rename only.
media-modal and the RSS template call useEffect after an early return, so hook order changes between renders. Both are real defects that need a restructure plus regression tests, which is out of scope for a tooling change — suppressed with a @todo so the gate can go green.
region and touch-region now depend on regionId, which is derived from the region prop and fixed for a mounted region, so the listener registration and the regionReady event stay correct if it ever changes. The remaining client findings are suppressed with a rationale: the App boot sequence must run exactly once, and the two region effects that read currentSlide would change when queued slides swap in — that needs its own change with client tests. Slide templates (assets/shared/templates, custom-templates-example) build a BaseSlideExecution on every render and start it from a mount-only effect; satisfying the rule there means reworking the slide lifecycle across every template, so they stay at warn for now.
Follow-up notes — fallout from this PRFile these as issues, then replace the Blocking-ish (real defects, suppressed to get the gate green)
Burn-downs (warnings today, promote to error per directory as they clear)
From the plan
Items 7, 9 and 10 concern the type-check ratchet and are on that PR instead. |
CI gate strengthening, rankedEvaluation of static analysis, linting, code-quality tools and testing frameworks against the Root-cause classes in the twelve bugs
Cross-cutting: four of the twelve ( The ranking
Notes on the ones that need more than a table rowRank 2 — Vite asset-resolution gate. Vite left Rank 6 — Spec-derived fixtures. Rank 7 — Playwright E2E. The infrastructure already exists (28 specs, CI workflow, Rank 9 — Coverage. Currently not configured at all: Evaluated and rejected
Honest gaps
Suggested sequence
|
There was a problem hiding this comment.
AI Review
Verdict: approve with changes. Solid PR — clean commit separation, honest scoping, real defects found. The overrides block is the best-built part: single @types/react 18.3.31 across the tree despite @tiptap/react peering ^17||^18||^19, and react-intl.typescript: "$typescript" handles the optional TS peer properly.
Blocking:
lint:js has no --max-warnings. 181 exhaustive-deps + 19 leak warnings float free, so the burn-down plan has no enforcement. One flag.
.ts is in the glob with no TS parser installed. Green today only because enhanced-api.ts happens to contain no TS syntax — the first annotation added there breaks lint with a parse error, and the checkJs PR invites exactly that. Install typescript-eslint, or drop ts,tsx from script and config.
Commit 45ea7a7 changes screen-client behaviour, untested, in the code that produced 523. Needs a second reviewer; you're sole participant.
Should fix before merge:
4. set-state-in-effect: "off" blinds you to the 513/524 class — your second-largest bug family. Make it "warn".
5. Add linterOptions: { reportUnusedDisableDirectives: "error" } — starts at zero, protects commit 2's cleanup.
6. Config block 2 omits ts,tsx, so enhanced-api.ts sits at warn while its neighbours are error.
7. dom-no-dangerously-set-innerhtml → error now; 2 sites, and react/no-danger was just removed from one.
8. Taskfile: eslint --fix before Prettier, not after.
9. Workflow: npm ci over npm install; add permissions: contents: read and a concurrency group. No engines field while ESLint 10 needs Node ^20.19||>=22.
Notes: @eslint-react peers are typescript: "*", so the commit message's "^5.9 required" isn't a declared constraint. Type-aware machinery (project-service, type-utils, typescript-estree) already ships transitively — enabling recommended-typescript is one package plus config, cheaper than the PR implies. e92edd2 changes reconciliation identity; flag as behavioural. Glob is assets/ only, so config files are ungated — relevant to the 392/507 drift class. Unrelated: @vitejs/plugin-react-oxc is in prod deps, and react-bootstrap 1.6.8 (Bootstrap 4) runs against bootstrap 5.3.7.
2 participants
Restores frontend linting, dropped in the 3.0 monorepo consolidation, as a CI gate.
Changes
eslint.config.mjs) with@eslint-react,eslint-config-prettier@types/reactto 18 via npmoverridesso type resolution is deterministicprototype access, ~21 stale 2.x
eslint-disabledirectivesgetXxxColumnshelpers touseXxxColumns(they call hooks)Why
eslint-plugin-reactpeer-caps at ESLint 9, so the modern successor@eslint-reactis usedinstead; its recommended config bundles the hooks rules.
Two classes are left as warnings rather than blocking this PR:
exhaustive-deps(181) and theleaked timer/fetch/listener rules. Conditional-hook errors are suppressed with
@TODOmarkers.Follow-ups in a comment below.