refactor(skills): apply simplify-pass cleanups across the stack (#2612)

## Problem

Several small pieces of logic were duplicated or misplaced across the skills feature, making it harder to maintain and extend. Specifically: the "already installed locally" marking was coupled to the team skills fetch, `stripFrontmatter` existed only in the UI package, error-handling patterns were repeated inline across many components, and the card row layout was copy-pasted across three different list surfaces.

## Changes

**Decouple local-install marking from the team skills fetch**
`listTeamSkills` no longer accepts `localSkillNames` and no longer marks skills as installed. A new pure function `markInstalledTeamSkills` handles that separately. `useTeamSkills` now fetches the team listing once and applies `markInstalledTeamSkills` in a `useMemo`, so local skill changes never trigger a refetch of the remote listing. The query key structure is also cleaned up with a shared `teamSkillsKeys.all` root so invalidations are consistent.

**Move `stripFrontmatter` to `@posthog/shared`**
The UI-local `stripFrontmatter.ts` is deleted. A CRLF-aware version now lives in `packages/shared/src/skills.ts` and is exported from the package index. All consumers (`SkillDetailPanel`, `MarketplaceSkillPanel`, `TeamSkillDetailPanel`, the workspace-server export path) import from `@posthog/shared`.

**Centralise skill error helpers**
A new `skillErrors.ts` module exposes `isSkillExistsError` and `skillErrorDescription`. All the scattered `error instanceof Error ? error.message : ""` / `.includes("already exists")` patterns across `SkillDetailPanel`, `TeamSkillDetailPanel`, `MarketplaceSkillPanel`, `SkillFileEditor`, `SkillManifestEditor`, `NewSkillDialog`, and `SkillFileEditor` are replaced with calls to these helpers.

**Extract `SkillListCard` shared component**
The identical card-row markup (icon box, title/subtitle text block, trailing slot, selection highlight) was duplicated in `SkillCard`, `TeamSkillsSection`, and `MarketplaceBrowse`. It is now a single `SkillListCard` component with `icon`, `title`, `subtitle`, `isSelected`, `onClick`, and `trailing` props.

**Extract `ReplaceSkillDialog` shared component**
The confirm-overwrite `AlertDialog` was duplicated in `MarketplaceSkillPanel`, `TeamSkillDetailPanel`, and `SkillDetailPanel`. It is now a single `ReplaceSkillDialog` component parameterised by `skillName`, `verb` ("Reinstalling" / "Importing"), and `onConfirm`.

**Consolidate `getUserSkillsDir` and `isProbablyText`**
Both helpers were defined inline in multiple workspace-server files. They now live in `skill-discovery.ts` and are imported wherever needed, removing the repeated `os.homedir()` path construction.

**Parallelise file I/O in the workspace server**
Sequential `for` loops in `mirrorUserSkillsToCodex`, `exportSkill`, and `installSkill` are replaced with `Promise.all` calls.

**Move `installsFormatter` to `useMarketplace.ts`**
The `Intl.NumberFormat` instance was duplicated in `MarketplaceBrowse` and `MarketplaceSkillPanel`; it is now exported once from `useMarketplace.ts`.

**Remove unused `resolveLlmSkill` API method**
`resolveLlmSkill` and its `LlmSkillResolveResponse` type are removed from the API client as they are no longer called anywhere.

## How did you test this?

- Updated unit tests for `TeamSkillsService.listTeamSkills` to reflect the removed `localSkillNames` parameter.
- Added a new `markInstalledTeamSkills` unit test covering name-based matching and unrelated local names.
- Existing tests for `publishSkill` and other service methods continue to pass.

## Automatic notifications

- [ ] Publish to changelog?
- [ ] Alert Sales and Marketing teams?

Author: k11kirky

packages/api-client/src/posthog-client.ts

@@ -189,18 +189,6 @@ export interface LlmSkillFileInput {
   content_type?: string;
 }
 
-export interface LlmSkillResolveResponse {
-  skill: LlmSkill;
-  versions: Array<{
-    id: string;
-    version: number;
-    created_by: LlmSkillCreatedBy | null;
-    created_at: string;
-    is_latest: boolean;
-  }>;
-  has_more: boolean;
-}
-
 export interface SignalSourceConfig {
   id: string;
   source_product:
@@ -3724,22 +3712,6 @@ export class PostHogAPIClient {
     return (await response.json()) as LlmSkill;
   }
 
-  /** Resolves a team skill plus its version history. */
-  async resolveLlmSkill(name: string): Promise<LlmSkillResolveResponse> {
-    const teamId = await this.getTeamId();
-    const urlPath = `/api/environments/${teamId}/llm_skills/resolve/name/${encodeURIComponent(name)}`;
-    const url = new URL(`${this.api.baseUrl}${urlPath}`);
-    const response = await this.api.fetcher.fetch({
-      method: "get",
-      url,
-      path: urlPath,
-    });
-    if (!response.ok) {
-      throw new Error(`Failed to resolve team skill: ${response.statusText}`);
-    }
-    return (await response.json()) as LlmSkillResolveResponse;
-  }
-
   /** Creates a brand-new team skill (version 1). */
   async createLlmSkill(input: {
     name: string;

packages/core/src/skills/teamSkillsService.test.ts

@@ -4,7 +4,10 @@ import type {
 } from "@posthog/api-client/posthog-client";
 import { describe, expect, it, vi } from "vitest";
 import type { SkillsWorkspaceClient } from "./teamSkillsService";
-import { TeamSkillsService } from "./teamSkillsService";
+import {
+  markInstalledTeamSkills,
+  TeamSkillsService,
+} from "./teamSkillsService";
 
 function makeService(
   workspace: Partial<SkillsWorkspaceClient> = {},
@@ -39,21 +42,18 @@ function makeClient(result: LlmSkillListItem[] | null): PostHogAPIClient {
 
 describe("TeamSkillsService.listTeamSkills", () => {
   it("reports the feature as unavailable when the API returns null", async () => {
-    const listing = await makeService().listTeamSkills(makeClient(null), []);
+    const listing = await makeService().listTeamSkills(makeClient(null));
 
     expect(listing).toEqual({ available: false, skills: [] });
   });
 
-  it("maps team skills and marks ones that exist locally", async () => {
+  it("maps team skills", async () => {
     const client = makeClient([
       makeItem({}),
       makeItem({ id: "skill-2", name: "release-notes", created_by: null }),
     ]);
 
-    const listing = await makeService().listTeamSkills(client, [
-      "release-notes",
-      "unrelated-local",
-    ]);
+    const listing = await makeService().listTeamSkills(client);
 
     expect(listing.available).toBe(true);
     expect(listing.skills).toEqual([
@@ -66,11 +66,7 @@ describe("TeamSkillsService.listTeamSkills", () => {
         createdByEmail: "dev@posthog.com",
         installedLocally: false,
       },
-      expect.objectContaining({
-        name: "release-notes",
-        createdByEmail: null,
-        installedLocally: true,
-      }),
+      expect.objectContaining({ name: "release-notes", createdByEmail: null }),
     ]);
   });
 
@@ -80,13 +76,34 @@ describe("TeamSkillsService.listTeamSkills", () => {
       makeItem({ id: "skill-1b", version: 2 }),
     ]);
 
-    const listing = await makeService().listTeamSkills(client, []);
+    const listing = await makeService().listTeamSkills(client);
 
     expect(listing.skills).toHaveLength(1);
     expect(listing.skills[0]?.id).toBe("skill-1b");
   });
 });
 
+describe("markInstalledTeamSkills", () => {
+  it("marks team skills that exist locally by name", async () => {
+    const listing = await makeService().listTeamSkills(
+      makeClient([
+        makeItem({}),
+        makeItem({ id: "skill-2", name: "release-notes" }),
+      ]),
+    );
+
+    const marked = markInstalledTeamSkills(listing, [
+      "release-notes",
+      "unrelated-local",
+    ]);
+
+    expect(marked.skills.map((s) => [s.name, s.installedLocally])).toEqual([
+      ["pr-shepherd", false],
+      ["release-notes", true],
+    ]);
+  });
+});
+
 describe("TeamSkillsService.publishSkill", () => {
   const exported = {
     name: "pr-shepherd",

packages/core/src/skills/teamSkillsService.ts

@@ -35,6 +35,24 @@ export interface SkillsWorkspaceClient {
   ): Promise<{ path: string }>;
 }
 
+/**
+ * Marks team skills that already exist locally by name. Pure, so callers
+ * can re-apply it when the local listing changes without refetching.
+ */
+export function markInstalledTeamSkills(
+  listing: TeamSkillsListing,
+  localSkillNames: string[],
+): TeamSkillsListing {
+  const localNames = new Set(localSkillNames);
+  return {
+    ...listing,
+    skills: listing.skills.map((skill) => ({
+      ...skill,
+      installedLocally: localNames.has(skill.name),
+    })),
+  };
+}
+
 @injectable()
 export class TeamSkillsService {
   constructor(
@@ -63,24 +81,18 @@ export class TeamSkillsService {
   }
 
   /**
-   * Lists team skills merged with the local listing: the availability
-   * decision (flag off → absent group, no errors) and the "already
-   * installed locally" marking both live here, so the UI keeps one hook.
+   * Lists the team's latest skills, owning the availability decision
+   * (flag off → absent group, no errors). Combine with
+   * markInstalledTeamSkills for the merged local+team view.
    */
-  async listTeamSkills(
-    client: PostHogAPIClient,
-    localSkillNames: string[],
-  ): Promise<TeamSkillsListing> {
+  async listTeamSkills(client: PostHogAPIClient): Promise<TeamSkillsListing> {
     const items = await client.listLlmSkills();
     if (items === null) {
       return { available: false, skills: [] };
     }
-    const localNames = new Set(localSkillNames);
     return {
       available: true,
-      skills: items
-        .filter((item) => item.is_latest)
-        .map((item) => toTeamSkillInfo(item, localNames)),
+      skills: items.filter((item) => item.is_latest).map(toTeamSkillInfo),
     };
   }
 
@@ -151,17 +163,14 @@ export class TeamSkillsService {
   }
 }
 
-function toTeamSkillInfo(
-  item: LlmSkillListItem,
-  localNames: Set<string>,
-): TeamSkillInfo {
+function toTeamSkillInfo(item: LlmSkillListItem): TeamSkillInfo {
   return {
     id: item.id,
     name: item.name,
     description: item.description,
     version: item.latest_version ?? item.version,
     updatedAt: item.updated_at,
     createdByEmail: item.created_by?.email ?? null,
-    installedLocally: localNames.has(item.name),
+    installedLocally: false,
   };
 }

packages/host-router/src/routers/skills.router.ts

@@ -63,11 +63,7 @@ export const skillsRouter = router({
     .mutation(({ ctx, input }) =>
       ctx.container
         .get<SkillsService>(SKILLS_SERVICE)
-        .saveSkillManifest(input.skillPath, {
-          name: input.name,
-          description: input.description,
-          body: input.body,
-        }),
+        .saveSkillManifest(input.skillPath, input),
     ),
   saveFile: publicProcedure
     .input(saveSkillFileInput)

packages/shared/src/index.ts

@@ -166,6 +166,7 @@ export type {
   SkillInfo,
   SkillSource,
 } from "./skills";
+export { SKILL_EXISTS_MARKER, stripFrontmatter } from "./skills";
 export type {
   ArtifactType,
   PostHogAPIConfig,

packages/shared/src/skills.ts

@@ -31,3 +31,19 @@ export interface ExportedSkill {
   body: string;
   files: ExportedSkillFile[];
 }
+
+/**
+ * Server "skill already exists" messages must include this marker verbatim;
+ * the UI keys its overwrite-confirmation flow on it.
+ */
+export const SKILL_EXISTS_MARKER = "already exists";
+
+/**
+ * Strips a leading YAML frontmatter block from a SKILL.md document.
+ * CRLF-aware so render (UI) and export (workspace-server) agree on the body.
+ */
+export function stripFrontmatter(content: string): string {
+  const match = content.match(/^---[ \t]*\r?\n[\s\S]*?\r?\n---[ \t]*\r?\n?/);
+  if (!match) return content;
+  return content.slice(match[0].length).replace(/^(?:[ \t]*\r?\n)+/, "");
+}

packages/ui/src/features/skills/MarketplaceBrowse.tsx

@@ -11,16 +11,14 @@ import {
 } from "@radix-ui/themes";
 import { useState } from "react";
 import { MarketplaceSkillPanel } from "./MarketplaceSkillPanel";
+import { SkillListCard } from "./SkillListCard";
 import { useSkillsSidebarStore } from "./skillsSidebarStore";
 import {
+  installsFormatter,
   type MarketplaceSkillSummary,
   useMarketplaceSearch,
 } from "./useMarketplace";
 
-const installsFormatter = new Intl.NumberFormat(undefined, {
-  notation: "compact",
-});
-
 export function MarketplaceBrowse() {
   const [query, setQuery] = useState("");
   const { debounced: debouncedQuery } = useDebouncedValue(query, 300);
@@ -68,52 +66,41 @@ export function MarketplaceBrowse() {
             ) : (
               <Flex direction="column" gap="1">
                 {results.map((result) => (
-                  <Flex
+                  <SkillListCard
                     key={result.id}
-                    align="center"
-                    gap="2"
-                    px="3"
-                    py="2"
-                    className={`cursor-pointer rounded-lg border transition-colors ${
-                      selected?.id === result.id
-                        ? "border-accent-8 bg-accent-3"
-                        : "border-gray-6 bg-gray-2 hover:border-gray-8 hover:bg-gray-3"
-                    }`}
-                    onClick={() =>
-                      setSelected((prev) =>
-                        prev?.id === result.id ? null : result,
-                      )
-                    }
-                  >
-                    <Box className="flex shrink-0 items-center justify-center rounded bg-gray-4 p-1.5">
+                    icon={
                       <Storefront
                         size={14}
                         weight="duotone"
                         className="text-gray-11"
                       />
-                    </Box>
-                    <Flex direction="column" gap="0" className="min-w-0 flex-1">
-                      <Text className="truncate font-medium text-[13px] text-gray-12">
-                        {result.name}
-                      </Text>
-                      <Text className="truncate text-[12px] text-gray-10">
-                        {result.source}
-                      </Text>
-                    </Flex>
-                    {result.installed && (
-                      <Badge
-                        size="1"
-                        variant="soft"
-                        color="green"
-                        className="shrink-0"
-                      >
-                        Installed
-                      </Badge>
-                    )}
-                    <Text className="shrink-0 text-[12px] text-gray-9 tabular-nums">
-                      {installsFormatter.format(result.installs)}
-                    </Text>
-                  </Flex>
+                    }
+                    title={result.name}
+                    subtitle={result.source}
+                    isSelected={selected?.id === result.id}
+                    onClick={() =>
+                      setSelected((prev) =>
+                        prev?.id === result.id ? null : result,
+                      )
+                    }
+                    trailing={
+                      <>
+                        {result.installed && (
+                          <Badge
+                            size="1"
+                            variant="soft"
+                            color="green"
+                            className="shrink-0"
+                          >
+                            Installed
+                          </Badge>
+                        )}
+                        <Text className="shrink-0 text-[12px] text-gray-9 tabular-nums">
+                          {installsFormatter.format(result.installs)}
+                        </Text>
+                      </>
+                    }
+                  />
                 ))}
               </Flex>
             )}