Add pagination info in the query parameter by sadiqkhoja · Pull Request #1417 · getodk/central-frontend

sadiqkhoja · 2025-11-18T00:50:33Z

Closes getodk/central#1348

What has been done to verify that this works as intended?

Manual verification + added few tests.

Why is this the best possible solution? Were any other approaches considered?

Please see inline comments.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

matthew-white · 2025-11-18T06:28:20Z

A few quick thoughts from me!

Order of PRs

I think this PR will conflict with getodk/central#1434, just to the extent that I'll be adding an EntityTableView component for that. Should I go ahead and push a PR for that now? Or would it be easier to get this one merged first? I've been working on other aspects of entity mapping, so I'd be happy for this one to go first if it could get merged quickly.

Toggling to/from map view

If there are pagination-related query parameters, do those currently persist when toggling to map view? For example:

Navigate to the table.
Move to the second page of the table (?page-number=2).
Move to map view.
- I would expect the page-number query parameter to be removed at this point.
Move back to table view.
- I would expect to see the first page of the table.

Composable

How about we add a composable for the pagination-related query parameters? I'm thinking of something similar to useDateRangeQueryRef(). I could see such a composable being useful in the future beyond submissions and entities. The composable would probably have to take an argument for pageSizeOptions.

matthew-white · 2025-11-18T06:38:20Z

+      const lastPage = Math.max(0, Math.ceil(pagination.count / pagination.size) - 1);
+      if (pagination.page > lastPage) {
+        pagination.page = lastPage;
+        fetchChunk(false);


Are you specifying false so that the snapshot filter isn't reset?

One thing I'm thinking about is that it'd be good to show a loading message during this second fetchChunk(). Otherwise, if that request takes a while, the user might get confused and think that lastPage is empty. Maybe we could show the loading message by setting odata.data to null here before calling fetchChunk(). Or maybe that logic belongs in fetchChunk(), and fetchChunk() just needs different options than clear and refresh. 🤔

introduced skipSettingSnapshotFilter in fetchChunk

removed skipSettingSnapshotFilter as we are no longer using snapshot filters.

and set fetchChunk(true) so that loading message is shown

matthew-white · 2025-11-18T06:39:33Z

+        if (!num || num < 0) return 0;
+        return num - 1;
+      },
+      toQuery: (value) => ({ 'page-number': value + 1 })


What do you think about removing the query parameter for the first page? Since the first page is the default — one less query parameter in the URL.

Suggested change

toQuery: (value) => ({ 'page-number': value + 1 })

toQuery: (value) => ({ 'page-number': value === 0 ? null : value + 1 })

matthew-white · 2025-11-18T06:49:28Z


 describe('SubmissionFieldDropdown', () => {
+  beforeEach(() => {
+    mockLogin();


Why is mockLogin() needed here out of curiosity? Something to do with the router?

because I changed the router from mock to test in test/util/submission.js, with that change request to sessions is made causing the tests to fail.

Another thing I can do is to add replace method to the list of supported method for mockRouter.

Another thing I can do is to add replace method to the list of supported method for mockRouter.

I'm open to this. It sure would be nice for there to be fewer cases in which testRouter() is needed. As-is, the difference between mockRouter() and testRouter() can feel sort of complicated.

Part of my idea for mockRouter() was that it isolates the individual component being tested from all the logic in the router (the beforeEach guards, etc.). To keep with that theme, maybe replace() could look something like this in mockRouter():

replace: (location) => { currentRoute.value = router.resolve(location); },

In other words, it's not actually doing a navigation (it's not actually calling router.replace()). Instead, it's just setting currentRoute as if the navigation took place. mockRouter() would no longer be read-only, but it would still be different from testRouter() in that it would never run navigation hooks.

Some other things would have to change in mockRouter() though, like the way that $route is provided on app install. It's just a constant right now.

This path sounds promising, but also no need to go down it right now if it doesn't feel like the right time. I'm happy with us using mockLogin() here.

sadiqkhoja · 2025-11-26T20:05:58Z

        :top="pagination.size"
        :filter="odataFilter != null || !!searchTerm"
-        :total-count="dataset.dataExists ? dataset.entities : 0"/>
+        :total-count="dataset.dataExists && !pagination.page ? dataset.entities : 0"/>


I don't want to show "Loading first N entities...", with this change it will always display "Loading entiities..." when there is a pagination information in the query parameter on the first loading. Note that on change pages using pagination control doesn't show any loading message; we show spinner in the pagination control.

Could you say more about why you don't want to show "Loading first N entities…"? I'm OK with that change, just wondering whether Nicole suggested it or if there was another reason.

Note that on change pages using pagination control doesn't show any loading message

If that's the case, I sort of think we should simplify OdataLoadingMessage. It has so many cases, many of which aren't about the first page. If some of its i18n messages aren't useful anymore, it'd be nice to remove them so that translators don't have to translate them.

One option is that we make that change in a follow-up PR. In that case, maybe you could file a refactor issue about it?

It'd be great if we could resolve this comment before merging. If this PR will make several i18n messages irrelevant, I think it'd be best to remove them. That could be done in a follow-up PR, but it'd be nice to track the need to do so in a new issue.

sadiqkhoja · 2025-11-26T20:10:21Z

+    fromQuery: (query) => {
+      const num = Number.parseInt(query['page-number'], 10);
+      if (!num || num < 0) return 0;
+      return num - 1;


should I update the query parameter here? if the given value is out of valid range? we don't do that for invalid values of filters, right?

we don't do that for invalid values of filters, right?

That's right. If filter query parameters are supplied invalid values, those values are generally ignored. The URL isn't changed/corrected, but nothing explodes either. The invalid values fall back to something reasonable.

In some cases, we can't know right away whether a value is valid. For example, specifying a submitter ID that doesn't exist: we can't know that it's wrong until we receive the response for the list of submitters (async).

should I update the query parameter here?

I'm open to it, but it feels more complex to me. It seems simpler not to change the route if we don't need to. It should be pretty rare that someone specifies an invalid value.

if the given value is out of valid range?

"Out of valid range" just refers to if (!num || num < 0) return 0; right? That logic sounds great to me.

sadiqkhoja · 2025-11-26T20:18:56Z


 describe('SubmissionFieldDropdown', () => {
+  beforeEach(() => {
+    mockLogin();


because I changed the router from mock to test in test/util/submission.js, with that change request to sessions is made causing the tests to fail.

Another thing I can do is to add replace method to the list of supported method for mockRouter.

sadiqkhoja · 2025-11-26T20:22:17Z

Order of PRs

I am okay with either way, I can hold this PR till maps for Entities are done or at list table-view and map-view files are created.

Toggling to/from map view

Fixed it

Composable

Created pagination-query-ref.js

sadiqkhoja · 2025-11-26T20:23:28Z

+export default (pageSizeOptions = [250, 500, 1000]) => {
+  const pageNumber = useQueryRef({
+    fromQuery: (query) => {
+      const num = Number.parseInt(query['page-number'], 10);


I using page-number for the readability sake, we can also go with just page.

I definitely prefer page over page-number. It's much shorter, and I see it on many sites. page + page-size sounds like a nice combo to me. I don't have the strongest feelings about it though, we could run with page-number for now and see if anyone else on the team voices an opinion.

sadiqkhoja · 2025-11-26T20:24:28Z

+    fromQuery: (query) => {
+      const num = Number.parseInt(query['page-number'], 10);
+      if (!num || num < 0) return 0;
+      return num - 1;


I guess page number starting from 1 is more acceptable than 0.

sadiqkhoja · 2025-11-26T20:25:29Z

+    toQuery: (value) => ({ 'page-number': value === 0 ? null : value + 1 })
+  });
+
+  const pageSize = useQueryRef({


Expecting absolute value for page sizes like 250, 500, 1000 instead of index of options here.

sadiqkhoja · 2026-03-05T21:21:41Z

@matthew-white this is ready for re-review.

matthew-white

It's looking good!

Probably the main question I still have is where/when pagination.page is set to 0.

In some cases, I made non-essential suggestions. I know we want to get this PR merged soon, so I'm happy with us skipping some of those suggestions or filing follow-up issues about them.

matthew-white · 2026-03-15T04:54:26Z

-  const first = clear || refresh;
-  if (first) {
+  if (refresh) {
    pagination.page = 0;


I'm trying to think through the ramifications of pagination.page = 0; for a background refresh. Wouldn't that cause the pagination control to immediately change to the first page? But the actual table wouldn't change until the request completes. If that takes a while, I think it'd be confusing for the pagination control to say that it's on the first page. Or even worse, if the request results in an error, the pagination control would incorrectly think that it's on the first page.

If it's a background refresh, I feel like pagination.page should be set to 0 only once the response is received.

It kind of feels like that would be changing the current behavior of background refresh though. Right now, does it refresh the current page rather than returning to the first page? If so, I feel like changing that behavior should maybe go in its own PR. Then this PR could just be focused on query parameters.

I have tested on test.getodk.cloud and on pressing of refresh button it goes back to the first page.

What I have done now is that $skip would send 0 if refresh or clear is true otherwise use pagination options. And pagination.page=0 happens only in the success response.

matthew-white · 2026-03-15T05:01:03Z

    :count="pagination.count" :size-options="pageSizeOptions"
    :spinner="odata.awaitingResponse"
    :removed="pagination.removed"
    @update:page="handlePageChange"/>


Instead of calling handlePageChange this way by listening for an event, I'm wondering whether we should add a watcher on pagination.page. Now there are multiple pathways that can change pagination.page. If the user changes the page number in the URL, that will change pagination.page, and I'd expect that to have an effect on the page itself. If that's hard to do, it's probably not a big deal, but that's how filters work right now. Hopefully just making it a watcher would work out.

when I change the page number in the URL, I see whole page gets reloaded both in chrome and firefox, which sets the correct pagination options before request is made hence page itself shows the correct data. I hope I understood your comment correctly.

Ah right, I forgot that's how it works. This might have been a concern under the old hash-based routing, but not anymore.

matthew-white · 2026-03-15T05:03:20Z

 fetchChunk(true);
-watch([() => props.filter, () => props.deleted], () => { fetchChunk(true); });
+watch([() => props.filter, () => props.deleted], () => {
+  pagination.page = 0;


Right now, if you change the selection of columns / form fields, you're sent back to the first page. But I'm only seeing this line pagination.page = 0; for filter changes, not a change to the column selection. I could be convinced that the current behavior isn't important and is fine to change, but I'm wondering if there's a different way to approach this. I kind of like how fetchChunk() used to be responsible for setting pagination.page in more cases. What if fetchChunk() took a page number argument that defaulted to 0? That would also work for the case where the user navigates past the last page. Like I said, I'm open to various approaches here, I just wanted to throw out these thoughts.

Since I have move pagination.page=0 in the then of the request, it will retain the existing behavior of changing columns.

matthew-white · 2026-03-15T05:04:07Z

+/*
+Copyright 2025 ODK Central Developers
+See the NOTICE file at the top-level directory of this distribution and at
+https://github.com/getodk/central-frontend/blob/master/NOTICE.
+
+This file is part of ODK Central. It is subject to the license terms in
+the LICENSE file found in the top-level directory of this distribution and at
+https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
+including this file, may be copied, modified, propagated, or distributed
+except according to the terms contained in the LICENSE file.
+*/


We don't need these file headers for new files anymore. 🥳 So this can be removed.

matthew-white · 2026-03-15T05:12:22Z

+
+import useQueryRef from './query-ref';
+
+export default (pageSizeOptions = [250, 500, 1000]) => {


I'm not sure we need a default value here. It's hard for me to think of a case where pageSizeOptions wouldn't be provided. sizeOptions is a required prop in the Pagination component, so it should always exist. If we were to add the concept of a default set of size options, that should probably go in multiple places (e.g., Pagination as well).

matthew-white · 2026-03-15T05:22:06Z

+        });
+    });
+
+    it('floors page-size to nearest valid value when invalid page-size is provided in URL', () => {


For some of these edge cases, I feel like they'd be better tested in unit tests of the usePaginationQueryRef() composable rather than in every component that uses the composable. There are already so many tests of the EntityList component.

withSetup() provides a way to unit-test a composable. We have unit tests of useQueryRef() that you might find interesting to look at.

Given that these tests are already written, I'm happy to run with them as-is. Mostly I just wanted to share the thought that I think there can be benefit to unit-testing composables and that withSetup() provides a way to do so.

matthew-white · 2026-03-15T05:31:02Z

-      router: mockRouter(form.publishedAt != null
-        ? `/projects/${project.id}/forms/${encodeURIComponent(form.xmlFormId)}/submissions`
-        : `/projects/${project.id}/forms/${encodeURIComponent(form.xmlFormId)}/draft`)


That's interesting that it's OK for these paths to go away. I see above that loadEntityList() has never specified a path to mockRouter(). There would probably be options to specify a path if we needed to. Maybe by calling .route(), similar to what's done in test/components/submission/filters.spec.js. There, loadSubmissionList() is called with testRouter(), then .route() is called so that the initial route is set correctly. If it's working now though, I guess nothing needs to change 🤷 — no path is needed here. But maybe let's at least get rid of testRouter() in submission/filters.spec.js if testRouter() is going to be the default going forward.

matthew-white

I haven't taken a look at all the changes yet, but I thought I'd leave some initial comments about mockRouter() and replace(). Related comment: #1417 (comment)

matthew-white · 2026-04-19T04:53:01Z

I think these comments are out-of-date now:

The mock router can now navigate.

It's no longer read-only.

matthew-white · 2026-04-19T04:53:11Z

+    push: sinon.fake(),
+    replace: (loc) => {
+      currentRoute.value = router.resolve(loc);
+    },


I don't feel like it totally works to make this change on its own. For example, $route is currently set to a constant at install time under the assumption that it will never change. But that assumption won't be true anymore.

I'm thinking we could create a Proxy object that delegates to currentRoute.value. Something like:

const routeProxy = new Proxy(currentRoute, { get: (route, prop) => route.value[prop] }); app.config.globalProperties.$route = routeProxy; app.provide(routeLocationKey, routeProxy);

matthew-white · 2026-04-19T04:55:54Z

      app.component('RouterView', RouterViewStub);
    },
-    push: sinon.fake()
+    push: sinon.fake(),


What do you think about making push() the same as replace()? It feels a little unexpected that replace() will change the route, while push() won't. push() could remain a Sinon fake. So something like:

push: sinon.fake(loc => { currentRoute.value = router.resolve(loc); }),

I'm not sure how tests currently use push(), so maybe this would cause problems, but hopefully that'd work.

matthew-white · 2026-05-04T20:15:21Z

@@ -129,6 +129,25 @@ describe('SubmissionList', () => {
          });
        });
    });
+
+    it('makes the correct request after refresh button is pressed on second page', () => {


Could you add a similar test for EntityList? It looks to me like SubmissionList and EntityList work slightly differently in this area.

matthew-white · 2026-05-04T20:17:52Z

 // For defineExpose()
 const exposedFetch = {
-  fetchData: (clear = true) => fetchChunk(clear, !clear),
+  fetchData: (clear = true) => fetchChunk(clear, clear ? 0 : pagination.page),


Suggested change

fetchData: (clear = true) => fetchChunk(clear, clear ? 0 : pagination.page),

fetchData: (clear = true) => fetchChunk(clear),

I'm thinking that the second argument here should always be 0 (or should be omitted). At #1417 (comment), you wrote:

I have tested on test.getodk.cloud and on pressing of refresh button it goes back to the first page.

But to me, that doesn't seem to be what this line is doing. If clear is false (if the refresh button has been pressed), then the user will remain on their current pagination.page, not go back to the first page.

matthew-white · 2026-05-04T20:43:01Z

I think this comment thread still needs to be resolved: #1417 (comment).

I glanced over all the discussion above, and with the exception of that thread and my review today, I didn't see any other open threads.

matthew-white

Looks great! 💯 I know we've been discussing this one for a while, but I think this PR has ended up in a really nice place. I think it says something that it now only modifies 8 files. I really like the unit tests of the usePaginationQueryRef() composable — they feel like they're at the right level.

There are still a few comments left to resolve. Nothing big though, so I'll go ahead and approve. @sadiqkhoja, feel free to merge once you've resolved those comments!

PR Feedback + Created paginationQueryRef composable + Removed pagination query parameter when navigating to the map view + Included skipSettingSnapshotFilter to the fetchChunk function

introduced `page` arg in fetchChunk() set page=0 in success response of the request removed copyright header removed default pageSizeOptions from pagination-query-ref various changes to the tests

sadiqkhoja changed the title ~~Feature getodk/central#1348: Add pagination info in the query parameter~~ Add pagination info in the query parameter Nov 18, 2025

matthew-white reviewed Nov 18, 2025

View reviewed changes

sadiqkhoja force-pushed the features/pagination-details-in-url branch 3 times, most recently from 8081b98 to 0bdd1e6 Compare November 26, 2025 20:12

sadiqkhoja commented Nov 26, 2025

View reviewed changes

sadiqkhoja marked this pull request as ready for review November 26, 2025 20:25

sadiqkhoja requested a review from matthew-white November 26, 2025 20:26

sadiqkhoja force-pushed the features/pagination-details-in-url branch from 0bdd1e6 to 66eb211 Compare December 6, 2025 04:23

sadiqkhoja requested review from matthew-white and removed request for matthew-white December 6, 2025 04:39

sadiqkhoja force-pushed the features/pagination-details-in-url branch 2 times, most recently from 91183bc to 116a34d Compare March 5, 2026 21:03

matthew-white reviewed Mar 15, 2026

View reviewed changes

sadiqkhoja force-pushed the features/pagination-details-in-url branch from 116a34d to 300df5e Compare April 10, 2026 19:56

sadiqkhoja requested a review from matthew-white April 10, 2026 20:05

sadiqkhoja force-pushed the features/pagination-details-in-url branch from 300df5e to d43c995 Compare April 10, 2026 20:11

matthew-white reviewed Apr 19, 2026

View reviewed changes

matthew-white reviewed May 4, 2026

View reviewed changes

matthew-white approved these changes May 5, 2026

View reviewed changes

sadiqkhoja added 4 commits May 15, 2026 12:46

Feature getodk/central#1348: Add pagination info in the query parameter

f76cc85

PR Feedback + Created paginationQueryRef composable + Removed pagination query parameter when navigating to the map view + Included skipSettingSnapshotFilter to the fetchChunk function

PR Feedback:

35e567b

introduced `page` arg in fetchChunk() set page=0 in success response of the request removed copyright header removed default pageSizeOptions from pagination-query-ref various changes to the tests

make currentRoute in mockRouter a proxy

7785e38

fix fetchChunk arg and updated a test

878f5b8

sadiqkhoja force-pushed the features/pagination-details-in-url branch from 5e35277 to 9510b70 Compare May 15, 2026 19:27

remove irrelevant messages from loading message component

2d7d63f

sadiqkhoja force-pushed the features/pagination-details-in-url branch from 9510b70 to 2d7d63f Compare May 15, 2026 19:35

	toQuery: (value) => ({ 'page-number': value + 1 })
	toQuery: (value) => ({ 'page-number': value === 0 ? null : value + 1 })


		import useQueryRef from './query-ref';

		export default (pageSizeOptions = [250, 500, 1000]) => {

	fetchData: (clear = true) => fetchChunk(clear, clear ? 0 : pagination.page),
	fetchData: (clear = true) => fetchChunk(clear),

Conversation

sadiqkhoja commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

Uh oh!

matthew-white commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadiqkhoja Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-white Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadiqkhoja Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-white May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadiqkhoja commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadiqkhoja commented Mar 5, 2026

Uh oh!

matthew-white left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sadiqkhoja commented Nov 18, 2025 •

edited

Loading

matthew-white commented Nov 18, 2025 •

edited

Loading

sadiqkhoja Mar 5, 2026 •

edited

Loading

matthew-white Nov 18, 2025 •

edited

Loading

sadiqkhoja Nov 26, 2025 •

edited

Loading

matthew-white May 4, 2026 •

edited

Loading

matthew-white commented May 4, 2026 •

edited

Loading