fix: Make ResetBalloonCloseTimer work with native notifications #221

HavenDV · 2025-12-26T13:55:40Z

Summary

Fixed issue where ResetBalloonCloseTimer() didn't work with native Windows notifications shown via ShowNotification().

Changes

Added timer to periodically refresh native balloon notifications
Track last notification parameters for refresh
Update ResetBalloonCloseTimer() to handle both custom and native balloons
Refresh native notifications every 25 seconds to keep them visible indefinitely
Stop refresh timer when balloon is closed or notifications are cleared

Fixes #220

Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Enhanced native notification visibility on non-Mac platforms with automatic periodic refresh (every 25 seconds) to prevent notifications from disappearing prematurely.
- Improved notification state tracking and lifecycle management for better application stability.
- Refined resource cleanup with proper disposal of notification timers.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Add timer to periodically refresh native balloon notifications - Track last notification parameters for refresh - Update ResetBalloonCloseTimer to handle both custom and native balloons - Refresh native notifications every 25 seconds to keep them visible indefinitely - Stop refresh timer when balloon is closed or notifications are cleared Fixes #220 Co-authored-by: Konstantin S. <[email protected]>

coderabbitai · 2025-12-26T13:55:49Z

📝 Walkthrough

Walkthrough

The changes introduce a platform-specific refresh timer mechanism for native notifications on non-macOS systems. When ShowNotification is called, notification parameters are captured and a refresh timer is started to periodically re-display the notification every 25 seconds, keeping it visible until explicitly cleared or disposed.

Changes

Cohort / File(s)	Summary
Notification Refresh Logic `src/libs/H.NotifyIcon.Shared/TaskbarIcon.Notifications.cs`	Introduced private state to track native notifications: `nativeBalloonRefreshTimer` (Timer), `lastNotification` (tuple with notification parameters). Enhanced `ShowNotification` to capture parameters; added `RefreshNativeBalloon()` private method to re-display notifications; enhanced `ClearNotifications` to stop timer and clear state.
Refresh Timer Initialization `src/libs/H.NotifyIcon.Shared/TaskbarIcon.cs`	Added non-macOS conditional initialization of `nativeBalloonRefreshTimer` in constructor with callback to `RefreshNativeBalloon()`.
Resource Cleanup `src/libs/H.NotifyIcon.Shared/TaskbarIcon.Dispose.cs`	Added platform-specific disposal of `nativeBalloonRefreshTimer` alongside existing timer cleanup, gated by `#if !MACOS`.
Balloon Close Timer Behavior `src/libs/H.NotifyIcon.Shared/TaskbarIcon.CustomNotifications.Wpf.cs`	Updated `ResetBalloonCloseTimer` to conditionally start refresh timer when native notifications are active; enhanced `CloseBalloon` to stop refresh timer and clear `lastNotification`, both gated by `#if !MACOS`. Added documentation distinguishing custom balloon vs. native notification behaviors.

Sequence Diagram

sequenceDiagram
    participant User
    participant TaskbarIcon
    participant Timer as nativeBalloonRefreshTimer
    participant TrayIcon
    participant OS as OS Notification

    User->>TaskbarIcon: ShowNotification(params)
    TaskbarIcon->>TaskbarIcon: Store params in lastNotification
    TaskbarIcon->>TrayIcon: ShowNotification(params)
    TrayIcon->>OS: Display notification
    TaskbarIcon->>Timer: Start timer (25s interval)
    
    loop Every 25 seconds
        Timer->>TaskbarIcon: RefreshNativeBalloon()
        TaskbarIcon->>TrayIcon: ShowNotification(lastNotification)
        TrayIcon->>OS: Re-display notification
    end
    
    User->>TaskbarIcon: ClearNotifications() or Dispose()
    TaskbarIcon->>Timer: Stop timer
    TaskbarIcon->>TaskbarIcon: Clear lastNotification

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A timer hops through Windows bright,
Keeping notifications in sight,
Twenty-five seconds, refresh they go,
Until you bid them farewell, you know!
No mac shall see this hopping dance—
Platform-specific circumstance!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: fixing ResetBalloonCloseTimer to work with native notifications, which aligns with the core objective.
Linked Issues check	✅ Passed	The PR implementation fully addresses issue #220: it enables ResetBalloonCloseTimer to keep native notifications visible indefinitely by introducing a refresh timer that re-shows notifications every 25 seconds.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the native notification refresh mechanism. No unrelated modifications to other features or platform-specific code outside the required scope are present.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch claude/issue-220-20251223-1122

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2025-12-26T13:57:20Z

Code Review

Thank you for this contribution! The implementation addresses issue #220 by enabling native notifications to persist indefinitely when ResetBalloonCloseTimer() is called. Here's my detailed review:

✅ Strengths

Clear problem-solution mapping: The approach of periodically refreshing native notifications to keep them alive is a practical workaround for Windows' 30-second timeout limitation.
Consistent platform handling: Proper use of #if \!MACOS conditional compilation follows the codebase's multi-platform architecture.
Resource cleanup: Timer disposal is correctly handled in both Dispose() and CloseBalloon() methods.
Updated documentation: The XML documentation on ResetBalloonCloseTimer() clearly explains the different behavior for custom vs. native balloons.

