1528059 - Dismissed Pocket stories come back until stories update

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Ed Lee :Mardak]

Looks like TopStoriesFeed only persists removing blocked urls when it processes a new set of stories:

https://github.com/mozilla/activity-stream/blob/master/lib/TopStoriesFeed.jsm#L256

This means if a user dismisses a story, it'll disappear immediately thanks to Reducer but then I'm guessing on SYSTEM_TICK 5 minutes later, the story comes back because it's probably in TopStoriesFeed.stories.

Only after a story update (every 30 minutes) will it be filtered out for good.

Comment 1

[Ed Lee :Mardak]

I happened to have 64.0.2 release and a quick test shows the dismissed article coming back after waiting 5 minutes. So this isn't a recent regression

Comment 2

[Ed Lee :Mardak]

I believe this regressed with bug 1479458 since Firefox 63. It added a doContentUpdate() in SYSTEM_TICK:

https://github.com/mozilla/activity-stream/commit/be0ccddd3e6f7d7adffa55b0440f8a1104286ca5#diff-ab2f31cc847abe1fcf02369e79b4dd4dR494

And it reads the stories from this.stories:

https://github.com/mozilla/activity-stream/commit/be0ccddd3e6f7d7adffa55b0440f8a1104286ca5#diff-ab2f31cc847abe1fcf02369e79b4dd4dR105

… instead of accessing the redux state that has stories removed, e.g.,

this.store.getState().Sections.find(s => s.id === SECTION_ID)

Comment 3

[Scott [:thecount] Downe]

Good find. Yup I think you're right about this being a regression because of doContentUpdate.

I'm not sure if what's in state is usable as a replacement to this.stories. I think redux does modification on it (spocs and deduping). Either way, it makes me nervous that we might start seeing 2 of the same spocs side by side by doing this. We generally only use store in specific cases in this file because of how much redux can change stories, iirc, but I can confirm.

Another options is, doContentUpdate just didn't send stories unless fetchStories updated something, because this.stories gets properly transform'd inside of fetchStories, it should have the blocked list.

An easier but less optimal option is to always run this.stories through a .filter(s => !NewTabUtils.blockedLinks.isBlocked({"url": s.url})) before doContentUpdate in system tick.

Comment 4

[Ed Lee :Mardak]

Redux state doesn't update main for spoc stuff as TopStoriesFeed uses OnlyToOneContent:
https://github.com/mozilla/activity-stream/blob/48e00d4e4a29d8ddbc6f6889674c01c7e012ca86/lib/TopStoriesFeed.jsm#L446-L462

… where that action creator explicitly says to skipMain:
https://github.com/mozilla/activity-stream/blob/48e00d4e4a29d8ddbc6f6889674c01c7e012ca86/common/Actions.jsm#L240-L249

So the simplest fix should be just changing updateProps.rows = this.stories to use this.store.getState().Sections.find(s => s.id === SECTION_ID).rows

There's an existing handler for link blocks that filters out spocs, so that could be reused to filter out this.stories instead of iterating over all blocked pages (which aren't limited to just stories) on every update.
https://github.com/mozilla/activity-stream/blob/48e00d4e4a29d8ddbc6f6889674c01c7e012ca86/lib/TopStoriesFeed.jsm#L649-L651

Comment 5

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4808

Comment 6

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/3d9f5f3d386b