feat(FN-1986): improve skills content loading and mobile detail UX

- Normalize GET /api/skills/:id/content handling by decoding encoded IDs and returning consistent invalid-id and not-found responses - Update readSkillContent to use stat-based file vs directory detection, read SKILL.md robustly, and list non-SKILL.md entries - Refine SkillsView interactions to ignore toggle clicks, support explicit retry of failed content fetches, and show a SKILL.md fallback placeholder - Add/adjust dashboard route, component, and mobile CSS tests to cover content route behavior, retry flow, and touch-friendly detail panel styles
2026-04-18 14:09:47 -07:00
parent 7affac3d4b
commit 5499405fc9
7 changed files with 290 additions and 165 deletions
--- a/packages/dashboard/src/tests/routes-skills.test.ts
+++ b/packages/dashboard/src/tests/routes-skills.test.ts
@@ -130,6 +130,23 @@ function createMockSkillsAdapter(overrides?: Partial<SkillsAdapter>): SkillsAdap
        fallbackUsed: false,
      },
    }),
+    readSkillContent: vi.fn().mockImplementation(async (_rootDir: string, skillId: string) => {
+      if (skillId === "npm::skills/nonexistent") {
+        throw new Error(`Skill not found: ${skillId}`);
+      }
+      if (skillId === "invalid") {
+        throw new Error(`Invalid skill ID format: ${skillId}`);
+      }
+
+      return {
+        name: "example/SKILL.md",
+        skillMd: "# Example Skill\n\nDetails here.",
+        files: [
+          { name: "references", relativePath: "references", type: "directory" as const },
+          { name: "notes.txt", relativePath: "notes.txt", type: "file" as const },
+        ],
+      };
+    }),
    ...overrides,
  };
 }
@@ -200,6 +217,78 @@ describe("Skills routes", () => {
    });
  });

