fix(skills): show the new skill after creating it

The create page navigated to the new skill's detail route, but the
unsaved-changes guard immediately undid it: `isDirty` was derived from
`createSkill.isSuccess`, so on success the guard's effect fired
`history.back()` to pop its sentinel entry and cancelled the navigation
that had just run. The header stayed on "New skill" with the fields
intact, and nothing confirmed the save.

The guard now exposes `release()` to retire itself for the rest of the
mount, and the create page calls it before navigating with `replace` so
the sentinel entry is consumed rather than stacked.

Also from a cleanup pass over the same surfaces:
- `useCreateSkill` resolves the created row, so the page reads `created.id`
  instead of re-deriving it from the response list
- seed the list cache with the upsert's authoritative list verbatim; the
  id-merge kept the previous ordering and appended the new skill last
- treat placeholder data as loading in the detail page, which could
  otherwise flash "Skill not found." on a workspace switch
- seed credential-detail drafts on id change rather than in a value-keyed
  effect, so a background refetch can't clobber an in-progress edit
- toast on save and on delete failure, which were both silent

Author: waleedlatif1

apps/sim/app/workspace/[workspaceId]/components/credential-detail/hooks/use-credential-detail-form.ts

@@ -1,6 +1,6 @@
 'use client'
 
-import { useCallback, useEffect, useState } from 'react'
+import { useCallback, useState } from 'react'
 import { toast } from '@sim/emcn'
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
@@ -30,11 +30,16 @@ export function useCredentialDetailForm({
 
   const [displayNameDraft, setDisplayNameDraft] = useState('')
   const [descriptionDraft, setDescriptionDraft] = useState('')
+  const [prevCredentialId, setPrevCredentialId] = useState<string | null>(null)
 
-  useEffect(() => {
-    setDisplayNameDraft(credential?.displayName ?? '')
-    setDescriptionDraft(credential?.description ?? '')
-  }, [credential?.id, credential?.displayName, credential?.description])
+  // Seed drafts when the credential first resolves (or the route id changes); a
+  // background refetch of the same credential must not clobber an in-progress
+  // edit — nor silently reset isDirty, which pops the guard's history sentinel.
+  if (credential && credential.id !== prevCredentialId) {
+    setPrevCredentialId(credential.id)
+    setDisplayNameDraft(credential.displayName ?? '')
+    setDescriptionDraft(credential.description ?? '')
+  }
 
   const isDisplayNameDirty = credential ? displayNameDraft !== credential.displayName : false
   const isDescriptionDirty = credential

apps/sim/app/workspace/[workspaceId]/components/credential-detail/hooks/use-unsaved-changes-guard.ts

@@ -26,9 +26,13 @@ interface UseUnsavedChangesGuardParams {
 export function useUnsavedChangesGuard({ isDirty, backHref }: UseUnsavedChangesGuardParams) {
   const router = useRouter()
   const [showUnsavedAlert, setShowUnsavedAlert] = useState(false)
+  const [isReleased, setIsReleased] = useState(false)
   const hasSentinelRef = useRef(false)
 
   useEffect(() => {
+    // Released: the caller is navigating away, so leave the seeded entry alone —
+    // popping it would cancel that navigation.
+    if (isReleased) return
     if (!isDirty) {
       // Clean again while still mounted (saved/reverted): pop the seeded entry so
       // it can't pile up across edit/save cycles. This runs in the effect body,
@@ -58,7 +62,7 @@ export function useUnsavedChangesGuard({ isDirty, backHref }: UseUnsavedChangesG
       window.removeEventListener('beforeunload', handleBeforeUnload)
       window.removeEventListener('popstate', handlePopState)
     }
-  }, [isDirty])
+  }, [isDirty, isReleased])
 
   const handleBackClick = useCallback(
     (event: MouseEvent<HTMLAnchorElement>) => {
@@ -75,5 +79,13 @@ export function useUnsavedChangesGuard({ isDirty, backHref }: UseUnsavedChangesG
     router.push(backHref)
   }, [router, backHref])
 
-  return { showUnsavedAlert, setShowUnsavedAlert, handleBackClick, confirmDiscard }
+  /**
+   * Retires the guard for the rest of this mount: no unload warning, no Back
+   * trap, and no pop of the seeded entry when the form goes clean. Call it
+   * immediately before navigating away on a successful save, and navigate with
+   * `router.replace` so the seeded entry is the one consumed.
+   */
+  const release = useCallback(() => setIsReleased(true), [])
+
+  return { showUnsavedAlert, setShowUnsavedAlert, handleBackClick, confirmDiscard, release }
 }

apps/sim/app/workspace/[workspaceId]/skills/[skillId]/skill-detail.tsx

@@ -44,7 +44,11 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
   const router = useRouter()
   const skillsHref = `/workspace/${workspaceId}/skills`
 
-  const { data: skills = [], isPending: skillsLoading } = useSkills(workspaceId)
+  const { data: skills = [], isPending, isPlaceholderData } = useSkills(workspaceId)
+  // `keepPreviousData` can serve another workspace's list while this one loads
+  // (the key is workspace-scoped), which reads as success with no match — treat
+  // that as loading so the detail never flashes "Skill not found."
+  const skillsLoading = isPending || isPlaceholderData
   const updateSkill = useUpdateSkill()
   const deleteSkill = useDeleteSkill()
   const skill = skills.find((s) => s.id === skillId) ?? null
@@ -112,6 +116,7 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
         },
       })
       setErrors({})
+      toast.success(`Saved "${nameDraft}"`)
     } catch (error) {
       if (isSkillNameConflictError(error)) {
         setErrors({ name: getErrorMessage(error, 'This skill name is already taken.') })
@@ -129,8 +134,14 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
     setShowDeleteConfirm(false)
     try {
       await deleteSkill.mutateAsync({ workspaceId, skillId: skill.id })
-      router.push(skillsHref)
+      // The skill is gone from the list cache, so this surface is about to have no
+      // entity to guard — retire it before navigating, as the create page does.
+      guard.release()
+      router.replace(skillsHref)
     } catch (error) {
+      toast.error("Couldn't delete skill", {
+        description: getErrorMessage(error, 'Please try again in a moment.'),
+      })
       logger.error('Failed to delete skill', error)
     }
   }

apps/sim/app/workspace/[workspaceId]/skills/new/skill-create.tsx

@@ -50,11 +50,7 @@ export function SkillCreate({ workspaceId }: SkillCreateProps) {
   const [contentSeed, setContentSeed] = useState(0)
   const [errors, setErrors] = useState<SkillFieldErrors>({})
 
-  // Drops on success so the guard pops its history sentinel before we navigate —
-  // otherwise Back from the new skill lands on a stale, empty create form.
-  const isDirty =
-    !createSkill.isSuccess &&
-    (!!nameDraft.trim() || !!descriptionDraft.trim() || !!contentDraft.trim())
+  const isDirty = !!nameDraft.trim() || !!descriptionDraft.trim() || !!contentDraft.trim()
 
   const guard = useUnsavedChangesGuard({ isDirty, backHref: skillsHref })
 
@@ -72,16 +68,17 @@ export function SkillCreate({ workspaceId }: SkillCreateProps) {
     }
 
     try {
-      const created = await createSkill.mutateAsync({
+      const { created } = await createSkill.mutateAsync({
         workspaceId,
         skill: { name: nameDraft, description: descriptionDraft, content: contentDraft },
       })
       setErrors({})
-      // The upsert responds with the caller's whole skill list (built-ins
-      // included), not just the new row — match by name, which is unique per
-      // workspace, rather than trusting the first element.
-      const createdId = created.find((skill) => skill.name === nameDraft)?.id
-      router.push(createdId ? `${skillsHref}/${createdId}` : skillsHref)
+      toast.success(`Created "${nameDraft}"`)
+      // Retire the guard before navigating — otherwise the form going clean pops
+      // its seeded history entry and lands us back on this create form. `replace`
+      // consumes that entry instead of stacking another.
+      guard.release()
+      router.replace(created ? `${skillsHref}/${created.id}` : skillsHref)
     } catch (error) {
       if (isSkillNameConflictError(error)) {
         setErrors({ name: getErrorMessage(error, 'This skill name is already taken.') })

apps/sim/hooks/queries/skills.ts

@@ -26,7 +26,8 @@ export const skillsKeys = {
   all: ['skills'] as const,
   lists: () => [...skillsKeys.all, 'list'] as const,
   list: (workspaceId: string) => [...skillsKeys.lists(), workspaceId] as const,
-  members: (skillId?: string) => [...skillsKeys.all, 'members', skillId ?? ''] as const,
+  memberLists: () => [...skillsKeys.all, 'members'] as const,
+  members: (skillId?: string) => [...skillsKeys.memberLists(), skillId ?? ''] as const,
 }
 
 /**
@@ -88,17 +89,18 @@ export function useCreateSkill() {
       })
 
       logger.info(`Created skill: ${s.name}`)
-      return data
+      // The upsert responds with the caller's whole skill list (built-ins
+      // included), not just the new row. Resolve the created row here — by name,
+      // which is unique per workspace and which a same-named built-in is filtered
+      // out of — so consumers get an id instead of re-deriving one.
+      return { skills: data, created: data.find((skill) => skill.name === s.name) ?? null }
     },
-    onSuccess: (data, variables) => {
-      queryClient.setQueryData<SkillDefinition[]>(
-        skillsKeys.list(variables.workspaceId),
-        (prev) => {
-          const byId = new Map((prev ?? []).map((skill) => [skill.id, skill]))
-          for (const skill of data) byId.set(skill.id, skill)
-          return Array.from(byId.values())
-        }
-      )
+    onSuccess: ({ skills }, variables) => {
+      // The response is the same authoritative list GET /api/skills returns for
+      // this caller, so seed the cache with it verbatim. Merging by id would keep
+      // the previous ordering and append the new skill at the bottom until the
+      // refetch lands.
+      queryClient.setQueryData<SkillDefinition[]>(skillsKeys.list(variables.workspaceId), skills)
     },
     onSettled: (_data, _error, variables) => {
       queryClient.invalidateQueries({ queryKey: skillsKeys.list(variables.workspaceId) })
@@ -169,8 +171,9 @@ export function useUpdateSkill() {
       }
     },
     onSettled: (_data, _error, variables) => {
+      // Only name/description/content go over the wire here, none of which can
+      // change the editor roster — no need to invalidate it.
       queryClient.invalidateQueries({ queryKey: skillsKeys.list(variables.workspaceId) })
-      queryClient.invalidateQueries({ queryKey: skillsKeys.members(variables.skillId) })
     },
   })
 }