Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(skills): show the new skill after creating it#5949
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
e538a562228383ee807566aad77eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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), []) | ||
waleedlatif1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return { | ||
| showUnsavedAlert, | ||
| setShowUnsavedAlert, | ||
| handleBackClick, | ||
| confirmDiscard, | ||
| release, | ||
| rearm, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
waleedlatif1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } catch (error) { | ||
| if (isSkillNameConflictError(error)) { | ||
| setErrors({ name: getErrorMessage(error, 'This skill name is already taken.') }) | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.