feat: Add relaxed interaction mode and fix citation indicators#145
feat: Add relaxed interaction mode and fix citation indicators#145bensonwong merged 10 commits intomainfrom
Conversation
## Summary of Changes ### 1. Fixed X icon for URL not found indicator (UrlCitationComponent.tsx) - Changed import from `CloseIcon` to `XCircleIcon` for error states - Updated `renderStatusIndicator()` to use `XCircleIcon` with centered alignment (not subscript) for error states - Changed `STATUS_ICONS` to use `⊗` symbol instead of "404" text for all error states ### 2. Fixed X icon for not found indicator (CitationComponent.tsx) - Updated `MissIndicator` component to use `XCircleIcon` instead of a text character `✕` - Changed positioning from subscript (`top-[0.15em]`) to centered (`items-center`) ### 3. Added `interactionMode` prop to CitationComponent - Added new type `CitationInteractionMode` = `"eager" | "relaxed"` in types.ts with documentation - Added `interactionMode` prop to `CitationComponentProps` with documentation - Updated `handleMouseEnter` to not show popover on hover when in `relaxed` mode - Updated `handleClick` to open popover on first click in `relaxed` mode (instead of zooming image) ### 4. Updated mouse pointer for relaxed mode - In `eager` mode with image: `cursor-zoom-in` (click zooms) - In `relaxed` mode without popover open: `cursor-pointer` (click opens popover) - In `relaxed` mode with popover open + image: `cursor-zoom-in` (click zooms) ### 5. Exported new types - Exported `CitationInteractionMode` from types.ts, CitationComponent.tsx, and react/index.ts - Exported `XCircleIcon` from react/index.ts ### Interaction Behavior Summary: | Mode | Hover | Click (no popover) | Click (popover open) | |------|-------|-------------------|---------------------| | `eager` (default) | Shows popover | Zooms image | Zooms image | | `relaxed` | Style effects only | Opens popover | Zooms image |
## Summary of All Changes ### 1. Fixed X icon for "not found" indicator - **CitationComponent.tsx**: Updated `MissIndicator` to use `XCircleIcon` (circle with X) instead of `✕` text character, and centered it instead of subscript positioning - **UrlCitationComponent.tsx**: Updated error indicator to use `XCircleIcon` with centered alignment - **UrlCitationComponent.tsx**: Changed `STATUS_ICONS` to use `⊗` symbol for all error states instead of "404" ### 2. Added `interactionMode` prop to CitationComponent - Added new type `CitationInteractionMode` = `"eager" | "relaxed"` - **`eager` (default)**: Hover shows popover, click opens image/expands details - **`relaxed`**: Hover only applies style effects (no popover), click opens popover first, then second click zooms image - Updated cursor to show `cursor-pointer` in relaxed mode when popover isn't open yet ### 3. Fixed UrlCitationComponent click behavior - **Before**: Clicking didn't open the URL unless `openUrlOnClick` was true - **After**: Clicking always opens the URL in a new tab (hover reveals external link icon as visual hint) - Deprecated `openUrlOnClick` prop since clicking now always opens the URL - Updated `showExternalLinkOnHover` to default to `true`
…the miss/partial-without-image state will always show the anchor text next to the icon, just like the URL citations do. The change removes the conditional that was hiding the anchor text when a humanizing message existed. Now: - The header shows: `[X icon] "anchor text"` - Below it, the humanizing message provides additional context: "We couldn't find 'anchor text' in this document." This provides a consistent experience between URL and document citations.
✅ Playwright Test ReportStatus: Tests passed 📊 Download Report & Snapshots (see Artifacts section) What's in the Visual SnapshotsThe gallery includes visual snapshots for:
Run ID: 21583341862 |
Code Review - PR #145Thank you for this PR! The implementation looks solid overall. Here are my findings: ✅ Positive Aspects
🐛 Potential Issues1. Deprecated prop not handled (UrlCitationComponent.tsx:217)The
// Current (line 208):
openUrlOnClick: _openUrlOnClick, // Deprecated, clicking always opens URL now
// Suggestion: Add deprecation warning
const openUrlOnClick = _openUrlOnClick;
if (openUrlOnClick !== undefined && process.env.NODE_ENV !== 'production') {
console.warn('openUrlOnClick is deprecated. URL citations now always open on click.');
}2. MissIndicator centering inconsistencyThe PR description mentions centering the not-found indicator, and the code uses This creates visual inconsistency where the red X appears centered while green checks appear as subscripts. Consider:
Current: // Line 733 - MissIndicator: centered
className="inline-flex items-center ml-0.5 size-2.5..."
// Line 706 - PartialIndicator: subscript
className="inline-flex relative ml-0.5 top-[0.15em] size-2.5..."3. Incomplete humanizing message logic change (CitationComponent.tsx:1407)The comment was updated to say "Humanizing message provides additional context below the header" and the Question: Does this cause the anchor text to appear twice when a humanizing message exists? Please verify this doesn't create duplicate text displays. Before: anchorText={humanizingMessage ? undefined : anchorText}
// Comment: "When humanizingMessage exists, skip anchorText in header to avoid redundancy"After: anchorText={anchorText}
// Comment: "Humanizing message provides additional context below the header"🎯 Best PracticesType SafetyGood use of the discriminated union type for AccessibilityThe 🧪 Testing ConcernsMissing test coverage: No test files found for React components. Consider adding tests for:
📊 PerformanceNo concerns - the new logic uses existing hooks properly and doesn't introduce unnecessary re-renders. 🔒 SecurityNo concerns identified. 📝 DocumentationThe CLAUDE.md should be updated to document the new Suggested addition: #### Interaction Modes
Control how eagerly citations respond to interactions:
```tsx
// Eager (default): hover shows popover, click opens image
<CitationComponent citation={citation} verification={verification} />
// Relaxed: hover only styles, click shows popover
<CitationComponent
citation={citation}
verification={verification}
interactionMode="relaxed"
/>Use |
The type was being exported twice from react/index.ts: - Once from types.js (line 25) - Once from CitationComponent.js (line 89) This caused a TypeScript build error. Keeping only the export from types.js. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code ReviewSummaryThis PR introduces a new ✅ Strengths
🔍 Code Quality Issues1. Potential Race Condition in Click Handler// Line 1797-1800
if (isRelaxedMode && !isHovering) {
setIsHovering(true);
return;
}Issue: In relaxed mode, the first click sets Recommendation: Consider using a ref to track click count or adding a small debounce to ensure state updates are processed. 2. Inconsistent Error Icon Usage// UrlCitationComponent.tsx line 61-67
error_timeout: { icon: "⊗", label: "Timed out", className: "text-red-500 dark:text-red-400" },
error_not_found: { icon: "⊗", label: "Not found", className: "text-red-500 dark:text-red-400" },Issue: The Recommendation: Either:
3. Deprecated Prop Not Removed// Line 208
openUrlOnClick: _openUrlOnClick, // Deprecated, clicking always opens URL nowIssue: The deprecated prop is accepted but ignored. This could lead to confusion for developers who update the package expecting the prop to still work. Recommendation: Consider removing the prop in the next major version, or log a deprecation warning when it's used: if (_openUrlOnClick !== undefined) {
console.warn('openUrlOnClick prop is deprecated and will be removed in v2.0');
}4. Missing Dependency Array Item// Line 1812-1824
[
behaviorConfig,
eventHandlers,
citation,
citationKey,
verification?.verificationImageBase64,
isMiss,
isMobile,
isRelaxedMode,
isHovering, // ⚠️ This is state, so including it is correct
getBehaviorContext,
applyBehaviorActions,
],Concern: Including 🎯 Design & UX Considerations5. Double-Click Requirement in Relaxed ModeThe relaxed mode requires users to click once to see the popover, then click again to zoom the image. For users accustomed to single-click interactions, this might feel unintuitive. Suggestion: Consider adding a visual hint (e.g., "Click to zoom" text in the popover footer) when in relaxed mode to guide users. 6. Mobile vs Desktop ParityThe PR description states that relaxed mode mimics mobile behavior on desktop. However, there's a subtle difference:
Question: Should these use the same mechanism for consistency? The different implementations could lead to subtle behavioral differences. 🔒 Security & Performance7. No Security ConcernsThe changes don't introduce any security vulnerabilities. The external link opening uses proper 8. Performance ImpactThe additional conditional checks in click/hover handlers are minimal and won't cause noticeable performance issues. The cursor class calculation is memoized through the props object. 🧪 Testing RecommendationsThe PR includes a test plan checklist but no automated tests. Consider adding:
📝 Documentation Gaps9. CLAUDE.md Updates NeededThe // For dense citation layouts
<CitationComponent
citation={citation}
verification={verification}
interactionMode="relaxed"
/>10. Migration GuideConsider adding a migration note for the deprecated 🎨 Minor Style Suggestions11. Comment Clarity// Line 1795-1796
// Relaxed mode: first click shows popover, second click opens image
// (similar to mobile behavior but for desktop)Suggestion: Expand this comment to explain why relaxed mode exists: // Relaxed mode: Reduces hover popover noise in dense citation layouts.
// First click shows popover, second click opens image (similar to mobile UX).📊 Overall Assessment
✅ Recommended Actions Before Merge
🎉 ConclusionThis is a solid PR that adds a useful feature for handling dense citation layouts. The code is well-written and maintains backward compatibility. With the minor issues addressed, this will be a valuable addition to the library. Recommendation: Approve with minor changes requested (issues #2 and #3). |
Updated tests to reflect that clicking URL citations now opens the URL by default. The openUrlOnClick prop is deprecated. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review for PR #145: Add relaxed interaction mode and fix citation indicatorsSummaryThis PR introduces a new ✅ Strengths
🔍 Code Quality ObservationsGood Practices
Minor Issues
🐛 Potential Issues
🎯 Suggestions
🔒 Security
⚡ Performance
📋 Test Coverage
Overall AssessmentLGTM with minor suggestions. The code is well-written, properly typed, and the feature is useful. The minor issues mentioned above are not blockers and can be addressed in follow-up work if desired. Recommended next steps:
Great work on maintaining consistency across document and URL citations! 🎉 |
…components.md - Document eager vs relaxed interaction modes for CitationComponent - Add props reference for interactionMode - Add notes about UrlCitationComponent click behavior and XCircleIcon usage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📚 Documentation PreviewThe documentation has been built successfully. To preview locally:
Workflow run: 21583341831 |
Code Review: PR #145 - Add Relaxed Interaction Mode and Fix Citation IndicatorsOverviewThis PR introduces a new ✅ Strengths
🔍 Issues & Suggestions1. Missing Test Coverage for New
|
Tests cover: - Eager mode: hover shows popover, click opens image, cursor classes - Relaxed mode: hover only styles (no popover), first click shows popover, second click opens image, cursor class transitions - Integration with behaviorConfig and eventHandlers - Edge cases: no image available, cursor reset after closing overlay Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changed popover detection from [data-popover] to [data-state="open"] which is the correct Radix UI selector for open popover content. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review for PR #145SummaryThis PR adds a new ✅ Strengths1. Excellent Design & UX Considerations
2. Strong Test Coverage
3. Excellent Documentation
4. Consistent Visual Language
5. Backward Compatibility
🔍 Issues & Suggestions1. Potential State Inconsistency in Relaxed Mode (Priority: High)Location: Issue: In relaxed mode, clicking opens the popover by setting Suggestion: Add a state flag like 2. Missing Accessibility Consideration (Priority: Medium)Location: Keyboard navigation in relaxed mode Issue: The interaction mode logic is only applied to mouse clicks. Keyboard users will trigger Suggestion: Consider adding 3. Deprecated Prop Handling (Priority: Medium)Location: Issue: The deprecated Suggestion: Add a development warning when the prop is used. 4. Type Safety Enhancement (Priority: Low)Location: The 5. Performance Issue (Priority: Low)Location: The cursor class computation runs on every render. Consider memoizing with 🛡️ Security Considerations✅ No security concerns identified
⚡ Performance ConsiderationsMinor cursor class calculation could be optimized, but overall performance impact is negligible. 🧪 Test Coverage✅ Excellent test coverage with all major scenarios covered. Suggestion: Add one test case for issue #1 (popover closing on mouse leave after click-to-open). 📚 Documentation Quality✅ Excellent with clear examples, comparison tables, and thorough JSDoc comments. ✅ ApprovalThis PR is well-designed, thoroughly tested, and excellently documented. The core functionality works as intended, and the identified issues are relatively minor refinements that could be addressed in follow-up PRs. Recommendation: Approve with suggestions for follow-up improvements. The new Great work! 🎉 |
Removed waitFor that was causing timeout in CI. When no image is available, the test just verifies that clicking doesn't open any dialog and cursor stays as pointer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Add relaxed interaction mode and fix citation indicatorsOverall, this is a well-executed PR with thoughtful UX improvements, comprehensive testing, and excellent documentation. The code quality is high, and the changes are backward compatible. Here's my detailed review: ✅ Strengths1. Excellent Test Coverage
2. Backward Compatibility
3. Documentation
4. UX Improvements
🔍 Code Quality ObservationsStrong Points
Minor Concerns1. Cursor Logic Complexity (src/react/CitationComponent.tsx:2169-2180)The cursor class calculation uses nested ternary operators which can be hard to read: const cursorClass = isRelaxedMode
? isHovering && hasImage
? "cursor-zoom-in"
: "cursor-pointer"
: hasImage
? "cursor-zoom-in"
: "cursor-pointer";Suggestion: Consider extracting to a helper function for clarity: const getCursorClass = () => {
if (isRelaxedMode) {
return isHovering && hasImage ? "cursor-zoom-in" : "cursor-pointer";
}
return hasImage ? "cursor-zoom-in" : "cursor-pointer";
};
const cursorClass = getCursorClass();2. Popover State Reset (src/react/CitationComponent.tsx:1359)When closing the image overlay in relaxed mode, the comment says "popover closed when overlay closes", but I don't see explicit code resetting Verification needed: Does the overlay close handler properly reset the popover state? The test at line 357-383 seems to expect cursor reset, but verify the implementation handles this. 3. Potential Race Condition (src/react/CitationComponent.tsx:1847-1852)In cancelHoverCloseTimeout();
// In relaxed mode, don't show popover on hover (only style hover effects)
if (!isRelaxedMode) {
setIsHovering(true);
}This is probably fine since you'd only have a pending timeout if the popover was already open, but worth considering edge cases. 🚨 Issues Found1. Deprecated Prop Still Functional (src/react/UrlCitationComponent.tsx:835)The Recommendation: Remove the parameter entirely in the next major version, or add a console warning if it's provided: if (_openUrlOnClick !== undefined) {
console.warn('UrlCitationComponent: openUrlOnClick is deprecated. Clicking always opens the URL.');
}2. Documentation Comment Mismatch (src/react/CitationComponent.tsx:1697)The comment removed at line 1386 said:
But the new behavior always shows anchor text (line 1406). While this is intentional and improves consistency, the commit message at line 1698 says:
Potential issue: For very long anchor texts, showing both the anchor text in the header AND in the humanizing message might create visual clutter. Consider truncating long anchor texts in the header when a humanizing message exists. 3. Missing Type Export Check (src/react/index.ts:918)The duplicate export issue was fixed in commit 🔐 Security Considerations✅ No security concerns identified. The changes are purely UI/UX focused and don't introduce any new attack vectors. 🎯 Performance Considerations
📊 Test Coverage AnalysisWell-Covered:
Potential Gaps:
🎨 UX/Design FeedbackIcon Change (✕ → ⊗)The change from a text character to Consider: Adding a subtle animation (scale or fade-in) when the error indicator appears to draw attention to verification failures. Relaxed Mode Cursor FeedbackThe cursor transitions ( External Link Icon Always ShowingHaving the external link icon show on hover (even though click always opens URL) is good UX. It provides visual affordance without requiring a separate click target. 📝 Documentation ReviewCLAUDE.md Changes (lines 29-64)✅ Excellent comparison table and clear use cases AGENTS.md Changes (lines 9-16)✅ Concise and helpful for AI agents docs/components.md Changes (lines 76-112)✅ Thorough with usage examples Suggestion: Add a visual example or screenshot showing the difference between eager and relaxed modes in the docs site. 🔧 Suggestions for Follow-up
✅ Approval RecommendationLGTM with minor suggestions. This PR is ready to merge. The issues identified are minor and can be addressed in follow-up PRs if desired. The core functionality is solid, well-tested, and properly documented. Key Metrics
Great work! 🎉 |
PR Review: Add relaxed interaction mode and fix citation indicatorsOverall, this is a well-implemented feature with excellent test coverage and documentation. Strengths
Code Quality ObservationsMinor Issues1. Potential State Race Condition (CitationComponent.tsx:1804-1807) 2. Cursor Class Logic Readability (CitationComponent.tsx:2174-2186) 3. UrlCitationComponent Behavior Change (UrlCitationComponent.tsx:264-266) Security, Performance, Test Coverage✅ Security: No concerns - noopener,noreferrer correctly used Final AssessmentOverall: Approved ✅ This is production-ready code. The identified issues are minor and can be addressed in follow-up PRs if needed. Great work! |
…nent The openUrlOnClick prop was never used by users and has been completely removed rather than kept for backwards compatibility. Clicking a URL citation now always opens the URL in a new tab. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Add relaxed interaction mode and fix citation indicatorsSummaryThis PR introduces a ✅ Strengths1. Excellent Test Coverage
2. Good Documentation
3. Clean API Design
4. Consistent Icon Updates
🔍 Issues & Concerns1. URL Citation Click Behavior Change (Breaking)The PR changes Location: // Before: Conditional based on openUrlOnClick prop
if (openUrlOnClick) {
window.open(url, "_blank", "noopener,noreferrer");
}
// After: Always opens URL
window.open(url, "_blank", "noopener,noreferrer");Impact:
Recommendation:
2. Potential State Management Bug in Relaxed ModeWhen the image overlay closes in relaxed mode, the popover state may not properly reset. Location: The logic assumes that clicking the overlay will close the popover, but I don't see explicit code to reset Test that might reveal this: // The test at line 358-383 checks this but may not catch timing issues
it("resets to cursor-pointer after closing image overlay", () => {
// ... opens image, then clicks overlay to close
// Should verify isHovering state, not just cursor class
});Recommendation:
3. Documentation InconsistencyThe documentation shows different cursor behaviors in different places: In CLAUDE.md (lines 62-63): In docs/components.md (line 110): The second description is more general but loses the nuance about relaxed mode's state-dependent cursor. Recommendation: Keep the detailed explanation from CLAUDE.md or unify both. 💡 Minor Suggestions4. Status Icons Text AlternativeIn Location: Trade-off: Consistency vs. Information Density
Recommendation: This is acceptable if the error type is shown on hover/in the popover. Verify that 5. Missing Type ExportThe new Location to verify: 6. Test Timing DependenciesThe tests use hardcoded delays like Location: Recommendation: Consider using 🔐 Security Review✅ No security concerns identified. The changes are UI/interaction-focused with no new external inputs or XSS vectors. 🎨 Code Quality
📊 Performance Considerations✅ No performance concerns. The additional state checks ( Final RecommendationApprove with minor revisions:
The feature itself is well-designed and thoroughly tested. With the breaking change properly communicated, this is ready to merge. Reviewed by Claude Code 🤖 |
Summary
interactionModeprop toCitationComponentwith two modes:eager(default): Hover shows popover immediately, click opens image/expands detailsrelaxed: Hover only applies style effects (no popover), click opens popover first, then second click zooms imageXCircleIcon(circle with X) instead of text character, centered instead of subscript positioningChanges
New
interactionModePropUse
relaxedmode when citations are densely packed and hover popovers would be distracting:eager(default)relaxedIcon Fixes
MissIndicatornow usesXCircleIconSVG instead of✕text characterUrlCitationComponentuseXCircleIconwith centered alignmentSTATUS_ICONSuses⊗symbol instead of "404" text for error statesURL Citation Behavior
openUrlOnClickprop is now deprecated (clicking always opens URL)Cursor Updates
eagermode with image:cursor-zoom-inrelaxedmode without popover:cursor-pointerrelaxedmode with popover + image:cursor-zoom-inTest plan
interactionMode="eager"shows popover on hoverinteractionMode="relaxed"only shows style hover effects, click opens popover