Repository navigation
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.
|
|
@DylanDylann 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] |
|
the failing test is not related to this PR and will merge main to fix soon |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf2bbc3d83
ℹ️ 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".
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@daledah Could you fix the failed check? |
|
@DylanDylann i resolved conflict and the failed check |
|
@DylanDylann i updated, please check again |
The catch is that App/src/libs/actions/IOU/TrackExpense.ts Lines 910 to 916 in e6ea035 But your goal is already met: since |
|
@daledah Don't worry about the self-DM report case, because self-DMs don't have a draft report, so it will always be false |
|
The only logic I suggest moving to the component level is |
|
@DylanDylann I removed that line from the action - it now just trusts the passed report. This is equivalent for every caller (none pass an empty/report-less object), so the |
| report, | ||
| parentChatReport: parentChatReportParam, | ||
| action, |
There was a problem hiding this comment.
COuld we remove report param?
There was a problem hiding this comment.
report and parentChatReport look duplicated. I mean, we should remove report param
There was a problem hiding this comment.
@DylanDylann They look duplicated in most callers, but they're not the same thing. parentChatReport is the (already-resolved) parent chat report, while report is the source report - which can be a money request/expense report (e.g. in the categorize/share/move flows via useExpenseSubmission). We still need report to compute isMoneyRequestReport / moneyRequestReportID, ... deriving those from parentChatReport (always a chat report) would make moneyRequestReportID empty and the expense would no longer be added to the existing money request report.
There was a problem hiding this comment.
In the useExpenseSubmission case, isMoneyRequestReport should be checked via parentChatReport. First, could you explain why you pass parentChatReport: isSelfDMDestination ? selfDMReport : currentChatReport in useExpenseSubmission?
There was a problem hiding this comment.
@DylanDylann i checked again, report is redundant, so I've removed it.
could you explain why you pass parentChatReport: isSelfDMDestination ? selfDMReport : currentChatReport
parentChatReport is the chat the expense attaches to, so it must always be a chat: for a self-DM destination we force the self-DM, otherwise currentChatReport (the report itself, or its parent chat if report is a money-request report). It always resolves to a chat, so isMoneyRequestReport is always false here - that's why dropping report is safe.
|
@daledah Could you please review the PR again? You marked some of my previous comments as resolved, but I still see them in the codebase. Please only resolve a discussion once it's actually done |
|
@DylanDylann i updated, please check again |
|
@DylanDylann i updated, please check again |
|
@daledah replied #95570 (comment), let me know when the PR is ready |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d94b1d40ef
ℹ️ 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".
| const targetChatForDuplicate = isSourcePolicyValid ? chatReport : activePolicyExpenseChat; | ||
| const [isDraftChatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_DRAFT}${targetChatForDuplicate?.reportID}`, {selector: isDraftReportSelector}); |
There was a problem hiding this comment.
Subscribe to the expense duplicate's target chat
When the source expense belongs to an accessible policy other than the default expense policy, this subscribes to the source chatReport draft state, but duplicateExpenseTransaction targets activePolicyExpenseChat with defaultExpensePolicy and forwards this boolean as its draft state. Consequently, duplicating into a draft default workspace can treat it as non-draft and omit the workspace-creation Onyx/API data; use the active policy expense chat's draft state for the single-expense duplicate path, separately from the report-duplicate target.
Useful? React with 👍 / 👎.
|
@DylanDylann The scope of this PR is quite complex and difficult to test and review, so I think we should split it into multiple PRs. I'll close this PR, and we have another PR open here |
Explanation of Change
Fixed Issues
$ #66414
PROPOSAL:
Tests
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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
Screen.Recording.2026-07-09.at.10.20.02.mov