Skip to content

fix(web): hide resource subscription UI when server lacks subscribe capability - #1479

Merged
cliffhall merged 4 commits into
v2/mainfrom
v2/1478-hide-subscribe-when-unsupported
Jun 14, 2026
Merged

fix(web): hide resource subscription UI when server lacks subscribe capability#1479
cliffhall merged 4 commits into
v2/mainfrom
v2/1478-hide-subscribe-when-unsupported

Conversation

@cliffhall

@cliffhall cliffhall commented Jun 14, 2026

Copy link
Copy Markdown
Member

Closes #1478

Summary

On the Resources screen, the Subscribe/Unsubscribe button and the Subscriptions accordion section were always rendered, even when the connected server doesn't advertise the resources.subscribe capability — controls that can't do anything useful for such servers.

This threads a subscriptionsSupported flag, derived in App.tsx from capabilities?.resources?.subscribe === true, through InspectorViewResourcesScreen to the leaf components (matching the existing completionsSupported pattern). When the server doesn't support subscriptions:

  • ResourcePreviewPanel hides the Subscribe/Unsubscribe button (Refresh stays).
  • ResourceControls omits the Subscriptions accordion section entirely — dropping it from allSections so the expand/collapse-all toggle and open-section bookkeeping stay correct even if a stale "subscriptions" entry persists from a prior subscription-capable session.

The flag defaults to true at the leaf components, so existing callers, tests, and stories keep rendering the subscription controls unless a server is explicitly marked unsupported.

Testing

  • Added unit tests for the hidden vs. shown states across ResourceControls, ResourcePreviewPanel, and ResourcesScreen.
  • Added Storybook SubscriptionsUnsupported stories (with play assertions where applicable) for all three components.
  • npm run validate ✅ and npm run test:storybook ✅ (360 stories).

🤖 Generated with Claude Code


Also in this PR: Servers screen scrollbar treatment (follow-up to #1474)

The Servers screen grid had the same scrollbar issue #1474 fixed for the History/Network/Logging list panels: the ScrollArea.Autosize used the default type="hover" with no offsetScrollbars, so the bar popped in on hover and overlaid the right edge of the server cards (occluding their action icons / status badges).

Applied the same type="scroll" + offsetScrollbars treatment to ServerListScreen, and reused the shared expectScrollbarGutterIdleHidden story assertion to regression-protect the gutter + idle-hidden behavior.


Also in this PR: InspectorView shell no longer scrolls as a whole

The entire InspectorView could gain a page-level scrollbar (see the reported screenshot): AppShell.Main carried Mantine's default padding="md" on top of the fixed-header offset, but every screen already sizes itself to calc(100vh - header) with its own xl padding (no allowance for Main's padding). That extra inset pushed screen content past the viewport, so the page scrolled instead of confining overflow to each screen's inner ScrollArea.

Fix: set the shell padding to 0 (Main now contributes only the header offset, matching what the screens were built for) and clamp the Main slot to height: 100dvh + overflow: hidden in the theme — so the view can never scroll regardless of content; only the inner ScrollArea regions do. Added an InspectorView story play assertion guarding the invariant (verified it fails without the fix) plus a ManyServers fixture confirming it holds under overflow-inducing content.

