fix(tokens): refresh takes forever when no identity defined#290
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new crate-private constant for the "No identities found" message was introduced in the backend module. The token balance query logic now returns an error using this constant instead of a successful message. The UI was updated to recognize this constant and improved its handling and logging of unsupported messages and actions. Changes
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
Addresses a bug where token refresh would hang indefinitely if no identity is loaded by surfacing a dedicated error constant and updating both the backend query and UI handling.
- Introduced a
NO_IDENTITIES_FOUNDconstant for a consistent “no identities” error. - Modified
query_my_token_balancesto return an error when no identities exist. - Updated the TokensScreen to detect and handle the new constant, plus added logging for unsupported messages and actions.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/tokens/tokens_screen.rs | Imported NO_IDENTITIES_FOUND, added a check for it in the refresh logic, and improved logging for messages and actions |
| src/backend_task/tokens/query_my_token_balances.rs | Changed empty‐identity branch to return Err(NO_IDENTITIES_FOUND) instead of a success message |
| src/backend_task/mod.rs | Added NO_IDENTITIES_FOUND constant to centralize the “no identities” error message |
Comments suppressed due to low confidence (1)
src/backend_task/tokens/query_my_token_balances.rs:26
- Add a unit test for
query_my_token_balancesto cover the case when no identities are found, ensuring it returns theNO_IDENTITIES_FOUNDerror.
return Err(NO_IDENTITIES_FOUND.to_string());
| | msg.contains("Failed to fetch token balances") | ||
| | msg.contains("Failed to get estimated rewards") | ||
| | msg.eq(NO_IDENTITIES_FOUND) |
There was a problem hiding this comment.
Use logical OR (||) instead of bitwise OR (|) when combining boolean conditions to enable short-circuit evaluation and improve readability.
| | msg.contains("Failed to fetch token balances") | |
| | msg.contains("Failed to get estimated rewards") | |
| | msg.eq(NO_IDENTITIES_FOUND) | |
| || msg.contains("Failed to fetch token balances") | |
| || msg.contains("Failed to get estimated rewards") | |
| || msg.eq(NO_IDENTITIES_FOUND) |
| pub mod tokens; | ||
| pub mod update_data_contract; | ||
|
|
||
| // TODO: Refactor how we handle errors and messages, and remove it from here |
There was a problem hiding this comment.
[nitpick] Add a doc comment explaining the purpose and usage of the NO_IDENTITIES_FOUND constant to help future maintainers understand its role.
| // TODO: Refactor how we handle errors and messages, and remove it from here | |
| // TODO: Refactor how we handle errors and messages, and remove it from here | |
| /// A constant string used to indicate that no identities were found during a task. | |
| /// This is typically used in error messages or logs to provide a clear and consistent | |
| /// description of the issue. Future refactoring may relocate or replace this constant. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/backend_task/mod.rs (1)
40-41: Good addition for consistency, but address the TODO.The constant helps standardize error messages across the codebase, which improves maintainability. The crate-private visibility is appropriate for this internal constant.
The TODO comment indicates this is a temporary solution. Would you like me to help design a more robust error handling system to replace this string-based approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/backend_task/mod.rs(1 hunks)src/backend_task/tokens/query_my_token_balances.rs(3 hunks)src/ui/tokens/tokens_screen.rs(3 hunks)
🔇 Additional comments (6)
src/backend_task/tokens/query_my_token_balances.rs (2)
3-3: LGTM!Clean import of the standardized constant.
25-27:Details
✅ Verification successful
Excellent fix for the hanging refresh issue!
Converting the no-identities condition from a success message to an error is the right approach. This should prevent the infinite refresh loops described in the PR objectives by allowing calling code to handle this condition explicitly rather than continuing processing.
Please verify that all calling code properly handles this new error condition to ensure graceful user experience:
🏁 Script executed:
#!/bin/bash # Description: Find all callers of query_my_token_balances to verify error handling # Expected: All callers should handle the NO_IDENTITIES_FOUND error appropriately echo "Searching for callers of query_my_token_balances..." rg -A 10 "query_my_token_balances" --type rust echo -e "\nSearching for error handling patterns..." rg -A 5 -B 5 "NO_IDENTITIES_FOUND" --type rustLength of output: 6620
🏁 Script executed:
#!/bin/bash # Locate the definition of BackendTaskSuccessResult for context echo "Searching for BackendTaskSuccessResult definition..." rg -n "struct BackendTaskSuccessResult" -n --type rust || echo "Definition not found"Length of output: 222
All callers handle the new error condition correctly
- In
src/backend_task/tokens/mod.rs, theErr(NO_IDENTITIES_FOUND)is mapped to
"Failed to fetch token balances: No identities found".- In
src/ui/tokens/tokens_screen.rs, the UI checksmsg.contains("Failed to fetch token balances"), which catches this case and setsrefreshing_statustoNotRefreshingwith an appropriate message.No further changes are needed—calling code already handles the
NO_IDENTITIES_FOUNDerror gracefully.src/ui/tokens/tokens_screen.rs (4)
45-46: LGTM! Clean import organization.The import changes properly group
TokenTaskand add the necessaryNO_IDENTITIES_FOUNDconstant. The organization follows Rust conventions and supports the new error handling functionality.
5524-5524: Correct implementation of NO_IDENTITIES_FOUND handling.The addition of the
NO_IDENTITIES_FOUNDcondition properly extends the existing message handling pattern for the MyTokens subscreen. This aligns with the backend changes and ensures consistent error handling.
5529-5533: Excellent debugging improvement.The structured debug logging provides valuable information for troubleshooting unsupported messages. Using
tracing::debug!with structured fields (?msg,?msg_type) follows best practices and will aid in debugging without cluttering logs.
5805-5808: Good improvement to action handling.Replacing the wildcard match with explicit
AppAction::Nonehandling and trace logging for unsupported actions improves code clarity and debugging capabilities. The structured logging will help identify when new action types need explicit handling.
…h-forever-no-identity
Refreshing tokens takes forever when no identity is loaded.
Summary by CodeRabbit