Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Text-to-Speech evaluation UI and multiple Next.js app-router API proxy handlers for TTS dataset/run/result endpoints, plus a small auth-guard tweak to an STT dataset route; proxies forward Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser Client
participant NextAPI as Next.js API Route
participant Backend as Backend API
rect rgba(100,150,200,0.5)
Note over User,Backend: Create Dataset / Run
User->>Browser: Submit form (includes X-API-KEY)
Browser->>NextAPI: POST /api/evaluations/tts/... (forward X-API-KEY & JSON)
NextAPI->>Backend: POST /api/v1/evaluations/tts/... (forward headers & body)
Backend-->>NextAPI: 200/201 JSON
NextAPI-->>Browser: Return backend JSON/status
end
rect rgba(150,100,200,0.5)
Note over User,Backend: Polling & Feedback
Browser->>NextAPI: GET /api/evaluations/tts/runs?...
NextAPI->>Backend: GET /api/v1/evaluations/tts/runs?...
Backend-->>NextAPI: runs + results
NextAPI-->>Browser: return JSON/status
Browser->>NextAPI: PATCH /api/evaluations/tts/results/{id} (is_correct/comments)
NextAPI->>Backend: PATCH /api/v1/evaluations/tts/results/{id}
Backend-->>NextAPI: updated result
NextAPI-->>Browser: updated JSON/status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 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.
Actionable comments posted: 7
🤖 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/evaluations/tts/datasets/`[dataset_id]/route.ts:
- Around line 9-16: The handler currently reads const apiKey =
request.headers.get('X-API-KEY') and forwards an empty string to fetch, which
proxies unauthenticated callers; change this to reject missing keys up front by
checking if apiKey is falsy and returning a 401/unauthorized response
immediately (do not call fetch), and only call
fetch(`${backendUrl}/api/v1/evaluations/tts/datasets/${dataset_id}`) with the
'X-API-KEY' header when apiKey is present; also ensure you do not send an empty
header value when apiKey is absent.
In `@app/api/evaluations/tts/datasets/route.ts`:
- Around line 3-15: The GET handler currently proxies even when X-API-KEY is
missing; update the GET function to read the API key via
request.headers.get('X-API-KEY') and fail fast by returning a 401 response (use
NextResponse.json or equivalent) if the header is falsy, matching the POST
behavior, before calling fetch to
`${backendUrl}/api/v1/evaluations/tts/datasets`; ensure the same header value is
forwarded when present.
In `@app/api/evaluations/tts/results/`[result_id]/route.ts:
- Around line 7-19: This handler currently reads apiKey but forwards an empty
header; add an upfront auth check (mirror other GET routes) that returns a 401
via NextResponse.json when apiKey is falsy, and only proceed to call
fetch(`${backendUrl}/api/v1/evaluations/tts/results/${result_id}`) when apiKey
exists; use the apiKey variable name for the check and ensure you do not call
fetch or forward an empty 'X-API-KEY' header when missing.
In `@app/api/evaluations/tts/runs/`[run_id]/route.ts:
- Around line 9-23: The route currently extracts apiKey (const apiKey =
request.headers.get('X-API-KEY')) but still forwards empty headers to the
backend; add an early guard that checks apiKey and returns a 401 response
immediately when missing (before constructing backendUrlWithParams or calling
fetch) so unauthenticated reads are rejected locally; update the handler
surrounding apiKey extraction (in route.ts) to perform this check and
short-circuit with a 401/Unauthorized response if apiKey is falsy.
In `@app/api/evaluations/tts/runs/route.ts`:
- Around line 3-15: The GET handler currently proxies requests even when the
X-API-KEY header is missing; add the same API key validation used by the POST
flow: in the exported async function GET(request: Request) check
request.headers.get('X-API-KEY') (the apiKey variable) and if falsy return a 401
response (e.g., NextResponse.json with a descriptive message and status 401)
before performing the fetch to `${backendUrl}/api/v1/evaluations/tts/runs`; keep
the existing fetch headers logic so valid requests continue to forward the API
key.
In `@app/text-to-speech/page.tsx`:
- Around line 894-900: The updateFeedback function is including is_correct even
when null (causing PATCH to send is_correct: null) — change it to only set
payload.is_correct when the value is a boolean (e.g., typeof isCorrect ===
"boolean") and remove any non-null assertions like result.is_correct!; apply the
same guard to the other feedback update call that currently forces
result.is_correct (the similar block mentioned in the review) so comment-only
updates omit is_correct entirely.
- Around line 305-319: The samples mapping currently drops per-sample language
(validSamples -> samples used in the POST to /api/evaluations/tts/datasets), so
the per-sample language selector is ignored; update the mapping that creates
samples so each sample includes its language (e.g., add a language or
language_id field using getLanguageId(sample.language) or the appropriate
sample.language value) before JSON.stringify in the body sent by the fetch call,
ensuring the API receives per-sample language data alongside text while keeping
existing top-level name/description/language_id fields (refer to validSamples,
samples, getLanguageId, and the fetch body).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5f5bd98-f233-4bd2-a595-1d9dc4de6525
📒 Files selected for processing (7)
app/api/evaluations/tts/datasets/[dataset_id]/route.tsapp/api/evaluations/tts/datasets/route.tsapp/api/evaluations/tts/results/[result_id]/route.tsapp/api/evaluations/tts/runs/[run_id]/route.tsapp/api/evaluations/tts/runs/route.tsapp/components/Sidebar.tsxapp/text-to-speech/page.tsx
Comment on lines
+3
to
+15
| export async function GET(request: Request) { | ||
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000'; | ||
| const apiKey = request.headers.get('X-API-KEY'); | ||
|
|
||
| try { | ||
| const response = await fetch(`${backendUrl}/api/v1/evaluations/tts/datasets`, { | ||
| headers: { | ||
| 'X-API-KEY': apiKey || '', | ||
| }, | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| return NextResponse.json(data, { status: response.status }); |
There was a problem hiding this comment.
Enforce X-API-KEY on GET too.
POST already returns 401 when the header is missing, but GET still proxies the request with an empty API key. Please fail fast here as well so dataset reads follow the same auth boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/evaluations/tts/datasets/route.ts` around lines 3 - 15, The GET
handler currently proxies even when X-API-KEY is missing; update the GET
function to read the API key via request.headers.get('X-API-KEY') and fail fast
by returning a 401 response (use NextResponse.json or equivalent) if the header
is falsy, matching the POST behavior, before calling fetch to
`${backendUrl}/api/v1/evaluations/tts/datasets`; ensure the same header value is
forwarded when present.
Comment on lines
+7
to
+19
| const { result_id } = await params; | ||
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000'; | ||
| const apiKey = request.headers.get('X-API-KEY'); | ||
|
|
||
| try { | ||
| const response = await fetch(`${backendUrl}/api/v1/evaluations/tts/results/${result_id}`, { | ||
| headers: { | ||
| 'X-API-KEY': apiKey || '', | ||
| }, | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| return NextResponse.json(data, { status: response.status }); |
There was a problem hiding this comment.
Mirror the existing GET auth check here.
The rest of the app’s API routes already reject missing X-API-KEY headers up front, but this handler still forwards an empty one. That weakens the API boundary for result reads and should be fixed before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/evaluations/tts/results/`[result_id]/route.ts around lines 7 - 19,
This handler currently reads apiKey but forwards an empty header; add an upfront
auth check (mirror other GET routes) that returns a 401 via NextResponse.json
when apiKey is falsy, and only proceed to call
fetch(`${backendUrl}/api/v1/evaluations/tts/results/${result_id}`) when apiKey
exists; use the apiKey variable name for the check and ensure you do not call
fetch or forward an empty 'X-API-KEY' header when missing.
Comment on lines
+9
to
+23
| const apiKey = request.headers.get('X-API-KEY'); | ||
|
|
||
| const { searchParams } = new URL(request.url); | ||
| const queryString = searchParams.toString(); | ||
|
|
||
| try { | ||
| const backendUrlWithParams = queryString | ||
| ? `${backendUrl}/api/v1/evaluations/tts/runs/${run_id}?${queryString}` | ||
| : `${backendUrl}/api/v1/evaluations/tts/runs/${run_id}`; | ||
|
|
||
| const response = await fetch(backendUrlWithParams, { | ||
| headers: { | ||
| 'X-API-KEY': apiKey || '', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Guard unauthenticated run reads locally.
This route should return 401 before the fetch when X-API-KEY is missing. Right now it forwards an empty header and relies on the backend to reject it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/evaluations/tts/runs/`[run_id]/route.ts around lines 9 - 23, The
route currently extracts apiKey (const apiKey =
request.headers.get('X-API-KEY')) but still forwards empty headers to the
backend; add an early guard that checks apiKey and returns a 401 response
immediately when missing (before constructing backendUrlWithParams or calling
fetch) so unauthenticated reads are rejected locally; update the handler
surrounding apiKey extraction (in route.ts) to perform this check and
short-circuit with a 401/Unauthorized response if apiKey is falsy.
Comment on lines
+3
to
+15
| export async function GET(request: Request) { | ||
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000'; | ||
| const apiKey = request.headers.get('X-API-KEY'); | ||
|
|
||
| try { | ||
| const response = await fetch(`${backendUrl}/api/v1/evaluations/tts/runs`, { | ||
| headers: { | ||
| 'X-API-KEY': apiKey || '', | ||
| }, | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| return NextResponse.json(data, { status: response.status }); |
There was a problem hiding this comment.
Apply the same 401 check on run listing.
POST rejects missing API keys, but this GET path still proxies anonymous traffic to the backend. Keeping both methods consistent avoids surprising auth behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/evaluations/tts/runs/route.ts` around lines 3 - 15, The GET handler
currently proxies requests even when the X-API-KEY header is missing; add the
same API key validation used by the POST flow: in the exported async function
GET(request: Request) check request.headers.get('X-API-KEY') (the apiKey
variable) and if falsy return a 401 response (e.g., NextResponse.json with a
descriptive message and status 401) before performing the fetch to
`${backendUrl}/api/v1/evaluations/tts/runs`; keep the existing fetch headers
logic so valid requests continue to forward the API key.
Comment on lines
+305
to
+319
| const samples = validSamples.map(sample => ({ | ||
| text: sample.text.trim(), | ||
| })); | ||
|
|
||
| const response = await fetch('/api/evaluations/tts/datasets', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'X-API-KEY': apiKeys[0].key, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| name: datasetName.trim(), | ||
| description: datasetDescription.trim() || undefined, | ||
| language_id: getLanguageId(datasetLanguage), | ||
| samples, |
There was a problem hiding this comment.
Per-sample language is collected but never sent.
The form lets users set a language on each sample, but this payload strips every sample down to { text }. That makes the language selector misleading and breaks mixed-language datasets.
Suggested fix
const samples = validSamples.map(sample => ({
text: sample.text.trim(),
+ language_id: getLanguageId(sample.language),
}));📝 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.
Suggested change
| const samples = validSamples.map(sample => ({ | |
| text: sample.text.trim(), | |
| })); | |
| const response = await fetch('/api/evaluations/tts/datasets', { | |
| method: 'POST', | |
| headers: { | |
| 'X-API-KEY': apiKeys[0].key, | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| name: datasetName.trim(), | |
| description: datasetDescription.trim() || undefined, | |
| language_id: getLanguageId(datasetLanguage), | |
| samples, | |
| const samples = validSamples.map(sample => ({ | |
| text: sample.text.trim(), | |
| language_id: getLanguageId(sample.language), | |
| })); | |
| const response = await fetch('/api/evaluations/tts/datasets', { | |
| method: 'POST', | |
| headers: { | |
| 'X-API-KEY': apiKeys[0].key, | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| name: datasetName.trim(), | |
| description: datasetDescription.trim() || undefined, | |
| language_id: getLanguageId(datasetLanguage), | |
| samples, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/text-to-speech/page.tsx` around lines 305 - 319, The samples mapping
currently drops per-sample language (validSamples -> samples used in the POST to
/api/evaluations/tts/datasets), so the per-sample language selector is ignored;
update the mapping that creates samples so each sample includes its language
(e.g., add a language or language_id field using getLanguageId(sample.language)
or the appropriate sample.language value) before JSON.stringify in the body sent
by the fetch call, ensuring the API receives per-sample language data alongside
text while keeping existing top-level name/description/language_id fields (refer
to validSamples, samples, getLanguageId, and the fetch body).
app/text-to-speech/page.tsx
Outdated
Comment on lines
+894
to
+900
| const updateFeedback = async (resultId: number, isCorrect: boolean, comment?: string) => { | ||
| if (apiKeys.length === 0) return; | ||
|
|
||
| try { | ||
| const payload: { is_correct?: boolean; comment?: string } = {}; | ||
| if (isCorrect !== undefined) payload.is_correct = isCorrect; | ||
| if (comment !== undefined) payload.comment = comment; |
There was a problem hiding this comment.
Don’t force is_correct on comment-only updates.
Line 1226 uses result.is_correct!, but that field is nullable. If the reviewer types a comment before choosing Yes/No, this PATCH can send is_correct: null and fail or overwrite the existing value unexpectedly. Only include is_correct when it is actually boolean.
Suggested fix
- const updateFeedback = async (resultId: number, isCorrect: boolean, comment?: string) => {
+ const updateFeedback = async (resultId: number, isCorrect?: boolean | null, comment?: string) => {
if (apiKeys.length === 0) return;
try {
const payload: { is_correct?: boolean; comment?: string } = {};
- if (isCorrect !== undefined) payload.is_correct = isCorrect;
+ if (typeof isCorrect === 'boolean') payload.is_correct = isCorrect;
if (comment !== undefined) payload.comment = comment; onBlur={(e) => {
if (result.status === 'SUCCESS') {
- updateFeedback(result.id, result.is_correct!, e.target.value);
+ updateFeedback(result.id, result.is_correct ?? undefined, e.target.value);
}
}}Also applies to: 1217-1227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/text-to-speech/page.tsx` around lines 894 - 900, The updateFeedback
function is including is_correct even when null (causing PATCH to send
is_correct: null) — change it to only set payload.is_correct when the value is a
boolean (e.g., typeof isCorrect === "boolean") and remove any non-null
assertions like result.is_correct!; apply the same guard to the other feedback
update call that currently forces result.is_correct (the similar block mentioned
in the review) so comment-only updates omit is_correct entirely.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/text-to-speech/page.tsx (1)
958-964:⚠️ Potential issue | 🟠 MajorStill sending
is_correct: nullon comment-only updates.Line 1284 can pass
nullvia the non-null assertion, and the guards on Line 963 and Line 978 treat anything exceptundefinedas present. The PATCH proxy forwards that body unchanged, so a comment blur can clear or reject the verdict instead of updating only the comment.Suggested fix
- const updateFeedback = async (resultId: number, isCorrect: boolean, comment?: string) => { + const updateFeedback = async (resultId: number, isCorrect?: boolean | null, comment?: string) => { if (apiKeys.length === 0) return; try { const payload: { is_correct?: boolean; comment?: string } = {}; - if (isCorrect !== undefined) payload.is_correct = isCorrect; + if (typeof isCorrect === 'boolean') payload.is_correct = isCorrect; if (comment !== undefined) payload.comment = comment; @@ if (!response.ok) throw new Error('Failed to update feedback'); setResults(prev => prev.map(r => - r.id === resultId ? { ...r, ...(isCorrect !== undefined && { is_correct: isCorrect }), ...(comment !== undefined && { comment }) } : r + r.id === resultId ? { ...r, ...(typeof isCorrect === 'boolean' && { is_correct: isCorrect }), ...(comment !== undefined && { comment }) } : r )); @@ onBlur={(e) => { if (result.status === 'SUCCESS') { - updateFeedback(result.id, result.is_correct!, e.target.value); + updateFeedback(result.id, result.is_correct ?? undefined, e.target.value); } }}Also applies to: 977-979, 1282-1285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/text-to-speech/page.tsx` around lines 958 - 964, The updateFeedback function is incorrectly treating null as a valid value and sending is_correct: null; change the guard so payload.is_correct is only set when isCorrect is a boolean (e.g., use typeof isCorrect === 'boolean' or isCorrect !== null && isCorrect !== undefined) and keep the comment assignment as only when comment !== undefined; apply the same strict boolean check to the other similar update sites referenced (the other payload constructions around the comment update and the other updateFeedback call sites) so comment-only updates never include is_correct: null.
🤖 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/text-to-speech/page.tsx`:
- Around line 144-151: Several icon-only buttons (e.g., the play button with
onPlayToggle and the buttons that toggle the sidebar, remove a sample, go back,
and refresh runs) lack accessible names; add an accessible name to each such
button by setting an appropriate aria-label (or aria-labelledby) or by including
a visually-hidden text node (sr-only) inside the button. Locate the buttons
using the existing handler identifiers (onPlayToggle, the sidebar toggle
handler, the sample removal handler, the back navigation handler, and the
refresh runs handler) and add concise, descriptive labels like "Play audio",
"Toggle sidebar", "Remove sample", "Go back", and "Refresh runs" to their button
elements.
- Around line 348-360: The auto-refresh check is case-sensitive and misses
statuses like "RUNNING" or "PENDING"; update the condition inside the useEffect
that checks runs to normalize statuses (e.g., use run.status?.toLowerCase())
before comparing to 'running', 'processing', or 'pending', ensuring you handle
undefined/null safely; keep the surrounding logic (activeTab === 'evaluations',
hasRunningEvals variable, and calling loadRuns/clearInterval) unchanged while
replacing the run.status checks with the normalized comparisons.
- Around line 503-537: The loadResults handler can be overwritten by slower
responses; fix it by tracking and ignoring stale responses: add a persistent
request identifier or AbortController that you update before each call in
loadResults (e.g., increment a latestLoadId ref or set currentAbortController),
include that id/abort in the async fetch, and when the response arrives check
the id still matches (or ensure previous requests are aborted) before calling
setResults or setSelectedRunId; update loadResults to use apiKeys and the fetch
URL as before but gate the final setResults/setSelectedRunId with the id/abort
check so only the most recent response takes effect.
---
Duplicate comments:
In `@app/text-to-speech/page.tsx`:
- Around line 958-964: The updateFeedback function is incorrectly treating null
as a valid value and sending is_correct: null; change the guard so
payload.is_correct is only set when isCorrect is a boolean (e.g., use typeof
isCorrect === 'boolean' or isCorrect !== null && isCorrect !== undefined) and
keep the comment assignment as only when comment !== undefined; apply the same
strict boolean check to the other similar update sites referenced (the other
payload constructions around the comment update and the other updateFeedback
call sites) so comment-only updates never include is_correct: null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 564a7e82-180f-4c7a-abec-8d73f38de4ca
📒 Files selected for processing (1)
app/text-to-speech/page.tsx
Comment on lines
+144
to
+151
| <button | ||
| onClick={onPlayToggle} | ||
| className="w-6 h-6 flex items-center justify-center rounded-full flex-shrink-0 border-2" | ||
| style={{ | ||
| borderColor: colors.accent.primary, | ||
| backgroundColor: 'transparent', | ||
| color: colors.accent.primary, | ||
| }} |
There was a problem hiding this comment.
Add accessible names to the icon-only buttons.
Line 145, Line 562, Line 815, Line 1149, and Line 1166 render buttons whose only content is an SVG. Without an accessible name, screen-reader users cannot tell which control plays audio, toggles the sidebar, removes a sample, goes back, or refreshes runs.
Suggested fix
- <button
+ <button
+ aria-label={isPlaying ? 'Pause audio sample' : 'Play audio sample'}
onClick={onPlayToggle}
className="w-6 h-6 flex items-center justify-center rounded-full flex-shrink-0 border-2"
@@
- <button
+ <button
+ aria-label={sidebarCollapsed ? 'Expand sidebar' : 'Collapse sidebar'}
onClick={() => setSidebarCollapsed(!sidebarCollapsed)}
className="p-1.5 rounded-md"
@@
- <button
+ <button
+ aria-label={`Remove sample ${idx + 1}`}
onClick={() => removeTextSample(sample.id)}
className="p-1 rounded flex-shrink-0"
@@
- <button
+ <button
+ aria-label="Back to evaluation runs"
onClick={() => setSelectedRunId(null)}
className="p-1.5 rounded hover:bg-opacity-10"
@@
- <button
+ <button
+ aria-label="Refresh evaluation runs"
onClick={loadRuns}
disabled={isLoadingRuns}
className="p-1.5 rounded"Also applies to: 561-565, 814-817, 1148-1151, 1165-1169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/text-to-speech/page.tsx` around lines 144 - 151, Several icon-only
buttons (e.g., the play button with onPlayToggle and the buttons that toggle the
sidebar, remove a sample, go back, and refresh runs) lack accessible names; add
an accessible name to each such button by setting an appropriate aria-label (or
aria-labelledby) or by including a visually-hidden text node (sr-only) inside
the button. Locate the buttons using the existing handler identifiers
(onPlayToggle, the sidebar toggle handler, the sample removal handler, the back
navigation handler, and the refresh runs handler) and add concise, descriptive
labels like "Play audio", "Toggle sidebar", "Remove sample", "Go back", and
"Refresh runs" to their button elements.
Comment on lines
+348
to
+360
| // Auto-refresh runs every 10 seconds if there are running evaluations | ||
| useEffect(() => { | ||
| if (activeTab !== 'evaluations') return; | ||
|
|
||
| const hasRunningEvals = runs.some(run => | ||
| run.status === 'running' || run.status === 'processing' || run.status === 'pending' | ||
| ); | ||
|
|
||
| if (hasRunningEvals) { | ||
| const interval = setInterval(() => { | ||
| loadRuns(); | ||
| }, 10000); | ||
| return () => clearInterval(interval); |
There was a problem hiding this comment.
Normalize run status before deciding whether to poll.
This interval check is case-sensitive, but the same file already uses run.status.toLowerCase() elsewhere. If the API returns RUNNING or PENDING, auto-refresh never starts and the runs list stays stale until a manual refresh.
Suggested fix
- const hasRunningEvals = runs.some(run =>
- run.status === 'running' || run.status === 'processing' || run.status === 'pending'
- );
+ const hasRunningEvals = runs.some(run =>
+ ['running', 'processing', 'pending'].includes(run.status.toLowerCase())
+ );📝 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.
Suggested change
| // Auto-refresh runs every 10 seconds if there are running evaluations | |
| useEffect(() => { | |
| if (activeTab !== 'evaluations') return; | |
| const hasRunningEvals = runs.some(run => | |
| run.status === 'running' || run.status === 'processing' || run.status === 'pending' | |
| ); | |
| if (hasRunningEvals) { | |
| const interval = setInterval(() => { | |
| loadRuns(); | |
| }, 10000); | |
| return () => clearInterval(interval); | |
| // Auto-refresh runs every 10 seconds if there are running evaluations | |
| useEffect(() => { | |
| if (activeTab !== 'evaluations') return; | |
| const hasRunningEvals = runs.some(run => | |
| ['running', 'processing', 'pending'].includes(run.status.toLowerCase()) | |
| ); | |
| if (hasRunningEvals) { | |
| const interval = setInterval(() => { | |
| loadRuns(); | |
| }, 10000); | |
| return () => clearInterval(interval); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/text-to-speech/page.tsx` around lines 348 - 360, The auto-refresh check
is case-sensitive and misses statuses like "RUNNING" or "PENDING"; update the
condition inside the useEffect that checks runs to normalize statuses (e.g., use
run.status?.toLowerCase()) before comparing to 'running', 'processing', or
'pending', ensuring you handle undefined/null safely; keep the surrounding logic
(activeTab === 'evaluations', hasRunningEvals variable, and calling
loadRuns/clearInterval) unchanged while replacing the run.status checks with the
normalized comparisons.
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 (1)
app/evaluations/page.tsx (1)
451-451:⚠️ Potential issue | 🟡 MinorUse appropriate semantic HTML for inline text emphasis.
<blockquote>is intended for extended quotations from external sources and will render as a block element with default margins. For inline emphasis of UI element names, use<code>or a styled<span>.Proposed fix
- <li>Click <blockquote>Run Evaluation </blockquote> to start the evaluation process</li> + <li>Click <code className="px-1 rounded" style={{ backgroundColor: '#f5f5f5' }}>Run Evaluation</code> to start the evaluation process</li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/page.tsx` at line 451, Replace the block-level <blockquote> used around the inline UI label "Run Evaluation" with appropriate inline semantics: change the markup in the list item that renders 'Click <blockquote>Run Evaluation</blockquote> to start the evaluation process' to use <code> (or a styled <span> with an aria-label if needed) so the text is inline and semantically appropriate for a UI control name; keep the surrounding <li> and visible text unchanged.
🧹 Nitpick comments (3)
app/text-to-speech/page.tsx (1)
106-123: UnstableonPlayTogglereference causes effect to re-run on every render.The
onPlayTogglecallback is created inline in the parent component, so it changes identity on every render. This causes the useEffect to continuously add/remove event listeners, potentially causing audio playback glitches.♻️ Proposed fix: Use ref to track callback
+ const onPlayToggleRef = useRef(onPlayToggle); + onPlayToggleRef.current = onPlayToggle; + useEffect(() => { const audio = audioRef.current; if (!audio) return; const handleLoadedMetadata = () => setDuration(audio.duration); const handleTimeUpdate = () => setCurrentTime(audio.currentTime); - const handleEnded = () => onPlayToggle(); + const handleEnded = () => onPlayToggleRef.current(); audio.addEventListener('loadedmetadata', handleLoadedMetadata); audio.addEventListener('timeupdate', handleTimeUpdate); audio.addEventListener('ended', handleEnded); return () => { audio.removeEventListener('loadedmetadata', handleLoadedMetadata); audio.removeEventListener('timeupdate', handleTimeUpdate); audio.removeEventListener('ended', handleEnded); }; - }, [onPlayToggle]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/text-to-speech/page.tsx` around lines 106 - 123, The effect is re-running because onPlayToggle is an unstable inline callback; make the effect depend only on stable values by storing onPlayToggle in a ref and using that ref in the event handler: create a ref like onPlayToggleRef = useRef(onPlayToggle), update onPlayToggleRef.current whenever onPlayToggle changes (e.g., in a separate useEffect or directly before registering listeners), and change handleEnded to call onPlayToggleRef.current(); then keep the main useEffect that registers listeners dependent only on audioRef (not onPlayToggle) so it won't re-run on every render; reference audioRef, onPlayToggleRef, and handleEnded in your changes.app/evaluations/page.tsx (2)
91-119: Consider wrappingloadStoredDatasetsinuseCallback.The
loadStoredDatasetsfunction is called inside auseEffectbut isn't included in the dependency array. While this works because the effect triggers onapiKeyschange, wrapping it inuseCallbackwould make the dependency relationship explicit and satisfy thereact-hooks/exhaustive-depsrule.Suggested approach
+ const loadStoredDatasets = useCallback(async () => { - const loadStoredDatasets = async () => { if (!apiKeys.length) return; // ... rest of function - }; + }, [apiKeys]); useEffect(() => { if (apiKeys.length > 0) { loadStoredDatasets(); } - }, [apiKeys]); + }, [apiKeys, loadStoredDatasets]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/page.tsx` around lines 91 - 119, Wrap the loadStoredDatasets function in React's useCallback and include apiKeys in its dependency array so the function identity is stable and the react-hooks/exhaustive-deps rule is satisfied; replace the current declaration of loadStoredDatasets with a useCallback(() => { ... }, [apiKeys]) (keeping references to setStoredDatasets and fetch call intact), ensure useEffect depends on loadStoredDatasets (or keep [apiKeys, loadStoredDatasets]) and add the useCallback import if not already present.
417-458: Consider consistent use of color tokens throughout.The file mixes centralized color tokens (e.g.,
colors.bg.primaryon lines 418-419) with hardcoded hex values (e.g.,#737373,#171717,#fafafaon lines 429-431, 442-446). For maintainability and theme consistency, consider using color tokens throughout the entire file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/page.tsx` around lines 417 - 458, The tooltip UI mixes centralized theme tokens with hardcoded hex colors; replace hardcoded values used in the tooltip/button (e.g., '#737373', '#fafafa', '#171717', '#e5e5e5', '#ffffff') with the appropriate properties from the shared colors object used elsewhere in this component (reference showHowItWorksTooltip state, the button rendering block and the tooltip div) so styling consistently uses colors.bg, colors.text, colors.border (or other existing tokens) for backgroundColor, color and borderColor; update the inline styles in the button onMouseEnter/onMouseLeave block and the tooltip container and headers to use those tokens.
🤖 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/evaluations/page.tsx`:
- Line 451: Replace the block-level <blockquote> used around the inline UI label
"Run Evaluation" with appropriate inline semantics: change the markup in the
list item that renders 'Click <blockquote>Run Evaluation</blockquote> to start
the evaluation process' to use <code> (or a styled <span> with an aria-label if
needed) so the text is inline and semantically appropriate for a UI control
name; keep the surrounding <li> and visible text unchanged.
---
Nitpick comments:
In `@app/evaluations/page.tsx`:
- Around line 91-119: Wrap the loadStoredDatasets function in React's
useCallback and include apiKeys in its dependency array so the function identity
is stable and the react-hooks/exhaustive-deps rule is satisfied; replace the
current declaration of loadStoredDatasets with a useCallback(() => { ... },
[apiKeys]) (keeping references to setStoredDatasets and fetch call intact),
ensure useEffect depends on loadStoredDatasets (or keep [apiKeys,
loadStoredDatasets]) and add the useCallback import if not already present.
- Around line 417-458: The tooltip UI mixes centralized theme tokens with
hardcoded hex colors; replace hardcoded values used in the tooltip/button (e.g.,
'#737373', '#fafafa', '#171717', '#e5e5e5', '#ffffff') with the appropriate
properties from the shared colors object used elsewhere in this component
(reference showHowItWorksTooltip state, the button rendering block and the
tooltip div) so styling consistently uses colors.bg, colors.text, colors.border
(or other existing tokens) for backgroundColor, color and borderColor; update
the inline styles in the button onMouseEnter/onMouseLeave block and the tooltip
container and headers to use those tokens.
In `@app/text-to-speech/page.tsx`:
- Around line 106-123: The effect is re-running because onPlayToggle is an
unstable inline callback; make the effect depend only on stable values by
storing onPlayToggle in a ref and using that ref in the event handler: create a
ref like onPlayToggleRef = useRef(onPlayToggle), update onPlayToggleRef.current
whenever onPlayToggle changes (e.g., in a separate useEffect or directly before
registering listeners), and change handleEnded to call
onPlayToggleRef.current(); then keep the main useEffect that registers listeners
dependent only on audioRef (not onPlayToggle) so it won't re-run on every
render; reference audioRef, onPlayToggleRef, and handleEnded in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3509bdea-736b-4643-9771-4259a9aa365d
📒 Files selected for processing (3)
app/components/TabNavigation.tsxapp/evaluations/page.tsxapp/text-to-speech/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/text-to-speech/page.tsx (2)
106-123: Stabilize theonPlayTogglecallback to avoid re-attaching event listeners on every render.The
onPlayToggleprop is included in the dependency array, but if the parent component doesn't wrap it inuseCallback, this effect will re-run and re-attach event listeners on every render.♻️ Suggested fix: Remove onPlayToggle from deps, use a ref
+ const onPlayToggleRef = useRef(onPlayToggle); + onPlayToggleRef.current = onPlayToggle; + useEffect(() => { const audio = audioRef.current; if (!audio) return; const handleLoadedMetadata = () => setDuration(audio.duration); const handleTimeUpdate = () => setCurrentTime(audio.currentTime); - const handleEnded = () => onPlayToggle(); + const handleEnded = () => onPlayToggleRef.current(); audio.addEventListener('loadedmetadata', handleLoadedMetadata); audio.addEventListener('timeupdate', handleTimeUpdate); audio.addEventListener('ended', handleEnded); return () => { audio.removeEventListener('loadedmetadata', handleLoadedMetadata); audio.removeEventListener('timeupdate', handleTimeUpdate); audio.removeEventListener('ended', handleEnded); }; - }, [onPlayToggle]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/text-to-speech/page.tsx` around lines 106 - 123, The effect currently re-attaches audio event listeners whenever the onPlayToggle prop identity changes; stabilize it by removing onPlayToggle from the dependency array and instead store the latest callback in a ref (e.g., create onPlayToggleRef.current and update it inside a useEffect when onPlayToggle changes), then have handleEnded call onPlayToggleRef.current(); keep audioRef, handleLoadedMetadata, handleTimeUpdate, and the add/removeEventListener logic the same but with a stable dependency array (no onPlayToggle) so listeners are not re-attached on every render.
232-232: Consider using a constant instead of state forleftPanelWidth.The setter is never used, making
useStateunnecessary overhead.♻️ Suggested fix
- const [leftPanelWidth] = useState(450); + const leftPanelWidth = 450;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/text-to-speech/page.tsx` at line 232, Replace the unnecessary React state for leftPanelWidth with a plain constant: remove the useState call that creates const [leftPanelWidth] = useState(450) and instead declare a constant (e.g., const LEFT_PANEL_WIDTH = 450 or const leftPanelWidth = 450) in page.tsx; also remove the unused useState import if it becomes unused and update any references to use the constant name.
🤖 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/text-to-speech/page.tsx`:
- Around line 770-784: The "Add Sample" button currently uses a conditional
onClick and visual styles to appear disabled; instead set the button's disabled
attribute to apiKeys.length === 0, keep onClick bound to addTextSample
unconditionally, and update the style logic to depend on the disabled state (or
apiKeys.length) so the button is non-focusable and not interactive when no API
keys exist; target the button element and the addTextSample and apiKeys symbols
when making this change.
---
Nitpick comments:
In `@app/text-to-speech/page.tsx`:
- Around line 106-123: The effect currently re-attaches audio event listeners
whenever the onPlayToggle prop identity changes; stabilize it by removing
onPlayToggle from the dependency array and instead store the latest callback in
a ref (e.g., create onPlayToggleRef.current and update it inside a useEffect
when onPlayToggle changes), then have handleEnded call
onPlayToggleRef.current(); keep audioRef, handleLoadedMetadata,
handleTimeUpdate, and the add/removeEventListener logic the same but with a
stable dependency array (no onPlayToggle) so listeners are not re-attached on
every render.
- Line 232: Replace the unnecessary React state for leftPanelWidth with a plain
constant: remove the useState call that creates const [leftPanelWidth] =
useState(450) and instead declare a constant (e.g., const LEFT_PANEL_WIDTH = 450
or const leftPanelWidth = 450) in page.tsx; also remove the unused useState
import if it becomes unused and update any references to use the constant name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c8f67d-c974-453d-a642-fd062c402df9
📒 Files selected for processing (1)
app/text-to-speech/page.tsx
Comment on lines
+770
to
+784
| <button | ||
| onClick={apiKeys.length > 0 ? addTextSample : undefined} | ||
| className="flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium" | ||
| style={{ | ||
| backgroundColor: apiKeys.length > 0 ? colors.accent.primary : colors.bg.secondary, | ||
| color: apiKeys.length > 0 ? '#fff' : colors.text.secondary, | ||
| cursor: apiKeys.length > 0 ? 'pointer' : 'not-allowed', | ||
| }} | ||
| > | ||
| <svg className="w-4 h-4" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | ||
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 4v16m8-8H4" /> | ||
| </svg> | ||
| Add Sample | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Use disabled attribute instead of conditional onClick for the "Add Sample" button.
The button is visually styled as disabled when apiKeys.length === 0, but it remains focusable and interactive because disabled is not set. Keyboard users can still tab to and activate the button (though nothing happens).
🔧 Suggested fix
<button
- onClick={apiKeys.length > 0 ? addTextSample : undefined}
+ onClick={addTextSample}
+ disabled={apiKeys.length === 0}
className="flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium"
style={{
backgroundColor: apiKeys.length > 0 ? colors.accent.primary : colors.bg.secondary,
color: apiKeys.length > 0 ? '#fff' : colors.text.secondary,
- cursor: apiKeys.length > 0 ? 'pointer' : 'not-allowed',
+ cursor: apiKeys.length === 0 ? 'not-allowed' : 'pointer',
+ opacity: apiKeys.length === 0 ? 0.6 : 1,
}}
>📝 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.
Suggested change
| <button | |
| onClick={apiKeys.length > 0 ? addTextSample : undefined} | |
| className="flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium" | |
| style={{ | |
| backgroundColor: apiKeys.length > 0 ? colors.accent.primary : colors.bg.secondary, | |
| color: apiKeys.length > 0 ? '#fff' : colors.text.secondary, | |
| cursor: apiKeys.length > 0 ? 'pointer' : 'not-allowed', | |
| }} | |
| > | |
| <svg className="w-4 h-4" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | |
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 4v16m8-8H4" /> | |
| </svg> | |
| Add Sample | |
| </button> | |
| </div> | |
| <button | |
| onClick={addTextSample} | |
| disabled={apiKeys.length === 0} | |
| className="flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium" | |
| style={{ | |
| backgroundColor: apiKeys.length > 0 ? colors.accent.primary : colors.bg.secondary, | |
| color: apiKeys.length > 0 ? '#fff' : colors.text.secondary, | |
| cursor: apiKeys.length === 0 ? 'not-allowed' : 'pointer', | |
| opacity: apiKeys.length === 0 ? 0.6 : 1, | |
| }} | |
| > | |
| <svg className="w-4 h-4" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | |
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 4v16m8-8H4" /> | |
| </svg> | |
| Add Sample | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/text-to-speech/page.tsx` around lines 770 - 784, The "Add Sample" button
currently uses a conditional onClick and visual styles to appear disabled;
instead set the button's disabled attribute to apiKeys.length === 0, keep
onClick bound to addTextSample unconditionally, and update the style logic to
depend on the disabled state (or apiKeys.length) so the button is non-focusable
and not interactive when no API keys exist; target the button element and the
addTextSample and apiKeys symbols when making this change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/api/evaluations/stt/datasets/[dataset_id]/route.ts (1)
1-1: Unused import:NextRequest.The
NextRequesttype is imported but not used—the function parameter usesRequestinstead. Consider removing the unused import for cleanliness.🧹 Proposed fix
-import { NextResponse, NextRequest } from 'next/server'; +import { NextResponse } from 'next/server';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/evaluations/stt/datasets/`[dataset_id]/route.ts at line 1, The import list includes an unused symbol NextRequest from 'next/server' while the handler uses the global Request type; remove NextRequest from the import (leave NextResponse) to clean up the unused import in the import declaration at the top of the module and avoid the unused symbol warning.
🤖 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/evaluations/stt/datasets/`[dataset_id]/route.ts:
- Line 1: The import list includes an unused symbol NextRequest from
'next/server' while the handler uses the global Request type; remove NextRequest
from the import (leave NextResponse) to clean up the unused import in the import
declaration at the top of the module and avoid the unused symbol warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cd74793-0aa5-4b53-aa2b-9112e0ab83dc
📒 Files selected for processing (2)
app/api/evaluations/stt/datasets/[dataset_id]/route.tsapp/api/evaluations/tts/datasets/[dataset_id]/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/evaluations/tts/datasets/[dataset_id]/route.ts
AkhileshNegi
force-pushed
the
feature/tts-evaluation
branch
from
773c47b to
f32ad58
Compare
Prajna1999
approved these changes
Mar 13, 2026
Closed
This was referenced Mar 17, 2026
Merged
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
2 participants
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
Target issue is #65
New Features
Bug Fixes
Style