Fix out-of-bounds crash when revealing a navigated Reader comment#25609
Open
crazytonyli wants to merge 5 commits into
Open
Fix out-of-bounds crash when revealing a navigated Reader comment#25609crazytonyli wants to merge 5 commits into
crazytonyli wants to merge 5 commits into
Conversation
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 32407 | |
| Version | PR #25609 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | e64d887 | |
| Installation URL | 1du9neslk8t3o |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 32407 | |
| Version | PR #25609 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | e64d887 | |
| Installation URL | 1mdg18tbh3dbo |
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
The reveal animation captured the target comment's index path and reused it across an async gap (up to ~1.8s). If a background comments sync purged rows in that window, the deferred scrollToRow referenced a now out-of-bounds row and crashed with an NSRangeException. Resolve the comment's current index path by ID at scroll time instead, via a shared revealComment helper that bounds-checks before scrolling, and route scrollToComment through it.
Navigating to a specific comment (e.g. from a notification) could leave the list scrolled to the top instead of the target comment, intermittently. The page-1 comments sync purges every cached comment beyond the first page, so a later-page target reached from a warm cache was deleted right after the reveal scrolled to it, collapsing the list to the top. The reveal now waits for that destructive initial sync to finish (re-paging to reload the target if it was purged) before scrolling the comment into view, which makes the existing scroll reliable.
The reveal no longer relies on a per-cell render callback to re-scroll, so the method is dead. Drop it along with its only call site in the cell configuration.
crazytonyli
force-pushed
the
bugfix/cmm-2091-proper-fix
branch
from
c7462fa to
e64d887
Compare
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
2 participants
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.
Note
The first commit formats the code. The second commit is the real change.
Description
Fixes https://linear.app/a8c/issue/CMM-2091.
#25608 fixes the same issue, but with a much safer approach, because it's changing the release branch. If this PR is merged, I'll discard the #25608 changes when merging the release branch into the trunk branch.
Testing instructions
In addition to verify the Linear issue is fixed, we also need to test the "reveal comment" code continues to work. You can follow the same steps in the Linear issue, except you need to add 100 comments to Author's post, instead of just 5.