fix: resolve public room message send failure after app restart with emoji text#90238
Conversation
|
@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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 664cbee431
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| transactionThreadReport, | ||
| hasOlderActions, | ||
| hasNewerActions, | ||
| newestFetchedReportActionID: reportPaginationState?.newestFetchedReportActionID, |
There was a problem hiding this comment.
Avoid skipping newer fetch for transaction thread report
Passing newestFetchedReportActionID here changes loadNewerChats() behavior for mixed report/transaction-thread views: in useLoadReportActions, the cursor path returns immediately after getNewerActions(reportID, newestFetchedReportActionID), so the fallback branch that also fetches transactionThreadReport.reportID never runs. In reports where reportActions combine both report-level and transaction-thread actions, newer thread messages can stop loading after initial pagination state is set, leaving the thread stale until a full reopen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@nabi-ebrahimi this seems plausible to me - can you check if this is a valid issue?
There was a problem hiding this comment.
@chuckdries I think its valid but couldn't reproduce this issue so i made these changes to make sure transactionThreadReport don't stop loading.
There is an early return here, but from my testing it doesn’t appear to cause the issue mentioned.
…local actions and preserving thread newer fetches
…ge-send-after-restart
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppvideo_2026-05-19_16-52-24.mp4Android: mWeb ChromeScreen.Recording.2026-05-19.at.16.42.06.moviOS: HybridAppScreen.Recording.2026-05-19.at.16.40.02.moviOS: mWeb SafariScreen.Recording.2026-05-19.at.16.37.56.movMacOS: Chrome / SafariScreen.Recording.2026-05-19.at.16.35.16.mov |
|
@eVoloshchak Just to note, messages can get removed as violation
|
|
@nabi-ebrahimi, this doesn't work reliably right after you refresh the page
Screen.Recording.2026-05-14.at.13.08.57.movScreen.Recording.2026-05-14.at.12.59.45.mov |
|
@eVoloshchak Thanks for reviewing. After removing The same bug reproducible on staging. I fixed it at this commit |
flaviadefaria
left a comment
There was a problem hiding this comment.
From a product standpoint, the fix looks good to me.
|
@eVoloshchak, gentle bump. thanks |
| transactionThreadReport, | ||
| hasOlderActions, | ||
| hasNewerActions, | ||
| newestFetchedReportActionID: reportPaginationState?.newestFetchedReportActionID, |
There was a problem hiding this comment.
@nabi-ebrahimi this seems plausible to me - can you check if this is a valid issue?
|
|
||
| // Keep transaction thread in sync for mixed report+thread views. | ||
| if (isTransactionThreadReport) { | ||
| getNewerActions(transactionThreadReportID, transactionThreadNewestAction?.reportActionID); |
There was a problem hiding this comment.
Just for my understanding, can you explain what this change does?
There was a problem hiding this comment.
I made this change to address codex feedback.
It ensures that newer messages in the transaction thread are also fetched.
There was a problem hiding this comment.
I see. I think ideally we'd grab newestFetchedReportActionID for the transaction thread as well and pass it here, but that seems like a lot of work and I don't think it's strictly necessary? So we'll roll with this for now
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Note for QA: since the test steps include pasting part of the URL into the console, this is only testable in desktop browsers |
|
🚧 @chuckdries 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.3.90-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no documentation changes required. I reviewed the changes in this PR against the help site articles under Conclusion: This is an internal bug fix to report-action pagination logic — it does not change any documented, user-facing feature, setting, or workflow, so no help site updates are needed and no draft docs PR was created. Why no docs changes are neededThe PR modifies pagination internals so that locally-known newer messages stay visible when the default newest page starts at an older action:
These are behind-the-scenes correctness fixes (messages display as users already expect). There is no new UI, button, tab, setting, or feature to document. A search of @nabi-ebrahimi — since no help site changes are required, no draft docs PR was created, so there's nothing further to review here. If you believe a specific article should be updated, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.90-3 🚀
|


Explanation of Change
This change makes the report action list keep any already-known newer messages visible when loading the default newest page. Previously, if the first fetched pagination page started at an older action, locally available newer actions before that page could be left out of the continuous chain, making the conversation appear to start later than the data we already had.
By widening the default chain to include those known newer actions, the UI can show the most complete current conversation state without waiting for another fetch. The report view also passes the newest fetched action ID into the loading hook so pagination decisions are based on the report’s actual fetch state. A unit test covers the case where a newer action exists before the first fetched page and verifies it stays included.
Fixed Issues
$ #89557
PROPOSAL: #89557 (comment)
Tests
Enter the following message:
g'morn, one for hourish then 🚴 to 🏦
Attempt to send the message.
Verify that the message is successfully posted in the room.
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Edited_20260511_231213.mp4
Android: mWeb Chrome
Recording_20260511_231643.mp4
iOS: Native
Screen.Recording.2026-05-11.at.11.46.11.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-05-11.at.11.51.44.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-11.at.10.26.58.PM.mov