From c3d9c665a6c4d2d45c98aeb7833367d694a14cdb Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 10:07:22 -0400 Subject: [PATCH] fix: carry the value convention to the badges, and pin the layout it claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1 on #2329. Four findings, all correct. The whole-modal claim was false for the three rows whose value is a Badge: `ThemeBadge` defaults to `fw: 600`, so Transport, Era and Status still rendered bold-label/bold-value. A `ValueBadge` constant carries the normal weight for Transport and Status, and `EraBadge` takes an optional `fw` so Connection Info can ask for it without changing the standalone chip in ProtocolListPanel. The chip still reads as a chip — its emphasis is the outline and colour, not the font weight. Three test weaknesses, each of which let a regression through: - The weight assertion used `queryByText(...)?.style.fontWeight ?? ""`, which passes vacuously when the row is absent entirely. Now `getByText`, and deleting a value row fails three tests instead of none. - The full-width test asserted only the transparent background, so swapping `FullWidthField` back for the two-column `SimpleGrid` would still have passed. It now asserts the structure: label and value are siblings, and the value is in no SimpleGrid. - `OAuthTokenField`'s bold caption was asserted nowhere — the weight test renders no token. A case now covers Access Token and ID Token through it. Note for anyone asserting a Badge's weight later: `getByText` lands on the inner label span and `fw` is applied to the root, so the assertion has to walk up or it reads an empty string and passes against anything. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB Signed-off-by: cliffhall --- .../components/elements/EraBadge/EraBadge.tsx | 12 +++- .../ConnectionInfoContent.test.tsx | 55 ++++++++++++++++++- .../ConnectionInfoContent.tsx | 18 +++--- 3 files changed, 75 insertions(+), 10 deletions(-) diff --git a/clients/web/src/components/elements/EraBadge/EraBadge.tsx b/clients/web/src/components/elements/EraBadge/EraBadge.tsx index 37c4ebe4..d3caa1d0 100644 --- a/clients/web/src/components/elements/EraBadge/EraBadge.tsx +++ b/clients/web/src/components/elements/EraBadge/EraBadge.tsx @@ -5,14 +5,22 @@ import { formatEra, isModernEra } from "./eraUtils"; export interface EraBadgeProps { /** The negotiated protocol era; `undefined` renders as Legacy. */ era: ProtocolEra | undefined; + /** + * Font weight override. The app-wide `ThemeBadge` defaults to `fw: 600`, + * which is right where this badge is a standalone chip (ProtocolListPanel's + * header). Connection Info renders it as the *value* half of a label/value + * row, where the convention is bold label / normal value, so it passes 400. + * Left undefined the theme default applies (#2328). + */ + fw?: number; } // Labels a connection's negotiated protocol era (SEP §7.8). Feed it from // connection state only — see the note in `eraUtils` on why the era must never // be inferred from individual message frames. -export function EraBadge({ era }: EraBadgeProps) { +export function EraBadge({ era, fw }: EraBadgeProps) { return ( - + {formatEra(era)} ); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 1a525839..ba4fb132 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -590,8 +590,22 @@ describe("ConnectionInfoContent", () => { for (const label of ["Name", "Protocol", "Client ID", "Scopes"]) { expect(screen.getAllByText(label)[0]).toHaveStyle({ fontWeight: "600" }); } + // `getByText`, not `queryByText` + `?? ""`: an absent value would make the + // optional form pass vacuously (undefined → "" → not "600"), so the test + // would go green on a row that had stopped rendering at all. for (const value of ["Everything Server", "read"]) { - expect(screen.queryByText(value)?.style.fontWeight ?? "").not.toBe("600"); + expect(screen.getByText(value).style.fontWeight).not.toBe("600"); + } + + // Badge values count too. `ThemeBadge` defaults to `fw: 600`, so Status, + // Transport and Era read bold-label/bold-value unless overridden — the + // whole-modal claim is false without this. + for (const badge of ["streamable-http", "Legacy", "Authorized"]) { + // `getByText` lands on the Badge's inner label span; `fw` is applied to + // the root, so walk up to it or the assertion reads an empty string and + // passes against anything. + const root = screen.getByText(badge).closest('[class*="Badge-root"]'); + expect((root as HTMLElement | null)?.style.fontWeight).toBe("400"); } }); @@ -623,6 +637,45 @@ describe("ConnectionInfoContent", () => { backgroundColor: "transparent", }); } + + // The background alone does not pin the *layout*: swapping FullWidthField + // back for the two-column SimpleGrid would keep it transparent and still + // pass. Assert the structure that makes the value full width — label and + // value are siblings in a column, and neither sits in a SimpleGrid. + for (const [label, value] of [ + [ + "Client ID", + "http://127.0.0.1:8093/client-metadata.json?profile=long-enough-to-wrap", + ], + ["Auth URL", "https://auth.example.com/authorize"], + ]) { + const labelNode = screen.getByText(label); + const valueNode = screen.getByText(value); + expect(valueNode.parentElement).toBe(labelNode.parentElement); + expect(valueNode.closest('[class*="SimpleGrid"]')).toBeNull(); + } + }); + + it("bolds the token captions, which are field labels in the same list", () => { + renderWithMantine( + , + ); + + // OAuthTokenField renders its own caption, so the weight convention has to + // be asserted through it — the other weight test renders no token at all. + for (const caption of ["Access Token", "ID Token"]) { + expect(screen.getByText(caption).style.fontWeight).toBe("600"); + } }); it("groups the full-width fields at the end, Client ID directly above Access Token", () => { diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx index 447f1f79..1c04c951 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -92,6 +92,13 @@ const ValueText = Text.withProps({ size: "sm", }); +// A badge standing in as the *value* half of a label/value row. The app-wide +// `ThemeBadge` defaults to `fw: 600`, which is right for a standalone chip but +// makes these rows read bold-label/bold-value — the one convention this modal +// is not supposed to have. The chip still reads as a chip: its emphasis comes +// from the outline and colour, not the font weight (#2328). +const ValueBadge = Badge.withProps({ variant: "outline", fw: 400 }); + // Shown for Name/Version when the server didn't report `serverInfo` — an em dash // plus an explicit note so the client-side catalog fallback is never mistaken // for a value the server sent. Exported so tests/stories assert against it @@ -331,10 +338,10 @@ export function ConnectionInfoContent({ {protocolVersion || "—"} Transport - {transport} + {transport} Era - + Session {formatSession(protocolEra, transport)} @@ -452,12 +459,9 @@ export function ConnectionInfoContent({ {formatProtocol(oauth.protocol)} Status - + {oauth.authorized ? "Authorized" : "Not authorized"} - + {oauth.clientRegistrationKind && (