refactor(skills): one definition of the skill fields across every surface (#5916)

* refactor(skills): one definition of the skill fields across every surface

The Name / Description / Content trio was implemented three times — once in the
canvas modal and once each in the create and detail pages — and had already
drifted: the name hint appeared on create, was missing entirely on detail, and
sat in a different slot in the modal; the content editor was 260px on the pages
and 200px in the modal; only the modal marked the fields required.

Extract SkillFields, the DetailSection form both full-page surfaces render, and
move the shared copy into skill-copy.ts. The modal keeps ChipModalField — that
is required inside a ChipModalBody, so it cannot share the pages' JSX — but now
reads the same placeholder, hint, and max-length constants, so the wording can
no longer diverge.

The detail page's local FieldLockTooltip moves into SkillFields and is now
available to both pages, and the dynamic rich-editor import drops from three
copies to two.

No behavioral change beyond the drift being resolved: the name hint now shows on
the detail page as it already did on create.

* refactor(skills): derive modal saving state, collapse the field message line

From the cleanup passes over this branch:

- SkillModal mirrored `mutation.isPending` into a local `saving` useState, which
  .claude/rules/sim-hooks.md forbids. It also carried a latent bug: the
  `finally { setSaving(false) }` ran after `onSave()` had closed the modal, so it
  set state on an unmounting component, and a throw from `onSave()` would leave
  the modal stuck in a saving state. Derived from the two mutations instead.
- The hint/error line under a field was written three times in SkillFields, in
  three slightly different shapes. One FieldMessage helper now renders all three.
- The name hint is an authoring instruction, so it no longer shows under a field
  that cannot be edited (a built-in skill, or a viewer who is not an editor).
This commit is contained in:
Waleed
2026-07-23 20:14:37 -07:00
committed by GitHub
parent 6dcc65be89
commit bee45621ec
6 changed files with 258 additions and 224 deletions
@@ -1,33 +1,24 @@
'use client'
import { type ReactNode, useState } from 'react'
import {
Chip,
ChipConfirmModal,
ChipInput,
ChipLink,
ChipTextarea,
chipFieldSurfaceClass,
cn,
Send,
Tooltip,
toast,
} from '@sim/emcn'
import { useState } from 'react'
import { Chip, ChipConfirmModal, ChipLink, Send, toast } from '@sim/emcn'
import { ArrowLeft } from '@sim/emcn/icons'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import dynamic from 'next/dynamic'
import { useRouter } from 'next/navigation'
import { AddPeopleModal } from '@/components/permissions'
import { SkillTile } from '@/app/workspace/[workspaceId]/components'
import {
CredentialDetailHeading,
CredentialDetailLayout,
DetailSection,
UnsavedChangesModal,
useUnsavedChangesGuard,
} from '@/app/workspace/[workspaceId]/components/credential-detail'
import { SkillEditorsCard } from '@/app/workspace/[workspaceId]/skills/[skillId]/components/skill-editors-card'
import {
type SkillFieldErrors,
SkillFields,
} from '@/app/workspace/[workspaceId]/skills/components/skill-fields'
import { useSkillEditorsController } from '@/app/workspace/[workspaceId]/skills/components/skill-members'
import {
isSkillNameConflictError,
@@ -36,47 +27,8 @@ import {
} from '@/app/workspace/[workspaceId]/skills/components/utils'
import { useDeleteSkill, useSkills, useUpdateSkill } from '@/hooks/queries/skills'
const RichMarkdownField = dynamic(
() =>
import(
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-field'
).then((m) => m.RichMarkdownField),
{
ssr: false,
loading: () => <div className={cn('min-h-[260px]', chipFieldSurfaceClass)} />,
}
)
const logger = createLogger('SkillDetail')
interface FieldErrors {
name?: string
description?: string
content?: string
}
interface FieldLockTooltipProps {
reason: string | null
children: ReactNode
}
/**
* Wraps a read-only field so hovering it explains why editing is locked.
* Renders children unchanged when the field is editable. The wrapper div
* receives the hover events a disabled control swallows.
*/
function FieldLockTooltip({ reason, children }: FieldLockTooltipProps) {
if (!reason) return <>{children}</>
return (
<Tooltip.Root>
<Tooltip.Trigger asChild>
<div>{children}</div>
</Tooltip.Trigger>
<Tooltip.Content>{reason}</Tooltip.Content>
</Tooltip.Root>
)
}
interface SkillDetailProps {
workspaceId: string
skillId: string
@@ -110,7 +62,7 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
const [contentDraft, setContentDraft] = useState('')
/** Bumped to remount the seed-once rich Content editor on programmatic sets. */
const [contentSeed, setContentSeed] = useState(0)
const [errors, setErrors] = useState<FieldErrors>({})
const [errors, setErrors] = useState<SkillFieldErrors>({})
const [shareOpen, setShareOpen] = useState(false)
const [showDeleteConfirm, setShowDeleteConfirm] = useState(false)
const [prevSkillId, setPrevSkillId] = useState<string | null>(null)
@@ -138,7 +90,7 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
const handleSave = async () => {
if (!skill || !canEdit || !isDirty || updateSkill.isPending) return
const newErrors: FieldErrors = {}
const newErrors: SkillFieldErrors = {}
const nameError = validateSkillName(nameDraft)
if (nameError) newErrors.name = nameError
if (!descriptionDraft.trim()) newErrors.description = 'Description is required'
@@ -252,70 +204,29 @@ export function SkillDetail({ workspaceId, skillId }: SkillDetailProps) {
subtitle={isBuiltin ? 'Built-in skill' : skill.description}
/>
<DetailSection title='Name'>
<FieldLockTooltip reason={lockReason}>
<ChipInput
id='skill-name'
value={nameDraft}
onChange={(event) => {
setNameDraft(event.target.value)
if (errors.name) setErrors((prev) => ({ ...prev, name: undefined }))
}}
placeholder='my-skill-name'
autoComplete='off'
data-lpignore='true'
disabled={readOnly}
error={!!errors.name}
/>
</FieldLockTooltip>
{errors.name && (
<p className='mt-[9px] text-[var(--text-error)] text-caption'>{errors.name}</p>
)}
</DetailSection>
<DetailSection title='Description'>
<FieldLockTooltip reason={lockReason}>
<ChipTextarea
id='skill-description'
rows={3}
value={descriptionDraft}
onChange={(event) => {
setDescriptionDraft(event.target.value)
if (errors.description) setErrors((prev) => ({ ...prev, description: undefined }))
}}
placeholder='What this skill does and when to use it...'
maxLength={1024}
autoComplete='off'
data-lpignore='true'
disabled={readOnly}
/>
</FieldLockTooltip>
{errors.description && (
<p className='mt-[9px] text-[var(--text-error)] text-caption'>{errors.description}</p>
)}
</DetailSection>
<DetailSection title='Content'>
<FieldLockTooltip reason={lockReason}>
<RichMarkdownField
key={`${skill.id}:${contentSeed}`}
value={contentDraft}
onChange={(value) => {
setContentDraft(value)
if (errors.content) setErrors((prev) => ({ ...prev, content: undefined }))
}}
placeholder='Skill instructions in markdown...'
minHeight={260}
disabled={readOnly}
error={!!errors.content}
workspaceId={workspaceId}
onPasteText={handleContentPaste}
/>
</FieldLockTooltip>
{errors.content && (
<p className='mt-[9px] text-[var(--text-error)] text-caption'>{errors.content}</p>
)}
</DetailSection>
<SkillFields
name={nameDraft}
description={descriptionDraft}
content={contentDraft}
onNameChange={(value) => {
setNameDraft(value)
if (errors.name) setErrors((prev) => ({ ...prev, name: undefined }))
}}
onDescriptionChange={(value) => {
setDescriptionDraft(value)
if (errors.description) setErrors((prev) => ({ ...prev, description: undefined }))
}}
onContentChange={(value) => {
setContentDraft(value)
if (errors.content) setErrors((prev) => ({ ...prev, content: undefined }))
}}
errors={errors}
contentKey={`${skill.id}:${contentSeed}`}
workspaceId={workspaceId}
disabled={readOnly}
lockReason={lockReason}
onPasteText={handleContentPaste}
/>
{!isBuiltin && <SkillEditorsCard editors={editors} canEdit={canEdit} />}
</CredentialDetailLayout>
@@ -0,0 +1,14 @@
/**
* Field copy shared by every skill editing surface. The two pages share JSX via
* `SkillFields`, but the canvas modal must frame its fields with
* `ChipModalField` — required inside a `ChipModalBody` — so it cannot. These
* strings are what keep the modal in step with the pages.
*/
export const SKILL_NAME_PLACEHOLDER = 'my-skill-name'
export const SKILL_NAME_HINT = 'Lowercase letters, numbers, and hyphens (e.g. my-skill)'
export const SKILL_DESCRIPTION_PLACEHOLDER = 'What this skill does and when to use it...'
export const SKILL_CONTENT_PLACEHOLDER = 'Skill instructions in markdown...'
/** Mirrors `skillDescriptionSchema` in the contract. */
export const SKILL_DESCRIPTION_MAX_LENGTH = 1024
@@ -0,0 +1,4 @@
export {
type SkillFieldErrors,
SkillFields,
} from '@/app/workspace/[workspaceId]/skills/components/skill-fields/skill-fields'
@@ -0,0 +1,166 @@
'use client'
import type { ReactNode } from 'react'
import { ChipInput, ChipTextarea, chipFieldSurfaceClass, cn, Tooltip } from '@sim/emcn'
import dynamic from 'next/dynamic'
import { DetailSection } from '@/app/workspace/[workspaceId]/components/credential-detail'
import {
SKILL_CONTENT_PLACEHOLDER,
SKILL_DESCRIPTION_MAX_LENGTH,
SKILL_DESCRIPTION_PLACEHOLDER,
SKILL_NAME_HINT,
SKILL_NAME_PLACEHOLDER,
} from '@/app/workspace/[workspaceId]/skills/components/skill-copy'
const RichMarkdownField = dynamic(
() =>
import(
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-field'
).then((m) => m.RichMarkdownField),
{
ssr: false,
loading: () => <div className={cn('min-h-[260px]', chipFieldSurfaceClass)} />,
}
)
export interface SkillFieldErrors {
name?: string
description?: string
content?: string
}
interface SkillFieldsProps {
name: string
description: string
content: string
onNameChange: (value: string) => void
onDescriptionChange: (value: string) => void
onContentChange: (value: string) => void
errors: SkillFieldErrors
/** Remounts the seed-once rich editor when content is set programmatically. */
contentKey: string | number
workspaceId: string
disabled?: boolean
/**
* Why the fields are locked (built-in skill, or viewer is not an editor).
* Renders a tooltip explaining it — a disabled control swallows hover, so each
* field is wrapped rather than the control itself.
*/
lockReason?: string | null
/** Intercepts a full SKILL.md paste into Content. */
onPasteText?: (text: string) => boolean
}
interface FieldLockTooltipProps {
reason: string | null | undefined
children: ReactNode
}
function FieldLockTooltip({ reason, children }: FieldLockTooltipProps) {
if (!reason) return <>{children}</>
return (
<Tooltip.Root>
<Tooltip.Trigger asChild>
<div>{children}</div>
</Tooltip.Trigger>
<Tooltip.Content>{reason}</Tooltip.Content>
</Tooltip.Root>
)
}
/**
* The hint-or-error line under a field. Renders nothing when there is neither,
* so a field without a message reserves no space.
*/
function FieldMessage({ error, hint }: { error?: string; hint?: string }) {
const message = error ?? hint
if (!message) return null
return (
<p
className={cn(
'mt-[9px] text-caption',
error ? 'text-[var(--text-error)]' : 'text-[var(--text-muted)]'
)}
>
{message}
</p>
)
}
/**
* The Name / Description / Content trio as the full-page skill surfaces render
* it — `DetailSection` rows with the error line beneath each field. Shared by
* the create and detail pages so the copy, sizing, and error treatment cannot
* drift between them. The canvas modal frames the same fields with
* `ChipModalField` (required inside a `ChipModalBody`) and shares the copy
* constants instead.
*/
export function SkillFields({
name,
description,
content,
onNameChange,
onDescriptionChange,
onContentChange,
errors,
contentKey,
workspaceId,
disabled = false,
lockReason,
onPasteText,
}: SkillFieldsProps) {
return (
<>
<DetailSection title='Name'>
<FieldLockTooltip reason={lockReason}>
<ChipInput
id='skill-name'
value={name}
onChange={(event) => onNameChange(event.target.value)}
placeholder={SKILL_NAME_PLACEHOLDER}
autoComplete='off'
data-lpignore='true'
disabled={disabled}
error={!!errors.name}
/>
</FieldLockTooltip>
<FieldMessage error={errors.name} hint={disabled ? undefined : SKILL_NAME_HINT} />
</DetailSection>
<DetailSection title='Description'>
<FieldLockTooltip reason={lockReason}>
<ChipTextarea
id='skill-description'
rows={3}
value={description}
onChange={(event) => onDescriptionChange(event.target.value)}
placeholder={SKILL_DESCRIPTION_PLACEHOLDER}
maxLength={SKILL_DESCRIPTION_MAX_LENGTH}
autoComplete='off'
data-lpignore='true'
disabled={disabled}
error={!!errors.description}
/>
</FieldLockTooltip>
<FieldMessage error={errors.description} />
</DetailSection>
<DetailSection title='Content'>
<FieldLockTooltip reason={lockReason}>
<RichMarkdownField
key={contentKey}
value={content}
onChange={onContentChange}
placeholder={SKILL_CONTENT_PLACEHOLDER}
minHeight={260}
disabled={disabled}
error={!!errors.content}
workspaceId={workspaceId}
onPasteText={onPasteText}
/>
</FieldLockTooltip>
<FieldMessage error={errors.content} />
</DetailSection>
</>
)
}
@@ -15,6 +15,13 @@ import {
import { getErrorMessage } from '@sim/utils/errors'
import dynamic from 'next/dynamic'
import { useParams } from 'next/navigation'
import {
SKILL_CONTENT_PLACEHOLDER,
SKILL_DESCRIPTION_MAX_LENGTH,
SKILL_DESCRIPTION_PLACEHOLDER,
SKILL_NAME_HINT,
SKILL_NAME_PLACEHOLDER,
} from '@/app/workspace/[workspaceId]/skills/components/skill-copy'
import { SkillImport } from '@/app/workspace/[workspaceId]/skills/components/skill-import'
import {
isSkillNameConflictError,
@@ -73,7 +80,6 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
*/
const [contentSeed, setContentSeed] = useState(0)
const [errors, setErrors] = useState<FieldErrors>({})
const [saving, setSaving] = useState(false)
const [activeTab, setActiveTab] = useState<TabValue>('create')
const [prevOpen, setPrevOpen] = useState(false)
const [prevInitialValues, setPrevInitialValues] = useState(initialValues)
@@ -96,6 +102,8 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
description !== initialValues.description ||
content !== initialValues.content
const saving = createSkill.isPending || updateSkill.isPending
const handleSave = async () => {
const newErrors: FieldErrors = {}
@@ -115,8 +123,6 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
return
}
setSaving(true)
try {
if (initialValues) {
await updateSkill.mutateAsync({
@@ -137,8 +143,6 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
} else {
setErrors({ general: 'Failed to save skill. Please try again.' })
}
} finally {
setSaving(false)
}
}
@@ -205,10 +209,10 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
if (errors.name || errors.general)
setErrors((prev) => ({ ...prev, name: undefined, general: undefined }))
}}
placeholder='my-skill-name'
placeholder={SKILL_NAME_PLACEHOLDER}
required
error={errors.name}
hint='Lowercase letters, numbers, and hyphens (e.g. my-skill)'
hint={SKILL_NAME_HINT}
disabled={readOnly || saving}
/>
@@ -221,8 +225,8 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
if (errors.description || errors.general)
setErrors((prev) => ({ ...prev, description: undefined, general: undefined }))
}}
placeholder='What this skill does and when to use it...'
maxLength={1024}
placeholder={SKILL_DESCRIPTION_PLACEHOLDER}
maxLength={SKILL_DESCRIPTION_MAX_LENGTH}
required
error={errors.description}
disabled={readOnly || saving}
@@ -237,7 +241,7 @@ export function SkillModal({ open, onOpenChange, onSave, initialValues }: SkillM
if (errors.content || errors.general)
setErrors((prev) => ({ ...prev, content: undefined, general: undefined }))
}}
placeholder='Skill instructions in markdown...'
placeholder={SKILL_CONTENT_PLACEHOLDER}
minHeight={200}
maxHeight={360}
disabled={readOnly || saving}
@@ -1,28 +1,22 @@
'use client'
import { useState } from 'react'
import {
Chip,
ChipInput,
ChipLink,
ChipTextarea,
chipFieldSurfaceClass,
cn,
toast,
} from '@sim/emcn'
import { Chip, ChipLink, toast } from '@sim/emcn'
import { ArrowLeft } from '@sim/emcn/icons'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import dynamic from 'next/dynamic'
import { useRouter } from 'next/navigation'
import { SkillTile } from '@/app/workspace/[workspaceId]/components'
import {
CredentialDetailHeading,
CredentialDetailLayout,
DetailSection,
UnsavedChangesModal,
useUnsavedChangesGuard,
} from '@/app/workspace/[workspaceId]/components/credential-detail'
import {
type SkillFieldErrors,
SkillFields,
} from '@/app/workspace/[workspaceId]/skills/components/skill-fields'
import { SkillImportButton } from '@/app/workspace/[workspaceId]/skills/components/skill-import'
import {
isSkillNameConflictError,
@@ -32,25 +26,8 @@ import {
} from '@/app/workspace/[workspaceId]/skills/components/utils'
import { useCreateSkill } from '@/hooks/queries/skills'
const RichMarkdownField = dynamic(
() =>
import(
'@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-field'
).then((m) => m.RichMarkdownField),
{
ssr: false,
loading: () => <div className={cn('min-h-[260px]', chipFieldSurfaceClass)} />,
}
)
const logger = createLogger('SkillCreate')
interface FieldErrors {
name?: string
description?: string
content?: string
}
interface SkillCreateProps {
workspaceId: string
}
@@ -71,7 +48,7 @@ export function SkillCreate({ workspaceId }: SkillCreateProps) {
const [contentDraft, setContentDraft] = useState('')
/** Bumped to remount the seed-once rich Content editor on programmatic sets. */
const [contentSeed, setContentSeed] = useState(0)
const [errors, setErrors] = useState<FieldErrors>({})
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.
@@ -84,7 +61,7 @@ export function SkillCreate({ workspaceId }: SkillCreateProps) {
const handleCreate = async () => {
if (createSkill.isPending) return
const newErrors: FieldErrors = {}
const newErrors: SkillFieldErrors = {}
const nameError = validateSkillName(nameDraft)
if (nameError) newErrors.name = nameError
if (!descriptionDraft.trim()) newErrors.description = 'Description is required'
@@ -161,70 +138,28 @@ export function SkillCreate({ workspaceId }: SkillCreateProps) {
subtitle='Write a skill, or import an existing SKILL.md'
/>
<DetailSection title='Name'>
<ChipInput
id='skill-name'
value={nameDraft}
onChange={(event) => {
setNameDraft(event.target.value)
if (errors.name) setErrors((prev) => ({ ...prev, name: undefined }))
}}
placeholder='my-skill-name'
autoComplete='off'
data-lpignore='true'
disabled={createSkill.isPending}
error={!!errors.name}
/>
<p
className={cn(
'mt-[9px] text-caption',
errors.name ? 'text-[var(--text-error)]' : 'text-[var(--text-muted)]'
)}
>
{errors.name ?? 'Lowercase letters, numbers, and hyphens (e.g. my-skill)'}
</p>
</DetailSection>
<DetailSection title='Description'>
<ChipTextarea
id='skill-description'
rows={3}
value={descriptionDraft}
onChange={(event) => {
setDescriptionDraft(event.target.value)
if (errors.description) setErrors((prev) => ({ ...prev, description: undefined }))
}}
placeholder='What this skill does and when to use it...'
maxLength={1024}
autoComplete='off'
data-lpignore='true'
disabled={createSkill.isPending}
error={!!errors.description}
/>
{errors.description && (
<p className='mt-[9px] text-[var(--text-error)] text-caption'>{errors.description}</p>
)}
</DetailSection>
<DetailSection title='Content'>
<RichMarkdownField
key={contentSeed}
value={contentDraft}
onChange={(value) => {
setContentDraft(value)
if (errors.content) setErrors((prev) => ({ ...prev, content: undefined }))
}}
placeholder='Skill instructions in markdown...'
minHeight={260}
disabled={createSkill.isPending}
error={!!errors.content}
workspaceId={workspaceId}
onPasteText={handleContentPaste}
/>
{errors.content && (
<p className='mt-[9px] text-[var(--text-error)] text-caption'>{errors.content}</p>
)}
</DetailSection>
<SkillFields
name={nameDraft}
description={descriptionDraft}
content={contentDraft}
onNameChange={(value) => {
setNameDraft(value)
if (errors.name) setErrors((prev) => ({ ...prev, name: undefined }))
}}
onDescriptionChange={(value) => {
setDescriptionDraft(value)
if (errors.description) setErrors((prev) => ({ ...prev, description: undefined }))
}}
onContentChange={(value) => {
setContentDraft(value)
if (errors.content) setErrors((prev) => ({ ...prev, content: undefined }))
}}
errors={errors}
contentKey={contentSeed}
workspaceId={workspaceId}
disabled={createSkill.isPending}
onPasteText={handleContentPaste}
/>
</CredentialDetailLayout>
<UnsavedChangesModal