-
Notifications
You must be signed in to change notification settings - Fork 8
Push Notifications without Clerk #76
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
Merged
Merged
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
ad10dfa
chore: create a plan
carlassmann 4bee727
chore: change push notification logic to not rely on clerk
carlassmann 83c9e81
fix: address review issues in push notification refactor
carlassmann ab68fda
feat: dynamic stale threshold based on future reminders
carlassmann ed0c0f9
chore: add tests
carlassmann 4c472bb
chore: improve testability
carlassmann e999d97
chore: address minor issues
carlassmann 1dec5eb
chore: write ATPs
carlassmann a257c34
Merge branch 'main' into ccssmnn/push-notifications-without-clerk
carlassmann 5c03912
fix: address coderabbit findings
carlassmann ba0a413
fix: address nitpick changes
carlassmann e052ada
fix: address minor issues
carlassmann df27bad
feat: add CI and don't run checks on vercel builds
carlassmann 779f9af
fix: address findings
carlassmann 655e87e
fix: tests
carlassmann f70ed20
fix: tests
carlassmann 7a6256b
chore: test push notification logic, harmonize logs
carlassmann 359ef78
chore: final review and fixes
carlassmann 70aa8eb
chore: no build in CI
carlassmann 4f107e8
chore: improve implementation
carlassmann 2c9d2eb
fix: delete old settings only after new ones are persisted
carlassmann 27573b6
fix: clear cached worker on rejection to allow retries
carlassmann aaa0018
fix: Add device only after successful server registration
carlassmann 69fa7af
fix: more improvements
carlassmann 8d60d95
fix: type error in notification-registration
carlassmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| name: CI | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
|
|
||
| jobs: | ||
| check: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Run typecheck, lint, and format checks | ||
| run: bun run check | ||
|
|
||
| - name: Run tests | ||
| run: bun run test:run | ||
|
|
||
| build: | ||
| runs-on: ubuntu-latest | ||
| needs: check | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Build | ||
| run: bun run build:prod | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| # ATP: Push Notification Migration | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - Device with existing push notifications enabled | ||
| - Device/browser for new user signup | ||
| - Server deployed with new code | ||
| - Ability to trigger cron job (`/push/deliver-notifications`) | ||
| - Jazz dev tools access | ||
| - `CRON_SECRET` from `.env` | ||
|
|
||
| ## Trigger Cron Job | ||
|
|
||
| ```bash | ||
| curl -X GET http://localhost:4321/api/push/deliver-notifications \ | ||
| -H "Authorization: Bearer $CRON_SECRET" | ||
| ``` | ||
|
|
||
| For local testing: | ||
|
|
||
| ```bash | ||
| curl -X GET http://localhost:4321/api/push/deliver-notifications \ | ||
| -H "Authorization: Bearer $CRON_SECRET" | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## ATP 1: Existing User Migration | ||
|
|
||
| **Goal:** Verify existing users migrate to group-owned settings and continue receiving notifications | ||
|
|
||
| ### Pre-flight | ||
|
|
||
| 1. Open Jazz dev tools as existing user with push enabled | ||
| 2. Note `notificationSettings` owner (should be Account, not Group) | ||
|
|
||
| ### Steps | ||
|
|
||
| | Step | Action | Expected | | ||
| | ---- | ------------------------------------------------------- | ------------------------------------------------------------------------------------------------- | | ||
| | 1 | Open app | App loads | | ||
| | 2 | Check browser console | `[Notifications] Migrating to shareable group` → `Migration complete` → `Registration successful` | | ||
| | 3 | Check Jazz dev tools | `notificationSettings` owner is now Group with server as member | | ||
| | 4 | Check `root.notificationSettingsRefs` on server account | Entry exists with your `userId`, recent `lastSyncedAt`, link to your settings | | ||
| | 5 | Verify notification settings UI | All previously registered devices still visible | | ||
| | 6 | Trigger cron job | User appears in results, `success: true` | | ||
| | 7 | Receive push notification | Notification arrives on device | | ||
|
|
||
| ### Failure indicators | ||
|
|
||
| - Console shows `Migration failed` or `Registration failed` | ||
| - Devices disappeared from settings | ||
| - Server refs list empty or missing user | ||
| - Cron returns no results or `success: false` | ||
|
|
||
| --- | ||
|
|
||
| ## ATP 2: New User Push Setup | ||
|
|
||
| **Goal:** Verify new users can enable push and receive notifications | ||
|
|
||
| | Step | Action | Expected | | ||
| | ---- | ----------------------------------------- | ------------------------------------------------ | | ||
| | 1 | Sign up as new user | Account created | | ||
| | 2 | Go to Settings → Notifications | Page loads | | ||
| | 3 | Enable push notifications, add device | Device added, success toast | | ||
| | 4 | Check console | `Registration successful` (no migration message) | | ||
| | 5 | Check server's `notificationSettingsRefs` | New user entry exists | | ||
| | 6 | Trigger cron job | New user in results | | ||
| | 7 | Receive push notification | Notification arrives | | ||
|
|
||
| --- | ||
|
|
||
| ## ATP 3: Re-registration on App Open | ||
|
|
||
| **Goal:** Verify `latestReminderDueDate` updates on each app open | ||
|
|
||
| | Step | Action | Expected | | ||
| | ---- | --------------------------------------------------------------------- | -------------------------------------------------------------- | | ||
| | 1 | As existing user, create reminder for future date (e.g., 2 weeks out) | Reminder created | | ||
| | 2 | Close and reopen app | App loads | | ||
| | 3 | Check console | `Registration successful` | | ||
| | 4 | Check `notificationSettings` in Jazz dev tools | `latestReminderDueDate` matches furthest reminder (YYYY-MM-DD) | | ||
| | 5 | Check server ref | `lastSyncedAt` updated to recent time | | ||
|
|
||
| --- | ||
|
|
||
| ## ATP 4: Stale Ref Cleanup | ||
|
|
||
| **Goal:** Verify inactive users are cleaned up from server refs | ||
|
|
||
| | Step | Action | Expected | | ||
| | ---- | --------------------------------------------------------------------------------- | --------------------------------------------------------------------- | | ||
| | 1 | Register a test user | User in server refs | | ||
| | 2 | In Jazz dev tools, manually set `lastSyncedAt` to 31+ days ago | - | | ||
| | 3 | Ensure user has no future reminders (or set `latestReminderDueDate` to past date) | - | | ||
| | 4 | Trigger cron | Console shows `Marking stale ref for removal`, user removed from refs | | ||
| | 5 | Open app as that user | Re-registers, back in refs | | ||
|
|
||
| --- | ||
|
|
||
| ## Verification Checklist | ||
|
|
||
| - [ ] ATP 1: Existing user migration | ||
| - [ ] ATP 2: New user setup | ||
| - [ ] ATP 3: Re-registration updates | ||
| - [ ] ATP 4: Stale cleanup |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import { describe, test, expect } from "vitest" | ||
| import { findLatestFutureDate } from "#app/lib/reminder-utils" | ||
|
|
||
| describe("findLatestFutureDate", () => { | ||
| let today = "2025-01-15" | ||
|
|
||
| test("returns undefined for empty list", () => { | ||
| expect(findLatestFutureDate([], today)).toBeUndefined() | ||
| }) | ||
|
|
||
| test("returns undefined when all reminders are in the past", () => { | ||
| let reminders = [ | ||
| { dueAtDate: "2025-01-10", deleted: false, done: false }, | ||
| { dueAtDate: "2025-01-14", deleted: false, done: false }, | ||
| ] | ||
| expect(findLatestFutureDate(reminders, today)).toBeUndefined() | ||
| }) | ||
|
|
||
| test("returns the only future reminder", () => { | ||
| let reminders = [{ dueAtDate: "2025-01-20", deleted: false, done: false }] | ||
| expect(findLatestFutureDate(reminders, today)).toBe("2025-01-20") | ||
| }) | ||
|
|
||
| test("returns today's date as valid future", () => { | ||
| let reminders = [{ dueAtDate: "2025-01-15", deleted: false, done: false }] | ||
| expect(findLatestFutureDate(reminders, today)).toBe("2025-01-15") | ||
| }) | ||
|
|
||
| test("returns the latest of multiple future reminders", () => { | ||
| let reminders = [ | ||
| { dueAtDate: "2025-01-20", deleted: false, done: false }, | ||
| { dueAtDate: "2025-02-15", deleted: false, done: false }, | ||
| { dueAtDate: "2025-01-25", deleted: false, done: false }, | ||
| ] | ||
| expect(findLatestFutureDate(reminders, today)).toBe("2025-02-15") | ||
| }) | ||
|
|
||
| test("ignores deleted reminders", () => { | ||
| let reminders = [ | ||
| { dueAtDate: "2025-02-15", deleted: true, done: false }, | ||
| { dueAtDate: "2025-01-20", deleted: false, done: false }, | ||
| ] | ||
| expect(findLatestFutureDate(reminders, today)).toBe("2025-01-20") | ||
| }) | ||
|
|
||
| test("ignores done reminders", () => { | ||
| let reminders = [ | ||
| { dueAtDate: "2025-02-15", deleted: false, done: true }, | ||
| { dueAtDate: "2025-01-20", deleted: false, done: false }, | ||
| ] | ||
| expect(findLatestFutureDate(reminders, today)).toBe("2025-01-20") | ||
| }) | ||
|
|
||
| test("returns undefined when all future reminders are deleted or done", () => { | ||
| let reminders = [ | ||
| { dueAtDate: "2025-02-15", deleted: true, done: false }, | ||
| { dueAtDate: "2025-01-20", deleted: false, done: true }, | ||
| { dueAtDate: "2025-01-10", deleted: false, done: false }, | ||
| ] | ||
| expect(findLatestFutureDate(reminders, today)).toBeUndefined() | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: ccssmnn/tilly
Length of output: 234
🏁 Script executed:
Repository: ccssmnn/tilly
Length of output: 1186
🏁 Script executed:
Repository: ccssmnn/tilly
Length of output: 646
Pin Bun to a specific version instead of using
latestto keep CI reproducible.Using
latestcan break builds when Bun releases new versions. Create a.bun-versionfile with your target version and update the workflow to use it (or pin directly), then update both occurrences at lines 16-18 and 36-38.🤖 Prompt for AI Agents