fix: allow reset wallet to finish #1345
Conversation
WalkthroughThe changes modify a method's visibility in the Changes
Sequence Diagram(s)sequenceDiagram
participant SF as SecurityFragment
participant ABS as AbstractBindServiceActivity
participant VM as WalletViewModel
SF->>ABS: doUnbindService()
SF->>VM: triggerWipe()
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[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 (2)
wallet/src/de/schildbach/wallet/ui/AbstractBindServiceActivity.java (1)
81-86: LGTM! Consider adding documentation.The change to make
doUnbindService()public is safe and necessary for the wallet reset functionality. Consider adding a documentation comment to explain when this method should be called outside the activity lifecycle.Add a documentation comment:
+ /** + * Unbinds from the BlockchainService if currently bound. + * This method is safe to call multiple times and is automatically called in onPause(). + * Only call this method directly when you need to force unbinding outside the activity lifecycle. + */ public void doUnbindService() {wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt (1)
201-201: Add error handling for the activity cast.While the cast is safe in the current implementation, it's good practice to handle potential errors for maintainability.
- (requireActivity() as AbstractBindServiceActivity).doUnbindService() + try { + (requireActivity() as AbstractBindServiceActivity).doUnbindService() + } catch (e: Exception) { + log.error("Failed to unbind service during wallet reset", e) + // Continue with wipe as service will be unbound when activity is destroyed + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wallet/src/de/schildbach/wallet/ui/AbstractBindServiceActivity.java(1 hunks)wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt (1)
197-207: LGTM! The service unbinding fix looks correct.The addition of
doUnbindService()beforetriggerWipe()ensures that the BlockchainService is properly unbound before the wallet reset process begins, which should fix the completion issue.
Issue being fixed or feature implemented
Reset wallet doesn't finish, this will ensure that it does.
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit