Repository navigation
feat(kix): install kix.quest as a phone app with push notifications - #165
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kix.quest installs as a PWA with a web app manifest, app icons, and a service worker. Maintainers turn on Web Push per device from /inbox or /account; only decisions push (agent questions, publication decisions, account limits, failed jobs, new issues, PRs ready for review), once each, coalesced per item by notification tag and push Topic, and a tap opens the item in the inbox. Subscriptions live in D1 per user and session, are deleted on sign-out and when the push service reports them gone. RFC 8291 encryption and VAPID signing use WebCrypto directly. The nav and the installed app's icon show the unread decision count, and the phone nav is tighter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the quest is implemented and its file deleted in this PR. (Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1a9907c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const subscription = supported && (await (await registration).pushManager.getSubscription()); | ||
| const on = !!subscription && Notification.permission === "granted"; | ||
| const blocked = supported && Notification.permission === "denied"; | ||
| // The inbox's compact control steps aside once there is nothing to switch on. | ||
| control.hidden = (on || blocked) && "compact" in control.dataset && !message; |
There was a problem hiding this comment.
When a user signs out, /sign-out removes the D1 subscription but does not unsubscribe the browser's PushSubscription. After the user signs back in, this check treats that stale browser subscription as enabled, hides the compact control, and never posts it back to /push/subscribe; the UI therefore claims the device is notified while the server has no destination and sends it nothing. Reconcile the existing subscription with the server before reporting notifications as on.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f17641a. Signing out now also unsubscribes the browser's own subscription (the sign-out form waits for it), so the next person to sign in on that browser opts in for themselves. When the switch loads with notification permission granted, it stores any existing browser subscription again for the current session, so it never shows a device as notified that the server forgot (sign-out without JS, or a session that expired and signed back in).
(Written by Claude Opus 5.5)
| const project = await env.DB.prepare("SELECT id,owner,name,state FROM github_projects WHERE id=?") | ||
| .bind(decision.project) | ||
| .first<Enrollment>(); | ||
| if (project) await deliver(env, { ...decision, project }, targets); |
There was a problem hiding this comment.
When a project is paused while an attempt is still running, that attempt may subsequently ask a question, hit an account limit, or fail. Those paths call push.job, and this lookup accepts the paused project and delivers the notification anyway, unlike webhook notifications and the documented rule that paused projects do not push. Restrict this lookup to enabled projects before delivery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f17641a. notify now looks up only enabled projects, so a job that asks, hits a limit, or fails after its project pauses doesn't push. The question test covers it.
(Written by Claude Opus 5.5)
Rename the push migration to 0012_push.sql, after main's 0011_project_scan.sql. Signing out now drops the browser's subscription too, and the switch stores an existing subscription again when it loads, so it never shows a device as notified that the server forgot. Jobs on a paused project no longer push. Main keeps quests available across webhooks that cannot change them, so the badge in the new-issue test now counts the ready quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f176415dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const synced = (async () => { | ||
| if (!supported || !controls.length || Notification.permission !== "granted") return; | ||
| const subscription = await (await registration).pushManager.getSubscription(); | ||
| if (subscription) await post("/push/subscribe", subscription.toJSON()); |
There was a problem hiding this comment.
When browser-side unsubscribe does not happen—for example, JavaScript was disabled during sign-out or unsubscribe() rejects—the server deletes the old session's row but the browser retains its PushSubscription. On the next account's inbox or account page, this unconditional POST transfers the endpoint to that user through the server's ON CONFLICT update even though they never clicked the enable button. The newly added unconditional resync is fresh evidence distinct from the prior stale-state report; reclaim an existing subscription only after verifying that it belonged to the same user, or require a new click.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in dc9e51c. The switch no longer stores the browser's subscription on load. It asks a new /push/check whether the server notifies that endpoint for the signed-in user. A match moves to the current session, so an expired session's device stays on and signing out here forgets it. Otherwise the switch shows notifications off, and turning them on again takes a click. That also made the sign-out handler from 6f17641 unnecessary, so I removed it.
(Written by Claude Opus 5.5)
| export async function devices(env: Env, project: number): Promise<Device[]> { | ||
| if (!keys(env)) return []; | ||
| const { results } = await env.DB.prepare( | ||
| "SELECT s.endpoint,s.p256dh,s.auth,s.user FROM push_subscriptions s JOIN github_projects p ON p.enrolledBy=s.user WHERE p.id=?", |
There was a problem hiding this comment.
When the enrolled user loses their maintain/admin role without the project being disconnected, this join still selects all of their devices for subsequent webhook and job notifications. inbox.load excludes that project's items when its live authority check fails, while disconnecting also requires current authority, so the former maintainer can keep receiving issue, PR, and job-title notifications for a project no longer present in their decision inbox and cannot disable only that project. Revalidate or prune the recipient's authority before delivery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in dc9e51c. deliver now rechecks each recipient's current maintainer authority (GitHubService.maintains, the same check the inbox makes before listing a project's items) and skips anyone who fails it. A test covers a maintainer demoted to write.
(Written by Claude Opus 5.5)
…ent maintainers The switch no longer stores a browser's leftover subscription for whoever signs in. It asks /push/check whether the server notifies that endpoint for the signed-in user, which moves a match to the current session, and shows notifications off otherwise, so turning them on again takes a click. The sign-out handler that dropped the browser's subscription is no longer needed. Each push rechecks that its recipient still maintains the project, as the inbox does before listing its items. Tests sign with the test App key by default so background work in the runner hub can make that check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # cloudflare/src/inbox/page.tsx # cloudflare/test/publish.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c64c41f65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await fetch("/push/subscribe", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ ...subscription.toJSON(), replaces: old.endpoint }), | ||
| }); |
There was a problem hiding this comment.
When a retained browser subscription rotates while a different user is signed in, this unconditional POST stores the replacement for the current user through /push/subscribe, even if that user never enabled notifications. This can happen after the prior user signs out while the browser retains its subscription, so the next account is silently opted in. The remaining pushsubscriptionchange POST is fresh evidence distinct from the fixed page-load resync; check that the old endpoint belongs to the signed-in user before replacing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 8132d53. A subscribe that names replaces now stores the new endpoint only when the replaced one belongs to the signed-in user, and otherwise returns 403 and stores nothing. A test covers a rotation while another user is signed in.
(Written by Claude Opus 5.5)
| const result = await c.env.DB.prepare("UPDATE push_subscriptions SET session=? WHERE endpoint=? AND user=?") | ||
| .bind(session.session.id, body.endpoint, session.user.id) | ||
| .run(); | ||
| return c.json({ stored: result.meta.changes > 0 }); |
There was a problem hiding this comment.
When an operator replaces the VAPID key pair, existing browser subscriptions remain bound to the old public key, but this endpoint-only check still reports them as stored. The UI consequently shows notifications as enabled and hides the enable action, while sends signed with the new key are rejected and retained as transient failures rather than removed. Store or compare the application-server key used by each subscription, or clear mismatched subscriptions so the device can enroll again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 8132d53 on the client. Before checking or enabling, the switch compares the browser subscription's applicationServerKey with the page's current VAPID key. On a mismatch it removes the subscription from the server and the browser, so the device shows off and can enroll again with the new key. Devices that never reopen the page keep their stale rows, which only log failed sends. I didn't add a key column, because the client check covers the UI case without a schema change.
(Written by Claude Opus 5.5)
A rotated subscription replaces the old one only when the signed-in user enabled it, so a rotation never opts in whoever is signed in now. The switch drops a browser subscription made for an earlier VAPID key pair, which could not receive pushes and would block enrolling with the new key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8132d5353b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // This device stops notifying once it signs out. | ||
| if (session) await push.forget(c.env.DB, session.session.id); |
There was a problem hiding this comment.
If a push has already been accepted while the device is offline, deleting only the D1 row does not cancel that message at the push service; with the 24-hour TTL in push/notify.ts, the retained browser subscription can therefore display the previous user's project and item title after sign-out, potentially while another user is using the device. Fresh evidence since the earlier thread is that the current app.js no longer attaches the browser-side sign-out unsubscribe handler added in 6f17641, so sign-out needs to deactivate the PushSubscription as well as forget the server row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 7a68873. The sign-out form now unsubscribes the browser's push subscription before it submits, then the server forgets the session's rows. Unsubscribing discards any push the push service still holds for that device. A test runs app.js against a stand-in browser and checks that the unsubscribe happens before the submit.
A sign-out with JavaScript off still deletes the server rows but can't drop the browser subscription. A push already queued for that device can then still arrive within its 24-hour TTL. I recorded that gap in the PR body and the Worker README.
(Written by Claude Opus 5.5)
The sign-out form unsubscribes the browser's push subscription before it submits, so a push the push service still holds for that device is discarded rather than shown to whoever uses it next. A sign-out without JavaScript leaves such pushes to arrive within their 24-hour TTL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a68873f56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Seen once shown, so the nav's count agrees with the page that shows it. | ||
| if (active?.need) await inbox.markSeen(c.env.DB, user, active); |
There was a problem hiding this comment.
When /inbox opens an unread active item, page() computes and embeds the unread count before this update runs, so the returned nav and data-unread still include the decision now being shown. In particular, tapping a notification leaves the installed-app badge and nav one too high until another page load; mark the item before computing the rendered count or explicitly adjust that count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in dadfe66. /inbox marks its active decision seen before rendering, and drops it from the page's own "new" count. The nav, data-unread, and the installed-app badge now leave out the decision being shown. The push test checks the counts on that page.
(Written by Claude Opus 5.5)
| registration | ||
| .then((found) => found.pushManager.getSubscription()) | ||
| .then((subscription) => subscription?.unsubscribe()) | ||
| .catch(() => {}) | ||
| .finally(() => form.submit()); |
There was a problem hiding this comment.
When a user signs out with a nonzero unread badge, this handler unsubscribes and submits without clearing that badge. The redirect lands on a signed-out page where Layout no longer loads /app.js, so the old account's badge remains on the installed app indefinitely; clear it before submitting, independently of whether a push subscription exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in dadfe66. The sign-out handler now clears the app badge along with unsubscribing, and runs whether or not push is supported or a subscription exists. It submits once both finish. The app.js test covers both steps.
(Written by Claude Opus 5.5)
…ign-out The inbox marks its active decision seen before the nav and the app badge count what is new, so opening a notification lowers the badge at once. The sign-out form also clears the app badge, which signed-out pages never update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # cloudflare/src/inbox/model.ts
Rename the push migration to 0014_push.sql after 0012_project_members.sql and 0013_reconcile.sql. Push recipients are now the project's members. A webhook's notification goes after its background read, so a title the webhook lacks can come from GitHub. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for head Since the first review
Decisions
Known gaps and follow-ups
(Written by Claude Opus 5.5) |
Implements and deletes quest/m0/kix/mobile.md: kix.quest installs as a phone or desktop app and sends a Web Push notification whenever something needs a decision.
What changes
start_url: /inbox, standalone), full-bleed app icons (design/theme/icon.svgand its 192/512 PNG exports, maskable-safe), anapple-touch-icon, and a service worker at/sw.js. Signed-in pages load/app.js, which registers it./inbox(compact, until this device is on) and/accountshow a switch. Permission is requested only from the button click. On iPhone/iPad outside a Home Screen app it explains how to add Quest to the Home Screen. Subscriptions are stored in a newpush_subscriptionstable (migration0014_push.sql), keyed by endpoint, with the user and the session that enabled them. Only known push services (FCM, Mozilla, Apple, Windows) are accepted./push/check, which also moves it to the current session), so whoever signs in next on that browser turns notifications on with their own click. A rotated subscription (pushsubscriptionchange) replaces the old one only for the user who enabled it, and the switch drops a browser subscription made for an earlier VAPID key. A 404/410 from the push service deletes the subscription.pushsubscriptionchangere-registers a rotated subscription and replaces the old one.cloudflare/src/push/notify.ts): an agent question (runnerquestion), a publication decision, an account limit, a failed job, a new or reopened issue, and a PR opened non-draft or marked ready for review. Progress, presence, completions, draft PRs, other webhook updates, and paused projects (including jobs that stop after a pause) never push. A webhook's push goes after its background read (#170), so a title the webhook lacks can come from GitHub. Each push fires once, on the fresh transition (retries and duplicate webhooks don't re-push). It carries only the project, the item title, the inbox URL, and the unread count.tagper item replaces the previous notification on the device. One pushTopicper item lets the push service replace an undelivered message. A tap opens/inbox?project=…&item=…. There are no answer buttons.cloudflare/src/push/webpush.tsimplements RFC 8291 aes128gcm encryption and RFC 8292 VAPID ES256 signing with WebCrypto, which the Workers runtime supports. It is tested against RFC 8291's worked example byte for byte, and the VAPID JWT is verified with the public key.just worker-seededleaves notifications off (empty VAPID values), since a push would leave localhost.VAPID_PUBLIC_KEY(var) plus theVAPID_PRIVATE_KEYsecret. Both are optional, like Google sign-in: notifications stay off until both are set.bun scripts/vapid.tsprints a pair. README,.dev.vars.example, and deployment steps are updated.Decisions I made (please confirm)
VAPID_PRIVATE_KEYtosecrets.requiredand breaking deploys and local dev until keys exist.devices()joinsgithub_project_members, from #174). Each push rechecks their current maintainer authority on GitHub, as the inbox does, and skips them if it fails.opened/reopenedorready_for_review. The push text uses the signed webhook's title, display only, and nothing about it is stored.Urgency: high, title cut at 120 characters, VAPIDsub= theAUTH_URLorigin.Validation
just checkpasses.just testpasses: 285 CLI tests and 295 Worker tests, including the newtest/push.test.tsand a hub test intest/jobs.test.ts.Suggested follow-ups
## Decisions neededhand-back, once the inbox models them as decision items.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)