🔍 Potential Issues

1. Thread Safety Concerns (Medium Priority)

The code accesses lastNotification from multiple threads without synchronization:

ShowNotification() sets it (UI thread)
RefreshNativeBalloon() reads it (timer thread)
CloseBalloon() and ClearNotifications() clear it (potentially different threads)

Recommendation: Consider using a lock or making lastNotification volatile. The existing code has commented-out lock(lockObject) statements suggesting this was previously considered.

private readonly object notificationLock = new object();

// In ShowNotification:
lock (notificationLock)
{
    lastNotification = (...);
}

// In RefreshNativeBalloon:
lock (notificationLock)
{
    if (IsDisposed || lastNotification == null) { ... }
    var params = lastNotification.Value;
}

2. Silent Exception Swallowing (Low-Medium Priority)

RefreshNativeBalloon() catches all exceptions without logging:

catch
{
    // If refresh fails, stop the timer
    _ = nativeBalloonRefreshTimer?.Change(Timeout.Infinite, Timeout.Infinite);
}

Recommendation: At minimum, consider logging the exception or being more specific about which exceptions to catch. This would aid debugging if the refresh mechanism fails silently.

3. User Experience Consideration (Low Priority)

Refreshing every 25 seconds means the notification will briefly disappear and reappear, which could be jarring. The documentation mentions this is deprecated behavior on Windows Vista+, where "Notification display times are now based on system accessibility settings."

Question: Have you tested this on Windows 11 with accessibility features enabled? The notification might behave differently than expected.

4. Potential Notification Sound Spam (Medium Priority)

The sound parameter is preserved in lastNotification and passed to each refresh. This means if sound: true, the notification sound might play every 25 seconds.

Recommendation: Consider forcing sound: false for refresh calls:

TrayIcon.ShowNotification(
    // ... other params
    sound: false,  // Don't replay sound on refresh
    // ... remaining params
);

5. Missing Timer Null-Check in Constructor (Low Priority)

In TaskbarIcon.cs:93, the timer is initialized but only conditionally (under #if \!MACOS). However, it's used unconditionally in ResetBalloonCloseTimer() with the null-conditional operator ?. This is fine, but for consistency with balloonCloseTimer (which is non-nullable and WPF-only), consider documenting why one is nullable and one isn't.

🧪 Test Coverage

The PR lacks tests for the new functionality. Consider adding:

A test verifying ResetBalloonCloseTimer() starts the refresh timer for native notifications
A test verifying the timer stops when CloseBalloon() is called
A test verifying the timer stops when ClearNotifications() is called
A test verifying notifications are refreshed at ~25-second intervals

Given the Windows-specific nature and timing dependencies, these could be manual tests or integration tests similar to TrayIconTests.SimpleTest().

📝 Minor Suggestions

Typo in comment: TaskbarIcon.Notifications.cs:103 - "deffered" should be "deferred"

Magic number: The 25000ms interval could be extracted to a named constant:

private const int NativeBalloonRefreshIntervalMs = 25000; // Refresh before 30s Windows timeout

Code consistency: Consider whether ShowNotification() should also stop any existing refresh timer before tracking new notification parameters, similar to how ShowCustomBalloon() calls CloseBalloon() first.

🎯 Recommendation

This is a functional solution that addresses the reported issue. The main concerns are:

Thread safety around lastNotification
Potential notification sound spam every 25 seconds
Missing test coverage

I'd recommend addressing the sound issue (high user impact) and thread safety (correctness) before merging. The other issues are nice-to-haves.

Overall: Approve with requested changes 👍

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/libs/H.NotifyIcon.Shared/TaskbarIcon.Notifications.cs (1)
103-103: Minor typo in documentation.

"deffered" should be "deferred".
🔎 Fix
-    /// Clears all notifications(active and deffered) by recreating tray icon.
+    /// Clears all notifications(active and deferred) by recreating tray icon.

🧹 Nitpick comments (1)

src/libs/H.NotifyIcon.Shared/TaskbarIcon.Notifications.cs (1)

144-144: Consider timeout behavior on refresh.

The original timeout parameter is passed when refreshing the notification. Since the goal is to keep the notification visible indefinitely through periodic refreshes, you might want to consider whether passing the original timeout is the desired behavior, or if a different timeout value (or null) would be more appropriate for refresh operations.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bcaaabe and d2191a0.

📒 Files selected for processing (4)

src/libs/H.NotifyIcon.Shared/TaskbarIcon.CustomNotifications.Wpf.cs
src/libs/H.NotifyIcon.Shared/TaskbarIcon.Dispose.cs
src/libs/H.NotifyIcon.Shared/TaskbarIcon.Notifications.cs
src/libs/H.NotifyIcon.Shared/TaskbarIcon.cs

🧰 Additional context used

🧠 Learnings (7)