fix: use correct store value for limit alert #101
Conversation
Summary of ChangesHello @shanimal08, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the daily transaction limit warning was not appearing correctly due to an incorrect state store being referenced. It refactors the state management by migrating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the exceededDailyLimit state from the account store to the bridge store to fix a bug with the daily limit alert. The changes are logical and the related code has been cleaned up appropriately. I've identified a significant issue where the bridge operation appears to be throwing exceptions, which contradicts our guidelines, and its current catch block silently ignores non-limit related errors, which could impact error diagnosis and user experience. I've also noted a minor unrelated change that could use some clarification. Please see my detailed comments.
| .catch((e) => { | ||
| console.error('[ TAPPLET-BRIDGE ] Bridge operation failed:', e) | ||
| const error = e as Error | ||
| const isLimitError = | ||
| error?.message?.includes(DAILY_LIMIT_ERROR_TYPE) || error?.message?.includes(DAILY_LIMIT_ERROR) | ||
| setExceededDailyLimit(isLimitError) | ||
| error?.message?.includes(DAILY_LIMIT_ERROR) || error?.message?.includes(DAILY_LIMIT_ERROR_TYPE) | ||
| if (isLimitError) { | ||
| console.warn('[ TAPPLET-BRIDGE ] Bridge operation failed:', e) | ||
| setExceededDailyLimit(true) | ||
| setIsModalOpen(false) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The bridgeToTari() function (or similar bridge operation) should handle its own exceptions and not re-throw them, as its callers do not expect to handle exceptions. The presence of this catch block suggests that the bridge operation is throwing exceptions, which contradicts this guideline. Ideally, the bridge operation should be refactored to handle all its internal errors and return a result that indicates success or failure without throwing, thus making this catch block unnecessary. If, however, this catch block remains, the updated error handling has introduced a potential issue: non-limit errors are now silently ignored. Previously, all errors were logged. Now, only limit-related errors are handled, and other errors are swallowed, which could make debugging difficult and lead to a poor user experience as the transaction would fail without any feedback. I suggest re-introducing logging for all errors and considering how other types of failures should be communicated to the user. At a minimum, we should log them as errors.
.catch((e) => {
const error = e as Error
const isLimitError =
error?.message?.includes(DAILY_LIMIT_ERROR) || error?.message?.includes(DAILY_LIMIT_ERROR_TYPE)
if (isLimitError) {
console.warn('[ TAPPLET-BRIDGE ] Bridge operation failed: Daily limit reached.', e)
setExceededDailyLimit(true)
setIsModalOpen(false)
} else {
console.error('[ TAPPLET-BRIDGE ] Bridge operation failed:', e)
// Consider setting a generic error state to show a failure modal to the user.
}
})
References
- The
bridgeToTari()function should handle its own exceptions and not re-throw them, as its callers do not expect to handle exceptions.
store/account.ts
Outdated
|
|
||
| export const setTariAccount = async () => { | ||
| if (process.env.NODE_ENV === 'development') return | ||
| // if (process.env.NODE_ENV === 'development') return |
There was a problem hiding this comment.
This line, which prevents setTariAccount from running in a development environment, has been commented out. This change seems unrelated to the main purpose of this pull request, which is to fix the limit alert. Could you please clarify the reason for this change or remove it if it was unintentional?
There was a problem hiding this comment.
oof got me there robot, commented out for testing, thx 😬
) # IMPORTANT ~~don't merge until tari-project/wxtm-bridge-frontend#101 is merged and deployed with `0.4.1` as the tag~~ merged :D ## Description see tari-project/wxtm-bridge-frontend#101 for full context - updated bridge binary version (to the version where the limit alert display is fixed) ## Motivation and Context - daily limit notification wasn't working ## How Has This Been Tested? - locally: https://github.com/user-attachments/assets/ae33e601-ef46-4b0f-8364-ba2f66226ea8
2 participants
Description
useBridgeStoreMotivation and Context
How Has This Been Tested?
Screen.Recording.2026-01-20.at.18.13.17.mov