Skip to content

Conversation

@gabrypavanello
Copy link
Contributor

Major layout reorganization of the Inspector dashboard.

Layout Changes:

  • Right panel: Agent/Events/Logs as tabbed sidebar (one at a time, default Agent). Collapsible, resizable, persisted tab.
  • Left panel: MCP Primitives always visible on left (no title header). Collapse button in tabs row.
  • Center: Screencast with crystalline star placeholder when no active widget.
  • Globals bar: Horizontal bar below screencast, only when streaming, collapsible.
  • Toolbar: Simplified to 2 toggle buttons (primitives + right panel).

Visual Polish:

  • Sirius star logo in header + "sirius-mcp inspector" title
  • Mirrored panel headers (collapse arrows on matching sides)
  • Matched placeholder fonts, colors, resize handle widths across panels
  • Dark toolbar button backgrounds (no UA white bleed)

Files: 12 changed, ~800 additions
Build ✅ | Tests 813/813 ✅

Sirius and others added 15 commits January 31, 2026 18:40
…laceholder, globals bar

Layout reorganization:
- Right panel: Agent/Events/Logs as tabs (one at a time, default Agent)
  Collapsible, resizable, persisted active tab
- Left panel: MCP Primitives always visible (no title header),
  collapse button moved into tabs row
- Center: Screencast with animated star placeholder when no widget
- Globals bar: Horizontal bar below screencast, only when streaming,
  collapsible with expand button
- Toolbar: Simplified to primitives + right panel toggles
- New components: RightPanel, NoWidgetPlaceholder
- Updated: GlobalsPanel (horizontal layout), McpPrimitivesPanel (no header),
  Toolbar (2 buttons), EventsPanel/AgentPanel (showTitle/showClearButton props)
- Added starFloat/starBlink CSS keyframes for tamagotchi animation

Build ✅, 813/813 tests ✅
Replace basic tamagotchi face with elegant 8-pointed crystalline star:
- Sharp elongated cardinal + diagonal points with glass-like gradients
- Glowing center with radial pulse animation
- Two slowly rotating orbital rings (digital/binary feel)
- Inner crystalline facets showing light refraction
- Ambient sparkle dots
- Subtle outer halo
- Animations: float, core pulse, orbit spin, sparkle
- Shifted from teal (#20b2aa) to icy steel blue (#7aabcf, #a8c8e8, #b8d0e8)
- Brighter/lighter overall: higher opacity facets, whiter gradients
- Wider star point edges (18px spread instead of 12)
- Larger SVG viewport (180px vs 160px)
- Updated glow/halo to match icy blue palette
Widened all 8 points at the base — ~36px spread instead of 18.
Classic compass rose proportions, no more needle spikes.
Inner facets scaled to match.
- Empty states (No agent events, No logs, etc.) now vertically centered
  using flex: 1 instead of height: 100%
- Forced sans-serif font on empty labels (was inheriting monospace from
  events/logs containers)
- Containers now flex columns so child empty states can stretch
- Both left and right panels have consistent centered placeholders
Rewrote star as single 16-vertex polygon with mathematically computed
coordinates — perfect 8-fold rotational symmetry guaranteed.
- Cardinal tips at R=88, diagonal at R=62, valleys at R=36
- Brighter gradients: higher opacities, more white (#d6e5f8, #e8f2ff)
- Inner diamond facets + refraction lines for crystalline look
- Bigger core glow (r=22), brighter center
- Cleaner structure: one polygon instead of 8 separate shapes
Replaced crystalline SVG with 15x15 pixel grid star.
Pure white (#fff) pixels, 8px per pixel block, pixelated rendering.
Subtle white glow. Clean retro aesthetic.
21x21 grid with clear star shape — thin pointed rays extending
from a solid core. 6px per pixel, crisp edges rendering.
Replaced all SVG/pixel-art code with a single crystalline star PNG.
Gentle floating animation kept. Cleaned up unused styles and keyframes.
- Replaced header logo with sirius-star.png
- Title: 'sirius-mcp inspector'
- Replaced existing logo.png and sirius-star.png with new versions.
- Ensured consistency with recent design updates.
- Collapse arrow moved to left side of right panel tabs (mirrors left panel)
- Removed Clear button from right panel header
- Placeholder font color matched (#4b5563) across both panels
- Right resize handle matched left: 6px, #2d2f2f background
- Replaced logo.png and sirius-star.png with new versions to align with design updates.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 31, 2026

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Right-side tabbed panel (Agent, Events, Logs) — resizable, collapsible, per-tab counts; active tab and width persisted
    • Animated center placeholder when no active widget ("No active widget yet — ask your agent to test")
  • UI Updates

    • Environment moved to a horizontal globals bar beneath main content — collapsible with persisted state
    • Left primitives panel always rendered; header shown only when centered
    • Dashboard title updated
  • UX Improvements

    • Panels can hide titles and Clear buttons via new visibility controls

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Reworks the dashboard into Layout V2: adds a collapsible, resizable RightPanel (Agent/Events/Logs) with persisted active tab and width; moves globals into a horizontal environment bar under the screencast; always-renders left MCP primitives panel (header hidden when left); removes bottom-panel usage and adds NoWidgetPlaceholder.

Changes

Cohort / File(s) Summary
Layout Spec
LAYOUT_SPEC.md
New specification describing Dashboard Layout V2, panel behaviors, persistence keys, and target component changes.
Dashboard Core
packages/inspector/src/dashboard/react/InspectorDashboard.tsx
Main layout rewritten to use RightPanel and NoWidgetPlaceholder; removed bottom-panel plumbing; InspectorDashboard props simplified to { baseUrl }; added persistence for right-panel collapsed state and width; globals collapse persisted; header title updated.
Right-side UI
packages/inspector/src/dashboard/react/components/RightPanel.tsx, packages/inspector/src/dashboard/react/components/NoWidgetPlaceholder.tsx
Added RightPanel: resizable, collapsible sidebar with Agent/Events/Logs tabs, persisted active tab/width, tab counts and clear handlers; added NoWidgetPlaceholder component (star + message) for idle center state.
Panels — header flags
packages/inspector/src/dashboard/react/components/AgentPanel.tsx, .../EventsPanel.tsx, .../LogsPanel.tsx
Added optional showTitle?: boolean and showClearButton?: boolean (default true) to control header title and Clear button when panels are embedded in RightPanel.
Globals & Primitives
packages/inspector/src/dashboard/react/components/GlobalsPanel.tsx, .../McpPrimitivesPanel.tsx
Converted GlobalsPanel into a horizontal globals bar with isCollapsed/onToggleCollapse API and flattened item layout; McpPrimitivesPanel header and collapse control repositioned depending on position.
Toolbar & Icons
packages/inspector/src/dashboard/react/components/Toolbar.tsx
Toolbar API simplified to toggles for primitives (left) and right panel only; removed logs/globals toggles; updated icons and button behaviors accordingly.
Styles & Animations
packages/inspector/src/dashboard/react/styles.ts, .../keyframes.css
Extensive style additions for rightPanel, globals bar, NoWidgetPlaceholder, layout tweaks; added starFloat keyframes; adjusted fonts, logo size, and transition behavior.
Resize utility
packages/inspector/src/dashboard/react/hooks/useResizablePanelWidth.ts
Hook extended with `resizeDirection: "left"

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Toolbar
    participant Dashboard
    participant RightPanel
    participant Panels
    participant Storage as localStorage

    User->>Toolbar: click Right panel toggle
    Toolbar->>Dashboard: onToggleRightPanel()
    Dashboard->>RightPanel: render with isCollapsed flag

    User->>RightPanel: click tab (Agent/Events/Logs)
    RightPanel->>Panels: render selected panel
    RightPanel->>Storage: persist activeTab

    User->>RightPanel: drag resize handle
    RightPanel->>RightPanel: compute width (useResizablePanelWidth with resizeDirection)
    RightPanel->>Storage: persist width

    User->>RightPanel: click collapse
    RightPanel->>Dashboard: onToggleCollapse()
    Dashboard->>Storage: persist collapsed state
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the major layout changes: right panel with tabs, star placeholder, and globals bar reorganization.
Description check ✅ Passed The description provides relevant context about the layout reorganization, covering right panel, left panel, center, globals bar, toolbar changes, and visual polish.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/dashboard-layout-v2

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review: Dashboard Layout V2

Summary

This is a well-executed major UI refactor that reorganizes the Inspector dashboard layout. The changes are substantial (~740 additions, ~350 deletions across 14 files) but focused and intentional. Overall code quality is high with good adherence to the repository's TypeScript standards.


✅ Strengths

Architecture & Design

  • Clean component separation: New RightPanel and NoWidgetPlaceholder components follow single-responsibility principle
  • Consistent state management: Proper use of localStorage for persistence with sensible defaults
  • Type safety: Strong TypeScript usage throughout with no any types introduced
  • Backwards compatibility: Changes maintain existing functionality while reorganizing layout

Code Quality

  • Props interface design: Well-structured with optional parameters and clear documentation
  • React best practices: Proper use of useCallback, useMemo, useEffect with correct dependency arrays
  • CSS-in-JS consistency: Maintains existing styling patterns and naming conventions
  • Accessibility: ARIA labels added to buttons (aria-label, aria-pressed)

Testing & Build

  • According to PR description: Build ✅ and Tests 813/813 ✅ — excellent!

⚠️ Issues Found

1. Missing LogsPanel prop interface extension (Medium Priority)

Location: packages/inspector/src/dashboard/react/components/RightPanel.tsx:1019

The RightPanel uses LogsPanel with showHeader={false}, but LogsPanel.tsx doesn't have showTitle or showClearButton props like EventsPanel and AgentPanel do.

Issue: Inconsistent API across panel components.

Recommendation:

// Add to LogsPanel.tsx LogsPanelProps interface
showTitle?: boolean;
showClearButton?: boolean;

Then update the header rendering to conditionally show title/clear button like the other panels.


2. Potential layout shift on globals bar toggle (Low Priority)

Location: packages/inspector/src/dashboard/react/components/GlobalsPanel.tsx:645-774

The globals bar changes from collapsed (thin bar with button) to expanded (full content) without a fixed height, which could cause jarring layout shifts during screencast playback.

Recommendation: Consider using a fixed or min-height transition to smooth the collapse/expand animation.


3. useResizablePanelWidth direction multiplier logic (Low Priority)

Location: packages/inspector/src/dashboard/react/hooks/useResizablePanelWidth.ts:1223-1226

The new resizeDirection parameter inverts deltaX for right panels, which is correct. However, the hook doesn't validate that resizeDirection is only "left" or "right".

Current code:

const directionMultiplier = resizeDirection === "right" ? -1 : 1;

Recommendation: Add type guard or default case to prevent unexpected behavior if an invalid direction is passed (though TypeScript already enforces this at compile time, runtime safety is good practice).


4. Magic numbers in styles (Informational)

Location: packages/inspector/src/dashboard/react/styles.ts

Several new styles use magic numbers without named constants:

  • Line 1467: gap: "1.5rem" in noWidgetWrapper
  • Line 1498: gap: "0.75rem" in globalsBarContent
  • Line 1360: width: "6px" for resize handle

Impact: Low — these are small, context-specific values.

Recommendation: Consider extracting frequently reused spacing values into named constants at the top of the file for maintainability.


5. Incomplete clear button functionality in RightPanel (Medium Priority)

Location: packages/inspector/src/dashboard/react/components/RightPanel.tsx

The RightPanel header doesn't include a "Clear" button for the active tab, even though the spec mentions it should have one (LAYOUT_SPEC.md:59).

Current: Each panel has its own clear button hidden via showClearButton={false}.

Recommendation: Add a unified clear button in the RightPanel header that clears the currently active tab's content. This would improve UX consistency.


🎯 Best Practices Validation

Type Safety: No any types, strict TypeScript compliance
No Circular Dependencies: Clean import structure
Export Contract: All exports go through package index files
Async Handling: Proper await patterns (no middleware here, but noted)
Test Coverage: All tests passing (813/813)
Formatting: Consistent with ESLint/Prettier rules


🚀 Performance Considerations

Positive

  • Memoization: Good use of useMemo for tabs array in RightPanel (line 954)
  • Conditional rendering: Proper early returns for collapsed panels reduce unnecessary renders
  • CSS transitions: Hardware-accelerated transitions (opacity, width)

Watch Points

  • Auto-scroll in LogsPanel (line 70-74): Runs on every log change. With high-frequency logging, this could cause performance issues. Consider debouncing or using requestAnimationFrame.
  • localStorage writes: Multiple localStorage writes on every state change (lines 207-221 in InspectorDashboard). This is generally fine but could be batched for heavy interaction patterns.

🔒 Security Review

✅ No security concerns identified:

  • No direct DOM manipulation
  • No user input sanitization issues (all text is React-rendered)
  • No external script injection vectors
  • LocalStorage keys are namespaced properly

📝 Documentation

Good

  • LAYOUT_SPEC.md provides excellent context for the changes
  • Component JSDoc comments are clear and consistent
  • Prop interfaces have descriptive comments

Could Improve

  • Consider adding a migration note in CHANGELOG or package README about the UI layout changes
  • The removal of initialPanelHeight and minPanelHeight from InspectorDashboardProps is a breaking change — should be documented if this is a public API

🎨 Visual Polish Notes

Based on the spec and code:

  • ✅ Sirius star logo implementation looks clean
  • ✅ Mirrored panel collapse buttons (left: ◀, right: ▶)
  • ✅ Crystalline star placeholder with floating animation
  • ✅ Dark toolbar button backgrounds to prevent UA white bleed
  • ✅ Matched resize handle widths (6px) across panels

Recommendations Summary

High Priority

None — code is production-ready as-is.

Medium Priority

  1. Add missing prop interface consistency to LogsPanel
  2. Consider adding unified clear button to RightPanel header per spec

Low Priority

  1. Smooth globals bar height transitions
  2. Extract magic number spacing values to constants
  3. Consider debouncing auto-scroll in LogsPanel for performance

Final Verdict

✅ APPROVE with minor suggestions

This is excellent work. The refactor achieves its goals cleanly, maintains type safety, passes all tests, and follows the repository's strict standards. The issues identified are minor and don't block merging.

The code demonstrates strong React/TypeScript fundamentals and thoughtful component design. Great job! 🎉


Reviewed by: Claude Sonnet 4.5
Review Date: 2026-01-31

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/inspector/src/dashboard/react/InspectorDashboard.tsx (1)

29-36: ⚠️ Potential issue | 🟡 Minor

Remove unused props from interface definition.

The InspectorDashboardProps interface defines initialPanelHeight and minPanelHeight (lines 32-35), but these props are not destructured or used in the component signature on line 48. This creates a misleading API surface.

🧹 Proposed fix to clean up unused props
 export interface InspectorDashboardProps {
   /** Base URL for the inspector API (default: current origin) */
   baseUrl?: string;
-  /** Initial panel height in pixels (default: 200) */
-  initialPanelHeight?: number;
-  /** Minimum panel height in pixels (default: 100) */
-  minPanelHeight?: number;
 }

Also applies to: 48-48

🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/Toolbar.tsx`:
- Line 67: The inline JSX comment "Primitives panel toggle - only shown when
session is active" in the Toolbar component is stale because the button is now
rendered unconditionally; remove or update that comment. Locate the comment
inside the Toolbar.tsx JSX (the JSX comment near the primitives panel toggle
button) and either delete it or replace it with an accurate brief note (e.g.,
"Primitives panel toggle") so the comment matches current behavior in the
Toolbar component.
🧹 Nitpick comments (4)
LAYOUT_SPEC.md (1)

5-19: Consider adding language identifiers to ASCII diagram code blocks.

The fenced code blocks for ASCII diagrams lack language identifiers. While these are plain text diagrams, adding text or plaintext as the language identifier would satisfy markdownlint and improve tooling compatibility.

📝 Proposed fix
-```
+```text
 ┌─────────────────────────────────────────────────────┐

Apply the same change to both diagram blocks.

Also applies to: 23-39

packages/inspector/src/dashboard/react/components/RightPanel.tsx (1)

23-25: Consider removing unused clear callbacks from props.

The onClearLogs, onClearEvents, and onClearAgentEvents callbacks are passed to child panels, but since showClearButton={false} is set for all panels, these callbacks will never be invoked. If Clear functionality is intentionally removed from the right panel, consider removing these props to simplify the interface.

Alternatively, if Clear functionality may be re-added later, keeping them is acceptable for forward compatibility.

Also applies to: 126-146

packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx (1)

792-800: Consider consolidating duplicate collapse button logic.

The collapse button rendering logic is duplicated in two places:

  • Lines 792-800: When position="left" with resizeHandleProps
  • Lines 845-853: When position="left" without resizeHandleProps

Both blocks are identical. Consider extracting a helper or restructuring to avoid duplication.

♻️ Proposed refactor

Extract the collapse button into a local variable or helper:

const collapseButton = position === "left" && onToggleCollapse && (
  <button
    style={{ ...localStyles.collapseBtn, marginLeft: "auto" }}
    onClick={onToggleCollapse}
    title="Collapse panel"
  ></button>
);

Then use {collapseButton} in both tabs sections.

Also applies to: 845-853

packages/inspector/src/dashboard/react/components/GlobalsPanel.tsx (1)

43-45: Consider returning null instead of a hidden div.

When the panel is not visible, returning null is cleaner and avoids adding an unnecessary DOM node.

♻️ Proposed simplification
   if (!isVisible) {
-    return <div style={{ display: "none" }} />;
+    return null;
   }

Matches EventsPanel and AgentPanel API for consistent prop interface
across all three panel components.
@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review - PR #135: Dashboard Layout V2

Overview

This PR implements a major layout reorganization of the Inspector dashboard with ~750 additions and ~350 deletions across 12 files. The changes successfully transform the dashboard from a bottom-panel design to a right-panel tabbed interface with improved UX.

✅ Strengths

Architecture & Design

  • Clean separation of concerns: New RightPanel and NoWidgetPlaceholder components are well-isolated
  • Consistent styling: All new components follow the existing color scheme (#0d0e0e backgrounds, #2d2f2f borders, #20b2aa teal accents)
  • Spec-driven development: LAYOUT_SPEC.md provides excellent documentation of the design intent
  • State management: Proper use of localStorage for persistence (panel collapse states, active tab)

Code Quality

  • TypeScript: Strong typing throughout with proper interfaces (RightPanelProps, GlobalsPanelProps)
  • React patterns: Good use of hooks (useMemo, useCallback, useEffect)
  • Accessibility: Proper ARIA labels on buttons (aria-label, aria-pressed)
  • Cleanup: Removed unused props from InspectorDashboardProps (initialPanelHeight, minPanelHeight)

UX Improvements

  • Bidirectional resize: New resizeDirection parameter in useResizablePanelWidth hook supports both left and right panels
  • Visual consistency: Matched panel headers with collapse arrows on appropriate sides
  • Persistent state: Tab selection and panel states preserved across sessions
  • Progressive disclosure: Globals bar only shows when streaming, reducing visual clutter

⚠️ Issues Found

🔴 Critical

1. Missing Clear Functionality in Right Panel

Location: packages/inspector/src/dashboard/react/components/RightPanel.tsx:126-146

The spec requires "Include a 'Clear' button in the panel header" but the implementation hides clear buttons:

showClearButton={false}  // Lines 135, 144

Impact: Users cannot clear logs/events from the right panel, breaking expected functionality.

Recommendation: Either:

  • Add a unified clear button in the panel header that clears the active tab
  • Set showClearButton={true} for each panel
  • Add individual clear buttons next to each tab

2. Unused Props in InspectorDashboard Interface

Location: packages/inspector/src/dashboard/react/InspectorDashboard.tsx:29-36

The interface still declares removed props:

export interface InspectorDashboardProps {
  baseUrl?: string;
  initialPanelHeight?: number;  // ❌ Not used
  minPanelHeight?: number;      // ❌ Not used
}

Impact: Misleading API surface, potential confusion for consumers.

Recommendation: Remove unused props from the interface.


🟡 Moderate

3. Inconsistent Panel Count Display

Location: packages/inspector/src/dashboard/react/components/RightPanel.tsx:112-120

Count badges are always visible even when zero, which differs from typical UI patterns.

Recommendation: Consider hiding counts when zero or styling them differently.


4. Missing Keyboard Navigation

Location: RightPanel.tsx, McpPrimitivesPanel.tsx

Tab switching requires mouse clicks. No keyboard shortcuts (arrow keys, Tab key) are implemented.

Recommendation: Add keyboard event handlers for tab navigation (onKeyDown).


5. No Animation for Star Placeholder

Location: packages/inspector/src/dashboard/react/components/NoWidgetPlaceholder.tsx:14

The spec mentions "CSS animation: gentle floating/bobbing, blinking eyes" but only floating is implemented:

@keyframes starFloat {
  0%, 100% { transform: translateY(0); }
  50% { transform: translateY(-6px); }
}

Recommendation: The current implementation is acceptable (blinking eyes would require SVG animation), but consider documenting this deviation from spec.


6. Globals Bar Toggle Missing

Location: packages/inspector/src/dashboard/react/components/GlobalsPanel.tsx:89-96

The globals bar has a collapse button, but there's no visible UI to re-expand it when collapsed (button is hidden inside collapsed content).

Recommendation: Add a small expand button at the bottom of the screencast area when collapsed.


🟢 Minor

7. Comment Mismatch in Toolbar

Location: packages/inspector/src/dashboard/react/components/Toolbar.tsx:67

Comment says "only shown when session is active" but the button is always rendered:

{/* Primitives panel toggle - only shown when session is active */}
<button ...>  // ← Actually always shown

Recommendation: Remove outdated comment or update logic.


8. Hardcoded Width Values

Location: useResizablePanelWidth.ts:122

const reservedWidth = 400; // Reserve space for main content

This magic number is duplicated in the validation logic.

Recommendation: Extract to a named constant or make it configurable.


9. Default Export Inconsistency

Location: Multiple files (RightPanel.tsx:153, NoWidgetPlaceholder.tsx:20, GlobalsPanel.tsx:123)

Some components have both named and default exports, others only named exports.

Recommendation: Stick to named exports only (per repo's typical pattern) for consistency.


🧪 Test Coverage

Observation: No new tests were added for the new components (RightPanel, NoWidgetPlaceholder) or refactored components (GlobalsPanel).

Recommendation: Add component tests covering:

  • Tab switching in RightPanel
  • State persistence (localStorage)
  • Resize behavior with resizeDirection: 'right'
  • Collapse/expand transitions
  • Globals bar visibility logic

🚀 Performance

Good:

  • useMemo properly used for tabs array
  • No unnecessary re-renders detected
  • Transitions use CSS, not JS animations

Consider:

  • The tabs.map() in RightPanel recalculates on every render when counts change. This is acceptable for small arrays but could be optimized if needed.

♿ Accessibility

Good:

  • ARIA labels present on buttons
  • aria-pressed correctly used for tab state
  • Semantic HTML structure

Missing:

  • Keyboard navigation for tabs
  • Focus management when panels collapse/expand
  • Screen reader announcements for dynamic content updates

🎨 Visual Polish Verification

Based on the spec, these visual requirements were met:

  • ✅ Sirius star logo in header
  • ✅ "sirius-mcp inspector" title
  • ✅ Mirrored panel headers (collapse arrows on matching sides)
  • ✅ Dark toolbar button backgrounds
  • ✅ Matched placeholder fonts and colors
  • ✅ Resize handle widths consistent (6px)

📋 Recommendations Summary

Must Fix (Before Merge)

  1. ❌ Add clear functionality to right panel or explain removal
  2. ❌ Remove unused props from InspectorDashboardProps interface

Should Fix (Follow-up)

  1. Add keyboard navigation for tabs
  2. Add component tests for new UI
  3. Fix globals bar expand UX when collapsed
  4. Update outdated comments

Nice to Have

  1. Extract magic numbers to constants
  2. Standardize export patterns
  3. Consider hiding zero counts in tab badges

✨ Conclusion

This is a high-quality refactor that successfully implements the layout spec with clean code and good TypeScript practices. The architecture is sound and the UX improvements are significant.

Status: ✅ Approve with minor fixes

The critical issues are straightforward to address and don't block the core functionality. Once the unused props are removed and clear button functionality is restored/documented, this is ready to merge.

Great work on the comprehensive layout overhaul! 🎉


Test Status: ✅ Build passing (813/813 tests)
Files Changed: 12 files (+752/-354 lines)
Complexity: Moderate to High (major layout restructure)

- Remove stale comment in Toolbar.tsx
- Remove unused initialPanelHeight/minPanelHeight from InspectorDashboardProps
- Add unified Clear button to RightPanel header (per-tab callbacks)
- Add 'Show Globals' button overlay when globals bar is collapsed
- Hide zero count badges in RightPanel tabs
@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review: PR #135 - Dashboard Layout V2

Summary

Major layout reorganization of the Inspector dashboard with significant UX improvements. The PR successfully implements a modern 3-column layout with tabbed right panel, always-visible left panel, and improved visual polish.

Overall Assessment:APPROVE with minor suggestions


✅ Strengths

Architecture & Design

  • Clean component separation: RightPanel and NoWidgetPlaceholder are well-isolated with clear responsibilities
  • Consistent state management: Proper use of localStorage for persistence across all panels
  • Type safety: Excellent TypeScript usage with no any types (follows repo standards)
  • Backwards compatibility: Props interface changes are additive (e.g., showTitle, showClearButton)

Code Quality

  • Hook usage: Proper useMemo and useCallback to prevent unnecessary re-renders
  • Accessibility: Good use of aria-label and aria-pressed attributes
  • Clean up: Successfully removed unused props (initialPanelHeight, minPanelHeight) from InspectorDashboardProps
  • DRY improvements: Unified panel component API with showTitle/showClearButton props

UX Improvements

  • Persistent state: Active tab, panel widths, and collapse states all persist to localStorage
  • Visual consistency: Mirrored panel headers, matched fonts/colors, consistent resize handles
  • Conditional rendering: Globals bar only shows when streaming, placeholder when idle

🔍 Issues & Suggestions

1. Test Coverage ⚠️ HIGH PRIORITY

Issue: No tests added for new components (RightPanel, NoWidgetPlaceholder)

Impact: According to AGENTS.md line 12, repo has 50% coverage thresholds enforced. New components without tests could break CI.

Recommendation:

// packages/inspector/tests/right-panel.test.tsx
import { render, screen, fireEvent } from '@testing-library/react';
import { RightPanel } from '../src/dashboard/react/components/RightPanel';

describe('RightPanel', () => {
  it('should persist active tab to localStorage', () => {
    // Test tab switching and localStorage
  });
  
  it('should render correct panel based on active tab', () => {
    // Test conditional rendering
  });
  
  it('should handle collapse/expand', () => {
    // Test visibility toggle
  });
});

Files to test:

  • packages/inspector/src/dashboard/react/components/RightPanel.tsx
  • packages/inspector/src/dashboard/react/components/NoWidgetPlaceholder.tsx
  • packages/inspector/src/dashboard/react/components/GlobalsPanel.tsx (horizontal layout changes)

2. Resize Direction Logic 🐛 MEDIUM PRIORITY

File: packages/inspector/src/dashboard/react/hooks/useResizablePanelWidth.ts:1305

Issue: The resizeDirection multiplier logic is correct but could be clearer:

const directionMultiplier = resizeDirection === "right" ? -1 : 1;
const newWidth = Math.min(effectiveMaxWidth, Math.max(minWidth, startWidth + directionMultiplier * deltaX));

Suggestion: Add a comment explaining the inversion:

// Right panels grow leftward (negative deltaX), left panels grow rightward (positive deltaX)
const directionMultiplier = resizeDirection === "right" ? -1 : 1;

3. Type Safety - Hardcoded Strings 📝 LOW PRIORITY

File: packages/inspector/src/dashboard/react/components/RightPanel.tsx:50

Issue: String literal type checking could be more robust:

if (stored === "agent" || stored === "events" || stored === "logs") {
  return stored;
}

Suggestion: Use a type guard or validation array:

const VALID_TABS: RightPanelTab[] = ["agent", "events", "logs"];
const isValidTab = (tab: string): tab is RightPanelTab => 
  VALID_TABS.includes(tab as RightPanelTab);

// Usage:
const stored = window.localStorage.getItem(ACTIVE_TAB_STORAGE_KEY);
if (stored && isValidTab(stored)) {
  return stored;
}

4. Accessibility - Keyboard Navigation ♿ MEDIUM PRIORITY

Files:

  • RightPanel.tsx:117-140
  • McpPrimitivesPanel.tsx:789-811

Issue: Tab buttons don't have keyboard navigation support (arrow keys, Home/End)

Recommendation: Add keyboard handler for better a11y:

const handleKeyDown = (e: React.KeyboardEvent, currentIndex: number) => {
  if (e.key === 'ArrowRight' || e.key === 'ArrowLeft') {
    e.preventDefault();
    const direction = e.key === 'ArrowRight' ? 1 : -1;
    const newIndex = (currentIndex + direction + tabs.length) % tabs.length;
    setActiveTab(tabs[newIndex].id);
  }
};

5. Performance - Conditional Rendering ⚡ LOW PRIORITY

File: RightPanel.tsx:153-173

Issue: All three panel components render but only one is visible:

{activeTab === "logs" && <LogsPanel ... />}
{activeTab === "events" && <EventsPanel ... />}
{activeTab === "agent" && <AgentPanel ... />}

Current: Fine for this use case, but hooks inside panels will re-run on every parent render.

Consideration: If panels become heavier, consider lazy loading or useMemo wrappers.


6. Empty State Consistency 🎨 LOW PRIORITY

Files: Multiple panel components

Good: You fixed empty state centering (InspectorDashboard.tsx:679-707)

Suggestion: The "No agent events", "No logs", etc. messages are defined inline. Consider extracting to constants:

const EMPTY_MESSAGES = {
  agent: "No agent events yet",
  events: "No events recorded",
  logs: "No logs captured",
} as const;

7. Magic Numbers 📊 LOW PRIORITY

File: styles.ts (multiple locations)

Issue: Several magic numbers for sizing:

width: "20px",  // logo
height: "20px",
gap: "0.4rem",  // header spacing

Suggestion: Extract to named constants at top of file:

const SIZES = {
  logoSize: "20px",
  headerGap: "0.4rem",
  // ... etc
} as const;

🔒 Security

No security concerns identified

  • No user input handling in new components
  • No XSS vectors
  • localStorage usage is safe (no sensitive data)

🚀 Performance

Generally good

  • Proper memoization with useMemo and useCallback
  • Efficient event handlers
  • No unnecessary re-renders detected

Minor note: Consider virtualizing logs/events lists if they grow very large (not urgent).


📝 Documentation

Good JSDoc comments on new components

Enhancement: Consider adding inline comments for complex layout logic in InspectorDashboard.tsx:450-527 explaining the 3-column structure.


✅ Adherence to Repo Standards (AGENTS.md)

Standard Status Notes
TypeScript strict mode ✅ Pass No any types used
Export contract ✅ Pass Components exported properly
Zod v4 N/A No schema validation in this PR
Express v5 N/A Frontend-only changes
Tests required ⚠️ Missing Need tests for new components
Coverage thresholds ⚠️ Risk May fail CI without tests
No circular deps ✅ Pass Clean imports
await next() in middleware N/A No middleware changes

🎯 Final Checklist

Before merge:

  • Add tests for RightPanel component
  • Add tests for NoWidgetPlaceholder component
  • Add tests for GlobalsPanel horizontal layout
  • Run pnpm build && pnpm test && pnpm lint && pnpm typecheck (per AGENTS.md line 53)
  • Verify coverage thresholds pass

Optional enhancements (can be follow-up PRs):

  • Add keyboard navigation to tabs
  • Add unit test for resize direction logic
  • Extract magic numbers to constants
  • Consider empty state message constants

🎉 Conclusion

Excellent work on this major UI refactor! The code quality is high, the architecture is clean, and the UX improvements are significant. The main blocker is test coverage for the new components per repo standards.

Recommendation: Add tests for the new components, then this is ready to merge.

Great job! 🚀


Review conducted following repository standards in /AGENTS.md and TypeScript/React best practices.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/RightPanel.tsx`:
- Around line 47-61: The current activeTab initializer and useEffect access
window.localStorage directly which can throw in some browsers; wrap both getItem
and setItem calls in try/catch and fall back to the default ("agent") on error.
Specifically, update the state initializer (the useState(() => { ... }) block
referencing ACTIVE_TAB_STORAGE_KEY and window.localStorage.getItem) to catch
exceptions and return "agent" if reading fails, and update the useEffect that
calls window.localStorage.setItem(ACTIVE_TAB_STORAGE_KEY, activeTab) to perform
the set inside a try/catch (no-op on error); keep the existing typeof window !==
"undefined" guards.

In `@packages/inspector/src/dashboard/react/InspectorDashboard.tsx`:
- Around line 125-142: The localStorage reads for persisted state (in the
useState initializers for isRightPanelCollapsed and isGlobalsBarCollapsed) and
any corresponding writes when toggling those states must be wrapped in try/catch
to avoid runtime exceptions when storage is unavailable or blocked; update the
initializer logic in InspectorDashboard so each useState uses a safe read that
returns false on error, and update any setter handlers (the functions that call
localStorage.setItem for "mcp-dashboard-right-panel-collapsed" and
"mcp-dashboard-globals-bar-collapsed") to catch and ignore/storage-fail
gracefully, preserving the in-memory state even if localStorage fails.
🧹 Nitpick comments (1)
packages/inspector/src/dashboard/react/components/Toolbar.tsx (1)

67-88: Add aria-pressed to expose toggle state.
This makes the two buttons act like proper pressed toggles for assistive tech.

Suggested diff
       <button
         style={{
           ...styles.toolbarBtn,
           ...(isPrimitivesPanelVisible ? styles.toolbarBtnActive : {}),
         }}
         onClick={onTogglePrimitivesPanel}
         title={isPrimitivesPanelVisible ? "Hide MCP Primitives" : "Show MCP Primitives"}
         aria-label={isPrimitivesPanelVisible ? "Hide MCP Primitives" : "Show MCP Primitives"}
+        aria-pressed={isPrimitivesPanelVisible}
       >
         <PrimitivesIcon />
       </button>
       <button
         style={{
           ...styles.toolbarBtn,
           ...(isRightPanelVisible ? styles.toolbarBtnActive : {}),
         }}
         onClick={onToggleRightPanel}
         title={isRightPanelVisible ? "Hide Right Panel" : "Show Right Panel"}
         aria-label={isRightPanelVisible ? "Hide Right Panel" : "Show Right Panel"}
+        aria-pressed={isRightPanelVisible}
       >
         <RightPanelIcon />
       </button>

@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review: Dashboard Layout V2

This is a substantial UI refactoring that successfully reorganizes the Inspector dashboard. The implementation is solid with good attention to detail. Here are my findings:

✅ Strengths

Architecture & Design:

  • Clean separation of concerns with the new RightPanel component
  • Consistent state management using localStorage for persistence
  • Well-structured IIFE for handleClear logic (lines 82-90 in RightPanel.tsx)
  • Good use of useMemo for tabs array to prevent unnecessary re-renders

Code Quality:

  • TypeScript types are properly defined throughout
  • Follows existing component patterns and styling conventions
  • CSS transitions are smooth and consistent (0.15s-0.25s ease)
  • Proper cleanup in resize hooks (useResizablePanelWidth.ts:90-103)

UX Polish:

  • Mirrored collapse arrows (◀ left, ▶ right) for visual symmetry
  • Tab counts provide useful context
  • Persisted preferences (active tab, panel states, widths)
  • Bidirectional resize support with resizeDirection parameter

🔍 Code Quality Observations

1. RightPanel.tsx - Early Returns & Conditional Rendering
The component returns early when collapsed (line 78-80), which is efficient, but the resize handle is then lost. The current implementation renders the resize handle outside the conditional (lines 99-106), which is correct. ✅

2. useResizablePanelWidth.ts - Direction Multiplier Logic
Lines 113-126: The directionMultiplier approach is clever and clean:

const directionMultiplier = resizeDirection === 'right' ? -1 : 1;
const newWidth = Math.min(effectiveMaxWidth, Math.max(minWidth, startWidth + directionMultiplier * deltaX));

This elegantly handles both left-to-right and right-to-left panel resizing. ✅

3. GlobalsPanel.tsx - Horizontal Layout Conversion
The conversion from vertical sections to horizontal key-value pairs is well done. The component properly handles both collapsed and expanded states.

⚠️ Minor consideration: The globalsBarLoading message is "Loading environment…" but there's no actual loading state tracking. This is just a fallback when globals is null, which is fine but the semantics could be clearer.

4. NoWidgetPlaceholder.tsx - Animation
The starFloat keyframe animation is smooth, but the component is very simple (20 lines including imports). Consider whether this warrants a separate file vs. inline definition in InspectorDashboard. Current approach is fine for maintainability. ✅

🐛 Potential Issues

1. InspectorDashboard.tsx - Conditional Display Logic ⚠️
Lines 408-449: The screencast display condition changed from hasActiveSession to isStreaming:

{isStreaming ? (/* screencast */) : (<NoWidgetPlaceholder />)}

Question: What happens if there's an active session but streaming hasn't started yet? The old logic showed primitives in center, the new logic shows the placeholder. Is this the intended behavior? This might cause a flash of placeholder before streaming starts.

Recommendation: Verify the state transition flow. Consider:

{hasActiveSession && isStreaming ? (/* screencast */) : hasActiveSession ? (/* loading state? */) : (<NoWidgetPlaceholder />)}

2. RightPanel.tsx - Noop Function Pattern ℹ️
Line 46: const noop = (): void => undefined;

This is defined inside the component, creating a new function instance on every render. While this works, it could be hoisted outside the component for better performance:

const noop = (): void => undefined;  // Outside component

export function RightPanel({ ... }) {
  // ...
}

Impact is minimal, but follows best practices.

3. Missing ARIA Labels ⚠️

  • Toolbar.tsx: Buttons have aria-label
  • RightPanel.tsx: Tab buttons use aria-pressed (line 126) ✅ but Clear button (lines 143-149) lacks aria-label
  • GlobalsPanel.tsx: Expand button (lines 697-704) lacks aria-label

Recommendation: Add aria-labels for better accessibility:

<button aria-label="Clear current tab items" ...>Clear</button>
<button aria-label="Show environment variables" ...>Show Environment</button>

📊 Performance Considerations

1. Style Object Spreading
Throughout the components, styles are spread multiple times:

style={{
  ...styles.rightPanel,
  width: isCollapsed ? 0 : panelWidth,
  ...(isCollapsed ? styles.rightPanelCollapsed : {}),
}}

This is fine and follows the existing pattern. The ternary for conditional spreads is efficient. ✅

2. LocalStorage Sync
Multiple useEffect hooks persist state to localStorage. These are properly debounced (e.g., resize hook only saves when !isResizing at line 79 in useResizablePanelWidth). ✅

🔒 Security Concerns

LocalStorage Usage: Multiple values are stored/retrieved from localStorage:

  • Panel widths
  • Collapsed states
  • Active tab selection

The code properly validates retrieved values:

  • Width values are clamped to min/max ranges (useResizablePanelWidth.ts:42-47) ✅
  • Tab selection validates against known tabs (RightPanel.tsx:50) ✅

No XSS or injection risks identified. ✅

🧪 Test Coverage

Based on the PR description: "Build ✅ | Tests 813/813 ✅"

All tests are passing, which is excellent. However, I cannot verify:

  • Are there tests for the new RightPanel component?
  • Are there tests for the bidirectional resize behavior?
  • Are there tests for localStorage persistence?

Recommendation: Ensure test coverage for the new components matches the 50% threshold specified in AGENTS.md:51.

📝 Documentation

LAYOUT_SPEC.md: Excellent addition! This spec document clearly describes the changes and provides visual diagrams. This is great for future maintainers. ✅

Missing:

  • No updates to package READMEs or component-level documentation
  • The InspectorDashboardProps interface removed initialPanelHeight and minPanelHeight props (lines 154-157 removed) - this is a breaking API change that should be noted in a CHANGELOG if this is a public API.

🎨 Visual/UX Considerations

1. Logo Size Change
Lines 1369-1372 in styles.ts: Logo reduced from 28px to 20px. Title gap reduced from 0.75rem to 0.4rem. This makes the header more compact, which is good for space but ensure it's still readable on different displays.

2. Globals Bar Collapsed State
The globals bar collapse button appears only when streaming (line 425). The "Show Globals" button is quite small (0.625rem font). Consider if this is accessible enough on mobile/touch devices.

3. Tab Styling
The right panel tabs use very small fonts (0.6875rem for tab text, 0.5625rem for counts). Ensure this is legible at 100% zoom on 1080p displays.

📋 Code Style & Conventions

Follows repo conventions: ✅

  • Uses strict TypeScript (no any types)
  • Proper use of React.CSSProperties type casting
  • Consistent with existing ESLint/Prettier style
  • Follows export patterns (re-exports through index.ts)

Zod v4 / Express v5: N/A for this PR (UI-only changes)

🚀 Overall Assessment

Score: 8.5/10

This is a high-quality refactoring that significantly improves the dashboard layout. The code is well-structured, follows TypeScript best practices, and maintains consistency with the existing codebase.

Before merging:

  1. ✅ Verify the isStreaming vs hasActiveSession conditional logic (see issue Add Claude Code GitHub Workflow #1)
  2. ⚠️ Add missing aria-label attributes for accessibility
  3. ℹ️ Consider hoisting the noop function
  4. 📋 Verify test coverage for new components
  5. 📋 Document the breaking API change if InspectorDashboardProps is public

Nice work overall! The mirrored collapse arrows, persisted state, and clean component separation are all excellent touches. 🌟

@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review: Dashboard Layout V2

Overview

This is a substantial UI reorganization (~800 additions, ~360 deletions across 15 files) that restructures the inspector dashboard layout. The PR description states that builds and tests (813/813) are passing. ✅


✅ Strengths

1. Well-Documented Design

  • The LAYOUT_SPEC.md provides excellent documentation of the changes with ASCII diagrams
  • Clear before/after layout comparison
  • Specific implementation requirements listed

2. Clean Component Separation

  • New RightPanel.tsx component properly encapsulates tabbed sidebar logic
  • NoWidgetPlaceholder.tsx is a focused, single-purpose component
  • Good separation of concerns with the horizontal GlobalsPanel refactor

3. Proper State Management

  • Consistent use of localStorage for persistence (panel states, active tab)
  • Effect hooks properly sync state to localStorage
  • Good use of useCallback for memoized handlers

4. Type Safety

  • Proper TypeScript types throughout
  • Good use of union types (e.g., RightPanelTab = "agent" | "events" | "logs")
  • Props interfaces are well-defined

5. Accessibility

  • aria-label and aria-pressed attributes on interactive elements
  • Proper button roles and titles

⚠️ Issues & Concerns

1. CRITICAL: Missing Resize Direction Logic 🔴

File: packages/inspector/src/dashboard/react/hooks/useResizablePanelWidth.ts:1313-1326

The resize logic assumes left-to-right dragging, but the right panel needs right-to-left behavior. While resizeDirection parameter is added, the implementation may not work correctly:

const directionMultiplier = resizeDirection === "right" ? -1 : 1;
const newWidth = Math.min(
  effectiveMaxWidth,
  Math.max(minWidth, startWidth + directionMultiplier * deltaX)
);

Issue: For a right-side panel, when you drag the resize handle LEFT (negative deltaX), the panel should GROW. The multiplier inverts the delta, but you may need to verify this works as expected in practice.

Recommendation: Add integration tests or manually verify that:

  • Dragging right panel handle LEFT increases panel width
  • Dragging right panel handle RIGHT decreases panel width

2. Potential Bug: Conditional Display Logic 🟡

File: packages/inspector/src/dashboard/react/InspectorDashboard.tsx:419-458

The center display logic changed from:

const hasActiveSession = !!selectedSessionId && !!displayImageData;

To:

{isStreaming ? (
  /* Screencast */
) : (
  /* NoWidgetPlaceholder */
)}

Issue: The isStreaming flag may have different semantics than hasActiveSession. This could cause:

  • Placeholder showing when a session exists but isn't streaming yet
  • Incorrect conditional rendering if isStreaming doesn't match the previous logic

Recommendation: Verify that isStreaming accurately represents when the screencast should display vs. placeholder. Consider adding a comment explaining the logic.


3. Potential State Inconsistency 🟡

File: packages/inspector/src/dashboard/react/components/RightPanel.tsx:1044-1052

const handleClear = (() => {
  if (activeTab === "agent") {
    return onClearAgent ?? noop;
  }
  if (activeTab === "events") {
    return onClearEvents ?? noop;
  }
  return onClearLogs ?? noop;
})();

Issue: This IIFE runs on every render. While not a major performance issue, it's unnecessary work.

Recommendation: Move this into a useMemo or useCallback:

const handleClear = useCallback(() => {
  if (activeTab === "agent") {
    return onClearAgent?.() ?? noop();
  }
  if (activeTab === "events") {
    return onClearEvents?.() ?? noop();
  }
  return onClearLogs?.() ?? noop();
}, [activeTab, onClearAgent, onClearEvents, onClearLogs]);

4. Prop Type Inconsistency 🟡

File: Multiple panel components (AgentPanel.tsx, EventsPanel.tsx, LogsPanel.tsx)

All three panel components now accept showTitle and showClearButton props, but the pattern differs between components:

  • LogsPanel: Uses showHeader={false} in RightPanel (line 1116)
  • EventsPanel & AgentPanel: Use showHeader={true} but hide individual elements (lines 1120-1134)

Recommendation: Standardize the approach. Either:

  • Always use showHeader={false} and let the parent handle headers, OR
  • Always use fine-grained control (showTitle, showClearButton)

Pick one pattern and apply consistently.


5. Missing Animation Keyframe Risk 🟡

File: packages/inspector/src/dashboard/react/styles.ts:1590

noWidgetStar: {
  animation: "starFloat 4s ease-in-out infinite",
  // ...
}

Issue: The starFloat animation is defined in keyframes.css:1358-1366. If that CSS file isn't loaded or bundled correctly, the animation will silently fail.

Recommendation:

  • Ensure keyframes.css is imported in the component/bundle
  • Consider adding a comment referencing the CSS file location
  • Test that the animation works in production builds

6. localStorage Keys Could Conflict 🟡

Multiple localStorage keys are used:

  • "mcp-dashboard-right-panel-collapsed"
  • "mcp-dashboard-right-panel-tab"
  • "mcp-dashboard-globals-bar-collapsed"
  • "mcp-dashboard-right-panel-width"

Issue: If multiple inspector instances run on different ports/domains, these keys could collide or persist incorrectly.

Recommendation: Consider namespacing keys with connection ID or base URL if multi-instance scenarios are possible.


7. Globals Bar Visibility Logic 🟡

File: packages/inspector/src/dashboard/react/InspectorDashboard.tsx:492-500

{isStreaming && (
  <GlobalsPanel
    globals={displayGlobals}
    isVisible={true}
    isCollapsed={isGlobalsBarCollapsed}
    onToggleCollapse={toggleGlobalsBar}
  />
)}

Issue: The GlobalsPanel component checks if (!isVisible) return <div style={{ display: "none" }} /> (line 674), but the parent always passes isVisible={true}. This makes the isVisible prop redundant.

Recommendation: Remove the isVisible prop entirely since visibility is controlled by conditional rendering in the parent.


🔍 Performance Considerations

Positive:

  • Proper use of useMemo for computed values (tabs array in RightPanel:1025-1032)
  • State updates are batched appropriately
  • No obvious N+1 rendering issues

Concerns:

  • The resize handlers add mouse event listeners to document. Ensure these are properly cleaned up (they appear to be in the cleanup function, which is good)
  • Multiple useEffect hooks for localStorage persistence - consider consolidating if performance becomes an issue

🔒 Security Considerations

Low Risk:

  • localStorage usage is safe (no sensitive data)
  • No XSS vectors introduced
  • No unsafe DOM manipulation
  • Image sources are from bundled assets (sirius-star.png, logo.png)

Note:

  • The displayImageData rendering (<img src={displayImageData}) should be validated elsewhere in the codebase to ensure it's sanitized base64 data

🧪 Test Coverage

Missing Tests:

This is a major UI refactor but the PR doesn't include new tests for:

  1. RightPanel component behavior
  2. NoWidgetPlaceholder rendering
  3. GlobalsPanel horizontal layout
  4. Panel collapse/expand state persistence
  5. Tab switching and localStorage sync
  6. Resize handle direction logic (especially critical for the right panel)

Recommendation: According to AGENTS.md:51-53:

ALWAYS add/adjust tests under the owning package's tests/ when changing behavior; coverage thresholds are enforced at the repo level.

While the PR states "813/813 tests passing", these are likely existing tests. New UI components should have corresponding tests to:

  • Verify panel state management
  • Test resize behavior
  • Validate localStorage persistence
  • Ensure proper rendering of new components

📋 Additional Recommendations

1. Toolbar Icon Semantics

The RightPanelIcon in Toolbar.tsx:1201-1220 uses the same SVG as the old GlobalsIcon. Consider using a distinct icon to avoid user confusion since the panel now contains Agent/Events/Logs, not globals.

2. Remove Unused Props

InspectorDashboard removed initialPanelHeight and minPanelHeight props (lines 154-157). Verify that no external code relies on these props to avoid breaking changes.

3. Console Warning Check

Ensure no React warnings in console:

  • Keys on mapped elements (tabs, events, logs)
  • Proper event handler types
  • No state updates on unmounted components

4. Accessibility Audit

  • Verify keyboard navigation works for all new panels
  • Test screen reader announcements for panel state changes
  • Ensure focus management when collapsing/expanding panels

🎯 Summary

Category Status
Code Quality ✅ Good
Architecture ✅ Clean separation of concerns
Type Safety ✅ Proper TypeScript usage
Performance 🟡 Minor optimizations possible
Security ✅ No concerns
Test Coverage 🔴 Missing tests for new components
Documentation ✅ Excellent spec document

Action Items (Priority Order):

  1. 🔴 HIGH: Add tests for new components (RightPanel, NoWidgetPlaceholder, refactored GlobalsPanel)
  2. 🔴 HIGH: Verify resize direction logic works correctly for right panel
  3. 🟡 MEDIUM: Validate isStreaming vs hasActiveSession logic equivalence
  4. 🟡 MEDIUM: Optimize handleClear with useCallback
  5. 🟡 MEDIUM: Standardize panel header visibility pattern
  6. 🟢 LOW: Consider localStorage key namespacing
  7. 🟢 LOW: Remove redundant isVisible prop from GlobalsPanel

Verdict:

Approve with recommendations. The code is well-structured and follows repository conventions, but the lack of tests for significant UI changes is concerning per AGENTS.md requirements. The resize direction logic needs verification before merge.

Great work on the comprehensive spec document and clean component architecture! 🌟

@claude
Copy link
Contributor

claude bot commented Jan 31, 2026

Code Review: Dashboard Layout V2

Overview

This PR implements a major UI reorganization of the Inspector dashboard. The changes are well-structured and follow the repository's coding standards. Overall, this is a solid implementation with good attention to detail.


✅ Code Quality & Best Practices

Strengths:

  • Clean architecture: The new RightPanel and NoWidgetPlaceholder components are well-isolated with clear responsibilities
  • Type safety: All new components use proper TypeScript interfaces with no any types (adheres to repo's strict TS rules)
  • State management: Good use of useMemo, useEffect, and proper dependency arrays
  • Accessibility: ARIA labels on buttons (aria-label, aria-pressed) improve screen reader support
  • Error handling: Try-catch blocks around localStorage access handle private browsing mode gracefully
  • Consistency: Follows existing patterns (storage keys, transition timings, color schemes)

Code highlights:

  • RightPanel.tsx:47-59: Safe localStorage initialization with fallback to "agent" default
  • InspectorDashboard.tsx:204-225: Proper cleanup of old panel-related state and localStorage keys
  • useResizablePanelWidth.ts:322-338: Bidirectional resize support with proper direction multiplier

🐛 Potential Issues

1. Missing resize handle when panel is collapsed (Minor)

RightPanel.tsx:106-114 - The resize handle is rendered even when isCollapsed is true (though the parent returns early at line 87, the resize handle is outside the early return). This could lead to unexpected DOM structure.

Suggested fix:

if (isCollapsed) {
  return <div style={panelStyle} />;
}

return (
  <>
    <div {...resizeHandleProps} /* ... */ />
    <div style={panelStyle}>
      {/* panel content */}
    </div>
  </>
);

2. Potential memory leak in connection cache (Low)

InspectorDashboard.tsx:57 - The connectionCacheRef Map grows indefinitely as connections are created. When connections are closed, their cached state is never cleaned up.

Suggested fix: Add cleanup in the connection close handler or add a useEffect that prunes stale connection IDs.

3. Inconsistent null-safety in GlobalsPanel (Minor)

GlobalsPanel.tsx:89-92 - The destructuring doesn't handle the case where individual properties might be undefined/null before checking them in the Item component.

Example: If globals.theme is undefined, it still attempts to render it. The Item component handles this, but it's better to be defensive at the destructuring level.


⚡ Performance Considerations

Positive:

  • useMemo for tabs array in RightPanel.tsx:71-78 prevents unnecessary re-renders
  • ✅ Conditional rendering of GlobalsPanel only when streaming
  • ✅ Early return for collapsed panels reduces DOM updates

Potential optimizations:

  1. handleClear IIFE (RightPanel.tsx:90-98) - This function is recreated on every render. Consider using useCallback:

    const handleClear = useCallback(() => {
      if (activeTab === "agent") return onClearAgent ?? noop;
      if (activeTab === "events") return onClearEvents ?? noop;
      return onClearLogs ?? noop;
    }, [activeTab, onClearAgent, onClearEvents, onClearLogs]);
  2. Panel content components - All three panels (LogsPanel, EventsPanel, AgentPanel) are rendered conditionally, which is good. However, consider using React.memo on these child components if they're re-rendering unnecessarily.


🔒 Security Concerns

✅ No security issues identified.

  • localStorage access is properly wrapped in try-catch blocks
  • No user input is directly rendered without sanitization
  • No dynamic script execution or innerHTML usage
  • Event handlers are properly typed and scoped

🧪 Test Coverage

Observations:

  • The PR description mentions "Tests 813/813 ✅" - excellent!
  • No test files were modified in this PR, which suggests existing tests still pass

Recommendations:

  1. Consider adding specific tests for:

    • Right panel tab switching and persistence
    • Resize handle behavior for both left and right panels
    • Globals bar collapse/expand states
    • NoWidgetPlaceholder rendering when no session is active
  2. Test localStorage failure scenarios (important for private browsing mode)

  3. Test the new resize direction logic in useResizablePanelWidth (the resizeDirection parameter)


📝 Additional Observations

Documentation

  • LAYOUT_SPEC.md is a nice addition for documenting the design rationale
  • Consider adding JSDoc comments to the new resize direction parameter in useResizablePanelWidth.ts:322

Styling

  • The new star animation (keyframes.css:10-18) is clean and performant
  • The globals bar horizontal layout is more space-efficient
  • Color consistency maintained (#20b2aa teal accents)

Binary assets

  • Two new/modified image files (logo.png, sirius-star.png) - ensure these are optimized for web (consider WebP format for better compression)

🎯 Summary

Overall Assessment: ✅ Approve with minor suggestions

This is a well-executed UI refactoring that:

  • ✅ Follows repository conventions (TypeScript strict mode, Zod v4, Express v5 compatibility)
  • ✅ Maintains backwards compatibility (localStorage keys properly migrated)
  • ✅ Improves UX (tabbed panels, better space utilization)
  • ✅ Includes proper accessibility features
  • ⚠️ Has minor issues that should be addressed but don't block merging

Priority fixes:

  1. Fix resize handle rendering when collapsed (low impact, but cleaner DOM)
  2. Add connection cache cleanup to prevent memory growth over time
  3. Consider the performance optimizations for handleClear

Nice-to-haves:

  • Add tests for the new components
  • Optimize binary assets
  • Document the resize direction parameter

Great work overall! The reorganization significantly improves the dashboard's usability and maintains high code quality standards. 🌟

@gabrypavanello gabrypavanello merged commit 71a6cca into main Jan 31, 2026
4 checks passed
@gabrypavanello gabrypavanello deleted the feat/dashboard-layout-v2 branch January 31, 2026 19:49
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