Skip to content

Honor ledger API offset pagination#867

Open
laughlife wants to merge 2 commits into
ramimbo:mainfrom
laughlife:codex/ledger-api-offset-799
Open

Honor ledger API offset pagination#867
laughlife wants to merge 2 commits into
ramimbo:mainfrom
laughlife:codex/ledger-api-offset-799

Conversation

@laughlife
Copy link
Copy Markdown

@laughlife laughlife commented Jun 4, 2026

Summary

  • Add canonical bounded offset support to GET /api/v1/ledger.
  • Apply the offset in recent_ledger_entries() after the newest-first ordering.
  • Document limit + offset pagination for the ledger API.
  • Add regression coverage for limit=1&offset=1, offset=0, negative/oversized offsets, non-canonical offset=01, and repeated offset values.

Source report

Fixes the production issue reported in #798: #798 (comment)

Current production evidence before this fix:

https://api.mrwk.online/api/v1/ledger?limit=1 -> rows=1 seqs=[1178]
https://api.mrwk.online/api/v1/ledger?limit=1&offset=1 -> rows=1 seqs=[1178]
https://api.mrwk.online/api/v1/ledger?offset=1 -> rows=50 seqs=[1178,1177,1176]

offset is silently ignored today. With this change, limit=1&offset=1 returns the second newest ledger entry instead of the newest one again.

Validation

  • uv run --extra dev pytest tests/test_api_mcp.py::test_ledger_api_honors_canonical_offset -q
  • uv run --extra dev pytest tests/test_api_mcp.py tests/test_ledger_views.py tests/test_docs_public_urls.py -q
  • uv run --extra dev ruff check app/main.py app/ledger_views.py tests/test_api_mcp.py docs/api-examples.md
  • uv run --extra dev ruff format --check app/main.py app/ledger_views.py tests/test_api_mcp.py
  • uv run --extra dev mypy app/main.py app/ledger_views.py
  • uv run --extra dev python scripts/docs_smoke.py
  • git diff --check
  • git merge-tree origin/main HEAD

Summary by CodeRabbit

  • New Features

    • Ledger API now supports offset-based pagination and returns entries in newest-first order.
    • Query parameters for paging are strictly validated (non-negative, canonical integers, no duplicates).
  • Documentation

    • API examples updated to show how to page results using limit (1–200) and offset.
  • Tests

    • Added tests covering pagination behavior and input validation for offset.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a722dfda-47e3-452e-ac6e-f408abce4e77

📥 Commits

Reviewing files that changed from the base of the PR and between 2531f4b and cb6c425.

📒 Files selected for processing (1)
  • tests/test_api_mcp.py

📝 Walkthrough

Walkthrough

The PR adds offset-based pagination to the newest-first ledger list: data access accepts offset, the API validates and forwards limit+offset, docs show usage, and tests check valid pages and invalid offset inputs.

Changes

Ledger offset pagination

Layer / File(s) Summary
Ledger pagination foundation
app/ledger_views.py
recent_ledger_entries function signature adds an offset parameter (defaulting to 0) and the SQL query applies both limit and offset when ordering by descending sequence.
API endpoint offset pagination
app/main.py
GET /api/v1/ledger endpoint adds offset query parameter validation (rejecting repeated and non-canonical values) and introduces a ledger_rows(limit, offset) helper that calls the updated data layer function.
Documentation and test
docs/api-examples.md, tests/test_api_mcp.py
API examples show the offset parameter and pagination guidance for acceptable limit range and canonical non-negative offset; new test validates offset=0 and offset=1 behavior and confirms rejection of invalid offset inputs (negative, oversized, non-canonical, repeated).

Possibly related PRs

  • ramimbo/mergework#366: Extracted recent_ledger_entries with limit-only pagination; this PR extends it with offset support for complete cursor-like pagination.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically names the changed surface—ledger API offset pagination—matching the PR's main objective of adding offset support.
Description check ✅ Passed The description covers the required sections: summary of changes, evidence of the problem and validation steps, and the motivation (issue #798). All test evidence checkboxes are documented via explicit validation commands.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR only modifies API implementation and tests; no changes to README or public artifacts. No investment, price, cash-out, or fabricated payout claims present in modified code or documentation.
Bounty Pr Focus ✅ Passed PR references issues #798/#799, diff matches stated files, includes comprehensive regression test with validation coverage, and maintains focused scope without unrelated changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 32c08e9c-f8f0-4ee8-a1a1-27e5f6d74709

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 2531f4b.

📒 Files selected for processing (4)
  • app/ledger_views.py
  • app/main.py
  • docs/api-examples.md
  • tests/test_api_mcp.py

Comment thread tests/test_api_mcp.py
Copy link
Copy Markdown
Contributor

@xiefuzheng713-alt xiefuzheng713-alt left a comment

Choose a reason for hiding this comment

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

I reviewed the current head for ledger offset pagination and found it aligned with the bounty scope.

I checked:

  • app/ledger_views.py:
    ecent_ledger_entries(session, limit, offset) now applies .offset(offset) after ordering by LedgerEntry.sequence.desc().
  • app/main.py: /api/v1/ledger now accepts offset with explicit query validation: repeated param rejection, canonical-int rejection, and passes through to the data access helper.
  • docs/api-examples.md: usage snippet now shows limit + offset paging.
  • tests/test_api_mcp.py: added regression coverage for:
    • offset=1 paging behavior (second item)
    • offset=0 equivalence
    • invalid offset cases: negative, overflow, non-canonical, repeated

The query contract in this PR now matches the style already used on other bounded list endpoints. I did not see functional regressions in this change set.

Copy link
Copy Markdown

@tudorian95 tudorian95 left a comment

Choose a reason for hiding this comment

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

Reviewed current head cb6c425279fbc3d73177e8a6aedf029b34b0e251 for Bounty #838.

What I checked:

  • Inspected the focused diff in app/main.py, app/ledger_views.py, tests/test_api_mcp.py, and docs/api-examples.md.
  • Confirmed the live production before-state is still reproducible with harmless GETs: GET /api/v1/ledger?limit=1 returned sequence 1178, GET /api/v1/ledger?limit=1&offset=1 also returned 1178, while GET /api/v1/ledger?limit=2 returned 1178,1177.
  • In Docker/uv on a fresh checkout of this PR head:
    • uv run --extra dev pytest tests/test_api_mcp.py::test_ledger_api_honors_canonical_offset -q -> 1 passed, 1 existing Starlette/httpx warning.
    • uv run --extra dev pytest tests/test_api_mcp.py tests/test_ledger_views.py tests/test_docs_public_urls.py -q -> 143 passed, 1 existing warning.
    • uv run --extra dev ruff check app/main.py app/ledger_views.py tests/test_api_mcp.py docs/api-examples.md -> passed.
    • uv run --extra dev ruff format --check app/main.py app/ledger_views.py tests/test_api_mcp.py -> passed.
    • uv run --extra dev mypy app/main.py app/ledger_views.py -> success.
    • uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.
    • git diff --check -> clean.
  • git merge-tree --write-tree origin/main HEAD succeeded with tree 5346d2a17c095c2271e9eb4c968f620bcb09450a.
  • GitHub reports the current head as MERGEABLE.

The change keeps existing newest-first ledger behavior while threading a bounded canonical offset through the read-only list endpoint and its query helper. I did not find blockers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants