fix: address Copilot review round 10 (#2263)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-06 02:40:24 -04:00
commit 10c8f5deb3
3 changed files with 26 additions and 15 deletions

View file

@ -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();
},
};

View file

@ -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();
});

View file

@ -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: <MdVerifiedUser aria-hidden size={12} />,
});
// The action column: sized to its button and right-aligned, so the buttons form
@ -1270,7 +1275,12 @@ export function SkillsScreen({
</Alert>
)}
{issues.length === 0 ? (
<Alert color="green" title="Conforms">
// 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.
<Alert color="green" title="No structural issues">
No structural issues found in this entry.
</Alert>
) : (
@ -1571,17 +1581,18 @@ export function SkillsScreen({
</Accordion.Control>
<Accordion.Panel tabIndex={0}>
<FramedContent>
{/* 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. */}
<ContentViewer
block={{
type: "text",
text: previewParts.frontmatter,
}}
mimeType="text/yaml"
copyable
{/* `CodeHighlight` directly, NOT `ContentViewer`.
The promise this section makes is the bytes the server
served, and `ContentViewer`'s plain-text branch
pretty-prints anything that parses as JSON — which
every JSON mapping is also valid YAML, so a
frontmatter of `{"name":"a"}` came back re-serialised
while the section claimed to be showing it verbatim.
For a conformance tool that is the one thing this must
not do. */}
<CodeHighlight
language="yaml"
code={previewParts.frontmatter}
/>
</FramedContent>
</Accordion.Panel>