Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 29 additions & 3 deletions app/src/components/skills/SkillsExplorerTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,14 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
const [catalogError, setCatalogError] = useState<string | null>(null);
const [catalogInitialized, setCatalogInitialized] = useState(false);
const [installingId, setInstallingId] = useState<string | null>(null);
// Catalog entry ids we just installed this session. The "installed" badge is
// otherwise derived purely from `isCatalogEntryInstalled`, a heuristic that
// maps a refetched installed skill (whose post-install id/location can differ
// from the catalog entry) back to the catalog card. When that mapping misses,
// a successful install fell back to "Install" — the only signal was a fleeting
// toast, so the card looked unchanged (#4150). Recording the installed entry
// id here makes the card flip to "Installed" deterministically on success.
const [installedEntryIds, setInstalledEntryIds] = useState<Set<string>>(new Set());

const [sources, setSources] = useState<string[]>([]);
const [activeSources, setActiveSources] = useState<Set<string>>(new Set());
Expand Down Expand Up @@ -638,6 +646,16 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
[skills]
);

// A catalog entry counts as installed if the refetched installed list maps
// back to it (`isCatalogEntryInstalled`) OR we installed it this session. The
// latter guarantees the card reflects a successful install even when the
// heuristic key-match misses (#4150).
const entryInstalled = useCallback(
(entry: CatalogEntry): boolean =>
installedEntryIds.has(entry.id) || isCatalogEntryInstalled(entry, installedKeys),
[installedEntryIds, installedKeys]
);

const filteredSkills = useMemo(() => {
const q = searchQuery.toLowerCase().trim();
if (!q) return skills;
Expand Down Expand Up @@ -701,6 +719,14 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
setInstallingId(entry.id);
try {
const result = await skillRegistryApi.install(entry.id);
// Authoritatively mark this entry installed so the card flips to
// "Installed" on success regardless of whether the refetched list maps
// back to it via the install-key heuristic (#4150).
setInstalledEntryIds(prev => {
const next = new Set(prev);
next.add(entry.id);
return next;
});
// Await the refetch so `installedKeys` is fresh before the button
// re-renders — otherwise it briefly flips back to "Install" between
// clearing the installing state and the list updating. `fetchSkills`
Expand Down Expand Up @@ -975,7 +1001,7 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
<CatalogTile
key={`${entry.source}-${entry.id}`}
entry={entry}
installed={isCatalogEntryInstalled(entry, installedKeys)}
installed={entryInstalled(entry)}
installing={installingId === entry.id}
onClick={() => setDetailEntry(entry)}
onInstall={() => void handleRegistryInstall(entry)}
Expand Down Expand Up @@ -1014,13 +1040,13 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
<SkillDetailDialog
entry={detailEntry}
skill={detailSkill}
installed={detailEntry ? isCatalogEntryInstalled(detailEntry, installedKeys) : true}
installed={detailEntry ? entryInstalled(detailEntry) : true}
onClose={() => {
setDetailEntry(null);
setDetailSkill(null);
}}
onInstall={
detailEntry && !isCatalogEntryInstalled(detailEntry, installedKeys)
detailEntry && !entryInstalled(detailEntry)
? () => {
void handleRegistryInstall(detailEntry);
setDetailEntry(null);
Expand Down
41 changes: 41 additions & 0 deletions app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,47 @@ describe('SkillsExplorerTab', () => {
expect(within(tile).getByTestId('registry-install-built-in/apple-notes')).toBeInTheDocument();
});

// #4150: a successful install must flip the card to "Installed" even when the
// refetched installed list does NOT map back to the catalog entry via the
// install-key heuristic — otherwise the card reverted to "Install" and the
// only signal of success was a fleeting toast.
it('marks a catalog entry installed on success even when the refetched list does not map back', async () => {
const { workflowsApi } = await import('../../../services/api/workflowsApi');
const { skillRegistryApi } = await import('../../../services/api/skillRegistryApi');
const catalogEntry = {
...MOCK_CATALOG_ENTRY,
id: 'built-in/apple-notes',
name: 'Apple Notes',
docs_path: 'skills/apple-notes/SKILL.md',
};
// The installed list never resolves to anything that maps back to the entry
// (simulates a post-install id/location the heuristic can't match).
vi.mocked(workflowsApi.listWorkflows).mockResolvedValue([]);
vi.mocked(skillRegistryApi.browse).mockResolvedValue([catalogEntry]);
vi.mocked(skillRegistryApi.install).mockResolvedValue({
url: '',
stdout: '',
stderr: '',
newSkills: ['apple-notes'],
});

render(<SkillsExplorerTab />);

const installBtn = await screen.findByTestId('registry-install-built-in/apple-notes');
await act(async () => {
fireEvent.click(installBtn);
});

const tile = screen.getByTestId('registry-tile-built-in/apple-notes');
await waitFor(() => {
expect(within(tile).getByText('Installed')).toBeInTheDocument();
});
expect(skillRegistryApi.install).toHaveBeenCalledWith('built-in/apple-notes');
expect(
within(tile).queryByTestId('registry-install-built-in/apple-notes')
).not.toBeInTheDocument();
});

it('has an install from URL button', async () => {
const { workflowsApi } = await import('../../../services/api/workflowsApi');
vi.mocked(workflowsApi.listWorkflows).mockResolvedValue([]);
Expand Down
Loading