You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Rework chat scrolling and automatic history loading - #323
Fixes#285. Reworks channel scrolling and automatic history loading, replacing the previous insertion-triggered logic. A pure scroll policy now drives a DOM adapter that keeps the list pinned to the bottom, preserves the reading position while older messages load, and fetches older history automatically.
What changed
Pure scroll policy — app/utils/chat-scroll-state.js decides stick/detach/anchor/auto-load as a testable reducer; app/components/chat-scroller.js is the DOM adapter, wired up by app/modifiers/chat-scroll.js.
Automatic history loading — level-triggered and single-flight: it loads when near the top or when filling a short list, and keeps going after each page (including through sparse/empty pages) until the viewport is filled or history is exhausted. The manual "Load previous messages" control is gone, and "Beginning of history" only appears once history has actually been loaded.
Scroll position — preserved when older messages are prepended by re-anchoring the visible message in the same turn as the DOM update (no one-frame jump); missing/stale anchors no longer yank the view once the user is back at the bottom.
Rendering window — renders the latest messages first and reveals older ones in increments instead of rendering the whole history.
Message identity & dedupe — app/utils/message-key.js derives a stable, CSS-safe key (server stanza id, author-scoped transport id, or timestamp + content hash for id-less remoteStorage IRC logs); duplicates are dropped.
Scroll-to-bottom button — replaces the old jump control: bottom-right, grey when idle and blue with a new-message count, with a custom smooth scroll that honours prefers-reduced-motion and keeps keyboard focus in the list.
Supporting changes — coms.loadOlderMessages advances a cursor and returns { added, hasMore }; a new untrack-wrapped on-change modifier replaces the removed on-channel-change / on-users-change modifiers.
Testing
pnpm lint is clean.
pnpm test:ember: 211 pass, 3 skip, 0 fail.
New unit tests for the scroll reducer, message keys, and pagination, plus integration tests for the scroller (initial scroll, sticky bottom, auto-load and viewport fill, in-flight loads, anchor preservation, smooth scrolling, repeated activation) and the on-change modifier.
Notes for reviewers
chat-scroller.js is the trickiest part — the maintenance lock, single-flight requests, and synchronous anchor restore are the key safeguards against the historical scroll feedback loops, so it's worth a close look.
Give messages a stable `key` for list rendering and scroll anchoring: the
transport id when present, otherwise a composite of the timestamp and a hash of
the author and content (remoteStorage IRC logs have no message ids).
`BaseChannel.addMessage` assigns the key and ignores duplicates.
Add `coms.loadOlderMessages`, which fetches the next archive page and reports
`{ added, hasMore }`. `loadArchiveMessages` now returns that summary and sets
`channel.hasOlderMessages` when history is exhausted. The channel tracks
`hasOlderMessages` / `loadingOlderMessages` for the UI.
Fixes#285.
Replace the ad-hoc, insertion-triggered scroll handling in the channel
container with a three-layer design:
- a pure scroll policy (app/utils/chat-scroll-state.js) with unit tests;
- a ChatScroller component that adapts it to the DOM (stick to bottom,
detach on scroll, anchor-preserving history loads, auto-loading);
- a chat-scroll modifier that wires up the ResizeObserver and listeners.
Auto-loading is level-triggered and single-flight (near the top, or filling a
short list) with a no-progress guard, and is re-checked after each page, so
reaching the top while a page is still loading loads the next one without
scrolling down and back up.
Scroll position is preserved when older messages are prepended by restoring the
anchored message in the same turn as the DOM update (no one-frame jump), and
prepends/appends that happen together are handled independently.
Also:
- add a bottom-right "scroll to bottom" button (grey chevron when idle, blue
with a new-message count when there are new messages) with a smooth scroll
that honors prefers-reduced-motion;
- fix on-change to depend only on its value (run the callback in untrack) so
callbacks that read tracked state don't retrigger it;
- remove the now-unused on-channel-change/on-users-change modifiers;
- show "Beginning of history" only once history has actually been loaded.
Covered by new unit and integration tests.
For instant programmatic scrolls (scroll to bottom, anchor restore), record the
maintenance lock's observed position as the post-scroll target, so any deviating
scroll event is treated as the user interrupting and releases the lock
immediately. Previously such an event could be misread as our own scroll in
progress (moving toward the target), leaving stickToBottom stuck true and
preventing auto-loading after the user reached the top.
Also make the in-flight auto-load test wait for the initial programmatic scroll
to settle before scrolling, removing a timing dependence that surfaced on
Chrome 154.
A rejected archive fetch reset `loadingOlderMessages` while leaving
`hasOlderMessages` true, so the level-triggered auto-load immediately retried the
same failing page: repeated requests for the same page plus an unhandled promise
rejection while the viewport stayed near the top.
Catch the failure in `loadOlderMessages`, set a `historyLoadFailed` flag that
disables auto-loading (via `canLoadOlderMessages`), and surface an inline
"Retry" affordance. This stops the loop and makes the failure visible and
recoverable. The retry resets the flag and attempts the page again.
Observe channel changes instead of user list recomputations
app/components/user-list.hbs:1
@users changes not only when switching channels, but whenever sortedUserList recomputes after a user joins or leaves (app/models/base_channel.js:112-117,198-207). This now resets the rendered window to 50 and scrolls to the top on every presence update. Observe @channel instead, which matches the stated channel-switch behavior.
Delay atBottom state until smooth scrolling completes
app/utils/chat-scroll-state.js:82
Marking atBottom true before a smooth scroll reaches its target removes the jump button immediately. Because that sticky button is in the observed content wrapper’s layout, its removal triggers handleContentResize, whose sticky-state correction calls applyScrollToBottom() without the smooth flag and turns the requested animation into an instant jump. Keep the detached state until the target is reached, then transition to atBottom from the completion path.
- Guard the history-load failure flag against channel changes/destruction, so a
late rejection from a previous channel doesn't show the retry error on the
newly active channel or hide its history loading.
- Announce the history-load error with role="alert".
- Reset the user list on channel changes, not on every user-list recomputation,
so presence updates no longer reset the render window and scroll to the top.
- Mark the list at bottom only once a programmatic scroll settles (new
`scroll-settled` event) instead of immediately in `scroll-to-bottom`. This
keeps the jump button rendered during a smooth scroll; removing it mid-flight
resized the observed content wrapper and turned the animation into an instant
jump.
Render window remains zero after asynchronous archive load
app/components/channel-container.js:57
renderedStartIndex is initialized only from the messages present at the channel change, but the initial IRC archive load is started without awaiting it (app/services/coms.js:356-358). For a newly joined channel this is commonly zero; if the fetched day later contains hundreds of messages, the index remains zero and the component renders the entire archive instead of the intended latest 50. Initialize/adjust the render window when that initial asynchronous batch arrives, or await the initial load before resetting the window.
Default chevron and text color fails WCAG contrast requirements
The default white chevron/text on #9ca3af has only a 2.54:1 contrast ratio, below the WCAG 3:1 minimum for essential graphical controls (and 4.5:1 for normal text). Use a darker default such as #6b7280, which provides 4.83:1 against white.
The tests rely on fixed wall-clock sleeps to outwait the component’s 150 ms maintenance timer and native scroll events. Under a busy CI browser those callbacks can run later than 200 ms, making the subsequent synthetic scroll execute while maintenance is still locked and causing intermittent failures. Instrument the component’s async maintenance with an Ember test waiter (or wait on an observable condition) so settled() can synchronize deterministically.
- Scope transport ids by author and prefer server stanza ids in `messageKey`, so
two senders using the same (sender-scoped) id no longer collapse into one
message when `BaseChannel.addMessage` dedupes channel-wide.
- Initialize the render window as an "auto" window (latest N) derived from the
live message list, so the asynchronously loaded IRC archive is windowed
correctly instead of rendering the entire batch. Revealing older history
materializes a fixed start index.
- Guard and cancel the scroll maintenance timer on destruction, and instrument
the scroller's deferred async work (maintenance timer and frame callbacks)
with an Ember test waiter so `settled()` synchronizes deterministically.
- Replace fixed sleeps in the scroller tests with `settled()` / observable
waits, and add regression tests for the message-key scoping and initial
render window.
Reset scrolling on channel object changes, not just IDs
app/components/channel-container.hbs:34
Resetting by channel.id misses channel switches when two channel instances share an ID (for example, the same IRC/XMPP room opened through different accounts). In that case ChatScroller retains the prior channel’s detached/sticky state, item keys, and anchor. Use the channel object as the reset key so every actual channel switch resets scrolling.
Preserve focus when hiding the scroll-to-bottom button
app/components/channel-container.hbs:84
Activating this button eventually sets scroll.isAtBottom and removes the focused button from the DOM, leaving keyboard and screen-reader focus on the document body. Move focus to the scroll region or newest message before hiding the button (and make that target programmatically focusable) so keyboard users retain a predictable position.
Add pagination service tests for cursors and exhaustion
app/services/coms.js:431
The new pagination contract is not covered by the existing tests/unit/services/coms-test.js: the integration tests replace this service with stubs, so they do not verify cursor advancement, the { added, hasMore } result, or exhaustion behavior. Add service tests for a successful page (including duplicate filtering) and an empty cursor to protect the state that drives auto-loading.
The native `scrollTo({ behavior: 'smooth' })` engaged the browser's
overscroll/chaining animation when it reached the container edge, pulling the
content up and settling it again (janky), because the chat scroller sits inside
other `overflow: auto` ancestors.
Drive the user-initiated scroll-to-bottom with a short rAF animation that sets
`scrollTop` directly: monotonic, ease-out, never overshoots. Native overscroll
is left untouched for user scrolls. The animation stays under the maintenance
lock and test waiter, and is cancelled on destruction or user interruption.
The user-initiated scroll-to-bottom had three problems:
- A scroll event queued from a previous programmatic scroll could arrive
before the first animation frame. maintenanceTarget was still null at that
point, so handleScroll ended maintenance immediately and the animation was
cancelled: the button often did not scroll at all after loading history.
Seed the target synchronously and ignore the scrollend events our own
frames produce.
- The animation moved a fixed fraction of the remaining distance in the
first frame, which read as a jump when starting from far up. Replace it
with a capped, time-normalized exponential decay: a gentle start followed
by a long, monotonic ease-out.
- The jump-to-latest button was an in-flow sticky child of the scroll
content, so adding/removing it changed scrollHeight and shifted the view
by the button's height. Move it into a zero-height sticky zone so toggling
it never affects the scrollable height.
Also clamp every programmatic scroll to the real maximum (scrollHeight -
clientHeight) instead of assigning scrollHeight, which overshoots.
Transport ids were scoped to their author with a NUL separator, but message
keys are written to data-message-key attributes and looked up with a selector
built from CSS.escape, which rewrites U+0000 to U+FFFD. The anchor lookup
therefore never matched a live (id-carrying) message, so prepending older
history silently failed to restore the reading position.
Encode the author/id tuple as JSON instead: it is injective for string tuples
and CSS-safe. Update the key expectations and add a regression test asserting
keys contain no NUL.
The scroller reset key was channel.id, but two channel instances can share an
id (e.g. the same IRC room opened through different accounts), so switching
between them kept the previous channel's sticky/anchor/item state. Key the
reset on the channel object, matching the existing on-change handler.
Activating the scroll-to-bottom button removes it from the DOM once we settle at
the bottom, which dropped keyboard/screen-reader focus to the document body.
Make the scroll region programmatically focusable and move focus there
(preventScroll, so it doesn't fight the animation) before scrolling; suppress
the outline since it is not a tab stop.
Cover the loadOlderMessages/loadArchiveMessages contract directly (integration
tests stub the service): cursor advancement, duplicate filtering, the
{ added, hasMore } result, and exhaustion when there is no previous cursor.
Skip archive fetch when initial in-memory batch is 50 or fewer
app/components/channel-container.js:109
When the initial archive batch has 50 or fewer records, this sets the effective window start from 0 to 0 and returns without fetching despite channel.hasOlderMessages being true. Because the rendered DOM does not change, the ResizeObserver need not fire again, so a short channel can remain unfilled and older history is never requested. Only return when this step actually revealed hidden in-memory records; otherwise fall through to the archive fetch.
Load guard permanently blocks history after unchanged scroll height
app/components/chat-scroller.js:440
Once repeated loads leave scrollHeight unchanged, this guard permanently suppresses all later requests until the component resets or content grows. Sparse IRC archives can legitimately span many empty/duplicate pages while their cursor still advances; after the guard trips, the removed manual “Load previous messages” control leaves older history unreachable. Bound only the automatic burst and provide a user-triggered resume/reset, or incorporate cursor progress into the guard.
Idle button background fails required contrast ratio
The idle button’s #9ca3af background has only a 2.54:1 contrast ratio against both the white chevron and the white page, below the 3:1 requirement for essential graphical objects and UI component boundaries. Use a darker grey while retaining a distinguishable hover state.
The first loadOlderMessages call materialized the auto-window and returned
unconditionally. When the in-memory batch was within the render window
(<= INITIAL_RENDERED_MESSAGES) the window start stayed at 0, so nothing was
revealed, the DOM did not resize, and auto-loading was never re-triggered:
older history silently became unreachable. Only return when the window was
actually hiding older in-memory messages; otherwise fall through to the
archive fetch.
The automatic viewport-fill burst stopped after a few attempts whose
scrollHeight did not change, but sparse archives legitimately return
empty/duplicate pages that advance the archive cursor without changing the
height. Once the guard tripped, older history was unreachable (the manual
'Load previous messages' control was removed). Count progress as either the
content growing or the archive cursor advancing, and only stop after several
consecutive attempts with neither. Make Channel.searchedPreviousLogsUntilDate
tracked and pass it to the scroller so cursor-only progress is observable.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Short overflowing histories can become inaccessible, interrupted smooth scrolling can report a false bottom state, and new UI elements have insufficient contrast.
Error text and retry control lack sufficient contrast
app/components/channel-container.hbs:42
text-neutral-400 is approximately 2.49:1 against this white background, below the 4.5:1 minimum for small text; the error and its retry control may be unreadable for low-vision users. Use at least text-neutral-600.
This issue also appears on line 47 of the same file.
Timeout falsely marks viewport as settled at bottom
app/components/chat-scroller.js:232
If the smooth-scroll safety timeout fires before the target is reached (the background-tab case described above), endMaintenance() still dispatches scroll-settled solely because stickToBottom is true. This marks atBottom true and removes the jump button while the viewport remains mid-list. Only settle when the current position actually reached maintenanceTarget; otherwise preserve/recompute the detached state so the user can retry.
The idle button’s #9ca3af background has only about 2.54:1 contrast with both its white chevron and the surrounding white content, below the 3:1 requirement for graphical controls. Use a darker neutral so the control and icon remain perceivable.
requestOlderIfNeeded refused to load while stickToBottom was true. With a
small overflow (<= AT_BOTTOM_THRESHOLD) every scroll position reads as 'at
bottom', so stickToBottom never clears and the guard made older history
unreachable — especially now that the manual loader is gone. The earlier
'!nearTop && !fits' check already excludes the bottom of a genuinely long
list, so the extra guard is unnecessary. Update the fill test to the new stop
condition (bottom no longer within the top zone).
endMaintenance dispatched scroll-settled whenever stickToBottom was set,
without checking whether the target was actually reached. If the safety
timeout cut a smooth scroll short (a backgrounded tab, or a very long
distance), the viewport was marked atBottom and the jump button removed while
the view was still mid-list. Pass whether the target was reached to the
reducer and only settle when it was; otherwise stay detached so the user can
retry.
text-neutral-400 is about 2.5:1 on the white message background, below the
4.5:1 minimum for small text. Use text-neutral-600 for the history-load error
(and its retry control) and the 'Beginning of history' marker.
Prevent duplicate scroll animations from creating uncancellable loops
app/components/chat-scroller.js:286
The jump button remains rendered until scrolling settles, so a rapid second activation can enter this method while the first animation is active. That starts a second independent requestAnimationFrame loop and overwrites the tracked frame ID, causing doubled movement and leaving one loop uncancellable. Make animation startup idempotent.
Prevent stale history anchors from overriding restored bottom position
app/utils/chat-scroll-state.js:145
older-prepended restores any cached anchor even after the user has returned to the bottom. If a history request starts while detached but finishes after the user clicks “Scroll to latest,” stickToBottom is true while the old anchor remains, so the completed prepend can yank the viewport back into history. Preserve the anchor only while detached; sticky state should remain at the bottom.
The jump button stays rendered until the scroll settles, so activating it again
before then started a second, parallel requestAnimationFrame loop. That doubled
the movement and overwrote the tracked frame id, leaving the first loop
uncancellable. Make the animation startup idempotent.
older-prepended always emitted RestoreAnchor, so a stale anchor captured while
reading history could yank the viewport back up if the load finished after the
user returned to the bottom (e.g. clicked 'scroll to latest'). Only restore the
anchor while detached; while sticking to the bottom, keep the view there.
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
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.
Summary
Fixes #285. Reworks channel scrolling and automatic history loading, replacing the previous insertion-triggered logic. A pure scroll policy now drives a DOM adapter that keeps the list pinned to the bottom, preserves the reading position while older messages load, and fetches older history automatically.
What changed
app/utils/chat-scroll-state.jsdecides stick/detach/anchor/auto-load as a testable reducer;app/components/chat-scroller.jsis the DOM adapter, wired up byapp/modifiers/chat-scroll.js.app/utils/message-key.jsderives a stable, CSS-safe key (server stanza id, author-scoped transport id, or timestamp + content hash for id-less remoteStorage IRC logs); duplicates are dropped.prefers-reduced-motionand keeps keyboard focus in the list.coms.loadOlderMessagesadvances a cursor and returns{ added, hasMore }; a newuntrack-wrappedon-changemodifier replaces the removedon-channel-change/on-users-changemodifiers.Testing
pnpm lintis clean.pnpm test:ember: 211 pass, 3 skip, 0 fail.on-changemodifier.Notes for reviewers
chat-scroller.jsis the trickiest part — the maintenance lock, single-flight requests, and synchronous anchor restore are the key safeguards against the historical scroll feedback loops, so it's worth a close look.