-
Notifications
You must be signed in to change notification settings - Fork 912
SaaS: Native Push Notifications #2397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
olivaresf
wants to merge
80
commits into
basecamp:main
Choose a base branch
from
olivaresf:saas-push-notifications
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+1,323
−207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Phase 1 of native push notifications implementation: - Add action_push_native gem to Gemfile.saas for SaaS-only native push - Add migration for action_push_native_devices table - Create ApplicationPushNotification model in saas/app/models/ - Create ApplicationPushNotificationJob in saas/app/jobs/ - Create push.yml config in saas/config/ with APNs/FCM settings The migration needs MySQL running to execute (SaaS mode uses MySQL). Config placeholders (team_id, topic, project_id) need to be filled in. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Include the devices association in User model via the SaaS engine to prepare for device registration functionality. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add /users/devices routes for device registration - Create DevicesController with index, create, destroy actions - Add devices index view for managing registered devices - Add native_devices partial to notification settings (SaaS only) - Add skeleton controller tests for Phase 4 implementation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create NotificationPusher::Native concern for sending to native devices - Prepend native concern via SaaS engine - Add device fixtures for testing - Add PushNotificationTestHelper for test assertions - Implement full controller tests for device registration - Add NotificationPusher model tests for native push logic Native push notifications now send alongside web push when users have registered mobile devices. Supports iOS (APNs) and Android (FCM) with platform-specific features like time-sensitive delivery and data-only messages. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The schema version was set to the first migration's timestamp (2026_01_14_203313) but already included the unique index from the second migration (2026_01_15_000000). This updates the version to correctly reflect all applied migrations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the push_to_web method which duplicated the base class's push_to_user logic. Now directly calls push_to_user with a guard for empty subscriptions, keeping the optimization while reducing code duplication. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Devices are now identified by (owner, uuid) instead of token alone. This allows multiple users to have device records with the same push token, with notifications correctly routed to the authenticated user. Key changes: - Add uuid column to action_push_native_devices - Replace unique index on token with composite (owner_type, owner_id, uuid) - Controller now looks up by user + uuid, updates token on registration - Require uuid parameter in device registration API This prevents potential token hijacking where one user could take over another user's push notifications by registering their token. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verifies that when a user has both web push subscriptions and native devices registered, notifications are delivered to both channels. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace three separate migrations with one that creates the action_push_native_devices table with the final schema: - uuid column (not null) - Composite unique index on (owner_type, owner_id, uuid) This avoids unnecessary intermediate schema changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The _native_devices partial now lives in the SaaS engine and is only rendered when Fizzy.saas? is true. This keeps SaaS-specific views out of the self-hosted codebase entirely. In self-hosted mode, the partial doesn't exist and the render call is skipped via the conditional. If someone tried to render it directly, Rails would raise ActionView::MissingTemplate. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clearer naming now that we have both web and native push delivery. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use find_or_initialize_by instead of find_or_create_by to avoid inserting a record with only uuid before all required fields are set. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fizzy uses UUID primary keys, so the polymorphic owner reference needs to specify type: :uuid to match. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Native push is disabled in local environments by default, but can now be enabled by setting ENABLE_NATIVE_PUSH=true for testing the full push notification flow. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Usage: bin/dev --apns This loads APNs credentials from 1Password and enables native push delivery in development. Requires SaaS mode to be enabled. Also updates apns-dev to export ENABLE_NATIVE_PUSH=true so that loading credentials automatically enables push delivery. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Native push notifications require SaaS mode, so automatically enable it when the --apns flag is used instead of showing an error. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Consolidates the session reference and index cleanup into the original CreateActionPushNativeDevices migration for a cleaner migration history. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…sh-notifications * 'main' of https://github.com/basecamp/fizzy: Make the CLAUDE.md stub less obtrusive Only index up to 32KB of search content Use inline spacing variable Actually make it affect larger screens too Use CSS variable for panel size in delete dialogs Add viewport padding to dialogs on mobile Prevent page scrolling when modal dialog is open
…into saas-push-notifications * 'saas-push-notifications' of github.com:olivaresf/fizzy: Squash device migrations into single table creation Remove redundant owner index from devices table Fix reference to `user.devices`, left-over from the identity switch Tidy up saas engine a bit more Link devices to sessions for automatic cleanup on logout Rename Push to PushTarget for better readability Move payload method to Notification and make accessors public Extract payload building into dedicated classes Refactor notification push system with registry pattern Change device ownership from User to Identity Simplify device routes and use ActiveRecord validations Refactor devices controller and extract registration to model
Devices should persist independently of sessions - when a session is deleted, the device registration should remain valid. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…into saas-push-notifications * 'saas-push-notifications' of github.com:olivaresf/fizzy: Remove foreign key constraint from devices to sessions
- Add disallow_account_scope to skip tenant requirement - Move routes to saas/config/routes.rb (engine routes) - Use saas.devices_path/saas.device_path for engine route helpers - Update tests to work without tenant context Devices belong to Identity (global), not Account, so they don't need tenant context in the URL. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use ActionPushNative's new on_load hook to configure the database connection,
following the same pattern as Active Storage and Action Text:
ActiveSupport.on_load(:action_push_native_record) do
connects_to database: { writing: :saas, reading: :saas }
end
This allows ApplicationPushDevice to inherit directly from ActionPushNative::Device
without needing an intermediate abstract class.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4079b89 to
0eb1b1e
Compare
Use polymorphism instead of case statements in Native push target: - DefaultPayload#category returns "default", #high_priority? returns false - EventPayload#category returns "assignment"/"comment"/"card" based on action - MentionPayload#category returns "mention", #high_priority? returns true This simplifies the Native push target by delegating source-specific logic to the appropriate payload classes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace separate WebPushJob and NativePushJob with a single PushJob that calls notification.push, which iterates over registered targets. Each target handles its own delivery - Web pushes synchronously via the pool, Native enqueues device-level jobs via deliver_later_to. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
"Push to target" reads naturally - we push the notification to the target. "Target processes" also makes sense - the target receives and handles the notification in its own way. - Add class method PushTarget.process(notification) that instantiates and calls the instance method - Rename instance method from push to process - Add private push_to helper in Pushable for readable iteration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Each target now implements process directly with its own logic, rather than using processable?/perform_push hooks. The pushable? check is done once in Notification#push before iterating targets. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
No need for push notification secrets in staging, as we won't allow push notifications there. Also, no need to store APNs topic in 1P, as it's not a secret.
Turns out, files can't be referenced within a field group in 1Password, they need to live at the root of the item.
…into saas-push-notifications * 'saas-push-notifications' of github.com:olivaresf/fizzy: Store `APNS_ENCRYPTION_KEY` and `FCM_ENCRYPTION_KEY` at the root Add `FCM_ENCRYPTION_KEY` to Kamal secrets and organize them Simplify PushTarget by removing template method pattern Rename push to process on PushTarget for clearer semantics Consolidate push jobs into single Notification::PushJob Move category and high_priority to payload classes Move push priority concerns from Event and Mention into Native push target Move devices table to saas database Make devices controller untenanted with engine routes Change priority notification level for mentions and assignments # Conflicts: # saas/app/models/notification/push_target/native.rb
Easier than having Kamal support attached files as secrets. Also: remove Rails credentials fallback from config file, as we don't use that in the app, and don't have the fallbacks anywhere.
* main: Correctly initialise WebPush connection (basecamp#2417) Update models, views, and fixtures for polymorphic reactions Make Reaction polymorphic Return to Board page when clearing filters Bump card perma z-index when dialog is open Only submit on blur if the input has a value
When you had already granted notification permission but hadn't completed the subscription flow (no service worker or no push subscription), the UI showed neither the subscribe button nor the enabled state, leaving you stuck with no way to subscribe, and wrong instructions to fix it. Instead, let's just show the button to allow you to subscribe.
The push subscription requires an active service worker. When registering a new service worker, we now wait for it to become active using navigator.serviceWorker.ready before attempting to subscribe to push notifications. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The web push payload sends the URL in data.url but the service worker was looking for data.path, resulting in undefined URLs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
After releasing the new version: https://github.com/rails/action_push_native/releases/tag/v0.3.1
Move avatar_background_color logic from helper to User::Avatar concern so it can be accessed from models. Include creator_id, creator_initials, and creator_avatar_color in native push notifications for local avatar rendering on iOS. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Include the shortened familiar name format (e.g., "Salvador D.") for display in iOS notification titles. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables native push notifications for iOS and Android on Fizzy-SaaS.