Site Logs: Pagination for Requests Broken

[Aurorum]

Quick summary

The Pagination under Site Logs only increments by one, which makes it extremely difficult to access older requests.

Steps to reproduce

1. Start at /site-logs/
2. Scroll to the bottom where Pagination is
3. Click an older page or try to select a custom number

Pagination should navigate to that location, but I could only navigate one page back/forward. This makes the feature a lot less useful than it would otherwise be!

[masperber]

I can reproduce this bug on my end as well. Clicking on a page number only navigates the active page forward or backward by one.

[mrfoxtalbot]

I can replicate this. The pagination does not work as one would expect: it only goes up or down 1 page at a time:

https://github.com/user-attachments/assets/425a3573-e6c6-4e65-98b1-4f02533e1df3

[mrfoxtalbot]

This is one of our newer features and it sounds like a relatively easy fix. Could someone from @Automattic/lego help us triage this? Thank you!

[masperber]

I believe that the issue is here: https://github.com/Automattic/wp-calypso/blob/trunk/client/components/pagination/pagination-page.jsx

	clickHandler = ( event ) => {
		event.stopPropagation();
		const { currentPage, pageClick, pageNumber, totalPages } = this.props;

		switch ( pageNumber ) {
			case 'previous':
				if ( currentPage - 1 < 1 ) {
					return;
				}
				pageClick( currentPage - 1 );
				break;
			case 'next':
				if ( currentPage + 1 > totalPages ) {
					return;
				}
				pageClick( currentPage + 1 );
				break;
			default:
				if ( currentPage === pageNumber ) {
					return;
				}
				pageClick( pageNumber );
				break;
		}
	};

It looks like the clickHander event is not reaching the default case, and instead it is returning either previous or next.

[masperber]

I checked a bit more, and I think in fact the issue is here: https://github.com/Automattic/wp-calypso/blob/trunk/client/my-sites/site-monitoring/logs-tab.tsx (134)

	const handlePageClick = ( nextPageNumber: number ) => {
		if ( isInitialLoading ) {
			return;
		}

		const nextPageIndex = nextPageNumber - 1;
		if ( nextPageIndex < currentPageIndex && currentPageIndex > 0 ) {
			setCurrentPageIndex( currentPageIndex - 1 );
		} else if (
			nextPageIndex > currentPageIndex &&
			( currentPageIndex + 1 ) * pageSize < ( data?.total_results ?? 0 )
		) {
			setCurrentPageIndex( currentPageIndex + 1 );
		}

		setAutoRefresh( false );
	};

It seems as though this is not implementing the clickHandler prototype found in Pagination. So, I propose the following change:

	const handlePageClick = ( nextPageNumber: number ) => {
		if ( isInitialLoading ) {
			return;
		}

		const nextPageIndex = nextPageNumber - 1;
		if ( nextPageIndex < currentPageIndex && nextPageIndex >= 0 ) {
			setCurrentPageIndex( nextPageIndex );
		} else if (
			nextPageIndex > currentPageIndex &&
			( nextPageIndex ) * pageSize < ( data?.total_results ?? 0 )
		) {
			setCurrentPageIndex( nextPageIndex );
		}

		setAutoRefresh( false );
	};

[taipeicoder]

@masperber will you submit the PR to fix this issue? Lego is happy to have either review the PR or create the PR. Let me know!

[masperber]

Thank you @taipeicoder !

PR here: https://github.com/Automattic/wp-calypso/pull/93998

[masperber]

@taipeicoder I discovered that the change that I proposed will allow you to select any tab. However, when new data is loaded, it will always load the next 50 records, and not the 50 records from the page that you are interested in. I suppose that this could be the underlying reason why you could only increment or decrement the page number by one. Once all of the pages have been loaded, then there is no risk that the wrong data will be loaded on the wrong page.

Do you have any thoughts on how this could be circumvented? I think I understand the underlying issue: in the event that there are thousands of logs, it could be difficult to handle that much data at once, so forcing the user to iterate through the logs one page at a time will sidestep this by loading only one page at a time as the user clicks.

[mrfoxtalbot]

Circling back to this, I noticed that this issue does not have anybody assigned to it. @taipeicoder, @masperber, what can we do to move this forward? Thank you!

CC @jartes @inaikem

[fushar]

I am looking at it now. 😄

[fushar]

So, it turns out to be a "regression". We were not supposed to be able to skip through the pages in the pagination; only prev and next pages are supported due to the limitation of the backend endpoint. See the original PR that introduced the pagination:

• https://github.com/Automattic/wp-calypso/pull/74935

I've submitted a PR https://github.com/Automattic/wp-calypso/pull/97097 to restore the original behavior.