Skip to content

Conversation

@bensonwong
Copy link
Contributor

@bensonwong bensonwong commented Feb 10, 2026

Summary

  • Add CitationPage import to drawer types
  • Extend CitationDrawerItem interface with optional page field to support rendering
    verified citation pages in the drawer
  • Enable drawer to display visual proof/verification pages alongside citation
    metadata

Changes

CitationDrawer Types

  • Import CitationPage type from src/types/boxes.js
  • Add optional page?: CitationPage | null field to CitationDrawerItem interface
  • Include JSDoc comment documenting the purpose of the new field

Migration

Fully backwards compatible - the page field is optional and will not break existing
implementations.

@bensonwong bensonwong changed the title Adding CitationPage to drawer type chore: Add CitationPage to CitationDrawerItem type Feb 10, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

✅ 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: 21858680127

@claude
Copy link

claude bot commented Feb 10, 2026

Code Review: Adding CitationPage to drawer type

Summary

This PR adds an optional page field to the CitationDrawerItem interface, allowing citation drawer items to include page render data for verified pages.

✅ Positives

  1. Clean Type Addition: The change is minimal and focused - adding a single optional field with proper TypeScript typing.

  2. Proper Import: Correctly imports CitationPage from the canonical location (../types/boxes.js), following the project's import conventions.

  3. Good Documentation: The JSDoc comment clearly explains the field's purpose: "Optional page render for the citation's verified page".

  4. Backward Compatible: Using page?: CitationPage | null makes this field optional, so existing code won't break.

  5. Consistent Pattern: This matches the existing pattern in CitationComponent.tsx:848, which already uses the same type signature.

🔍 Observations

  1. No Breaking Changes: Since this is an optional field, no existing consumers of CitationDrawerItem need to be updated.

  2. Type Safety: The CitationPage | null union type properly handles cases where page data might not be available.

  3. Follows CLAUDE.md Guidelines:

    • Imports from canonical location (no re-exports)
    • Uses proper TypeScript types
    • Clear documentation

📋 Suggestions

  1. Consider Adding Tests: While the type change is simple, consider adding a test case that verifies the drawer handles items with and without the page field. Example:

    it('handles citations with page data', () => {
      const itemWithPage: CitationDrawerItem = {
        citationKey: 'test',
        citation: mockCitation,
        verification: mockVerification,
        page: { pageNumber: 1, dimensions: { width: 800, height: 600 }, source: 'https://...' }
      };
      // Test rendering...
    });
  2. Future Enhancement: Consider documenting in CLAUDE.md how developers should populate the page field when using the drawer, especially since it's used with onViewPageClick callback in CitationComponent.

✅ Approval

This is a clean, well-typed change that extends the drawer functionality without breaking existing code. The PR is ready to merge.

Status: ✅ APPROVED


Review performed by Claude Code

@claude
Copy link

claude bot commented Feb 10, 2026

Code Review - PR #204: Add CitationPage to CitationDrawerItem type

Summary

This PR adds an optional page field to the CitationDrawerItem interface to support displaying verification page images in the drawer. The change is minimal, well-documented, and fully backwards compatible.

✅ Strengths

  1. Backwards Compatibility: The page field is optional (page?: CitationPage | null), ensuring existing implementations continue to work without modification.

  2. Consistency: This follows the same pattern already established in CitationComponent.tsx:848, which has an identical page?: CitationPage | null field. Good consistency across the codebase.

  3. Type Safety: Properly imports CitationPage from the canonical location (src/types/boxes.js) and maintains strong typing.

  4. Clear Documentation: The JSDoc comment clearly explains the purpose: "Optional page render for the citation's verified page".

  5. Proper Type Union: Using CitationPage | null allows consumers to explicitly signal "no page data" vs undefined "not checked yet" state.

📝 Minor Observations

  1. Test Coverage: The existing test helper createItem() in src/__tests__/CitationDrawer.test.tsx:249 doesn't include the new page field, but since it's optional and uses Partial<CitationDrawerItem>, tests will continue to pass. Consider adding a test case that verifies the drawer handles the page field correctly when provided (though this might be out of scope for this PR).

  2. Implementation Status: The page field is currently not being read/used in CitationDrawer.tsx (verified via grep). This suggests follow-up work will implement the actual rendering logic. This is perfectly acceptable for a preparatory type change.

  3. Example Code: The example in examples/nextjs-ai-sdk/src/utils/citationDrawerAdapter.ts doesn't populate the page field yet, but since it's optional, this is fine. Future updates can add page data as needed.

🎯 Code Quality

  • Style: Follows existing conventions perfectly
  • Documentation: Clear and concise
  • Type Safety: Excellent - no any types, proper nullability
  • Dependencies: Correct import path, no circular dependencies
  • Security: No security concerns
  • Performance: No performance impact (just a type definition)

✅ Verdict

Approved - This is a clean, well-thought-out change that lays the groundwork for future functionality. The change is:

  • Fully backwards compatible
  • Type-safe
  • Consistent with existing patterns
  • Well-documented

No changes requested. Ready to merge! 🚀

@bensonwong bensonwong merged commit 7ee5059 into main Feb 10, 2026
3 checks passed
@bensonwong bensonwong deleted the chore/add-citationpage-to-drawer-types branch February 10, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant