Add wallet summary cards#489
Conversation
Bounty ramimbo#427: app/public_routes.py
Bounty ramimbo#427: app/templates/wallets.html
Bounty ramimbo#427: tests/test_wallet_api.py
|
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 (1)
📝 WalkthroughWalkthroughBackend adds Decimal formatting helper and includes a ChangesWallet Summary Display
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 |
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed current head 0c375d3776c36c237541f815bad467772ee7e5f4 as a non-author reviewer.
I checked the wallet summary path in app/public_routes.py, app/templates/wallets.html, and tests/test_wallet_api.py. The new cards are derived from the same 100 displayed wallet rows, count linked GitHub wallets from wallet_to_dict() output, and sum the rendered MRWK balances with Decimal before formatting. The regression covers three displayed wallets, one linked wallet, and a combined 12.5 MRWK balance while leaving the existing wallet form/table flow intact.
Validation run locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py -q-> 32 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q-> 414 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py-> docs smoke okuv run --extra dev ruff check app/public_routes.py tests/test_wallet_api.py-> passeduv run --extra dev ruff format --check app/public_routes.py tests/test_wallet_api.py-> 2 files already formatteduv run --extra dev python -m mypy app-> success, 30 source filesgit diff --check origin/main...HEAD-> cleangit diff --no-ext-diff origin/main..HEAD | gitleaks stdin --no-banner --redact --exit-code 1-> no leaks
Limitation: GitHub check rollup still showed only CodeRabbit pending at readback, so this review is based on the source/test validation above.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d901226e-b3de-4269-acdd-93972ebe2313
📒 Files selected for processing (3)
app/public_routes.pyapp/templates/wallets.htmltests/test_wallet_api.py
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Callable | ||
| from decimal import Decimal |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff format --check .
ruff check .
mypy appRepository: ramimbo/mergework
Length of output: 181
Blocker: required mypy app check didn’t run (mypy not installed)
ruff format --check .andruff check .pass.mypy appfails to execute in this environment (mypy: command not found), so the required type-checking gate isn’t satisfied.- Functional change in
app/public_routes.py(e.g., addedfrom decimal import Decimal) looks consistent with the intended formatting/helper usage.
prettyboyvic
left a comment
prettyboyvic
left a comment
There was a problem hiding this comment.
Reviewed current head 0c375d3776c36c237541f815bad467772ee7e5f4 for the #447 review bounty.
I am requesting changes for one UX/data-contract issue in the new summary cards: the page only loads the newest 100 wallets, but the card label says Total wallet balance. In wallets_page_context(), the query is limited to 100 wallets, wallet_rows is built from those rows, and total_balance_mrwk is the sum of only that displayed slice. The template then renders it as Total wallet balance, which reads as an all-wallet ledger total.
Local repro on this head with 101 registered wallets funded at 1 MRWK each:
wallets_shown_card_100 True
total_card_contains_100 True
actual_wallets 101
So the page correctly says 100 wallets are shown, but the adjacent Total wallet balance card reports 100 MRWK even though the registered wallet balance total is 101 MRWK. That can mislead contributors or maintainers using the page as a public wallet overview once the registry exceeds the display limit.
Suggested fix: either rename the card to something explicitly scoped like Shown wallet balance, or compute true aggregate counts/balance across all registered wallets while keeping the table itself limited to 100 rows. Add a regression with more than 100 wallets so this label/aggregate behavior stays intentional.
Validation run locally on this head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py -q -> 32 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 414 passed
uv run --extra dev ruff check app/public_routes.py tests/test_wallet_api.py -> passed
uv run --extra dev ruff format --check app/public_routes.py tests/test_wallet_api.py -> 2 files already formatted
uv run --extra dev python -m mypy app -> success, no issues in 30 source files
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
git diff --check d8532d4d909f47c689d3b88eb21461243097673f...HEAD -> clean
GitHub readback before posting: PR is open/non-draft at 0c375d3776c36c237541f815bad467772ee7e5f4; merge state is currently UNSTABLE while checks/CodeRabbit are still processing.
|
Follow-up for the wallet summary label finding:
Validation after the update:
|
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed the updated head 5e81c44aeb322617c6f98820267dff3844264e14 after the wallet-summary follow-up. The previous stale-head UX concern is addressed: the context key and UI label now say displayed_balance_mrwk / Displayed wallet balance, so the card is explicitly scoped to the 100 rendered wallet rows rather than implying an all-wallet total.
Validation run locally on the updated head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py -q-> 32 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q-> 414 passeduv run --extra dev ruff check app/public_routes.py tests/test_wallet_api.py-> passeduv run --extra dev ruff format --check app/public_routes.py tests/test_wallet_api.py-> 2 files already formatteduv run --extra dev python -m mypy app-> success, 30 source filesPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit diff --no-ext-diff origin/main..HEAD | gitleaks stdin --no-banner --redact --exit-code 1-> no leaks
GitHub readback before posting: PR open/non-draft, MERGEABLE, current head 5e81c44aeb322617c6f98820267dff3844264e14, CodeRabbit recent review has no actionable comments.
er1c-cartman
left a comment
er1c-cartman
left a comment
There was a problem hiding this comment.
Reviewed current head 5e81c44aeb322617c6f98820267dff3844264e14 for the #447 review bounty.
I would hold this for one small test-quality fix. The code behavior is now scoped correctly to displayed wallets: wallets_page_context() computes displayed_balance_mrwk from the same 100 rendered wallet rows, and the template labels it as Displayed wallet balance, so the earlier human concern about implying an all-wallet total is addressed.
The remaining issue is in tests/test_wallet_api.py: the new summary assertions check only loose fragments like "3</strong>", "1</strong>", and "12.5 MRWK</strong>". Those can pass if the values appear in the wrong card, or if a future template change renders misleading summary labels while the same numeric fragments still exist elsewhere in the page. Since the PR's main behavior is the wallet summary cards, the regression should bind each expected value to its label, for example by asserting scoped HTML/regex pairs for Wallets shown -> 3, Linked GitHub wallets -> 1, and Displayed wallet balance -> 12.5 MRWK.
Evidence checked:
- Inspected
app/public_routes.py,app/templates/wallets.html, andtests/test_wallet_api.pyon current head. - Confirmed the summary is intentionally display-slice scoped, not all-wallet scoped.
- Confirmed CodeRabbit status is success, but there are still unresolved review threads; the pytest/mypy environment comments are covered by the local validation below, while the assertion-strength issue remains actionable.
Validation run locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows -q-> 1 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_wallet_api.py -q-> 32 passed./.venv/bin/python -m ruff check app/public_routes.py tests/test_wallet_api.py-> passed./.venv/bin/python -m ruff format --check app/public_routes.py tests/test_wallet_api.py-> 2 files already formattedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m mypy app/public_routes.py-> successgit diff --check origin/main...HEAD-> clean
No secrets, wallet material, private data, production mutation, price/exchange/liquidity claim, or off-ramp claim was used.
Bind each summary value to its rendered label so the wallet summary regression cannot pass on loose numeric fragments.
|
Follow-up for the newer test-strength review:
This addresses the requested regression-strength issue without changing runtime behavior. |
adliebe
left a comment
adliebe
left a comment
There was a problem hiding this comment.
Reviewed the current head a787012a44a677f1d7bf5a39fc0155b6501c852c after the wallet-summary assertion follow-up.
This resolves the remaining issue from the earlier reviews: tests/test_wallet_api.py now binds each summary value to its rendered label with scoped regex assertions, covering Wallets shown -> 3, Linked GitHub wallets -> 1, and Displayed wallet balance -> 12.5 MRWK. That prevents the regression from passing on loose numeric fragments or values appearing under the wrong card.
Evidence checked:
app/public_routes.pystill computes the summary from the displayed 100 wallet rows and formats the displayed balance without implying an all-wallet total.app/templates/wallets.htmllabels the balance asDisplayed wallet balance.tests/test_wallet_api.pynow importsreand asserts label/value pairs in the rendered wallet summary.- GitHub readback before posting: PR is open, non-draft, current head
a787012a44a677f1d7bf5a39fc0155b6501c852c, and CodeRabbit status is success.
Validation:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py::test_wallet_pages_expose_transfer_and_github_claim_flows -q->1 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_wallet_api.py -q->32 passeduv run --extra dev ruff check app/public_routes.py tests/test_wallet_api.py-> passeduv run --extra dev ruff format --check app/public_routes.py tests/test_wallet_api.py->2 files already formatteduv run --extra dev python -m mypy app/public_routes.py-> successPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py->docs smoke okgit diff --check origin/main...HEAD-> clean
No private wallet material, secrets, payout promises, exchange/off-ramp claims, or private data are introduced.
5 participants
Bounty #427
Summary:
Evidence:
Verification:
Summary by CodeRabbit