Limit public activity feed rows#751
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 (1)
📝 WalkthroughWalkthroughThis PR adds optional ChangesActivity Limit Parameter
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e55e57a-ba6f-4a9d-9a67-a22b1941e3ec
📒 Files selected for processing (3)
app/activity.pyapp/serializers.pytests/test_activity.py
attaboy11
left a comment
attaboy11
left a comment
There was a problem hiding this comment.
Reviewed PR #751 at current head 4f708a9be789ce92ff0aca2585702d694d2076c1 as a non-author.
No blocking issue found in the activity limit fix.
What I checked:
app/activity.pynow exposes a typedlimitquery parameter on both/api/v1/activityand/activity, bounded to 1..200 by FastAPI validation.app/serializers.pyapplies the limit after filtering tocontributors,pending_payouts, andrecentwhile keeping totals over the full matching set.- Omitted
limitpreserves the existing default behavior: full contributor rows plus existing 100-row caps for pending/recent. tests/test_activity.pycovers valid limiting, combinedq+limit, invalid non-integer input, and the 0 / 201 boundary validation cases added in the latest commit.
Validation run from an isolated PR worktree:
/Users/mycomputo/Documents/Codex/2026-05-31/goal-goal-find-and-execute-legitimate/work/repos/mergework/.venv/bin/python -m pytest tests/test_activity.py tests/test_activity_routes.py -q-> 7 passed, 1 warning- targeted Ruff check -> passed
- targeted Ruff format check -> 3 files already formatted
mypy app/activity.py app/serializers.py-> passedgit diff --check origin/main...HEAD-> passedgit merge-tree --write-tree origin/main HEAD-> clean tree4a080b04da1f487d37ab160a4fe65e009077e5fc- hosted Quality/readiness/docs/image check is successful on this head; CodeRabbit was still pending at review time.
Minor non-blocking note: the HTML page itself now accepts and applies limit; the existing “View JSON activity” link does not preserve limit, but that does not block the reported API correctness fix.
No private data, credentials, wallet material, production mutation, payout execution, exchange, bridge, cash-out, price claims, or fabricated payout claims used.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_activity.py (2)
227-249: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider edge cases where limit equals or exceeds the dataset size.
The test proves valid limiting (limit=1), combined filtering+limiting (q+limit=2), and boundary validation (0, 201). Adding cases for
limit=3(exact dataset size) andlimit=100(exceeds dataset) would verify the implementation gracefully handles these realistic scenarios without error.Proposed additions
limited = client.get("/api/v1/activity?limit=1") filtered_limited = client.get("/api/v1/activity?q=github&limit=2") + exact_size = client.get("/api/v1/activity?limit=3") + exceeds_size = client.get("/api/v1/activity?limit=100") invalid = client.get("/api/v1/activity?limit=notanint") @@ assert len(filtered_payload["contributors"]) == 2 assert len(filtered_payload["recent"]) == 2 + assert exact_size.status_code == 200 + exact_payload = exact_size.json() + assert len(exact_payload["contributors"]) == 3 + assert len(exact_payload["recent"]) == 3 + + assert exceeds_size.status_code == 200 + exceeds_payload = exceeds_size.json() + assert len(exceeds_payload["contributors"]) == 3 + assert len(exceeds_payload["recent"]) == 3 + assert invalid.status_code == 422
201-242: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winVerify that pending_payouts is also subject to limiting.
The test proves
contributorsandrecentare constrained by the limit parameter, butpending_payoutscannot be verified because the test setup contains no pending proposals. Line 242 only confirms an empty list remains empty. Consider adding one or two pending proposals to the setup to prove all three arrays mentioned in the serializer contract are properly limited.Example setup addition
After creating the bounty and before the pay_bounty loop, add:
from app.treasury import propose_treasury_action pending_bounty = create_bounty( session, repo="ramimbo/mergework", issue_number=657, issue_url="https://github.com/ramimbo/mergework/issues/657", title="Pending limit test", reward_mrwk="50", acceptance="Pending payouts should also respect limit.", ) propose_treasury_action( session, action="pay_bounty", payload={ "bounty_id": pending_bounty.id, "to_account": "github:alice", "submission_url": "https://github.com/ramimbo/mergework/pull/657", "accepted_by": "maintainer", }, proposed_by="maintainer", ) propose_treasury_action( session, action="pay_bounty", payload={ "bounty_id": pending_bounty.id, "to_account": "github:bob", "submission_url": "https://github.com/ramimbo/mergework/pull/658", "accepted_by": "maintainer", }, proposed_by="maintainer", )Then update line 242:
- assert len(limited_payload["pending_payouts"]) == 0 + assert len(limited_payload["pending_payouts"]) == 1As per coding guidelines for
tests/**/*.py: "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 352eef96-2435-441b-85ab-44e3d43e937a
📒 Files selected for processing (1)
tests/test_activity.py
attaboy11
left a comment
attaboy11
left a comment
There was a problem hiding this comment.
Reviewed updated PR #751 head 64147ac5e2388c410ea4a8a8802c34909b082589 after the author added docs, page-link preservation, default limit echoing, and the CodeRabbit-suggested edge-case coverage.
No blocking issue found in the updated activity limit implementation.
What I checked:
limitnow defaults to100and is echoed in the activity API payload./activitypreserves non-defaultlimitin its “View JSON activity” link.- The serializer caps
contributors,pending_payouts, andrecentwhile preserving full-set totals. - The new tests cover pending-payout limiting, exact-size and exceeds-size limits, docs examples, and default empty-feed shape.
- The follow-up diff from my earlier reviewed head
4f708a9to64147acis limited to activity route/serializer/template/docs/tests.
Validation from the isolated PR worktree:
- full suite:
/Users/mycomputo/Documents/Codex/2026-05-31/goal-goal-find-and-execute-legitimate/work/repos/mergework/.venv/bin/python -m pytest -q-> 593 passed, 1 warning - targeted pytest:
tests/test_activity.py tests/test_activity_routes.py tests/test_docs_public_urls.py::test_api_examples_document_activity_limit tests/test_serializers.py::test_activity_serializers_skip_malformed_public_proofs-> 9 passed, 1 warning - targeted Ruff check -> passed
- targeted Ruff format check -> 6 files already formatted
mypy app/activity.py app/serializers.py-> passedgit diff --check origin/main...HEAD-> passedgit merge-tree --write-tree origin/main HEAD-> clean treebe7af5696f8802440d58014851266b674702ceab- hosted Quality/readiness/docs/image is successful on this head; CodeRabbit was still pending at review time.
No private data, credentials, wallet material, production mutation, payout execution, exchange, bridge, cash-out, price claims, or fabricated payout claims used.
|
Scope sanity for the latest head
This is intended to make the current scope easy to verify without relying on the stale earlier warning text. |
prettyboyvic
left a comment
prettyboyvic
left a comment
There was a problem hiding this comment.
Reviewed current head 64147ac5e2388c410ea4a8a8802c34909b082589 as a non-author against refreshed origin/main 9792898ea3ee754490ccb4abb9f2ecebe0759777.
The activity limit implementation still looks focused and healthy on its own, but the PR now needs a current-main refresh before it can merge:
- GitHub reports
mergeStateStatus: DIRTYfor PR #751. git merge-tree --write-tree origin/main refs/remotes/origin/pr-751exits non-zero with content conflicts inapp/activity.pyandtests/test_activity.py.- The conflict is real integration work: current
mainadded activityqcontrol-character rejection inapp/activity.py, plus UTC-explicit activity timestamp expectations intests/test_activity.py; this PR adds the activitylimitroute/serializer behavior and related tests in the same areas. - The earlier approval was valid for the then-current base, but it used
origin/main05faf1f89932d59f2935e83b71fc42ffef363c7f; the current base has moved since then.
What I inspected:
app/activity.py,app/serializers.py,app/templates/activity.html,docs/api-examples.md,tests/test_activity.py,tests/test_activity_routes.py,tests/test_docs_public_urls.py, andtests/test_serializers.py.- The current-main activity changes from merge base
05faf1f89932d59f2935e83b71fc42ffef363c7fto9792898ea3ee754490ccb4abb9f2ecebe0759777.
Branch-local validation on the PR head still passes:
uv run --extra dev python -m pytest tests/test_activity.py tests/test_activity_routes.py tests/test_serializers.py::test_activity_serializers_skip_malformed_public_proofs tests/test_docs_public_urls.py::test_api_examples_document_activity_limit -q-> 9 passed, 1 warning.uv run --extra dev ruff check app/activity.py app/serializers.py tests/test_activity.py tests/test_activity_routes.py tests/test_serializers.py tests/test_docs_public_urls.py-> passed.uv run --extra dev ruff format --check app/activity.py app/serializers.py tests/test_activity.py tests/test_activity_routes.py tests/test_serializers.py tests/test_docs_public_urls.py-> 6 files already formatted.uv run --extra dev python -m mypy app/activity.py app/serializers.py-> success.git diff --check origin/main...HEAD-> clean.- Hosted Quality/readiness/docs/image and CodeRabbit are successful on the reviewed head.
Suggested path: rebase or merge current main, resolve app/activity.py so it preserves both raw q control-character rejection and the new bounded limit parameter, then update tests/test_activity.py without losing the UTC timestamp assertions or the activity-limit coverage. After that, rerun the targeted activity/docs/serializer tests plus Ruff/mypy.
No private data, credentials, wallet material, production mutation, payout execution, ledger mutation, exchange, bridge, cash-out, price behavior, or fabricated payout claims were used.
JONASXZB
left a comment
JONASXZB
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — reviewed current open head 64147ac5e2388c410ea4a8a8802c34909b082589 against current origin/main f8fc36265ea763bcaabb3555d3ee4e0c885a4953 as a non-author.
The activity limit behavior still validates on the branch head, but this PR is not merge-ready against current main.
Evidence checked:
- Files inspected:
app/activity.py,app/serializers.py,app/templates/activity.html,docs/api-examples.md,tests/test_activity.py,tests/test_activity_routes.py,tests/test_docs_public_urls.py, andtests/test_serializers.py. - Branch-local focused validation passes:
.venv/bin/python -m pytest tests/test_activity.py tests/test_activity_routes.py tests/test_serializers.py tests/test_docs_public_urls.py -q-> 49 passed, 1 warning. - Docs smoke passes:
.venv/bin/python scripts/docs_smoke.py-> docs smoke ok. - Ruff passes:
.venv/bin/ruff check app/activity.py app/serializers.py tests/test_activity.py tests/test_activity_routes.py tests/test_serializers.py tests/test_docs_public_urls.py-> all checks passed; format check reports 6 files already formatted. git diff --check origin/main...HEADis clean.- Current-base mergeability is blocked:
git merge-tree --write-tree origin/main HEADexits non-zero with content conflicts inapp/activity.pyandtests/test_activity.py. - GitHub reports
mergeable=false/mergeable_state=dirty; CodeRabbit and the hosted quality/readiness/docs/image check are success on the branch head.
Requested follow-up: rebase or merge current main, resolve the activity route/test conflicts, then rerun the focused activity/docs/serializer tests and Ruff checks.
|
Reviewed current head Requesting changes until the current-main conflict is resolved. Evidence checked:
Suggested follow-up: refresh against current No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange, bridge, cash-out, live trading, local code mutation, git command, or worktree change was used. |
5 participants
/claim #656
Summary
limitquery parameter to/api/v1/activityand/activity.qand non-defaultlimitin the activity page JSON link, and document the activity limit response shape.Evidence
/api/v1/activity?limit=...accepted a caller-supplied limit but returned the full activity lists, so public consumers could not request bounded feeds.Intended scope
app/activity.py,app/serializers.py,app/templates/activity.html,docs/api-examples.md, and focused tests.Checklist
/claim #656included.limitvalue through the shared context.limitvalues return FastAPI validation errors.0and201.contributors,recent, andpending_payoutsare all limited.Validation
Safety
Testing and reproduction were read-only against public MRWK endpoints. This PR only changes list limiting/validation for activity surfaces and adds regression/documentation coverage.
Summary by CodeRabbit
New Features
limitquery parameter to activity endpoints (default 100, range 1–200). Returned arrays (contributors,pending_payouts,recent) are capped tolimitwhile totals reflect the full matching set; responses include the effectivelimit.UI
limitin the query string.Tests
Documentation
limitexamples and behavior.