Fixing unread message counting bug

We assumed that with message ranges (id1,id2) we can count the number of
messages in the range by id2-id1. This is of course wrong because some
of the ids in the range can belong to other threads, causing the code to
significantly overcount read messages.

To fix this problem this commit introduces a new column to the ranges,
so that we store ranges in the form of (id1,id2,count), and can count
the read message count reliable for threads by a simple group by
thread_id sum(count).

Author: dutow

app/controllers/topics_controller.rb

@@ -313,11 +313,9 @@ def preload_topic_states
       aware_map = ThreadAwareness.where(user: current_user, topic_id: topic_ids)
                                  .pluck(:topic_id, :aware_until_message_id)
                                  .to_h
-      read_rows = MessageReadRange.where(user: current_user, topic_id: topic_ids)
-                                  .pluck(:topic_id, :range_start_message_id, :range_end_message_id)
-      read_ranges = read_rows.each_with_object(Hash.new { |h, k| h[k] = [] }) do |(tid, s, e), acc|
-        acc[tid] << [s, e]
-      end
+      read_counts = MessageReadRange.where(user: current_user, topic_id: topic_ids)
+                                    .group(:topic_id)
+                                    .sum(:message_count)
       global_aware_before = current_user.aware_before
 
       team_readers = preload_team_reader_states(topic_ids, last_ids)
@@ -327,19 +325,19 @@ def preload_topic_states
     @topics.each do |topic|
       last_id = last_ids[topic.id]
       last_time = last_times[topic.id]
-    if user_signed_in?
-      aware_until = aware_map[topic.id]
-      total = total_counts[topic.id].to_i
-      ranges = read_ranges[topic.id] || []
-      read_count = ranges.sum do |(s, e)|
-        next 0 unless s && e
-        (e - s + 1)
+      if user_signed_in?
+        aware_until = aware_map[topic.id]
+        total = total_counts[topic.id].to_i
+        read_count = read_counts[topic.id].to_i
+        status = compute_topic_status(total:, last_time:, aware_until:, read_count:, global_aware_before:)
+        progress = compute_progress(total:, read_count:)
+      else
+        status = :new
+        aware_until = nil
+        read_count = 0
+        progress = 0
       end
-      status = compute_topic_status(total:, last_time:, aware_until:, read_count:, global_aware_before:)
-      progress = compute_progress(total:, read_count:)
-    else
-      status = :new
-    end
+
       @topic_states[topic.id] = { status:, aware_until:, read_count:, last_id:, last_time:, progress:, team_readers: team_readers[topic.id] || [] }
     end
   end

app/models/message_read_range.rb

@@ -24,7 +24,16 @@ def self.add_range(user:, topic:, start_id:, end_id:, read_at: Time.current)
         overlapping.delete_all
       end
 
-      create!(user:, topic:, range_start_message_id: s, range_end_message_id: e, read_at:)
+      count = Message.where(topic_id: topic.id, id: s..e).count
+
+      create!(
+        user: user,
+        topic: topic,
+        range_start_message_id: s,
+        range_end_message_id: e,
+        message_count: count,
+        read_at: read_at
+      )
     end
   end
 

db/migrate/20251217180000_add_message_count_to_message_read_ranges.rb

@@ -0,0 +1,36 @@
+class AddMessageCountToMessageReadRanges < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  class MigrationMessageReadRange < ApplicationRecord
+    self.table_name = "message_read_ranges"
+  end
+
+  class MigrationMessage < ApplicationRecord
+    self.table_name = "messages"
+  end
+
+  def up
+    add_column :message_read_ranges, :message_count, :integer, default: 0, null: false
+
+    say_with_time "Backfilling message_count for existing message_read_ranges" do
+      MigrationMessageReadRange.reset_column_information
+
+      MigrationMessageReadRange.find_in_batches(batch_size: 500) do |batch|
+        updates = batch.map do |range|
+          count = MigrationMessage.where(topic_id: range.topic_id, id: range.range_start_message_id..range.range_end_message_id).count
+          [range.id, count]
+        end
+
+        updates.each_slice(100) do |slice|
+          slice.each do |id, count|
+            MigrationMessageReadRange.where(id: id).update_all(message_count: count)
+          end
+        end
+      end
+    end
+  end
+
+  def down
+    remove_column :message_read_ranges, :message_count
+  end
+end

db/schema.rb

@@ -28,7 +28,6 @@
 
       it "shows topics with most recent activity first" do
         get topics_path
-        # topic2 has more recent message, should appear first
         topic1_position = response.body.index(topic1.title)
         topic2_position = response.body.index(topic2.title)
         expect(topic2_position).to be < topic1_position
@@ -76,7 +75,6 @@
       it "shows flat view (oldest first)" do
         get topic_path(topic)
         expect(response.body).to include('messages-container flat')
-        # root should appear before reply
         root_position = response.body.index(root_message.body)
         reply_position = response.body.index(reply_message.body)
         expect(root_position).to be < reply_position