Phase 2: Refactor Server/Settings/ViewShell cluster to MCP SDK types

[cliffhall]

Summary

• ServerCard: Replace flat scalar props with grouped objects (config: MCPServerConfig, info?: Implementation, connection: ConnectionState). Remove auto-connect useEffect (dumb component violation). Derive transport and command/url from config.
• ServerInfoContent: Replace flat props with initializeResult: InitializeResult and clientCapabilities: ClientCapabilities. Derive capability lists by iterating ServerCapabilities record.
• ServerSettingsForm: Collapse flat setting props into settings: InspectorServerSettings. Add InspectorServerSettings type to core/mcp/types.ts.
• ImportServerJsonPanel: Replace scattered scalar props with draft: InspectorServerJsonDraft.
• ViewHeader: Replace serverName: string with serverInfo: Implementation. Use ConnectionStatus from core types.
• ConnectedView: Update to pass serverInfo instead of serverName. Use ConnectionStatus type.
• ServerAddMenu, ServerListControls: No changes needed (spec says "Unchanged").
• All stories updated for new prop shapes across ServerCard, ServerInfoContent, ServerSettingsForm, ImportServerJsonPanel, ViewHeader, ServerListScreen, ConnectedView, and UnconnectedView.

Local types deleted/replaced

Old type	Replaced by
ServerCardProps.name/version/transport/status/error (flat)	config: MCPServerConfig + info?: Implementation + connection: ConnectionState
ServerInfoContentProps.name/version/protocolVersion/serverCapabilities[]	initializeResult: InitializeResult (SDK)
ServerInfoContentProps.clientCapabilities: CapabilityItemProps[]	clientCapabilities: ClientCapabilities (SDK)
ServerSettingsFormProps flat setting props	settings: InspectorServerSettings (new core type)
ImportServerJsonPanelProps flat draft props	draft: InspectorServerJsonDraft (existing core type)
ViewHeaderProps.serverName: string	serverInfo: Implementation (SDK)
ServerCard auto-connect useEffect	Removed (dumb component violation)

New core type

• InspectorServerSettings added to core/mcp/types.ts

Test plan

• Verify npm run build passes (typecheck + Vite build)
• Verify npm run lint passes
• Open ServerCard stories in Storybook — verify all states render correctly
• Open ServerInfoContent stories — verify capabilities derived from record
• Open ServerSettingsForm stories — verify accordion sections work
• Open ImportServerJsonPanel stories — verify draft object renders
• Open ViewHeader stories — verify server name from Implementation
• Open ConnectedView stories — verify header shows server info
• Open UnconnectedView stories — verify ManyServers and WithSettingsModal

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 14s —— View job

---

Code Review: Phase 2 — Server/Settings/ViewShell Refactor

Tasks

• Gather context and understand request
• Read all changed files
• Analyze core types, components, and stories
• Post comprehensive review

---

Overall this is a clean refactor: props are properly grouped into structured objects, the useEffect auto-connect violation is removed, and the stories accurately reflect the new shapes. A few items below worth addressing before merge.

---

Issues

1. getCapabilityEntries will always return supported: true — missing unsupported capabilities

ServerInfoContent.tsx lines 44–48:

function getCapabilityEntries(
  capabilities: Record<string, unknown>,
): { capability: CapabilityKey; supported: boolean }[] {
  return Object.entries(capabilities).map(([key, value]) => ({
    capability: key as CapabilityKey,
    supported: value != null,   // always true — Object.entries only yields present keys
  }));
}

Object.entries() only iterates keys that actually exist in the object. A capability absent from the capabilities record never appears in the result, so CapabilityItem will never render a red ✗ for a missing capability. The old API explicitly passed supported: false for absent capabilities.

Whether the new "show only what's supported" design is intentional is fine — but if the intent is to mirror the old behaviour and show green/red for all known capability keys, you'd need to iterate a fixed key set rather than Object.entries(). Fix this →

