Closed Bug 1528059 Opened 2 years ago Closed 2 years ago

Dismissed Pocket stories come back until stories update

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 67
Iteration:
67.2 - Feb 11 - 24
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: Mardak, Assigned: thecount)

References

Details

(Keywords: github-merged, regression)

Attachments

(1 file)

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.

Iteration: --- → 67.2 - Feb 11 - 24
Priority: -- → P1

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

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)
Blocks: 1479458
Flags: needinfo?(sdowne)

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.

Flags: needinfo?(sdowne)
Flags: needinfo?(edilee)
Assignee: nobody → sdowne

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

Flags: needinfo?(edilee)
Blocks: 1532321
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: github-merged
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.