Handle the Distance and Rate fields for splits#79099
Handle the Distance and Rate fields for splits#79099lakchote merged 49 commits intoExpensify:mainfrom
Conversation
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.
|
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| shouldShowRightIcon | ||
| titleStyle={styles.flex1} | ||
| style={[styles.moneyRequestMenuItem]} | ||
| onPress={() => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Inline arrow function creates a new reference on every render, breaking memoization of the optimized MenuItemWithTopDescription component.
Use useCallback to memoize the onPress handler:
const handleDistancePress = useCallback(() => {
if (isOdometerDistance) {
Navigation.navigate(
ROUTES.MONEY_REQUEST_STEP_DISTANCE_ODOMETER.getRoute(
CONST.IOU.ACTION.EDIT,
CONST.IOU.TYPE.SPLIT_EXPENSE,
CONST.IOU.OPTIMISTIC_TRANSACTION_ID,
reportID,
Navigation.getActiveRoute(),
),
);
return;
}
// ... rest of the logic
}, [isOdometerDistance, isManualDistance, reportID]);
| titleStyle={styles.flex1} | ||
| style={[styles.moneyRequestMenuItem]} | ||
| onPress={() => { | ||
| Navigation.navigate( |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Inline arrow function creates a new reference on every render, breaking memoization of the optimized MenuItemWithTopDescription component.
Use useCallback to memoize the onPress handler:
const handleRatePress = useCallback(() => {
Navigation.navigate(
ROUTES.MONEY_REQUEST_STEP_DISTANCE_RATE.getRoute(
CONST.IOU.ACTION.EDIT,
CONST.IOU.TYPE.SPLIT_EXPENSE,
CONST.IOU.OPTIMISTIC_TRANSACTION_ID,
reportID,
Navigation.getActiveRoute(),
),
);
}, [reportID]);![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: 63bfa66d0a
ℹ️ 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".
| setOptimisticWaypoints(newWaypoints); | ||
| Promise.all([ | ||
| removeWaypoint(transaction, emptyWaypointIndex.toString(), shouldUseTransactionDraft(action)), | ||
| removeWaypoint(currentTransaction, emptyWaypointIndex.toString(), shouldUseTransactionDraft(action)), | ||
| updateWaypointsUtil(transactionID, newWaypoints, shouldUseTransactionDraft(action)), |
There was a problem hiding this comment.
When editing a split distance expense (iouType SPLIT/SPLIT_EXPENSE), this path still calls removeWaypoint/updateWaypointsUtil with only shouldUseTransactionDraft(action). For split edits that evaluates to false, so these helpers write to the main TRANSACTION (or TRANSACTION_DRAFT) store instead of SPLIT_TRANSACTION_DRAFT. As a result, dragging/reordering waypoints (or removing an empty waypoint after dragging) updates the wrong Onyx key and the split draft shown in SplitExpenseEditPage won’t reflect the change, so the edit is lost when saving the split. Consider passing the split draft into removeWaypoint and adding split-draft support to updateWaypointsUtil for this flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
cc @ZhenjaHorbach what do you think of codex's comment here?
There was a problem hiding this comment.
Makes sense
Fixed!
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good from a product perspective 👍
|
I’ll take a look at this PR later today. @ZhenjaHorbach, there's still a conflict and failing unit tests. |
I'm on it. |
|
Backend PR is up for review, it fixes the issue. |
|
@ZhenjaHorbach @suneox it's now fixed you can resume the PR/review, thanks! |
|
@suneox can you review it please? |
| setOptimisticWaypoints(newWaypoints); | ||
| Promise.all([ | ||
| removeWaypoint(transaction, emptyWaypointIndex.toString(), shouldUseTransactionDraft(action)), | ||
| removeWaypoint(currentTransaction, emptyWaypointIndex.toString(), shouldUseTransactionDraft(action)), | ||
| updateWaypointsUtil(transactionID, newWaypoints, shouldUseTransactionDraft(action)), |
There was a problem hiding this comment.
cc @ZhenjaHorbach what do you think of codex's comment here?
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 576c1bf0b4
ℹ️ 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".
| } else if (splitExpenseDraftTransaction?.comment?.customUnit?.quantity) { | ||
| quantity = splitExpenseDraftTransaction?.comment?.customUnit?.quantity; |
There was a problem hiding this comment.
Treating customUnit.quantity as a truthy check drops valid 0 values, so when a split distance is edited to zero the code skips the distance recalculation path and leaves updatedItem.amount at the old base splitExpenseDraftTransaction.amount. In the split-expense edit flow this means saving a zero-distance split can silently keep the previous non-zero amount, so the saved split no longer matches what the user entered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think 0 is not a valid case for distance
There was a problem hiding this comment.
It doesn't make sense to have a 0 distance split, in my opinion.
|
🚧 @lakchote 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/lakchote in version: 9.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
6 participants
Explanation of Change
Fixed Issues
$ #78891
PROPOSAL:
Tests
Preconditions: User is an Expensifail account and admin have a Workspace
Offline tests
Preconditions: User is an Expensifail account and admin have a Workspace
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Preconditions: User is an Expensifail account and admin have a Workspace
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
2026-01-13.13.21.34.mov