Skip to content

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

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

Merged
waleedlatif1 merged 2 commits into from
Jul 24, 2026
+258 −224
Merged

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

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
6d87eee
refactor(skills): one definition of the skill fields across every sur…
waleedlatif1 Jul 24, 2026
210c288
refactor(skills): derive modal saving state, collapse the field messa…
waleedlatif1 Jul 24, 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
151 changes: 31 additions & 120 deletions apps/sim/app/workspace/[workspaceId]/skills/[skillId]/skill-detail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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>
Expand Down
14 changes: 14 additions & 0 deletions apps/sim/app/workspace/[workspaceId]/skills/components/skill-copy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions apps/sim/app/workspace/[workspaceId]/skills/components/skill-fields/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export {
type SkillFieldErrors,
SkillFields,
} from '@/app/workspace/[workspaceId]/skills/components/skill-fields/skill-fields'
166 changes: 166 additions & 0 deletions apps/sim/app/workspace/[workspaceId]/skills/components/skill-fields/skill-fields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
</>
)
}
Loading
Loading