refactor(citation): drawer source-only view + popover expanded-view portal

[bensonwong]

Summary

CitationDrawer — source-only view

• Removed StatusProgressBar, ViewModeToggle (by-status / by-source toggle), and the status-grouped view — drawer now renders source-grouped citations only
• Removed internal lightbox/zoom overlay; removed ZoomInIcon import
• Removed StatusView render path and groupCitationsByStatus usage
• Added DrawerSourceHeading for favicon + source name label in the drawer header
• Simplified CitationDrawerTrigger to reuse trigger icons in the header and remove status-specific UI

CitationComponent — portal-based expanded views

• Added full-screen expanded-page view (← Back navigation) rendered via createPortal to document.body, independent of Radix positioning
• Added expanded-evidence modal (keyhole click): centered overlay sized to the snippet/evidence image, with backdrop-click dismiss
• Refactored PopoverViewState from 2 states ("summary" | "expanded") to 3 ("summary" | "expanded-evidence" | "expanded-page") so each mode can have distinct layout behaviour
• handleKeyholeClick pre-validates evidence image availability before transitioning, preventing a mobile stuck-state where the portal returns null but outside-tap is blocked
• Removed onImageClick callback threading through DefaultPopoverContent; expanded state is now driven entirely through onViewStateChange
• Made PagePill the close button in the expanded view; removed external proof link
• Made keyhole pan hints clickable to scroll the viewer rather than trigger expand
• Reused SourceContextHeader in ExpandedPageViewer for consistent header UI
• Added resolveExpandedImage unit tests

Other

• VerificationLog.tsx: removed duplicated colour-scheme helper, simplified layout
• icons.tsx: added pan/arrow and stack icons used by new UI
• Playwright drawer showcase spec updated for new source-only layout
• ShowcaseComponents.tsx: added showcase for new expanded-view states

Test plan

• Open a citation popover — verify summary view renders correctly
• Click the keyhole strip on a verified citation — evidence image modal appears, backdrop click and ← Back both dismiss it
• Click the expand button (full-page proof) — full-screen expanded-page overlay renders; ← Back returns to summary
• Press ESC in either expanded mode — overlay dismisses
• On mobile/touch: tap outside while expanded — popover stays open (not dismissed); tap ← Back to close
• Keyhole click on a citation with no evidence image — no transition occurs (no stuck state)
• Open the citations drawer — only source-grouped view renders; no status toggle visible
• Run bun test — all 803 tests pass

[claude]

Code Review: PR #273 - Citation UI Refactoring

Thanks for this substantial refactoring! The portal-based expanded views and simplified drawer are well-architected overall. Here's my detailed review:

---

🔴 CRITICAL ISSUES

1. Unsafe Type Cast Violates CLAUDE.md Guidelines

File: src/react/CitationDrawerTrigger.tsx:307

onKeyDown={e => {
  if (e.key === "Enter" || e.key === " ") {
    e.preventDefault();
    handleProofClick(e as unknown as React.MouseEvent);  // ❌ UNSAFE
  }
}}

Issue: Casting KeyboardEvent to MouseEvent violates CLAUDE.md "No Unsafe Casts" rule. This is type-unsafe since MouseEvent has different properties (clientX, clientY, button, etc.).

Fix: Create separate handlers or accept a union type:

const handleProofClick = (e: React.MouseEvent | React.KeyboardEvent) => {
  e.preventDefault();
  // ... rest of logic
};

---

🟠 HIGH PRIORITY BUGS

2. Event Propagation Bug in "Read More" Button

File: src/react/CitationDrawer.tsx:670-676

<button
  type="button"
  onClick={handleReadMore}  // ❌ Missing stopPropagation
  className="ml-1 text-blue-500..."
>
  Read more
</button>

Issue: This button is inside an expandable item (which has onClick={handleClick} at line 531). Without stopPropagation(), clicking "Read more" will bubble up and collapse the item while trying to expand it.

Expected: Read more modal opens, item stays expanded
Actual: Item collapses due to event bubbling

Fix:

onClick={(e) => {
  e.stopPropagation();
  handleReadMore();
}}

Note: Other buttons properly use this pattern (lines 285, 690).

---

🟡 MEDIUM PRIORITY

3. State Management Edge Case: wasAutoExpanded Reset

File: src/react/CitationDrawer.tsx:416-426, 634-636

useEffect(() => {
  if (defaultExpanded) {
    setIsExpanded(true);
    setWasAutoExpanded(true);
  }
}, [defaultExpanded]);

Issue: If defaultExpanded changes from true → false, wasAutoExpanded doesn't reset. The pulse animation key stays "remembered" even after collapse.

Fix:

useEffect(() => {
  setIsExpanded(defaultExpanded);
  setWasAutoExpanded(defaultExpanded);
}, [defaultExpanded]);

4. Discriminated Union Type Narrowing

File: src/react/CitationDrawer.tsx:489

const isDocument = citation.type === "document" || (!citation.type && citation.attachmentId);

Question: This suggests citation.type might be optional. According to CLAUDE.md, discriminated unions should have all constructors explicitly set the discriminator. Can you verify:

• Is type optional in the Citation type definition?
• If so, should all citation constructors be updated to always set type?

The fallback logic works but makes the discriminator less clear.

---

✅ EXCELLENT PATTERNS

Security: Image Source Validation

All image rendering paths correctly use isValidProofImageSrc() from the canonical location (src/react/constants.ts). This properly blocks:

• SVG data URIs (XSS protection)
• javascript: URIs
• Untrusted hosts
• Path traversal attempts

Test coverage in resolveExpandedImage.test.ts is exemplary with 28+ security-focused test cases. 🎉

Portal Null Safety

{popoverViewState !== "summary" && (() => {
  const container = getPortalContainer();
  if (!container) return null;  // ✓ Good SSR protection
  return createPortal(..., container);
})()}

Properly handles SSR environments where document.body may not exist.

---

📝 TEST COVERAGE GAPS

The resolveExpandedImage tests are excellent, but missing coverage for:

1. Portal rendering with SSR (null container scenario)
2. State transitions: summary → expanded-evidence → summary
3. State transitions: summary → expanded-page → summary
4. Keyboard navigation (Escape key closes expanded views)
5. Mobile touch: tap outside doesn't dismiss expanded portal

Recommendation: Add component or E2E tests for these expanded view interactions.

---

🔒 CLAUDE.md COMPLIANCE

