Adding checks for ok status code in responses#2178
Open
sergey3bv wants to merge 2 commits intogetAlby:masterfrom
Open
Adding checks for ok status code in responses#2178sergey3bv wants to merge 2 commits intogetAlby:masterfrom
sergey3bv wants to merge 2 commits intogetAlby:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdded explicit HTTP status validation across multiple API clients and frontend fetchers: responses are read fully, checked for 200 OK (or res.ok in frontend), logged on failure with status and body, and returned as errors before any JSON parsing occurs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/rebalance.go (1)
87-94: Minor inconsistency in log field naming.The log field uses
"statusCode"(camelCase), while other files in this PR use"status_code"(snake_case). Consider using consistent naming across the codebase for easier log querying.Suggested fix
if res.StatusCode != http.StatusOK { logger.Logger.WithFields(logrus.Fields{ "request": newRspCreateOrderRequest, "body": string(body), - "statusCode": res.StatusCode, + "status_code": res.StatusCode, }).Error("rebalance create_order endpoint returned non-success code")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/rebalance.go` around lines 87 - 94, The log field name is inconsistent: change the "statusCode" field used in the logger.Logger.WithFields call within the rebalance create_order response handling (where newRspCreateOrderRequest, body and res are available) to "status_code" to match the snake_case convention used elsewhere; update only the key name in that WithFields map so the error log call and returned error behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/rebalance.go`:
- Around line 87-94: The log field name is inconsistent: change the "statusCode"
field used in the logger.Logger.WithFields call within the rebalance
create_order response handling (where newRspCreateOrderRequest, body and res are
available) to "status_code" to match the snake_case convention used elsewhere;
update only the key name in that WithFields map so the error log call and
returned error behavior remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0657150a-f788-4785-8081-c12cf0bf97d6
📒 Files selected for processing (7)
api/api.goapi/esplora.goapi/rebalance.gofrontend/src/screens/internal-apps/ZapPlanner.tsxfrontend/src/screens/settings/Settings.tsxlnclient/phoenixd/phoenixd.goswaps/swaps_service.go
Contributor
Author
|
Hey, @michael1011, there is a weird error in CI, could you please take a look? |
I don't know why your Mac code signing certificates are not being loaded |
rolznz
reviewed
Mar 25, 2026
rolznz
reviewed
Mar 25, 2026
rolznz
reviewed
Mar 25, 2026
rolznz
reviewed
Mar 25, 2026
sergey3bv
force-pushed
the
fix/response-ok-check
branch
from
a8f1d7c to
4792c9e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lnclient/phoenixd/phoenixd.go (1)
110-120: Centralize Phoenixd response decoding and wrap low-level failures.The new
io.ReadAll→ status check →json.Unmarshalflow is repeated six times, and the bareerrreturns still lose which Phoenixd endpoint failed when reads or decodes blow up. A small helper would keep the status gate consistent and preserve endpoint context everywhere.♻️ Helper sketch
+func decodePhoenixResponse(resp *http.Response, endpoint string, target interface{}) error { + body, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("read phoenixd %s response: %w", endpoint, err) + } + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("phoenixd %s returned non-success status: %d %s", endpoint, resp.StatusCode, string(body)) + } + if err := json.Unmarshal(body, target); err != nil { + return fmt.Errorf("decode phoenixd %s response: %w", endpoint, err) + } + return nil +}As per coding guidelines,
Use error wrapping with fmt.Errorf("context: %w", err) for debugging.Also applies to: 158-168, 220-230, 267-277, 308-318, 357-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/phoenixd/phoenixd.go` around lines 110 - 120, Extract the repeated pattern into a small helper (e.g., phoenixdDecodeResponse(resp *http.Response, endpoint string, out interface{}) error) that does io.ReadAll, checks resp.StatusCode==http.StatusOK, and json.Unmarshal into out, and update each callsite (the /getbalance flow that populates BalanceResponse and the similar blocks at the other locations) to call this helper; ensure the helper wraps low-level errors with context using fmt.Errorf("%s read body: %w", endpoint, err) and fmt.Errorf("%s decode: %w", endpoint, err) and returns a single formatted error for non-200 statuses like fmt.Errorf("%s returned non-success status: %d %s", endpoint, resp.StatusCode, string(body)).frontend/src/screens/settings/Settings.tsx (1)
31-37: Extract thealbyRatesFetcherto a shared utility.Both
Settings.tsx(lines 31–37) andZapPlanner.tsx(lines 119–131) implement identical inline fetchers for the same endpoint using rawfetch(). This duplicates logic and violates the guideline to use therequest()helper fromfrontend/src/utils/request.ts. Extract this to a shared hook or extendrequest()to support absolute URLs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/screens/settings/Settings.tsx` around lines 31 - 37, Duplicate inline fetch logic for albyRatesFetcher used in Settings.tsx and ZapPlanner.tsx should be moved to a shared utility and wired to the existing request() helper: create and export a single function (e.g., albyRatesFetcher or getAlbyRates) in the shared utils module where request() lives, implement it to call request() (or extend request() to accept absolute URLs) and perform the same error handling and typed JSON return, then replace the inline fetch implementations in both Settings.tsx and ZapPlanner.tsx with imports of that shared function and remove the duplicated code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/screens/settings/Settings.tsx`:
- Around line 31-37: Duplicate inline fetch logic for albyRatesFetcher used in
Settings.tsx and ZapPlanner.tsx should be moved to a shared utility and wired to
the existing request() helper: create and export a single function (e.g.,
albyRatesFetcher or getAlbyRates) in the shared utils module where request()
lives, implement it to call request() (or extend request() to accept absolute
URLs) and perform the same error handling and typed JSON return, then replace
the inline fetch implementations in both Settings.tsx and ZapPlanner.tsx with
imports of that shared function and remove the duplicated code.
In `@lnclient/phoenixd/phoenixd.go`:
- Around line 110-120: Extract the repeated pattern into a small helper (e.g.,
phoenixdDecodeResponse(resp *http.Response, endpoint string, out interface{})
error) that does io.ReadAll, checks resp.StatusCode==http.StatusOK, and
json.Unmarshal into out, and update each callsite (the /getbalance flow that
populates BalanceResponse and the similar blocks at the other locations) to call
this helper; ensure the helper wraps low-level errors with context using
fmt.Errorf("%s read body: %w", endpoint, err) and fmt.Errorf("%s decode: %w",
endpoint, err) and returns a single formatted error for non-200 statuses like
fmt.Errorf("%s returned non-success status: %d %s", endpoint, resp.StatusCode,
string(body)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0932b56-9a72-4b42-af57-95811541ef7a
📒 Files selected for processing (7)
api/api.goapi/esplora.goapi/rebalance.gofrontend/src/screens/internal-apps/ZapPlanner.tsxfrontend/src/screens/settings/Settings.tsxlnclient/phoenixd/phoenixd.goswaps/swaps_service.go
✅ Files skipped from review due to trivial changes (2)
- swaps/swaps_service.go
- api/api.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/rebalance.go
- frontend/src/screens/internal-apps/ZapPlanner.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should close #327
Summary by CodeRabbit