fix: preserve split child transactions when report is deleted #79192
+156
−13
Conversation
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
melvin-bot
bot
requested review from
a team,
JmillsExpensify and
ikevin127
and removed request for
a team
|
@ikevin127 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] |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
MobileMage
force-pushed
the
fix/78933-split-status-removed-after-edit
branch
from
e8b7e6f to
44898ed
Compare
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
MobileMage
force-pushed
the
fix/78933-split-status-removed-after-edit
branch
from
44898ed to
d91dccc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b7e6f489
ℹ️ 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".
src/libs/TransactionUtils/index.ts
Outdated
Comment on lines
2424
to
2429
| const isSplitChild = currentTransaction?.comment?.originalTransactionID === originalTransactionID; | ||
| // For split children, include them even if their report was deleted. | ||
| // The split relationship is defined by originalTransactionID, not the report. | ||
| return ( | ||
| currentTransaction?.comment?.originalTransactionID === originalTransactionID && | ||
| !!currentReport && | ||
| isSplitChild && | ||
| (!!currentReport || currentTransaction?.comment?.source === CONST.IOU.TYPE.SPLIT) && |
There was a problem hiding this comment.
Because this now includes split children even when their report is gone, getChildTransactions can return transactions whose report (and report actions) were cleared by deleteReport. In updateSplitTransactions, those transactions can fall into undeletedTransactions, and getDeleteTrackExpenseInformation immediately dereferences reportAction.reportActionID/childReportID; if the report actions were nulled, currentReportAction is undefined and this path will throw when a user removes that split from the edit list. Consider filtering out split children whose report/actions are missing or guarding currentReportAction before attempting a delete.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
MobileMage
force-pushed
the
fix/78933-split-status-removed-after-edit
branch
3 times, most recently
from
1c17eb8 to
f5449f7
Compare
ikevin127
reviewed
Jan 9, 2026
src/libs/TransactionUtils/index.ts
Outdated
| // Include split children even if their report was deleted, but exclude explicitly orphaned | ||
| // transactions (reportID '0'). The split relationship is defined by originalTransactionID. | ||
| const hasValidReportID = !!currentTransaction?.reportID && currentTransaction.reportID !== '0'; | ||
| return ( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ikevin127
reviewed
Jan 9, 2026
src/libs/TransactionUtils/index.ts
Outdated
| const isSplitChild = currentTransaction?.comment?.originalTransactionID === originalTransactionID; | ||
| // Include split children even if their report was deleted, but exclude explicitly orphaned | ||
| // transactions (reportID '0'). The split relationship is defined by originalTransactionID. | ||
| const hasValidReportID = !!currentTransaction?.reportID && currentTransaction.reportID !== '0'; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.mov |
Contributor
|
🟡 NAB - Missing Test Case (Minor) Consider adding a test for transactions where source is NOT it('should exclude non-split transactions when their report was deleted', () => {
const childTransaction = generateTransaction({
transactionID: 'child-non-split',
reportID: 'deleted-report-id',
comment: {
originalTransactionID,
source: undefined, // Not a split
},
});
const transactions = {
[`${ONYXKEYS.COLLECTION.TRANSACTION}child-non-split`]: childTransaction,
};
const result = TransactionUtils.getChildTransactions(transactions, reportCollectionDataSet, originalTransactionID);
expect(result).toHaveLength(0);
});Why: This ensures the fallback behavior for non-split child transactions (requiring |
This comment was marked as outdated.
This comment was marked as outdated.
MobileMage
force-pushed
the
fix/78933-split-status-removed-after-edit
branch
3 times, most recently
from
b44e152 to
f56dd4f
Compare
When a report containing a split expense child was deleted, the getChildTransactions function excluded the child transaction because it required the report to exist. This caused incorrect isReverseSplitOperation determination, which reverted the split and cleared split metadata. The fix adds an optional includeOrphaned parameter to getChildTransactions: - Default (false): excludes orphaned transactions (reportID '0') for processing - When true: includes all children for accurate counting In updateSplitTransactions, we compare counts to detect orphaned children and prevent incorrect split reversal. Fixed Issues: Expensify#78933
MobileMage
force-pushed
the
fix/78933-split-status-removed-after-edit
branch
from
f56dd4f to
a529d10
Compare
Contributor
|
@MobileMage Thanks for the updates - the fix works now, reviewing the code once again since there's been significant changes 🔄 Tip As a tip please try to not force-push anymore as it interferes with branch history and makes things difficult for reviewer and also in case a revert is required (here's a paragraph from the guidelines where this is mentioned:
It's okay if you have multiple commits - that's better than force-pushing since it doesn't mess with the PR commit history 👍 |
ikevin127
reviewed
Jan 11, 2026
src/libs/actions/IOU/index.ts
Outdated
| const hasEditableSplitExpensesLeft = splitExpenses.some((expense) => (expense.statusNum ?? 0) < CONST.REPORT.STATUS_NUM.SUBMITTED); | ||
| const isReverseSplitOperation = splitExpenses.length === 1 && originalChildTransactions.length > 0 && hasEditableSplitExpensesLeft; | ||
| // Don't revert split if there are orphaned children (reportID '0') - they're still part of the split | ||
| const allChildTransactions = getChildTransactions(allTransactionsList, allReportsList, originalTransactionID, true); |
Contributor
There was a problem hiding this comment.
A couple lines above (line 11956) we already call getChildTransactions without the includeOrphaned param.
Issue: This iterates over all transactions twice - for large transaction lists, this will impact performance.
Suggested Fix:
// Get all children once (including orphaned), then filter for non-orphaned
const allChildTransactions = getChildTransactions(allTransactionsList, allReportsList, originalTransactionID, true);
const originalChildTransactions = allChildTransactions.filter(
(tx) => tx?.reportID !== CONST.REPORT.UNREPORTED_REPORT_ID
);
const isCreationOfSplits = originalChildTransactions.length === 0;
const hasEditableSplitExpensesLeft = splitExpenses.some((expense) => (expense.statusNum ?? 0) < CONST.REPORT.STATUS_NUM.SUBMITTED);
// Don't revert split if there are orphaned children (reportID '0') - they're still part of the split
const isReverseSplitOperation =
splitExpenses.length === 1 && originalChildTransactions.length > 0 && hasEditableSplitExpensesLeft && allChildTransactions.length === originalChildTransactions.length;Bug if Not Addressed: In workspaces with thousands of transactions, users may experience noticeable lag when editing split expenses due to redundant iteration.
Contributor
|
✅ Completed PR Reviewer Checklist. Awaiting issues to be addressed by PR author before moving forward to approving.
|
Contributor
Author
|
Thanks for the review @ikevin127! Re: Force-pushing - Noted, I'll avoid force-pushing during reviews going forward. Thanks for the tip! Re: Performance issue - Good catch! Fixed in the latest commit ( const allChildTransactions = getChildTransactions(..., true);
const originalChildTransactions = allChildTransactions.filter(
(tx) => tx?.reportID !== CONST.REPORT.UNREPORTED_REPORT_ID
);Re: Recordings - I'll get the platform recordings done today. |
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.
Explanation of Change
When a report containing a split expense child was deleted, the
getChildTransactionsfunction excluded the child transaction because it required the report to exist (!!currentReport). This caused incorrectisReverseSplitOperationdetermination, which reverted the split and cleared split metadata.The fix includes split children (identified by
comment.source === CONST.IOU.TYPE.SPLIT) even if their report was deleted, since the split relationship is defined byoriginalTransactionID, not the report.Fixed Issues
$ #78933
PROPOSAL: #78933 (comment)
Tests
Offline tests
N/A - This fix affects transaction querying logic that doesn't have offline-specific behavior.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari