Skip to content

Share me page account summary context#919

Open
pqmfei wants to merge 1 commit into
ramimbo:mainfrom
pqmfei:pqmfei/846-share-me-page-account-summary
Open

Share me page account summary context#919
pqmfei wants to merge 1 commit into
ramimbo:mainfrom
pqmfei:pqmfei/846-share-me-page-account-summary

Conversation

@pqmfei
Copy link
Copy Markdown
Contributor

@pqmfei pqmfei commented Jun 5, 2026

Summary

Refs Bounty #846.

  • Extracts a small _github_account_summary() helper for the logged-in GitHub account portion of me_page_context().
  • Keeps anonymous defaults in the page context and updates them only when a GitHub login is present.
  • Adds coverage for a GitHub account with balance but no linked wallet, preserving the empty linked-wallet field.

Duplicate / Scope Check

Validation

  • .\.venv\Scripts\python.exe -m pytest tests\test_me_page.py -q -> 3 passed
  • .\.venv\Scripts\python.exe -m pytest tests\test_me_page.py tests\test_wallet_api.py tests\test_account_routes.py -q -> 55 passed, 1 existing Starlette/httpx warning
  • .\.venv\Scripts\python.exe -m ruff check app\me.py tests\test_me_page.py -> passed
  • .\.venv\Scripts\python.exe -m ruff format --check app\me.py tests\test_me_page.py -> 2 files already formatted
  • .\.venv\Scripts\python.exe -m mypy app\me.py -> success
  • git diff --check origin/main...HEAD -> clean
  • git merge-tree --write-tree origin/main HEAD -> clean tree abda6f4c7b8cc6d19c1ef4bc0fd2711b121cfce1

No wallet registration, wallet signing, balance calculation, ledger mutation, treasury behavior, payout behavior, proposal execution, admin-token behavior, private data, exchange, bridge, cash-out, or MRWK price behavior changed.

Summary by CodeRabbit

  • Bug Fixes
    • GitHub balance is now properly reported for accounts without a linked wallet.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 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: 45bdac31-38c7-4774-a09c-2a198695ecd3

📥 Commits

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

📒 Files selected for processing (2)
  • app/me.py
  • tests/test_me_page.py

📝 Walkthrough

Walkthrough

_github_account_summary extracts GitHub account balance and linked wallet address computation into a helper function. me_page_context refactored to initialize context with defaults and conditionally invoke the helper when a login is provided. New test validates balance reporting when no wallet is linked.

Changes

GitHub Account Context Refactor

Layer / File(s) Summary
GitHub account summary helper and context refactor
app/me.py
New _github_account_summary helper centralizes computation of GitHub balance and linked wallet address. me_page_context simplified to initialize defaults and conditionally update context with computed values when login is provided.
Test balance reporting without linked wallet
tests/test_me_page.py
New test seeds a GitHub balance for alice, calls me_page_context without registering a linked wallet, and asserts the context includes the computed balance while keeping linked_wallet_address empty.

Possibly Related PRs

  • ramimbo/mergework#376: Previous changes to /me context assembly in app/me.py and corresponding tests; this PR's refactor builds on that context extraction.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Share me page account summary context' clearly describes the main change: extracting and sharing a helper for the account summary portion of me_page_context().
Description check ✅ Passed The description includes a clear summary of changes, evidence of scope checking and validation testing, though the 'Test Evidence' checklist section is not explicitly completed with checkmarks.
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 No investment, price, cash-out, fabricated payout claims found. MRWK described as native ledger coin; future bridges/snapshots require discussion.
Bounty Pr Focus ✅ Passed PR diff matches stated scope: app/me.py with helper and function; tests/test_me_page.py with 3 focused tests including new balance-without-wallet case. No unrelated changes. All imports verified.

✏️ 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
Contributor

@akmhatey-ai akmhatey-ai 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 b794183cc56186a67f5a90321a8be9d9e96fd383.

Verdict: approved from the me-page context refactor and behavior-preservation angle.

What I checked:

  • #846 is live, labeled mrwk:bounty, and reserved on the public MRWK bounty row.
  • The diff is limited to app/me.py and tests/test_me_page.py.
  • _github_account_summary() preserves the existing logged-in GitHub balance and linked-wallet lookup behavior while keeping anonymous defaults explicit in me_page_context().
  • The added regression covers the missing middle case: a GitHub account with a ledger balance but no linked wallet reports the balance and keeps linked_wallet_address empty.
  • Same-surface duplicate check: #643 had no visible pull/919, #919, or PR #919 review-bounty claim before this review, and GitHub showed zero current-head human reviews on PR #919 before posting.

Validation I ran locally on a clean PR worktree:

  • uv run --python 3.12 --extra dev python -m pytest tests/test_me_page.py tests/test_wallet_api.py tests/test_account_routes.py -q -> 55 passed, 1 existing Starlette/httpx warning.
  • uv run --python 3.12 --extra dev ruff check app/me.py tests/test_me_page.py -> passed.
  • uv run --python 3.12 --extra dev ruff format --check app/me.py tests/test_me_page.py -> 2 files already formatted.
  • uv run --python 3.12 --extra dev mypy app/me.py -> success.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree 61b1279c6d7f479fda1c8eff16e5633ced995194.
  • gitleaks detect --source . --no-banner --redact --log-level warn -> no findings.

Limitation: GitHub currently shows only CodeRabbit in the status rollup and mergeStateStatus=UNSTABLE; I treated that as a hosted-check limitation, not as proof of normal hosted CI completion.

No private data, credentials, wallet material, production mutation, payout execution, treasury execution, ledger mutation, exchange, bridge, cash-out, price behavior, or fabricated payout claims were used.

Copy link
Copy Markdown

@catcherintheroad-hub catcherintheroad-hub 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 b794183cc56186a67f5a90321a8be9d9e96fd383 for #838.

This remains a narrow me-page context refactor. I inspected app/me.py and tests/test_me_page.py: _github_account_summary() preserves the logged-in GitHub balance lookup and linked-wallet lookup, while me_page_context() still returns anonymous defaults until a GitHub login is present. The added test covers the middle case where a GitHub ledger account has a balance but no linked wallet, keeping linked_wallet_address empty.

Validation on this exact head against current origin/main:

  • uv run --python 3.12 --extra dev python -m pytest tests/test_me_page.py tests/test_wallet_api.py tests/test_account_routes.py -q -> 55 passed, 1 existing Starlette/httpx warning.
  • uv run --python 3.12 --extra dev ruff check app/me.py tests/test_me_page.py -> passed.
  • uv run --python 3.12 --extra dev ruff format --check app/me.py tests/test_me_page.py -> 2 files already formatted.
  • uv run --python 3.12 --extra dev mypy app/me.py -> success.
  • uv run --python 3.12 --extra dev python scripts/docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree 73afd75dededef72c9998cd44499917908d56946.

GitHub state checked before review: PR open, non-draft, CodeRabbit successful, and mergeStateStatus=UNSTABLE because no hosted Quality/readiness/docs/image check is present for this head. I did not find a code or mergeability blocker in local validation.

Scope: read-only review of me-page account context construction and tests only. No wallet registration/signing, balance calculation semantics, ledger mutation, treasury/proposal execution, payout execution, admin-token behavior, private data, credentials, bridge/exchange/cash-out behavior, or MRWK price behavior changed or claimed.

Copy link
Copy Markdown

@Errordog2 Errordog2 left a comment

Choose a reason for hiding this comment

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

Bounty #838 current-head evidence review.

Reviewed current head b794183cc56186a67f5a90321a8be9d9e96fd383 as a non-author.

Verdict: approve. The helper extraction keeps the anonymous me_page_context() defaults unchanged, calls the GitHub balance/wallet lookup only when a login is present, and preserves the empty linked-wallet field when an account has a balance but no linked wallet. The new regression test covers that no-wallet balance case directly.

Evidence checked:

  • Inspected app/me.py and tests/test_me_page.py in the current diff.
  • Verified GET /pulls/919 reports mergeable=true for this head; GitHub returned no hosted check runs for the commit, so I used local validation as the review evidence.
  • Duplicate/overlap search for open PRs mentioning me_page_context, app/me.py, and test_me_page only found #919 for the relevant scopes.

Validation run locally on Python 3.12.13:

  • python -m pytest tests/test_me_page.py -q -> 3 passed.
  • python -m pytest tests/test_me_page.py tests/test_wallet_api.py tests/test_account_routes.py -q -> 55 passed, 1 existing Starlette/httpx warning.
  • python -m ruff check app/me.py tests/test_me_page.py -> passed.
  • python -m ruff format --check app/me.py tests/test_me_page.py -> 2 files already formatted.
  • python -m mypy app/me.py -> success.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree 73afd75dededef72c9998cd44499917908d56946.

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.

4 participants