Skip to content

improvement(knowledge): make KB modals toast-only for errors#5349

Open
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
kb-modal-toast-errors
Open

improvement(knowledge): make KB modals toast-only for errors#5349
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
kb-modal-toast-errors

Conversation

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator

Summary

  • Follow-up to fix(knowledge): surface KB description validation errors and raise limit to 10k #5347. Standardize both KB modals on a single error-surfacing mechanism: toast.
  • Create modal: validation errors already toast via onInvalid; removed the inline description/regexPattern field messages and the submitStatus state + <ChipModalError> block. Upload failures now toast in the catch.
  • Add onError: toast.error to useCreateKnowledgeBase (matching useUpdateKnowledgeBase) so server errors surface from the hook — the create modal no longer hand-rolls server-error handling.
  • Edit modal: dropped the redundant inline nameError/descriptionError/error state and <ChipModalError> (server errors already toast via the update hook's onError); validation now toasts.

Type of Change

  • Improvement

Testing

Tested manually. bun run lint, bun run check:api-validation:strict, and tsc --noEmit all pass.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

🤖 Generated with Claude Code

@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 2, 2026 2:12am

Request Review

@cursor

cursor Bot commented Jul 2, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
UI-only error presentation changes in knowledge modals; no auth, data model, or API contract changes.

Overview
Knowledge base create and edit modals now surface failures only through toasts, not inline modal error UI.

The create flow drops submitStatus, <ChipModalError>, per-field error props from react-hook-form, and the upload hook’s inline uploadError display. Validation still toasts via onInvalid (generic fallback copy tweaked). After a failed upload (with orphan KB cleanup), the modal toasts and stays open instead of rethrowing into a submit error banner. Create API failures are handled in useCreateKnowledgeBase with onError: toast.error, aligned with update.

The edit modal removes inline name/description/server error state and <ChipModalError>; validation failures toast, and save errors continue to toast from the update mutation hook.

Reviewed by Cursor Bugbot for commit 815278f. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR standardizes error surfacing in both Knowledge Base modals to use toasts exclusively, removing the inline ChipModalError blocks and per-field error state that previously duplicated the same messages.

  • Create modal: Dropped SubmitStatus state and ChipModalError; upload failures now toast directly and return early; server errors surface via a new onError handler added to useCreateKnowledgeBase, matching the pattern already present in useUpdateKnowledgeBase.
  • Edit modal: Removed nameError, descriptionError, and error state along with ChipModalError; validate() is simplified to return a string directly; validation failures toast on submit; server-error handling now relies entirely on useUpdateKnowledgeBase's onError.
  • knowledge.ts: One-line addition of onError: (error) => toast.error(error.message, { duration: 5000 }) to useCreateKnowledgeBase.

Confidence Score: 4/5

Safe 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 onSave throws, relying entirely on useUpdateKnowledgeBase's onError to surface the toast — a coupling that works today but is not enforced by the component's interface. If onSave is ever backed by something other than that specific hook, save failures will produce no user-visible feedback.

edit-knowledge-base-modal.tsx — the catch block in handleSubmit no longer shows any feedback if onSave throws outside the mutation's onError path.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx Removed SubmitStatus state and ChipModalError block; upload errors now toast inline and return early rather than re-throwing to the outer catch; outer catch silently logs (toast comes from hook's onError). Field-level boolean error highlighting retained; only the inline error text on Description and Regex Pattern fields was removed.
apps/sim/app/workspace/[workspaceId]/knowledge/components/edit-knowledge-base-modal/edit-knowledge-base-modal.tsx Removed nameError, descriptionError, error state and ChipModalError; validation now toasts directly; server-error catch only logs, relying on the parent hook's onError to surface the message. Implicit coupling introduced: if onSave is ever not backed by useUpdateKnowledgeBase, errors are silently swallowed.
apps/sim/hooks/queries/kb/knowledge.ts Added onError toast to useCreateKnowledgeBase, matching the pattern already used in useUpdateKnowledgeBase. Change is minimal and correct.

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
Loading
%%{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
Loading

Comments Outside Diff (1)

  1. apps/sim/app/workspace/[workspaceId]/knowledge/components/edit-knowledge-base-modal/edit-knowledge-base-modal.tsx, line 92-108 (link)

    P2 Name and description fields no longer show error styling on failed save

    The error props were removed from both ChipModalField elements. Users get a toast when validation fails, but the offending field is no longer highlighted — there's no visual pointer to what needs fixing. The create modal still passes error={Boolean(errors.xxx)} to its inner inputs, so the two modals are now inconsistent in how they handle field-level feedback.

    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!

Reviews (1): Last reviewed commit: "improvement(knowledge): make KB modals t..." | Re-trigger Greptile

Comment on lines 75 to 82
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)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.onError already toasted before this PR, and it's the shared surface for both callers of this modal (knowledge.tsxhandleUpdateKnowledgeBase) and the inline rename in [id]/base.tsx (kbRename → same hook). Adding a toast.error in this catch would double-toast every current caller.
  • This PR aligned useCreateKnowledgeBase to 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.

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

Addressed the field-highlighting inconsistency (P2) in 815278f — instead of adding highlighting to the edit modal (its ChipModalField exposes a ReactNode error, so it can only show the inline message text we intentionally removed), I dropped the create modal's boolean error={Boolean(errors.x)} highlighting so both modals are now uniformly toast-only. Both surface the error message via toast; neither adds inline field chrome.

Re: the catch-block note (P1) — replied inline. Short version: server-error toasts are owned by the mutation hook's onError (the shared surface for the edit modal and the [id]/base.tsx inline rename), so a catch-level toast would double-fire. By design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant