feat: implement empty state for server list

[kantord]

fixes #94

recording-2025-11-28_12-54-32.mp4

recording-2025-11-28_13-00-09.mp4

[kantord]

@claude can you please do a deep review of this PR?

[claude]

Claude finished @kantord's task —— View job

---

Deep PR Review - Empty State Implementation

Todo List

• Read and analyze all changed files
• Review empty state component implementation
• Review mock scenario system
• Review API client changes
• Review test changes and coverage
• Check TypeScript strict mode compliance
• Check Server/Client component boundaries
• Provide comprehensive feedback

---

🎯 Overview

This PR successfully implements empty state functionality for the server list with two scenarios:

1. No servers available - When the catalog is empty
2. No search results - When search filters return no matches

The implementation includes a well-designed mock scenario system for testing different states and comprehensive test coverage.

✅ Strengths

1. Excellent Component Design

The EmptyState component (src/app/catalog/components/empty-state.tsx) uses discriminated unions effectively:

type EmptyStateProps = NoServersEmptyStateProps | NoResultsEmptyStateProps;

This ensures type safety and prevents invalid prop combinations. Great use of TypeScript!

2. Smart Mock Scenario System

The new mockScenario utility (src/mocks/mockScenario.ts) is clever:

• Cookie-based activation via X-Mock-Scenario header
• Uses Proxy pattern for clean API
• Allows testing different backend states without code changes
• Good separation of concerns with scenarios in separate files

3. Comprehensive Test Coverage

The test files show excellent coverage:

• Tests both empty states (no servers vs no results)
• Tests search functionality thoroughly
• Tests interaction (clear search button)
• Tests view mode persistence
• 134 lines of new tests in servers.test.tsx alone!

4. Clean Integration

The changes to Servers component are minimal and logical:

if (filteredServers.length === 0) {
  if (searchQuery) {
    return <EmptyState variant="no-results" searchQuery={searchQuery} onClearSearch={onClearSearch} />;
  }
  return <EmptyState variant="no-servers" />;
}

Clear, readable, and follows the existing patterns.

5. No TypeScript Strict Mode Violations

✅ No any types found
✅ All types properly defined
✅ Good use of discriminated unions

⚠️ Issues & Concerns

🔴 Critical: Server/Client Component Boundary Violation

Location: src/lib/api-client.ts lines 72-80

The API client is marked with "use server" but accesses cookies and headers from Next.js:

const cookieStore = await cookies();
const mockScenario = cookieStore.get(MOCK_SCENARIO_COOKIE)?.value;
if (mockScenario) {
  requestHeaders[MOCK_SCENARIO_HEADER] = mockScenario;
}

Problem: This creates a runtime dependency on the mock scenario cookie in production server actions. While this might work in development, it:

1. Couples production code to testing infrastructure - The API client shouldn't know about mock scenarios
2. Adds unnecessary overhead - Every API request checks for a test cookie
3. Could expose testing mechanisms - If someone sets the cookie in production

Recommended Fix:
The mock scenario system should be isolated to the MSW handlers. Consider one of these approaches:

Option A: Environment-based guard

const cookieStore = await cookies();
const mockScenario = process.env.NODE_ENV === 'development' 
  ? cookieStore.get(MOCK_SCENARIO_COOKIE)?.value 
  : undefined;

Option B: Feature flag

const cookieStore = await cookies();
const mockScenario = process.env.ENABLE_MOCK_SCENARIOS === 'true'
  ? cookieStore.get(MOCK_SCENARIO_COOKIE)?.value 
  : undefined;

Option C: Complete separation (Most robust)

• Keep API client clean in production
• Only add mock scenario headers in a development-only middleware or wrapper
• Use Next.js middleware to inject the header when the cookie is present

🟡 Medium: Mock Scenario Implementation Details

Location: src/mocks/mockScenario.ts lines 30-44

Issue 1: Silent pass-through behavior
When the scenario header doesn't match, the handler returns undefined, causing MSW to pass through to the next handler. This is correct, but the extensive console logging might be noisy:

console.log(
  `[mockScenario] ${method.toUpperCase()} ${info.request.url}`,
  `| header "${SCENARIO_HEADER}"="${headerValue}"`,
  `| expected="${scenario}"`,
  `| match=${headerValue === scenario}`,
);

Suggestion: Use environment-based logging:

if (process.env.DEBUG_MOCK_SCENARIOS) {
  console.log(`[mockScenario] ${method.toUpperCase()} ${info.request.url}`, ...);
}

Issue 2: Type safety
The Proxy return type Record<HttpMethod, typeof http.get> isn't fully accurate since each method has different signatures. While it works, it's slightly misleading.

🟡 Medium: Component Coupling

Location: src/app/catalog/components/servers.tsx line 28

The comment suggests future refactoring:

// this will be replace by nuqs later
const filteredServers = useMemo(() => {

Question: Is filtering meant to be moved to URL query params via nuqs? If so:

• Should this PR include that work, or is it tracked separately?
• The current implementation works well, but if it's temporary, consider adding a TODO or issue reference

🟢 Minor: Code Style & Best Practices

1. Console logging in production code

Location: src/mocks/mockScenario.ts lines 32-36, 42

Console logs should be removed or made conditional for production:

if (process.env.NODE_ENV === 'development') {
  console.log(`[mockScenario] ACTIVATED scenario "${scenario}"`);
}

2. Missing JSDoc for exported functions

Location: src/app/catalog/components/servers-wrapper.tsx line 38

The handleClearSearch function could benefit from documentation explaining its purpose, especially since it's passed down as a prop.

3. Empty state illustrations accessibility

Location: src/app/catalog/components/empty-state.tsx lines 23-27

The illustrations should have aria-hidden="true" or proper role and aria-label attributes:

<IllustrationNoSearchResults className="size-32" aria-hidden="true" />

This is important for screen readers - decorative images should be hidden from accessibility tree.

📊 Test Quality Analysis

Excellent Coverage:

• ✅ Empty state rendering (both variants)
• ✅ Search filtering (name, title, description, case-insensitive)
• ✅ Clear search button interaction
• ✅ View mode persistence with search
• ✅ Illustration rendering

Missing Test Cases:

1. Edge case: What happens when a server has no name field? (Line 44 in servers.tsx has a guard, but it's not tested)
2. Edge case: Empty search string (just whitespace) - does searchQuery.trim() work correctly?
3. Accessibility: No tests for keyboard navigation to "Clear search" button
4. Integration: No test verifying the mock scenario system actually works end-to-end

🏗️ Architecture Feedback

Positive Patterns:

1. ✅ Server Component first approach - Components correctly use "use client" only when needed
2. ✅ Discriminated unions - Excellent TypeScript usage for variant props
3. ✅ Single Responsibility - Each component has a clear, focused purpose
4. ✅ Test organization - Tests are co-located with components

Concerns:

1. ⚠️ Testing infrastructure in production code - The mock scenario logic shouldn't be in the API client
2. ⚠️ Cookie-based testing - While clever, this approach could be fragile. Consider MSW-only solutions

🎨 UI/UX Review

Positive:

• Clear, user-friendly messages
• Appropriate illustrations for each state
• Actionable CTA ("Clear search") for no-results state
• Good spacing and typography

Suggestions:

1. Consider adding more guidance - The "No servers available" state could include:

   • Link to documentation
   • "What are MCP servers?" help text
   • Expected timeline for when servers might be available

2. Loading state - Is there a loading state before empty state appears? Users might see a flash of empty state while servers are loading.

📝 Documentation

Missing:

1. No documentation of the mock scenario system in README or development docs
2. No examples of how to use mock scenarios in development
3. The MOCK_SCENARIO_COOKIE and MOCK_SCENARIO_HEADER constants aren't documented

Recommended additions:

## Testing Different States

Use the mock scenario system to test different backend states:

1. Set a cookie: `document.cookie = "mock-scenario=empty-servers"`
2. Reload the page
3. The UI will now show the empty servers state

Available scenarios:
- `empty-servers` - No servers in catalog

🔒 Security Review

✅ No security issues identified

• No user input is rendered without sanitization
• No XSS vulnerabilities
• Mock scenario system is development-only (but needs guards - see critical issue)

🚀 Performance

✅ No performance concerns:

• useMemo correctly used for filtering
• No unnecessary re-renders
• Illustrations are likely SVG components (efficient)

📋 Recommendations Summary

Must Fix (Before Merge):

1. 🔴 Add environment guards to mock scenario cookie reading in api-client.ts
2. 🔴 Add accessibility attributes to illustrations (aria-hidden or proper labels)

Should Fix (Before Merge):

1. 🟡 Reduce console logging or make it conditional on debug flag
2. 🟡 Document the mock scenario system in README or DEVELOPMENT.md
3. 🟡 Clarify the TODO comment about nuqs refactoring

Nice to Have (Follow-up):

1. 🟢 Add edge case tests (missing name, whitespace search)
2. 🟢 Add JSDoc for exported functions
3. 🟢 Consider more helpful empty state content (links, help text)
4. 🟢 Verify loading state → empty state transition

🎬 Conclusion

This is a well-implemented PR with excellent TypeScript usage, good test coverage, and a clever mock scenario system. The main concern is the coupling between production API client code and testing infrastructure, which should be addressed before merge.

The code quality is high, follows Next.js App Router patterns correctly, and adheres to the project's strict TypeScript guidelines. With the critical issue addressed, this will be a solid addition to the codebase.

Recommendation: Request changes for the environment guard on mock scenario cookie reading, then approve.

---

Branch: create-empty-states
Job: View run

[Copilot]

Pull request overview

This PR implements an empty state feature for the server list, enhancing the user experience when no servers are available or when search results are empty. It introduces a mock scenario system for testing different backend states in development, and adds a reusable EmptyState component with contextual illustrations and actions.

• Adds mock scenario infrastructure allowing developers to test different backend states via cookies
• Implements EmptyState component with two variants: "no-items" for empty catalog and "no-matching-items" for search results
• Updates server list to display contextual empty states with clear search functionality

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/mocks/mockScenario.ts	New utility using Proxy pattern to create scenario-specific MSW handlers activated by header matching
src/mocks/scenarios/empty-servers.ts	Empty servers scenario handler returning empty list for testing
src/mocks/handlers.ts	Integrates scenario handlers with correct precedence ordering
src/lib/api-client.ts	Adds development-only mock scenario header support based on cookie value
src/app/catalog/components/empty-state.tsx	New reusable empty state component with variant-based illustrations
src/app/catalog/components/servers.tsx	Implements empty state logic with search-aware variants and clear action
src/app/catalog/components/servers-wrapper.tsx	Adds clear search handler and passes it to Servers component
src/app/catalog/components/__tests__/servers.test.tsx	Comprehensive test coverage for empty state behavior and user interactions
src/app/catalog/components/__tests__/servers-wrapper.test.tsx	Integration test for clear search functionality from empty state

[peppescg]

Nice the empty state 🚀
I have some doubts on the dynamic mock of a empty response, I don't see the value honestly