refactor: PureReportActionItem, IouReportActionMessage#88691
refactor: PureReportActionItem, IouReportActionMessage#88691cristipaval merged 10 commits intoExpensify:mainfrom
Conversation
LukasMod
changed the title
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.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
melvin-bot
Bot
requested review from
situchan and
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const isHoldReportAction = [CONST.REPORT.ACTIONS.TYPE.HOLD, CONST.REPORT.ACTIONS.TYPE.UNHOLD].some((type) => type === action.actionName); | ||
| const fragments = getReportActionMessageFragments(translate, action); | ||
|
|
||
| const renderReportActionItemFragments = (shouldWrapInText: boolean): ReactElement | ReactElement[] => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-4 (docs)
renderReportActionItemFragments is an internal render helper defined inside the component body and called in the return statement (renderReportActionItemFragments(isApprovedOrSubmittedReportAction)). This closure captures the entire component scope, preventing React Compiler from independently memoizing it.
Extract this into a separate component with explicit props:
function ReportActionItemFragments({fragments, action, reportID, iouMessage, style, displayAsGroup, isApprovedOrSubmittedReportAction, isHoldReportAction, shouldWrapInText}: Props) {
const styles = useThemeStyles();
const reportActionItemFragments = fragments.map((fragment, index) => (
<ReportActionItemFragment
key={`actionFragment-${action.reportActionID}-${index}`}
// ... props
/>
));
return shouldWrapInText ? <Text style={styles.ltr}>{reportActionItemFragments}</Text> : <>{reportActionItemFragments}</>;
}Then use it in the return:
{!isHidden ? (
<ReportActionItemFragments ... shouldWrapInText={isApprovedOrSubmittedReportAction} />
) : (
<Text ...>{translate('moderation.flaggedContent')}</Text>
)}Reviewed at: 2894a40 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
React Compiler compliance passes on the original, the closure here is not opaque to RC. This component renders once per visible report action, the extraction would add a component frame and an extra useThemeStyles() call per row for no realized benefit. I will keep it as it was originally (it was moved to new file)
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@situchan How is it going? I'm preparing another round of decomposition PRs and plan to work on top of this one |
|
Should be done today. Please merge main |
|
@situchan I merged main again 👍 it's ready for review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Refactoring Verification:
|
| Original logic | New location | Correct? |
|---|---|---|
useOnyx for REPORT, TRANSACTION, BANK_ACCOUNT_LIST |
IouReportActionMessage (only mounts for IOU actions) |
Yes |
iouMessage computation |
IouReportActionMessage |
Yes |
renderReportActionItemFragments + all ReportActionItemFragment props |
ReportActionMessageContent |
Yes |
isApprovedOrSubmittedReportAction, isHoldReportAction |
ReportActionMessageContent |
Yes |
isHidden → flagged content text |
ReportActionMessageContent |
Yes |
isMemberChangeAction early return |
ReportActionItemMessage (unchanged) |
Yes |
| Room description update early return | ReportActionItemMessage (unchanged) |
Yes |
isReimbursementDirectionInformationRequiredAction early return |
ReportActionItemMessage (unchanged) |
Yes |
Intentional removal: shouldShowAddBankAccountButton
Confirmed dead code. hasMissingInvoiceBankAccount(reportID) returns true only when isSettled(reportID) is true, but the outer expression required !isSettled(reportID). The AND is always false — the button could never render. Removal is correct.
Minor improvements in the refactoring
reportIDis now wrapped withgetNonEmptyStringOnyxID()in theuseOnyxcall forREPORTinsideIouReportActionMessage, preventing subscription to an invalid Onyx key whenreportIDis empty/undefined. The original lacked this guard.action.actionName === CONST.REPORT.ACTIONS.TYPE.IOUreplaced withisActionOfType(action, CONST.REPORT.ACTIONS.TYPE.IOU)— functionally equivalent, better type narrowing.
Conclusion
The three Onyx subscriptions (REPORT, TRANSACTION, BANK_ACCOUNT_LIST) are now isolated behind the isMoneyRequestAction gate, so non-IOU message renders no longer pay the subscription cost. All props passed to ReportActionItemFragment are identical. The new unit tests cover the IOU edge subtypes and the isHidden branch. No regressions identified.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @cristipaval 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/cristipaval in version: 9.3.66-0 🚀
|
|
No help site changes required. This PR is a purely internal code refactoring that splits |
6 participants
Explanation of Change
Extracted a new IouReportActionMessage component into
src/pages/inbox/report/actionContents/IouReportActionMessage.tsx. All IOU specific logic and useOnyx calls (REPORT, TRANSACTION, BANK_ACCOUNT_LIST) now live inside it. ReportActionItemMessage dispatches to the new component whenisMoneyRequestAction(action)is true, and otherwise renders the non IOU branches (member change, room description update, reimbursement direction, plain text fragments) without touching those subscriptions.The
IouReportActionMessagecomponent (extracted fromReportActionItemMessage) cannot be reached through any normal user action in the current frontend. It only renders when an IOU action withoriginalMessage.typeofdelete,reject,cancel, orapprovelands in Onyx with a still-existing transaction. No current FE flow produces these. The only deterministic way to exercise the path is to mock an action via the dev console.Dispatch in
PureReportActionItemcatches every common case before this code runs:-
CREATE/SPLIT/TRACKtoMoneyRequestAction.-
PAYtoPaymentContent.-
REIMBURSEMENT_QUEUEDandREIMBURSEMENT_DEQUEUEDto dedicated content components.The cost of keeping the component is essentially zero since it only mounts when actually needed. Removing the rendering would silently break any such message, so I decided to keep it anyway with additional unit tests that checks this specific branch case.
Also I removed
shouldShowAddBankAccountButtonas this path was now dead code. The shouldShowAddBankAccountButtongate combinedhasMissingInvoiceBankAccount(reportID)(which itself requiresisSettled(reportID)to be true) with!isSettled(reportID)` on the outer expression. Both calls evaluate the same pure function with the same argument, so one branch requires X = true while the other requires X = false, making the AND chain impossible to satisfy. The button could not render under any state. It was introduced 2 years ago here: #48014 and probably it was not used anymore, button is rendered in such cases via other PureReportActionItem branch.Fixed Issues
$ #88601
PROPOSAL:
Tests
Smoke tests:
Test 1: Regular chat message renders correctly
Test 2: Update a room description
Test 3: Invite or remove a member from a room
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