chore: run eslint over test files and fix surfaced errors#374
Conversation
The `lint` script only scanned `src/`, so the jest config block for `tests/**` in eslint.config.mjs never actually ran. Extend the script to cover `tests/` and fix the 3 errors it surfaces: - domHook.js: setter used `return` for control flow, but a setter's return value is silently discarded (no-setter-return) -> use if/else - scroll.test.js: oversized deltaY literal lost precision (no-loss-of-precision) -> 1e20, same magnitude - drop dead `eslint-disable no-param-reassign` directives in domHook.js and useScrollTo.tsx (the rule isn't enabled)
|
@aojunhao123 is attempting to deploy a commit to the afc163's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the linting configuration in package.json to include the tests directory and additional file extensions, removes obsolete ESLint disable comments, simplifies a large number representation in tests, and refactors the spyElementPrototypes utility in tests/utils/domHook.js. Feedback on these changes suggests removing the deprecated --ext option from the ESLint command and adding a safety guard check in domHook.js to prevent potential runtime TypeError crashes when calling originDescriptor.set.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "deploy": "npm run gh-pages", | ||
| "gh-pages": "npm run compile && cross-env GH_PAGES=1 npm run docs:build && npm run docs:deploy", | ||
| "lint": "eslint src/ --ext .tsx,.ts", | ||
| "lint": "eslint src/ tests/ --ext .tsx,.ts,.jsx,.js", |
There was a problem hiding this comment.
In ESLint v9+ (and flat config mode, which is active here via eslint.config.mjs), the --ext command-line option has been deprecated and removed. Running eslint with --ext will result in a fatal error. Since flat config automatically determines which files to lint based on the files patterns in your configuration, you can safely remove the --ext option entirely.
| "lint": "eslint src/ tests/ --ext .tsx,.ts,.jsx,.js", | |
| "lint": "eslint src tests", |
| if (spyProp.set) { | ||
| return spyProp.set.call(this, originDescriptor, value); | ||
| spyProp.set.call(this, originDescriptor, value); | ||
| } else { | ||
| originDescriptor.set(value); | ||
| } |
There was a problem hiding this comment.
If originDescriptor is undefined (which happens if the property did not exist on Element.prototype originally) or if it is a data descriptor (which does not have a set method), calling originDescriptor.set(value) will throw a TypeError at runtime. Adding a guard check for originDescriptor?.set prevents potential test crashes.
| if (spyProp.set) { | |
| return spyProp.set.call(this, originDescriptor, value); | |
| spyProp.set.call(this, originDescriptor, value); | |
| } else { | |
| originDescriptor.set(value); | |
| } | |
| if (spyProp.set) { | |
| spyProp.set.call(this, originDescriptor, value); | |
| } else if (originDescriptor && originDescriptor.set) { | |
| originDescriptor.set(value); | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #374 +/- ##
==========================================
- Coverage 96.99% 96.87% -0.12%
==========================================
Files 19 19
Lines 831 832 +1
Branches 205 206 +1
==========================================
Hits 806 806
- Misses 25 26 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Summary
The
lintscript only scannedsrc/, so thejestconfig block fortests/**ineslint.config.mjsnever actually ran — test files were effectively unlinted. This extends the script to covertests/and fixes the errors that surface once it does:lintscript:eslint src/→eslint src/ tests/ --ext .tsx,.ts,.jsx,.js, so the existing (but dead) jest config block finally applies. CI runsut lint→ this same script, so coverage extends to CI automatically.tests/utils/domHook.js: the propertyset()usedreturn foo()for control flow, but a setter's return value is silently discarded (no-setter-return) → rewritten asif/else, behavior unchanged.tests/scroll.test.js:deltaY: 99999999999999999999lost precision at runtime (no-loss-of-precision) →1e20, same magnitude, still scrolls past the end.eslint-disable no-param-reassigndirectives indomHook.jsanduseScrollTo.tsx(the rule isn't enabled, so they were flagged as unused).Result:
npm run lintreports 0 errors acrosssrc/+tests/(14 pre-existingreact-hooks/exhaustive-depswarnings insrc/are unchanged — those deps are intentionally omitted).Test plan
npm run lint— 0 errorsnpm test— 279/279 passing🤖 Generated with Claude Code