From 10c8f5deb36e6724f4b255853b8b58e50bfb761e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 02:40:24 -0400 Subject: [PATCH] fix: address Copilot review round 10 (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero inline comments, three findings in the review's suppressed block — the second round where the inline count alone would have ended the cycle. All three are places where my own words did not match the code. The Frontmatter section claimed to show "the raw YAML the server served, not a re-serialised object". It did not: `ContentViewer`'s plain-text branch pretty-prints anything that parses as JSON, and every JSON mapping is also valid YAML — so `{"name":"a"}` came back re-serialised beneath a comment promising verbatim bytes. For a conformance tool that is the one thing it must not do. It now renders through `CodeHighlight` directly. The green alert was titled "Conforms". Consolidating every verdict into this one section means it can sit directly above a red digest mismatch, where an unqualified "Conforms" contradicts the result on screen — the static checks passing says nothing about the bytes. Retitled "No structural issues", which is what it actually summarises. The contradiction was created by the consolidation and the title was never revisited. And the PR description said the shield stays on both integrity-checking controls, while the per-row Verify rendered text only. The claim was right and the code was not, so the row button now carries it too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.stories.tsx | 2 +- .../SkillsScreen/SkillsScreen.test.tsx | 4 +-- .../screens/SkillsScreen/SkillsScreen.tsx | 35 ++++++++++++------- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index 2c480950..bfe69899 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -132,7 +132,7 @@ export const ConformingSkill: Story = { const control = canvas.getByRole("button", { name: /Conformance/ }); await expect(control).toHaveAttribute("aria-expanded", "false"); await userEvent.click(control); - await expect(canvas.getByText("Conforms")).toBeInTheDocument(); + await expect(canvas.getByText("No structural issues")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 7c5843da..9513bf70 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -211,10 +211,10 @@ describe("SkillsScreen", () => { await user.click(screen.getByText("data-analysis")); const control = screen.getByRole("button", { name: /Conformance/ }); expect(control).toHaveAttribute("aria-expanded", "false"); - expect(screen.queryByText("Conforms")).not.toBeInTheDocument(); + expect(screen.queryByText("No structural issues")).not.toBeInTheDocument(); await user.click(control); - expect(screen.getByText("Conforms")).toBeInTheDocument(); + expect(screen.getByText("No structural issues")).toBeInTheDocument(); expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); }); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index cb72458d..47101900 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -33,6 +33,7 @@ import { type SkillIssue, type SkillVerification, } from "@inspector/core/mcp/skills.js"; +import { CodeHighlight } from "../../elements/CodeHighlight/CodeHighlight"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { ListToggle } from "../../elements/ListToggle/ListToggle"; import { useValueChange } from "../../../hooks/useValueChange"; @@ -372,9 +373,13 @@ const InlineRow = Group.withProps({ wrap: "nowrap", }); +// The per-row integrity check. Carries the same shield as "Verify all" — both +// verify digests, and giving only one of them the icon made the icon look +// decorative rather than meaningful. const RowVerifyButton = Button.withProps({ variant: "subtle", size: "compact-xs", + leftSection: , }); // The action column: sized to its button and right-aligned, so the buttons form @@ -1270,7 +1275,12 @@ export function SkillsScreen({ )} {issues.length === 0 ? ( - + // Titled for the check it actually summarises. Now that + // every verdict renders in this one section, an + // unqualified "Conforms" sits directly above a red digest + // mismatch and flatly contradicts it — the static checks + // passing says nothing about the bytes. + No structural issues found in this entry. ) : ( @@ -1571,17 +1581,18 @@ export function SkillsScreen({ - {/* The raw YAML the server served, not a re-serialised - object: this app carries no YAML parser, and for a - conformance tool the bytes on the wire are the more - useful answer anyway. */} -