Bounty #427: Add ledger row limit controls#490
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds ledger page pagination via a validated ChangesLedger pagination feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Reviewed PR #490 against bounty #427. Evidence: inspected Local verification note: I attempted focused pytest runs, but the local checkout currently reports Minor non-blocking suggestion: consider adding an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da651210-f202-4275-bd93-af61a942e2bc
📒 Files selected for processing (4)
app/public_routes.pyapp/static/style.cssapp/templates/ledger.htmltests/test_api_mcp.py
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
I reviewed current head 307cb1582c9b9388caa8ef34256f3152be05badf and I think this needs two small fixes before merge.
-
app/static/style.css: the new.ledger-toolbar buttonrule does not override the later globalinput, textarea, button { width: 100%; }, so the toolbar submit button inherits full width instead of staying inline with the select and JSON link. Add an explicit toolbar override such aswidth: autoor scope the global form-button rule away from the toolbar. -
tests/test_api_mcp.py: the page route is capped atlimit <= 200, but the new regression only asserts the lower rejection case (limit=0). Please add an upper-bound assertion for/ledger?limit=201so the UI/page contract is pinned on both sides of the accepted range.
Validation I ran on this head:
MERGEWORK_DATABASE_URL=sqlite:////tmp/mergework-pr490-test.sqlite3 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py::test_ledger_page_exposes_limit_controls tests/test_api_mcp.py::test_ledger_api_rejects_out_of_range_limits -q-> 4 passedMERGEWORK_DATABASE_URL=sqlite:////tmp/mergework-pr490-test2.sqlite3 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py -q-> 77 passedMERGEWORK_DATABASE_URL=sqlite:////tmp/mergework-pr490-full.sqlite3 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q-> 415 passeduv run --extra dev ruff check app/public_routes.py tests/test_api_mcp.py-> passeduv run --extra dev ruff format --check app/public_routes.py tests/test_api_mcp.py-> already formatteduv run --extra dev python -m mypy app-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangitleaks detect --no-banner --redact --source . --log-opts origin/main..HEAD --exit-code 1-> no leaks found
No secrets or private data were used.
|
Pushed follow-up Changes:
Validation:
Post-push GitHub CI and CodeRabbit are still pending at readback. |
|
Current-head review for PR #490 at I reviewed the ledger limit controls after the follow-up commit. The two issues called out in the earlier stale-head review appear addressed: Files inspected: Validation I ran locally:
Assessment: this is a useful #427 site/workflow improvement. It is small, test-backed, reuses the existing API limit contract, and makes the public ledger easier to inspect without widening the API surface or adding MRWK price/liquidity claims. |
jtc268
left a comment
jtc268
left a comment
There was a problem hiding this comment.
Reviewed current head 82f18c324206d62008495bc52ba2f30326b48006.
The earlier change-request items are addressed: .ledger-toolbar button { width: auto; } overrides the global full-width button rule, and the regression now checks /ledger?limit=201 as well as the lower bound. I also checked that /ledger passes the bounded limit through to api_ledger(limit) and that the JSON link preserves the selected limit.
Validation:
../mergework/.venv/bin/python -m pytest tests/test_api_mcp.py::test_ledger_page_exposes_limit_controls tests/test_api_mcp.py::test_ledger_page_highlights_bounty_payment_and_release tests/test_api_mcp.py::test_ledger_page_uses_wrapping_entry_cards -q-> 3 passed../mergework/.venv/bin/python -m pytest tests/test_api_mcp.py -q-> 77 passed../mergework/.venv/bin/python -m ruff check app/public_routes.py tests/test_api_mcp.py-> passed../mergework/.venv/bin/python -m ruff format --check app/public_routes.py tests/test_api_mcp.py-> 2 files already formatted../mergework/.venv/bin/python -m mypy app/public_routes.py-> success../mergework/.venv/bin/python scripts/docs_smoke.py-> docs smoke okgit diff --check-> clean
Approving.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 82f18c3 against refreshed origin/main.
Verdict: request changes.
The ledger limit implementation itself still validates locally: the page exposes bounded row controls, preserves custom valid limits, links to the matching JSON endpoint, and the prior toolbar width / upper-bound test follow-up is present. However, the branch is no longer merge-ready against current main because merge-tree reports a content conflict in app/public_routes.py.
Validation on this head:
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_ledger_page_exposes_limit_controls tests\test_api_mcp.py::test_ledger_api_rejects_out_of_range_limits tests\test_api_mcp.py::test_ledger_page_highlights_bounty_payment_and_release tests\test_api_mcp.py::test_ledger_page_uses_wrapping_entry_cards -q -> 6 passed
- ..venv\Scripts\python.exe -m ruff check app\public_routes.py tests\test_api_mcp.py -> passed
- ..venv\Scripts\python.exe -m ruff format --check app\public_routes.py tests\test_api_mcp.py -> 2 files already formatted
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok
- git diff --check origin/main...HEAD -> clean
- git merge-tree --write-tree origin/main HEAD -> conflict in app/public_routes.py
Required follow-up: rebase/merge current main and resolve app/public_routes.py, then rerun the focused ledger tests. I do not see a feature-level blocker beyond the stale/conflicting base.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head 4a64000f86432740a7e35aeeade9adbb5959395f after the upstream merge.
Verdict: approve.
The prior merge-readiness blocker is resolved: git merge-tree --write-tree origin/main HEAD now succeeds. The ledger row-limit feature still looks focused and useful: /ledger accepts the bounded limit query, passes it through to the existing ledger API path, exposes common row-count options plus custom valid limits, keeps the JSON link in sync, and includes the toolbar width / upper-bound regression follow-up.
Validation:
git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD->82102aec3f1d189d0eede233df45b4407c64bc69.PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_ledger_page_exposes_limit_controls tests\test_api_mcp.py::test_ledger_api_rejects_out_of_range_limits tests\test_api_mcp.py::test_ledger_page_highlights_bounty_payment_and_release tests\test_api_mcp.py::test_ledger_page_uses_wrapping_entry_cards -q --tb=short-> 6 passed...\mergework\.venv\Scripts\python.exe -m ruff check app\public_routes.py tests\test_api_mcp.py-> passed...\mergework\.venv\Scripts\python.exe -m ruff format --check app\public_routes.py tests\test_api_mcp.py-> 2 files already formatted...\mergework\.venv\Scripts\python.exe -m mypy app\public_routes.py-> success.PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke ok.- GitHub CI
Quality, readiness, docs, and image checksis green at readback.
I do not see a remaining blocker.
|
Pushed Addressed:
Validation:
|
6 participants
Bounty #427
Summary
/ledgerpage so contributors can choose how many recent ledger entries to inspect./api/v1/ledgerlimitbounds and link the rendered page to the matching JSON response.Before / After
/ledgerrendered the default recent ledger window with no page-level way to inspect a smaller or larger recent set./ledger?limit=Nshows the number of rendered entries, offers common row limits, rejects out-of-range limits through the existing FastAPI1..200validation, and links to/api/v1/ledger?limit=N./ledger?limit=25in the Money workspace receipt; I did not commit the binary screenshot into this repository.Duplicate / Scope Notes
Validation
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py::test_ledger_api_rejects_out_of_range_limits tests/test_api_mcp.py::test_ledger_page_exposes_limit_controls tests/test_api_mcp.py::test_ledger_page_highlights_bounty_payment_and_release tests/test_api_mcp.py::test_ledger_page_uses_wrapping_entry_cards -q-> 6 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q-> 415 passeduv run --extra dev ruff check app tests-> passeduv run --extra dev ruff format --check app tests-> 67 files already formatteduv run --extra dev python -m mypy app-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check-> exit 0; Git only printed Windows LF-to-CRLF working-copy warningsgit diff | gitleaks stdin --redact --no-banner --no-color-> no leaks foundLimitations
Summary by CodeRabbit
New Features
Style
Tests