[2.x] fix: prevent discussion list cache wipe on back-nav to tag pages#4598
Merged
[2.x] fix: prevent discussion list cache wipe on back-nav to tag pages#4598
Conversation
DiscussionListState.requestParams returned `this.params.filter` by reference. Extenders (tags, subscriptions) then mutated that object to build the outgoing API filter, silently writing their additions back into the stored state. On the next mount, the freshly-supplied filter no longer equalled the mutated stored copy, so `paramsChanged()` tripped and the paginated cache was wiped — the visible "back button resets to page 1" symptom on tag pages (#4583). Clone `filter` before handing it out so extender mutation lands on the per-request object, not on `this.params`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4583.
What
DiscussionListState.requestParamsreturnedthis.params.filterby reference. Any extender that mutatedparams.filterinside arequestParamsextender callback (tags, subscriptions) silently wrote those mutations back into the stored state.On the next mount of
IndexPage,app.search.state.params()produced a freshfilter: {}(plus whatever tag slug etc.), andparamsChanged()compared that against the now-mutated stored filter viaJSON.stringify— they differed, sorefreshParamswiped the paginated cache and reloaded page 1. That's the regression reported in #4583: load-more a tag page, click a discussion, press back → you land on page 1./allwas unaffected because nothing mutatesparams.filterthere.How
Clone
this.params.filterinto a fresh object when handing it to callers, so extender mutation lands on the per-request copy rather than the stored state. One-line change; diff's in DiscussionListState.ts.Repro (before fix)
On a local 2.x instance, driven via Playwright against a tag with >40 discussions:
After the fix,
after back: 60 discussionson both/t/generaland/all.Root-cause trace (for reviewer context)
Inside
paramsChangedduring back-navigation, diagnostic logging showed:— the stored filter had been mutated to
{tag: "general"}by the tags extender'srequestParamsextender on the first load, but the freshparams()call during the second mount returned{}. Mutation source: the extender writesparams.filter.tag = this.params.tags, whereparamswas the same reference asthis.params.Test
Added
tests/unit/forum/states/DiscussionListState.test.tsthat constructs aDiscussionListState, callsrequestParams(), mutates the returnedfilter, and assertsthis.params.filteris untouched. Verified the test fails on the pre-fix code and passes after.Reviewers should focus on
{ ...this.params.filter }the right level of cloning (shallow,filteronly)? I considered deep-cloning the whole params but the only object-typed field in therequestParamsoutput that extenders are known to mutate isfilter.includeis already a freshly-built array. Wider cloning can come later if another footgun surfaces.PaginatedListState.requestParamsbase class (which also returnsthis.paramsby reference). I've left that alone for now to keep this RC-safe; happy to do it as a follow-up if preferred.Necessity
Confirmed
/t/<tag>and/all, both fixed; neither regressed).composer test). — N/A, frontend-only.Required changes