Skip to content

Conversation

@murdercode
Copy link
Owner

This pull request adds a new "Hide Icon on Close" feature that allows users to hide the app icon from the dock when the main window is closed, while keeping the app running in the system tray. The implementation includes both frontend UI changes and backend logic, with platform-specific support for macOS. The most important changes are grouped below.

Feature Implementation: Hide Icon on Close

  • Added a new hide_icon_on_close setting to AppSettings and ensured it is properly handled in defaults and serialization in src-tauri/src/lib.rs. [1] [2]
  • Implemented logic in the window close event to hide the app icon from the dock on macOS if hide_icon_on_close is enabled, using the new set_dock_visibility command. [1] [2]
  • Registered the set_dock_visibility command for use in the app, enabling dock visibility toggling from both backend and frontend.

Frontend UI & Settings Management

  • Added a checkbox UI for "Hide Icon on Close" in the settings section of src/index.html, with a description of its behavior.
  • Updated SettingsManager in src/managers/settings-manager.js to support loading, saving, and toggling the new setting, including user feedback and auto-save logic. [1] [2] [3] [4]

Platform Support

  • Added cocoa and objc dependencies to src-tauri/Cargo.toml to enable macOS-specific dock visibility control.

These changes collectively provide a seamless user experience for controlling the app's dock visibility when closing the window, with robust settings management and cross-platform considerations.

@murdercode murdercode linked an issue Aug 22, 2025 that may be closed by this pull request
@claude
Copy link

claude bot commented Aug 22, 2025

Claude finished @murdercode's task —— View job


Code Review Complete ✅

I've reviewed the "Hide Icon on Close" feature implementation. Here's my comprehensive analysis:

Todo List:

  • Read and analyze all changed files
  • Review backend Rust code (lib.rs, Cargo.toml)
  • Review frontend code (HTML, JavaScript)
  • Check for security concerns with new dependencies
  • Assess code quality and best practices
  • Look for potential bugs and edge cases
  • Evaluate performance considerations
  • Check test coverage gaps
  • Provide comprehensive feedback

Overall Assessment: ✅ Well-Implemented Feature

This is a solid implementation of the "Hide Icon on Close" feature with good attention to platform-specific requirements and user experience.


🔍 Detailed Code Review

Strengths

1. Platform-Specific Implementation

  • Correctly uses #[cfg(target_os = "macos")] conditional compilation
  • Proper macOS API integration using NSApplicationActivationPolicy
  • Graceful handling for non-macOS platforms with appropriate error messages

2. State Management

  • Clean integration with existing AppSettings struct
  • Proper serialization/deserialization with #[serde(default)]
  • Consistent default values (false - disabled by default)

3. UI Integration

  • Well-placed checkbox in Advanced Settings section
  • Clear description explaining the behavior
  • Proper event handling with auto-save functionality

4. Thread Safety

  • Uses app.run_on_main_thread() for macOS UI operations (line 1344-1347 in lib.rs)
  • Async handling in window close events prevents blocking

⚠️ Areas for Improvement

1. Error Handling (Minor)

// lib.rs:1064 - Error is silently ignored
let _ = set_dock_visibility(app_handle_clone.clone(), false).await;

Recommendation: Log errors for debugging:

if let Err(e) = set_dock_visibility(app_handle_clone.clone(), false).await {
    eprintln!("Failed to set dock visibility: {}", e);
}

2. Code Duplication
The dock visibility restoration logic is repeated in show_window() (lines 618-632). Consider extracting to a helper function.

3. Settings Validation (Minor)
No validation for the boolean setting, but this is acceptable for a simple boolean flag.

🛡️ Security Analysis

Dependencies Added:

  • cocoa = "0.25" - Well-established macOS library (✅ Safe)
  • objc = "0.2" - Standard Objective-C runtime binding (✅ Safe)

Security Considerations:

  • ✅ No user input processing in security-sensitive areas
  • ✅ Platform-specific code properly guarded
  • ✅ No elevated privileges required
  • ✅ Uses safe NSApplication APIs

🚀 Performance Impact

Positive:

  • Minimal runtime overhead - only active on window close
  • Platform-specific compilation prevents unused code on other platforms
  • Uses efficient async/await patterns

Considerations:

  • Native API calls have negligible performance impact
  • No memory leaks detected in the implementation

🧪 Test Coverage

Missing Test Scenarios:

  1. Unit Tests: No tests for set_dock_visibility_native() function
  2. Integration Tests: Window close behavior with different setting values
  3. Platform Tests: Behavior verification on non-macOS systems

Recommended Test Cases:

#[cfg(test)]
mod tests {
    #[cfg(target_os = "macos")]
    #[test]
    fn test_dock_visibility_toggle() {
        // Test dock visibility changes
    }
    
    #[cfg(not(target_os = "macos"))]
    #[test]
    fn test_unsupported_platform_error() {
        // Test error handling on non-macOS
    }
}

🐛 Potential Edge Cases

1. Race Conditions (Low Risk)

  • Multiple rapid window close/show cycles might cause state inconsistencies
  • Mitigation: Current implementation is reasonably safe due to main thread execution

2. Settings Persistence (Low Risk)

  • If app crashes during dock state change, dock might remain hidden
  • Mitigation: macOS typically restores dock visibility on app termination

📝 Code Quality Assessment

Rust Code (lib.rs):

  • ✅ Follows Rust idioms and conventions
  • ✅ Proper error handling patterns
  • ✅ Good use of conditional compilation
  • ✅ Clean separation of concerns

Frontend Code (settings-manager.js):

  • ✅ Consistent with existing patterns
  • ✅ Proper async/await usage
  • ✅ Good user feedback integration
  • ✅ Auto-save functionality works correctly

HTML (index.html):

  • ✅ Semantic markup
  • ✅ Appropriate placement in UI hierarchy
  • ✅ Clear user-facing text

🎯 Recommendations

High Priority:

  1. Add error logging for dock visibility failures
  2. Add unit tests for the new functionality

Medium Priority:

  1. Extract helper function for dock visibility restoration logic
  2. Consider graceful degradation message for users on older macOS versions

Low Priority:

  1. Add integration tests for window state management
  2. Consider adding telemetry for feature usage (if analytics are enabled)

📊 Final Rating: 8.5/10

This is a well-implemented feature that demonstrates:

  • ✅ Good understanding of platform-specific requirements
  • ✅ Proper integration with existing architecture
  • ✅ Clean code organization and patterns
  • ✅ Appropriate security considerations
  • ✅ Solid user experience design

The implementation is production-ready with only minor improvements suggested. Great work! 🎉


@murdercode murdercode merged commit fd3fe18 into main Aug 22, 2025
1 check passed
@murdercode murdercode deleted the hide-mac-icon branch August 22, 2025 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide Mac Menu Item

2 participants