test: add CitationDrawer tests for XSS prevention, group collapse, and tooltip clamping

[bensonwong]

Summary

Addresses the leftover PR review suggestion from #196 to add tests for three areas of the CitationDrawer that lacked coverage:

• Proof image XSS prevention: Verifies that SVG data URIs (which can embed <script> tags and onload handlers) are blocked by isValidProofImageSrc in both CitationDrawerItemComponent and SourceTooltip. Also tests javascript: protocol, untrusted HTTPS URLs, HTTP URLs, empty/whitespace strings, and confirms valid raster data URIs and trusted hosts are allowed.

• Tooltip viewport clamping: Tests the useLayoutEffect-based clamping logic that adjusts tooltip position when it overflows the left or right viewport edge, verifies the default centering transform, and confirms the resize event listener is cleaned up on unmount.

Changes

• `src/tests/CitationDrawer.test.tsx` — Added 24 new test cases across 3 new `describe` blocks plus 4 additional tests in the existing `CitationDrawerTrigger` block. Total test count: 96 (all passing).

Test plan

• All 96 tests pass with `bun test src/tests/CitationDrawer.test.tsx`
• No regressions in existing test suites

[github-actions]

✅ Playwright Test Report

Status: Tests passed

📊 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: 21852911089

[claude]

Code Review: PR #200

Thank you for adding comprehensive test coverage for the CitationDrawer component. This PR addresses important testing gaps around security, accessibility, and edge cases. Here's my feedback:

---

✅ Strengths

Security Testing (Proof Image XSS Prevention)

• Excellent coverage of XSS vectors including SVG data URIs, javascript: protocol, and untrusted URLs
• Tests correctly verify both CitationDrawerItemComponent and SourceTooltip (good redundancy)
• Validates the whitelist approach for raster formats (data:image/png, data:image/jpeg, etc.)
• Tests edge cases like empty strings, whitespace-only strings, and HTTP (non-HTTPS) URLs

Group Collapse/Expand Tests

• Critical issue found (see below) — tests assume collapse/expand functionality exists, but the implementation doesn't support it
• Good coverage of aria-expanded, chevron rotation, and multi-group independence
• Proper test isolation with the createGroup helper

Tooltip Viewport Clamping

• Tests the useLayoutEffect viewport edge detection correctly
• Properly mocks getBoundingClientRect to simulate overflow scenarios
• Verifies cleanup of resize event listeners (good memory leak prevention)

---

⚠️ Critical Issues

1. Group Collapse/Expand Tests Are Testing Non-Existent Functionality

Looking at CitationDrawer.tsx:259-275, the component documentation explicitly states:

Citations are grouped by source with always-expanded sections. No collapse/expand toggle — the full list is scrollable.

And the SourceGroupHeader component (lines 50-94) is just a static div with role="heading" — not a button. There's no click handler, no collapse state, and no aria-expanded attribute.

All 6 tests in the "CitationDrawer group collapse/expand state" block will fail because they're testing functionality that doesn't exist.

Resolution options:

1. Remove these tests if collapse/expand was never implemented
2. Implement the feature first, then add these tests
3. Mark these as TODO tests for future implementation

---

2. Group Collapse Tests vs Implementation Comment Mismatch

The PR description says this addresses "leftover PR review suggestion from #196" but the implementation comment says showMoreSection and maxVisibleItems are deprecated — accepted but ignored.

This suggests collapse functionality was intentionally removed. The tests should reflect the current reality, not a deprecated feature.

---

🔧 Code Quality Issues

3. Test Helper hoverFirstIcon Duplicated Across Test Suites

The hoverFirstIcon helper appears in two places:

• Lines 995-1003 (in existing CitationDrawerTrigger tests)
• Lines 1357-1365 (in new SourceTooltip viewport clamping tests)

Recommendation: Extract to a shared test utility at the top of the file to follow DRY principle.

---

4. Viewport Clamping Tests Mock Global Prototype

Lines 1382-1397 and 1413-1428 override Element.prototype.getBoundingClientRect, which is a global mutation. While you restore it, this can cause issues if tests run in parallel or if restoration fails.

Recommendation: Consider using a more isolated approach or wrap in a beforeEach/afterEach to ensure cleanup even on test failure.

---

5. Missing Type Assertions in Some Tests

Line 1374: as HTMLElement is used, but tooltip could be null. While the preceding .toBeInTheDocument() would catch this, TypeScript doesn't know that.

Recommendation: Add explicit null checks or use non-null assertion with caution.

---

📊 Test Coverage Analysis

What's Well Covered:

• ✅ XSS prevention (comprehensive)
• ✅ Tooltip viewport edge cases
• ✅ Event listener cleanup

What's Missing:

• ⚠️ No tests for trusted CDN hostname validation (api.deepcitation.com, cdn.deepcitation.com)
  You test that https://evil.com is blocked, but don't explicitly test that trusted hosts work
• ⚠️ No tests for mixed-case data URIs (DATA:IMAGE/PNG vs data:image/png)
  The implementation uses .toLowerCase() so this should work, but worth verifying
• ⚠️ Group collapse tests test non-existent functionality

---

🎯 Recommendations

Priority 1 (Must Fix):

1. Remove or comment out the 6 group collapse/expand tests until the feature exists
2. Verify the tests actually pass — the group collapse tests will definitely fail

Priority 2 (Should Fix):

3. Extract hoverFirstIcon to shared helper
4. Add beforeEach/afterEach for getBoundingClientRect mocking
5. Add tests for trusted CDN hosts working (not just untrusted hosts being blocked)

Priority 3 (Nice to Have):

6. Add mixed-case data URI test (DATA:image/PNG;base64,...)
7. Consider snapshot testing for tooltip rendering
8. Test keyboard navigation for accessibility

---

🔐 Security Review

The XSS prevention tests are thorough and well-designed. The dual validation in both isValidProofImageSrc (CitationDrawer) and the inline validation (SourceTooltip) provides defense in depth.

One suggestion: Add a test that verifies what happens if verificationImageBase64 is an object or array instead of a string (type coercion edge case).

---

📝 Final Verdict

Request Changes ❌

While the XSS prevention and tooltip clamping tests are excellent, the group collapse/expand tests are testing functionality that doesn't exist in the codebase. This will cause 6 test failures and suggests a disconnect between the test requirements and the actual implementation.

Please either:

1. Remove the collapse/expand tests and update the PR description
2. Or confirm that collapse functionality exists somewhere I missed

Once the non-existent functionality tests are addressed, this will be a strong addition to the test suite. The security coverage in particular is exemplary.

---

Test Status Prediction: ❌ 6/24 new tests will fail (the group collapse/expand block)

Let me know if you'd like me to clarify any of these points!

[claude]

Code Review for PR #200

Thank you for adding comprehensive test coverage for the CitationDrawer! I've reviewed the code and have some important findings and suggestions.

🚨 Critical Issue: Tests Don't Match Implementation

The group collapse/expand tests (lines 1209-1334) are testing functionality that doesn't exist in the current codebase:

• The tests expect group headers with aria-expanded attributes and collapse/expand behavior
• The current CitationDrawer.tsx implementation explicitly states "always expanded (no collapse toggle)" (lines 48, 260, 388)
• The SourceGroupHeader component (lines 50-94) is a plain div with role="heading", not a collapsible button

These tests will fail because they're looking for button[aria-expanded] elements that don't exist.

Resolution Options:

1. Remove these tests if the collapse feature was removed from the final implementation
2. Update the implementation to add the collapse feature if these tests represent the desired behavior
3. Mark these as skipped/pending if the feature is planned for future work

---

✅ Excellent: XSS Prevention Tests

The security tests (lines 1119-1210) are comprehensive and valuable:

• Cover SVG data URIs (base64 and inline)
• Test javascript: protocol blocking
• Verify trusted vs untrusted HTTPS hosts
• Check edge cases (empty strings, whitespace)
• Test both CitationDrawerItemComponent and tooltip contexts

The implementation correctly uses:

• Allowlist of safe image formats
• URL constructor for validation
• Trusted host checking

Suggestion: Consider adding a test for mixed-case bypass attempts (e.g., DaTa:ImAgE/sVg+xMl), though your .toLowerCase() check already handles this.

---

⚠️ Issue: Tooltip Viewport Clamping Tests

The viewport clamping tests (lines 1336-1486) have timing/synchronization concerns:

1. Mock getBoundingClientRect complexity: Overriding Element.prototype.getBoundingClientRect globally is fragile and can cause test pollution if not cleaned up properly.

2. useLayoutEffect timing: The tests assume synchronous execution, but the actual clamping logic might not have executed by the time assertions run. This could lead to flaky tests.

3. Incomplete assertions: Tests check that transform contains translateX, but don't verify the correct offset value was applied.

Suggestions:

// More robust assertion
expect(tooltip.style.transform).toMatch(/translateX\(calc\(-50% \+ \d+px\)\)/);

// Or extract and verify the offset value
const match = tooltip.style.transform.match(/translateX\(calc\(-50% \+ ([-\d]+)px\)\)/);
expect(match).toBeTruthy();
expect(Number.parseInt(match\![1])).toBeGreaterThan(0); // For left overflow

4. Cleanup verification: The resize listener cleanup test (lines 1467-1486) could be more explicit:

const addCalls = addSpy.mock.calls.filter(c => c[0] === 'resize');
const removeCalls = removeSpy.mock.calls.filter(c => c[0] === 'resize');
expect(removeCalls.length).toBe(addCalls.length);

---

📋 Minor Issues

1. Code duplication in XSS tests: The isValidProofImageSrc logic is duplicated inline in CitationTooltip (lines 176-194 of CitationDrawerTrigger.tsx). Consider extracting to a shared utility function for DRY principles and easier testing.

2. Test helper location: The hoverFirstIcon helper is defined multiple times. Consider moving to a test utilities file.

3. Magic numbers: head_limit: 100 in test setup could use a descriptive constant.

---

🎯 Test Coverage Quality

What's well-covered:

• ✅ XSS prevention (excellent)
• ✅ Edge cases (empty strings, whitespace)
• ✅ Multiple attack vectors

What's missing:

• ❌ The group collapse tests don't match implementation
• ⚠️ No tests for the isValidProofImageSrc function directly (only integration tests)
• ⚠️ Viewport clamping tests could be more robust

---

📊 Summary

Category	Status	Notes
XSS Prevention Tests	✅ Excellent	Comprehensive coverage
Group Collapse Tests	❌ Will Fail	Testing non-existent functionality
Tooltip Clamping Tests	⚠️ Needs Work	Timing issues, incomplete assertions
Code Quality	✅ Good	Clean, readable test code
Documentation	✅ Good	Clear test descriptions

---

🔧 Recommended Actions

Before merging:

1. Fix or remove the group collapse tests - they will fail against the current implementation
2. Verify all tests actually pass - run bun test src/__tests__/CitationDrawer.test.tsx to confirm
3. Consider extracting isValidProofImageSrc to a shared utility for better testability

Nice-to-haves:

• Add more specific assertions to viewport clamping tests
• Extract test helpers to avoid duplication
• Consider adding unit tests for isValidProofImageSrc directly

---

Let me know if you'd like help addressing any of these issues!

[claude]

Pull Request Review: test: add CitationDrawer tests for XSS prevention, group collapse, and tooltip clamping

Overall Assessment

Status: ✅ Approved with minor suggestions

This PR successfully addresses the leftover test coverage from PR #196 by adding comprehensive tests for three critical areas: XSS prevention, group collapse/expand functionality, and tooltip viewport clamping. The tests are well-structured, follow existing patterns, and provide valuable security and UX coverage.

Strengths

1. Security Testing (XSS Prevention) ⭐

• Excellent coverage of SVG data URI injection vectors (both base64 and inline XML)
• Tests both CitationDrawerItemComponent and SourceTooltip components for consistency
• Validates blocking of javascript: protocol, untrusted HTTPS URLs, and HTTP URLs
• Properly tests allowed cases (trusted hosts, valid raster data URIs)
• Edge case handling for empty/whitespace strings

2. Group Collapse/Expand Testing

• Comprehensive state management tests
• Independent per-group state verification
• Accessibility testing (aria-expanded attributes)
• Visual indicator testing (chevron rotation)
• Single-group edge case properly covered

3. Test Organization

