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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR modernizes the project's linting and type safety infrastructure. It establishes Husky pre-commit hooks with Changes
Sequence Diagram(s)Conditions for generation not met. Changes consist primarily of type safety improvements, linting configuration, error handling standardization, and minor code cleanup across distributed components, without introducing new functional features or substantially altering control flow between 3+ distinct components. Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR spans 60+ files with heterogeneous changes across configuration, type safety, error handling, and component updates. While many error-handling changes are repetitive (catch parameter typing), the diversity of affected areas—API routes, React components, TypeScript interfaces, linting configs, and CI workflows—combined with several files containing substantial refactoring (SimplifiedConfigEditor, evaluations pages) and scattered ESLint suppression reasoning, demands moderate-to-high review complexity and thoroughness to verify consistency and correctness. 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: 3
🧹 Nitpick comments (2)
jest.config.ts (1)
141-142: Consider configuringsetupFilesAfterEnvfor@testing-library/jest-dommatchers.The project includes
@testing-library/jest-domwhich provides useful matchers liketoBeInTheDocument(). To make these available in tests, create a setup file and reference it here.♻️ Proposed configuration
Create a file
jest.setup.ts:import '@testing-library/jest-dom';Then update the config:
// A list of paths to modules that run some code to configure or set up the testing framework before each test - // setupFilesAfterEnv: [], + setupFilesAfterEnv: ['<rootDir>/jest.setup.ts'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.config.ts` around lines 141 - 142, The Jest config is not enabling `@testing-library/jest-dom` matchers; create a setup file (e.g., jest.setup.ts) that imports '@testing-library/jest-dom' and then update the jest.config.ts setting setupFilesAfterEnv to include that setup file (use the path string like '<rootDir>/jest.setup.ts') so matchers such as toBeInTheDocument() are available in tests..github/workflows/lint-build.yml (1)
25-29: Remove redundant test step.Running both
npm testandnpm run test:coverageis redundant sincetest:coverageexecutes all tests with coverage enabled. Consider keeping only the coverage step.♻️ Proposed fix
- name: Run Linting run: npm run lint - - name: Run Tests - run: npm test - - name: Run Tests with Coverage run: npm run test:coverage - name: Build the Project run: npm run build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint-build.yml around lines 25 - 29, Remove the redundant "Run Tests" job step that runs "npm test" and keep only the coverage step "Run Tests with Coverage" that runs "npm run test:coverage"; specifically delete the step whose name is "Run Tests" and the command "npm test" from the workflow so only the "Run Tests with Coverage" step remains to execute tests with coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/lint-build.yml:
- Around line 15-20: Add an explicit Node.js setup step using actions/setup-node
before the "Install dependencies" step so the workflow uses a reproducible Node
version; insert a step named like "Setup Node.js" that uses actions/setup-node
and sets node-version (e.g., '20' or the project's required version/read from
.tool-versions) so subsequent steps such as "Install dependencies" (run: npm ci)
run on the specified Node runtime.
In `@eslint.config.mjs`:
- Around line 19-28: Remove the duplicate "no-unused-vars" key from the rules
object in eslint.config.mjs: keep a single declaration (preferably the
comment-backed one that clarifies using `@typescript-eslint`) and delete the other
"no-unused-vars": "off" entry so the rules object no longer contains duplicate
keys; verify the remaining keys include "@typescript-eslint/no-unused-vars":
"error" and the "no-unused-vars" disablement only once.
In `@jest.config.ts`:
- Around line 150-152: Uncomment and set the Jest configuration key
testEnvironment to "jsdom" in the Jest config (the testEnvironment setting in
jest.config.ts) so React/component tests have DOM APIs available; update the
exported config object to include testEnvironment: "jsdom" (replace the
commented line // testEnvironment: "jest-environment-node" with testEnvironment:
"jsdom").
---
Nitpick comments:
In @.github/workflows/lint-build.yml:
- Around line 25-29: Remove the redundant "Run Tests" job step that runs "npm
test" and keep only the coverage step "Run Tests with Coverage" that runs "npm
run test:coverage"; specifically delete the step whose name is "Run Tests" and
the command "npm test" from the workflow so only the "Run Tests with Coverage"
step remains to execute tests with coverage.
In `@jest.config.ts`:
- Around line 141-142: The Jest config is not enabling `@testing-library/jest-dom`
matchers; create a setup file (e.g., jest.setup.ts) that imports
'@testing-library/jest-dom' and then update the jest.config.ts setting
setupFilesAfterEnv to include that setup file (use the path string like
'<rootDir>/jest.setup.ts') so matchers such as toBeInTheDocument() are available
in tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22fb67ca-c842-4480-b6e6-5a568b4572e5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/lint-build.yml.husky/pre-commitREADME.mdapp/components/prompt-editor/DiffView.tsxapp/knowledge-base/page.tsxeslint.config.mjseslint.config.mtsjest.config.tspackage.json
💤 Files with no reviewable changes (1)
- eslint.config.mts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/SimplifiedConfigEditor.tsx (2)
257-421:⚠️ Potential issue | 🟠 MajorPrevent duplicate saves while request is in flight.
handleSaveConfigcan be triggered repeatedly before prior requests finish, which risks duplicate config/version writes.Suggested fix
+ const [isSaving, setIsSaving] = useState<boolean>(false); const handleSaveConfig = async () => { + if (isSaving) return; + setIsSaving(true); if (!configName.trim()) { toast.error("Please enter a configuration name"); + setIsSaving(false); return; } const apiKey = getApiKey(); if (!apiKey) { toast.error("No API key found. Please add an API key in the Keystore."); + setIsSaving(false); return; } try { // existing save logic... setHasUnsavedChanges(false); setCommitMessage(""); } catch (e) { console.error("Failed to save config:", e); toast.error("Failed to save configuration. Please try again."); + } finally { + setIsSaving(false); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` around lines 257 - 421, handleSaveConfig can be invoked multiple times before an earlier save completes; add an "isSaving" lock (state or ref) and check it at the start of handleSaveConfig to early-return if already saving, set isSaving = true before the first network request and set it false in all exit paths (success, error, and early returns), and wire that same isSaving flag into the UI (e.g., disable the Save button) so duplicate config/version creations are prevented; reference handleSaveConfig, setHasUnsavedChanges, setCommitMessage and savedConfigs/update flow when adding the lock and clearing it in finally/error handlers.
548-640:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the icon-only Run Evaluation button.
This button has no text label, so screen readers may announce it ambiguously.
Suggested fix
<button onClick={onRunEvaluation} + aria-label={isEvaluating ? "Evaluation in progress" : "Run evaluation"} + title={isEvaluating ? "Evaluation in progress" : "Run evaluation"} disabled={ ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` around lines 548 - 640, The Run Evaluation button (onRunEvaluation handler, isEvaluating state, the two SVG icons) is icon-only and lacks an accessible name—add an explicit accessible label by providing an aria-label (e.g., aria-label="Run evaluation") or include a visually-hidden text element inside the button that changes with isEvaluating (e.g., "Running evaluation" vs "Run evaluation") so screen readers get meaningful context while preserving the current visual appearance.
🧹 Nitpick comments (13)
app/evaluations/[id]/page.tsx (3)
153-157: Inconsistent error typing in catch block.Other catch blocks in this file use
catch (err: unknown)with type guards, but this one leaves the error untyped. For consistency with the PR's safer error handling approach, consider typing this asunknown.Suggested fix
- } catch (err) { + } catch (err: unknown) { console.error( `Failed to fetch assistant config for ${assistantId}:`, err, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 153 - 157, The catch block that currently reads catch (err) for the assistant config fetch should be changed to catch (err: unknown) and then use a type guard before logging; update the error handling around the console.error call that references assistantId to first check if err is an Error (e.g., err instanceof Error) and log err.message (or otherwise stringify unknown errors) so the console.error call remains safe and consistent with other blocks in this file.
141-142: Verify iftoastshould be included in the dependency array.The
eslint-disablecomment suppresses thereact-hooks/exhaustive-depswarning. Thetoastobject is used inside this callback (lines 114-115) but is not listed in the dependencies. IfuseToast()returns a stable reference, this is fine, but the blanket suppression may hide legitimate warnings for other missing dependencies.Consider either adding
toastto the dependency array or using a more targeted inline suppression comment that documents whytoastis intentionally omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 141 - 142, The effect currently suppresses react-hooks/exhaustive-deps while using the toast object from useToast inside the callback; update the dependency handling so it’s explicit: either add toast to the effect dependency array (the useEffect where apiKeys, selectedKeyId, jobId, exportFormat are listed) or replace the blanket eslint-disable with a targeted inline suppression and a short comment explaining why toast is intentionally stable (e.g., provided by useToast). Locate the useEffect and the useToast() invocation to make the change and ensure the dependency list accurately reflects all used variables.
428-429: Remove unusedstatusColorvariable instead of suppressing the lint warning.The
statusColoris computed but never used. Rather than suppressing the lint error, consider removing this dead code. If it's planned for future use, it can be added back when needed.Suggested fix
- // eslint-disable-next-line `@typescript-eslint/no-unused-vars` - const statusColor = getStatusColor(job.status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 428 - 429, The variable statusColor assigned via getStatusColor(job.status) is unused; remove this dead code instead of suppressing the linter. Delete the line that declares statusColor (the const statusColor = getStatusColor(job.status);) and any accompanying eslint-disable comment, or if you intend to use it later, leave a TODO comment and actually use statusColor where the job status is rendered; locate the assignment near the evaluation page rendering logic referencing job.status to make the change.app/components/SimplifiedConfigEditor.tsx (3)
524-539: Finish color centralization in this style block.This block still mixes hardcoded hex values with
colors, which makes theme updates brittle.Based on learnings: Use centralized colors from
/app/lib/colors.tsinstead of hardcoded hex values for styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` around lines 524 - 539, In SimplifiedConfigEditor (the style block that sets backgroundColor, borderColor, color and the onMouseEnter/onMouseLeave handlers for the element using hasUnsavedChanges), replace all hardcoded hex literals ("#fffbeb", "#fef3c7", "#fafafa", "#ffffff", "#fcd34d", "#b45309", "#e5e5e5", "#171717") with centralized tokens from the colors export in /app/lib/colors.ts (you already use colors.bg.primary) so theme values come from a single source; update the conditional expressions to reference the appropriate colors.* properties (e.g., colors.bg.warning, colors.bg.hover, colors.border.highlight, colors.text.warning, colors.border.default, colors.text.primary) and ensure the onMouseEnter/onMouseLeave handlers set e.currentTarget.style.backgroundColor to those colors.* values instead of hex strings.
87-87: Extractkaapi_api_keysinto a shared localStorage key constant.Hardcoding this key in-component makes future key migrations error-prone.
Based on learnings: Avoid hardcoding status values and localStorage keys; define them as constants in utility files for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` at line 87, The component hardcodes the localStorage key "kaapi_api_keys" (see localStorage.getItem("kaapi_api_keys") in SimplifiedConfigEditor.tsx); extract this into a shared constant (e.g., export const KAAPI_API_KEYS = "kaapi_api_keys") in a central constants or localStorageKeys utility module, update SimplifiedConfigEditor.tsx to import and use that constant where getItem/setItem are used, and update any other modules that reference the same literal to use the new constant to ensure a single source of truth for the key.
467-468: ReplaceanyinhandleConfigChangewith a field-to-type map to maintain type safety.Using
anyhere undermines the type-safety improvements this PR is aiming for. The callback receives typed values (string, number, Tool[]) from ConfigDrawer but loses that type information.However, the suggested refactor needs refinement:
providershould accept any string value (not just"openai") since ConfigDrawer dynamically generates optionsvectorStoreIdsappears unused in ConfigDrawer calls and may not need inclusion in the ConfigChangeMapConsider using a generic overload approach instead for better flexibility:
const handleConfigChange = <K extends keyof typeof handlers>( field: K, value: Parameters<typeof handlers[K]>[0], ) => { handlers[field](value); };Or adjust the suggested
ConfigChangeMapto match actual ConfigDrawer usage patterns while removingvectorStoreIds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/SimplifiedConfigEditor.tsx` around lines 467 - 468, The handleConfigChange function currently types its value as any; replace this with a field-to-type mapping or a generic overload so callers retain precise types (string, number, Tool[], etc.). Either (a) define a ConfigChangeMap matching actual ConfigDrawer fields (omit unused vectorStoreIds and make provider string rather than a literal) and type handleConfigChange as (field: keyof ConfigChangeMap, value: ConfigChangeMap[typeof field]), or (b) implement the suggested generic overload using handlers: const handleConfigChange = <K extends keyof typeof handlers>(field: K, value: Parameters<typeof handlers[K]>[0]) => { handlers[field](value); } so handleConfigChange and referenced handlers preserve exact parameter types; update references to handleConfigChange accordingly.app/document/page.tsx (2)
155-157: Consider applying the same error typing pattern to other catch blocks for consistency.The
fetchDocumentscatch block was updated to usecatch (err: unknown)with aninstanceof Errorguard, but other catch blocks in this file still use implicitanytyping:
- Line 155:
catch (error)inhandleUpload- Line 195:
catch (error)inhandleDeleteDocument- Line 237:
catch (err)inhandleSelectDocumentFor consistency with the PR's type safety improvements, these should follow the same pattern.
♻️ Suggested fix for consistent error typing
- } catch (error) { + } catch (error: unknown) { console.error('Upload error:', error);- } catch (error) { + } catch (error: unknown) { console.error('Delete error:', error);- } catch (err) { + } catch (err: unknown) { console.error('Failed to fetch document details:', err);Also applies to: 195-197, 237-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/document/page.tsx` around lines 155 - 157, Several catch blocks in this file use untyped catch parameters; update handleUpload, handleDeleteDocument, and handleSelectDocument to use catch (err: unknown) and then narrow with if (err instanceof Error) to read err.message (or use a fallback like 'Unknown error') when logging or toasting, mirroring the fetchDocuments change and ensuring consistent type-safe error handling across those handlers.
53-58: ESLint disable is acceptable, but consider usinguseCallbackfor cleaner dependency management.The disable comment suppresses the warning about
fetchDocumentsnot being in the dependency array. While this works, wrappingfetchDocumentsinuseCallbackwith[apiKey]as dependencies would eliminate the need for the disable comment and make the data flow more explicit.♻️ Optional refactor using useCallback
+import { useState, useEffect, useCallback } from 'react'; -import { useState, useEffect } from 'react';- const fetchDocuments = async () => { + const fetchDocuments = useCallback(async () => { if (!apiKey) { setError('No API key found. Please add an API key in the Keystore.'); return; } // ... rest of implementation - }; + }, [apiKey]); useEffect(() => { if (apiKey) { fetchDocuments(); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [apiKey]); + }, [apiKey, fetchDocuments]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/document/page.tsx` around lines 53 - 58, The effect currently suppresses the exhaustive-deps ESLint rule because fetchDocuments is not stable; wrap fetchDocuments in useCallback with [apiKey] (or whatever external values it reads) so it is memoized, then include that memoized fetchDocuments in the useEffect dependency array and remove the eslint-disable comment; update the function referenced as fetchDocuments (and any helper it uses) to be declared via React.useCallback and keep the useEffect dependency list to [apiKey, fetchDocuments] (or just [fetchDocuments] if apiKey is captured inside the callback).app/components/prompt-editor/ConfigEditorPane.tsx (1)
101-102: Theanycast bypasses provider type safety.The
CompletionConfig.provideris typed as the literal'openai', but castingnewProvider as anybypasses this. While the dropdown currently only offers valid options, this cast could mask issues if the dropdown options expand.Consider a type assertion that maintains some safety:
♻️ Optional: Use a type predicate for safer provider casting
+const isValidProvider = (p: string): p is 'openai' => p === 'openai'; + const handleProviderChange = (newProvider: string) => { + if (!isValidProvider(newProvider)) { + console.warn(`Invalid provider: ${newProvider}`); + return; + } onConfigChange({ ...configBlob, completion: { ...configBlob.completion, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - provider: newProvider as any, + provider: newProvider, params: { ...params, model: MODEL_OPTIONS[newProvider as keyof typeof MODEL_OPTIONS][0].value, }, }, }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigEditorPane.tsx` around lines 101 - 102, The code is bypassing type safety by casting newProvider to any for CompletionConfig.provider; replace the any cast with a safe assertion like newProvider as CompletionConfig['provider'] (or, even better, validate via a type predicate e.g. isValidProvider(value): value is CompletionConfig['provider'] and only set provider when it returns true) so the assignment to CompletionConfig.provider preserves the literal union type instead of using any.app/components/ConfigDrawer.tsx (1)
142-159: Consider using theformatDateTimeutility or memoizingDate.now().The
formatTimestampfunction callsDate.now()on every invocation during render, which is why thereact-hooks/purityrule flags it. This could cause inconsistent relative times if the component re-renders frequently.Based on learnings, the codebase has a
formatDateTime()utility in/app/components/utils.ts. Consider using it or refactoring to avoid impure calls during render.♻️ Suggested: Pass current time as parameter
-const formatTimestamp = (timestamp: number | string) => { - // eslint-disable-next-line react-hooks/purity - const now = Date.now(); +const formatTimestamp = (timestamp: number | string, now: number) => { const date = typeof timestamp === 'string' ? new Date(timestamp).getTime() : timestamp; // ... rest of function }; // In component, memoize current time per render: +const now = Date.now(); // Then pass `now` to formatTimestamp calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ConfigDrawer.tsx` around lines 142 - 159, The formatTimestamp function makes an impure call to Date.now() each render; replace its internal Date.now() usage by either (a) delegating to the existing formatDateTime utility (imported from app/components/utils.ts) to format relative times, or (b) change formatTimestamp signature to accept a currentTime parameter (number) and compute diffs against that value so callers pass a memoized Date.now() (e.g., computed with useMemo/useState at the component level). Update all callers of formatTimestamp accordingly (or call formatDateTime instead) and remove the inline Date.now() call inside formatTimestamp to satisfy the react-hooks/purity rule.app/components/prompt-editor/CurrentConfigTab.tsx (1)
65-74: Type narrowing is incomplete forvalue: unknown.The change from
anytounknownis a good type safety improvement, but the narrowing logic is incomplete:
- Line 68:
value as stringassumes the caller passed a string, butunknowndoesn't guarantee this.- Lines 70-71: The
anycast bypasses type checking entirely.Given that callers pass different types (
e.target.valuefor strings,parseInt(...)for numbers), consider adding runtime type guards:♻️ Optional: Add type guards for safer narrowing
const updateTool = (index: number, field: keyof Tool, value: unknown) => { const newTools = [...tools]; if (field === 'knowledge_base_ids') { + if (typeof value !== 'string') { + console.warn('Expected string for knowledge_base_ids'); + return; + } - newTools[index][field] = [value as string]; + newTools[index][field] = [value]; + } else if (field === 'max_num_results') { + newTools[index][field] = typeof value === 'number' ? value : 20; } else { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (newTools[index] as any)[field] = value; + // type field is readonly 'file_search', no update needed } onToolsChange(newTools); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/CurrentConfigTab.tsx` around lines 65 - 74, The updateTool function uses value: unknown but unsafely narrows it (casts to string and uses any); update updateTool to perform runtime type guards before assigning into newTools: for field === 'knowledge_base_ids' ensure value is a string or an array of strings (if string, wrap into [value]; if Array, validate every item is a string) and for other fields use typeof checks (e.g., typeof value === 'string' for string fields, typeof value === 'number' for numeric fields, typeof value === 'boolean' for booleans) or Array.isArray where appropriate, then assign with proper typing to newTools[index][field]; keep using the Tool type and call onToolsChange(newTools) only after validation to avoid unsafe any casts in updateTool.app/components/prompt-editor/ABTestTab.tsx (1)
42-47: Consider a type-safe alternative to theanycast.The
anycast is a workaround for TypeScript's limitation with dynamic property assignment. While acceptable, a slightly cleaner approach would preserve type information:♻️ Optional: Type-safe field update
const updateVariant = (index: number, field: keyof LegacyVariant, value: string) => { const newVariants = [...variants]; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (newVariants[index] as any)[field] = value; + newVariants[index] = { ...newVariants[index], [field]: value }; onVariantsChange(newVariants); };This avoids the
anycast by using object spread with computed property, which TypeScript handles safely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ABTestTab.tsx` around lines 42 - 47, The updateVariant function uses an any cast to assign a dynamic field on a LegacyVariant, which loses type safety; instead, create a new variant object via object spread and a computed property to update the dynamic key (e.g., const updated = { ...newVariants[index], [field]: value } ), replace newVariants[index] with that updated object, and then call onVariantsChange(newVariants); updateVariant, LegacyVariant, variants, and onVariantsChange are the symbols to change so you avoid the (newVariants[index] as any)[field] cast and preserve type checking.app/components/prompt-editor/ConfigDrawer.tsx (1)
62-63: Remove the unusedcurrentContentprop from the interface and destructuring.The prop is declared in the
ConfigDrawerPropsinterface (line 25) and destructured (line 63) but never used within the component. Suppressing the lint warning witheslint-disable-next-linemasks dead code that should be cleaned up. RemovecurrentContentfrom both the interface definition and the destructuring statement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigDrawer.tsx` around lines 62 - 63, Remove the dead prop `currentContent` from the component's props interface and destructuring: in the `ConfigDrawerProps` interface delete the `currentContent` declaration and in the `ConfigDrawer` functional component remove `currentContent` from the destructured parameters (also remove the eslint-disable comment). Verify there are no other references to `currentContent` in this file (e.g., inside `ConfigDrawer` body or helper functions) and run type checks to ensure no remaining references break compilation.
🤖 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/components/SimplifiedConfigEditor.tsx`:
- Line 76: The provider useState in SimplifiedConfigEditor is typed as a loose
string allowing invalid values; change its type to the exact literal union used
by ConfigBlob.completion.provider (use that type or extract the provider type
alias) so provider and setProvider only accept valid provider literals, update
handleConfigChange to validate/guard incoming values before calling setProvider
(do not accept arbitrary strings) and remove or replace any force-cast to
'openai' (the cast at the code handling update in handleConfigChange and where
provider is assumed to be 'openai'); locate the provider state, the
handleConfigChange function, and any places that cast provider to 'openai' and
enforce the typed literal instead.
In `@app/components/speech-to-text/ModelComparisonCard.tsx`:
- Around line 72-77: The effect currently only collapses when status ===
'pending' but includes modelId in its dependency array, so changing modelId in a
non-pending state won't reset expansion; update the logic to reset expansion on
model switch by either (a) removing modelId from this effect if collapse should
only happen on pending, or (b) add an explicit reset for model changes—e.g., in
the existing useEffect or a new useEffect watching modelId call
setIsExpanded(false) unconditionally—so refer to useEffect, status, modelId, and
setIsExpanded when applying the change.
In `@app/evaluations/`[id]/page.tsx:
- Around line 161-179: The function fetchConfigInfo currently issues fetches for
`/api/configs/${configId}` and
`/api/configs/${configId}/versions/${configVersion}` but never uses or returns
the responses, making it dead network work; either remove fetchConfigInfo and
all its call sites (including where it's invoked during component load and any
effect/handler calls) if configVersionInfo/state is intentionally removed, or
restore the function to parse and return the JSON (e.g., await
configResponse.json(), await versionResponse.json()) and update the caller(s) to
consume and set the restored state (configVersionInfo) or otherwise use the
returned data (update state setters, props, or downstream logic); ensure to keep
the try/catch and handle non-ok responses by returning null or throwing so
callers can handle errors.
- Around line 206-208: Sanitize LLM answers before pushing into CSV: when
building the CSV row where row.push uses (group.llm_answers[i] ||
"").replace(...), detect if the trimmed value begins with any of the
CSV-formula-leading characters (=, +, -, @) and prefix it (e.g., with a single
quote) the same way score comments are sanitized; keep the existing quote
escaping .replace(/"/g, '""') and newline removal, but add the leading-character
check on the sanitized string prior to wrapping in surrounding quotes so formula
injection is prevented for group.llm_answers.
- Around line 266-268: Replace the manual .replace(...) sanitization for CSV on
item.input?.question, item.output?.answer, and item.metadata?.ground_truth with
the existing sanitizeCSVCell utility (call sanitizeCSVCell(<value>, true)) just
like score.comment uses; update the three CSV cell entries that currently build
`"${(...).replace(...)}"` to instead use
`"${sanitizeCSVCell(item.input?.question, true)}"`,
`"${sanitizeCSVCell(item.output?.answer, true)}"`, and
`"${sanitizeCSVCell(item.metadata?.ground_truth, true)}"` (referencing these
exact property names and the sanitizeCSVCell function so the same
CSV/formula-injection protections are applied consistently).
In `@app/knowledge-base/page.tsx`:
- Around line 245-246: Remove the incorrect eslint-disable comment sitting above
the top-level async function fetchCollections; instead either wrap
fetchCollections with React.useCallback (so it can be safely referenced in the
useEffect dependency array) and supply the correct dependencies, or move the
function definition inside the useEffect where it's used so no external
dependency suppression is needed; ensure the useEffect that references
fetchCollections lists the stable callback or no dependencies as appropriate and
delete the obsolete "// eslint-disable-next-line react-hooks/exhaustive-deps"
line above fetchCollections.
---
Outside diff comments:
In `@app/components/SimplifiedConfigEditor.tsx`:
- Around line 257-421: handleSaveConfig can be invoked multiple times before an
earlier save completes; add an "isSaving" lock (state or ref) and check it at
the start of handleSaveConfig to early-return if already saving, set isSaving =
true before the first network request and set it false in all exit paths
(success, error, and early returns), and wire that same isSaving flag into the
UI (e.g., disable the Save button) so duplicate config/version creations are
prevented; reference handleSaveConfig, setHasUnsavedChanges, setCommitMessage
and savedConfigs/update flow when adding the lock and clearing it in
finally/error handlers.
- Around line 548-640: The Run Evaluation button (onRunEvaluation handler,
isEvaluating state, the two SVG icons) is icon-only and lacks an accessible
name—add an explicit accessible label by providing an aria-label (e.g.,
aria-label="Run evaluation") or include a visually-hidden text element inside
the button that changes with isEvaluating (e.g., "Running evaluation" vs "Run
evaluation") so screen readers get meaningful context while preserving the
current visual appearance.
---
Nitpick comments:
In `@app/components/ConfigDrawer.tsx`:
- Around line 142-159: The formatTimestamp function makes an impure call to
Date.now() each render; replace its internal Date.now() usage by either (a)
delegating to the existing formatDateTime utility (imported from
app/components/utils.ts) to format relative times, or (b) change formatTimestamp
signature to accept a currentTime parameter (number) and compute diffs against
that value so callers pass a memoized Date.now() (e.g., computed with
useMemo/useState at the component level). Update all callers of formatTimestamp
accordingly (or call formatDateTime instead) and remove the inline Date.now()
call inside formatTimestamp to satisfy the react-hooks/purity rule.
In `@app/components/prompt-editor/ABTestTab.tsx`:
- Around line 42-47: The updateVariant function uses an any cast to assign a
dynamic field on a LegacyVariant, which loses type safety; instead, create a new
variant object via object spread and a computed property to update the dynamic
key (e.g., const updated = { ...newVariants[index], [field]: value } ), replace
newVariants[index] with that updated object, and then call
onVariantsChange(newVariants); updateVariant, LegacyVariant, variants, and
onVariantsChange are the symbols to change so you avoid the (newVariants[index]
as any)[field] cast and preserve type checking.
In `@app/components/prompt-editor/ConfigDrawer.tsx`:
- Around line 62-63: Remove the dead prop `currentContent` from the component's
props interface and destructuring: in the `ConfigDrawerProps` interface delete
the `currentContent` declaration and in the `ConfigDrawer` functional component
remove `currentContent` from the destructured parameters (also remove the
eslint-disable comment). Verify there are no other references to
`currentContent` in this file (e.g., inside `ConfigDrawer` body or helper
functions) and run type checks to ensure no remaining references break
compilation.
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 101-102: The code is bypassing type safety by casting newProvider
to any for CompletionConfig.provider; replace the any cast with a safe assertion
like newProvider as CompletionConfig['provider'] (or, even better, validate via
a type predicate e.g. isValidProvider(value): value is
CompletionConfig['provider'] and only set provider when it returns true) so the
assignment to CompletionConfig.provider preserves the literal union type instead
of using any.
In `@app/components/prompt-editor/CurrentConfigTab.tsx`:
- Around line 65-74: The updateTool function uses value: unknown but unsafely
narrows it (casts to string and uses any); update updateTool to perform runtime
type guards before assigning into newTools: for field === 'knowledge_base_ids'
ensure value is a string or an array of strings (if string, wrap into [value];
if Array, validate every item is a string) and for other fields use typeof
checks (e.g., typeof value === 'string' for string fields, typeof value ===
'number' for numeric fields, typeof value === 'boolean' for booleans) or
Array.isArray where appropriate, then assign with proper typing to
newTools[index][field]; keep using the Tool type and call
onToolsChange(newTools) only after validation to avoid unsafe any casts in
updateTool.
In `@app/components/SimplifiedConfigEditor.tsx`:
- Around line 524-539: In SimplifiedConfigEditor (the style block that sets
backgroundColor, borderColor, color and the onMouseEnter/onMouseLeave handlers
for the element using hasUnsavedChanges), replace all hardcoded hex literals
("#fffbeb", "#fef3c7", "#fafafa", "#ffffff", "#fcd34d", "#b45309", "#e5e5e5",
"#171717") with centralized tokens from the colors export in /app/lib/colors.ts
(you already use colors.bg.primary) so theme values come from a single source;
update the conditional expressions to reference the appropriate colors.*
properties (e.g., colors.bg.warning, colors.bg.hover, colors.border.highlight,
colors.text.warning, colors.border.default, colors.text.primary) and ensure the
onMouseEnter/onMouseLeave handlers set e.currentTarget.style.backgroundColor to
those colors.* values instead of hex strings.
- Line 87: The component hardcodes the localStorage key "kaapi_api_keys" (see
localStorage.getItem("kaapi_api_keys") in SimplifiedConfigEditor.tsx); extract
this into a shared constant (e.g., export const KAAPI_API_KEYS =
"kaapi_api_keys") in a central constants or localStorageKeys utility module,
update SimplifiedConfigEditor.tsx to import and use that constant where
getItem/setItem are used, and update any other modules that reference the same
literal to use the new constant to ensure a single source of truth for the key.
- Around line 467-468: The handleConfigChange function currently types its value
as any; replace this with a field-to-type mapping or a generic overload so
callers retain precise types (string, number, Tool[], etc.). Either (a) define a
ConfigChangeMap matching actual ConfigDrawer fields (omit unused vectorStoreIds
and make provider string rather than a literal) and type handleConfigChange as
(field: keyof ConfigChangeMap, value: ConfigChangeMap[typeof field]), or (b)
implement the suggested generic overload using handlers: const
handleConfigChange = <K extends keyof typeof handlers>(field: K, value:
Parameters<typeof handlers[K]>[0]) => { handlers[field](value); } so
handleConfigChange and referenced handlers preserve exact parameter types;
update references to handleConfigChange accordingly.
In `@app/document/page.tsx`:
- Around line 155-157: Several catch blocks in this file use untyped catch
parameters; update handleUpload, handleDeleteDocument, and handleSelectDocument
to use catch (err: unknown) and then narrow with if (err instanceof Error) to
read err.message (or use a fallback like 'Unknown error') when logging or
toasting, mirroring the fetchDocuments change and ensuring consistent type-safe
error handling across those handlers.
- Around line 53-58: The effect currently suppresses the exhaustive-deps ESLint
rule because fetchDocuments is not stable; wrap fetchDocuments in useCallback
with [apiKey] (or whatever external values it reads) so it is memoized, then
include that memoized fetchDocuments in the useEffect dependency array and
remove the eslint-disable comment; update the function referenced as
fetchDocuments (and any helper it uses) to be declared via React.useCallback and
keep the useEffect dependency list to [apiKey, fetchDocuments] (or just
[fetchDocuments] if apiKey is captured inside the callback).
In `@app/evaluations/`[id]/page.tsx:
- Around line 153-157: The catch block that currently reads catch (err) for the
assistant config fetch should be changed to catch (err: unknown) and then use a
type guard before logging; update the error handling around the console.error
call that references assistantId to first check if err is an Error (e.g., err
instanceof Error) and log err.message (or otherwise stringify unknown errors) so
the console.error call remains safe and consistent with other blocks in this
file.
- Around line 141-142: The effect currently suppresses
react-hooks/exhaustive-deps while using the toast object from useToast inside
the callback; update the dependency handling so it’s explicit: either add toast
to the effect dependency array (the useEffect where apiKeys, selectedKeyId,
jobId, exportFormat are listed) or replace the blanket eslint-disable with a
targeted inline suppression and a short comment explaining why toast is
intentionally stable (e.g., provided by useToast). Locate the useEffect and the
useToast() invocation to make the change and ensure the dependency list
accurately reflects all used variables.
- Around line 428-429: The variable statusColor assigned via
getStatusColor(job.status) is unused; remove this dead code instead of
suppressing the linter. Delete the line that declares statusColor (the const
statusColor = getStatusColor(job.status);) and any accompanying eslint-disable
comment, or if you intend to use it later, leave a TODO comment and actually use
statusColor where the job status is rendered; locate the assignment near the
evaluation page rendering logic referencing job.status to make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 106a48c6-897c-4a6d-8fd4-a8e1a81fdbde
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.github/workflows/lint-build.ymlapp/api/assistant/[assistant_id]/route.tsapp/api/collections/[collection_id]/route.tsapp/api/collections/jobs/[job_id]/route.tsapp/api/collections/route.tsapp/api/configs/[config_id]/route.tsapp/api/configs/[config_id]/versions/[version_number]/route.tsapp/api/configs/[config_id]/versions/route.tsapp/api/credentials/[provider]/route.tsapp/api/credentials/route.tsapp/api/document/[document_id]/route.tsapp/api/document/route.tsapp/api/evaluations/[id]/route.tsapp/api/evaluations/datasets/[dataset_id]/route.tsapp/api/evaluations/datasets/route.tsapp/api/evaluations/stt/datasets/[dataset_id]/route.tsapp/api/evaluations/stt/files/route.tsapp/api/evaluations/stt/results/[result_id]/route.tsapp/api/evaluations/stt/runs/[run_id]/route.tsapp/api/evaluations/stt/samples/[sample_id]/route.tsapp/api/evaluations/tts/datasets/[dataset_id]/route.tsapp/api/evaluations/tts/results/[result_id]/route.tsapp/api/evaluations/tts/runs/[run_id]/route.tsapp/components/ConfigDrawer.tsxapp/components/ConfigModal.tsxapp/components/ConfigSelector.tsxapp/components/DetailedResultsTable.tsxapp/components/Sidebar.tsxapp/components/SimplifiedConfigEditor.tsxapp/components/evaluations/DatasetsTab.tsxapp/components/evaluations/EvaluationsTab.tsxapp/components/prompt-editor/ABTestTab.tsxapp/components/prompt-editor/ConfigDiffPane.tsxapp/components/prompt-editor/ConfigDrawer.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/CurrentConfigTab.tsxapp/components/prompt-editor/HistorySidebar.tsxapp/components/prompt-editor/HistoryTab.tsxapp/components/speech-to-text/ModelComparisonCard.tsxapp/components/speech-to-text/WaveformVisualizer.tsxapp/components/types.tsapp/configurations/page.tsxapp/configurations/prompt-editor/page.tsxapp/configurations/prompt-editor/types.tsapp/datasets/page.tsxapp/document/page.tsxapp/evaluations/[id]/page.tsxapp/evaluations/page.tsxapp/keystore/page.tsxapp/knowledge-base/page.tsxapp/lib/configTypes.tsapp/settings/credentials/page.tsxapp/speech-to-text/page.tsxapp/text-to-speech/page.tsxeslint.config.mjspackage.json
✅ Files skipped from review due to trivial changes (25)
- app/api/evaluations/tts/runs/[run_id]/route.ts
- app/api/evaluations/tts/results/[result_id]/route.ts
- app/api/evaluations/stt/results/[result_id]/route.ts
- app/api/evaluations/stt/files/route.ts
- app/api/configs/[config_id]/route.ts
- app/components/prompt-editor/HistoryTab.tsx
- .github/workflows/lint-build.yml
- app/api/configs/[config_id]/versions/[version_number]/route.ts
- app/api/configs/[config_id]/versions/route.ts
- app/components/speech-to-text/WaveformVisualizer.tsx
- app/components/prompt-editor/HistorySidebar.tsx
- app/components/evaluations/DatasetsTab.tsx
- app/components/ConfigSelector.tsx
- app/configurations/prompt-editor/page.tsx
- app/components/Sidebar.tsx
- app/components/types.ts
- app/api/document/route.ts
- app/components/DetailedResultsTable.tsx
- package.json
- app/configurations/prompt-editor/types.ts
- app/components/prompt-editor/ConfigDiffPane.tsx
- app/keystore/page.tsx
- app/datasets/page.tsx
- app/components/ConfigModal.tsx
- app/api/evaluations/stt/runs/[run_id]/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.mjs
👮 Files not reviewed due to content moderation or server errors (8)
- app/components/evaluations/EvaluationsTab.tsx
- app/evaluations/page.tsx
- app/configurations/page.tsx
- app/settings/credentials/page.tsx
- app/speech-to-text/page.tsx
- app/text-to-speech/page.tsx
- app/api/credentials/[provider]/route.ts
- app/api/evaluations/datasets/route.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/lint-build.yml:
- Around line 16-20: The workflow step mistakenly contains duplicate "uses" keys
and an invalid "cache: 'npm'" input for actions/checkout@v4; split this into two
steps: keep a single checkout step using actions/checkout@v4 (remove the
duplicate uses and the cache input) and add a separate setup step using
actions/setup-node (e.g., actions/setup-node@v3) where you place the
Node.js-specific inputs like cache: 'npm' and node-version; update or remove the
duplicate "uses: actions/checkout@v4" and ensure the new actions/setup-node step
appears after the checkout step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68debf87-df0a-4bbd-995f-2a0f1fef446c
📒 Files selected for processing (1)
.github/workflows/lint-build.yml
Collaborator
Author
|
@Prajna1999 @vprashrex Could you please perform a sanity check of the feature flows on your side to ensure everything is working as expected? |
vprashrex
approved these changes
Mar 24, 2026
Collaborator
|
@Ayush8923 (NitPick) can rename lint.yml to ci.yml .. in future we can add type check or anything in one ci.yml file .. |
Collaborator
Author
I have updated the file name @vprashrex as per your suggestion. Thanks! |
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.
Issue: #84
Summary:
Summary by CodeRabbit
New Features
Documentation
npm test,npm run test:coverage,npm run lint:fix) and pre-commit hooks configuration details.Chores