From ed88eb02051370d03d4576813c6dc2ec5bbf066f Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Wed, 19 Aug 2026 18:08:04 -0600 Subject: [PATCH] Fix desktop messages not appearing live until refocus (#59) Desktop mode's focus gating (from #49) made losing OS focus send "leave" for every open room, which stopped live message delivery to that room, not just notification eligibility -- so a message wouldn't render until the room was manually left and rejoined. Room join/leave is now gated on visibility alone, matching the browser; notification eligibility gets its own separate signal (a "focus"/"blur" WS frame tracked by a new Redis-backed FocusPresence), so a connected-but-unfocused desktop member still gets notified without losing live delivery. Co-Authored-By: Claude Sonnet 5 --- backend/app/main.py | 2 + backend/app/routers/webhooks.py | 3 +- backend/app/services/message_events.py | 37 +++++--- backend/app/ws/chat.py | 32 ++++++- backend/app/ws/focus_presence.py | 45 ++++++++++ backend/tests/test_desktop_notifications.py | 81 +++++++++++++++++ frontend/src/ws/useChatSocket.ts | 99 +++++++++++++-------- 7 files changed, 250 insertions(+), 49 deletions(-) create mode 100644 backend/app/ws/focus_presence.py diff --git a/backend/app/main.py b/backend/app/main.py index 1d6ab5d..4c9658a 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -27,6 +27,7 @@ from app.routers import ( from app.ws.broadcaster import Broadcaster from app.ws.chat import router as ws_router from app.ws.connection_manager import ConnectionManager +from app.ws.focus_presence import FocusPresence from app.ws.global_presence import GlobalPresence from app.ws.presence import Presence @@ -82,6 +83,7 @@ def create_app() -> FastAPI: app.state.redis = Redis.from_url(settings.redis_url, decode_responses=True) app.state.presence = Presence(app.state.redis) app.state.global_presence = GlobalPresence(app.state.redis) + app.state.focus_presence = FocusPresence(app.state.redis) app.state.broadcaster = Broadcaster(app.state.redis, app.state.connection_manager) app.include_router(health.router) diff --git a/backend/app/routers/webhooks.py b/backend/app/routers/webhooks.py index 4c46e0f..cc19e6d 100644 --- a/backend/app/routers/webhooks.py +++ b/backend/app/routers/webhooks.py @@ -25,4 +25,5 @@ async def incoming_webhook_endpoint( broadcaster = request.app.state.broadcaster presence = request.app.state.presence - await broadcast_new_message(db, broadcaster, presence, room.id, message, sender) + focus_presence = request.app.state.focus_presence + await broadcast_new_message(db, broadcaster, presence, focus_presence, room.id, message, sender) diff --git a/backend/app/services/message_events.py b/backend/app/services/message_events.py index f2b0c32..109a303 100644 --- a/backend/app/services/message_events.py +++ b/backend/app/services/message_events.py @@ -10,6 +10,7 @@ from app.services.link_preview_service import fetch_and_broadcast_link_preview from app.services.push_service import send_push_to_user from app.services.webhook_service import dispatch_event from app.ws.broadcaster import Broadcaster +from app.ws.focus_presence import FocusPresence from app.ws.presence import Presence @@ -17,6 +18,7 @@ async def _notify_offline_members( db: AsyncSession, broadcaster: Broadcaster, presence: Presence, + focus_presence: FocusPresence, room_id: uuid.UUID, sender: User, message: Message, @@ -25,12 +27,28 @@ async def _notify_offline_members( select(RoomMembership.user_id).where(RoomMembership.room_id == room_id) ) member_ids = {row[0] for row in result.all()} + connected_ids = await presence.connected_user_ids(room_id) # Subtract the sender explicitly rather than relying on them being # "connected" (true for the WS path, since they just sent this over an # active connection -- not true for the incoming-webhook REST path, # which has no WS connection for the attributed sender at all). - offline_ids = member_ids - await presence.connected_user_ids(room_id) - {sender.id} - if not offline_ids: + offline_ids = member_ids - connected_ids - {sender.id} + + # #59: a desktop-mode member can be *connected* to this room's channel + # (it's open on screen, live messages are rendering) while their window + # sits unfocused behind something else -- still exactly the situation a + # desktop notification should fire for, same as #49's original intent. + # This used to be handled by the client faking "offline" (leaving the + # room's channel on blur), which also silently stopped live delivery to + # that room; FocusPresence is a separate signal so notification + # eligibility no longer has to ride on room-connection state at all. + connected_but_unfocused_ids = { + user_id + for user_id in connected_ids - {sender.id} + if await focus_presence.is_unfocused(user_id) + } + notify_ids = offline_ids | connected_but_unfocused_ids + if not notify_ids: return result = await db.execute( @@ -38,12 +56,10 @@ async def _notify_offline_members( ) mentioned_ids = {row[0] for row in result.all()} - # This is also exactly the right audience for "give this room an unread - # dot": presence.connected_user_ids(room_id) means "has this room's - # channel joined right now" -- which the client only does while the tab - # is genuinely foregrounded (see useChatSocket.ts's visibility-gated - # join/leave), so a backgrounded-but-open room correctly lands here too, - # not just rooms that aren't open at all. + # Unread-dot audience stays exactly offline_ids, not notify_ids: a + # connected-but-unfocused member still has the room open and rendering + # on screen right now, so it isn't actually "unread" for them the way a + # room they haven't got open at all is. for user_id in offline_ids: await broadcaster.publish_to_user( user_id, @@ -56,7 +72,7 @@ async def _notify_offline_members( room = await db.get(Room, room_id) title = f"#{room.name}" if room else "New message" - for user_id in offline_ids: + for user_id in notify_ids: mentioned = user_id in mentioned_ids if message.content: prefix = f"{sender.username} mentioned you: " if mentioned else f"{sender.username}: " @@ -131,6 +147,7 @@ async def broadcast_new_message( db: AsyncSession, broadcaster: Broadcaster, presence: Presence, + focus_presence: FocusPresence, room_id: uuid.UUID, message: Message, sender: User, @@ -161,7 +178,7 @@ async def broadcast_new_message( await broadcaster.publish_to_user( unhidden_user_id, {"type": "room_added", "room_id": str(room_id)} ) - await _notify_offline_members(db, broadcaster, presence, room_id, sender, message) + await _notify_offline_members(db, broadcaster, presence, focus_presence, room_id, sender, message) await dispatch_event(db, "message.created", room_id, payload) _maybe_fetch_link_preview(broadcaster, room_id, message) diff --git a/backend/app/ws/chat.py b/backend/app/ws/chat.py index 206874a..c739c06 100644 --- a/backend/app/ws/chat.py +++ b/backend/app/ws/chat.py @@ -36,6 +36,7 @@ class ClientEnvelope(BaseModel): file_id: uuid.UUID | None = None message_id: uuid.UUID | None = None emoji: str | None = None + focused: bool | None = None async def _is_room_member(db: AsyncSession, room_id: uuid.UUID, user_id: uuid.UUID) -> bool: @@ -79,8 +80,14 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) manager = websocket.app.state.connection_manager presence = websocket.app.state.presence global_presence = websocket.app.state.global_presence + focus_presence = websocket.app.state.focus_presence broadcaster = websocket.app.state.broadcaster joined_rooms: set[uuid.UUID] = set() + # Tracks this connection's last-reported focus state (see the "focus" + # envelope below) so the disconnect cleanup can release FocusPresence's + # refcount if the socket closes while still blurred -- mirroring how + # joined_rooms tracks per-connection room membership for its own cleanup. + is_blurred = False manager.register_user(user.id, websocket) # Only broadcast on a genuine offline->online transition (this user's # first open connection), not for every extra tab -- broadcast_member_ @@ -133,6 +140,25 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) await presence.leave(envelope.room_id, user.id) joined_rooms.discard(envelope.room_id) + elif envelope.type == "focus": + # Sent only by the desktop client (#59), independent of + # room join/leave -- see FocusPresence's docstring for + # why widening desktop_notification eligibility this + # way no longer needs to touch live room delivery at + # all, unlike the "leave the room's channel on blur" + # approach this replaced. + if envelope.focused is None: + await websocket.send_json({"type": "error", "detail": "focused required"}) + continue + if envelope.focused: + if is_blurred: + await focus_presence.mark_focused(user.id) + is_blurred = False + else: + if not is_blurred: + await focus_presence.mark_blurred(user.id) + is_blurred = True + elif envelope.type == "message": if envelope.room_id is None or ( not envelope.content @@ -185,7 +211,9 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) # incoming-webhook path also calls it, and a webhook's # attributed sender may not actually be watching. await mark_room_read(db, envelope.room_id, user.id) - await broadcast_new_message(db, broadcaster, presence, envelope.room_id, message, user) + await broadcast_new_message( + db, broadcaster, presence, focus_presence, envelope.room_id, message, user + ) elif envelope.type == "edit": if envelope.room_id is None or envelope.message_id is None or not envelope.content: @@ -268,5 +296,7 @@ async def chat_endpoint(websocket: WebSocket, db: AsyncSession = Depends(get_db) manager.unregister_user(user.id, websocket) for room_id in joined_rooms: await presence.leave(room_id, user.id) + if is_blurred: + await focus_presence.mark_focused(user.id) if await global_presence.disconnect(user.id): await broadcast_member_updated(db, broadcaster, user.id) diff --git a/backend/app/ws/focus_presence.py b/backend/app/ws/focus_presence.py new file mode 100644 index 0000000..6e550df --- /dev/null +++ b/backend/app/ws/focus_presence.py @@ -0,0 +1,45 @@ +import uuid + +from redis.asyncio import Redis + + +class FocusPresence: + """Cross-instance "is this desktop-mode user's window currently + unfocused," used only to widen desktop_notification/push eligibility + beyond plain room-connection state (#59). A Redis hash (field = + user_id, value = refcount of that user's currently-blurred desktop + connections), parallel to Presence/GlobalPresence. + + Absence from this hash is the default and means "focused." That default + is also exactly right for every browser-tab connection: only the + desktop client ever sends focus/blur frames at all (see chat.py's + "focus" envelope handling), so a browser user never appears here -- + their attention is already fully captured by Presence's room-connection + state, which stays visibility-gated with no separate focus signal. + + Refcounted for the same multi-connection reason as Presence/ + GlobalPresence, with the same known simplification: two desktop windows + for one user, one focused and one blurred, count as "unfocused" here + (refcount > 0) even though the user does have attention somewhere. That + errs toward notifying rather than silently missing one, which is the + safer failure mode for a notification. + """ + + def __init__(self, redis: Redis) -> None: + self._redis = redis + + def _key(self) -> str: + return "presence:unfocused" + + async def mark_blurred(self, user_id: uuid.UUID) -> None: + await self._redis.hincrby(self._key(), str(user_id), 1) + + async def mark_focused(self, user_id: uuid.UUID) -> None: + key = self._key() + field = str(user_id) + remaining = await self._redis.hincrby(key, field, -1) + if remaining <= 0: + await self._redis.hdel(key, field) + + async def is_unfocused(self, user_id: uuid.UUID) -> bool: + return await self._redis.hexists(self._key(), str(user_id)) diff --git a/backend/tests/test_desktop_notifications.py b/backend/tests/test_desktop_notifications.py index 82381fc..783a40d 100644 --- a/backend/tests/test_desktop_notifications.py +++ b/backend/tests/test_desktop_notifications.py @@ -128,6 +128,87 @@ def test_desktop_notification_not_sent_to_room_member_who_is_present(ws_client_f assert live_message["type"] == "message" +def test_desktop_notification_sent_to_connected_but_blurred_member(ws_client_factory): + # #59: bob keeps the room's channel joined (so live delivery to a room + # actually open on screen never stops) but reports his desktop window + # as unfocused via a "focus" frame -- the whole point of this fix is + # that notification eligibility no longer needs the client to fake + # "offline" by leaving the room's channel, which used to also break + # live delivery until the room was manually left and rejoined. + instance1 = ws_client_factory() + instance2 = ws_client_factory() + + alice = _register_ws(instance1, _unique("alice")) + room = instance1.post("/api/rooms", json={"name": _unique("general")}).json() + + _register_ws(instance2, _unique("bob")) + instance2.post(f"/api/rooms/{room['id']}/join") + + with instance2.websocket_connect("/ws/chat") as bob_ws: + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert bob_ws.receive_json()["type"] == "joined" + + bob_ws.send_json({"type": "focus", "focused": False}) + # Sync barrier -- see test_mentions.py's identical pattern: a + # second (idempotent) join only acks once the prior "focus" + # frame's own handling (and commit) has completed. + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert bob_ws.receive_json()["type"] == "joined" + + with instance1.websocket_connect("/ws/chat") as alice_ws: + alice_ws.send_json({"type": "join", "room_id": room["id"]}) + assert alice_ws.receive_json()["type"] == "joined" + message = _send_and_sync(alice_ws, room["id"], "you awake?") + assert message["type"] == "message" + + # Live delivery still works -- bob's room channel was never left. + live_message = _recv(bob_ws) + assert live_message["type"] == "message" + assert live_message["content"] == "you awake?" + + # ...and he's still notified, despite being "connected" to the room. + desktop_note = _recv(bob_ws) + assert desktop_note == { + "type": "desktop_notification", + "id": message["id"], + "room_id": room["id"], + "title": f"#{room['name']}", + "body": f"{alice['username']}: you awake?", + } + + +def test_desktop_notification_not_sent_after_refocus(ws_client_factory): + # Proves the "focus" signal is a live toggle, not one-way -- blurring + # and then refocusing before the message arrives must fully cancel out. + instance1 = ws_client_factory() + instance2 = ws_client_factory() + + alice = _register_ws(instance1, _unique("alice")) + room = instance1.post("/api/rooms", json={"name": _unique("general")}).json() + + _register_ws(instance2, _unique("bob")) + instance2.post(f"/api/rooms/{room['id']}/join") + + with instance2.websocket_connect("/ws/chat") as bob_ws: + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert bob_ws.receive_json()["type"] == "joined" + + bob_ws.send_json({"type": "focus", "focused": False}) + bob_ws.send_json({"type": "focus", "focused": True}) + bob_ws.send_json({"type": "join", "room_id": room["id"]}) + assert bob_ws.receive_json()["type"] == "joined" + + with instance1.websocket_connect("/ws/chat") as alice_ws: + alice_ws.send_json({"type": "join", "room_id": room["id"]}) + assert alice_ws.receive_json()["type"] == "joined" + _send_and_sync(alice_ws, room["id"], "hello again") + + # Refocused before the message arrived -- only the live broadcast, + # same as test_desktop_notification_not_sent_to_room_member_who_is_present. + live_message = _recv(bob_ws) + assert live_message["type"] == "message" + + def test_desktop_notification_not_sent_to_non_member(ws_client_factory): instance1 = ws_client_factory() instance2 = ws_client_factory() diff --git a/frontend/src/ws/useChatSocket.ts b/frontend/src/ws/useChatSocket.ts index 6e2ed9e..54d003f 100644 --- a/frontend/src/ws/useChatSocket.ts +++ b/frontend/src/ws/useChatSocket.ts @@ -10,20 +10,23 @@ interface UseChatSocketOptions { const RECONNECT_BASE_DELAY_MS = 1000 const RECONNECT_MAX_DELAY_MS = 30000 -// #49 follow-up: a bare `document.visibilityState` check (below) means "not -// minimized/hidden" -- in a browser tab that's a decent proxy for "the user -// could be looking at this," since visibility already tracks whether this is -// the active tab. Inside an Electron BrowserWindow it isn't: visibilityState -// only flips on minimize/hide, not on losing OS focus, so a window sitting -// open-but-unfocused behind another app never registers as "gone." That's -// exactly the state a desktop notification needs to fire in, so desktop mode -// additionally requires document.hasFocus(). Gated on desktopMode so regular -// browser-tab behavior (already relied on by Web Push and the unread dot) is -// completely unchanged. const desktopMode = isDesktopNotificationsSupported() -function isPresent(): boolean { - return document.visibilityState === 'visible' && (!desktopMode || document.hasFocus()) +// Room join/leave (live message delivery) depends only on visibility -- +// "not minimized/hidden" -- exactly like a browser tab, in every mode. +// +// #49 originally had desktop mode additionally require document.hasFocus() +// here, on the theory that losing OS focus should count as "not present" +// the same way backgrounding a browser tab does. #59: that conflated two +// separate concerns onto one signal -- losing focus made the desktop client +// send "leave" for every open room, which stopped *live delivery* to a room +// still fully visible on screen, not just notification eligibility. A +// message wouldn't appear until the room was manually left and rejoined +// (e.g. switching rooms and back), which is what actually got reported. +// Focus now drives its own separate signal (see the "focus" WS frame below +// and FocusPresence server-side) instead of gating room membership at all. +function isVisible(): boolean { + return document.visibilityState === 'visible' } // One connection per authenticated session, established as soon as the app @@ -47,7 +50,7 @@ export function useChatSocket({ onUnauthenticated }: UseChatSocketOptions) { // backgrounded user the same as a disconnected one instead of assuming a // live WebSocket delivery the user can't actually see will do the job. const desiredRoomsRef = useRef(new Set()) - const isVisibleRef = useRef(isPresent()) + const isVisibleRef = useRef(isVisible()) const sendRoomFrame = useCallback((type: 'join' | 'leave', roomId: string) => { const ws = socketRef.current @@ -56,6 +59,18 @@ export function useChatSocket({ onUnauthenticated }: UseChatSocketOptions) { } }, []) + // #59: reports this desktop window's focus state as its own signal, + // completely separate from room join/leave above -- see FocusPresence + // server-side. No-op (and never called) outside desktop mode, matching + // how the server only ever expects "focus" frames from the desktop + // client at all. + const sendFocusFrame = useCallback(() => { + const ws = socketRef.current + if (ws && ws.readyState === WebSocket.OPEN) { + ws.send(JSON.stringify({ type: 'focus', focused: document.hasFocus() })) + } + }, []) + useEffect(() => { let stopped = false let reconnectDelay = RECONNECT_BASE_DELAY_MS @@ -91,12 +106,18 @@ export function useChatSocket({ onUnauthenticated }: UseChatSocketOptions) { // comment): reconnecting from a backgrounded tab should stay // "left" for the same reason backgrounding leaves in the first // place (see desiredRoomsRef's comment above). - isVisibleRef.current = isPresent() + isVisibleRef.current = isVisible() if (isVisibleRef.current) { for (const roomId of desiredRoomsRef.current) { sendRoomFrame('join', roomId) } } + // The server's FocusPresence state for this user doesn't survive a + // dropped connection either (see chat.py's disconnect cleanup) -- + // report the current value fresh on every (re)connect, not just on + // the next focus/blur transition, so a reconnect while unfocused + // (e.g. after a deploy) doesn't leave the server assuming focused. + if (desktopMode) sendFocusFrame() } ws.onmessage = (event) => { @@ -139,35 +160,39 @@ export function useChatSocket({ onUnauthenticated }: UseChatSocketOptions) { socketRef.current?.close() socketRef.current = null } - }, [sendRoomFrame]) + }, [sendRoomFrame, sendFocusFrame]) useEffect(() => { - function handlePresenceChange() { - const present = isPresent() - if (present === isVisibleRef.current) return - isVisibleRef.current = present + function handleVisibilityChange() { + const visible = isVisible() + if (visible === isVisibleRef.current) return + isVisibleRef.current = visible for (const roomId of desiredRoomsRef.current) { - sendRoomFrame(present ? 'join' : 'leave', roomId) - } - } - document.addEventListener('visibilitychange', handlePresenceChange) - // Only in desktop mode -- see isPresent()'s comment above. Blur/focus on - // a browser tab fire on every click into/out of the page (e.g. opening - // devtools), which would be a far noisier signal than intended there; - // browser tabs stay on visibilitychange alone, unchanged from before. - if (desktopMode) { - window.addEventListener('focus', handlePresenceChange) - window.addEventListener('blur', handlePresenceChange) - } - return () => { - document.removeEventListener('visibilitychange', handlePresenceChange) - if (desktopMode) { - window.removeEventListener('focus', handlePresenceChange) - window.removeEventListener('blur', handlePresenceChange) + sendRoomFrame(visible ? 'join' : 'leave', roomId) } } + document.addEventListener('visibilitychange', handleVisibilityChange) + return () => document.removeEventListener('visibilitychange', handleVisibilityChange) }, [sendRoomFrame]) + // #59: focus/blur reporting, entirely separate from the visibility effect + // above -- losing OS focus no longer touches room membership at all, just + // this signal (consumed server-side by FocusPresence to widen desktop- + // notification eligibility). Only in desktop mode: on a browser tab, + // focus/blur fire on every click into/out of the page (e.g. opening + // devtools), a far noisier signal than intended, and browser tabs don't + // need it anyway -- visibility alone already matches pre-#49 behavior + // there. + useEffect(() => { + if (!desktopMode) return + window.addEventListener('focus', sendFocusFrame) + window.addEventListener('blur', sendFocusFrame) + return () => { + window.removeEventListener('focus', sendFocusFrame) + window.removeEventListener('blur', sendFocusFrame) + } + }, [sendFocusFrame]) + const subscribe = useCallback((handler: (envelope: ServerEnvelope) => void) => { subscribersRef.current.add(handler) return () => { @@ -193,7 +218,7 @@ export function useChatSocket({ onUnauthenticated }: UseChatSocketOptions) { // room stays in desiredRoomsRef regardless, so the next genuine // foreground transition (handleVisibilityChange below) still joins // it, just deferred instead of wrongly immediate. - isVisibleRef.current = isPresent() + isVisibleRef.current = isVisible() if (isVisibleRef.current) sendRoomFrame('join', roomId) }, [sendRoomFrame],