mirror of
https://github.com/discourse/discourse.git
synced 2025-05-31 01:17:16 +08:00
FEATURE: thread pagination (#22624)
Prior to this commit we were loading a large number of thread messages without any pagination. This commit attempts to fix this and also improves the following points: - code sharing between channels and threads: Attempts to reuse/share the code use in channels for threads. To make it possible part of this code has been extracted in dedicated helpers or has been improved to reduce the duplication needed. Examples of extracted helpers: - `stackingContextFix`: the ios hack for rendering bug when momentum scrolling is interrupted - `scrollListToMessage`, `scrollListToTop`, `scrollListToBottom`: a series of helper to correctly scroll to a specific position in the list of messages - better general performance of listing messages: One of the main changes which has been made is to remove the computation of visible message during scroll, it will only happen when needed (update last read for example). This constant recomputation of `message.visible` on intersection observer event while scrolling was consuming a lot of CPU time.
This commit is contained in:
Joffrey JAFFEUX
@ -0,0 +1,57 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
require "rails_helper"
|
||||
|
||||
RSpec.describe Chat::Api::ChannelMessagesController do
|
||||
fab!(:current_user) { Fabricate(:user) }
|
||||
fab!(:channel) { Fabricate(:chat_channel) }
|
||||
|
||||
before do
|
||||
SiteSetting.chat_enabled = true
|
||||
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
|
||||
channel.add(current_user)
|
||||
sign_in(current_user)
|
||||
end
|
||||
|
||||
describe "index" do
|
||||
describe "success" do
|
||||
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
|
||||
fab!(:message_2) { Fabricate(:chat_message) }
|
||||
|
||||
it "works" do
|
||||
get "/chat/api/channels/#{channel.id}/messages"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["messages"].map { |m| m["id"] }).to contain_exactly(
|
||||
message_1.id,
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
context "when channnel doesn’t exist" do
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/-999/messages"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
|
||||
context "when target message doesn’t exist" do
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/#{channel.id}/messages?target_message_id=-999"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
|
||||
context "when user can’t see channel" do
|
||||
fab!(:channel) { Fabricate(:private_category_channel) }
|
||||
|
||||
it "returns a 403" do
|
||||
get "/chat/api/channels/#{channel.id}/messages"
|
||||
|
||||
expect(response.status).to eq(403)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -0,0 +1,77 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
require "rails_helper"
|
||||
|
||||
RSpec.describe Chat::Api::ChannelThreadMessagesController do
|
||||
fab!(:current_user) { Fabricate(:user) }
|
||||
fab!(:thread) do
|
||||
Fabricate(:chat_thread, channel: Fabricate(:chat_channel, threading_enabled: true))
|
||||
end
|
||||
|
||||
before do
|
||||
SiteSetting.chat_enabled = true
|
||||
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
|
||||
thread.channel.add(current_user)
|
||||
sign_in(current_user)
|
||||
end
|
||||
|
||||
describe "index" do
|
||||
describe "success" do
|
||||
fab!(:message_1) { Fabricate(:chat_message, thread: thread) }
|
||||
fab!(:message_2) { Fabricate(:chat_message) }
|
||||
|
||||
it "works" do
|
||||
get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["messages"].map { |m| m["id"] }).to contain_exactly(
|
||||
thread.original_message.id,
|
||||
message_1.id,
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
context "when thread doesn’t exist" do
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/#{thread.channel.id}/threads/-999/messages"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
|
||||
context "when target message doesn’t exist" do
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages?target_message_id=-999"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
|
||||
context "when user can’t see channel" do
|
||||
fab!(:thread) do
|
||||
Fabricate(
|
||||
:chat_thread,
|
||||
channel: Fabricate(:private_category_channel, threading_enabled: true),
|
||||
)
|
||||
end
|
||||
|
||||
it "returns a 403" do
|
||||
get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages"
|
||||
|
||||
expect(response.status).to eq(403)
|
||||
end
|
||||
end
|
||||
|
||||
context "when channel disabled threading" do
|
||||
fab!(:thread) do
|
||||
Fabricate(:chat_thread, channel: Fabricate(:chat_channel, threading_enabled: false))
|
||||
end
|
||||
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages"
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
@ -159,396 +159,6 @@ RSpec.describe Chat::Api::ChannelsController do
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "when include_messages is true" do
|
||||
fab!(:current_user) { Fabricate(:user) }
|
||||
fab!(:channel_1) { Fabricate(:category_channel) }
|
||||
fab!(:other_user) { Fabricate(:user) }
|
||||
|
||||
describe "target message lookup" do
|
||||
let!(:message) { Fabricate(:chat_message, chat_channel: channel_1) }
|
||||
let(:chatable) { channel_1.chatable }
|
||||
|
||||
before { sign_in(current_user) }
|
||||
|
||||
context "when the message doesn’t belong to the channel" do
|
||||
let!(:message) { Fabricate(:chat_message) }
|
||||
|
||||
it "returns a 404" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
|
||||
expect(response.status).to eq(404)
|
||||
end
|
||||
end
|
||||
|
||||
context "when the chat channel is for a category" do
|
||||
it "ensures the user can access that category" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id)
|
||||
|
||||
group = Fabricate(:group)
|
||||
chatable.update!(read_restricted: true)
|
||||
Fabricate(:category_group, group: group, category: chatable)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
expect(response.status).to eq(403)
|
||||
|
||||
GroupUser.create!(user: current_user, group: group)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id)
|
||||
end
|
||||
end
|
||||
|
||||
context "when the chat channel is for a direct message channel" do
|
||||
let(:channel_1) { Fabricate(:direct_message_channel) }
|
||||
|
||||
it "ensures the user can access that direct message channel" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
expect(response.status).to eq(403)
|
||||
|
||||
Chat::DirectMessageUser.create!(user: current_user, direct_message: chatable)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
target_message_id: message.id,
|
||||
include_messages: true,
|
||||
}
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "messages pagination and direction" do
|
||||
let(:page_size) { 30 }
|
||||
|
||||
message_count = 70
|
||||
message_count.times do |n|
|
||||
fab!("message_#{n}") do
|
||||
Fabricate(
|
||||
:chat_message,
|
||||
chat_channel: channel_1,
|
||||
user: other_user,
|
||||
message: "message #{n}",
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
before do
|
||||
sign_in(current_user)
|
||||
Group.refresh_automatic_groups!
|
||||
end
|
||||
|
||||
it "errors for user when they are not allowed to chat" do
|
||||
SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:staff]
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.status).to eq(403)
|
||||
end
|
||||
|
||||
it "errors when page size is over the maximum" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: Chat::MessagesQuery::MAX_PAGE_SIZE + 1,
|
||||
}
|
||||
expect(response.status).to eq(400)
|
||||
expect(response.parsed_body["errors"]).to include(
|
||||
"Page size must be less than or equal to #{Chat::MessagesQuery::MAX_PAGE_SIZE}",
|
||||
)
|
||||
end
|
||||
|
||||
it "errors when page size is nil" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json", params: { include_messages: true }
|
||||
expect(response.status).to eq(400)
|
||||
expect(response.parsed_body["errors"]).to include("Page size can't be blank")
|
||||
end
|
||||
|
||||
it "returns the latest messages in created_at, id order" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
messages = response.parsed_body["chat_messages"]
|
||||
expect(messages.count).to eq(page_size)
|
||||
expect(messages.first["id"]).to eq(message_40.id)
|
||||
expect(messages.last["id"]).to eq(message_69.id)
|
||||
end
|
||||
|
||||
it "returns `can_flag=true` for public channels" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_flag"]).to be true
|
||||
end
|
||||
|
||||
it "returns `can_flag=true` for DM channels" do
|
||||
dm_chat_channel = Fabricate(:direct_message_channel, users: [current_user, other_user])
|
||||
get "/chat/api/channels/#{dm_chat_channel.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_flag"]).to be true
|
||||
end
|
||||
|
||||
it "returns `can_moderate=true` based on whether the user can moderate the chatable" do
|
||||
1.upto(4) do |n|
|
||||
current_user.update!(trust_level: n)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_moderate"]).to be false
|
||||
end
|
||||
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_moderate"]).to be false
|
||||
|
||||
current_user.update!(admin: true)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_moderate"]).to be true
|
||||
current_user.update!(admin: false)
|
||||
|
||||
SiteSetting.enable_category_group_moderation = true
|
||||
group = Fabricate(:group)
|
||||
group.add(current_user)
|
||||
channel_1.category.update!(reviewable_by_group: group)
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_moderate"]).to be true
|
||||
end
|
||||
|
||||
it "serializes `user_flag_status` for user who has a pending flag" do
|
||||
chat_message = channel_1.chat_messages.last
|
||||
reviewable = flag_message(chat_message, current_user)
|
||||
score = reviewable.reviewable_scores.last
|
||||
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
|
||||
expect(response.parsed_body["chat_messages"].last["user_flag_status"]).to eq(
|
||||
score.status_for_database,
|
||||
)
|
||||
end
|
||||
|
||||
it "doesn't serialize `reviewable_ids` for non-staff" do
|
||||
reviewable = flag_message(channel_1.chat_messages.last, Fabricate(:admin))
|
||||
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
|
||||
expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to be_nil
|
||||
end
|
||||
|
||||
it "serializes `reviewable_ids` correctly for staff" do
|
||||
admin = Fabricate(:admin)
|
||||
sign_in(admin)
|
||||
reviewable = flag_message(channel_1.chat_messages.last, admin)
|
||||
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to eq(reviewable.id)
|
||||
end
|
||||
|
||||
it "correctly marks reactions as 'reacted' for the current_user" do
|
||||
heart_emoji = ":heart:"
|
||||
smile_emoji = ":smile"
|
||||
last_message = channel_1.chat_messages.last
|
||||
last_message.reactions.create(user: current_user, emoji: heart_emoji)
|
||||
last_message.reactions.create(user: Fabricate(:admin), emoji: smile_emoji)
|
||||
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
|
||||
reactions = response.parsed_body["chat_messages"].last["reactions"]
|
||||
heart_reaction = reactions.find { |r| r["emoji"] == heart_emoji }
|
||||
expect(heart_reaction["reacted"]).to be true
|
||||
smile_reaction = reactions.find { |r| r["emoji"] == smile_emoji }
|
||||
expect(smile_reaction["reacted"]).to be false
|
||||
end
|
||||
|
||||
it "sends the last message bus id for the channel" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
page_size: page_size,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["channel_message_bus_last_id"]).not_to eq(nil)
|
||||
end
|
||||
|
||||
describe "scrolling to the past" do
|
||||
it "returns the correct messages in created_at, id order" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_40.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::PAST,
|
||||
}
|
||||
messages = response.parsed_body["chat_messages"]
|
||||
expect(messages.count).to eq(page_size)
|
||||
expect(messages.first["id"]).to eq(message_10.id)
|
||||
expect(messages.last["id"]).to eq(message_39.id)
|
||||
end
|
||||
|
||||
it "returns 'can_load...' properly when there are more past messages" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_40.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::PAST,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to be true
|
||||
expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil
|
||||
end
|
||||
|
||||
it "returns 'can_load...' properly when there are no past messages" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_3.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::PAST,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to be false
|
||||
expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil
|
||||
end
|
||||
end
|
||||
|
||||
describe "scrolling to the future" do
|
||||
it "returns the correct messages in created_at, id order when there are many after" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_10.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::FUTURE,
|
||||
}
|
||||
messages = response.parsed_body["chat_messages"]
|
||||
expect(messages.count).to eq(page_size)
|
||||
expect(messages.first["id"]).to eq(message_11.id)
|
||||
expect(messages.last["id"]).to eq(message_40.id)
|
||||
end
|
||||
|
||||
it "return 'can_load..' properly when there are future messages" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_10.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::FUTURE,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil
|
||||
expect(response.parsed_body["meta"]["can_load_more_future"]).to be true
|
||||
end
|
||||
|
||||
it "returns 'can_load..' properly when there are no future messages" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
include_messages: true,
|
||||
target_message_id: message_60.id,
|
||||
page_size: page_size,
|
||||
direction: Chat::MessagesQuery::FUTURE,
|
||||
}
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil
|
||||
expect(response.parsed_body["meta"]["can_load_more_future"]).to be false
|
||||
end
|
||||
end
|
||||
|
||||
describe "without direction (latest messages)" do
|
||||
it "signals there are no future messages" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
page_size: page_size,
|
||||
include_messages: true,
|
||||
}
|
||||
|
||||
expect(response.parsed_body["meta"]["can_load_more_future"]).to eq(false)
|
||||
end
|
||||
|
||||
it "signals there are more messages in the past" do
|
||||
get "/chat/api/channels/#{channel_1.id}.json",
|
||||
params: {
|
||||
page_size: page_size,
|
||||
include_messages: true,
|
||||
}
|
||||
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(true)
|
||||
end
|
||||
|
||||
it "signals there are no more messages" do
|
||||
new_channel = Fabricate(:category_channel)
|
||||
Fabricate(
|
||||
:chat_message,
|
||||
chat_channel: new_channel,
|
||||
user: other_user,
|
||||
message: "message",
|
||||
)
|
||||
chat_messages_qty = 1
|
||||
|
||||
get "/chat/api/channels/#{new_channel.id}.json",
|
||||
params: {
|
||||
page_size: chat_messages_qty + 1,
|
||||
include_messages: true,
|
||||
}
|
||||
|
||||
expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(false)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "#destroy" do
|
||||
|
||||
@ -15,13 +15,7 @@ describe ListController do
|
||||
Fabricate(:direct_message_channel, users: [current_user, user_1])
|
||||
public_channel_1 = Fabricate(:chat_channel)
|
||||
public_channel_2 = Fabricate(:chat_channel)
|
||||
|
||||
Fabricate(
|
||||
:user_chat_channel_membership,
|
||||
user: current_user,
|
||||
chat_channel: public_channel_1,
|
||||
following: true,
|
||||
)
|
||||
public_channel_1.add(current_user)
|
||||
|
||||
# warm up
|
||||
get "/latest.html"
|
||||
@ -41,12 +35,7 @@ describe ListController do
|
||||
end
|
||||
end.count
|
||||
|
||||
Fabricate(
|
||||
:user_chat_channel_membership,
|
||||
user: current_user,
|
||||
chat_channel: public_channel_2,
|
||||
following: true,
|
||||
)
|
||||
public_channel_2.add(current_user)
|
||||
user_2 = Fabricate(:user)
|
||||
Fabricate(:direct_message_channel, users: [current_user, user_2])
|
||||
|
||||
|
||||
Reference in New Issue
Block a user