feat(notifications): add push proxy registration for remote delivery#536
feat(notifications): add push proxy registration for remote delivery#536cogwheel0 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds optional remote push wakeup support via a Firebase-backed token provider, a push proxy HTTP client, and a ChangesRemote Push Notification Feature
Sequence Diagram(s)sequenceDiagram
participant App as NotificationSettingsPage
participant Service as RemotePushService
participant API as ApiService
participant Proxy as RemotePushProxyClient
participant Store as RemotePushSubscriptionStore
participant Firebase as FirebaseRemotePushTokenProvider
rect rgba(100, 149, 237, 0.5)
note over App,Firebase: Admin enables server auto-configure
App->>API: setAdminUserWebhooksEnabled(true)
App->>API: refreshBackendConfig()
end
rect rgba(144, 238, 144, 0.5)
note over App,Firebase: User enables system notifications
App->>Service: syncForCurrentSettings(requestPermission: true)
Service->>Firebase: initialize() + requestPermission()
Service->>Firebase: getDeviceToken()
Firebase-->>Service: RemotePushDeviceToken
Service->>Store: getOrCreateInstallationId()
Service->>Store: read(serverId)
Service->>Proxy: register(serverId, serverUrl, userId, token)
Proxy-->>Service: RemotePushRegistration (subscriptionId, webhookUrl)
Service->>API: updateUserNotificationWebhook(webhookUrl)
Service->>Store: write(newSubscription)
Service->>Proxy: unregister(previousSubscription)
Service-->>App: RemotePushSyncResult(registered)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (trimmed.isEmpty) return null; | ||
| final uri = Uri.tryParse(trimmed); | ||
| if (uri == null || !uri.hasScheme || uri.host.isEmpty) return null; | ||
| if (uri.scheme != 'https' && uri.scheme != 'http') return null; |
There was a problem hiding this comment.
A build can set CONDUIT_PUSH_PROXY_BASE_URL to an http URL and still pass isConfigured. That sends device push tokens, server URLs, user IDs, and installation IDs to the proxy without TLS during registration.
Context Used: AGENTS.md (source)
| ref | ||
| .read(remotePushServiceProvider) |
There was a problem hiding this comment.
Signing out of one session now calls unregisterAllLocalSubscriptions(), which iterates every stored remote_push_subscription_v1_* entry. A user with multiple saved servers can sign out of one account and silently unregister remote notifications for the others until each server is opened and synced again.
Context Used: AGENTS.md (source)
| unawaited(remotePush.start()); | ||
| unawaited(remotePush.syncForCurrentSettings()); |
There was a problem hiding this comment.
Remote push sync runs only when this keepAlive listener is first built. If the active server changes without a notification-setting toggle or Firebase token refresh, the socket listener rebinds to the new socket but the new server never gets its webhook registered, leaving remote notifications stale or missing for that server.
Context Used: AGENTS.md (source)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ); | ||
| }), | ||
| ); | ||
| unawaited( |
There was a problem hiding this comment.
🟡 Medium providers/app_startup_providers.dart:121
unregisterAllLocalSubscriptions() is fire-and-forget via unawaited, so a new subscription can be written for the same serverId before the snapshot from the previous sign-out is fully processed. The still-running cleanup then deletes that new local record while the server-side push registration remains active. This leaves the app with a live remote-push registration and no local tracking, so subsequent sign-out or disable flows cannot unregister it and the device keeps receiving pushes. Consider awaiting the cleanup (or guarding it against concurrent writes) before new subscriptions are registered.
Also found in 1 other location(s)
lib/features/notifications/views/notification_settings_page.dart:183
When the system-notifications toggle is turned on,
_setSystemstartsremotePushService.syncForCurrentSettings()withunawaited(...)and discards itsRemotePushSyncResult. Any registration failure (serverUserWebhooksDisabled,permissionDenied,existingWebhookConflict,tokenUnavailable,failed, etc.) is therefore silent: the switch stays enabled even though remote/background delivery was not configured. This leaves users believing notifications are active when the new push-registration step actually failed.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/core/providers/app_startup_providers.dart around line 121:
`unregisterAllLocalSubscriptions()` is fire-and-forget via `unawaited`, so a new subscription can be written for the same `serverId` before the snapshot from the previous sign-out is fully processed. The still-running cleanup then deletes that new local record while the server-side push registration remains active. This leaves the app with a live remote-push registration and no local tracking, so subsequent sign-out or disable flows cannot unregister it and the device keeps receiving pushes. Consider awaiting the cleanup (or guarding it against concurrent writes) before new subscriptions are registered.
Also found in 1 other location(s):
- lib/features/notifications/views/notification_settings_page.dart:183 -- When the system-notifications toggle is turned on, `_setSystem` starts `remotePushService.syncForCurrentSettings()` with `unawaited(...)` and discards its `RemotePushSyncResult`. Any registration failure (`serverUserWebhooksDisabled`, `permissionDenied`, `existingWebhookConflict`, `tokenUnavailable`, `failed`, etc.) is therefore silent: the switch stays enabled even though remote/background delivery was not configured. This leaves users believing notifications are active when the new push-registration step actually failed.
| stackTrace: st, | ||
| scope: 'notifications/push', | ||
| ); | ||
| return const <RemotePushSubscription>[]; |
There was a problem hiding this comment.
🟡 Medium services/remote_push_subscription_store.dart:56
readAll() catches every _storage.readAll() failure and returns an empty list. When unregisterAllLocalSubscriptions() calls this during sign-out, a transient keychain/keystore error causes the caller to see zero subscriptions, skip all unregister calls, and skip deleting local records — leaving the previous account's webhooks active with no retry path. Consider rethrowing after logging so the error propagates to the caller.
- return const <RemotePushSubscription>[];
+ rethrow;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/features/notifications/services/remote_push_subscription_store.dart around line 56:
`readAll()` catches every `_storage.readAll()` failure and returns an empty list. When `unregisterAllLocalSubscriptions()` calls this during sign-out, a transient keychain/keystore error causes the caller to see zero subscriptions, skip all unregister calls, and skip deleting local records — leaving the previous account's webhooks active with no retry path. Consider rethrowing after logging so the error propagates to the caller.
| final remotePush = ref.read(remotePushServiceProvider); | ||
| unawaited(remotePush.start()); | ||
| unawaited(remotePush.syncForCurrentSettings()); | ||
| _remoteTapSub = remotePush.taps.listen((tap) { |
There was a problem hiding this comment.
🟡 Medium providers/notification_socket_listener.dart:147
_remoteTapSub routes remote push taps straight into _handleTap using only NotificationTap.kind and sourceId, with no check that the tap belongs to the current user/session. Remote push payloads carry no server or account identity, and logout only clears local notifications — FCM notifications already in the OS tray remain. When a user signs out or switches servers and taps a stale remote notification, this handler deep-links the old sourceId into the current session, reopening the wrong chat/channel or routing the deep link against the wrong backend.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/features/notifications/providers/notification_socket_listener.dart around line 147:
`_remoteTapSub` routes remote push taps straight into `_handleTap` using only `NotificationTap.kind` and `sourceId`, with no check that the tap belongs to the current user/session. Remote push payloads carry no server or account identity, and logout only clears local notifications — FCM notifications already in the OS tray remain. When a user signs out or switches servers and taps a stale remote notification, this handler deep-links the old `sourceId` into the current session, reopening the wrong chat/channel or routing the deep link against the wrong backend.
| return _requireResponseMap(response.data, 'admin config'); | ||
| } | ||
|
|
||
| Future<Map<String, dynamic>> setAdminUserWebhooksEnabled(bool enabled) async { |
There was a problem hiding this comment.
🟡 Medium services/api_service.dart:706
setAdminUserWebhooksEnabled performs a read-modify-write on the full admin config: it GETs the entire document, changes one key, then POSTs the whole stale snapshot back. If any other admin session modifies a different setting between the GET and the POST, this call silently reverts that change. Since only ENABLE_USER_WEBHOOKS needs updating, consider posting only that field to avoid overwriting concurrent changes.
Also found in 1 other location(s)
lib/features/notifications/services/remote_push_subscription_store.dart:15
getOrCreateInstallationId()performs a non-atomic read/then-write on_installationKey.RemotePushService.syncForCurrentSettings()can be started concurrently from app startup, settings toggles, and token refresh callbacks, so two first-run calls can both readnull, generate different UUIDs, and both register separate remote subscriptions. Only the last-written installation/subscription remains in secure storage, leaving the other proxy subscription orphaned and still receiving pushes with no way to unregister it later.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/core/services/api_service.dart around line 706:
`setAdminUserWebhooksEnabled` performs a read-modify-write on the full admin config: it GETs the entire document, changes one key, then POSTs the whole stale snapshot back. If any other admin session modifies a different setting between the GET and the POST, this call silently reverts that change. Since only `ENABLE_USER_WEBHOOKS` needs updating, consider posting only that field to avoid overwriting concurrent changes.
Also found in 1 other location(s):
- lib/features/notifications/services/remote_push_subscription_store.dart:15 -- `getOrCreateInstallationId()` performs a non-atomic read/then-write on `_installationKey`. `RemotePushService.syncForCurrentSettings()` can be started concurrently from app startup, settings toggles, and token refresh callbacks, so two first-run calls can both read `null`, generate different UUIDs, and both register separate remote subscriptions. Only the last-written installation/subscription remains in secure storage, leaving the other proxy subscription orphaned and still receiving pushes with no way to unregister it later.
| ); | ||
| final nextWebhookUrl = _normalizeStringForWebhookMerge(webhookUrl); | ||
|
|
||
| if (currentWebhookUrl != null && currentWebhookUrl != expectedWebhookUrl) { |
There was a problem hiding this comment.
🟠 High services/api_service.dart:118
_mergeUserNotificationWebhookSettings throws UserNotificationWebhookConflictException whenever an existing webhook_url is present and expectedCurrentWebhookUrl is null, even when the existing URL already matches webhookUrl. After an app reinstall or cleared local store, the first sync passes expectedCurrentWebhookUrl == null; if the server still has the previous webhook, line 118 rejects the idempotent update and the caller unregisters the new proxy subscription, leaving remote push broken. Consider treating expectedCurrentWebhookUrl == null as a match when currentWebhookUrl == nextWebhookUrl.
if (currentWebhookUrl != null &&
- currentWebhookUrl != expectedWebhookUrl) {
+ currentWebhookUrl != nextWebhookUrl &&
+ currentWebhookUrl != expectedWebhookUrl) {
throw UserNotificationWebhookConflictException(🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/core/services/api_service.dart around line 118:
`_mergeUserNotificationWebhookSettings` throws `UserNotificationWebhookConflictException` whenever an existing `webhook_url` is present and `expectedCurrentWebhookUrl` is `null`, even when the existing URL already matches `webhookUrl`. After an app reinstall or cleared local store, the first sync passes `expectedCurrentWebhookUrl == null`; if the server still has the previous webhook, line 118 rejects the idempotent update and the caller unregisters the new proxy subscription, leaving remote push broken. Consider treating `expectedCurrentWebhookUrl == null` as a match when `currentWebhookUrl == nextWebhookUrl`.
| if (nestedLdap is bool) enableLdap = nestedLdap; | ||
| final nestedLoginForm = features['enable_login_form']; | ||
| if (nestedLoginForm is bool) enableLoginForm = nestedLoginForm; | ||
| final nestedUserWebhooks = features['enable_user_webhooks']; |
There was a problem hiding this comment.
🟡 Medium models/backend_config.dart:392
In BackendConfig.fromJson, the nested fallback for features['enable_user_webhooks'] overwrites the already-parsed top-level json['enable_user_webhooks'] value unconditionally, unlike the other auth fields which only overwrite when the top-level value wasn't set. If a server sends both enable_user_webhooks and features.enable_user_webhooks with different values during a rollout, the app ignores the canonical top-level flag and uses the legacy nested value. Add a guard so the nested value is only used when the top-level one was absent.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/core/models/backend_config.dart around line 392:
In `BackendConfig.fromJson`, the nested fallback for `features['enable_user_webhooks']` overwrites the already-parsed top-level `json['enable_user_webhooks']` value unconditionally, unlike the other auth fields which only overwrite when the top-level value wasn't set. If a server sends both `enable_user_webhooks` and `features.enable_user_webhooks` with different values during a rollout, the app ignores the canonical top-level flag and uses the legacy nested value. Add a guard so the nested value is only used when the top-level one was absent.
|
|
||
| import 'remote_push_models.dart'; | ||
|
|
||
| class RemotePushProxyClient { |
There was a problem hiding this comment.
🟠 High services/remote_push_proxy_client.dart:7
register() transmits the APNs/FCM push token, user_id, and server metadata to _baseUri without enforcing https. When the build config supplies an http URL (e.g. a misconfigured CONDUIT_PUSH_PROXY_BASE_URL), this code sends those secrets in cleartext over the network. Consider rejecting non-https base URIs in the constructor or register() so a misconfiguration fails fast instead of leaking tokens.
Also found in 1 other location(s)
lib/features/notifications/services/remote_push_build_config.dart:58
proxyBaseUriaccepts bothhttpsandhttpURLs, so a build can be marked configured with an insecureCONDUIT_PUSH_PROXY_BASE_URL.RemotePushProxyClientwill then send device push tokens plus server/user/installation identifiers over cleartext HTTP, allowing interception or tampering of remote-push registration traffic.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/features/notifications/services/remote_push_proxy_client.dart around line 7:
`register()` transmits the APNs/FCM push token, `user_id`, and server metadata to `_baseUri` without enforcing `https`. When the build config supplies an `http` URL (e.g. a misconfigured `CONDUIT_PUSH_PROXY_BASE_URL`), this code sends those secrets in cleartext over the network. Consider rejecting non-`https` base URIs in the constructor or `register()` so a misconfiguration fails fast instead of leaking tokens.
Also found in 1 other location(s):
- lib/features/notifications/services/remote_push_build_config.dart:58 -- `proxyBaseUri` accepts both `https` and `http` URLs, so a build can be marked configured with an insecure `CONDUIT_PUSH_PROXY_BASE_URL`. `RemotePushProxyClient` will then send device push tokens plus server/user/installation identifiers over cleartext HTTP, allowing interception or tampering of remote-push registration traffic.
| ); | ||
| } | ||
| if (Platform.isIOS) { | ||
| for (var attempt = 0; attempt < 6; attempt++) { |
There was a problem hiding this comment.
🟡 Medium services/remote_push_token_provider.dart:139
On iOS, getDeviceToken() polls FirebaseMessaging.instance.getAPNSToken() only six times with 350 ms delays (~2.1 s total) and returns null if no APNs token is available yet. Because onTokenRefresh only fires for FCM tokens, not APNs tokens, there is no later callback to retry once APNs registration completes, so a slow or offline APNs registration at launch leaves the device unregistered for push for the entire session. Consider increasing the retry window or retrying later (e.g., on app focus / connectivity restore) so registration eventually succeeds.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/features/notifications/services/remote_push_token_provider.dart around line 139:
On iOS, `getDeviceToken()` polls `FirebaseMessaging.instance.getAPNSToken()` only six times with 350 ms delays (~2.1 s total) and returns `null` if no APNs token is available yet. Because `onTokenRefresh` only fires for FCM tokens, not APNs tokens, there is no later callback to retry once APNs registration completes, so a slow or offline APNs registration at launch leaves the device unregistered for push for the entire session. Consider increasing the retry window or retrying later (e.g., on app focus / connectivity restore) so registration eventually succeeds.
| return _proxyClientFactory(uri); | ||
| } | ||
|
|
||
| Future<BackendConfig?> _freshBackendConfig() async { |
There was a problem hiding this comment.
🟠 High services/remote_push_service.dart:294
enableForActiveServer() calls disableForActiveServer(removeServerWebhook: true) whenever _freshBackendConfig() returns null or stale data after a refresh failure, because line 143 checks backendConfig?.enableUserWebhooks != true. A transient network error refreshing backendConfigProvider tears down an existing push subscription even though the server's webhook capability was never actually confirmed to be disabled. Consider distinguishing a refresh failure from a confirmed enableUserWebhooks: false so transient errors don't destroy an active subscription.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @lib/features/notifications/services/remote_push_service.dart around line 294:
`enableForActiveServer()` calls `disableForActiveServer(removeServerWebhook: true)` whenever `_freshBackendConfig()` returns `null` or stale data after a refresh failure, because line 143 checks `backendConfig?.enableUserWebhooks != true`. A transient network error refreshing `backendConfigProvider` tears down an existing push subscription even though the server's webhook capability was never actually confirmed to be disabled. Consider distinguishing a refresh failure from a confirmed `enableUserWebhooks: false` so transient errors don't destroy an active subscription.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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.
Inline comments:
In `@lib/core/models/backend_config.dart`:
- Around line 392-395: The nested `features['enable_user_webhooks']` fallback in
`BackendConfig` is overwriting the canonical top-level `enable_user_webhooks`
value when both are present. Update the parsing logic to track whether the
top-level flag was already provided before applying the nested fallback, and
only read the nested value when the top-level key was absent. Keep the fix
localized to the `BackendConfig` mapping/parsing code that sets
`enableUserWebhooks`.
In `@lib/core/services/api_service.dart`:
- Around line 118-123: The conflict guard in api_service.dart’s webhook update
flow is too eager and throws even when no expected current URL was provided.
Update the check around the UserNotificationWebhookConflictException in
updateUserNotificationWebhook() / the webhook comparison logic so it only
compares currentWebhookUrl against expectedWebhookUrl when expectedWebhookUrl is
actually supplied, and otherwise allows the update to proceed idempotently.
In `@lib/features/notifications/services/remote_push_build_config.dart`:
- Around line 53-59: The proxyBaseUri validation in RemotePushBuildConfig
currently allows plain HTTP, which should be rejected for the push registration
flow. Update the proxyBaseUri getter to only accept HTTPS by default, or
explicitly allow HTTP only for localhost/debug-only cases, while keeping the
existing Uri parsing and host checks intact. Focus on the proxyBaseUrl-to-Uri
validation logic in proxyBaseUri so misconfigured non-secure endpoints are not
accepted.
In `@lib/features/notifications/services/remote_push_proxy_client.dart`:
- Around line 8-13: `RemotePushProxyClient` currently creates its own default
`http.Client` but never closes it, so the register/unregister flows in
`RemotePushService` can leak resources. Update `RemotePushProxyClient` to either
expose a `close()`/`dispose()` method that shuts down the internally created
client when no external client was injected, or refactor `RemotePushService` to
own and reuse a shared `http.Client` across `register` and `unregister`. Use the
`RemotePushProxyClient` constructor and the service methods that instantiate it
to wire up the lifecycle cleanup.
In `@lib/features/notifications/services/remote_push_service.dart`:
- Around line 73-75: Serialize all remote-push mutation paths in
RemotePushService so token refreshes, syncForCurrentSettings(), and
disableForActiveServer() cannot overlap for the same server. Add a single
in-flight guard or per-server mutex around the shared mutation flow before
touching the proxy, store, or webhook, and ensure the listener in
tokenRefreshes.listen waits on that same serialized path instead of firing
syncForCurrentSettings() unawaited. Update the mutation entry points in
RemotePushService to share the same lock so concurrent enables/disables cannot
race on previous subscription state or proxy registration.
- Around line 159-208: The sync flow in remote_push_service.dart updates the
server webhook before the new RemotePushSubscription is durably stored, so a
later _store.write(subscription) failure leaves remote state active with no
local record. Fix _sync or the surrounding registration sequence by making
persistence happen before calling api.updateUserNotificationWebhook(), or add
compensation on write failure that clears the webhook and unregisters the newly
created subscription via
_unregisterBestEffort/_unregisterSubscriptionBestEffort. Keep the existing error
handling around DebugLogger.error and RemotePushSyncResult, but ensure the
server-side webhook is only changed once local storage is safe.
- Around line 220-247: `disableForActiveServer()` and
`unregisterAllLocalSubscriptions()` are deleting the stored subscription even
when cleanup fails, which makes retries impossible. Update the cleanup flow
around `_unregisterSubscriptionBestEffort()` and
`api.updateUserNotificationWebhook()` so the local record is only removed after
remote cleanup succeeds, or mark the subscription as pending cleanup for later
retry. Keep the durable `subscriptionId`/`proxyBaseUrl` in `_store` on failure
so failed cleanup can be retried instead of being lost.
In `@lib/features/notifications/services/remote_push_token_provider.dart`:
- Around line 209-217: The source ID selection in remote_push_token_provider’s
token parsing currently uses one shared fallback list, which can pick the wrong
identifier for a notification kind like channel_message when both chat_id and
channel_id are present. Update the source lookup logic around the sourceId
resolution to keep the generic conduit_source_id and source_id fallbacks first,
then choose kind-specific fields based on the parsed notification kind (for
example, prefer channel_id for channel messages and chat_id for chat messages).
Use the existing parsing path in remote_push_token_provider to branch by
notification kind before calling _firstNonEmptyString.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c3e9f81-fe0f-44ea-bec9-05808548c6c8
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
docs/push-notifications.mdios/Runner.xcodeproj/project.pbxprojios/Runner/Info.plistios/Runner/Runner.entitlementslib/core/models/backend_config.dartlib/core/models/server_user_settings.dartlib/core/providers/app_startup_providers.dartlib/core/services/api_service.dartlib/features/notifications/providers/notification_socket_listener.dartlib/features/notifications/services/remote_push_build_config.dartlib/features/notifications/services/remote_push_models.dartlib/features/notifications/services/remote_push_proxy_client.dartlib/features/notifications/services/remote_push_service.dartlib/features/notifications/services/remote_push_subscription_store.dartlib/features/notifications/services/remote_push_token_provider.dartlib/features/notifications/views/notification_settings_page.dartlib/l10n/app_en.arbpubspec.yamltest/core/models/server_backed_settings_models_test.darttest/core/services/api_service_notification_webhook_test.darttest/features/notifications/remote_push_token_provider_test.dart
| final nestedUserWebhooks = features['enable_user_webhooks']; | ||
| if (nestedUserWebhooks is bool) { | ||
| enableUserWebhooks = nestedUserWebhooks; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve top-level enable_user_webhooks precedence.
This fallback block overwrites the canonical top-level value whenever both keys are present. A payload like {'enable_user_webhooks': true, 'features': {'enable_user_webhooks': false}} now parses as false, so the UI can hide webhook support even when the server explicitly enabled it. Track whether the top-level key was seen before applying the nested fallback.
Suggested fix
- bool enableUserWebhooks = false;
+ bool enableUserWebhooks = false;
+ var hasTopLevelEnableUserWebhooks = false;
...
final userWebhooksValue = json['enable_user_webhooks'];
- if (userWebhooksValue is bool) enableUserWebhooks = userWebhooksValue;
+ if (userWebhooksValue is bool) {
+ enableUserWebhooks = userWebhooksValue;
+ hasTopLevelEnableUserWebhooks = true;
+ }
...
final nestedUserWebhooks = features['enable_user_webhooks'];
- if (nestedUserWebhooks is bool) {
+ if (nestedUserWebhooks is bool && !hasTopLevelEnableUserWebhooks) {
enableUserWebhooks = nestedUserWebhooks;
}🤖 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/core/models/backend_config.dart` around lines 392 - 395, The nested
`features['enable_user_webhooks']` fallback in `BackendConfig` is overwriting
the canonical top-level `enable_user_webhooks` value when both are present.
Update the parsing logic to track whether the top-level flag was already
provided before applying the nested fallback, and only read the nested value
when the top-level key was absent. Keep the fix localized to the `BackendConfig`
mapping/parsing code that sets `enableUserWebhooks`.
| if (currentWebhookUrl != null && currentWebhookUrl != expectedWebhookUrl) { | ||
| throw UserNotificationWebhookConflictException( | ||
| currentWebhookUrl: currentWebhookUrl, | ||
| expectedWebhookUrl: expectedWebhookUrl, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the conflict check truly optional.
This guard throws on any existing webhook when expectedCurrentWebhookUrl is omitted, so updateUserNotificationWebhook() is no longer idempotent. Retrying with the same target URL—or recovering after local state loss—still raises UserNotificationWebhookConflictException instead of converging. Gate the conflict on a supplied expected value.
Suggested fix
- if (currentWebhookUrl != null && currentWebhookUrl != expectedWebhookUrl) {
+ if (expectedWebhookUrl != null &&
+ currentWebhookUrl != null &&
+ currentWebhookUrl != expectedWebhookUrl) {
throw UserNotificationWebhookConflictException(
currentWebhookUrl: currentWebhookUrl,
expectedWebhookUrl: expectedWebhookUrl,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (currentWebhookUrl != null && currentWebhookUrl != expectedWebhookUrl) { | |
| throw UserNotificationWebhookConflictException( | |
| currentWebhookUrl: currentWebhookUrl, | |
| expectedWebhookUrl: expectedWebhookUrl, | |
| ); | |
| } | |
| if (expectedWebhookUrl != null && | |
| currentWebhookUrl != null && | |
| currentWebhookUrl != expectedWebhookUrl) { | |
| throw UserNotificationWebhookConflictException( | |
| currentWebhookUrl: currentWebhookUrl, | |
| expectedWebhookUrl: expectedWebhookUrl, | |
| ); | |
| } |
🤖 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/core/services/api_service.dart` around lines 118 - 123, The conflict
guard in api_service.dart’s webhook update flow is too eager and throws even
when no expected current URL was provided. Update the check around the
UserNotificationWebhookConflictException in updateUserNotificationWebhook() /
the webhook comparison logic so it only compares currentWebhookUrl against
expectedWebhookUrl when expectedWebhookUrl is actually supplied, and otherwise
allows the update to proceed idempotently.
| Uri? get proxyBaseUri { | ||
| final trimmed = proxyBaseUrl.trim(); | ||
| if (trimmed.isEmpty) return null; | ||
| final uri = Uri.tryParse(trimmed); | ||
| if (uri == null || !uri.hasScheme || uri.host.isEmpty) return null; | ||
| if (uri.scheme != 'https' && uri.scheme != 'http') return null; | ||
| return uri; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject plain-HTTP push proxy URLs.
proxyBaseUri currently accepts http, but the registration flow sends the device push token, user ID, installation ID, and webhook URL through this base URI. A misconfigured build would leak notification credentials in plaintext. Restrict this to https, or gate http to explicit localhost/debug-only cases.
🤖 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/features/notifications/services/remote_push_build_config.dart` around
lines 53 - 59, The proxyBaseUri validation in RemotePushBuildConfig currently
allows plain HTTP, which should be rejected for the push registration flow.
Update the proxyBaseUri getter to only accept HTTPS by default, or explicitly
allow HTTP only for localhost/debug-only cases, while keeping the existing Uri
parsing and host checks intact. Focus on the proxyBaseUrl-to-Uri validation
logic in proxyBaseUri so misconfigured non-secure endpoints are not accepted.
| RemotePushProxyClient({required Uri baseUri, http.Client? httpClient}) | ||
| : _baseUri = baseUri, | ||
| _httpClient = httpClient ?? http.Client(); | ||
|
|
||
| final Uri _baseUri; | ||
| final http.Client _httpClient; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and its nearby context.
git ls-files 'lib/features/notifications/services/remote_push_proxy_client.dart' 'lib/features/notifications/services/remote_push_service.dart'
wc -l lib/features/notifications/services/remote_push_proxy_client.dart lib/features/notifications/services/remote_push_service.dart
cat -n lib/features/notifications/services/remote_push_proxy_client.dart | sed -n '1,220p'
printf '\n--- remote_push_service ---\n'
cat -n lib/features/notifications/services/remote_push_service.dart | sed -n '1,260p'
# Search for any close/dispose/ownership handling around http.Client and RemotePushProxyClient.
rg -n "RemotePushProxyClient|http\.Client|close\(\)|dispose\(|Client\(" lib/features/notifications -SRepository: cogwheel0/conduit
Length of output: 17071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cleanup paths and the helper methods that create proxy clients.
sed -n '250,340p' lib/features/notifications/services/remote_push_service.dart
# Look for any custom disposal/closing behavior around RemotePushProxyClient or http.Client across the repo.
rg -n "RemotePushProxyClient.*close|RemotePushProxyClient.*dispose|_httpClient\.close\(|http\.Client\(\)" lib -SRepository: cogwheel0/conduit
Length of output: 2611
Close internally owned http.Client instances. RemotePushService creates fresh RemotePushProxyClient instances for register/unregister flows, but the default http.Client in RemotePushProxyClient is never disposed. Add a close()/dispose() path or share an injected client from the service.
🤖 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/features/notifications/services/remote_push_proxy_client.dart` around
lines 8 - 13, `RemotePushProxyClient` currently creates its own default
`http.Client` but never closes it, so the register/unregister flows in
`RemotePushService` can leak resources. Update `RemotePushProxyClient` to either
expose a `close()`/`dispose()` method that shuts down the internally created
client when no external client was injected, or refactor `RemotePushService` to
own and reuse a shared `http.Client` across `register` and `unregister`. Use the
`RemotePushProxyClient` constructor and the service methods that instantiate it
to wire up the lifecycle cleanup.
| _tokenRefreshSub ??= _tokenProvider.tokenRefreshes.listen((_) { | ||
| unawaited(syncForCurrentSettings()); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize remote-push mutations per server.
tokenRefreshes.listen fires syncForCurrentSettings() unawaited, and the settings UI also drives syncForCurrentSettings() / disableForActiveServer() independently. Because these paths share no lock, two enables can read the same previous subscription, create separate proxy registrations, race on webhook updates, and leave orphaned proxy state or delete the just-written local record. Gate all mutation paths behind a single in-flight operation, or a per-server mutex, before touching the proxy, store, or server webhook.
Also applies to: 88-109, 112-200, 212-247
🤖 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/features/notifications/services/remote_push_service.dart` around lines 73
- 75, Serialize all remote-push mutation paths in RemotePushService so token
refreshes, syncForCurrentSettings(), and disableForActiveServer() cannot overlap
for the same server. Add a single in-flight guard or per-server mutex around the
shared mutation flow before touching the proxy, store, or webhook, and ensure
the listener in tokenRefreshes.listen waits on that same serialized path instead
of firing syncForCurrentSettings() unawaited. Update the mutation entry points
in RemotePushService to share the same lock so concurrent enables/disables
cannot race on previous subscription state or proxy registration.
| final installationId = await _store.getOrCreateInstallationId(); | ||
| final registration = await client.register( | ||
| serverId: api.serverConfig.id, | ||
| serverUrl: api.serverConfig.url, | ||
| userId: user.id, | ||
| installationId: installationId, | ||
| deviceToken: deviceToken, | ||
| ); | ||
| final subscription = RemotePushSubscription( | ||
| serverId: api.serverConfig.id, | ||
| userId: user.id, | ||
| installationId: installationId, | ||
| subscriptionId: registration.subscriptionId, | ||
| webhookUrl: registration.webhookUrl, | ||
| proxyBaseUrl: proxyBaseUri.toString(), | ||
| platform: deviceToken.platform, | ||
| tokenType: deviceToken.tokenType, | ||
| updatedAt: DateTime.now(), | ||
| ); | ||
|
|
||
| try { | ||
| await api.updateUserNotificationWebhook( | ||
| webhookUrl: subscription.webhookUrl, | ||
| expectedCurrentWebhookUrl: previous?.webhookUrl, | ||
| ); | ||
| } on UserNotificationWebhookConflictException catch (e) { | ||
| await _unregisterBestEffort(client, subscription); | ||
| return RemotePushSyncResult( | ||
| RemotePushSyncStatus.existingWebhookConflict, | ||
| error: e, | ||
| ); | ||
| } catch (e) { | ||
| await _unregisterBestEffort(client, subscription); | ||
| rethrow; | ||
| } | ||
|
|
||
| await _store.write(subscription); | ||
| if (previous != null && | ||
| previous.subscriptionId != subscription.subscriptionId) { | ||
| await _unregisterSubscriptionBestEffort(previous); | ||
| } | ||
| return const RemotePushSyncResult(RemotePushSyncStatus.registered); | ||
| } catch (e, st) { | ||
| DebugLogger.error( | ||
| 'remote push sync failed', | ||
| error: e, | ||
| stackTrace: st, | ||
| scope: 'notifications/push', | ||
| ); | ||
| return RemotePushSyncResult(RemotePushSyncStatus.failed, error: e); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't update the server webhook before local persistence is durable.
If updateUserNotificationWebhook() succeeds but _store.write(subscription) throws, the catch at Lines 201-208 reports failed after the proxy subscription and server webhook are already live. At that point there is no local record left to disable or unregister that remote state later. Persist the new subscription before mutating the server webhook, or compensate on write failure by clearing the webhook and unregistering the new proxy subscription.
🤖 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/features/notifications/services/remote_push_service.dart` around lines
159 - 208, The sync flow in remote_push_service.dart updates the server webhook
before the new RemotePushSubscription is durably stored, so a later
_store.write(subscription) failure leaves remote state active with no local
record. Fix _sync or the surrounding registration sequence by making persistence
happen before calling api.updateUserNotificationWebhook(), or add compensation
on write failure that clears the webhook and unregisters the newly created
subscription via _unregisterBestEffort/_unregisterSubscriptionBestEffort. Keep
the existing error handling around DebugLogger.error and RemotePushSyncResult,
but ensure the server-side webhook is only changed once local storage is safe.
| if (removeServerWebhook) { | ||
| try { | ||
| await api.updateUserNotificationWebhook( | ||
| webhookUrl: null, | ||
| expectedCurrentWebhookUrl: subscription.webhookUrl, | ||
| ); | ||
| } on UserNotificationWebhookConflictException { | ||
| // The user or another integration replaced the webhook; leave it alone. | ||
| } catch (e, st) { | ||
| DebugLogger.error( | ||
| 'failed to remove remote push webhook', | ||
| error: e, | ||
| stackTrace: st, | ||
| scope: 'notifications/push', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await _unregisterSubscriptionBestEffort(subscription); | ||
| await _store.delete(subscription.serverId); | ||
| } | ||
|
|
||
| Future<void> unregisterAllLocalSubscriptions() async { | ||
| final subscriptions = await _store.readAll(); | ||
| for (final subscription in subscriptions) { | ||
| await _unregisterSubscriptionBestEffort(subscription); | ||
| await _store.delete(subscription.serverId); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep failed cleanup operations retryable.
disableForActiveServer() logs webhook-removal failures, and _unregisterSubscriptionBestEffort() swallows proxy errors, but both callers still delete the stored subscription afterward. That drops the only durable subscriptionId/proxyBaseUrl needed to retry cleanup, so disable/sign-out can leave orphaned proxy registrations or a server webhook still pointing at them. Only delete the local record after cleanup succeeds, or persist a pending-cleanup state for retry.
Also applies to: 250-266
🤖 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/features/notifications/services/remote_push_service.dart` around lines
220 - 247, `disableForActiveServer()` and `unregisterAllLocalSubscriptions()`
are deleting the stored subscription even when cleanup fails, which makes
retries impossible. Update the cleanup flow around
`_unregisterSubscriptionBestEffort()` and `api.updateUserNotificationWebhook()`
so the local record is only removed after remote cleanup succeeds, or mark the
subscription as pending cleanup for later retry. Keep the durable
`subscriptionId`/`proxyBaseUrl` in `_store` on failure so failed cleanup can be
retried instead of being lost.
| final sourceId = _firstNonEmptyString([ | ||
| data['conduit_source_id'], | ||
| data['source_id'], | ||
| data['sourceId'], | ||
| data['chat_id'], | ||
| data['chatId'], | ||
| data['channel_id'], | ||
| data['channelId'], | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select source-id fallbacks based on the parsed notification kind.
Line 209 uses one shared fallback list, so a channel_message payload that also includes chat_id will route to chat_id before channel_id. Keep generic conduit_source_id/source_id fallbacks, then prefer kind-specific IDs.
Suggested fix and regression coverage
- final sourceId = _firstNonEmptyString([
- data['conduit_source_id'],
- data['source_id'],
- data['sourceId'],
- data['chat_id'],
- data['chatId'],
- data['channel_id'],
- data['channelId'],
- ]);
+ final sourceId = switch (kind) {
+ NotificationKind.chatCompletion => _firstNonEmptyString([
+ data['conduit_source_id'],
+ data['source_id'],
+ data['sourceId'],
+ data['chat_id'],
+ data['chatId'],
+ ]),
+ NotificationKind.channelMessage => _firstNonEmptyString([
+ data['conduit_source_id'],
+ data['source_id'],
+ data['sourceId'],
+ data['channel_id'],
+ data['channelId'],
+ ]),
+ _ => null,
+ };+ test('prefers channel source fields for channel payloads', () {
+ final tap = notificationTapFromRemoteMessageDataForTest({
+ 'kind': 'channel_message',
+ 'chat_id': 'chat-1',
+ 'channel_id': 'channel-1',
+ });
+
+ check(tap).isNotNull();
+ check(tap!.kind).equals(NotificationKind.channelMessage);
+ check(tap.sourceId).equals('channel-1');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final sourceId = _firstNonEmptyString([ | |
| data['conduit_source_id'], | |
| data['source_id'], | |
| data['sourceId'], | |
| data['chat_id'], | |
| data['chatId'], | |
| data['channel_id'], | |
| data['channelId'], | |
| ]); | |
| final sourceId = switch (kind) { | |
| NotificationKind.chatCompletion => _firstNonEmptyString([ | |
| data['conduit_source_id'], | |
| data['source_id'], | |
| data['sourceId'], | |
| data['chat_id'], | |
| data['chatId'], | |
| ]), | |
| NotificationKind.channelMessage => _firstNonEmptyString([ | |
| data['conduit_source_id'], | |
| data['source_id'], | |
| data['sourceId'], | |
| data['channel_id'], | |
| data['channelId'], | |
| ]), | |
| _ => null, | |
| }; |
🤖 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/features/notifications/services/remote_push_token_provider.dart` around
lines 209 - 217, The source ID selection in remote_push_token_provider’s token
parsing currently uses one shared fallback list, which can pick the wrong
identifier for a notification kind like channel_message when both chat_id and
channel_id are present. Update the source lookup logic around the sourceId
resolution to keep the generic conduit_source_id and source_id fallbacks first,
then choose kind-specific fields based on the parsed notification kind (for
example, prefer channel_id for channel messages and chat_id for chat messages).
Use the existing parsing path in remote_push_token_provider to branch by
notification kind before calling _firstNonEmptyString.
1 participant
Summary
Verification
Notes
Note
Add remote push notification registration via a push proxy and Firebase
RemotePushServicethat conditionally registers/unregisters device tokens with a Conduit-owned push proxy based on build-time Dart defines, server feature flags, OS permissions, and user preferences.FirebaseRemotePushTokenProviderbacked by FCM/APNs to obtain device tokens, surface tap events (including cold-start), and handle token refreshes.RemotePushProxyClientto register/unregister installations with the proxy, andRemotePushSubscriptionStoreto persist subscription state in secure storage across restarts.ApiServicewith methods to read/write the user notification webhook URL (with conflict protection viaUserNotificationWebhookConflictException) and to toggleENABLE_USER_WEBHOOKSvia the admin API.NotificationSettingsPageso enabling notifications triggers push registration with result feedback, and adds an admin-only toggle to configure per-user webhooks on the server._cleanupUserScopedProvidersAfterSignOutnow unregisters all locally tracked push subscriptions.firebase_core,firebase_messaging) are now required at runtime; builds without the expected Dart defines will skip remote push silently.📊 Macroscope summarized 0ac45b8. 16 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR adds remote push notification support through a Conduit push proxy. The main changes are:
Confidence Score: 4/5
The remote push feature needs fixes before merge because configuration and lifecycle paths can expose registration data or leave notifications registered incorrectly.
The review focused on the changed notification registration, startup, and socket-listener paths, with findings tied to concrete control-flow and configuration behavior. Local runtime reproduction was limited by the absence of Flutter tooling in the environment.
lib/features/notifications/services/remote_push_build_config.dart; lib/core/providers/app_startup_providers.dart; lib/features/notifications/providers/notification_socket_listener.dart
Security Review
The push proxy configuration accepts plaintext
httpendpoints for registration, which can expose push tokens and related identifiers in transit.What T-Rex did
Reviews (1): Last reviewed commit: "feat(notifications): register push proxy..." | Re-trigger Greptile
Context used: