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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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

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

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

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?

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
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
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
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
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
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
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