Conversation
…into beast/finalize-notification
…into beast/finalize-notification
… also unregister + insert new
|
The PR looks solid overall. It correctly implements the remote notification handling, updates the backend endpoint to production, and handles device registration/unregistration lifecycles. However, I have a few observations and questions: 1.
|
n13
left a comment
There was a problem hiding this comment.
Don't send pub key
Avoid accessing mnemonic if not needed
|
I have removed auth from registering remote notifications |
|
The PR looks like a solid implementation of the remote notification features, especially the simplification of the 1. Critical: Wallet Reset is Blocked by Network FailuresIn // mobile-app/lib/v2/screens/settings/settings_screen.dart
Future<void> _resetAndClearData() async {
if (FeatureFlags.enableRemoteNotifications) {
try {
await ref.read(firebaseMessagingServiceProvider).unregisterDevice();
} catch (e) {
if (mounted) {
context.showErrorToaster(message: 'Failed to unregister device: $e');
}
return; // <--- This return aborts the entire reset process!
}
}
// ... clearing data ...
}If the user is offline or the Recommendation: try {
await ref.read(firebaseMessagingServiceProvider).unregisterDevice();
} catch (e) {
debugPrint('Failed to unregister device (non-fatal): $e');
// Continue with reset even if this fails
}2.
|
|
I agree with these ^^^^ |
n13
left a comment
There was a problem hiding this comment.
Reset should not wait definitely not fail.
The rest also are good
|
The PR looks solid and addresses the critical feedback from the previous review. Here is my analysis of the changes: Key Improvements & Fixes
Minor Suggestion (Nit)In 792: await ref.read(firebaseMessagingServiceProvider).insertNewAddress(accountToSave.accountId);The Since the UI doesn't handle errors from this call (they are swallowed inside the service), you can safely run this unawaited to keep the UI snappy: // Recommended: Run unawaited so the sheet closes immediately
ref.read(firebaseMessagingServiceProvider).insertNewAddress(accountToSave.accountId);ConclusionApprove. The critical blockers are resolved. The minor suggestion about |
n13
left a comment
There was a problem hiding this comment.
I am OK with this but please check all uses of the new token functions and make sure none of them block the UI
The UI does not have to wait for this, it's useless to do so
We have in the past often managed to add unnecessary wait loops without user interface, so let's avoid this in the future.
n13
left a comment
There was a problem hiding this comment.
Actually this is a change request after all - none of these call can block the UI
|
The PR has been updated to address the feedback regarding blocking UI calls. I have reviewed the latest changes in SummaryThe author has successfully removed the
Issue Found: Unhandled Future Error in
|
|
One more tiny change request |
Summary
Update the remote message handling to properly mapped into notification data known in the mobile app. Also updated the endpoint to production one.