-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create usePaginatedReportActions hook #44412
Conversation
# Conflicts: # src/pages/home/ReportScreen.tsx
tests/ui/UnreadIndicatorsTest.tsx
Outdated
expect(unreadIndicator).toHaveLength(1); | ||
|
||
// Leave a comment as the current user and verify the indicator is not removed | ||
const reportActionsBefore = (await TestHelper.onyxGet(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`)) as Record<string, ReportAction>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 535 -> 547 are the most important changes in this test file - the rest of it is just using async
/await
instead of promise chains without further changes.
These lines simulate a server response setting a correct value for previousReportActionID
when our action is finalized.
It's not 100% clear why this wasn't needed before, but it seems like using useOnyx
results in faster re-renders in a test environment, and that test was a false positive before just testing the content we had on the screen before calling Report.addComment
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test to verify the new comment is visible on the screen after it's added, to prevent false positives in the future.
@stitesExpensify 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeRecord_2024-06-28-01-03-48.mp4Android: mWeb ChromeRecord_2024-06-28-00-21-28.mp4Record_2024-06-28-00-31-46.mp4iOS: NativeScreen.Recording.2024-06-27.at.11.14.28.PM-1.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Plus.-.2024-06-27.at.23.27.45-1.movMacOS: Chrome / SafariScreen.Recording.2024-06-27.at.11.10.29.PM-1.movMacOS: DesktopScreen.Recording.2024-06-27.at.11.56.51.PM-1.mov |
This issue #41962 (comment) seems to be reproducable on this branch as well
edit: Repro on main also |
Looks like this is reproducable on main also not a blocker for this or main PR. Its coming from #42592, setting |
This comment was marked as resolved.
This comment was marked as resolved.
🎯 @ishpaul777, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #44567. |
# Conflicts: # src/pages/ReportDetailsPage.tsx
*/ | ||
function usePaginatedReportActions(reportID?: string, reportActionID?: string) { | ||
// Use `||` instead of `??` to handle empty string. | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even have this rule? This is like the 3rd time I've seen it dismissed today.
^ Not saying this is causing any problems yet, there's just a deploy blocker that can't be reproduced on my local staging env (not sure why) so I'm trying to see if any adhoc builds work |
Sorry to say that this is causing #44583 :/ Gonna get a revert up |
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.3-7 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
Details
This PR is part of #41962, done in the hopes of reducing the diff, avoiding conflicts, and reducing the likelihood of regressions.
Fixed Issues
$ see the list in #41962
Tests / QA Steps
Offline tests
n/a
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop