[2.x] fix: avoid re-fetching the page of posts embedded in the initial page load#4703
Merged
imorland merged 2 commits intoJun 11, 2026
Merged
Conversation
… load Since flarum#4156, DiscussionPage.show() constructs PostStreamState without seeding it, so every cold discussion-page load re-fetches /api/posts?filter[discussion]=N&page[near]=M — data the preloaded document already contains. The request is serialized between the JS boot and the first post paint, adding a full API round-trip to the LCP critical path of every cold discussion view. Restore the 1.x seeding, scoped so it cannot reintroduce flarum#4137: only the preloaded-document path seeds the stream, and only with the longest run of store-loaded posts contiguous in postIds() order, so stray posts included via other relationships fall through to the normal fetch. Fixes flarum#4702 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Includes transpiled JS/TS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
imorland
approved these changes
Jun 11, 2026
imorland
left a comment
Member
There was a problem hiding this comment.
Looks solid to me, approving.
Couple of minor things;
- Please don't commit the
distfiles in future, they can easily cause merge conflicts if the js changes on2.xbetween the time of the PR and merge. The Flarum Bot handles building at merge time. - I would have liked to have seen a regression test/guard here. I'll follow up immediately after the merge with one this time, just something to note for the future :)
imorland
added a commit
that referenced
this pull request
Jun 11, 2026
Locks in the behaviour of #4703 and guards against reintroducing #4137: - A contiguous preloaded seed produces a visible window covering exactly those posts, with no gaps (no perpetually-loading post). - An empty seed leaves the window at the baseline, so in-app navigation (where the API show response embeds no posts) is unchanged. - preloadedNearPage() returns only the longest contiguous run of store-loaded posts, dropping stray non-contiguous posts (e.g. pulled in by extension relationships) — the non-contiguity that caused #4137. Verified the contiguity assertions fail if the guard is removed.
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 #4702
Changes proposed in this pull request:
Since #4156,
DiscussionPage.show()constructsPostStreamStatewithout seeding it, so on every cold discussion-page load the stream re-fetchesGET /api/posts?filter[discussion]=N&page[near]=M— data thatContent\Discussionalready embedded in the preloaded document (it needs it for the noscript content anyway) and thatpreloadedApiDocument()has already pushed into the store. The request is serialized after the JS boot and before the first post can paint, so it adds a full API round-trip (typically 0.5–1.5 s for real visitors) to the LCP critical path of every cold discussion view. 1.x rendered straight from the payload.This PR restores the seeding, scoped so it cannot reintroduce #4137:
load()passesthis.preloadedNearPage(preloadedDiscussion)toshow()only on the preloaded-document path. API show responses (post-refactor: remove listing of posts in the show discussion endpoint #4067) embed no posts page, so for in-app navigationpreloadedNearPage()returns[]and the stream fetches exactly as before.preloadedNearPage()returns the longest run of store-loaded posts contiguous inpostIds()order. Stray posts pulled in by other relationships (e.g. extension includes — real production payloads do contain them) therefore can't corruptvisibleStart/visibleEnd, which was the [2.0.0-beta.1] Opening a discussion not always loads all posts #4137 failure mode that fix: discussion posts not always properly loaded #4156 fixed by removing the old extraction.show()accepts the seed as an optional second parameter (default[]), so existing callers and subclasses are unaffected.nearparam is present, the pop-in target falls back to the first preloaded post's number (matching 1.x), then to 1.Reviewers should focus on:
preloadedNearPage()as the guard against [2.0.0-beta.1] Opening a discussion not always loads all posts #4137.show()signature extension (optional param) for compatibility with extensions overridingshow().Measured impact (production rc.3 forum, 41 extensions; details in #4702):
QA performed (production mirror with real data):
/d/{id}/{slug}/400deep link: window around post 400 rendered with no posts request.Necessity
Confirmed