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