Add support to splits for negtive values#81905
Conversation
melvin-bot
bot
requested review from
dukenv0307 and
trjExpensify
and removed request for
a team
|
@dukenv0307 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] |
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
…to negative-values-for-splits
…callstack-internal/Expensify-App into negative-values-for-splits
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const transactionDetails: Partial<TransactionDetails> = getTransactionDetails(transaction, undefined, currentPolicy) ?? {}; | ||
| const transactionDetailsAmount = transactionDetails?.amount ?? 0; | ||
| const transactionDetailsAmount = useMemo(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The transactionDetailsAmount and splitExpenses values are derived from props/state and should be computed directly in the component body rather than wrapped in useMemo. These are simple property accesses and transformations that don't need memoization - the computation cost is negligible.
Using useMemo here adds unnecessary complexity without performance benefit. The dependencies transactionDetails.amount and draftTransaction?.comment?.splitExpenses will cause re-computation on every change anyway, so you're not preventing any renders.
Suggested fix:
const transactionDetailsAmount = typeof transactionDetails?.amount === 'number' ? transactionDetails.amount : 0;
const splitExpenses = draftTransaction?.comment?.splitExpenses ?? [];This keeps the logic simple and direct while maintaining the same behavior.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| useEffect(() => { | ||
| setErrorMessage(''); | ||
| }, [sumOfSplitExpenses, splitExpenses]); | ||
| const draftTransactionError = useMemo(() => getLatestErrorMessage(draftTransaction ?? {}), [draftTransaction]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The draftTransactionError value is derived directly from draftTransaction and should be computed in the component body without useMemo. This is a simple function call that doesn't benefit from memoization.
The original code with useEffect was indeed problematic (violates PERF-6), but the fix should be a direct computation, not useMemo:
Suggested fix:
const draftTransactionError = getLatestErrorMessage(draftTransaction ?? {});This keeps the code simple and correctly derives state from props without unnecessary hooks.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| @@ -134,32 +138,45 @@ function calculateSplitPercentagesFromAmounts(amountsInCents: number[], totalInC | |||
| return amountsInCents.map(() => 0); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The magic number 10 appears multiple times in decimal precision calculations without clear documentation. While the comment mentions "one decimal place (0.1 precision)", the relationship between the number 10 and this precision is not immediately obvious.
Suggested fix:
Define a named constant to clarify the relationship:
const DECIMAL_PRECISION_MULTIPLIER = 10; // For 0.1 precision (one decimal place)
const roundToOneDecimal = (value: number): number => Math.round(value * DECIMAL_PRECISION_MULTIPLIER) / DECIMAL_PRECISION_MULTIPLIER;
const floorToOneDecimal = (value: number): number => Math.floor(value * DECIMAL_PRECISION_MULTIPLIER) / DECIMAL_PRECISION_MULTIPLIER;This makes the code more maintainable if precision requirements change.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // See https://github.com/Expensify/App/issues/51868 for more information | ||
| autoCapitalize="words" | ||
| inputMode={CONST.INPUT_MODE.DECIMAL} | ||
| inputMode={!props.keyboardType ? CONST.INPUT_MODE.DECIMAL : undefined} |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The inputMode conditional logic \!props.keyboardType ? CONST.INPUT_MODE.DECIMAL : undefined is duplicated across multiple components (AmountTextInput, NumberWithSymbolForm, and indirectly in BaseTextInput as isKeyboardType).
This pattern appears in at least 3 different files with the same logic. If this behavior needs to change, it would require updating multiple locations.
Suggested fix:
Extract this into a reusable utility function:
function getInputMode(keyboardType?: KeyboardTypeOptions, defaultInputMode = CONST.INPUT_MODE.DECIMAL): InputModeOptions | undefined {
return keyboardType ? undefined : defaultInputMode;
}
// Usage:
inputMode={getInputMode(props.keyboardType)}This ensures consistency and makes the relationship between keyboardType and inputMode explicit in one place.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // Height fix is needed only for Text single line inputs | ||
| const shouldApplyHeight = !shouldUseFullInputHeight && !isMultiline && !isMarkdownEnabled; | ||
| const accessibilityLabel = [label, hint].filter(Boolean).join(', '); | ||
| const isKeyboardType = props.keyboardType ? undefined : props.inputMode; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The variable name isKeyboardType is misleading - it suggests a boolean check but actually computes the inputMode value. This makes the code harder to understand.
Suggested fix:
Rename to clearly indicate what the variable contains:
const effectiveInputMode = props.keyboardType ? undefined : props.inputMode;
// Usage:
inputMode={!disableKeyboard ? effectiveInputMode : CONST.INPUT_MODE.NONE}This makes the intent clear: when a keyboard type is explicitly provided, we don't set an input mode (leaving it undefined).
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-01-27.10.04.26.movAndroid: mWeb Chrome2026-01-27.10.04.26.moviOS: HybridApp2026-01-27.09.51.28.moviOS: mWeb Safari2026-01-27.09.55.16.movMacOS: Chrome / Safari2026-01-27.09.37.13.movDetails |
|
Will check the failing checks |
…to negative-values-for-splits
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.
|
|
Strange 2026-02-10.12.59.24.mov |
|
this seems to be a bug on main for me as I just check and same is happening there |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Yep, makes sense this is allowed for parity. 👍
|
@kubabutkiewicz |
…to negative-values-for-splits
|
@ZhenjaHorbach seems to be good now |
|
LGTM! |
|
Okay @kubabutkiewicz |
|
yes will check it |
…to negative-values-for-splits
|
@ZhenjaHorbach sorry, but I am looking at those messages and checking the app and I have no idea what is wrong, could you clarify? |
We have a split with a negative amount 
When we open edit screen for this split and update the distance the header does not match the actual values (should be 
And when we save these changes 
So I suppose we need to fix header on edit screen after editing distance |
|
@kubabutkiewicz |
…to negative-values-for-splits
|
@ZhenjaHorbach fix pushed! thanks for explenation! |
|
The header on Edit screen is not fixed (with negative and positive amounts) 2026-03-13.09.35.08.movAnd everything is good with positive amounts on staging 2026-03-13.09.38.55.movSo I suppose it should work the same |
…o take into account the distance chnges
|
Yeah sorry @ZhenjaHorbach I think I still didnt understand the full issue, pushed a fix for that |
…to negative-values-for-splits
|
@kubabutkiewicz |
|
But the last fix works! |
…to negative-values-for-splits
|
conflitcs resolved |
|
🚧 @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.39-0 🚀
|
|
Deploy Blocker ##85545 was identified to be related to this PR. |
|
Deploy Blocker #85551 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.39-3 🚀
|
8 participants
Explanation of Change
Fixed Issues
$#81591
$ #63311
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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