Fix InboxListView NPE when switching tabs#39
Merged
Conversation
openVisibleMessages was launched on Dispatchers.IO and called LinearLayoutManager.findFirst/LastCompletelyVisibleItemPosition() from a worker thread. Those APIs walk RecyclerView children and read View.getLayoutParams(), which is not thread-safe and races with the layout pass. Switching tabs while the list was settling reliably crashed with a NullPointerException inside ViewBoundsCheck.findOneViewWithinBoundFlags. Resolve the visible index range and snapshot the message list on the main thread, then move only the markAsOpened() network calls to Dispatchers.IO via withContext. Bound the snapshot subList defensively against stale indexes. Co-authored-by: Cursor <cursoragent@cursor.com>
The .toList() snapshot was added defensively, but adapter mutations (setInbox / addPage / addMessage / removeMessage) all run on the main thread, and openVisibleMessages now does too. The defensive copy was adding O(totalMessages) main-thread work on every scroll event for no benefit. Iterable.filter already returns a fresh List, so the IO block can still iterate messagesToOpen safely if the underlying adapter list mutates after we hand it off. Co-authored-by: Cursor <cursoragent@cursor.com>
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
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.
Summary
openVisibleMessageswas launched onDispatchers.IOand calledLinearLayoutManager.findFirstCompletelyVisibleItemPosition()/findLastCompletelyVisibleItemPosition()from a worker thread. Those APIs walkRecyclerViewchildren and readView.getLayoutParams(), which is not thread-safe and races with the layout pass. Switching tabs while the list was still settling reliably reproduced a fatalNullPointerExceptioninsideViewBoundsCheck.findOneViewWithinBoundFlags:Fix
Dispatchers.MainsoLayoutManagerand adapter reads happen on the UI thread.subList(...).filter { !it.isOpened }on the main thread.Iterable.filterreturns a fresh list, so it's safe to hand off to IO even if the adapter mutates afterwards.coerceAtLeast(0)/coerceAtMost(messages.size)against stale indexes.message.markAsOpened()) toDispatchers.IOviawithContext.Performance
The fix is essentially free on the main thread:
findFirst/LastCompletelyVisibleItemPosition()walk visible children only (~10–30 views), microseconds.subList(...).filter { !it.isOpened }iterates only the visible slice.A
.toList()snapshot was tried in the first commit but dropped in the follow-up: it added O(totalMessages) main-thread work on every scroll event for no benefit, since adapter mutations are also confined to the main thread now.Pre-existing follow-up (not in this PR)
recyclerView.setOnScrollChangeListenerfires on every pixel of scroll, so a flick can launch dozens ofopenVisibleMessagescoroutines per second. They mostly no-op via the early-exit, but the launches themselves aren't free. Worth throttling/debouncing or migrating toRecyclerView.OnScrollListenerwithonScrollStateChangedin a separate change.Test plan
setInbox,addPage,addMessage,removeMessage, and the scroll listener still call intoopenVisibleMessagescleanly (no thread-confinement asserts).