Repository navigation
fix: let the API take a page size for replies like the web does - #1242
sulimanbenhalim wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Validate and apply reply limits app/Http/Requests/ListApiCommitRepliesRequest.php, app/Http/Controllers/Api/CommitReplyController.php, tests/Feature/Controllers/Api/CommitReplyControllerTest.php, KNOWLEDGE_BASE.md |
The request validates limit as an integer from 1 through 50. The controller passes it to pagination, using the default when omitted. Tests cover five-reply pages, following the next link, and validation errors for limits 0 and 51. The signed API documentation states the page-size behavior. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related PRs
- dev-null-sh/dev-null.sh#1168: Changes visibility filtering for replies and signed API responses, which also affects replies returned by this API flow.
Suggested reviewers: simonhamp, paulinevos
Merge Risk: 
Minimal · up to 40582
-
The pagination change has no identified merge-blocking risk.
Security architecture risk: 
Minimal · up to 40582
-
The new page-size option is bounded to 1–50 replies and preserves the existing default of 50. Authentication, visibility controls, and reply selection remain unchanged. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The new parameter changes how many already-visible replies are returned per page, not which reply dataset the caller may access. Its maximum equals the previous fixed page size, so it does not increase per-response record exposure or caller authority.
Trust Boundaries and Controls
- observed — The replies route remains behind required signed-request authentication and API throttling. Authentication verifies an approved device, rejects banned users, and establishes the device owner's identity. The signed message includes the request path and query, while commit visibility authorization remains unchanged.
-
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passedFull details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
@simonhamp this is gonna help in the app, what do you think ? |
|
Yeh definitely 👍🏼 |
Summary
limit=5since Open replies from the replies button, five at a time NativePHP/dev-null-app#91 but the signed API's replies endpoint ignores it and sends 50 every timelimitsince feat: choose how many replies to show at a time #599 so this gives the API the same parameterChanges
ListApiCommitRepliesRequesttakeslimit(1 to 50) likeListCommitRepliesRequestand the API'sCommitReplyControllerpasses it toCommitRepliesQuery::paginate()with 50 when it's missingKNOWLEDGE_BASE.mdunder the signed APIValidation
CommitReplyControllerTest: 12 replies come back 5 a page withlimitkept onlinks.nextand a limit of 0 or 51 gets a 422limitorlinks.nextloses it and the second when the rule is taken outIndependent review
6b92c0fdheadc50a7189and its one note (the next page test passed with or withoutlimiton it) is fixed in40582f07Limitations
Summary by CodeRabbit
limitoption.