Design and build Delight app shell with core surfaces

[thejackluo]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
delight	Ready	Preview	Comment	Nov 18, 2025 7:50pm

[thejackluo]

great, mostly frontend changes

[claude]

Code Review: Delight App Shell Implementation

🎯 Executive Summary

This PR successfully implements the app shell with 6 main surfaces (Dashboard, Missions, Memory, Narrative, Progress, Lab) using Next.js 15, React 19, and TypeScript. The implementation demonstrates solid architecture and clean code patterns, but requires several critical fixes before merging.

Overall Assessment: ✅ APPROVE WITH REQUIRED CHANGES

---

🚨 Critical Issues (MUST FIX)

1. Missing Clerk Middleware ⚠️ BLOCKER

File: packages/frontend/middleware.ts (does not exist)

Issue: Authentication protection is not enabled. Per CLAUDE.md requirements, Clerk middleware is required for server-side auth.

Required fix:

// packages/frontend/middleware.ts
import { clerkMiddleware } from "@clerk/nextjs/server";

export default clerkMiddleware();

export const config = {
  matcher: ['/((?!.*\\..*|_next).*)', '/', '/(api|trpc)(.*)'],
};

Impact: HIGH - All authenticated routes are currently unprotected.

---

2. Type Mismatch: Memory Field Naming

Files:

• src/lib/types/index.ts (line 97)
• Backend: app/models/memory.py

Issue: Frontend uses tier: MemoryTier but backend model uses memory_type: MemoryType. This will cause API integration failures.

Recommendation: Align on memory_type or create explicit mapping in API client:

// Future API client
function transformMemory(backendMemory: BackendMemory): Memory {
  return {
    ...backendMemory,
    tier: backendMemory.memory_type, // Map field
  };
}

---

3. Missing camelCase/snake_case Transformation Layer

Issue: Frontend uses camelCase (clerkUserId, createdAt, valueCategory) but backend uses snake_case (clerk_user_id, created_at, value_category). No transformation exists.

Recommendation: Create API client with automatic case conversion:

// lib/api/client.ts
import humps from 'humps'; // or write custom transformer

export async function apiRequest<T>(endpoint: string): Promise<T> {
  const res = await fetch(`${process.env.NEXT_PUBLIC_API_URL}${endpoint}`);
  const data = await res.json();
  return humps.camelizeKeys(data) as T; // Convert snake_case → camelCase
}

---

🔴 High Priority Issues

4. Missing API Client Layer

Issue: Components import mock data directly. No abstraction layer for API calls.

Current pattern (problematic):

import { mockMissions } from '@/lib/mock/data'; // Direct import

Recommended pattern:

// lib/api/missions.ts
export async function getMissions(): Promise<Mission[]> {
  // For now, return mocks - easy to swap later
  return mockMissions;
  
  // Future implementation:
  // return apiRequest<Mission[]>('/api/v1/missions');
}

// In component
const missions = await getMissions();

Action: Create lib/api/client.ts that returns mock data initially, making backend integration straightforward.

---

5. DCI Threshold Inconsistency

Location: src/lib/utils.ts:37-47

Issue: getDCIStatusColor() uses different thresholds than DCI_THRESHOLDS constant.

Constants say:

FRAGILE: 0.3, STEADY: 0.6, STRONG: 0.8, EXCELLENT: 0.9

Function uses:

if (score >= 0.8) // Should be 0.9 for EXCELLENT
else if (score >= 0.4) // Should be 0.3 for FRAGILE

Fix:

export function getDCIStatusColor(score: number): string {
  if (score >= DCI_THRESHOLDS.EXCELLENT) return "text-green-600";
  else if (score >= DCI_THRESHOLDS.STRONG) return "text-blue-600";
  else if (score >= DCI_THRESHOLDS.STEADY) return "text-yellow-600";
  else return "text-red-600";
}

---

6. Missing dynamic = "force-dynamic" on Authenticated Pages

Issue: Only 2 pages export dynamic = "force-dynamic" but all authenticated pages need this for Clerk to work properly with server components.

Required fix: Add to all pages in app/(app)/:

export const dynamic = "force-dynamic";

Affected files:

• ✅ dashboard/page.tsx (already has it)
• ✅ lab/page.tsx (already has it)
• ❌ missions/page.tsx (missing)
• ❌ memory/page.tsx (missing)
• ❌ narrative/page.tsx (missing)
• ❌ progress/page.tsx (missing)

---

7. Hard-coded Mock User ID Security Risk

Location: src/lib/mock/data.ts:33

const MOCK_USER_ID = 'user_mock_001';

Issue: Risk of accidentally exposing data cross-user when switching to real API.

Recommendation: Add prominent warning comment:

// ⚠️ SECURITY WARNING: This mock user ID is used for all test data.
// When connecting to real API, replace ALL references with actual user ID from Clerk.
// Search codebase for "MOCK_USER_ID" before deploying to production.
const MOCK_USER_ID = 'user_mock_001';

---

🟡 Medium Priority Issues

8. No Error Handling in Lab Page

Location: app/(app)/lab/page.tsx:210-223

Issue: Mock chat and memory search have no try-catch blocks, loading states, or error messages.

Example fix:

const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<string | null>(null);

const handleSendMessage = async () => {
  if (!inputMessage.trim()) return;
  
  setIsLoading(true);
  setError(null);
  
  try {
    // Easy to replace with real API later
    const response = await mockChatAPI(inputMessage);
    setChatMessages(prev => [...prev, 
      { role: 'user', content: inputMessage },
      { role: 'assistant', content: response }
    ]);
  } catch (err) {
    setError('Failed to send message. Please try again.');
  } finally {
    setIsLoading(false);
    setInputMessage('');
  }
};

---

9. Accessibility Issues

Issue 1: Missing ARIA labels on close buttons

// memory/page.tsx:172
<button
  onClick={() => setSelectedMemory(null)}
  className="text-gray-400 hover:text-gray-600"
  aria-label="Close detail panel" // ← ADD THIS
>
  ✕
</button>

Issue 2: Divs used for clickable cards instead of buttons

// memory/page.tsx:121 - NOT accessible via keyboard
<div onClick={() => setSelectedMemory(memory)} className="...cursor-pointer">

// Fix:
<button 
  onClick={() => setSelectedMemory(memory)} 
  className="...w-full text-left"
  aria-pressed={selectedMemory?.id === memory.id}
>

---

10. Missing Loading States

Issue: All components render static mock data with no loading indicators. This will cause flash of empty content when connected to API.

Recommendation: Add skeleton states:

export function MissionsList() {
  const [loading, setLoading] = useState(true);
  
  if (loading) {
    return <MissionsSkeleton />; // Or simple "Loading..."
  }
  
  return (/* existing JSX */);
}

---

🟢 Low Priority Issues (Can Defer)

11. Inconsistent Date Formatting

Multiple components format dates differently. Create centralized utility:

// lib/utils.ts
export function formatDate(date: Date | string, format: 'short' | 'long' = 'short'): string {
  const d = typeof date === 'string' ? new Date(date) : date;
  
  return d.toLocaleDateString('en-US', 
    format === 'long' 
      ? { weekday: 'long', month: 'long', day: 'numeric', year: 'numeric' }
      : { month: 'short', day: 'numeric', year: 'numeric' }
  );
}

12. Magic Numbers in Chart Rendering

Location: components/app/DCIMiniChart.tsx:49

style={{ height: `${Math.max(height, 10)}%` }} // What is 10?

Fix:

const MIN_BAR_HEIGHT_PERCENT = 10;
style={{ height: `${Math.max(height, MIN_BAR_HEIGHT_PERCENT)}%` }}

---

✅ Strengths (Excellent Work!)

1. Comprehensive TypeScript Types - Full type coverage in lib/types/index.ts
2. Well-Structured Mock Data - Realistic test data covering edge cases
3. Clean Component Architecture - Proper separation of concerns
4. Modern React Patterns - Correct use of React 19 features
5. Tailwind Best Practices - Consistent spacing, responsive design
6. Good Documentation - Clear TODO comments for backend integration
7. Realistic UI/UX - Professional design matching brand colors

---

📋 BMAD Compliance Checklist

✅ GOOD:

• Clear story implementation (app shell surfaces)
• Incremental development approach
• Good inline documentation

⚠️ GAPS:

• Missing tests for new pages (BMAD requires tests before merge)
• No story summary documenting completion
• Missing acceptance criteria checklist

Recommendation: Add basic Playwright E2E test:

// tests/e2e/navigation.spec.ts
test('user can navigate between all app surfaces', async ({ page }) => {
  await page.goto('/dashboard');
  
  await page.click('text=Missions');
  await expect(page).toHaveURL('/missions');
  
  await page.click('text=Memory');
  await expect(page).toHaveURL('/memory');
  
  // Test all 6 surfaces
});

---

🎯 Action Items Before Merge

Required (BLOCKER)

• Add middleware.ts with Clerk authentication
• Add export const dynamic = "force-dynamic" to missions, memory, narrative, progress pages
• Fix DCI threshold inconsistency in utils.ts
• Add security warning comment to MOCK_USER_ID

Strongly Recommended

• Create API client abstraction layer (lib/api/client.ts)
• Add basic error handling to Lab page
• Fix accessibility issues (ARIA labels, button elements)
• Add basic E2E navigation test

Can Defer to Follow-up PR

• Case transformation utility
• Loading skeleton states
• Date formatting utility
• Extract magic numbers

---

📊 Summary

Stats:

• Files changed: 28
• Lines added: ~20,574
• Critical issues: 6
• High priority issues: 2
• Medium priority issues: 4

Verdict: This is excellent foundation work with clean architecture and comprehensive implementation. The critical issues are straightforward to fix (~2-3 hours). Once addressed, this will be production-ready for the mock data phase.

Estimated time to address required changes: 2-3 hours

Great work on building a solid app shell! 🚀