Skip to content

feat: add includeTotal option to pagination#430

Closed
wschurman wants to merge 1 commit intomainfrom
wschurman/02-09-feat_add_includetotalcount_to_pagination
Closed

feat: add includeTotal option to pagination#430
wschurman wants to merge 1 commit intomainfrom
wschurman/02-09-feat_add_includetotalcount_to_pagination

Conversation

@wschurman
Copy link
Member

@wschurman wschurman commented Feb 10, 2026

Why

A frequent operation that is needed during pagination is computation of the total number of pages. Some relay-style paginators (connections, edges) expose a totalCount field on the Connection type.

This PR adds this feature. Note that I'm not a strong proponent for this, but claude is and it wouldn't stop trying to implement it or suggest it in code review throughout this process. So we built it, but I'm open to not landing it if we think it'll be abused (query on every page, for example). Claude and my research seem to think that it makes sense and is aligned with other libraries like apollo.

How

See docblock for includeTotal for summary, but at a high level this does either a postgres window function or a count(*) to get the count. Window functions are more efficient since the postgres execution can fetch that information in the same query, but they only work when cursors are not used (otherwise the window function would apply over the rows selected by the cursor condition).

Test Plan

Full test coverage, many tests.

@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (58deffd) to head (d3a2418).

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #430    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          108       108            
  Lines        15606     15882   +276     
  Branches      1384      1412    +28     
==========================================
+ Hits         15606     15882   +276     
Flag Coverage Δ
integration 26.00% <85.27%> (+1.03%) ⬆️
unittest 95.91% <57.53%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch from 9458e67 to 50a20a7 Compare February 10, 2026 04:41
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch 4 times, most recently from 2b437e1 to 4439b05 Compare February 10, 2026 23:12
@wschurman wschurman changed the title feat: add includeTotalCount to pagination feat: add includeTotal option to pagination Feb 10, 2026
@wschurman wschurman force-pushed the wschurman/02-05-feat_add_paginated_loader_to_entity-database-adapter-knex branch from 0b4f87d to 1218be7 Compare February 11, 2026 03:30
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch from 4439b05 to 9a0ae15 Compare February 11, 2026 03:30
@wschurman wschurman force-pushed the wschurman/02-05-feat_add_paginated_loader_to_entity-database-adapter-knex branch from 1218be7 to 93efc2f Compare February 12, 2026 01:31
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch 2 times, most recently from 2bd993a to a3ffbaf Compare February 12, 2026 03:14
@wschurman wschurman force-pushed the wschurman/02-05-feat_add_paginated_loader_to_entity-database-adapter-knex branch 3 times, most recently from 0829b03 to 351382a Compare February 12, 2026 03:21
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch 2 times, most recently from 56aeab5 to f2efc1c Compare February 12, 2026 03:28
@wschurman wschurman force-pushed the wschurman/02-05-feat_add_paginated_loader_to_entity-database-adapter-knex branch 2 times, most recently from b43bb56 to 7f45f60 Compare February 12, 2026 03:41
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch 2 times, most recently from 1f7172a to 5ec7479 Compare February 12, 2026 04:05
@wschurman wschurman marked this pull request as ready for review February 12, 2026 04:15
@wschurman wschurman requested review from ide and quinlanj February 12, 2026 04:16
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

I am skeptical of this feature for a few reasons. They can be addressed but I'll write them out here:

  1. We don't use this feature currently. If an LLM keeps wanting to add it, we can add a CLAUDE.md saying not to suggest it.
  2. This could be problematically slow depending on the number of rows needed to be scanned. I'm thinking we'd want a way to cap the count. The UX would show the number of pages if it's under, say, 20, but otherwise just show "more".
  3. Refetching the count during pagination is wasteful. Only the initial query (which isn't necessarily for page 1) needs the count, and then incremental loads should not compute it by default.

@wschurman wschurman changed the base branch from wschurman/02-05-feat_add_paginated_loader_to_entity-database-adapter-knex to graphite-base/430 February 12, 2026 19:49
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch from 5ec7479 to 6d88125 Compare February 12, 2026 19:49
@graphite-app graphite-app bot changed the base branch from graphite-base/430 to main February 12, 2026 19:49
@wschurman wschurman force-pushed the wschurman/02-09-feat_add_includetotalcount_to_pagination branch from 6d88125 to d3a2418 Compare February 12, 2026 19:49
Copy link
Member Author

I think we're in agreement. I could imagine in the future exposing a loaderMethod, countForSQLAsync or something so that it is separate from (3). But 1 and 2 are definitely still concerns. We do use count in a few places in the Expo backend (count of apps for an account), but it's definitely not the majority case for paginated interfaces.

Closing for now.

@wschurman wschurman closed this Feb 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments