From 82b13a1a127d9f070d7056e3ed19de0d896a4c13 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 09:30:17 -0400 Subject: [PATCH 1/3] fix: bold the labels, not the values, and give Connection Info's URLs full width MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connection Info rendered every label/value row as a plain label against a bold value. That is backwards for a reference panel: the label is the fixed scaffolding a reader scans down, the value is what differs per connection, and bolding every value means nothing stands out. Reversed across the whole modal — Server Implementation, Discovery and OAuth Details, plus the token captions — rather than in one section, since two halves disagreeing is worse than either convention. Client ID and Auth URL were a Code block in the right half of a two-column grid, so a long value got roughly half the modal width. A CIMD client id IS a URL, so it is long by construction, and it broke mid-token — `…/client-metadata.` / `json` — which reads as a rendering fault rather than as one value. Both now put their label on its own line with the value full width beneath, the way OAuthTokenField already lays out a token, and neither carries an inset surface any more: the grey framed a value that is simply the answer to "Client ID", not a quoted blob needing separation from prose. The transparent treatment is a new `wrapping-plain` theme variant rather than a change to `wrapping`. That one is shared with ElicitationUrlPanel, ListLoadError, MalformedItemsWarning and the CSV fallback, where the inset is doing real work; a test asserts `wrapping` keeps its surface so those consumers cannot be changed by accident. Closes #2328 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB Signed-off-by: cliffhall --- .../ConnectionInfoContent.test.tsx | 58 ++++++++++++++++++ .../ConnectionInfoContent.tsx | 55 ++++++++++------- .../ConnectionInfoContent/OAuthTokenField.tsx | 5 +- clients/web/src/theme/Code.test.tsx | 60 +++++++++++++++++++ clients/web/src/theme/Code.ts | 12 +++- 5 files changed, 168 insertions(+), 22 deletions(-) create mode 100644 clients/web/src/theme/Code.test.tsx diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index d181c36a..53b225e1 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -567,6 +567,64 @@ describe("ConnectionInfoContent", () => { expect(screen.getByText("token-123")).toBeInTheDocument(); }); + it("weights labels bold and values normal, in both halves of the modal", () => { + renderWithMantine( + , + ); + + // The convention is the whole point of #2328: the label is the fixed + // scaffolding a reader scans down, the value is what differs. Asserted in + // Server Implementation *and* OAuth Details, because the defect being + // guarded against is the two halves disagreeing — checking one alone would + // pass on a modal that is internally inconsistent. + for (const label of ["Name", "Protocol", "Client ID", "Scopes"]) { + expect(screen.getAllByText(label)[0]).toHaveStyle({ fontWeight: "600" }); + } + for (const value of ["Everything Server", "read"]) { + expect(screen.queryByText(value)?.style.fontWeight ?? "").not.toBe("600"); + } + }); + + it("gives Client ID and Auth URL the full width, with no inset surface", () => { + renderWithMantine( + , + ); + + // A CIMD client id IS a URL, so it is long by construction. In the + // two-column grid it got half the modal and broke mid-token + // (`…/client-metadata.` / `json`), which reads as a rendering fault rather + // than as one value. + for (const value of [ + "http://127.0.0.1:8093/client-metadata.json?profile=long-enough-to-wrap", + "https://auth.example.com/authorize", + ]) { + expect(screen.getByText(value)).toHaveStyle({ + backgroundColor: "transparent", + }); + } + }); + it("renders EMA idp session when provided", () => { renderWithMantine( void; } -const ValueText = Text.withProps({ +// Label/value pairs in this modal read label-bold, value-normal: the label is +// the fixed scaffolding a reader scans down, and the value is the thing that +// differs per connection. The reverse (which this was until #2328) bolded every +// answer, so nothing stood out and the two columns fought each other. +const FieldLabel = Text.withProps({ size: "sm", fw: 600, }); +const ValueText = Text.withProps({ + size: "sm", +}); + // 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 @@ -104,7 +112,14 @@ const SectionHeading = Title.withProps({ // `Code` block — that keeps the whole value visible and removes a scroll region // that would otherwise need its own keyboard access (axe // `scrollable-region-focusable`). -const ValueCode = Code.withProps({ variant: "wrapping" }); +const ValueCode = Code.withProps({ variant: "wrapping-plain" }); + +// A long OAuth value (client id, auth URL) gets its label on its own line and +// the value across the full modal width beneath it, the way `OAuthTokenField` +// already lays out a token. In the two-column grid these values had roughly +// half the width and wrapped mid-token — `…/client-metadata.` / `json` — which +// reads as a rendering fault rather than as one URL (#2328). +const FullWidthField = Stack.withProps({ gap: 4 }); // One declared sub-option of an extension. Mirrors `CapabilityItem`'s ✓/✗ row // rather than reusing it: that element's `capability` prop is the closed union @@ -306,22 +321,22 @@ export function ConnectionInfoContent({ Server Implementation - Name + Name {displayName} - Version + Version {displayVersion} - Protocol + Protocol {protocolVersion || "—"} - Transport + Transport {transport} - Era + Era - Session + Session {formatSession(protocolEra, transport)} @@ -330,7 +345,7 @@ export function ConnectionInfoContent({ Discovery - Supported versions + Supported versions {discoverResult.supportedVersions.length > 0 ? discoverResult.supportedVersions.join(", ") @@ -433,10 +448,10 @@ export function ConnectionInfoContent({ OAuth Details - Protocol + Protocol {formatProtocol(oauth.protocol)} - Status + Status {oauth.clientId && ( - - Client ID + + Client ID {oauth.clientId} - + )} {oauth.clientRegistrationKind && ( - Client registration + Client registration {formatClientRegistrationKind(oauth.clientRegistrationKind)} @@ -460,19 +475,19 @@ export function ConnectionInfoContent({ )} {oauth.protocol === "ema" && oauth.idpSession && ( - IdP session + IdP session {formatIdpSession(oauth.idpSession)} )} {oauth.authUrl && ( - - Auth URL + + Auth URL {oauth.authUrl} - + )} {oauth.scopes && oauth.scopes.length > 0 && ( - Scopes + Scopes {formatScopes(oauth.scopes)} )} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx index abd5c4a9..d2031938 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx @@ -26,7 +26,10 @@ const CaptionRow = Flex.withProps({ wrap: "nowrap", }); -const Caption = Text.withProps({ size: "sm" }); +// Bold, matching every other field label in Connection Info (#2328) — the +// token rows sit in the same OAuth Details list as Client ID and Scopes, so a +// lighter caption here would break the column a reader scans down. +const Caption = Text.withProps({ size: "sm", fw: 600 }); const Toolbar = Flex.withProps({ gap: 4, diff --git a/clients/web/src/theme/Code.test.tsx b/clients/web/src/theme/Code.test.tsx new file mode 100644 index 00000000..a78fea4b --- /dev/null +++ b/clients/web/src/theme/Code.test.tsx @@ -0,0 +1,60 @@ +import { describe, it, expect } from "vitest"; +import { Code } from "@mantine/core"; +import { renderWithMantine } from "../test/renderWithMantine"; + +// `ThemeCode.extend` is applied transitively wherever a Code renders, but no +// single component exercises every branch of its styles callback. Same reason +// as `Paper.test.tsx` (#1787 brought theme/** under the coverage gate). +// +// The load-bearing assertion here is the `wrapping` / `wrapping-plain` split +// (#2328): `wrapping-plain` exists so Connection Info can drop the inset +// surface behind a label/value pair *without* changing `wrapping`, which +// ElicitationUrlPanel, ListLoadError, MalformedItemsWarning and the CSV +// fallback all rely on to separate a raw value from the prose around it. + +describe("ThemeCode variants", () => { + it("renders the default variant", () => { + const { getByText } = renderWithMantine(default); + expect(getByText("default")).toBeTruthy(); + }); + + it("wraps long values in both wrapping variants", () => { + for (const variant of ["wrapping", "wrapping-plain"] as const) { + const { getByText } = renderWithMantine( + {variant}, + ); + const style = getByText(variant).style; + expect(style.wordBreak).toBe("break-all"); + expect(style.whiteSpace).toBe("pre-wrap"); + } + }); + + it("strips the inset surface for wrapping-plain only", () => { + const { getByText: getPlain } = renderWithMantine( + plain, + ); + expect(getPlain("plain").style.backgroundColor).toBe("transparent"); + expect(getPlain("plain").style.padding).toBe("0px"); + + // The guard that makes this change safe: `wrapping` keeps its surface, so + // every other consumer of it is untouched. + const { getByText: getInset } = renderWithMantine( + inset, + ); + expect(getInset("inset").style.backgroundColor).not.toBe("transparent"); + expect(getInset("inset").style.padding).not.toBe("0px"); + }); + + it("clips instead of wrapping for the nowrap variant", () => { + const { getByText } = renderWithMantine(n); + const style = getByText("n").style; + expect(style.whiteSpace).toBe("nowrap"); + expect(style.overflow).toBe("hidden"); + expect(style.textOverflow).toBe("ellipsis"); + }); + + it("drops the margin on a block Code", () => { + const { getByText } = renderWithMantine(b); + expect(getByText("b").style.margin).toBe("0px"); + }); +}); diff --git a/clients/web/src/theme/Code.ts b/clients/web/src/theme/Code.ts index b54a8eee..86efd014 100644 --- a/clients/web/src/theme/Code.ts +++ b/clients/web/src/theme/Code.ts @@ -10,10 +10,20 @@ export const ThemeCode = Code.extend({ backgroundColor: "var(--inspector-code-surface, var(--inspector-surface-code))", }; - if (props.variant === "wrapping") { + if (props.variant === "wrapping" || props.variant === "wrapping-plain") { root.wordBreak = "break-all"; root.whiteSpace = "pre-wrap"; } + if (props.variant === "wrapping-plain") { + // Wraps like `wrapping`, but with no inset surface behind it. Used for + // label/value rows that read as *values* rather than as quoted blobs — + // Connection Info's Client ID and Auth URL. A separate variant rather + // than a tweak to `wrapping`, because that one is shared with panels + // (elicitation URLs, load errors, CSV fallback) where the grey inset is + // doing real work separating a raw value from the prose around it. + root.backgroundColor = "transparent"; + root.padding = "0"; + } if (props.block) { root.margin = "0"; } From 664c9f88006a0b7ff873ee42da85b7af6639febf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 09:56:33 -0400 Subject: [PATCH 2/3] fix: group Connection Info's full-width OAuth fields above the token rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client ID sat third in OAuth Details, between Status and Client registration, so its label-over-value block interrupted the run of inline two-column rows and broke the scan down the label column for everything after it. Access Token and ID Token already used that layout, at the bottom. Moved Client ID and Auth URL down to sit with them: the inline rows now run Protocol, Status, Client registration, IdP session, Scopes, and the full-width fields follow as one group — Auth URL, Client ID, Access Token, ID Token — so Client ID is directly above the only other field a reader sees laid out the same way. A test asserts the order off the DOM rather than each field's presence, since order is the property; putting Client ID back in its old slot fails it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB Signed-off-by: cliffhall --- .../ConnectionInfoContent.test.tsx | 51 +++++++++++++++++++ .../ConnectionInfoContent.tsx | 28 ++++++---- 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 53b225e1..1a525839 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -625,6 +625,57 @@ describe("ConnectionInfoContent", () => { } }); + it("groups the full-width fields at the end, Client ID directly above Access Token", () => { + renderWithMantine( + , + ); + + // Order is the assertion, so read the labels off the DOM rather than + // checking each is merely present: the inline two-column rows come first, + // then every label-over-value field together. Interleaving them is what + // this guards against — it broke the scan down the label column, and the + // tokens already used the full-width layout at the bottom. + const order = [ + "Client registration", + "Scopes", + "Auth URL", + "Client ID", + "Access Token", + ]; + const positions = order.map((label) => { + const node = screen.getAllByText(label)[0]; + expect(node).toBeInTheDocument(); + return ( + node.compareDocumentPosition(screen.getAllByText("Protocol")[0]) & + Node.DOCUMENT_POSITION_PRECEDING + ); + }); + expect(positions.every(Boolean)).toBe(true); + + const labelNode = (label: string) => screen.getAllByText(label)[0]; + for (let i = 0; i < order.length - 1; i++) { + const earlier = labelNode(order[i]); + const later = labelNode(order[i + 1]); + expect( + earlier.compareDocumentPosition(later) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + } + }); + it("renders EMA idp session when provided", () => { renderWithMantine( - {oauth.clientId && ( - - Client ID - {oauth.clientId} - - )} {oauth.clientRegistrationKind && ( Client registration @@ -479,17 +473,29 @@ export function ConnectionInfoContent({ {formatIdpSession(oauth.idpSession)} )} + {oauth.scopes && oauth.scopes.length > 0 && ( + + Scopes + {formatScopes(oauth.scopes)} + + )} + {/* The full-width fields are kept together at the end of the + section, directly above the token rows, which use the same + label-over-value layout. Interleaved with the inline two-column + rows they broke the scan down the label column for every row + after them, and Client ID — the one most likely to be long — + was the worst offender (#2328). */} {oauth.authUrl && ( Auth URL {oauth.authUrl} )} - {oauth.scopes && oauth.scopes.length > 0 && ( - - Scopes - {formatScopes(oauth.scopes)} - + {oauth.clientId && ( + + Client ID + {oauth.clientId} + )} {oauth.accessToken && ( Date: Fri, 11 Sep 2026 10:07:22 -0400 Subject: [PATCH 3/3] 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 && (