-
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 all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
ad10dfa
chore: create a plan
ccssmnn 4bee727
chore: change push notification logic to not rely on clerk
ccssmnn 83c9e81
fix: address review issues in push notification refactor
ccssmnn ab68fda
feat: dynamic stale threshold based on future reminders
ccssmnn ed0c0f9
chore: add tests
ccssmnn 4c472bb
chore: improve testability
ccssmnn e999d97
chore: address minor issues
ccssmnn 1dec5eb
chore: write ATPs
ccssmnn a257c34
Merge branch 'main' into ccssmnn/push-notifications-without-clerk
ccssmnn 5c03912
fix: address coderabbit findings
ccssmnn ba0a413
fix: address nitpick changes
ccssmnn e052ada
fix: address minor issues
ccssmnn df27bad
feat: add CI and don't run checks on vercel builds
ccssmnn 779f9af
fix: address findings
ccssmnn 655e87e
fix: tests
ccssmnn f70ed20
fix: tests
ccssmnn 7a6256b
chore: test push notification logic, harmonize logs
ccssmnn 359ef78
chore: final review and fixes
ccssmnn 70aa8eb
chore: no build in CI
ccssmnn 4f107e8
chore: improve implementation
ccssmnn 2c9d2eb
fix: delete old settings only after new ones are persisted
ccssmnn 27573b6
fix: clear cached worker on rejection to allow retries
ccssmnn aaa0018
fix: Add device only after successful server registration
ccssmnn 69fa7af
fix: more improvements
ccssmnn 8d60d95
fix: type error in notification-registration
ccssmnn 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,27 @@ | ||
| 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 | ||
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() | ||
| }) | ||
| }) |
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,161 @@ | ||
| import { useEffect, useRef } from "react" | ||
| import { useAccount } from "jazz-tools/react" | ||
| import { | ||
| Group, | ||
| generateAuthToken, | ||
| type co, | ||
| type ResolveQuery, | ||
| } from "jazz-tools" | ||
| import { PUBLIC_JAZZ_WORKER_ACCOUNT } from "astro:env/client" | ||
| import { UserAccount } from "#shared/schema/user" | ||
| import { tryCatch } from "#shared/lib/trycatch" | ||
| import { | ||
| migrateNotificationSettings, | ||
| addServerToGroup, | ||
| } from "#app/lib/notification-settings-migration" | ||
| import { triggerNotificationRegistration } from "#app/lib/notification-registration" | ||
| import { findLatestFutureDate } from "#app/lib/reminder-utils" | ||
|
|
||
| export { useRegisterNotifications } | ||
|
|
||
| let notificationSettingsQuery = { | ||
| root: { | ||
| notificationSettings: true, | ||
| people: { $each: { reminders: { $each: true } } }, | ||
| }, | ||
| } as const satisfies ResolveQuery<typeof UserAccount> | ||
|
|
||
| type LoadedAccount = co.loaded< | ||
| typeof UserAccount, | ||
| typeof notificationSettingsQuery | ||
| > | ||
|
|
||
| /** | ||
| * Hook that registers notification settings with the server. | ||
| * Handles migration from account-owned to group-owned settings. | ||
| * Runs once on app start. | ||
| */ | ||
| function useRegisterNotifications(): void { | ||
| let registrationRan = useRef(false) | ||
| let me = useAccount(UserAccount, { resolve: notificationSettingsQuery }) | ||
|
|
||
| useEffect(() => { | ||
| if (registrationRan.current || !me.$isLoaded) return | ||
| if (!me.root.notificationSettings) return | ||
|
|
||
| registrationRan.current = true | ||
| registerNotificationSettings(me).catch(error => { | ||
| console.error("[Notifications] Registration error:", error) | ||
| registrationRan.current = false | ||
| }) | ||
| }, [me.$isLoaded, me]) | ||
| } | ||
|
|
||
| async function registerNotificationSettings(me: LoadedAccount): Promise<void> { | ||
| let notificationSettings = me.root.notificationSettings | ||
| if (!notificationSettings) return | ||
|
|
||
| let serverAccountId = PUBLIC_JAZZ_WORKER_ACCOUNT | ||
| if (!serverAccountId) { | ||
| console.error("[Notifications] PUBLIC_JAZZ_WORKER_ACCOUNT not configured") | ||
| return | ||
| } | ||
|
|
||
| // Sync language from root to notification settings | ||
| let rootLanguage = me.root.language | ||
| if (rootLanguage && notificationSettings.language !== rootLanguage) { | ||
| notificationSettings.$jazz.set("language", rootLanguage) | ||
| } | ||
|
|
||
| // Compute and sync latestReminderDueDate | ||
| let latestDueDate = computeLatestReminderDueDate(me) | ||
| if (latestDueDate !== notificationSettings.latestReminderDueDate) { | ||
| notificationSettings.$jazz.set("latestReminderDueDate", latestDueDate) | ||
| } | ||
|
|
||
| // Check if settings are owned by a shareable group | ||
| // The key difference: if owner is an Account vs a Group | ||
| let owner = notificationSettings.$jazz.owner | ||
| let isShareableGroup = owner instanceof Group | ||
|
|
||
| if (!isShareableGroup) { | ||
| console.log("[Notifications] Migrating to shareable group") | ||
| let migrationResult = await tryCatch( | ||
| migrateNotificationSettings(notificationSettings, serverAccountId, { | ||
| loadAs: me, | ||
| rootLanguage, | ||
| }), | ||
| ) | ||
| if (!migrationResult.ok) { | ||
| console.error("[Notifications] Migration failed:", migrationResult.error) | ||
| return | ||
| } | ||
| let { newSettings, cleanup } = migrationResult.data | ||
| // Update root to point to new settings before cleanup | ||
| me.root.$jazz.set("notificationSettings", newSettings) | ||
| // Defer cleanup to next tick so new settings are persisted first | ||
| setTimeout(cleanup, 0) | ||
| notificationSettings = newSettings | ||
| console.log("[Notifications] Migration complete") | ||
| } else { | ||
| // Ensure server worker is a member | ||
| let group = owner as Group | ||
| let serverIsMember = group.members.some( | ||
| m => m.account?.$jazz.id === serverAccountId, | ||
| ) | ||
| if (!serverIsMember) { | ||
| let addResult = await tryCatch( | ||
| addServerToGroup(group, serverAccountId, { loadAs: me }), | ||
| ) | ||
| if (!addResult.ok) { | ||
| console.error( | ||
| "[Notifications] Failed to add server to group:", | ||
| addResult.error, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Register with server using Jazz auth | ||
| let authToken = generateAuthToken(me) | ||
| let registerResult = await triggerNotificationRegistration( | ||
| notificationSettings.$jazz.id, | ||
| authToken, | ||
| ) | ||
|
|
||
| if (!registerResult.ok) { | ||
| console.error("[Notifications] Registration failed:", registerResult.error) | ||
| return | ||
| } | ||
|
|
||
| console.log("[Notifications] Registration successful") | ||
| } | ||
|
|
||
| function computeLatestReminderDueDate(me: LoadedAccount): string | undefined { | ||
| let reminders = extractReminders(me) | ||
| let timezone = | ||
| me.root.notificationSettings?.timezone || | ||
| Intl.DateTimeFormat().resolvedOptions().timeZone | ||
| let today = new Date() | ||
| .toLocaleDateString("sv-SE", { timeZone: timezone }) | ||
| .slice(0, 10) | ||
| return findLatestFutureDate(reminders, today) | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function extractReminders( | ||
| me: LoadedAccount, | ||
| ): { dueAtDate: string; deleted: boolean; done: boolean }[] { | ||
| let reminders: { dueAtDate: string; deleted: boolean; done: boolean }[] = [] | ||
| for (let person of me.root.people.values()) { | ||
| if (!person || person.deletedAt) continue | ||
| for (let reminder of person.reminders.values()) { | ||
| if (!reminder) continue | ||
| reminders.push({ | ||
| dueAtDate: reminder.dueAtDate, | ||
| deleted: !!reminder.deletedAt, | ||
| done: !!reminder.done, | ||
| }) | ||
| } | ||
| } | ||
| return reminders | ||
| } | ||
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,43 @@ | ||
| import { apiClient } from "#app/lib/api-client" | ||
| import { tryCatch } from "#shared/lib/trycatch" | ||
|
|
||
| export { triggerNotificationRegistration } | ||
|
|
||
| type RegistrationResult = { ok: true } | { ok: false; error: string } | ||
|
|
||
| async function triggerNotificationRegistration( | ||
| notificationSettingsId: string, | ||
| authToken: string, | ||
| ): Promise<RegistrationResult> { | ||
| let result = await tryCatch( | ||
| apiClient.push.register.$post( | ||
| { | ||
| json: { notificationSettingsId }, | ||
| }, | ||
| { | ||
| headers: { | ||
| Authorization: `Jazz ${authToken}`, | ||
| }, | ||
| }, | ||
| ), | ||
| ) | ||
|
|
||
| if (!result.ok) { | ||
| console.error("[Notifications] Registration failed:", result.error) | ||
| return { ok: false, error: "Network error" } | ||
| } | ||
|
|
||
| if (!result.data.ok) { | ||
| let errorData = await tryCatch( | ||
| result.data.json() as Promise<{ message?: string }>, | ||
| ) | ||
| let errorMessage = errorData.ok | ||
| ? errorData.data.message || "Unknown error" | ||
| : "Unknown error" | ||
| console.error("[Notifications] Registration error:", errorMessage) | ||
| return { ok: false, error: errorMessage } | ||
| } | ||
|
|
||
| console.log("[Notifications] Registration triggered successfully") | ||
| return { ok: true } | ||
| } |
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