Skip to content

Conversation

@bensonwong
Copy link
Contributor

@bensonwong bensonwong commented Feb 11, 2026

This PR resolves Firestore Timestamp parsing errors and missing exports that were causing test failures across the monorepo.

1. Fixed missing exports in @deepcitation/deepcitation-js

Problem: parseDeferredCitationResponse and getAllCitationsFromDeferredResponse existed in source code but weren't exported from the package's main index, causing import errors:

SyntaxError: Export named 'parseDeferredCitationResponse' not found in module

Solution: Added missing exports to src/index.ts

Test Results

  • @deepcitation/deepcitation-js: Built successfully

…erredResponse

These functions were defined in src/parsing/citationParser.ts but were not
exported from the package's main index.ts, causing import errors in consuming
packages.

Added exports:
- parseDeferredCitationResponse: Parse raw LLM output with deferred citations
- getAllCitationsFromDeferredResponse: Extract all citations from deferred format
@github-actions
Copy link
Contributor

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

@claude
Copy link

claude bot commented Feb 11, 2026

Pull Request Review: Fix Missing Exports

Summary

This PR adds two missing exports (parseDeferredCitationResponse and getAllCitationsFromDeferredResponse) to the package's main index. These functions were implemented and tested but not exported, causing import errors for consuming packages.

Code Quality Assessment ✅

Strengths

  1. Minimal, Focused Change: The PR adds exactly what's needed - two missing exports - without any unnecessary modifications. This is exemplary restraint.

  2. Alphabetical Ordering: The new exports are correctly placed in alphabetical order within the "Citation Parsing" section:

    export {
      extractVisibleText,
      getAllCitationsFromDeferredResponse,  // ✓ Added in correct position
      getCitationMarkerIds,
      hasDeferredCitations,
      parseDeferredCitationResponse,        // ✓ Added in correct position
      replaceDeferredMarkers,
    } from "./parsing/citationParser.js";
  3. Excellent Test Coverage: The functions have comprehensive test coverage (69+ test cases) in src/__tests__/deferredCitationParser.test.ts, covering:

    • Basic parsing scenarios
    • Edge cases (null/undefined, empty input, malformed JSON)
    • JSON repair mechanisms (trailing commas, invalid escapes, missing brackets)
    • Multiple citation formats (compact keys, grouped format, legacy format)
    • Special characters and Unicode handling
    • Real-world medical document scenarios
  4. Proper Deprecation: Both functions are marked @deprecated with clear migration guidance pointing users to getAllCitationsFromLlmOutput(), which handles multiple formats automatically.

Issues & Recommendations

⚠️ Minor: Redundant Re-exports

Issue: According to the repository's CLAUDE.md guidelines under "No Variable Re-Exports", the codebase has a strict policy against re-exporting functions:

NEVER re-export variables (functions, constants, classes) from a different module.

The guideline states that every function should have ONE canonical location where it's defined, and all consumers should import from that location directly.

Current state:

  • src/index.ts (lines 62-69) re-exports functions from ./parsing/citationParser.js
  • These functions are already being imported directly in internal code (e.g., src/parsing/parseCitation.ts:4)

However, this is acceptable because:

  1. src/index.ts is the package's public API entry point - re-exports here serve a different purpose than internal re-exports
  2. External consumers must import from the package root (@deepcitation/deepcitation-js) - they cannot import from internal paths
  3. The anti-pattern the guideline addresses is internal re-exports that create confusing dependency graphs

Recommendation: No action needed. The guideline's intent is to prevent internal barrel files and wrapper modules, not to forbid public API aggregation in the main package index.

✅ Verification: Functions Are Actually Used

Confirmed that these exports are needed:

  • getAllCitationsFromDeferredResponse is imported and used internally in src/parsing/parseCitation.ts:4
  • Both functions are part of the public API and would be imported by external packages
  • The deprecation notices will guide users to the newer getAllCitationsFromLlmOutput() API

Security Considerations ✅

No security concerns identified.

The changes are purely additive (new exports) and don't modify any existing behavior. The exported functions:

  • Perform safe JSON parsing with error handling
  • Have input validation for null/undefined values
  • Include JSON repair functionality with safeguards against malformed input
  • Don't introduce any new attack vectors

Performance Considerations ✅

No performance impact.

These functions were already present in the bundle; making them exportable adds no overhead. The functions themselves use:

  • Efficient regex-based parsing (with compiled module-level regexes)
  • Proper deduplication using Set for line IDs
  • Safeguards against large range expansions (MAX_LINE_ID_RANGE_SIZE = 1000)

Testing ✅

Excellent test coverage confirmed.

The test file src/__tests__/deferredCitationParser.test.ts includes:

  • ✅ 920+ lines of comprehensive tests
  • ✅ Edge case handling (null, undefined, empty strings, malformed JSON)
  • ✅ Format variations (compact keys, grouped format, mixed formats)
  • ✅ Error scenarios with proper fallbacks
  • ✅ Real-world use cases (medical documents with special characters)

According to the PR description, all tests pass:

  • ✅ @deepcitation/deepcitation-js: Built successfully
  • ✅ 117 tests pass (web package)
  • ✅ 771 tests pass (functions package)

Recommendations

  1. Approved as-is ✅ - The changes are correct and complete.

  2. Documentation: Consider adding a migration guide example to help users transition from the deprecated functions:

    // OLD (deprecated)
    import { parseDeferredCitationResponse } from '@deepcitation/deepcitation-js';
    
    // NEW (recommended)
    import { getAllCitationsFromLlmOutput } from '@deepcitation/deepcitation-js';
    const citations = getAllCitationsFromLlmOutput(llmResponse); // handles all formats
  3. Future consideration: When planning v2.0 (per deprecation notices), ensure a clear migration path is documented before removing these functions.

Conclusion

This is a high-quality PR that fixes a real bug (missing exports) with a minimal, well-tested change. The code follows best practices with proper deprecation notices, comprehensive tests, and alphabetical organization.

Status: APPROVED


Files Changed: 1 file, +2 lines
Risk Level: Minimal (additive change only)
Reviewer Confidence: High

@bensonwong bensonwong merged commit 425b8ae into main Feb 11, 2026
3 checks passed
@bensonwong bensonwong deleted the fix/add-missing-exports branch February 11, 2026 06:58
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