fix: message list loads the entire room history while hidden - #41787
fix: message list loads the entire room history while hidden#41787ndo84bw wants to merge 2 commits into
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: fba9afa The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details🔇 Additional comments (1)
WalkthroughThe message history loader now ignores zero-height message containers. A regression test verifies that hidden containers do not request older messages. A patch changeset documents the scroll-position fix. ChangesHidden message pagination
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change prevents hidden message lists from automatically loading room history and includes a focused unit test; no actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
`RoomLayout` hides the whole message body with `display: none` once a contextual bar takes the full room width, which happens below 600px of room width. A hidden element reports `clientHeight` and `scrollTop` as 0, so the position check in `useGetMore`, `scrollTop <= clientHeight / 3`, is permanently true. The MutationObserver and ResizeObserver on that same element then drive the loading themselves: each loaded page mutates the DOM, the observer fires, the next page loads. Nothing stops it until the room has no history left. On a room with a few thousand messages the client pulls all of them, 50 at a time, and runs into the DDP rate limiter. `RoomHistoryManager.restoreScroll` cannot compensate either, because it measures `scrollHeight` on the same hidden element and gets `0 - 0` as the height difference. So when the panel closes, the scroll offset is unchanged while the list is several times taller, and the user is left far in the past. Bail out when the list is not rendered. The guard reads the `clientHeight` that `getBoundingClientRect` already returned rather than the element again, so it is the exact value the check uses, and it stays testable: jsdom performs no layout and reports 0 for every element. Measured on a room with 5162 messages at 580px room width: the list grew from 2738px to 29018px within 30 seconds of having a thread open; with this guard it stays at 2738px and no `loadHistory` call is made at all. Assisted-by: claude-code:claude-opus-5
3f92dfb to
5f74bb0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hey @ndo84bw, thanks for the contribution! The fix is valid. I also added a few minor adjustments and just asked the team to review. For the internal controls, the ticket is SUP-1104. RC team: Just a reminder that we need to include this in 8.8 and then backport it to the previous affected versions, since this issue has been happening since 8.5, IIRC. |
5f74bb0 to
fba9afa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.8.0 #41787 +/- ##
================================================
Coverage ? 69.34%
================================================
Files ? 4255
Lines ? 168665
Branches ? 30073
================================================
Hits ? 116953
Misses ? 46535
Partials ? 5177
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jessicaschelly
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
|
We’ll move forward with #41454, which was opened earlier. Anyway, thanks for the contribution! |
4 participants
Proposed changes (including videos or screenshots)
RoomLayouthides the entire message body withdisplay: noneonce a contextual bar takes the full room width, which happens below 600px of room width - not viewport width, since the breakpoints come from aResizeObserveron the room element:A hidden element reports
clientHeightandscrollTopas0, so the load-older-messages check inuseGetMoreis permanently true:The
MutationObserverandResizeObserverattached to that same element then drive the loading themselves: every page that arrives mutates the DOM, the observer fires, the next page loads.Nothing stops it until the room has no history left. The client pulls the whole room 50 messages at a time and runs into the DDP rate limiter - the tail of the request list is
loadHistoryreturning HTTP 400too-many-requests.RoomHistoryManager.restoreScrollcannot compensate, because it measuresscrollHeighton the same hidden element and gets0 - 0as the height difference. So when the panel closes, the scroll offset is unchanged while the list is several times taller, and the user is left far in the past.This PR bails out when the list is not rendered, and adds a unit test for it.
Before - one pixel narrower, thread opened, nobody scrolling: a stream of
loadHistorycallsending in HTTP 400, and the list is far up when the thread closes.
2026-08-14-nachladeschleife-880-vs-879-develop.mp4
After - same steps: 5 requests for the whole open-wait-close cycle (because scroll in thread), none of them
loadHistory, and the list stays where it was.2026-08-14-nach-fix-879-develop.mp4
Control at 1220px room width: list height unchanged in both cases, no jump - so the trigger is the
hidden body, not opening a panel as such.
Issue(s)
Fixes #41132
Related to #41159, which addresses a different threshold - see Further comments.
Steps to test or reproduce
Four conditions have to hold at once, and two of them are easy to destroy by accident, which is
probably why this has looked flaky:
hasMoremust still be true - reload the page first. Once a room's history has been fullypulled in a session, that room is immune for the rest of the session.
userInteractedmust be true - scroll the main list once with the wheel, or click in it. Theflag is a per-mount closure variable and resets on every room switch.
virtualizer keeps the list anchored there and the growth stays invisible.
Then:
threshold sits exactly between 880px and 879px of window width:
Sanity checks that should not trigger it: a wide window, closing the thread again within a
second, a second attempt without reloading, or opening the thread from the Threads contextual bar
without touching the main list first.
Tested on
8.8.0-developat02e633cf27and on8.6.1atbfd782302d, in a room with 5162messages spanning five years and a thread with 60 replies, at 580px room width.
Further comments
The guard reads the
clientHeightthatgetBoundingClientRectalready returned rather than askingthe element again. That is the exact value the position check uses, and it keeps the hook testable:
jsdom performs no layout and reports
0for every element, so a guard readingelement.clientHeightdirectly would make the two existing tests in
useGetMore.spec.tsxfail.Alternatives considered:
gatedCheckso only observer-driven calls are skipped. That leaves the samealways-true condition in place for any other caller, and a hidden element cannot emit a scroll
event anyway, so the guard belongs where the value is read.
would need the layout state threaded into this hook.
hideBodyso the situation cannot arise. That is what fix: prevent scroll jump when closing thread in narrow layout #41159 approaches from the layoutside, but the
0 <= 0fragility would remain for any other case that hides the list.On #41159: it takes the aside out of the flex flow, which addresses a different threshold. Between
600px and 768px of room width the body is not hidden but squeezed - at 600px room width the message
list drops to 220px, text rewraps, cached row heights go stale, and the scroll position is off by a
noticeable amount when the panel closes. That is a separate defect with its own trigger; this change
does not touch that path and does not conflict with it, and the
hideBodybranch below 600px staysas it is.
Summary by CodeRabbit