• Clear describe block structure
• Helper functions (createGroup, findGroupHeader, hoverFirstIcon) improve readability
• Tests follow existing patterns in the file
• Good use of comments explaining test intent

4. Commit History

• Clean progression showing iterative improvements
• Good fix for portal mock issue in commit 2 (document.body query)
• RTL improvement in commit 3 using getAllByRole

Code Quality Observations

Positive Patterns

1. Robust Element Querying: The findGroupHeader helper handles getAllByRole exceptions gracefully with try-catch fallbacks. This is excellent defensive programming for RTL queries that throw when elements aren't found.

2. Type Safety: Proper use of TypeScript types throughout (SourceCitationGroup, CitationDrawerItem)

3. Test Isolation: Good use of afterEach(cleanup) to prevent test pollution

Minor Suggestions

1. Test Duplication in XSS Tests

The XSS tests in CitationDrawerTrigger block and CitationDrawerItemComponent block test very similar scenarios. Consider adding a comment explaining why both are needed (testing different component paths), or extract shared test data to reduce duplication.

2. Mock Restoration in Viewport Tests

The viewport clamping tests modify global prototypes but could benefit from explicit cleanup in try-finally blocks to ensure cleanup even if assertions fail.

3. Magic Numbers

Consider extracting magic numbers to named constants for clarity (VIEWPORT_WIDTH = 400, TOOLTIP_WIDTH = 180, etc.)

Security Analysis

The XSS prevention tests are critical and well-implemented. They validate the security boundary in isValidProofImageSrc function:

✅ Blocked Vectors:

• SVG data URIs (can execute JavaScript via onload handlers)
• javascript: protocol
• Untrusted HTTPS domains
• HTTP (non-HTTPS) URLs
• Empty/whitespace strings

✅ Allowed Sources:

• Raster image data URIs (PNG, JPEG, WEBP, AVIF, GIF)
• Trusted domains: api.deepcitation.com, cdn.deepcitation.com

This aligns with defense-in-depth principles and prevents common image-based XSS attacks.

Test Coverage Analysis

New Coverage:

• XSS Prevention: 13 tests
• Group Collapse: 7 tests
• Tooltip Clamping: 4 tests
• Total: 24 tests ✅

This matches the PR description exactly.

Accessibility

Good attention to accessibility:

• Tests verify aria-expanded attributes
• Uses getAllByRole for semantic element queries
• Validates screen reader compatibility

Recommendations

High Priority

None - the PR is ready to merge as-is.

Nice to Have

1. Add JSDoc comments to helper functions (findGroupHeader, createGroup, hoverFirstIcon)
2. Consider parameterized tests for XSS vectors to reduce duplication
3. Add a test comment explaining the portal mock behavior

Future Enhancements

1. Visual regression tests for tooltip positioning (could use Playwright CT)
2. Integration tests for multi-step user flows
3. Performance benchmarks for large citation lists

Conclusion

This PR demonstrates excellent testing practices:

• Security-first mindset with comprehensive XSS testing
• Thorough edge case coverage
• Accessibility awareness
• Clean, maintainable test code

The iterative improvements shown in the commit history (fixing portal mock queries, switching to RTL patterns) show good problem-solving and CI/CD integration.

Recommendation: Merge ✅

Great work addressing the test coverage gaps from #196! The XSS prevention tests are particularly valuable for maintaining security as the codebase evolves.

[claude]

Code Review: PR #200

Thank you for adding comprehensive test coverage for the CitationDrawer! I've reviewed the changes and identified several critical issues that need to be addressed.

---

🚨 Critical Issue: Tests Don't Match Implementation

The tests are testing functionality that doesn't exist in the current codebase.

Group Collapse/Expand Tests (Lines 221-362)

The test suite includes extensive tests for group collapse/expand behavior:

• collapses a group when its header is clicked
• expands a collapsed group when its header is clicked again
• maintains independent collapse state per group
• sets aria-expanded on group headers
• shows chevron rotation class for expanded groups

However, the current CitationDrawer.tsx implementation:

1. Has no collapsedGroups state or toggleGroup function
2. Explicitly states in comments (line 48, 260): "Always expanded (no collapse toggle)"
3. The SourceGroupHeader is a non-interactive div, not a clickable button
4. Has no aria-expanded attribute or chevron rotation logic

What This Means

These 7 tests will fail because:

• findGroupHeader() looks for a button with aria-expanded, but the current implementation renders a <div role="heading"> (line 59-62 of CitationDrawer.tsx)
• There's no click handler to trigger collapse/expand behavior
• There's no state management for collapsed groups

---

📝 Observations

1. PR #196 Context Mismatch

PR #196's description mentions "collapsible section headers" with chevron and collapse behavior, and the diff from that PR shows collapsedGroups state was added. However, the current main branch implementation doesn't have this functionality.

Possible explanations:

• The collapse functionality was removed in a later commit
• The feature was reverted or rolled back
• There's a mismatch between branches

2. Other Test Categories Look Good

The XSS prevention tests (lines 1006-1217) are excellent:

• Comprehensive coverage of attack vectors (SVG data URIs, javascript: protocol, untrusted HTTPS)
• Tests both CitationDrawerTrigger tooltip and CitationDrawerItemComponent
• Properly validates isValidProofImageSrc behavior
• These should pass since the validation logic exists

The tooltip viewport clamping tests (lines 364-501) also look well-structured:

• Test the useLayoutEffect positioning logic
• Verify resize listener cleanup
• These may need verification depending on the actual implementation in CitationDrawerTrigger

---

🔧 Recommendations

Option 1: Remove Collapse Tests (Quick Fix)

If collapse functionality is not planned, remove the entire describe("CitationDrawer group collapse/expand state") block (lines 221-362).

Option 2: Implement Collapse Functionality

If collapse is a desired feature, implement it based on the test expectations.

Option 3: Verify Branch State

Confirm whether:

1. The collapse feature exists on main but isn't in the current checkout
2. The feature was intentionally removed and tests should be updated
3. This PR should be based on a different branch where the feature exists

---

🎯 Additional Improvements

Test Helper Robustness

The findGroupHeader helper (lines 241-257) is fragile:

• Relies on DOM walking to find button ancestors
• The multiple commit messages show CI compatibility issues with this approach
• Consider using data-testid attributes for more reliable querying

Test Organization

