Fix #3665 - Refactor timelines reducer #3686

Gargron · 2017-06-10T15:03:41Z

Before:

timelines
  home
  public
  community
  tag
  accounts_timelines
    3
    6
  accounts_media_timelines
    1
    3
    4

After:

timelines
  home
  public
  community
  hashtag:test
  hashtag:foobar
  account:3
  account:6
  account:1:media
  account:3:media
  account:4:media

Previously they all had different action creators and action signatures, e.g. FETCH_ACCOUNT_TIMELINE_SUCCESS specifically, etc. They also had special treatment in the reducer, with slightly different functions to handle each action. Also, there could be only one tag timeline, loading a new hashtag replaced the previous one.

Now everything is using the same action creators (with convenience curries) and the same action signatures e.g. TIMELINE_REFRESH_SUCCESS, and different hashtags can co-exist in the same world.

All types of timelines now have a flat structure and use the same reducer functions and actions

ykzts

The code looks good.

ykzts · 2017-06-10T15:35:39Z

@beatrix-bitrot I will ask you to check with the production:smile:

unarist · 2017-06-10T15:51:53Z

I found some warnings on Chrome 58 console:

./app/javascript/mastodon/features/report/index.js
88:24-46 "export 'refreshAccountTimeline' was not found in '../../actions/accounts'
./app/javascript/mastodon/features/report/index.js
93:26-48 "export 'refreshAccountTimeline' was not found in '../../actions/accounts'

Also this patch brings back account timeline issue #3447. Second fetching (with since_id) may not return next link, so it prevents load more if save that result to the state.

Gargron · 2017-06-10T15:57:19Z

@unarist expandTimeline uses max_id: ids.last(), ignoring the next path. This is because timelines can be cropped at around 40 items, and if you were using the next header, scrolling down again would bring you further and further into the past, skipping past the cropped items. Plus, as far as timelines are concerned max_id uses the primary ID

unarist · 2017-06-10T16:04:40Z

@Gargron next path isn't used actually, but existence of it is important, because account timeline's handleScrollToBottom() checks it as hasMore.

  handleScrollToBottom = () => {
    if (!this.props.isLoading && this.props.hasMore) {
      this.props.dispatch(expandAccountTimeline(Number(this.props.params.accountId)));
    }
  }

Did you checked the issue I mentioned is not reproduced with this patch?

Gargron · 2017-06-10T16:42:18Z

@unarist You are right. Perhaps hasMore should always be true. That's how it used to be at the very start...

clworld · 2017-06-10T17:18:28Z

@unarist @Gargron
I deployed this on my production. (my staging environment does not have enough number of toots…)
This PR reproduces issue #3447.

clworld · 2017-06-10T17:49:58Z

I got some errors in Console.
(Both firefox and chrome)
(In chrome only)
I'm trying git fetch/assets:precompile and restart puma, re-check this error is hard (Japan is 3:30 AM now.. timeline stops..)
After re-fetch/assets:precompile and restart puma I got r.include error. I can't determine when this error happen.(It seems timeline related..)
I got e.fastSeek only one times and can not re-produce e.fastSeek….(except when I add breakpoint for r.include error and stop js execution.) It seems unrelated to this PR and ignorable.

unarist · 2017-06-10T19:11:49Z

actions/timelines.js:

export function updateTimeline(timeline, status) {
  return (dispatch, getState) => {
    const references = status.reblog ? getState().get('statuses').filter((item, itemId) => (itemId === status.reblog.id || item.get('reblog') === status.reblog.id)).map((_, itemId) => itemId) : [];

~~getState().get('statuses')... and [] have includes() method, but [] has include() method...slightly different name.~~

And in updateTimeline() in reducers/timelines.js...

if (status.getIn(['reblog', 'id'], null) !== null) newIds = newIds.filterNot(item => references.include(item));

It should be includes() not include() . I think this is one of the issue @clworld reported..

Gargron · 2017-06-10T20:29:25Z

@unarist @clworld I have fixed the includes typo.

I am surprised if #3447 re-appears again, because expandTimeline does not use next? Let me double check

Gargron · 2017-06-10T20:30:47Z

Oh wait! The problem is only because hasMore blocks the loading of more stuff, right?

clworld · 2017-06-10T21:01:22Z

@Gargron
I deployed fix and I checked r.include error does not happen.
unarist seems already go to sleep 1.5 hour ago.. (5:55 a.m. now in Japan)

unarist · 2017-06-11T07:16:27Z

@Gargron

hasMore blocks the loading of more stuff,

Right. account/:id/statuses with since_id may returns response without next link header, then it overwrites timelines.['accounts:xxx'].next with null. This means making hasMore to be false, and blocks the loading. We shouldn't overwrite that property on the loading newer statuses.

Well, maybe account/:id/statuses with since_id option should returns next link header, since we don't know existence of older status than since_id value.

Gargron · 2017-06-11T10:10:48Z

@unarist Ah, I was thinking of a slightly different scenario. Same bug currently occurs in notification column too:

Scroll down to load more posts, or wait until >40 posts are loaded
If you are scrolled to top and a new post arrives, all posts after the 20th will be cut off
Scroll down again to the bottom to load more posts.

Expected: It will load the posts after the last visible one, without gaps
Reality: It will load posts after the last invisible one, because that's where the "next" pointed already

So I think that the usage of "next" in the timelines specifically should be removed, and hasMore should always be true.

unarist · 2017-06-11T12:02:42Z

hasMore should always be true.

We shouldn't make hasMore always true, because it allows the user loading more statuses infinitely (#2066).

Also timelines may have "end" especially on remote account timeline. So I've changed accounts/:id:/statuses API to return next link only if needed (#3311), to reduce redundant requests. but If you think handling this is too complicated, how about to set next to true on TIMELINE_REFRESH and keep current behavior on TIMELINE_EXPAND? This allows loading more at least once even on too short timeline, but it's not a huge problem.

Well, maybe we should store boolean value for next state instead of link header value itself to prevent misuse...

Gargron · 2017-06-11T12:33:33Z

@unarist Okay, I think my last commit should take care of the issue. I think I had just accidentally removed the fix that was already there, i.e. only set "next" if previous "next" wasn't set in refreshTimeline (that's the since_id case)

I also found that there was a typo in expandNotifications, which had been there for a long time, and fixed that too.

unarist · 2017-06-11T12:46:17Z

app/javascript/mastodon/actions/notifications.js

@@ -124,10 +124,9 @@ export function refreshNotificationsFail(error, skipLoading) {

 export function expandNotifications() {
  return (dispatch, getState) => {
-    const url    = getState().getIn(['notifications', 'next'], null);
    const lastId = getState().getIn(['notifications', 'items']).last();


Oh, lastId is whole entity, not id...

clworld · 2017-06-11T12:54:36Z

@unarist Yes. notifications stop doing load more...

clworld · 2017-06-11T13:01:27Z

I found glitch reported by beatrix on Discord exists.
It happens when:

open Mastodon with home timeline (scroll position is top of timeline)
new toot arrived.
Title indicates there is unread toots but scroll position is top of timeline already.
After scrolling (scroll down and move to top), unread indicator works good.

Gargron · 2017-06-11T14:57:23Z

@clworld @beatrix-bitrot @unarist Okay, fixed!

* Move ancestors/descendants out of timelines reducer * Refactor timelines reducer All types of timelines now have a flat structure and use the same reducer functions and actions * Reintroduce some missing behaviours * Fix wrong import in reports * Fix includes typo * Fix issue related to "next" pagination in timelines and notifications * Fix bug with timeline's initial state, expandNotifications

Gargron added 2 commits June 10, 2017 17:02

Move ancestors/descendants out of timelines reducer

715e181

Refactor timelines reducer

ba7c0a4

All types of timelines now have a flat structure and use the same reducer functions and actions

Gargron changed the title ~~Feature refactor timelines reducer~~ Refactor timelines reducer Jun 10, 2017

Gargron changed the title ~~Refactor timelines reducer~~ Fix #3665 - Refactor timelines reducer Jun 10, 2017

ykzts approved these changes Jun 10, 2017

View reviewed changes

Reintroduce some missing behaviours

173670c

Fix wrong import in reports

4c7c5fc

Fix includes typo

d4dc3ea

Fix issue related to "next" pagination in timelines and notifications

cb0bc71

unarist suggested changes Jun 11, 2017

View reviewed changes

Fix bug with timeline's initial state, expandNotifications

812a4ec

unarist approved these changes Jun 11, 2017

View reviewed changes

Gargron merged commit 47bf7a8 into master Jun 11, 2017

Gargron deleted the feature-refactor-timelines-reducer branch June 11, 2017 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3665 - Refactor timelines reducer #3686

Fix #3665 - Refactor timelines reducer #3686

Gargron commented Jun 10, 2017 •

edited

ykzts left a comment

ykzts commented Jun 10, 2017

unarist commented Jun 10, 2017 •

edited

Gargron commented Jun 10, 2017

unarist commented Jun 10, 2017 •

edited

Gargron commented Jun 10, 2017

clworld commented Jun 10, 2017

clworld commented Jun 10, 2017 •

edited

unarist commented Jun 10, 2017 •

edited

Gargron commented Jun 10, 2017

Gargron commented Jun 10, 2017

clworld commented Jun 10, 2017

unarist commented Jun 11, 2017 •

edited

Gargron commented Jun 11, 2017 •

edited

unarist commented Jun 11, 2017

Gargron commented Jun 11, 2017

unarist Jun 11, 2017 •

edited

clworld commented Jun 11, 2017

clworld commented Jun 11, 2017

Gargron commented Jun 11, 2017

Fix #3665 - Refactor timelines reducer #3686

Fix #3665 - Refactor timelines reducer #3686

Conversation

Gargron commented Jun 10, 2017 • edited

ykzts left a comment

Choose a reason for hiding this comment

ykzts commented Jun 10, 2017

unarist commented Jun 10, 2017 • edited

Gargron commented Jun 10, 2017

unarist commented Jun 10, 2017 • edited

Gargron commented Jun 10, 2017

clworld commented Jun 10, 2017

clworld commented Jun 10, 2017 • edited

unarist commented Jun 10, 2017 • edited

Gargron commented Jun 10, 2017

Gargron commented Jun 10, 2017

clworld commented Jun 10, 2017

unarist commented Jun 11, 2017 • edited

Gargron commented Jun 11, 2017 • edited

unarist commented Jun 11, 2017

Gargron commented Jun 11, 2017

unarist Jun 11, 2017 • edited

Choose a reason for hiding this comment

clworld commented Jun 11, 2017

clworld commented Jun 11, 2017

Gargron commented Jun 11, 2017

Gargron commented Jun 10, 2017 •

edited

unarist commented Jun 10, 2017 •

edited

unarist commented Jun 10, 2017 •

edited

clworld commented Jun 10, 2017 •

edited

unarist commented Jun 10, 2017 •

edited

unarist commented Jun 11, 2017 •

edited

Gargron commented Jun 11, 2017 •

edited

unarist Jun 11, 2017 •

edited