Repository navigation
chore(ci): lint the frontend with oxlint - #33
Conversation
Add oxlint with a React/TypeScript config and run `npm run lint` in the `build` job right after `npm ci`. typescript-eslint is not usable here: package.json pins TypeScript 7 (native), typescript-eslint peers on typescript <6.1.0 and its parser throws "does not support TS 7.0". oxlint parses TS itself, needs no compiler API, and ships the React hooks rules. correctness rules are errors, plus react/rules-of-hooks. Two React Compiler style rules (set-state-in-effect, refs) flag 8 existing sites, so they are warnings until the code is reworked; the lint run is green. Refs #7 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lint policy updateI've expanded the original Oxlint configuration to better reflect the needs of our React/TypeScript frontend. Changes to the lint policy:
The expanded policy initially produced a large number of diagnostics, including many caused by rules unsuitable for our setup. After refining the configuration, the latest run evaluates 179 rules across 48 files, reporting 29 existing warnings and no application-code errors. An intentionally introduced I've added The remaining warnings are intentionally nonblocking for now. They concern existing React state management, effect dependencies, accessibility, type imports, and variable shadowing. I'll address them in a separate follow-up PR rather than mixing application refactoring into this CI integration. This establishes a documented and actionable lint baseline without requiring unrelated changes to existing components. |
kctong529
left a comment
There was a problem hiding this comment.
Final review — Approved
Thanks for setting up Oxlint and integrating it into our frontend CI!
I've completed the local verification and reviewed the expanded lint configuration.
Verification:
npm run lint— passes with no errors; 29 existing warnings remain.- Negative lint test — confirmed that an intentionally introduced
debuggerstatement is detected as an error. npm run build— passed.npm run test:typecheck— passed.- GitHub Actions — all checks passed, including the Playwright acceptance tests.
The lint policy and coding conventions are now documented in docs/coding-conventions.md, with a reference from the README.
My local Playwright run encountered backend connection errors, likely due to differences in environment configuration. Since the acceptance suite passes in CI, I don't consider this a blocker for the PR.
No further blocking comments. Approved for merge.
Suggested follow-up
Kane, would you be willing to address the remaining 29 lint warnings in a separate branch and PR?
Feel free to use coding agents to assist with the refactoring, but please review their changes carefully. The goal is to improve code quality, not simply eliminate lint warnings.
A few things I'd like us to prioritize:
- Modularity and readability: Keep components and functions focused on clear responsibilities. Split large files into smaller modules when appropriate, without introducing unnecessary abstractions.
- Correctness: Address the underlying issues, particularly React state management, effect dependencies, ref usage, and accessibility. Avoid suppressing warnings or introducing workarounds solely to satisfy the linter.
- Documentation: Add concise inline comments explaining non-obvious behavior, and update relevant Markdown documentation when implementation decisions or component responsibilities change. Avoid excessive or redundant documentation.
- Testing: Preserve existing behavior and add focused regression tests where refactoring affects interactions, state management, or asynchronous operations.
- Small, focused commits: I strongly prefer incremental changes, ideally addressing one warning, component, or closely related concern per commit. Avoid modifying multiple unrelated files in the same commit. When changes across several files are necessary, keep them tightly related and explain the dependency.
- Reviewability: Run relevant checks after each change, keep the commit history easy to follow, and avoid combining unrelated refactors into a single PR.
There's no need to resolve all 29 warnings in one PR. We can split the work into smaller PRs, particularly if accessibility improvements and React state-management refactoring become substantial.
I'd especially appreciate avoiding large agent-generated commits that modify many files at once. Smaller, independently testable changes make it much easier to review the reasoning, identify regressions, and revert individual changes if necessary.
The objective is a cleaner, more modular, and maintainable frontend—not simply a lint report with zero warnings.
Thanks again for your work on this!
Refs #7. The remaining box, "link a passing run", can be ticked with this PR's CI run.
npm run lint(oxlint) and a Lint step right afternpm ciin thebuildjob. The README development table lists the new command.typescript <6.1.0, and its parser throws "does not support TS 7.0". oxlint parses TS/JSX itself, needs no compiler API, and ships the React hooks rules. It is one devDependency; the lockfile only grows by its per-platform binaries..oxlintrc.json):correctnessrules are errors, plusreact/rules-of-hooks.set-state-in-effectandrefs. They flag 8 existing sites, which are design-level, so I didn't refactor them here:App.tsx:77PlaceSearch.tsx:37PlaceTrips.tsx:58DashboardScreen.tsx:205TripScreen.tsx:319MapScreen.tsx:98and:104UploadPanel.tsx:128Checked locally:
npm ci,npm run lint(0 errors, 8 warnings),npm run buildandnpm run test:typecheckall pass.useStateand adebuggerstatement both fail lint as expected.🤖 Generated with Claude Code