Paginate ancestor statuses in public page (#7102)

This also limits the statuses returned by API, but pagination is not
implemented in Web API yet. I still expect it brings user experience
better than making a user wait to fetch all ancestor statuses and flooding
the column with them.
This commit is contained in:
Akihiko Odaki 2018-04-11 19:35:09 +09:00 committed by Eugen Rochko
parent d9b62e34da
commit 519119f657
7 changed files with 70 additions and 21 deletions

View file

@ -17,7 +17,7 @@ class Api::V1::StatusesController < Api::BaseController
end end
def context def context
ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(current_account) ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(DEFAULT_STATUSES_LIMIT, current_account)
descendants_results = @status.descendants(current_account) descendants_results = @status.descendants(current_account)
loaded_ancestors = cache_collection(ancestors_results, Status) loaded_ancestors = cache_collection(ancestors_results, Status)
loaded_descendants = cache_collection(descendants_results, Status) loaded_descendants = cache_collection(descendants_results, Status)

View file

@ -4,6 +4,8 @@ class StatusesController < ApplicationController
include SignatureAuthentication include SignatureAuthentication
include Authorization include Authorization
ANCESTORS_LIMIT = 20
layout 'public' layout 'public'
before_action :set_account before_action :set_account
@ -16,7 +18,8 @@ class StatusesController < ApplicationController
def show def show
respond_to do |format| respond_to do |format|
format.html do format.html do
@ancestors = @status.reply? ? cache_collection(@status.ancestors(current_account), Status) : [] @ancestors = @status.reply? ? cache_collection(@status.ancestors(ANCESTORS_LIMIT, current_account), Status) : []
@next_ancestor = @ancestors.size < ANCESTORS_LIMIT ? nil : @ancestors.shift
@descendants = cache_collection(@status.descendants(current_account), Status) @descendants = cache_collection(@status.descendants(current_account), Status)
render 'stream_entries/show' render 'stream_entries/show'

View file

@ -6,7 +6,8 @@
background: $simple-background-color; background: $simple-background-color;
.detailed-status.light, .detailed-status.light,
.status.light { .status.light,
.more.light {
border-bottom: 1px solid $ui-secondary-color; border-bottom: 1px solid $ui-secondary-color;
animation: none; animation: none;
} }
@ -290,6 +291,17 @@
text-decoration: underline; text-decoration: underline;
} }
} }
.more {
color: $classic-primary-color;
display: block;
padding: 14px;
text-align: center;
&:not(:hover) {
text-decoration: none;
}
}
} }
.embed { .embed {

View file

@ -3,8 +3,8 @@
module StatusThreadingConcern module StatusThreadingConcern
extend ActiveSupport::Concern extend ActiveSupport::Concern
def ancestors(account = nil) def ancestors(limit, account = nil)
find_statuses_from_tree_path(ancestor_ids, account) find_statuses_from_tree_path(ancestor_ids(limit), account)
end end
def descendants(account = nil) def descendants(account = nil)
@ -13,14 +13,21 @@ module StatusThreadingConcern
private private
def ancestor_ids def ancestor_ids(limit)
Rails.cache.fetch("ancestors:#{id}") do key = "ancestors:#{id}"
ancestor_statuses.pluck(:id) ancestors = Rails.cache.fetch(key)
if ancestors.nil? || ancestors[:limit] < limit
ids = ancestor_statuses(limit).pluck(:id).reverse!
Rails.cache.write key, limit: limit, ids: ids
ids
else
ancestors[:ids].last(limit)
end end
end end
def ancestor_statuses def ancestor_statuses(limit)
Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id]) Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id, limit: limit])
WITH RECURSIVE search_tree(id, in_reply_to_id, path) WITH RECURSIVE search_tree(id, in_reply_to_id, path)
AS ( AS (
SELECT id, in_reply_to_id, ARRAY[id] SELECT id, in_reply_to_id, ARRAY[id]
@ -34,7 +41,8 @@ module StatusThreadingConcern
) )
SELECT id SELECT id
FROM search_tree FROM search_tree
ORDER BY path DESC ORDER BY path
LIMIT :limit
SQL SQL
end end

View file

