feat: Add unfocused window overlay indicator

[kshivang]

Summary

• Add semi-transparent white overlay (15% opacity) when window loses focus
• Helps users quickly identify active terminal window
• New setting showUnfocusedOverlay (default: true) to toggle the feature

Test plan

• Click away from BossTerm window - overlay should appear
• Click back to BossTerm - overlay should disappear
• Test with multiple tabs and split panes
• Disable setting and verify overlay no longer appears

🤖 Generated with Claude Code

[claude]

Code Review

Summary

This PR adds a visual indicator (15% opacity white overlay) when the terminal window loses focus, helping users identify which window is active when working with multiple terminals.

✅ Strengths

1. Clean Implementation: The feature is implemented with minimal code changes (27 additions, 4 deletions) and integrates well with the existing architecture.

2. User Control: The showUnfocusedOverlay setting (default: true) gives users the ability to disable the feature if they prefer.

3. Reactive State Management: Uses derivedStateOf for efficient recomposition - the overlay will only recompose when window focus actually changes.

4. Proper UI Layering: Using Box to overlay the focus indicator on top of the terminal content is the correct approach, ensuring it doesn't interfere with the terminal UI.

5. Documentation: Good inline comments explaining the purpose of the overlay.

🔍 Observations & Suggestions

1. Performance Consideration - derivedStateOf Computation (Minor)

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

The derivedStateOf block will recompute whenever the composition recomposes. Since isWindowFocused() is a lambda that likely calls into AWT's window focus state, this could be called frequently during recompositions.

Suggestion: Consider if this is necessary. Since isWindowFocused is already a lambda parameter that's called reactively elsewhere (in TabController), you might be able to simplify to:

val isWindowFocusedState = isWindowFocused()

However, if the current approach ensures proper reactivity when focus changes, then it's fine as-is. Just verify that focus changes actually trigger recomposition correctly.

2. Overlay Color Choice (Design Consideration)

.background(Color.White.copy(alpha = 0.15f))

Using a white overlay works well for dark themes (which is the default), but may have less visual impact on light themes. Consider:

• Testing with light terminal themes to ensure visibility
• Potentially making the overlay color configurable or theme-aware in the future
• Alternative: Using a dim/desaturate effect instead of white overlay

This is not blocking - the current choice is reasonable for most users.

3. Settings Documentation (Enhancement)

The new setting showUnfocusedOverlay is well-documented in code. Consider adding it to:

• User-facing settings documentation (if exists)
• The test plan mentions disabling the setting, which is good

4. Indentation Inconsistency (Nitpick)

// Line 221-222
if (tabController.tabs.size > 1) {
TabBar(

Missing indentation for TabBar - should be indented under the if block. This is a minor style issue but affects code readability.

🧪 Testing Recommendations

The test plan in the PR description is good. Additional test cases to consider:

1. Rapid focus changes: Quickly switching between windows to ensure no visual glitches
2. Theme compatibility: Test with both dark and light terminal themes
3. Multiple windows: Verify each window independently shows/hides overlay based on its own focus state
4. Performance: Ensure no performance degradation during focus changes (should be negligible with current implementation)

🔒 Security & Robustness

No security concerns. The implementation is safe and doesn't introduce any vulnerabilities.

📊 Verdict

Recommendation: ✅ APPROVE with minor suggestions

This is a well-implemented feature that adds value to the user experience. The code is clean, follows best practices, and integrates seamlessly with the existing codebase. The minor observations above are suggestions for potential refinements, not blockers.

The feature aligns well with the project's goals of creating a modern, user-friendly terminal emulator. Great work!

---

Note: Don't forget to fix the indentation on line 222 before merging.

---

Review generated with assistance from Claude Code

[claude]

Additional Technical Review

I've reviewed this PR in detail and want to provide additional technical insights beyond the previous review.

✅ Implementation Quality

The implementation is solid overall. The approach of using a Box overlay is correct and performant.

🔍 Critical Technical Issues

1. Incorrect Use of derivedStateOf ⚠️

Current Code (Line 86):

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Issue: This is a misuse of derivedStateOf. The function is meant to derive state from other observable State objects, not to wrap lambda calls.

• derivedStateOf expects its lambda to read from State objects (using .value)
• It won't automatically track changes to isWindowFocused() since that lambda doesn't read any State
• This will only recompute on random recompositions, not when focus actually changes

Why it might seem to work: Incidental recompositions may make it appear functional, but it won't reliably react to focus changes.

Correct Implementation:

Since isWindowFocused is already a lambda parameter () -> Boolean, you should directly read the state it accesses:

// In Main.kt, isWindowFocused is passed as { window.isWindowFocused.value }
// So in TabbedTerminal, just read it directly:
val isWindowFocusedState = isWindowFocused()

Or if you need it to be a State object for composition observation:

// Better: Pass the State directly instead of a lambda
// Change signature to: isWindowFocused: State<Boolean>
val isWindowFocusedState by isWindowFocused

Recommendation: Either:

1. Read isWindowFocused() directly in the overlay condition (simplest)
2. Change the parameter from () -> Boolean to State<Boolean> and use delegation

2. Indentation Bug (Line 222)

if (tabController.tabs.size > 1) {
TabBar(  // ← Missing indentation

Should be:

if (tabController.tabs.size > 1) {
    TabBar(

🎨 Design Considerations

3. Overlay Color for Light Themes

The white overlay (Color.White.copy(alpha = 0.15f)) works well for dark themes but may have poor visibility on light backgrounds.

Suggestions:

• Consider using Color.Black.copy(alpha = 0.15f) for light themes
• Or use a desaturation effect instead of overlay
• Future enhancement: Add unfocusedOverlayColor setting

4. Accessibility Consideration

The 15% opacity overlay is subtle. Some users with visual impairments may prefer a more pronounced indicator. Consider:

• Making opacity configurable (unfocusedOverlayOpacity setting)
• Document the feature in accessibility guidelines

🧪 Testing Gaps

The PR description mentions split panes testing, but the previous review may have missed:

1. Focus tracking during rapid window switching: Does isWindowFocused.value update synchronously enough?
2. Multiple BossTerm windows: Each window should independently track its focus
3. Overlay z-index: Verify overlay doesn't block mouse events (it shouldn't with current implementation, but worth testing)

📝 Minor: Font Rendering Changes

The PR also includes font rendering changes in TerminalCanvasRenderer.kt:

// Line 820-836: Reordered font selection logic

This appears to be an unrelated bugfix for emoji rendering. While the change looks correct, it should ideally be in a separate commit or documented in the PR description. The logic now correctly prioritizes:

1. Emoji with variation selector → system font
2. Emoji/symbols without variation selector → terminal font
3. Math symbols → STIX font with fallback

This is good, but mixing features in one PR makes reviews harder and git history less clear.

🎯 Verdict

Status: Approve with Required Fix ⚠️

The feature is valuable and well-implemented, but the derivedStateOf usage must be fixed before merging. This isn't a style issue—it's a functional bug that will cause unreliable behavior.

📋 Before Merge Checklist

• Fix derivedStateOf misuse (line 86)
• Fix indentation (line 222)
• Test focus tracking works reliably after the fix
• Consider splitting font rendering changes to separate PR (optional)
• Add unfocusedOverlayOpacity setting for accessibility (optional future enhancement)

---

Great feature addition! The visual indicator will significantly improve multi-window workflows. Just need that one critical fix.

🤖 Technical review by Claude Sonnet 4.5

[claude]

Code Review: Unfocused Window Overlay Indicator

Summary

This PR adds a visual indicator (15% white overlay) when the terminal window loses focus, making it easier to identify the active window. The implementation is clean and follows the project's patterns well.

---

✅ Strengths

1. Minimal, Focused Changes: Only 47 additions/6 deletions across 3 files - very surgical implementation
2. User Configurability: Respects user preferences via showUnfocusedOverlay setting (default: true)
3. Reactive Design: Uses derivedStateOf for efficient state tracking without unnecessary recompositions
4. Proper Layer Ordering: Overlay correctly placed as the top layer in the Box hierarchy
5. Follows Project Conventions: Consistent with existing code style and Compose patterns

---

🔍 Code Quality Observations

TabbedTerminal.kt

• ✅ Good: Uses Box wrapper to layer overlay on top of existing UI
• ✅ Good: derivedStateOf prevents unnecessary recompositions
• ✅ Good: Overlay only renders when window is unfocused AND setting is enabled

TerminalCanvasRenderer.kt

• ⚠️ Mixed Change: The font rendering logic change (preferTerminalFontForSymbols) appears unrelated to the overlay feature
  • This change affects emoji/symbol font selection (Nerd Font vs system default)
  • Should this be in a separate PR? It's a distinct feature from the overlay indicator
  • The logic is sound, but mixing two unrelated features makes the PR harder to review and revert if needed

TerminalSettings.kt

• ✅ Good: Two new settings added with clear documentation
• ✅ Good: Sensible defaults (both true)
• ⚠️ Note: preferTerminalFontForSymbols is unrelated to the overlay feature (see above)

---

🐛 Potential Issues

1. Performance: Reactive State Tracking

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

• Issue: isWindowFocused() is called on every recomposition to check if the derived state needs updating
• Impact: Could be inefficient if isWindowFocused is not a Compose State object
• Question: Is isWindowFocused a () -> Boolean lambda that reads from a State/MutableState, or is it a regular function?
• Recommendation: If isWindowFocused doesn't read from Compose State internally, this derivedStateOf won't react to focus changes. Consider verifying this works as expected in testing.

2. Accessibility: Overlay Color Choice

.background(Color.White.copy(alpha = 0.15f))

• Issue: White overlay assumes a dark terminal background
• Impact: On light-colored terminal themes, a white overlay might be barely visible or increase brightness instead of dimming
• Recommendation: Consider using a semi-transparent black overlay (Color.Black.copy(alpha = 0.15f)) or making the overlay color theme-aware
  • Dark themes: White overlay (current)
  • Light themes: Black overlay
  • Or use a configurable overlay color in settings

3. Split Panes: Overlay Scope

• Observation: The overlay is applied to the entire TabbedTerminal Box
• Question: With split panes, does the overlay cover all panes when the window loses focus?
• Expected Behavior: Entire window should dim (which this implementation does)
• Recommendation: Verify this works correctly with split panes in testing (mentioned in test plan ✅)

---

🔒 Security Considerations

• ✅ No security concerns identified
• No user input handling or external data processing
• Pure UI rendering change

---

⚡ Performance Considerations

• ✅ Minimal Overhead: Single Box with background modifier only renders when unfocused
• ✅ Efficient Composables: No expensive operations in the overlay rendering path
• ⚠️ State Reactivity: Verify derivedStateOf actually reacts to focus changes (see Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1 above)

---

🧪 Test Coverage

• ⚠️ No Automated Tests: No unit/integration tests added
• ✅ Manual Test Plan: Comprehensive checklist provided in PR description
• Recommendation: Consider adding a simple test to verify:
  • Overlay renders when isWindowFocused = false and showUnfocusedOverlay = true
  • Overlay doesn't render when showUnfocusedOverlay = false

---

📋 Recommendations

High Priority

1. Separate PRs for Unrelated Features

   • Move preferTerminalFontForSymbols logic to a separate PR titled "feat: Add terminal font preference for symbols"
   • Keep this PR focused solely on the unfocused overlay indicator
   • Rationale: Single Responsibility Principle, easier review, easier rollback

2. Verify Reactive State Behavior

   • Test that the overlay actually appears/disappears when focus changes
   • If isWindowFocused() doesn't read from Compose State, the derivedStateOf won't trigger recomposition
   • May need to pass isWindowFocused as a State parameter instead of a lambda

3. Consider Overlay Color for Light Themes

   • Test on light terminal themes
   • Consider theme-aware overlay color or make it configurable

Medium Priority

4. Add Documentation Comment

   • Add a comment explaining the Box wrapper's purpose:

   // Wrap in Box to layer overlay on top when window loses focus
   Box(modifier = modifier.fillMaxSize()) {

5. Settings Default Values

   • Consider making showUnfocusedOverlay default to false for first release to avoid surprising users
   • Can enable by default in a future release after gathering feedback

Low Priority

6. Parameterize Overlay Opacity
   • Consider adding unfocusedOverlayOpacity: Float = 0.15f setting for user customization
   • Some users might prefer stronger/weaker dimming

---

🎯 Verdict

Overall Assessment: Good implementation with minor concerns

Recommendation:

• ✅ Approve with changes requested
• Split preferTerminalFontForSymbols into separate PR
• Verify reactive state behavior works correctly
• Test overlay visibility on light terminal themes

The core overlay feature is well-implemented and follows project conventions. The main issue is mixing two unrelated features in one PR. Once separated, this will be a clean, focused change.

---

📝 Testing Checklist Before Merge

• Verify overlay appears when clicking away from window
• Verify overlay disappears when clicking back to window
• Test with multiple tabs - overlay should cover entire window
• Test with split panes - overlay should cover all panes
• Test disabling showUnfocusedOverlay - overlay should not appear
• Test on light terminal theme - verify overlay is visible
• Test that focus tracking doesn't cause performance issues

---

Great work on a useful feature! 🎉 The implementation is solid, just needs the font rendering change split out and a few minor verifications.

[claude]

Code Review: Unfocused Window Overlay Indicator

Summary

This PR adds a visual indicator (semi-transparent white overlay) when the terminal window loses focus, along with improved symbol font rendering using a bundled Noto Sans Symbols 2 font. Overall, this is a well-implemented feature with good attention to detail.

---

✅ Strengths

1. Clean Implementation

• The overlay implementation is simple and non-invasive
• Proper use of Compose's derivedStateOf for reactive state tracking
• Settings-based toggle (showUnfocusedOverlay) gives users control
• Follows existing code patterns in the codebase

2. Font Loading Pattern

• Reuses the proven InputStream + temp file approach documented in CLAUDE.md
• Proper error handling with fallback to FontFamily.Default
• Lazy initialization for performance

3. Good Code Quality

• Clear comments explaining the purpose of each section
• Proper resource cleanup (deleteOnExit() for temp files)
• No backwards-compatibility hacks

---

🔍 Areas of Concern

1. Performance: Recomposition Trigger (Medium Priority)

Location: TabbedTerminal.kt:86

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Issue: This will trigger recomposition of the entire TabbedTerminal composable every time isWindowFocused() changes. While the overlay itself is lightweight, this could cause unnecessary work in the rest of the UI tree.

Recommendation: Consider using a MutableState that's updated only when focus changes, rather than polling via derivedStateOf:

val isWindowFocusedState = remember { mutableStateOf(isWindowFocused()) }

// Update via a callback or LaunchedEffect when focus changes
LaunchedEffect(Unit) {
    // If isWindowFocused is observable, subscribe to changes
}

Impact: Low-medium - unlikely to cause visible performance issues, but worth optimizing for best practices.

---

2. Font Loading: Resource Leak Risk (Low Priority)

Location: FontUtils.kt:131-137

Issue: The temp file created for NotoSansSymbols2-Regular.ttf relies on deleteOnExit(), which only cleans up when the JVM terminates. In long-running sessions, this could accumulate if the font is reloaded multiple times (though lazy prevents this in practice).

Recommendation: Consider using java.nio.file.Files.createTempFile() with StandardOpenOption.DELETE_ON_CLOSE for more aggressive cleanup, or verify that the lazy initialization prevents multiple loads.

Impact: Very low - the lazy wrapper already prevents multiple temp file creation.

---

3. Color Hardcoding (Low Priority)

Location: TabbedTerminal.kt:372

.background(Color.White.copy(alpha = 0.15f))

Issue: The white color is hardcoded, which may not work well with all color schemes (especially light themes where the distinction might be less visible).

Recommendation: Consider making the overlay color and opacity configurable via settings:

// In TerminalSettings.kt
val unfocusedOverlayColor: Color = Color.White
val unfocusedOverlayAlpha: Float = 0.15f

// In TabbedTerminal.kt
.background(settings.unfocusedOverlayColor.copy(alpha = settings.unfocusedOverlayAlpha))

Or use a dynamic approach based on the current theme (white for dark themes, black for light themes).

Impact: Low - mostly a UX polish issue.

---

4. Symbol Font Logic Clarity (Low Priority)

Location: TerminalCanvasRenderer.kt:823-831

Issue: The logic for when to use bundledSymbolFont vs FontFamily.Default is a bit complex and depends on platform-specific behavior. The comment mentions "works on Linux" but doesn't explain why macOS should use FontFamily.Default.

Recommendation: Add more detailed comments explaining:

• Why Noto Sans Symbols 2 is preferred on Linux (lack of system font coverage)
• Why macOS should use Apple Color Emoji
• What happens on Windows

Impact: Very low - documentation clarity only.

---

5. Missing Binary File Check

Issue: The PR includes a binary font file (NotoSansSymbols2-Regular.ttf) with no indication of:

• File size (for repository bloat concerns)
• License/attribution
• Version of Noto Sans Symbols 2

Recommendation: Add a comment or documentation noting:

• Font license (Noto fonts are Apache 2.0 or OFL)
• Font version/source
• File size (to track repository growth)

---

🔒 Security Review

✅ No security concerns identified:

• No XSS, SQL injection, or command injection vectors
• No user input processing in this PR
• Font loading uses standard Java APIs with proper error handling
• No network requests or external data sources

---

🧪 Test Coverage

Missing Tests:

• No unit tests for loadBundledSymbolFont() function
• No tests for overlay visibility logic
• No tests for preferTerminalFontForSymbols setting

Recommendation: Add tests for:

1. Font loading fallback behavior (when resource not found)
2. Overlay visibility based on focus state and setting toggle
3. Symbol font selection logic in TerminalCanvasRenderer

However, given that this is a UI feature and the codebase appears to rely on manual testing (per CLAUDE.md), this is acceptable for now.

---

📊 Performance Considerations

✅ Generally good:

• Lazy font loading prevents startup overhead
• Overlay is a simple box with solid color (very fast to render)
• Settings are cached (no repeated file I/O)

⚠️ Minor concern: See issue #1 above about recomposition scope.

---

🎯 Final Verdict

Recommendation: APPROVE with minor suggestions

This is a solid implementation of a useful feature. The code follows existing patterns, has proper error handling, and introduces no security risks. The performance concerns are minor and can be addressed in follow-up work if needed.

Suggested Follow-ups (Optional):

1. Optimize isWindowFocusedState to reduce recomposition scope
2. Make overlay color/opacity configurable for different themes
3. Add documentation for the bundled Noto Sans Symbols 2 font (license, version)

---

Test Plan Checklist

Before merging, please verify:

• Overlay appears when clicking away from BossTerm
• Overlay disappears when clicking back to BossTerm
• Overlay works correctly with multiple tabs
• Overlay works correctly with split panes
• Setting toggle (showUnfocusedOverlay) works as expected
• Symbol rendering works correctly on Linux and macOS
• No performance degradation during focus changes

---

Great work on this feature! 🚀

🤖 Generated with Claude Code

[claude]

Code Review for PR #114: Unfocused Window Overlay Indicator

Summary

This PR adds visual feedback when the terminal window loses focus and improves cross-platform symbol rendering. The implementation is generally solid with good architectural decisions. Below are my findings:

---

✅ Strengths

1. Window Focus Overlay

• Clean implementation using Compose's reactive state with derivedStateOf
• Proper use of Box layout to layer the overlay without disrupting existing UI
• Appropriate opacity (15%) - subtle but noticeable
• User-controllable via showUnfocusedOverlay setting

2. Font System Enhancement

• Excellent lazy loading pattern for bundledSymbolFont
• Proper error handling with fallback to FontFamily.Default
• Good separation of concerns with dedicated loadBundledSymbolFont()
• Follows existing font loading patterns (consistent with loadTerminalFont)

3. Cross-Platform Symbol Support

• Addresses real Linux font coverage issues
• Configurable via preferTerminalFontForSymbols setting
• Maintains macOS Apple Color Emoji support when disabled

---

⚠️ Issues & Concerns

1. Critical: Binary Font File Size (11 MB)

Severity: High - Performance & Distribution Impact

The NotoColorEmoji-Regular.ttf file is 11 MB, which:

• Increases application bundle size significantly (4.4x larger than MesloLGS)
• Will be loaded into memory on first use (lazy loading helps, but still occurs)
• May cause startup delay on first symbol render
• Impacts CI/CD pipeline (larger artifacts, slower downloads)

Recommendation:

• Consider subset font generation (include only needed symbols: ⏵ ★ ⚡ ⚠️ ⏸ ⏹ etc.)
• Tools like pyftsubset (fonttools) can reduce to <500 KB
• Alternative: Use Noto Sans Symbols 2 (641 KB, mentioned in commit) for basic symbols

Command for subsetting:

pyftsubset NotoColorEmoji-Regular.ttf \
  --unicodes="U+23F5,U+2605,U+26A1,U+26A0,U+23F8,U+23F9" \
  --output-file=NotoColorEmoji-Subset.ttf

2. Moderate: Font Loading Performance

Severity: Moderate - Runtime Performance

loadBundledSymbolFont() creates a temp file on every application launch:

• I/O overhead: 11 MB read + write to temp directory
• Happens on first symbol render (due to lazy initialization)
• May cause noticeable delay on slower storage (HDDs, network drives)

Current mitigation: Lazy loading delays impact until first use ✅

Potential improvement:

• Cache the temp file path/reuse across sessions (check if file exists before creating)
• Use Skiko's direct memory loading if supported (avoid temp file)

3. Minor: Outdated Comment

Severity: Low - Documentation Accuracy

Line 826 in TerminalCanvasRenderer.kt:

// Use bundled Noto Sans Symbols 2 - works on Linux
ai.rever.bossterm.compose.util.bundledSymbolFont

Issue: Comment mentions "Noto Sans Symbols 2" but code uses "Noto Color Emoji"

Fix: Update to:

// Use bundled Noto Color Emoji - works on Linux

4. Minor: Missing Fallback Test

Severity: Low - Error Handling

If font loading fails, fallback to FontFamily.Default is silent (only System.err):

• Users won't know if symbols are broken due to font load failure
• No logging for production debugging

Recommendation:

• Add structured logging (if available in project)
• Consider metrics/telemetry for font load failures

---

🔍 Code Quality & Best Practices

Architecture ✅

• Follows existing patterns (e.g., loadTerminalFont structure)
• Proper separation: FontUtils for loading, TerminalCanvasRenderer for rendering
• Reactive state management with derivedStateOf is idiomatic Compose

Error Handling ✅

• Try-catch with fallback in loadBundledSymbolFont()
• Graceful degradation (uses system font on failure)

Settings Design ✅

• Sensible defaults (showUnfocusedOverlay = true, preferTerminalFontForSymbols = true)
• Clear documentation in TerminalSettings.kt

Compose Best Practices ✅

• Proper use of remember for font loading
• Overlay implemented as sibling in Box (correct z-ordering)
• No unnecessary recomposition triggers

---

🧪 Testing Considerations

Recommended Test Cases:

1. Font Loading:

   • Test on Linux with missing system fonts
   • Test font load failure scenario
   • Verify fallback to FontFamily.Default

2. Overlay Behavior:

   • Test with multiple windows (focus switching)
   • Test with setting disabled (showUnfocusedOverlay = false)
   • Test overlay doesn't interfere with input events

3. Symbol Rendering:

   • Test symbols with preferTerminalFontForSymbols = true/false
   • Test emoji with variation selectors (☁️ vs ☁)
   • Test on macOS vs Linux rendering differences

4. Performance:

   • Measure first-render delay with 11 MB font
   • Test memory usage after font load
   • Test application startup time impact

---

🔒 Security Review

Font File Integrity ✅

• Using official Google Noto font (Apache 2.0 license)
• Binary file added to repo (standard practice for fonts)

Temp File Handling ✅

• tempFile.deleteOnExit() ensures cleanup
• No hardcoded paths or path traversal risks

Resource Loading ✅

• Uses safe getResourceAsStream (no user input)
• Null-safe with elvis operator

---

📊 Performance Impact Summary

Aspect	Impact	Severity
Bundle size increase	+11 MB	High
First symbol render delay	~100-300ms (estimated)	Moderate
Memory usage	+11 MB (loaded font)	Moderate
Runtime performance	Negligible (after load)	Low

---

🎯 Recommendations Priority

High Priority:

1. ⚠️ Subset the font file to reduce from 11 MB to <1 MB
   • Only include needed Unicode ranges
   • Or revert to Noto Sans Symbols 2 (641 KB) for basic symbols

Medium Priority:

2. Fix outdated comment in TerminalCanvasRenderer.kt:826
3. Add performance testing for font loading delay

Low Priority:

4. Add structured logging for font load failures
5. Consider font caching strategy (reuse temp file across launches)

---

✅ Approval Status

Overall Assessment: Good implementation with one critical concern (font file size).

Recommendation: Request changes to address font size before merge. The feature itself is well-implemented and follows project conventions.

---

Suggested Next Steps:

1. Subset font to include only needed symbols (high priority)
2. Update comment to reflect Noto Color Emoji usage
3. Test on Linux to verify symbol rendering works as expected
4. Consider adding font load timing metrics for monitoring

Great work on the cross-platform compatibility improvements! 🚀

[claude]

Code Review - PR #114: Unfocused Window Overlay Indicator

✅ Overall Assessment

This is a well-implemented feature with good cross-platform considerations. The code quality is solid, follows the project conventions, and addresses a real UX need. I have a few suggestions for improvement.

---

🎯 Strengths

1. Clean Architecture: The focus state tracking is properly separated and uses Compose's reactive patterns
2. Configurable: The showUnfocusedOverlay setting gives users control
3. Cross-Platform Font Handling: The bundled Noto Sans Symbols 2 font addresses Linux symbol rendering issues
4. Proper Resource Management: Font loading uses temp files with deleteOnExit() as per project patterns
5. Good Commit History: Clear, atomic commits with proper co-authoring

---

🔍 Code Quality Observations

TabbedTerminal.kt (Lines 85-86)

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Concern: This creates a derived state that calls a function on every recomposition. Since isWindowFocused() is already a () -> Boolean lambda, calling it repeatedly in derivedStateOf may be inefficient.

Recommendation: Consider using snapshotFlow or making isWindowFocused provide a State<Boolean> instead:

// Option 1: Direct state usage (preferred if isWindowFocused can return State)
val isWindowFocusedState = isWindowFocused()

// Option 2: If isWindowFocused must remain a lambda, use LaunchedEffect
val isWindowFocusedState = remember { mutableStateOf(isWindowFocused()) }
LaunchedEffect(Unit) {
    snapshotFlow { isWindowFocused() }.collect { isWindowFocusedState.value = it }
}

TabbedTerminal.kt (Lines 368-373)

if (!isWindowFocusedState && settings.showUnfocusedOverlay) {
    Box(modifier = Modifier.fillMaxSize().background(Color.White.copy(alpha = 0.15f)))
}

Observation: The overlay is positioned inside a Box wrapping the entire Column. This is correct for layering, but worth noting for future maintainers.

Minor Suggestion: Consider extracting the overlay color to a constant or theme:

private val UnfocusedOverlayColor = Color.White.copy(alpha = 0.15f)

---

🐛 Potential Issues

1. Font Loading Performance (FontUtils.kt:118-148)

The bundledSymbolFont is lazily loaded, which is good, but:

• Issue: If font loading fails, it falls back to FontFamily.Default, but the error is only printed to stderr
• Impact: Users won't know why symbols aren't rendering properly on Linux
• Recommendation: Consider logging to the application's log system or surfacing the error in the UI

2. Binary Font File Licensing (NotoSansSymbols2-Regular.ttf)

• Size: 641KB binary added to resources
• License: Apache 2.0 (confirmed in commit message)
• Recommendation: Add a THIRD_PARTY_LICENSES.md file or include license attribution in the README.md to comply with Apache 2.0 requirements

3. Settings Migration (TerminalSettings.kt)

• Issue: New settings preferTerminalFontForSymbols and showUnfocusedOverlay default to true
• Impact: Existing users' settings files won't have these keys, so defaults will apply (which is fine)
• Observation: No migration code needed since data class defaults handle this gracefully

---

⚡ Performance Considerations

1. Overlay Rendering (TabbedTerminal.kt:368-373)

• Current: Full-screen overlay redraws on every focus change
• Performance: Should be negligible since it's a simple transparent box
• Optimization Opportunity: If profiling shows issues, consider using Modifier.graphicsLayer { alpha = ... } instead of background() for hardware acceleration

2. Font Selection Per Character (TerminalCanvasRenderer.kt:820-839)

• Current: Font selection logic runs for every character during rendering
• Impact: The if/else chain is evaluated thousands of times per frame
• Recommendation: Consider caching font selections or using a lookup table if profiling shows this as a hotspot

---

🔒 Security Considerations

✅ No security concerns identified

• Font loading uses standard Compose APIs
• No user input is processed unsafely
• Settings are read-only at runtime (loaded from JSON)
• Overlay rendering is purely visual

---

🧪 Test Coverage

Missing: No automated tests for new features

• Recommendation: Add tests for:
  1. Font loading fallback behavior (when font file is missing)
  2. Overlay visibility based on focus state and settings
  3. Font selection logic for different character types

Test Plan: The PR's manual test plan is comprehensive:

• ✅ Focus/unfocus overlay behavior
• ✅ Multi-tab testing
• ✅ Settings toggle

Manual Testing Needed:

• Verify symbol rendering on Linux (⏵ ★ ⚡)
• Test with preferTerminalFontForSymbols = false on macOS
• Verify no performance regression with large terminal output

---

📝 Documentation

Missing: Documentation for new settings

• Recommendation: Update CLAUDE.md or user-facing docs to document:
  • showUnfocusedOverlay: What it does, why users might disable it
  • preferTerminalFontForSymbols: Platform-specific recommendations (true for Linux, false for macOS)

---

🎨 Code Style & Best Practices

✅ Follows project conventions:

• Uses project's font loading pattern (InputStream + temp file)
• Consistent with existing settings structure
• Proper use of Compose's remember, derivedStateOf, and LaunchedEffect

⚠️ Minor Nit: Line 827 uses fully qualified name ai.rever.bossterm.compose.util.bundledSymbolFont

• Recommendation: Add import to avoid FQN:

import ai.rever.bossterm.compose.util.bundledSymbolFont
// Then use: bundledSymbolFont

---

🚀 Recommendations Summary

Must Fix Before Merge:

• Add Apache 2.0 license attribution for Noto Sans Symbols 2 font

Should Fix:

• Optimize isWindowFocusedState derivation (avoid repeated function calls)
• Extract overlay color to constant or theme

Nice to Have:

• Add automated tests for font loading and overlay behavior
• Document new settings in user-facing docs
• Add import for bundledSymbolFont to avoid FQN
• Profile font selection logic if rendering performance regresses

---

🎉 Conclusion

This is a solid PR that addresses real UX needs (focus indication) and cross-platform rendering issues (Linux symbol support). The implementation is clean and follows project conventions. With minor improvements (license attribution, state optimization), this is ready to merge.

Rating: 8.5/10

Great work! 🚀

[claude]

Code Review

This PR adds two features: (1) unfocused window overlay and (2) platform-specific symbol font rendering. Overall the implementation is solid and follows project conventions, but I have some observations and suggestions.

---

✅ Strengths

1. Good separation of concerns: Overlay logic cleanly separated in TabbedTerminal.kt
2. Follows existing patterns: Uses the established settings system and lazy font loading approach
3. Platform awareness: Smart macOS vs Linux detection for font selection
4. Error handling: Proper fallbacks in font loading with error messages
5. Resource management: Temp files properly marked with deleteOnExit()
6. Documentation: Good inline comments explaining the logic

---

🔍 Issues & Suggestions

1. Performance: Overlay Recomposition

Location: TabbedTerminal.kt:86

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Issue: derivedStateOf creates a computed state, but isWindowFocused() is a function call that reads from MutableState<Boolean>. This should just read the state directly:

val isWindowFocusedState by isWindowFocused

Or simply use it inline:

if (!isWindowFocused.value && settings.showUnfocusedOverlay) {

Impact: Unnecessary recomposition overhead on every frame when the value hasn't changed.

---

2. Font Loading: Resource Leak Risk

Location: FontUtils.kt:131-137

Issue: If FontFamily() constructor throws an exception after creating the temp file, the file won't be deleted. The deleteOnExit() hook only works for graceful JVM shutdown.

Suggestion: Use try-finally or Kotlin's use pattern:

private fun loadBundledSymbolFont(): FontFamily {
    return try {
        val fontStream = object {}.javaClass.classLoader
            ?.getResourceAsStream("fonts/NotoSansSymbols2-Regular.ttf")
            ?: return FontFamily.Default

        val tempFile = java.io.File.createTempFile("NotoSansSymbols2", ".ttf")
        try {
            tempFile.deleteOnExit()
            fontStream.use { input ->
                tempFile.outputStream().use { output ->
                    input.copyTo(output)
                }
            }

            FontFamily(
                androidx.compose.ui.text.platform.Font(
                    file = tempFile,
                    weight = FontWeight.Normal
                )
            )
        } catch (e: Exception) {
            tempFile.delete()  // Clean up immediately on failure
            throw e
        }
    } catch (e: Exception) {
        System.err.println("Failed to load bundled symbol font: ${e.message}")
        FontFamily.Default
    }
}

---

3. Platform Detection: Runtime Check on Hot Path

Location: TerminalCanvasRenderer.kt:820

val isMacOS = System.getProperty("os.name")?.lowercase()?.contains("mac") == true

Issue: This system property lookup happens every time a symbol is rendered (potentially thousands of times per second during scrolling).

Suggestion: Move to a top-level constant or lazy property:

private val isMacOS: Boolean by lazy {
    System.getProperty("os.name")?.lowercase()?.contains("mac") == true
}

Or cache it in the rendering context.

---

4. Font Loading: Memory Leak Potential

Location: FontUtils.kt:118

val bundledSymbolFont: FontFamily by lazy {
    loadBundledSymbolFont()
}

Observation: The lazy delegate holds a reference to the FontFamily and temp file for the entire application lifetime. This is acceptable for a single font, but worth documenting.

Minor suggestion: Add a comment explaining this is intentional:

/**
 * Lazily loaded bundled symbol font (Noto Sans Symbols 2).
 * Loaded once and cached for application lifetime.
 * The temp file is marked deleteOnExit() and will be cleaned up on JVM shutdown.
 */
val bundledSymbolFont: FontFamily by lazy {
    loadBundledSymbolFont()
}

---

5. Overlay Rendering: Z-Index Ambiguity

Location: TabbedTerminal.kt:368-373

Observation: The overlay is rendered after the terminal content in the composition tree. While this typically works correctly (later items render on top), it's not guaranteed in all Compose scenarios.

Suggestion: Explicitly set z-index if ordering issues arise:

.background(Color.White.copy(alpha = 0.15f))
.zIndex(10f)  // Ensure overlay is on top

---

6. Settings: Missing Documentation

Location: TerminalSettings.kt:52-56

Observation: The preferTerminalFontForSymbols setting comment says "on macOS, this setting has no effect", but this isn't obvious to users.

Suggestion: Consider adding a platform check in settings validation or UI (future enhancement):

val preferTerminalFontForSymbols: Boolean = 
    if (System.getProperty("os.name")?.contains("mac", ignoreCase = true) == true) {
        false  // Always use system font on macOS
    } else {
        true   // Default to bundled font on Linux
    }

---

7. Binary File in Git

Location: compose-ui/src/desktopMain/resources/fonts/NotoSansSymbols2-Regular.ttf

Observation: Adding a 641KB binary file to the repository. This is acceptable for a font resource, but:

• Verify the Apache 2.0 license is compatible with your project
• Consider adding a fonts/LICENSE or fonts/README.md documenting the font source and license
• This increases repository clone size permanently (Git doesn't compress binaries well)

Suggestion: Add attribution file:

fonts/README.md:
# Fonts

## Noto Sans Symbols 2
- **Source**: https://github.com/notofonts/noto-fonts
- **License**: Apache 2.0
- **Version**: 2.008
- **Purpose**: Fallback for symbol rendering on Linux

---

🎯 Testing Recommendations

1. Overlay behavior:

   • Test rapid focus/unfocus cycles (does it flicker?)
   • Test with multiple tabs
   • Test with split panes
   • Verify overlay doesn't block input events

2. Font rendering:

   • Test on actual Linux system (not just macOS)
   • Verify symbols render correctly: ⏵ ★ ⚡ ☁️ ⭐ ❤️
   • Test fallback when bundled font fails to load
   • Verify no performance regression during scrolling

3. Settings persistence:

   • Verify showUnfocusedOverlay and preferTerminalFontForSymbols save/load correctly
   • Test default values for new users

4. Edge cases:

   • Window minimized (is it considered unfocused?)
   • Window on inactive virtual desktop
   • Multiple BossTerm windows (does each track focus correctly?)

---

🔒 Security

No security concerns identified. Font loading uses standard Java resource APIs and temp file creation is properly scoped.

---

📊 Performance

• Overlay: Minimal cost (one additional Box with solid color)
• Font loading: Lazy initialization is good, but see issue 🔗 Implement hyperlink detection and click handling #3 about hot path
• Memory: ~641KB for bundled font + temp file (acceptable)

---

🧪 Test Coverage

Missing: No unit tests for:

• Font loading fallback behavior
• Platform detection logic
• Overlay visibility logic

Suggestion: Add tests for loadBundledSymbolFont() error paths and platform detection.

---

✏️ Minor Nits

1. Formatting: Some inconsistent spacing in TerminalCanvasRenderer.kt:821-835 (minor)
2. Naming: isWindowFocusedState is redundant (State suffix) - just isWindowFocused or windowFocused would be clearer
3. Import organization: TabbedTerminal.kt has imports in non-standard order (grouped by package, but not alphabetized)

---

📝 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐ (4/5)	Well-structured, follows conventions
Bugs	⭐⭐⭐⭐ (4/5)	No critical bugs, minor resource leak risk
Performance	⭐⭐⭐ (3/5)	Hot path platform detection needs optimization
Security	⭐⭐⭐⭐⭐ (5/5)	No concerns
Tests	⭐⭐ (2/5)	Missing unit tests

Recommendation: ✅ Approve with minor fixes

The implementation is production-ready, but I'd recommend addressing issues #1, #3, and #7 before merging. Issues #2, #4, #5, #6 can be follow-up improvements.

Great work on the platform-specific font handling! The overlay feature will definitely improve UX for multi-window workflows.

[claude]

Code Review - PR #114: Unfocused Window Overlay Indicator

Summary

This PR adds two main features:

1. Unfocused window overlay: A semi-transparent white overlay (15% opacity) when the window loses focus
2. Symbol font customization: Adds Noto Sans Symbols 2 as a bundled font with platform-aware defaults and user-configurable options

---

✅ Code Quality & Best Practices

Strengths:

• Clean separation of concerns with dedicated settings for each feature
• Follows existing codebase patterns (lazy font loading, settings structure)
• Platform-aware defaults (macOS vs Linux) show good attention to UX
• Proper use of Compose remember and derivedStateOf for reactive state

Minor Concerns:

1. Missing UI for overlay toggle: The showUnfocusedOverlay setting is defined in TerminalSettings.kt but there's no corresponding UI control in VisualSettingsSection.kt to toggle it. Users can only change it via JSON editing.

   • Fix: Add a SettingsToggle in VisualSettingsSection.kt:

   SettingsToggle(
       label = "Show Unfocused Window Overlay",
       checked = settings.showUnfocusedOverlay,
       onCheckedChange = { onSettingsChange(settings.copy(showUnfocusedOverlay = it)) },
       description = "Semi-transparent overlay when window loses focus"
   )

2. Font loading error handling: loadBundledSymbolFont() catches exceptions but only logs to stderr. Consider whether silent fallback to FontFamily.Default is sufficient, or if a user-visible warning is needed.

---

🐛 Potential Bugs

Critical:

1. Overlay z-order with dialogs/menus: The overlay is applied at the root Box level in TabbedTerminal.kt. If modal dialogs or context menus are rendered as children of this Box, they will also be dimmed by the overlay. This could confuse users.

   • Impact: Medium - May affect usability if dialogs appear while window is unfocused
   • Recommendation: Test with search dialog, context menu, and settings panel open while window loses focus

2. Font resource path: The font is loaded from fonts/NotoSansSymbols2-Regular.ttf using classloader resource stream. Verify this works correctly in both:

   • Development (./gradlew run)
   • Production (packaged app)
   • Test command: ./gradlew :compose-ui:createDistributable and verify font loads in packaged app

Minor:
3. Nullable Boolean logic: The preferTerminalFontForSymbols setting uses Boolean? (nullable) which adds cognitive complexity. The logic \!ctx.settings.preferTerminalFontForSymbols\!\! uses double-bang which could NPE if settings parsing fails.

• Current: null = platform default, true = bundled, false = system
• Safer approach: Use non-null Boolean with explicit "auto" enum, or add null-safety:

val useSystemFont = when (ctx.settings.preferTerminalFontForSymbols) {
    null -> isMacOS  // Platform default
    true -> false    // Bundled font
    false -> true    // System font
}

---

⚡ Performance Considerations

Good:

• Font loading is lazy (by lazy) - only loads when first accessed
• Overlay uses simple Box with background() modifier - minimal overhead
• derivedStateOf for focus state prevents unnecessary recompositions

Potential Issue:

• Font switching on every character: In TerminalCanvasRenderer.kt, the font selection logic runs for every character drawn. While the actual font objects are cached, the conditional checks (if (isEmojiWithVariationSelector)...) execute per character.
  • Impact: Low - Modern CPUs handle this easily, but worth profiling during long file scrolls
  • Optimization (if needed): Cache font decisions per character class at a higher level

---

🔒 Security Concerns

Low Risk:

• Font loading uses temp files with deleteOnExit() - standard safe practice
• No user-controlled paths or external resources
• Binary font file (NotoSansSymbols2-Regular.ttf) should be verified:
  • Recommendation: Document font source and checksum in CLAUDE.md or README for supply chain transparency

---

🧪 Test Coverage

Missing Tests:
The PR description includes manual test plan but no automated tests. Consider adding:

1. Unit tests for loadBundledSymbolFont():

   • Successful load
   • Resource not found fallback
   • Exception handling

2. Integration tests for overlay:

   • Overlay visibility when isWindowFocused() == false
   • Overlay hidden when settings.showUnfocusedOverlay == false
   • Overlay color/opacity matches spec

3. Visual regression tests (if tooling exists):

   • Screenshot comparison with overlay enabled/disabled

---

📋 Additional Recommendations

1. Documentation:

   • Update CLAUDE.md with new font bundling pattern
   • Document overlay feature in user-facing docs

2. Accessibility:

   • 15% white overlay may not provide sufficient contrast on light backgrounds
   • Consider using a dark overlay (Color.Black.copy(alpha = 0.2f)) or making opacity/color configurable
   • Test with various terminal color schemes (Solarized Light, GitHub Light, etc.)

3. Consistency:

   • The symbol font feature is substantial enough to warrant its own PR
   • Consider splitting into two PRs: one for overlay, one for symbol font customization

---

🎯 Summary Rating

Category	Rating	Notes
Code Quality	🟢 Good	Clean, follows patterns
Bug Risk	🟡 Medium	Missing UI control, potential z-order issue
Performance	🟢 Good	No significant concerns
Security	🟢 Good	Standard practices followed
Test Coverage	🔴 Low	No automated tests

---

✏️ Action Items

Before Merge:

1. ✅ Add SettingsToggle for showUnfocusedOverlay in VisualSettingsSection.kt
2. ✅ Test overlay z-order with modal dialogs/menus
3. ✅ Verify font loading in packaged app (createDistributable)
4. ⚠️ Consider overlay color on light terminal themes (accessibility)

Post-Merge (Nice to Have):
5. Add unit tests for font loading
6. Document font source/checksum for supply chain security
7. Profile rendering performance with new font logic

---

🚀 Conclusion

This is a solid PR with useful features. The main blocker is the missing UI control for the overlay toggle. Once that's added and the z-order behavior is verified with dialogs, this should be safe to merge. The symbol font feature is well-implemented but consider splitting it into a separate PR for cleaner history.

Great work overall! 🎉

[claude]

Code Review - PR #114: Unfocused Window Overlay & Symbol Font Improvements

Great work on this feature! The implementation is well-structured and follows the project's patterns. Here's my detailed feedback:

---

✅ Strengths

1. Clean Architecture: The window focus overlay is elegantly implemented using Compose's Box layout with proper layering.

2. Platform Awareness: Smart platform-specific defaults (null value) for preferTerminalFontForSymbols is excellent design - macOS gets Apple Color Emoji, Linux gets bundled font.

3. User Control: The new Settings UI dropdown gives users full control over symbol font selection with clear labeling.

4. Font Loading Pattern: bundledSymbolFont follows the established lazy-loading pattern from loadTerminalFont(), consistent with CLAUDE.md guidelines.

5. Proper Error Handling: Font loading has try-catch with fallback to FontFamily.Default.

---

🐛 Issues & Concerns

1. Performance: Platform Detection in Render Loop (HIGH PRIORITY)

Location: TerminalCanvasRenderer.kt:820-826

val isMacOS = System.getProperty("os.name")?.lowercase()?.contains("mac") == true
val useSystemFontForEmoji = if (ctx.settings.preferTerminalFontForSymbols \!= null) {
    \!ctx.settings.preferTerminalFontForSymbols\!\!
} else {
    isMacOS
}

Problem: This code runs inside renderCharacter(), which is called for every character on screen (thousands of times per frame at 60fps). Platform detection is expensive and completely unnecessary in a hot loop.

Impact:

• ~80x24 = 1920 chars/frame × 60fps = 115,200 unnecessary system property lookups per second
• Even with adaptive debouncing (CLAUDE.md mentions 16-50ms intervals), this is still wasteful

Solution: Move platform detection to initialization:

// At class/object level or in RenderingContext
private val isMacOS = System.getProperty("os.name")?.lowercase()?.contains("mac") == true

// In renderCharacter(), simplified:
val useSystemFontForEmoji = when (ctx.settings.preferTerminalFontForSymbols) {
    null -> isMacOS  // Use cached platform check
    true -> false
    false -> true
}

2. Missing Setting in Visual Settings Section

Location: VisualSettingsSection.kt - Missing toggle for showUnfocusedOverlay

Problem: The PR adds showUnfocusedOverlay setting to TerminalSettings.kt:138 with a clear docstring:

"Show semi-transparent overlay when window loses focus. Helps identify which terminal window is currently active."

But there's no UI control in VisualSettingsSection.kt to toggle this feature on/off. Users can only change it by manually editing JSON.

Solution: Add a toggle above or below the Symbol Font dropdown:

SettingsToggle(
    label = "Show Unfocused Overlay",
    checked = settings.showUnfocusedOverlay,
    onCheckedChange = { onSettingsChange(settings.copy(showUnfocusedOverlay = it)) },
    description = "Semi-transparent white overlay when window loses focus"
)

3. Redundant Null Safety Operator

Location: TerminalCanvasRenderer.kt:823

val useSystemFontForEmoji = if (ctx.settings.preferTerminalFontForSymbols \!= null) {
    \!ctx.settings.preferTerminalFontForSymbols\!\!  // Double-bang after null check
} else {
    isMacOS
}

Issue: The \!\! operator is unnecessary after explicit null check. Use when for cleaner null handling (see solution in #1).

---

🔍 Potential Issues

4. Font Classification Logic Complexity

Location: TerminalCanvasRenderer.kt:827-850

The font selection logic is complex with overlapping conditions:

• isEmojiWithVariationSelector → Always FontFamily.Default
• isEmojiOrWideSymbol → Platform-dependent
• isTechnicalSymbol || isCursiveOrMath → STIX Two Math → terminal font

Questions:

1. Are isTechnicalSymbol and isCursiveOrMath mutually exclusive? The logic treats them identically.
2. Should technical symbols really fall through to bundledSymbolFont when preferTerminalFontForSymbols == true? This seems to contradict the commit message "fix: Restore original behavior for technical symbols on macOS" (commit 3fe7fd9).

Suggestion: Add comments explaining the classification hierarchy and rationale for each branch.

5. Font Loading Happens on First Render

Location: FontUtils.kt:115 - val bundledSymbolFont: FontFamily by lazy

Risk: If font loading throws an exception during first render, it could cause UI jank or delay. The lazy initialization is good, but consider:

1. Pre-loading fonts during app startup (before first render)
2. Logging when fallback occurs (currently only System.err.println)

6. Missing Font License Documentation

Added File: NotoSansSymbols2-Regular.ttf (641KB)

Issue: No LICENSE or README documenting:

• Font source (Google Fonts?)
• License type (Apache 2.0 per commit message?)
• Version/hash for reproducibility

Best Practice: Add compose-ui/src/desktopMain/resources/fonts/README.md with attribution.

---

🧪 Testing Concerns

1. No Unit Tests: The PR modifies font rendering logic but doesn't add tests. Consider:

   • Test font selection logic with mock settings
   • Test platform detection caching
   • Test overlay visibility based on focus state

2. Manual Testing Checklist: The PR description has a test plan, but consider adding:

   • Test with preferTerminalFontForSymbols set to null, true, false
   • Test symbol rendering (⏵ ★ ⚡) on both macOS and Linux
   • Test overlay with custom opacity settings (future feature?)

---

🎨 Code Style

1. Fully Qualified Name: ai.rever.bossterm.compose.util.bundledSymbolFont (lines 835, 842) is unusual. Why not import?

   import ai.rever.bossterm.compose.util.bundledSymbolFont

2. Comments: The code has decent comments, but the font classification logic (lines 827-850) would benefit from a block comment explaining the decision tree.

---

🔒 Security

No security concerns identified. Font loading uses standard resource streams with proper error handling.

---

📊 Performance Summary

Issue	Severity	Impact	Fix Difficulty
Platform detection in render loop	HIGH	~115K calls/sec	Easy (move to init)
Font loading on first render	LOW	Possible jank	Medium (preload fonts)

---

📝 Recommendations

Before Merge:

1. ✅ MUST: Move platform detection out of render loop
2. ✅ MUST: Add showUnfocusedOverlay toggle to Settings UI
3. ⚠️ SHOULD: Add font license documentation

Future Enhancements:

1. Add unit tests for font selection logic
2. Consider configurable overlay opacity/color
3. Profile font rendering performance on Linux with bundled font

---

🎯 Overall Assessment

Rating: 7.5/10

This is a solid feature with good architectural decisions, but has one critical performance issue (platform detection in render loop) and one usability issue (missing settings toggle). Once those are fixed, this will be production-ready.

The commit history shows good iteration (10 commits exploring different font solutions) and the final implementation is well-reasoned. Great work!

---

🤖 Generated with Claude Code - Review based on CLAUDE.md conventions

[claude]

Code Review: Unfocused Window Overlay + Symbol Font Improvements

Overview

This PR introduces two main features:

1. Unfocused window overlay - A semi-transparent white overlay when the window loses focus
2. Symbol font fallback system - Platform-specific font selection for emoji and symbols with bundled Noto Sans Symbols 2

✅ Strengths

1. Clean Architecture

• Proper separation of concerns with reactive state management
• Well-documented settings with clear descriptions
• Follows existing codebase patterns (similar to other overlay features)

2. Platform-Aware Design

• Smart platform detection for macOS vs Linux symbol rendering
• Tri-state setting (null/true/false) provides good flexibility
• Bundled Noto Sans Symbols 2 (641KB) is a reasonable size addition

3. Font Loading Best Practices

• Reuses established temp file + InputStream pattern from loadBundledFont()
• Proper error handling with fallback to FontFamily.Default
• Lazy initialization prevents unnecessary loading

4. Good Documentation

• Comprehensive CLAUDE.md updates with decision matrix
• Clear inline comments explaining font selection logic
• Settings descriptions help users understand the options

⚠️ Issues Found

CRITICAL: Missing UI Setting for Overlay Toggle

The showUnfocusedOverlay setting is defined in TerminalSettings.kt (line 138) and used in TabbedTerminal.kt (line 368), but there's no UI control in VisualSettingsSection.kt to toggle it.

Location: compose-ui/.../settings/sections/VisualSettingsSection.kt

Recommendation: Add a toggle in the "Window Style" section:

SettingsToggle(
    label = "Show Unfocused Window Overlay",
    checked = settings.showUnfocusedOverlay,
    onCheckedChange = { onSettingsChange(settings.copy(showUnfocusedOverlay = it)) },
    description = "Semi-transparent overlay when window loses focus"
)

MEDIUM: Potential Recomposition Performance Issue

// TabbedTerminal.kt:86
val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Issue: derivedStateOf with a function call will recompose every time the composable recomposes, not just when focus changes. If isWindowFocused() is expensive or causes side effects, this could impact performance.

Recommendation: If isWindowFocused is already a State<Boolean>, use it directly:

val isWindowFocusedState by isWindowFocused  // If it's already State<Boolean>

Or if it's a lambda that reads state internally, the current approach is fine. Please verify the implementation of isWindowFocused.

MEDIUM: Font Selection Logic Clarity

// TerminalCanvasRenderer.kt:822-826
val useSystemFontForEmoji = if (ctx.settings.preferTerminalFontForSymbols != null) {
    !ctx.settings.preferTerminalFontForSymbols!!
} else {
    isMacOS
}

Issue: The double negation (!preferTerminalFontForSymbols!!) is confusing. The setting name suggests "prefer terminal font" but the logic inverts it for "use system font".

Recommendation: Either:

1. Rename the setting to preferSystemFontForSymbols to match the logic
2. Or simplify the logic to avoid double negation

Current behavior is correct, but naming could be clearer.

🔍 Minor Observations

1. Overlay Color Choice

• White overlay at 15% opacity works well for light terminal themes
• Consider if this works for users with very light background colors
• Might want to test with light themes to ensure visibility

2. Font Caching
The bundledSymbolFont uses lazy initialization which is good, but with the new tri-state setting, users toggling between options won't incur re-loading costs since it's loaded once.

3. Documentation Quality
The decision matrix in CLAUDE.md is excellent - very clear for future maintainers.

4. Git Commit Format
Clean commit message format following project guidelines (no Co-Authored-By line as specified).

🧪 Testing Recommendations

1. Overlay Feature:

   • Test with light, dark, and custom themes
   • Test with multiple tabs (verify overlay covers all tabs)
   • Test with split panes (verify overlay covers entire window)
   • Rapidly switch focus in/out to check for flicker

2. Symbol Font:

   • Test on both macOS and Linux
   • Verify symbols render correctly: ⏵ ⏸ ★ ⚡ ☁️ ☀️
   • Test all three settings: Platform Default, Bundled, System Default
   • Check fallback behavior if Noto font fails to load

3. Performance:

   • Profile with React Dev Tools / Compose layout inspector
   • Check recomposition count when focus changes
   • Verify no frame drops during focus transitions

📋 Action Items

Must Fix Before Merge:

• Add showUnfocusedOverlay toggle to VisualSettingsSection.kt

Recommended:

• Verify isWindowFocused implementation for performance
• Consider renaming preferTerminalFontForSymbols to clarify logic

Optional:

• Test overlay with light terminal themes for visibility
• Add unit tests for font selection logic (if testing infrastructure exists)

🎯 Verdict

Approve with Changes: The implementation is solid and well-documented, but needs the UI control for the overlay setting before merge. Once that's added, this is ready to go.

The symbol font system is excellent - smart platform detection and good fallback behavior. The overlay feature is a nice UX improvement that helps with multi-window workflows.

Great work overall! 🚀

[claude]

Pull Request Review - PR #114

Summary

This PR adds two main features:

1. Unfocused window overlay indicator - Semi-transparent white overlay when terminal loses focus
2. Cross-platform symbol font support - Bundles Noto Sans Symbols 2 for Linux symbol rendering

Overall Assessment

✅ Approved with minor suggestions

The implementation is solid and follows the project's established patterns. The commit history shows thoughtful iteration and platform-specific considerations.

---

Detailed Review

🟢 Strengths

1. Well-documented commit history: The 11 commits show excellent incremental development with clear messages explaining the rationale for changes.

2. Platform-aware design: The symbol font logic properly handles macOS vs Linux differences with a sensible default (null = platform-specific).

3. Settings integration: Both features are properly exposed in settings with sensible defaults:

   • showUnfocusedOverlay: true
   • preferTerminalFontForSymbols: null (platform default)

4. Documentation: CLAUDE.md was updated with comprehensive technical documentation of the symbol font fallback system.

5. Correct rendering architecture: The font selection logic in TerminalCanvasRenderer.kt properly distinguishes between:

   • Emoji with variation selector → System font (color emoji)
   • Emoji/wide symbols → Platform-specific
   • Technical symbols → STIX Two Math → terminal font

6. Clean UI integration: The overlay uses Compose's Box layout correctly and the settings dropdown follows existing patterns.

---

🟡 Suggestions for Improvement

1. Missing License File for Bundled Font

The PR bundles NotoSansSymbols2-Regular.ttf (641KB) but doesn't include its license file.

Recommendation: Add compose-ui/src/desktopMain/resources/fonts/NotoSansSymbols2-LICENSE.txt with the Apache 2.0 license text, or add a section to a LICENSES.md file documenting third-party font licenses.

Location: The code comment mentions "Apache 2.0 license" but the actual license text should be included in the repository.

2. Potential Window Focus Performance Issue

In TabbedTerminal.kt:86, the focus state uses derivedStateOf:

val isWindowFocusedState by remember { derivedStateOf { isWindowFocused() } }

Concern: If isWindowFocused() is called frequently during recomposition, this could cause unnecessary recalculations.

Recommendation: Consider whether isWindowFocused should be a State<Boolean> parameter instead of a lambda, allowing proper Compose state observation. Example:

// In Main.kt
val isWindowFocused = remember { mutableStateOf(true) }
windowFocusListener = object : WindowFocusListener {
    override fun windowGainedFocus(e: WindowEvent) { isWindowFocused.value = true }
    override fun windowLostFocus(e: WindowEvent) { isWindowFocused.value = false }
}

// In TabbedTerminal
fun TabbedTerminal(
    isWindowFocused: State<Boolean>,  // Changed from () -> Boolean
    ...
)

This would eliminate the need for derivedStateOf and make the reactive dependency explicit.

3. Font Loading Error Handling

In FontUtils.kt:148, the error handler prints to stderr but returns FontFamily.Default:

} catch (e: Exception) {
    System.err.println("Failed to load bundled symbol font: " + e.message)
    FontFamily.Default
}

Recommendation: Consider whether this should log to the application's logging system (if one exists) rather than directly to stderr. Also, the error message could include the font file name for better debugging.

4. Settings UI - Missing "Unfocused Overlay" Toggle

The showUnfocusedOverlay setting was added to TerminalSettings.kt but I don't see a corresponding UI control in VisualSettingsSection.kt.

Recommendation: Add a toggle in the Visual Settings section:

SettingsToggle(
    label = "Show Unfocused Overlay",
    checked = settings.showUnfocusedOverlay,
    onCheckedChange = { onSettingsChange(settings.copy(showUnfocusedOverlay = it)) },
    description = "Dim window with semi-transparent overlay when not focused"
)

5. Overlay Color Hardcoded

The overlay uses a hardcoded white color with 15% opacity:

.background(Color.White.copy(alpha = 0.15f))

Consideration: This works well for light text on dark backgrounds (typical terminal), but if a user has a light background theme, the white overlay might be too subtle. Consider:

• Making it configurable in settings (opacity slider), or
• Using Color.Gray or Color.Black for better contrast across themes

This is a minor UX polish item, not a blocker.

---

🟢 Code Quality

• No security concerns: No injection vulnerabilities, proper resource handling
• No performance issues: Font loading is lazy and cached
• Follows project patterns: Uses existing settings infrastructure correctly
• Proper resource cleanup: Fonts use temp files with deleteOnExit()

---

🔍 Testing Recommendations

1. Font fallback testing: Test on Linux with symbols ⏵ ⏸ ★ ⚡ to ensure bundled font renders correctly
2. Platform testing: Verify null default works correctly on both macOS and Linux
3. Overlay visibility: Test overlay with different background colors/opacity settings
4. Settings persistence: Verify both new settings save/load correctly from JSON
5. Window focus: Test overlay appears/disappears correctly when switching between windows

---

📝 Minor Nitpicks

1. Commit hygiene: The PR follows the project's "no Co-Authored-By" guideline correctly ✅
2. Documentation: CLAUDE.md update is thorough and follows the established format ✅
3. Code formatting: Consistent with existing codebase ✅

---

Conclusion

This is a well-implemented PR that adds useful features with thoughtful platform-specific handling. The main suggestion is to add the font license file and consider exposing the overlay setting in the UI.

Recommendation: Approve and merge after addressing the missing font license file (item #1). Items #2-5 can be addressed in follow-up PRs if desired.

Great work! 🎉

[claude]

Code Review for PR #114: Add unfocused window overlay indicator

Overview

This PR introduces two main features:

1. Window Focus Overlay: A semi-transparent white overlay (15% opacity) when the terminal window loses focus
2. Cross-Platform Symbol Font Fallback: Platform-specific font selection for better symbol rendering on Linux

Positive Aspects ✅

Architecture & Design

• Clean separation of concerns with reactive state management
• Platform-aware logic that respects OS differences (macOS vs Linux)
• Proper use of Compose's remember and derivedStateOf for efficient recomposition
• Good fallback handling in font loading with proper error messages

Code Quality

• Well-documented code with clear comments explaining platform differences
• Consistent with existing codebase patterns (matches CLAUDE.md guidelines)
• Proper resource cleanup with tempFile.deleteOnExit()
• Good use of lazy initialization for bundled font (performance-conscious)

Documentation

• Comprehensive CLAUDE.md updates with clear tables explaining font selection logic
• Proper commit message format (without Co-Authored-By as per guidelines)
• Good description of the feature in PR summary

Issues & Concerns 🔴

1. CRITICAL: Missing UI Toggle for Unfocused Overlay

The showUnfocusedOverlay setting is defined in TerminalSettings.kt (line 144) and used in TabbedTerminal.kt (line 368), but there's no UI control in VisualSettingsSection.kt to toggle this feature.

Recommendation: Add a SettingsToggle in the Visual Settings section:

SettingsToggle(
    label = "Show Unfocused Overlay",
    checked = settings.showUnfocusedOverlay,
    onCheckedChange = { onSettingsChange(settings.copy(showUnfocusedOverlay = it)) },
    description = "Show semi-transparent overlay when window loses focus"
)

2. Code Organization: Misplaced Closing Brace

In TabbedTerminal.kt line 365, there's an extra closing brace that creates awkward indentation. The Column's closing brace should be at line 364, and line 365 is redundant.

Current (confusing):

            )
        }
        }  // ← Extra brace here

        // Semi-transparent overlay...

Should be:

            )
        }
    }

    // Semi-transparent overlay...

3. Performance Consideration: Platform Detection on Every Render

In TerminalCanvasRenderer.kt line 820, System.getProperty("os.name") is called every time drawChar() is invoked. This is inefficient as the OS doesn't change at runtime.

Recommendation: Move platform detection to a companion object or lazy val:

object TerminalCanvasRenderer {
    private val isMacOS by lazy { 
        System.getProperty("os.name")?.lowercase()?.contains("mac") == true 
    }
    // ... then use isMacOS in drawChar()
}

4. Nullable Logic Complexity

The preferTerminalFontForSymbols nullable Boolean with three states (null, true, false) adds cognitive complexity. The logic in lines 822-826 could be simplified with a helper function:

private fun shouldUseSystemFontForSymbols(settings: TerminalSettings): Boolean {
    return when (settings.preferTerminalFontForSymbols) {
        null -> isMacOS  // Platform default
        true -> false    // Force bundled
        false -> true    // Force system
    }
}

5. Font Loading Error Handling

While loadBundledSymbolFont() has try-catch, it prints to System.err but the error might go unnoticed. Consider logging to a proper logging framework if available, or at least using println() for visibility in user logs.

6. Test Coverage

The PR checklist shows unchecked test plan items. Consider testing:

• Multiple terminal windows with focus switching
• Settings persistence after restart
• Symbol rendering on both macOS and Linux (especially ⏵ ⏸ ★ ⚡)
• Performance impact of overlay (shouldn't affect rendering FPS)

7. Accessibility Consideration

A 15% white overlay might not be sufficient for users with visual impairments or in bright environments. Consider:

• Making opacity configurable (e.g., unfocusedOverlayOpacity: Float = 0.15f)
• Supporting different overlay colors (some users prefer dimming instead of whitening)

Minor Suggestions 💡

1. Type Safety: The font selection logic has many similar conditions. Consider using a sealed class or enum for font selection strategy:

sealed class SymbolFontStrategy {
    object SystemDefault : SymbolFontStrategy()
    object Bundled : SymbolFontStrategy()
    object PlatformDefault : SymbolFontStrategy()
}

2. Consistency: The bundled font size is 641KB. Verify this doesn't significantly impact the application bundle size and startup time.

3. Documentation: Add inline comments explaining why emoji with variation selectors always use FontFamily.Default (line 827-829) - not obvious to future maintainers.

Security Considerations 🔒

No security concerns identified. Font loading uses standard Java APIs with proper resource cleanup.

Performance Considerations ⚡

• OS property lookup should be cached (see issue 🔗 Implement hyperlink detection and click handling #3)
• Overlay rendering is efficient (simple Box with Color)
• Lazy font loading is good practice

Recommendation

Request Changes - The missing UI toggle for showUnfocusedOverlay is a critical omission that makes the feature non-configurable through the UI. This should be addressed before merging. The platform detection caching would also be a valuable performance improvement.

---

Overall, this is a well-structured PR with good documentation and solid implementation. The issues identified are mostly polish and optimization opportunities, with the exception of the missing UI toggle which should be addressed.

🤖 Generated with Claude Code