feat(pagination): preserve pagination state on truncation and natural…#659
feat(pagination): preserve pagination state on truncation and natural…#659sang-neo03 merged 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughmergePagedResults now derives the merged Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/client/client_test.go (1)
239-276: Natural-end test correctly exercises the deletion branch.The fixture flips
has_moretofalseon the second call and omitspage_token, validating thatmergePagedResultsclears the token when the API reports no more pages.Optional nit: consider also asserting
len(data["items"]) == 2to lock in that natural-end merging still aggregates items across both pages — pure pagination-state focus is fine, but a one-line items-count check would catch regressions in the merge loop while you're already exercising it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client_test.go` around lines 239 - 276, Add an assertion in TestPaginateAll_NaturalEndClearsPageToken to also verify that the merged items list actually contains both pages' items (e.g., assert len(data["items"]) == 2) so the test not only checks page_token clearing but also confirms mergePagedResults aggregated items across both calls; locate this in the test after extracting resultMap/data and add a single-line length check against data["items"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/client/client_test.go`:
- Around line 239-276: Add an assertion in
TestPaginateAll_NaturalEndClearsPageToken to also verify that the merged items
list actually contains both pages' items (e.g., assert len(data["items"]) == 2)
so the test not only checks page_token clearing but also confirms
mergePagedResults aggregated items across both calls; locate this in the test
after extracting resultMap/data and add a single-line length check against
data["items"].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d03758d1-e177-4bff-a726-71c5425ce091
📒 Files selected for processing (2)
internal/client/client_test.gointernal/client/pagination.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
==========================================
+ Coverage 62.67% 62.95% +0.27%
==========================================
Files 434 436 +2
Lines 38072 38597 +525
==========================================
+ Hits 23862 24299 +437
- Misses 12076 12150 +74
- Partials 2134 2148 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2636502d694c639d1a7ac14aa5abb36ee120f3fe🧩 Skill updatenpx skills add larksuite/cli#fix/page-all-has-more -y -g |
2 participants
Summary
Fix
--page-allsilently truncating data at the default--page-limit=10: merged responses hard-codedhas_more=false, so downstream consumers couldn't detect truncation (e.g. awiki node with 705 children returned 200 items with
has_more=false, silently losing 505 rows). The merge now surfaces the last page's realhas_more, letting callers detecttruncation and re-run with a larger
--page-limit.Changes
internal/client/pagination.go: replace the hard-codedhas_more=falseinmergePagedResultswith the real value from the last fetched page. Page tokens are still dropped fromthe merged view — the aggregate is not a resume cursor; callers should re-run with a larger
--page-limitto fetch more.internal/client/client_test.go: extendTestPaginateAll_PageLimitStopsPaginationto assert truncation now surfaceshas_more=trueand thatpage_tokenis dropped from theaggregate; add
TestPaginateAll_NaturalEndClearsPageTokento lock in the clean-exit path.Test Plan
go test ./internal/client/... ./cmd/api/... ./cmd/service/...)go build ./...passeslark-cli wiki spaces list --as user --page-all --page-limit 2 --format jsonworks as expected — old binary (1.0.17) returnshas_more=false; patched binary returnshas_more=true;itemscount is identical (40) on both.Related Issues
--page-allsilently truncates data at default--page-limit=10and falsely reportshas_more=false#590Summary by CodeRabbit
Release Notes
Bug Fixes
Tests