improvement(knowledge): make KB modals toast-only for errors#5349
improvement(knowledge): make KB modals toast-only for errors#5349TheodoreSpeaks wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview The create flow drops The edit modal removes inline name/description/server error state and Reviewed by Cursor Bugbot for commit 815278f. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR standardizes error surfacing in both Knowledge Base modals to use toasts exclusively, removing the inline
Confidence Score: 4/5Safe to merge after addressing the implicit error-handling contract in the edit modal's catch block. The create modal and hook changes are straightforward and correct. The edit modal's catch block now silently drops errors when edit-knowledge-base-modal.tsx — the catch block in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User submits form] --> B{Validation passes?}
B -- No --> C[onInvalid / toast.error]
B -- Yes --> D[createKnowledgeBaseMutation.mutateAsync]
D -- Error --> E[useCreateKnowledgeBase onError\ntoast.error message]
E --> F[Error re-throws to outer catch\nlogger.error only]
D -- Success --> G{Files selected?}
G -- No --> K[handleClose / success]
G -- Yes --> H[uploadFiles]
H -- Error --> I[Delete orphaned KB\nthen toast.error upload error\nreturn — modal stays open]
H -- Success --> K
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User submits form] --> B{Validation passes?}
B -- No --> C[onInvalid / toast.error]
B -- Yes --> D[createKnowledgeBaseMutation.mutateAsync]
D -- Error --> E[useCreateKnowledgeBase onError\ntoast.error message]
E --> F[Error re-throws to outer catch\nlogger.error only]
D -- Success --> G{Files selected?}
G -- No --> K[handleClose / success]
G -- Yes --> H[uploadFiles]
H -- Error --> I[Delete orphaned KB\nthen toast.error upload error\nreturn — modal stays open]
H -- Success --> K
|
| try { | ||
| await onSave(knowledgeBaseId, name.trim(), description.trim()) | ||
| onOpenChange(false) | ||
| } catch (err) { | ||
| logger.error('Error updating knowledge base:', err) | ||
| setError(getErrorMessage(err, 'Failed to update knowledge base')) | ||
| } finally { | ||
| setIsSubmitting(false) | ||
| } |
There was a problem hiding this comment.
Catch block silently swallows errors if
onSave doesn't self-toast
The catch here now only logs — it relies on onSave being backed by useUpdateKnowledgeBase, whose onError fires the toast before the error re-throws. That coupling is implicit: onSave is typed as a generic (id, name, description) => Promise<void>, so any future caller that passes a plain async function (no mutation onError) will silently swallow the error with zero user feedback. The removed getErrorMessage import was the fallback that made this component self-sufficient regardless of caller. Adding a defensive toast.error(getErrorMessage(err, 'Failed to update knowledge base')) in the catch is low-cost insurance and keeps the modal correct in isolation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Intentional — this is the established pattern in this codebase, not a new coupling introduced here. Server-error toasts are owned by the mutation hook's onError, not the component:
useUpdateKnowledgeBase.onErroralready toasted before this PR, and it's the shared surface for both callers of this modal (knowledge.tsx→handleUpdateKnowledgeBase) and the inline rename in[id]/base.tsx(kbRename→ same hook). Adding atoast.errorin this catch would double-toast every current caller.- This PR aligned
useCreateKnowledgeBaseto the same pattern, so both KB mutation hooks now own their error toasts.
So the catch intentionally only logs — the toast has already fired from the hook. The hypothetical "future caller passes a non-toasting onSave" would be introducing a caller that violates the hook-owns-errors convention; that caller would add its own handling. Leaving as-is.
|
Addressed the field-highlighting inconsistency (P2) in 815278f — instead of adding highlighting to the edit modal (its Re: the catch-block note (P1) — replied inline. Short version: server-error toasts are owned by the mutation hook's |
Summary
onInvalid; removed the inline description/regexPatternfield messages and thesubmitStatusstate +<ChipModalError>block. Upload failures now toast in the catch.onError: toast.errortouseCreateKnowledgeBase(matchinguseUpdateKnowledgeBase) so server errors surface from the hook — the create modal no longer hand-rolls server-error handling.nameError/descriptionError/errorstate and<ChipModalError>(server errors already toast via the update hook'sonError); validation now toasts.Type of Change
Testing
Tested manually.
bun run lint,bun run check:api-validation:strict, andtsc --noEmitall pass.Checklist
🤖 Generated with Claude Code