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.

diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb
index 28c28592ade93645fd51b4e8ca0ffa19aa9a7d11..e982413236c490e930e7bc2d1384cf9f9101243d 100644 (file)
--- a/app/controllers/api/v1/statuses_controller.rb
+++ b/app/controllers/api/v1/statuses_controller.rb
@@ -17,7 +17,7 @@ class Api::V1::StatusesController < Api::BaseController
   end
 
   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)
     loaded_ancestors    = cache_collection(ancestors_results, Status)
     loaded_descendants  = cache_collection(descendants_results, Status)

diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb
index 45226c8d2ae4edb75c398ad19ecfc1b322d716c0..41f098a43761f2dc70a743db51f76b3667fc2e48 100644 (file)
--- a/app/controllers/statuses_controller.rb
+++ b/app/controllers/statuses_controller.rb
@@ -4,6 +4,8 @@ class StatusesController < ApplicationController
   include SignatureAuthentication
   include Authorization
 
+  ANCESTORS_LIMIT = 20
+
   layout 'public'
 
   before_action :set_account
@@ -16,8 +18,9 @@ class StatusesController < ApplicationController
   def show
     respond_to do |format|
       format.html do
-        @ancestors   = @status.reply? ? cache_collection(@status.ancestors(current_account), Status) : []
-        @descendants = cache_collection(@status.descendants(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)
 
         render 'stream_entries/show'
       end

diff --git a/app/javascript/styles/mastodon/stream_entries.scss b/app/javascript/styles/mastodon/stream_entries.scss
index 442b143a0488bae93e4153074fbb81ddc6d92003..dfdc48d06106c635706e48f4834a718e3957421e 100644 (file)
--- a/app/javascript/styles/mastodon/stream_entries.scss
+++ b/app/javascript/styles/mastodon/stream_entries.scss
@@ -6,7 +6,8 @@
     background: $simple-background-color;
 
     .detailed-status.light,
-    .status.light {
+    .status.light,
+    .more.light {
       border-bottom: 1px solid $ui-secondary-color;
       animation: none;
     }
@@ -290,6 +291,17 @@
       text-decoration: underline;
     }
   }
+
+  .more {
+    color: $classic-primary-color;
+    display: block;
+    padding: 14px;
+    text-align: center;
+
+    &:not(:hover) {
+      text-decoration: none;
+    }
+  }
 }
 
 .embed {

diff --git a/app/models/concerns/status_threading_concern.rb b/app/models/concerns/status_threading_concern.rb
index b539ba10e2dd27e56499450c1bcb1fc7ca8ef050..fffc095ee0f9ef8b4f4db14982b26ded779853a1 100644 (file)
--- a/app/models/concerns/status_threading_concern.rb
+++ b/app/models/concerns/status_threading_concern.rb
@@ -3,8 +3,8 @@
 module StatusThreadingConcern
   extend ActiveSupport::Concern
 
-  def ancestors(account = nil)
-    find_statuses_from_tree_path(ancestor_ids, account)
+  def ancestors(limit, account = nil)
+    find_statuses_from_tree_path(ancestor_ids(limit), account)
   end
 
   def descendants(account = nil)
@@ -13,14 +13,21 @@ module StatusThreadingConcern
 
   private
 
-  def ancestor_ids
-    Rails.cache.fetch("ancestors:#{id}") do
-      ancestor_statuses.pluck(:id)
+  def ancestor_ids(limit)
+    key = "ancestors:#{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
 
-  def ancestor_statuses
-    Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id])
+  def ancestor_statuses(limit)
+    Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id, limit: limit])
       WITH RECURSIVE search_tree(id, in_reply_to_id, path)
       AS (
         SELECT id, in_reply_to_id, ARRAY[id]
@@ -34,7 +41,8 @@ module StatusThreadingConcern
       )
       SELECT id
       FROM search_tree
-      ORDER BY path DESC
+      ORDER BY path
+      LIMIT :limit
     SQL
   end
 

diff --git a/app/views/stream_entries/_status.html.haml b/app/views/stream_entries/_status.html.haml
index e2e1fdd1216037b9513e77a1b63e9f6a8f7bb016..2d0dafcb7fd55343274917846221d349512302e5 100644 (file)
--- a/app/views/stream_entries/_status.html.haml
+++ b/app/views/stream_entries/_status.html.haml
@@ -14,6 +14,10 @@
   entry_classes = h_class + ' ' + mf_classes + ' ' + style_classes
 
 - 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 }
 
 .entry{ class: entry_classes }

diff --git a/spec/models/concerns/status_threading_concern_spec.rb b/spec/models/concerns/status_threading_concern_spec.rb
index 62f5f6e317e295157f4163657bac69b29f7b3197..b8ebdd58cdafe9d8c9f1f4bb4a62fae56efbebea 100644 (file)
--- a/spec/models/concerns/status_threading_concern_spec.rb
+++ b/spec/models/concerns/status_threading_concern_spec.rb
@@ -14,34 +14,34 @@ describe StatusThreadingConcern do
     let!(:viewer) { Fabricate(:account, username: 'viewer') }
 
     it 'returns conversation history' do
-      expect(reply3.ancestors).to include(status, reply1, reply2)
+      expect(reply3.ancestors(4)).to include(status, reply1, reply2)
     end
 
     it 'does not return conversation history user is not allowed to see' do
       reply1.update(visibility: :private)
       status.update(visibility: :direct)
 
-      expect(reply3.ancestors(viewer)).to_not include(reply1, status)
+      expect(reply3.ancestors(4, viewer)).to_not include(reply1, status)
     end
 
     it 'does not return conversation history from blocked users' do
       viewer.block!(jeff)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
+      expect(reply3.ancestors(4, viewer)).to_not include(reply1)
     end
 
     it 'does not return conversation history from muted users' do
       viewer.mute!(jeff)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
+      expect(reply3.ancestors(4, viewer)).to_not include(reply1)
     end
 
     it 'does not return conversation history from silenced and not followed users' do
       jeff.update(silenced: true)
-      expect(reply3.ancestors(viewer)).to_not include(reply1)
+      expect(reply3.ancestors(4, viewer)).to_not include(reply1)
     end
 
     it 'does not return conversation history from blocked domains' do
       viewer.block_domain!('example.com')
-      expect(reply3.ancestors(viewer)).to_not include(reply2)
+      expect(reply3.ancestors(4, viewer)).to_not include(reply2)
     end
 
     it 'ignores deleted records' do
@@ -49,10 +49,32 @@ describe StatusThreadingConcern do
       second_status = Fabricate(:status, thread: first_status, account: alice)
 
       # Create cache and delete cached record
-      second_status.ancestors
+      second_status.ancestors(4)
       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
 

diff --git a/spec/views/stream_entries/show.html.haml_spec.rb b/spec/views/stream_entries/show.html.haml_spec.rb
index 59ea409907d27e123cef4a57b03eb42cd7bb8376..6074bbc2efafc4f44c7f0cf6603992447ec340ac 100644 (file)
--- a/spec/views/stream_entries/show.html.haml_spec.rb
+++ b/spec/views/stream_entries/show.html.haml_spec.rb
@@ -48,7 +48,7 @@ describe 'stream_entries/show.html.haml', without_verify_partial_doubles: true d
     assign(:stream_entry, reply.stream_entry)
     assign(:account, alice)
     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))
 
     render