convert some handlers to getAPIRouterNoError#4067
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHandlers for several API endpoints were changed to return JSON objects with a ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/handlers/handlers.go (1)
1479-1497:⚠️ Potential issue | 🟠 MajorRestore the account-validity guard before building Moonpay info.
Unlike
getMarketBtcDirectInfoandgetMarketBitrefillInfo, this handler now accepts any account returned byGetAccountFromCode, including offline or fatal ones. That can leak vendor-specific errors instead of the consistent “Account is not valid.” response.Suggested fix
- acct, err := handlers.backend.GetAccountFromCode(accountsTypes.Code(mux.Vars(r)["code"])) - if err != nil { - return result{Success: false, ErrorMessage: err.Error()} - } + code := accountsTypes.Code(mux.Vars(r)["code"]) + acct, err := handlers.backend.GetAccountFromCode(code) + accountValid := acct != nil && acct.Offline() == nil && !acct.FatalError() + if err != nil || !accountValid { + return result{Success: false, ErrorMessage: "Account is not valid."} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/handlers/handlers.go` around lines 1479 - 1497, Reintroduce the account-validity guard before calling market.MoonpayInfo: after retrieving acct with GetAccountFromCode, verify the account validity using the same check pattern used in getMarketBtcDirectInfo/getMarketBitrefillInfo (e.g., acct.IsValid() or handlers.backend.IsAccountValid(acct)); if the account is invalid or offline return result{Success: false, ErrorMessage: "Account is not valid."} instead of proceeding to build Moonpay params, then only call market.MoonpayInfo(acct, params) when the account is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/handlers/handlers.go`:
- Around line 544-557: The error branches in postAppConfig currently return a
failure response but do not log the error; update postAppConfig to log
decode/save failures using the structured logger: call
logging.Get().WithGroup("handlers").WithField("handler","postAppConfig") and
then .WithField("error", err) (or .WithError if available) before returning the
response; apply the same pattern to the other handler functions that return
{success:false} on decode/open/balance/save failures so all failure paths emit
structured logs with handler context and the error.
In `@frontends/web/src/components/anchor/anchor.tsx`:
- Around line 48-56: The current call to open(href) only notifies the user on
response.success=false but swallows transport/middleware failures in
.catch(console.error); update the promise chain around open(href) in the anchor
click handler (the open(...) call) to catch errors and surface them to the user
via alertUser, e.g., in the .catch block call alertUser with a translated
message using t('unknownError', { errorMessage: err.message || String(err) }) or
a generic fallback so network/exception failures are shown instead of only
logged to console.
In `@frontends/web/src/utils/config.ts`:
- Around line 43-48: The current branch clears pendingConfig and returns
nextConfig even when response.success is false on QtWebEngine/mobile; change the
logic in the .then((response: TSetConfigResponse) => { ... }) handler so that
pendingConfig is only cleared and nextConfig returned when response.success ===
true, and when response.success is false always throw an Error (using
response.errorMessage || 'Failed to update configuration') so callers see the
failure and unsaved changes remain; adjust any platform checks
(runningInQtWebEngine(), runningOnMobile()) so they do not bypass the failure
path.
- Line 4: Replace the relative import for runningInQtWebEngine and
runningOnMobile with the repo alias import; update the import statement that
currently references './env' to use the '@/utils/env' alias so the symbols
runningInQtWebEngine and runningOnMobile are imported via '@/utils/env' (update
in the module where config.ts defines/uses these imports).
---
Outside diff comments:
In `@backend/handlers/handlers.go`:
- Around line 1479-1497: Reintroduce the account-validity guard before calling
market.MoonpayInfo: after retrieving acct with GetAccountFromCode, verify the
account validity using the same check pattern used in
getMarketBtcDirectInfo/getMarketBitrefillInfo (e.g., acct.IsValid() or
handlers.backend.IsAccountValid(acct)); if the account is invalid or offline
return result{Success: false, ErrorMessage: "Account is not valid."} instead of
proceeding to build Moonpay params, then only call market.MoonpayInfo(acct,
params) when the account is valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e9692f1-1c13-45b9-be53-282193314843
📒 Files selected for processing (7)
backend/handlers/handlers.gofrontends/web/src/api/market.tsfrontends/web/src/api/system.tsfrontends/web/src/components/anchor/anchor.tsxfrontends/web/src/routes/market/moonpay.test.tsxfrontends/web/src/routes/market/moonpay.tsxfrontends/web/src/utils/config.ts
Switch POST /config to getAPIRouterNoError and return an explicit
success/failure response from the handler.
The old fetch/browser path rejected legacy { error } responses through
handleError(), while Qt/mobile transports returned the backend payload
directly and setConfig() ignored it. Preserve that split by only
throwing on success: false outside Qt/mobile; packaged app transports
still clear pendingConfig and resolve as before.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontends/web/src/components/anchor/anchor.tsx (1)
48-56:⚠️ Potential issue | 🟠 MajorHandle rejected
open()calls the same way as{ success: false }.Transport or middleware failures still go through
.catch(console.error), so clicking the link can fail without any user feedback. ReusealertUser(...)in the catch path instead of only logging.💡 Proposed fix
open(href) .then(response => { if (!response.success) { alertUser(response.errorMessage ? t('unknownError', { errorMessage: response.errorMessage }) : t('genericError')); } }) - .catch(console.error); + .catch(error => { + console.error(error); + alertUser( + error instanceof Error && error.message + ? t('unknownError', { errorMessage: error.message }) + : t('genericError')); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontends/web/src/components/anchor/anchor.tsx` around lines 48 - 56, The click handler currently calls open(href) and only alerts the user when response.success is false, while transport/middleware failures are only logged via .catch(console.error); modify the promise error path so that the catch handler calls alertUser(...) with the same localized message logic using t(...) (matching how non-success responses are handled) instead of just console.error, i.e., reuse alertUser and the existing t('unknownError', { errorMessage: ... }) / t('genericError') logic in the .catch branch for the open(href) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontends/web/src/utils/config.ts`:
- Around line 43-46: The fallback hardcoded string 'Failed to update
configuration' should be removed from the setConfig response handling so UI
strings remain translatable at the component layer; in the .then handler inside
setConfig (the function handling TSetConfigResponse) throw an Error using only
response.errorMessage (if present) or throw a generic Error without a
user-facing message (e.g., new Error()) so callers/components can supply a
translated fallback via useTranslation() and t(); do not introduce a new t()
usage in this utility module.
---
Duplicate comments:
In `@frontends/web/src/components/anchor/anchor.tsx`:
- Around line 48-56: The click handler currently calls open(href) and only
alerts the user when response.success is false, while transport/middleware
failures are only logged via .catch(console.error); modify the promise error
path so that the catch handler calls alertUser(...) with the same localized
message logic using t(...) (matching how non-success responses are handled)
instead of just console.error, i.e., reuse alertUser and the existing
t('unknownError', { errorMessage: ... }) / t('genericError') logic in the .catch
branch for the open(href) call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 870a0efd-fa28-410b-b928-e546ed785195
📒 Files selected for processing (7)
backend/handlers/handlers.gofrontends/web/src/api/market.tsfrontends/web/src/api/system.tsfrontends/web/src/components/anchor/anchor.tsxfrontends/web/src/routes/market/moonpay.test.tsxfrontends/web/src/routes/market/moonpay.tsxfrontends/web/src/utils/config.ts
| success: true; | ||
| } | { | ||
| success: false; | ||
| errorMessage?: string; |
There was a problem hiding this comment.
Many types use SuccessResponse and FailResponse, but this has slightly different properties. Instead of "errorMessage" it has just "message". Therer is a comment that this uses // if the backend uses maybeBB02Err.
export type SuccessResponse = {
success: true;
};
// if the backend uses maybeBB02Err
export type FailResponse = {
code?: number;
message?: string;
success: false;
};https://github.com/BitBoxSwiss/bitbox-wallet-app/blob/master/frontends/web/src/api/response.ts
I have no strong opinion on which one we should prefer, but it would be nice if we could just use those everywhere.
type TOpenResponse = SuccessResponse | FailResponse;Would it make sense to refactor maybeBB02Err to use errorMessage etc?
(also some endpoints return "errMsg")
There was a problem hiding this comment.
Almost all endpoints outside use errorMessage I think. Makes sense to me to make BitBox02 handlers use the same, but that's for another PR, as it is unrelated to this one.
There was a problem hiding this comment.
Ok then we should change FailResponse to use errorMessage in the future and use and extend from SuccessResponse FailResponse everywhere.
Should we rename SuccessResponse to SuccessResponseLegacy and already start with the new one?
But I'd love if in the end all use the same and we dont end up having a mix again.
| open(href) | ||
| .then(response => { | ||
| if (!response.success) { | ||
| alertUser(response.errorMessage |
There was a problem hiding this comment.
At some point we should stop using alertUser, this function was always a bit a hack but very convenient. Better would be to have an error state and render the error in the component.
There was a problem hiding this comment.
Can there be a solution that is as easy to use as alertUser? Making ad-hoc ways to display errors everywhere is difficult (for me).
Maybe you could make a PR sometime that removes all uses of alertUser and replaces it with a streamlined way of showing such messages?
There was a problem hiding this comment.
This is pretty easy have a look: https://github.com/BitBoxSwiss/bitbox-wallet-app/pull/4082/changes.
But I agree it would be nice to have a good reusable way.
In the future we may want to have a global notification center / error component. 🤷
There was a problem hiding this comment.
The issue with that one is that it's a lot more effort - need to think where to place the error message, test if it looks good, etc. alertUser "just works" - would be nice to have something that is as easy to use.
There was a problem hiding this comment.
it's harder yes but much nicer UI than popup. we also have popup over popup, somewhere in one case 3 popups over each other each with different dimension and visible. I'll find a screenshot once.
also from our friend gpt:
You’re right to call this a “sin” 😄 — a global mutable function that triggers UI from anywhere (including utilities) breaks React’s core model (unidirectional data flow + explicit state). Refactoring it is less about removing alertUser and more about restoring architectural boundaries.
There was a problem hiding this comment.
Keep as is for this PR, but if you don't mind maybe well change it for smth better and remove alertUser ok?
| Success bool `json:"success"` | ||
| ErrorMessage string `json:"errorMessage,omitempty"` |
There was a problem hiding this comment.
[nit] I'm almost thinking -but not sure- we could have a base response type like this one, and then most of the others would just either use this directly, or extend it with other fields
| } | { | ||
| success: false; | ||
| errorMessage?: string; | ||
| }; |
There was a problem hiding this comment.
fine for now, but we should try to do something like
SuccessResponse & {
url: string;
address: string;
} | FailResponse;
| frontend?: unknown; | ||
| }; | ||
|
|
||
| type TSetConfigResponse = null | undefined | { |
There was a problem hiding this comment.
Is this correct that it can return null | undefined ?
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
LGTM
'd love if we have those messages and codes consistently in all apis' in the futurte and can do TSuccessResponse & {custom: true} | TFailResponse.
3 participants
No description provided.