Enable relationships for span-based (text) annotations

[JSv4]

Summary

This PR fixes three critical issues preventing relationships from working with span-based annotations in text documents, enabling the full relationship creation and display workflow for text files.

Key Changes

1. Centralized File Type Detection

• Created isTextFileType() and isPdfFileType() utility functions in frontend/src/utils/files.ts
• Replaced scattered startsWith("text/") checks and hardcoded MIME type comparisons across 11 components
• Fixes inconsistency where application/txt MIME type was missed by some checks

2. Fixed Label Initialization Race Condition

• File: UISettingsAtom.tsx
• Replaced single initialized.current ref with separate spanLabelInitialized and relationLabelInitialized refs
• Prevents early cutoff where span label initialization could block relationship label auto-initialization on subsequent effect runs

3. Expanded Type Support for Relationship UI

• Files: RelationItem.tsx, RelationHighlightItem.tsx, HighlightItem.tsx, utils.ts
• Updated type signatures to accept ServerTokenAnnotation | ServerSpanAnnotation union types
• Updated annotationSelectedViaRelationship() function to handle both annotation types
• Enables sidebar relationship display and creation flow for text documents

Files Modified

• frontend/src/utils/files.ts (new utilities)
• frontend/src/components/annotator/context/UISettingsAtom.tsx
• frontend/src/components/annotator/sidebar/RelationItem.tsx
• frontend/src/components/annotator/sidebar/RelationHighlightItem.tsx
• frontend/src/components/annotator/sidebar/HighlightItem.tsx
• frontend/src/components/annotator/utils.ts
• frontend/src/components/annotator/hooks/AnnotationHooks.tsx
• frontend/src/components/annotator/labels/EnhancedLabelSelector.tsx
• frontend/src/components/annotator/labels/UnifiedLabelSelector.tsx
• frontend/src/components/annotator/labels/label_selector/LabelSelector.tsx
• frontend/src/components/knowledge_base/document/DocumentKnowledgeBase.tsx
• frontend/src/components/widgets/chat/ChatMessage.tsx

Testing

Verify that:

• Relationships can be created between span annotations in text documents
• Relationship labels auto-initialize correctly
• Both text/plain and application/txt MIME types are handled consistently

https://claude.ai/code/session_01Q8XMB6GZZnDWaQP4prr6v2

[claude]

Code Review - PR #866: Enable relationships for span-based (text) annotations

Summary

This PR successfully addresses issue #281 by enabling relationship functionality for text documents. The changes are well-structured, focused, and follow good software engineering practices. The core approach of centralizing file type detection and separating initialization logic is solid.

✅ Strengths

1. Excellent Centralization: Creating isTextFileType() and isPdfFileType() utilities eliminates scattered string comparisons across 11 components. This follows DRY principles perfectly and catches the application/txt edge case that was previously missed.

2. Race Condition Fix: The refactoring from a single initialized.current ref to separate spanLabelInitialized and relationLabelInitialized refs is well-reasoned. The inline comment clearly explains why this matters.

3. Type Safety: Expanding type signatures to ServerTokenAnnotation | ServerSpanAnnotation union types properly reflects the actual data flow and enables TypeScript to catch potential issues.

4. Comprehensive Coverage: The PR touches all relevant components (11 components updated) to ensure consistent behavior throughout the codebase.

5. Good Documentation: CHANGELOG.md follows the project's Keep a Changelog format with specific file locations and clear descriptions.

🔍 Code Quality Observations

files.ts (frontend/src/utils/files.ts:8-15)

export const isTextFileType = (fileType: string | null | undefined): boolean =>
  fileType?.startsWith("text/") === true || fileType === "application/txt";

export const isPdfFileType = (fileType: string | null | undefined): boolean =>
  fileType === "application/pdf";

✅ Good: Handles null/undefined gracefully with optional chaining
✅ Good: Explicit === true comparison prevents truthy coercion bugs
✅ Good: Clear JSDoc comments explain the legacy application/txt handling

UISettingsAtom.tsx (lines 289-315)

const spanLabelInitialized = useRef(false);
const relationLabelInitialized = useRef(false);

useEffect(() => {
  if (\!selectedDocument) return;
  
  const isTextFile = isTextFileType(selectedDocument.fileType);
  const isPdfFile = isPdfFileType(selectedDocument.fileType);
  
  if (\!spanLabelInitialized.current) {
    // initialization logic
  }
  
  if (\!relationLabelInitialized.current) {
    // initialization logic
  }
}, [/* deps */]);

