fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774)

[cliffhall]

Closes #1774

Problem

Follow-up from the #1773 review. A server that reports serverInfo: { name: "" } (an empty-but-present name) left ViewHeader rendering a nameless title, while the tab bar worked fine. App's object-level ?? fallback only fires when the whole serverInfo object is absent (the #1772 modern-omitted case), so a reported { name: "" } yields serverInfo.name === "" and reaches the header blank.

The Connection Info modal already degrades gracefully here (serverInfo.name || "—", #1773); only the header did not.

Decision

• Header: degrade a blank reported name to the active server's catalog name — the name the user gave the server. For display, a reported-but-empty name is no better than an omitted one, so this matches Modern-era connection with no serverInfo renders an empty header (no tabs) #1772's undefined-case behavior.
• serverInfoReported: left as serverInfo !== undefined (unchanged). This is the deliberate call the issue asked for — the flag tracks presence of the object, not name usability. Driving it off name emptiness would wrongly mask a reported version (a server could send { name: "", version: "1.2" }), and would make the modal's "not reported by server" label untruthful. The modal therefore stays faithful: a reported-but-empty name still reads as —, any reported version intact.

Implementation

The fallback is a pure display concern, so it lives in the view layer (InspectorView), which has both the reported name and the catalog list — resolveHeaderServerInfo(...). App's initializeResult and the modal path are untouched and remain faithful to the raw report.

Tests

Added two InspectorView tests (scoped to the header via role="banner" to exclude the ServerCard, which shows the catalog name unconditionally):

• reported name empty → header shows the catalog name
• reported name empty and no catalog match → header renders with no borrowed name (no crash), covering the no-fallback branch

npm run ci passes locally (validate, coverage gate, smoke, Storybook).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 36s —— View job

---

Review: fix(web) — degrade connected header to catalog name for empty serverInfo.name

• Read the PR diff
• Inspect InspectorView.tsx context (serversInput, activeServer, ViewHeader)
• Check the Modern-era connection with no serverInfo renders an empty header (no tabs) #1772/fix(web): render the connected header when a modern server omits serverInfo (#1772) #1773 sibling paths in App.tsx / ConnectionInfoContent.tsx
• Verify tests, branch coverage, CI
• Post review

Verdict: looks correct and well-scoped. The layering decision holds up under inspection, and I found no correctness bugs. Four minor points below, none blocking.

---

What I verified

• The modal really does stay faithful. ConnectionInfoModal is rendered from App.tsx:4465-4470 with App's untouched initializeResult + serverInfoReported, so resolveHeaderServerInfo (a view-local transform on the way into ViewHeader) cannot leak a borrowed name into the modal's "not reported by server" labeling. ConnectionInfoContent.tsx:212-217 still renders — for a reported-but-empty name and keeps any reported version. The PR's stated rationale for not touching serverInfoReported checks out — a { name: "", version: "1.2" } server would otherwise lose a real version row.
• The only two consumers of serverInfo.name in the app are ViewHeader.tsx:255 and ConnectionInfoContent.tsx:213; both are now handled, so there's no third surface left rendering a nameless title.
• No re-render hazard from the new object. resolveHeaderServerInfo returns a fresh object each render in the empty-name path, but ViewHeader diffs via the primitive snapshotKey string (ViewHeader.tsx:97-105), not object identity, so the adjust-state-during-render snapshot doesn't churn.
• Branch coverage of the new function is complete (truthy name via the pre-existing connected tests, empty+match, empty+no-match), and CI on this branch is green (run 30165018095). I did not run the suite locally — this is a fresh checkout with clients/web/node_modules absent, and installing wasn't worth it given green CI plus the author's local npm run ci.

Minor points

1. Whitespace-only name still slips through — InspectorView.tsx:188

if (serverInfo.name) is truthy for " ", so a server reporting a blank-but-non-empty name lands right back on a nameless-looking header. That's the same class of non-conforming server this PR targets, and the fix is one call:

if (serverInfo.name.trim()) return serverInfo;

Consider trimming the borrowed value's counterpart too — nothing else needs to change since the catalog name is user-supplied. Fix this →

2. The second test's assertion is vacuous — InspectorView.test.tsx:355-373

With servers: [], the string "Alpha" was never in the tree, so queryByText("Alpha") would pass identically with the fallback removed entirely — it asserts nothing about behavior. It does its stated job (covering the catalogName ? … : serverInfo else-branch), but the assertion should carry the claim in the test name: that the connected header still renders for a nameless server. Asserting a piece of connected chrome inside the banner (the Disconnect control, or the active tab) would make it a real regression guard rather than a coverage-only test. Fix this →

3. Two fallbacks, one undocumented from the other side — App.tsx:1490-1512 vs InspectorView.tsx:174-190

The new comment points up at App's ?? fallback, but App's long memo comment (which explicitly reasons about serverInfo absence and the modal's faithfulness) doesn't point down here. Someone revisiting that memo has no signal that the empty-name case is handled a layer below. A one-line cross-reference in App's comment (// A reported-but-empty name is degraded for display in InspectorView's resolveHeaderServerInfo (#1774).) would close the loop cheaply. The alternative design — thread the catalog name to ViewHeader as its own prop and resolve in one place — is cleaner in the abstract but would widen ViewHeader's API for one edge case; I think the current split is the right call, it just wants the back-reference.

4. Trivial: the servers.find(...) runs on every connected render rather than inside a memo. Catalogs are small and this is nowhere near a hot path, so I'd leave it — noting only because the servers memo doing a comparable pass sits ~120 lines below.

Nice touches worth calling out

The comment on resolveHeaderServerInfo explains why the fallback is view-local rather than just what it does, and the tests' role="banner" scoping (with the reason — the ServerCard shows the catalog name unconditionally, so an unscoped getByText("Alpha") would pass without the fix) is exactly the kind of thing that keeps a test honest.
· v2/fix-1774-empty-servername

[cliffhall]

Thanks for the review — addressed the three actionable points in fa8c196b.

1. Whitespace-only name slips through — fixed. resolveHeaderServerInfo now guards with serverInfo.name.trim(), so " " degrades to the catalog name like the empty case. Updated the explanatory comment to call out the whitespace class, and added an InspectorView test asserting a whitespace-only reported name shows the catalog name in the header (scoped to role="banner"). I left the display value un-trimmed when a real name is present — trimming a legitimately-spaced name felt like scope the bug doesn't call for, and the guard already covers the "blank-looking" case.

2. Vacuous test assertion — fixed. The no-catalog-match test now asserts the connected header chrome actually renders — the Disconnect control is present inside the banner — so it's a real regression guard for "the header still renders for a nameless server," while still covering the catalogName ? … : serverInfo else-branch. (Swapped the absent-"Alpha" check for an absent-"ghost" check alongside it, since "ghost" is the active id that must not leak in as a name.)

3. Undocumented from App's side — fixed. Added a back-reference in App's initializeResult memo comment pointing down to resolveHeaderServerInfo for the reported-but-blank-name case, so someone revisiting that memo has the signal. I kept the current split (rather than threading the catalog name into ViewHeader as its own prop) for the reason you noted — it avoids widening ViewHeader's API for one edge case.

4. servers.find on every connected render — left as-is, per your call. It's a tiny scan on a small catalog, well off any hot path, and wrapping it in a memo would add machinery for no measurable gain.

npm run ci is green locally (validate, coverage gate, smoke, Storybook).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 58s —— View job

---

Re-review: fa8c196b — whitespace guard, strengthened tests, cross-reference

• Read fa8c196b + full PR diff vs origin/v2/main
• Verify the .trim() guard and its interaction with ViewHeader
• Verify the strengthened no-catalog-match test actually guards
• Check the App cross-reference and remaining serverInfo.name surfaces
• Post review

All three actionable points are addressed correctly. One new observation, in the same class as point 1 but a file this PR doesn't touch — non-blocking.

---

Verification of the fixes

1. .trim() guard — InspectorView.tsx:190. " " now degrades to the catalog name, and both new tests genuinely distinguish the fix from its absence: without the trim, ViewHeader would snapshot serverName: " " (ViewHeader.tsx:255) and within(banner).getByText("Alpha") would fail. Agree with leaving the display value un-trimmed when a real name is present — the guard is about "blank-looking," not normalization, and trimming a name the server actually sent would be a different (unrequested) behavior.

2. Strengthened no-catalog-match test — InspectorView.test.tsx:376-405. Asserting getByRole("button", { name: "Disconnect from server" }) inside the banner makes it a real guard: the connected header must still mount for a nameless server. The queryByText("ghost") swap is a better absence check than the old one too — "ghost" is the activeServer id, so this now fails if someone "fixes" the no-match branch by borrowing the id as a label. Branch coverage of resolveHeaderServerInfo remains complete (truthy name via the pre-existing connected tests; blank+match ×2; blank+no-match).

3. Cross-reference — App.tsx:1507-1511. The memo comment now points down to resolveHeaderServerInfo and states why the borrowed name stays out of the faithful object. Both directions now documented; the split reads as a deliberate decision rather than an omission.

No new crash risk from the guard. serverInfo.name.trim() dereferences the same field ViewHeader.tsx:255 already dereferenced unconditionally, so a hypothetical runtime-absent serverInfo fails exactly where it did before — not a new surface.

New minor: the modal has the same whitespace gap — ConnectionInfoContent.tsx:207-217

This PR's own reasoning ("whitespace-only is the same non-conforming class as empty") applies one file over, where it isn't handled:

// … Both rows use `||` (not `??`) so a reported-but-
// empty name/version reads as unknown ("—") rather than a blank row …
const displayName = serverInfoReported ? serverInfo.name || "—" : SERVER_INFO_NOT_REPORTED_LABEL;

" " || "—" is " ", so a whitespace-only reported name renders a visually blank row — the exact outcome the comment says the || exists to prevent. Same for serverInfo.version. It's a two-token change (serverInfo.name.trim() || "—") and it keeps the modal faithful (still —, not a borrowed name), so it doesn't disturb the layering decision this PR defends. I'd call it in scope for #1774 since the header and the modal are the only two consumers of that field and you just tightened one of them — but it's equally reasonable to leave the modal to a follow-up given the diff is currently clean. Fix this →

Notes

• Point 4 (servers.find per render) — agreed, leave it. It mirrors what availableTabs already does at InspectorView.tsx:857 on the same inputs, so it's consistent with the surrounding code, not an outlier.
• I did not run the suite locally (fresh checkout, clients/web/node_modules absent). CI for fa8c196b was still in progress when I looked (run 30166162563) — worth a glance before merge; the previous commit's run was green and you report local npm run ci passing.

Verdict: approve-equivalent (I can't submit a formal GitHub approval). Nothing blocking; the modal whitespace item is your call on scope.
· v2/fix-1774-empty-servername

[cliffhall]

Good catch — took the modal fix; it's genuinely in scope (the header and the modal are the only two consumers of that field, and it'd be odd to tighten one and leave the other). Fixed in 2f508ffa.

Modal whitespace gap — fixed. ConnectionInfoContent now trims both the name and version rows before the || fallback, so a whitespace-only reported value reads as — instead of a visually blank row. The modal stays faithful — the fallback is still —, never the borrowed catalog name. Added a ConnectionInfoContent test asserting a whitespace-only reported name renders —.

One wrinkle worth flagging: I used ?.trim() (optional chaining), not the plain .trim() from the fix-link. The existing "renders an em-dash when server version is missing" test passes { name: "…" } as never — a non-conforming server with a runtime-absent version — and a bare .trim() throws on that undefined, where the old serverInfo.version || "—" tolerated it. ?.trim() restores that tolerance (missing → undefined → || "—"). I applied the same ?. hardening to the header's resolveHeaderServerInfo guard for the symmetric reason: a runtime-absent name should fall through to the catalog fallback, not throw.

Point 4 (servers.find per render) and your note that it mirrors availableTabs — agreed, left as-is.

npm run ci is green locally (validate, coverage gate, smoke, Storybook).

@claude review

[claude]

Claude finished @cliffhall's task in 2m 13s —— View job

---

Re-review: 2f508ffa — modal whitespace trim + ?. hardening

• Read 2f508ffa and the full PR diff vs origin/v2/main
• Verify the modal trim keeps the "not reported" vs "reported-but-blank" distinction
• Sanity-check the ?.trim() optional-chaining rationale
• Verify the new test actually distinguishes the fix
• Re-check every serverInfo.name/version consumer
• Post review

The modal fix is correct and the ?. reasoning holds. One follow-through gap: the ?. you added for name (both files) is a live branch that no test exercises — details below. Non-blocking but cheap to close, and it touches the CI branch gate.

---

Verification

Modal trim — ConnectionInfoContent.tsx:218-223. Both rows now read — for empty, whitespace-only, and runtime-absent values, and the serverInfoReported === false arm is untouched, so the "not reported by server" label stays truthful and the borrowed catalog name still can't reach the modal (ConnectionInfoContent.test.tsx:117-135 still guards that). The layering decision this PR defends is intact.

The new test genuinely distinguishes the fix — ConnectionInfoContent.test.tsx:99-115. Worth stating because it's easy to get wrong here: testing-library's default normalizer collapses/trims text, so without the trim the whitespace row would match neither " " nor "—" and getAllByText("—") would find 2, not 3. The assertion fails on the pre-fix code — a real guard, not a coverage-only test.

?.trim() over .trim() — agreed, and for the reason you gave. ConnectionInfoContent.test.tsx:50-64 passes { name: "No Version Server" } as never, so a bare .trim() on version would have thrown where serverInfo.version || "—" tolerated the omission. That's a real regression the ?. avoids, not speculative hardening.

No new crash surface from the header's ?. — ViewHeader.tsx:255 snapshots props.serverInfo.name; on the no-catalog-match path a runtime-absent name reaches snapshotKey (ViewHeader.tsx:97-105), where Array.join renders undefined as "", and React renders nothing. Same behavior as before the PR, and the typed-non-null field means no TS complaint. I also confirmed no lint rule objects: neither the root nor web eslint config enables type-aware @typescript-eslint/no-unnecessary-condition, so ?. on a non-nullable field isn't flagged.

Consumers are complete. A fresh grep across clients/web/src + core finds exactly four references to serverInfo.name/.version: ViewHeader.tsx:255, the two modal rows, and InspectorView.tsx:190. No third surface can render a nameless title, and the intentional divergence — header borrows the catalog name, modal shows — — is now documented from both directions (App.tsx:1507-1511 down, InspectorView.tsx:174-184 up).

Minor: the name ?. short-circuit is untested in both files

The version ?. is covered by the pre-existing as never test; the name ?. you added symmetrically is not — no test in either file passes a serverInfo without a name. So 2f508ffa adds two branches with zero coverage, and the comments now assert a tolerance ("a non-conforming server can omit it") that nothing pins down. Two tests mirroring the ones already there close it:

// ConnectionInfoContent — name absent, symmetric with the version-missing test
serverInfo: { version: "1.0.0" } as never   // → Name row reads "—"

// InspectorView — name absent, header borrows the catalog name
serverInfo: { version: "1.0.0" } as never   // → within(banner).getByText("Alpha")

This also matters for the gate: CI enforces per-file ≥90 on branches, and these two are uncovered. Your local npm run ci was green so the margin evidently absorbs them, but the tests make the intent explicit rather than relying on headroom. (Alternatively, if you consider the name omission unreachable in practice, AGENTS.md's policy is a justified /* v8 ignore */ — but given the version case proves such servers exist, I'd take the tests.) Fix this →

Notes

• The modal comment is now 10 lines for a 2-token expression. It's all accurate and the why (why ?., why || not ??, why the fallback isn't the catalog name) is the valuable part — but the "initialize mandates the fields, not non-empty values" sentence and the whitespace clause say roughly the same thing twice. Trim if it bugs you; not worth a commit on its own.
• CI for 2f508ffa was still in_progress when I checked (run 30166505162) — glance at it before merge. I did not run the suite locally: clients/web/node_modules is absent in this fresh checkout, so my verification is by reading, plus your local npm run ci.

Verdict: approve-equivalent (I can't submit a formal GitHub approval). Nothing blocking; the untested ?. branches are the only thing I'd fold in before merge, and even that is optional given the gate passes.
· v2/fix-1774-empty-servername

[cliffhall]

Fixed in dfdb50f3 — added the two tests that pin the name ?. short-circuit.

Untested name ?. branch — fixed. You're right that I'd introduced two branches leaning on gate headroom. Added:

• ConnectionInfoContent — a server reporting { version: "1.0.0" } as never (no name) renders the Name row as —, symmetric with the existing version-missing test.
• InspectorView — a connected server whose initializeResult.serverInfo omits name shows the catalog name "Alpha" in the header (scoped to role="banner").

Both are now real guards for the documented tolerance, not just coverage padding.

I left the modal comment as-is — the redundancy you flagged is mild and, as you said, not worth a churn commit on its own. Everything else (point 4, the borrowed-name faithfulness, consumer completeness) I'm leaving per your notes.

npm run ci is green locally (validate, coverage gate, smoke, Storybook).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 19s —— View job

---

Re-review: dfdb50f3 — pinning the name ?. short-circuit

• Read dfdb50f3 + full PR diff vs origin/v2/main
• Verify both new tests actually exercise the ?. branch
• Re-check fixture shapes (as never), assertion scoping, and the em-dash count
• Re-check the whole ViewHeader shows a nameless title when a server reports serverInfo with an empty name #1774 test matrix for gaps
• Post review

Both tests are correct and are real guards, not coverage padding. No further findings — this is done from my side.

---

Verification

Modal test — ConnectionInfoContent.test.tsx:99-115. Without the ?., serverInfo.name.trim() throws a TypeError during render and the test fails outright, so the branch is genuinely exercised. The em-dash count checks out: with version: "1.0.0" and protocolVersion: "2025-03-26" present, the three —s are the Name row plus the two extension sections — the same accounting the pre-existing version-missing test documents at line 61-62, and the same 3 the empty-name and whitespace-name tests assert. Placement directly above the whitespace test, between the empty-name and whitespace-name cases, keeps the four blank-value tests reading as one deliberate matrix.

Header test — InspectorView.test.tsx:354-375. Same story: pre-?. this throws inside resolveHeaderServerInfo, so within(banner).getByText("Alpha") can only pass with the optional chain in place. The role="banner" scoping is still doing the necessary work (ServerCard shows the catalog name unconditionally), and it asserts the positive behavior — the catalog name is borrowed — rather than merely "didn't crash", which is a stronger claim than the branch strictly needed.

The matrix is now complete across both consumers. resolveHeaderServerInfo's three branches: usable name (the pre-existing connected-header test), blank + catalog match (empty ×1, whitespace ×1, absent ×1), blank + no match (servers: [], asserting the Disconnect control). The modal covers reported-present, empty name, whitespace name, absent name, empty version, absent version, and serverInfoReported === false. Nothing in serverInfo.name/.version is left unpinned.

as never is consistent, not new debt. AGENTS.md forbids double casts; this is a single cast, and it mirrors the pre-existing { name: "No Version Server" } as never fixture two tests up — the established idiom in this file for "a non-conforming server omits a typed-non-null field." No as unknown as introduced anywhere in the PR.

Trivial, take or leave

toHaveLength(3) couples four modal tests to the count of unrelated —s elsewhere in the panel (the two extension sections), so adding an em-dash row anywhere in ConnectionInfoContent would break all four with a misleading failure. Scoping to the Name row's value would be more targeted. But the pattern predates this PR, the comment at line 61-62 explains where the 3 comes from, and changing it now would churn tests that are otherwise correct — I'd leave it. Worth a // keep in sync note only if someone touches that panel's layout.

Notes

• I did not run the suite locally — clients/web/node_modules is absent in this fresh checkout, so verification is by reading plus your local npm run ci.
• CI for dfdb50f3 was in_progress when I checked (run 30166777029) — glance at it before merge. Every prior commit's run on this branch was green.
• Per AGENTS.md, remember the post-merge steps for a v2/main PR: Closes #1774 won't auto-close on a non-default base, so close ViewHeader shows a nameless title when a server reports serverInfo with an empty name #1774 manually and move its board card to Done (248a3910).

Verdict: approve-equivalent (I can't submit a formal GitHub approval). Nothing blocking or outstanding — ready to merge once CI is green.
· v2/fix-1774-empty-servername

[cliffhall]

Thanks — appreciate the thorough passes. Agreed on leaving the toHaveLength(3) coupling as-is (predates this PR, and the line 61-62 comment documents the count) and the modal-comment redundancy. Nothing outstanding. I'll handle the post-merge steps you flagged — Closes #1774 won't auto-close on a v2/main base, so I'll close #1774 and move its board card to Done on merge.