feat(thread): anchor the focused note via maintainVisibleContentPosition#226
Open
Kelbie wants to merge 12 commits into
Open
feat(thread): anchor the focused note via maintainVisibleContentPosition#226Kelbie wants to merge 12 commits into
Kelbie wants to merge 12 commits into
Conversation
Opening a thread on a reply landed on the note, then dropped it when the
parent chain prepended above (the T1 full-thread update). Root cause: the
thread LegendList omitted `maintainVisibleContentPosition`, so @legendapp/list
v3 resolved it to `{ data: false, size: true }` and the prepend offset-
compensation (gated on `.data`) never ran.
Add bare `maintainVisibleContentPosition` (normalizes to
`{ data: true, size: true }`): `data` compensates contentOffset for the parent
prepend so the focused note holds; `size` absorbs estimate->measured
reconciliation. Keep `initialScrollIndex` to land on the tapped note, and
deliberately omit the DM ChatScreen's `initialScrollAtEnd` / `alignItemsAtEnd`
/ `maintainScrollAtEnd` — the thread anchors on the note and never auto-pins
to the bottom.
Replaces the manual scrollToIndex re-pin approach (PR #225), which raced the
library's own anchoring and still shifted.
Docs: ADR 0004 + CONTEXT.md "Thread anchor" glossary term.
…tion
Adding maintainVisibleContentPosition alone wasn't enough: mVCP compensates a
prepend by raising the scroll offset, but that's clamped at the max scroll
offset. A thread opened on a reply with little content below it can't scroll far
enough for the compensation to hold the note — so the note still dropped when the
parent chain loaded above.
Add `anchoredEndSpace={{ anchorIndex: targetIndex }}`: it reserves tail space
sized to `viewport - (content from the note down) - footer - paddingBottom`,
giving mVCP the scroll room it needs to keep the note fixed. The reserve shrinks
to 0 once the replies below already fill the screen, so there's no dead gap on
long threads, and it waits for measured sizes so it won't thrash on estimates.
The prop is read by the @legendapp/list@3.0.0 RN runtime but omitted from the
exported RN prop type, so it's typed locally and passed via spread (no `any`).
ADR 0004 + CONTEXT.md updated with the end-reserve rationale.
…ng fixes The mVCP + anchoredEndSpace anchoring landed correctly but sat on the first v3 release (3.0.0), where the exact paths it relies on were buggy: - 3.0.1 — scrollToIndex landed at the wrong location on iOS (our initialScrollIndex) - 3.0.3 — MVCP was mis-batched, "making scroll worse" (our data-anchoring path) - 3.0.4 — anchoredEndSpace reported stale end-space sizes during load (our reserve) - 3.0.5 — on-screen rows could stay stuck in outdated positions No breaking changes across 3.0.1..3.0.6; peer dep is just react. Prop contracts re-verified against 3.0.6 (mVCP normalize identical; anchoredEndSpace still type-omitted from the RN entry, so the local-typed spread stays correct). ADR 0004 records the >= 3.0.4 requirement.
…dEndSpace The forensic log showed the focused reply drops by exactly the prepended parent's estimate->measured miss (200->523, +323) and the scroll then clamps to ~36: there is no room below the note, so mVCP can't raise the offset enough to hold it and the list bottoms out (snaps) — the note can't be kept or refocused. The library's anchoredEndSpace was meant to provide that room but never showed up in the trace (opaque, no RN public type, version-fragile across 3.0.x). Replace it with an explicit reserve owned in ThreadView: add `viewport - (rows below the note x approx height)` to paddingBottom. Generous on short threads (where the note needs the room), shrinks to 0 as replies fill the screen (no dead gap on long threads). Deterministic, hot-reloads with the component, and emits a `thread.reserve` log so a trace can confirm it's live. ADR 0004 + CONTEXT.md updated (Focus reserve replaces End reserve).
Live on-device trace (now that the build actually runs this code) showed the residual: mVCP scrolls to hold the focused note when the parent prepends, but it anchors against the parent's estimatedItemSize (200) and never reconciles the gap when the parent measures to its real height (145) — leaving the note off by exactly estimate-measured (here +55px up; an earlier media parent measured 523 -> -323px down). A single global estimate can't cover parents ranging ~145-523px, and v3 has no per-item estimate. Own that one reconciliation: when a row above the note changes size before the reader has scrolled, counter-scroll by the delta (scrollToOffset(scroll + size - previous)). One-time per parent measure (not a sustained re-pin, so it doesn't race mVCP like PR #225 did); onScrollBeginDrag hands control back to the user. ADR 0004 updated.
…tter The counter-scroll lands the focused note correctly but the prepend + re-anchor plays out over several frames, which reads as jitter on screen (it "ends in the right place" but jumps to get there). Hide that reconciliation entirely: - Withhold parents from the list (listData filters them) and show only the seed (tapped note + skeletons) at full opacity. - When the full thread is ready: fade the list out (120ms), inject the parents while hidden (showParents), let the scroll settle, then fade back in (180ms). Reveal is gated on the above-note size-changes going quiet (each correction pushes it out; a fallback covers the no-change case). - Reconcile each above-note row exactly once (corrected-keys guard) so the counter-scroll can't oscillate. The reader sees: tapped note -> brief crossfade -> settled thread, never the reconcile. Removes the older per-row "reveal hack" (it only masked rows below the note). All source-level, so it hot-reloads on the Metro build. ADR 0004 updated (decision 6).
The fade existed only because the one-list approach reconciled the parent prepend in place and had to hide it. Resolve it properly: render two lists — a seed list (tapped note + replies, parents filtered out) the reader sees immediately, and the real list (full thread) that resolves OFF-SCREEN (opacity 0, pointerEvents none). Once the above-note rows stop changing size, swap instantly (revealed flips; seed unmounts). No fade needed: the resolved view is a pixel match for the seed — same note at the top, same replies below, parents just scrolled off above — and the once-per-row counter-scroll makes the landing exact. If a residual ever showed at the swap it would mean the reconcile isn't exact, which we'd fix rather than mask. Removes the fade machinery (reanimated shared value / withTiming / runOnJS) and the showParents data-gating. ADR 0004 decision 6 updated.
Threads worked except when a parent contained an image: an image with no imeta
dims (and not cached) reserves a 16:9 box, then reshapes to its real aspect on
load — a second, larger above-note size change that our once-per-row guard was
blocking, so nothing compensated it and the note shifted. (Videos are locked to
imeta-or-16:9 and don't reshape.)
Make the counter-scroll the sole, continuous owner of the size axis:
- mVCP -> { data: true, size: false }: library compensates the prepend (data); we
own all above-note size compensation (no double-correction / oscillation).
- Drop the once-per-row guard: compensate EVERY above-note size change by its exact
delta while the reader hasn't scrolled — covers the prepend estimate->measured
jump AND late media reshaping. Off-screen parents growing keeps the note fixed.
- Bump the off-screen settle fallback 600ms -> 900ms so a fast/cached parent image
loads + reshapes before the swap (shown already settled); slow network images
reshape post-swap and are absorbed by the counter-scroll.
Remaining limit: a first-load image with no imeta dims that loads slower than the
settle window can still nudge once before the counter-scroll catches it — the
durable fix is imeta dims (server) or the warmed aspect cache. ADR 0004 updated.
The note anchor is confirmed holding (focused-note pageY constant across a 49-reply load). Document the known, out-of-scope residual: replies settling BELOW the stable note still reflow (fixed-height reply skeletons vs variable real heights; reply images without imeta reshaping on load). The two-list swap can't hide it because the seed renders the same replies — it's the feed-wide variable-height / media-reservation problem, not thread-anchor work.
With 2+ parents the note shifted: my per-row counter-scroll summed deltas via
scrollToOffset(getState().scroll + delta), which undershoots when several parents
measure in a burst (plus their estimate churn). Live trace: 3 parents totalling
711px landed at scroll 575 (~136px short) and arrived as one visible jumpY:-530 at
the swap, leaving the note below the fold.
Stop delta-summing for the initial landing. Split onItemSizeChanged on revealedRef:
- before the swap (off-screen): don't counter-scroll; just wait for the parents to
go quiet, then land the note with a single authoritative
scrollToIndex(noteIndex, { viewPosition: 0 }) — correct for any parent count — and
reveal next frame so the scroll applied while hidden.
- after the swap: keep the per-delta counter-scroll for a slow parent image that
reshapes late (one change at a time → no burst/undershoot).
Single-parent path is unchanged in behaviour (scrollToIndex lands it the same).
ADR 0004 decisions 3 + 6 updated.
Replies sometimes showed un-settled with no skeleton→real fade. Cause: the real list resolves OFF-SCREEN, and it was rendering real reply rows there — so replies loaded (text + images) to a half-loaded state while hidden, then were shown mid-load at the swap with their REPLY_FADE_IN already spent invisibly. Gate real reply content to post-reveal: both lists show reply skeletons pre-reveal (seedData and the hidden realData filter out `reply` rows); real reply rows mount only once the list is visible, where the skeleton→real fade plays as designed. Parents are unaffected — they still settle off-screen for the landing (they're above the note, not below). ADR 0004 decision 7 added.
…hift Gating reply content to post-reveal stopped the half-loaded flash but the replies then reshaped on-screen after mounting (heights vs skeletons + reply images loading) — a reshift after appearing. Resolve the whole thread (incl. replies) off-screen instead: the real list renders replies the entire time (hidden), so they measure and load images while off-screen and are already settled when shown. Reveal now waits for ALL on-screen rows (parents AND first replies) to go quiet, bounded by an absolute cap so a slow image can't stall the seed. Then land the note (scrollToIndex) and CROSSFADE the real list in over the seed (skeletons → settled replies) — seamless for the note (pixel match), and the fade IS the skeleton→real transition the reply flow wants. Seed shows skeletons only, so reply images aren't double-loaded. Residual: a reply image slower than the cap settles after the crossfade (below the focus, rare; imeta/aspect-cache covers the common case). ADR 0004 (6,7) updated.
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.
Why
Opening a thread on a reply lands on that note, then the note drops when the parent/ancestor chain loads in above it. Root cause, verified against the installed
@legendapp/list@3.0.0source:LegendListomittedmaintainVisibleContentPosition. v3'snormalizeMaintainVisibleContentPositionresolves an omitted prop to{ data: false, size: true }..data(if (dataChanged && doMVCP && mvcp.data && …)), so withdata:falseit never runs.useThreaddelivers the thread in two passes — T0 seed shows the target (stable keyt_${id}), T1 prepends the entire parent chain above it in one update. With no data-anchoring,contentOffsetisn't compensated and the focused note falls by the parent chain's measured height.What
maintainVisibleContentPosition(→{ data: true, size: true }) to the thread list.datacompensates the parent prepend;sizeabsorbs estimate→measured reconciliation. Mirrors the DMChatScreen's anchoring.initialScrollIndex(land on the tapped note).initialScrollAtEnd/alignItemsAtEnd/maintainScrollAtEnd— the thread anchors on the note and must never auto-pin to bottom like a chat.Replaces the manual
scrollToIndexre-pin approach (PR #225), which raced the library's own anchoring and still shifted ("don't fight the library's correction").Verification
tscclean;eslint0 errors (pre-existingreact-perfwarnings only);knipclear for touched files; thread unit tests pass.naggFeedClient*test failures are pre-existing on the basefeat/nostr-data-layerbranch (confirmed identical with this change stashed) — not introduced here.ScrollViewmVCP can't be exercised in jest; the behavioral proof is on-device: open a thread on a reply — the focused note must stay fixed as parents load above and replies load below, with no auto-scroll to bottom.Notes
feat/nostr-data-layer(feat(nostr): tiered data-layer — ADR-0003 + engagement-store consolidation #224), per request, so it ships alongside the facade/ADR-0003 thread work.docs/adr/0004-thread-list-anchoring.md+CONTEXT.md"Thread anchor" glossary term.estimatedItemSizeunchanged.🤖 Generated with Claude Code