2. ClientCapabilities.experimental will appear as a capability item

The experimental key is defined as Record<string, unknown> on ClientCapabilities in the SDK. When present it would be passed through getCapabilityEntries(clientCapabilities) and render as an "Experimental" capability item. CapabilityItem's displayLabel map does have "experimental": "Experimental", so it won't crash — but depending on the intent, it may be better to filter this key before display, or handle it separately. Worth a deliberate decision either way.

3. Duplicate @mantine/core import in ViewHeader.tsx

ViewHeader.tsx lines 1–12:

import {
  ActionIcon, Anchor, Button, Group, Image, SegmentedControl, Text, Title,
} from "@mantine/core";
import { useComputedColorScheme } from "@mantine/core";  // duplicate

Two separate imports from the same module. useComputedColorScheme should be merged into the first import block. Linters typically flag this. Fix this →

---

Minor / Style

4. ConnectionState defined locally instead of in core

ConnectionState is exported from ServerCard.tsx and imported by ServerCard.stories.tsx. The PR moves InspectorServerSettings to core/mcp/types.ts — ConnectionState feels like the same class of type and would fit naturally there as well. Having callers reach into a leaf component file for a domain type is a code smell. Not a blocker, but worth aligning.

5. onConnectionModeChange could be narrowed

ServerSettingsForm.tsx line 26:

onConnectionModeChange: (mode: string) => void;

InspectorServerSettings.connectionMode is typed "proxy" | "direct", so the callback's parameter could be (mode: "proxy" | "direct") => void without losing flexibility — and would prevent callers from passing an arbitrary string. Fix this →

6. key={index} in KeyValueRows

ServerSettingsForm.tsx line 78:

{items.map((item, index) => (
  <Group key={index} grow>

Index keys cause React to lose state when items are removed or reordered. A stable key like ${item.key}-${index} or a generated id in the data model would be safer. Low priority since headers/metadata don't support drag-to-reorder today.

---

Positive Notes

• Removal of auto-connect useEffect — clean and correct. The side-effect was a dumb-component violation; calling both onSetActiveServer and onToggleConnection directly in the handlers is simpler and avoids the double-render.
• getTransport / getCommandOrUrl helpers — small pure functions are the right way to derive these from config rather than duplicating the logic in the template.
• InspectorServerSettings in core — the right home for that type; the settings form is now purely presentational.
• Story coverage — all affected components have updated stories that exercise the new prop shapes well, including edge cases (error state, compact mode, many servers).
• getCapabilityEntries accepting Record<string, unknown> — works because both ServerCapabilities and ClientCapabilities are structurally compatible. The displayLabel fallback (String(capability)) handles any SDK-added keys gracefully.

---

[cliffhall]

Addressed the review feedback in cc9be5e:

1. getCapabilityEntries always returning supported: true — Fixed. Now iterates fixed key sets (SERVER_CAPABILITY_KEYS and CLIENT_CAPABILITY_KEYS) and checks whether each key exists in the capabilities record. Unsupported capabilities correctly render with red ✗.

2. experimental appearing as capability item — This is now intentional and handled correctly. Both server and client fixed key sets include "experimental", and CapabilityItem already has a displayLabel entry for it. It shows green ✓ when present, red ✗ when absent — consistent with all other capabilities.

3. Duplicate @mantine/core import — Merged useComputedColorScheme into the single import block.

4. ConnectionState defined locally — Moved to core/mcp/types.ts alongside ConnectionStatus. ServerCard now imports it from core, and the stories import from core as well.

5. onConnectionModeChange narrowed — Changed from (mode: string) to (mode: "proxy" | "direct") to align with InspectorServerSettings.connectionMode. Added cast at the Mantine Select onChange boundary since it returns string | null.

6. key={index} in KeyValueRows — Acknowledged. Acceptable for now since headers/metadata don't support reordering. A stable key strategy can be added if drag-to-reorder is introduced.

[cliffhall]

Summary of all changes on this PR

Initial refactor (771a67e)

ServerCard: Replaced flat scalar props (name, version, transport, connectionMode, command, status, retryCount, error) with grouped objects (config: MCPServerConfig, info?: Implementation, connection: ConnectionState). Removed the auto-connect useEffect — dumb components must not self-dispatch side effects. Added getTransport() and getCommandOrUrl() helpers to derive display values from the config discriminated union.

ServerInfoContent: Replaced flat props with initializeResult: InitializeResult and clientCapabilities: ClientCapabilities from MCP SDK. Capability lists are now derived by iterating the ServerCapabilities record rather than accepting pre-built arrays.

ServerSettingsForm: Collapsed flat setting props (connectionMode, headers, metadata, connectionTimeout, requestTimeout, oauthClientId, oauthClientSecret, oauthScopes) into settings: InspectorServerSettings. Added InspectorServerSettings type to core/mcp/types.ts.

ImportServerJsonPanel: Replaced scattered scalar props (jsonContent, selectedPackageIndex, serverName) with draft: InspectorServerJsonDraft.

ViewHeader: Replaced serverName: string with serverInfo: Implementation. Uses ConnectionStatus from core types.

ConnectedView: Updated to pass serverInfo: Implementation instead of serverName. Uses ConnectionStatus type.

ServerAddMenu, ServerListControls: No changes needed (spec says "Unchanged").

All stories updated across ServerCard, ServerInfoContent, ServerSettingsForm, ImportServerJsonPanel, ViewHeader, ServerListScreen, ConnectedView, and UnconnectedView.

Code review feedback (cc9be5e)

• Fixed getCapabilityEntries in ServerInfoContent to iterate fixed key sets (SERVER_CAPABILITY_KEYS, CLIENT_CAPABILITY_KEYS) so unsupported capabilities render with red ✗ instead of only showing present/supported ones.
• Merged duplicate @mantine/core import in ViewHeader.
• Moved ConnectionState from ServerCard to core/mcp/types.ts alongside ConnectionStatus.
• Narrowed onConnectionModeChange from (mode: string) to (mode: "proxy" | "direct").

ServerSettingsModal creation (a6fc900)

Created a new ServerSettingsModal group component that manages:

• Accordion expand/collapse state with ListToggle expand-all/collapse-all button
• Add/remove/edit header and metadata key-value pairs
• Connection mode, timeout, and OAuth field changes
• All changes flow through a single onSettingsChange(settings) callback

Reverted ServerSettingsForm back to a controlled component (expandedSections / onExpandedSectionsChange props) — the form is a dumb renderer, the modal is the smart container.

Storybook action logging fix (e04befa, 29ce552)

Fixed ServerSettingsForm and ServerSettingsModal stories to use Storybook's useArgs hook from storybook/preview-api. This wires fn() callbacks through Storybook's args system so that every form interaction (add/remove/edit headers/metadata, accordion open/close, connection mode, timeouts, OAuth) both updates the UI and logs to the Actions panel. Previously, fn() calls made outside of args were invisible to the Actions addon.

OAuth callback improvement (d27018b)

Changed onOAuthChange from (field: string, value: string) to (oauth: OAuthSettings) where OAuthSettings = { clientId, clientSecret, scopes }. Each keystroke now reports the full OAuth state — all three fields together — instead of just the single field that changed.

Minor fixes

• Fixed ServerSettingsModal stories rendering as blank screen by wrapping in AppShell (e54c186)
• Removed dead ALL_SETTINGS_SECTIONS export that triggered react-refresh/only-export-components lint error

[cliffhall]

@claude review

[claude]

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

---

Code Review: Phase 2 — Server/Settings/ViewShell Refactor

Tasks

• Gather context and understand request
• Read all changed files
• Analyze core types, components, and stories
• Post comprehensive review

---

This is a well-structured follow-on to the first round — the previous feedback was addressed cleanly, and the new ServerSettingsModal is a solid smart-container pattern over the dumb ServerSettingsForm. A few items below worth considering before merge.

---

Issues

1. Inline style on Title inside ServerSettingsModal — violates project rule

ServerSettingsModal.tsx line 113:

<Title order={4} ta="center" style={{ flex: 1 }}>
  Server Settings
</Title>

The project guidelines explicitly prohibit inline styles: "NEVER use inline styles on a component." Mantine's Title accepts system props, so flex={1} works as a direct prop. The styles={{ title: { flex: 1 } }} on the Modal (line 121) is a second inline-style entry for the same layout concern and should also be removed — the w="100%" on the outer Group and flex={1} on the Title should be sufficient. Fix this →

2. handleTimeoutChange field parameter is string — bypasses type safety

ServerSettingsModal.tsx lines 88–90:

function handleTimeoutChange(field: string, value: number) {
  onSettingsChange({ ...settings, [field]: value });
}

The computed property spread { ...settings, [field]: value } accepts any string key, silently setting arbitrary properties on InspectorServerSettings. Since only "connectionTimeout" and "requestTimeout" are valid targets, the parameter type should be "connectionTimeout" | "requestTimeout". The same narrowing applies to onTimeoutChange: (field: string, value: number) => void in ServerSettingsFormProps (line 31) and its local handleTimeoutChange (line 116). Fix this →

3. No unit tests for ServerSettingsModal (or other modified components)

The project's AGENTS.md requires: "Ensure all code has corresponding tests" and "Ensure test coverage for each file is at least 90%." The new ServerSettingsModal has only a Storybook story. No .test.ts/.test.tsx files exist anywhere in the web client src/ tree — this applies to all changed components. This pattern predates this PR, but since ServerSettingsModal is newly introduced here, it's the right time to add unit tests for at least its key behaviors (expand/collapse all, handleAddHeader, handleRemoveHeader, onSettingsChange dispatch).

---

Minor / Style

4. OAuthSettings type should live in core/mcp/types.ts

OAuthSettings is defined in and exported from ServerSettingsForm.tsx (line 36–40), then re-imported in ServerSettingsModal.tsx. Callers who want to work with OAuth data have to reach into a leaf UI component file for a domain type. Like InspectorServerSettings (which was moved to core in this PR), OAuthSettings belongs in core/mcp/types.ts — or at minimum inlined as part of InspectorServerSettings rather than a separate detached type.

5. serverInfo.version renders without a fallback

ServerInfoContent.tsx line 93:

<ValueText>{serverInfo.version}</ValueText>

Implementation.version is optional in the SDK (version?: string). When absent, this renders undefined (React will silently skip it, so no crash) but the label "Version" will appear with an empty value. A ?? fallback like {serverInfo.version ?? "—"} would be cleaner.

6. key={index} in ImportServerJsonPanel — two additional instances

The validation results loop (line 100) and the packages Radio loop (line 118) both use key={index}. These are the same issue acknowledged in KeyValueRows from the previous review. Since the items are not reorderable this is low-priority, but worth flagging for consistency.

---

Positive Notes

• ServerSettingsModal smart-container pattern — clean separation. The modal owns mutable accordion state and dispatches a single onSettingsChange, while ServerSettingsForm remains a pure renderer. This is the right architecture.
• useArgs fix in stories — wiring fn() callbacks through Storybook's args system so Actions panel reflects every keystroke is the correct approach; the previous pattern was invisible to the addon.
• onOAuthChange signature change — passing the full OAuthSettings object instead of (field, value) tuples eliminates the stringly-typed field dispatch and makes the callback self-contained. Correct call.
• getCapabilityEntries fixed — iterating knownKeys and checking key in capabilities correctly shows unsupported capabilities as red ✗. The SERVER_CAPABILITY_KEYS / CLIENT_CAPABILITY_KEYS constant arrays are a clean solution.
• ConnectionState moved to core — removes the "reach into a leaf component for a domain type" smell flagged in the previous review.

---