+  describe("GET /api/skills/:id/content", () => {
+    it("returns skill content for a valid skill ID", async () => {
+      const mockAdapter = createMockSkillsAdapter();
+      const store = new MockStore();
+      const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
+
+      const skillId = "npm%253A%2540example%252Fskill%3A%3Askills%2Fexample%2FSKILL.md";
+      const res = await request(app, "GET", `/api/skills/${skillId}/content`);
+
+      expect(res.status).toBe(200);
+      expect(res.body).toEqual({
+        content: {
+          name: "example/SKILL.md",
+          skillMd: "# Example Skill\n\nDetails here.",
+          files: [
+            { name: "references", relativePath: "references", type: "directory" },
+            { name: "notes.txt", relativePath: "notes.txt", type: "file" },
+          ],
+        },
+      });
+      expect(mockAdapter.readSkillContent).toHaveBeenCalledWith(
+        "/tmp/fn-skills",
+        "npm:@example/skill::skills/example/SKILL.md",
+      );
+    });
+
+    it("returns 404 when skills adapter is not configured", async () => {
+      const store = new MockStore();
+      const app = createServer(store as any, {});
+
+      const res = await request(app, "GET", "/api/skills/npm%253A%2540example%252Fskill%3A%3Askills%2Fexample%2FSKILL.md/content");
+
+      expect(res.status).toBe(404);
+      expect(res.body).toEqual({
+        error: "Skills adapter not configured",
+        code: "adapter_not_configured",
+      });
+    });
+
+    it("returns 404 for non-existent skill", async () => {
+      const mockAdapter = createMockSkillsAdapter({
+        readSkillContent: vi.fn().mockRejectedValue(new Error("Skill not found: npm::skills/nonexistent")),
+      });
+      const store = new MockStore();
+      const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
+
+      const res = await request(app, "GET", "/api/skills/npm%253A%253Askills%252Fnonexistent/content");
+
+      expect(res.status).toBe(404);
+      expect(res.body).toEqual({
+        error: "Skill not found",
+        code: "skill_not_found",
+      });
+    });
+
+    it("returns 400 for invalid skill IDs", async () => {
+      const mockAdapter = createMockSkillsAdapter({
+        readSkillContent: vi.fn().mockRejectedValue(new Error("Invalid skill ID format: invalid")),
+      });
+      const store = new MockStore();
+      const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
+
+      const res = await request(app, "GET", "/api/skills/invalid/content");
+
+      expect(res.status).toBe(400);
+      expect(res.body).toEqual({
+        error: "Invalid skill ID format: invalid",
+        code: "invalid_skill_id",
+      });
+    });
+  });
+
  describe("PATCH /api/skills/execution", () => {
    it("toggles skill execution successfully", async () => {
      const mockAdapter = createMockSkillsAdapter();
--- a/packages/dashboard/src/routes.ts
+++ b/packages/dashboard/src/routes.ts
@@ -17332,6 +17332,53 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
    }
  });

+  /**
+   * GET /api/skills/:id/content
+   * Read the contents of a skill's SKILL.md file and list supplementary files.
+   * Params: id (URL-encoded skill ID)
+   * Query: projectId (optional) for multi-project context
+   * Response: { content: SkillContent }
+   * Error: 404 { error: string; code: "skill_not_found" | "adapter_not_configured" }
+   */
+  router.get("/skills/:id/content", async (req, res) => {
+    try {
+      const scopedStore = await getScopedStore(req);
+      const skillsAdapter = options?.skillsAdapter;
+
+      if (!skillsAdapter) {
+        res.status(404).json({ error: "Skills adapter not configured", code: "adapter_not_configured" });
+        return;
+      }
+
+      const encodedSkillId = req.params.id as string;
+      let skillId = encodedSkillId;
+      try {
+        skillId = decodeURIComponent(encodedSkillId);
+      } catch {
+        res.status(400).json({ error: "Invalid skill ID", code: "invalid_skill_id" });
+        return;
+      }
+
+      const rootDir = scopedStore.getRootDir();
+      const content = await skillsAdapter.readSkillContent(rootDir, skillId);
+
+      res.json({ content });
+    } catch (err: unknown) {
+      if (err instanceof ApiError) {
+        throw err;
+      }
+      if (err instanceof Error && err.message.includes("Skill not found")) {
+        res.status(404).json({ error: "Skill not found", code: "skill_not_found" });
+        return;
+      }
+      if (err instanceof Error && err.message.includes("Invalid skill ID")) {
+        res.status(400).json({ error: err.message, code: "invalid_skill_id" });
+        return;
+      }
+      rethrowAsApiError(err, "Failed to read skill content");
+    }
+  });
+
  /**
   * PATCH /api/skills/execution
   * Toggle a skill's enabled/disabled state.
@@ -17431,48 +17478,6 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
    }
  });

-  /**
-   * GET /api/skills/:id/content
-   * Read the contents of a skill's SKILL.md file and list supplementary files.
-   * Params: id (URL-encoded skill ID)
-   * Query: projectId (optional) for multi-project context
-   * Response: { content: SkillContent }
-   * Error: 404 { error: string; code: "skill_not_found" | "adapter_not_configured" }
-   */
-  router.get("/skills/:id/content", async (req, res) => {
-    try {
-      const scopedStore = await getScopedStore(req);
-      const skillsAdapter = options?.skillsAdapter;
-
-      if (!skillsAdapter) {
-        res.status(404).json({ error: "Skills adapter not configured", code: "adapter_not_configured" });
-        return;
-      }
-
-      const rootDir = scopedStore.getRootDir();
-      const skillId = req.params.id as string;
-
-      const content = await skillsAdapter.readSkillContent(rootDir, skillId);
-
-      res.json({ content });
-    } catch (err: unknown) {
-      if (err instanceof ApiError) {
-        throw err;
-      }
-      if (err instanceof Error) {
-        if (err.message.includes("Invalid skill ID")) {
-          res.status(400).json({ error: err.message, code: "invalid_skill_id" });
-          return;
-        }
-        if (err.message.includes("Skill not found")) {
-          res.status(404).json({ error: err.message, code: "skill_not_found" });
-          return;
-        }
-      }
-      rethrowAsApiError(err, "Failed to read skill content");
-    }
-  });
-
  // ── Remote Node Proxy Routes ───────────────────────────────────────────

  /** GET /api/proxy/:nodeId/health — Forward health check to remote node */
--- a/packages/dashboard/src/skills-adapter.ts
+++ b/packages/dashboard/src/skills-adapter.ts
@@ -5,8 +5,7 @@
 * by integrating with the pi-coding-agent package manager and skills.sh API.
 */

-import { access } from "node:fs/promises";
-import { readFile, writeFile, mkdir, readdir } from "node:fs/promises";
+import { access, readFile, writeFile, mkdir, readdir, stat } from "node:fs/promises";
 import { join, relative, dirname } from "node:path";

 /**
@@ -479,69 +478,44 @@ export function createSkillsAdapter(options: {
    },

    async readSkillContent(rootDir: string, skillId: string): Promise<SkillContent> {
-      // Parse the skill ID to get source and relativePath
      const parsed = parseSkillId(skillId);
      if (!parsed) {
        throw new Error(`Invalid skill ID format: ${skillId}`);
      }

-      // Find the skill in discovered skills to get its path
      const discovered = await this.discoverSkills(rootDir);
-      const skill = discovered.find((s) => s.id === skillId);
+      const skill = discovered.find((entry) => entry.id === skillId);
      if (!skill) {
        throw new Error(`Skill not found: ${skillId}`);
      }

-      // Determine the skill directory
-      // If path points to a file (e.g., SKILL.md), use dirname(path)
-      // If path points to a directory, use it directly
      let skillDir = skill.path;
      try {
-        const stat = await access(skill.path);
-        // Path exists, check if it's a file or directory
-        // We can't easily check with access(), so we try to read as file first
-        try {
-          await readFile(skill.path, "utf-8");
-          // It's a file, get the directory
-          skillDir = dirname(skill.path);
-        } catch {
-          // Not a file (might be a directory or error), use path as-is
-          skillDir = skill.path;
-        }
+        const skillPathStat = await stat(skill.path);
+        skillDir = skillPathStat.isFile() ? dirname(skill.path) : skill.path;
      } catch {
-        // Path doesn't exist, use dirname as fallback
        skillDir = dirname(skill.path);
      }

-      // Read SKILL.md from the skill directory
      const skillMdPath = join(skillDir, "SKILL.md");
      let skillMd = "";
-      if (await pathExists(skillMdPath)) {
-        try {
-          skillMd = await readFile(skillMdPath, "utf-8");
-        } catch {
-          // Ignore read errors, keep empty string
+      try {
+        skillMd = await readFile(skillMdPath, "utf-8");
+      } catch (error) {
+        const err = error as NodeJS.ErrnoException;
+        if (err.code !== "ENOENT") {
+          throw error;
        }
      }

-      // List files in the skill directory (non-recursive for MVP)
-      const files: SkillFileEntry[] = [];
-      try {
-        const entries = await readdir(skillDir, { withFileTypes: true });
-        for (const entry of entries) {
-          // Skip SKILL.md as it's shown separately
-          if (entry.name === "SKILL.md") {
-            continue;
-          }
-          files.push({
-            name: entry.name,
-            relativePath: entry.name,
-            type: entry.isDirectory() ? "directory" : "file",
-          });
-        }
-      } catch {
-        // Ignore readdir errors, return empty files array
-      }
+      const entries = await readdir(skillDir, { withFileTypes: true }).catch(() => []);
+      const files: SkillFileEntry[] = entries
+        .filter((entry) => entry.name !== "SKILL.md")
+        .map((entry) => ({
+          name: entry.name,
+          relativePath: entry.name,
+          type: entry.isDirectory() ? "directory" : "file",
+        }));

      return {
        name: skill.name,