Repository navigation
Show the latest page when a refresh misses more than one - #369
namespaceMarcello wants to merge 2 commits into
Conversation
After the cable reconnects, or a page hidden for over a minute comes back, the room asks for what was posted since the newest message it has. The refresh answered with the first page of that, appended. With more than a page posted meanwhile, the rest never arrived: the page still counted itself up to date, so nothing asked for more, and the next live message landed right after that first page, with the gap above it unmarked until a reload. When more than a page was posted since, the refresh now sends the latest page in place of the messages shown, the way reloading the room does, and older messages load on scrolling up as they do after a reload. The page keeps what the latest page can't know about: a message that arrived live while the refresh was on its way, and our own messages still being sent or that failed to send, whose text is nowhere else. A reader of older messages, from a search result or scrolled up past what the page holds, keeps their place instead, and still gets the edits and boosts of the messages on screen. The latest page loads when they return to it, as it already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Replacement can discard queued messages and unsaved edits, and disrupt scrolled-up readers.
3 open issues
What changed in this PR
Fixes room catch-up after long disconnections by loading the latest message page instead of appending an incomplete backlog.
Changes:
- Selects latest-page replacement when missed messages exceed a page.
- Adds client-side preservation and older-history handling.
- Adds controller and reconnect regression tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/system/catching_up_test.rb | Tests reconnect and message preservation. |
| test/controllers/rooms/refreshes_controller_test.rb | Tests page thresholds and update streams. |
| app/views/rooms/refreshes/show.turbo_stream.erb | Renders latest-page replacement streams. |
| app/models/message/pagination.rb | Extracts the creation-time scope. |
| app/javascript/controllers/messages_controller.js | Handles replacement and retained messages. |
| app/controllers/rooms/refreshes_controller.rb | Selects incremental or latest-page refresh. |
Review effort: Balanced
| const kept = replacing ? this.#messagesUnknownTo(streamElement) : [] | ||
|
|
||
| const didScroll = await this.#scrollManager.autoscroll(false, async () => { |
There was a problem hiding this comment.
Right: the snapshot was taken before ScrollManager's queue ran, so an append queued earlier rendered first and was then removed by the update. Rather than move the snapshot, c7b4b4b stops replacing the list. The refresh appends the first page as before and marks the stream incomplete when more remain; the page then pages forward from its last message. Nothing is removed, so nothing needs to be kept aside.
Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
| if (element.dataset.pendingMessage) { | ||
| return !pageClientMessageIds.has(element.dataset.clientMessageId) | ||
| } else { | ||
| return element.dataset.messageId && !pageIds.has(element.id) && this.#sortValue(element) >= pageEnd |
There was a problem hiding this comment.
Right, and the same held for anything else living inside a message. Addressed in c7b4b4b by not replacing the list at all: an incomplete refresh appends, and the rest arrives by paging forward. A system test opens the editor on a message, misses 45 messages, reconnects, and checks that the editor is still open once "Missed 44" has loaded.
Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
| // Older messages are on screen, so the latest page would only take the reader's place. Returning to the | ||
| // latest loads it anyway. | ||
| if (replacing) event.detail.render = () => {} |
There was a problem hiding this comment.
Right: upToDate only turns false once the bottom is trimmed, so a reader a page up took the replacement path. In c7b4b4b nothing is replaced. An incomplete refresh appends below the reader like any refresh and marks the paginator behind, so "Jump to newest message" resets to the latest page instead of scrolling to the old tail. While the page is behind, live messages are held rather than appended after the gap, and added once it catches up.
Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.
Replacing the messages with the latest page dropped whatever the page held that the latest page didn't: a message being edited, one queued to render, the place of someone reading a page up. Each fix for one of those left another open. The list now only grows. The refresh appends the first page of what was missed, as it always has, and says when there's more. The page then counts itself behind and loads the rest in order as its end comes into view, the way it already loads newer pages after a search result. Until it has caught up, a live message is held instead of appended after a gap, and added once the list reaches its end. A message appended live just before a refresh, and so above the ones the refresh missed, moves into order when the page that has it arrives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
When more than 40 messages are posted in a room while someone's browser is disconnected (a laptop asleep, a phone with the PWA in a pocket, a dropped connection), only the first 40 of them show up when it reconnects. The rest are skipped with no sign of it: the room carries on from the 40th as if it were up to date, and new messages land right after it. Only reloading the room shows them.
With this PR nothing is skipped, and the list still only grows. The refresh appends the first page of what was missed, as it does today, and says when there's more. The page then counts itself behind, shows "Jump to newest message", and loads the rest in order as its end comes into view, with the paging it already uses after a search result. Until it has caught up, a live message waits instead of being appended after a gap. The refresh replaces nothing on screen, so a message being edited, a message still being sent and a reader's place stay as they are.
Impact of the bug:
Present since the first commit of this repository, so in every release up to v1.5.2.
Tested with two controller tests and three system tests that fail on
mainand pass here, plus the full unit suite, rubocop, herb, brakeman and the system tests, all locally. Risk: the room's message list after a disconnection, and live messages while the list isn't at its end. Reverting the two commits restores today's behavior.Why it happens
On reconnect, and when a page hidden for over a minute comes back,
refresh_room_controller.jsasksRooms::RefreshesController#showfor what was created since the newest message it has. The controller answers withpage_created_since, which is the first page of that, appended. The page staysupToDate, so nothing asks for the rest, and the next live message is appended after the 40th. When the page isn't up to date, a live message is appended at the bottom anyway, andMessagePaginatorthen pages forward from it.The change
Rooms::RefreshesController#showalso checks whether more than a page was created since (created_since(time).paged?, a scope next topage_created_since), and the view marks itsappendstreamincompletewhen it was. Otherwise the response is the same as today.messages_controller.js#beforeStreamRender: anincompleteappend, on a page that is up to date, renders as usual and marks the paginator behind, soMessagePaginatorloads the next pages (after:the last message) as the end comes into view, and "Jump to newest message" shows. On a page that isn't up to date, a stream to the messages is held instead of rendered. Held messages are added once the page catches up (paging reaches the end, "Jump to newest message", or sending a message), and only those past the end of the list: the others were fetched on the way, or are older than the latest page.MessagePaginator#addPageremoves a message it is about to insert if the list already has it. A message appended live just before anincompleterefresh sits above the messages that refresh missed; when paging brings it again, the copy in order is the one kept.The first commit took another approach, replacing the list with the latest page. The review below and an adversarial pass before it showed what that drops (a message being edited, an append queued to render, a reader's place), and the second commit takes this approach instead.
Checks
test/controllers/rooms/refreshes_controller_test.rb: exactly a page of new messages is appended with noincomplete; with 45, the first 40 are appended and the stream isincomplete. The second fails onmain.test/system/catching_up_test.rb, three tests in headless Chrome:reconnect: true. The page then shows "Missed 0" to "Missed 44" once each, in order. Onmainit shows 0 to 39.main, 40 to 44 never show.mainboth appear at the bottom as soon as they arrive.incompletefrom the server, not marking the paginator behind, not removing the duplicate, not holding, adding held messages without the end check. Leaving out the add right after "Jump to newest message" or sending still passes, because the paginator's end-of-list callback adds them first in these runs; it stays so the order doesn't depend on that timing.Limits
incompleteand behaves as today.sinceis the newestupdated_atthe page has seen, in milliseconds, so the newest message it has counts as created since: with exactly 40 new messages the stream isincompleteand the 40th loads by paging. No message is left out either way.app/views/rooms/refreshes/show.turbo_stream.erbandapp/models/message/pagination.rb, which Use native shared fragments and simplify the local hot-path cache #350 also changes. Whichever lands second takes the other's rendering of the appended messages.Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.