🧹 [Code Health] Fix ESLint and TypeScript issues#75
🧹 [Code Health] Fix ESLint and TypeScript issues#75APPLEPIE6969 wants to merge 1 commit intomainfrom
Conversation
- Fixed react-hooks/set-state-in-effect issues using the mounted state pattern in pages reading from local storage (e.g., courses, dashboard, quizzes, schedule) and components (ThemeProvider). - Fixed a TS `any` lint error in the API route map function. - Updated `@ts-ignore` to more specific `@ts-expect-error` in setupTests, auth, and security.test.ts for NextAuth and test globals. 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.
|
Review Summary by QodoFix ESLint and TypeScript issues with mounted state pattern and type safety
WalkthroughsDescription• Fixed React set-state-in-effect ESLint errors using mounted state pattern • Replaced any type with concrete object type in API route • Migrated @ts-ignore to descriptive @ts-expect-error annotations • Improved type safety and eliminated linter warnings across codebase Diagramflowchart LR
A["ESLint/TS Issues"] --> B["Mounted State Pattern"]
A --> C["Type Safety"]
A --> D["Error Annotations"]
B --> E["Pages & Components"]
C --> F["API Route"]
D --> G["Auth & Tests"]
E --> H["Codebase Health"]
F --> H
G --> H
File Changes1. app/api/tutor/live/route.ts
|
📝 WalkthroughWalkthroughThe PR refactors loading state management across multiple page components to use a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Code Review by Qodo
1. Protected UI renders pre-redirect
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| useEffect(() => { | ||
| setMounted(true) | ||
| }, []) | ||
|
|
||
| const isLoading = !mounted || status === "loading" | ||
|
|
There was a problem hiding this comment.
1. Protected ui renders pre-redirect 🐞 Bug ⛨ Security
CreateCourse (and the same pattern in Schedule/Dashboard/MyQuizzes) now sets `isLoading = !mounted || status === "loading", so after mounted` flips true the page can render its main UI even when status === "unauthenticated" or onboarding is incomplete. Because the redirect happens in useEffect (post-paint), users can briefly see and interact with protected pages before navigation occurs.
Agent Prompt
### Issue description
Several pages redirect to `/login` or `/onboarding` inside `useEffect`, but their `isLoading` no longer accounts for those redirect conditions. After `mounted` becomes `true`, the main UI can render for at least one paint before the redirect effect runs.
### Issue Context
This was introduced by replacing explicit `isLoading` state updates with a `mounted` pattern. The redirect logic still happens in effects, so rendering must remain blocked while a redirect decision is pending.
### Fix Focus Areas
- app/courses/create/page.tsx[18-44]
- app/schedule/page.tsx[15-33]
- app/dashboard/page.tsx[69-112]
- app/quizzes/page.tsx[20-66]
### What to change
- Compute a `shouldRedirectToLogin` / `shouldRedirectToOnboarding` boolean (guarded by `mounted`), and include it in the render gate.
- Keep rendering the loading screen (or return `null`) while `shouldRedirect*` is true, so protected UI never renders before navigation.
Example pattern:
```ts
const email = session?.user?.email
const onboardingIncomplete = mounted && status === "authenticated" && email && !isOnboardingComplete(email)
const unauthenticated = mounted && status === "unauthenticated"
const isLoading = !mounted || status === "loading" || unauthenticated || onboardingIncomplete
useEffect(() => {
if (status === "unauthenticated") router.push("/login")
else if (status === "authenticated" && email && !isOnboardingComplete(email)) router.push("/onboarding")
}, [status, email, router])
```
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/dashboard/page.tsx (1)
63-100:⚠️ Potential issue | 🟠 MajorFix state-in-effect lint violation and prevent cached profile flash during auth redirects.
Line 95's
setIsMounted(true)violatesreact-hooks/set-state-in-effectand allowsgetUserProfile()(line 98) to execute before the auth effect (lines 71–92) can redirect unauthenticated users. This creates a brief window where cached stats from a previous session render before/loginor/onboardingnavigation completes. Remove theisMountedstate wrapper and defer profile loading until after authentication is confirmed, or reorganize effects to ensure redirects happen before any derived state reads from storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/dashboard/page.tsx` around lines 63 - 100, Remove the isMounted state and its setIsMounted usage; instead, gate profile retrieval and derived UI state on authenticated status so redirects run first. Concretely, delete the useState call for isMounted and the effect that calls setIsMounted(true), then move/getUserProfile() and any reads of profile (e.g., userStats, isLoading) into the authentication effect or a new effect that depends on status === "authenticated" and session; ensure the auth effect still performs router.push for unauthenticated or unfinished onboarding (status, session, router, isOnboardingComplete, recordActivity, isTutorialComplete) before calling getUserProfile() so cached profile data cannot flash during redirect.app/quizzes/page.tsx (1)
18-45:⚠️ Potential issue | 🟠 MajorThe mount gate doesn't prevent rendering before redirect completes.
When the component mounts, both effects run: the first calls
router.push()for unauthenticated or not-onboarded users (checked via localStorage inlib/userStore.ts:93-101), while the second immediately setsmounted=true. Once mounted,isLoadingflips to false (line 44), allowing the full quizzes page to render before therouter.push()navigation completes. The redirect is asynchronous and doesn't block the render cycle, creating a window where unauthorized users briefly see the quizzes shell before being redirected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/quizzes/page.tsx` around lines 18 - 45, The mounted flag is being set unconditionally which lets the page render before async router.push() completes; change the logic so mounted (or a new redirecting boolean) is only set true once you’ve verified no redirect is required: move the setMounted(true) into the authentication effect (the useEffect that checks status/session), or introduce a redirecting state that is set true before calling router.push()/router.replace and set false only after navigation resolves, and update isLoading to include redirecting; modify references to mounted, setMounted, isLoading, router.push/router.replace, isOnboardingComplete and getUserQuizzes accordingly so the UI doesn't render while a redirect is in-flight.app/schedule/page.tsx (1)
13-32:⚠️ Potential issue | 🟠 MajorWait to clear the spinner until both the session status and redirect logic have settled.
The
mountedstate clears the spinner (line 30) immediately on mount, but the first effect's redirect logic (lines 15-24) runs asynchronously—isOnboardingComplete()is a synchronous localStorage read (lib/userStore.ts:93-101), butrouter.push()doesn't block component render. Oncemounted=true, the spinner vanishes even if navigation is still in flight, allowing/scheduleto briefly render before the redirect completes. To fix this, block the spinner untilstatusis no longer"loading"OR until the redirects are guaranteed to have executed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/schedule/page.tsx` around lines 13 - 32, The spinner can clear too early because setMounted(true) runs on mount while the auth/redirect effect (which calls isOnboardingComplete() and router.push()) is still executing; fix by delaying mounted until the redirect logic has completed: remove the separate useEffect that sets mounted, and instead setMounted(true) at the end of the auth effect that checks status (inside the useEffect that reads status/session and calls isOnboardingComplete() and router.push()), or alternatively add a redirecting boolean (e.g., redirectPending) that you set to true immediately before any router.push() and false only after determining no navigation is required, and then compute isLoading = status === "loading" || !mounted || redirectPending so the spinner remains until redirects are handled.
🧹 Nitpick comments (1)
lib/auth.ts (1)
6-16: Prefer narrowing/removing this broad@ts-expect-erroron auth setup.Line 6 suppresses type errors for the entire
NextAuth(...)expression. That can mask future config/type regressions beyond the original issue.🔧 Refactor direction
-// `@ts-expect-error` Types may be incomplete or global variables need mocking -export const { handlers, signIn, signOut, auth } = NextAuth({ +const GOOGLE_CLIENT_ID = process.env.GOOGLE_CLIENT_ID; +const GOOGLE_CLIENT_SECRET = process.env.GOOGLE_CLIENT_SECRET; +const GITHUB_ID = process.env.GITHUB_ID; +const GITHUB_SECRET = process.env.GITHUB_SECRET; + +if (!GOOGLE_CLIENT_ID || !GOOGLE_CLIENT_SECRET || !GITHUB_ID || !GITHUB_SECRET) { + throw new Error("Missing OAuth environment variables"); +} + +export const { handlers, signIn, signOut, auth } = NextAuth({ providers: [ Google({ - clientId: process.env.GOOGLE_CLIENT_ID, - clientSecret: process.env.GOOGLE_CLIENT_SECRET, + clientId: GOOGLE_CLIENT_ID, + clientSecret: GOOGLE_CLIENT_SECRET, }), GitHub({ - clientId: process.env.GITHUB_ID, - clientSecret: process.env.GITHUB_SECRET, + clientId: GITHUB_ID, + clientSecret: GITHUB_SECRET, }), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/auth.ts` around lines 6 - 16, The broad `@ts-expect-error` on the entire NextAuth(...) call hides unrelated type issues; remove that broad suppression and instead declare a typed options object using NextAuthOptions (or the appropriate NextAuth types) and pass it into NextAuth, fixing any remaining provider or callback type mismatches by importing and using the provider config types (e.g. GoogleProviderConfig/GitHubProviderConfig) or explicit casts only where necessary; then export the resulting handlers, signIn, signOut, and auth from NextAuth(options). Ensure any remaining single-line unavoidable issues use targeted `@ts-expect-error` comments rather than a file-level suppression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/tutor/live/route.ts`:
- Around line 64-67: history is untrusted at runtime and the current inline
mapping treats any non-"ai" role as "user"; validate that history is an array
and each entry is an object with string role and content, filter out invalid or
empty items, normalize allowed roles only (map "ai" -> "model", leave "user" and
"system" as-is), and drop or log entries with other roles; then use the
validated list in the existing mapping code (reference the history variable and
the mapping block that produces { role: ..., parts: [{ text: ... }] }) so only
sanitized, normalized roles and string contents are emitted.
In `@components/ThemeProvider.tsx`:
- Around line 22-30: Validate the value returned from localStorage before
treating it as a Theme: don’t cast savedTheme to Theme directly—check that
savedTheme equals one of the allowed values (e.g., "light" or "dark") before
assigning it to initialTheme; if it’s invalid or missing, fall back to
window.matchMedia("(prefers-color-scheme: dark)") and then to the default
"light". Update the logic around savedTheme, initialTheme, and setThemeState (or
add a small isValidTheme helper) so only known Theme values are ever passed into
setThemeState and used as class names.
In `@lib/security.test.ts`:
- Around line 66-67: Update the misleading TypeScript suppression comment above
the failing call to verifyData so it reflects the actual test intent: instead of
mentioning "global variables need mocking", replace the `@ts-expect-error` message
to indicate that this test deliberately passes a non-string (an object) as the
second argument to verifyData to exercise error handling with invalid input
(e.g., "@ts-expect-error passing object instead of string to verifyData").
Ensure the comment is adjacent to the verifyData(testData, { not: 'a string' })
call so reviewers understand the intentional type mismatch.
---
Outside diff comments:
In `@app/dashboard/page.tsx`:
- Around line 63-100: Remove the isMounted state and its setIsMounted usage;
instead, gate profile retrieval and derived UI state on authenticated status so
redirects run first. Concretely, delete the useState call for isMounted and the
effect that calls setIsMounted(true), then move/getUserProfile() and any reads
of profile (e.g., userStats, isLoading) into the authentication effect or a new
effect that depends on status === "authenticated" and session; ensure the auth
effect still performs router.push for unauthenticated or unfinished onboarding
(status, session, router, isOnboardingComplete, recordActivity,
isTutorialComplete) before calling getUserProfile() so cached profile data
cannot flash during redirect.
In `@app/quizzes/page.tsx`:
- Around line 18-45: The mounted flag is being set unconditionally which lets
the page render before async router.push() completes; change the logic so
mounted (or a new redirecting boolean) is only set true once you’ve verified no
redirect is required: move the setMounted(true) into the authentication effect
(the useEffect that checks status/session), or introduce a redirecting state
that is set true before calling router.push()/router.replace and set false only
after navigation resolves, and update isLoading to include redirecting; modify
references to mounted, setMounted, isLoading, router.push/router.replace,
isOnboardingComplete and getUserQuizzes accordingly so the UI doesn't render
while a redirect is in-flight.
In `@app/schedule/page.tsx`:
- Around line 13-32: The spinner can clear too early because setMounted(true)
runs on mount while the auth/redirect effect (which calls isOnboardingComplete()
and router.push()) is still executing; fix by delaying mounted until the
redirect logic has completed: remove the separate useEffect that sets mounted,
and instead setMounted(true) at the end of the auth effect that checks status
(inside the useEffect that reads status/session and calls isOnboardingComplete()
and router.push()), or alternatively add a redirecting boolean (e.g.,
redirectPending) that you set to true immediately before any router.push() and
false only after determining no navigation is required, and then compute
isLoading = status === "loading" || !mounted || redirectPending so the spinner
remains until redirects are handled.
---
Nitpick comments:
In `@lib/auth.ts`:
- Around line 6-16: The broad `@ts-expect-error` on the entire NextAuth(...) call
hides unrelated type issues; remove that broad suppression and instead declare a
typed options object using NextAuthOptions (or the appropriate NextAuth types)
and pass it into NextAuth, fixing any remaining provider or callback type
mismatches by importing and using the provider config types (e.g.
GoogleProviderConfig/GitHubProviderConfig) or explicit casts only where
necessary; then export the resulting handlers, signIn, signOut, and auth from
NextAuth(options). Ensure any remaining single-line unavoidable issues use
targeted `@ts-expect-error` comments rather than a file-level suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7252e6d-5720-46a9-873e-16e69ba90771
📒 Files selected for processing (9)
app/api/tutor/live/route.tsapp/courses/create/page.tsxapp/dashboard/page.tsxapp/quizzes/page.tsxapp/schedule/page.tsxcomponents/ThemeProvider.tsxlib/auth.tslib/security.test.tslib/setupTests.ts
| ...history.map((msg: { role: string; content: string }) => ({ | ||
| role: msg.role === "ai" ? "model" : "user", | ||
| parts: [{ text: msg.content }], | ||
| })), |
There was a problem hiding this comment.
Validate and normalize history before mapping roles
Line 64’s inline type is compile-time only; history is still untrusted JSON at runtime. Also, Line 65 currently maps any non-"ai" role to "user", which misclassifies "system"/invalid roles. Please sanitize first, then map only allowed roles.
Proposed fix
+import type { ChatMessage } from "@/lib/types";
+
+function normalizeHistory(input: unknown): ChatMessage[] {
+ if (!Array.isArray(input)) return [];
+ return input
+ .filter((m): m is ChatMessage =>
+ !!m &&
+ typeof m === "object" &&
+ typeof (m as { role?: unknown }).role === "string" &&
+ typeof (m as { content?: unknown }).content === "string" &&
+ ["user", "ai", "system"].includes((m as { role: string }).role)
+ );
+}
...
-const history = formData.get("history") ? JSON.parse(formData.get("history") as string) : [];
+const rawHistory = formData.get("history");
+const history = rawHistory ? normalizeHistory(JSON.parse(rawHistory as string)) : [];
...
-...history.map((msg: { role: string; content: string }) => ({
- role: msg.role === "ai" ? "model" : "user",
+...history.map((msg) => ({
+ role: msg.role === "ai" ? "model" : msg.role === "user" ? "user" : "user",
parts: [{ text: msg.content }],
})),🤖 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, history is untrusted at
runtime and the current inline mapping treats any non-"ai" role as "user";
validate that history is an array and each entry is an object with string role
and content, filter out invalid or empty items, normalize allowed roles only
(map "ai" -> "model", leave "user" and "system" as-is), and drop or log entries
with other roles; then use the validated list in the existing mapping code
(reference the history variable and the mapping block that produces { role: ...,
parts: [{ text: ... }] }) so only sanitized, normalized roles and string
contents are emitted.
| const savedTheme = localStorage.getItem("theme") as Theme | null | ||
| let initialTheme: Theme = "light" | ||
| if (savedTheme) { | ||
| setThemeState(savedTheme) | ||
| initialTheme = savedTheme | ||
| } else if (window.matchMedia("(prefers-color-scheme: dark)").matches) { | ||
| setThemeState("dark") | ||
| } else { | ||
| setThemeState("light") | ||
| initialTheme = "dark" | ||
| } | ||
|
|
||
| setThemeState(initialTheme) |
There was a problem hiding this comment.
Validate the persisted theme before treating it as Theme.
localStorage is untyped input. The cast on Line 22 allows any string to flow into theme, and Line 41 later adds that value as a class name. If "theme" is corrupted, the app can end up with neither light nor dark applied.
🛡️ Proposed fix
- const savedTheme = localStorage.getItem("theme") as Theme | null
+ const savedTheme = localStorage.getItem("theme")
let initialTheme: Theme = "light"
- if (savedTheme) {
+ if (savedTheme === "light" || savedTheme === "dark") {
initialTheme = savedTheme
} else if (window.matchMedia("(prefers-color-scheme: dark)").matches) {
initialTheme = "dark"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const savedTheme = localStorage.getItem("theme") as Theme | null | |
| let initialTheme: Theme = "light" | |
| if (savedTheme) { | |
| setThemeState(savedTheme) | |
| initialTheme = savedTheme | |
| } else if (window.matchMedia("(prefers-color-scheme: dark)").matches) { | |
| setThemeState("dark") | |
| } else { | |
| setThemeState("light") | |
| initialTheme = "dark" | |
| } | |
| setThemeState(initialTheme) | |
| const savedTheme = localStorage.getItem("theme") | |
| let initialTheme: Theme = "light" | |
| if (savedTheme === "light" || savedTheme === "dark") { | |
| initialTheme = savedTheme | |
| } else if (window.matchMedia("(prefers-color-scheme: dark)").matches) { | |
| initialTheme = "dark" | |
| } | |
| setThemeState(initialTheme) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ThemeProvider.tsx` around lines 22 - 30, Validate the value
returned from localStorage before treating it as a Theme: don’t cast savedTheme
to Theme directly—check that savedTheme equals one of the allowed values (e.g.,
"light" or "dark") before assigning it to initialTheme; if it’s invalid or
missing, fall back to window.matchMedia("(prefers-color-scheme: dark)") and then
to the default "light". Update the logic around savedTheme, initialTheme, and
setThemeState (or add a small isValidTheme helper) so only known Theme values
are ever passed into setThemeState and used as class names.
| // @ts-expect-error Types may be incomplete or global variables need mocking | ||
| const isValid = verifyData(testData, { not: 'a string' }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/security.test.ts | head -100Repository: APPLEPIE6969/StudyFlow
Length of output: 3287
🏁 Script executed:
cat -n lib/security.ts | head -50Repository: APPLEPIE6969/StudyFlow
Length of output: 1703
Update the @ts-expect-error comment to reflect the actual test intent.
The current comment references "global variables need mocking" which is unrelated to this test. The test intentionally passes an object instead of the expected string to verify error handling.
✏️ Suggested edit
- // `@ts-expect-error` Types may be incomplete or global variables need mocking
+ // `@ts-expect-error` Intentional type mismatch: verifyData expects signature as string
const isValid = verifyData(testData, { not: 'a string' });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-expect-error Types may be incomplete or global variables need mocking | |
| const isValid = verifyData(testData, { not: 'a string' }); | |
| // `@ts-expect-error` Intentional type mismatch: verifyData expects signature as string | |
| const isValid = verifyData(testData, { not: 'a string' }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security.test.ts` around lines 66 - 67, Update the misleading TypeScript
suppression comment above the failing call to verifyData so it reflects the
actual test intent: instead of mentioning "global variables need mocking",
replace the `@ts-expect-error` message to indicate that this test deliberately
passes a non-string (an object) as the second argument to verifyData to exercise
error handling with invalid input (e.g., "@ts-expect-error passing object
instead of string to verifyData"). Ensure the comment is adjacent to the
verifyData(testData, { not: 'a string' }) call so reviewers understand the
intentional type mismatch.
1 participant
🎯 What:
react-hooks/set-state-in-effecterrors by adopting the mounted state pattern (deriving data during render post-mount) inapp/courses/create/page.tsx,app/dashboard/page.tsx,app/quizzes/page.tsx,app/schedule/page.tsx, andcomponents/ThemeProvider.tsx.anyusage with a concrete object type inapp/api/tutor/live/route.ts.@ts-ignoreannotations to descriptive@ts-expect-errorinlib/auth.ts,lib/setupTests.ts, andlib/security.test.ts.💡 Why:
set-state-in-effectavoids cascading renders and hydration mismatches.anytypes improves static type safety.@ts-expect-errorinstead of@ts-ignoreclarifies developer intent when working around global variable/third-party type definitions while ensuring compilation checks remain robust.✅ Verification:
npm run lintsuccessfully, verifying state initialization andanyrules.npm run testsuccessfully with passing assertions for auth and security logic.✨ Result:
PR created automatically by Jules for task 16151366583073454146 started by @APPLEPIE6969
Summary by CodeRabbit
Refactor
Chores