feat: browser notification when game starts while tab is inactive#3665
Open
baculinivan-web wants to merge 23 commits intoopenfrontio:mainfrom
Open
feat: browser notification when game starts while tab is inactive#3665baculinivan-web wants to merge 23 commits intoopenfrontio:mainfrom
baculinivan-web wants to merge 23 commits intoopenfrontio:mainfrom
Conversation
|
|
Contributor
WalkthroughAdds opt-in browser notifications for game-start events: new translation strings, a persisted user setting and toggle, a notification prompt component shown when joining a lobby, JoinLobbyModal and Client wiring to request permission and emit a browser Notification when a joined game starts while the tab is not visible. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JoinModal as "JoinLobbyModal"
participant Client
participant UserSettings
participant BrowserAPI as "Notification API"
User->>JoinModal: open join modal / click Join
JoinModal->>UserSettings: check browserNotifications() and dismissal flag
alt prompt shown
JoinModal->>User: show <notification-prompt>
User->>JoinModal: click Enable
JoinModal->>BrowserAPI: requestPermission() (if "default")
BrowserAPI-->>User: browser permission prompt
User-->>BrowserAPI: Grant/Deny
alt granted
JoinModal->>UserSettings: toggleBrowserNotifications() (set true)
else denied
JoinModal->>User: show denied hint
end
end
JoinModal->>Client: lobby join completes (game starting)
Client->>UserSettings: browserNotifications()
alt enabled and document.visibilityState != "visible" and Notification exists and permission == "granted"
Client->>BrowserAPI: new Notification(title, {body, icon})
BrowserAPI->>User: show browser notification
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/Main.ts`:
- Around line 872-873: Replace the hardcoded English strings passed to the
Notification constructor in Main.ts with i18n lookups using the keys defined in
resources/lang/en.json (e.g., use i18n.t('notifications.game_start.title') and
i18n.t('notifications.game_start.body') or the project's translation helper such
as t(...) if available) so the Notification("OpenFront - Game Starting!", {
body: "Your game is starting. Come back to play!" }) call uses translated title
and body; keep the Notification call and options shape the same, just substitute
the literal strings with the appropriate translation function and keys.
- Around line 865-887: The sendGameStartNotification flow should stop calling
Notification.requestPermission() in non-user-gesture contexts; modify
sendGameStartNotification (and its inner send) to only act when
Notification.permission === "granted" and userSettings.browserNotifications() is
true, remove the Notification.requestPermission() branch, and wrap the new
Notification(...) call in a try-catch to guard against runtime errors; keep
checks for "Notification" in window and document.visibilityState !== "visible"
and ensure you reference sendGameStartNotification, send,
Notification.permission, and userSettings.browserNotifications() when making the
changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77529a1e-0cd8-428c-adec-68aefc7cb443
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/Main.tssrc/client/UserSettingModal.tssrc/core/game/UserSettings.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/NotificationPrompt.ts`:
- Around line 17-27: The handleEnable() currently calls dismiss() which writes
settings.notificationPromptDismissed to localStorage; stop persisting the
dismissal when the user clicks Enable by either removing the this.dismiss() call
from handleEnable() and instead just set this.visible = false;
this.requestUpdate();, or change dismiss() to accept a boolean (e.g.,
dismiss(persist: boolean)) and call dismiss(false) from handleEnable() while
keeping the original persist=true behavior for the "Not now" path; update
references to handleEnable and dismiss accordingly so only the "Not now" flow
writes settings.notificationPromptDismissed.
In `@src/client/JoinLobbyModal.ts`:
- Around line 298-312: In JoinLobbyModal.open, reset the modal's
showNotificationPrompt flag before evaluating the gating logic so stale true
values don't persist across opens; specifically, set this.showNotificationPrompt
= false at the start of the open() method (before the existing conditional that
may set it to true), then keep the existing checks and only set it to true when
all conditions pass.
- Around line 449-454: handleEnableNotifications is using
toggleBrowserNotifications which can flip the value if called twice; make the
handler idempotent by explicitly setting the setting to enabled instead of
toggling or by checking the current state before toggling. Concretely, replace
the toggleBrowserNotifications call with an explicit setter on userSettings
(e.g., setBrowserNotificationsEnabled(true)) or wrap the toggle in a guard like
if (!userSettings.browserNotificationsEnabled) {
userSettings.toggleBrowserNotifications(); } so repeated calls to
handleEnableNotifications cannot turn notifications off; keep
Notification.requestPermission() and showNotificationPrompt = false as-is.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f3a7368-803d-4ca5-aec2-2459e589434b
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/JoinLobbyModal.tssrc/client/components/NotificationPrompt.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/NotificationPrompt.ts`:
- Around line 48-56: The two buttons in the NotificationPrompt component (the
element invoking this.handleEnable and the one invoking this.dismiss) are
missing explicit types and can accidentally submit a surrounding form; update
both button elements to include type="button" to prevent implicit form
submission, i.e., add the type attribute to the button that calls handleEnable
and the button that calls dismiss within the NotificationPrompt component.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c4344ee-0bc3-40ef-bdd2-efe015dcf2f8
📒 Files selected for processing (2)
src/client/JoinLobbyModal.tssrc/client/components/NotificationPrompt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/JoinLobbyModal.ts
baculinivan-web
force-pushed
the
feature/browser-notifications-on-game-start
branch
from
0327496 to
b4bd2ba
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/components/NotificationPrompt.ts (1)
48-56:⚠️ Potential issue | 🟡 MinorAdd explicit
type="button"to both buttons.Without
type, a button defaults to submit and can trigger accidental form submission when nested in a form.Suggested fix
<button + type="button" class="px-3 py-1 text-xs font-semibold bg-blue-500/20 hover:bg-blue-500/40 border border-blue-500/40 rounded-lg text-blue-300 transition-colors" `@click`=${this.handleEnable} > ${translateText("notification_prompt.enable")} </button> <button + type="button" class="px-3 py-1 text-xs text-white/40 hover:text-white/70 transition-colors" `@click`=${this.dismiss} > ${translateText("notification_prompt.dismiss")} </button>#!/bin/bash # Verify button tags in this component that miss explicit type= rg -nP '<button(?![^>]*\btype=)' src/client/components/NotificationPrompt.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/NotificationPrompt.ts` around lines 48 - 56, The two buttons in NotificationPrompt (the one that calls this.handleEnable and the one that calls this.dismiss) are missing explicit type attributes and will default to submit inside forms; update both <button> elements to include type="button" so they do not trigger form submission (locate the buttons in src/client/components/NotificationPrompt.ts where `@click`=${this.handleEnable} and `@click`=${this.dismiss} are used and add the type attribute to each).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/components/NotificationPrompt.ts`:
- Around line 48-56: The two buttons in NotificationPrompt (the one that calls
this.handleEnable and the one that calls this.dismiss) are missing explicit type
attributes and will default to submit inside forms; update both <button>
elements to include type="button" so they do not trigger form submission (locate
the buttons in src/client/components/NotificationPrompt.ts where
`@click`=${this.handleEnable} and `@click`=${this.dismiss} are used and add the type
attribute to each).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2836bf93-e85e-47f1-99cc-0fda2e3028a3
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/components/NotificationPrompt.tssrc/core/game/UserSettings.tstests/client/BrowserNotifications.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/Main.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/lang/en.json
- src/client/JoinLobbyModal.ts
- src/core/game/UserSettings.ts
Implements issue openfrontio#3632. When a player joins a lobby and switches to another tab while waiting, they now receive a browser notification when the game starts. Changes: - UserSettings: add browserNotifications() / toggleBrowserNotifications() - GameStartingModal: call sendBrowserNotification() on show(), requests permission if needed, only fires when tab is not visible - UserSettingModal: add toggle in Basic Settings - en.json: add translation keys for notification text and setting label
Notification was firing too early (during asset loading). Moved to lobbyHandle.join.then() which fires when the game is actually ready to play and spawn selection is available.
Shows a dismissible banner inside JoinLobbyModal when: - user has never played before (gamesPlayed === 0) - browser notifications are not enabled - permission is not denied - prompt was not previously dismissed Clicking 'Enable' turns on the setting and requests permission. Clicking 'Not now' permanently dismisses via localStorage.
Remove Notification.requestPermission() from sendGameStartNotification since it cannot be called outside a user gesture context. Permission is now requested only when the user explicitly enables the setting. Wrap new Notification() in try-catch to guard against runtime errors.
- NotificationPrompt: handleEnable no longer calls dismiss(), only hides the prompt without persisting dismissal to localStorage; only the 'Not now' path writes notificationPromptDismissed - JoinLobbyModal.open: reset showNotificationPrompt to false at the start to prevent stale true values across multiple opens - JoinLobbyModal.handleEnableNotifications: guard toggle with a check so repeated calls cannot flip notifications off
baculinivan-web
force-pushed
the
feature/browser-notifications-on-game-start
branch
from
eead8aa to
3c4d04c
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/components/NotificationPrompt.ts`:
- Around line 30-33: The dismissForever method currently calls
localStorage.setItem which can throw and prevent the prompt from closing; wrap
the storage write in a try/catch and move the state changes (this.visible =
false; this.requestUpdate()) into a finally block so the prompt always closes
even if localStorage fails; optionally log the caught error (e.g., console.warn
or a logger) inside the catch for diagnostics while ensuring dismissForever
always sets visibility and requests an update.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cebe7893-6024-4466-a8db-f24e8cf5ed9d
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/components/NotificationPrompt.tssrc/core/game/UserSettings.tstests/client/BrowserNotifications.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/Main.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/game/UserSettings.ts
- resources/lang/en.json
- src/client/JoinLobbyModal.ts
- tests/client/BrowserNotifications.test.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved and changes approved. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/Main.ts (1)
883-905: Small simplification: inline thesend()helper.The inner
send()is defined and then immediately called once on line 904 — no deferral, no reuse. Folding it into the main body makes the control flow easier to read (one early-return chain instead of a nested closure), which fits the "clean, idiomatic TS" vibe.♻️ Proposed simplification
private sendGameStartNotification() { if (!this.userSettings.browserNotifications()) return; if (!("Notification" in window)) return; if (Notification.permission !== "granted") return; + if (document.visibilityState === "visible") return; - const send = () => { - if (document.visibilityState !== "visible") { - try { - new Notification( - translateText("game_starting_modal.notification_title"), - { - body: translateText("game_starting_modal.notification_body"), - icon: "/favicon.ico", - }, - ); - } catch (e) { - console.warn("[Notification] Failed to send notification:", e); - } - } - }; - - send(); + try { + new Notification( + translateText("game_starting_modal.notification_title"), + { + body: translateText("game_starting_modal.notification_body"), + icon: "/favicon.ico", + }, + ); + } catch (e) { + console.warn("[Notification] Failed to send notification:", e); + } }Side note: the
try/catchis well placed — Chrome on Android throws aTypeErrorfromnew Notification(...)even when permission is granted (it requires a Service Worker there), so the guard is genuinely useful, not just defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` around lines 883 - 905, The helper function send() inside sendGameStartNotification is defined and immediately invoked, so inline its body into sendGameStartNotification to simplify control flow: remove the nested send() function and replace the call to send() with the same conditional (if (document.visibilityState !== "visible") { try { new Notification(...) } catch (e) { console.warn(...) } }), preserving the existing early-return checks (this.userSettings.browserNotifications(), "Notification" in window, Notification.permission === "granted") and keeping the try/catch and translateText/icon usage exactly as in the removed send() block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 455-463: handleEnableNotifications currently flips
userSettings.browserNotifications before asking the browser; change it so the
stored setting is only enabled when the browser permission is actually granted:
if Notification.permission === "granted" then call
userSettings.toggleBrowserNotifications(), otherwise call
Notification.requestPermission() (use the Promise form) and inside
.then(permission => { if (permission === "granted")
userSettings.toggleBrowserNotifications(); }). Keep the existing
showNotificationPrompt update behavior but do not toggle the stored setting
until permission resolves to "granted"; use the handleEnableNotifications and
userSettings.browserNotifications / toggleBrowserNotifications symbols to locate
the code.
---
Nitpick comments:
In `@src/client/Main.ts`:
- Around line 883-905: The helper function send() inside
sendGameStartNotification is defined and immediately invoked, so inline its body
into sendGameStartNotification to simplify control flow: remove the nested
send() function and replace the call to send() with the same conditional (if
(document.visibilityState !== "visible") { try { new Notification(...) } catch
(e) { console.warn(...) } }), preserving the existing early-return checks
(this.userSettings.browserNotifications(), "Notification" in window,
Notification.permission === "granted") and keeping the try/catch and
translateText/icon usage exactly as in the removed send() block.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cae35e91-f3e8-41b7-8266-e58bf97f9d5f
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/JoinLobbyModal.tssrc/client/Main.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/client/JoinLobbyModal.ts (2)
53-55: Nit: group reactive state together.
userSettingsis a plain instance field sandwiched between@state()fields. Not a bug, just easier on the eyes if it sits above (or below) the reactive-state block. Feel free to ignore if you prefer the current grouping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/JoinLobbyModal.ts` around lines 53 - 55, Move the plain instance field userSettings so it sits with the reactive state declarations: place "private userSettings = new UserSettings()" either immediately above or immediately below the `@state`() properties (e.g., near "private showNotificationPrompt") so all reactive/state-related fields are grouped together; keep the existing `@state`() decorator on showNotificationPrompt and do not change its name or type.
465-472: Tiny safety net: add a.catchtorequestPermission().In some browsers (older Safari, certain iframes/insecure contexts)
Notification.requestPermission()can reject instead of resolving to"denied". Since no handler is attached, that becomes an unhandled promise rejection. A one-line.catchkeeps the flow tidy and idiomatic.🛡️ Proposed tweak
- } else if (Notification.permission === "default") { - Notification.requestPermission().then((permission) => { - if ( - permission === "granted" && - !this.userSettings.browserNotifications() - ) { - this.userSettings.toggleBrowserNotifications(); - } - }); - } + } else if (Notification.permission === "default") { + Notification.requestPermission() + .then((permission) => { + if ( + permission === "granted" && + !this.userSettings.browserNotifications() + ) { + this.userSettings.toggleBrowserNotifications(); + } + }) + .catch((e) => { + console.warn("[Notification] requestPermission failed:", e); + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/JoinLobbyModal.ts` around lines 465 - 472, The Notification.requestPermission() call inside JoinLobbyModal should attach a .catch handler to avoid unhandled promise rejections; update the call that currently checks permission and calls this.userSettings.toggleBrowserNotifications() to append a .catch that handles or logs the rejection (e.g., swallow or call a logger) so rejected promises from Notification.requestPermission() won’t surface as unhandled errors.tests/client/BrowserNotifications.test.ts (2)
86-161: Consider one more case:Notificationnot present at all.All tests in this block assume
Notificationexists onglobalThis.open()guards with"Notification" in window, but there is no test covering the branch where the global is missing (e.g. older browsers, some WebViews). A quickvi.stubGlobal("Notification", undefined)case would lock in the "do not show prompt / do not crash" contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/BrowserNotifications.test.ts` around lines 86 - 161, Add a test covering the case when Notification is not defined: create a new it() where you stub global Notification to undefined (vi.stubGlobal("Notification", undefined)), instantiate JoinLobbyModal via makeModal(), call m.open(), and assert (m as any).showNotificationPrompt is false (and that open() doesn't throw). Reference JoinLobbyModal.open and the showNotificationPrompt property to locate where to add this test alongside the other notification tests.
193-199: Flushing microtasks with a singleawait Promise.resolve()is fragile.
handleEnableNotifications()chains.then()ontoNotification.requestPermission(), so one microtask is enough today — but any future tweak (e.g. adding.catch(...), or awaiting an extra step) silently breaks these assertions. Prefer returning the promise fromhandleEnableNotificationsandawaiting it directly, or usevi.waitFor(...)/ a microtask-flush helper for resilience.💡 Suggested approach
- const m = makeModal(); - (m as any).handleEnableNotifications(); - await Promise.resolve(); - expect((m as any).userSettings.browserNotifications()).toBe(true); + const m = makeModal(); + (m as any).handleEnableNotifications(); + await vi.waitFor(() => + expect((m as any).userSettings.browserNotifications()).toBe(true), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/BrowserNotifications.test.ts` around lines 193 - 199, The test's microtask flush is fragile; update handleEnableNotifications so it returns the Promise from Notification.requestPermission (or otherwise returns a Promise that resolves when the enable flow completes) and then change the test to await that returned promise instead of using await Promise.resolve(); alternatively modify the test to use vi.waitFor(...) to wait until (m as any).userSettings.browserNotifications() becomes true. Target the handleEnableNotifications method and the test expectation in BrowserNotifications.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/client/BrowserNotifications.test.ts`:
- Around line 115-120: Test name is misleading because makeModal().open()
doesn't read "gamesPlayed"; either remove the test or make its intent explicit.
Fix by either deleting the it("shows prompt even when gamesPlayed > 0", ...)
block, or rename it to something like it("open() unaffected by unrelated
localStorage keys") and add a short comment stating this guards against
accidental gating by other keys; if the original intent was to gate by
gamesPlayed instead, update the modal logic (open()) to consult
ls.getItem("gamesPlayed") and adjust assertions accordingly. Ensure references
to makeModal(), open(), showNotificationPrompt, and ls.setItem("gamesPlayed",
...) are updated to match the chosen approach.
---
Nitpick comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 53-55: Move the plain instance field userSettings so it sits with
the reactive state declarations: place "private userSettings = new
UserSettings()" either immediately above or immediately below the `@state`()
properties (e.g., near "private showNotificationPrompt") so all
reactive/state-related fields are grouped together; keep the existing `@state`()
decorator on showNotificationPrompt and do not change its name or type.
- Around line 465-472: The Notification.requestPermission() call inside
JoinLobbyModal should attach a .catch handler to avoid unhandled promise
rejections; update the call that currently checks permission and calls
this.userSettings.toggleBrowserNotifications() to append a .catch that handles
or logs the rejection (e.g., swallow or call a logger) so rejected promises from
Notification.requestPermission() won’t surface as unhandled errors.
In `@tests/client/BrowserNotifications.test.ts`:
- Around line 86-161: Add a test covering the case when Notification is not
defined: create a new it() where you stub global Notification to undefined
(vi.stubGlobal("Notification", undefined)), instantiate JoinLobbyModal via
makeModal(), call m.open(), and assert (m as any).showNotificationPrompt is
false (and that open() doesn't throw). Reference JoinLobbyModal.open and the
showNotificationPrompt property to locate where to add this test alongside the
other notification tests.
- Around line 193-199: The test's microtask flush is fragile; update
handleEnableNotifications so it returns the Promise from
Notification.requestPermission (or otherwise returns a Promise that resolves
when the enable flow completes) and then change the test to await that returned
promise instead of using await Promise.resolve(); alternatively modify the test
to use vi.waitFor(...) to wait until (m as
any).userSettings.browserNotifications() becomes true. Target the
handleEnableNotifications method and the test expectation in
BrowserNotifications.test.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc0aa5fd-ef45-4add-a5b2-6d5d0307b81c
📒 Files selected for processing (2)
src/client/JoinLobbyModal.tstests/client/BrowserNotifications.test.ts
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved and changes approved. |
evanpelle
reviewed
Apr 18, 2026
Comment on lines
+91
to
+92
| <path | ||
| d="M70.697 39.701c-2.907-12.028-8.596-24.974-14.216-24.974-.278 0-.556.033-.823.097-1.225.296-2.2 1.229-2.916 2.744-.076.06-.159.106-.222.186-3.146 4.022-14.412 17.306-27.265 20.412l-9.82 2.534c-.039.009-.077.021-.115.033-3.009.827-5.549 2.72-7.184 5.362-1.71 2.766-2.23 6.034-1.463 9.205s2.722 5.841 5.506 7.52c2.771 1.672 6.048 2.169 9.225 1.402l2.036-.492h.002.001l2.449-.592 8.126 16.697c1.058 2.172 3.24 3.438 5.509 3.438.897 0 1.808-.197 2.667-.615 3.031-1.477 4.297-5.144 2.821-8.176l-6.57-13.5c11.731-.099 23.711 5.353 26.449 6.681.98.73 1.973 1.135 2.964 1.135.278 0 .556-.033.823-.098 2.594-.627 4.085-4.076 4.312-9.975.204-5.297-.611-12.053-2.296-19.024zM20.792 61.695c-2.505.607-5.088.215-7.271-1.102s-3.72-3.415-4.321-5.905-.195-5.058 1.148-7.228c1.323-2.141 3.406-3.66 5.872-4.281l.866-.21 4.481 18.538zM41.056 80.32c-1.742.848-3.853.12-4.7-1.623l-2.949-6.059 6.409-2.895 2.861 5.878c.849 1.742.121 3.85-1.621 4.699zm-2.378-12.916l-6.409 2.895-3.789-7.786 3.01-.728c1.351-.326 2.733-.541 4.129-.667zm-7.8-8.146l-6.784 1.639-4.481-18.539 6.255-1.515c10.808-2.612 20.417-11.869 25.586-17.701-.046.531-.084 1.079-.107 1.657-.107 2.758.069 5.917.495 9.291.006.048-.006.094.005.143.004.016.016.027.02.043.392 3.051.984 6.273 1.776 9.548.776 3.211 1.752 6.486 2.87 9.564.006.061-.008.119.007.18.043.177.127.328.23.465 1.283 3.438 2.746 6.592 4.308 9.1-6.992-2.773-19.305-6.505-30.18-3.875zm23.744-24.021c1.959.539 4.284 3.213 5.269 7.284.977 4.046.149 7.465-1.333 8.859-.85-2.446-1.662-5.163-2.388-8.168-.659-2.726-1.173-5.407-1.548-7.975zm15.774 23.388c-.189 4.871-1.304 7.301-2.326 7.548-.068.017-.138.024-.212.024-.23 0-.486-.071-.764-.208-.109-.137-.245-.255-.41-.341l-.404-.205c-1.881-1.551-4.42-5.476-6.779-11.51 2.93-1.8 4.207-6.688 2.917-12.024-1.297-5.364-4.683-9.147-8.139-9.372-.3-2.763-.423-5.352-.335-7.639.188-4.871 1.303-7.3 2.325-7.547.068-.017.138-.024.212-.024 2.507 0 8.112 8.185 11.689 22.985 1.631 6.745 2.421 13.248 2.226 18.313zm10.23-19.726c-.574-.012-1.091-.407-1.234-.991-.171-.697.256-1.401.953-1.572l8.494-2.084c.696-.172 1.401.256 1.572.953s-.256 1.401-.953 1.572l-8.494 2.084c-.113.028-.227.041-.338.038zm-6.435-14.206c-.348-.015-.689-.17-.933-.456-.466-.547-.399-1.368.148-1.833l7.659-6.514c.546-.464 1.368-.399 1.832.148s.399 1.368-.148 1.833l-7.659 6.514c-.26.222-.582.322-.899.308zm17.87 32.089c-.171.029-.351.025-.532-.02l-9.416-2.326c-.698-.172-1.122-.877-.95-1.573s.873-1.126 1.573-.95l9.416 2.326c.698.172 1.122.877.95 1.573-.126.517-.547.885-1.041.97z" |
Collaborator
There was a problem hiding this comment.
can you move this to at's own svg in the resources folder, and then import it like: import megaphoneIcon from './megaphone.svg?raw'
(the raw inlines it into the js bundle)
| * Visibility is controlled by the parent via the `visible` property. | ||
| */ | ||
| @customElement("notification-prompt") | ||
| export class NotificationPrompt extends LitElement { |
Collaborator
There was a problem hiding this comment.
I think we already have a prompt component? @ryanbarlow97 can we reuse an existing one?
Contributor
Author
There was a problem hiding this comment.
Checked the components directory — there's no existing reusable prompt/banner component. NotLoggedInWarning is the closest thing but it's a button, not a banner. Happy to extract a generic one if you'd prefer, but kept it self-contained for now to avoid scope creep.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/client/BrowserNotifications.test.ts (2)
195-217: Consider one extra test for idempotency on the async path.The current idempotency test (lines 211-217) only covers the synchronous
"granted"branch. The more interesting case is two rapid clicks while permission is"default": both calls enter therequestPermission()branch, both.thencallbacks fire, and you rely on!this.userSettings.browserNotifications()guarding the second toggle. A test like:it("async path is idempotent - two clicks while default do not disable", async () => { const m = makeModal(); (m as any).handleEnableNotifications(); (m as any).handleEnableNotifications(); await Promise.resolve(); await Promise.resolve(); expect((m as any).userSettings.browserNotifications()).toBe(true); });would pin that guard. Nice-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/BrowserNotifications.test.ts` around lines 195 - 217, Add a new test that verifies idempotency for the async permission path: call makeModal(), invoke (m as any).handleEnableNotifications() twice in succession while notifMock.requestPermission is left to resolve (i.e., permission initially "default"), await two microtask ticks (e.g., two Promise.resolve() awaits) to let both requestPermission then-callbacks run, and assert (m as any).userSettings.browserNotifications() is true; this ensures the guard in handleEnableNotifications prevents the second callback from disabling notifications.
131-137: Optional: fold the cache reset into your test helpers.
(UserSettings as any).cache = new Map()is repeated here after als.setItembecauseUserSettingshas a module-level static cache that must be flushed once storage is seeded post-resetStorage(). It works, but the pattern is easy to forget in a future test and will cause confusing stale reads.One small helper keeps it obvious:
function seedLS(key: string, value: string) { ls.setItem(key, value); (UserSettings as any).cache = new Map(); }Then the test becomes
seedLS("settings.browserNotifications", "true"). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/BrowserNotifications.test.ts` around lines 131 - 137, The test resets localStorage then directly manipulates the module-level UserSettings cache via (UserSettings as any).cache = new Map(), which is repeated and easy to forget; add a small test helper (e.g., seedLS) that calls ls.setItem(key, value) and clears UserSettings.cache, then update this test (and other similar tests) to call that helper instead of manually touching (UserSettings as any).cache so storage seeding and cache flush are always done together; reference symbols: UserSettings (cache), ls.setItem, resetStorage(), and the test helper name you add (e.g., seedLS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 455-474: In handleEnableNotifications, handle two cases: add a
.catch handler to the Notification.requestPermission() promise (or use try/catch
with async) to avoid unhandled rejections and log/ignore failures gracefully,
and explicitly handle Notification.permission === "denied" by showing a
user-facing hint instead of silently closing the prompt (e.g., set a flag or
call a UI method to display the notification_prompt.denied_hint localized
message); update references to userSettings.browserNotifications() and
userSettings.toggleBrowserNotifications() unchanged, but ensure
showNotificationPrompt is only cleared when appropriate and that the denied hint
key is added to resources/lang/en.json.
---
Nitpick comments:
In `@tests/client/BrowserNotifications.test.ts`:
- Around line 195-217: Add a new test that verifies idempotency for the async
permission path: call makeModal(), invoke (m as any).handleEnableNotifications()
twice in succession while notifMock.requestPermission is left to resolve (i.e.,
permission initially "default"), await two microtask ticks (e.g., two
Promise.resolve() awaits) to let both requestPermission then-callbacks run, and
assert (m as any).userSettings.browserNotifications() is true; this ensures the
guard in handleEnableNotifications prevents the second callback from disabling
notifications.
- Around line 131-137: The test resets localStorage then directly manipulates
the module-level UserSettings cache via (UserSettings as any).cache = new Map(),
which is repeated and easy to forget; add a small test helper (e.g., seedLS)
that calls ls.setItem(key, value) and clears UserSettings.cache, then update
this test (and other similar tests) to call that helper instead of manually
touching (UserSettings as any).cache so storage seeding and cache flush are
always done together; reference symbols: UserSettings (cache), ls.setItem,
resetStorage(), and the test helper name you add (e.g., seedLS).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae78e9a6-a0f4-41d5-ae16-a3e74540e6d4
⛔ Files ignored due to path filters (1)
src/client/components/megaphone.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
src/client/JoinLobbyModal.tssrc/client/components/NotificationPrompt.tstests/client/BrowserNotifications.test.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 314-323: Wrap the localStorage read in a try/catch inside the same
function (likely open() in JoinLobbyModal) so a SecurityError doesn't block
execution: attempt to read
localStorage.getItem("settings.notificationPromptDismissed") and if it throws,
treat dismissed as false (or fallback to a safe default), then continue with the
existing condition that checks this.userSettings.browserNotifications(),
Notification, and Notification.permission and set this.showNotificationPrompt
accordingly; ensure failure to access storage does not prevent the rest of
open() from running (including startTrackingLobby()).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6b6d914-3566-4d92-836e-07b6a64df9e7
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/JoinLobbyModal.tssrc/client/components/NotificationPrompt.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- src/client/components/NotificationPrompt.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/JoinLobbyModal.ts (1)
321-327:⚠️ Potential issue | 🟠 MajorGuard the
UserSettingscalls too.
open()now catches the direct dismissal read, but Line 323 can still throw throughUserSettings.getCached()because it also readslocalStorage. That aborts beforestartTrackingLobby(). The enable path can also throw while reading/writing the setting. Since this prompt is optional, skip or log the prompt flow when settings storage is blocked.Suggested fix
public open(lobbyId: string = "", lobbyInfo?: GameInfo | PublicGameInfo) { super.open(); this.showNotificationPrompt = false; this.notificationDeniedHint = ""; // Show notification prompt whenever notifications are not enabled and not permanently dismissed - let dismissed = false; try { - dismissed = + const dismissed = localStorage.getItem("settings.notificationPromptDismissed") === "true"; - } catch { - // localStorage unavailable (e.g. SecurityError in private mode) — treat as not dismissed - } - if ( - !dismissed && - !this.userSettings.browserNotifications() && - "Notification" in window && - Notification.permission !== "denied" - ) { - this.showNotificationPrompt = true; + const notificationsEnabled = this.userSettings.browserNotifications(); + + if ( + !dismissed && + !notificationsEnabled && + "Notification" in window && + Notification.permission !== "denied" + ) { + this.showNotificationPrompt = true; + } + } catch (e) { + console.warn("[JoinLobbyModal] Failed to evaluate notification prompt:", e); } if (lobbyId) {+ private enableBrowserNotificationsSetting() { + try { + if (!this.userSettings.browserNotifications()) { + this.userSettings.toggleBrowserNotifications(); + } + } catch (e) { + console.warn("[Notifications] Failed to persist setting:", e); + } + } + private handleEnableNotifications() { this.showNotificationPrompt = false; if (!("Notification" in window)) return; if (Notification.permission === "granted") { - if (!this.userSettings.browserNotifications()) { - this.userSettings.toggleBrowserNotifications(); - } + this.enableBrowserNotificationsSetting(); } else if (Notification.permission === "denied") { this.notificationDeniedHint = translateText( "notification_prompt.denied_hint", ); this.showNotificationPrompt = true; @@ Notification.requestPermission() .then((permission) => { if (permission === "granted") { - if (!this.userSettings.browserNotifications()) { - this.userSettings.toggleBrowserNotifications(); - } + this.enableBrowserNotificationsSetting(); } else if (permission === "denied") {#!/bin/bash # Verify the storage-backed UserSettings call chain used by JoinLobbyModal. rg -n -C3 'browserNotifications\(|toggleBrowserNotifications|getCached|setCached|localStorage\.(getItem|setItem)' src/client/JoinLobbyModal.ts src/core/game/UserSettings.tsAlso applies to: 470-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/JoinLobbyModal.ts` around lines 321 - 327, Guard calls into UserSettings in JoinLobbyModal: wrap calls to this.userSettings.browserNotifications(), getCached()/setCached() chain, and toggleBrowserNotifications() used in the notification-prompt flow in try/catch and if any storage access throws, skip showing the prompt and log a warning instead of letting the error propagate; specifically, protect the block that sets this.showNotificationPrompt (and the complementary flow that enables notifications) so that on error you call a logger (or console.warn) and proceed to call startTrackingLobby() normally without attempting the optional prompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/JoinLobbyModal.ts`:
- Around line 321-327: Guard calls into UserSettings in JoinLobbyModal: wrap
calls to this.userSettings.browserNotifications(), getCached()/setCached()
chain, and toggleBrowserNotifications() used in the notification-prompt flow in
try/catch and if any storage access throws, skip showing the prompt and log a
warning instead of letting the error propagate; specifically, protect the block
that sets this.showNotificationPrompt (and the complementary flow that enables
notifications) so that on error you call a logger (or console.warn) and proceed
to call startTrackingLobby() normally without attempting the optional prompt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c62db0f4-c315-41e9-a7fc-fde74bc0dbcf
📒 Files selected for processing (1)
src/client/JoinLobbyModal.ts
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
4 participants
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.
Resolves #3632
Description:
Adds an opt-in browser notification that fires when a game starts and the player is on a different tab. This addresses the issue where players miss their game after joining a lobby with up to 2-minute wait times.
How it works:
lobbyHandle.joinresolves), a notification is sent — but only if the tab is not currently visibleFiles changed:
src/core/game/UserSettings.ts— addedbrowserNotifications()andtoggleBrowserNotifications()src/client/Main.ts— addedsendGameStartNotification()called onjoin.then()src/client/UserSettingModal.ts— added toggle UI + requests permission on enablesrc/client/JoinLobbyModal.ts— shows notification prompt on first lobby joinsrc/client/components/NotificationPrompt.ts— new dismissible banner componentresources/lang/en.json— added translation keystests/client/BrowserNotifications.test.ts— 17 tests covering all new behaviourPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
fghjk_60845