@ -14,6 +14,10 @@
entry_classes = h_class + ' ' + mf_classes + ' ' + style_classes entry_classes = h_class + ' ' + mf_classes + ' ' + style_classes
- if status.reply? && include_threads - if status.reply? && include_threads
- if @next_ancestor
.entry{ class: entry_classes }
= link_to short_account_status_url(@next_ancestor.account.username, @next_ancestor), class: 'more light' do
= t('statuses.show_more')
= render partial: 'stream_entries/status', collection: @ancestors, as: :status, locals: { is_predecessor: true, direct_reply_id: status.in_reply_to_id } = render partial: 'stream_entries/status', collection: @ancestors, as: :status, locals: { is_predecessor: true, direct_reply_id: status.in_reply_to_id }
.entry{ class: entry_classes } .entry{ class: entry_classes }

View file

@ -14,34 +14,34 @@ describe StatusThreadingConcern do
let!(:viewer) { Fabricate(:account, username: 'viewer') } let!(:viewer) { Fabricate(:account, username: 'viewer') }
it 'returns conversation history' do it 'returns conversation history' do
expect(reply3.ancestors).to include(status, reply1, reply2) expect(reply3.ancestors(4)).to include(status, reply1, reply2)
end end
it 'does not return conversation history user is not allowed to see' do it 'does not return conversation history user is not allowed to see' do
reply1.update(visibility: :private) reply1.update(visibility: :private)
status.update(visibility: :direct) status.update(visibility: :direct)
expect(reply3.ancestors(viewer)).to_not include(reply1, status) expect(reply3.ancestors(4, viewer)).to_not include(reply1, status)
end end
it 'does not return conversation history from blocked users' do it 'does not return conversation history from blocked users' do
viewer.block!(jeff) viewer.block!(jeff)
expect(reply3.ancestors(viewer)).to_not include(reply1) expect(reply3.ancestors(4, viewer)).to_not include(reply1)
end end
it 'does not return conversation history from muted users' do it 'does not return conversation history from muted users' do
viewer.mute!(jeff) viewer.mute!(jeff)
expect(reply3.ancestors(viewer)).to_not include(reply1) expect(reply3.ancestors(4, viewer)).to_not include(reply1)
end end
it 'does not return conversation history from silenced and not followed users' do it 'does not return conversation history from silenced and not followed users' do
jeff.update(silenced: true) jeff.update(silenced: true)
expect(reply3.ancestors(viewer)).to_not include(reply1) expect(reply3.ancestors(4, viewer)).to_not include(reply1)
end end
it 'does not return conversation history from blocked domains' do it 'does not return conversation history from blocked domains' do
viewer.block_domain!('example.com') viewer.block_domain!('example.com')
expect(reply3.ancestors(viewer)).to_not include(reply2) expect(reply3.ancestors(4, viewer)).to_not include(reply2)
end end
it 'ignores deleted records' do it 'ignores deleted records' do
@ -49,10 +49,32 @@ describe StatusThreadingConcern do
second_status = Fabricate(:status, thread: first_status, account: alice) second_status = Fabricate(:status, thread: first_status, account: alice)
# Create cache and delete cached record # Create cache and delete cached record
second_status.ancestors second_status.ancestors(4)
first_status.destroy first_status.destroy
expect(second_status.ancestors).to eq([]) expect(second_status.ancestors(4)).to eq([])
end
it 'can return more records than previously requested' do
first_status = Fabricate(:status, account: bob)
second_status = Fabricate(:status, thread: first_status, account: alice)
third_status = Fabricate(:status, thread: second_status, account: alice)
# Create cache
second_status.ancestors(1)
expect(third_status.ancestors(2)).to eq([first_status, second_status])
end
it 'can return fewer records than previously requested' do
first_status = Fabricate(:status, account: bob)
second_status = Fabricate(:status, thread: first_status, account: alice)
third_status = Fabricate(:status, thread: second_status, account: alice)
# Create cache
second_status.ancestors(2)
expect(third_status.ancestors(1)).to eq([second_status])
end end
end end

View file

@ -48,7 +48,7 @@ describe 'stream_entries/show.html.haml', without_verify_partial_doubles: true d
assign(:stream_entry, reply.stream_entry) assign(:stream_entry, reply.stream_entry)
assign(:account, alice) assign(:account, alice)
assign(:type, reply.stream_entry.activity_type.downcase) assign(:type, reply.stream_entry.activity_type.downcase)
assign(:ancestors, reply.stream_entry.activity.ancestors(bob) ) assign(:ancestors, reply.stream_entry.activity.ancestors(1, bob) )
assign(:descendants, reply.stream_entry.activity.descendants(bob)) assign(:descendants, reply.stream_entry.activity.descendants(bob))
render render