Conversation
tgolen
changed the title
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
@situchan Are you up for reviewing this one as well? |
melvin-bot
bot
requested review from
aldo-expensify
and removed request for
a team
|
@aldo-expensify 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${CHILD_REPORT_ID}`, { | ||
| [CHILD_REPORT_ACTION_ID]: childReportAction1, | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line lacks a justification comment explaining why the rule is being disabled. ESLint rule disables without justification can mask underlying issues.
Add a comment explaining why the naming convention rule needs to be disabled here, e.g.:
// eslint-disable-next-line @typescript-eslint/naming-convention -- Onyx keys use numeric string IDs which violate naming conventions
'301': childReportAction2,Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-27.at.1.50.13.AM.mov |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
| import {cleanUpMoneyRequest} from '@userActions/IOU'; | ||
| import {navigateToConciergeChatAndDeleteReport} from '@userActions/Report'; | ||
| import {clearAllRelatedReportActionErrors} from '@userActions/ReportActions'; | ||
| import {clearAllRelatedReportActionErrors} from '@userActions/ClearReportActionErrors'; |
There was a problem hiding this comment.
Move this line above
This should fix prettier diff.
|
Hmm still prettier issue on other files |
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
NAB: we can remove first param - reportID (unused) as cleanup.
|
No product review needed |
|
@situchan Good catch on that unused parameter. I've updated this PR to remove it. |
|
Let's also update App/.claude/skills/coding-standards/rules/perf-2-early-return.md Lines 14 to 30 in 1d81969 Edit: Hmm, actually need |
|
@marcaaron 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] |
Yeah, this is kind of an awkward example now that this method also doesn't use |
|
still prettier diff. |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
|
Yay, all tests are passing now |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Changes LGTM. Nice tests!
| function createMockReportAction(overrides: Partial<ReportAction> = {}): ReportAction { | ||
| const errorTimestamp = Date.now() * 1000; | ||
| return { | ||
| reportActionID: REPORT_ACTION_ID, | ||
| actorAccountID: ACTOR_ACCOUNT_ID, | ||
| created: '2025-01-01 12:00:00.000', | ||
| actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, | ||
| automatic: false, | ||
| shouldShow: true, | ||
| avatar: '', | ||
| person: [{type: 'TEXT', style: 'strong', text: ACTOR_EMAIL}], | ||
| message: [{type: 'COMMENT', html: 'Test message with error', text: 'Test message with error'}], | ||
| errors: { | ||
| [errorTimestamp]: 'Something went wrong. Please try again.', | ||
| }, | ||
| ...overrides, | ||
| } as ReportAction; | ||
| } |
There was a problem hiding this comment.
NAB, are there any helpers we can use for this kind of test setup? I feel like there should be stuff in tests/utils we can use possibly.
|
There are some helpers already, yeah. I'll try to find one
…On Thu, Feb 26, 2026 at 5:28 PM Marc Glasser ***@***.***> wrote:
***@***.**** approved this pull request.
Changes LGTM. Nice tests!
------------------------------
In tests/ui/ClearReportActionErrorsUITest.tsx
<#83109 (comment)>:
> +function createMockReportAction(overrides: Partial<ReportAction> = {}): ReportAction {
+ const errorTimestamp = Date.now() * 1000;
+ return {
+ reportActionID: REPORT_ACTION_ID,
+ actorAccountID: ACTOR_ACCOUNT_ID,
+ created: '2025-01-01 12:00:00.000',
+ actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT,
+ automatic: false,
+ shouldShow: true,
+ avatar: '',
+ person: [{type: 'TEXT', style: 'strong', text: ACTOR_EMAIL}],
+ message: [{type: 'COMMENT', html: 'Test message with error', text: 'Test message with error'}],
+ errors: {
+ [errorTimestamp]: 'Something went wrong. Please try again.',
+ },
+ ...overrides,
+ } as ReportAction;
+}
NAB, are there any helpers we can use for this kind of test setup? I feel
like there should be stuff in tests/utils we can use possibly.
—
Reply to this email directly, view it on GitHub
<#83109 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB4M4JOHDHGVS4MIKFD4N6FULAVCNFSM6AAAAACV26GWCWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQNRUGI2TAMRVHE>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
|
🚧 @marcaaron 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/marcaaron in version: 9.3.31-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.31-12 🚀
|
5 participants
This is some tricky code. I am using
Onyx.connectWithoutView()here, because the function needs access to the entire collection of reports and all report actions. The UI component that uses the clear report function is a list item component, so I did not want to cause performance issues with subscribing to those two large collections.It is extremely hard to test this because it requires there to a dismissable error on transactions. I've added plenty of unit tests, including a UI test which should verify that this PR is working as intended. That is why I think it's OK to have this be NO QA.
Explanation of Change
This is part of the process to remove a global reference from
getOriginalReportID()(18 references). Here is the full refactoring plan:reportActionsParamparameter and fallback to the global Onyx value forallReportActionsundefinedfor the parameterreportActionsavailable to pass that as the parameterreportActionsas the parameterreportActionsParamtoreportActionsallReportActionsand the Onyx connectionFixed Issues
Part of #66419
Tests
Add this exception to the top of
requestMoney()in Web-E hereOffline tests
Same as tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari