Repository navigation
Conversation
|
@MelvinBot review |
|
@Eskalifer1 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] |
|
Hi @sosek108 let me know if you need my help here! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3593a5e52
ℹ️ 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".
ReviewOne blocking issue: ESLint fails — two unused Fix: delete both comments ( Worth fixing before mergeNothing in CI runs this test on this PR. Test steps are stale. They say "Verify all 3 scenarios pass"; the file has 5 tests. Nits
What I verified as correct
I could not execute the perf test in this environment, so the findings above are static plus the ESLint run. The |
There was a problem hiding this comment.
Can you update/ add the test comments explaining the tests with Given, When, Then structure?
There was a problem hiding this comment.
Done, I've created Given, When, Then structure in each test
|
Test failing is not ours |
|
@mountiny Kind bump on this simple PR |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.73-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.73-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This is PR 1 of a 10-PR stack decomposing the MoneyRequestReport list components (
MoneyRequestReportActionsList/MoneyRequestReportTransactionList) into focused hooks and presentational sub-components.Before any refactoring lands, this PR adds a Reassure performance test for
MoneyRequestReportActionsListso every subsequent slice in the stack can be measured against a baseline:Fixed Issues
$ #99919
PROPOSAL:
Tests
NODE_OPTIONS=--experimental-vm-modules npm run perf-test measure -- --testMatch '<rootDir>/tests/perf-test/MoneyRequestReportActionsList.perf-test.tsx'Note: CI does not run Reassure on this PR (
reassurePerformanceTests.ymlignorestests/**and the regular Jest jobs excludetests/perf-test/**), so here is the local run (10 runs each) this stack will baseline against:Offline tests
N/A — test-only change, no production code touched.
QA Steps
None — automated perf test only, no production code touched.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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