Repository navigation
Skip redundant selection updates in the message edit composer - #104833
Conversation
Co-authored-by: Eugene Voloshchak <eVoloshchak@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Web test steps verification results: ❌ fail I couldn't run the test, so this result is not evidence of an app bug. The tester finished onboarding and sent "Hello world test" in Concierge and in a self DM, but it couldn't open the message edit composer. The headless web session never shows the hover toolbar with Edit, and the test tool can't press Up arrow, right-click, or long-press on web. It also can't read the JS console on web. Please test the edit flow and check the console by hand.
|
|
🤖 Android test steps verification results: ✅ pass On Android, the edit composer opened with the message text. Typing, cursor moves, selection, and deleting all worked, and saving showed "(edited)". The app had no crash, freeze, or red box during the flow.
|
|
|
I updated the PR description:
I left QA Steps and Screenshots/Videos as they were, since you didn't ask me to change them. |
|
@MelvinBot, populate QA steps too (same as Tests) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20261025-200158-1792954886450.mp4Android: mWeb ChromeScreen.Recording.2026-10-25.at.19.25.12.moviOS: HybridAppScreen.Recording.2026-10-25.at.19.23.05.moviOS: mWeb SafariScreen.Recording.2026-10-25.at.19.20.51.movMacOS: Chrome / SafariScreen.Recording.2026-10-25.at.19.19.24.mov |
|
I filled in QA Steps with the same 7 steps as Tests. The rest of the PR description is unchanged. |
eVoloshchak
left a comment
There was a problem hiding this comment.
LGTM!
@MelvinBot, mark this as ready fore review
|
I marked this PR as ready for review. |
|
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.28-4 🚀
|
|
No help site update needed. This PR fixes an internal render loop that could crash the message edit composer, without changing any feature, label, or workflow that a help article describes. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.30-0 🚀
Bundle Size Analysis (Sentry): |





Explanation of Change
On web, editing a message can crash with React error #185 ("Maximum update depth exceeded"), starting from
setSelectioninReportActionItemMessageEdit.Each keystroke runs
setSelection, which writes the selection to local state and writes a new merged copy to the edit context. The effect that syncs the context back into local state then runs and callssetSelectionStatewith a different object, even thoughstartandenddid not change. React can't skip that update, so every keystroke adds an extra nested render.This PR changes that effect to:
startandendare unchanged, so React bails out of the redundant render.draft.lengthandsetSelectiondependencies.draft.lengthis already covered bydefaultSelection.There are no reliable reproduction steps, so this is the agreed low-risk fix to see whether the Sentry error stops.
Fixed Issues
$ #104748
PROPOSAL: #104748 (comment)
Tests
(edited)label.Offline tests
QA Steps
(edited)label.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari