Skip to content

chore(frontend): add ESLint config, fix findings, and gate lint in CI - #1353

Merged
cristim merged 1 commit into
mainfrom
chore/frontend-eslint-config
Jul 3, 2026
Merged

chore(frontend): add ESLint config, fix findings, and gate lint in CI#1353
cristim merged 1 commit into
mainfrom
chore/frontend-eslint-config

Conversation

@cristim

@cristim cristim commented Jul 3, 2026

Copy link
Copy Markdown
Member

What was missing

frontend/package.json declared "lint": "eslint src/**/*.ts" with eslint ^8.50 and @typescript-eslint ^6 as dev-dependencies, but no ESLint config file existed anywhere in frontend/. Running npm run lint produced "Oops! Something went wrong!" and exited non-zero. ESLint was also absent from the CI workflow (frontend-build.yml).

Config format decision

Uses the legacy .eslintrc.json format (ESLint 8.x default). This works under both @typescript-eslint v6 (current main) and v8 (pending PR #1345, which bumps @typescript-eslint while keeping eslint ^8.50). ESLint 8.x picks up legacy config automatically without any opt-in flag. Flat config (eslint.config.js) is opt-in on ESLint 8 and belongs with a future ESLint 9 upgrade — that migration is out of scope here.

Config choices:

  • root: true — prevents ESLint from walking up to any parent config
  • @typescript-eslint/parser with ecmaVersion: 2020, sourceType: module
  • plugin:@typescript-eslint/recommended (non-type-aware; no parserOptions.project) — avoids the performance cost of type-checked rules on a codebase of this size; the type-aware ruleset can be added incrementally later
  • env: browser, jest, es2020

.eslintignore added to exclude webpack.config.js (CommonJS, not in src/) and dist/.

Rules adjusted and why

Rule Config Reason
@typescript-eslint/no-explicit-any "warn" (downgraded from default "error") The codebase uses any heavily at DOM and API boundaries across ~90 sites; fixing all of them is a separate undertaking outside this PR's scope
@typescript-eslint/no-unused-vars ["error", { argsIgnorePattern: "^_", varsIgnorePattern: "^_" }] Keeps the recommended error but permits the standard underscore-prefix convention for intentionally-unused parameters
@typescript-eslint/no-var-requires "off" in overrides for src/__tests__/**/*.ts Jest's module-mocking pattern (jest.mock(...) + require() inside test bodies/hooks) requires calling require() at runtime after the mock is installed; static ESM import cannot replicate this pattern
@typescript-eslint/no-require-imports "off" in overrides for src/__tests__/**/*.ts Same reason as above (alias for the same rule family)

All other plugin:@typescript-eslint/recommended rules remain at their default severity.

Source fixes (zero blanket suppressions)

File Fix
src/settings.ts:161 let savedSnapshotconst savedSnapshot (prefer-const: never reassigned, only mutated via index)
src/users/state.ts:20 export let selectedUserIdsexport const selectedUserIds (prefer-const: Set is only mutated via .add/.clear/.delete, never reassigned)
src/__tests__/recommendations.test.ts:1296 firstRow?.querySelector(...)!firstRow!.querySelector(...)! (fixes no-non-null-asserted-optional-chain: optional chain combined with non-null assertion is contradictory; the row is guaranteed to exist after loadRecommendations())
src/users/userList.ts:242,341 Replaced // eslint-disable-next-line no-unsanitized/property with plain explanatory comments. ESLint 8 reports an error for disable comments referencing rules from uninstalled plugins; eslint-plugin-no-unsanitized is not a project dependency

CI change

Added a Lint step to .github/workflows/frontend-build.yml before the existing TypeScript typecheck step, matching the existing step style.

Verification

npm run lint      exit 0  (0 errors, 89 warnings — all no-explicit-any)
npm run typecheck exit 0
npm run build     exit 0  (webpack compiled with 1 warning — stale browserslist)
npm test          exit 0  (2553 passed, 1 skipped, 77 suites)

Note on PR #1345 lockfile

This PR adds no new npm dependencies (config only). The package-lock.json is untouched. PR #1345 (which bumps @typescript-eslint to ^8) will produce its own lockfile update; the two do not conflict since the .eslintrc.json format used here is compatible with both v6 and v8 under ESLint 8.x.

Closes #1350

Summary by CodeRabbit

  • Chores
    • Added frontend linting to the PR build checks, so code issues are caught earlier.
    • Improved ESLint setup for the frontend to better handle TypeScript and generated files.
  • Tests
    • Tightened a frontend test to fail more clearly when expected UI elements are missing.
  • Refactor
    • Made a few internal state values more stable to prevent accidental reassignment.

@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/low Minor harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible labels Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4105cf6b-66ae-44e6-b756-533d36925b96

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an ESLint configuration (.eslintrc.json) and ignore file (.eslintignore) for the frontend, wires a lint step into the CI workflow before typecheck/build, and applies minor code changes (const bindings, updated comments, non-null assertion) to satisfy lint rules.

Changes

ESLint setup and lint fixes

Layer / File(s) Summary
ESLint configuration and ignore rules
frontend/.eslintrc.json, frontend/.eslintignore
Adds TypeScript-aware ESLint config with recommended rules, custom no-explicit-any/unused-vars levels, a test-file override, and an ignore file excluding dist/, node_modules/, and webpack.config.js.
CI lint step wiring
.github/workflows/frontend-build.yml
Adds a "Lint" step running npm run lint after npm ci and before typecheck/build.
Lint-driven code fixes
frontend/src/settings.ts, frontend/src/users/state.ts, frontend/src/users/userList.ts, frontend/src/__tests__/recommendations.test.ts
Converts savedSnapshot and selectedUserIds from let to const, replaces eslint-disable suppressions in userList.ts with explanatory comments, and replaces optional chaining with a non-null assertion in a test.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • LeanerCloud/CUDly#160: Both PRs touch frontend/src/__tests__/recommendations.test.ts, with the retrieved PR substantially rewriting the recommendations test suite that this PR's checkbox check change relates to.
  • LeanerCloud/CUDly#232: Both PRs modify .github/workflows/frontend-build.yml to change frontend CI steps.
  • LeanerCloud/CUDly#283: Overlaps directly with the retrieved PR's modifications to the same recommendations test suite touched here.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main frontend ESLint and CI lint-gating changes.
Linked Issues check ✅ Passed The PR adds ESLint config, fixes src lint findings, and wires lint into CI, matching #1350.
Out of Scope Changes check ✅ Passed The changes stay focused on ESLint setup, lint fixes, and CI gating, with no unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/frontend-eslint-config

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Observation for a possible follow-up: the removed eslint-disable-next-line no-unsanitized/property directives were vestigial (the plugin was never installed, so the rule never ran). Given the repo's standing innerHTML/XSS review rule, actually installing eslint-plugin-no-unsanitized would put a real gate behind those sites - worth considering as a small follow-up to #1350.

@cristim

cristim commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 60526ce into main Jul 3, 2026
14 of 17 checks passed
@cristim
cristim deleted the chore/frontend-eslint-config branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(frontend): add missing ESLint config and wire lint into CI (salvaged from #1331)

1 participant