refactor(skills): use one row component across every update state

The ready and running views were separate components with different row
shapes, so pressing Update swapped the dialog's body for a different layout.
They are now the same `SkillUpdateRow` instances throughout — only the status
slot's contents change — and a test asserts the row is literally the same DOM
node from "update available" through pending to the result.

- Collapse each skill's locations behind its own disclosure. A skill with
  several plugin-cache copies was dumping every path inline and burying the
  actions; the row now shows a location count and expands on demand.
- Put status in a single slot between the name and the count rather than a
  leading icon column. A leading icon has nothing to show in the resting state
  and reserving its box just indented every name past an empty gap.
- Pin the running/finished run's names in `groupSkillFreshness` so a successful
  update doesn't drop its own rows the instant the re-scan lands.

`skill-freshness-group.tsx` becomes `skill-location-chip-copy.ts` — only its
chip label/tooltip helpers survived, and it no longer holds JSX.
This commit is contained in:
Brennan Benson
2026-07-27 12:05:13 -07:00
parent 2384c79cf0
commit a63abe6ffb
12 changed files with 357 additions and 107 deletions
@@ -49,15 +49,22 @@ vi.mock('@/components/ui/dialog', () => ({
}))
vi.mock('@/components/ui/collapsible', () => ({
// Forward data-* props: the unified skill row tags its Collapsible root with
// the identifiers the assertions below read.
Collapsible: ({
children,
defaultOpen = false
defaultOpen = false,
...rest
}: {
children?: ReactNode
defaultOpen?: boolean
}) => {
} & Record<string, unknown>) => {
const [open] = useState(defaultOpen)
return <div data-collapsible-open={String(open)}>{children}</div>
return (
<div {...rest} data-collapsible-open={String(open)}>
{children}
</div>
)
},
CollapsibleTrigger: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
CollapsibleContent: ({ children }: { children?: ReactNode }) => <div>{children}</div>
@@ -214,7 +221,7 @@ describe('SkillFreshnessUpdateDialog', () => {
expect(container?.textContent).toContain('keeps running in the background')
expect(container?.querySelector('[role="progressbar"]')).not.toBeNull()
expect(
container?.querySelector('[data-skill-result="orca-cli"]')?.getAttribute('data-status')
container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label')
).toBe('pending')
})
@@ -240,7 +247,7 @@ describe('SkillFreshnessUpdateDialog', () => {
expect(container?.textContent).toContain('Updated 1 skill')
expect(
container?.querySelector('[data-skill-result="orca-cli"]')?.getAttribute('data-status')
container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label')
).toBe('done')
expect(findButton('Done')).toBeDefined()
// The re-scan is what makes the result trustworthy, so it must be requested.
@@ -267,15 +274,65 @@ describe('SkillFreshnessUpdateDialog', () => {
expect(container?.textContent).toContain('Updated 1 of 2 skills')
expect(
container?.querySelector('[data-skill-result="orca-cli"]')?.getAttribute('data-status')
container?.querySelector('[data-skill-row="orca-cli"]')?.getAttribute('data-state-label')
).toBe('done')
expect(
container?.querySelector('[data-skill-result="orchestration"]')?.getAttribute('data-status')
container?.querySelector('[data-skill-row="orchestration"]')?.getAttribute('data-state-label')
).toBe('failed')
expect(container?.textContent).toContain('skills update exited with code 1')
expect(findButton('Retry')).toBeDefined()
})
it('keeps the same row elements across the whole run instead of swapping layouts', async () => {
await renderDialog()
await openViaRequest()
const before = container?.querySelector('[data-skill-row="orca-cli"]')
expect(before?.getAttribute('data-state-label')).toBe('available')
await emitRun({ state: 'running', names: ['orca-cli'], startedAt: 1, output: '' })
const during = container?.querySelector('[data-skill-row="orca-cli"]')
expect(during).toBe(before)
expect(during?.getAttribute('data-state-label')).toBe('pending')
// The re-scan makes the skill current, which would otherwise drop it from
// the group list and blank the row out mid-transition.
mocks.inventory = {
schemaVersion: 1,
installations: [placement('orca-cli', { status: 'current', installedReleaseRevision: 2 })],
eligibleUpdateNames: [],
scannedAt: 5
}
await emitRun({ state: 'success', names: ['orca-cli'], finishedAt: 2, output: 'done' })
await rerender()
const after = container?.querySelector('[data-skill-row="orca-cli"]')
expect(after).toBe(before)
expect(after?.getAttribute('data-state-label')).toBe('done')
})
it('collapses each skill’s locations behind its own disclosure', async () => {
mocks.inventory = {
schemaVersion: 1,
installations: [
placement('orca-cli'),
placement('orca-cli', {
rootId: 'plugin',
topology: 'plugin-cache',
status: 'inaccessible'
})
],
eligibleUpdateNames: ['orca-cli'],
scannedAt: 1
}
await renderDialog()
await openViaRequest()
const row = container?.querySelector('[data-skill-row="orca-cli"]')
expect(row).not.toBeNull()
// Closed by default — the paths are behind the row's own trigger.
expect(row?.getAttribute('data-collapsible-open')).toBe('false')
expect(container?.textContent).toContain('2 locations')
})
it('shows the captured log verbatim without parsing it', async () => {
await renderDialog()
await openViaRequest()
@@ -15,8 +15,7 @@ import {
import { Collapsible, CollapsibleContent, CollapsibleTrigger } from '@/components/ui/collapsible'
import { TooltipProvider } from '@/components/ui/tooltip'
import { groupSkillFreshness } from './skill-freshness-grouping'
import { SkillFreshnessGroup } from './skill-freshness-group'
import { SkillUpdateResultRows } from './skill-update-result-rows'
import { SkillUpdateRow } from './SkillUpdateRow'
import { SummaryHeadline, summarizeInventory } from './skill-freshness-summary-headline'
import {
acknowledgeSkillUpdateRun,
@@ -67,16 +66,39 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
const [copied, setCopied] = useState(false)
const inventory = state.inventory
const eligibleNames = useMemo(() => inventory?.eligibleUpdateNames ?? [], [inventory])
const isRunning = run.state === 'running'
const showResult = run.state === 'success' || run.state === 'error'
const runNames = useMemo(() => (run.state === 'idle' ? [] : run.names), [run])
const groups = useMemo(
() =>
inventory ? groupSkillFreshness(inventory.installations, inventory.eligibleUpdateNames) : [],
[inventory]
inventory
? groupSkillFreshness(inventory.installations, inventory.eligibleUpdateNames, runNames)
: [],
[inventory, runNames]
)
const hasBlockedGroup = groups.some((group) => group.status === 'cannot-update')
const blockedCount = groups.filter((group) => group.status === 'cannot-update').length
const summaryKind = summarizeInventory(inventory, hasBlockedGroup)
const isRunning = run.state === 'running'
const showResult = run.state === 'success' || run.state === 'error'
// Why: one row list for every state. The rows are identical objects across the
// transition, so pressing Update changes each row's leading icon in place
// instead of swapping the dialog's body for a different component.
const rows = useMemo(() => {
const failed = new Set(run.state === 'error' ? run.failedNames : [])
const inRun = new Set(runNames)
return groups.map((group) => {
if (inRun.has(group.name)) {
if (isRunning) {
return { group, state: 'pending' as const }
}
return { group, state: failed.has(group.name) ? ('failed' as const) : ('done' as const) }
}
return {
group,
state: group.status === 'cannot-update' ? ('blocked' as const) : ('available' as const)
}
})
}, [groups, isRunning, run, runNames])
const handleOpenChange = (next: boolean): void => {
if (next) {
@@ -193,27 +215,27 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
)}
{isRunning ? (
<>
{/* Indeterminate on purpose: the CLI reports no parseable progress. */}
<div
role="progressbar"
aria-label={translate(
'auto.components.skills.SkillFreshnessUpdateDialog.progressAria',
'Updating skills'
)}
className="h-1 overflow-hidden rounded-full bg-secondary"
>
<div className="h-full w-2/5 animate-[skill-update-slide_1.35s_ease-in-out_infinite] rounded-full bg-primary motion-reduce:w-full motion-reduce:animate-none motion-reduce:opacity-40" />
</div>
<SkillUpdateResultRows names={run.names} pending />
</>
// Indeterminate on purpose: the CLI reports no parseable progress.
<div
role="progressbar"
aria-label={translate(
'auto.components.skills.SkillFreshnessUpdateDialog.progressAria',
'Updating skills'
)}
className="h-1 overflow-hidden rounded-full bg-secondary"
>
<div className="h-full w-2/5 animate-[skill-update-slide_1.35s_ease-in-out_infinite] rounded-full bg-primary motion-reduce:w-full motion-reduce:animate-none motion-reduce:opacity-40" />
</div>
) : null}
{showResult ? (
<SkillUpdateResultRows
names={run.names}
failedNames={run.state === 'error' ? run.failedNames : []}
/>
{rows.length > 0 ? (
<div className="min-w-0">
<TooltipProvider>
{rows.map((row) => (
<SkillUpdateRow key={row.group.name} group={row.group} state={row.state} />
))}
</TooltipProvider>
</div>
) : null}
{run.state === 'error' ? (
@@ -252,16 +274,6 @@ export function SkillFreshnessUpdateDialog(): React.JSX.Element {
{isRunning || showResult ? <RunLog output={run.output} /> : null}
{!isRunning && !showResult && groups.length > 0 ? (
<div className="min-w-0 divide-y divide-border/40">
<TooltipProvider>
{groups.map((group) => (
<SkillFreshnessGroup key={group.name} group={group} />
))}
</TooltipProvider>
</div>
) : null}
<DialogFooter className="sm:justify-between">
{isRunning || showResult ? (
<span />
@@ -0,0 +1,140 @@
import { CheckCircle2, ChevronDown, Circle, XCircle } from 'lucide-react'
import type { SkillFreshnessGroupModel } from './skill-freshness-grouping'
import { translate } from '@/i18n/i18n'
import { Badge } from '@/components/ui/badge'
import { Collapsible, CollapsibleContent, CollapsibleTrigger } from '@/components/ui/collapsible'
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'
import { chipLabel, chipTooltip } from './skill-location-chip-copy'
import { skippedReason } from './skill-freshness-skipped-reason'
export type SkillRowState = 'available' | 'blocked' | 'pending' | 'done' | 'failed'
/**
* The single status slot, sitting between the name and the location count.
*
* Why one slot rather than a leading icon column: a leading icon has nothing to
* show in the resting state, and reserving the box for it just indents every
* name past an empty gap. Swapping badge for icon in place keeps the name
* flush-left and the row's right edge fixed in every state.
*/
function StateSlot({ state }: { state: SkillRowState }): React.JSX.Element {
switch (state) {
case 'done':
return <CheckCircle2 className="size-4 shrink-0 text-emerald-600 dark:text-emerald-400" />
case 'failed':
return <XCircle className="size-4 shrink-0 text-destructive" />
case 'pending':
return <Circle className="size-4 shrink-0 text-muted-foreground" />
case 'blocked':
return (
<Badge
variant="outline"
className="shrink-0 border-amber-600/50 text-amber-700 dark:border-amber-400/40 dark:text-amber-400"
>
{translate('auto.components.skills.SkillFreshnessRow.statusCantUpdate', 'Skipped')}
</Badge>
)
case 'available':
return (
<Badge variant="secondary" className="shrink-0">
{translate(
'auto.components.skills.SkillFreshnessRow.statusUpdateAvailable',
'Update available'
)}
</Badge>
)
}
}
/**
* One skill in the update dialog, used unchanged in every state.
*
* The header keeps identical geometry from "update available" through running to
* the result — only the status slot's contents change — so pressing Update
* doesn't replace the dialog's layout with a different one. Locations live
* behind a per-skill disclosure instead of being dumped inline, because a skill
* with several plugin-cache copies otherwise buries the actions.
*/
export function SkillUpdateRow({
group,
state
}: {
group: SkillFreshnessGroupModel
state: SkillRowState
}): React.JSX.Element {
const locationCount = group.locations.length
return (
<Collapsible
data-skill-row={group.name}
data-state-label={state}
// Negative margin lets the hover surface breathe past the text while the
// name itself still lines up with the dialog's other content.
className="-mx-1.5 border-t border-border/60 py-0.5 first:border-t-0"
>
<CollapsibleTrigger
className={`group flex w-full min-w-0 items-center gap-3 rounded-md px-1.5 py-2 text-left transition-opacity hover:bg-accent/60 ${
state === 'pending' ? 'opacity-60' : ''
}`}
>
<span className="min-w-0 flex-1 truncate text-[13px] font-medium text-foreground">
{group.name}
</span>
<StateSlot state={state} />
<span className="shrink-0 text-[11px] tabular-nums text-muted-foreground">
{locationCount === 1
? translate('auto.components.skills.SkillUpdateRow.oneLocation', '1 location')
: translate(
'auto.components.skills.SkillUpdateRow.manyLocations',
'{{value0}} locations',
{ value0: locationCount }
)}
</span>
<ChevronDown className="size-3.5 shrink-0 text-muted-foreground transition-transform group-data-[state=open]:rotate-180" />
</CollapsibleTrigger>
{state === 'failed' ? (
<p className="px-1.5 pb-1.5 text-xs leading-5 text-muted-foreground">
{translate(
'auto.components.skills.SkillUpdateResultRows.stillOutdated',
'Still out of date after the update ran.'
)}
</p>
) : null}
<CollapsibleContent className="overflow-hidden data-[state=closed]:animate-collapsible-up data-[state=open]:animate-collapsible-down">
<div className="flex flex-col gap-2 px-1.5 pb-2 pt-0.5">
{state === 'blocked' ? (
<p className="text-xs leading-5 text-muted-foreground">
{skippedReason(group.locations)}
</p>
) : null}
{group.locations.map((location) => (
<div key={location.id} className="flex min-w-0 items-center gap-2">
{/* Why: plugin-cache paths nest arbitrarily deep. Without an explicit
shrink basis the unbreakable string sets the dialog's width and
pushes the footer actions off-screen. */}
<span
className="min-w-0 flex-1 truncate font-mono text-[11px] text-muted-foreground"
title={location.path}
>
{location.path}
</span>
{location.chip ? (
<Tooltip>
<TooltipTrigger asChild>
<Badge variant="outline" className="shrink-0 cursor-help border-dashed">
{chipLabel(location.chip)}
</Badge>
</TooltipTrigger>
<TooltipContent className="max-w-xs text-pretty">
{chipTooltip(location.chip)}
</TooltipContent>
</Tooltip>
) : null}
</div>
))}
</div>
</CollapsibleContent>
</Collapsible>
)
}
@@ -60,12 +60,19 @@ export function locationChip(installation: SkillFreshnessInstallation): SkillLoc
* update disposition. Only skills with an out-of-date official copy are returned —
* up-to-date, unrecognized-only, and unreadable-only skills have nothing to change
* here, so they are omitted entirely.
*
* `alwaysIncludeNames` overrides that filter. A successful update makes every
* targeted skill current, which would otherwise drop its row the instant the
* re-scan lands — the dialog passes the running/finished run's names so the same
* rows stay put from "update available" through to the result.
*/
export function groupSkillFreshness(
installations: readonly SkillFreshnessInstallation[],
eligibleUpdateNames: readonly string[]
eligibleUpdateNames: readonly string[],
alwaysIncludeNames: readonly string[] = []
): SkillFreshnessGroupModel[] {
const eligible = new Set(eligibleUpdateNames)
const pinned = new Set(alwaysIncludeNames)
const byName = new Map<string, SkillFreshnessInstallation[]>()
for (const installation of installations) {
const entries = byName.get(installation.name) ?? []
@@ -74,7 +81,7 @@ export function groupSkillFreshness(
}
const groups: SkillFreshnessGroupModel[] = []
for (const [name, entries] of byName) {
if (!entries.some((entry) => entry.status === 'outdated')) {
if (!pinned.has(name) && !entries.some((entry) => entry.status === 'outdated')) {
continue
}
const locations = entries
@@ -0,0 +1,77 @@
import type { SkillLocationChip } from './skill-freshness-grouping'
import { translate } from '@/i18n/i18n'
export function chipLabel(chip: SkillLocationChip): string {
switch (chip) {
case 'current':
return translate('auto.components.skills.SkillFreshnessRow.chipCurrent', 'Current')
case 'unrecognized':
return translate('auto.components.skills.SkillFreshnessRow.chipUnrecognized', 'Unrecognized')
case 'inaccessible':
return translate('auto.components.skills.SkillFreshnessRow.chipInaccessible', 'Inaccessible')
case 'duplicate':
return translate('auto.components.skills.SkillFreshnessRow.chipDuplicate', 'Duplicate')
case 'external-link':
return translate('auto.components.skills.SkillFreshnessRow.chipExternalLink', 'External link')
case 'broken-link':
return translate('auto.components.skills.SkillFreshnessRow.chipBrokenLink', 'Broken link')
case 'read-only':
return translate('auto.components.skills.SkillFreshnessRow.chipReadOnly', 'Read only')
case 'in-a-repo':
return translate('auto.components.skills.SkillFreshnessRow.chipInRepo', 'In a repo')
case 'plugin-cache':
return translate('auto.components.skills.SkillFreshnessRow.chipPluginCache', 'Plugin cache')
}
}
// Why: chips describe only what a location *is*; the effect on the update
// command lives in the per-skill sentence, so the two never say it twice.
export function chipTooltip(chip: SkillLocationChip): string {
switch (chip) {
case 'current':
return translate(
'auto.components.skills.SkillFreshnessRow.tipCurrent',
'This copy matches the current official version.'
)
case 'unrecognized':
return translate(
'auto.components.skills.SkillFreshnessRow.tipUnrecognized',
'This copy doesn’t match any official version — it may be modified, or a different skill with the same name.'
)
case 'inaccessible':
return translate(
'auto.components.skills.SkillFreshnessRow.tipInaccessible',
'Orca couldn’t read this copy (a permissions or file error).'
)
case 'duplicate':
return translate(
'auto.components.skills.SkillFreshnessRow.tipDuplicate',
'A separate copy of this skill, installed apart from the main one.'
)
case 'external-link':
return translate(
'auto.components.skills.SkillFreshnessRow.tipExternalLink',
'A shortcut pointing outside Orca’s skill folders.'
)
case 'broken-link':
return translate(
'auto.components.skills.SkillFreshnessRow.tipBrokenLink',
'A shortcut to something that no longer exists.'
)
case 'read-only':
return translate(
'auto.components.skills.SkillFreshnessRow.tipReadOnly',
'This copy is in a read-only location.'
)
case 'in-a-repo':
return translate(
'auto.components.skills.SkillFreshnessRow.tipInRepo',
'This copy lives inside a project, not your global skills.'
)
case 'plugin-cache':
return translate(
'auto.components.skills.SkillFreshnessRow.tipPluginCache',
'This copy is managed by a plugin.'
)
}
}
@@ -1,63 +0,0 @@
import { CheckCircle2, Circle, XCircle } from 'lucide-react'
import { translate } from '@/i18n/i18n'
export type SkillRunRowStatus = 'pending' | 'done' | 'failed'
function statusIcon(status: SkillRunRowStatus): React.JSX.Element {
if (status === 'done') {
return <CheckCircle2 className="size-4 shrink-0 text-emerald-600 dark:text-emerald-400" />
}
if (status === 'failed') {
return <XCircle className="size-4 shrink-0 text-destructive" />
}
return <Circle className="size-4 shrink-0 text-muted-foreground" />
}
/**
* Per-skill outcome rows for a finished or in-flight run.
*
* Rendered from the run itself rather than the freshness groups: a successful
* update makes every skill current, which drops it out of `groupSkillFreshness`
* entirely, so the groups are empty exactly when the results matter most.
*/
export function SkillUpdateResultRows({
names,
failedNames,
pending
}: {
names: readonly string[]
failedNames?: readonly string[]
pending?: boolean
}): React.JSX.Element {
const failed = new Set(failedNames ?? [])
return (
<div className="flex flex-col">
{names.map((name) => {
const status: SkillRunRowStatus = pending ? 'pending' : failed.has(name) ? 'failed' : 'done'
return (
<div
key={name}
data-skill-result={name}
data-status={status}
className={`flex min-w-0 items-center gap-3 border-t border-border/60 py-2.5 first:border-t-0 first:pt-0.5 ${
pending ? 'opacity-55' : ''
}`}
>
{statusIcon(status)}
<div className="min-w-0 flex-1">
<div className="truncate text-[13px] font-medium text-foreground">{name}</div>
{status === 'failed' ? (
<div className="text-xs leading-5 text-muted-foreground">
{translate(
'auto.components.skills.SkillUpdateResultRows.stillOutdated',
'Still out of date after the update ran.'
)}
</div>
) : null}
</div>
</div>
)
})}
</div>
)
}
+4
View File
@@ -3815,6 +3815,10 @@
"SkillUpdateResultRows": {
"stillOutdated": "Still out of date after the update ran."
},
"SkillUpdateRow": {
"oneLocation": "1 location",
"manyLocations": "{{value0}} locations"
},
"SkillFreshnessStatusPill": {
"updateAvailable": "Update available",
"upToDate": "Up to date",
+4
View File
@@ -3798,6 +3798,10 @@
},
"SkillUpdateResultRows": {
"stillOutdated": "Still out of date after the update ran."
},
"SkillUpdateRow": {
"oneLocation": "1 location",
"manyLocations": "{{value0}} locations"
}
},
"sidebar": {
+4
View File
@@ -3798,6 +3798,10 @@
},
"SkillUpdateResultRows": {
"stillOutdated": "Still out of date after the update ran."
},
"SkillUpdateRow": {
"oneLocation": "1 location",
"manyLocations": "{{value0}} locations"
}
},
"sidebar": {
+4
View File
@@ -3798,6 +3798,10 @@
},
"SkillUpdateResultRows": {
"stillOutdated": "Still out of date after the update ran."
},
"SkillUpdateRow": {
"oneLocation": "1 location",
"manyLocations": "{{value0}} locations"
}
},
"sidebar": {
+4
View File
@@ -3798,6 +3798,10 @@
},
"SkillUpdateResultRows": {
"stillOutdated": "Still out of date after the update ran."
},
"SkillUpdateRow": {
"oneLocation": "1 location",
"manyLocations": "{{value0}} locations"
}
},
"sidebar": {