Skip to content

fix: prevent spontaneous scroll jumps when reading old messages#668

Open
torlando-tech wants to merge 10 commits intomainfrom
worktree-message-scrolling
Open

fix: prevent spontaneous scroll jumps when reading old messages#668
torlando-tech wants to merge 10 commits intomainfrom
worktree-message-scrolling

Conversation

@torlando-tech
Copy link
Copy Markdown
Owner

Summary

  • Remove _messagesRefreshTrigger from the paging flow's combine() — it was creating a brand-new PagingSource every 30 seconds (and on each delivery receipt), discarding all loaded pages and causing the viewport to jump back toward recent messages
  • Room's PagingSource already auto-invalidates on DB writes with proper key-based scroll position preservation
  • formatTimestamp() is a pure function that recomputes relative times on each recomposition, so no data-layer refresh was ever needed for timestamp updates

Test plan

  • MessagingViewModelTest — all pass
  • MessagingViewModelImageLoadingTest — all pass
  • Manual: open a long conversation, scroll far up, wait 60+ seconds — viewport should stay put

🤖 Generated with Claude Code

Remove _messagesRefreshTrigger from the paging flow's combine().
Every 30s (and on each delivery receipt), the trigger incremented,
causing flatMapLatest to create a brand-new PagingSource — discarding
all loaded pages. With enablePlaceholders=false, the rebuilt source
could only approximately restore scroll position, jumping the user
back toward recent messages.

Room's PagingSource already auto-invalidates on DB writes, preserving
scroll position via key-based anchoring within the existing Pager.
formatTimestamp() is a pure function that recomputes relative times
on each recomposition, so no data-layer refresh was ever needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread app/src/main/java/com/lxmf/messenger/ui/screens/MessagingScreen.kt
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 12, 2026

Greptile Summary

This PR removes _messagesRefreshTrigger from the paging combine() in MessagingViewModel, which was forcing a brand-new PagingSource every 30 s and on every delivery receipt — destroying all loaded pages and snapping the viewport back toward recent messages. Timestamp updates are now driven by a Compose-level timestampTick clock passed as a parameter to MessageBubble, triggering recomposition of only visible items without touching PagingData.

Confidence Score: 5/5

Safe to merge — the fix is correct and all remaining findings are style-level.

The root cause (PagingSource recreation on every tick/delivery receipt) is accurately identified and properly fixed. Room's PagingSource reliably auto-invalidates on DB writes; the old workaround comment about cachedIn() blocking propagation was incorrect. The Compose-level timestampTick clock pattern (passing an unused-but-tracked parameter to force recomposition) is a well-established technique. Import reordering and test-mock additions are cosmetic/correctness improvements with no risk.

No files require special attention.

Important Files Changed

Filename Overview
app/src/main/java/network/columba/app/viewmodel/MessagingViewModel.kt Removes _messagesRefreshTrigger from combine() and drops the refreshTimestamps() helper; Room's PagingSource auto-invalidation now handles delivery-status and new-message refreshes correctly.
app/src/main/java/network/columba/app/ui/screens/MessagingScreen.kt Drops LaunchedEffect(timestampTick) { viewModel.refreshTimestamps() }; instead passes timestampTick directly to MessageBubble so Compose recomposes only visible bubbles on each clock tick.
app/src/main/java/network/columba/app/nomadnet/PartialManager.kt Import block reordered to standard alphabetical Kotlin style; no logic changes.
app/src/main/java/network/columba/app/viewmodel/NomadNetBrowserViewModel.kt Import block reordered to standard alphabetical Kotlin style; no logic changes.
app/src/test/java/network/columba/app/viewmodel/MapViewModelTest.kt Adds previously missing mock for settingsRepository.mapStylePreferenceFlow to fix a test compilation/runtime gap.
app/src/test/java/network/columba/app/viewmodel/NomadNetBrowserViewModelTest.kt Import block reordered to match the source-file style change; no logic changes.
micron/src/test/java/network/columba/app/micron/MicronParserTest.kt Three method-chain expressions reformatted for readability; no logic changes.

Sequence Diagram

sequenceDiagram
    participant DB as Room DB
    participant PS as PagingSource
    participant VM as MessagingViewModel
    participant SC as MessagingScreen
    participant MB as MessageBubble

    note over VM,SC: BEFORE (causes scroll jumps)
    DB->>PS: DB write (delivery receipt)
    PS-->>VM: invalidate (auto)
    VM->>VM: _messagesRefreshTrigger++
    VM->>PS: new PagingSource created
    note over SC: All loaded pages discarded → scroll jumps

    note over VM,SC: AFTER (this PR)
    DB->>PS: DB write (delivery receipt)
    PS-->>VM: invalidate (auto, key-based)
    note over SC: Scroll position preserved

    loop Every 30 s (RESUMED lifecycle)
        SC->>SC: rememberLifecycleTickerMillis tick
        SC->>MB: timestampTick (new value)
        MB->>MB: recompose → formatTimestamp() re-runs
        note over MB: Only visible bubbles recompose
    end
Loading

Reviews (9): Last reviewed commit: "Stub mapStylePreferenceFlow in MapViewMo..." | Re-trigger Greptile

@sentry
Copy link
Copy Markdown
Contributor

sentry Bot commented Mar 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…ption

Use a Compose-level clock state (timestampTick) that triggers recomposition
of visible message items every 30s and on ON_RESUME, instead of the old
_messagesRefreshTrigger which recreated the entire PagingData pipeline.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread data/src/main/java/com/lxmf/messenger/data/db/ColumbaDatabase.kt Outdated
Keep enrichSentInterfaceOnDelivery and sentInterface UI/DB/AIDL additions
from main; drop re-added _messagesRefreshTrigger (scroll-jump cause,
replaced by Compose-level timestampTick in prior commit).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech torlando-tech force-pushed the worktree-message-scrolling branch from 6af99ad to a16dcc1 Compare March 12, 2026 03:13
Comment thread python/reticulum_wrapper.py Outdated
The original scroll-fix commit over-removed code: enrichSentInterfaceOnDelivery(),
the getNextHopInterfaceName query in handleSendSuccess(), and the merge missed
main's format_interface_name fix in reticulum_wrapper.py. Restore all of these —
they are independent of the scroll-position bug (which was caused solely by
_messagesRefreshTrigger recreating PagingData).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

The branch had stale GitHub Actions versions from before main upgraded
them. Take main's workflows verbatim to resolve version downgrades and
restore logcat timeout guards.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

Move the snapshot dependency from a bare read in the items lambda to an
explicit parameter on MessageBubble. This ensures recomposition of
formatTimestamp() even if MessageUi is stabilised in the future (the
previous approach relied on ByteArray making MessageUi unstable).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

…lling

# Conflicts:
#	app/src/main/java/network/columba/app/ui/screens/MessagingScreen.kt
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai review

torlando-tech and others added 2 commits April 22, 2026 02:15
- Remove unused mutableLongStateOf import; ticker is produceState-backed
  via rememberLifecycleTickerMillis
- Suppress UNUSED_PARAMETER for MessageBubble.timestampTick — the param
  is intentionally unread; its sole purpose is to drive recomposition so
  formatTimestamp() re-evaluates with the current wall clock

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same regression as fixed in PR #840: after merging main with the dark-mode
picker (PR #839), MapViewModel reads mapStylePreferenceFlow at construction
time but MapViewModelTest's SettingsRepository mock doesn't stub it.

Without this stub the entire MapViewModelTest suite fails with
MockKException on the shard-2 CI shard. The fix is a single-line
addition returning the production default (AUTO).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant