Skip to content

fix(chat): correct push notification click-through URL#831

Merged
penso merged 2 commits intomainfrom
precious-sunflower
Apr 22, 2026
Merged

fix(chat): correct push notification click-through URL#831
penso merged 2 commits intomainfrom
precious-sunflower

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented Apr 22, 2026

Summary

  • Push notification "View" CTA was navigating to /chat/session:42 which doesn't match any SPA route, causing a 404
  • Fixed URL construction to produce /chats/session/42, matching the frontend sessionPath() in router.ts
  • Extracted push_notification_url() helper with unit tests

Closes #773

Validation

Completed

  • cargo +nightly-2025-11-30 fmt --all -- --check passes
  • cargo test -p moltis-chat push_notification_url — 3 tests pass
  • No other callers construct push URLs with the old /chat/ prefix

Remaining

  • just lint
  • just test
  • Manual QA: enable push notifications, trigger a heartbeat, tap the notification CTA

Manual QA

  1. Enable push notifications in Settings > Notifications
  2. Add a heartbeat cron job
  3. Wait for the heartbeat to fire
  4. Tap "View" on the push notification
  5. Verify it navigates to the correct chat session (no 404)

The push notification URL was built as `/chat/{session_key}` (e.g.
`/chat/session:42`), but the SPA router expects `/chats/session/42` —
plural prefix with colons replaced by slashes. This caused a 404 when
tapping "View" on a PWA push notification.

Extract `push_notification_url()` to mirror the frontend `sessionPath()`
logic and add unit tests for standard, nested, and plain session keys.

Closes #773

Entire-Checkpoint: 2a575930b294
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

This PR fixes a broken push notification click-through URL by replacing the old /chat/session:42 construction with a new push_notification_url() helper that produces /chats/session/42, correctly matching the frontend sessionPath() in router.ts. The change is small and well-tested with three unit tests covering the common case, nested keys, and keys without colons.

Confidence Score: 5/5

Safe to merge — the fix is correct, well-tested, and the only remaining note is a minor feature-gate alignment.

All findings are P2 (style/cleanup); the logic fix itself is correct and unit-tested. No data integrity or runtime issues found.

No files require special attention.

Important Files Changed

Filename Overview
crates/chat/src/channels.rs Introduces push_notification_url() helper fixing the URL from /chat/session:42/chats/session/42; adds three unit tests. Minor: function lacks the same #[cfg(feature = "push-notifications")] guard as its only caller.

Sequence Diagram

sequenceDiagram
    participant Runtime as ChatRuntime
    participant Channels as channels.rs
    participant PushSvc as PushNotificationService
    participant Client as Mobile Client

    Runtime->>Channels: send_chat_push_notification(session_key, text)
    Channels->>Channels: push_notification_url(session_key)
    Note over Channels: "session:42" → "/chats/session/42"
    Channels->>PushSvc: send_push_notification(title, summary, url, session_key)
    PushSvc-->>Client: Push notification with url="/chats/session/42"
    Client->>Client: Tap "View" CTA
    Client->>Client: Navigate to /chats/session/42 ✅
    Note over Client: was /chat/session:42 → 404 ❌
Loading

Reviews (1): Last reviewed commit: "fix(chat): correct push notification cli..." | Re-trigger Greptile

Comment thread crates/chat/src/channels.rs
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented Apr 22, 2026

Merging this PR will not alter performance

✅ 39 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing precious-sunflower (e9fcfaf) with main (e11d2ba)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Add #[cfg(any(feature = "push-notifications", test))] to avoid
dead-code warnings when the feature is disabled, while keeping
unit tests compilable unconditionally.

Entire-Checkpoint: 783c8e3f7c41
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/chat/src/channels.rs 92.85% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@penso penso merged commit 7c673a2 into main Apr 22, 2026
40 of 44 checks passed
@penso penso deleted the precious-sunflower branch April 22, 2026 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Push message CTA ends in 404 on PWA

1 participant