Skip to content

[2.x] fix: avoid re-fetching the page of posts embedded in the initial page load#4703

Merged
imorland merged 2 commits into
flarum:2.xfrom
ekumanov:fix/discussionpage-seed-preloaded-posts
Jun 11, 2026
Merged

[2.x] fix: avoid re-fetching the page of posts embedded in the initial page load#4703
imorland merged 2 commits into
flarum:2.xfrom
ekumanov:fix/discussionpage-seed-preloaded-posts

Conversation

@ekumanov

Copy link
Copy Markdown
Contributor

Fixes #4702

Changes proposed in this pull request:

Since #4156, DiscussionPage.show() constructs PostStreamState without seeding it, so on every cold discussion-page load the stream re-fetches GET /api/posts?filter[discussion]=N&page[near]=M — data that Content\Discussion already embedded in the preloaded document (it needs it for the noscript content anyway) and that preloadedApiDocument() 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() passes this.preloadedNearPage(preloadedDiscussion) to show() 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 navigation preloadedNearPage() returns [] and the stream fetches exactly as before.
  • preloadedNearPage() returns the longest run of store-loaded posts contiguous in postIds() order. Stray posts pulled in by other relationships (e.g. extension includes — real production payloads do contain them) therefore can't corrupt visibleStart/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.
  • When no near param 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:

Measured impact (production rc.3 forum, 41 extensions; details in #4702):

  • Docker mirror of the production site (same machine/DB, only the bundle changed): the posts request is eliminated on cold loads; no functional change.
  • Production, same host, same day: Lighthouse mobile (simulated Slow 4G) LCP on a discussion page 7.7 s → 6.2 s (median of 3); one fewer API request per cold discussion view.

QA performed (production mirror with real data):

  • Cold landing on a 4-post and a 749-post discussion: no posts request, correct window, scrubber total correct.
  • /d/{id}/{slug}/400 deep link: window around post 400 rendered with no posts request.
  • Non-contiguous extension include in the payload: correctly excluded by the contiguity rule.
  • In-app navigation from the index: still fetches (discussion show + posts near) and renders, preserving [2.0.0-beta.1] Opening a discussion not always loads all posts #4137 behavior.
  • Event posts (e.g. discussion renamed) inside the seeded window render fine; no console errors anywhere.

Necessity

Confirmed

  • Frontend changes: tested on a local Flarum installation (and a production mirror, see QA above).
  • Backend changes: n/a — no backend changes.

ekumanov and others added 2 commits June 10, 2026 21:23
… 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>
@ekumanov ekumanov requested a review from a team as a code owner June 10, 2026 18:23
@imorland imorland changed the title fix: avoid re-fetching the page of posts embedded in the initial page load [2.x] fix: avoid re-fetching the page of posts embedded in the initial page load Jun 11, 2026

@imorland imorland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid to me, approving.

Couple of minor things;

  • Please don't commit the dist files in future, they can easily cause merge conflicts if the js changes on 2.x between 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 imorland added this to the 2.0.0-rc.4 milestone Jun 11, 2026
@imorland imorland merged commit 4984002 into flarum:2.x Jun 11, 2026
19 checks passed
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants