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

src/components/submission/table-view.vue

+      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

src/components/entity/list.vue

+        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

test/components/submission/field-dropdown.spec.js


 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

src/components/entity/list.vue

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

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

src/composables/pagination-query-ref.js

+    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

test/components/submission/field-dropdown.spec.js


 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

src/composables/pagination-query-ref.js

+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

src/composables/pagination-query-ref.js

+    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

src/composables/pagination-query-ref.js

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

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

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

src/components/submission/table-view.vue

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

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

src/components/submission/table-view.vue

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

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

src/components/submission/table-view.vue

 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.

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

src/composables/pagination-query-ref.js

+/*
+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

src/composables/pagination-query-ref.js

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

test/components/entity/list.spec.js

+        });
+    });
+
+    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

test/util/submission.js

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

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 from 66eb211 to 91183bc Compare March 5, 2026 20:33

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

116a34d

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

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

matthew-white reviewed Mar 15, 2026

View reviewed changes

	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]) => {

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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