feat: deep-link a notification by ts on the notifications page (#2018) - #2518
Conversation
Push notifications relayed off the gateway (e.g. an ntfy bridge with a Click URL) could open /notifications but not the specific note the push was about. The page now honours /notifications?note=<url-encoded ts>: the id is captured off the URL, the param is consumed with a history replace (reload/back cannot re-select and re-ack), and the id resolves against the asynchronously loaded feed through the page's existing row-select path — so auto-ack-on-select, mobile full-width detail, and desktop behavior stay identical to a real tap. Desktop additionally scrolls the matching feed row into view. An unknown/expired id degrades to the plain page with nothing selected and no error surface. No new backend surface: ts already is the store id (every mutating notification API keys on it, and both the push response and the WS envelope carry it), so any pusher already holds the value it needs. Closes #2018
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Deep link rides the existing select path with disarm/replace semantics thought through — a proportionate, contract-documented solution to a real push-relay gap. Watch
[DESIGN-REVIEWED] eb491b1 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've analyzed the diff thoroughly. Let me verify the resolve/disarm effect interaction once more against the feed's stacking logic — that's the only place with real state-machine complexity, and I've traced it: capture → pending state → resolve-via-handleSelect (which nulls pending, so a user tap disarms), reveal scoped to The only thing outside the feature itself: four binary PNGs committed under a No blocking findings. FINDING — temp-screenshots/notification-deep-link/*.png — four binary screenshots committed under a scratch [OPUS-REVIEWED] eb491b1 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — 🟡 CONCERNSAdvisory UX-level review of UX-Verdict: CONCERNS The PR's own desktop screenshot shows the deep-linked note still Unread after selection — the promised auto-ack visibly doesn't stick. Watch
[UX-REVIEWED] eb491b1 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
…otdev#2018) (kirodotdev#2518) Push notifications relayed off the gateway (e.g. an ntfy bridge with a Click URL) could open /notifications but not the specific note the push was about. The page now honours /notifications?note=<url-encoded ts>: the id is captured off the URL, the param is consumed with a history replace (reload/back cannot re-select and re-ack), and the id resolves against the asynchronously loaded feed through the page's existing row-select path — so auto-ack-on-select, mobile full-width detail, and desktop behavior stay identical to a real tap. Desktop additionally scrolls the matching feed row into view. An unknown/expired id degrades to the plain page with nothing selected and no error surface. No new backend surface: ts already is the store id (every mutating notification API keys on it, and both the push response and the WS envelope carry it), so any pusher already holds the value it needs. Closes kirodotdev#2018
Summary
Push notifications relayed off the gateway (e.g. an ntfy bridge watching the WS notification stream with a Click URL) could open
/notificationsbut not the specific notification the push was about, leaving the user to hunt a busy feed.The notifications page now honours a deep-link query param:
group_keystack hiding the target and scrolls the row into view via its own container ref (never a document-scoped query, which could hit the bell popover's rows).No new backend surface:
tsalready is the store id — every mutating notification API keys on it, and both the push response and the WS envelope carry it, so any pusher already holds the value it needs. Param documented for external pushers indocs/system-specs/features/app-notifications.md(including the percent-encoding requirement — ISOtscontains+).Closes #2018
Screenshots
Captured against a live gateway serving this build (seeded feed, real deep-link navigation; post-navigation URL verified param-free).
Testing
website:npx tsc -bclean;npx vitest runfull suite 11,801 passed (877 files); jscpd 0 clones.src/test/NotificationsPage.deepLink.test.tsx(6 tests): valid id selects + acks + scrolls; param consumed with REPLACE (back-nav probe asserts the entry is gone and nothing re-acks); deep link survives a slow feed load; explicit tap disarms a pending deep link; collapsedgroup_keystack expands to reveal the target; unknown id degrades quietly (bystander seeded unacked so the no-ack assertion is falsifiable)..pychanges (docs only).isort/flake8/mypygreen; fullpytest40,268 passed with 76 failures all pre-existing host-environment classes (this box's/tmpis owned by uid 65534, breaking AppArmor-ownership and unix-socket assertions;KIROCREW_PORTenv leak from the live gateway) — verified unrelated by re-running samples with sanitized env.scripts/docs-lint.shandscripts/check_brand_name.pygreen.Pre-push review fleet
Two model-pinned READ-ONLY reviewers ran before push (GPT 5.6 Sol — codex charter; Opus 5 — claude charter + BASE-ref AUTOSDE rules; no fallback models needed).
fetchNotificationsresponses can overwrite an optimistic ack) — pre-existing onmain: the wholesalefetchNotifications.fulfilleditems replacement races the fire-and-forget ack for a plain tapped row too; this PR adds no new fetch. Filed as a follow-up issue rather than expanding this PR's scope.