feat(reader): Text selection → translation with flashcard creation

[Peleke]

Summary

• Highlight text in reader → get AI translation via GPT-4o-mini
• TranslationSheet component (responsive: bottom sheet mobile, sidebar desktop)
• Audio playback of source text (TTS)
• Create flashcard from translation (front=source, back=translation)
• Dialog portal fix for proper modal z-index

Changes

• app/api/translate/route.ts - GPT-4o-mini translation endpoint
• lib/hooks/useTextSelection.ts - Text selection detection hook
• components/reader/TranslationSheet.tsx - Translation UI
• components/reader/TextRenderPanel.tsx - Integration
• components/ui/dialog.tsx - Portal fix (createPortal to body)
• tests/e2e/reader/translation.spec.ts - 12 E2E test cases

Test plan

• Visit /reader, enter Spanish/Latin text, click "Render"
• Highlight a phrase → TranslationSheet appears with translation
• Click audio button → TTS plays source text
• Click "Create Flashcard" → modal appears above sheet
• Click word → TranslationSheet closes, DefinitionSidebar opens
• Run npx playwright test tests/e2e/reader/translation.spec.ts

🤖 Generated with Claude Code

[claude]

PR Review: Reader Translation Feature

Overview

This PR adds text selection-based translation to the reader with flashcard creation. The implementation is well-structured and follows the project's established patterns. Below are my findings organized by category.

---

✅ Strengths

1. Clean Architecture: The mutual exclusion pattern between TranslationSheet and DefinitionSidebar is well-implemented
2. Good UX Design: Responsive design (bottom sheet on mobile, sidebar on desktop) matches DefinitionSidebar pattern
3. Comprehensive Testing: 12 E2E test cases cover the main user flows effectively
4. Proper Error Handling: Translation API includes validation, error states, and user feedback
5. Accessibility: ARIA labels, keyboard support (Escape key), and semantic HTML

---

🔴 Critical Issues

1. Missing Authentication Check in Translation API

File: app/api/translate/route.ts

The translation endpoint doesn't verify user authentication. All other API routes in the codebase use createClient() from lib/supabase/server.ts to ensure the user is authenticated.

Risk: Unauthenticated users could abuse the translation API, leading to unexpected OpenAI costs.

Fix:

import { createClient } from '@/lib/supabase/server'

export async function POST(request: Request) {
  // Add authentication check
  const supabase = await createClient()
  const { data: { user }, error } = await supabase.auth.getUser()
  
  if (error || !user) {
    return NextResponse.json(
      { error: 'Unauthorized' },
      { status: 401 }
    )
  }
  
  // ... rest of the implementation
}

2. Potential Memory Leak in useTextSelection Hook

File: lib/hooks/useTextSelection.ts:60-84

The hook sets up event listeners on document but the debounce timeout (timeoutId) is cleared in the cleanup but the selection state persists across rapid component unmounts/remounts.

Issue: If a user rapidly navigates or the component remounts while a timeout is pending, setSelectedText could be called after unmount.

Fix: Add a mounted ref or use AbortController pattern:

useEffect(() => {
  if (disabled) {
    setSelectedText(null)
    return
  }
  
  let mounted = true
  // ... existing code ...
  
  const handleMouseUp = () => {
    if (timeoutId) clearTimeout(timeoutId)
    timeoutId = setTimeout(() => {
      if (mounted) handleSelectionChange()
    }, 10)
  }
  
  // ... rest of implementation ...
  
  return () => {
    mounted = false
    document.removeEventListener('mouseup', handleMouseUp)
    document.removeEventListener('touchend', handleTouchEnd)
    if (timeoutId) clearTimeout(timeoutId)
  }
}, [containerRef, minLength, maxLength, disabled])

---

⚠️ Important Issues

3. Missing Rate Limiting

The translation endpoint uses GPT-4o-mini without rate limiting. Consider adding:

• Per-user rate limits (e.g., 50 translations/hour)
• Request deduplication (cache translations in Supabase like TTS audio)
• Cost tracking

4. LangChain Dependency for Simple Translation

File: app/api/translate/route.ts:32-44

Using @langchain/openai for a simple translation adds unnecessary complexity and bundle size. The translation is straightforward enough to use OpenAI SDK directly.

Suggestion: Use OpenAI SDK directly like other endpoints, or document why LangChain is needed here.

5. Translation Not Cached

Unlike TTS audio (which is cached in Supabase per CLAUDE.md), translations are fetched every time. This could lead to:

• Increased API costs for repeated phrases
• Slower UX on repeated selections

Suggestion: Consider caching translations in Supabase similar to the audio caching pattern.

---

🟡 Minor Issues & Suggestions

6. Text Selection UX Edge Cases

File: lib/hooks/useTextSelection.ts

• Selecting text while audio is playing correctly disables selection, but the user gets no feedback
• No indication that text selection is disabled during playback
• Consider showing a toast: "Pause audio to select text"

7. Inconsistent Language Type Handling

File: app/api/translate/route.ts:8

The language enum is ['es', 'la', 'is'] but the translation prompt only mentions Spanish, Latin, and Icelandic generically. Consider using the LANGUAGE_NAMES map in the prompt for consistency.

Current:

const prompt = `Translate the following ${langName} text to English.`

This is actually fine - no change needed.

8. Missing Loading State During Translation Fetch

File: components/reader/TextRenderPanel.tsx:60-111

The translation fetch happens in a useEffect without cancellation if the user selects different text rapidly. Consider:

• Showing which text is being translated in the loading state
• Aborting in-flight requests when new text is selected (AbortController is used correctly, good!)

9. Z-Index Concerns

File: components/ui/dialog.tsx:17,27

The Dialog now uses z-[100] which is quite high. Make sure this doesn't conflict with other overlays (modals, dropdowns, etc.). Consider:

• Documenting z-index layers in a central location
• Using CSS variables for z-index values

10. E2E Test Robustness

File: tests/e2e/reader/translation.spec.ts

Tests use click({ clickCount: 3 }) to select text, which is browser-dependent. Consider:

• Using Playwright's selectText() API or dispatching selection events
• Adding wait states for network requests to avoid flakiness

Example flaky test scenario:

// Line 64: Could be flaky if translation takes >10s
await expect(page.getByRole('heading', { name: 'Translation' }))
  .toBeVisible({ timeout: 10000 })

Consider adding network wait:

await Promise.all([
  page.waitForResponse(resp => resp.url().includes('/api/translate')),
  readingPanel.click({ clickCount: 3 })
])

---

🔒 Security Review

No SQL Injection Risks

✅ All user inputs are properly validated with Zod

XSS Risks

✅ Translation output is rendered as text, not HTML

API Key Exposure

✅ OpenAI API key is server-side only

Missing CSRF Protection

⚠️ Consider adding request origin validation if translation is expensive

---

📊 Performance Considerations

1. Translation Latency: GPT-4o-mini is fast, but users might expect instant results. Consider:

   • Showing estimated wait time
   • Implementing optimistic caching

2. Text Selection Performance: The 10ms debounce is good, but consider increasing to 100-200ms to reduce API calls during text dragging

3. Memory: createPortal is used correctly for the Dialog, good!

---

🧪 Test Coverage

Strengths:

• 12 test cases cover main flows
• Tests mutual exclusion behavior
• Tests keyboard shortcuts
• Tests error states

Missing:

• Translation API unit tests
• useTextSelection hook tests
• Error scenarios (API failures, network errors)
• Boundary testing (max length 500 chars)

---

📋 Recommendations Summary

Must Fix Before Merge:

1. ✅ Add authentication to /api/translate/route.ts
2. ✅ Fix potential memory leak in useTextSelection.ts

Should Fix:
3. Add rate limiting to translation endpoint
4. Consider caching translations in Supabase
5. Add unit tests for translation API

Nice to Have:
6. Consider using OpenAI SDK directly instead of LangChain
7. Improve E2E test robustness with network waits
8. Document z-index layering strategy
9. Add user feedback when selection is disabled during audio playback

---

Code Quality: 8/10

This is a well-implemented feature that follows the project's conventions. The main concerns are authentication (critical), potential memory leak (important), and missing rate limiting (important for cost control). Once the critical issues are addressed, this will be ready to merge.

Great work on the comprehensive E2E tests and the clean mutual exclusion pattern! 🎉

[claude]

Pull Request Review: Text Selection → Translation with Flashcard Creation

Overview

This PR adds a well-designed text selection → translation feature to the reader. The implementation is solid with good separation of concerns, comprehensive E2E tests, and proper error handling. Below are my observations and recommendations.

---

✅ Strengths

1. Architecture & Code Quality

• Clean separation: Custom hook (useTextSelection) properly encapsulated
• Mutual exclusion: Correctly handles translation sheet vs definition sidebar states
• Reusable components: Good reuse of FlashcardSaver and AudioButton
• Portal pattern: Fixed dialog z-index issues with createPortal to body

2. Error Handling

• Proper AbortController usage for fetch cancellation (components/reader/TextRenderPanel.tsx:64)
• Zod validation on API endpoint (app/api/translate/route.ts:7-8)
• Graceful error states in UI with user-friendly messages

3. Test Coverage

• Comprehensive E2E tests: 12 test cases covering happy paths, error states, and interactions
• Tests multiple languages (Spanish, Latin, Icelandic)
• Tests mutual exclusion between translation and definition sidebars

4. UX Design

• Responsive design: bottom sheet on mobile, sidebar on desktop
• Loading states with spinner
• Escape key and click-to-close support
• Proper debouncing in text selection

---

🔴 Issues & Concerns

1. SECURITY: Missing Authentication Check

Location: app/api/translate/route.ts:21

The API lacks authentication checks. Any unauthenticated user could call this endpoint and consume OpenAI credits.

Recommendation: Add Supabase auth check at the start of the POST handler.

2. PERFORMANCE: No Translation Caching

Location: app/api/translate/route.ts:31-45

Unlike the definition sidebar which uses DictionaryCache, translations are fetched fresh every time. This unnecessarily increases costs and latency.

Recommendation: Implement translation caching similar to dictionary caching.

3. UX: Text Selection Feedback

Location: lib/hooks/useTextSelection.ts:35-60

When selection exceeds length limits (500 chars), there's no visual feedback. Users might be confused why nothing happens.

Recommendation: Add toast notification when selection exceeds limits.

4. ACCESSIBILITY: Missing ARIA Labels

• Translation sheet backdrop has no aria-label
• No focus trap when sheet is open
• Keyboard navigation needs improvement

---

⚠️ Potential Bugs

1. Race Condition in Selection Handler

Location: components/reader/TextRenderPanel.tsx:64-111

If a user quickly selects multiple phrases, state updates could race despite AbortController.

Recommendation: Add cleanup flag to prevent stale state updates.

2. Memory Leak: Portal Mounting

Location: components/ui/dialog.tsx:12-22

The mounted state has no cleanup on unmount.

Recommendation: Add cleanup in useEffect return function.

3. Edge Case: Empty Translation Response

Location: app/api/translate/route.ts:45

If GPT-4o-mini returns an empty string, the code doesn't handle it.

Recommendation: Add validation for empty translation before returning response.

---

🎯 Priority Recommendations

Must Fix (Before Merge):

1. Add authentication check to /api/translate endpoint
2. Fix potential race condition in translation fetch
3. Handle empty translation response

Should Fix (Next PR):

1. Implement translation caching for cost reduction
2. Add accessibility improvements (focus trap, ARIA labels)
3. Add user feedback for selection length violations

Nice to Have:

1. Extract magic numbers to constants (e.g., 10ms debounce timeout)
2. Improve error handling consistency
3. Add more edge case tests

---

📊 Summary

Category	Score	Notes
Code Quality	8/10	Clean, well-structured, good patterns
Security	6/10	Missing auth check (critical)
Performance	7/10	No caching for translations
Test Coverage	9/10	Excellent E2E coverage
Accessibility	6/10	Missing ARIA labels, focus management
Documentation	8/10	Good PR description, clear code

Overall: Strong implementation with some important security and performance issues to address. The architecture is solid and the feature is well-tested. Fix the authentication issue before merging.

---

✨ Positive Notes

• Excellent use of AbortController for cleanup
• Great test coverage with realistic scenarios
• Smart reuse of existing components
• Clean separation of concerns with custom hook
• Responsive design that works across devices
• Proper TypeScript usage throughout

Great work overall! The feature adds real value to the platform. Address the security issue and this will be ready to ship.