Browse Source

Fix home regeneration (#6251)

* Fix regeneration marker not being removed after completion

* Return HTTP 206 from /api/v1/timelines/home if regeneration in progress
Prioritize RegenerationWorker by putting it into default queue

* Display loading indicator and poll home timeline while it regenerates

* Add graphic to regeneration message

* Make "not found" indicator consistent with home regeneration
Eugen Rochko 6 years ago
parent
commit
7badad7797

+ 9 - 1
app/controllers/api/v1/timelines/home_controller.rb

@@ -9,7 +9,11 @@ class Api::V1::Timelines::HomeController < Api::BaseController
 
   def show
     @statuses = load_statuses
-    render json: @statuses, each_serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new(@statuses, current_user&.account_id)
+
+    render json: @statuses,
+           each_serializer: REST::StatusSerializer,
+           relationships: StatusRelationshipsPresenter.new(@statuses, current_user&.account_id),
+           status: regeneration_in_progress? ? 206 : 200
   end
 
   private
@@ -57,4 +61,8 @@ class Api::V1::Timelines::HomeController < Api::BaseController
   def pagination_since_id
     @statuses.first.id
   end
+
+  def regeneration_in_progress?
+    Redis.current.exists("account:#{current_account.id}:regeneration")
+  end
 end

BIN
app/javascript/images/elephant-friend.png


File diff suppressed because it is too large
+ 0 - 0
app/javascript/images/elephant_ui_disappointed.svg


File diff suppressed because it is too large
+ 0 - 0
app/javascript/images/elephant_ui_working.svg


BIN
app/javascript/images/mastodon-not-found.png


+ 9 - 4
app/javascript/mastodon/actions/timelines.js

@@ -19,13 +19,14 @@ export const TIMELINE_DISCONNECT = 'TIMELINE_DISCONNECT';
 
 export const TIMELINE_CONTEXT_UPDATE = 'CONTEXT_UPDATE';
 
-export function refreshTimelineSuccess(timeline, statuses, skipLoading, next) {
+export function refreshTimelineSuccess(timeline, statuses, skipLoading, next, partial) {
   return {
     type: TIMELINE_REFRESH_SUCCESS,
     timeline,
     statuses,
     skipLoading,
     next,
+    partial,
   };
 };
 
@@ -88,7 +89,7 @@ export function refreshTimeline(timelineId, path, params = {}) {
   return function (dispatch, getState) {
     const timeline = getState().getIn(['timelines', timelineId], ImmutableMap());
 
-    if (timeline.get('isLoading') || timeline.get('online')) {
+    if (timeline.get('isLoading') || (timeline.get('online') && !timeline.get('isPartial'))) {
       return;
     }
 
@@ -104,8 +105,12 @@ export function refreshTimeline(timelineId, path, params = {}) {
     dispatch(refreshTimelineRequest(timelineId, skipLoading));
 
     api(getState).get(path, { params }).then(response => {
-      const next = getLinks(response).refs.find(link => link.rel === 'next');
-      dispatch(refreshTimelineSuccess(timelineId, response.data, skipLoading, next ? next.uri : null));
+      if (response.status === 206) {
+        dispatch(refreshTimelineSuccess(timelineId, [], skipLoading, null, true));
+      } else {
+        const next = getLinks(response).refs.find(link => link.rel === 'next');
+        dispatch(refreshTimelineSuccess(timelineId, response.data, skipLoading, next ? next.uri : null, false));
+      }
     }).catch(error => {
       dispatch(refreshTimelineFail(timelineId, error, skipLoading));
     });

+ 7 - 2
app/javascript/mastodon/components/missing_indicator.js

@@ -2,9 +2,14 @@ import React from 'react';
 import { FormattedMessage } from 'react-intl';
 
 const MissingIndicator = () => (
-  <div className='missing-indicator'>
+  <div className='regeneration-indicator missing-indicator'>
     <div>
-      <FormattedMessage id='missing_indicator.label' defaultMessage='Not found' />
+      <div className='regeneration-indicator__figure' />
+
+      <div className='regeneration-indicator__label'>
+        <FormattedMessage id='missing_indicator.label' tagName='strong' defaultMessage='Not found' />
+        <FormattedMessage id='missing_indicator.sublabel' defaultMessage='This resource could not be found' />
+      </div>
     </div>
   </div>
 );

+ 19 - 2
app/javascript/mastodon/components/status_list.js

@@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
 import StatusContainer from '../containers/status_container';
 import ImmutablePureComponent from 'react-immutable-pure-component';
 import ScrollableList from './scrollable_list';
+import { FormattedMessage } from 'react-intl';
 
 export default class StatusList extends ImmutablePureComponent {
 
@@ -16,6 +17,7 @@ export default class StatusList extends ImmutablePureComponent {
     trackScroll: PropTypes.bool,
     shouldUpdateScroll: PropTypes.func,
     isLoading: PropTypes.bool,
+    isPartial: PropTypes.bool,
     hasMore: PropTypes.bool,
     prepend: PropTypes.node,
     emptyMessage: PropTypes.node,
@@ -48,8 +50,23 @@ export default class StatusList extends ImmutablePureComponent {
   }
 
   render () {
-    const { statusIds, ...other } = this.props;
-    const { isLoading } = other;
+    const { statusIds, ...other }  = this.props;
+    const { isLoading, isPartial } = other;
+
+    if (isPartial) {
+      return (
+        <div className='regeneration-indicator'>
+          <div>
+            <div className='regeneration-indicator__figure' />
+
+            <div className='regeneration-indicator__label'>
+              <FormattedMessage id='regeneration_indicator.label' tagName='strong' defaultMessage='Loading&hellip;' />
+              <FormattedMessage id='regeneration_indicator.sublabel' defaultMessage='Your home feed is being prepared!' />
+            </div>
+          </div>
+        </div>
+      );
+    }
 
     const scrollableContent = (isLoading || statusIds.size > 0) ? (
       statusIds.map((statusId) => (

+ 36 - 1
app/javascript/mastodon/features/home_timeline/index.js

@@ -1,6 +1,6 @@
 import React from 'react';
 import { connect } from 'react-redux';
-import { expandHomeTimeline } from '../../actions/timelines';
+import { expandHomeTimeline, refreshHomeTimeline } from '../../actions/timelines';
 import PropTypes from 'prop-types';
 import StatusListContainer from '../ui/containers/status_list_container';
 import Column from '../../components/column';
@@ -16,6 +16,7 @@ const messages = defineMessages({
 
 const mapStateToProps = state => ({
   hasUnread: state.getIn(['timelines', 'home', 'unread']) > 0,
+  isPartial: state.getIn(['timelines', 'home', 'isPartial'], false),
 });
 
 @connect(mapStateToProps)
@@ -26,6 +27,7 @@ export default class HomeTimeline extends React.PureComponent {
     dispatch: PropTypes.func.isRequired,
     intl: PropTypes.object.isRequired,
     hasUnread: PropTypes.bool,
+    isPartial: PropTypes.bool,
     columnId: PropTypes.string,
     multiColumn: PropTypes.bool,
   };
@@ -57,6 +59,39 @@ export default class HomeTimeline extends React.PureComponent {
     this.props.dispatch(expandHomeTimeline());
   }
 
+  componentDidMount () {
+    this._checkIfReloadNeeded(false, this.props.isPartial);
+  }
+
+  componentDidUpdate (prevProps) {
+    this._checkIfReloadNeeded(prevProps.isPartial, this.props.isPartial);
+  }
+
+  componentWillUnmount () {
+    this._stopPolling();
+  }
+
+  _checkIfReloadNeeded (wasPartial, isPartial) {
+    const { dispatch } = this.props;
+
+    if (wasPartial === isPartial) {
+      return;
+    } else if (!wasPartial && isPartial) {
+      this.polling = setInterval(() => {
+        dispatch(refreshHomeTimeline());
+      }, 3000);
+    } else if (wasPartial && !isPartial) {
+      this._stopPolling();
+    }
+  }
+
+  _stopPolling () {
+    if (this.polling) {
+      clearInterval(this.polling);
+      this.polling = null;
+    }
+  }
+
   render () {
     const { intl, hasUnread, columnId, multiColumn } = this.props;
     const pinned = !!columnId;

+ 6 - 2
app/javascript/mastodon/features/list_timeline/index.js

@@ -120,13 +120,17 @@ export default class ListTimeline extends React.PureComponent {
     if (typeof list === 'undefined') {
       return (
         <Column>
-          <LoadingIndicator />
+          <div className='scrollable'>
+            <LoadingIndicator />
+          </div>
         </Column>
       );
     } else if (list === false) {
       return (
         <Column>
-          <MissingIndicator />
+          <div className='scrollable'>
+            <MissingIndicator />
+          </div>
         </Column>
       );
     }

+ 1 - 0
app/javascript/mastodon/features/ui/containers/status_list_container.js

@@ -47,6 +47,7 @@ const makeMapStateToProps = () => {
   const mapStateToProps = (state, { timelineId }) => ({
     statusIds: getStatusIds(state, { type: timelineId }),
     isLoading: state.getIn(['timelines', timelineId, 'isLoading'], true),
+    isPartial: state.getIn(['timelines', timelineId, 'isPartial'], false),
     hasMore: !!state.getIn(['timelines', timelineId, 'next']),
   });
 

+ 3 - 2
app/javascript/mastodon/reducers/timelines.js

@@ -30,7 +30,7 @@ const initialTimeline = ImmutableMap({
   items: ImmutableList(),
 });
 
-const normalizeTimeline = (state, timeline, statuses, next) => {
+const normalizeTimeline = (state, timeline, statuses, next, isPartial) => {
   const oldIds    = state.getIn([timeline, 'items'], ImmutableList());
   const ids       = ImmutableList(statuses.map(status => status.get('id'))).filter(newId => !oldIds.includes(newId));
   const wasLoaded = state.getIn([timeline, 'loaded']);
@@ -41,6 +41,7 @@ const normalizeTimeline = (state, timeline, statuses, next) => {
     mMap.set('isLoading', false);
     if (!hadNext) mMap.set('next', next);
     mMap.set('items', wasLoaded ? ids.concat(oldIds) : ids);
+    mMap.set('isPartial', isPartial);
   }));
 };
 
@@ -124,7 +125,7 @@ export default function timelines(state = initialState, action) {
   case TIMELINE_EXPAND_FAIL:
     return state.update(action.timeline, initialTimeline, map => map.set('isLoading', false));
   case TIMELINE_REFRESH_SUCCESS:
-    return normalizeTimeline(state, action.timeline, fromJS(action.statuses), action.next);
+    return normalizeTimeline(state, action.timeline, fromJS(action.statuses), action.next, action.partial);
   case TIMELINE_EXPAND_SUCCESS:
     return appendNormalizedTimeline(state, action.timeline, fromJS(action.statuses), action.next);
   case TIMELINE_UPDATE:

+ 39 - 4
app/javascript/styles/mastodon/components.scss

@@ -2303,7 +2303,7 @@
   }
 }
 
-.missing-indicator {
+.regeneration-indicator {
   text-align: center;
   font-size: 16px;
   font-weight: 500;
@@ -2314,11 +2314,46 @@
   flex: 1 1 auto;
   align-items: center;
   justify-content: center;
+  padding: 20px;
 
   & > div {
-    background: url('../images/mastodon-not-found.png') no-repeat center -50px;
-    padding-top: 210px;
     width: 100%;
+    background: transparent;
+    padding-top: 0;
+  }
+
+  &__figure {
+    background: url('../images/elephant_ui_working.svg') no-repeat center 0;
+    width: 100%;
+    height: 160px;
+    background-size: contain;
+    position: absolute;
+    top: 50%;
+    left: 50%;
+    transform: translate(-50%, -50%);
+  }
+
+  &.missing-indicator {
+    padding-top: 20px + 48px;
+
+    .regeneration-indicator__figure {
+      background-image: url('../images/elephant_ui_disappointed.svg');
+    }
+  }
+
+  &__label {
+    margin-top: 200px;
+
+    strong {
+      display: block;
+      margin-bottom: 10px;
+      color: lighten($ui-base-color, 34%);
+    }
+
+    span {
+      font-size: 15px;
+      font-weight: 400;
+    }
   }
 }
 
@@ -2749,7 +2784,6 @@
 @keyframes heartbeat {
   from {
     transform: scale(1);
-    transform-origin: center center;
     animation-timing-function: ease-out;
   }
 
@@ -2775,6 +2809,7 @@
 }
 
 .pulse-loading {
+  transform-origin: center center;
   animation: heartbeat 1.5s ease-in-out infinite both;
 }
 

+ 1 - 0
app/services/precompute_feed_service.rb

@@ -3,5 +3,6 @@
 class PrecomputeFeedService < BaseService
   def call(account)
     FeedManager.instance.populate_feed(account)
+    Redis.current.del("account:#{account.id}:regeneration")
   end
 end

+ 1 - 1
app/workers/regeneration_worker.rb

@@ -3,7 +3,7 @@
 class RegenerationWorker
   include Sidekiq::Worker
 
-  sidekiq_options queue: 'pull', backtrace: true, unique: :until_executed
+  sidekiq_options unique: :until_executed
 
   def perform(account_id, _ = :home)
     account = Account.find(account_id)

+ 32 - 8
spec/controllers/concerns/user_tracking_concern_spec.rb

@@ -43,15 +43,39 @@ describe ApplicationController, type: :controller do
       expect_updated_sign_in_at(user)
     end
 
-    it 'regenerates feed when sign in is older than two weeks' do
-      allow(RegenerationWorker).to receive(:perform_async)
-      user.update(current_sign_in_at: 3.weeks.ago)
-      sign_in user, scope: :user
-      get :show
+    describe 'feed regeneration' do
+      before do
+        alice = Fabricate(:account)
+        bob   = Fabricate(:account)
 
-      expect_updated_sign_in_at(user)
-      expect(Redis.current.get("account:#{user.account_id}:regeneration")).to eq 'true'
-      expect(RegenerationWorker).to have_received(:perform_async)
+        user.account.follow!(alice)
+        user.account.follow!(bob)
+
+        Fabricate(:status, account: alice, text: 'hello world')
+        Fabricate(:status, account: bob, text: 'yes hello')
+        Fabricate(:status, account: user.account, text: 'test')
+
+        user.update(last_sign_in_at: 'Tue, 04 Jul 2017 14:45:56 UTC +00:00', current_sign_in_at: 'Wed, 05 Jul 2017 22:10:52 UTC +00:00')
+
+        sign_in user, scope: :user
+      end
+
+      it 'sets a regeneration marker while regenerating' do
+        allow(RegenerationWorker).to receive(:perform_async)
+        get :show
+
+        expect_updated_sign_in_at(user)
+        expect(Redis.current.get("account:#{user.account_id}:regeneration")).to eq 'true'
+        expect(RegenerationWorker).to have_received(:perform_async)
+      end
+
+      it 'regenerates feed when sign in is older than two weeks' do
+        get :show
+
+        expect_updated_sign_in_at(user)
+        expect(Redis.current.zcard(FeedManager.instance.key(:home, user.account_id))).to eq 3
+        expect(Redis.current.get("account:#{user.account_id}:regeneration")).to be_nil
+      end
     end
 
     def expect_updated_sign_in_at(user)

Some files were not shown because too many files changed in this diff