fix(messaging): stop older-message loads from force-scrolling to bottom

[Yeraze]

Summary

Fixes #4476 — scrolling up in a Meshtastic channel (or DM conversation) to load older messages immediately snapped the view back to the bottom, every time, making it impossible to actually read history.

Root cause: in src/hooks/useMessagingView.ts, the "scroll to bottom on channel/DM entry" effects (GATED EFFECT #1 and GATED EFFECT #4) depend on channelMessages/messages so a still-in-flight initial fetch can still trigger the entry scroll after a channel/tab switch. But that dependency also makes the effects re-fire on every messages-array change — including loadMoreChannelMessages/loadMoreDirectMessages prepending an older page during infinite scroll. Those loaders already restore scroll position correctly via requestAnimationFrame, but ~100–150ms later the entry-scroll effect fires again and unconditionally sets scrollTop = scrollHeight, undoing it.

• src/hooks/useMessagingView.ts — gate both entry-scroll effects behind a ref (pendingChannelScrollRef / pendingDMScrollRef) that's only reset when selectedChannel/selectedDMNode or activeTab actually changes. Later messages-array updates from pagination or new poll messages no longer re-trigger the forced scroll; the existing "auto-scroll on new message only if already near bottom" effects are untouched and still handle the live-update case.

Test plan

• New regression test: scroll position survives an older-page load after the initial entry scroll has already happened
• New regression test: the entry-scroll still fires once when the channel/tab is (re-)entered
• npx vitest run src/hooks/useMessagingView.test.ts — 10/10 pass
• npx vitest run src/hooks/ — 427/427 pass
• npm run typecheck — clean
• npm run lint:ci — clean (no FAIL lines outside .claude/worktrees)

---

Generated by Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review — fix(messaging): stop older-message loads from force-scrolling to bottom

• Gather context and understand the PR
• Read changed files
• Analyze implementation
• Post review feedback

---

Summary

This is a well-targeted, minimal fix for a real UX bug (#4476). The root cause analysis is accurate and the approach is sound. Here's a detailed review:

---

Correctness ✅

The fix correctly identifies the problem: GATED EFFECTs #1 and #4 had channelMessages/messages as dependencies so they could catch a still-in-flight initial fetch — but that same dependency caused them to re-fire on every messages-array update, including pagination prepends. The pending-ref gate solves this cleanly.

The two-effect pattern is correct: One effect resets pendingChannelScrollRef.current = true when selectedChannel/activeTab changes; the second effect performs the scroll only when the flag is set (and clears it). This ensures the scroll fires at most once per channel/tab entry, even if channelMessages updates multiple times before the first messages arrive.

---

Code Quality ✅

The implementation is minimal and well-commented. The inline comment block at src/hooks/useMessagingView.ts:361-371 clearly explains:

• Why channelMessages must remain a dependency
• Why it caused the bug
• How pendingChannelScrollRef gates it

The parallel treatment of pendingDMScrollRef at lines 489-492 is consistent with the channel approach.

---

One Subtle Issue: Potential Timing Race ⚠️

File: src/hooks/useMessagingView.ts:373-394

const pendingChannelScrollRef = useRef(true);
useEffect(() => {
  pendingChannelScrollRef.current = true;  // resets immediately (synchronously)
}, [selectedChannel, activeTab]);

useEffect(() => {
  if (activeTab === 'channels' && selectedChannel >= 0 && pendingChannelScrollRef.current) {
    // ...
    if (hasMessages) {
      pendingChannelScrollRef.current = false;
      setTimeout(() => {
        // scroll happens 100ms later
      }, 100);
    }
  }
}, [selectedChannel, activeTab, channelMessages, setIsChannelScrolledToBottom]);

React processes effects in declaration order within the same render. Since the "reset" effect is declared before the "scroll" effect, both run in the same render pass when selectedChannel/activeTab changes. The reset fires first, then the scroll effect fires and sees pendingChannelScrollRef.current === true. This is the intended behavior and it works correctly.

However, there's a subtle edge case: if the second effect fires before the first (e.g., due to StrictMode double-invocation or concurrent features causing different render ordering), the flag wouldn't be set in time. In practice React guarantees declaration order for sibling effects in the same commit, so this is safe — but worth noting for future maintainers.

---

Test Coverage ✅ Excellent

The two new regression tests in useMessagingView.test.ts:312-411 are exactly right:

1. does not reset scrollTop when older messages are prepended after the initial entry scroll — directly exercises the bug scenario: entry scroll fires and consumes the flag, user scrolls to a position, pagination prepends messages, verify scrollTop is unchanged.

2. still scrolls to bottom once when the channel/tab is (re-)entered — verifies the flag re-arms correctly when activeTab changes, ensuring the fix doesn't over-suppress the entry scroll.

Both tests use fake timers correctly to drain the setTimeout calls and Promise.resolve() microtasks.

---

Minor Observation: setTimeout vs requestAnimationFrame consistency

The pagination loaders (loadMoreChannelMessages, loadMoreDirectMessages) use requestAnimationFrame to restore scroll position (lines 193-197, 262-267), while the entry-scroll effects use setTimeout(..., 100) / setTimeout(..., 150) (lines 386-391, 506-510). This is an existing pattern, not introduced by this PR. The requestAnimationFrame path runs approximately 16ms after the current frame, while setTimeout(fn, 100) runs ~100ms later — so the entry scroll always fires well after the rAF scroll restoration, which is why the old code produced the race (entry scroll fires last and "wins"). The fix avoids the race entirely by preventing the entry scroll from re-firing at all after the flag is cleared, regardless of timing.

---

Security / Performance ✅

No security concerns. The refs are not exposed in the return value. No new API calls are introduced. The change is purely additive state tracking with negligible overhead (two boolean refs, two single-dep effects).

---

Verdict

This is a clean, minimal, well-tested fix. The approach is idiomatic for React scroll management. The regression tests cover both the bug scenario and the positive case. Approved with no blocking issues.