🧹 [code health] Fix ESLint warnings and remove anti-patterns#73
🧹 [code health] Fix ESLint warnings and remove anti-patterns#73APPLEPIE6969 wants to merge 1 commit intomainfrom
Conversation
🎯 What - Fixed `@typescript-eslint/no-explicit-any` errors in multiple files by applying proper types or `@ts-expect-error`. - Fixed `react-hooks/set-state-in-effect` lint warnings. After a previous code review, removed a timeout-based anti-pattern and instead explicitly suppressed the warning with `// eslint-disable-next-line` for essential UI initializations from synchronous stores. - Restored Google Fonts `<link rel="preconnect">` tags to fix a performance regression. - Fixed unused variable warnings by prefixing names with `_` or removing them. - Fixed unescaped entities and `@ts-ignore` comments. 💡 Why - Ensure the codebase meets ESLint standards and builds cleanly. - Improve types and safety. - Keep standard web performance practices intact. ✅ Verification - Ran `npm run lint` successfully with 0 errors. - Ran `npm run test` successfully with all 70 tests passing. ✨ Result - Cleaner, safer codebase with no build-breaking lint errors and optimized font loading. Co-authored-by: APPLEPIE6969 <242827480+APPLEPIE6969@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR systematically improves TypeScript type safety and ESLint compliance across the codebase by adding targeted lint suppressions for intentional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
lib/ai-utils.ts (1)
49-49: Consider moving import to top of file.The import statement for
ChatMessageis placed after theparseJSONFromAIfunction definition. While this works due to ES module hoisting, placing imports at the top of the file is the conventional style and improves readability.♻️ Suggested move
+import type { ChatMessage } from "./types.ts"; + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` export function parseJSONFromAI(text: string): any { // ... function body } - -import type { ChatMessage } from "./types.ts";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ai-utils.ts` at line 49, Move the type import for ChatMessage to the top of the file to follow conventional module ordering and improve readability; specifically, relocate the line importing ChatMessage so it appears before the parseJSONFromAI function (and other top-level code), keeping the import syntax intact and ensuring no other code is reordered or altered.app/quiz/generator/page.tsx (1)
36-36: Type assertion may be unnecessary.The
LANGUAGESconstant is already typed as{ value: string; label: string }[](viaas constinlib/constants.ts), which is structurally compatible withSelectOption[]. Inapp/profile/page.tsx(line 186),LANGUAGESis passed directly toSelectwithout any cast.Consider removing the assertion for consistency:
♻️ Suggested change
-const languageOptions = LANGUAGES as { value: string; label: string }[]; +const languageOptions = LANGUAGES;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/quiz/generator/page.tsx` at line 36, Remove the redundant type assertion on LANGUAGES: replace the explicit cast used to define languageOptions with a direct reference to the existing LANGUAGES constant (which already matches the { value: string; label: string }[] shape), and pass that direct value to the Select component; update the code that defines languageOptions (currently using LANGUAGES as { value: string; label: string }[]) so it simply uses LANGUAGES (or remove languageOptions and pass LANGUAGES directly to Select) to match the usage in app/profile/page.tsx and avoid the unnecessary assertion.app/api/tutor/live/route.ts (1)
64-67: Consider using the canonicalChatMessagetype for stricter validation.The inline type annotation
{ role: string; content: string }is looser than theChatMessageinterface defined inlib/types.ts, which constrainsroleto"user" | "ai" | "system". Using the canonical type would catch invalid role values at compile time.♻️ Suggested improvement
+import type { ChatMessage } from "@/lib/types"; // ... then in the map: - ...history.map((msg: { role: string; content: string }) => ({ + ...history.map((msg: ChatMessage) => ({ role: msg.role === "ai" ? "model" : "user", parts: [{ text: msg.content }], })),Note: The
historyfromformData(line 34) is unvalidated JSON, so runtime validation may also be warranted to ensure it conforms toChatMessage[]before use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/tutor/live/route.ts` around lines 64 - 67, The inline type { role: string; content: string } used when mapping history should be replaced with the canonical ChatMessage type from lib/types.ts (e.g., change the map signature to use ChatMessage) and add runtime validation of the parsed history variable (from formData) to ensure it is a ChatMessage[] before mapping; validate each message.role is one of "user"|"ai"|"system" and message.content is a string, then perform the existing mapping (role -> "model"|"user" and parts construction) only after validation to avoid runtime errors from untrusted JSON.lib/security.ts (2)
34-41: Explicit hex format validation is a defensive improvement, but not critical with the current length check.Node.js
Buffer.from(signature, 'hex')does truncate on invalid hex characters or odd-length input instead of throwing. However, the current code's length comparison (line 37–39) catches any buffer size mismatch resulting from truncation, sinceexpectedBufferis always 32 bytes from SHA256. Adding explicit format validation before decoding strengthens the implementation as a defense-in-depth measure:try { const expectedSignature = signData(data); + if ( + signature.length !== expectedSignature.length || + !/^[0-9a-f]+$/i.test(signature) + ) { + return false; + } const signatureBuffer = Buffer.from(signature, 'hex');This makes the hex validation explicit and avoids relying on the downstream length check alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security.ts` around lines 34 - 41, Add an explicit hex-format validation before decoding the incoming signature: check that the incoming signature string (the variable signature) matches the expected hex pattern/length (e.g., 64 hex chars for a SHA-256 hex) and return false immediately if it does not, then proceed to Buffer.from(...) and the existing length/timingSafeEqual check; update the logic around signature, expectedSignature and crypto.timingSafeEqual to perform this defensive validation prior to decoding.
7-8: Replace broadanytype withJsonValuefor exported crypto functions.Both
signDataandverifyDataexplicitly useJSON.stringify(data), which only accepts JSON-serializable types. Defining a constrainedJsonValuetype eliminates the need for the lint disable and prevents accidental non-serializable inputs at compile time. All current usages (test data objects and API response data) are JSON-serializable, making this refactor safe and beneficial.♻️ Suggested typing refactor
+type JsonValue = + | string + | number + | boolean + | null + | { [key: string]: JsonValue } + | JsonValue[]; + -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -export function signData(data: any): string { +export function signData(data: JsonValue): string { ... -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -export function verifyData(data: any, signature: string): boolean { +export function verifyData(data: JsonValue, signature: string): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security.ts` around lines 7 - 8, Change the exported crypto helpers to accept a JSON-serializable type instead of any: replace the parameter type for signData(data: any) and verifyData(data: any) with a constrained JsonValue (or equivalent union type) and remove the "// eslint-disable-next-line `@typescript-eslint/no-explicit-any`" comment; update/declare a JsonValue type (or import one) and ensure both signData and verifyData use it so the functions still call JSON.stringify(data) but now enforce compile-time JSON-serializability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/api/tutor/live/route.ts`:
- Around line 64-67: The inline type { role: string; content: string } used when
mapping history should be replaced with the canonical ChatMessage type from
lib/types.ts (e.g., change the map signature to use ChatMessage) and add runtime
validation of the parsed history variable (from formData) to ensure it is a
ChatMessage[] before mapping; validate each message.role is one of
"user"|"ai"|"system" and message.content is a string, then perform the existing
mapping (role -> "model"|"user" and parts construction) only after validation to
avoid runtime errors from untrusted JSON.
In `@app/quiz/generator/page.tsx`:
- Line 36: Remove the redundant type assertion on LANGUAGES: replace the
explicit cast used to define languageOptions with a direct reference to the
existing LANGUAGES constant (which already matches the { value: string; label:
string }[] shape), and pass that direct value to the Select component; update
the code that defines languageOptions (currently using LANGUAGES as { value:
string; label: string }[]) so it simply uses LANGUAGES (or remove
languageOptions and pass LANGUAGES directly to Select) to match the usage in
app/profile/page.tsx and avoid the unnecessary assertion.
In `@lib/ai-utils.ts`:
- Line 49: Move the type import for ChatMessage to the top of the file to follow
conventional module ordering and improve readability; specifically, relocate the
line importing ChatMessage so it appears before the parseJSONFromAI function
(and other top-level code), keeping the import syntax intact and ensuring no
other code is reordered or altered.
In `@lib/security.ts`:
- Around line 34-41: Add an explicit hex-format validation before decoding the
incoming signature: check that the incoming signature string (the variable
signature) matches the expected hex pattern/length (e.g., 64 hex chars for a
SHA-256 hex) and return false immediately if it does not, then proceed to
Buffer.from(...) and the existing length/timingSafeEqual check; update the logic
around signature, expectedSignature and crypto.timingSafeEqual to perform this
defensive validation prior to decoding.
- Around line 7-8: Change the exported crypto helpers to accept a
JSON-serializable type instead of any: replace the parameter type for
signData(data: any) and verifyData(data: any) with a constrained JsonValue (or
equivalent union type) and remove the "// eslint-disable-next-line
`@typescript-eslint/no-explicit-any`" comment; update/declare a JsonValue type (or
import one) and ensure both signData and verifyData use it so the functions
still call JSON.stringify(data) but now enforce compile-time
JSON-serializability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07f5f177-6b55-425f-9944-fa38092d9b4a
📒 Files selected for processing (19)
app/api/tutor/live/route.tsapp/courses/create/page.tsxapp/create/page.tsxapp/dashboard/page.tsxapp/profile/page.tsxapp/quiz/[id]/page.tsxapp/quiz/generator/page.tsxapp/quizzes/page.tsxapp/schedule/page.tsxcomponents/VoiceInput.tsxlib/ai-utils.tslib/auth.tslib/i18n-utils.tslib/i18n.tsxlib/ratelimit.test.tslib/security.test.tslib/security.tslib/setupTests.tsmiddleware.ts
💤 Files with no reviewable changes (1)
- app/quiz/[id]/page.tsx
1 participant
🎯 What
This PR addresses several code health issues identified by the ESLint configuration:
anywith specific types or suppressed them safely using@ts-expect-errorwhere the types were genuinely missing or complex.react-hooks/set-state-in-effectrule by strategically adding// eslint-disable-next-linewhere state setting from synchronous logic insideuseEffectis required and safe for initialization. (Removed a previous anti-pattern that usedsetTimeoutto mask this).preconnecttags forfonts.googleapis.comto maintain optimal font loading performance._prefix applied where the variable is required by the function signature but unused).react/no-unescaped-entitiesand replaced outdated@ts-ignorewith@ts-expect-error.💡 Why
eslinterrors.✅ Verification
npm run lintreports 0 errors.npm run testreports 100% test success across 70 tests.✨ Result
PR created automatically by Jules for task 17931064704645035311 started by @APPLEPIE6969
Summary by CodeRabbit
Bug Fixes
Refactor