From d8f0e772e9ac43bdd5bb22acc620d35b6e9311ab Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 00:47:07 -0400 Subject: [PATCH] fix: seed the section state on mount, and drop a dead dynamic branch (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4. Both findings arrived in the review's *suppressed* block with zero inline comments, so a round that reported "0 new comments" was in fact carrying two real defects. `useValueChange` deliberately does not fire on the first render, so the clean-entry auto-collapse it drives never applied to a screen that MOUNTS with a skill already selected — a restored `SkillsUiState` does exactly that, and Conformance would sit expanded until some later selection change started honouring the rule. The rule was written entirely in the change handler and the initial state was never considered. `openSections` now seeds through `initialOpenSections`, applying the same judgement up front while the render-time synchronisation continues to handle every subsequent change. The Resources panel still held `selected.resources === DYNAMIC_RESOURCES ? … : …` inside a `{!isDynamic && …}` guard, and `isDynamic` is that exact comparison, so the alert arm was unreachable. It is residue from moving the dynamic banner into Conformance: the outer guard was added and the inner ternary never removed. The manifest table now renders directly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 31 +++ .../screens/SkillsScreen/SkillsScreen.tsx | 243 ++++++++++-------- 2 files changed, 161 insertions(+), 113 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index e947e3ba..ca0be00a 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -218,6 +218,37 @@ describe("SkillsScreen", () => { expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); }); + it("collapses Conformance for a clean skill selected BEFORE mount", () => { + // `useValueChange` deliberately does not fire on the first render, so the + // auto-collapse it drives cannot cover a screen that mounts with a skill + // already chosen — a restored `SkillsUiState` does exactly that. The + // initialiser has to apply the same rule, or the behaviour only starts + // working after some later selection change (#2263). + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + }); + + it("opens Conformance for a skill WITH findings selected before mount", () => { + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + }); + it("opens Conformance for an entry that has findings", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 3a8125d4..875af397 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -366,6 +366,31 @@ const SkillTitle = Text.withProps({ */ const SECTION_FLEX = "0 0 auto"; +/** Every section this screen can render, in display order. */ +const ALL_SECTIONS = ["conformance", "resources", "frontmatter", "resource"]; + +/** + * The open set for the FIRST render. + * + * `useValueChange` deliberately does not fire on the first render, so the + * clean-entry auto-collapse it drives cannot cover a screen that mounts with a + * skill already selected — a restored `SkillsUiState` does exactly that. This + * applies the same rule up front, so the behaviour does not depend on a later + * selection change to start working. + */ +function initialOpenSections( + skills: SkillEntry[], + selectedSkillUri: string | undefined, +): string[] { + if (selectedSkillUri === undefined) return ALL_SECTIONS; + const wanted = skillUriIdentity(selectedSkillUri); + const entry = skills.find((skill) => skillUriIdentity(skill.uri) === wanted); + if (entry === undefined) return ALL_SECTIONS; + return checkSkillConformance(entry).length > 0 + ? ALL_SECTIONS + : ALL_SECTIONS.filter((section) => section !== "conformance"); +} + /** * The file viewer's flex, which is deliberately NOT `sectionFlex`. * @@ -539,12 +564,15 @@ export function SkillsScreen({ // carries the whole answer; nothing else here can be summarised by its header, // so opening collapsed would just hide content behind a click the user has no // reason to expect. - const [openSections, setOpenSections] = useState([ - "conformance", - "resources", - "frontmatter", - "resource", - ]); + // + // The initialiser has to make that judgement too, not just the + // `useValueChange` below: that hook deliberately does not fire on the first + // render, so a screen MOUNTING on an already-selected clean skill — a + // restored `SkillsUiState`, say — would otherwise show Conformance expanded + // and only start honouring the rule after some later selection change. + const [openSections, setOpenSections] = useState(() => + initialOpenSections(skills, ui.selectedSkillUri), + ); // Monotonic attempt token, shared by every on-demand action here: a manifest // row's verification, the SKILL.md preview, and the `skills/get` fetch. One // counter rather than three because it only has to be *increasing*, and each @@ -1308,114 +1336,103 @@ export function SkillsScreen({ - {selected.resources === DYNAMIC_RESOURCES ? ( - - This skill declares{" "} - resources: "dynamic" — its - files are generated, so no manifest is advertised and - integrity cannot be verified. - - ) : ( - - - - URI - Size - Digest - Verification - {/* The action gets its own column so the buttons - line up down the table. Sharing a cell with - the verdict badge staggered them, because the - badge's width tracks its label — "—", - "checking…", "verified" and "mismatch" are all - different sizes. + + + + URI + Size + Digest + Verification + {/* The action gets its own column so the buttons + line up down the table. Sharing a cell with + the verdict badge staggered them, because the + badge's width tracks its label — "—", + "checking…", "verified" and "mismatch" are all + different sizes. - The header is named for screen readers but not - shown: a visible label over a column of - buttons is noise, while an *empty* `th` is an - axe `empty-table-header` violation and leaves - the column unnamed in a table's header - navigation. */} - - Actions - - - - - {manifest.map((resource, index) => { - const state = fileStates[index]; - const color = - state?.status === "done" - ? verificationColor(state.verification.status) - : state?.status === "error" - ? "red" - : "gray"; - // Compared by identity for the same reason every - // other URI comparison here is: a server that - // canonicalizes an escape is naming the same - // file, and the row the user just clicked must - // not read as unselected because of a spelling. - const showing = - previewUri !== undefined && - skillUriIdentity(resource.uri) === - skillUriIdentity(previewUri); - return ( - // Index-keyed for the same reason the verdicts - // are: a duplicated URI is a case this screen - // reports, so it must not also collide two rows - // into one. - - - - showResource(resource.uri, manifestKey) - } - > - {resource.uri} - - - {resource.size ?? "—"} - - {shortDigest(resource.digest)} - - - - {verificationLabel(state)} - - - - - void verifyRow( - index, - resource, - manifestKey, - ) - } - > - Verify - - - - ); - })} - - - )} + The header is named for screen readers but not + shown: a visible label over a column of + buttons is noise, while an *empty* `th` is an + axe `empty-table-header` violation and leaves + the column unnamed in a table's header + navigation. */} + + Actions + + + + + {manifest.map((resource, index) => { + const state = fileStates[index]; + const color = + state?.status === "done" + ? verificationColor(state.verification.status) + : state?.status === "error" + ? "red" + : "gray"; + // Compared by identity for the same reason every + // other URI comparison here is: a server that + // canonicalizes an escape is naming the same + // file, and the row the user just clicked must + // not read as unselected because of a spelling. + const showing = + previewUri !== undefined && + skillUriIdentity(resource.uri) === + skillUriIdentity(previewUri); + return ( + // Index-keyed for the same reason the verdicts + // are: a duplicated URI is a case this screen + // reports, so it must not also collide two rows + // into one. + + + + showResource(resource.uri, manifestKey) + } + > + {resource.uri} + + + {resource.size ?? "—"} + + {shortDigest(resource.digest)} + + + + {verificationLabel(state)} + + + + + void verifyRow( + index, + resource, + manifestKey, + ) + } + > + Verify + + + + ); + })} + +