Fix 2 notification bugs#3237
Conversation
julien4215
force-pushed
the
fix-notification
branch
from
ee8c142 to
fa82af9
Compare
|
|
||
| // notifications from socket are only displayed if app is in foreground | ||
| final binding = TestWidgetsFlutterBinding.ensureInitialized(); | ||
| binding.handleAppLifecycleStateChanged(AppLifecycleState.resumed); |
There was a problem hiding this comment.
I don't think this is useful. More useful would be a test that assert that the notification is not shown in background.
There was a problem hiding this comment.
The tests fail without it. I think it doesn't consider that the app is in foreground by default.
There was a problem hiding this comment.
That would be very weird if the app wasn't in foreground by default during tests. The tests would not even work if that was the case.
I can't accept the PR until I have the real explanation for why the tests fail with the new code.
And also it needs a test that shows the notification is not shown in background.
There was a problem hiding this comment.
I have added a test that shows no local notification is disaplayed in background.
When I print the value of lifecycleState in the test, it is null. I don't find it that surprising that the app is not set to foreground during tests since it doesn't run a on an emulator or a device. By looking quickly at the Flutter source code, it seems to leave the initial value empty https://github.com/flutter/flutter/blob/924134a44c189315be2148659913dda1671cbe99/packages/flutter_test/lib/src/window.dart#L320-L328.
julien4215
force-pushed
the
fix-notification
branch
from
fa82af9 to
8030600
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses two notification-related UX/behavior bugs in the challenge flow: ensuring challenge-created notifications are correctly parsed when tapped, and preventing duplicate/overridden challenge notifications when the app is backgrounded.
Changes:
- Fixes the
channelIdforChallengeCreatedNotificationso notification payloads deserialize to the correct type when opened. - Gates socket-driven challenge notifications to only display while the app is foregrounded.
- Updates/extends tests to cover lifecycle-dependent notification display behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/src/model/notifications/notifications.dart |
Corrects ChallengeCreatedNotification.channelId and updates its documentation. |
lib/src/model/challenge/challenge_service.dart |
Adds lifecycle-state gating to suppress socket notifications while backgrounded. |
test/model/challenge/challenge_service_test.dart |
Adds lifecycle setup/reset and a new test ensuring no socket notification while backgrounded. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // only display the notifications if the app is in the foregroud because fcm already | ||
| // shows notifications when the app is in the background | ||
| if (WidgetsBinding.instance.lifecycleState == AppLifecycleState.resumed) { | ||
| await Future.wait( | ||
| _current?.inward.whereNot((challenge) => prevInwardIds.contains(challenge.id)).map(( | ||
| challenge, | ||
| ) async { | ||
| return await notificationService.show(ChallengeNotification(challenge)); | ||
| }) ?? | ||
| <Future<int>>[], | ||
| ); | ||
| } |
@julien4215 were you able to reproduce this? See the copilot comment for the related change you made in challenge_service. I'm very cautious about this, because I don't think we've received complaints about this. I don't want to introduce new problems by solving non-existent ones. |
|
FYI I cherry-picked your commit that fixes the wrong ID. So if we do a release before this PR is merged, at least we'll have this fix. |
julien4215
force-pushed
the
fix-notification
branch
3 times, most recently
from
f71980f to
7164db3
Compare
Yes but it was with the fdroid version. However the behavior should be the same as with FCM. Can you try to reproduce with FCM? I am not familiar with how to test FCM notifications locally. |
notification-bug.webmHere is a video of the issue with fcm. The behavior is a little different than what I got with Unified Push. The notification doesn't get overriden. Instead there are two notifications shown for the same challenge. |
app is in the foregroud because fcm already shows the notification when the app is in the background. Before this fix, when the app is in the background, it might show the notification from the socket (when the socket is still alive in the background) which is later overriden by the notification from fcm resulting in a confusing experience for the user.
when the app is in the background
julien4215
force-pushed
the
fix-notification
branch
from
7164db3 to
1520514
Compare
It is quite hard to tell what fcm considers as background and what lifecycle states correspond to it. Maybe there isn't even a clear correspondance. Anyway it is better to have duplicate notifications than missing a notification so I took copilot changes. If we want something cleaner, I think we will have to rethink how challenge notifications are handled (I have added a TODO so that we don't forget there is room for improvement). Until then, it should somewhat solve duplicated notifications for challenges. |
Fix wrong channel id for challenge created notifications.
The wrong channel id caused the notification to be parsed incorrectly when the user taps on it. Because of that, it would bring the user to the home screen instead of the challenge screen.
Only display the challenge received notifications from the socket if the app is in the foregroud because fcm already shows the notification when the app is in the background.
Before this fix, when the app is in the background, it might show the notification from the socket (when the socket is still alive in the background) which is later overriden by the notification from fcm resulting in a confusing experience for the user.