Guideline	Status	Notes
Security Patterns	✅	Correct use of isValidProofImageSrc(), extractDomain()
No Variable Re-exports	✅	All imports from canonical locations
Type Safety	⚠️	One unsafe cast violation (item #1)
Discriminated Unions	⚠️	Type narrowing works but type optionality unclear

---

📊 SUMMARY

Security: Strong image validation, one unsafe type cast
Bugs: One event propagation bug, one state management edge case
Code Quality: Clean refactoring, good architectural separation
Tests: Excellent security tests, gaps in UI interaction tests

Recommended Actions Before Merge:

1. ✅ Fix unsafe cast (CitationDrawerTrigger.tsx:307)
2. ✅ Fix "Read more" propagation (CitationDrawer.tsx:670)
3. 📝 Consider adding expanded view interaction tests
4. 🤔 Clarify citation.type optionality for discriminated union compliance

Overall this is solid work! The portal architecture is well-designed and the security patterns are excellent. Just need to address the two concrete bugs above.

[github-actions]

❌ Playwright Test Report

Status: Tests failed

📊 Download Report & Snapshots (see Artifacts section)

What's in the Visual Snapshots

The gallery includes visual snapshots for:

• 🖥️ Desktop showcase (all variants × all states)
• 📱 Mobile showcase (iPhone SE viewport)
• 📟 Tablet showcase (iPad viewport)
• 🔍 Popover states (verified, partial, not found)
• 🔗 URL citation variants

---

Run ID: 22180179836

[claude]

Code Review for PR #273

This is a substantial refactor that simplifies the citation drawer and enhances the popover with portal-based expanded views. Overall, the code quality is high with good attention to security and architecture. Here are my findings:

✅ Strengths

1. Security

• ✅ Proper use of isValidProofImageSrc() validation throughout (lines 668, 678, 691, 697 in CitationComponent.tsx)
• ✅ Correct portal container usage with SSR safety via getPortalContainer()
• ✅ Good validation cascade in resolveExpandedImage() - skips invalid sources and falls through to next tier
• ✅ Comprehensive security tests in resolveExpandedImage.test.ts covering SVG data URIs, path traversal, untrusted hosts

2. Architecture & Best Practices

• ✅ Follows CLAUDE.md conventions: flattenCitations() moved to canonical location in CitationDrawer.utils.tsx (line 410) instead of re-exporting
• ✅ Direct imports from canonical locations (CitationDrawer.tsx:30-31)
• ✅ Proper type discrimination with 3-state PopoverViewState instead of binary expanded flag
• ✅ Portal-based overlays correctly escape Radix positioning constraints
• ✅ Good separation of concerns: resolveExpandedImage() exported for testing with biome-ignore comment

3. Code Quality

• ✅ Comprehensive test coverage for resolveExpandedImage() (54 new tests covering all tiers + edge cases)
• ✅ Clear documentation in comments explaining cascade priority and security model
• ✅ Proper cleanup of dead code (StatusProgressBar, ViewModeToggle, status-grouped view)

⚠️ Areas for Improvement

1. Potential Bug: Missing Evidence Image Validation (High Priority)
In CitationComponent.tsx:1780, the keyhole click handler validates evidence images:

if (!evidenceSrc || !isValidProofImageSrc(evidenceSrc)) return;

However, this validation happens after the state transition in some code paths. Consider the flow:

• Line 3033: Portal render checks if (!evidenceSrc || !isValidProofImageSrc(evidenceSrc)) return null;
• This is correct, BUT the guard at line 1780 in handleKeyholeClick should happen before any state updates

Recommendation: Verify the event handler sequence - ensure validation always precedes state changes to prevent stuck states mentioned in the PR description.

2. Type Safety: Discriminated Union Completeness
Per CLAUDE.md, when modifying discriminator fields, all constructors must be checked. The PopoverViewState type changed from 2 states to 3 states:

// Old: "summary" | "expanded"
// New: "summary" | "expanded-evidence" | "expanded-page"

Recommendation: Grep for all places that create or switch on viewState to ensure exhaustive handling. Missing: explicit check that all switch statements are exhaustive.

3. Performance: Portal Rendering Overhead
Three separate portals are now possible:

1. Drawer portal (CitationDrawer.tsx:1092)
2. Evidence modal portal (CitationComponent.tsx:3035)
3. Expanded page portal (CitationComponent.tsx:3085)

Each portal triggers layout recalculation. While this is likely acceptable for UI overlays, consider:

• The "1×1 invisible placeholder" pattern (line 3028) when expanded - does this cause unnecessary Radix re-renders?
• Document-level ESC listener is added/removed frequently (lines ~2100+) - consider debouncing or single global handler

4. Accessibility: Missing ARIA Landmarks
The pan hint buttons (← Pan / Pan →) have biome-ignore for a11y:

// biome-ignore lint/a11y/useKeyWithClickEvents

While the comment mentions "keyboard access handled by parent/document-level handlers," the pan hints are now clickable (line 72c8613 commit).

Recommendation: Add role="button" and tabIndex={0} to make them keyboard-accessible, or document why they should remain pointer-only.

5. Code Clarity: Complex Portal Logic
The expanded view rendering logic spans ~50 lines with nested ternaries (lines 3025-3100). Consider extracting into separate components:

<ExpandedEvidenceModal />
<ExpandedPageViewer />

This would improve testability and readability.

🔍 Minor Issues

6. Unused Parameter Naming Convention
CitationDrawer.tsx:463 and VerificationLog.tsx use _anchorText prefix for unused params. While this works, the CLAUDE.md anti-pattern warning suggests deleting unused code entirely rather than renaming.

Question: Is anchorText needed for future use, or can the parameter be removed from the function signature?

7. Test Coverage Gap
The new resolveExpandedImage() tests are excellent, but missing:

• Integration tests for the 3-state view transitions
• Tests for the "stuck state" bug mentioned in PR description (evidence image availability check)

Recommendation: Add a test case:

it("prevents expanded-evidence transition when evidence image is invalid", () => {
  // Simulate keyhole click with missing/invalid verificationImageSrc
  // Assert viewState remains "summary"
});

8. Magic Numbers
CitationDrawer.tsx:1033 has maxIcons={5} hardcoded. Consider extracting to a named constant:

const DRAWER_MAX_STATUS_ICONS = 5;

🎯 Summary

Code Quality: ⭐⭐⭐⭐ (4/5)
Security: ⭐⭐⭐⭐⭐ (5/5)
Adherence to Conventions: ⭐⭐⭐⭐⭐ (5/5)

Overall Assessment: This is a well-executed refactor with strong security practices and proper architectural patterns. The main concerns are around validation timing (item #1), accessibility (item #4), and test coverage for the new 3-state model (item #7). The no-re-exports convention is correctly followed, and the canonical location pattern is properly maintained.

Recommendation: Approve with minor revisions. Address items #1, #4, and #7 before merging. Items #2-3, #5-6, and #8 are nice-to-haves that could be follow-up tasks.

---

References:

• Security validation: src/react/CitationComponent.tsx:658-707
• Canonical locations: src/react/CitationDrawer.utils.tsx:410, src/react/CitationDrawerTrigger.tsx:305
• Portal usage: src/react/CitationComponent.tsx:3035, 3085
• Test coverage: src/__tests__/resolveExpandedImage.test.ts

[claude]

PR 273 Review: Drawer Source-Only View + Popover Expanded-View Portal

Summary

This PR refactors the citation drawer to a simplified source-only view and introduces portal-based expanded views for citation popovers. The changes are well-structured and demonstrate good engineering practices with comprehensive test coverage.

---

Positive Aspects

Architecture & Design

1. Portal-Based Rendering: The new portal-based approach for expanded views (createPortal) properly separates overlays from Radix positioning constraints, preventing z-index conflicts. The SSR-safe getPortalContainer() pattern is correctly applied.

2. State Machine Clarity: Expanding PopoverViewState from 2 to 3 states (summary | expanded-evidence | expanded-page) makes the UI modes explicit and easier to reason about.

3. Pre-validation Pattern: The handleKeyholeClick function pre-validates image availability before transitioning states, preventing mobile stuck-states. This is excellent defensive programming:

   if (!evidenceSrc || !isValidProofImageSrc(evidenceSrc)) return;

4. Comprehensive Test Coverage: New resolveExpandedImage.test.ts provides thorough security validation testing (SVG data URIs, path traversal, protocol-relative URLs, etc.) with 61 new test cases covering the 4-tier fallback cascade.

Code Quality

5. Security-First Approach: All image sources are validated through isValidProofImageSrc() before rendering, blocking SVG data URIs, javascript: URIs, untrusted hosts, path traversal, and protocol-relative URLs.

6. Canonical Exports: The PR correctly exports resolveExpandedImage and AnchorTextFocusedImage from their implementation file with no re-exports found elsewhere.

7. Proper Icon Usage: New icons (HandIcon, ExpandArrowsIcon) follow the existing pattern with size controlled by parent containers and consistent accessibility attributes.

---

Issues Found

Security Concerns (Medium Priority)

1. Mixed URL Domain Validation Patterns ⚠️

Location: src/react/SourcesListComponent.utils.tsx:34-63

Issue: The file mixes safe isDomainMatch() calls with unsafe .includes() substring matching:

// ✓ Safe
if (isDomainMatch(url, "twitter.com") || isDomainMatch(url, "x.com")) return "social";

// ✗ Unsafe - vulnerable to spoofing
if (url.includes("mastodon")) return "social";  // matches evil.com/mastodon
if (url.includes("scholar.google")) return "academic";  // matches evil.com/scholar.google
if (url.includes("pubmed")) return "academic";  // matches evil.com/pubmed
if (url.includes("news.")) return "news";  // matches evil.com/news.fake
if (url.includes("discourse") || url.includes("forum")) return "forum";

Impact: An attacker could host malicious content at evil.com/mastodon/phishing.html and have it categorized as "social media" with different trust indicators.

Recommendation: Replace all .includes() calls with proper domain checks or pathname checks:

// For substrings in path, check pathname explicitly
if (isDomainMatch(url, "mastodon.social") || 
    new URL(url).pathname.includes("/discourse")) return "forum";

// For generic patterns like "news.", validate it's in the domain
if (extractDomain(url).startsWith("news.")) return "news";

CLAUDE.md Reference: Section "Important: Security Patterns" → "URL Domain Matching"

2. Unsafe TypeScript Casts Found ⚠️

Locations:

• src/react/SourcesListComponent.tsx:164: e as unknown as React.MouseEvent<HTMLDivElement>
• src/react/prefetchCache.ts:73: window as unknown as WindowWithPrefetchCache
• src/react/primitives.tsx:161: e as unknown as MouseEvent<HTMLSpanElement>
• src/react/CitationVariants.tsx:103: e as unknown as React.MouseEvent<HTMLSpanElement>

Issue: Multiple instances of as unknown as type casts without justification comments. While some may be legitimate (like the window cast for extending the global), event casts suggest potential type mismatches.

Recommendation:

1. Add comments explaining why each cast is safe
2. Consider using type guards instead where possible
3. For event casts, investigate if the underlying event types can be fixed

CLAUDE.md Reference: Section "Important: Type Safety" → "No Unsafe Casts"

Code Quality Issues (Low Priority)

3. New Exports Not Verified in Index Files

Issue: resolveExpandedImage and AnchorTextFocusedImage are exported from CitationComponent.tsx but need verification they're properly exported from /src/react/index.ts for public API access (if intended).

Recommendation: If these are meant to be public APIs, add to src/react/index.ts. If test-only, consider moving to a separate test utils file.

CLAUDE.md Reference: Section "Important: Type Safety" → "Export Verification"

4. Screenshot Normalization Function Lacks Input Validation

Location: src/react/CitationComponent.tsx:646-648

function normalizeScreenshotSrc(raw: string): string {
  return raw.startsWith("data:") ? raw : `data:image/jpeg;base64,${raw}`;
}

Issue: Assumes input is always a string. While isValidProofImageSrc() validates the result, it's safer to validate inputs earlier.

Recommendation:

function normalizeScreenshotSrc(raw: string): string {
  if (!raw || typeof raw !== 'string') {
    throw new Error('Invalid screenshot data');
  }
  return raw.startsWith("data:") ? raw : `data:image/jpeg;base64,${raw}`;
}

Performance Considerations

5. Repeated normalizeScreenshotSrc() Calls

Locations: Multiple calls in CitationComponent.tsx and CitationDrawer.tsx

Issue: The same verification.url?.webPageScreenshotBase64 value is normalized multiple times without memoization.

Recommendation: Consider memoizing the normalized value:

const normalizedUrlScreenshot = useMemo(() => {
  const raw = verification?.url?.webPageScreenshotBase64;
  return raw ? normalizeScreenshotSrc(raw) : undefined;
}, [verification?.url?.webPageScreenshotBase64]);

Impact: Minor - normalization is cheap, but memoization would align with React best practices.

---

CLAUDE.md Compliance

✅ Compliant Areas

1. No Variable Re-Exports: No barrel re-exports found
2. Security Pattern Usage: Properly uses isValidProofImageSrc() throughout
3. Canonical Locations: New functions are defined in their canonical locations
4. Type Safety: No discriminated union issues; new interfaces are properly typed

⚠️ Non-Compliant Areas

1. Security Patterns: Mixed use of safe isDomainMatch() and unsafe .includes() in SourcesListComponent.utils.tsx (Issue fix: correct package exports to point to lib/ instead of src #1 above)
2. Type Safety: Multiple as unknown as casts without justification (Issue chore: update package-lock.json and ignore tsbuildinfo #2 above)

---

Test Coverage Assessment

New Tests: resolveExpandedImage.test.ts ✅

Strengths:

• 61 comprehensive test cases covering all security edge cases
• Tests the complete 4-tier fallback cascade
• Validates rejection of malicious inputs (SVG, javascript:, path traversal)
• Tests new URL screenshot tier (tier 3)
• Good coverage of null/undefined handling

Updated Tests

CitationComponentBehavior.test.tsx: Updated selectors from [role='dialog'] to specific component queries. More fragile but necessary given portal refactor.

CitationDrawer.test.tsx: Updated to reflect source-only view (no status toggle).

dotIndicator.test.tsx: Updated size expectations (minor styling tweak).

Missing Test Coverage

1. Portal rendering: No tests verify portal renders to document.body vs parent component
2. Keyboard navigation: ESC key dismissal not explicitly tested for new expanded states
3. Touch device behavior: No tests for touch-specific interaction patterns
4. Error boundaries: No tests for image load failures in expanded view

---

Suggestions & Improvements

High Priority

1. Fix Security Issue fix: correct package exports to point to lib/ instead of src #1: Replace all unsafe .includes() domain checks with isDomainMatch() or proper URL parsing
2. Document Type Casts: Add comments justifying each as unknown as cast or remove them

Medium Priority

3. Memoize Screenshot Normalization: Prevent redundant string operations
4. Add Input Validation: Strengthen normalizeScreenshotSrc() with explicit checks
5. Add Portal Tests: Verify portal mounting behavior and cleanup

Low Priority

6. Consider Consolidation: normalizeScreenshotSrc() is called in both files - consider extracting to a shared utility
7. Error Handling: Add explicit error handling for malformed base64 data
8. Accessibility: Verify screen reader announcements work correctly with portal-based overlays

---

Summary & Recommendation

Overall Assessment: ✅ Approve with minor changes requested

This PR demonstrates strong engineering practices with thorough security validation, comprehensive test coverage, and clear architectural improvements. The portal-based approach solves real positioning issues, and the simplified drawer is easier to maintain.

Critical Path:

1. Fix Issue fix: correct package exports to point to lib/ instead of src #1 (unsafe domain matching) - REQUIRED for security
2. Address Issue chore: update package-lock.json and ignore tsbuildinfo #2 (document type casts) - REQUIRED for maintainability
3. Other suggestions are optional improvements

Test Coverage: Excellent - 61 new tests with strong security focus. Minor gaps in portal behavior and edge cases but nothing blocking.

Code Quality: Very high - follows established patterns, good separation of concerns, proper use of React hooks and portals.

CLAUDE.md Compliance: Mostly compliant - two violations noted above should be addressed.

---

Estimated Risk: Low (with security fixes applied)
Merge Recommendation: Approve after addressing Issues #1 and #2