feat: Enhanced hyperlink system with rich metadata, selection improvements, and migration guide

[kshivang]

Summary

Hyperlink Enhancements

• Rich Hyperlink Click Metadata: onLinkClick callback now receives HyperlinkInfo with full metadata (type, url, patternId, isFile, isFolder, scheme)
• Fallback Behavior: onLinkClick returns Boolean - return true if handled, false to use default browser/finder behavior
• File/Folder Path Detection: iTerm2-style clickable file paths with Cmd/Ctrl+Click support
• Wrapped Hyperlink Detection: Links spanning multiple terminal lines are now properly detected
• Custom Hyperlink Patterns: hyperlinkRegistry parameter for per-instance custom patterns (JIRA, GitHub issues, etc.)
• URL Pattern Fix: Exclude trailing punctuation (., ,, ;, !) from URL matches

Selection System Improvements

• Content-Anchored Selection: Selection tracking survives scrolling and new output
• Cache Invalidation: Track last snapshot by identity to skip unnecessary rebuilds
• Configurable Selection Alpha: New selectionAlpha setting (0.0-1.0, default 0.6)
• URL Regex Robustness: Support for ftp, ssh, file, mailto schemes and www patterns

Documentation

• Migration Guide: Added to all docs (embedding.md, tabbed-terminal.md, wiki pages)
• Breaking Change Documentation: onLinkClick signature change from Unit to Boolean
• hyperlinkRegistry Documentation: Custom pattern examples for JIRA, GitHub issues

Other Fixes

• PWD Environment Fix: Starship prompt compatibility with workingDirectory parameter
• Cross-Platform Tests: FilePathResolver tests now pass on Windows CI
• Unit Tests: 40+ tests for selection edge cases, file path detection, and trailing punctuation

Breaking Changes

onLinkClick Signature Change (v1.0.65+)

// Before
onLinkClick: ((HyperlinkInfo) -> Unit)? = null

// After
onLinkClick: ((HyperlinkInfo) -> Boolean)? = null

See Migration Guide for details.

Test plan

• Unit tests pass: ./gradlew :compose-ui:desktopTest
• Verify Cmd/Ctrl+Click on URLs opens browser
• Verify Cmd/Ctrl+Click on file paths opens in Finder/editor
• Verify custom onLinkClick can return false to trigger default behavior
• Verify selection persists when new output arrives
• Verify wrapped URLs across lines are clickable
• Verify trailing punctuation excluded from URLs

🤖 Generated with Claude Code

[claude]

Code Review - PR #207: Selection System Follow-up Improvements

Overview

This PR implements all 4 tasks from issue #204 with solid implementation quality. The code is well-structured, thoroughly tested, and follows the project's conventions. Overall, this is excellent work and ready to merge with only minor suggestions.

---

✅ Strengths

1. Comprehensive Test Coverage (319 lines of tests!)

   • 40+ tests covering URL patterns, emails, paths, quoted strings
   • Edge case testing (empty strings, boundaries, whitespace)
   • Pattern priority verification
   • Long line windowing optimization tests
   • Registry management tests

2. Smart Cache Invalidation

   • Identity-based tracking using lastCachedSnapshot is elegant
   • Avoids unnecessary rebuilds while maintaining correctness
   • Thread safety considerations documented

3. Enhanced URL Support

   • Added ftp://, ftps://, ssh://, file:// schemes
   • www. pattern support (priority 95)
   • mailto: with query string support
   • RFC 3986 compliant character set

4. Configurable Selection Alpha

   • Default 0.6 provides good balance
   • Proper bounds checking with coerceIn(0f, 1f)
   • Integrated into TerminalSettingsOverride for per-instance customization

---

🔍 Code Quality Observations

SelectionTracker.kt

Excellent: Cache invalidation logic is clean and safe. Identity check === is correct since snapshots are new objects on each mutation.

Minor suggestion: Consider adding a comment about thread safety expectations:

/**
 * Invalidate the cache, forcing a rebuild on next resolveToCoordinates() call.
 * Call this if external factors may have changed line positions.
 * Thread safety: Must be called from UI thread only (same as other methods).
 */
fun invalidateCache() {
    lastCachedSnapshot = null
}

SmartWordSelection.kt

Very good: The regex patterns are well thought out. However, I noticed a potential issue:

⚠️ Regex Escape Issue (Line 64):

regex = Regex("""(?<![/\w])www\.[\w\-._~:/?#\[\]@!$&'()*+,;=%]+""")

The negative lookbehind (?<![/\w]) might not behave as expected because \w inside a character class [] is treated literally, not as a word character shorthand. Consider:

regex = Regex("""(?<!/)(?<!\w)www\.[\w\-._~:/?#\[\]@!$&'()*+,;=%]+""")

Or more elegantly:

regex = Regex("""(?<![/\w])www\.[\w\-._~:/?#\[\]@!$&'()*+,;=%]+""")

Actually, looking more carefully, character classes in lookbehinds DO work correctly in Kotlin regex. This is not an issue - my apologies for the confusion. The pattern is correct as-is.

SmartWordSelectionTest.kt

Excellent test coverage. A few suggestions:

1. Add test for www regex edge case:

@Test
fun testWwwUrlNotInMiddleOfPath() {
    // Should NOT match www in "http://example.com/www.backup"
    val text = "Check http://example.com/www.backup here"
    val result = SmartWordSelection.selectWordAt(text, 30) // On "www"
    assertNotNull(result)
    // Should select entire URL, not just www.backup
    assertTrue(text.substring(result.first, result.second + 1).startsWith("http://"))
}

2. Add test for trailing punctuation:

@Test
fun testUrlWithTrailingPunctuation() {
    // Common case: URL at end of sentence
    val text = "Visit https://example.com."
    val result = SmartWordSelection.selectWordAt(text, 15)
    assertNotNull(result)
    // Should NOT include trailing period
    assertEquals("https://example.com", text.substring(result.first, result.second + 1))
}

3. Add test for mailto query params:

@Test
fun testMailtoWithQueryParams() {
    val text = "Send to mailto:user@example.com?subject=Hello&body=World"
    val result = SmartWordSelection.selectWordAt(text, 15)
    assertNotNull(result)
    assertEquals("mailto:user@example.com?subject=Hello&body=World", 
                 text.substring(result.first, result.second + 1))
}

TerminalSettings.kt

Perfect: The coerceIn(0f, 1f) ensures bounds safety, and the default of 0.6 is sensible.

---

🔒 Security Considerations

✅ No security concerns identified. Regex patterns are for matching only (no eval/exec).

---

⚡ Performance Considerations

1. Cache Invalidation ✅

   • Identity check === is O(1)
   • Avoids unnecessary lineIndexCache.clear() and rebuild

2. Regex Performance ✅

   • Quick checks optimize common cases
   • Windowed search for long lines (O(window_size) vs O(line_length))

3. Copy-on-Write Pattern Usage ✅

   • CopyOnWriteArrayList appropriate for read-heavy registry

---

📝 Documentation

Good: Comments are clear and helpful. KDoc is present on public APIs.

Suggestion: Add KDoc to selectionAlpha in TerminalSettings.kt explaining valid range:

/**
 * Selection highlight opacity (0.0 to 1.0).
 * Lower values make text more readable through selection highlight.
 * Default 0.6 provides good contrast while keeping text visible.
 * Values outside 0.0-1.0 will be clamped automatically.
 */
val selectionAlpha: Float = 0.6f,

---

🧪 Test Coverage Assessment

Excellent: 40+ tests covering:

• ✅ All new URL schemes (ftp, ssh, file, mailto, www)
• ✅ Edge cases (empty, boundaries, whitespace)
• ✅ Pattern priority verification
• ✅ Registry management (add, remove, reset)
• ✅ Long line windowing optimization

Missing (non-blocking, future enhancement):

• Surrogate pairs and emoji in selection (mentioned in Selection system follow-up improvements #204 but deferred)
• Wide characters (CJK) - partially covered but could expand

---

🎯 Recommendations

Before Merge:

1. ✅ Tests pass (mentioned in PR description - awaiting confirmation)
2. ✅ Manual testing checklist in PR description

Future Enhancements (separate PR):

1. Add the 3 test cases I suggested above for edge cases
2. Consider adding integration tests for double-click selection behavior
3. Document the selectionAlpha setting in user-facing docs/settings UI

---

✅ Final Verdict

LGTM with minor documentation suggestions.

This PR:

• ✅ Implements all 4 tasks from Selection system follow-up improvements #204
• ✅ Has excellent test coverage
• ✅ Follows project conventions (per CLAUDE.md)
• ✅ No bugs or security issues identified
• ✅ Performance optimizations preserved
• ✅ Backward compatible

Approval Status: Approved pending successful test execution.

Great work! This is a solid contribution that improves the selection system meaningfully while maintaining code quality.

---

Reviewed by Claude Code using project guidelines from CLAUDE.md

[claude]

Code Review: Selection System Follow-up Improvements (#204)

Summary

This PR successfully implements all 4 tasks from issue #204 with high-quality code. The implementation is well-architected, thoroughly tested, and follows the project's conventions. Great work overall!

---

✅ Strengths

1. Excellent Test Coverage (SmartWordSelectionTest.kt)

• 40+ comprehensive tests covering all edge cases
• Well-organized test categories (URLs, emails, paths, quotes, unicode, long lines)
• Tests verify pattern priority correctly
• Good edge case coverage (empty strings, boundaries, whitespace)

2. Clean Cache Invalidation (SelectionTracker.kt)

• Simple and effective identity-based cache invalidation using lastCachedSnapshot
• Avoids unnecessary rebuilds with reference equality check (===)
• Added invalidateCache() for manual clearing when needed
• Good documentation explaining the approach

3. Extended URL Pattern Support (SmartWordSelection.kt)

• Added ftp://, ftps://, ssh://, file://, mailto: schemes
• RFC 3986 compliant character set
• Added www. pattern (priority 95) - nice touch
• Proper priority ordering ensures correct matching

4. Configurable Selection Alpha (TerminalSettings.kt)

• Range validation (0.0-1.0) with sensible default (0.6)
• Applied to rendering in TerminalCanvasRenderer.kt:914
• Properly integrated with TerminalSettingsOverride
• Good documentation explaining the purpose

5. New FilePathResolver (158 lines)

• Comprehensive path resolution (Unix, Windows, home-relative, relative)
• Smart caching with TTL (5 seconds) and max size (1000)
• Thread-safe with ConcurrentHashMap
• Well-documented quick check methods

---

🔍 Issues & Concerns

1. Test File Location Mismatch (CRITICAL)

File: compose-ui/src/commonTest/kotlin/.../SmartWordSelectionTest.kt

The test file is placed in commonTest but tests a desktopMain class (SmartWordSelection). This will likely cause compilation errors because:

• SmartWordSelection is in desktopMain scope
• commonTest cannot access desktopMain classes
• Should be in compose-ui/src/desktopTest/kotlin/... instead

Recommendation:

# Move to correct location
git mv compose-ui/src/commonTest/kotlin/ai/rever/bossterm/compose/selection/SmartWordSelectionTest.kt \
       compose-ui/src/desktopTest/kotlin/ai/rever/bossterm/compose/selection/SmartWordSelectionTest.kt

2. File Path Hyperlink Detection May Have Performance Impact

Files: HyperlinkDetector.kt, FilePathResolver.kt

The new file path detection adds 4 patterns that perform filesystem I/O:

• Every render cycle will call FilePathResolver.exists() for potential paths
• Even with caching (5s TTL), this could impact performance on slow filesystems or network drives
• The cache has no limit on concurrent file checks

Concerns:

• What happens if working directory is on a network mount or slow filesystem?
• Cache eviction happens only when size > 1000, not proactively
• No timeout on filesystem operations

Recommendations:

1. Add a setting to disable file path detection (see detectFilePaths parameter - this is good!)
2. Consider async filesystem checks with a callback pattern
3. Add metrics/logging for cache hit rates and filesystem check latency
4. Document the performance characteristics in the FilePathResolver class

3. Path Validation Inconsistency

File: HyperlinkDetector.kt:348-355

val url = if (pattern.pathValidator != null) {
    pattern.pathValidator.invoke(match.value, workingDirectory) ?: continue
} else {
    pattern.urlTransformer(match.value)
}

When pathValidator returns null (path doesn't exist), the loop continues to the next match. However:

• This creates a different UX than URL patterns (always hyperlinked)
• File paths that don't exist yet won't be clickable (could be intentional for creation)
• No visual distinction between "path exists" and "not a path"

Question: Is this the intended behavior? Consider:

• Should non-existent paths still be hyperlinked (maybe open parent directory)?
• Should there be a visual indicator (different color) for non-existent paths?

4. Regex Pattern Edge Cases

File: SmartWordSelection.kt:48

regex = Regex("""(?:https?|ftps?|ssh|file)://[\w\-._~:/?#\[\]@!$&'()*+,;=%]+""")

Issues:

• Matches https://example.com)) including the trailing parentheses
• Matches https://example.com, including the comma
• Doesn't handle balanced parentheses (common in Wikipedia URLs)

Example failure cases:

• Check this link (https://example.com/page) for details → selects the closing paren
• Visit https://en.wikipedia.org/wiki/Cat_(disambiguation) → truncated at first paren

Recommendation: Consider iTerm2's approach:

• Strip common trailing punctuation: ,, ., ), ], }
• Handle balanced parentheses specially for Wikipedia-style URLs
• See: https://gitlab.com/gnachman/iterm2/-/blob/master/sources/VT100Terminal.m#L1000

5. Missing Test Coverage for FilePathResolver

File: No test file for FilePathResolver.kt

This is a complex utility with:

• Path resolution logic (4 different types)
• Caching with TTL
• Thread safety requirements
• Platform-specific behavior (Windows vs Unix)

Recommendation: Add FilePathResolverTest.kt covering:

• Path resolution for all path types
• Cache hit/miss behavior
• Cache eviction when size > 1000
• TTL expiration
• Thread safety (concurrent access)
• Edge cases (null working directory, invalid paths)

---

🎯 Best Practices & Style

✅ Good

• Proper use of CopyOnWriteArrayList for thread safety in WordSelectionPatternRegistry
• Clear separation of concerns (resolver, detector, patterns)
• Good documentation and inline comments
• Follows Kotlin conventions (data classes, object singletons)
• Proper use of IdentityHashMap for object identity tracking

⚠️ Suggestions

1. FilePathResolver.kt:86-88 - Consider removeIf in a separate pass to avoid ConcurrentModificationException risk:

   if (pathCache.size > MAX_CACHE_SIZE) {
       val toRemove = pathCache.filter { it.value.second < now - CACHE_TTL_MS }.keys
       toRemove.forEach { pathCache.remove(it) }
   }

2. HyperlinkDetector.kt:410 - The canContainHyperlink quick check could return false positives:

   if (line.contains('/') && !line.contains("://")) {
       return true  // Could match "1/2/3" (not a path)
   }

   Consider more precise checks like line.contains("/path") or line.contains("./")

---

🔒 Security

No security concerns identified. The file path resolution properly canonicalizes paths and doesn't introduce command injection or path traversal vulnerabilities.

---

📊 Performance

Cache Statistics

• FilePathResolver cache: 1000 entries, 5s TTL
• SelectionTracker cache: Identity-based, no TTL (invalidated by snapshot change)

Potential Optimizations

1. Lazy pattern compilation - Patterns are compiled eagerly in registry init
2. Batch filesystem checks - Current implementation checks one path at a time
3. Background cache warming - Could pre-populate cache for common paths

---

📝 Documentation

Missing

• CHANGELOG entry for new features
• User-facing documentation for selectionAlpha setting
• Performance notes in FilePathResolver class header

Suggestions

• Add example to selectionAlpha setting comment showing how to adjust for light themes
• Document the detectFilePaths parameter in user docs

---

✅ Test Plan Status

Per PR description:

• Unit tests pass: ./gradlew :compose-ui:desktopTest (⚠️ after moving test file)
• Manual testing: double-click on various URLs, emails, paths
• Verify selection transparency with default 0.6 alpha

Recommendation: Before merging, please verify:

1. Tests pass after moving to desktopTest
2. Manual test on macOS and Linux (file path detection behavior differs)
3. Performance test with network-mounted working directory

---

🎉 Conclusion

This is high-quality work that successfully addresses all requirements from #204. The main issues are:

1. MUST FIX: Move test file to desktopTest
2. SHOULD FIX: Add unit tests for FilePathResolver
3. CONSIDER: URL regex improvements for trailing punctuation

The code is production-ready after addressing the test file location issue. Great job on the comprehensive test coverage and clean implementation!

Recommendation: Request changes - Fix test location, then approve.

[claude]

Updated Code Review - PR #207 (Final Review)

I've reviewed the latest changes (commits c04ce50 and f91c2b3). Great work addressing the previous feedback! The addition of FilePathResolverTest.kt (233 lines) and HyperlinkDetectorPathTest.kt (335 lines) significantly improves test coverage.

---

🎯 Status: Almost Ready to Merge

✅ What's Been Fixed Since Last Review

1. ✅ FilePathResolver Test Coverage - Comprehensive tests added covering:

   • Path resolution for all types (Unix, Windows, home-relative, relative)
   • Cache behavior and TTL
   • Thread safety considerations
   • Edge cases

2. ✅ HyperlinkDetector Path Tests - Excellent coverage for:

   • Path detection and validation
   • Working directory context
   • Platform-specific behavior

---

⚠️ Remaining Critical Issue

MUST FIX: Test File Location Mismatch

File: compose-ui/src/commonTest/kotlin/.../SmartWordSelectionTest.kt

Problem:

• SmartWordSelection.kt is in desktopMain (desktop-specific code)
• SmartWordSelectionTest.kt is in commonTest (common/shared code)
• This will cause compilation failure because commonTest cannot access desktopMain classes

Fix:

# Move test file to correct location
git mv compose-ui/src/commonTest/kotlin/ai/rever/bossterm/compose/selection/SmartWordSelectionTest.kt \
       compose-ui/src/desktopTest/kotlin/ai/rever/bossterm/compose/selection/SmartWordSelectionTest.kt

Why This Matters:

• Kotlin Multiplatform projects have strict source set boundaries
• commonTest can only test commonMain code
• desktopTest can test both commonMain and desktopMain code
• Running ./gradlew :compose-ui:desktopTest will likely fail with the current setup

---

📊 Code Quality Assessment

Overall: Excellent 🌟

Strengths:

1. Comprehensive Test Coverage - 587 lines of new tests (40+ in SmartWordSelection, 233 in FilePathResolver, 335 in HyperlinkDetector)
2. Clean Architecture - Good separation of concerns (resolver, detector, patterns)
3. Performance Optimization - Smart caching with identity checks in SelectionTracker
4. Documentation - Clear comments and KDoc
5. Settings Integration - selectionAlpha properly integrated with override support

Code Quality Observations:

SelectionTracker.kt ✅

• Cache invalidation using identity check (===) is correct and efficient
• Good documentation on thread safety expectations

SmartWordSelection.kt ✅

• Extended URL patterns (ftp, ssh, file, mailto, www) are well-implemented
• RFC 3986 compliant character set
• Windowed search optimization for long lines

FilePathResolver.kt ✅

• Comprehensive path resolution logic
• Smart caching with 5s TTL and 1000 entry max
• Thread-safe with ConcurrentHashMap
• Good edge case handling

TerminalSettings.kt ✅

• selectionAlpha properly bounded with coerceIn(0f, 1f)
• Sensible default (0.6)

---

🔍 Minor Suggestions (Non-Blocking)

1. URL Regex Trailing Punctuation

File: SmartWordSelection.kt:48

The current regex matches trailing punctuation that's often not part of URLs:

"Visit https://example.com." → selects the period
"Link (https://example.com)" → selects the closing paren

Consideration: This is common across terminal emulators and might be intentional for simplicity. If it becomes a UX issue, consider post-processing to strip common trailing punctuation (,.;)]}>).

2. File Path Performance on Network Filesystems

Files: HyperlinkDetector.kt, FilePathResolver.kt

The exists() checks could be slow on network mounts. Mitigation strategies already in place:

• ✅ 5-second cache TTL
• ✅ detectFilePaths parameter for disabling
• ✅ Quick checks to skip obvious non-paths

Optional Enhancement: Consider adding a timeout for filesystem operations or making them async (future PR).

3. Documentation Enhancement

Add example to selectionAlpha setting:

/**
 * Selection highlight opacity (0.0 to 1.0).
 * Lower values make text more readable through selection highlight.
 * Default 0.6 provides good contrast while keeping text visible.
 * 
 * Examples:
 * - 0.3 for light themes / high contrast text
 * - 0.6 (default) balanced visibility
 * - 0.8 for dark themes / low contrast text
 */
val selectionAlpha: Float = 0.6f,

---

🔒 Security

✅ No security concerns. Path resolution properly uses canonicalFile to prevent traversal attacks.

---

⚡ Performance

✅ Well-optimized:

• Identity-based cache invalidation (O(1))
• Windowed regex search for long lines
• CopyOnWriteArrayList for read-heavy pattern registry
• Smart filesystem caching with TTL

---

✅ Test Plan Verification

Per PR description:

• Unit tests implemented (587 new lines)
• Build verification needed: ./gradlew :compose-ui:desktopTest (after fixing test location)
• Manual testing: double-click on URLs, emails, paths
• Verify selection alpha transparency

---

📝 Recommendations

Before Merge (Required):

1. Fix test file location (move SmartWordSelectionTest.kt to desktopTest)
2. Verify tests pass: Run ./gradlew :compose-ui:desktopTest
3. Manual testing on macOS and Linux (file path behavior may differ)

After Merge (Optional):

1. Add user-facing documentation for selectionAlpha setting
2. Consider adding performance metrics for file path detection
3. Document shell integration examples for path detection

---

🎉 Final Verdict

Status: ✅ Approve with Changes Required

This PR successfully implements all 4 tasks from #204 with excellent code quality and test coverage. The only blocking issue is the test file location, which is a quick fix.

Action Required:

• Move SmartWordSelectionTest.kt to desktopTest
• Verify build passes
• Then merge ✅

Great work on this comprehensive improvement to the selection system! The added functionality (file path detection, configurable alpha, extended URL support) and the test coverage make this a solid contribution.

---

Reviewed by Claude Code following CLAUDE.md guidelines

[claude]

PR Review: Selection System Follow-up Improvements (#204)

Overall Assessment

This is a well-implemented, comprehensive PR that addresses all 4 tasks from issue #204. The code quality is excellent, with strong test coverage, proper documentation, and thoughtful architectural decisions. ✅ Recommended for merge after addressing a few minor points below.

---

Task-by-Task Review

✅ Task 1: Cache Invalidation for buildLineIndexCache()

Location: SelectionTracker.kt:168-171

Implementation: Uses identity check (===) on VersionedBufferSnapshot to skip rebuilds.

Strengths:

• Correct use of identity semantics for cache invalidation
• Leverages existing snapshot infrastructure
• Clear documentation of rationale

Concern: The cache grows unbounded for the lifetime of the SelectionTracker. If a user runs the terminal for days with heavy scrolling, could lineIndexCache accumulate stale entries? Consider adding an optional invalidateCache() call when the cache size exceeds a threshold (e.g., 10,000 entries).

---

✅ Task 2: Configurable Selection Alpha

Location: TerminalSettings.kt:76-80, TerminalSettings.kt:669

Implementation: New selectionAlpha: Float = 0.6f setting, applied via .copy(alpha = ...) to selectionColorValue.

Strengths:

• Uses coerceIn(0f, 1f) for bounds safety
• Proper integration with TerminalSettingsOverride
• Good default value (0.6 balances visibility and readability)

---

✅ Task 3: URL Regex Robustness

Location: SmartWordSelection.kt:43-66

Implementation: Extended URL pattern to RFC 3986 charset, added ftp, ftps, ssh, file, mailto:, and www. patterns.

Strengths:

• RFC 3986 compliance is excellent
• Good priority ordering (100 for scheme URLs, 99 for mailto, 95 for www)
• quickCheck optimizations reduce regex overhead

⚠️ Issues Found:

1. www pattern has lookahead bug (line 64): The negative lookbehind (?<![/\w]) may not work correctly. Consider using word boundaries: \bwww\.

2. Scheme URL pattern may be too greedy: The RFC 3986 charset could accidentally match trailing punctuation like http://example.com/*. Consider ending patterns at common terminators.

---

✅ Task 4: Unit Tests - Exceptional Coverage!

Location: 3 new test files totaling 887 lines of tests

Files:

• SmartWordSelectionTest.kt (319 lines, 40+ tests)
• FilePathResolverTest.kt (233 lines, 30+ tests)
• HyperlinkDetectorPathTest.kt (335 lines, 25+ tests)

Strengths:

• Exceptional test coverage for URLs, emails, paths, quoted strings, edge cases
• Tests validate both positive and negative cases
• Good coverage of Unicode, long lines, priority ordering
• Proper cleanup with @BeforeTest/@AfterTest

Minor Issues:

• Some macOS-specific path tests may be flaky due to /tmp symlink resolution
• Missing negative tests for malformed patterns (unclosed quotes, invalid URLs)

---

Architecture & Design Review

✅ File Path Detection System

New Files: FilePathResolver.kt, HyperlinkInfo.kt

Strengths:

• Excellent separation of concerns
• 5-second cache for exists() checks reduces filesystem I/O
• Rich metadata (HyperlinkType enum, isFile/isFolder, scheme)
• Graceful degradation (paths only hyperlinked if they exist)

Concerns:

1. Cache eviction strategy: When hitting MAX_CACHE_SIZE, all expired entries are removed at once, which could be expensive. Consider LRU eviction.

2. Thread safety: removeIf iterates the entire ConcurrentHashMap. Under high load, this could block. Consider deferring cleanup to a background task.

---

⚠️ API Breaking Change: onLinkClick Signature

Changed: onLinkClick: ((String) -> Unit)? → onLinkClick: ((HyperlinkInfo) -> Boolean)?

Justification: Rich metadata enables proper link handling (folders vs files vs URLs).

⚠️ Breaking Change Risk: This affects existing consumers of TabbedTerminal and EmbeddableTerminal. Consider:

• Documenting migration path in CHANGELOG
• Bumping semantic version appropriately

---

Security Review

⚠️ Path Traversal Risk

Location: FilePathResolver.resolvePath() (lines 48-57)

Issue: Relative paths (./, ../) are resolved using canonicalFile, which follows symlinks. A malicious terminal app could emit paths like ../../../etc/passwd.

Current Mitigation: exists() check prevents non-existent paths from being hyperlinked.

Recommendation: Add optional path boundary validation to restrict resolved paths to within a safe directory.

---

Performance Considerations

✅ Cache Optimizations

• SelectionTracker cache: Identity check is O(1)
• FilePathResolver cache: 5-second TTL with 1000-entry limit

⚠️ Regex Performance

The RFC 3986 charset could cause backtracking on adversarial input. The quickCheck callbacks provide fast early-exit, but consider adding length limits.

---

Final Recommendations

Must Fix Before Merge

1. ⚠️ Fix www pattern regex (negative lookbehind issue)
2. ⚠️ Document breaking change in CHANGELOG (onLinkClick signature)
3. ⚠️ Add path traversal safety (optional boundary validation)

Should Fix (High Priority)

4. Improve cache eviction strategy (LRU instead of full sweep)
5. Add platform conditionals for Windows-specific tests
6. Add integration test for onLinkClick callback

Nice to Have

7. Add benchmarks for regex performance on long lines
8. Document migration guide for API consumers
9. Add cache hit/miss metrics for debugging

---

Summary

This PR delivers high-quality, production-ready code with exceptional test coverage (887 lines of tests!). The architecture is clean and the performance optimizations are well-thought-out.

Main concerns:

1. Breaking API change needs documentation
2. Minor regex bug in www pattern
3. Potential security risk from unrestricted path resolution

With the "Must Fix" items addressed, this PR is ready for merge. Excellent work! 🎉

---

Reviewed by: Claude Sonnet 4.5 via Claude Code
Date: 2026-01-02
Commit: 0388c32

[claude]

Code Review: PR #207 - Enhanced Hyperlink System

This is an excellent, well-architected PR that significantly enhances BossTerm's hyperlink capabilities. The code quality is high, with comprehensive testing and thoughtful performance optimization.

Strengths

Architecture & Design

• Rich Metadata API: HyperlinkInfo provides excellent semantic information (type, isFile, isFolder, scheme, patternId)
• Extensibility: hyperlinkRegistry parameter allows per-instance customization without modifying global state
• Fallback Pattern: onLinkClick callback returning Boolean is clean
• Separation of Concerns: FilePathResolver is cleanly separated from hyperlink detection

Performance Optimizations

• 5-second TTL cache prevents excessive filesystem checks
• Cache size limit (1000 entries) prevents memory leaks
• Quick check callbacks avoid expensive regex operations
• Selection cache invalidation using snapshot identity avoids unnecessary rebuilds

Test Coverage

• 894 lines of tests across 3 test files
• Excellent coverage of edge cases: empty strings, boundaries, Unicode, long lines

Cross-Platform Support

• Handles Unix/Windows paths, home-relative, and relative paths
• Platform detection in tests

Issues & Suggestions

1. Security: Path Traversal Risk (Medium Priority)

Location: FilePathResolver.kt:56-59

While .canonicalFile resolves ../ sequences, there is no validation that resolved paths stay within safe boundaries. Consider adding optional whitelist validation for security-conscious deployments.

Severity: Medium - requires user Cmd/Ctrl+Click action

2. Cache Eviction Race Condition (Low Priority)

Location: FilePathResolver.kt:85-89

ConcurrentHashMap.entries.removeIf() is not atomic. Consider synchronized block with double-check locking.

Impact: Minimal - worst case is redundant evictions

3. Edge Case: Symlink Loops (Low Priority)

Location: FilePathResolver.kt:58, 65

.canonicalFile throws IOException on symlink loops. Wrap in try-catch to silently ignore.

4. Documentation: Missing Examples (Low Priority)

Add KDoc examples for onLinkClick callback showing different link type handling.

5. Testing: macOS /tmp Symlink Edge Case

Test acknowledges /tmp -> /private/tmp on macOS but does not verify both paths work.

Overall Assessment

Rating: 5/5 - Production Ready

The architecture is excellent, testing is comprehensive, and the feature set is well-designed. Issues are mostly edge cases unlikely to occur in practice.

Recommended Actions Before Merge:

1. High Priority: Add symlink loop exception handling
2. Medium Priority: Consider path traversal protection
3. Low Priority: Add code examples to KDoc

Approved for Merge

Great work! This is exactly the kind of thoughtful, well-tested enhancement that makes a terminal emulator production-grade.

[claude]

PR Review: Enhanced Hyperlink System with Rich Metadata

Summary

This is a well-architected and thoughtful PR that significantly enhances BossTerm's hyperlink system. The implementation follows best practices with strong thread safety, comprehensive testing, and excellent documentation.

---

✅ Strengths

1. Excellent Architecture

• Rich metadata system: HyperlinkInfo provides comprehensive information (type, patternId, isFile/isFolder, scheme) enabling sophisticated handling
• Extensible design: HyperlinkRegistry with priority-based pattern matching allows easy customization
• Proper separation of concerns: Clean split between detection (HyperlinkDetector), resolution (FilePathResolver), and metadata (HyperlinkInfo)

2. Thread Safety

• CopyOnWriteArrayList for pattern registry (lines: HyperlinkDetector.kt:108)
• ConcurrentHashMap for path cache (lines: FilePathResolver.kt:23)
• Proper use of immutable data classes

3. Performance Optimizations

• Smart caching with 5-second TTL for filesystem checks (FilePathResolver.kt:24)
• Quick-check lambdas to avoid expensive regex on unsuitable text (HyperlinkDetector.kt:122)
• Cache eviction policy with size limit (FilePathResolver.kt:25, 86-89)
• Identity-based snapshot caching in SelectionTracker (SelectionTracker.kt:31)

4. Comprehensive Testing

• 40+ unit tests for selection edge cases (SmartWordSelectionTest.kt: 378 lines)
• File path detection tests (FilePathResolverTest.kt: 239 lines)
• Hyperlink path detection tests (HyperlinkDetectorPathTest.kt: 335 lines)
• Good coverage of edge cases, Unicode, trailing punctuation

5. Documentation

• Clear inline comments explaining complex logic
• Comprehensive examples in class KDoc
• Updated all user-facing documentation (embedding.md, tabbed-terminal.md)

---

🔍 Code Quality Observations

1. API Design - Return Boolean Pattern

The new onLinkClick: (HyperlinkInfo) -> Boolean API is excellent:

• Composable behavior: Return true for custom handling, false for default
• Rich context: Provides all metadata needed for intelligent decisions
• Backwards compatible: Optional parameter with sensible defaults

onLinkClick = { info ->
    when (info.type) {
        HyperlinkType.FILE -> {
            customEditor.open(info.url)
            true  // Handled
        }
        HyperlinkType.HTTP -> false  // Use default browser
        else -> false
    }
}

2. Path Validation Pattern

The pathValidator callback pattern (HyperlinkDetector.kt:64-74) is clever:

• Only creates hyperlinks for paths that actually exist
• Avoids cluttering terminal with false-positive path detection
• Returns null to skip match if path doesn't exist

3. Wrapped Line Detection

The collectWrappedLines implementation (HyperlinkDetector.kt:452-516) correctly handles:

• Walking backwards to find logical line start
• Padding to terminal width based on visual width (handles CJK/emoji correctly)
• Using ColumnConversionUtils for proper DWC handling

---

⚠️ Potential Issues & Recommendations

1. Security: Path Traversal Risk (Medium)

Location: FilePathResolver.kt:58

File(cwd, path).canonicalFile

Issue: Relative paths like ../../etc/passwd could escape intended directories if workingDirectory is user-controlled.

Recommendation: Add validation to ensure resolved path stays within allowed directories:

val resolved = File(cwd, path).canonicalFile
if (!resolved.startsWith(cwd)) return null  // Path escaped working directory

2. Race Condition in Cache Eviction (Low)

Location: FilePathResolver.kt:86-89

if (pathCache.size > MAX_CACHE_SIZE) {
    pathCache.entries.removeIf { it.value.second < expiredThreshold }
}

Issue: Between size check and eviction, other threads could add entries. This could cause cache to grow beyond MAX_CACHE_SIZE.

Impact: Low - cache would be slightly larger than intended, not a critical issue.

Recommendation: Consider using synchronized or atomic operations if strict size limit is important.

3. Null Pointer Risk in Error Handling (Low)

Location: HyperlinkDetector.kt:613-620

val startRow = rowSpans.keys.minOrNull()
    ?: error("rowSpans unexpectedly empty after non-empty check")

Issue: While the check at line 607 protects this, the error messages could be clearer about what went wrong.

Recommendation: Include contextual information in error messages:

?: error("BUG: rowSpans empty for hyperlink ${hyperlink.url} at row ${hyperlink.row}")

4. Regex Performance on Very Long Lines (Low)

Location: HyperlinkDetector.kt:348

val matches = pattern.regex.findAll(text)

Issue: For extremely long lines (e.g., minified JSON), regex matching could be slow.

Mitigation: You already have quickCheck optimization, which helps. Consider adding line length threshold:

if (text.length > 10000) continue  // Skip regex for very long lines

5. PWD Environment Variable Setting

Location: The PR mentions "PWD Environment Fix" for Starship prompt

I'd like to verify this is being set correctly. Could you confirm that PWD is set before the shell process starts, not after? Some shells (like fish) expect PWD to be inherited from the parent process.

---

🧪 Testing Recommendations

1. Security Test Cases

Add tests for path traversal scenarios:

@Test
fun testPathTraversalBlocked() {
    val workingDir = "/home/user/project"
    val maliciousPath = "../../../etc/passwd"
    val resolved = FilePathResolver.resolveAndValidate(maliciousPath, workingDir)
    // Should either be null or stay within /home/user
}

2. Edge Case: Empty Regex Matches

What happens if a custom pattern matches empty strings? Add validation:

if (match.value.isEmpty()) continue

3. Cache Correctness

Test that cache properly invalidates after TTL expires (FilePathResolverTest could add this).

---

📝 Minor Suggestions

1. Typo in Comment

FilePathResolver.kt:37: "optionally using a working directory" - consider "using an optional working directory"

2. Magic Numbers

Consider extracting to constants:

private const val MAX_LINE_LENGTH_FOR_REGEX = 10000
private const val WRAPPED_LINE_SEARCH_WINDOW = 150

3. Documentation Clarity

HyperlinkInfo.kt:47: Consider documenting that isBuiltin is based on patternId prefix, not actual registration.

---

🔐 Security Assessment

Overall: GOOD ✓

• No command injection risks (uses Runtime.exec(arrayOf(...)) correctly)
• No XSS risks (terminal output, not web context)
• Path validation exists (canonical file resolution)
• No SQL injection (no database interaction)

Areas to Harden:

1. Path traversal protection (see recommendation feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1)
2. Consider max URL length limits to prevent DoS via extremely long hyperlink matches

---

📊 Performance Assessment

Overall: EXCELLENT ✓

• Smart caching strategies throughout
• Quick-check optimizations to avoid expensive regex
• Copy-on-write data structures for thread safety without locks
• Identity-based snapshot caching prevents unnecessary rebuilds

Measured Impact:

The cached path existence checking (5-second TTL) is a great trade-off between accuracy and performance.

---

✅ Test Coverage Assessment

Coverage: COMPREHENSIVE ✓

• 378 lines of selection tests
• 239 lines of path resolver tests
• 335 lines of hyperlink path detection tests
• Edge cases well covered (trailing punctuation, Unicode, wrapped lines, etc.)

Gaps:

• No tests for custom pattern registration/removal
• No tests for onLinkClick fallback behavior
• No tests for concurrent access to FilePathResolver cache

---

🎯 Conclusion

This is high-quality work that meaningfully improves BossTerm's capabilities. The architecture is solid, performance is well-optimized, and testing is comprehensive.

Recommended Actions Before Merge:

1. Must Fix: Add path traversal validation (security)
2. Should Fix: Add tests for custom pattern registration
3. Nice to Have: Extract magic numbers to constants

Approval: ✅ APPROVED (pending path traversal fix)

Great work! This sets a strong foundation for advanced hyperlink features.

---

📚 Follow-up Suggestions

1. Consider adding hyperlink preview on hover (tooltip with URL)
2. Keyboard navigation through hyperlinks (Tab to cycle)
3. Right-click context menu for "Copy Link Address"
4. Pattern profiles (e.g., "jira", "github") for common workflows

🤖 Generated with Claude Code