…apability (#1478)

Thread a `subscriptionsSupported` flag derived from
`capabilities?.resources?.subscribe === true` from App through
InspectorView → ResourcesScreen to the leaf components. When false:

- ResourcePreviewPanel hides the Subscribe/Unsubscribe button (Refresh
  stays).
- ResourceControls omits the Subscriptions accordion section entirely,
  dropping it from `allSections` so the expand/collapse-all toggle and
  open-section bookkeeping stay correct even if a stale "subscriptions"
  entry persists from a prior subscription-capable session.

The flag defaults to true at the leaf components so existing callers,
tests, and stories keep rendering the subscription controls unless a
server is explicitly marked unsupported.

Adds unit tests and Storybook stories covering the hidden and shown
states across all three components.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jun 14, 2026
cliffhall and others added 2 commits June 14, 2026 11:33
…treatment (#1474)

The server grid's ScrollArea.Autosize used the default type="hover" with no
offsetScrollbars, so the scrollbar popped in on hover and overlaid the right
edge of the server cards (occluding their action icons / status badges) — the
same problem #1474 fixed for the History/Network/Logging list panels.

Apply the same `type="scroll"` + `offsetScrollbars` treatment, and reuse the
shared `expectScrollbarGutterIdleHidden` story assertion to regression-protect
the gutter + idle-hidden behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AppShell.Main carried Mantine's default `padding="md"` on top of the
fixed-header offset, but every screen already sizes itself to
`calc(100vh - header)` and supplies its own `xl` padding (with no
allowance for Main's padding). That extra inset pushed screen content
past the viewport, so the whole InspectorView gained a page-level
scrollbar instead of confining overflow to each screen's inner
ScrollArea.

Set the shell `padding` to 0 (Main now contributes only the header
offset, matching what the screens were built for) and clamp the Main
slot to `height: 100dvh` with `overflow: hidden` in the theme, so the
view can never scroll regardless of content — only the inner ScrollArea
regions do.

Adds an InspectorView story play assertion guarding the invariant
(Main clips + is viewport-clamped) and a ManyServers fixture that
confirms it holds under enough content to overflow, with the inner
server-grid ScrollArea absorbing the overflow.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

  • Read the changed files
  • Review subscription-capability gating
  • Review Servers screen scrollbar change
  • Review InspectorView shell scroll fix
  • Post review feedback

Overall this is a clean, well-scoped PR. The capability-gating follows the established completionsSupported pattern exactly, prop drilling is consistent (App → InspectorView → ResourcesScreen → leaf), and the leaf-level = true defaults keep every existing caller/test/story unchanged. Test + story coverage for both states is present across all three components. No blocking issues — a few minor observations below.

✅ Subscription gating — looks correct

  • App.tsx:2319 deriving capabilities?.resources?.subscribe === true is the right strict check: subscribe is an optional boolean in the MCP ServerCapabilities, so === true correctly treats both undefined and explicit false as unsupported (unlike completions !== undefined, which is correct for its shape).
  • The accordion bookkeeping in ResourceControls.tsx is the subtle part, and it's handled well. Both allExpanded (lines 124–126) and visibleOpenSections (138–141) now filter against allSections, so a stale "subscriptions" entry persisted in the parent's open-sections from a prior subscription-capable session can't make allExpanded undercount or surface a phantom panel. Good defensive thinking, and the comments explain why.

🟡 Minor: stale "subscriptions" is re-appended to parent state

In handleOpenSectionsChange (line 154), intendedButEmptySections still includes a stale "subscriptions" value (it's filtered out of visibleOpenSections, so it lands in the "intended but empty" bucket and gets re-appended via onOpenSectionsChange). This is harmless today — nothing renders it, and allExpanded/visibleOpenSections both ignore it — so it's purely a cosmetic lingering entry in persisted state. Not worth changing for this PR, but worth being aware of; if you wanted it spotless you could intersect intendedButEmptySections with allSections too.

🟡 Minor: 100dvh (Main clamp) vs 100vh (screens) unit mismatch

theme/AppShell.ts clamps main to height: "100dvh", but every screen sizes its content to calc(100vh - header) (e.g. ResourcesScreen.tsx:77, ServerListScreen.tsx:141). On desktop these are identical, so the fix works. On a mobile viewport where dvh < vh (dynamic browser toolbars), a screen's 100vh child would exceed Main's 100dvh content box, and since Main is now overflow: hidden the bottom would be clipped rather than scrollable. Given this is a desktop-oriented tool it's low-risk, but aligning the screens to 100dvh (or Main to 100vh) would keep the two assumptions in lockstep. Fix this →

🟡 Minor: main height clamp is a global theme change

The styles.main override in theme/AppShell.ts applies to every AppShell.Main instance, not just InspectorView. The only real-app usage is InspectorView; the other 8 <AppShell> matches are Storybook decorators, so impact is effectively nil. Just flagging that a per-instance styles/className on the InspectorView AppShell would scope the invariant more tightly than a global theme rule — your call given the project's "prefer theme over inline" convention.

✅ Scrollbar + shell-scroll fixes

  • ServerListScreen now matches the type="scroll" + offsetScrollbars treatment from History/Network/Logging lists: scrollbar appears on hover and overlays the cards #1474 — consistent with History/Network/Logging, and the reused expectScrollbarGutterIdleHidden assertion regression-guards it. 👍
  • The padding={0} + Main overflow: hidden combination is the right root-cause fix for the page-level scrollbar (Mantine keeps the header-offset padding-top independent of the padding prop, so the header offset is preserved). The added InspectorView play assertion guarding the no-scroll invariant plus the ManyServers overflow fixture is exactly the kind of regression protection this warrants.

…tions intent

Responding to the @claude review on #1479:

- Align all screen height calcs from `100vh` to `100dvh` so they match the
  Main slot's `100dvh` clamp. On a mobile viewport where `dvh < vh` a `100vh`
  child would have exceeded Main's `100dvh` content box and been clipped by
  `overflow: hidden`; the units are now in lockstep.
- Restrict `intendedButEmptySections` in ResourceControls to `allSections` so a
  stale "subscriptions" entry persisted from a prior subscription-capable
  session is dropped from persisted open-sections instead of being perpetually
  re-appended once the section is no longer rendered. Adds a regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the review! Addressed in 21b13ff:

🟡 100dvh vs 100vh unit mismatch — Fixed. Aligned every screen's height calc (ResourcesScreen, ToolsScreen, ServerListScreen, PromptsScreen, HistoryScreen, TasksScreen, AppsScreen, NetworkScreen, LoggingScreen) from 100vh to 100dvh so they're in lockstep with the Main slot's 100dvh clamp. On a mobile viewport where dvh < vh a 100vh child would have exceeded Main's 100dvh content box and been clipped by overflow: hidden — now they match. Updated the related comments too.

🟡 Stale "subscriptions" re-appended to parent state — Fixed. intendedButEmptySections in ResourceControls is now intersected with allSections, so a stale "subscriptions" value persisted from a prior subscription-capable session is dropped from persisted open-sections rather than perpetually re-appended once the section is no longer rendered. Added a regression test (drops a stale 'subscriptions' entry from persisted state when subscriptions are unsupported).

🟡 main height clamp is a global theme change — Keeping it in the theme intentionally. The project's AGENTS.md conventions explicitly prefer theme variants/global theme rules over per-instance className/inline styling, and as you noted the only real-app AppShell is InspectorView (the other matches are Storybook decorators), so the blast radius is nil. A global theme rule is the convention-aligned home for this invariant.

npm run validate and npm run test:storybook both pass.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 53s —— View job


Code Review (round 2)

  • Read the latest changes (incl. commit 21b13ffe addressing prior review)
  • Verify the two follow-up fixes (viewport unit alignment, stale-section drop)
  • Spot-check remaining gating / scroll logic
  • Post review feedback

Both follow-ups from the previous review are addressed cleanly, and the rest of the PR is unchanged from the already-approved round. No blocking issues — this looks ready to merge. ✅

✅ Viewport unit alignment (100vh100dvh)

theme/AppShell.ts:18-21 clamps main to 100dvh + overflow: hidden, and every screen now sizes to calc(100dvh - header) to match (ResourcesScreen.tsx:77,129, ServerListScreen.tsx:140, plus the other 7 screens). The two assumptions are now in lockstep, so the mobile-viewport clipping edge case (dvh < vh → screen content exceeds Main's content box → clipped) can't occur. The updated comments in AppShell.ts explain the why well.

✅ Stale "subscriptions" no longer re-appended

ResourceControls.tsx:150-153 now intersects intendedButEmptySections with allSections, so a stale "subscriptions" entry persisted from a prior subscription-capable session is dropped from persisted open-sections instead of being perpetually re-appended through handleOpenSectionsChange. I traced the three consumers and they're all consistent now:

  • allExpanded (124-126) — filters against allSections
  • visibleOpenSections (138-141) — filters against allSections
  • intendedButEmptySections (150-153) — now filters against allSections

handleToggleList writes [...allSections] (no longer carries "subscriptions" when unsupported), so expand-all also drops the stale entry. The regression test added in the commit guards exactly this path.

✅ Gating + wiring unchanged and correct

App.tsx:2319 (capabilities?.resources?.subscribe === true) → InspectorViewResourcesScreenResourceControls / ResourcePreviewPanel, with leaf-level = true defaults preserving every existing caller/test/story. ResourcePreviewPanel.tsx:233 gates only the SubscribeButton (Refresh stays), and ResourceControls.tsx:266 omits the whole accordion item. Matches the established completionsSupported pattern.

✅ Scroll fixes unchanged

ServerListScreen type="scroll" + offsetScrollbars (consistent with #1474) and the padding={0} + Main overflow:hidden shell-scroll fix are as previously reviewed, both regression-guarded by story play assertions.

Nice work threading the flag and tightening the persisted-state handling.

@cliffhall
cliffhall merged commit ef921ba into v2/main Jun 14, 2026
1 check passed
@cliffhall
cliffhall deleted the v2/1478-hide-subscribe-when-unsupported branch June 14, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide Subscribe/Unsubscribe button and Subscriptions section when server doesn't support resource subscriptions

1 participant