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 (2)
📝 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 |
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
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. |
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