Skip to content

Phase 2: Refactor Logs cluster to MCP SDK types#1215

Merged
cliffhall merged 2 commits into
v2/mainfrom
v2/phase-2-logs
Apr 20, 2026
Merged

Phase 2: Refactor Logs cluster to MCP SDK types#1215
cliffhall merged 2 commits into
v2/mainfrom
v2/phase-2-logs

Conversation

@cliffhall
Copy link
Copy Markdown
Member

Summary

  • LogControls: types currentLevel, visibleLevels, and all handler params with LoggingLevel instead of string; LOG_LEVELS array and LEVEL_COLORS record properly typed
  • LogStreamPanel: types visibleLevels as Record<LoggingLevel, boolean>
  • LoggingScreen: types currentLevel, onSetLevel, internal visibleLevels state and toggle handlers with LoggingLevel; extracts NO_LEVELS_VISIBLE as module-level constant

This is the smallest cluster — the LogEntry element was already refactored in Phase 1 to use LoggingMessageNotification["params"] and LoggingLevel from the SDK, so the group/screen layer just needed its type signatures tightened to match.

No ConnectedView story changes needed — it already passes valid LoggingLevel literals.

Test plan

  • npm run format — passes
  • npm run lint — passes
  • npm run build (includes tsc -b) — passes
  • Visually verify Storybook stories for LogControls, LogStreamPanel, LoggingScreen
  • Verify ConnectedView LoggingActive story renders correctly

🤖 Generated with Claude Code

Replace string-typed log levels with SDK LoggingLevel across the Logs
group components and LoggingScreen.

- LogControls: type currentLevel, visibleLevels, and all handlers with
  LoggingLevel instead of string; type LOG_LEVELS array and LEVEL_COLORS
  record with LoggingLevel
- LogStreamPanel: type visibleLevels as Record<LoggingLevel, boolean>
  instead of Record<string, boolean>
- LoggingScreen: type currentLevel, onSetLevel, visibleLevels state,
  and toggle handlers with LoggingLevel; extract NO_LEVELS_VISIBLE as
  a module-level constant
- Stories already used LoggingLevel literals; updated type annotations
  to use Record<LoggingLevel, boolean>
- No ConnectedView story changes needed (already passes valid
  LoggingLevel values)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

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


Code Review

  • Gather context and read changed files
  • Review LogControls changes
  • Review LogStreamPanel changes
  • Review LoggingScreen changes
  • Review stories changes
  • Post final review

Clean, focused refactoring. The changes are minimal, consistent, and improve type safety throughout the logging cluster. A few observations:

as LoggingLevel type assertion — LogControls.tsx:76

onChange={(value) => {
  if (value) onSetLevel(value as LoggingLevel);
}}

Mantine's Select.onChange returns string | null, so after the null check value is string. The cast to LoggingLevel is safe in practice because data is populated exclusively from LOG_LEVELS — but it's still a type assertion that silently bypasses the compiler. A type-safe alternative:

onChange={(value) => {
  if (value && (LOG_LEVELS as string[]).includes(value)) {
    onSetLevel(value as LoggingLevel);
  }
}}

Or, if you'd rather keep it tight, a simple LOG_LEVELS.includes(value as LoggingLevel) guard works too. Not a blocker, just worth noting for completeness. Fix this →

LOG_LEVELS exhaustiveness gap

LEVEL_COLORS: Record<LoggingLevel, { c: string }> is exhaustive by construction — TypeScript will error if the SDK adds a new level and LEVEL_COLORS doesn't cover it. But LOG_LEVELS: LoggingLevel[] won't catch a missing entry since LoggingLevel[] just means "an array of valid levels", not "an array of all levels". If LoggingLevel ever gains a new member, it would silently not appear in the Select dropdown or filter list. This is a pre-existing concern, not introduced here.

NO_LEVELS_VISIBLE extraction — good call

Moving this from a per-render Object.fromEntries(Object.keys(...)) inside the component body to a typed module-level constant is strictly better:

  • Eliminates the allocation on every render
  • Replaces Record<string, boolean> with Record<LoggingLevel, boolean>
  • Avoids Object.keys returning untyped string[]

as const removal on LOG_LEVELS

The original ] as const was required to derive a readonly tuple type. Now that the type is stated explicitly as LoggingLevel[], the as const is redundant — removing it is correct.

Overall

This is a solid, self-contained improvement. The type signatures throughout the logging cluster are now consistent with the SDK types used in LogEntry (from Phase 1). No behavior changes, no API surface changes — just tighter types. The stories update to Record<LoggingLevel, boolean> correctly mirrors the prop change.

The as LoggingLevel assertion is the only thing worth a follow-up, and even that is benign given how data is constructed.

Validate the Select value against LOG_LEVELS before casting to
LoggingLevel, ensuring the compiler isn't silently bypassed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

Addressed in b068b56:

as LoggingLevel type assertion — Added a LOG_LEVELS.includes() guard before the cast, so the value is validated at runtime before being passed to onSetLevel. The as LoggingLevel on the includes argument is still needed since LOG_LEVELS is typed as LoggingLevel[] and value is string, but the actual narrowing happens via the runtime check.

@cliffhall cliffhall merged commit a1052a1 into v2/main Apr 20, 2026
2 checks passed
@cliffhall cliffhall deleted the v2/phase-2-logs branch April 20, 2026 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant