feat(browser): PR 10 — Web Push (browser-scope v1)#311
Merged
Conversation
…arent tab-focus postMessage + data_dir mkdir
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces comprehensive browser push notification support across desktop and backend systems. It adds VAPID keypair management, push subscription persistence with muting controls, service-worker-based notification delivery, tab-focus tracking, and per-agent notification toggles in the UI. The system integrates with chat, drive-started, and download-finished events to trigger notifications when the user is not already viewing the relevant tab. ChangesBrowser Push Notification System
Sequence DiagramsequenceDiagram
participant User
participant Client as Browser Client
participant SW as Service Worker
participant Server as Backend
participant Store as Push Store
participant Sender as pywebpush
User->>Client: Click "Enable Notifications"
Client->>Client: Request Notification.permission
Client->>Server: bootstrapPushSubscription()
Server->>Server: Load/Create VAPID keypair
Client->>Client: Create service worker subscription
Client->>Server: POST /subscribe with endpoint & keys
Server->>Store: Upsert subscription
Store-->>Server: ✓
Server-->>Client: {"ok": true}
User->>Client: Focus different tab
Client->>SW: postMessage tab-focus
SW->>Server: WebSocket: tab-focus event
Server->>Server: CopilotHub.set_focused_tab()
Client->>Server: Chat message on pinned agent
Server->>Server: _maybe_send_chat_push()
alt User not viewing that tab
Server->>Store: is_push_muted(user, agent, "chat")?
alt Not muted
Server->>Sender: webpush(subscription, payload)
Sender-->>SW: Push delivery
SW->>SW: Show notification
User->>User: Sees notification
end
else User already viewing tab
Server->>Server: Suppress (focused_tab match)
end
User->>SW: Click notification
SW->>Client: postMessage taos-push:click
Client->>Client: Focus/open window
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR introduces a substantial, multi-layer feature spanning frontend and backend with dense logic in push delivery (rate limiting, concurrent sending, error handling), complex triggering logic (focus-aware suppression, optimistic UI with rollback), and deep integration with WebSockets, service workers, and storage. The changes are heterogeneous across many files but tightly coupled semantically, requiring careful review of coordination between client subscription, server delivery, storage persistence, and UI state management. The comprehensive test suites mitigate some risk but the sheer scope and novelty of push infrastructure demands careful scrutiny. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (11 files changed since last review)
Reviewed by grok-code-fast-1:optimized:free · 170,000 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/BrowserApp/TabRenderer.test.tsx`:
- Around line 334-335: Remove the dead-variable declarations in the test: delete
the unused consts messages and origPostMessage from TabRenderer.test.tsx (the
lines declaring const messages: any[] = []; and const origPostMessage =
window.postMessage.bind(window);). Ensure no other code references messages or
origPostMessage before removing, and run the tests to confirm no regressions.
In `@tests/routes/desktop_browser/test_push_subscriptions.py`:
- Line 75: Replace blocking time.sleep calls inside async test coroutines with
non-blocking sleeps: add an import for the asyncio module at the top of the test
file and change each use of time.sleep(1) found inside async def tests to use an
awaited asyncio sleep (i.e., await asyncio.sleep(1)) so the event loop is not
blocked.
In `@tests/routes/desktop_browser/test_push_triggers.py`:
- Around line 544-547: In the test cleanup blocks where a background coroutine
variable named "task" is awaited, remove the broad exception tuple and only
catch asyncio.CancelledError (i.e., change "except (asyncio.CancelledError,
Exception): pass" to "except asyncio.CancelledError: pass") so other exceptions
(test failures) are not swallowed; update both occurrences in
tests/routes/desktop_browser/test_push_triggers.py that use await task in
teardown/cleanup.
In `@tinyagentos/routes/desktop_browser/copilot_agent_ws.py`:
- Around line 226-240: The spawned tasks created with
asyncio.create_task(_maybe_send_chat_push(...)) and
asyncio.create_task(_maybe_send_drive_push(...)) can raise exceptions after
scheduling that aren’t caught by the surrounding try/except; wrap the call body
in a small coroutine wrapper (e.g., async def _safe_send_chat_push(...): try:
await _maybe_send_chat_push(...); except Exception: _logger.warning("chat push
failed", exc_info=True)) and schedule the wrapper instead (or attach a done
callback that logs exceptions from the Task); update both call sites that
currently call asyncio.create_task(...) to schedule the safe wrapper so
exceptions from push.send (or other awaits inside _maybe_send_* functions) are
caught and logged.
In `@tinyagentos/routes/desktop_browser/copilot_ws.py`:
- Around line 168-181: The focused-tab cache (_focused_tabs) currently never
gets cleared so suppression can persist after a tab disconnects; modify the code
to track liveness and clear stale focus entries: add a
clear_focused_tab(user_id, window_id, tab_id) method (or a general
clear_focused_tab(user_id)) and call it from the iframe disconnect handler when
the disconnecting (window_id, tab_id) matches the stored tuple, or instead store
a last_seen timestamp in _focused_tabs and update _maybe_send_chat_push to
ignore entries older than a short TTL; update set_focused_tab, get_focused_tab
and any disconnect handler that handles iframe websocket close to remove the
entry when appropriate, and ensure _maybe_send_chat_push uses the new liveness
check rather than relying on a permanent tuple.
In `@tinyagentos/routes/desktop_browser/push_routes.py`:
- Around line 22-26: The code uses a module-global _vapid_cache which leaks
VAPID keys across FastAPI app instances; replace this with per-app state:
remove/stop using the global _vapid_cache and instead store the keypair on the
FastAPI app state (e.g., app.state.vapid_keypair). Change _get_vapid to accept
the FastAPI app (or a Request) and check app.state.vapid_keypair, calling
load_or_create_vapid_keypair(data_dir) only if absent, then return that value;
make the analogous change for the other cached accessor (the one on lines 34-39)
so all VAPID accessors read/write app.state.vapid_keypair instead of the
module-global.
In `@tinyagentos/routes/desktop_browser/store.py`:
- Around line 1036-1068: In upsert_push_subscription, ensure you deduplicate by
endpoint: before inserting, delete any other rows for the same user that have
the same endpoint but a different device_id (e.g. execute "DELETE FROM
push_subscriptions WHERE user_id = ? AND endpoint = ? AND device_id != ?" with
params (user_id, endpoint, device_id)), then proceed with the existing INSERT
... ON CONFLICT(user_id, device_id) ... (so created_at is preserved on
conflict). Use the existing function upsert_push_subscription and table/columns
push_subscriptions, user_id, device_id, endpoint to locate where to add the
DELETE.
In `@tinyagentos/routes/desktop_browser/vapid.py`:
- Around line 36-48: The code that atomically creates the VAPID PEM using
os.open(..., os.O_EXCL) can raise FileExistsError if another process created the
file between pem_path.exists() and os.open; update the block that uses Vapid01,
pem_path and os.open to catch FileExistsError around the os.open call: on
FileExistsError fall back to loading the existing key via
Vapid01.from_pem(pem_path.read_bytes()), otherwise proceed to write pem_bytes to
the newly-opened fd (ensuring fd is closed in finally) and set vapid = the new
Vapid01 instance; this prevents the process from crashing on concurrent startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13c8d037-114b-41ab-baf0-23acd2b37dca
📒 Files selected for processing (32)
desktop/src/apps/BrowserApp/AgentPanel.notifications.test.tsxdesktop/src/apps/BrowserApp/AgentPanel.test.tsxdesktop/src/apps/BrowserApp/AgentPanel.tsxdesktop/src/apps/BrowserApp/BrowserApp.tsxdesktop/src/apps/BrowserApp/SettingsPanel.test.tsxdesktop/src/apps/BrowserApp/SettingsPanel.tsxdesktop/src/apps/BrowserApp/TabRenderer.test.tsxdesktop/src/apps/BrowserApp/TabRenderer.tsxdesktop/src/lib/browser-push-api.test.tsdesktop/src/lib/browser-push-api.tsdesktop/src/lib/browser-push-bootstrap.test.tsdesktop/src/lib/browser-push-bootstrap.tspyproject.tomltests/routes/desktop_browser/test_push_mutes.pytests/routes/desktop_browser/test_push_routes.pytests/routes/desktop_browser/test_push_send.pytests/routes/desktop_browser/test_push_subscriptions.pytests/routes/desktop_browser/test_push_triggers.pytests/routes/desktop_browser/test_vapid.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/copilot.jstinyagentos/routes/desktop_browser/copilot_agent_ws.pytinyagentos/routes/desktop_browser/copilot_ws.pytinyagentos/routes/desktop_browser/download.pytinyagentos/routes/desktop_browser/push.pytinyagentos/routes/desktop_browser/push_routes.pytinyagentos/routes/desktop_browser/schema.pytinyagentos/routes/desktop_browser/store.pytinyagentos/routes/desktop_browser/sw.jstinyagentos/routes/desktop_browser/vapid.py
| original_last_seen = rows[0]["last_seen_at"] | ||
|
|
||
| # Advance wall clock by at least 1 second so last_seen_at changes | ||
| time.sleep(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate blocking sleeps in async tests.
rg -n "time\\.sleep\\(" tests/routes/desktop_browser/test_push_subscriptions.py -C 2Repository: jaylfc/tinyagentos
Length of output: 300
🏁 Script executed:
cat -n tests/routes/desktop_browser/test_push_subscriptions.py | head -100Repository: jaylfc/tinyagentos
Length of output: 4205
🏁 Script executed:
tail -n +200 tests/routes/desktop_browser/test_push_subscriptions.py | head -30Repository: jaylfc/tinyagentos
Length of output: 679
🏁 Script executed:
head -20 tests/routes/desktop_browser/test_push_subscriptions.pyRepository: jaylfc/tinyagentos
Length of output: 510
Use async sleep in async tests to avoid blocking the event loop.
Lines 75 and 206 call time.sleep(1) inside async def tests. This blocks the event loop; use await asyncio.sleep(...) instead.
Add the import:
import asyncioReplace both calls:
await asyncio.sleep(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/routes/desktop_browser/test_push_subscriptions.py` at line 75, Replace
blocking time.sleep calls inside async test coroutines with non-blocking sleeps:
add an import for the asyncio module at the top of the test file and change each
use of time.sleep(1) found inside async def tests to use an awaited asyncio
sleep (i.e., await asyncio.sleep(1)) so the event loop is not blocked.
…up, app-state VAPID, endpoint dedup)
Owner
Author
|
@coderabbitai full review please — addressed all 8 actionable findings from your initial pass at
Backend 290/290, frontend 775/775, tsc 0 errors all green. |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes the BrowserApp v2 epic by laying the Web Push platform primitive scoped to BrowserApp v1 triggers. After this PR: cross-device push works for the three v1 trigger sites (chat / drive-started / download-finished), and the primitive is reusable for the cross-app aggregator follow-up.
What landed
Backend
pywebpushdependency + VAPID keypair bootstrap (vapid.py, persisted todata/vapid.pemmode 0600, lazy idempotent generation)push_subscriptionsstore methods (upsert/list/delete/delete-by-endpoint with multi-user isolation)GET /push/vapid-public-key(public),POST /push/subscribe,GET /push/subscriptions(strips p256dh/auth keys),DELETE /push/subscriptions/{device_id},GET/PUT /push/mutespush.send(user_id, payload, *, devices, store, vapid)with 30/min/user sliding-window rate limit, 410-Gone subscription cleanup, per-endpoint VAPID claims, 5s send timeoutpush_mutestable keyed on(user_id, agent_id, kind)where kind ∈{chat, drive-started, download-finished}app.state.vapid_keypair)Service Worker (extends
sw.jsfrom PR 8 — no second SW)pushhandler: parse JSON payload, fall back to generic title/body,event.waitUntil(showNotification(...))notificationclickhandler: focus matching same-origin window + postMessagetaos-push:click, fallbackopenWindow('/')Frontend
browser-push-api.ts— six wrappersbrowser-push-bootstrap.ts— idempotent subscription flow (gated byNotification.permission, persistentdevice_idin localStorage, re-POSTs existing subscriptions to refreshlast_seen_at)BrowserApp.tsxcalls bootstrap after SW registration (fire-and-forget)AgentPanel.tsxnotifications section with three toggles per pinned agent, optimistic update + revert on failureSettingsPanel.tsxtri-state Enable Notifications button (granted / denied / default)TabRenderer.tsxpostMessagestaos-copilot:tab-focusto iframes on active-tab changeTriggers (v1)
copilot_agent_ws.py→_maybe_send_chat_push(mute + tab-focus gated)copilot_agent_ws.py→_maybe_send_drive_pushdownload.py→ push.send viaasyncio.create_taskCopilotHub.set_focused_tab/get_focused_tab,tab-focusWS event, parent-shell postMessage fromTabRenderer.tsxOut of scope (deferred)
notifications.py) — Foundations brainstormTest Plan
pytest tests/routes/desktop_browser/→ 504/504 passingnpm test -- --run→ 775/775 passing,tsc --noEmitcleanuser_idfrom session; never from request bodyO_EXCLclients.claim()(preserves PR 8 invariant)After this PR
This closes the BrowserApp v2 epic per spec §10:
Summary by CodeRabbit
New Features
Tests