Fix page_n not found error in paginated exports - #25
Conversation
cli_progress_message() was passed .envir = arg$current_env (the user's
calling environment), so the {page_n} glue expression failed on page 2+
because page_n is a local variable inside .paginate_export(), not in the
caller's scope.
Fix: use rlang::current_env() so cli evaluates {page_n} and {arg$form} in
the function's own environment where both are defined.
Fixes DP-1547.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR updates line 127 in ChangesExport Handler Environment Context
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
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.
🧹 Nitpick comments (1)
R/export_handler.R (1)
124-128: ⚡ Quick winAdd a regression test for page-scoped glue interpolation.
This fix is correct, but please add a test that exercises interactive pagination beyond page 1 and asserts the progress message path does not error on
{page_n}interpolation.🤖 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 `@R/export_handler.R` around lines 124 - 128, Add a regression test that simulates interactive pagination past the first page and asserts the progress message interpolation of {page_n} does not error: create a test in testthat that sets arg$option$interactive_mode to TRUE, sets page_n > 1 (e.g. 2), and invokes the code path in R/export_handler.R that calls cli::cli_progress_message (use the same public function that triggers export_handler.R logic); capture or expect the message (expect_message or withCallingHandlers) and assert the message contains the rendered page number (e.g. "Fetching page 2") and that no error is thrown during interpolation of {page_n}, referencing arg$option$interactive_mode, page_n, arg$form and cli::cli_progress_message in the test.
🤖 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.
Nitpick comments:
In `@R/export_handler.R`:
- Around line 124-128: Add a regression test that simulates interactive
pagination past the first page and asserts the progress message interpolation of
{page_n} does not error: create a test in testthat that sets
arg$option$interactive_mode to TRUE, sets page_n > 1 (e.g. 2), and invokes the
code path in R/export_handler.R that calls cli::cli_progress_message (use the
same public function that triggers export_handler.R logic); capture or expect
the message (expect_message or withCallingHandlers) and assert the message
contains the rendered page number (e.g. "Fetching page 2") and that no error is
thrown during interpolation of {page_n}, referencing
arg$option$interactive_mode, page_n, arg$form and cli::cli_progress_message in
the test.
…-1547) Two tests in test-export_handler.R: 1. Positive case: calls cli::cli_progress_message() with the exact format string and .envir = rlang::current_env() from .paginate_export(), asserts no error is thrown, and verifies the rendered text contains "Fetching page 2" and the form name. 2. Negative case: mirrors the broken code path (.envir = arg$current_env where page_n is absent) and asserts the error is produced, confirming that a reversion of the fix would be caught. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- export_extract.R, export_handler.R: replace [.fn()] cross-reference syntax with `.fn()` backtick prose for internal @nord functions; Roxygen cannot resolve links to undocumented topics and emits a warning for each - smartabaseR.R: replace deprecated @doctype package / @name pattern with the "_PACKAGE" sentinel as required by Roxygen >= 7.3 - DESCRIPTION: bump RoxygenNote to 7.3.3 (written automatically by document()) - man/: regenerate smartabaseR-package.Rd, remove stale smartabaseR.Rd Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
3 participants
Summary
6914587) where any paginated export with more than one page of results would fail with! object 'page_n' not found..envir = arg\$current_envto.envir = rlang::current_env()in the page-progress message inside.paginate_export().Root cause
In
.paginate_export(), the progress message shown on page 2+ uses a{page_n}glue expression:Passing
.envir = arg\$current_envtellsclito evaluate{page_n}in the user's calling environment, wherepage_ndoes not exist — it is a local variable inside.paginate_export(). On the first page this block is skipped (page_n > 1LisFALSE), so single-page exports worked fine. Any multi-page result set crashed immediately when fetching page 2.The fix uses
rlang::current_env()so bothpage_nandarg\$formare evaluated in the function's own scope where both are defined.Test plan
Availabilityform with a 6-year date range (confirmed! object 'page_n' not foundon page 2)devtools::test())Reviewer notes
Only one line changed. No behaviour change beyond unblocking multi-page exports. The
arg\$current_envpattern used in otherclicalls throughout the file remains correct because those messages only referencearg\$...fields, which are accessible via the call stack —page_nis the only purely-local variable used in a progress format string.Resolves DP-1547.
🤖 Generated with Claude Code
Summary by CodeRabbit