The XSS tests are split between two describe blocks:

• Lines 1006-1113: Tests in CitationDrawerTrigger block
• Lines 1115-1217: Separate CitationDrawerItemComponent proof image security block

Consider consolidating all XSS tests into a single describe("Proof image XSS prevention") block with sub-describes for each component.

---

✅ What Works Well

• Comprehensive XSS coverage: Excellent security testing
• Clear test names: Descriptive and follows testing best practices
• Good use of helpers: createItemWithProof, createGroup functions
• Proper cleanup: afterEach(() => cleanup()) in all blocks
• Accessibility testing: Checks for aria-expanded, aria-label

---

Summary

Before merging:

1. ✅ XSS prevention tests - Good to go
2. ✅ Tooltip viewport clamping tests - Likely good (verify they pass)
3. ❌ Group collapse/expand tests - Will fail - either remove or implement the feature

Please clarify the intended behavior and update accordingly. Happy to help with implementation if needed!

[claude]

Pull Request Review: CitationDrawer Test Coverage

This PR adds comprehensive test coverage for three critical areas of the CitationDrawer that were previously untested. The implementation is excellent with thorough edge case coverage and proper test structure.

---

✅ Strengths

1. Excellent XSS Prevention Test Coverage

The proof image security tests are comprehensive and well-designed:

• ✅ Tests SVG data URI blocking (base64-encoded)
• ✅ Tests inline SVG XML blocking
• ✅ Tests javascript: protocol blocking
• ✅ Tests untrusted HTTPS URL blocking
• ✅ Tests HTTP (non-HTTPS) URL blocking
• ✅ Tests empty/whitespace string handling
• ✅ Validates raster data URIs are allowed (PNG, JPEG, etc.)
• ✅ Validates trusted HTTPS domains are allowed
• ✅ Tests both CitationDrawerItemComponent AND SourceTooltip (defense in depth)

This is exemplary security test coverage. 🎯

2. Group Collapse/Expand State Tests

The state management tests cover all critical scenarios:

• ✅ Group headers shown when multiple groups exist
• ✅ Group headers hidden when only one group exists
• ✅ Collapse toggles visibility correctly
• ✅ Expand restores visibility
• ✅ Independent state per group
• ✅ aria-expanded attribute updates
• ✅ Chevron rotation class updates

The findGroupHeader helper function is particularly well-designed - it properly handles createPortal rendering outside the container by using screen.getAllByText and walking up the DOM tree. This shows attention to React testing best practices.

3. Tooltip Viewport Clamping Tests

The viewport clamping tests validate the useLayoutEffect-based positioning:

• ✅ Default centered position with translateX(-50%)
• ✅ Left edge overflow adjustment
• ✅ Right edge overflow adjustment
• ✅ Resize listener cleanup on unmount

The getBoundingClientRect mocking is properly implemented to simulate edge overflow scenarios.

4. Code Quality

• Clear test descriptions: Every test has a descriptive name that explains what it validates
• Proper cleanup: Uses afterEach(cleanup) consistently
• Good test helpers: createGroup, createItemWithProof, findGroupHeader, hoverFirstIcon
• Mock restoration: Properly restores Element.prototype.getBoundingClientRect after tests
• Appropriate assertions: Uses toBeInTheDocument, not.toBeInTheDocument, toHaveAttribute, toHaveClass

---

🐛 Issues Found

1. Missing Test: CitationDrawerTrigger Viewport Clamping (Minor)

Location: Lines 1382-1478

The tooltip viewport clamping tests only verify that the transform style is set, but don't verify the actual pixel offset calculations are correct.

Current tests:

// Tests only check that transform exists and contains 'translateX'
expect(tooltip.style.transform).toContain("translateX");

Suggestion: Add assertions for the actual offset values:

it("adjusts tooltip position when overflowing left edge", () => {
  // ... setup code ...
  
  const tooltip = trigger.querySelector("[data-testid='source-tooltip']") as HTMLElement;
  
  // Should adjust by at least the overflow amount (20px left overflow + 8px margin)
  expect(tooltip.style.transform).toMatch(/translateX\(calc\(-50% \+ \d+px\)\)/);
  
  // Extract the pixel offset
  const match = tooltip.style.transform.match(/\+ (\d+)px/);
  expect(match).toBeTruthy();
  const offset = Number.parseInt(match\![1], 10);
  expect(offset).toBeGreaterThan(20); // Should push right by more than the 20px overflow
});

Impact: Low - current tests still validate core functionality

---

2. Test Coverage Gap: Tooltip Clamping with Proof Image Loading (Low)

Location: Lines 1382-1478

The tooltip clamping logic runs in useLayoutEffect(() => { ... }, []) with an empty dependency array. If a proof image loads after the tooltip mounts, it could change the tooltip dimensions and potentially cause overflow.

Missing test scenario:

it("re-clamps when proof image loads and changes tooltip dimensions", async () => {
  const groups = [createGroup("TestSource")];
  const { getByTestId } = render(<CitationDrawerTrigger citationGroups={groups} />);
  
  const trigger = getByTestId("citation-drawer-trigger");
  hoverFirstIcon(trigger);
  
  const tooltip = trigger.querySelector("[data-testid='source-tooltip']") as HTMLElement;
  const initialTransform = tooltip.style.transform;
  
  // Simulate proof image load changing dimensions
  const proofImg = tooltip.querySelector('img[alt="Verification proof"]');
  if (proofImg) {
    fireEvent.load(proofImg);
    // Tooltip should recalculate position
    // NOTE: This might fail because useLayoutEffect has empty deps\!
  }
});

Note: This test would likely expose a bug in the current implementation - the useLayoutEffect won't re-run when proof images load because it has an empty dependency array. However, addressing this is out of scope for this PR.

---

3. Test Readability: Magic Numbers (Minor)

Location: Lines 1407-1435

The mock getBoundingClientRect uses hardcoded values without explanation:

return { left: -20, right: 160, top: 0, bottom: 50, width: 180, height: 50, x: -20, y: 0, toJSON: () => ({}) } as DOMRect;

Suggestion: Add comments explaining the overflow scenario:

// Tooltip is 180px wide, positioned -20px off left edge (20px overflow)
return { 
  left: -20,      // 20px overflow on left
  right: 160,     // -20 + 180 = 160
  width: 180,     // Tooltip width
  // ... rest
} as DOMRect;

Impact: Very low - just improves maintainability

---

💡 Suggestions

1. Consider Testing Error Boundaries (Optional Enhancement)

If isValidProofImageSrc has a bug that throws an exception (e.g., malformed regex), the component should gracefully handle it. Consider adding a test:

it("gracefully handles validation errors", () => {
  const malformedItem = createItemWithProof("data:image/png;malformed");
  
  // Should not throw
  expect(() => {
    render(<CitationDrawerItemComponent item={malformedItem} />);
  }).not.toThrow();
});

---

2. Test Group Collapse Animation Classes (Optional)

The PR description mentions "chevron rotation" but the test only checks the rotate-90 class exists/doesn't exist. Consider testing the actual rotation:

it("animates chevron rotation on collapse/expand", () => {
  // ... setup ...
  
  const chevron = alphaHeader.querySelector("svg");
  
  // Should have transition class for smooth animation
  expect(chevron).toHaveClass("transition-transform");
  expect(chevron).toHaveClass("rotate-90"); // Expanded
  
  fireEvent.click(alphaHeader);
  
  // Should remove rotation class (collapsed state)
  expect(chevron).not.toHaveClass("rotate-90");
});

---

3. Test Tooltip data-testid Attribute (Optional)

Line 1238 adds data-testid="source-tooltip" which is great for testing. Consider documenting this in a comment:

data-testid="source-tooltip" // For testing viewport clamping behavior

---

🔍 Code Quality Assessment

Strengths

• 96 total tests with all passing - excellent coverage
• Consistent test structure across all three areas
• Proper React Testing Library usage (screen, fireEvent, cleanup)
• Good helper functions that reduce duplication
• Thorough edge case coverage (empty strings, whitespace, various XSS vectors)

Areas for Improvement

• Minor: Add comments to magic numbers in mocks
• Optional: Test actual pixel offset calculations in viewport clamping
• Optional: Add proof image load dimension change test (might expose existing bug)

---

📊 Test Coverage Summary

Area	Tests Added	Coverage
XSS Prevention	13 tests	Excellent ✅
Group Collapse/Expand	7 tests	Excellent ✅
Tooltip Viewport Clamping	4 tests	Good ✅
CitationDrawerTrigger Enhancements	4 tests	Good ✅
Total	28 tests	Excellent ✅

---

✅ Recommendation

APPROVE

This PR provides excellent test coverage for critical security and UX features. The XSS prevention tests are particularly thorough and demonstrate strong security awareness. The group collapse/expand tests validate complex state management correctly. The viewport clamping tests cover the essential scenarios.

Blocking concerns: None
Optional improvements: See suggestions above (all low-priority)

This addresses the leftover PR review suggestions from #196 excellently. Great work! 🎉

---

Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2026-02-10

[claude]

Code Review: PR #200

Thank you for adding comprehensive test coverage for these critical security and UX features! Overall, this is excellent work. The tests are well-structured, thoroughly cover edge cases, and follow good testing practices.

✅ Strengths

1. Security-First Testing: The XSS prevention tests are thorough and cover all major attack vectors:

   • SVG data URIs (with base64 and inline XML)
   • javascript: protocol
   • Untrusted HTTPS/HTTP URLs
   • Empty/whitespace strings
   • Valid cases (raster data URIs and trusted hosts)

2. Test Organization: Excellent use of describe blocks to group related tests. The structure is clean and easy to navigate.

3. Helper Functions: The findGroupHeaderByCount() helper is clever - using unique citation count text to identify headers avoids DOM query fragility across different jsdom implementations.

4. CI Compatibility: The commit history shows thoughtful iteration to ensure tests work in CI environments. The final solution using citation count badges as unique identifiers is robust.

5. Coverage: 24 new tests covering three distinct areas (XSS prevention, group collapse/expand, viewport clamping) is comprehensive.

🔍 Issues & Suggestions

1. Missing Type Safety in Test Mocks (High Priority)

The viewport clamping tests mock getBoundingClientRect but have a type safety issue:

// Line ~1456
Element.prototype.getBoundingClientRect = function () {
  if (this.getAttribute?.('data-testid') === 'source-tooltip') {
    return { left: -20, right: 160, ... } as DOMRect;
  }
  return originalGetBCR.call(this);
};

Issue: The returned object doesn't implement all DOMRect properties/methods. While as DOMRect suppresses errors, this could mask issues.

Suggestion: Use a more complete mock:

return {
  left: -20,
  right: 160,
  top: 0,
  bottom: 50,
  width: 180,
  height: 50,
  x: -20,
  y: 0,
  toJSON: () => ({})
} as DOMRect;

Or better yet, create a helper:

function createMockDOMRect(overrides: Partial<DOMRect>): DOMRect {
  return {
    left: 0, right: 0, top: 0, bottom: 0,
    width: 0, height: 0, x: 0, y: 0,
    toJSON: () => ({}),
    ...overrides
  } as DOMRect;
}

2. Incomplete Viewport Clamping Assertions (Medium Priority)

The viewport overflow tests verify that transform contains translateX, but don't verify the correct adjustment was applied:

// Line ~1453 - Left overflow test
expect(tooltip.style.transform).toContain('translateX');
// Should verify the adjustment is positive (pushing right)

Suggestion: Add specific assertions:

// Left overflow: should have positive adjustment
const match = tooltip.style.transform.match(/translateX\(calc\(-50% \+ (-?\d+)px\)\)/);
expect(match).toBeTruthy();
const adjustment = Number.parseInt(match![1]);
expect(adjustment).toBeGreaterThan(0); // Pushed right

// Right overflow: should have negative adjustment
expect(adjustment).toBeLessThan(0); // Pushed left

3. Test Cleanup Issues (Medium Priority)

The viewport tests modify global state but cleanup could be more robust:

// Line ~1448
Object.defineProperty(window, 'innerWidth', { value: 400, writable: true });
// ... test runs ...
Element.prototype.getBoundingClientRect = originalGetBCR; // Restored at end

Issue: If a test throws, window.innerWidth isn't restored to original value.

Suggestion: Use try/finally or afterEach:

it('adjusts tooltip position when overflowing right edge', () => {
  const originalGetBCR = Element.prototype.getBoundingClientRect;
  const originalInnerWidth = window.innerWidth;
  
  try {
    Object.defineProperty(window, 'innerWidth', { value: 400, writable: true });
    // ... test code ...
  } finally {
    Element.prototype.getBoundingClientRect = originalGetBCR;
    Object.defineProperty(window, 'innerWidth', { value: originalInnerWidth, writable: true });
  }
});

4. Redundant Test Documentation (Low Priority)

Some tests have verbose comments that restate what the test name already says:

it('blocks SVG data URI proof images (XSS vector)', () => {
  // ... test code ...
  
  // No proof image should render  ← Redundant
  const proofImg = container.querySelector('img[alt="Verification proof"]');
  expect(proofImg).not.toBeInTheDocument();
});

The test name already communicates the intent. Comments are helpful for why something is done, not what.

Suggestion: Remove redundant comments or make them explain the "why":

// SVG data URIs can execute JavaScript via onload handlers and embedded scripts
const proofImg = container.querySelector('img[alt="Verification proof"]');
expect(proofImg).not.toBeInTheDocument();

5. Test Data Duplication (Low Priority)

Multiple tests create similar SourceCitationGroup objects with slight variations. Consider extracting to a factory:

const createTestGroup = (overrides?: Partial<SourceCitationGroup>): SourceCitationGroup => ({
  sourceName: 'TestSource',
  sourceDomain: 'testsource.com',
  citations: [{
    citationKey: 'ts-0',
    citation: { siteName: 'TestSource', title: 'Article 1' },
    verification: { status: 'found' },
  }],
  additionalCount: 0,
  ...overrides,
});

🛡️ Security

Excellent security coverage. The XSS prevention tests cover all major attack vectors:

• ✅ SVG with embedded scripts
• ✅ Protocol-based attacks (javascript:)
• ✅ Untrusted domains
• ✅ Protocol downgrade (HTTP)
• ✅ Edge cases (empty, whitespace)

One additional test to consider:

it('blocks data URI with incorrect MIME type claiming to be image', () => {
  const item = createItemWithProof('data:text/html;base64,PHNjcmlwdD5hbGVydCgneHNzJyk8L3NjcmlwdD4=');
  // Should block text/html even if it claims to be an image
});

🎯 Performance

No performance concerns. Test execution should be fast since:

• No network calls
• Minimal DOM manipulation
• Mock-based approach for layout calculations

📊 Test Coverage

Before: 72 tests
After: 96 tests (+24)

Coverage gaps filled:

• ✅ Proof image XSS prevention (CitationDrawerItemComponent)
• ✅ Proof image XSS prevention (SourceTooltip)
• ✅ Group collapse/expand state management
• ✅ Tooltip viewport clamping
• ✅ Resize listener cleanup

📝 Minor Nits

1. Line 1244: Comment formatting inconsistency

   / Data URI: allow only safe raster formats (no SVG)

   Should be // (double slash) not / (single slash)

2. Line 1029: Consider adding a test for multiple XSS vectors in sequence to ensure filtering doesn't break on edge cases.

✅ Recommendations

Must address (before merge):

• Fix type safety in DOMRect mocks (fix: correct package exports to point to lib/ instead of src #1)
• Add proper cleanup for global state modifications (Fix/npm build #3)

Should address (nice to have):

• Add specific assertions for viewport clamping adjustments (chore: update package-lock.json and ignore tsbuildinfo #2)
• Reduce test data duplication (Feature/key span #5)

Optional:

• Remove redundant comments (Patch/examples #4)
• Add additional MIME type security test

🎉 Conclusion

This PR significantly improves test coverage for critical security and UX features. The XSS prevention tests are particularly valuable. With the minor fixes above (especially #1 and #3), this will be ready to merge.

Great work on the iterative CI fixes! The final solution using citation count badges is elegant and robust.

Overall: ✅ Approve with minor changes

[claude]

Code Review - PR #200

Overall Assessment

✅ Strong PR - This adds excellent test coverage for critical security features (XSS prevention), UI state management (group collapse), and responsive behavior (tooltip viewport clamping). The tests are well-structured, thorough, and follow existing patterns in the codebase.

---

✅ Strengths

1. Security Testing Excellence

   • Comprehensive XSS attack vector coverage (SVG data URIs, javascript: protocol, untrusted URLs)
   • Tests both CitationDrawerItemComponent and SourceTooltip components for defense-in-depth
   • Good coverage of edge cases (empty strings, whitespace, HTTP vs HTTPS, inline XML)

2. Clear Test Structure

   • Well-organized describe blocks with descriptive names
   • Helper functions (findGroupHeaderByCount, hoverFirstIcon, createGroup) improve readability
   • Excellent inline comments explaining test rationale

3. DOM Query Resilience

   • The evolution through multiple commits shows thoughtful debugging for CI environment compatibility
   • Final solution using citation count text as unique identifier is clever and robust
   • tagName.toUpperCase() comparison handles jsdom case inconsistencies

4. Test Coverage Breadth

   • 24 new tests covering previously untested critical areas
   • State management tested (independent per-group collapse state)
   • Accessibility features verified (aria-expanded attributes)
   • Lifecycle testing (resize listener cleanup)

---

🔍 Minor Suggestions

1. Mock Cleanup in Viewport Tests

The viewport clamping tests modify Element.prototype.getBoundingClientRect and window.innerWidth but only restore some mocks. Consider using beforeEach/afterEach for cleaner teardown:

describe("SourceTooltip viewport clamping", () => {
  let originalGetBCR: typeof Element.prototype.getBoundingClientRect;
  let originalInnerWidth: number;
  
  beforeEach(() => {
    originalGetBCR = Element.prototype.getBoundingClientRect;
    originalInnerWidth = window.innerWidth;
  });
  
  afterEach(() => {
    Element.prototype.getBoundingClientRect = originalGetBCR;
    Object.defineProperty(window, "innerWidth", { 
      value: originalInnerWidth, 
      writable: true 
    });
    cleanup();
  });
  
  // tests...
});

Impact: Low - current approach works but this would be more defensive against test interference.

2. DOM Walking Null Safety

In findGroupHeaderByCount, the while (node) loop is safe, but TypeScript's strict null checks might benefit from explicit handling:

const findGroupHeaderByCount = (countText: string): HTMLElement | null => {
  try {
    const el = screen.getByText(countText);
    let node: HTMLElement | null = el;
    while (node) {
      if (node.tagName.toUpperCase() === "BUTTON" && node.hasAttribute("aria-expanded")) {
        return node;
      }
      node = node.parentElement;
    }
  } catch {
    // Silently fail if text not found
  }
  return null;
};

Current code is fine - this is just documenting the error handling strategy more explicitly.

3. Assertion Specificity in Viewport Tests

The viewport clamping tests verify transform contains translateX and -50%, but don't verify the offset adjustment was actually applied. Consider more specific assertions:

it("adjusts tooltip position when overflowing left edge", () => {
  // ... setup ...
  
  const tooltip = trigger.querySelector("[data-testid='source-tooltip']") as HTMLElement;
  const transform = tooltip.style.transform;
  
  // Should have centering + positive offset to push right
  expect(transform).toMatch(/translateX\(calc\(-50% \+ \d+px\)\)/);
  // Or extract and verify offset is positive
  const match = transform.match(/translateX\(calc\(-50% \+ (-?\d+)px\)\)/);
  expect(match).toBeTruthy();
  expect(Number(match\![1])).toBeGreaterThan(0);
});

Current approach is acceptable - just verifying the property exists may be sufficient given the complexity of mocking layout.

---

📋 Additional Observations

1. Test Count: Going from 72 to 96 tests is a significant improvement in coverage
2. No Breaking Changes: All new tests, no modifications to existing behavior
3. Documentation: The commit messages clearly explain the debugging journey through CI issues
4. Pattern Consistency: Uses existing test patterns (createGroup helpers, afterEach cleanup)

---

Security Analysis ✅

The XSS prevention tests verify critical attack vectors:

• SVG data URIs: Can embed <script> tags and onload handlers ✓
• javascript: protocol: Classic XSS vector ✓
• Untrusted HTTPS URLs: Defense against external image hijacking ✓
• HTTP downgrade: Prevents mixed content issues ✓

The implementation (isValidProofImageSrc) correctly validates against these threats.

---

Performance Considerations ✅

• Tests use lightweight mocks (createPortal mock renders in-place)
• No unnecessary async operations
• DOM queries are efficient (direct element selection, role queries)
• Helper functions prevent code duplication

---

Test Coverage Assessment ✅

Previously Uncovered Areas Now Tested:

• ✅ Proof image security validation (9 tests)
• ✅ Group collapse/expand state management (7 tests)
• ✅ Tooltip viewport clamping logic (4 tests)
• ✅ Additional CitationDrawerTrigger security tests (4 tests)

Remaining Gaps (not blockers for this PR):

• Keyboard navigation for group collapse/expand
• Touch/mobile-specific tooltip behavior
• Multiple simultaneous tooltip edge cases

---

Recommendation: ✅ APPROVE

This PR significantly improves test coverage for security-critical features and complex UI state management. The tests are well-written, thorough, and demonstrate careful debugging through CI environment differences. No blocking issues identified.

Suggested follow-ups (optional, not blockers):

1. Consider the mock cleanup pattern for viewport tests
2. Add keyboard navigation tests in a future PR
3. Document the findGroupHeaderByCount pattern for future test authors

[claude]

Code Review - PR #200

Thank you for addressing the leftover test coverage from PR #196. This is a solid test suite addition with excellent coverage of security, UI behavior, and edge cases. Here's my detailed review:

---

✅ Strengths

1. Security Testing (XSS Prevention)

The XSS prevention tests are excellent and critical for security:

• ✅ Comprehensive coverage of SVG data URI attacks (base64 and inline XML)
• ✅ Tests for javascript: protocol blocking
• ✅ Validates untrusted HTTPS and HTTP URL blocking
• ✅ Confirms trusted hosts and safe raster formats are allowed
• ✅ Tests both CitationDrawerItemComponent and tooltip contexts

These tests directly validate the isValidProofImageSrc function in CitationDrawer.tsx:26-44 and the duplicate validation logic in CitationDrawerTrigger.tsx:176-194.

2. Tooltip Viewport Clamping

Great testing of the useLayoutEffect-based clamping logic:

• ✅ Validates default centering with translateX(-50%)
• ✅ Tests left and right edge overflow adjustments
• ✅ Confirms resize listener cleanup on unmount
• ✅ Uses mocking appropriately for getBoundingClientRect

3. Test Quality

• ✅ Well-structured with clear describe blocks
• ✅ Good use of helper functions (createItemWithProof, createGroup, hoverFirstIcon)
• ✅ Descriptive test names that explain intent
• ✅ Proper cleanup with afterEach

---

⚠️ Critical Issue: Group Collapse Tests Don't Match Implementation

The CitationDrawer group collapse/expand state test suite (lines 1196-1353) appears to test functionality that doesn't exist:

Problem:

• Tests expect collapsible groups with aria-expanded, clickable headers, and chevron rotation
• The actual SourceGroupHeader component (CitationDrawer.tsx:50-94) has:
  • Comment: "Always expanded (no collapse toggle)" (line 48)
  • No button wrapper
  • No click handler
  • No aria-expanded attribute
  • No chevron icon
  • role="heading" not button

Evidence from implementation:

// CitationDrawer.tsx:59-92
return (
  <div
    className="w-full px-4 py-2.5..."
    role="heading"  // Not a button!
    aria-level={3}
  >
    {/* No chevron, no click handler */}
  </div>
);

Tests that will fail:

• findGroupHeaderByCount looks for BUTTON with aria-expanded (line 1226) - won't find it
• Lines 1257, 1275, 1296, 1314, 1327 - click events on non-existent buttons
• Line 1331 - expects chevron SVG that doesn't exist

Resolution needed:

1. If collapse functionality is planned but not yet implemented, these tests should be in a separate PR with the implementation
2. If I'm misunderstanding the component structure, please point me to where the collapse logic exists
3. The tests reference findGroupHeaderByCount which walks up to find a button - but the header is not a button

---

🔍 Additional Observations

Minor: Code Duplication

The proof image validation logic is duplicated between:

• CitationDrawer.tsx:26-44 (isValidProofImageSrc function)
• CitationDrawerTrigger.tsx:176-194 (inline IIFE in CitationTooltip)

Suggestion: Extract the validation to a shared utility function to maintain DRY and ensure consistent security checks.

Test Organization

The 4 new tests added to the existing CitationDrawerTrigger block (lines 984-1095) fit well, but consider grouping all XSS-related tests together for easier navigation.

---

📊 Test Coverage Summary

Working tests (20/28):

• ✅ 4 tooltip XSS tests (lines 984-1095)
• ✅ 9 CitationDrawerItemComponent security tests (lines 1117-1194)
• ✅ 4 tooltip viewport clamping tests (lines 1384-1477)

Questionable tests (7/28):

• ❓ Group collapse tests (lines 1235-1352) - functionality appears not to exist

---

🎯 Recommendations

1. Verify group collapse implementation - Either remove these tests or show me where the collapse feature is implemented
2. Consider extracting validation logic - DRY up the proof image security checks
3. Run the test suite - Confirm all 96 tests actually pass (the group collapse tests should fail based on my reading)

---

✨ Verdict

The XSS prevention and viewport clamping tests are production-ready and valuable. However, the group collapse test suite needs clarification before this PR can be merged. Please verify whether those tests pass, and if so, help me understand where the collapse functionality is implemented.

Great work on the security testing - those XSS vectors are exactly what need to be blocked! 🔒

[claude]

PR Review: Add CitationDrawer Tests for XSS Prevention, Group Collapse, and Tooltip Clamping

Thanks for adding these comprehensive tests! Overall, this is a solid test suite addition with good coverage of security concerns and UI behavior. Here's my detailed feedback:

✅ Strengths

1. Excellent XSS Security Coverage - The tests thoroughly verify that malicious proof images are blocked:

   • SVG data URIs (base64 and inline)
   • javascript: protocol
   • Untrusted HTTPS URLs
   • HTTP URLs
   • Empty/whitespace strings
   • Tests cover both CitationDrawerItemComponent and CitationTooltip (via CitationDrawerTrigger)

2. Good Test Organization - Well-structured with clear describe blocks and descriptive test names

3. Proper Cleanup - Uses afterEach(cleanup) consistently

4. Helpful Comments - Test descriptions clearly explain what's being verified

⚠️ Issues Found

Critical: Tests Don't Match Implementation

The "CitationDrawer group collapse/expand state" test suite (lines 1198-1336) tests functionality that does not exist in the current implementation:

Evidence from CitationDrawer.tsx:48-49:

/**
 * Source group header displayed in the drawer.
 * Shows favicon, source name, and citation count. Always expanded (no collapse toggle).
 */

Evidence from CitationDrawer.tsx:260-261:

/**
 * CitationDrawer displays a collection of citations in a drawer/bottom sheet.
 * Citations are grouped by source with always-expanded sections. No collapse/expand toggle —

The Problem:

• Tests like "collapses a group when its header is clicked" expect clicking on "2 citations" to collapse/expand groups
• The current SourceGroupHeader component has no click handler, no state management, and no collapse functionality
• These tests will fail because the expected behavior doesn't exist

Recommendation:

• Either remove these tests if collapse functionality isn't planned
• Or implement the collapse functionality first before adding tests
• Check if these tests were written for a different branch/version

Minor: Potential Test Flakiness

Line 1440-1460 - Resize listener cleanup test:

it("cleans up resize listener on tooltip unmount", () => {
  const addSpy = jest.spyOn(window, "addEventListener");
  const removeSpy = jest.spyOn(window, "removeEventListener");

Concern: This test might be flaky because:

• It doesn't wait for the tooltip unmount delay (TOOLTIP_HIDE_DELAY_MS = 80ms)
• The tooltip uses a delayed hide mechanism (line 431 in CitationDrawerTrigger.tsx)
• fireEvent.mouseLeave(trigger) triggers the delay, but the test doesn't wait for it

Fix: Add a small wait or use act() with a timer:

// Mouse leave to unmount tooltip
fireEvent.mouseLeave(trigger);

// Wait for tooltip hide delay
await act(async () => {
  await new Promise(resolve => setTimeout(resolve, 100));
});

// Should clean up resize listener
expect(removeSpy).toHaveBeenCalledWith("resize", expect.any(Function));

📝 Code Quality Observations

1. Good DRY with Helper Functions - The hoverFirstIcon helper (lines 958, 1358) reduces duplication

2. Consistent Test Data - Using createItemWithProof and createGroup factory functions is excellent

3. Test Coverage - The XSS tests are comprehensive and cover edge cases well

🔍 Security Review

The XSS prevention tests align perfectly with the security implementation in CitationDrawer.tsx:16-44 and CitationDrawerTrigger.tsx:176-194:

• ✅ Blocks SVG data URIs (can contain <script> tags)
• ✅ Allows only safe raster formats (PNG, JPEG, WebP, AVIF, GIF)
• ✅ Validates HTTPS URLs against trusted hosts
• ✅ Rejects HTTP, javascript:, and other protocols

This is excellent defensive coding!

📊 Test Metrics

• Total new tests: 28 (24 in PR description, but I count 28 in the diff)
• Security tests: 13 (XSS prevention)
• Group collapse tests: 7 (non-functional - see issue above)
• Tooltip clamping tests: 4
• Additional trigger tests: 4

✅ Recommendations

Before merging:

1. MUST FIX: Remove or update the "CitationDrawer group collapse/expand state" tests (lines 1198-1336) as they test non-existent functionality

2. SHOULD FIX: Add delay handling to the resize listener cleanup test to prevent flakiness

3. NICE TO HAVE: Consider adding a test that verifies the actual proof image src attribute matches the validated input for the positive cases

Example:

it("allows valid raster data URI proof images in tooltip", () => {
  // ... existing code ...
  const proofImg = trigger.querySelector('img[alt="Verification proof"]');
  expect(proofImg).toHaveAttribute('src', 'data:image/png;base64,iVBORw0KGgo=');
});

🎯 Verdict

Do Not Merge until the group collapse tests issue is resolved. The XSS and tooltip clamping tests are excellent and ready to go, but ~25% of the new tests will fail because they test functionality that doesn't exist in the codebase.

Let me know if you have questions or need clarification on any of these points!