Update Inline Style & Refactor code up to 500 LOC#158
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates UI styling to Tailwind, extracts prompt-editor subcomponents and a persistence hook, centralizes STT data in a new hook, introduces dataset listing and modal components with modal-based upload/delete flows, updates waveform/canvas color resolution, and enables ESLint max-lines. ChangesTailwind Styling Migration
Prompt Editor Refactoring with Hooks & Components
Datasets Modal UI Refactoring
Speech-to-Text Data Management Hook
Linting Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…tTech4DevAI/kaapi-frontend into feat/update-inline-styles
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(main)/configurations/prompt-editor/page.tsx (1)
178-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against non-numeric/
NaNurlVersion.
parseInt(urlVersion)has no radix and no NaN check. A URL with?version=foo(or anything non-numeric) will handNaNtoloadSingleVersion, which is likely to result in a 404/500 or a confusing error. Validate and fall back to the latest version on bad input.🛡️ Suggested guard
- const versionNum = urlVersion - ? parseInt(urlVersion) - : items.reduce((a, b) => (b.version > a.version ? b : a)).version; + const parsed = urlVersion ? parseInt(urlVersion, 10) : NaN; + const latest = items.reduce((a, b) => + b.version > a.version ? b : a, + ).version; + const versionNum = Number.isFinite(parsed) ? parsed : latest;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/configurations/prompt-editor/page.tsx around lines 178 - 185, The code currently does parseInt(urlVersion) without a radix or NaN check which can pass NaN into loadSingleVersion; change the logic around urlVersion/versionNum to use parseInt(urlVersion, 10) and validate it with Number.isNaN (or Number.isFinite) and if the parsed value is invalid or urlVersion is falsy, fall back to computing the latest version from items (the existing items.reduce(...) expression); keep using loadSingleVersion(urlConfigId, versionNum), then applyConfig and setEditorReady as before.app/(main)/datasets/page.tsx (1)
53-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch these client-side requests to
clientFetch.This page is a client component, so using
apiFetchhere skips the shared 401 refresh /AUTH_EXPIRED_EVENThandling the repo expects.As per coding guidelines, "Use
clientFetch(endpoint, options)on the client-side for API requests, which handles token refresh on 401 errors and dispatches AUTH_EXPIRED_EVENT when refresh fails".Also applies to: 103-106, 135-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/datasets/page.tsx around lines 53 - 56, Replace client-side uses of apiFetch with clientFetch so token refresh and AUTH_EXPIRED_EVENT handling occur; specifically change the call at the top of page.tsx (the apiFetch<Dataset[] | { data: Dataset[] }>( "/api/evaluations/datasets", apiKey?.key ?? "" )) to use clientFetch with the same endpoint and pass the API key via the options object (e.g., clientFetch("/api/evaluations/datasets", { apiKey: apiKey?.key ?? "" })) and preserve the expected return type handling, update the other occurrences mentioned (the calls around 103-106 and 135-139) similarly, and update imports to import clientFetch (remove or stop using apiFetch) so AUTH_EXPIRED_EVENT/token refresh behavior is used.
🧹 Nitpick comments (4)
app/hooks/useSttData.ts (1)
65-78: ⚡ Quick winConsider extracting response normalization logic.
The nested conditionals handling multiple API response shapes are defensive but difficult to maintain. Consider extracting this pattern into a reusable helper function since similar logic appears in
loadDatasets(lines 103-107),loadRuns(lines 126-129), andloadResults(lines 150-164).♻️ Example refactoring approach
// In a shared utility file function normalizeArrayResponse<T>( data: unknown, possibleKeys: string[] ): T[] { if (Array.isArray(data)) return data; for (const key of possibleKeys) { const value = (data as any)?.[key]; if (Array.isArray(value)) return value; if (value?.data && Array.isArray(value.data)) return value.data; } return []; } // Usage in hook const rawList = normalizeArrayResponse<RawLanguage>( data, ['languages', 'data'] );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useSttData.ts` around lines 65 - 78, Extract the repeated response-normalization logic into a reusable helper (e.g., normalizeArrayResponse<T>(data: unknown, possibleKeys: string[]): T[]) placed in a shared utilities module, then replace the conditional block that builds rawList in useSttData.ts (the block assigning rawList) with a call to normalizeArrayResponse<RawLanguage>(data, ['languages','data']); likewise update the similar normalization usages in loadDatasets, loadRuns, and loadResults to call this helper so all variants (array, {data: [...]} and nested {data:{data:[...]}}) are handled consistently and return an empty array by default.app/hooks/useConfigPersistence.ts (1)
47-50: 💤 Low valueMove auth check before the name-validation toast.
A user not logged in gets "Please enter a configuration name" instead of "Please log in…" if their name is blank. Reordering the two guards (or combining them) gives more accurate feedback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/hooks/useConfigPersistence.ts` around lines 47 - 50, In useConfigPersistence, the guard that shows the "Please enter a configuration name" toast is currently executed before checking authentication; change the order (or combine the checks) so isAuthenticated from useAuth is evaluated first and, if false, show the "Please log in…" toast (before validating name/activeKey). Update the logic around toast, activeKey and isAuthenticated (and preserve setIsSaving/isSaving behavior) so unauthenticated users receive the login message instead of the name-validation message.app/(main)/configurations/prompt-editor/page.tsx (2)
230-238: 💤 Low valueRedundant effect dependencies.
provider,temperature, andtoolsare all subsumed bycurrentConfigBlob(which is already a dep) — the effect body never references those three standalone state values. They cause the diff effect to re-run on every keystroke that touchessetProvider/setTemperature/setToolsindependently ofcurrentConfigBlob. Trim the deps to what the effect actually reads.}, [ selectedConfigId, currentContent, currentConfigBlob, - provider, - temperature, - tools, savedConfigs, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/configurations/prompt-editor/page.tsx around lines 230 - 238, The effect's dependency array includes provider, temperature, and tools even though the effect body only reads selectedConfigId, currentContent, currentConfigBlob, and savedConfigs; remove provider, temperature, and tools from the dependency list in the effect (the array currently passed to useEffect) so it depends only on the values actually referenced (selectedConfigId, currentContent, currentConfigBlob, savedConfigs) and ensure any lint rule complaints are addressed by keeping the actual referenced symbols in the array.
27-82: 🏗️ Heavy liftAddress the lint complexity warning by extracting URL-init/diff effects.
Static analysis flags
PromptEditorContentat complexity 12 / 41 statements, exceeding the configured limits (10 / 20). The save/rename extraction in this PR is a solid step, but the URL-param initialization effect (lines 156-196) and the unsaved-changes diff effect (lines 206-238) are both self-contained and prime candidates to move into dedicated hooks (e.g.useUrlConfigInit,useUnsavedConfigChanges) to bring this component back under the project's lint budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/configurations/prompt-editor/page.tsx around lines 27 - 82, PromptEditorContent is flagged for cyclomatic complexity; extract the URL-param initialization effect and the unsaved-changes diff effect into two hooks to reduce complexity. Create useUrlConfigInit that encapsulates the effect currently using searchParams/urlConfigId/urlVersion/urlDatasetId/urlExperimentName/fromEvaluations/isNewConfig and calls back into PromptEditorContent via setters (e.g., setEditorReady, setSelectedConfigId, setCurrentConfigBlob, setCurrentContent, setCurrentConfigName, setCurrentConfigVersion, setCurrentConfigParentId) and uses functions from the component (loadVersionsForConfig, loadSingleVersion) plus version maps (hookVersionItemsMap, stableVersionItemsMap) to perform the same initialization logic. Create useUnsavedConfigChanges that contains the effect comparing currentConfigBlob/currentContent/selectedVersion/compareWith to setHasUnsavedChanges and setCommitMessage, and expose any small helpers needed by the component. Replace the two in-component effects with calls to useUrlConfigInit(...) and useUnsavedConfigChanges(...), passing only the minimal state and callbacks (also include editorInitialized and setEditorReady where relevant) so PromptEditorContent retains behavior while staying under lint complexity limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(main)/datasets/page.tsx:
- Around line 153-155: When datasets change you must clamp currentPage so it
never exceeds totalPages: calculate totalPages = Math.ceil(datasets.length /
ITEMS_PER_PAGE) and, if currentPage > Math.max(1, totalPages), set currentPage =
Math.max(1, totalPages) before computing currentDatasets; implement this clamp
in the component (e.g., a useEffect watching datasets or right after computing
totalPages) so datasets.slice(start, start + ITEMS_PER_PAGE) always uses a valid
page.
In `@app/`(main)/speech-to-text/page.tsx:
- Around line 68-72: The effect that updates datasetLanguageId (useEffect) will
overwrite a user-selected language whenever languages changes; modify the effect
so it only sets setDatasetLanguageId(languages[0].id) when the current
datasetLanguageId is still the default/uninitialized value (e.g., 1 or null) to
avoid clobbering user selection, and include datasetLanguageId in the dependency
array so the guard uses the latest value; keep references to useEffect,
languages, datasetLanguageId, and setDatasetLanguageId when making the change.
In `@app/components/ComingSoon.tsx`:
- Line 60: The Button in the ComingSoon component currently calls router.back()
which can do nothing for direct-entry visits; update the onClick handler for the
Button (in ComingSoon) to attempt router.back() and fall back to a safe route
(e.g., router.push('/') or router.replace('/dashboard')) when history is
absent—detect with a check like window.history.length <= 1 or by awaiting
router.back() and then calling the fallback; ensure you reference the same
Button onClick and the router instance used in ComingSoon.
In `@app/components/datasets/DatasetListing.tsx`:
- Around line 35-42: The Upload CTA in DatasetListing (the Button that calls
onUploadNew with the PlusIcon) is still active when the component renders the
"Login required" state; update DatasetListing to check the auth state (e.g., the
user or isAuthenticated prop/state used to render the login message) and either
hide the Button entirely or render it disabled and no-op when auth is missing.
Ensure the conditional references the same auth variable used elsewhere in
DatasetListing so unauthenticated users cannot trigger onUploadNew.
In `@app/components/datasets/UploadDatasetModal.tsx`:
- Around line 32-33: The hidden native file input isn't being cleared when
resetting the form, so re-selecting the same CSV may not trigger onChange;
update the reset/close/success paths that currently clear selectedFile
(references: fileInputRef, selectedFile, datasetName, isUploading) to also clear
the native input by setting fileInputRef.current.value to an empty string (and
keep fileInputRef.current = null if you already null it), and ensure this is
done wherever selectedFile is reset (e.g., after successful upload and on modal
close) so the browser will fire onChange for the same file next time.
- Around line 36-42: The modal can still close via backdrop click or Escape even
when an upload is in flight; update UploadDatasetModal so Modal respects the
upload state: either pass Modal props to disable backdrop/Escape dismissal if
supported (e.g., disableBackdropClick/disableEscapeKeyDown or equivalent) or
guard the onClose handler so it becomes a no-op while uploadInFlight/isUploading
is true; ensure you reference the Modal usage and the onClose prop in
UploadDatasetModal and use the existing upload state
(uploadInFlight/isUploading) to prevent premature closing.
In `@app/components/prompt-editor/LoadConfigDropdown.tsx`:
- Around line 82-95: handleSelectVersion can silently do nothing when a
requested config isn't found and loadSingleVersion is absent or returns null;
update handleSelectVersion to handle that error path by showing user feedback
and closing the dropdown: if full is falsy and either loadSingleVersion is
undefined or returns null, call setIsOpen(false) and surface an error (e.g.,
trigger the existing toast/notification utility or call a provided onError
handler) before returning, otherwise proceed to call onLoadConfig(config) as
before; reference handleSelectVersion, savedConfigs, loadSingleVersion,
onLoadConfig, and setIsOpen when making the changes.
In `@app/components/prompt-editor/ToolsSection.tsx`:
- Around line 40-44: The list rendering in tools.map uses key={index}, which
breaks reconciliation when items are removed (see tools.map,
onRemoveTool(index), showTooltip and Field/input), so replace the positional key
with a stable per-tool identifier: add an id property to the Tool objects when
they are created (or generate a stable id in the code path that adds tools) and
use key={tool.id} in the map and for any related keyed elements like
showTooltip; ensure all usages that relied on index for identity (tooltip keys,
focused/controlled Field inputs) switch to tool.id so focus/IME/tooltip state
stays consistent after removals.
- Around line 87-98: The numeric input handling for tool.max_num_results is
fragile: stop using parseInt(e.target.value) || 20 because it forces 20 for
empty or "0" and lacks radix/constraints; instead, keep the raw input when
e.target.value === "" so the field can be temporarily empty, parse with
parseInt(value, 10) only when value !== "" and Number.isInteger(parsed) to
produce a sanitized integer, clamp with Math.max(0, parsed) if you want
non-negative results, and call onUpdateTool(index, "max_num_results",
sanitizedValue) (or null/undefined for empty) accordingly; also add input
attributes min={0} step={1} to the <input> to prevent negatives/decimals and
make typing predictable.
In `@app/hooks/useConfigPersistence.ts`:
- Around line 74-83: The aggregation of max_num_results is incorrect: the
forEach with the condition allKnowledgeBaseIds.length ===
tool.knowledge_base_ids.length only matches on the first pushed tool and is
unclear; instead explicitly pick the first tool's max_num_results (or clearly
document a different policy). Replace the loop logic around
currentConfigBlob.completion.params.tools so that you compute
allKnowledgeBaseIds by flattening tool.knowledge_base_ids (e.g., using flatMap
or reduce) and set maxNumResults = tools[0].max_num_results when tools.length >
0 (or if you intended "last tool wins", assign maxNumResults =
tool.max_num_results on each iteration); adjust references to tools,
allKnowledgeBaseIds, and maxNumResults accordingly.
---
Outside diff comments:
In `@app/`(main)/configurations/prompt-editor/page.tsx:
- Around line 178-185: The code currently does parseInt(urlVersion) without a
radix or NaN check which can pass NaN into loadSingleVersion; change the logic
around urlVersion/versionNum to use parseInt(urlVersion, 10) and validate it
with Number.isNaN (or Number.isFinite) and if the parsed value is invalid or
urlVersion is falsy, fall back to computing the latest version from items (the
existing items.reduce(...) expression); keep using
loadSingleVersion(urlConfigId, versionNum), then applyConfig and setEditorReady
as before.
In `@app/`(main)/datasets/page.tsx:
- Around line 53-56: Replace client-side uses of apiFetch with clientFetch so
token refresh and AUTH_EXPIRED_EVENT handling occur; specifically change the
call at the top of page.tsx (the apiFetch<Dataset[] | { data: Dataset[] }>(
"/api/evaluations/datasets", apiKey?.key ?? "" )) to use clientFetch with the
same endpoint and pass the API key via the options object (e.g.,
clientFetch("/api/evaluations/datasets", { apiKey: apiKey?.key ?? "" })) and
preserve the expected return type handling, update the other occurrences
mentioned (the calls around 103-106 and 135-139) similarly, and update imports
to import clientFetch (remove or stop using apiFetch) so
AUTH_EXPIRED_EVENT/token refresh behavior is used.
---
Nitpick comments:
In `@app/`(main)/configurations/prompt-editor/page.tsx:
- Around line 230-238: The effect's dependency array includes provider,
temperature, and tools even though the effect body only reads selectedConfigId,
currentContent, currentConfigBlob, and savedConfigs; remove provider,
temperature, and tools from the dependency list in the effect (the array
currently passed to useEffect) so it depends only on the values actually
referenced (selectedConfigId, currentContent, currentConfigBlob, savedConfigs)
and ensure any lint rule complaints are addressed by keeping the actual
referenced symbols in the array.
- Around line 27-82: PromptEditorContent is flagged for cyclomatic complexity;
extract the URL-param initialization effect and the unsaved-changes diff effect
into two hooks to reduce complexity. Create useUrlConfigInit that encapsulates
the effect currently using
searchParams/urlConfigId/urlVersion/urlDatasetId/urlExperimentName/fromEvaluations/isNewConfig
and calls back into PromptEditorContent via setters (e.g., setEditorReady,
setSelectedConfigId, setCurrentConfigBlob, setCurrentContent,
setCurrentConfigName, setCurrentConfigVersion, setCurrentConfigParentId) and
uses functions from the component (loadVersionsForConfig, loadSingleVersion)
plus version maps (hookVersionItemsMap, stableVersionItemsMap) to perform the
same initialization logic. Create useUnsavedConfigChanges that contains the
effect comparing currentConfigBlob/currentContent/selectedVersion/compareWith to
setHasUnsavedChanges and setCommitMessage, and expose any small helpers needed
by the component. Replace the two in-component effects with calls to
useUrlConfigInit(...) and useUnsavedConfigChanges(...), passing only the minimal
state and callbacks (also include editorInitialized and setEditorReady where
relevant) so PromptEditorContent retains behavior while staying under lint
complexity limits.
In `@app/hooks/useConfigPersistence.ts`:
- Around line 47-50: In useConfigPersistence, the guard that shows the "Please
enter a configuration name" toast is currently executed before checking
authentication; change the order (or combine the checks) so isAuthenticated from
useAuth is evaluated first and, if false, show the "Please log in…" toast
(before validating name/activeKey). Update the logic around toast, activeKey and
isAuthenticated (and preserve setIsSaving/isSaving behavior) so unauthenticated
users receive the login message instead of the name-validation message.
In `@app/hooks/useSttData.ts`:
- Around line 65-78: Extract the repeated response-normalization logic into a
reusable helper (e.g., normalizeArrayResponse<T>(data: unknown, possibleKeys:
string[]): T[]) placed in a shared utilities module, then replace the
conditional block that builds rawList in useSttData.ts (the block assigning
rawList) with a call to normalizeArrayResponse<RawLanguage>(data,
['languages','data']); likewise update the similar normalization usages in
loadDatasets, loadRuns, and loadResults to call this helper so all variants
(array, {data: [...]} and nested {data:{data:[...]}}) are handled consistently
and return an empty array by default.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c308d75-599e-410a-9015-babb5940b01a
📒 Files selected for processing (29)
app/(main)/configurations/prompt-editor/page.tsxapp/(main)/datasets/page.tsxapp/(main)/evaluations/page.tsxapp/(main)/speech-to-text/page.tsxapp/(main)/text-to-speech/page.tsxapp/components/ComingSoon.tsxapp/components/datasets/DatasetCard.tsxapp/components/datasets/DatasetListing.tsxapp/components/datasets/DatasetListingSkeleton.tsxapp/components/datasets/DeleteDatasetModal.tsxapp/components/datasets/UploadDatasetModal.tsxapp/components/evaluations/EvalDatasetDescription.tsxapp/components/prompt-editor/ConfigDiffPane.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/ConfigNameSection.tsxapp/components/prompt-editor/LoadConfigDropdown.tsxapp/components/prompt-editor/PromptDiffPane.tsxapp/components/prompt-editor/ToolsSection.tsxapp/components/speech-to-text/AudioPlayer.tsxapp/components/speech-to-text/AudioPlayerFromUrl.tsxapp/components/speech-to-text/DatasetDescription.tsxapp/components/speech-to-text/TranscriptionDiffViewer.tsxapp/components/speech-to-text/WaveformVisualizer.tsxapp/components/text-to-speech/AudioPlayerFromUrl.tsxapp/components/text-to-speech/DatasetDescription.tsxapp/hooks/useConfigPersistence.tsapp/hooks/useSttData.tsapp/lib/colors.tseslint.config.mjs
💤 Files with no reviewable changes (1)
- app/lib/colors.ts
Comment on lines
+153
to
+155
| const totalPages = Math.ceil(datasets.length / ITEMS_PER_PAGE); | ||
| const start = (currentPage - 1) * ITEMS_PER_PAGE; | ||
| const currentDatasets = datasets.slice(start, start + ITEMS_PER_PAGE); |
There was a problem hiding this comment.
Clamp currentPage after deletes and refetches.
If the last item on the last page is removed, currentPage can point past totalPages. The next render slices an empty page and shows “No datasets found” even though earlier pages still contain data.
Proposed fix
const totalPages = Math.ceil(datasets.length / ITEMS_PER_PAGE);
+ useEffect(() => {
+ if (totalPages === 0 && currentPage !== 1) {
+ setCurrentPage(1);
+ return;
+ }
+
+ if (totalPages > 0 && currentPage > totalPages) {
+ setCurrentPage(totalPages);
+ }
+ }, [currentPage, totalPages]);
+
const start = (currentPage - 1) * ITEMS_PER_PAGE;
const currentDatasets = datasets.slice(start, start + ITEMS_PER_PAGE);📝 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 totalPages = Math.ceil(datasets.length / ITEMS_PER_PAGE); | |
| const start = (currentPage - 1) * ITEMS_PER_PAGE; | |
| const currentDatasets = datasets.slice(start, start + ITEMS_PER_PAGE); | |
| const totalPages = Math.ceil(datasets.length / ITEMS_PER_PAGE); | |
| useEffect(() => { | |
| if (totalPages === 0 && currentPage !== 1) { | |
| setCurrentPage(1); | |
| return; | |
| } | |
| if (totalPages > 0 && currentPage > totalPages) { | |
| setCurrentPage(totalPages); | |
| } | |
| }, [currentPage, totalPages]); | |
| const start = (currentPage - 1) * ITEMS_PER_PAGE; | |
| const currentDatasets = datasets.slice(start, start + ITEMS_PER_PAGE); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/`(main)/datasets/page.tsx around lines 153 - 155, When datasets change
you must clamp currentPage so it never exceeds totalPages: calculate totalPages
= Math.ceil(datasets.length / ITEMS_PER_PAGE) and, if currentPage > Math.max(1,
totalPages), set currentPage = Math.max(1, totalPages) before computing
currentDatasets; implement this clamp in the component (e.g., a useEffect
watching datasets or right after computing totalPages) so datasets.slice(start,
start + ITEMS_PER_PAGE) always uses a valid page.
Comment on lines
68
to
+72
| useEffect(() => { | ||
| loadLanguages(); | ||
| loadDatasets(); | ||
| if (activeTab === "evaluations") { | ||
| loadRuns(); | ||
| if (languages.length > 0 && languages[0]?.id) { | ||
| setDatasetLanguageId(languages[0].id); | ||
| } | ||
| }, [apiKeys, activeTab]); | ||
| }, [languages]); |
There was a problem hiding this comment.
Potential race condition with language initialization.
The effect unconditionally sets datasetLanguageId whenever languages changes. If the user manually selects a language before languages finishes loading, their selection will be overridden. While unlikely in practice (languages loads quickly on mount), consider guarding the effect:
useEffect(() => {
if (languages.length > 0 && languages[0]?.id && datasetLanguageId === 1) {
setDatasetLanguageId(languages[0].id);
}
}, [languages, datasetLanguageId]);This preserves user selection unless still at the default fallback value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/`(main)/speech-to-text/page.tsx around lines 68 - 72, The effect that
updates datasetLanguageId (useEffect) will overwrite a user-selected language
whenever languages changes; modify the effect so it only sets
setDatasetLanguageId(languages[0].id) when the current datasetLanguageId is
still the default/uninitialized value (e.g., 1 or null) to avoid clobbering user
selection, and include datasetLanguageId in the dependency array so the guard
uses the latest value; keep references to useEffect, languages,
datasetLanguageId, and setDatasetLanguageId when making the change.
| e.currentTarget.style.backgroundColor = colors.accent.primary; | ||
| }} | ||
| > | ||
| <Button variant="primary" size="md" onClick={() => router.back()}> |
There was a problem hiding this comment.
Add a fallback when browser history is empty
router.back() may do nothing for direct-entry visits, which can trap users on this page. Add a fallback route (e.g., home or dashboard) when history is unavailable.
Suggested patch
- <Button variant="primary" size="md" onClick={() => router.back()}>
+ <Button
+ variant="primary"
+ size="md"
+ onClick={() => {
+ if (window.history.length > 1) router.back();
+ else router.push("/");
+ }}
+ >
← Go Back
</Button>📝 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 variant="primary" size="md" onClick={() => router.back()}> | |
| <Button | |
| variant="primary" | |
| size="md" | |
| onClick={() => { | |
| if (window.history.length > 1) router.back(); | |
| else router.push("/"); | |
| }} | |
| > | |
| ← Go Back | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/ComingSoon.tsx` at line 60, The Button in the ComingSoon
component currently calls router.back() which can do nothing for direct-entry
visits; update the onClick handler for the Button (in ComingSoon) to attempt
router.back() and fall back to a safe route (e.g., router.push('/') or
router.replace('/dashboard')) when history is absent—detect with a check like
window.history.length <= 1 or by awaiting router.back() and then calling the
fallback; ensure you reference the same Button onClick and the router instance
used in ComingSoon.
Comment on lines
+35
to
+42
| <div className="flex items-center justify-between mb-4"> | ||
| <h2 className="text-base font-semibold text-text-primary"> | ||
| Your Datasets | ||
| </h2> | ||
| <Button variant="primary" size="sm" onClick={onUploadNew}> | ||
| <PlusIcon className="w-4 h-4" /> | ||
| Upload New Dataset | ||
| </Button> |
There was a problem hiding this comment.
Disable or hide the upload CTA when auth is missing.
The component renders a “Login required” state, but the header still lets unauthenticated users open the upload flow.
Proposed fix
- <Button variant="primary" size="sm" onClick={onUploadNew}>
+ <Button
+ variant="primary"
+ size="sm"
+ onClick={onUploadNew}
+ disabled={!isAuthenticated}
+ >
<PlusIcon className="w-4 h-4" />
Upload New Dataset
</Button>📝 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
| <div className="flex items-center justify-between mb-4"> | |
| <h2 className="text-base font-semibold text-text-primary"> | |
| Your Datasets | |
| </h2> | |
| <Button variant="primary" size="sm" onClick={onUploadNew}> | |
| <PlusIcon className="w-4 h-4" /> | |
| Upload New Dataset | |
| </Button> | |
| <div className="flex items-center justify-between mb-4"> | |
| <h2 className="text-base font-semibold text-text-primary"> | |
| Your Datasets | |
| </h2> | |
| <Button | |
| variant="primary" | |
| size="sm" | |
| onClick={onUploadNew} | |
| disabled={!isAuthenticated} | |
| > | |
| <PlusIcon className="w-4 h-4" /> | |
| Upload New Dataset | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/datasets/DatasetListing.tsx` around lines 35 - 42, The Upload
CTA in DatasetListing (the Button that calls onUploadNew with the PlusIcon) is
still active when the component renders the "Login required" state; update
DatasetListing to check the auth state (e.g., the user or isAuthenticated
prop/state used to render the login message) and either hide the Button entirely
or render it disabled and no-op when auth is missing. Ensure the conditional
references the same auth variable used elsewhere in DatasetListing so
unauthenticated users cannot trigger onUploadNew.
Comment on lines
+32
to
+33
| const fileInputRef = useRef<HTMLInputElement>(null); | ||
| const isDisabled = !selectedFile || !datasetName.trim() || isUploading; |
There was a problem hiding this comment.
Clear the hidden file input when the form resets.
Resetting selectedFile alone doesn't reset the native file input. Re-selecting the same CSV after closing/success can stop firing onChange.
Proposed fix
-import { useRef, type ChangeEvent } from "react";
+import { useEffect, useRef, type ChangeEvent } from "react";
@@
const fileInputRef = useRef<HTMLInputElement>(null);
const isDisabled = !selectedFile || !datasetName.trim() || isUploading;
+
+ useEffect(() => {
+ if (!selectedFile && fileInputRef.current) {
+ fileInputRef.current.value = "";
+ }
+ }, [selectedFile]);Also applies to: 53-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/datasets/UploadDatasetModal.tsx` around lines 32 - 33, The
hidden native file input isn't being cleared when resetting the form, so
re-selecting the same CSV may not trigger onChange; update the
reset/close/success paths that currently clear selectedFile (references:
fileInputRef, selectedFile, datasetName, isUploading) to also clear the native
input by setting fileInputRef.current.value to an empty string (and keep
fileInputRef.current = null if you already null it), and ensure this is done
wherever selectedFile is reset (e.g., after successful upload and on modal
close) so the browser will fire onChange for the same file next time.
Comment on lines
+36
to
+42
| <Modal | ||
| open={open} | ||
| onClose={onClose} | ||
| title="Upload New Dataset" | ||
| maxWidth="max-w-2xl" | ||
| maxHeight="max-h-[90vh]" | ||
| > |
There was a problem hiding this comment.
Block backdrop/Escape dismissal while upload is in flight.
Right now the footer disables Cancel, but the modal itself can still close through onClose. That allows the form to reset while the request is still running.
Proposed fix
<Modal
open={open}
- onClose={onClose}
+ onClose={isUploading ? () => {} : onClose}
title="Upload New Dataset"
maxWidth="max-w-2xl"
maxHeight="max-h-[90vh]"
>📝 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
| <Modal | |
| open={open} | |
| onClose={onClose} | |
| title="Upload New Dataset" | |
| maxWidth="max-w-2xl" | |
| maxHeight="max-h-[90vh]" | |
| > | |
| <Modal | |
| open={open} | |
| onClose={isUploading ? () => {} : onClose} | |
| title="Upload New Dataset" | |
| maxWidth="max-w-2xl" | |
| maxHeight="max-h-[90vh]" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/datasets/UploadDatasetModal.tsx` around lines 36 - 42, The
modal can still close via backdrop click or Escape even when an upload is in
flight; update UploadDatasetModal so Modal respects the upload state: either
pass Modal props to disable backdrop/Escape dismissal if supported (e.g.,
disableBackdropClick/disableEscapeKeyDown or equivalent) or guard the onClose
handler so it becomes a no-op while uploadInFlight/isUploading is true; ensure
you reference the Modal usage and the onClose prop in UploadDatasetModal and use
the existing upload state (uploadInFlight/isUploading) to prevent premature
closing.
Comment on lines
+82
to
+95
| const handleSelectVersion = async (item: ConfigVersionItems) => { | ||
| const full = savedConfigs.find( | ||
| (c) => c.config_id === item.config_id && c.version === item.version, | ||
| ); | ||
| const config = | ||
| full ?? | ||
| (loadSingleVersion | ||
| ? await loadSingleVersion(item.config_id, item.version) | ||
| : null); | ||
| if (config) { | ||
| onLoadConfig(config); | ||
| setIsOpen(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Silent failure when loadSingleVersion returns null.
If savedConfigs does not contain the requested version and loadSingleVersion either isn't provided or resolves to null (e.g., network error, missing version), handleSelectVersion returns without invoking onLoadConfig and without closing the dropdown — the user gets no feedback and the click appears to do nothing. Consider surfacing an error toast and/or closing the dropdown in that path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/prompt-editor/LoadConfigDropdown.tsx` around lines 82 - 95,
handleSelectVersion can silently do nothing when a requested config isn't found
and loadSingleVersion is absent or returns null; update handleSelectVersion to
handle that error path by showing user feedback and closing the dropdown: if
full is falsy and either loadSingleVersion is undefined or returns null, call
setIsOpen(false) and surface an error (e.g., trigger the existing
toast/notification utility or call a provided onError handler) before returning,
otherwise proceed to call onLoadConfig(config) as before; reference
handleSelectVersion, savedConfigs, loadSingleVersion, onLoadConfig, and
setIsOpen when making the changes.
Comment on lines
+40
to
+44
| {tools.map((tool, index) => ( | ||
| <div | ||
| key={index} | ||
| className="p-3 rounded mb-2 border border-border bg-bg-secondary" | ||
| > |
There was a problem hiding this comment.
Avoid array index as React key for a removable list.
Tools are removable via onRemoveTool(index), so using key={index} makes React reconcile by position. Combined with showTooltip (also keyed by index) and the controlled Field/<input> children, this causes tooltip / focus / IME composition state to attach to whichever tool now occupies that slot after a removal. Prefer a stable per-tool identifier — either an id on the Tool type or a synthetic stable key generated when the tool is added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/prompt-editor/ToolsSection.tsx` around lines 40 - 44, The list
rendering in tools.map uses key={index}, which breaks reconciliation when items
are removed (see tools.map, onRemoveTool(index), showTooltip and Field/input),
so replace the positional key with a stable per-tool identifier: add an id
property to the Tool objects when they are created (or generate a stable id in
the code path that adds tools) and use key={tool.id} in the map and for any
related keyed elements like showTooltip; ensure all usages that relied on index
for identity (tooltip keys, focused/controlled Field inputs) switch to tool.id
so focus/IME/tooltip state stays consistent after removals.
Comment on lines
+87
to
+98
| <input | ||
| type="number" | ||
| value={tool.max_num_results} | ||
| onChange={(e) => | ||
| onUpdateTool( | ||
| index, | ||
| "max_num_results", | ||
| parseInt(e.target.value) || 20, | ||
| ) | ||
| } | ||
| className="w-full px-3 py-2 rounded-lg border border-border bg-bg-primary text-text-primary text-sm placeholder:text-neutral-400 focus:outline-none focus:ring-accent-primary/20 focus:border-accent-primary transition-colors" | ||
| /> |
There was a problem hiding this comment.
Max Results input has fragile parsing and no bounds.
A few issues with the numeric input:
parseInt(e.target.value) || 20snaps the value back to 20 whenever the user clears the field or types0, which makes editing the value awkward (the field can't be transiently empty).parseIntis missing an explicit radix.- No
min/stepattributes, so the spinner allows negatives and decimals thatparseIntwill silently truncate.
♻️ Suggested change
<input
type="number"
+ min={1}
+ step={1}
value={tool.max_num_results}
onChange={(e) =>
- onUpdateTool(
- index,
- "max_num_results",
- parseInt(e.target.value) || 20,
- )
+ onUpdateTool(
+ index,
+ "max_num_results",
+ e.target.value === ""
+ ? 20
+ : Math.max(1, parseInt(e.target.value, 10) || 20),
+ )
}📝 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
| <input | |
| type="number" | |
| value={tool.max_num_results} | |
| onChange={(e) => | |
| onUpdateTool( | |
| index, | |
| "max_num_results", | |
| parseInt(e.target.value) || 20, | |
| ) | |
| } | |
| className="w-full px-3 py-2 rounded-lg border border-border bg-bg-primary text-text-primary text-sm placeholder:text-neutral-400 focus:outline-none focus:ring-accent-primary/20 focus:border-accent-primary transition-colors" | |
| /> | |
| <input | |
| type="number" | |
| min={1} | |
| step={1} | |
| value={tool.max_num_results} | |
| onChange={(e) => | |
| onUpdateTool( | |
| index, | |
| "max_num_results", | |
| e.target.value === "" | |
| ? 20 | |
| : Math.max(1, parseInt(e.target.value, 10) || 20), | |
| ) | |
| } | |
| className="w-full px-3 py-2 rounded-lg border border-border bg-bg-primary text-text-primary text-sm placeholder:text-neutral-400 focus:outline-none focus:ring-accent-primary/20 focus:border-accent-primary transition-colors" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/prompt-editor/ToolsSection.tsx` around lines 87 - 98, The
numeric input handling for tool.max_num_results is fragile: stop using
parseInt(e.target.value) || 20 because it forces 20 for empty or "0" and lacks
radix/constraints; instead, keep the raw input when e.target.value === "" so the
field can be temporarily empty, parse with parseInt(value, 10) only when value
!== "" and Number.isInteger(parsed) to produce a sanitized integer, clamp with
Math.max(0, parsed) if you want non-negative results, and call
onUpdateTool(index, "max_num_results", sanitizedValue) (or null/undefined for
empty) accordingly; also add input attributes min={0} step={1} to the <input> to
prevent negatives/decimals and make typing predictable.
Comment on lines
+74
to
+83
| const tools = currentConfigBlob.completion.params.tools || []; | ||
| const allKnowledgeBaseIds: string[] = []; | ||
| let maxNumResults = 20; | ||
|
|
||
| tools.forEach((tool) => { | ||
| allKnowledgeBaseIds.push(...tool.knowledge_base_ids); | ||
| if (allKnowledgeBaseIds.length === tool.knowledge_base_ids.length) { | ||
| maxNumResults = tool.max_num_results; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Obscure max_num_results aggregation — looks like a latent bug.
The condition allKnowledgeBaseIds.length === tool.knowledge_base_ids.length is only true on the very first iteration that pushes anything (i.e., when allKnowledgeBaseIds was empty just before the push). For two or more tools it silently falls through, so maxNumResults ends up taking only the first tool's value while every tool's knowledge_base_ids are flattened. If the intent is "use the first tool's max", say so explicitly; if the intent is "use the last/aggregate", the code does the wrong thing.
♻️ Suggested rewrite (assumes "first tool wins")
- const tools = currentConfigBlob.completion.params.tools || [];
- const allKnowledgeBaseIds: string[] = [];
- let maxNumResults = 20;
-
- tools.forEach((tool) => {
- allKnowledgeBaseIds.push(...tool.knowledge_base_ids);
- if (allKnowledgeBaseIds.length === tool.knowledge_base_ids.length) {
- maxNumResults = tool.max_num_results;
- }
- });
+ const tools = currentConfigBlob.completion.params.tools || [];
+ const allKnowledgeBaseIds: string[] = tools.flatMap(
+ (t) => t.knowledge_base_ids,
+ );
+ // Multiple file_search tools are flattened on the wire; preserve the
+ // first tool's max_num_results as the canonical value.
+ const maxNumResults = tools[0]?.max_num_results ?? 20;📝 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 tools = currentConfigBlob.completion.params.tools || []; | |
| const allKnowledgeBaseIds: string[] = []; | |
| let maxNumResults = 20; | |
| tools.forEach((tool) => { | |
| allKnowledgeBaseIds.push(...tool.knowledge_base_ids); | |
| if (allKnowledgeBaseIds.length === tool.knowledge_base_ids.length) { | |
| maxNumResults = tool.max_num_results; | |
| } | |
| }); | |
| const tools = currentConfigBlob.completion.params.tools || []; | |
| const allKnowledgeBaseIds: string[] = tools.flatMap( | |
| (t) => t.knowledge_base_ids, | |
| ); | |
| // Multiple file_search tools are flattened on the wire; preserve the | |
| // first tool's max_num_results as the canonical value. | |
| const maxNumResults = tools[0]?.max_num_results ?? 20; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/hooks/useConfigPersistence.ts` around lines 74 - 83, The aggregation of
max_num_results is incorrect: the forEach with the condition
allKnowledgeBaseIds.length === tool.knowledge_base_ids.length only matches on
the first pushed tool and is unclear; instead explicitly pick the first tool's
max_num_results (or clearly document a different policy). Replace the loop logic
around currentConfigBlob.completion.params.tools so that you compute
allKnowledgeBaseIds by flattening tool.knowledge_base_ids (e.g., using flatMap
or reduce) and set maxNumResults = tools[0].max_num_results when tools.length >
0 (or if you intended "last tool wins", assign maxNumResults =
tool.max_num_results on each iteration); adjust references to tools,
allKnowledgeBaseIds, and maxNumResults accordingly.
vprashrex
approved these changes
May 11, 2026
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
Two-part cleanup across feature pages and shared components:
colorsobject references and inline style objects with Tailwind utilities + semantic CSS variables (bg-bg-secondary,text-text-primary,border-border, etc.) so styling stays consistent with the design tokens and themable fromglobals.css.Changes
prompt-editor/page.tsxinto auseConfigPersistencehook (~150 lines removed from the page).ConfigEditorPane.tsxintoLoadConfigDropdown,ConfigNameSection, andToolsSection.ConfigDiffPane.tsx: inline colors → semantic Tailwind classes; introduced aVersionPillcomponent for consistent version display.datasets/page.tsxinto dedicated components underapp/components/datasets/:DatasetCard.tsx— individual card with metadata + delete actionDatasetListing.tsx— list display, pagination, empty/error statesDatasetListingSkeleton.tsx— loading skeletonUploadDatasetModal.tsx— upload form (uses sharedField+Modal)DeleteDatasetModal.tsx— delete confirmation (uses sharedModal)useSttDatahook (~140 lines lifted out of the page).colorsimport in favor of Tailwind semantic tokens.colors.*references and inline style objects → Tailwind semantic classes.ComingSoon.tsxnow uses the sharedButtonprimitive instead of a bespoke button.Summary by CodeRabbit
New Features
Style
Refactor
Chore