Add transparent cursor-based pagination for eventsearch, filteredeven… - #24
Merged
Conversation
…tsearch, and synchronise All three AMS endpoints that support cursor-based pagination now automatically fetch all pages and return a single combined tibble. No API-breaking changes — existing user code works without modification. Key changes: - export_body.R: inject `paginate = TRUE` and conditional cursor into request bodies; synchronise uses a nested `pagination` object while eventsearch/filteredeventsearch use top-level fields - export_extract.R: add .extract_cursor() to pull nextCursor / pagination.cursor from responses; uses check_type = FALSE for AMS's missing Content-Type header - export_flatten.R: filter to the `events` array before gather_array() to avoid a tidyjson crash on the sibling `nextCursor` string key - export_handler.R: add .paginate_export() loop, .combine_paginated_pages() to reconstruct sb_df attributes across pages, and branch .export_handler() to use the new loop for paginating endpoints only Documents the known server-side eventsearch/filteredeventsearch cursor bug (strict > comparison orphans ~1 event per page boundary) — tracked in AMS-7144. The synchronise endpoint is unaffected; our implementation is correct per protocol. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cursor-based pagination across export flows: body builders accept cursors, a new extractor reads endpoint-specific cursors, flattening handles event arrays correctly, the handler paginates and combines pages, and integration tests are updated to new test columns. ChangesExport pagination implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@R/export_handler.R`:
- Around line 106-133: The pagination repeat loop can spin forever if the API
returns an identical cursor; modify the loop in export_handler.R (the block that
increments page_n, calls .build_export_body/.make_request/.extract_cursor and
appends page_result) to track the previous cursor (e.g. last_cursor) and detect
if cursor == last_cursor (treat as failure), and also enforce a configurable
page ceiling (use an option like arg$option$max_pages or a sensible default) to
break and raise an error if exceeded; on detection, stop further requests and
raise a clear error message indicating non-advancing cursor or max page limit
reached so callers can handle it.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a642cd1b-c119-4f78-9b5b-4e55b5c83a66
📒 Files selected for processing (4)
R/export_body.RR/export_extract.RR/export_flatten.RR/export_handler.R
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/testthat/test-api_integration.R (1)
209-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the final value-set assertion to avoid false positives.
Line 213 currently allows passes when expected values are missing (subset check only). Use exact set equality to assert the intended outcome.
Proposed fix
- expect_true(all(unique_values %in% c(123, 999, 543))) + expect_setequal(unique_values, c(123, 999, 543))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/testthat/test-api_integration.R` around lines 209 - 214, The test currently computes unique_values from check_upsert_results and then uses expect_true(all(unique_values %in% c(123, 999, 543))) which allows missing expected values; replace that subset-style assertion with an exact set equality check (e.g., use testthat::expect_setequal(unique_values, c(123, 999, 543)) or compare sorted vectors with expect_equal(sort(unique_values), sort(c(123, 999, 543)))) so the test fails if any expected value is missing or any unexpected value is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/testthat/test-api_integration.R`:
- Around line 209-214: The test currently computes unique_values from
check_upsert_results and then uses expect_true(all(unique_values %in% c(123,
999, 543))) which allows missing expected values; replace that subset-style
assertion with an exact set equality check (e.g., use
testthat::expect_setequal(unique_values, c(123, 999, 543)) or compare sorted
vectors with expect_equal(sort(unique_values), sort(c(123, 999, 543)))) so the
test fails if any expected value is missing or any unexpected value is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6b46ecd-cced-490e-ad5b-3cabb4a499b4
📒 Files selected for processing (1)
tests/testthat/test-api_integration.R
|
Lgtm |
rparastw
approved these changes
May 27, 2026
Add two safety checks to .paginate_export() in response to PR review: 1. Duplicate-cursor detection: if the server returns the same cursor on consecutive pages (a stuck loop), abort immediately with a clear error rather than looping indefinitely. 2. Page ceiling: abort with a clear error if page_n exceeds max_pages (default 1000; overridable via arg$option$max_pages). Prevents runaway requests if duplicate-cursor detection is somehow bypassed. Both error paths call clear_progress_id() first to clean up CLI state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
rparastw
approved these changes
May 27, 2026
rpparas
approved these changes
Jun 2, 2026
This was referenced Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds automatic, transparent cursor-based pagination to the three AMS endpoints that support it. No breaking changes — existing user code works without modification.
sb_get_event()(eventsearch/filteredeventsearch) now fetches all pages automaticallysb_sync_event()(synchronise) now fetches all pages, accumulatingdeleted_event_idacross pages and takingnew_sync_timefrom the last pageprofilesearch,usersearch,groupmembers,listgroups) are unchangedChanges
export_body.R: injectpaginate = TRUEand conditional cursor into request bodies;synchroniseuses a nestedpaginationobject,eventsearch/filteredeventsearchuse top-level fieldsexport_extract.R: add.extract_cursor()to pullnextCursor/pagination.cursorfrom responsesexport_flatten.R: filter to theeventsarray beforegather_array()to avoid a tidyjson crash on the siblingnextCursorstring keyexport_handler.R: add.paginate_export()loop and.combine_paginated_pages()to reconstructsb_dfattributes across pagesKnown server-side limitation
The
eventsearch/filteredeventsearchcursors use a strict>comparison that orphans ~1 event per page boundary. This is a server-side bug tracked in AMS-7144 — our implementation is correct per the documented protocol.synchroniseis unaffected.Test plan
testthatsuite passes (4 pre-existing network failures unrelated to this change)sb_get_event()returns same row count as non-paginated baselinesb_sync_event()returns correctnew_sync_timeattribute and accumulatesdeleted_event_idacross pagesclass(result)includes"sb_df"on paginated results🤖 Generated with Claude Code
EOF
Summary by CodeRabbit