Merged
Conversation
feat: Added re-evaluate button on the session card to ensure api fallback
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for system-craft-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughBackend expands session-claiming to a three-step fallback for evaluation; frontend makes evaluate POST fire-and-forget with immediate redirect, adds re-evaluate controls and collapsible question panel; result page implements in-place polling with timeout and cancellation; AI client adds per-request timeout/abort and adjusts retry behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend as Backend API (/api/interview/[id]/evaluate)
participant DB as Database
participant AI as AI Service
participant ResultPage as Result Page (polling)
Client->>Backend: POST /api/interview/[id]/evaluate (fire-and-forget)
alt Claim submitted
Backend->>DB: findOneAndUpdate status='submitted' → 'evaluating'
DB-->>Backend: updated
else Reclaim stale evaluating
Backend->>DB: findOneAndUpdate status='evaluating' & updatedAt < now-2min → 'evaluating'
DB-->>Backend: updated
else Re-evaluate evaluated
Backend->>DB: findOneAndUpdate status='evaluated' → 'evaluating'
DB-->>Backend: updated
end
Backend->>AI: Generate evaluation (with AbortController + timeout)
AI-->>Backend: evaluation result / error
Backend->>DB: update session → status='evaluated' + results
DB-->>Backend: confirmed
Backend-->>Client: 201 Created / response
Client->>ResultPage: Redirect to /interview/[id]/result
loop poll interval (up to MAX_POLLS)
ResultPage->>Backend: GET /api/interview/[id]
Backend->>DB: fetch session
DB-->>Backend: session
Backend-->>ResultPage: session.status
alt status == 'evaluating' or stale
ResultPage->>ResultPage: show "Evaluating..." and schedule next poll
else status == 'evaluated'
ResultPage->>ResultPage: show results and stop polling
else status == 'in_progress'
ResultPage->>Client: redirect/back
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/ai/geminiClient.ts (1)
75-92:⚠️ Potential issue | 🟠 MajorPreserve timeout/abort errors instead of masking them as generic failures.
At Line 89–Line 92, timeout aborts eventually become a generic error, so callers can’t apply timeout-specific handling/recovery.
💡 Suggested fix
} catch (error: unknown) { const errMsg = error instanceof Error ? error.message : String(error); const status = (error as { status?: number })?.status; + const errName = (error as { name?: string })?.name; + const isAbort = errName === 'AbortError' || /aborted|timeout/i.test(errMsg); // Don't retry non-transient errors if (status === 401 || errMsg.includes('Invalid API key')) { console.error('OpenRouter auth error:', errMsg); throw new Error('AI API key is invalid. Please update OPENROUTER_API_KEY.'); } if (status === 402) { console.error('OpenRouter billing error:', errMsg); throw new Error('AI billing issue. Please check your OpenRouter credits.'); } + if (isAbort && attempt === retries) { + throw new Error(`AI request timed out after ${timeoutMs}ms.`); + } if (attempt === retries) { console.error(`OpenRouter generation failed after ${retries + 1} attempts:`, error); throw new Error('Failed to generate AI content. Please try again.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ai/geminiClient.ts` around lines 75 - 92, The catch block in the retry loop masks timeout/abort errors as generic failures; detect abort/timeout errors (e.g., error.name === 'AbortError', (error as any).code === 'ETIMEDOUT', or errMsg.includes('timeout')/ 'timed out') inside the catch and immediately rethrow the original error instead of wrapping it, so callers can handle timeouts; keep the existing non-transient checks (401/402) and the final retry-failure behavior for other errors. Use the local symbols in this scope (attempt, retries, errMsg, status) and return/throw the original error object when it’s an abort/timeout.app/api/interview/[id]/evaluate/route.ts (1)
70-72:⚠️ Potential issue | 🟡 MinorError message is outdated after adding new claim paths.
The error message states sessions must be
"submitted"to evaluate, but the code now also accepts stale"evaluating"sessions (>2 min) and"evaluated"sessions for re-evaluation. The remaining failure cases are"in_progress"or a non-stale"evaluating"session.📝 Proposed fix to clarify the error message
return NextResponse.json( - { error: `Cannot evaluate session with status "${exists.status}". Must be "submitted".` }, + { error: `Cannot evaluate session with status "${exists.status}". Session must be "submitted", "evaluated", or stuck in "evaluating" for over 2 minutes.` }, { status: 409 } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/interview/`[id]/evaluate/route.ts around lines 70 - 72, Update the error message returned by NextResponse.json when rejecting evaluation (the block that currently references exists.status) to reflect the new allowed states: accept "submitted", "evaluated" (for re-eval), and stale "evaluating" (>2 min); state that the request is invalid only for "in_progress" or a non-stale "evaluating" session. Locate the check around exists.status in route.ts (the NextResponse.json call) and change the message to list the allowed statuses and the specific failure cases ("in_progress" or non-stale "evaluating") so the error accurately describes why evaluation was denied.
🧹 Nitpick comments (3)
app/api/interview/[id]/evaluate/route.ts (1)
48-52: RedundantupdatedAtassignment.With
timestamps: truein theInterviewSessionSchema(seesrc/lib/db/models/InterviewSession.ts:275), Mongoose automatically updatesupdatedAton every write operation. The explicitupdatedAt: new Date()in the$setis redundant and can be removed.♻️ Proposed simplification
session = await InterviewSession.findOneAndUpdate( { _id: id, userId: user._id, status: 'evaluating', updatedAt: { $lt: twoMinutesAgo } }, - { $set: { status: 'evaluating', updatedAt: new Date() } }, + { $set: { status: 'evaluating' } }, { new: false } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/interview/`[id]/evaluate/route.ts around lines 48 - 52, The InterviewSession.findOneAndUpdate call is redundantly setting updatedAt: new Date() despite InterviewSessionSchema using timestamps: true; remove the explicit updatedAt assignment from the $set object so only the needed fields (e.g., status) are updated and rely on Mongoose to update updatedAt automatically; locate the call to InterviewSession.findOneAndUpdate in route.ts and delete the updatedAt: new Date() entry in the $set payload.app/interview/page.tsx (1)
376-396: Consider hiding Re-evaluate button for actively evaluating sessions.Showing "Re-evaluate" for sessions with
status === 'evaluating'may confuse users. If clicked while evaluation is actively running (not stale), the backend will return a 409 error because the session doesn't meet any claim criteria.Consider only showing the button for
'submitted'and'evaluated'statuses, or displaying "Evaluating..." text instead of a clickable button whenstatus === 'evaluating'.♻️ Proposed UX improvement
- {['submitted', 'evaluating', 'evaluated'].includes(session.status) && ( + {['submitted', 'evaluated'].includes(session.status) && ( <button onClick={(e) => handleReEvaluate(e, session.id)} disabled={reEvaluatingId === session.id}Alternatively, render a non-interactive "Evaluating..." indicator when
session.status === 'evaluating'andreEvaluatingId !== session.id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/interview/page.tsx` around lines 376 - 396, The Re-evaluate button currently appears for sessions with status 'evaluating', which can cause 409s; update the rendering logic in the component that uses session.status, reEvaluatingId and handleReEvaluate so the clickable button is only shown for statuses 'submitted' and 'evaluated'; when session.status === 'evaluating' render a non-interactive indicator (e.g., plain "Evaluating..." text or a disabled button state) and ensure reEvaluatingId still controls the animated "Evaluating" spinner state to avoid showing a clickable control while evaluation is active.app/interview/[id]/result/page.tsx (1)
68-74: Consider adding a maximum polling duration or retry limit.The polling continues indefinitely while the status is
'submitted'or'evaluating'. If the backend evaluation gets permanently stuck (despite the 2-minute stale recovery), the user would see the "Evaluating" spinner forever.Consider adding a timeout (e.g., 5 minutes) after which the UI shows an error with a retry option.
♻️ Proposed enhancement with polling timeout
useEffect(() => { let pollTimer: NodeJS.Timeout | null = null; let cancelled = false; + const pollStartTime = Date.now(); + const MAX_POLL_DURATION_MS = 5 * 60 * 1000; // 5 minutes const fetchResult = async () => { if (!user?.uid || !id) return; try { if (!isEvaluating) setIsLoading(true); const response = await authFetch(`/api/interview/${id}`); if (!response.ok) { throw new Error('Failed to load results'); } const data = await response.json(); if (cancelled) return; if (data.session.status === 'evaluated') { // Evaluation complete — show results setIsEvaluating(false); setSession(data.session); setIsLoading(false); return; // stop polling } if (['submitted', 'evaluating'].includes(data.session.status)) { + // Check if we've exceeded max polling duration + if (Date.now() - pollStartTime > MAX_POLL_DURATION_MS) { + setIsEvaluating(false); + setError('Evaluation is taking longer than expected. Please try again later.'); + setIsLoading(false); + return; + } // Still evaluating — show spinner and poll again setIsEvaluating(true); setIsLoading(false); pollTimer = setTimeout(fetchResult, 3000); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/interview/`[id]/result/page.tsx around lines 68 - 74, The current polling loop inside the fetchResult branch that checks data.session.status ('submitted'|'evaluating') can run forever; modify the logic around setIsEvaluating/setIsLoading/pollTimer to enforce a max polling duration or attempt limit by recording a start timestamp or an attempt counter (e.g., pollStartTime or pollAttempts) when the first poll begins, incrementing attempts or comparing elapsed time on each retry, and if the limit is reached stop scheduling pollTimer, set an error state (e.g., setIsEvaluating(false); setIsLoading(false); setError(...)) and surface a retry action that calls fetchResult again resetting the counter/timestamp; update fetchResult and any state initializers so retry resets the timer/counter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/api/interview/`[id]/evaluate/route.ts:
- Around line 70-72: Update the error message returned by NextResponse.json when
rejecting evaluation (the block that currently references exists.status) to
reflect the new allowed states: accept "submitted", "evaluated" (for re-eval),
and stale "evaluating" (>2 min); state that the request is invalid only for
"in_progress" or a non-stale "evaluating" session. Locate the check around
exists.status in route.ts (the NextResponse.json call) and change the message to
list the allowed statuses and the specific failure cases ("in_progress" or
non-stale "evaluating") so the error accurately describes why evaluation was
denied.
In `@src/lib/ai/geminiClient.ts`:
- Around line 75-92: The catch block in the retry loop masks timeout/abort
errors as generic failures; detect abort/timeout errors (e.g., error.name ===
'AbortError', (error as any).code === 'ETIMEDOUT', or
errMsg.includes('timeout')/ 'timed out') inside the catch and immediately
rethrow the original error instead of wrapping it, so callers can handle
timeouts; keep the existing non-transient checks (401/402) and the final
retry-failure behavior for other errors. Use the local symbols in this scope
(attempt, retries, errMsg, status) and return/throw the original error object
when it’s an abort/timeout.
---
Nitpick comments:
In `@app/api/interview/`[id]/evaluate/route.ts:
- Around line 48-52: The InterviewSession.findOneAndUpdate call is redundantly
setting updatedAt: new Date() despite InterviewSessionSchema using timestamps:
true; remove the explicit updatedAt assignment from the $set object so only the
needed fields (e.g., status) are updated and rely on Mongoose to update
updatedAt automatically; locate the call to InterviewSession.findOneAndUpdate in
route.ts and delete the updatedAt: new Date() entry in the $set payload.
In `@app/interview/`[id]/result/page.tsx:
- Around line 68-74: The current polling loop inside the fetchResult branch that
checks data.session.status ('submitted'|'evaluating') can run forever; modify
the logic around setIsEvaluating/setIsLoading/pollTimer to enforce a max polling
duration or attempt limit by recording a start timestamp or an attempt counter
(e.g., pollStartTime or pollAttempts) when the first poll begins, incrementing
attempts or comparing elapsed time on each retry, and if the limit is reached
stop scheduling pollTimer, set an error state (e.g., setIsEvaluating(false);
setIsLoading(false); setError(...)) and surface a retry action that calls
fetchResult again resetting the counter/timestamp; update fetchResult and any
state initializers so retry resets the timer/counter.
In `@app/interview/page.tsx`:
- Around line 376-396: The Re-evaluate button currently appears for sessions
with status 'evaluating', which can cause 409s; update the rendering logic in
the component that uses session.status, reEvaluatingId and handleReEvaluate so
the clickable button is only shown for statuses 'submitted' and 'evaluated';
when session.status === 'evaluating' render a non-interactive indicator (e.g.,
plain "Evaluating..." text or a disabled button state) and ensure reEvaluatingId
still controls the animated "Evaluating" spinner state to avoid showing a
clickable control while evaluation is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c74dd356-fdf5-42da-9a20-2b2f3ea31c14
📒 Files selected for processing (5)
app/api/interview/[id]/evaluate/route.tsapp/interview/[id]/page.tsxapp/interview/[id]/result/page.tsxapp/interview/page.tsxsrc/lib/ai/geminiClient.ts
- This makes sure the canvas gets approx 300px of more space
fix(ui): The interview question bar now collapsible
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/interview/QuestionPanel.tsx (1)
12-14: Keep the new toggle API internally consistent.
onToggleis optional inQuestionPanelProps, but both branches always render a clickable expand/collapse button. Guard the button ononToggle, or make the prop required, so the component cannot expose a no-op control.Also applies to: 37-43, 76-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/interview/QuestionPanel.tsx` around lines 12 - 14, QuestionPanelProps currently marks onToggle as optional while the QuestionPanel component always renders a clickable expand/collapse control; make the API consistent by either (A) changing QuestionPanelProps to require onToggle (remove the ? from onToggle) so the button always has a handler, or (B) guard the button render inside the QuestionPanel component (and related render branches around lines ~37-43 and ~76-82) so the expand/collapse control is only rendered or interactive when onToggle is defined (e.g., render a non-interactive/hidden element or aria-disabled state when onToggle is absent); update any call sites of QuestionPanel to match the chosen approach.
🤖 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/interview/`[id]/result/page.tsx:
- Around line 91-96: The catch block in the polling logic currently logs the
error and calls setError/setIsLoading but does not clear the evaluation gate,
leaving the UI stuck in the isEvaluating branch; update that catch handler to
also call the state setter that exits evaluation (e.g., setIsEvaluating(false))
so the error screen can render, and apply the same fix to the similar catch at
the other polling location referenced (the block around 110-135); keep the
existing cancelled check and error message logic.
---
Nitpick comments:
In `@components/interview/QuestionPanel.tsx`:
- Around line 12-14: QuestionPanelProps currently marks onToggle as optional
while the QuestionPanel component always renders a clickable expand/collapse
control; make the API consistent by either (A) changing QuestionPanelProps to
require onToggle (remove the ? from onToggle) so the button always has a
handler, or (B) guard the button render inside the QuestionPanel component (and
related render branches around lines ~37-43 and ~76-82) so the expand/collapse
control is only rendered or interactive when onToggle is defined (e.g., render a
non-interactive/hidden element or aria-disabled state when onToggle is absent);
update any call sites of QuestionPanel to match the chosen approach.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ae953d0-7bcf-452b-867b-6392eaedc209
📒 Files selected for processing (6)
app/api/interview/[id]/evaluate/route.tsapp/interview/[id]/page.tsxapp/interview/[id]/result/page.tsxapp/interview/page.tsxcomponents/interview/QuestionPanel.tsxsrc/lib/ai/geminiClient.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/ai/geminiClient.ts
- app/api/interview/[id]/evaluate/route.ts
- app/interview/page.tsx
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/interview/`[id]/result/page.tsx:
- Around line 45-48: The polling closure captures isEvaluating causing
setIsLoading(true) to run every poll; change to use a mutable ref (e.g.,
isEvaluatingRef) to reflect current evaluating state inside the polling loop
(update isEvaluatingRef.current when you call setIsEvaluating) and read
isEvaluatingRef.current where you currently check isEvaluating before calling
setIsLoading, so the recursive setTimeout callback sees fresh state; ensure
pollTimer is cleared in the cleanup (use clearTimeout(pollTimer)) and
increment/limit pollAttempts against MAX_POLLS as before.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9def0f4b-8a15-4858-9d50-2a37a0da92cb
📒 Files selected for processing (2)
app/interview/[id]/result/page.tsxnext.config.ts
✅ Files skipped from review due to trivial changes (1)
- next.config.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
postcss.config.mjs (1)
1-5: Align with lint rule by exporting a named config object.No functional issue here, but this currently triggers the lint warning shown in checks. Consider assigning to a variable before
export defaultto keep CI clean.Proposed lint-friendly refactor
-export default { +const config = { plugins: { "@tailwindcss/postcss": {}, }, }; + +export default config;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postcss.config.mjs` around lines 1 - 5, Assign the PostCSS configuration object to a named constant (e.g., config) instead of inlining it, include the plugins entry with "@tailwindcss/postcss", then export that named constant (export const config = ...) and also export it as the default (export default config) so the lint rule is satisfied while preserving the same configuration structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@postcss.config.mjs`:
- Around line 1-5: Assign the PostCSS configuration object to a named constant
(e.g., config) instead of inlining it, include the plugins entry with
"@tailwindcss/postcss", then export that named constant (export const config =
...) and also export it as the default (export default config) so the lint rule
is satisfied while preserving the same configuration structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 169eb3e6-d686-4271-9b19-96a8562d6ff1
📒 Files selected for processing (2)
app/interview/[id]/result/page.tsxpostcss.config.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/interview/[id]/result/page.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes