Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TheodoreSpeaks wants to merge 2 commits into
base: staging
Choose a base branch
from
+17 −67
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f530893
improvement(knowledge): make KB modals toast-only for errors
TheodoreSpeaks Jul 2, 2026
815278f
improvement(knowledge): drop field highlighting for uniform toast-onl…
TheodoreSpeaks Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 10 additions & 35 deletions .../app/workspace/[workspaceId]/knowledge/components/create-base-modal/create-base-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
ChipInput,
ChipModal,
ChipModalBody,
ChipModalError,
ChipModalField,
ChipModalFooter,
ChipModalHeader,
Expand Down Expand Up @@ -119,11 +118,6 @@ const FormSchema = z
type FormInputValues = z.input<typeof FormSchema>
type FormValues = z.output<typeof FormSchema>

interface SubmitStatus {
type: 'success' | 'error'
message: string
}

export const CreateBaseModal = memo(function CreateBaseModal({
open,
onOpenChange,
Expand All @@ -134,11 +128,10 @@ export const CreateBaseModal = memo(function CreateBaseModal({
const createKnowledgeBaseMutation = useCreateKnowledgeBase(workspaceId)
const deleteKnowledgeBaseMutation = useDeleteKnowledgeBase(workspaceId)

const [submitStatus, setSubmitStatus] = useState<SubmitStatus | null>(null)
const [files, setFiles] = useState<File[]>([])
const [fileError, setFileError] = useState<string | null>(null)

const { uploadFiles, isUploading, uploadProgress, uploadError, clearError } = useKnowledgeUpload({
const { uploadFiles, isUploading, uploadProgress, clearError } = useKnowledgeUpload({
workspaceId,
})

Expand All @@ -149,14 +142,11 @@ export const CreateBaseModal = memo(function CreateBaseModal({
onOpenChange(open)
}

const {
register,
handleSubmit,
reset,
watch,
setValue,
formState: { errors },
} = useForm<FormInputValues, unknown, FormValues>({
const { register, handleSubmit, reset, watch, setValue } = useForm<
FormInputValues,
unknown,
FormValues
>({
resolver: zodResolver(FormSchema),
defaultValues: {
name: '',
Expand All @@ -178,7 +168,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({

useEffect(() => {
if (open) {
setSubmitStatus(null)
setFileError(null)
setFiles([])
reset({
Expand Down Expand Up @@ -236,13 +225,11 @@ export const CreateBaseModal = memo(function CreateBaseModal({
(fieldError) => typeof fieldError?.message === 'string'
)?.message
toast.error(
typeof firstMessage === 'string' ? firstMessage : 'Please fix the highlighted fields'
typeof firstMessage === 'string' ? firstMessage : 'Please fix the errors and try again'
)
}

const onSubmit = async (data: FormValues) => {
setSubmitStatus(null)

try {
const strategyOptions: StrategyOptions | undefined =
data.strategy === 'regex' && data.regexPattern
Expand Down Expand Up @@ -289,7 +276,8 @@ export const CreateBaseModal = memo(function CreateBaseModal({
} catch (deleteError) {
logger.error('Failed to delete orphaned knowledge base:', deleteError)
}
throw uploadError
toast.error(getErrorMessage(uploadError, 'Failed to upload files'))
return
}
}

Expand All @@ -298,10 +286,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
handleClose(false)
} catch (error) {
logger.error('Error creating knowledge base:', error)
setSubmitStatus({
type: 'error',
message: getErrorMessage(error, 'An unknown error occurred'),
})
}
}

Expand All @@ -325,7 +309,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
<ChipInput
placeholder='Enter knowledge base name'
{...register('name')}
error={Boolean(errors.name)}
autoComplete='off'
autoCorrect='off'
autoCapitalize='off'
Expand All @@ -334,12 +317,11 @@ export const CreateBaseModal = memo(function CreateBaseModal({
/>
</ChipModalField>

<ChipModalField type='custom' title='Description' error={errors.description?.message}>
<ChipModalField type='custom' title='Description'>
<ChipTextarea
placeholder='Describe this knowledge base (optional)'
rows={4}
{...register('description')}
error={Boolean(errors.description)}
/>
</ChipModalField>

Expand All @@ -352,7 +334,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
step={1}
placeholder='100'
{...register('minChunkSize', { valueAsNumber: true })}
error={Boolean(errors.minChunkSize)}
autoComplete='off'
data-form-type='other'
/>
Expand All @@ -366,7 +347,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
step={1}
placeholder='1024'
{...register('maxChunkSize', { valueAsNumber: true })}
error={Boolean(errors.maxChunkSize)}
autoComplete='off'
data-form-type='other'
/>
Expand All @@ -385,7 +365,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
step={1}
placeholder='200'
{...register('overlapSize', { valueAsNumber: true })}
error={Boolean(errors.overlapSize)}
autoComplete='off'
data-form-type='other'
/>
Expand All @@ -410,13 +389,11 @@ export const CreateBaseModal = memo(function CreateBaseModal({
<ChipModalField
type='custom'
title='Regex Pattern'
error={errors.regexPattern?.message}
hint='Text will be split at each match of this regex pattern.'
>
<ChipInput
placeholder='e.g. \\n\\n or (?<=\\})\\s*(?=\\{)'
{...register('regexPattern')}
error={Boolean(errors.regexPattern)}
autoComplete='off'
data-form-type='other'
/>
Expand Down Expand Up @@ -520,8 +497,6 @@ export const CreateBaseModal = memo(function CreateBaseModal({
</div>
</ChipModalField>
)}

<ChipModalError>{uploadError?.message || submitStatus?.message}</ChipModalError>
</ChipModalBody>

<ChipModalFooter
Expand Down
36 changes: 4 additions & 32 deletions ...workspaceId]/knowledge/components/edit-knowledge-base-modal/edit-knowledge-base-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@ import { memo, useRef, useState } from 'react'
import {
ChipModal,
ChipModalBody,
ChipModalError,
ChipModalField,
ChipModalFooter,
ChipModalHeader,
toast,
} from '@sim/emcn'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import { KNOWLEDGE_BASE_DESCRIPTION_MAX_LENGTH } from '@/lib/knowledge/constants'
import type { ChunkingConfig } from '@/lib/knowledge/types'

Expand Down Expand Up @@ -41,10 +39,7 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
}: EditKnowledgeBaseModalProps) {
const [name, setName] = useState(initialName)
const [description, setDescription] = useState(initialDescription)
const [nameError, setNameError] = useState<string | null>(null)
const [descriptionError, setDescriptionError] = useState<string | null>(null)
const [isSubmitting, setIsSubmitting] = useState(false)
const [error, setError] = useState<string | null>(null)

/**
* Seed the fields only on the closed → open transition (render-phase reset),
Expand All @@ -56,34 +51,16 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
if (open) {
setName(initialName)
setDescription(initialDescription)
setNameError(null)
setDescriptionError(null)
setError(null)
}
}

const validate = (): string | null => {
let firstError: string | null = null

if (!name.trim()) {
setNameError('Name is required')
firstError ??= 'Name is required'
} else if (name.trim().length > 100) {
setNameError('Name must be less than 100 characters')
firstError ??= 'Name must be less than 100 characters'
} else {
setNameError(null)
}

if (!name.trim()) return 'Name is required'
if (name.trim().length > 100) return 'Name must be less than 100 characters'
if (description.length > KNOWLEDGE_BASE_DESCRIPTION_MAX_LENGTH) {
const message = `Description must be ${KNOWLEDGE_BASE_DESCRIPTION_MAX_LENGTH} characters or less`
setDescriptionError(message)
firstError ??= message
} else {
setDescriptionError(null)
return `Description must be ${KNOWLEDGE_BASE_DESCRIPTION_MAX_LENGTH} characters or less`
}

return firstError
return null
}

const handleSubmit = async () => {
Expand All @@ -94,14 +71,12 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
}

setIsSubmitting(true)
setError(null)

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

@greptile-apps greptile-apps Bot Jul 2, 2026

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!

@TheodoreSpeaks TheodoreSpeaks Jul 2, 2026

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.tsx → handleUpdateKnowledgeBase) 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.

Expand All @@ -121,7 +96,6 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
onChange={setName}
placeholder='Enter knowledge base name'
required
error={nameError ?? undefined}
autoComplete='off'
/>
<ChipModalField
Expand All @@ -131,7 +105,6 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
onChange={setDescription}
placeholder='Describe this knowledge base (optional)'
rows={4}
error={descriptionError ?? undefined}
/>
{chunkingConfig && (
<ChipModalField type='custom' title='Chunking Configuration'>
Expand Down Expand Up @@ -166,7 +139,6 @@ export const EditKnowledgeBaseModal = memo(function EditKnowledgeBaseModal({
</div>
</ChipModalField>
)}
<ChipModalError>{error}</ChipModalError>
</ChipModalBody>
<ChipModalFooter
onCancel={() => onOpenChange(false)}
Expand Down
3 changes: 3 additions & 0 deletions apps/sim/hooks/queries/kb/knowledge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,9 @@ export function useCreateKnowledgeBase(workspaceId?: string) {

return useMutation({
mutationFn: createKnowledgeBase,
onError: (error) => {
toast.error(error.message, { duration: 5000 })
},
onSettled: () => {
queryClient.invalidateQueries({
queryKey: knowledgeKeys.lists(),
Expand Down
Loading