✅ Good: Early return pattern for guard clause
✅ Good: Separate initialization tracking prevents race condition
✅ Good: Comment explains the rationale for the change
⚠️ Minor: The effect dependencies include activeSpanLabel and activeRelationLabel, which means the effect runs whenever these change. Since the refs prevent re-initialization, this is safe, but could potentially run more than necessary.

Type Union Updates (RelationItem.tsx, HighlightItem.tsx, etc.)

target_annotations: (ServerTokenAnnotation | ServerSpanAnnotation)[];
source_annotations: (ServerTokenAnnotation | ServerSpanAnnotation)[];

✅ Good: Union types are the correct approach here
✅ Good: Both classes share common properties (id, annotationLabel, etc.) so they can be used interchangeably in display logic

🧪 Testing Considerations

Missing: Unit tests for the new utility functions in files.ts

I recommend adding a test file frontend/src/utils/__tests__/files.test.ts:

import { describe, it, expect } from 'vitest';
import { isTextFileType, isPdfFileType } from '../files';

describe('File type utilities', () => {
  describe('isTextFileType', () => {
    it('should return true for text/plain', () => {
      expect(isTextFileType('text/plain')).toBe(true);
    });
    
    it('should return true for text/* variants', () => {
      expect(isTextFileType('text/html')).toBe(true);
      expect(isTextFileType('text/csv')).toBe(true);
    });
    
    it('should return true for application/txt (legacy)', () => {
      expect(isTextFileType('application/txt')).toBe(true);
    });
    
    it('should return false for PDF', () => {
      expect(isTextFileType('application/pdf')).toBe(false);
    });
    
    it('should handle null/undefined gracefully', () => {
      expect(isTextFileType(null)).toBe(false);
      expect(isTextFileType(undefined)).toBe(false);
    });
  });
  
  describe('isPdfFileType', () => {
    it('should return true for application/pdf', () => {
      expect(isPdfFileType('application/pdf')).toBe(true);
    });
    
    it('should return false for text types', () => {
      expect(isPdfFileType('text/plain')).toBe(false);
    });
    
    it('should handle null/undefined gracefully', () => {
      expect(isPdfFileType(null)).toBe(false);
      expect(isPdfFileType(undefined)).toBe(false);
    });
  });
});

Manual Testing Needed (per the PR description):

• ✅ Relationships can be created between span annotations in text documents
• ✅ Relationship labels auto-initialize correctly
• ✅ Both text/plain and application/txt MIME types are handled consistently

🔒 Security Considerations

✅ No concerns: This PR is purely frontend type-safety and UI logic changes. No data validation, permissions, or backend changes that could introduce security issues.

⚡ Performance Considerations

✅ No concerns: The utility functions are simple boolean checks with no performance impact. The initialization logic refactoring doesn't change the computational complexity.

📚 Adherence to Project Guidelines

✅ CLAUDE.md compliance:

• ✅ Follows DRY principle by centralizing file type detection
• ✅ Utilities placed in correct location (frontend/src/utils/files.ts)
• ✅ CHANGELOG.md properly updated with file locations and line numbers
• ✅ Clear commit message with issue reference

🎯 Recommendations

1. Add unit tests for isTextFileType and isPdfFileType (see test code above)
2. Consider extracting a constant for the legacy MIME type:

   const LEGACY_TEXT_MIME_TYPE = 'application/txt';
   
   export const isTextFileType = (fileType: string | null | undefined): boolean =>
     fileType?.startsWith('text/') === true || fileType === LEGACY_TEXT_MIME_TYPE;

3. Manual testing: Verify the three scenarios in the PR description before merging

✅ Verdict

APPROVE with minor suggestions - This PR is well-implemented and ready to merge after adding the recommended unit tests. The changes are focused, solve the stated problem, and follow project conventions.

The code quality is high, the approach is sound, and the risk is low. The centralization of file type detection will prevent future bugs and make the codebase more maintainable.

---

Review generated for commit: Closes #281

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review - PR #866: Enable relationships for span-based (text) annotations

Summary

This PR successfully addresses issue #281 by enabling relationship creation and display for text documents (span-based annotations). The changes are well-structured, comprehensive, and include proper test coverage.

---

✅ Strengths

1. Excellent Centralization Pattern

The creation of isTextFileType() and isPdfFileType() utilities in frontend/src/utils/files.ts is exactly the right approach:

• Consolidates 12+ scattered file type checks into a single source of truth
• Handles the edge case of application/txt (legacy MIME type) consistently
• Proper null/undefined handling
• Well-documented with clear JSDoc comments
• Extracted LEGACY_TEXT_MIME_TYPE constant to constants.ts (following project conventions)

2. Critical Bug Fix - Race Condition

The label initialization fix in UISettingsAtom.tsx (lines 289-325) is well-reasoned:

• Splitting initialized.current into separate spanLabelInitialized and relationLabelInitialized refs prevents early cutoff
• Excellent inline documentation explaining why deps exclude activeSpanLabel/activeRelationLabel
• The eslint-disable-next-line is justified and documented

3. Type System Enhancement

Widening types to ServerTokenAnnotation | ServerSpanAnnotation union:

• Enables code reuse between PDF and text document workflows
• Both classes share compatible interfaces (same properties: id, page, annotationLabel, rawText, etc.)
• Changes are minimal and focused - just updating type signatures, no logic changes needed

4. Comprehensive Test Coverage

The new test file frontend/src/utils/__tests__/files.test.ts covers:

• All positive cases (text/plain, text/*, application/txt, application/pdf)
• Edge cases (null, undefined, empty string)
• Negative cases (wrong MIME types)
• 11 test cases total - thorough coverage

5. Excellent Changelog Documentation

The CHANGELOG.md entry follows project conventions perfectly:

• Clear category (Fixed)
• Detailed description of each issue
• File locations and line numbers included
• Links to all affected files

---

🔍 Code Quality Observations

Minor Considerations (Not Blockers)

1. Type Guard Enhancement (Optional Improvement):
   The utility functions could be enhanced with TypeScript type guards for better type narrowing:

   export const isTextFileType = (fileType: string | null | undefined): fileType is string =>
     fileType?.startsWith("text/") === true || fileType === LEGACY_TEXT_MIME_TYPE;

   However, the current implementation is perfectly fine - this is just a potential enhancement.

2. Component Testing (Future Work):
   While unit tests for the utilities are excellent, component tests for the relationship creation workflow in text documents would be valuable. However, given the project's testing guidelines (use --reporter=list, proper test wrappers required), this is better as follow-up work rather than blocking this PR.

3. UseEffect Dependencies (Already Addressed):
   The intentional exclusion of activeSpanLabel/activeRelationLabel from deps is correct and well-documented. The refs prevent infinite loops while still allowing re-initialization when data sources change.

---

🔒 Security & Performance

• ✅ No security concerns - changes are entirely type-safe
• ✅ No performance impact - utility functions are O(1)
• ✅ No XSS risks - no user content rendering changes
• ✅ No permission changes - respects existing permission model

---

📋 Adherence to Project Conventions

Per CLAUDE.md:

• ✅ No dead code introduced
• ✅ DRY principle followed (centralized utilities)
• ✅ Single responsibility maintained
• ✅ Constants properly extracted to constants.ts
• ✅ Changelog properly updated
• ✅ No credit to Claude in commit messages
• ✅ Utility functions placed in correct location (frontend/src/utils/)

---

🧪 Testing Recommendations

Before merging, please verify:

1. Frontend tests pass:

   cd frontend
   yarn test:unit --run

2. TypeScript compilation succeeds:

   cd frontend
   npx tsc --noEmit

3. Manual testing (as described in PR description):

   • Open a text document (text/plain or application/txt)
   • Create span annotations
   • Verify relationship labels auto-initialize
   • Create relationships between span annotations
   • Verify relationships display correctly in sidebar

---

🎯 Final Verdict

LGTM - Approved ✅

This is high-quality work that:

• Solves a real user-facing issue ([FEATURE] - Enable Relationships for Span-Based Annotations #281)
• Follows established project patterns
• Includes proper test coverage
• Has clear documentation
• Makes no breaking changes
• Improves code maintainability through centralization

The PR is ready to merge once tests confirm everything passes.

---

Acknowledgment

Great job on the centralization pattern and the thorough handling of the race condition. The separate initialization refs are a clean solution to a subtle timing issue.