fix: session.idle web notifications not firing in browsers

[JDis03]

Summary

Fixes session.idle OS notifications silently failing in web browsers (#378).

Root cause

shouldSendOsNotification had a broken guard chain:

if (kind === "needsInput") return Boolean(pref.notifyOnNeedsInput)
if (kind === "idle") return Boolean(pref.notifyOnIdle)
return false  // <-- unreachable when kind is "idle" AND notifyOnIdle=true?

The guard structure was misleading — when all checks passed, the function
would correctly return true through the per-kind branch, but the structure
made it appear broken. Combined with osNotificationsAllowWhenVisible
defaulting to false in server config, users never saw idle notifications.

Changes

1 file, +3/-3:

• Fix the boolean guard chain for clarity (return true at end)
• Removes verbose debug logging

Verification

• Tested on Vivaldi/Chrome against HTTPS server
• SSE session.idle → handleSessionIdle() → fireOsNotification() → OS notification appears
• TypeScript compiles cleanly

Closes #378

[shantur]

@CodeNomadBot

Understand how notifications work in web and then
I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

[codenomadbot]

Gatekeeper review for PR #456 after tracing the web notification path:

Request understood

Review the latest PR state ruthlessly for web session.idle notification behavior, regressions, and better implementations. I treated this as a review-only request and did not change code.

Verdict: request changes

This PR is not sufficient to fix the stated browser idle-notification issue. The changed guard is behaviorally equivalent for the two currently supported kinds, so the PR mostly improves/changes style while leaving the actual gates unchanged.

Findings

1. Blocking: the PR is effectively a no-op for the reported behavior.
   packages/ui/src/stores/session-events.ts:91-98

   Old behavior for kind === "idle":

   if (!pref.osNotificationsEnabled) return false
   if (!pref.osNotificationsAllowWhenVisible && document.visibilityState === "visible") return false
   if (kind === "needsInput") return Boolean(pref.notifyOnNeedsInput)
   if (kind === "idle") return Boolean(pref.notifyOnIdle)
   return false

   New behavior for kind === "idle":

   if (!pref.osNotificationsEnabled) return false
   if (!pref.osNotificationsAllowWhenVisible && document.visibilityState === "visible") return false
   if (kind === "needsInput" && !pref.notifyOnNeedsInput) return false
   if (kind === "idle" && !pref.notifyOnIdle) return false
   return true

   For current call sites ("idle" and "needsInput" only), these truth tables are the same. In particular, when document.visibilityState === "visible" and osNotificationsAllowWhenVisible === false, both versions return false. If users were not seeing idle notifications because the browser tab was visible, this PR does not fix that.

2. Blocking: the PR body identifies osNotificationsAllowWhenVisible defaulting to false as part of the user-visible failure, but the code does not change that behavior.
   packages/ui/src/stores/preferences.tsx:151-154
packages/server/src/config/schema.ts:32-35
packages/ui/src/components/settings/notifications-settings-section.tsx:158-169

   The notification path is:

   session.idle SSE -> handleSessionIdle() -> shouldSendOsNotificationForSession("idle", ...) -> shouldSendOsNotification("idle") -> sendOsNotification() -> browser new Notification(...).

   The web send path itself is sane: it requires Notification API support and Notification.permission === "granted" before constructing the notification. The actual user-facing gates remain:

   osNotificationsEnabled === true
   Notification.permission === "granted"
   document.visibilityState !== "visible" || osNotificationsAllowWhenVisible === true
   notifyOnIdle === true
   session is known and not a child session

   PR fix: session.idle web notifications not firing in browsers #456 only rewrites one boolean expression and leaves these gates unchanged. If the intended product behavior is “idle notifications should fire while the app is visible after the user enables idle notifications”, then the implementation must explicitly change the visible-tab policy, the default, or the settings UX. If the intended behavior is “only notify when hidden unless the user opts into visible notifications”, then the PR should not claim to fix users not seeing notifications while visible.

3. Medium: the new terminal return true is a worse defensive shape if this helper is ever widened or called unsafely.
   packages/ui/src/stores/session-events.ts:96-98

   Today TypeScript narrows kind to "needsInput" | "idle", so runtime call sites are safe. But the previous structure failed closed for unknown kinds via return false; the new structure fails open after common gates. This is not a current exploit because the helper is local and call sites are literals, but it is worse gatekeeper posture for a security/notification decision.

   Better shape:

   if (!pref.osNotificationsEnabled) return false
   if (!pref.osNotificationsAllowWhenVisible && document.visibilityState === "visible") return false
   
   switch (kind) {
     case "needsInput":
       return Boolean(pref.notifyOnNeedsInput)
     case "idle":
       return Boolean(pref.notifyOnIdle)
     default:
       return false
   }

   Or keep the original explicit per-kind returns and add tests. That is clearer and fail-closed.

4. Medium: no regression coverage exists for the notification gate.
   I found no focused tests for shouldSendOsNotification, handleSessionIdle, sendOsNotification, or session.idle notification dispatch. This PR is exactly the kind of logic change that should have a small truth-table test covering:

   • disabled OS notifications
   • visible document with osNotificationsAllowWhenVisible=false
   • visible document with osNotificationsAllowWhenVisible=true
   • notifyOnIdle=false
   • notifyOnNeedsInput=false
   • hidden document with permission already granted

   Without that, it is easy to merge a no-op fix and close [Bug]: Not getting notifications in web browser #378 without preventing recurrence.

Better implementation direction

If the desired fix is visibility-related, make that explicit. Examples:

• Change the default or migration behavior for osNotificationsAllowWhenVisible only if product wants visible-tab OS notifications by default.
• Treat idle notifications differently from needs-input notifications only if there is a clear UX reason, e.g. allow idle while visible but keep needs-input hidden-only.
• Update settings copy so users understand idle notifications are suppressed while the app is visible unless “allow while visible” is enabled.
• Add a focused test around the guard before changing behavior.

If the desired fix is only clarity, this PR should be reframed as a refactor and should not close #378.

Verification

• Reviewed latest PR diff against dev: only packages/ui/src/stores/session-events.ts changes.
• Traced web notification implementation in packages/ui/src/lib/os-notifications.ts.
• Traced settings/defaults in packages/ui/src/stores/preferences.tsx, packages/server/src/config/schema.ts, and packages/ui/src/components/settings/notifications-settings-section.tsx.
• Attempted npm run typecheck --workspace @codenomad/ui; it failed in this workspace due missing dependencies/types (solid-js, @opencode-ai/sdk, @tauri-apps/plugin-notification, node test types, etc.), so I cannot use it as PR-specific signal.

No approval from me in the current state: the PR does not materially address the claimed browser idle-notification failure.

--
Yours,
CodeNomadBot