Skip to content

fix(api): correct GetManyClients paging (#51) - #86

Merged
brinehart merged 1 commit into
epic/must-have-fixesfrom
fix/must-fix-51
Nov 2, 2026
Merged

brinehart merged 1 commit into
epic/must-have-fixesfrom
fix/must-fix-51

Conversation

@brinehart

Copy link
Copy Markdown
Collaborator

Closes #51

What

Fixes paging in GetManyClients (ClientsGrpcService.cs) per .kiro/specs/must-fix-51/spec.md.

  • request.Page is now a 1-based page number (default 1).
  • request.Skip is reused as the page size (default DefaultPageSize = 10).
  • Offset is (page - 1) * pageSize, so consecutive pages no longer overlap.
  • NextUrl is built from the same page/pageSize variables (page={page+1}&skip={pageSize}).
  • Total is set on every response; NextUrl is emitted only when more rows exist beyond the window.

Proto

Comment-only edit to the page/skip field docs in client.proto to match the new meaning. No field added, removed, or renumbered — no schema change, no client regen forced.

Tests

New MicroZen.Api.Test/Services/ClientsGrpcServiceTests.cs (EF Core InMemory), seeding 25 clients:

  1. Page 1 returns the first 10 by name; Total=25; NextUrl has page=2, skip=10.
  2. Page 2 returns 11–20 and is disjoint from page 1; NextUrl has page=3.
  3. Last page (page 3) returns 21–25 with no NextUrl.
  4. Defaults (Page=0, Skip=0) behave as page 1, size 10.
  5. Total is set on every response.

The test project aliases the transitive MicroZen.OAuth2 reference so the duplicated MicroZen.Grpc.V1 proto types resolve unambiguously to MicroZen.Api.

Scope / notes

Verification (from the worktree)

  • npx nx affected -t lint test build — green (16/16 tests).
  • dotnet test core/csharp/MicroZen.Core.sln — green (16/16).
  • npx nx format:check passes for all files in this change; the only flags are pre-existing tracked spec .md files and untracked local .nx/ data, both outside GetManyClients paging defaults are wrong and NextUrl double-counts #51's scope.

@nx-cloud

nx-cloud Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 80be9eb

Command Status Duration Result
nx affected -t lint test build ✅ Succeeded 32s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-11-02 03:48:40 UTC

@brinehart brinehart left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed against .kiro/specs/must-fix-51/spec.md. All acceptance points met.

Paging logic (ClientsGrpcService.cs)

  • page is a 1-based page number, default 1. ✅
  • skip reused as page size, default DefaultPageSize = 10. ✅
  • offset = (page - 1) * pageSize, fed to .Skip(skip); .Take(pageSize). ✅
  • NextUrl rebuilt from the same page/pageSize vars (page={page+1}&skip={pageSize}), consistent with the window. ✅
  • hasMore = skip + clients.Count < total correctly suppresses NextUrl on the last page (replaces the old no-op anyMoreClients guard). ✅
  • Total set on every response, not only when a next page exists. ✅

Proto (client.proto) — comment-only edits to page/skip docs; no field added, removed, or renumbered. No schema break. ✅

Tests (ClientsGrpcServiceTests.cs, 25 seeded clients)

  • Page 1 → first 10 by name, Total=25, NextUrl has page=2/skip=10. ✅
  • Page 2 → clients 11–20, asserted disjoint from page 1. ✅ (proves non-overlapping)
  • Last page (3) → 21–25, no NextUrl. ✅
  • Defaults (Page=0,Skip=0) behave as page 1 size 10. ✅
  • Total set on every response. ✅

Scope — the searchTerm $"%{request}%" bug (#50) is left untouched in the method; the diff does not modify those lines. ✅

CI — gh pr checks 86: dotnet pass, main pass. ✅

No change requests.

@brinehart

Copy link
Copy Markdown
Collaborator Author

ALL-CLEAR -- PR #86 fully satisfies .kiro/specs/must-fix-51/spec.md (1-based page, skip-as-page-size, offset=(page-1)*pageSize, consistent NextUrl, Total always set, proto comment-only, paging-only scope with #50 untouched, non-overlapping page1/page2 tests) and CI checks are green (dotnet + main).

Treat request.Page as a 1-based page number (default 1) and reuse
request.Skip as the page size (default 10). Compute the offset as
(page - 1) * pageSize so consecutive pages no longer overlap, and build
NextUrl from the same page/pageSize variables. Set Total on every
response and emit NextUrl only when more rows exist beyond the window.

Update the client.proto comments for page/skip to match the new meaning
(comment-only, no schema or field-number change).

Add ClientsGrpcServiceTests covering page 1, a non-overlapping page 2,
the last page without NextUrl, default paging, and Total on every
response. Alias the transitive MicroZen.OAuth2 reference in the test
project so the duplicated MicroZen.Grpc.V1 proto types resolve to
MicroZen.Api.

Scope: paging only; the searchTerm filter (#50) is untouched.

Closes #51
@brinehart
brinehart merged commit 68148de into epic/must-have-fixes Nov 2, 2026
2 checks passed
brinehart added a commit that referenced this pull request Nov 2, 2026
Treat request.Page as a 1-based page number (default 1) and reuse
request.Skip as the page size (default 10). Compute the offset as
(page - 1) * pageSize so consecutive pages no longer overlap, and build
NextUrl from the same page/pageSize variables. Set Total on every
response and emit NextUrl only when more rows exist beyond the window.

Update the client.proto comments for page/skip to match the new meaning
(comment-only, no schema or field-number change).

Add ClientsGrpcServiceTests covering page 1, a non-overlapping page 2,
the last page without NextUrl, default paging, and Total on every
response. Alias the transitive MicroZen.OAuth2 reference in the test
project so the duplicated MicroZen.Grpc.V1 proto types resolve to
MicroZen.Api.

Scope: paging only; the searchTerm filter (#50) is untouched.

Closes #51

Co-authored-by: brinehart <ben.rinehart@avifoodsystems.comc>
@brinehart
brinehart deleted the fix/must-fix-51 branch November 2, 2026 04:03
@brinehart brinehart linked an issue Nov 2, 2026 that may be closed by this pull request
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.

GetManyClients paging defaults are wrong and NextUrl double-counts

1 participant