Resolve #313: paginationLimitExceeded carries accumulated records

[leogdion]

Summary

Resolves #313 ("MaxPage Error Should Return Results too."), which spun out of review comment #r3210049916 on queryAllRecords. When MistKit's auto-paginating helpers hit their maxPages cap, they used to throw an error that dropped the partial result. Callers had to start over or accept silent data loss. Now the error carries every record collected before the cap so callers can resume or surface partial results.

• Breaking — CloudKitError.paginationLimitExceeded: case shape changes from (maxPages: Int, recordsCollected: Int) to (maxPages: Int, records: [RecordInfo]). errorDescription keeps the same wording via records.count.
• queryAllRecords throws with records: allRecords. Docstring corrected — it previously claimed the throw was .invalidResponse.
• fetchAllRecordChanges gains an explicit maxPages: parameter (default 1 000, was a hard-coded local) and now throws .paginationLimitExceeded on overflow instead of .invalidResponse. Existing callers continue to work — maxPages is defaulted.
• Tests in CloudKitServiceQueryPagination/+ErrorCases.swift and CloudKitServiceFetchChanges/+ErrorHandling.swift drive the overflow path with maxPages: 2 and assert the carried records.
• ReleaseNotes.md flags this under v1.0.0-beta.1.

The PR #298 review (which inspired the work) was largely addressed in commit 7a5da7a already; verified by reading the current files. The one nit left was the queryAllRecords maxPages docstring claiming .invalidResponse — fixed here next to the code change.

Test plan

• swift build from repo root — clean (only pre-existing import warnings)
• swift test — 483 MistKit tests pass (incl. two new overflow tests)
• cd Examples/MistDemo && swift test — 901 tests pass
• cd Examples/CelestraCloud && swift build — clean (its .paginationLimitExceeded switch case has no value bindings, so the shape change is invisible)
• cd Examples/BushelCloud && swift build — clean
• ./Scripts/lint.sh — passes

🤖 Generated with Claude Code

---

Perform an AI-assisted review on

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9b3a7bad-c842-4c2a-bea1-2db50b5df7ac

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 313-maxpage-error

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review: PR #326 — paginationLimitExceeded carries accumulated records

Overview

This PR fixes a real data-loss problem: when queryAllRecords / fetchAllRecordChanges hit their page cap, they previously threw an error that silently dropped every record already collected. The fix promotes the error payload from an Int count to the actual [RecordInfo] slice, so callers can recover partial results. fetchAllRecordChanges also gains an explicit maxPages: parameter (was a hard-coded local) to match the API surface of queryAllRecords.

---

What's good

• The change is well-motivated. Silent data loss on overflow is a nasty failure mode; carrying the records out in the error is the right call.
• Consistent API. Both auto-paginating helpers now share the same maxPages: parameter, default (1,000), and error contract.
• No accidental breakage in examples. CelestraError.swift:147 matches paginationLimitExceeded without binding, so the case-shape change is invisible there — correctly noted in the PR description.
• Tests cover exactly what matters. Both new tests drive the overflow path with maxPages: 2, enqueue three pages of staged responses, and assert the content (not just the count) of the returned records. That's solid.
• Loop semantics are correct. The guard pageCount < maxPages placement fires after 2 successful fetches, so records.count == 5 (3 + 2) is the right expectation.
• Stuck-token detection in fetchAllRecordChanges mirrors the stuck-marker detection already in queryAllRecords — good symmetry.
• Breaking change is documented clearly in ReleaseNotes.md under Error Handling, with a link to the issue.

---

Concerns and suggestions

1. Memory pressure at the error site (worth documenting)

Carrying [RecordInfo] in the thrown error means up to maxPages x resultsLimit fully-materialized records stay alive until the error is discarded. At the defaults (1,000 pages x 200 records) that is 200,000 RecordInfo objects — significant on a server. The existing - Warning: blocks in the docstrings are a good place to add one line:

/// - Warning: When `maxPages` is exceeded the thrown error carries every
///   record collected so far, which can be large. Extract or release the
///   error promptly to avoid holding excess memory.

This is documentation only — no logic change needed.

2. RecordManaging protocol docstring is out of date

Sources/MistKit/Protocols/RecordManaging.swift:65 — the queryAllRecords(recordType:) declaration says only Throws: CloudKit errors if the query fails. The concrete CloudKitService override can now throw .paginationLimitExceeded(maxPages:records:). The protocol-level doc should mention it:

/// - Throws: CloudKit errors, including `.paginationLimitExceeded(maxPages:records:)`
///           when the auto-paginating implementation reaches its page cap.

3. Minor: per-page-local record names in tests are correct but surprising

Both overflow tests assert that record names repeat across pages (e.g. ["record-0", "record-1", "record-2", "record-0", "record-1"]) because the mock generates indices starting at 0 per page. A one-line comment near the assertion would make this immediately clear to a future reader rather than looking like a copy-paste error.

4. Pre-existing gap: mid-pagination invalidResponse throw drops records (not a blocker)

CloudKitService+SyncOperations.swift:201-203:

if result.moreComing && result.syncToken == nil {
    throw CloudKitError.invalidResponse
}

This still drops allRecords on the floor. It is a pre-existing edge case and out of scope for this PR, but since we are now consistently carrying partial results it would be worth tracking in a follow-up issue.

---

Summary

The core change is correct and the tests prove it. Suggestions 1 and 2 are documentation-only and would improve the public API before the beta ships. Suggestion 3 is a nice-to-have. Suggestion 4 is a pre-existing gap worth a follow-up issue.

Approve once the doc nits are addressed (or noted as follow-up issues).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.45%. Comparing base (7a5da7a) to head (5383333).

Files with missing lines	Patch %	Lines
...Kit/Service/ResponseProcessing/CloudKitError.swift	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           v1.0.0-beta.1     #326       +/-   ##
==================================================
+ Coverage          51.25%   69.45%   +18.19%     
==================================================
  Files                128      529      +401     
  Lines               3533    14457    +10924     
==================================================
+ Hits                1811    10041     +8230     
- Misses              1722     4416     +2694     

Flag	Coverage Δ
mistdemo-spm-macos	58.64% <0.00%> (?)
mistdemo-swift-6.2-jammy	58.65% <0.00%> (?)
mistdemo-swift-6.2-noble	58.63% <0.00%> (?)
mistdemo-swift-6.3-jammy	58.65% <0.00%> (?)
mistdemo-swift-6.3-noble	58.65% <0.00%> (?)
spm	51.28% <81.81%> (+0.02%)	⬆️
swift-6.1-jammy	51.31% <81.81%> (?)
swift-6.1-noble	51.31% <81.81%> (?)
swift-6.2-jammy	51.37% <81.81%> (?)
swift-6.2-noble	51.71% <81.81%> (?)
swift-6.3-jammy	51.34% <81.81%> (?)
swift-6.3-noble	51.37% <81.81%> (?)

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.

[claude]

Code Review

Overview

This PR resolves a real usability gap: when auto-paginating helpers hit maxPages, the partial result was silently dropped. Callers now receive every record accumulated before the cap via the error payload, enabling resume or surface-partial-results patterns. The implementation is clean and consistent across both queryAllRecords and fetchAllRecordChanges.

---

Breaking Change

CloudKitError.paginationLimitExceeded changes from (maxPages: Int, recordsCollected: Int) to (maxPages: Int, records: [RecordInfo]). Any exhaustive switch or pattern-match on this case will fail to compile. This is correctly flagged in the PR description but worth a callout:

• Consumers upgrading need to update every case .paginationLimitExceeded(let maxPages, let recordsCollected) binding
• CelestraCloud and BushelCloud are confirmed unaffected because neither binds the associated values — good validation step

---

Loop Refactor in fetchAllRecordChanges — Correct and Cleaner

The while moreComing / moreComing = true hack is replaced by repeat { } while moreComing:

// Old: required a sentinel true to force the first iteration
var moreComing = true
while moreComing { … }

// New: semantically correct — always execute once, then check the condition
var moreComing = false
repeat { … } while moreComing

This is a genuine improvement. moreComing = false accurately reflects "we don't know yet", and repeat { } while makes the intent clear without the sentinel. The guard pageCount < maxPages at the top of the block preserves the maxPages: 0 short-circuit behavior.

---

Error Normalisation in fetchAllRecordChanges

Previously this function threw CloudKitError.invalidResponse on overflow — semantically wrong (there's nothing invalid about the response; the caller just asked for too much). Replacing it with .paginationLimitExceeded is a correctness fix. Callers who were catching .invalidResponse for the overflow case will need to update, but that was always wrong to rely on.

---

Memory Consideration

[RecordInfo] inside an error value is a heap allocation that lives as long as the error is retained. With the 1,000-page default at up to 200 records/page, a worst-case error can carry ~200,000 RecordInfo values. This is acceptable — any caller that hit the cap already had all those records in memory — but worth noting in the summary doc so callers don't log the full error object naively. The existing SecureLogging / redaction infrastructure doesn't cover this automatically since the records are embedded in the error description count, not the records themselves.

---

Test Quality

Both new tests are well-structured:

• maxPages: 2 with three queued pages is the minimal scenario that exercises the cap — good choice
• Assertions on records.count and records.map(\.recordName) verify accumulation, not just the error type
• The repeated "record-0", "record-1" names across pages (both pages generate names starting from 0) are correct given per-page mock generation — a brief inline comment explaining why names repeat would help future readers who might think it's a copy-paste mistake

Minor nit: The QueryPagination+ErrorCases.swift test doesn't enqueue a third page response, relying on the default. A comment noting that the third page is intentionally never reached (the cap fires before it's needed) would make the intent explicit.

---

Documentation

All doc-comment updates are accurate:

• queryAllRecords: correctly changes .invalidResponse → .paginationLimitExceeded and documents the records payload
• fetchAllRecordChanges: removes the "(or if the page count exceeds 1000)" clause from -Warning: since that case is now documented in -Throws:
• maxPages parameter doc is added consistently in both methods

---

Sendable Compliance

RecordInfo is public struct RecordInfo: Codable, Sendable, so embedding [RecordInfo] in CloudKitError: Sendable is safe — no implicit conformance gap.

---

Summary

Area	Verdict
Correctness	✅ Both loop refactor and error propagation are correct
Breaking change	⚠️ Acknowledged — semver bump required
Memory	ℹ️ Expected tradeoff, worth a note in release notes
Tests	✅ Cover the new path with precise assertions
Docs	✅ Accurate and consistent

Overall this is a solid, well-scoped change. The one concrete suggestion: add a short comment in the new test explaining why both pages emit record-0 … record-N (per-page generation resets the counter), so future readers don't flag it as a copy-paste error.

🤖 Generated with Claude Code