desktop: fix scrollback paging and visible history depth#1153
Merged
Conversation
Fix the scrollback regressions as one coherent pagination contract: - Keep backward-paged history in the cache instead of applying the newest-window cap to older roots, and gate the channel intro on the rendered timeline being caught up with the live cache before declaring the true channel start reached. - Fetch historical channel rows with content kinds only, then backfill reactions, edits, and deletions by `#e` reference so page limits buy visible history depth without rendering stale overlays. - Share one older-history pager between cold load and scroll-up. It targets 30 top-level rendered rows, uses 200-event pages, caps one pass at three pages, and starts cold history at 300 events. - Drop in-flight observer triggers instead of queueing retries, and require a parked top sentinel to leave/re-enter before another fetch once the timeline is scrollable. This removes the spinner-flash/flood pattern. - Keep the older-history spinner pinned in the timeline viewport while loading. Adds regression coverage for the timeline cap, aux backfill, content-kind parity, non-overlapping history fetches, viewport-visible loading state, and fixtures large enough to exercise pagination beyond the larger cold window. Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
The older-history spinner was bound to isFetchingOlder, which clears the moment the fetch resolves. Rows render off a deferred (concurrent) snapshot a frame or two later, so the spinner vanished before the new rows appeared. Hold it visible while the rendered timeline is still behind the history prepend. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
First channel load showed skeleton, then briefly the older-fetch spinner, then messages. The cold-load query seeds rows before its row-floor top-up finishes, so the loading latch settled (dataLength>0) while the fetch was still running, dropping the skeleton and exposing the spinner. Keep the timeline in its loading state while the initial fetch is in flight (before the channel has settled once), so the skeleton stays up until the first load is fully done. The per-channel settle latch still owns later refetch blips, so no skeleton bounce on revisit. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Add a scroll-history regression for sparse/reply-heavy cold loads: while the initial history query is still topping up to the visible-row floor, the timeline must keep the skeleton visible and must not expose the older-history spinner. Validation: - bin/pnpm --dir desktop exec playwright test tests/e2e/scroll-history.spec.ts --workers=1 - bin/pnpm --dir desktop exec biome check tests/e2e/scroll-history.spec.ts Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Collaborator
Author
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a1a39061
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| kinds: [...CHANNEL_AUX_EVENT_KINDS], | ||
| "#h": [channelId], | ||
| "#e": messageIds, |
There was a problem hiding this comment.
When a channel is reopened with a cached reaction that was removed while this client was unsubscribed, this aux backfill only queries deletion events whose #e points at loaded message ids. Reaction removals are kind:5 events whose #e points at the reaction event id (desktop/src-tauri/src/events.rs:440-442), so after the main history query became content-only, a stale cached reaction can survive the refetch unless the live-subscription overlap happens to include its deletion. The backfill needs a second pass for deletions targeting fetched/cached aux event ids, or stale aux events need to be dropped when they are no longer returned.
Useful? React with 👍 / 👎.
Fix the two PR #1153 smoke failures seen in CI: - Trim the scroll-history cap-crossing fixture from 2600 to 2100 older roots, still above the 2000-row cap but below the 30s CI timeout cliff. - Update the small-channel content test for the new correct behavior: a tiny fully-loaded channel is genuinely at its start, so the channel intro is visible immediately while welcome-only actions remain hidden. Validation: - CI=1 bin/pnpm --dir desktop exec playwright test tests/e2e/channels.spec.ts --project=smoke --workers=1 --retries=0 --reporter=line - CI=1 bin/pnpm --dir desktop exec playwright test tests/e2e/scroll-history.spec.ts --project=smoke --grep "channel intro stays hidden while paginating past" --workers=1 --retries=0 --reporter=line - bin/pnpm --dir desktop exec biome check tests/e2e/scroll-history.spec.ts tests/e2e/channels.spec.ts Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
The thread inline-expansion smoke test was asserting a fixed top offset for the nested reply row. Current layout places the row lower than that hard-coded value while still fully visible in the thread scroll body and unobscured by the composer, which is the behavior the test needs to protect. Validation: - cd desktop && ../bin/biome check tests/e2e/messaging.spec.ts - CI=1 bin/pnpm --dir desktop exec playwright test tests/e2e/messaging.spec.ts --project=smoke --grep "opens a single-level thread panel" --workers=1 --retries=0 --reporter=line - CI=1 bin/pnpm --dir desktop exec playwright test tests/e2e/messaging.spec.ts --project=smoke --workers=1 --reporter=line - CI=1 bin/pnpm --dir desktop exec playwright test --project=smoke --shard=2/4 --workers=1 --reporter=line Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
The reaction (kind:7) and reaction-removal (kind:5) events carry only an `e` tag and no channel `h` tag, so the `#h`-scoped aux-backfill query never returned them. On history reload a removed reaction's deletion tombstone was never fetched and the reaction reappeared. The `#e` reference is unique event ids and already fully specific, so the channel scope is redundant for the aux events that carry it and fatal for those that don't. Add a unit test asserting both aux filters key on `#e` only. Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co> Co-authored-by: Pinky <44b8e82baa6e0e254e0208d68f335c283c94e7b78dd1fa10d5a49d3f13dd0435@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
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
1 participant
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 the scrollback issues as a single pagination contract instead of a set of one-off tweaks:
#ereference so page limits buy visible depth without stale overlaysRoot causes
#ebackfill would miss late overlays for visible old messages.Validation
bin/pnpm --dir desktop typecheckbin/pnpm --dir desktop test— 1031 passedbin/pnpm --dir desktop checkbin/pnpm --dir desktop buildbin/pnpm --dir desktop exec playwright test scroll-history --workers=1— 11 passed