fix: profile grid pagination + scroll restoration - #536
Conversation
The profile grid passed `react-infinite-scroll-component` a `dataLength` taken from the rendered video list, which is deduplicated by addressable key and filtered against the viewer's block list. That component re-arms its internal `actionTriggered` guard only when `dataLength` changes, so any fetched page whose rows all collapse — duplicate `pubkey:kind:d-tag` rows, or a page of entirely blocked authors — left the guard latched and the grid stopped paginating for good, while `hasNextPage` was still true. Report a fetched-row count from `useVideoProvider`, taken from the unfiltered query pages so block filtering can't flatten it, and drive the scroll trigger from that. Closes #380
Scroll restoration issued a single `window.scrollTo` from a layout effect. On back navigation into a feed the rows have not rendered yet, so the document is shorter than the saved offset, the browser clamps the request, and the viewer lands near the top — exactly what deep scrolling into a profile grid hits. Retry across frames until the document can hold the saved offset, giving up after a timeout, and bail out as soon as the viewer scrolls so restoration never fights a deliberate gesture. Forward navigation still lands at the top. Closes #379
Hand-over detection listened for wheel, touchstart and keydown. Dragging the scrollbar fires none of them, so on a page whose saved offset can never be reached the loop re-pins the viewport every frame for the full three seconds while the viewer tries to drag away. That is exactly the short-page case the retry loop exists to serve.
The layout-effect cleanup saved window.scrollY unconditionally. While a restore is still chasing its target the window is clamped short of it, because the content that would make room has not rendered. Navigating away mid-restore therefore persisted that partial value over the real one, and every interrupted back-navigation walked the feed closer to the top. Only save once the restore has settled, meaning it reached its target, timed out, or was handed over to the viewer. In the hand-over case the viewer's own position is the one worth keeping. This predates the branch, and the same probe fails identically on the merge base, but it is local to the code this branch rewrites and it defeats the feature the branch adds, so it is cleaned up here rather than left behind.
`html { scroll-behavior: smooth }` applies app-wide (src/index.css:233), and the
positional `window.scrollTo(x, y)` form scrolls with behavior "auto", which
resolves to the root element's computed value. So every attempt in the restore
loop started or retargeted an animated scroll.
Two consequences, both measured in the repo's own Chromium against a page tall
enough to honour the offset in a single write:
- The loop chased its own animation rather than the page's height: 154 frames
over 1271ms, against 1 frame and 0ms once the animation is out of the way.
That spent nearly half of RESTORE_TIMEOUT_MS before the position landed.
- Cancelling the loop did not cancel the animation. Stopping on a real
`mousedown` recorded stop() at y=292 while the page carried on to y=6000, so
the handover listeners could not actually hand the page back.
Restore with the options form and an explicit `behavior: 'instant'`, verified to
land synchronously despite the CSS. Forward navigation stays on the positional
form, so its behaviour is unchanged.
Both test files mocked `scrollTo` with positional parameters; they now read the
offset from either call shape, and a new assertion pins the call form so the
animation cannot come back unnoticed.
The cleanup decided whether an offset was worth saving by asking whether the retry loop was still pending. But cancelling the loop is not the same as moving the page: `mousedown` and `keydown` hand control back without scrolling anywhere. So a click during a restore stopped the loop while the page still sat at the offset the loop had been clamped to, cleanup read that as "settled", and the clamped value overwrote the offset the restore was chasing — the same defect the pending check was added to prevent, reached through the handover listeners rather than through an interrupted navigation. Back into a feed saved at 1800 with the grid still short (clamps to 150), one mousedown, then forward and back again restored 150. Without the mousedown the same sequence restored 1800, so the check works and the listeners are the hole. Compare the live offset against what the loop last wrote instead. Reaching the target, a wheel, a touch, and a scrollbar drag that actually moved all still save; a click, a keystroke, and a timeout on a page that never grew all leave the saved offset alone. One case changes deliberately: a restore that times out no longer persists its clamped position, because under this rule the viewer never chose that offset. The two existing cases pass either way, because `fireEvent.click` dispatches no `mousedown` and the scrollbar-drag case assigns a new scrollY afterwards. The added case fails without the fix.
Stub the profile chrome around the grid — header, pinned videos, lists, the grid itself and the list-mode feed — so the test exercises the infinite-scroll wiring rather than the whole page's data fetching, and assert the grid still renders only the deduplicated videos while dataLength reports fetched rows. Rendering the page pulls in a large module graph, so the case carries its own timeout rather than relying on the default. This work belongs with the pagination fix; it was committed against the unrelated auth change by mistake.
Deploying divine-web with
|
| Latest commit: |
84f75fa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc720c89.divine-web.pages.dev |
| Branch Preview URL: | https://fix-profile-grid-pagination.divine-web.pages.dev |
🚀 Preview DeploymentLast updated:
|
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Checked both diagnoses against the library source and the backend rather than taking the description on trust. They hold up.
- The library guard is exactly as described —
componentDidUpdatebails whendataLengthis unchanged, soactionTriggerednever clears (react-infinite-scroll-component@6.1.0,dist/index.js:374-377). - Duplicate rows are real.
/api/users/{pubkey}/videosis a plainLIMIT/OFFSETovervideo_statswith no d-tag dedup and noORDER BYtiebreaker (divine-funnelcake crates/clickhouse/src/client.rs:5566-5574), so rows repeat across pages. Offset advances by raw row count on both ends —funnelcakeClient.ts:458,handlers.rs:2971-2975. The cursor claim is right. vineIdis thedtag (videoParser.ts:373) andparseVideoEventsdrops kind-34236 events without one, sovineId || idnever applies to addressable videos. Matches NIP-01'skind:pubkey:didentity; divine-mobile uses the same key and isn't touched by this change.- The tests earn their place: 9 of them fail on an
origin/mainworktree and pass here.tscclean, eslint 0 errors / 17 warnings (same asmain), authorship intact on all four reviewer commits.
One correctness gap to fix before merge — it's in bf304fd1, which is mine. The conditional save also drops positions the viewer did choose. Repro inline, with a control. Four smaller notes inline; none of those block.
I'm leaving all of these as comments rather than pushing, because each remedy involves a call that's yours: the fix for the blocking one has a tempting version that reopens exactly what bf304fd1 closed.
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
✅ back-navigation now keeps the position the viewer actually chose — replies on the threads.
Both diagnoses in the description hold up against the library source and the backend, and the pagination fix does what it says. I pushed five commits rather than leaving comments; the description now carries them.
Fixed on the branch
- The blocking finding on
ScrollToTop: a viewer position that coincided with an interrupted restore's clamped offset was discarded, which made the top of the feed unsaveable and left a stale offset to be restored next time. - Two holes in that gate — a browser-initiated scroll being read as viewer intent, and the
pagehidesave bypassing the gate entirely. - The fetched count infinite scroll paginates on no longer depends on how many rows survived parsing, and is now type-required so it cannot revert silently.
Checks
tsc,eslint(0 errors, 17 warnings, unchanged frommain), the full suite at 1760 tests, andvite build— all green locally, andtest (20.x)is green on this head, which is the same command as a unit including the install step.- The two new regression tests fail against the previous commit with the reported symptoms, so they pin the behaviour rather than describing it.
- Local runs alone would not have discharged the declared check: it is a multi-command script, so CI is what covers it end to end.
Not fixed here
Three things are deliberately left, each with the reason on its thread or in the description: the smooth scroll-to-top on forward navigation, the List-view latch in #529, and a transform crash on malformed rows that is worth its own issue.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Re-reviewing the head after the takeover commits. My earlier CHANGES_REQUESTED is settled — all five threads are answered and resolved, and I checked the fixes by breaking them rather than by reading them. Nothing new to push.
What I verified
The blocking one (4e58977, 55ab5b7). Put the old component back under the current suite — git show 4884bbc:src/components/ScrollToTop.tsx over HEAD — and three tests fail. The first is the repro I posted:
× persists the top of the feed after an interrupted restore → expected 1800 to be +0
× ignores a scroll the viewer did not cause → expected 300 to be 1800
× does not let pagehide persist an in-flight restore → expected 150 to be 1800
So the teardown equality is gone, and each hole found in its replacement is pinned by a case that fails without the fix. I also walked the paths the equality used to swallow: the loop's own writes can't latch (they land on written, and the listener doesn't exist until handOver); the input set covers every way I could find to move the page by hand, including the ones that fire nothing else — scrollbar drag and middle-click autoscroll are mousedown, find-in-page latches on the Ctrl+F keydown before the find bar takes focus; and the throwaway cleanup React 18 StrictMode runs on mount can't persist anything, because viewerMoved is still false there.
Test coverage (4e58977). isViewerChosen: () => false now fails two tests where it used to leave the suite green. The mock dispatching a real scroll on every write is the part that mattered — without it, "the loop wrote this" and "the viewer did" were indistinguishable and the whole gate rested on a property nothing exercised.
The count (c623b87, 84f75fa). Better than the wording change I asked for; I flagged the comment and missed that the number under it had the same defect one layer down. fetchedRows reads exactly the rows transformFunnelcakeResponse starts from — response.videos, or the bare array on the edge-injected path — so it isn't a parallel notion of a page. The required-field argument holds: dropping fetchedRows: page.fetchedRows from the network return fails the build rather than quietly reverting to the parsed length, because the query function stops satisfying FunnelcakeVideoPage and it cascades out through TQueryFnData.
The two declined findings. Both correctly declined. The bottom-parked viewer resuming on the next nudge is onScrollListener-only next in the library, and the description now says so given #380 is marked closed. scroll-behavior: smooth on the forward-navigation path is a UX call, and this branch is byte-identical to main there.
Validation
tsc -p tsconfig.app.json --noEmit clean; eslint clean on every changed source file. Ran the six affected suites — 60 tests, all green — plus the three mutations above to confirm each guard actually fails when removed. CI is green on this head, which is the arbiter for the full suite given the local flakiness under load noted in the description.
Notes, not requests
- One residue in the
fetchedCountshape: a page the API returns with zero rows buthas_more: trueand a cursor leaves the count flat, so the trigger latches exactly as before. It takes the backend contradicting itself, and it's much narrower than what this closes — worth knowing it's the last surviving path of #380 on this surface. - On the null-
tagsbug flagged for separate tracking: insidetransformFunnelcakeResponseit's caught and the row is dropped, as the description says. ButConfirmedTab.tsx:28callstransformFunnelcakeVideounguarded during render, so there it's a render crash rather than a dropped row. Worth carrying into whatever issue tracks it. - The documented gate residue — an input latching a browser-initiated scroll — is the right call to leave open. A timing window that narrows rather than shuts the hole, and whose wrong value discards positions the viewer really chose, is the louder failure.
Approving. Both fixes do what they claim, the mechanisms that replaced my findings are each pinned by a test that fails without them, and what's left open is documented and tracked.
4 participants
Split out of #531, which bundled these with an unrelated NIP-46 change. Every review finding on that PR landed against the auth commit; these two fixes drew none, so they shouldn't wait on another auth round.
Closes #380, #379.
Carries the reviewer commits from #531 that belong to this work, with original authorship intact — four from @mbradley and @realmeylisdev. #531 keeps the auth work.
1. Profile video grid stops paginating mid-scroll (#380)
react-infinite-scroll-componentre-arms its internalactionTriggeredguard only whendataLengthchanges (dist/index.js:374-377— literally "do nothing when dataLength is unchanged").ProfilePagepassed the rendered list length, which is deduplicated by addressable key and filtered against the viewer's block list.Any fetched page whose rows all collapsed — duplicate
pubkey:kind:d-tagrows, or a page of entirely blocked authors — leftdataLengthflat, the guard latched, and the grid stopped paginating permanently whilehasNextPagewas stilltrue.The server cursor was never at fault:
fetchUserVideosadvances offset by raw row count.Fix:
useVideoProviderreportsfetchedCountfrom the unfiltered query pages — counting filtered pages would reintroduce the stall the moment a page is entirely blocked authors — and the scroll trigger keys off that.2. Grid loses scroll position on back navigation (#379)
Restoration issued a single synchronous
window.scrollTofrom a layout effect. On back navigation the rows haven't laid out, so the document is shorter than the saved offset, the browser clamps, and the viewer lands near the top.Fix: retry across frames until the document can hold the offset, give up after a timeout, and bail as soon as the viewer takes over. Forward navigation still lands at the top.
The regression test models the real failure — a
scrollTomock that clamps to a document height which grows after the first attempt. The pre-existing tests couldn't catch this because their mock always accepted the target.Reviewer fixes on top
702c5fef(@mbradley) — a scrollbar drag fired none ofwheel/touchstart/keydown, so the loop re-pinned the viewport every frame for the full timeout while the user dragged. Addsmousedown.6ce0e105(@mbradley) — cleanup savedwindow.scrollYunconditionally, including the clamped value a restore was still chasing, so each interrupted back-navigation walked the saved position toward the top. Pre-dates this work; fixed here because it defeats the feature.62296380(@realmeylisdev) —html { scroll-behavior: smooth }is app-wide, and positionalscrollTo(x, y)inherits it, so every attempt was animating. Measured 154 frames / 1271ms to satisfy the target on a page already tall enough, versus 1 frame with the animation out of the way. Worse, cancelling the loop didn't cancel the animation, so the handover listeners couldn't actually hand the page back. Now uses the options form withbehavior: 'instant', with an assertion pinning the call shape.bf304fd1(@realmeylisdev) —mousedowncancellation punched a hole in the previous fix: a click hands control back without moving the viewport, so the loop stopped at the clamped offset and cleanup persisted it as if settled. Now asks whether the viewer actually scrolled instead of asking whether the loop stopped. (Superseded in mechanism by the reviewer commits below, which keep the same intent.)Validation
npm test(the CI command). Local full-suite runs on this machine are currently flaky under load —origin/mainalone fails 8 tests across 5 files the same way, and every one passes in isolation on both branches — so CI is the arbiter here rather than my local numbers.tsc -p tsconfig.app.json --noEmitclean, eslint 0 errors / 17 warnings (unchanged frommain), build succeeds.Note on the last commit
4884bbcbmoves test work that was committed against the auth change by mistake — component stubs, a dedup assertion, and a per-case timeout forProfilePage.infiniteScroll.test.tsx. It belongs with the pagination fix and is here rather than in #531.Reviewer takeover commits (@dcadenas)
Pushed on top of
4884bbcbwhile reviewing. The two fixes above are unchanged in intent; these close defects found in how they were implemented.4e589779— @realmeylisdev's blocking finding. "Only persist what the viewer chose" was decided by comparing the final offset against what the restore loop last wrote, sampled once at teardown. When a restore is interrupted that value is the clamped offset — on a grid that hasn't laid out, exactly0— so any viewer position that happened to coincide with it was discarded, which made the top of the feed unsaveable and left a stale offset to be restored next time. Now latches a real scroll after handover instead.55ab5b70— two holes in that gate. Ascrollthe viewer did not cause (scroll anchoring, the document shrinking, focus-driven scroll) was read as intent, so a stray event could still hand a clamped offset back into storage; a scroll now only counts once a real input event has fired. Separately, thepagehidesave bypassed the gate entirely — and the module scope, so the saved-position map, survives bfcache — so it now clears the same guard. The test harness also dispatches a realscrollevent on everyscrollTo, so the loop's own writes are no longer silent in the suite.c623b87e,84f75fa1—fetchedCountwas taken after the transform had dropped rows and deduplicated within the page, so the count infinite scroll paginates on still depended on how much of a page survived.transformToVideoPagenow carriesfetchedRows— the row count the API returned — andFunnelcakeVideoPagedeclares it required, which is what surfaced two logged-out early-return paths that carried no count at all.Known residue, deliberately not fixed: the viewer-took-over gate latches on any input rather than correlating an input with the scroll it caused, so an unrelated tap or keystroke during an interrupted restore can still let a browser-initiated scroll be persisted. Closing that means a timing heuristic that narrows the window rather than shutting it, and whose wrong value would discard positions the viewer really chose — the louder failure. Left documented in
ScrollToTop.tsx.Unrelated, found while reviewing and worth separate tracking:
transformFunnelcakeVideothrows on a row with a nulltagselement —funnelcakeTransform.ts:169-170andparseLoopsFromTagsat:174guard a missingtagsarray, not a null element inside it. Only reachable on malformed API rows, andfetchedRowsalready keeps it from stalling pagination.Deliberately out of scope
#529 — the same
dataLengthstall shape is still live inVideoFeed.tsx(×2),VideoPage.tsxandSearchPage.tsx.#530 —
ProfilePage's vanity-URLhistory.replaceStatedesyncs the scroll-restoration key, so profiles with a verified NIP-05 can still lose position in production despite fix 2. It's skipped in DEV, so it won't reproduce locally. Separate root cause; worth knowing this PR doesn't fully close the user-visible symptom for those profiles.