Skip to content

Conversation

@tolik0
Copy link
Contributor

@tolik0 tolik0 commented Nov 12, 2025

Summary by CodeRabbit

  • Refactor

    • Improved pagination logic to better track stream slice context during range reduction, enhancing accuracy in complex pagination scenarios.
  • Tests

    • Updated pagination tracker tests to align with refined slice reduction behavior and verify correct slice handling.

Copilot AI review requested due to automatic review settings November 12, 2025 19:37
@tolik0 tolik0 self-assigned this Nov 12, 2025
@tolik0 tolik0 requested review from maxi297 and removed request for Copilot November 12, 2025 19:37
@github-actions github-actions bot added the bug Something isn't working label Nov 12, 2025
@github-actions
Copy link

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@tolik0/reset-pagination/fix-split-using-cursor#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch tolik0/reset-pagination/fix-split-using-cursor

Helpful Resources

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment

📝 Edit this welcome message.

@github-actions
Copy link

PyTest Results (Fast)

3 812 tests  +1   3 801 ✅ +1   7m 2s ⏱️ -10s
    1 suites ±0      11 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit a246bac. ± Comparison against base commit f0443aa.

@github-actions
Copy link

PyTest Results (Full)

3 815 tests  +1   3 803 ✅ +1   11m 6s ⏱️ +2s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit a246bac. ± Comparison against base commit f0443aa.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

📝 Walkthrough

Walkthrough

The PaginationTracker.reduce_slice_range_if_possible method signature was updated to accept two stream slice parameters—previous_stream_slice and original_stream_slice—instead of one. Corresponding implementations in SimpleRetriever and test cases were updated to pass both parameters, enabling the pagination logic to differentiate between original and previous slice contexts.

Changes

Cohort / File(s) Summary
Core implementation
airbyte_cdk/sources/declarative/retrievers/pagination_tracker.py
Updated method signature to accept previous_stream_slice and original_stream_slice parameters; moved FailureType import from airbyte_cdk.sources.declarative.models to airbyte_cdk.models; refined logic to use original slice for reduction decisions when cursor is present and compare retry counter against previous slice instead of input slice; added parameter docstring.
Consumer update
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py
Modified _read_pages to capture the original stream_slice at method start; updated call to reduce_slice_range_if_possible to pass both stream_slice (previous) and original_stream_slice parameters instead of a single argument.
Test updates
unit_tests/sources/declarative/retrievers/test_pagination_tracker.py
Updated all test call sites to pass both current_slice and original_slice parameters to reduce_slice_range_if_possible; added new test verifying cursor is invoked with the original slice during range reduction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • pagination_tracker.py requires careful attention to the dual-slice logic—particularly how original_stream_slice is used for reduction decisions versus previous_stream_slice for retry comparison, and whether the state reset behavior is correct.
  • simple_retriever.py needs verification that original_stream_slice is correctly captured at the method entry point and passed through the pagination reset flow without mutation.
  • Test coverage should confirm the new dual-parameter behavior is exercised across different cursor and retry scenarios, especially the newly added test for cursor invocation with the original slice.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly references the main change: fixing pagination reset logic to properly use cursor information when splitting slices.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tolik0/reset-pagination/fix-split-using-cursor

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f0443aa and a246bac.

📒 Files selected for processing (3)
  • airbyte_cdk/sources/declarative/retrievers/pagination_tracker.py (2 hunks)
  • airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (2 hunks)
  • unit_tests/sources/declarative/retrievers/test_pagination_tracker.py (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-18T23:40:06.391Z
Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Applied to files:

  • unit_tests/sources/declarative/retrievers/test_pagination_tracker.py
🧬 Code graph analysis (3)
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (1)
airbyte_cdk/sources/declarative/retrievers/pagination_tracker.py (1)
  • reduce_slice_range_if_possible (49-77)
airbyte_cdk/sources/declarative/retrievers/pagination_tracker.py (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
  • FailureType (546-549)
airbyte_cdk/sources/types.py (1)
  • StreamSlice (75-169)
airbyte_cdk/sources/streams/concurrent/cursor.py (1)
  • reduce_slice_range (541-567)
unit_tests/sources/declarative/retrievers/test_pagination_tracker.py (3)
airbyte_cdk/sources/declarative/retrievers/pagination_tracker.py (1)
  • reduce_slice_range_if_possible (49-77)
airbyte_cdk/sources/types.py (3)
  • StreamSlice (75-169)
  • partition (99-104)
  • cursor_slice (107-112)
airbyte_cdk/sources/streams/concurrent/cursor.py (1)
  • reduce_slice_range (541-567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: Check: source-intercom
  • GitHub Check: Check: source-pokeapi
  • GitHub Check: Check: destination-motherduck
  • GitHub Check: Check: source-shopify
  • GitHub Check: Check: source-hardcoded-records
  • GitHub Check: Pytest (All, Python 3.12, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.13, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Manifest Server Docker Image Build
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Analyze (python)

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

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

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

LGTM!

@tolik0 tolik0 merged commit 0ac72f5 into main Nov 13, 2025
30 of 31 checks passed
@tolik0 tolik0 deleted the tolik0/reset-pagination/fix-split-using-cursor branch November 13, 2025 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants