feat: P2P chat push wake-up + register mostro_pubkey whitelist field#590
feat: P2P chat push wake-up + register mostro_pubkey whitelist field#590AndreaDiazCorreia wants to merge 4 commits into
Conversation
…details, and whitelist alignment
… whitelist support - Call /api/notify after sending P2P chat messages to wake the peer's device via the push server - P2P chat events use sharedKey.public, which the server's Nostr listener doesn't match; sender-triggered notification closes that gap - Strictly fire-and-forget: swallow 400/429/network errors without retrying or failing the chat send - Include mostro_pubkey in /api/register when getMostroPubkey callback is set,
WalkthroughThis PR implements the Phase 4 P2P chat push notification sender-triggered wake-up flow: adds a fire-and-forget notifyPeer() call, wires a getMostroPubkey callback for registration, refactors PushNotificationService for DI/testability, and integrates notifyPeer into the P2P send path with tests and docs updates. ChangesP2P Chat Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77f1bd618e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
There is a real correctness issue in the current implementation of notifyPeer().
Right now the method returns early when isPushEnabledInSettings is false. That setting represents whether this device wants to receive push notifications and whether this device's token should stay registered. But notifyPeer() is not about receiving on this device, it is about waking the counterparty's device after a successful P2P chat send.
With the current guard, the behavior becomes:
- sender disables local push in settings
- sender can still send P2P chat messages
/api/notifyis skipped entirely- recipient may be correctly registered and expecting push wake-ups, but never gets one
So the sender's local receive preference incorrectly suppresses the peer's wake-up path. Those concerns should stay separate. The recipient's own registration state already determines whether the push server can deliver anything.
I think notifyPeer() should not be gated on isPushEnabledInSettings. Keep the platform support guard if needed, but the local receive preference should not control outgoing peer wake-ups. Once that is fixed, the rest of the PR looks coherent: the /api/register whitelist alignment, DI/testability improvements, and fire-and-forget notify flow all make sense.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/push_notification_service.dart (1)
147-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat HTTP 202 as successful registration.
registerTokencurrently accepts only200as success. With a202register contract, this returns false and never tracks the trade pubkey for re-registration.Proposed fix
- if (response.statusCode == 200) { - final data = jsonDecode(response.body); - if (data['success'] == true) { + if (response.statusCode == 200 || response.statusCode == 202) { + Map<String, dynamic> data = {}; + try { + data = jsonDecode(response.body) as Map<String, dynamic>; + } catch (_) { + // Keep empty map; 202 is still accepted by contract. + } + if (data['success'] == true || + data['accepted'] == true || + response.statusCode == 202) { debugPrint( 'PushService: Token registered for trade ${_shortenPubkey(tradePubkey)}'); _registeredTradePubkeys.add(tradePubkey); return true; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/push_notification_service.dart` around lines 147 - 166, registerToken currently treats only HTTP 200 as success; update the success logic in the block that checks response.statusCode so that statusCode 200 or 202 are treated as successful registration. Specifically, in registerToken, if response.statusCode == 200 keep the existing jsonDecode(response.body) and the data['success'] == true check, but also treat response.statusCode == 202 as success (no body parsing required): in both success cases call _shortenPubkey(tradePubkey) for the debugPrint, add tradePubkey to _registeredTradePubkeys, and return true; preserve the existing logger.e and false return for other statuses and keep the existing timeout on _httpClient.post.
🧹 Nitpick comments (2)
test/services/push_notification_service_test.dart (1)
169-245: ⚡ Quick winAdd a success-path test for HTTP 202 on
/api/register.These tests currently model successful registration as
200, which can mask regressions against the documented deployed contract (202accepted). Add at least one success test assertingregisterTokenreturns true on202.Suggested test adjustment
group('PushNotificationService.registerToken', () { + test('treats 202 from /api/register as success', () async { + final mockClient = MockClient((request) async { + if (request.url.path == '/api/health') { + return http.Response('{"status":"ok"}', 200); + } + return http.Response('{"accepted":true}', 202); + }); + + final service = _buildService(httpClient: mockClient); + final ok = await service.registerToken(_validPubkey); + expect(ok, isTrue); + }); + test('includes mostro_pubkey when callback returns a non-empty value', () async {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/services/push_notification_service_test.dart` around lines 169 - 245, Add a success-path test that asserts registerToken returns true when the /api/register mock responds with HTTP 202 (Accepted). In the PushNotificationService.registerToken group, add a new test (using MockClient and _buildService like the existing tests) that captures the outgoing request, returns a 202 response body similar to the current 200 cases, then awaits service.registerToken(_validPubkey) and expects the result to be true; reuse captured, mockClient, and response JSON structure from the existing tests so the only change is the status code (202) and the final expect on the returned bool from registerToken.lib/services/push_notification_service.dart (1)
255-256: ⚡ Quick winUse
loggerinstead ofdebugPrintin the new notifyPeer error path.Route this ignored transport error through the shared logger for consistency with app logging policy.
Proposed fix
- debugPrint('PushService: notifyPeer transport error (ignored): $e'); + logger.w('PushService: notifyPeer transport error (ignored): $e');As per coding guidelines: “Always use the pre-configured singleton
loggerinstance … for logging.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/push_notification_service.dart` around lines 255 - 256, In the notifyPeer error path inside PushService replace the debugPrint call with the shared singleton logger: locate the debugPrint('PushService: notifyPeer transport error (ignored): $e') in notifyPeer and change it to use the pre-configured logger (e.g., logger.w or logger.warning) so the ignored transport error is routed through the app's centralized logging; keep the same message and include the error variable $e.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/services/push_notification_service.dart`:
- Around line 147-166: registerToken currently treats only HTTP 200 as success;
update the success logic in the block that checks response.statusCode so that
statusCode 200 or 202 are treated as successful registration. Specifically, in
registerToken, if response.statusCode == 200 keep the existing
jsonDecode(response.body) and the data['success'] == true check, but also treat
response.statusCode == 202 as success (no body parsing required): in both
success cases call _shortenPubkey(tradePubkey) for the debugPrint, add
tradePubkey to _registeredTradePubkeys, and return true; preserve the existing
logger.e and false return for other statuses and keep the existing timeout on
_httpClient.post.
---
Nitpick comments:
In `@lib/services/push_notification_service.dart`:
- Around line 255-256: In the notifyPeer error path inside PushService replace
the debugPrint call with the shared singleton logger: locate the
debugPrint('PushService: notifyPeer transport error (ignored): $e') in
notifyPeer and change it to use the pre-configured logger (e.g., logger.w or
logger.warning) so the ignored transport error is routed through the app's
centralized logging; keep the same message and include the error variable $e.
In `@test/services/push_notification_service_test.dart`:
- Around line 169-245: Add a success-path test that asserts registerToken
returns true when the /api/register mock responds with HTTP 202 (Accepted). In
the PushNotificationService.registerToken group, add a new test (using
MockClient and _buildService like the existing tests) that captures the outgoing
request, returns a 202 response body similar to the current 200 cases, then
awaits service.registerToken(_validPubkey) and expects the result to be true;
reuse captured, mockClient, and response JSON structure from the existing tests
so the only change is the status code (202) and the final expect on the returned
bool from registerToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71a08277-d608-49d8-b027-e8d54ed60719
📒 Files selected for processing (2)
lib/services/push_notification_service.darttest/services/push_notification_service_test.dart
There was a problem hiding this comment.
I did another strict pass after the latest change.
The real correctness issue I had flagged earlier is fixed: notifyPeer() is no longer gated on the sender's local push-receive preference, so outgoing peer wake-ups are not accidentally suppressed.
The follow-up CodeRabbit point is also addressed correctly: registerToken() now treats HTTP 202 Accepted as a successful registration path and still tracks the trade pubkey for future re-registration.
At this point the PR looks coherent end-to-end:
- publish succeeds before
notifyPeer()is fired notifyPeer()remains fire-and-forget and does not break chat send on transport/rate-limit failures/api/registerincludesmostro_pubkeywhen available for trusted-instance whitelisting- the new tests cover the sensitive request-shape and behavior cases
I am comfortable approving this.
1 participant
Summary
Closes Phase 4 of the chat notifications plan: wake the peer's device via FCM when a P2P chat message is sent, and align
/api/registerwith the push server's trusted-instance whitelist.notifyPeer()posts to/api/notifyafterpublishEventsucceeds (fire-and-forget, no retries, swallows 400/429/network errors).tradeKey.public; P2P chat events usesharedKey.public.registerToken()now includesmostro_pubkeywhen agetMostroPubkeycallback is set, wired fromsettingsProvider.mostroPublicKeyinmain.dart. Backwards-compatible: servers without the whitelist enabled ignore the field.docs/plans/CHAT_NOTIFICATIONS_PLAN.mdto match the deployed server contract (202 always, indistinguishable registered/unregistered, byte-identical 429 bodies, real rate limits).Test plan
flutter analyze— cleanflutter test— 368 passed, including 10 new unit tests forPushNotificationServicemostro_pubkeyappears in/api/registerbody via server logsSummary by CodeRabbit
New Features
Documentation
Tests