From a740581f208775dd46238969cd246fb9e257b777 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 16:11:37 +0200 Subject: [PATCH] Keep an unread notice older than the latest read from marking the room The job picks the room's members once and then publishes to them one at a time, as the request did before it. A member who opens the room during that loop gets the read event in their other tabs and then the older notice, which marked the room unread again. Both events now carry the server time, and the sidebar ignores a notice dated before the room's latest read. The notice still moves a direct room to the top. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9 --- app/channels/presence_channel.rb | 2 +- .../controllers/read_rooms_controller.js | 4 +-- .../controllers/rooms_list_controller.js | 17 ++++++++-- app/models/message/broadcasts.rb | 2 +- test/channels/presence_channel_test.rb | 10 ++++++ test/channels/unread_rooms_channel_test.rb | 6 ++-- test/system/unread_rooms_test.rb | 32 +++++++++++++++++++ 7 files changed, 64 insertions(+), 9 deletions(-) diff --git a/app/channels/presence_channel.rb b/app/channels/presence_channel.rb index 9c49c13..3ad3bc9 100644 --- a/app/channels/presence_channel.rb +++ b/app/channels/presence_channel.rb @@ -22,6 +22,6 @@ class PresenceChannel < RoomChannel end def broadcast_read_room - ActionCable.server.broadcast "user_#{current_user.id}_reads", { room_id: membership.room_id } + ActionCable.server.broadcast "user_#{current_user.id}_reads", { room_id: membership.room_id, at: Time.current.to_fs(:epoch) } end end diff --git a/app/javascript/controllers/read_rooms_controller.js b/app/javascript/controllers/read_rooms_controller.js index 422808d..ca8f6e2 100644 --- a/app/javascript/controllers/read_rooms_controller.js +++ b/app/javascript/controllers/read_rooms_controller.js @@ -16,7 +16,7 @@ export default class extends Controller { }) } - #read = ({ room_id }) => { - this.dispatch("read", { detail: { roomId: room_id } }) + #read = ({ room_id, at }) => { + this.dispatch("read", { detail: { roomId: room_id, at } }) } } diff --git a/app/javascript/controllers/rooms_list_controller.js b/app/javascript/controllers/rooms_list_controller.js index afd9981..4868904 100644 --- a/app/javascript/controllers/rooms_list_controller.js +++ b/app/javascript/controllers/rooms_list_controller.js @@ -7,6 +7,7 @@ export default class extends Controller { static classes = [ "unread" ] #disconnected = true + #readAt = new Map() async connect() { this.channel ??= await cable.subscribeTo({ channel: "UnreadRoomsChannel" }, { @@ -27,9 +28,13 @@ export default class extends Controller { this.read({ detail: { roomId: Current.room.id } }) } - read({ detail: { roomId } }) { + read({ detail: { roomId, at } }) { const room = this.#findRoomTarget(roomId) + if (at) { + this.#readAt.set(Number(roomId), Math.max(Number(at), this.#readAt.get(Number(roomId)) ?? 0)) + } + if (room) { room.classList.remove(this.unreadClass) this.dispatch("read", { detail: { targetId: roomId } }) @@ -47,11 +52,11 @@ export default class extends Controller { this.#disconnected = true } - #unread({ roomId }) { + #unread({ roomId, at }) { const unreadRoom = this.#findRoomTarget(roomId) if (unreadRoom) { - if (Current.room.id != roomId) { + if (Current.room.id != roomId && !this.#readSince(roomId, at)) { unreadRoom.classList.add(this.unreadClass) } @@ -59,6 +64,12 @@ export default class extends Controller { } } + // Notices fan out one member at a time, so one can arrive after the member has already + // read the room in another tab. It still reorders the room, but doesn't mark it unread. + #readSince(roomId, at) { + return Number(at) <= this.#readAt.get(Number(roomId)) + } + #findRoomTarget(roomId) { return this.roomTargets.find(roomTarget => roomTarget.dataset.roomId == roomId) } diff --git a/app/models/message/broadcasts.rb b/app/models/message/broadcasts.rb index 6ae7569..7118f82 100644 --- a/app/models/message/broadcasts.rb +++ b/app/models/message/broadcasts.rb @@ -13,7 +13,7 @@ module Message::Broadcasts # only the ones who still have it unread or are in it now: by the time this runs, # someone may have opened the room and moved on, and would see it marked unread again. def broadcast_unread_room - payload = ActiveSupport::JSON.encode(roomId: room.id) + payload = ActiveSupport::JSON.encode(roomId: room.id, at: created_at.to_fs(:epoch)) room.memberships.unread.or(room.memberships.connected).pluck(:user_id).each do |user_id| ActionCable.server.broadcast UnreadRoomsChannel.stream_name_for(user_id), payload, coder: nil diff --git a/test/channels/presence_channel_test.rb b/test/channels/presence_channel_test.rb index 99227a6..881f255 100644 --- a/test/channels/presence_channel_test.rb +++ b/test/channels/presence_channel_test.rb @@ -40,6 +40,16 @@ class PresenceChannelTest < ActionCable::Channel::TestCase end end + test "subscribing tells the user's other tabs when they read the room" do + membership = users(:david).memberships.first + + freeze_time do + assert_broadcast_on "user_#{users(:david).id}_reads", { room_id: membership.room_id, at: Time.current.to_fs(:epoch) } do + subscribe room_id: membership.room_id + end + end + end + test "unsubscribing marks the membership as disconnected" do membership = users(:david).memberships.first subscribe room_id: membership.room_id diff --git a/test/channels/unread_rooms_channel_test.rb b/test/channels/unread_rooms_channel_test.rb index b3f36ed..48ca520 100644 --- a/test/channels/unread_rooms_channel_test.rb +++ b/test/channels/unread_rooms_channel_test.rb @@ -26,14 +26,16 @@ class UnreadRoomsChannelTest < ActionCable::Channel::TestCase test "a member is told about activity in their own room" do direct = rooms(:bender_and_kevin) + message = nil broadcasts = capture_unread_broadcasts_for(users(:kevin)) do perform_enqueued_jobs only: Message::BroadcastUnreadRoomJob do - direct.messages.create!(body: "Private", creator: users(:bender), client_message_id: "member").broadcast_create + message = direct.messages.create!(body: "Private", creator: users(:bender), client_message_id: "member") + message.broadcast_create end end - assert_equal [ direct.id ], broadcasts.collect { |broadcast| broadcast["roomId"] } + assert_equal [ { "roomId" => direct.id, "at" => message.created_at.to_fs(:epoch) } ], broadcasts end private diff --git a/test/system/unread_rooms_test.rb b/test/system/unread_rooms_test.rb index ea68ba0..f2164c4 100644 --- a/test/system/unread_rooms_test.rb +++ b/test/system/unread_rooms_test.rb @@ -27,4 +27,36 @@ class UnreadRoomsTest < ApplicationSystemTestCase join_room designers_room assert_room_read designers_room end + + test "a notice about a message from before the room was read in another tab doesn't mark it unread again" do + using_session("Kevin in HQ") do + sign_in "kevin@37signals.com" + join_room rooms(:hq) + broadcast_unread_notice rooms(:designers), to: users(:kevin), at: Time.current + assert_room_unread rooms(:designers) + end + + sent_before_reading = Time.current + using_session("Kevin in Designers") do + sign_in "kevin@37signals.com" + join_room rooms(:designers) + end + + using_session("Kevin in HQ") do + assert_room_read rooms(:designers) + + broadcast_unread_notice rooms(:designers), to: users(:kevin), at: sent_before_reading + broadcast_unread_notice rooms(:bender_and_kevin), to: users(:kevin), at: Time.current + assert_selector "#" + dom_id(rooms(:bender_and_kevin), :list) + ".unread" + assert_room_read rooms(:designers) + + broadcast_unread_notice rooms(:designers), to: users(:kevin), at: Time.current + assert_room_unread rooms(:designers) + end + end + + private + def broadcast_unread_notice(room, to:, at:) + ActionCable.server.broadcast UnreadRoomsChannel.stream_name_for(to.id), { roomId: room.id, at: at.to_fs(:epoch) } + end end