chore: fix all linting errors and enforce zero-warning policy for CI#667
chore: fix all linting errors and enforce zero-warning policy for CI#667CodeWithCJ merged 2 commits intoCodeWithCJ:mainfrom
Conversation
Summary of ChangesHello @Sim-sat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the frontend's code quality and maintainability by systematically addressing a large volume of ESLint warnings. The primary focus was on managing the widespread use of the Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a great step towards improving code quality by addressing a large number of linting errors and enforcing a zero-warning policy. The main approach of suppressing no-explicit-any and react-hooks/exhaustive-deps errors is a pragmatic way to establish a clean baseline. My review focuses on a few key areas where these suppressions might hide underlying issues or where a better fix is readily available. I've highlighted potential bugs related to React hooks dependencies, suggested type safety improvements, and pointed out a couple of other potential issues. Moving forward, it would be beneficial to create follow-up tasks to gradually address the suppressed any types by introducing proper typing, which will make the codebase more robust and maintainable.
| }, []); | ||
|
|
There was a problem hiding this comment.
Suppressing the exhaustive-deps rule can lead to bugs due to stale closures. This useEffect has an empty dependency array but uses maxY and setDefaultPosition from the component's scope. These functions should be included in the dependency array, and to prevent re-renders, they should be memoized with useCallback or useMemo.
This issue is present in multiple places in this file. Please address all instances to ensure the component behaves as expected.
There was a problem hiding this comment.
Yes of course but we can't fix 150 bugs at once
| try { | ||
| await this.html5QrCode.clear(); | ||
| } catch (e) {} | ||
| this.html5QrCode.clear(); |
There was a problem hiding this comment.
The clear() method on html5QrCode returns a Promise. By not await-ing it, you risk unhandled promise rejections if the operation fails. The original try...catch with await was safer. It's best to restore the await and handle potential errors.
try {
await this.html5QrCode.clear();
} catch (e) {
console.error('Error clearing html5-qrcode on start failure:', e);
}There was a problem hiding this comment.
not true doesn't return a promise. using await here is wrong
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| message: string | ((...args: any[]) => string) | undefined, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ...args: any[] | ||
| ): string | undefined => { |
There was a problem hiding this comment.
While suppressing the no-explicit-any lint rule works, a better long-term solution is to use the unknown type for improved type safety. This avoids the need for eslint-disable and makes the function signature more robust.
message: string | ((...args: unknown[]) => string) | undefined,
...args: unknown[]
): string | undefined => {
There was a problem hiding this comment.
cant fix 400 errors at once
|
|
||
| setEngineInstance(engine); | ||
| try { | ||
| localStorage.setItem('barcodeScannerEngine', selectedEngine); | ||
| } catch {} | ||
| localStorage.setItem('barcodeScannerEngine', selectedEngine); |
There was a problem hiding this comment.
Removing the try...catch block around localStorage.setItem could lead to unhandled exceptions if localStorage is unavailable (e.g., in private browsing mode, or if disabled by the user). It's safer to wrap this call in a try...catch and log any errors, rather than letting them crash the application or failing silently.
try {
localStorage.setItem('barcodeScannerEngine', selectedEngine);
} catch (e) {
console.warn('Failed to save barcodeScannerEngine to localStorage', e);
}
There was a problem hiding this comment.
the catch was empty so it no point in using it
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (prefs as any).include_bmr_in_net_calories || false | ||
| ); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const age = (userProfile as any)?.date_of_birth | ||
| ? new Date().getFullYear() - | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| new Date((userProfile as any).date_of_birth).getFullYear() | ||
| : 0; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const gender = (userProfile as any)?.gender; | ||
|
|
||
| if ( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (prefs as any).bmr_algorithm && | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (mostRecentWeight as any)?.weight && | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (mostRecentHeight as any)?.height && | ||
| age && | ||
| gender | ||
| ) { | ||
| try { | ||
| const bmrValue = calculateBmr( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (prefs as any).bmr_algorithm as BmrAlgorithm, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (mostRecentWeight as any).weight, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (mostRecentHeight as any).height, | ||
| age, | ||
| gender, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (mostRecentBodyFat as any)?.body_fat_percentage | ||
| ); |
There was a problem hiding this comment.
There are numerous as any type assertions in this block when accessing properties from prefs, userProfile, mostRecentWeight, etc. This undermines type safety and can hide potential bugs. It would be better to define proper interfaces for these objects (e.g., UserProfile, UserPreferences) and use them for type checking. This would make the code safer, more readable, and easier to maintain.
| // eslint-disable-next-line react-hooks/immutability | ||
| currentCumulativeDistance += lapDistance; | ||
| currentCumulativeDuration += lapDurationMinutes; |
There was a problem hiding this comment.
Mutating variables (currentCumulativeDistance, currentCumulativeDuration) inside a .map() function is a side effect that makes the code harder to understand and debug. The .map() function should ideally be pure. It's better to calculate these cumulative values in a separate step, for example, by using a forEach loop before mapping or by using reduce to build the new array with cumulative values.
|
Gemini doesn't understand the point of the pr... |
|
@Sim-sat 
|
|
@CodeWithCJ Yes, they suppress the linting errors. They are a message to eslint. If you don't know eslint it's like a strict spellchecker. It's extremly strict and errors don't always mean something broken, just that's it's not the best way of doing something. However we should still strive to remove these in the future. Fixing all 600 of them would take forever. Now we can run the check in the CI to at least prevent new errors from coming in. |
|
Thanks. I thought the commented out is a section where the code doesn't get executed. I use that to disable a code or make some notes for me. didnt know about this. thats why I wondered why swagger api doc were all in commented out section. |
|
@CodeWithCJ Yes you have to get used to it. It's not really code thats getting executed, it's just a way to communicate with developments tool like swagger, eslint or prettier that have the goal to make development easier, but don't influence the code |
2 participants
Description