tsk-vkdf7k [OPEN] BUG (Jay): tapping a push notification never opens - #2179
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
nemotron-ultra-kilo review VERDICT: Approved with minor concerns around defensive checks and test coverage gaps.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
Reviewed against the card (tsk-vkdf7k). All three faults addressed, and the constraint that mattered most was honoured. Arming auto-merge.
The thing I most wanted to see: it reuses the existing mapper instead of writing a second routing table. import { sourceToTarget, targetToAction } from "@/lib/server-notifications";
...
const route = targetToAction(payload.target);
const { action, meta } = route.action ? route : sourceToTarget(source);Zero new routing tables in the diff. And that precedence (typed Tests on both sides plus the backend ( One thing still outstanding, and it is not a blocker for this merge. The card required verification on the REAL mobile PWA, not only in vitest. I cannot do that: Jay reported it from his device and it is his device to confirm on. So the code and the tests are reviewed and I am merging on that basis, because the path is currently 100% broken and this is strictly better, but the device check is still owed. Jay, when you are up: tap a decision notification on the phone, with the app backgrounded (that was the "goes nowhere" case) and again with it closed (the "wrong destination" case). If either still misbehaves, reopen rather than assuming the tests covered it. Good, well-scoped work. |
PR Summary by QodoFix push-notification click routing across backend, service worker, and app shell
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
|
nemotron-super review VERDICT: Blocking issue found due to missing type guard for data.source in service worker.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: The fix correctly addresses the push notification routing issue with proper cross-origin guards, but has minor test gaps and one unverified dependency.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Fallback deep link unreachable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const rawUrl = typeof data["url"] === "string" && data["url"] ? (data["url"] as string) : ""; | ||
| let target = rawUrl ? rawUrl : "/"; | ||
| try { | ||
| const u = new URL(target, self.location.origin); | ||
| target = u.origin === self.location.origin && u.pathname.startsWith("/") |
There was a problem hiding this comment.
1. Fallback deep link unreachable 🐞 Bug ≡ Correctness
In sw.ts the source-derived deep-link (/desktop?app=...) is only built when data.url is missing/empty, but the backend always populates payload.data.url with "/desktop" for rows without an explicit deep link. This means when no client is open, notification clicks will still open plain /desktop and not the routed app.
Agent Prompt
### Issue description
The SW notificationclick fallback only runs when `rawUrl` is empty and `target === "/"`, but backend pushes always include `data.url` (defaulting to `"/desktop"`). This makes the new source/target routing dead in the “no existing client window” case.
### Issue Context
- Backend `_safe_url()` returns `"/desktop"` when `row.data.url` is missing/unsafe, and `_build_payload()` always includes that URL inside `payload.data`.
- The SW currently treats any non-empty `data.url` as an explicit deep link and skips building the `/desktop?app=...` fallback.
### Fix Focus Areas
- desktop/src/sw.ts[200-232]
- desktop/src/lib/__tests__/sw-push-handlers.test.ts[55-93]
- tinyagentos/notifications_push.py[172-217]
- tests/test_notifications_push.py[137-168]
### Suggested fix
Update the SW fallback condition to also treat the backend sentinel shell URL as “no explicit deep link”, e.g.:
- consider `rawUrl === "" || rawUrl === "/desktop" || rawUrl === "/desktop/"` as “no explicit url”, and
- build `/desktop?app=...` when `data.source`/`data.target` indicates a routable destination.
Also update/extend the replicated SW unit test to include `url: "/desktop"` in `notificationData` so it matches real payloads.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export function usePushClickHandler(openWindow: OpenWindow): void { | ||
| useEffect(() => { | ||
| const handler = (e: Event) => { | ||
| const msg = (e as MessageEvent).data; | ||
| if (!msg || msg.type !== "taos-push:click") return; | ||
| const payload = msg.data || {}; | ||
| const source = typeof payload.source === "string" ? payload.source : ""; | ||
| const route = targetToAction(payload.target); | ||
| const { action, meta } = route.action ? route : sourceToTarget(source); | ||
| if (!action) return; | ||
| const size = getApp(action)?.defaultSize ?? FALLBACK_SIZE; | ||
| const props = meta && Object.keys(meta).length ? meta : undefined; | ||
| openWindow(action, size, props); | ||
| }; | ||
| window.addEventListener("message", handler); | ||
| return () => window.removeEventListener("message", handler); |
There was a problem hiding this comment.
2. Untrusted push-click messages 🐞 Bug ⛨ Security
usePushClickHandler opens apps on any window 'message' event whose data has type 'taos-push:click', without validating MessageEvent.origin or MessageEvent.source. This allows any embedded frame/window with a postMessage channel to the shell to trigger the mapped app launches (via source/target routing).
Agent Prompt
### Issue description
`usePushClickHandler()` trusts any `window` message with `{ type: "taos-push:click" }` and routes it into `openWindow(...)` without checking the sender. This makes the shell susceptible to unwanted app launches from other frames/windows that can postMessage to it.
### Issue Context
Other postMessage consumers in the repo explicitly validate the sender (e.g. `e.source === iframe.contentWindow`) before acting.
### Fix Focus Areas
- desktop/src/hooks/use-push-click.ts[13-29]
- desktop/src/apps/BrowserApp/agent-ws-bridge.ts[66-76]
- desktop/src/apps/SandboxedAppWindow.tsx[161-175]
### Suggested fix
Harden the handler by authenticating the message source:
- Prefer listening on `navigator.serviceWorker`’s `message` event (ServiceWorkerContainer) rather than `window`, and only accept events whose `event.source` matches the active controller (when present).
- If you must keep `window` messaging, add strict checks (at minimum `event.origin === window.location.origin`) and, if possible, validate `event.source` against an expected trusted sender.
- Keep the existing schema checks, but also validate that `msg.data` is a plain object before reading `source/target`.
- Update `desktop/src/hooks/use-push-click.test.ts` accordingly to dispatch the event on the correct target and to include sender validation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Autonomous build of board card tsk-vkdf7k.
Files:
desktop/src/App.tsx | 3 +
desktop/src/hooks/use-push-click.test.ts | 69 ++++++++++++++++++++++
desktop/src/hooks/use-push-click.ts | 30 ++++++++++
desktop/src/lib/tests/sw-push-handlers.test.ts | 64 +++++++++++++++++++-
desktop/src/sw.ts | 19 +++++-
tests/test_notifications_push.py | 12 +++-
tinyagentos/notifications_push.py | 14 ++++-
8 files changed, 214 insertions(+), 7 deletions(-)