-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Use pagination metadata for report actions #49958
base: main
Are you sure you want to change the base?
Use pagination metadata for report actions #49958
Conversation
de00d0b
to
eda8d00
Compare
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-05.at.12.38.21.AM-1.movAndroid: mWeb ChromeScreen.Recording.2024-10-05.at.1.45.20.AM-1.moviOS: NativeScreen.Recording.2024-10-05.at.12.20.46.AM-1.moviOS: mWeb SafariScreen.Recording.2024-10-05.at.12.17.01.AM-1.movMacOS: Chrome / SafariScreen.Recording.2024-10-05.at.12.05.26.AM-1.movMacOS: DesktopScreen.Recording.2024-10-05.at.1.08.16.AM-1.mov |
@janicduplessis we have conflicts. code wise changes as is looks good to me, can you open this PR once you are done with your testing |
@ishpaul777 Awesome! Gonna work on conflicts and finalizing testing + author checklist now. |
eda8d00
to
7463dd4
Compare
@ishpaul777 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] |
PR is ready! |
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.
All yours @roryabraham
looks like conflicts |
// Detect if we are at the start of the list. This will always be the case for the initial request with no cursor. | ||
// For previous requests we check that no new data is returned. Ideally the server would return that info. | ||
if ((type === 'initial' && !cursorID) || (type === 'next' && newPage.length === 1 && newPage.at(0) === cursorID)) { | ||
if (response.hasNewerActions === false || (type === 'initial' && !cursorID)) { |
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.
hmmm I'm wondering if it actually should be more simply like this:
if (response.hasNewerActions === false || (type === 'initial' && !cursorID)) { | |
if (response.hasNewerActions === false) { |
because the back-end should always return hasNewerActions
, even if OpenReport
is called without a cursorID
.
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.
When I tested this it did not, that is why I added this condition back.
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.
I can't find a reference to cursorID
in the backend, is that the reportActionID
we are linking to?
If so, that seems right, we only send back both hasOlderActions
and hasNewerActions
when there is a reportActionID
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.
Yes, it is only use in the frontend, it is set here
App/src/libs/actions/Report.ts
Line 973 in 0c4b2d1
cursorID: reportActionID, |
Would it be possible to make the backend return both hasNext and hasPrevious even if no reportActionID is sent, it would simply the code here?
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.
Just to make sure, OpenReport
without a cursorID
means the user is opening the report at the last action right?
Also tagging in @rlinoz to take over internal reviewer role in my absence |
Conflicts fixed! |
Details
Use the new
hasNewerActions
andhasOlderActions
fields from the report pagination api calls to determine if new actions need to be loaded instead of relying on heuristics like the type of action. This simplifies the code and makes it less "hacky". This will also help reduce calls toGetNewerActions
in certain cases.Fixed Issues
$ #41153
PROPOSAL:
Tests
GetNewerActions
is made when linking to latest message in a chatOffline tests
QA Steps
Test 1: Basic Navigation
Note: Links can be copied to any message and used in any order.
Test 2: Popup Navigation
Test 3: Cache and Logout
Test 4:
Notes:
Ensure the chat contains at least 150 messages. Initially, up to 50 messages are rendered. Navigating to the 70th message should allow further navigation up to the 150th message, even with gaps.
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
MacOS: Desktop