Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use pagination metadata for report actions #49958

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Use pagination metadata for report actions #49958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7463dd4
Use pagination metadata for report actions
janicduplessis Oct 3, 2024
8134090
Add start marker when initial request with no cursor
janicduplessis Oct 3, 2024
39d1f8a
Merge branch 'main' into @janic/pagination-meta
janicduplessis Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/hooks/usePaginatedReportActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ function usePaginatedReportActions(reportID?: string, reportActionID?: string) {
});
const [reportActionPages] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_PAGES}${reportIDWithDefault}`);

const reportActions = useMemo(() => {
const {
data: reportActions,
hasNextPage,
hasPreviousPage,
} = useMemo(() => {
if (!sortedAllReportActions?.length) {
return [];
return {data: [], hasNextPage: false, hasPreviousPage: false};
}
return PaginationUtils.getContinuousChain(sortedAllReportActions, reportActionPages ?? [], (reportAction) => reportAction.reportActionID, reportActionID);
}, [reportActionID, reportActionPages, sortedAllReportActions]);
Expand All @@ -34,6 +38,8 @@ function usePaginatedReportActions(reportID?: string, reportActionID?: string) {
reportActions,
linkedAction,
sortedAllReportActions,
hasOlderActions: hasNextPage,
hasNewerActions: hasPreviousPage,
};
}

Expand Down
10 changes: 3 additions & 7 deletions src/libs/Middleware/Pagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ type PaginationCommonConfig<TResourceKey extends OnyxCollectionKey = OnyxCollect
pageCollectionKey: TPageKey;
sortItems: (items: OnyxValues[TResourceKey]) => Array<PagedResource<TResourceKey>>;
getItemID: (item: PagedResource<TResourceKey>) => string;
isLastItem: (item: PagedResource<TResourceKey>) => boolean;
};

type PaginationConfig<TResourceKey extends OnyxCollectionKey, TPageKey extends OnyxPagesKey> = PaginationCommonConfig<TResourceKey, TPageKey> & {
Expand Down Expand Up @@ -85,7 +84,7 @@ const Pagination: Middleware = (requestResponse, request) => {
return requestResponse;
}

const {resourceCollectionKey, pageCollectionKey, sortItems, getItemID, isLastItem, type} = paginationConfig;
const {resourceCollectionKey, pageCollectionKey, sortItems, getItemID, type} = paginationConfig;
const {resourceID, cursorID} = request;
return requestResponse.then((response) => {
if (!response?.onyxData) {
Expand All @@ -106,13 +105,10 @@ const Pagination: Middleware = (requestResponse, request) => {

const newPage = sortedPageItems.map((item) => getItemID(item));

// 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)) {
Copy link
Contributor

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:

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Contributor

@rlinoz rlinoz Oct 7, 2024 •
edited
Loading

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

Copy link
Contributor Author

@janicduplessis janicduplessis Oct 7, 2024 •
edited
Loading

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?

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is odd, because of this other comment #41153 (comment) 😄

Anyway, I am not sure what it means calling OpenReport without a reportActionID, how can I say whether a report has newer actions if I don't know related to what?

I mean, the code we are talking about implies that if type is initial and no cursorID then hasNewerActions should be false, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand in #35011 we want to open report at the last unread message, so we want to remove the assumption that calling OpenReport without a reportActionID means that there are no newer actions and instead rely on hasNewerActions returned from the backend. However at the moment it seems like that information is not returned.

I think our 2 options for now is:

  1. Update the backend to always return hasNewerActions even when called with no reportActionID (I do not have access to the backend so it would need to be done by an Expensify engineer).
  2. Just leave the code as is for now and remove the check for type is initial and no cursorID whenever we support opening report at the last unread message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah I finally understand, thanks for explaining, let me check what would need to change in order for 1 work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I figured what needs to be done, but then the App behaves in a few weird ways.

First, if you open a direct link to the report it will load only the unread page and won't load the newer ones, despite having the hasNewerActions: true and the code suggestion above applied

Second, if you have the app open and receive the messages then open the report we call ReadNewestAction before we call OpenReport, then we can't calculate whatever you have unread
image

I think we should merge this PR and figure out this problems in the other issue, what do you think @janicduplessis ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good

newPage.unshift(CONST.PAGINATION_START_ID);
}
const pageItem = sortedPageItems.at(-1);
if (pageItem && isLastItem(pageItem)) {
if (response.hasOlderActions === false) {
newPage.push(CONST.PAGINATION_END_ID);
}

Expand Down
20 changes: 15 additions & 5 deletions src/libs/PaginationUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,19 @@ function mergeAndSortContinuousPages<TResource>(sortedItems: TResource[], pages:

/**
* Returns the page of items that contains the item with the given ID, or the first page if null.
* Also returns whether next / previous pages can be fetched.
* See unit tests for example of inputs and expected outputs.
*
* Note: sortedItems should be sorted in descending order.
*/
function getContinuousChain<TResource>(sortedItems: TResource[], pages: Pages, getID: (item: TResource) => string, id?: string): TResource[] {
function getContinuousChain<TResource>(
sortedItems: TResource[],
pages: Pages,
getID: (item: TResource) => string,
id?: string,
): {data: TResource[]; hasNextPage: boolean; hasPreviousPage: boolean} {
if (pages.length === 0) {
return id ? [] : sortedItems;
return {data: id ? [] : sortedItems, hasNextPage: false, hasPreviousPage: false};
}

const pagesWithIndexes = getPagesWithIndexes(sortedItems, pages, getID);
Expand All @@ -185,15 +191,15 @@ function getContinuousChain<TResource>(sortedItems: TResource[], pages: Pages, g

// If we are linking to an action that doesn't exist in Onyx, return an empty array
if (index === -1) {
return [];
return {data: [], hasNextPage: false, hasPreviousPage: false};
}

const linkedPage = pagesWithIndexes.find((pageIndex) => index >= pageIndex.firstIndex && index <= pageIndex.lastIndex);

const item = sortedItems.at(index);
// If we are linked to an action in a gap return it by itself
if (!linkedPage && item) {
return [item];
return {data: [item], hasNextPage: false, hasPreviousPage: false};
}

if (linkedPage) {
Expand All @@ -206,7 +212,11 @@ function getContinuousChain<TResource>(sortedItems: TResource[], pages: Pages, g
}
}

return page ? sortedItems.slice(page.firstIndex, page.lastIndex + 1) : sortedItems;
if (!page) {
return {data: sortedItems, hasNextPage: false, hasPreviousPage: false};
}

return {data: sortedItems.slice(page.firstIndex, page.lastIndex + 1), hasNextPage: page.lastID !== CONST.PAGINATION_END_ID, hasPreviousPage: page.firstID !== CONST.PAGINATION_START_ID};
}

export default {mergeAndSortContinuousPages, getContinuousChain};
1 change: 0 additions & 1 deletion src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ registerPaginationConfig({
pageCollectionKey: ONYXKEYS.COLLECTION.REPORT_ACTIONS_PAGES,
sortItems: (reportActions) => ReportActionsUtils.getSortedReportActionsForDisplay(reportActions, true),
getItemID: (reportAction) => reportAction.reportActionID,
isLastItem: (reportAction) => reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED,
});

function clearGroupChat() {
Expand Down
4 changes: 3 additions & 1 deletion src/pages/home/ReportScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
const [isLinkingToMessage, setIsLinkingToMessage] = useState(!!reportActionIDFromRoute);

const [currentUserAccountID = -1] = useOnyx(ONYXKEYS.SESSION, {selector: (value) => value?.accountID});
const {reportActions, linkedAction, sortedAllReportActions} = usePaginatedReportActions(reportID, reportActionIDFromRoute);
const {reportActions, linkedAction, sortedAllReportActions, hasNewerActions, hasOlderActions} = usePaginatedReportActions(reportID, reportActionIDFromRoute);

const [isBannerVisible, setIsBannerVisible] = useState(true);
const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({});
Expand Down Expand Up @@ -756,6 +756,8 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro
{!shouldShowSkeleton && report && (
<ReportActionsView
reportActions={reportActions}
hasNewerActions={hasNewerActions}
hasOlderActions={hasOlderActions}
report={report}
parentReportAction={parentReportAction}
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
Expand Down
25 changes: 20 additions & 5 deletions src/pages/home/report/ReportActionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ type ReportActionsViewProps = {
/** The reportID of the transaction thread report associated with this current report, if any */
// eslint-disable-next-line react/no-unused-prop-types
transactionThreadReportID?: string | null;

/** If the report has newer actions to load */
hasNewerActions: boolean;

/** If the report has older actions to load */
hasOlderActions: boolean;
};

let listOldID = Math.round(Math.random() * 100);
Expand All @@ -76,6 +82,8 @@ function ReportActionsView({
isLoadingNewerReportActions = false,
hasLoadingNewerReportActionsError = false,
transactionThreadReportID,
hasNewerActions,
hasOlderActions,
}: ReportActionsViewProps) {
useCopySelectionHelper();
const reactionListRef = useContext(ReactionListContext);
Expand Down Expand Up @@ -253,7 +261,7 @@ function ReportActionsView({
*/
const fetchNewerAction = useCallback(
(newestReportAction: OnyxTypes.ReportAction) => {
if (isLoadingNewerReportActions || isLoadingInitialReportActions || (reportActionID && isOffline)) {
if (!hasNewerActions || isLoadingNewerReportActions || isLoadingInitialReportActions || (reportActionID && isOffline)) {
return;
}

Expand All @@ -270,7 +278,7 @@ function ReportActionsView({
Report.getNewerActions(reportID, newestReportAction.reportActionID);
}
},
[isLoadingNewerReportActions, isLoadingInitialReportActions, reportActionID, isOffline, transactionThreadReport, reportActionIDMap, reportID],
[isLoadingNewerReportActions, isLoadingInitialReportActions, reportActionID, isOffline, transactionThreadReport, reportActionIDMap, reportID, hasNewerActions],
);

const hasMoreCached = reportActions.length < combinedReportActions.length;
Expand All @@ -279,7 +287,6 @@ function ReportActionsView({
const hasCachedActionOnFirstRender = useInitialValue(() => reportActions.length > 0);
const hasNewestReportAction = reportActions.at(0)?.created === report.lastVisibleActionCreated || reportActions.at(0)?.created === transactionThreadReport?.lastVisibleActionCreated;
const oldestReportAction = useMemo(() => reportActions?.at(-1), [reportActions]);
const hasCreatedAction = oldestReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED;

useEffect(() => {
const wasLoginChangedDetected = prevAuthTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS && !session?.authTokenType;
Expand Down Expand Up @@ -343,7 +350,7 @@ function ReportActionsView({
}

// Don't load more chats if we're already at the beginning of the chat history
if (!oldestReportAction || hasCreatedAction) {
if (!oldestReportAction || !hasOlderActions) {
return;
}

Expand All @@ -367,11 +374,11 @@ function ReportActionsView({
isLoadingOlderReportActions,
isLoadingInitialReportActions,
oldestReportAction,
hasCreatedAction,
reportID,
reportActionIDMap,
transactionThreadReport,
hasLoadingOlderReportActionsError,
hasOlderActions,
],
);

Expand Down Expand Up @@ -524,6 +531,14 @@ function arePropsEqual(oldProps: ReportActionsViewProps, newProps: ReportActions
return false;
}

if (oldProps.hasNewerActions !== newProps.hasNewerActions) {
return false;
}

if (oldProps.hasOlderActions !== newProps.hasOlderActions) {
return false;
}

return lodashIsEqual(oldProps.report, newProps.report);
}

Expand Down
6 changes: 6 additions & 0 deletions src/types/onyx/Response.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ type Response = {

/** The email of the user */
email?: string;

/** If there is older data to load for pagination commands */
hasOlderActions?: boolean;

/** If there is newer data to load for pagination commands */
hasNewerActions?: boolean;
};

export default Response;
83 changes: 48 additions & 35 deletions tests/ui/PaginationTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,47 +128,60 @@ function buildReportComments(count: number, initialID: string, reverse = false)
}

function mockOpenReport(messageCount: number, initialID: string) {
fetchMock.mockAPICommand('OpenReport', ({reportID}) =>
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
value: buildReportComments(messageCount, initialID),
},
]
: [],
);
fetchMock.mockAPICommand('OpenReport', ({reportID}) => {
const comments = buildReportComments(messageCount, initialID);
return {
onyxData:
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
value: comments,
},
]
: [],
hasOlderActions: !comments['1'],
hasNewerActions: !!reportID,
};
});
}

function mockGetOlderActions(messageCount: number) {
fetchMock.mockAPICommand('GetOlderActions', ({reportID, reportActionID}) =>
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
// The API also returns the action that was requested with the reportActionID.
value: buildReportComments(messageCount + 1, reportActionID),
},
]
: [],
);
fetchMock.mockAPICommand('GetOlderActions', ({reportID, reportActionID}) => {
// The API also returns the action that was requested with the reportActionID.
const comments = buildReportComments(messageCount + 1, reportActionID);
return {
onyxData:
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
value: comments,
},
]
: [],
hasOlderActions: comments['1'] != null,
};
});
}

function mockGetNewerActions(messageCount: number) {
fetchMock.mockAPICommand('GetNewerActions', ({reportID, reportActionID}) =>
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
// The API also returns the action that was requested with the reportActionID.
value: buildReportComments(messageCount + 1, reportActionID, true),
},
]
: [],
);
fetchMock.mockAPICommand('GetNewerActions', ({reportID, reportActionID}) => ({
onyxData:
reportID === REPORT_ID
? [
{
onyxMethod: 'merge',
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
// The API also returns the action that was requested with the reportActionID.
value: buildReportComments(messageCount + 1, reportActionID, true),
},
]
: [],
hasNewerActions: messageCount > 0,
}));
}

/**
Expand Down
Loading
Loading