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

* 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

* improvement(skills): align the custom tools row and harden the guard lifecycle

Follow-ups from an audit of the skills and custom tools surfaces.

Custom tools row, now matching the skills row exactly:
- the trailing arrow could be squeezed by a long description; the shared row
  owns `flex-shrink-0` for its trailing slot, so the five callers that
  hand-rolled it (and the two that forgot) all get it
- drop a redundant `cursor-pointer` — Tailwind v3's preflight already sets it
  on `button`
- the tile chrome was defined twice, in ResourceTile and again in
  SettingsResourceRow, despite ResourceTile's doc claiming to be the single
  source. Both now share RESOURCE_TILE_BASE/RESOURCE_TILE_FILL, and
  ResourceTile's redundant wrapper div is gone
- skills' row uses the text-sm/text-caption tokens instead of literal pixels;
  the added gap-[1px] offsets the 21px->20px line-height so the row stays 39px

Guard and cache correctness:
- the delete path released the guard after the await, but the delete is
  optimistic — the row leaves the cache first, so the form went clean
  mid-flight and popped the sentinel before the release landed. Release up
  front and rearm if the request fails
- hold the loading frame across the whole optimistic delete instead of
  flashing "Skill not found." on the way out
- custom tools had the same placeholder-data bug just fixed in skill detail,
  and worse: a deep-linked id could resolve against the workspace just left
- keep cached rows the create response omits, so a concurrent create isn't
  dropped until the refetch lands

* fix(skills): retire the in-app back guard on release too

release() suppressed the unload warning and the browser Back trap, but the
in-app back link still keyed only on isDirty — so the Skills chip could open
the unsaved-changes modal after a successful create, while the drafts were
still populated and the navigation was already in flight.

* fix(skills): track a sentinel consumed while the guard is released

release() intentionally leaves the seeded history entry in place, but it also
drops the popstate listener — so Back during the released window (an optimistic
delete's round-trip) consumed that entry with nothing to record it. hasSentinelRef
stayed true, and a failed delete's rearm() then skipped re-seeding, leaving the
surface with no Back confirm despite unsaved edits.

The released branch now keeps a bookkeeping-only popstate listener that clears
the ref, so rearm() seeds a fresh entry when the old one is gone.

Author: waleedlatif1

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

@@ -26,9 +26,22 @@ 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(() => {
+    // The caller is navigating away — popping the seeded entry would cancel it. But
+    // Back during that window consumes the entry with no listener left to re-push
+    // it, so track that: a later rearm() must seed a fresh one rather than trust a
+    // stale ref and leave the surface unguarded.
+    if (isReleased) {
+      if (!hasSentinelRef.current) return
+      const handleSentinelConsumed = () => {
+        hasSentinelRef.current = false
+      }
+      window.addEventListener('popstate', handleSentinelConsumed)
+      return () => window.removeEventListener('popstate', handleSentinelConsumed)
+    }
     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,22 +71,42 @@ export function useUnsavedChangesGuard({ isDirty, backHref }: UseUnsavedChangesG
       window.removeEventListener('beforeunload', handleBeforeUnload)
       window.removeEventListener('popstate', handlePopState)
     }
-  }, [isDirty])
+  }, [isDirty, isReleased])
 
   const handleBackClick = useCallback(
     (event: MouseEvent<HTMLAnchorElement>) => {
-      if (isDirty) {
+      if (isDirty && !isReleased) {
         event.preventDefault()
         setShowUnsavedAlert(true)
       }
     },
-    [isDirty]
+    [isDirty, isReleased]
   )
 
   const confirmDiscard = useCallback(() => {
     setShowUnsavedAlert(false)
     router.push(backHref)
   }, [router, backHref])
 
-  return { showUnsavedAlert, setShowUnsavedAlert, handleBackClick, confirmDiscard }
+  /**
+   * Retires the guard: no unload warning, no Back trap (browser or the in-app back
+   * link), and no pop of the seeded entry when the form goes clean. Call it before
+   * navigating away on a successful save, and navigate with `router.replace` so the
+   * seeded entry is the one consumed. An operation that goes clean before it
+   * resolves (an optimistic delete) must release up front and {@link rearm} if it
+   * fails.
+   */
+  const release = useCallback(() => setIsReleased(true), [])
+
+  /** Restores guarding after a released operation failed and the surface stays. */
+  const rearm = useCallback(() => setIsReleased(false), [])
+
+  return {
+    showUnsavedAlert,
+    setShowUnsavedAlert,
+    handleBackClick,
+    confirmDiscard,
+    release,
+    rearm,
+  }
 }

apps/sim/app/workspace/[workspaceId]/components/resource-tile/index.ts

@@ -1 +1,5 @@
-export { ResourceTile } from '@/app/workspace/[workspaceId]/components/resource-tile/resource-tile'
+export {
+  RESOURCE_TILE_BASE,
+  RESOURCE_TILE_FILL,
+  ResourceTile,
+} from '@/app/workspace/[workspaceId]/components/resource-tile/resource-tile'

apps/sim/app/workspace/[workspaceId]/components/resource-tile/resource-tile.tsx

@@ -1,20 +1,30 @@
 import type { ComponentType } from 'react'
+import { cn } from '@sim/emcn'
 
 interface ResourceTileProps {
   icon: ComponentType<{ className?: string }>
 }
 
+/**
+ * Geometry and border of the square resource tile — the single source for that
+ * chrome, shared by {@link ResourceTile} and `SettingsResourceRow` so the skills,
+ * custom tools, and settings surfaces cannot drift apart. Pair with a fill. Sizing
+ * the glyph is the tile's job: the descendant rule outranks an icon's own class.
+ */
+export const RESOURCE_TILE_BASE =
+  'flex size-9 flex-shrink-0 items-center justify-center overflow-hidden rounded-xl border border-[var(--border-1)] [&_svg]:size-5'
+
+/** Filled treatment worn by the skills and custom tools resource tiles. */
+export const RESOURCE_TILE_FILL = 'bg-[var(--surface-4)] dark:bg-[var(--surface-5)]'
+
 /**
  * Square glyph tile identifying a workspace resource — the leading visual on a
- * resource's row and on its detail heading. Single source for that chrome so
- * the skills and custom tools surfaces cannot drift apart.
+ * resource's row and on its detail heading.
  */
 export function ResourceTile({ icon: Icon }: ResourceTileProps) {
   return (
-    <div className='size-9 flex-shrink-0'>
-      <div className='flex size-full items-center justify-center rounded-xl border border-[var(--border-1)] bg-[var(--surface-4)] dark:bg-[var(--surface-5)]'>
-        <Icon className='size-5 text-[var(--text-icon)]' />
-      </div>
+    <div className={cn(RESOURCE_TILE_BASE, RESOURCE_TILE_FILL)}>
+      <Icon className='text-[var(--text-icon)]' />
     </div>
   )
 }

apps/sim/app/workspace/[workspaceId]/settings/components/byok/byok-key-manager.tsx

@@ -270,7 +270,7 @@ export function BYOKKeyManager(props: BYOKKeyManagerProps) {
     if (props.multiKey) {
       const keyCount = getProviderKeys(provider.id).length
       return (
-        <div className='flex flex-shrink-0 items-center gap-2'>
+        <div className='flex items-center gap-2'>
           <span className='text-[var(--text-muted)] text-caption'>
             {keyCount} {keyCount === 1 ? 'key' : 'keys'}
           </span>
@@ -283,7 +283,7 @@ export function BYOKKeyManager(props: BYOKKeyManagerProps) {
 
     if (readOnly) return null
     return (
-      <div className='flex flex-shrink-0 items-center gap-2'>
+      <div className='flex items-center gap-2'>
         <Chip onClick={() => openEditModal(provider.id)}>Update</Chip>
         <Chip onClick={() => openDeleteConfirm(provider.id)}>Delete</Chip>
       </div>

apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/custom-tools.tsx

@@ -26,7 +26,10 @@ export function CustomTools() {
   const workspacePermissions = useUserPermissionsContext()
   const canEdit = canMutateWorkspaceSettingsSection('custom-tools', workspacePermissions)
 
-  const { data: tools = [], isLoading, error } = useCustomTools(workspaceId)
+  const { data: tools = [], isPending, isPlaceholderData, error } = useCustomTools(workspaceId)
+  // Placeholder data is another workspace's tools reading as success — treat it as
+  // loading, or a deep-linked id resolves against the workspace the user just left.
+  const isLoading = isPending || isPlaceholderData
 
   const [searchTerm, setSearchTerm] = useSettingsSearch()
   const [selectedToolId, setSelectedToolId] = useQueryState(customToolIdParam.key, {
@@ -119,7 +122,7 @@ export function CustomTools() {
               key={tool.id}
               type='button'
               onClick={() => void setSelectedToolId(tool.id)}
-              className='w-full cursor-pointer rounded-lg p-2 text-left transition-colors hover-hover:bg-[var(--surface-active)]'
+              className='w-full rounded-lg p-2 text-left transition-colors hover-hover:bg-[var(--surface-active)]'
             >
               <SettingsResourceRow
                 icon={<Wrench className='text-[var(--text-icon)]' />}

apps/sim/app/workspace/[workspaceId]/settings/components/recently-deleted/recently-deleted.tsx

@@ -464,22 +464,18 @@ export function RecentlyDeleted() {
                 }
                 trailing={
                   !canRestore ? null : isRestoring ? (
-                    <Chip variant='primary' disabled className='shrink-0'>
+                    <Chip variant='primary' disabled>
                       Restoring...
                     </Chip>
                   ) : isRestored ? (
-                    <div className='flex shrink-0 items-center gap-2'>
+                    <div className='flex items-center gap-2'>
                       <span className='text-[var(--text-muted)] text-small'>Restored</span>
                       <Chip variant='primary' onClick={() => handleView(resource)}>
                         View
                       </Chip>
                     </div>
                   ) : (
-                    <Chip
-                      variant='primary'
-                      onClick={() => void handleRestore(resource)}
-                      className='shrink-0'
-                    >
+                    <Chip variant='primary' onClick={() => void handleRestore(resource)}>
                       Restore
                     </Chip>
                   )

apps/sim/app/workspace/[workspaceId]/settings/components/settings-resource-row/settings-resource-row.tsx

@@ -1,5 +1,9 @@
 import type { ReactNode } from 'react'
 import { cn } from '@sim/emcn'
+import {
+  RESOURCE_TILE_BASE,
+  RESOURCE_TILE_FILL,
+} from '@/app/workspace/[workspaceId]/components/resource-tile'
 
 /**
  * The canonical settings "resource row": a rounded-bordered icon tile, a
@@ -29,13 +33,13 @@ interface SettingsResourceRowProps {
   title: ReactNode
   /** Secondary muted line — truncates. */
   description?: ReactNode
-  /** Trailing element pinned to the row's end (chips, actions menu, status). */
+  /**
+   * Trailing element pinned to the row's end (chips, actions menu, status). The row
+   * keeps it at its natural size — callers never need their own `flex-shrink-0`.
+   */
   trailing?: ReactNode
 }
 
-const TILE_BASE =
-  'flex size-9 flex-shrink-0 items-center justify-center overflow-hidden rounded-xl border border-[var(--border-1)] [&_svg]:size-5'
-
 export function SettingsResourceRow({
   icon,
   iconFill = false,
@@ -49,8 +53,8 @@ export function SettingsResourceRow({
       <div className='flex min-w-0 items-center gap-2.5'>
         <div
           className={cn(
-            TILE_BASE,
-            iconFilled ? 'bg-[var(--surface-4)] dark:bg-[var(--surface-5)]' : 'bg-[var(--bg)]',
+            RESOURCE_TILE_BASE,
+            iconFilled ? RESOURCE_TILE_FILL : 'bg-[var(--bg)]',
             iconFill ? '[&_img]:size-full' : '[&_img]:size-5'
           )}
         >
@@ -63,7 +67,7 @@ export function SettingsResourceRow({
           )}
         </div>
       </div>
-      {trailing}
+      {trailing ? <div className='flex flex-shrink-0 items-center'>{trailing}</div> : null}
     </div>
   )
 }

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` carries the old cache entry across a workspace-id change,
+  // so another workspace's list can read 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.') })
@@ -127,10 +132,17 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
   const handleConfirmDelete = async () => {
     if (!skill) return
     setShowDeleteConfirm(false)
+    // Optimistic: the skill leaves the list cache before the request resolves, so
+    // this surface goes clean mid-flight — release up front, rearm if it fails.
+    guard.release()
     try {
       await deleteSkill.mutateAsync({ workspaceId, skillId: skill.id })
-      router.push(skillsHref)
+      router.replace(skillsHref)
     } catch (error) {
+      guard.rearm()
+      toast.error("Couldn't delete skill", {
+        description: getErrorMessage(error, 'Please try again in a moment.'),
+      })
       logger.error('Failed to delete skill', error)
     }
   }
@@ -172,7 +184,10 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
       </>
     ) : null
 
-  if (skillsLoading && !skill) {
+  // A delete is optimistic and settles before `router.replace` commits, so the row
+  // is gone for a few frames while this surface is still mounted — hold the loading
+  // frame through both phases instead of flashing "Skill not found." on the way out.
+  if ((skillsLoading || deleteSkill.isPending || deleteSkill.isSuccess) && !skill) {
     return (
       <CredentialDetailLayout back={back} actions={actions}>
         <p className='py-12 text-center text-[var(--text-muted)] text-sm'>Loading…</p>

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,16 @@ 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}"`)
+      // Detach the guard so its Back trap can't fire mid-navigation; `replace` then
+      // consumes the seeded entry rather than 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/app/workspace/[workspaceId]/skills/skills.tsx

@@ -34,10 +34,10 @@ function SkillItem({ name, description, onClick }: SkillItemProps) {
       className='flex items-center gap-2.5 rounded-lg p-2 text-left transition-colors hover-hover:bg-[var(--surface-active)]'
     >
       <SkillTile />
-      <div className='flex min-w-0 flex-1 flex-col'>
-        <span className='truncate text-[14px] text-[var(--text-body)]'>{name}</span>
+      <div className='flex min-w-0 flex-1 flex-col justify-center gap-[1px]'>
+        <span className='truncate text-[var(--text-body)] text-sm'>{name}</span>
         {description && (
-          <span className='truncate text-[12px] text-[var(--text-muted)]'>{description}</span>
+          <span className='truncate text-[var(--text-muted)] text-caption'>{description}</span>
         )}
       </div>
       <ArrowRight className='size-4 flex-shrink-0 text-[var(--text-icon)]' />