Unified Sidebar — Servers, Primitives & Connection Form - #156
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a server-centric MCP primitives sidebar, persistent stopped-connection storage with start/stop/delete flows, per-connection primitives caching, a SidebarConnectionForm for HTTP/stdio connections, a detailed PrimitiveDetail viewer/executor, tests, and InspectorDashboard wiring and UI-state persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarForm as SidebarConnectionForm
participant Dashboard as InspectorDashboard
participant MCP as MCP_Server
User->>SidebarForm: Fill transport & params, Click Connect
SidebarForm->>SidebarForm: Validate & build ConnectionParams
SidebarForm->>Dashboard: onConnect(params)
Dashboard->>MCP: Open connection (HTTP or stdio)
MCP-->>Dashboard: Connection established
Dashboard->>Dashboard: Add live server to serverDataList
Dashboard-->>SidebarForm: Signal success (close form)
Dashboard-->>User: Sidebar shows new server block
sequenceDiagram
participant User
participant Panel as McpPrimitivesPanel
participant Dashboard as InspectorDashboard
participant Detail as PrimitiveDetail
participant MCP as MCP_Server
User->>Panel: Select primitive (serverId, kind, name)
Panel->>Dashboard: onSelectPrimitive(...)
Dashboard->>Dashboard: Resolve primitive (live OR cached)
Dashboard->>Detail: Render resolved primitive
User->>Detail: Execute with params
Detail->>Dashboard: onExecute(primitive, params)
Dashboard->>MCP: Execute primitive on server
MCP-->>Dashboard: Execution result
Dashboard->>Detail: Return result
Detail-->>User: Display response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
Code Review: PR #156 - Unified SidebarSummaryThis PR implements a major UX redesign consolidating server management, primitives browsing, and connection controls into a unified left sidebar. Adds ~3000 lines across 3 new components. ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx`:
- Around line 1028-1029: The current branch in PrimitiveDetail.tsx assigns
params[name] = parseFloat(val) when inputType === "number" but parseFloat can
produce NaN; update this to validate the parsed value: call const parsed =
parseFloat(val) and if Number.isNaN(parsed) do not assign NaN to params[name]
(either delete/skip the key or assign null/undefined) and set/trigger a
validation error state for the field (e.g., via existing form error handler or
setState) so invalid numeric input is not sent to the server; keep the
assignment only when Number.isFinite(parsed).
In `@packages/inspector/src/dashboard/react/components/SidebarConnectionForm.tsx`:
- Around line 202-206: The current args splitting in SidebarConnectionForm uses
trimmedArgs.split(/\s+/) which does not respect quoted strings despite the
comment; update the implementation so that when assigning (params as
Extract<ConnectionParams, { transport: "stdio" }>).args you parse trimmedArgs
with a quoted-string-aware splitter (e.g., implement and call a splitArgs
function that uses a regex to capture tokens and quoted groups) or, if quoted
handling is not required, change the comment to accurately state that splitting
is simple whitespace-only; ensure the change is applied where trimmedArgs is
transformed and assigned to params.args so flags like --config "path with
spaces" remain a single argument.
In `@packages/inspector/src/dashboard/react/InspectorDashboard.tsx`:
- Around line 594-598: The restart logic in InspectorDashboard.tsx currently
always constructs ConnectionParams with transport: "http" (const params:
ConnectionParams = { transport: "http", url: conn.url }), which loses stdio
command/args; update the code to read the original transport and params from the
connection instead of assuming HTTP—store the original ConnectionParams on the
connection object when the connection is created (e.g., attach
conn.connectionParams or conn.originalParams) and, in the restart handler, use
conn.connectionParams if present; if not present, derive transport from existing
metadata on conn (e.g., conn.type or conn.transport) and only reconstruct
url-based HTTP params as a fallback so stdio connections preserve their
command/args.
In `@packages/inspector/tests/unified-sidebar.test.ts`:
- Around line 14-16: The test suite is still disabled with describe.skip in
unified-sidebar.test.ts while SidebarConnectionForm and PrimitiveDetail are
implemented; remove or replace describe.skip to enable the tests (or if tests
are still incomplete, update the top-of-file comment to accurately state current
status) and then run/adjust tests to pass; specifically locate the
describe.skip(...) block in unified-sidebar.test.ts and either change it to
describe(...) to enable tests and fix any failing assertions against
SidebarConnectionForm and PrimitiveDetail, or update the placeholder comment
text to reflect that those components are implemented and add new tests to cover
their functionality to meet the 50% coverage requirement.
🧹 Nitpick comments (4)
packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx (2)
795-804: DeprecatedexecCommand('copy')fallback.The
document.execCommand('copy')API is deprecated. While it's used as a fallback for older browsers, consider whether this fallback is still necessary for your target browser support. If modern browsers are the target, the fallback could be simplified to just show an error message.
1345-1347: Consider alternative to eslint-disable for unused variable.The destructuring pattern to exclude
kindrequires an eslint-disable comment. A cleaner approach could use a helper or direct property omission:Alternative approach
- // Extract data without the kind field for JSON export - // eslint-disable-next-line `@typescript-eslint/no-unused-vars` - const { kind, ...primitiveData } = primitive; + // Extract data without the kind field for JSON export + const primitiveData = Object.fromEntries( + Object.entries(primitive).filter(([key]) => key !== "kind") + );Or keep the current pattern if you prefer its clarity—the eslint-disable is acceptable here.
packages/inspector/src/dashboard/react/components/SidebarConnectionForm.tsx (1)
167-170: ConsiderrequestAnimationFrameinstead ofsetTimeoutfor focus.The 50ms delay is a reasonable workaround, but
requestAnimationFramewould be more reliable for waiting until after render:Alternative approach
// Focus the appropriate input after a brief delay - setTimeout(() => { + requestAnimationFrame(() => { urlInputRef.current?.focus(); - }, 50); + });packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx (1)
143-145: Type guard could be more robust.The current guard checks for
"position"and"tools"properties. While this works, checking for"servers"(unique to new API) would be more explicit:Alternative type guard
function isLegacyProps(props: McpPrimitivesPanelProps): props is McpPrimitivesPanelLegacyProps { - return "position" in props && "tools" in props; + return !("servers" in props); }This is cleaner since
serversis required in the new API and absent in legacy.
| if (inputType === "number" && typeof val === "string") { | ||
| params[name] = parseFloat(val); |
There was a problem hiding this comment.
parseFloat may produce NaN for invalid input.
If the user enters non-numeric text in a number field, parseFloat(val) returns NaN, which will be sent to the server. Consider validating the parsed value:
Proposed fix
if (inputType === "number" && typeof val === "string") {
- params[name] = parseFloat(val);
+ const parsed = parseFloat(val);
+ if (!Number.isNaN(parsed)) {
+ params[name] = parsed;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (inputType === "number" && typeof val === "string") { | |
| params[name] = parseFloat(val); | |
| if (inputType === "number" && typeof val === "string") { | |
| const parsed = parseFloat(val); | |
| if (!Number.isNaN(parsed)) { | |
| params[name] = parsed; | |
| } |
🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx` around
lines 1028 - 1029, The current branch in PrimitiveDetail.tsx assigns
params[name] = parseFloat(val) when inputType === "number" but parseFloat can
produce NaN; update this to validate the parsed value: call const parsed =
parseFloat(val) and if Number.isNaN(parsed) do not assign NaN to params[name]
(either delete/skip the key or assign null/undefined) and set/trigger a
validation error state for the field (e.g., via existing form error handler or
setState) so invalid numeric input is not sent to the server; keep the
assignment only when Number.isFinite(parsed).
| * NOTE: This test file is SKIPPED until TASK-026 is fully implemented. | ||
| * Components like SidebarConnectionForm and PrimitiveDetail are part of later subtasks. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the components exist
echo "=== Looking for component files ==="
find packages/inspector -name "*.tsx" -o -name "*.ts" | grep -E "(SidebarConnectionForm|PrimitiveDetail)" | head -20
echo ""
echo "=== Listing all tsx files in packages/inspector ==="
fd -t f '\.tsx$' packages/inspector | head -30
echo ""
echo "=== Checking test file status ==="
cat packages/inspector/tests/unified-sidebar.test.ts | head -25Repository: AndurilCode/mcp-apps-kit
Length of output: 2769
🏁 Script executed:
#!/bin/bash
echo "=== SidebarConnectionForm.tsx content ==="
wc -l packages/inspector/src/dashboard/react/components/SidebarConnectionForm.tsx
head -40 packages/inspector/src/dashboard/react/components/SidebarConnectionForm.tsx
echo ""
echo "=== PrimitiveDetail.tsx content ==="
wc -l packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx
head -40 packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx
echo ""
echo "=== Check if test file has describe.skip ==="
grep -n "describe.skip" packages/inspector/tests/unified-sidebar.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2771
Enable tests for implemented components or update outdated placeholder comment.
The components referenced in the test file comment are now fully implemented:
SidebarConnectionForm.tsxcontains 370 lines of actual implementationPrimitiveDetail.tsxcontains 1458 lines of actual implementation
However, the entire test suite remains skipped with describe.skip() and the comment claims these "are part of later subtasks" and "not yet implemented." This test file should either have its tests enabled to cover the implemented functionality or the placeholder comment updated to reflect current status. Leaving nearly 1,800 lines of new component code untested while the entire test suite is skipped risks coverage threshold violations per repository requirements (50% lines/functions/branches/statements).
🤖 Prompt for AI Agents
In `@packages/inspector/tests/unified-sidebar.test.ts` around lines 14 - 16, The
test suite is still disabled with describe.skip in unified-sidebar.test.ts while
SidebarConnectionForm and PrimitiveDetail are implemented; remove or replace
describe.skip to enable the tests (or if tests are still incomplete, update the
top-of-file comment to accurately state current status) and then run/adjust
tests to pass; specifically locate the describe.skip(...) block in
unified-sidebar.test.ts and either change it to describe(...) to enable tests
and fix any failing assertions against SidebarConnectionForm and
PrimitiveDetail, or update the placeholder comment text to reflect that those
components are implemented and add new tests to cover their functionality to
meet the 50% coverage requirement.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return null; | ||
| } | ||
|
|
||
| function queryAllByText(text: string): Element[] { |
Code Review: PR #156 - Unified SidebarSummarySignificant UI redesign consolidating server management, primitives, and connection controls into unified sidebar. 6 files changed: +3,756/-227 lines. ✅ Strengths1. Excellent TypeScript Type Safety
2. Backward Compatibility
3. React Best Practices
4. Robust localStorage
5. Strong Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx`:
- Around line 1139-1165: The selection logic uses resource.name whereas the list
key uses resource.uri, causing mismatches; update McpPrimitivesPanel to use
resource.uri for selection: change calls to isPrimitiveSelected("resource", ...)
and handlePrimitiveClick("resource", ...) to pass resource.uri instead of
resource.name, update data-testid usage if needed to reflect uri, and then
update the selection resolver in InspectorDashboard (or wherever selection is
resolved) to compare on uri rather than name so selections remain stable and
unambiguous across resources.
In `@packages/inspector/src/dashboard/react/InspectorDashboard.tsx`:
- Around line 25-32: The import includes a value symbol PrimitiveDetail that's
never used; remove it or make it a type-only import. Edit the import from
"./components/PrimitiveDetail" so it only imports the type (import type {
Primitive } from "./components/PrimitiveDetail") or delete PrimitiveDetail from
the import list entirely to resolve the TS6133 unused value error.
| {filteredResources.map((resource) => { | ||
| const isSelected = isPrimitiveSelected("resource", resource.name); | ||
| return ( | ||
| <div | ||
| key={resource.uri} | ||
| style={{ | ||
| ...localStyles.primitiveItem, | ||
| ...(isSelected | ||
| ? localStyles.primitiveItemActive | ||
| : hoveredItem === `resource-${resource.uri}` | ||
| ? localStyles.primitiveItemHover | ||
| : {}), | ||
| }} | ||
| onClick={() => handlePrimitiveClick("resource", resource.name)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| handlePrimitiveClick("resource", resource.name); | ||
| } | ||
| }} | ||
| onMouseEnter={() => setHoveredItem(`resource-${resource.uri}`)} | ||
| onMouseLeave={() => setHoveredItem(null)} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-selected={isSelected} | ||
| data-testid={`resource-item-${resource.name}`} | ||
| > |
There was a problem hiding this comment.
Use a stable identifier for resource selection (prefer uri).
Selection currently uses resource.name while the list key uses resource.uri, which can lead to ambiguous selections if names collide or differ from URIs. Consider using uri as the selection identifier for resources and update the resolver accordingly (e.g., in InspectorDashboard) to match on uri.
Proposed adjustment in this file
-const isSelected = isPrimitiveSelected("resource", resource.name);
+const isSelected = isPrimitiveSelected("resource", resource.uri);
...
-onClick={() => handlePrimitiveClick("resource", resource.name)}
+onClick={() => handlePrimitiveClick("resource", resource.uri)}🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx`
around lines 1139 - 1165, The selection logic uses resource.name whereas the
list key uses resource.uri, causing mismatches; update McpPrimitivesPanel to use
resource.uri for selection: change calls to isPrimitiveSelected("resource", ...)
and handlePrimitiveClick("resource", ...) to pass resource.uri instead of
resource.name, update data-testid usage if needed to reflect uri, and then
update the selection resolver in InspectorDashboard (or wherever selection is
resolved) to compare on uri rather than name so selections remain stable and
unambiguous across resources.
Code Review: Unified Sidebar — Servers, Primitives & Connection FormSummaryThis PR implements a substantial UI redesign that consolidates server management, primitives browsing, and connection controls into a unified left sidebar. The implementation is well-architected with good separation of concerns, comprehensive test coverage (596 new test lines), and follows the repository TypeScript strict mode requirements. ✅ Strengths1. Architecture & Design
2. Code Quality
3. User Experience
4. Testing
|
Code Review - PR #156: Unified SidebarI've conducted a comprehensive review of this PR implementing the unified sidebar feature. Overall, this is a well-structured implementation that successfully consolidates server management, primitives browsing, and connection controls. StrengthsArchitecture & Design
Code Quality
Issues & Concerns1. TabBar Component Inconsistency (CRITICAL)Location: InspectorDashboard.tsx:761-767 The PR description states "Tab bar removed" as acceptance criteria, but the TabBar component is still rendered in the code. This creates inconsistency between the spec and implementation. Recommendation: If tabs should be removed per the unified sidebar design, remove this component. If tabs are staying, update the acceptance criteria and PR description. 2. Potential Memory LeakLocation: InspectorDashboard.tsx:106-117 These caches grow unbounded as connections are created. When connections are closed, cached data is deleted from connectionCacheRef (line 572) but not from primitivesPerConnectionRef. Recommendation: Add cleanup for primitivesPerConnectionRef.current.delete(id) in handleCloseConnection 3. Disabled Action ButtonLocation: PrimitiveDetail.tsx:1442-1453 The Run/Read/Use action button is disabled with "Coming soon" title, but the PR includes complete action forms (ToolRunForm, ResourceReadForm, PromptUseForm). Either enable the action button to trigger the action forms, or remove the unused action form components. The forms appear fully functional with mock execution support. 4. Search Functionality MissingAcceptance Criteria: "Search filters across all servers/primitives" While McpPrimitivesPanel has a search input field in the UI, there's no actual filtering logic implemented in the component. Implement search filtering or remove the search input to match actual functionality. 5. Error Handling in Connection FormLocation: SidebarConnectionForm.tsx:203-206 Simple whitespace splitting doesn't respect quoted strings. For example: --message "hello world" would incorrectly split. Use a proper argument parser or document this limitation. 6. Type Casting Without ValidationLocation: InspectorDashboard.tsx:48 While there's validation after the cast, the initial cast happens before validation. Move the type assertion after validation for better safety. 7. Inconsistent Naming
Recommendation: Standardize naming conventions for consistency. Recommendations SummaryMust Fix (Before Merge)
Should Fix
Nice to Have
Overall AssessmentThis is solid work that successfully implements a complex UI feature. The code follows repository standards (strict TypeScript, no any types, proper exports through index files). The component architecture is clean and testable. Rating: 8/10 - High quality implementation with a few issues that should be addressed before merge. Generated with Claude Code |
Code Review: Unified Sidebar — Servers, Primitives & Connection FormThis PR implements a major UI refactor that consolidates the left panel into a unified sidebar with server blocks, primitives, and an inline connection form. Overall, the implementation is solid with good test coverage and attention to detail. Here are my findings: ✅ Strengths
🔍 Issues & Recommendations1. Potential Memory Leak in Primitives CacheLocation: Issue: The Recommendation: Add cleanup logic when connections are removed. 2. Race Condition in Server Start/StopLocation: Issue: Recommendation: Only remove from stopped list after successful connection. 3. Incomplete Server Info in ServerDataLocation: Issue: Hardcoded transport type loses information. If the dashboard is extended to support stdio or SSE connections in the future, this will break. Recommendation: Derive transport from connection metadata if available. 4. Missing Accessibility LabelsLocation: Issue: Several interactive elements lack proper ARIA labels:
5. Potential Performance Issue with Large Server ListsLocation: Issue: The component renders all servers and their primitives on every update. With 10+ servers with 50+ tools each, this could cause jank. Recommendation:
6. Unclear localStorage Key Collision RiskLocation: Issue: If multiple dashboards are embedded on the same page (different baseUrls), they'll share the same stopped connections in localStorage. Recommendation: Namespace localStorage keys by baseUrl to prevent collisions. 🛡️ Security Considerations
⚡ Performance Notes
🧪 Test CoverageExcellent: 596 lines of new tests covering:
Missing Coverage:
📋 Code Quality
✏️ Minor Nitpicks
🎯 VerdictApprove with suggestions. This is a well-implemented feature with good test coverage and type safety. The issues identified are mostly minor and can be addressed in follow-up PRs if time is constrained. Priority fixes before merge:
Nice-to-haves:
Great work on this substantial refactor! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
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)
92-104:⚠️ Potential issue | 🔴 CriticalRemove unused
setActiveConnectionId— build is failing.The pipeline reports TS6133:
setActiveConnectionIdis destructured but never used. Either remove it from the destructuring or use it.Proposed fix
const { connections, activeConnectionId, - setActiveConnectionId, isCreating, error: connectionError, createConnection,
🧹 Nitpick comments (2)
packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx (1)
35-38: Transport type union is absorbed bystring.The union
"stdio" | "http" | "sse" | stringis equivalent to juststringsince the string type absorbs the literals. If you want type safety for known transports while allowing extensions, consider:- transport?: "stdio" | "http" | "sse" | string; + transport?: "stdio" | "http" | "sse" | (string & {});The
(string & {})trick preserves autocomplete for known values while still accepting any string.packages/inspector/src/dashboard/react/InspectorDashboard.tsx (1)
43-62: Consider using Zod for localStorage validation.The manual shape validation works, but per coding guidelines, Zod v4 should be used for schema/validation. This would provide more robust parsing and clearer error handling.
Suggested approach
import { z } from "zod"; const StoppedConnectionSchema = z.object({ id: z.string(), name: z.string(), url: z.string(), params: z.object({ transport: z.string().optional(), url: z.string().optional(), command: z.string().optional(), args: z.array(z.string()).optional(), }).passthrough(), }); function loadStoppedConnections(): StoppedConnection[] { if (typeof window === "undefined") return []; try { const stored = localStorage.getItem(STOPPED_CONNECTIONS_KEY); if (!stored) return []; const parsed = JSON.parse(stored); const result = z.array(StoppedConnectionSchema).safeParse(parsed); return result.success ? result.data : []; } catch { return []; } }As per coding guidelines: "Always use Zod v4 for schema/validation; do not use Zod v3 assumptions"
Code Review: PR #156 - Unified Sidebar✅ Overall AssessmentThis is a well-architected and comprehensive refactoring that successfully consolidates the inspector dashboard's UI into a unified sidebar. The implementation demonstrates strong attention to:
🎯 Acceptance Criteria CoverageAll acceptance criteria appear to be met:
🔍 Code Quality HighlightsExcellent Patterns 👍
function loadStoppedConnections(): StoppedConnection[] {
if (typeof window === "undefined") return [];
try {
const stored = localStorage.getItem(STOPPED_CONNECTIONS_KEY);
if (!stored) return [];
const parsed = JSON.parse(stored) as StoppedConnection[];
// Validate shape before using
return parsed.filter(/* validation logic */);
} catch {
return [];
}
}Excellent defensive programming with SSR awareness and validation.
|
There was a problem hiding this comment.
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)
95-95:⚠️ Potential issue | 🟡 MinorRemove unused
setActiveConnectionIdto fix pipeline failure.TS6133 reports
setActiveConnectionIdis declared but never used. Remove it from the destructuring.🐛 Proposed fix
const { connections, activeConnectionId, - setActiveConnectionId, isCreating, error: connectionError,
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx`:
- Line 9: The React import at the top of McpPrimitivesPanel.tsx is missing
useMemo which causes a runtime error when serverHistory is computed with
useMemo; update the import statement that currently lists useState, useCallback,
useRef, useEffect to also include useMemo so the hook used in the serverHistory
calculation is available (i.e., add useMemo to the named imports alongside the
existing hooks).
🧹 Nitpick comments (4)
packages/inspector/src/dashboard/react/components/SidebarConnectionForm.tsx (1)
331-355: Consider using a stable key instead of array index for history items.Using
key={idx}can cause React reconciliation issues if entries are reordered or have identical names. IfServerHistoryEntryhad a unique identifier or the params could derive one, that would be more robust.packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx (1)
35-54: Type forparams.transportis overly broad.The
transportfield accepts"stdio" | "http" | "sse" | string, where the trailingstringmakes the union redundant — any string is accepted. If the intent is to allow unknown transports, consider using a discriminated union or removing the specific literals.Option: Narrow the type or document extensibility
params?: { - transport?: "stdio" | "http" | "sse" | string; + transport?: "stdio" | "http" | "sse"; [key: string]: unknown; };Or if extensibility is intentional, just use
stringalone with a comment.packages/inspector/src/dashboard/react/InspectorDashboard.tsx (2)
43-62: Consider using Zod for localStorage validation.The manual shape validation works but is verbose. Since the coding guidelines mention Zod v4 for schema validation, this would be a good candidate for a Zod schema to ensure type-safe parsing.
Example with Zod
import { z } from "zod"; const StoppedConnectionSchema = z.object({ id: z.string(), name: z.string(), url: z.string(), params: z.object({ transport: z.enum(["http", "stdio"]), url: z.string().optional(), command: z.string().optional(), args: z.array(z.string()).optional(), }), }); function loadStoppedConnections(): StoppedConnection[] { if (typeof window === "undefined") return []; try { const stored = localStorage.getItem(STOPPED_CONNECTIONS_KEY); if (!stored) return []; const parsed = JSON.parse(stored); return z.array(StoppedConnectionSchema).parse(parsed); } catch { return []; } }As per coding guidelines: "Always use Zod v4 for schema/validation".
299-300: Hardcoded HTTP transport limits future extensibility.The comment notes "Dashboard only supports HTTP transport", but
StoppedConnection.paramstype supports stdio. If stdio support is added later, servers stopped via stdio would reconnect as HTTP. Consider either:
- Storing the original transport from the connection, or
- Restricting
StoppedConnection.paramsto HTTP-only at the type level.
Code Review: PR #156 - Unified SidebarOverviewThis PR implements a significant UI redesign combining server management, primitives browsing, and connection controls into a unified sidebar. The implementation is well-structured with 3,761 additions across 6 files, including comprehensive test coverage. ✅ Strengths1. Excellent TypeScript Type Safety
2. Robust Error Handling
3. React Best Practices
4. Comprehensive Test Coverage
5. Backward Compatibility
|
Code Review: PR #156 - Unified SidebarSummaryThis PR successfully implements a unified sidebar that combines server management, primitives browsing, and connection controls. The implementation is well-structured with good test coverage and maintains backwards compatibility. However, there are some performance optimizations and minor improvements that should be considered. ✅ Strengths1. Excellent Type Safety
2. Backwards Compatibility
3. Good Test Coverage
4. Security Considerations
|
| Metric | Value | Status |
|---|---|---|
| Files Changed | 6 | ✅ |
| Lines Added | 3,756 | |
| Lines Deleted | 228 | ✅ |
| Test Coverage | New tests added | ✅ |
| TypeScript Strict | Compliant | ✅ |
No any Types |
Compliant | ✅ |
| Backwards Compat | Maintained | ✅ |
🎯 Action Items
Before Merge (High Priority):
- Split PrimitiveDetail.tsx into smaller components
- Add error boundaries around new components
- Optimize primitives cache effect
Post-Merge (Medium Priority):
- Refactor InspectorDashboard.tsx hook complexity
- Add loading state management for server operations
- Move test helpers to shared testing package
🏁 Conclusion
This PR demonstrates solid engineering with good type safety, backwards compatibility, and test coverage. The main concerns are around component size and performance optimization. The code is functional and secure, but would benefit from the refactoring suggestions above for long-term maintainability.
Recommendation: ✅ Approve with suggested improvements
The critical issues can be addressed in follow-up PRs if needed, but optimizing the cache effect before merging would be ideal.
Review conducted following repository standards from AGENTS.md
Code Review: Unified Sidebar — Servers, Primitives & Connection FormI've reviewed PR #156 and here's my comprehensive feedback: ✅ StrengthsArchitecture & Design
Code Quality
🔍 Issues & Concerns1. Security: XSS Vulnerability in Server Names
|
Code Review: Unified Sidebar ImplementationSummaryThis PR successfully implements a unified sidebar for server management, primitives browsing, and connection controls. The implementation shows strong architectural patterns but requires attention to security, performance, and accessibility before production deployment. Overall Assessment: 7.5/10 - Approve with required changes 🎯 Strengths
🚨 Critical Issues (Must Fix)1. Security: Plaintext localStorage StorageFile: Connection parameters (including URLs) are stored without encryption: function loadStoppedConnections(): StoppedConnection[] {
const stored = localStorage.getItem(STOPPED_CONNECTIONS_KEY);
const parsed = JSON.parse(stored) as StoppedConnection[];Impact: Sensitive connection details exposed in browser storage 2. Performance: Large Component Re-rendersFile: const serverDataList: ServerData[] = useMemo(() => {
return connections
.filter((conn) => conn.status === "connected")
.map((conn) => {
// Heavy computation on every connection changeImpact: Performance degradation with multiple servers 3. Memory Leak: Uncleaned TimeoutsFile: useEffect(() => {
if (isOpen) {
setTimeout(() => {
urlInputRef.current?.focus();
}, 50);
}
}, [isOpen]);Impact: Potential memory leaks if component unmounts during timeout
|
There was a problem hiding this comment.
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)
92-100:⚠️ Potential issue | 🔴 CriticalRemove unused
setActiveConnectionId(TS6133 build failure)The variable is destructured at line 95 but never used in the component. Remove it from destructuring to resolve the build error.
Proposed fix
const { connections, activeConnectionId, - setActiveConnectionId, isCreating,
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx`:
- Around line 1436-1447: The footer action button in PrimitiveDetail.tsx is
never interactive: it is rendered only when mode === "browse" but has disabled
and buttonDisabled styles and no click handler, so handleActionClick is unused;
remove the disabled attribute and buttonDisabled styling from the button and add
an onClick={handleActionClick} (and remove or update the "Coming soon" title),
ensuring the button still renders in the browse footer and triggers the existing
handleActionClick handler to open Run/Read/Use forms.
| {/* Footer - only in browse mode */} | ||
| {mode === "browse" && ( | ||
| <div style={styles.footer} data-testid="primitive-detail-footer"> | ||
| <CopyJsonButton data={primitiveData} /> | ||
| <button | ||
| style={{ ...styles.button, ...styles.buttonDisabled }} | ||
| disabled | ||
| title="Coming soon" | ||
| data-testid="action-btn" | ||
| > | ||
| {icon} {label} | ||
| </button> |
There was a problem hiding this comment.
Action mode is unreachable (button disabled)
handleActionClick is never used and the footer action button is disabled, so Run/Read/Use forms can’t be accessed. Wire the button to handleActionClick (or remove action-mode UI).
Proposed fix
- <button
- style={{ ...styles.button, ...styles.buttonDisabled }}
- disabled
- title="Coming soon"
- data-testid="action-btn"
- >
- {icon} {label}
- </button>
+ <button
+ style={{ ...styles.button, ...styles.buttonPrimary }}
+ onClick={handleActionClick}
+ data-testid="action-btn"
+ >
+ {icon} {label}
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Footer - only in browse mode */} | |
| {mode === "browse" && ( | |
| <div style={styles.footer} data-testid="primitive-detail-footer"> | |
| <CopyJsonButton data={primitiveData} /> | |
| <button | |
| style={{ ...styles.button, ...styles.buttonDisabled }} | |
| disabled | |
| title="Coming soon" | |
| data-testid="action-btn" | |
| > | |
| {icon} {label} | |
| </button> | |
| {/* Footer - only in browse mode */} | |
| {mode === "browse" && ( | |
| <div style={styles.footer} data-testid="primitive-detail-footer"> | |
| <CopyJsonButton data={primitiveData} /> | |
| <button | |
| style={{ ...styles.button, ...styles.buttonPrimary }} | |
| onClick={handleActionClick} | |
| data-testid="action-btn" | |
| > | |
| {icon} {label} | |
| </button> |
🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsx` around
lines 1436 - 1447, The footer action button in PrimitiveDetail.tsx is never
interactive: it is rendered only when mode === "browse" but has disabled and
buttonDisabled styles and no click handler, so handleActionClick is unused;
remove the disabled attribute and buttonDisabled styling from the button and add
an onClick={handleActionClick} (and remove or update the "Coming soon" title),
ensuring the button still renders in the browse footer and triggers the existing
handleActionClick handler to open Run/Read/Use forms.
Code Review: Unified Sidebar — Servers, Primitives & Connection FormSummaryThis PR successfully implements a comprehensive redesign of the left panel, unifying server management, primitives browsing, and connection controls into a cohesive sidebar. The implementation demonstrates strong adherence to React best practices, type safety, and thoughtful UX considerations. ✅ Strengths1. Excellent Type Safety
2. Robust Error Handling
3. Performance Considerations
4. State Management
5. Test Coverage
6. Code Organization
🔍 Areas for Improvement1. Security Considerations
|
Code Review: Unified Sidebar — Servers, Primitives & Connection FormI've completed a thorough review of this PR. Overall, this is a well-implemented feature with clean architecture and good attention to detail. ✅ StrengthsCode Quality
Architecture
|
Code Review - PR #156: Unified SidebarExecutive SummaryThis PR successfully implements a unified sidebar for server management and primitives browsing. The code is well-structured with good separation of concerns, comprehensive test coverage (596 lines of tests), and follows the repository's TypeScript strict mode requirements. Strengths1. Code Quality & Architecture
2. User Experience
3. Testing
Issues FoundCRITICAL: Disabled Action ButtonsLocation: PrimitiveDetail.tsx:1440-1448 Action buttons (Run/Read/Use) are permanently disabled despite complete implementation. Recommendation: Enable buttons and wire onExecute, add feature flag, OR document why disabled MEDIUM: Potential Issues1. Copy Button Memory Leak (PrimitiveDetail.tsx:794)
2. Type Safety (SidebarConnectionForm.tsx:250)
3. Shell Argument Parsing (SidebarConnectionForm.tsx:250)
LOW: Code Style
Security Review✅ No critical security issues found Performance Analysis✅ Good use of useMemo and useCallback Test CoverageQuality: EXCELLENT (597 lines) Missing:
Repository Standards✅ Strict TypeScript RecommendationsMust Fix Before Merge:
Should Fix: Nice to Have: Overall Assessment
This is high-quality work with excellent architecture and testing. The identified issues are minor and mostly polish items. The disabled action buttons are the only blocking concern. Great work on this feature! 🎉 |
Code Review: PR #156 - Unified Sidebar ImplementationSummaryThis PR successfully implements a unified sidebar combining server management, primitives browsing, and connection controls. Overall assessment: 8.5/10 - Production-ready with minor fixes recommended. Stats: 3,891 additions, 233 deletions across 6 files 🟢 StrengthsCode Quality
Security
🟡 Issues FoundHigh Priority (Must Fix Before Merge)1. localStorage Validation Gap -
Fix: Add proper validation for transport property. 2. Race Condition in handleStartServer - If user rapidly clicks Start twice on same server, both async operations run. Need to track in-progress operations. Medium Priority3. Stdio Args Parsing - 4. selectedPrimitive Not Cleared - 5. KeyframeStyles Re-injected - Low Priority6. ServerBlock Re-renders - Consider 📊 Test Coverage AssessmentStrengths: Comprehensive 596-line test suite with proper setup/teardown Gaps:
Recommendation: Add 5-10 tests for error scenarios 🎯 Repository Standards
📝 RecommendationsBefore Merge (Required)
Should Address (Strongly Recommended)
Nice to Have
✨ Final VerdictAPPROVE with recommendations - High-quality work following repository standards. Two high-priority issues should be addressed before merge to prevent edge case bugs. Medium/low priority items can be in follow-up PRs. Great job on test coverage and accessibility! 🎉 Review by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx`:
- Around line 687-702: The useEffect in McpPrimitivesPanel schedules nested
requestAnimationFrame calls to setShow but never cancels them, risking state
updates after unmount; modify the effect in McpPrimitivesPanel.tsx to capture
the RAF ids (e.g., const raf1 = requestAnimationFrame(...); const raf2 =
requestAnimationFrame(...)) and on cleanup call cancelAnimationFrame for each id
in addition to clearing the exit timeout so that any pending RAFs are cancelled
before unmount and setShow/setRender cannot be invoked afterward.
| useEffect(() => { | ||
| if (isVisible) { | ||
| setRender(true); | ||
| // Trigger animation after mount | ||
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| setShow(true); | ||
| }); | ||
| }); | ||
| } else { | ||
| setShow(false); | ||
| // Wait for exit animation | ||
| const timer = setTimeout(() => setRender(false), 200); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [isVisible]); |
There was a problem hiding this comment.
Cancel scheduled animation frames on unmount.
AnimatedDetailWrapper schedules nested requestAnimationFrame calls but doesn’t cancel them on cleanup, which can set state after unmount. Add RAF cleanup alongside the existing timeout cleanup.
Proposed fix
useEffect(() => {
+ let rafId1: number | undefined;
+ let rafId2: number | undefined;
+ let timer: ReturnType<typeof setTimeout> | undefined;
+
if (isVisible) {
setRender(true);
// Trigger animation after mount
- requestAnimationFrame(() => {
- requestAnimationFrame(() => {
+ rafId1 = requestAnimationFrame(() => {
+ rafId2 = requestAnimationFrame(() => {
setShow(true);
});
});
} else {
setShow(false);
// Wait for exit animation
- const timer = setTimeout(() => setRender(false), 200);
- return () => clearTimeout(timer);
+ timer = setTimeout(() => setRender(false), 200);
}
+
+ return () => {
+ if (rafId1 !== undefined) cancelAnimationFrame(rafId1);
+ if (rafId2 !== undefined) cancelAnimationFrame(rafId2);
+ if (timer !== undefined) clearTimeout(timer);
+ };
}, [isVisible]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (isVisible) { | |
| setRender(true); | |
| // Trigger animation after mount | |
| requestAnimationFrame(() => { | |
| requestAnimationFrame(() => { | |
| setShow(true); | |
| }); | |
| }); | |
| } else { | |
| setShow(false); | |
| // Wait for exit animation | |
| const timer = setTimeout(() => setRender(false), 200); | |
| return () => clearTimeout(timer); | |
| } | |
| }, [isVisible]); | |
| useEffect(() => { | |
| let rafId1: number | undefined; | |
| let rafId2: number | undefined; | |
| let timer: ReturnType<typeof setTimeout> | undefined; | |
| if (isVisible) { | |
| setRender(true); | |
| // Trigger animation after mount | |
| rafId1 = requestAnimationFrame(() => { | |
| rafId2 = requestAnimationFrame(() => { | |
| setShow(true); | |
| }); | |
| }); | |
| } else { | |
| setShow(false); | |
| // Wait for exit animation | |
| timer = setTimeout(() => setRender(false), 200); | |
| } | |
| return () => { | |
| if (rafId1 !== undefined) cancelAnimationFrame(rafId1); | |
| if (rafId2 !== undefined) cancelAnimationFrame(rafId2); | |
| if (timer !== undefined) clearTimeout(timer); | |
| }; | |
| }, [isVisible]); |
🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/components/McpPrimitivesPanel.tsx`
around lines 687 - 702, The useEffect in McpPrimitivesPanel schedules nested
requestAnimationFrame calls to setShow but never cancels them, risking state
updates after unmount; modify the effect in McpPrimitivesPanel.tsx to capture
the RAF ids (e.g., const raf1 = requestAnimationFrame(...); const raf2 =
requestAnimationFrame(...)) and on cleanup call cancelAnimationFrame for each id
in addition to clearing the exit timeout so that any pending RAFs are cancelled
before unmount and setShow/setRender cannot be invoked afterward.
Code Review: PR #156 - Unified Sidebar — Servers, Primitives & Connection FormSummaryThis PR implements a significant UX redesign of the MCP Inspector dashboard, consolidating server management, primitives browsing, and connection controls into a unified left sidebar. The implementation adds 3,900 lines across 6 files with strong test coverage and adherence to TypeScript strict mode. Overall Assessment: ✅ Approved with Minor Recommendations The code quality is excellent, follows repository conventions, and includes comprehensive tests. However, there are some areas for optimization and minor concerns to address. 🎯 What's Good1. Excellent Type Safety
2. Strong Test Coverage
3. Performance Optimizations
4. User Experience
|
| Metric | Value | Status |
|---|---|---|
| Lines Added | 3,900 | |
| Lines Deleted | 233 | ✅ |
| Files Changed | 6 | ✅ |
| TypeScript Errors | 0 | ✅ |
| Test Coverage | Comprehensive | ✅ |
| Largest File | 2,009 lines |
✅ Checklist Review
Per AGENTS.md requirements:
- ✅ TypeScript Strict Mode: No
anytypes, properunknownnarrowing - ✅ Zod v4 / Express v5: Not applicable to this PR
- ✅ Export Contract: All exports flow through proper index files
- ✅ Testing: Comprehensive test coverage added
- ✅ No Circular Dependencies: Proper import hierarchy
- ✅ localStorage Safety: SSR-safe with
typeof windowchecks
🎬 Next Steps
Required Before Merge:
- ✅ Tests pass (already verified)
- ✅ TypeScript compiles (already verified)
- ✅ Lint passes (verify with
pnpm lint)
Recommended (Optional):
- Consider splitting large component files for maintainability
- Extract complex state logic into custom hooks
- Consolidate style constants into shared files
- Add development logging for localStorage errors
🏆 Conclusion
This is a well-crafted PR that demonstrates strong engineering practices:
- Excellent type safety and adherence to repository standards
- Comprehensive test coverage
- Thoughtful performance optimizations
- Good user experience with proper state persistence
The recommendations above are primarily about long-term maintainability rather than blocking issues. The code is production-ready as-is.
Great work on this significant UX improvement! 🎉
Code Review: PR #156 - Unified SidebarOverviewMajor UI refactor combining server management, primitives browsing, and connection controls. +3,902/-233 lines across 6 files. ✅ Strengths
🔍 Critical Issues (P0 - Must Fix)1. Type Safety: Unsafe param validationFile: InspectorDashboard.tsx:43-50 2. Security: Missing input sanitizationFile: SidebarConnectionForm.tsx 3. Memory Leak: Unbounded cacheFile: InspectorDashboard.tsx:83 4. Error Swallowing: Silent localStorage failuresMultiple files High Priority (P1)5. Large Components
6. Magic NumberssetTimeout delays and timeouts need named constants 7. Test Coverage GapsMissing: error handling, memory leaks, reconnection edge cases 📊 Assessment
🎯 Action ItemsBefore Merge:
Soon After: 📝 VerdictRequest Changes Strong work with valuable UX improvements, but critical security and type safety issues must be addressed first. Approve after P0 fixes. |
Code Review: Unified Sidebar — Servers, Primitives & Connection FormThis is a significant UI refactor that consolidates server management, primitives browsing, and connection controls into a unified sidebar. Overall, the implementation is solid and well-structured, but there are several areas for improvement. ✅ StrengthsArchitecture & Design
Code Quality
|
PR Review: Unified Sidebar — Servers, Primitives & Connection FormSummaryThis PR successfully implements a unified sidebar UI that consolidates server management, primitives browsing, and connection controls. The implementation is comprehensive with good test coverage and follows React best practices. Code Quality ✅Strengths
Areas for Improvement1. Potential Memory Leaks in Event Handlers
|
Code Review: PR #156 - Unified SidebarOverviewThis PR successfully implements a unified sidebar that combines server management, primitives browsing, and connection controls. The implementation is comprehensive with ~4000 lines added across 6 files, featuring a slide-over detail panel, inline connection form, and improved UX. ✅ StrengthsArchitecture & Design
Code Quality
UX Features
|
… persistence [TASK-026]
…sabled actions [TASK-026] Changes: - Move primitive detail view from center area to right panel - Style tool/resource/prompt items as card-like buttons (bolder, bigger) - Wire actual server info (transport: http, version, capabilities) - Increase font sizes in sidebar headers and content - Disable action buttons (Run/Read/Use) with 'Coming soon' tooltip The right panel now shows primitive detail when a primitive is selected, replacing the agent/events/logs tabs temporarily. Collapsing the right panel clears the selection. Server info now displays: - Transport: http (dashboard only supports HTTP) - Version: from serverInfo.version - Capabilities: derived from available primitives
- Sidebar now handles all connections via + button - Removed ConnectionBar (URL bar in header) - Removed TabBar (connection tabs) - Updated NoWidgetPlaceholder to point users to sidebar - Cleaned up orphaned state/handlers
- Remove border change on hover (just background) - Play/stop white icons instead of text buttons - Server info always visible (not collapsible) - Lighter gray for disconnected labels (#9ca3af) - Loading state when reconnecting servers - Server history dropdown in connection form
…TASK-026] - Start/stop button: white background, black icon - Each tool/resource/prompt is now a collapsible card with animation - Added bin (trash) icon button to remove servers completely - Cards expand to show description
…tems Server is already a collapsible card. Primitives should be simple clickable items that open the detail view, not collapsible cards. Kept: - White start/stop button with black icon - Delete (trash) button per server - Server as collapsible card (unchanged)
- Server name: bigger font (0.9375rem) and white color - Removed collapse arrow near server name - Detail appears with slide-in-from-left + fade animation (0.2s)
- Detail slides in from right-to-left on open - Detail slides out left-to-right on dismiss (animated) - Server info now displayed as horizontal chips/badges - Chips show: status, transport, version, capabilities
- Fixed exit animation not triggering (state machine approach) - Reduced translation distance (16px → 8px) to minimize jump - Cleaner enter/exit state transitions
- Enter: fade in + slide from right - Exit: fade out + slide to left - Uses requestAnimationFrame for reliable transition trigger
- Detail slides in from right, covering the server list - Full panel with solid background - Fade + slide animation (250ms) - Slides out to right on close
Server list now stays rendered underneath the sliding detail panel
84739e9 to
566df6f
Compare
Code Review: PR #156 - Unified SidebarSummaryThis is a well-architected UI redesign that consolidates server management, primitives browsing, and connection controls into a unified sidebar. The implementation demonstrates strong engineering practices with thorough testing, proper TypeScript typing, and backward compatibility. Strengths1. Architecture & Design
2. Code Quality
3. Testing
Issues & RecommendationsHIGH PRIORITY1. localStorage Security & Validation (InspectorDashboard.tsx:60-76)HTTP URLs are not validated - accepts malicious URLs like javascript:, file://, etc. Recommendation: Add URL protocol validation using Zod's url() and refine() methods to only allow http/https. 2. Resource Leak in SlideOverDetail (McpPrimitivesPanel.tsx:1092-1095)The double requestAnimationFrame in the enter animation path has no cleanup. If component unmounts during animation, RAF callbacks will still fire. Recommendation: Store rAF IDs and clean them up in the effect cleanup function. 3. Unbounded Memory Growth (InspectorDashboard.tsx:119)The primitivesPerConnectionRef cache grows indefinitely. While there's cleanup on connection close, rapid create/close cycles could cause issues. Recommendation: Add max size limit or implement LRU cache behavior. MEDIUM PRIORITY4. Missing Input Validation (SidebarConnectionForm.tsx:196-200)URL validation checks format but doesn't verify protocol safety. Recommendation: Check parsed URL protocol against allowed list (http:, https:). 5. Race Condition in handleStartServer (InspectorDashboard.tsx:340-358)Rapid double-clicks could create duplicate connections. Recommendation: Add guard check for reconnectingServerId. 6. Missing Error BoundaryLarge React components with animation should be wrapped in error boundaries. LOW PRIORITY (Code Quality)
Security AssessmentGood Practices:
Concerns:
Recommendations:
Performance ConsiderationsOptimizations Present:
Suggestions:
Overall AssessmentQuality Score: 8.5/10 This is high-quality production code with solid architecture, good testing coverage, proper accessibility support, and clean state management. Some security concerns need addressing before merge. Recommendation: APPROVE WITH REQUESTED CHANGES Action Items Before Merge:
Great work on this feature! 🎉 |
The unified sidebar wireframe (studies/unified-sidebar-wireframe.tsx) was a design artifact for TASK-026. PR #156 has been merged and the feature is shipped — the wireframe is no longer needed. Removes 1,066 lines of dead code and the empty studies/ directory.
The unified sidebar wireframe (studies/unified-sidebar-wireframe.tsx) was a design artifact for TASK-026. PR #156 has been merged and the feature is shipped — the wireframe is no longer needed. Removes 1,066 lines of dead code and the empty studies/ directory.
1 participant
Unified Sidebar — Servers, Primitives & Connection Form
Redesign the left panel to combine server management, primitives browsing, and connection controls into a single unified sidebar. Removes the tab bar; connection form embedded via + button; detail view in main area (mutually exclusive with right panel).
Acceptance Criteria
Agent Pipeline
Total agent time: 31.5m
Rigel's Report
Fixed all Altair issues: TypeScript errors, item selection wiring, mutual exclusivity, localStorage persistence. All checks pass.
Altair's Report
TypeScript errors in PrimitiveDetail.tsx and InspectorDashboard.tsx. Missing: item selection wiring, mutual exclusivity logic, localStorage persistence for left panel collapse.
Polaris's Report
All 1525 inspector tests passing (63 files). Fixed unified-sidebar.test.ts → .tsx extension. Example package failures unrelated to TASK-026.