Open
Conversation
- Default sort by last modified (updated_at) descending - Disable Save button when no changes have been made - Remove knowledge base name display, keep only vector store ID - Add assistantId to search parameters for backend filtering - Fix delete assistant (use deleteAssistantId variable, not id) - Remove 'No evals run' placeholder text - Add copy icon before assistant ID in list page
|
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:
WalkthroughThis PR introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 2 medium |
| BestPractice | 5 medium 2 minor 12 high |
| ErrorProne | 12 high |
| CodeStyle | 8 minor |
| Performance | 2 medium |
🟢 Metrics 406 complexity
Metric Results Complexity 406
TIP This summary will be updated as you push new changes. Give us feedback
|
🚀 Deployed on https://deploy-preview-3881--glific-frontend.netlify.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containers/Assistants/AssistantOptions/AssistantOptions.tsx (1)
483-486:⚠️ Potential issue | 🟡 MinorRemove duplicate
setFieldValuecall.
setFieldValue('knowledgeBaseVersionId', ...)is called twice consecutively on lines 483 and 484. The second call is redundant and should be removed.🔧 Proposed fix
setFieldValue('knowledgeBaseVersionId', knowledgeBaseData.knowledgeBase.knowledgeBaseVersionId); - setFieldValue('knowledgeBaseVersionId', knowledgeBaseData.knowledgeBase.knowledgeBaseVersionId); setTimeout(() => validateForm(), 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/AssistantOptions.tsx` around lines 483 - 486, Remove the duplicate setFieldValue call for 'knowledgeBaseVersionId' — in the block where you call setFieldValue('knowledgeBaseVersionId', knowledgeBaseData.knowledgeBase.knowledgeBaseVersionId) twice, delete the redundant second invocation so only one setFieldValue remains; keep the surrounding calls (the setTimeout(() => validateForm(), 0) and setFieldValue('knowledgeBaseName', ...)) intact to preserve flow.
🧹 Nitpick comments (5)
src/containers/Assistants/AssistantList/AssistantList.module.css (1)
40-48: Consider a larger hit area for the copy action.Line 41 uses very tight padding and a 12px icon; increasing the button target improves click/touch accessibility without changing behavior.
Suggested tweak
.CopyButton { - padding: 2px !important; + min-width: 24px; + min-height: 24px; + padding: 4px; + display: inline-flex; + align-items: center; + justify-content: center; color: `#6b7280`; svg { width: 12px; height: 12px; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantList/AssistantList.module.css` around lines 40 - 48, The .CopyButton styling uses very small padding and a 12px SVG which makes the hit target too small for touch/click; update the .CopyButton rules to increase the interactive area (e.g., raise padding to ~6–8px or add a min-width/min-height) and increase the nested svg size to around 16px so the visual remains proportional while improving accessibility; keep color and other visual styles unchanged and ensure any !important usage is preserved only if necessary.src/containers/Assistants/AssistantDetail/ConfigEditor.tsx (1)
149-150: Consider wrapping JSON.parse in try-catch for defensive error handling.If
version.settingscontains a malformed JSON string,JSON.parsewill throw and crash the component. While this may be unlikely if the backend is consistent, adding defensive error handling would improve resilience.🛡️ Proposed defensive parsing
const rawSettings = version.settings ?? {}; - const settings = typeof rawSettings === 'string' ? JSON.parse(rawSettings) : rawSettings; + let settings = rawSettings; + if (typeof rawSettings === 'string') { + try { + settings = JSON.parse(rawSettings); + } catch { + settings = {}; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantDetail/ConfigEditor.tsx` around lines 149 - 150, The parsing of version.settings is unguarded: when rawSettings (from version.settings) is a string the code calls JSON.parse(rawSettings) which can throw on malformed JSON; wrap the parse in a try-catch inside ConfigEditor.tsx (around the JSON.parse call that assigns settings) and on error fall back to a safe default (e.g., {} or rawSettings) and optionally surface/log the error (use existing logger or console.error) so the component does not crash.src/routes/AuthenticatedRoute/AuthenticatedRoute.tsx (2)
77-87: Naming deviates from coding guidelines.The variable was renamed from
routeStafftostaffRoutes. As per coding guidelines, route trees should userouteStafffor Staff role androuteAdminfor Manager/Admin/Glific_admin roles.Consider keeping the original naming convention for consistency with the documented patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/AuthenticatedRoute/AuthenticatedRoute.tsx` around lines 77 - 87, The variable staffRoutes deviates from the project's naming convention for route trees; rename staffRoutes back to routeStaff and update all references where it's imported/used (e.g., in AuthenticatedRoute component or any JSX/exports) to match the original convention; also verify other route trees follow the pattern (routeAdmin for Admin/Manager/Glific_admin) and update exports/uses accordingly to keep names consistent across the codebase.
89-165: Same naming deviation applies here.
routeAdminwas renamed toadminRoutes. Additionally, the conversion from<Routes>wrapper to a React fragment (<>) is necessary for the conditional route injection pattern used below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/AuthenticatedRoute/AuthenticatedRoute.tsx` around lines 89 - 165, The admin route block was renamed from routeAdmin to adminRoutes and must be consistently used and wrapped as a fragment (<>...</>) to support conditional route injection; update the AuthenticatedRoute component to reference adminRoutes (not routeAdmin) wherever injected and ensure the block declared as const adminRoutes uses a React fragment instead of a <Routes> wrapper so it can be conditionally inserted into the main <Routes> tree (look for symbols adminRoutes and AuthenticatedRoute to locate the changes).src/containers/Assistants/AssistantOptions/AssistantOptions.module.css (1)
85-127: Stylelint:globalwarnings are false positives.The
:globalpseudo-class is valid CSS Modules syntax for escaping local scoping. The Stylelint errors can be suppressed by configuringselector-pseudo-class-no-unknownto allow:global:{ "selector-pseudo-class-no-unknown": [true, { "ignorePseudoClasses": ["global"] }] }However, consider these concerns:
- Fixed dimensions (663px width, 510px min-height) may cause issues on smaller viewports or mobile devices.
- Heavy
!importantusage indicates fighting MUI defaults—consider using MUI'ssxprop or theme overrides for cleaner integration.:has()selector requires modern browser support (Chrome 105+, Safari 15.4+, Firefox 121+). Verify this aligns with your browser support matrix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/AssistantOptions.module.css` around lines 85 - 127, Stylelint is flagging valid CSS Modules :global usage and the stylesheet also uses rigid fixed sizes and many !important overrides; update the lint configuration to allow the pseudo-class (add "selector-pseudo-class-no-unknown": [true, { "ignorePseudoClasses": ["global"] }]) and then refactor the CSS selectors such as :global .kb-dialog-content and :global(.MuiDialog-paper):has(.kb-dialog-content) to be responsive and avoid !important: replace fixed width/min-height (663px / 510px) with responsive constraints (e.g., max-width and width using vw or clamp/min() functions or media queries), remove unnecessary !important by moving overrides into MUI theme overrides or Dialog PaperProps/sx, and either remove or provide a fallback for the :has() usage (or ensure supported browsers per your support matrix) so selectors like :global(.MuiDialog-paper):has(.kb-dialog-content) degrade safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/containers/Assistants/AssistantOptions/AssistantOptions.tsx`:
- Around line 483-486: Remove the duplicate setFieldValue call for
'knowledgeBaseVersionId' — in the block where you call
setFieldValue('knowledgeBaseVersionId',
knowledgeBaseData.knowledgeBase.knowledgeBaseVersionId) twice, delete the
redundant second invocation so only one setFieldValue remains; keep the
surrounding calls (the setTimeout(() => validateForm(), 0) and
setFieldValue('knowledgeBaseName', ...)) intact to preserve flow.
---
Nitpick comments:
In `@src/containers/Assistants/AssistantDetail/ConfigEditor.tsx`:
- Around line 149-150: The parsing of version.settings is unguarded: when
rawSettings (from version.settings) is a string the code calls
JSON.parse(rawSettings) which can throw on malformed JSON; wrap the parse in a
try-catch inside ConfigEditor.tsx (around the JSON.parse call that assigns
settings) and on error fall back to a safe default (e.g., {} or rawSettings) and
optionally surface/log the error (use existing logger or console.error) so the
component does not crash.
In `@src/containers/Assistants/AssistantList/AssistantList.module.css`:
- Around line 40-48: The .CopyButton styling uses very small padding and a 12px
SVG which makes the hit target too small for touch/click; update the .CopyButton
rules to increase the interactive area (e.g., raise padding to ~6–8px or add a
min-width/min-height) and increase the nested svg size to around 16px so the
visual remains proportional while improving accessibility; keep color and other
visual styles unchanged and ensure any !important usage is preserved only if
necessary.
In `@src/containers/Assistants/AssistantOptions/AssistantOptions.module.css`:
- Around line 85-127: Stylelint is flagging valid CSS Modules :global usage and
the stylesheet also uses rigid fixed sizes and many !important overrides; update
the lint configuration to allow the pseudo-class (add
"selector-pseudo-class-no-unknown": [true, { "ignorePseudoClasses": ["global"]
}]) and then refactor the CSS selectors such as :global .kb-dialog-content and
:global(.MuiDialog-paper):has(.kb-dialog-content) to be responsive and avoid
!important: replace fixed width/min-height (663px / 510px) with responsive
constraints (e.g., max-width and width using vw or clamp/min() functions or
media queries), remove unnecessary !important by moving overrides into MUI theme
overrides or Dialog PaperProps/sx, and either remove or provide a fallback for
the :has() usage (or ensure supported browsers per your support matrix) so
selectors like :global(.MuiDialog-paper):has(.kb-dialog-content) degrade safely.
In `@src/routes/AuthenticatedRoute/AuthenticatedRoute.tsx`:
- Around line 77-87: The variable staffRoutes deviates from the project's naming
convention for route trees; rename staffRoutes back to routeStaff and update all
references where it's imported/used (e.g., in AuthenticatedRoute component or
any JSX/exports) to match the original convention; also verify other route trees
follow the pattern (routeAdmin for Admin/Manager/Glific_admin) and update
exports/uses accordingly to keep names consistent across the codebase.
- Around line 89-165: The admin route block was renamed from routeAdmin to
adminRoutes and must be consistently used and wrapped as a fragment (<>...</>)
to support conditional route injection; update the AuthenticatedRoute component
to reference adminRoutes (not routeAdmin) wherever injected and ensure the block
declared as const adminRoutes uses a React fragment instead of a <Routes>
wrapper so it can be conditionally inserted into the main <Routes> tree (look
for symbols adminRoutes and AuthenticatedRoute to locate the changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a51a1c82-1234-4874-b98b-0a9554e800a0
📒 Files selected for processing (20)
src/containers/Assistants/AssistantDetail/AssistantDetail.test.tsxsrc/containers/Assistants/AssistantDetail/AssistantDetail.tsxsrc/containers/Assistants/AssistantDetail/ConfigEditor.test.tsxsrc/containers/Assistants/AssistantDetail/ConfigEditor.tsxsrc/containers/Assistants/AssistantList/AssistantList.module.csssrc/containers/Assistants/AssistantList/AssistantList.test.tsxsrc/containers/Assistants/AssistantList/AssistantList.tsxsrc/containers/Assistants/AssistantOptions/AssistantOptions.module.csssrc/containers/Assistants/AssistantOptions/AssistantOptions.test.tsxsrc/containers/Assistants/AssistantOptions/AssistantOptions.tsxsrc/containers/Assistants/Assistants.test.tsxsrc/containers/Assistants/Assistants.tsxsrc/graphql/queries/Organization.tssrc/i18n/en/en.jsonsrc/i18n/hi/hi.jsonsrc/mocks/Assistants.tssrc/mocks/Organization.tsxsrc/routes/AuthenticatedRoute/AuthenticatedRoute.test.tsxsrc/routes/AuthenticatedRoute/AuthenticatedRoute.tsxsrc/services/AuthService.tsx
Glific
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
issue/4963
|
| Run status |
|
| Run duration | 12m 52s |
| Commit |
|
| Committer | Mohd Shamoon |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
97
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/filesearch/Filesearch.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| File search > should create a new assistant with file upload |
Test Replay
Screenshots
|
|
cypress/e2e/chat/ChatCollection.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| ChatCollection > should send the emoji to collection |
Test Replay
Screenshots
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3881 +/- ##
==========================================
- Coverage 82.18% 82.17% -0.02%
==========================================
Files 311 311
Lines 13146 13148 +2
Branches 3038 3037 -1
==========================================
Hits 10804 10804
- Misses 1396 1399 +3
+ Partials 946 945 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
priyanshu6238
approved these changes
Apr 9, 2026
| {...queries} | ||
| {...columnAttributes} | ||
| searchParameter={['name']} | ||
| searchParameter={['name', 'assistantId']} |
Collaborator
There was a problem hiding this comment.
lets combine this values
…ant UI - Copy AssistantOptions into KnowledgeBaseOptions with new dialog design (Manage Knowledge Base title, Proceed button, two-column layout) - Revert AssistantOptions to pre-PR state for legacy CreateAssistant - ConfigEditor now uses KnowledgeBaseOptions - Show knowledgeBaseName as fallback when vectorStoreId not yet available - Show 'Knowledge Base upload in progress...' while awaiting refetch - Remove sort-by-name from AssistantList (sort by last updated only) - Fix filterAssistantsMock variables to match actual sort config - Fix Assistants test assertion (vectorStore ID replaces removed name display) - Use name_or_assistant_id as single search parameter
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx (3)
97-100: Useconstwith a conditional expression instead ofletreassignment.The
fileUploadDisabledvariable is reassigned immediately after declaration. This can be simplified to a single expression.Proposed fix
- let fileUploadDisabled = false; - if (isLegacyVectorStore || disabled) { - fileUploadDisabled = true; - } + const fileUploadDisabled = isLegacyVectorStore || disabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around lines 97 - 100, Replace the mutable let with an immutable const by computing fileUploadDisabled in one expression: change the current two-step assignment that declares fileUploadDisabled then reassigns it based on isLegacyVectorStore or disabled to a single const fileUploadDisabled = isLegacyVectorStore || disabled; so references to fileUploadDisabled in KnowledgeBaseOptions.tsx remain unchanged but the variable is no longer reassigned.
389-392: Parametererrorshadows component state variable.The
errorparameter in the catch block shadows theerrorstate variable declared at line 87. While this works correctly here, renaming for clarity would improve readability.Proposed fix
- .catch((error) => { - if (isAbortError(error)) return; - console.error('Error uploading files:', error); + .catch((uploadError) => { + if (isAbortError(uploadError)) return; + console.error('Error uploading files:', uploadError); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around lines 389 - 392, The catch block in the uploadFiles promise uses a parameter named error which shadows the component's error state (declared as error in KnowledgeBaseOptions). Rename the catch parameter (e.g., to uploadError or err) in the .catch(...) callback and update the console.error call to use the new name, ensuring isAbortError(uploadError) (or isAbortError(err)) check and the log reference are updated so the component state variable is no longer shadowed.
22-33: Props interface usesanytypes, reducing type safety.The
formikValues,formikErrors, andformikTouchedprops are typed asany, which bypasses TypeScript's type checking. Consider defining proper types based on the form's shape fromConfigEditor.tsx.Proposed type definitions
+interface AssistantFormValues { + name: string; + model: any; + instructions: string; + temperature: number; + knowledgeBaseVersionId: string; + knowledgeBaseName: string; + versionDescription: string; + initialFiles: UploadedFile[]; +} + interface KnowledgeBaseOptionsProps { - formikValues: any; + formikValues: AssistantFormValues; setFieldValue: (field: string, value: any) => void; - formikErrors: any; - formikTouched: any; + formikErrors: Partial<Record<keyof AssistantFormValues, string>>; + formikTouched: Partial<Record<keyof AssistantFormValues, boolean>>; knowledgeBaseId: string | null; isLegacyVectorStore: boolean; onFilesChange: (hasChanges: boolean) => void; vectorStoreId: string; validateForm: () => void; disabled: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around lines 22 - 33, The props interface KnowledgeBaseOptionsProps currently uses loose any types for formikValues, formikErrors, and formikTouched; replace these with precise types derived from the form shape defined in ConfigEditor.tsx (or import Formik's typed helpers) so TypeScript can enforce correctness. Update KnowledgeBaseOptionsProps to reference a concrete FormValues interface (e.g., ConfigFormValues) for formikValues and use FormikErrors<ConfigFormValues> and FormikTouched<ConfigFormValues> for formikErrors and formikTouched; also align setFieldValue's signature with Formik's SetFieldValue type if applicable. Ensure you import or declare the ConfigFormValues type from ConfigEditor.tsx (or a shared types file) and update all usages in KnowledgeBaseOptions component to match the new typed props.src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.module.css (1)
85-126: Consider reducing!importantoverrides and leveraging MUI theme.The
:globalselectors are valid CSS Modules syntax (static analysis warnings are false positives). However, the extensive use of!importantto override MUI dialog styles is heavy-handed. Consider using MUI'ssxprop orslotPropson theDialogBoxcomponent to apply these styles, which would integrate better with the theme system and avoid specificity wars.Additionally, hardcoded color values (e.g.,
#cccccc,#191c1a,#119656) could be replaced with theme variables fromsrc/config/theme.tsxfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.module.css` around lines 85 - 126, The CSS file overuses !important on selectors like :global(.MuiDialog-paper):has(.kb-dialog-content), :global .kb-dialog-content, and :global(.MuiDialog-paper) :global(.MuiDialogTitle-root); replace these heavy overrides by moving styling into the Dialog component via MUI props (use sx or slotProps on the Dialog/DialogPaper/DialogTitle/DialogActions) to set width, padding, border, radius, boxShadow and alignment without !important, and swap hardcoded colors (`#cccccc`, `#191c1a`, `#119656`) for theme tokens from src/config/theme.tsx so styles inherit the app theme and avoid specificity wars.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx`:
- Around line 710-731: The number input's disabled prop is inconsistent with the
Slider: update the input (role="sliderDisplay") so its disabled logic matches
the Slider by using disabled={disabled} (remove the && !isLegacyVectorStore
condition), ensuring both Slider and input are disabled together; locate the
input using formikValues.temperature, setFieldValue, and isLegacyVectorStore
symbols to change only the disabled prop.
- Around line 483-484: Remove the duplicate call to setFieldValue by keeping a
single invocation of setFieldValue('knowledgeBaseVersionId',
knowledgeBaseData.knowledgeBase.knowledgeBaseVersionId) in the AssistantOptions
component; delete the redundant repeated line so the field is only set once
(verify there are no other intended different fields to set in place of the
duplicate).
- Around line 712-714: The input element using role="sliderDisplay"
(name="sliderDisplay") is using an invalid ARIA role; remove that invalid role
and replace it with a test identifier (data-testid="sliderDisplay") and either
omit the role entirely or use a valid ARIA role such as "spinbutton" if you need
an explicit role for accessibility — update the <input name="sliderDisplay"> in
the KnowledgeBaseOptions component accordingly so tests can target data-testid
and accessibility uses a valid role.
- Around line 720-728: The onChange handler for the temperature input uses
parseFloat and only checks value < 0 || value > 2, which allows NaN to pass and
be set via setFieldValue; update the handler (the onChange that calls setError
and setFieldValue for 'temperature') to explicitly detect NaN (e.g.,
Number.isNaN(value) or isNaN(valueString) after parsing), treat it as invalid
(setError(true) and return), and only call setFieldValue('temperature', value)
when value is a valid number within [0,2]; ensure setError(false) is only
reached after both the NaN and range checks pass.
- Around line 102-108: Add a cleanup effect that aborts in-flight uploads and
prevents state updates after unmount: create a ref (e.g.,
activeUploadControllersRef) to store AbortController instances for each upload,
ensure the upload starter code (the functions that initiate uploads referenced
by isUploadDialogOpenRef and any upload handlers) registers its controller in
activeUploadControllersRef and passes controller.signal to fetch/axios, and add
a useEffect with a return cleanup that calls abort() on all controllers and
clears the ref (and optionally sets isUploadDialogOpenRef.current = false); also
guard state-updating callbacks (e.g., setFiles) to no-op if the request was
aborted (check controller.signal.aborted) so resolved promises won’t update
state after unmount.
---
Nitpick comments:
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.module.css`:
- Around line 85-126: The CSS file overuses !important on selectors like
:global(.MuiDialog-paper):has(.kb-dialog-content), :global .kb-dialog-content,
and :global(.MuiDialog-paper) :global(.MuiDialogTitle-root); replace these heavy
overrides by moving styling into the Dialog component via MUI props (use sx or
slotProps on the Dialog/DialogPaper/DialogTitle/DialogActions) to set width,
padding, border, radius, boxShadow and alignment without !important, and swap
hardcoded colors (`#cccccc`, `#191c1a`, `#119656`) for theme tokens from
src/config/theme.tsx so styles inherit the app theme and avoid specificity wars.
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx`:
- Around line 97-100: Replace the mutable let with an immutable const by
computing fileUploadDisabled in one expression: change the current two-step
assignment that declares fileUploadDisabled then reassigns it based on
isLegacyVectorStore or disabled to a single const fileUploadDisabled =
isLegacyVectorStore || disabled; so references to fileUploadDisabled in
KnowledgeBaseOptions.tsx remain unchanged but the variable is no longer
reassigned.
- Around line 389-392: The catch block in the uploadFiles promise uses a
parameter named error which shadows the component's error state (declared as
error in KnowledgeBaseOptions). Rename the catch parameter (e.g., to uploadError
or err) in the .catch(...) callback and update the console.error call to use the
new name, ensuring isAbortError(uploadError) (or isAbortError(err)) check and
the log reference are updated so the component state variable is no longer
shadowed.
- Around line 22-33: The props interface KnowledgeBaseOptionsProps currently
uses loose any types for formikValues, formikErrors, and formikTouched; replace
these with precise types derived from the form shape defined in ConfigEditor.tsx
(or import Formik's typed helpers) so TypeScript can enforce correctness. Update
KnowledgeBaseOptionsProps to reference a concrete FormValues interface (e.g.,
ConfigFormValues) for formikValues and use FormikErrors<ConfigFormValues> and
FormikTouched<ConfigFormValues> for formikErrors and formikTouched; also align
setFieldValue's signature with Formik's SetFieldValue type if applicable. Ensure
you import or declare the ConfigFormValues type from ConfigEditor.tsx (or a
shared types file) and update all usages in KnowledgeBaseOptions component to
match the new typed props.
🪄 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: f6bd1b7f-be46-4ab3-bd0b-ce5b36d27ff8
📒 Files selected for processing (4)
src/containers/Assistants/AssistantDetail/ConfigEditor.tsxsrc/containers/Assistants/AssistantList/AssistantList.tsxsrc/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.module.csssrc/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/Assistants/AssistantDetail/ConfigEditor.tsx
- src/containers/Assistants/AssistantList/AssistantList.tsx
Comment on lines
+102
to
+108
| useEffect(() => { | ||
| setFiles(formikValues.initialFiles.map(mapInitialFileToAssistantFile)); | ||
| }, [formikValues.initialFiles]); | ||
|
|
||
| useEffect(() => { | ||
| isUploadDialogOpenRef.current = showUploadDialog; | ||
| }, [showUploadDialog]); |
There was a problem hiding this comment.
Missing cleanup effect to abort uploads on component unmount.
If the component unmounts while uploads are in progress (e.g., user navigates away), the upload promises will still resolve and attempt to update state, causing React warnings about state updates on unmounted components. Add a cleanup effect to cancel active requests.
Proposed fix
useEffect(() => {
setFiles(formikValues.initialFiles.map(mapInitialFileToAssistantFile));
}, [formikValues.initialFiles]);
useEffect(() => {
isUploadDialogOpenRef.current = showUploadDialog;
}, [showUploadDialog]);
+ useEffect(() => {
+ return () => {
+ cancelActiveRequests();
+ pendingUploadQueueRef.current = [];
+ };
+ }, []);📝 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
| useEffect(() => { | |
| setFiles(formikValues.initialFiles.map(mapInitialFileToAssistantFile)); | |
| }, [formikValues.initialFiles]); | |
| useEffect(() => { | |
| isUploadDialogOpenRef.current = showUploadDialog; | |
| }, [showUploadDialog]); | |
| useEffect(() => { | |
| setFiles(formikValues.initialFiles.map(mapInitialFileToAssistantFile)); | |
| }, [formikValues.initialFiles]); | |
| useEffect(() => { | |
| isUploadDialogOpenRef.current = showUploadDialog; | |
| }, [showUploadDialog]); | |
| useEffect(() => { | |
| return () => { | |
| cancelActiveRequests(); | |
| pendingUploadQueueRef.current = []; | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around
lines 102 - 108, Add a cleanup effect that aborts in-flight uploads and prevents
state updates after unmount: create a ref (e.g., activeUploadControllersRef) to
store AbortController instances for each upload, ensure the upload starter code
(the functions that initiate uploads referenced by isUploadDialogOpenRef and any
upload handlers) registers its controller in activeUploadControllersRef and
passes controller.signal to fetch/axios, and add a useEffect with a return
cleanup that calls abort() on all controllers and clears the ref (and optionally
sets isUploadDialogOpenRef.current = false); also guard state-updating callbacks
(e.g., setFiles) to no-op if the request was aborted (check
controller.signal.aborted) so resolved promises won’t update state after
unmount.
src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+710
to
+731
| disabled={disabled} | ||
| /> | ||
| <input | ||
| role="sliderDisplay" | ||
| name="sliderDisplay" | ||
| type="number" | ||
| step={0.1} | ||
| min={0} | ||
| max={2} | ||
| value={formikValues.temperature} | ||
| onChange={(event) => { | ||
| const value = parseFloat(event.target.value); | ||
| if (value < 0 || value > 2) { | ||
| setError(true); | ||
| return; | ||
| } | ||
| setError(false); | ||
| setFieldValue('temperature', value); | ||
| }} | ||
| className={`${styles.SliderDisplay} ${error ? styles.Error : ''}`} | ||
| disabled={disabled && !isLegacyVectorStore} | ||
| /> |
There was a problem hiding this comment.
Inconsistent disabled logic between Slider and input.
The Slider is disabled when disabled is true (line 710), but the number input is disabled only when disabled && !isLegacyVectorStore (line 730). This means when viewing a legacy vector store assistant, users can edit the temperature via the input but not the slider, which seems unintentional.
Proposed fix for consistent behavior
<input
role="sliderDisplay"
name="sliderDisplay"
type="number"
step={0.1}
min={0}
max={2}
value={formikValues.temperature}
onChange={(event) => {
const value = parseFloat(event.target.value);
if (value < 0 || value > 2) {
setError(true);
return;
}
setError(false);
setFieldValue('temperature', value);
}}
className={`${styles.SliderDisplay} ${error ? styles.Error : ''}`}
- disabled={disabled && !isLegacyVectorStore}
+ disabled={disabled}
/>📝 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
| disabled={disabled} | |
| /> | |
| <input | |
| role="sliderDisplay" | |
| name="sliderDisplay" | |
| type="number" | |
| step={0.1} | |
| min={0} | |
| max={2} | |
| value={formikValues.temperature} | |
| onChange={(event) => { | |
| const value = parseFloat(event.target.value); | |
| if (value < 0 || value > 2) { | |
| setError(true); | |
| return; | |
| } | |
| setError(false); | |
| setFieldValue('temperature', value); | |
| }} | |
| className={`${styles.SliderDisplay} ${error ? styles.Error : ''}`} | |
| disabled={disabled && !isLegacyVectorStore} | |
| /> | |
| disabled={disabled} | |
| /> | |
| <input | |
| role="sliderDisplay" | |
| name="sliderDisplay" | |
| type="number" | |
| step={0.1} | |
| min={0} | |
| max={2} | |
| value={formikValues.temperature} | |
| onChange={(event) => { | |
| const value = parseFloat(event.target.value); | |
| if (value < 0 || value > 2) { | |
| setError(true); | |
| return; | |
| } | |
| setError(false); | |
| setFieldValue('temperature', value); | |
| }} | |
| className={`${styles.SliderDisplay} ${error ? styles.Error : ''}`} | |
| disabled={disabled} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around
lines 710 - 731, The number input's disabled prop is inconsistent with the
Slider: update the input (role="sliderDisplay") so its disabled logic matches
the Slider by using disabled={disabled} (remove the && !isLegacyVectorStore
condition), ensuring both Slider and input are disabled together; locate the
input using formikValues.temperature, setFieldValue, and isLegacyVectorStore
symbols to change only the disabled prop.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+712
to
+714
| <input | ||
| role="sliderDisplay" | ||
| name="sliderDisplay" |
There was a problem hiding this comment.
Invalid ARIA role sliderDisplay.
The role="sliderDisplay" attribute is not a valid ARIA role. This appears to be used as a test identifier—use data-testid instead. Valid roles for this context would be spinbutton (default for type="number" inputs) or no explicit role.
Proposed fix
<input
- role="sliderDisplay"
+ data-testid="sliderDisplay"
name="sliderDisplay"
type="number"📝 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 | |
| role="sliderDisplay" | |
| name="sliderDisplay" | |
| <input | |
| data-testid="sliderDisplay" | |
| name="sliderDisplay" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around
lines 712 - 714, The input element using role="sliderDisplay"
(name="sliderDisplay") is using an invalid ARIA role; remove that invalid role
and replace it with a test identifier (data-testid="sliderDisplay") and either
omit the role entirely or use a valid ARIA role such as "spinbutton" if you need
an explicit role for accessibility — update the <input name="sliderDisplay"> in
the KnowledgeBaseOptions component accordingly so tests can target data-testid
and accessibility uses a valid role.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+720
to
+728
| onChange={(event) => { | ||
| const value = parseFloat(event.target.value); | ||
| if (value < 0 || value > 2) { | ||
| setError(true); | ||
| return; | ||
| } | ||
| setError(false); | ||
| setFieldValue('temperature', value); | ||
| }} |
There was a problem hiding this comment.
Missing NaN validation for temperature input.
parseFloat returns NaN for non-numeric input (e.g., empty string or letters). Since NaN < 0 and NaN > 2 are both false, the validation passes and NaN gets set as the temperature value.
Proposed fix
onChange={(event) => {
const value = parseFloat(event.target.value);
- if (value < 0 || value > 2) {
+ if (Number.isNaN(value) || value < 0 || value > 2) {
setError(true);
return;
}
setError(false);
setFieldValue('temperature', value);
}}📝 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
| onChange={(event) => { | |
| const value = parseFloat(event.target.value); | |
| if (value < 0 || value > 2) { | |
| setError(true); | |
| return; | |
| } | |
| setError(false); | |
| setFieldValue('temperature', value); | |
| }} | |
| onChange={(event) => { | |
| const value = parseFloat(event.target.value); | |
| if (Number.isNaN(value) || value < 0 || value > 2) { | |
| setError(true); | |
| return; | |
| } | |
| setError(false); | |
| setFieldValue('temperature', value); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/containers/Assistants/AssistantOptions/KnowledgeBaseOptions.tsx` around
lines 720 - 728, The onChange handler for the temperature input uses parseFloat
and only checks value < 0 || value > 2, which allows NaN to pass and be set via
setFieldValue; update the handler (the onChange that calls setError and
setFieldValue for 'temperature') to explicitly detect NaN (e.g.,
Number.isNaN(value) or isNaN(valueString) after parsing), treat it as invalid
(setError(true) and return), and only call setFieldValue('temperature', value)
when value is a valid number within [0,2]; ensure setError(false) is only
reached after both the NaN and range checks pass.
Old CreateAssistant continues to use the original dialog (Manage Files, Save button, drag-drop upload). New AssistantDetail uses KnowledgeBaseOptions with the new design. Reverts dialog title assertions in Assistants.test.tsx.
priyanshu6238
requested changes
Apr 10, 2026
rvignesh89
reviewed
Apr 10, 2026
| @@ -0,0 +1,739 @@ | |||
| import { useMutation } from '@apollo/client'; | |||
Contributor
There was a problem hiding this comment.
We are not writing any tests for this page?
rvignesh89
approved these changes
Apr 10, 2026
priyanshu6238
approved these changes
Apr 10, 2026
Collaborator
priyanshu6238
left a comment
priyanshu6238
left a comment
There was a problem hiding this comment.
LGTM other then few minor issue
priyanshu6238
requested changes
Apr 10, 2026
Collaborator
priyanshu6238
left a comment
priyanshu6238
left a comment
There was a problem hiding this comment.
UI test case is failing ,can you fix it
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
3 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.
Assistant list UX fixes: default sort by updated_at desc, disable save when no changes, remove KB name, fix delete mutation variable, remove no-evals text, add copy icon for assistant ID. Note: assistantId search requires backend AssistantFilter update.
Summary by CodeRabbit
New Features
Improvements
Tests