Repository navigation
Conversation
…-actions-with-no-visible-page # Conflicts: # src/hooks/useReportActionsListModel.ts
|
@gijoe0295 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: 7b8482fb2a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
gijoe0295
left a comment
There was a problem hiding this comment.
Minor comment cleanups
| ? { | ||
| hasOnceLoadedReportActions: loadingState.hasOnceLoadedReportActions, | ||
| isLoadingInitialReportActions: loadingState.isLoadingInitialReportActions, | ||
| // Read by the backfill that recovers a chain with no visible actions to render. |
There was a problem hiding this comment.
| // Read by the backfill that recovers a chain with no visible actions to render. |
|
|
||
| useMarkOpenReportEndOnSkeleton(report, shouldShowInitialSkeleton); | ||
|
|
||
| // The list owns pagination, so it can't recover a chain that renders as empty — it isn't mounted yet. |
There was a problem hiding this comment.
| // The list owns pagination, so it can't recover a chain that renders as empty — it isn't mounted yet. |
| // The exact cursor `loadOlderChats` sends. Exposed so callers that need to tell whether a request | ||
| // made progress compare against the ID that was actually requested, not against the rendered chain | ||
| // (which can end on a transaction thread action or a synthetic one that never reaches the server). |
There was a problem hiding this comment.
| // The exact cursor `loadOlderChats` sends. Exposed so callers that need to tell whether a request | |
| // made progress compare against the ID that was actually requested, not against the rendered chain | |
| // (which can end on a transaction thread action or a synthetic one that never reaches the server). |
There was a problem hiding this comment.
Trimmed it to one line rather than dropping it. The cursor not matching the end of the rendered chain is what broke this hook while I was building it, so it is worth a note:
// The exact cursor `loadOlderChats` sends, which is not always the end of the rendered chain.| * Whether the initial OpenReport call is still in flight. Must come from the request queue | ||
| * (`useIsReportLoadPending`), not from the RAM-only loading flag: a flag that never cleared would | ||
| * block the backfill for as long as the report stays open. |
There was a problem hiding this comment.
| * Whether the initial OpenReport call is still in flight. Must come from the request queue | |
| * (`useIsReportLoadPending`), not from the RAM-only loading flag: a flag that never cleared would | |
| * block the backfill for as long as the report stays open. | |
| * Whether the initial OpenReport call is still in flight. Must come from the request queue (`useIsReportLoadPending`) |
| /** Whether the last GetOlderActions call failed */ | ||
| hasLoadingOlderReportActionsError: boolean | undefined; | ||
|
|
||
| /** The cursor `loadOlderChats` sends, i.e. the oldest action of the current report */ |
There was a problem hiding this comment.
| /** The cursor `loadOlderChats` sends, i.e. the oldest action of the current report */ | |
| /** The oldest action of the current report, i.e. the cursor `loadOlderChats` sends */ |
| const previousReportIDRef = useRef(reportID); | ||
|
|
||
| useEffect(() => { | ||
| // A different report gets its own cursor, otherwise the guard could carry over an ID this report never requested. |
There was a problem hiding this comment.
| // A different report gets its own cursor, otherwise the guard could carry over an ID this report never requested. |
| * If the report has newer actions to load. Only the newest chain is backfilled: a chain anchored on a | ||
| * linked or unread action sits in the middle of the history, so walking backwards from it could page | ||
| * through everything older while the visible actions sit on the newer side. |
There was a problem hiding this comment.
| * If the report has newer actions to load. Only the newest chain is backfilled: a chain anchored on a | |
| * linked or unread action sits in the middle of the history, so walking backwards from it could page | |
| * through everything older while the visible actions sit on the newer side. | |
| * If the report has newer actions to load |
| // A failed request consumes the cursor without advancing it, so without this one transient failure | ||
| // would strand the report on the skeleton — the symptom this hook exists to fix. Each cursor gets a | ||
| // single retry, so a request that keeps failing can't spin. |
There was a problem hiding this comment.
| // A failed request consumes the cursor without advancing it, so without this one transient failure | |
| // would strand the report on the skeleton — the symptom this hook exists to fix. Each cursor gets a | |
| // single retry, so a request that keeps failing can't spin. | |
| // A failed request consumes the cursor without advancing it. | |
| // Each cursor gets a single retry, so a request that keeps failing can't block. |
There was a problem hiding this comment.
Done, kept your wording but with "spin" instead of "block". The risk there is a repeating request loop rather than the backfill getting stuck.
| // Safety guard against an infinite request loop: if the cursor hasn't advanced since the last | ||
| // call, the server has no more actions to give us for this chain. |
There was a problem hiding this comment.
| // Safety guard against an infinite request loop: if the cursor hasn't advanced since the last | |
| // call, the server has no more actions to give us for this chain. | |
| // If the cursor hasn't advanced since the last call, the server has no more actions for this chain. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-07.at.01.27.55.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-08-07.at.00.36.08.moviOS: mWeb SafariScreen.Recording.2026-08-07.at.00.37.23.movMacOS: Chrome / SafariScreen.Recording.2026-08-06.at.23.52.08.mov |
@BartekObudzinski I'm not crystal-clear on this point. Can't we create several expenses then delete them? The number of deleted expenses should be large enough to cover a page? I think we can try to come up with manual test steps for this. |
|
Verified the fix itself: with the stored page pointed at only invisible actions, The expense route does not reproduce though. I tried 60+ messages deleted, then a fresh sign in, and got no skeleton. The reporter was deleting tracked expenses rather than messages, and the backend picks the OpenReport window, so we cannot force it to land entirely on invisible actions. |
|
@gijoe0295 can be rerevied |
|
@gijoe0295 How is the review going? |
|
Testing. Should be done soon |
|
GH actions are not working now |
…-actions-with-no-visible-page
mountiny
left a comment
There was a problem hiding this comment.
I think the comment is not strictly correct there, can you please update it in a follow up? I dont want to hold this PR on the comment so we can fix real bug users can hit sooner than later
| }; | ||
|
|
||
| /** | ||
| * Loads older chats in a report whose newest page contains no visible actions. |
There was a problem hiding this comment.
| * Loads older chats in a report whose newest page contains no visible actions. | |
| * Loads older report actions in a report whose newest page contains no visible actions. |
| /** | ||
| * Loads older chats in a report whose newest page contains no visible actions. | ||
| * | ||
| * The backend prioritizes IOU actions in OpenReport, so a report can come back with a page made up |
There was a problem hiding this comment.
I don think it prioritizes the IOU report actions, the actions are all sent in the order in which they had been created so in this case its latest 50 report actions are invisible, if you would send a comment that would be fixed I think
| * The backend prioritizes IOU actions in OpenReport, so a report can come back with a page made up | |
| * A report can come back with a page made up |
|
✋ 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.4.52-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required. ✅ This PR is an internal bug fix — it adds Why no docs update is needed
Since no documentation change is warranted, no draft help site PR was created — there is nothing to review on the docs side. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The list renders the chain of report actions covered by the stored pagination page. If every action on that page is invisible, deleted expenses for example, the chain is empty and
ReportActionsSkeletonGuardshows a skeleton instead of the list. That state cannot recover, because the list owns pagination and the list is what the skeleton replaced.Reported for a self DM whose imported card expenses had all been deleted. The Onyx export shows one page covering 31 deleted expense actions, with 49 real messages cached but sitting outside it.
This adds
useBackfillWhenNoVisibleActions, called from the skeleton guard. When the chain has no visible actions and older pages are still fetchable, it callsloadOlderChatsand walks back until visible actions appear.MoneyRequestReportActionsListalready does this for money request reports, which hit the same backend behaviour. Chats had no equivalent.The loop is guarded. The cursor is the exact action ID
loadOlderChatssends. Each cursor is requested once. A failed request re-arms that cursor once, so one transient failure does not strand the report. Offline and in flight requests block it.If the walk reaches the start of history with nothing visible, the skeleton stays. That needs a defined terminal state and is tracked separately.
Fixed Issues
$ #97574
PROPOSAL:
Tests
The stored page has to cover only invisible actions. The backend produces that on its own but not on demand, so set it up directly.
main, verify the report shows a skeleton and stays on it.Offline tests
QA Steps
The broken state comes from the backend returning a page made up entirely of invisible actions. It cannot be triggered on demand, only simulated locally, so there are no staging steps for it. Regression check instead:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Screen.Recording.2026-08-04.at.09.41.35.mov