Repository navigation
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. WalkthroughThe Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change extracts the /pnl report flow into a shared function without changing response bodies or status codes. No concrete merge-blocking risk was found. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: sharing the /pnl report path so other callers build the same report. |
| Description check | |
The description directly explains the run_pnl_report refactor, its purpose, behavior, risks, tests, and merge context. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 too large.)
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
8d9608f to
a8677c5
Compare
36276b0 to
7decac8
Compare
a8677c5 to
123d4a8
Compare
7decac8 to
c79354b
Compare
|
@rain-marvin review |
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR moves the body of GET /pnl into one shared function, run_pnl_report. The metrics task in the next PR can then build the same report that the endpoint returns. PnlReportAdmission::with_permits gives a non-HTTP caller its own semaphore, and PnlReportError carries the three failure kinds back to the handler. The handler maps them to the same 400, 500, 502 and 503 responses as before.
Overall read: a clean extraction. Every lane checked the order of the steps (query checks, the 10 s catch-up, the asOfRowid check, the permit, the Alpaca fetch, the replay with Utc::now()) and the error mapping against the old handler, and found no change in status codes, bodies, or permit lifetime. The live route still uses the shared 2-permit admission. The diff adds no business-sensitive data to this public repo. There are two minor comments. The catch-up timeout warning moved into the shared function, so its log target and text changed and the metrics caller upstack logs that error twice. Also, the new 502 and 503 mapping arms have no test.
claude-opus-5-5 · high · 50 min
c79354b to
7542282
Compare
0eb74db to
045776e
Compare
JuaniRios
changed the title
This branch has not been deployed
1 participant
Part of RAI-3092.
Moves the body of
GET /pnlinto one shared function,run_pnl_report, so the PnL metrics task builds the same report the endpoint builds. Item 4 of the RAI-2998 stack, part 1 of 3.Live effect: none.
/pnlreturns the same bodies and status codes · Risk: low (a refactor of the/pnlrequest path) · Ships: on the next bot releaseWhy
The
liq_pnl_*series must be the numbers/pnlgives for the same window. The exporter got them by calling/pnl. If the bot calls the same function, there is nothing to keep in sync.Decisions
run_pnl_report(deps, query, admission)insrc/dashboard/pnl/source.rsdoes what the handler did, in the same order: check the query, bring the ledger current within 10 s, checkasOfRowidagainst the head, take a permit, fetch the Alpaca activities, replay.PnlReportAdmission::with_permits(n)gives a caller its own semaphore, so the metrics task (next PR) never takes one of the 2 live/pnlpermits.PnlReportErrorhas 3 variants (report, catch-up timeout, Alpaca). The handler keeps only the mapping to the same 400, 500, 502 and 503 responses as before.Proof
/pnltests pass unchanged.run_pnl_reportreturn identical JSON for the same query, and each call sees fills recorded before it (each one runs its own ledger catch-up).st0x-hedgesuite on the top of this 3-PR stack: 4814 passed./pnlis exercised on staging after release.Merge conditions
Same as the stack below.