feat: bounded message window for centered Jump to Message#7388
feat: bounded message window for centered Jump to Message#7388diegolmello wants to merge 32 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements anchored, upper-bounded message windows ( ChangesBounded Message Window for Jump-to-Message
SDK TOTP Argument Handling
Sequence Diagram(s)sequenceDiagram
participant RoomView
participant RoomServices_getLocalAnchorTs
participant loadSurroundingMessages
participant anchorForTarget
participant ListContainer
participant useScroll
participant useMessages
participant WatermelonDB
RoomView->>RoomServices_getLocalAnchorTs: request local anchor (rid, targetTs)
alt local anchor found
RoomServices_getLocalAnchorTs-->>RoomView: returns highTs
else fallback
RoomView->>loadSurroundingMessages: fetch surrounding messages
loadSurroundingMessages->>anchorForTarget: compute highTs from rows
anchorForTarget-->>RoomView: returns highTs or null
end
RoomView->>ListContainer: jumpToMessage(messageId, highTs)
ListContainer->>useScroll: jumpToMessage(messageId, highTs)
useScroll->>useMessages: setHighTs(highTs) / fetchMessages()
useMessages->>WatermelonDB: observe / one-shot fetch (ts <= highTs)
WatermelonDB-->>useMessages: rows include target
useMessages-->>useScroll: messages updated -> complete jump (scroll & highlight)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
diegolmello
changed the title
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.74.0.109044 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.74.0.109049 |
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109048 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNT_NRiPGr9JlFGO-oxS3btdz00BqqUy1968eX9Aqax_wZ56RVsJ18G-ZPl0iIiIVd3V1dcmJqGxcgAkk0db |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/views/RoomView/List/hooks/useScroll.test.tsx (1)
19-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock
fetchMessageswith the same async contract as production.
useScrollexpectsfetchMessages: () => Promise<void>, but this helper injects a plainjest.fn()that returnsundefined. That means this suite won't catch promise-handling regressions in the new growth path. Make the default mock resolve a promise so the tests exercise the real contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.test.tsx` around lines 19 - 38, The test helper renderUseScroll currently injects fetchMessages as jest.fn() which returns undefined; update the default mock to match the production contract (a function returning a Promise<void>) so tests exercise async behavior—e.g. change the default fetchMessages parameter in renderUseScroll to an async-resolving mock (use fetchMessages = jest.fn().mockResolvedValue(undefined) or an async () => Promise.resolve()) and ensure useScroll is invoked with that mock; keep references to renderUseScroll, useScroll, makeListRef and makeMessagesIdsRef so the helper remains otherwise unchanged.app/views/RoomView/List/hooks/useScroll.ts (2)
72-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the in-flight jump during unmount cleanup.
This cleanup clears
pendingJump.current?.safety, but it never settles the promise returned byjumpToMessage(). If the list unmounts mid-jump (for example during a room switch), the only remaining resolution path is removed here and any caller awaiting that jump stays pending indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.ts` around lines 72 - 80, The cleanup in useEffect currently clears highlightTimeout.current and pendingJump.current.safety but doesn't settle the promise created by jumpToMessage(), leaving callers awaiting indefinitely; update the unmount cleanup in useScroll.ts to, after clearing pendingJump.current.safety, also call the stored completion callback on pendingJump.current (e.g., pendingJump.current.reject or pendingJump.current.resolve) with a cancellation/error value so the jumpToMessage promise is settled, then null out pendingJump.current to avoid leaks; reference pendingJump, jumpToMessage, highlightTimeout and the useEffect cleanup when making the change.
176-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind deferred scroll retries to the jump that scheduled them.
This timeout keeps
params.highestMeasuredFrameIndexfrom the old failure, but it re-reads the latest target id at fire time. If another jump starts before the timer runs and its target is still absent,targetIndexstays-1and this falls back to the previous jump's measured index, which can scroll the new window to the wrong row. Track/cancel the retry timer per jump before issuing the fallback scroll.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.ts` around lines 176 - 182, The deferred scroll retry must be bound to the specific jump that scheduled it: when scheduling the setTimeout inside useScroll, capture the current jump identifier (e.g., pendingJump.current?.messageId or an internal jumpId) and store the timer id on that jump (e.g., pendingJump.current.retryTimer or a map keyed by jumpId); when a new jump starts clear any existing retry timer for the previous jump; inside the timeout callback verify the captured jumpId still matches the active jump (e.g., pendingJump.current?.messageId === capturedJumpId) before falling back to params.highestMeasuredFrameIndex and calling listRef.current.scrollToIndex, and clear the stored retryTimer on success or cancellation so retries don’t apply to later jumps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/RoomView/List/hooks/useScroll.ts`:
- Around line 149-154: The code calls fetchMessages() fire-and-forget inside the
anchored-jump growth branch which can produce unhandled promise rejections; wrap
the fetchMessages() call with explicit error handling (e.g.
fetchMessages().catch(err => { /* surface error and abort the jump */ })) and on
error either abort the jump (use the existing jump state handler or call an
abort function) or set a visible failure state so the safety timeout doesn't
silently wait; ensure you reference and update jumpGrowthRetries.current and
MAX_JUMP_GROWTH_RETRIES logic consistently when handling the error so
retries/aborts remain correct.
---
Outside diff comments:
In `@app/views/RoomView/List/hooks/useScroll.test.tsx`:
- Around line 19-38: The test helper renderUseScroll currently injects
fetchMessages as jest.fn() which returns undefined; update the default mock to
match the production contract (a function returning a Promise<void>) so tests
exercise async behavior—e.g. change the default fetchMessages parameter in
renderUseScroll to an async-resolving mock (use fetchMessages =
jest.fn().mockResolvedValue(undefined) or an async () => Promise.resolve()) and
ensure useScroll is invoked with that mock; keep references to renderUseScroll,
useScroll, makeListRef and makeMessagesIdsRef so the helper remains otherwise
unchanged.
In `@app/views/RoomView/List/hooks/useScroll.ts`:
- Around line 72-80: The cleanup in useEffect currently clears
highlightTimeout.current and pendingJump.current.safety but doesn't settle the
promise created by jumpToMessage(), leaving callers awaiting indefinitely;
update the unmount cleanup in useScroll.ts to, after clearing
pendingJump.current.safety, also call the stored completion callback on
pendingJump.current (e.g., pendingJump.current.reject or
pendingJump.current.resolve) with a cancellation/error value so the
jumpToMessage promise is settled, then null out pendingJump.current to avoid
leaks; reference pendingJump, jumpToMessage, highlightTimeout and the useEffect
cleanup when making the change.
- Around line 176-182: The deferred scroll retry must be bound to the specific
jump that scheduled it: when scheduling the setTimeout inside useScroll, capture
the current jump identifier (e.g., pendingJump.current?.messageId or an internal
jumpId) and store the timer id on that jump (e.g.,
pendingJump.current.retryTimer or a map keyed by jumpId); when a new jump starts
clear any existing retry timer for the previous jump; inside the timeout
callback verify the captured jumpId still matches the active jump (e.g.,
pendingJump.current?.messageId === capturedJumpId) before falling back to
params.highestMeasuredFrameIndex and calling listRef.current.scrollToIndex, and
clear the stored retryTimer on success or cancellation so retries don’t apply to
later jumps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 436f179d-7006-45ba-b668-0297ccf794b2
📒 Files selected for processing (4)
app/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useScroll.test.tsxapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/RoomView/List/index.tsx
- app/views/RoomView/List/hooks/useMessages.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
|
iOS Build Available Rocket.Chat 4.74.0.109053 |
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109052 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNS7uZ4AhZ-xkYmUPeDNWCAF2nJVGxhh7KhvTZ76CZ-k05LgHg7EWL62vRhDuJ6wGVwsBm79DYavexy0IjcV |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109125 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSdYOuU-hecrJ1DugrVJkrwQEZqZd2b_4lSRntGyrWqc-n_VA9fqMa6EH8tlclb2jEDjYSawQUe18pVp8uy |
|
iOS Build Available Rocket.Chat 4.74.0.109126 |
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Missing a doc with architecture and flow
Add ARCHITECTURE.md and FLOWS.md covering the anchored Message Window model, jump lifecycle, raise/release climb, and the FlashList decision. Move tsToMs next to dayjs, drop the helper barrel export, revert the sdk 2FA-clear comment, and trim the List window/FAB comments.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Trim verbose comments across the List jump path, drop dangling ADR references, and add an explicit .catch on the bounded window-growth fetchMessages call so a grow failure surfaces via the safety net rather than as an unhandled rejection.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
GitHub Actions
Error
…ariants Drop JSDoc scaffolding from anchorResolver (signature is the doc; concepts live in docs/ARCHITECTURE.md) and collapse the __DEV__ equal-ts collision warning to a single line.
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
|
iOS Build Available Rocket.Chat 4.74.0.109130 |
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109129 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRZQSu6NZ_Rp7fl_Nqm3THgcnb8A_3lvVeLCRz4gFFxAMsNFvMp9QnGysTCosGV7agDq3Ju0UkdL-5mfYve |
Tapping "go to last message" from an anchored window swapped the tall anchored rows for the shorter, disjoint live tail in a single emit. A scroll offset deep enough for the tall content then sat past the short content, and the inverted list rendered that as a blank wedge with the FAB stuck visible — unrecoverable by swiping. Pin the viewport to offset 0 (newest) on the still-settled anchored content before releasing the bound: offset 0 is valid for any non-empty window, so the shorter tail can't strand it. Suppress maintainVisibleContentPosition across the swap (new isReleasing flag) so its offset adjustment for the disjoint key set can't drag the viewport back off-content, then re-pin once the live tail emits. Claude-Session: https://claude.ai/code/session_01M5tYizcndsPdF3cQ5eLR3q
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
android_build
GitHub Actions
Inactive
|
iOS Build Available Rocket.Chat 4.74.0.109151 |
|
Android Build Available Rocket.Chat 4.74.0.109158 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRc8RT-EoXq1r8hwv17DRHk5-8seQqLSFiIp4eIlb5B5nPei9GLLgxOe6taN0X-3kvdQbH1JO0Y1ej3PdGd |
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review of the bounded Message Window + anchored Jump-to-Message work — 15 inline notes below, grouped by kind in the order they appear.
- Correctness — the load-bearing one is the rejoin read in
useMessagesthat can truncate a still-present boundary loader and release the Anchored Window across an open Gap. The rest are lower severity: the auto-dispatch effect consuming the boundary loader while anchored, the jump completing before the target is confirmed on-screen, acountbump before theridguard, and two latent guards. - Tests —
cancelJumpToMessage/highlightedMessageIdhave no coverage; thegetLocalAnchormock can't catch query-shape regressions. - Docs/domain — a
setHighTsstep that doesn't exist and a boolean param label in the jump sequence diagram, a "no DB" self-contradiction in the layers table, and a rotting internal SHA on every diagram. - Slop/comments — a
| anythat defeats the jump's types, two over-long JSDocs, "clear the spinner" comments in a hook that owns no spinner, and a duplicated re-scroll block.
Two items aren't inline-able (their lines aren't in the diff): the loader-finder expression visibleMessages.find(m => m.t && MESSAGE_TYPE_ANY_LOAD.includes(...))?.id is duplicated verbatim in useMessages (the rid-change snapshot effect and the auto-dispatch effect) — worth one shared helper; and the glossary drifts in two spots ("page" in useScroll, "(historical)" for the Anchored Window in List/definitions.ts) versus the terms in UBIQUITOUS_LANGUAGE.md.
| // cannot see (the newly-written batch + any new Newer Loader). | ||
| const rows = (await database.active | ||
| .get('messages') | ||
| .query(Q.where('rid', rid), Q.where('ts', Q.gt(currentHighTs)), Q.sortBy('ts', Q.asc), Q.take(QUERY_SIZE + 1)) |
There was a problem hiding this comment.
Bug · high — spurious release across an open Gap. This rejoin read is Q.where('ts', Q.gt(highTs)), Q.sortBy('ts', asc), Q.take(QUERY_SIZE + 1) = 51 rows, with no hideSystemMessages filter (unlike the observation, which applies buildVisibleSystemTypesClause). loadNextMessages writes exactly QUERY_SIZE (50) messages plus one NEXT_CHUNK loader just above them, so 50 messages + loader already fill all 51 slots. Any pre-existing or system row in the (highTs, newLoaderTs] range pushes the boundary loader past slot 51; raiseOrRelease then receives loader-free rows, returns null, and releases the Anchored Window while the Gap is still open — the "no release across a Gap" invariant from ARCHITECTURE.md is violated, and count then grows by every row above the old bound.
The read shouldn't cap a slice of all rows — read the Newer Loaders above the bound directly and hand those to raiseOrRelease: Q.where('rid', rid), Q.where('t', NEXT_CHUNK), Q.where('ts', Q.gt(highTs)), Q.sortBy('ts', Q.desc), Q.take(1). Bounded, the loader can't be crowded out, and it mirrors getLocalAnchorTs. Also lets the separate fetchCount go.
| @@ -173,5 +270,5 @@ export const useMessages = ({ | |||
| } | |||
| }, [serverVersion, rid, t, hideSystemMessages, visibleMessages, dispatch, store]); | |||
There was a problem hiding this comment.
Bug · medium — auto-dispatch consumes the boundary loader while anchored. This effect (lines 251–271) has no highTs guard. In an Anchored Window the boundary Newer Loader sits at ts === highTs (it's inside the Q.lte(highTs) clause, so it's the first loader in visibleMessages). NEXT_CHUNK is in MESSAGE_TYPE_ANY_LOAD, so on a server ≥3.16.0 with hideSystemMessages set, the effect dispatches roomHistoryRequest for that boundary loader → loadNextMessages consumes it → the window auto-climbs (and can auto-release) with no user scroll, burning autoLoadCount toward MAX_AUTO_LOADS. That contradicts the "Anchored Window deliberately does not follow" contract — a user who jumped to an old message would see it drift toward live on its own.
Guard the effect on the bound (if (highTs != null) return;, with highTs added to the deps). The rejoin climb is the user-driven path via Load Newer, not this auto-loader.
| } | ||
| jump.scrolled = true; | ||
| scrollToTarget(jump.messageId, index); | ||
| completeJump(jump); |
There was a problem hiding this comment.
Bug · low — jump completes without confirming the target is on-screen. completeJump (highlight + resolve) runs immediately after scrollToTarget kicks off the two-pass scroll, regardless of whether the row actually landed in the viewport. The two-pass scroll and the frontier climb cover an unmeasured frame, but a post-measurement undershoot on a very tall target row resolves the jump with the message sitting just above the fold. Narrow trigger — flagging because the jump now reports success without the viewability check the prior flow waited on.
|
|
||
| const fetchMessages = useCallback(async () => { | ||
| unsubscribe(); | ||
| count.current += QUERY_SIZE; |
There was a problem hiding this comment.
Bug · low — count bumped before the rid guard. count.current += QUERY_SIZE runs before if (!rid) return, so a render with a falsy rid still inflates count; the next real fetch then issues take(100) instead of take(QUERY_SIZE). Move the increment below the guard. (If rid is in fact always truthy here, the guard is dead code — either way the increment belongs after it.)
| if (localAnchor != null) { | ||
| return localAnchor; | ||
| } | ||
| return target.ts ? tsToMs(target.ts) : null; |
There was a problem hiding this comment.
Bug · low (latent) — guard tests the raw value, not the computed ms. target.ts ? tsToMs(target.ts) : null falls to null for a numeric 0 or empty-string ts (no re-seed), whereas the server path anchors unconditionally. Unreachable today (a cached ts is a Date), but the intent is "anchor when ts is valid": const ms = tsToMs(target.ts); return Number.isFinite(ms) ? ms : null;.
| Scroll->>Scroll: completeJump — clear safety, highlight | ||
| ``` | ||
|
|
||
| _Last verified: f78d6a37c_ |
There was a problem hiding this comment.
Docs · low — rotting marker. _Last verified: f78d6a37c_ (repeated on every diagram — lines 29, 68, 100, 125, 159, 183, 209) pins a bare internal commit SHA that isn't resolvable from the doc, isn't updated on merge, and goes stale the next time anything around it changes. Drop it; git history already records when each diagram last moved.
| @@ -10,7 +10,9 @@ const getMessageInfo = async (messageId: string): Promise<TMessageModel | TThrea | |||
| id: message.id, | |||
There was a problem hiding this comment.
Slop · moderate — | any defeats the return type. The signature on line 6 is Promise<TMessageModel | TThreadMessageModel | any | null>; the | any collapses the whole union to any, so callers get no checking on the ts / shape the jump flow now depends on (resolveJumpAnchor reads target.ts / fromServer). All three returned literals here match the IJumpTarget shape already declared in resolveJumpAnchor.ts. Type the function Promise<IJumpTarget | null> and drop | any.
| import { tsToMs } from '../../../lib/dayjs'; | ||
| import { type TAnyMessageModel } from '../../../definitions'; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Slop · moderate — over-long doc comment. This 12-line JSDoc (and the 11-line one on resolveJumpAnchor) re-narrates what the code and ARCHITECTURE.md's Anchor-resolution section already state. The part that earns its place: "returns the ts of the nearest Newer Loader above the target — the upper bracket of its Chunk — or null when the cached region is contiguous to live." ~3 lines is enough.
|
|
||
| export const useScroll = ({ listRef, messagesIds }: { listRef: TListRef; messagesIds: TMessagesIdsRef }) => { | ||
| // Abort a jump whose target never re-observes within this window: release the anchor, drop to the Live | ||
| // Tail, clear the spinner. Does not cancel an in-flight scroll — completion is reactive on re-observe. |
There was a problem hiding this comment.
Slop · moderate — comment describes a side effect this hook doesn't own. useScroll has no spinner — the loading overlay is RoomView's sendLoadingEvent. "clear the spinner" here (and on lines 107 and 122) describes work this hook doesn't do; it resolves the jump promise and RoomView clears the overlay. Say "resolve the jump" / "release the anchor". Separately, the JUMP_SCROLL_POSITION comment (line 22) restates the literal values ("centered, offset clear, non-animated" = viewPosition: 0.5, viewOffset: 100, animated: false) — keep only the non-obvious animated: false rationale.
| // highestMeasuredFrameIndex), then re-attempt — it lands once measured, or re-fires to climb on. | ||
| if (targetIndex > params.highestMeasuredFrameIndex) { | ||
| listRef.current?.scrollToIndex({ index: params.highestMeasuredFrameIndex, animated: false }); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Slop · nit — duplicated re-scroll. This deferred block duplicates scrollToTarget's second pass (lines 159–168): same setTimeout(SCROLL_TO_INDEX_RETRY_DELAY), same findIndex settle, same scrollToIndex({ ...JUMP_SCROLL_POSITION }). Extract a shared reScrollWhenSettled(targetId) so the two can't drift apart.
…from consuming the boundary loader Read the rejoin signal from the Newer Loader directly (t=NEXT_CHUNK, ts>bound, desc, take 1) instead of a fixed-size, system-unfiltered row slab. A pre-existing or system row in the gap could push the boundary loader out of the fetched set, leaving the resolver loader-free and releasing the Anchored Window while the Gap was still open. Skip auto-dispatch while anchored (highTs guard + dependency): the boundary Newer Loader sits at ts === highTs, inside the observed range, so on servers >= 3.16.0 with hideSystemMessages set the effect would dispatch a history request and climb or release the window with no user scroll. Extract reScrollWhenSettled, shared by the two-pass scroll and the scroll-to-index-failed retry. Trim over-long JSDoc on getLocalAnchorTs and resolveJumpAnchor. Drop "spinner" wording from useScroll comments (the hook owns no spinner). Align FLOWS.md with the single jumpToMessage(id, highTs) entry point and the number | null signature, and fix glossary drift in window-growth comments. Add tests: cancelJumpToMessage branches and the highlightedMessageId lifecycle in useScroll; the exact query clause shape in getLocalAnchor. Claude-Session: https://claude.ai/code/session_01YQ7pAA9Tp5PQXC1j5b6Agx
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Follow-up addressing the review on this PR. Scope: moderate-or-higher findings plus all slop; low/nit items that aren't slop are deferred to a separate pass. Inline notes mark each change.
Addressed: the spurious release across an open Gap and the anchored auto-dispatch (correctness); both test gaps; the FLOWS.md call-graph/signature drift; the over-long JSDoc, the 'spinner' comments, and the duplicated re-scroll (slop); plus the duplicated loader-finder and the 'page'/'(historical)' glossary drift.
Not taken — | any on getMessageInfo's return: the suggested Promise<IJumpTarget | null> doesn't hold. The | any predates this PR, and the caller reads .rid / .replies / .tmsg / .t / .e2e / .tlm off the result and feeds it into model-typed navigation helpers; the returned literals carry fields beyond IJumpTarget. An honest fix cascades into those signatures — out of scope here, worth its own typing pass.
Deferred (low / latent, not slop): jump completing before confirming the target is on-screen; count bumped before the rid guard; the raw-value-vs-computed-ms guard in resolveJumpAnchor; the uncleared instance field on the main-list path; the ARCHITECTURE.md 'no DB' self-contradiction; the rotting verified-SHA marker.
Verified locally: RoomView suites 83/83, eslint clean on all touched files, tsc clean for these files (it also flags app/stacks/OutsideStack.tsx from the recent develop merge — unrelated to this change).
| // invocation is stale and must not mutate count / highTs for the new window. | ||
| const sub = subscription.current; | ||
|
|
||
| // Read the Newer Loader closest to the Live Tail directly (highest ts above the bound) so non-loader rows can't crowd the boundary loader out of the fetched set. |
There was a problem hiding this comment.
Rejoin now reads the boundary Newer Loader directly — NEXT_CHUNK, ts > bound, desc, take(1) — instead of a fixed-size, system-unfiltered slab above the bound. With the old slab read, a pre-existing or system row in the gap could push the boundary loader past the fetched rows; the resolver then saw no loader and released the Anchored Window while the Gap was still open. The release-time count query stays — it feeds reading-position growth and isn't redundant with a single-loader read.
| */ | ||
| useEffect(() => { | ||
| // Anchored Window deliberately does not auto-follow; only a Live Window auto-dispatches. | ||
| if (highTs != null) { |
There was a problem hiding this comment.
highTs != null guard (plus highTs in the deps) stops the Anchored Window auto-dispatching. The boundary Newer Loader sits at ts === highTs, inside the observed range; without this, servers >= 3.16.0 with hideSystemMessages set would consume it and climb or release the window with no user scroll.
| import { roomHistoryRequest } from '../../../../actions/room'; | ||
| import { isNewerLoader, raiseOrRelease, type AnchorMessage } from '../../services/anchorResolver'; | ||
|
|
||
| const findFirstLoaderId = (messages: TAnyMessageModel[]): string | null => |
There was a problem hiding this comment.
Extracted the duplicated loader-finder into findFirstLoaderId, shared by the rid-change snapshot effect and the auto-dispatch effect.
|
|
||
| export const useScroll = ({ listRef, messagesIds }: { listRef: TListRef; messagesIds: TMessagesIdsRef }) => { | ||
| // Abort a jump whose target never re-observes within this window: release the anchor, drop to the Live | ||
| // Tail, resolve the jump. Does not cancel an in-flight scroll — completion is reactive on re-observe. |
There was a problem hiding this comment.
Dropped the 'clear the spinner' wording here and in completeJump / abortJump — this hook owns no spinner (RoomView's loading event does). Restated as resolving the jump.
| // cap must cover the distance (5 stalled short). Bounded so an unreachable target aborts, not loops. | ||
| const MAX_SCROLL_TO_INDEX_RETRIES = 20; | ||
|
|
||
| // animated:false snaps straight to the target instead of smooth-scrolling through every row between here |
There was a problem hiding this comment.
JUMP_SCROLL_POSITION comment trimmed to the animated: false rationale; the prior text just restated the literal viewPosition / viewOffset values.
| import { type TAnyMessageModel } from '../../../definitions'; | ||
|
|
||
| /** | ||
| * ts of the nearest Newer Loader above the target = the upper bracket of its Chunk; null when the |
There was a problem hiding this comment.
JSDoc cut from 12 lines to 2; it re-narrated the code and ARCHITECTURE.md.
| } | ||
|
|
||
| /** | ||
| * Upper ts bound (ms) for the re-seeded window, or null to stay on the Live Tail. |
There was a problem hiding this comment.
JSDoc trimmed to the null cases plus the fromServer / cached split.
| expect(query).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('builds the correct query clause shape', async () => { |
There was a problem hiding this comment.
New test asserts the exact query clause shape (rid, NEXT_CHUNK, ts > target, asc, take(1)); dropping any clause now fails, where before only toHaveBeenCalled() was checked.
| await waitFor(() => expect(jumpResolved).toBe(true)); | ||
| }); | ||
|
|
||
| it('cancelJumpToMessage is a safe no-op when no jump is pending', () => { |
There was a problem hiding this comment.
Added coverage for both cancelJumpToMessage branches — no-op when nothing is pending, and abort + anchor release + safety-timer disarm when an unscrolled jump is pending — and for the highlightedMessageId lifecycle (set on completion, cleared after HIGHLIGHT_TIMEOUT).
| Resolve->>AR: anchorForServerChunk(rows, id, ts) | ||
| AR-->>Resolve: highTs (Newer Loader ts) | null | ||
| Resolve-->>RV: bound | ||
| RV->>Scroll: jumpToMessage(id, highTs) |
There was a problem hiding this comment.
Diagram corrected: RV makes one call, jumpToMessage(id, highTs), and useScroll sets the bound internally — the prior RV->Msgs: setHighTs step didn't exist. Diagram 1 likewise shows the arg as null, not anchored=false (signature is number | null).
1 participant
Proposed changes
Rebuilds Jump to Message on a bounded message window so a deep jump re-anchors in one step (O(1)) instead of growing the window page-by-page (O(pages)) — removing the root cause of the old 5s-race flakiness — without migrating off the custom native inverted list.
The whole capability is one optional upper
tsbound (highTs) on the existing WatermelonDB observation:highTs == null→ Live Window (newest-first, follows the Live Tail) — unchanged default behavior.highTs→ Anchored Window pinned around the target, below the Live Tail.A pure, DB/React-free anchor resolver decides the bound (
anchorForTarget) and climbs it back toward live as Newer Loaders are consumed (raiseOrRelease), releasing to a Live Window only once the Gap to the Live Tail has fully closed (invariant: never release across an open Gap). "Rejoin live" is now explicit — the Load Newer chain or the jump-to-bottom FAB (which stays visible while anchored).Also included:
take(count)and the list doesn't snap to the Live Tail.onScrollToIndexFailed(nogetItemLayout→ the inverted list re-fires it synchronously → stack overflow). The retry re-applies the same centering + header-clearing view offset as the initial scroll, so a distant jump that falls back to the retry path still lands centered instead of hidden behind the room header.getItemLayoutfor these variable-height messages, the firstscrollToIndexuses an estimated offset and can undershoot while the target's row is still unmeasured (e.g. entering a room fresh from the rooms list), leaving the target hidden above the viewport. The jump now scrolls a second time against the measured frame once the row has rendered, so the target always lands centered.isMessageInWindow), not message origin, so a locally-cached-but-out-of-window target no longer silently aborts; three roads (in-window / cached-out-of-window / server). A server-fetched target whose chunk reaches the Live Tail (e.g. a push-notification deep link onto a recent message) keeps the room in a Live Window instead of pinning it in an unreleasable historical view; the target-ts fallback covers the remaining cases (cached target with no bracketing Loader, target absent from the fetched chunk).loadSurroundingMessagescall; one-shot jump param so re-searching the same message re-fires; guard the anchor raise/release against a re-subscribe landing mid-flight (room switch / concurrent raise) so a stale read can't corrupt the new window.Architecture and flow diagrams are documented alongside the code:
app/views/RoomView/docs/ARCHITECTURE.md— the window model, layer responsibilities, anchor resolution, the rejoin (raise/release) climb, scroll landing, the rejected FlashList v2 migration and why, and the load-bearing invariants tied to the unit tests.app/views/RoomView/docs/FLOWS.md— sequence diagrams for each jump and window-management handshake (in-window jump, server-fetched anchor, deep-target growth + safety net, frontier climb, raise/release rejoin, jump-to-bottom, thread-jump timing).New domain vocabulary is in
UBIQUITOUS_LANGUAGE.md(§ Message Loading).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1224
How to test or reproduce
Screenshots
No static UI redesign; changes are behavioral (centered jump; FAB visibility while anchored).
Types of changes
Checklist
Further comments
The list engine is unchanged; a future FlashList migration remains possible but is neither required nor blocked by this work. Ordering stays
ts-only (ats + _idcomposite is a deferred follow-up noted inARCHITECTURE.md). New unit coverage:anchorResolver(including the server-chunk resolver),useMessages(release-window growth),useScroll(deferred + capped retry, header-clearing offset on retry, re-scroll after measurement, bounded growth + growth cap for deep anchored targets),getLocalAnchor.