Skip to content

Roll back optimistic transaction thread when OpenReport fails - #102056

Closed
MelvinBot wants to merge 4 commits into
mainfrom
claude-orphanedTransactionThreadFixBadge
Closed

MelvinBot wants to merge 4 commits into
mainfrom
claude-orphanedTransactionThreadFixBadge

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

Explanation of Change

When a user opens an expense while offline and the app has no transaction thread for it yet, ReportFetchHandler calls createTransactionThreadReport() right away instead of waiting for the report to load. That queues an OpenReport for a thread that only exists optimistically. If the expense is deleted before the queue drains, that OpenReport fails.

On failure openReport kept the report it had just created and merged errorFields.createChat onto it. SidebarUtils force-displays any report that carries errors, before the checks that would normally hide a transaction thread. So the dead thread showed up in the LHN as its own row: fallback avatar, "No activity yet", and a red Fix badge with nothing behind it to fix. The thread is an internal artifact — the user never asked to create a chat, and there is no retry affordance on the row — so the error was not actionable and could not be dismissed.

This makes failureData the inverse of optimisticData instead of decorating what it created:

  • New shouldRemoveOptimisticReportOnFailure param on openReport. When set and the call created the report, failureData sets report_<id>, reportActions_<id> and reportMetadata_<id> to null rather than merging a createChat / createChatThread error onto them. failureData is applied after the API response's own onyxData (OnyxUpdates.ts:74-99), so this also clears the errorFields.createChat the server writes on its 666 response.
  • The parent action's childReportID is cleared too, so it no longer points at a report that does not exist.
  • createTransactionThreadReport opts in. User-initiated creates (DM, group chat, workspace room) do not, and keep showing their retryable error exactly as before — covered by a new regression test.

One related fix in the same area: the guard added by Fix RBR on DM after paying deleted expense offline calls isDeletedAction(parentReportAction), and isDeletedAction(undefined) returns true. It was therefore suppressing parent-chat error propagation for any child whose parent action simply had not loaded yet. It now follows the codebase convention of checking the action exists first.

Notes for the reviewer

  • This is the write-side cause of [$250] IOU-Chat with Fix badge and an error appears in LHN after paid offline expense was deleted as well — the same BrowserStack TC. That issue's fix only guarded the child→parent propagation, which moved the badge off the DM row and onto a broken row of its own.
  • Behaviour change worth a look: if a user is viewing a transaction thread when its OpenReport fails, they now get the not-found page instead of "Unexpected error creating this chat. Please try again later." That matches what already happens when another user deletes the expense you are looking at, but it is a visible difference.
  • This does not address the second half of the product feedback on the issue — telling the payer that their offline payment failed because the expense was deleted. That needs copy sign-off and a decision on placement, so it is deliberately out of scope here.

Fixed Issues

$ #100676
PROPOSAL: #100676 (comment)

Tests

// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate the change works.

  • Verify that no errors appear in the JS console

Offline tests

// TODO: The human co-author must fill out the offline behavior they verified. The reported repro is itself an offline flow (User A pays while offline, User B deletes the expense, User A reconnects).

QA Steps

// TODO: The human co-author must fill out the QA steps before marking this PR as "ready for review".
// Please describe what QA needs to do to validate the change and which areas they need to test for regressions.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

AI Tests

Run locally on Expensify/App per the test-selection matrix:

  • npm run typecheck — passed
  • npm run lint on the changed files — passed
  • npm run lint-changed — passed
  • npm run spell-changed — passed
  • npm test -- tests/actions/ReportTest.ts — 309 passed, including the two new cases
  • Targeted suites for the touched code paths — all passed: SidebarUtilsTest, SidebarTest, SidebarFilterTest, ReportAttributesUtilsTest, ReportAttributesSelectorTest, OnyxDerivedTest, OnyxDerived/*, ReplaceOptimisticReportWithActualReportTest, ReportFetchHandlerTest, DeepLinkHandlerTest, ReportUtilsTest, MergeTransactionTest, AddExistingExpenseTest, IOUTest/DeleteMoneyRequestTest, IOUTest/TrackExpenseTest, IOUTest/DuplicateTest, MoneyRequestReportTransactionsNavigationTest, MoneyRequestReportPreview, TransactionThreadNavigationUtilsTest, MoneyReportTransactionThreadContext, ReportNotFoundGuardTest, ClearReportActionErrorsTest, DeleteReportTest
  • The new createTransactionThreadReport test was confirmed to fail without the fix (the report survives with errorFields.createChat), so it genuinely covers the regression

Could not run: the full npm test suite (exceeded the run time limit — CI will cover it), npm run storybook -- --smoke-test --ci (no component or story changes), and browser verification of the repro (no dev SSL cert available in this environment).

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

An offline OpenReport that optimistically creates a transaction thread kept
the report on failure and merged errorFields.createChat onto it. SidebarUtils
force-displays any report carrying errors, so the orphan surfaced in the LHN
as an empty row with a Fix badge the user could not dismiss.

Add shouldRemoveOptimisticReportOnFailure to openReport so app-created reports
undo the create instead of decorating it, and set it for
createTransactionThreadReport. User-initiated creates keep their retryable
error.

Also require the parent action to exist before treating it as deleted in the
reportAttributes propagation guard, since isDeletedAction(undefined) is true.

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team September 23, 2026 12:13
@melvin-bot melvin-bot Bot added Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web labels Sep 23, 2026
@MelvinBot

Copy link
Copy Markdown
Contributor Author

⚠️ A setup step failed before I could start. This is usually a temporary issue — try again in a few minutes, and if it persists post in #melvinbot-ai.

Run details

@codecov

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

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.

Files with missing lines Coverage Δ
...bs/actions/OnyxDerived/configs/reportAttributes.ts 96.40% <100.00%> (ø)
src/libs/actions/Report/index.ts 72.45% <90.90%> (-0.41%) ⬇️
... and 11 files with indirect coverage changes

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Web test steps verification: ❌ not run — no browser and no documented steps

I could not run this on Web. Two separate blockers:

  1. Browser control was unavailable for this run. Web prep failed (no dev SSL cert / no web backend), so NewDot could not be driven in a browser. No screenshots or recordings were produced — I am not going to claim visual proof I don't have.
  2. This PR has no documented testing steps to run. The ### Tests, ### Offline tests, and ### QA Steps sections are still // TODO: The human co-author must fill out... placeholders. Even with a working browser there is nothing written down to execute.

The repro that would need to be run is the one from IOU-Chat with Fix badge and an error appears in LHN after paid offline expense was deleted: it needs two accounts on two devices, User A offline paying an expense, User B deleting it, then User A reconnecting. That is a multi-device offline flow — worth noting it is not something a single-browser automated pass verifies cleanly even when the browser works.

What I did verify: the unit tests, executed locally on the PR head (d60fe8d), clean tree.

Run Result
tests/actions/ReportTest.ts -t "OpenReport fails" (the 2 new cases) ✅ Tests: 307 skipped, 2 passed, 309 total
tests/actions/ReportTest.ts (full file) ✅ Tests: 309 passed, 309 total
tests/unit/SidebarUtilsTest.ts + tests/unit/ReportFetchHandlerTest.tsx ✅ Tests: 118 passed, 118 total

The two new cases cover both halves of the change:

  • createTransactionThreadReport > should remove the optimistic thread instead of keeping a createChat error when OpenReport fails — asserts report_, reportActions_ and reportMetadata_ are all gone after the failure, and that the parent action's childReportID is cleared. This is the LHN row from the issue.
  • openReport > should keep a user-initiated new chat with a createChat error when OpenReport fails — the regression guard: a DM the user asked for still keeps its retryable errorFields.createChat.

Static read of the diff matches: the rollback is gated on Report/index.ts:1730 (isCreatingNewReport && shouldRemoveOptimisticReportOnFailure), so it can only ever null out a report this very call created, and only createTransactionThreadReport opts in.

What still needs a human on Web: the two-device offline repro from the linked issue, confirming (a) no orphaned row with a Fix badge appears in User A's LHN after reconnecting, and (b) the noted behaviour change — a user sitting on the transaction thread when OpenReport fails now sees the not-found page instead of the "Unexpected error creating this chat" message. The testing steps need to be written into the PR body first.

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Fixed and pushed — Oxfmt check now passes.

Cause: the new isEmptyObject import in src/libs/actions/OnyxDerived/configs/reportAttributes.ts:42 was added as its own trailing group instead of inside the existing @src/ group, so oxfmt --write . produced a diff.

Fix: 805dd6d moves it to sit after the @src/types/onyx import — the formatter's own output, no logic change.

 import type {PersonalDetails, PersonalDetailsList, Policy, Report, ...} from '@src/types/onyx';
+import {isEmptyObject} from '@src/types/utils/EmptyObject';
 
 import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
 
 import {isTrackIntentUserSelector} from '@selectors/Onboarding';
-
-import {isEmptyObject} from '@src/types/utils/EmptyObject';

The only other red check is Check independent approval, which needs a human reviewer and is not something I can resolve.

Verification
  • npm run fmt followed by git diff --name-only --exit-code (the exact CI step) — clean.
  • npm run typecheck — passed.
  • npx eslint on the changed file — 2 pre-existing no-direct-personal-details-list errors at lines 227 and 298, both untouched by this PR and grandfathered by the seatbelt ratchet. No new violations.
  • npm test on OnyxDerivedTest, SidebarUtilsTest, ReportAttributesUtilsTest — 143 passed.
  • CI after the push: Oxfmt check passes.

npm run lint-changed could not run in this environment — the checkout is shallow, so git merge-base against origin/main fails. I ran ESLint directly on the changed file instead.

I could not verify anything in a browser this run — no dev SSL cert was available — but this change is a formatter-only import reorder with no runtime effect.


view run

@mkhutornyi

Copy link
Copy Markdown
Contributor

@MelvinBot pull main

…ctionThreadFixBadge

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Merged the latest main into this branch and pushed it in fe93b71. The merge had no conflicts.


view run

…om action params

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Fixed the failing typecheck check in 16bead0.

The two new tests passed betas: undefined to createTransactionThreadReport and openReport. main has since removed betas from both param types, so after the merge from main, TypeScript rejected the extra property. I removed that line from both calls in tests/actions/ReportTest.ts.

npm run typecheck now passes locally, and both affected tests pass.

The Check independent approval failure is expected on a PR without approval, so I left it alone.


view run

@mkhutornyi

Copy link
Copy Markdown
Contributor

@MelvinBot close this PR

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 I couldn't close this PR. The command to close it needs approval in my environment, and that approval wasn't granted.

You're an assignee, so you can close it yourself with the Close pull request button below.


view run

@mkhutornyi

Copy link
Copy Markdown
Contributor

@MelvinBot close PR like you did in other PRs

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 I still can't close this PR. The close command is blocked in the environment this request runs in, and I can't approve it from here. Other PRs may have been closed from a different setup that allows it.

The PR is still open. As an assignee, you can close it with the Close pull request button below.


view run

@mkhutornyi

Copy link
Copy Markdown
Contributor

@MelvinBot close PR like you did in #101153 (comment)

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 I couldn't close this PR. This run doesn't have permission to run gh pr close, so the PR is still open.

You're the assignee, so you can close it yourself. Or retry @MelvinBot close this PR once the permission is restored.

Once it's closed, the branch claude-orphanedTransactionThreadFixBadge stays in place, so the PR can be reopened. IOU-Chat with Fix badge and an error appears in LHN after paid offline expense was deleted is still open and isn't linked for auto-close, so the work stays tracked there.


view run

@mkhutornyi mkhutornyi closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants