FIX: auto fill was not happening on first load (#21809)

This commit attempts to fix the case where the messages loaded initially don't fill the screen. It would prevent user to scroll and as a result to load more.

There are multiple fixes in this commit:
- the main fix is removing this code which was preventing the actual fill:

```javascript
        // prevents an edge case where user clicks bottom arrow
        // just after scrolling to top
        if (loadingPast && this.#isAtBottom()) {
          return;
        }
```

- ensures we always give a page site to the `chatApi.channel(...)` call if we have one, in the current state when `fetchFromLastRead` was `true` we would not set `args.page_size`
- ensures the `query_paginated_messages` is having a valid page size, which is not nil and not > `MAX_PAGE_SIZE`
- write a spec for the autofill, it was a challenging spec to write but it should give us the confidence we need here

plugins/chat/app/queries/chat/messages_query.rb

@@ -125,6 +125,8 @@ module Chat
       messages,
       target_message_id = nil
     )
+      page_size = [page_size || MAX_PAGE_SIZE, MAX_PAGE_SIZE].min
+
       if target_message_id.present?
         condition = direction == PAST ? "<" : ">"
         messages = messages.where("id #{condition} ?", target_message_id.to_i)

plugins/chat/assets/javascripts/discourse/components/chat-channel.js

@@ -94,7 +94,7 @@ export default class ChatLivePane extends Component {
 
   @action
   didResizePane() {
-    this.fillPaneAttempt();
+    this.debounceFillPaneAttempt();
     this.computeDatesSeparators();
     this.forceRendering();
   }
@@ -228,7 +228,7 @@ export default class ChatLivePane extends Component {
         this.loadedOnce = true;
         this.requestedTargetMessageId = null;
         this.loadingMorePast = false;
-        this.fillPaneAttempt();
+        this.debounceFillPaneAttempt();
         this.updateLastReadMessage();
         this.subscribeToUpdates(this.args.channel);
       });
@@ -276,12 +276,6 @@ export default class ChatLivePane extends Component {
           return;
         }
 
-        // prevents an edge case where user clicks bottom arrow
-        // just after scrolling to top
-        if (loadingPast && this.#isAtBottom()) {
-          return;
-        }
-
         const [messages, meta] = this.afterFetchCallback(
           this.args.channel,
           result
@@ -318,11 +312,23 @@ export default class ChatLivePane extends Component {
       .catch(this._handleErrors)
       .finally(() => {
         this[loadingMoreKey] = false;
-        this.fillPaneAttempt();
+        this.debounceFillPaneAttempt();
       });
   }
 
-  @debounce(500)
+  debounceFillPaneAttempt() {
+    if (!this.loadedOnce) {
+      return;
+    }
+
+    this._debouncedFillPaneAttemptHandler = discourseDebounce(
+      this,
+      this.fillPaneAttempt,
+      500
+    );
+  }
+
+  @bind
   fillPaneAttempt() {
     if (this._selfDeleted) {
       return;
@@ -333,11 +339,12 @@ export default class ChatLivePane extends Component {
       return;
     }
 
-    if (!this.args.channel?.messagesManager?.canLoadMorePast) {
+    if (!this.args.channel?.canLoadMorePast) {
       return;
     }
 
-    const firstMessage = this.args.channel?.messages?.[0];
+    const firstMessage = this.args.channel?.messages?.firstObject;
+
     if (!firstMessage?.visible) {
       return;
     }
@@ -1055,6 +1062,7 @@ export default class ChatLivePane extends Component {
   }
 
   #cancelHandlers() {
+    cancel(this._debouncedFillPaneAttemptHandler);
     cancel(this._onScrollEndedHandler);
     cancel(this._laterComputeHandler);
     cancel(this._debounceFetchMessagesHandler);

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -24,14 +24,13 @@ export default class ChatApi extends Service {
    */
   channel(channelId, data = {}) {
     const args = {};
+    args.page_size = data.pageSize;
 
     if (data.targetMessageId) {
       args.target_message_id = data.targetMessageId;
     } else if (data.fetchFromLastRead) {
       args.fetch_from_last_read = true;
     } else {
-      args.page_size = data.pageSize;
-
       if (data.direction) {
         args.direction = data.direction;
       }

plugins/chat/spec/queries/chat/messages_query_spec.rb

@@ -71,6 +71,11 @@ RSpec.describe Chat::MessagesQuery do
       end
     end
 
+    it "limits results of paginated query when page_size is not set" do
+      options[:target_message_id] = nil
+      stub_const(described_class, "MAX_PAGE_SIZE", 1) { expect(subject[:messages].length).to eq(1) }
+    end
+
     describe "when some messages are in threads" do
       fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
 

plugins/chat/spec/system/chat_channel_spec.rb

@@ -7,15 +7,41 @@ RSpec.describe "Chat channel", type: :system, js: true do
 
   let(:chat) { PageObjects::Pages::Chat.new }
   let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+  let(:sidebar_page) { PageObjects::Pages::Sidebar.new }
 
-  before { chat_system_bootstrap }
+  before do
+    chat_system_bootstrap
+    channel_1.add(current_user)
+    sign_in(current_user)
+  end
 
-  context "when sending a message" do
+  context "when first batch of messages doesnt fill page" do
     before do
-      channel_1.add(current_user)
+      50.times do
-      sign_in(current_user)
+        Fabricate(
+          :chat_message,
+          message: Faker::Lorem.characters(number: SiteSetting.chat_minimum_message_length),
+          user: current_user,
+          chat_channel: channel_1,
+        )
+      end
     end
 
+    it "autofills for more messages" do
+      chat.prefers_full_page
+      visit("/")
+      # cheap trick to ensure the messages don't fill the initial page
+      page.execute_script(
+        "document.head.insertAdjacentHTML('beforeend', `<style>.chat-message-text{font-size:3px;}</style>`)",
+      )
+      sidebar_page.open_channel(channel_1)
+
+      expect(channel_page).to have_no_loading_skeleton
+      expect(channel_page.messages).to have_x_messages(51)
+    end
+  end
+
+  context "when sending a message" do
     context "with lots of messages" do
       before { 50.times { Fabricate(:chat_message, chat_channel: channel_1) } }
 
@@ -71,11 +97,7 @@ RSpec.describe "Chat channel", type: :system, js: true do
   end
 
   context "when clicking the arrow button" do
-    before do
+    before { 50.times { Fabricate(:chat_message, chat_channel: channel_1) } }
-      channel_1.add(current_user)
-      50.times { Fabricate(:chat_message, chat_channel: channel_1) }
-      sign_in(current_user)
-    end
 
     it "jumps to the bottom of the channel" do
       unloaded_message = Fabricate(:chat_message, chat_channel: channel_1)
@@ -92,11 +114,6 @@ RSpec.describe "Chat channel", type: :system, js: true do
   end
 
   context "when returning to a channel where last read is not last message" do
-    before do
-      channel_1.add(current_user)
-      sign_in(current_user)
-    end
-
     it "jumps to the bottom of the channel" do
       channel_1.membership_for(current_user).update!(last_read_message: message_1)
       messages = 50.times.map { Fabricate(:chat_message, chat_channel: channel_1) }
@@ -112,9 +129,7 @@ RSpec.describe "Chat channel", type: :system, js: true do
 
     before do
       channel_1.add(other_user)
-      channel_1.add(current_user)
       50.times { Fabricate(:chat_message, chat_channel: channel_1) }
-      sign_in(current_user)
     end
 
     it "doesn’t scroll the pane" do
@@ -142,11 +157,7 @@ RSpec.describe "Chat channel", type: :system, js: true do
       )
     end
 
-    before do
+    before { channel_1.add(other_user) }
-      channel_1.add(other_user)
-      channel_1.add(current_user)
-      sign_in(current_user)
-    end
 
     it "highlights the mentions" do
       chat.visit_channel(channel_1)
@@ -185,8 +196,6 @@ RSpec.describe "Chat channel", type: :system, js: true do
     before do
       Fabricate(:chat_message, in_reply_to: message_1, user: other_user, chat_channel: channel_1)
       channel_1.add(other_user)
-      channel_1.add(current_user)
-      sign_in(current_user)
     end
 
     it "doesn’t show the reply-to line" do
@@ -203,8 +212,6 @@ RSpec.describe "Chat channel", type: :system, js: true do
       Fabricate(:chat_message, user: other_user, chat_channel: channel_1)
       Fabricate(:chat_message, in_reply_to: message_1, user: other_user, chat_channel: channel_1)
       channel_1.add(other_user)
-      channel_1.add(current_user)
-      sign_in(current_user)
     end
 
     it "shows the reply-to line" do
@@ -229,8 +236,6 @@ RSpec.describe "Chat channel", type: :system, js: true do
       Fabricate(:chat_message, user: other_user, chat_channel: channel_1)
       Fabricate(:chat_message, in_reply_to: message_2, user: current_user, chat_channel: channel_1)
       channel_1.add(other_user)
-      channel_1.add(current_user)
-      sign_in(current_user)
     end
 
     it "renders text in the reply-to" do
@@ -241,12 +246,7 @@ RSpec.describe "Chat channel", type: :system, js: true do
   end
 
   context "when messages are separated by a day" do
-    before do
+    before { Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.days.ago) }
-      Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.days.ago)
-
-      channel_1.add(current_user)
-      sign_in(current_user)
-    end
 
     it "shows a date separator" do
       chat.visit_channel(channel_1)
@@ -264,11 +264,6 @@ RSpec.describe "Chat channel", type: :system, js: true do
       \`\`\`
       MESSAGE
 
-    before do
-      channel_1.add(current_user)
-      sign_in(current_user)
-    end
-
     it "adds the correct lang" do
       chat.visit_channel(channel_1)
 
@@ -277,11 +272,7 @@ RSpec.describe "Chat channel", type: :system, js: true do
   end
 
   context "when scrolling" do
-    before do
+    before { 50.times { Fabricate(:chat_message, chat_channel: channel_1) } }
-      channel_1.add(current_user)
-      50.times { Fabricate(:chat_message, chat_channel: channel_1) }
-      sign_in(current_user)
-    end
 
     it "resets the active message" do
       chat.visit_channel(channel_1)

plugins/chat/spec/system/page_objects/chat/components/messages.rb

@@ -12,6 +12,10 @@ module PageObjects
           @context = context
         end
 
+        def component
+          find(context)
+        end
+
         def has_message?(**args)
           PageObjects::Components::Chat::Message.new(".chat-channel").exists?(**args)
         end
@@ -19,6 +23,10 @@ module PageObjects
         def has_no_message?(**args)
           !has_message?(**args)
         end
+
+        def has_x_messages?(count)
+          find(context).has_css?(SELECTOR, count: count, visible: :all)
+        end
       end
     end
   end