feat: soju.im/webpush push notifications#236
Open
ValwareIRC wants to merge 3 commits into
Open
Conversation
Requests the soju.im/webpush cap, reads the VAPID= ISUPPORT token onto server state, and on a webpush-capable server (with notification permission granted) subscribes the browser PushManager against that VAPID key and sends WEBPUSH REGISTER <endpoint> p256dh=..;auth=.. Service worker gains a push handler that parses the one-IRC-line payload and shows a notification (suppressed when a window is focused) plus a notificationclick that focuses/opens the app. Enabling notifications in settings is the user-gesture opt-in.
…tration The notifications category had no master enable toggle, and the enableNotifications GlobalSettings field had no UI control at all -- so there was no way to opt into push (or in-app notifications). Add a notifications.enable toggle (key enableNotifications, priority 0); flipping it on prompts for permission + WEBPUSH REGISTER on push-capable servers, flipping it off WEBPUSH UNREGISTERs.
The SW only showed the sender nick. Now channel highlights render as "alice in #weather" and DMs as "alice", with the network name (from the manifest) appended as context. Notification tag groups by channel for highlights / by sender for DMs so repeats collapse.
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete web push notification system for ObsidianIRC. It adds a user-facing notification toggle setting, negotiates push support with IRC servers, processes incoming push notifications in the service worker, and manages subscription lifecycle in user settings, with comprehensive localization across 18 languages. ChangesWeb Push Notification System Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (22)
src/locales/cs/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing Czech translations for newly added notification strings.
Line 877 and Line 2244 have empty
msgstr, so Czech users will see English fallback text for these new settings.Also applies to: 2244-2247
🤖 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 `@src/locales/cs/messages.po` around lines 877 - 880, The Czech translations are missing for the new notification strings—locate the msgid entries (e.g., "Enable Notifications" and the other notification msgid(s) referenced in the diff) in src/locales/cs/messages.po and fill their corresponding msgstr fields with proper Czech translations (ensure both occurrences noted in the review are updated so users don't see English fallbacks); verify translation accuracy and save the .po file so the build picks up the new Czech strings.src/locales/de/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGerman translations are missing for new web push settings.
Line 877 and Line 2244 are untranslated (
msgstr ""), so the new notification UI will render in English forde.Also applies to: 2244-2247
🤖 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 `@src/locales/de/messages.po` around lines 877 - 880, Add German translations for the new web push settings in the de messages.po file by replacing the empty msgstr values for the untranslated msgid entries (including msgid "Enable Notifications" and the other new web-push-related msgid's in that section) with appropriate German text (e.g., "Benachrichtigungen aktivieren" for "Enable Notifications"), ensure the translations are UTF-8 and msgfmt-compatible, and save the updated src/locales/de/messages.po so the UI renders in German.src/locales/es/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpanish catalog still contains English text for the new notification entries.
Line 1 includes untranslated values for the new web push strings, so
esusers will see English in this settings section.🤖 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 `@src/locales/es/messages.mjs` at line 1, The messages export contains English text for new web-push/notification entries (notably the key "DZ6N9L" and the app-slogan key "ObsidianIRC - Bringing IRC to the future") so Spanish users see English; open the messages JSON (export const messages) and replace those English values with the correct Spanish translations (e.g., translate the "DZ6N9L" web push description and the slogan value) and save the updated JSON string so the messages constant returns fully localized Spanish text.src/locales/es/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill missing Spanish translations for new notification strings.
Both new entries have empty
msgstr, so Spanish users will see English text in Settings.🌐 Proposed fix
#: src/lib/settings/definitions/allSettings.ts msgid "Enable Notifications" -msgstr "" +msgstr "Activar notificaciones" #: src/lib/settings/definitions/allSettings.ts msgid "Show desktop notifications for mentions and DMs. On servers that support push (soju.im/webpush) this also wakes this device when the app is closed." -msgstr "" +msgstr "Mostrar notificaciones de escritorio para menciones y mensajes directos. En servidores que admiten push (soju.im/webpush), esto también activa este dispositivo cuando la aplicación está cerrada."Also applies to: 2244-2247
🤖 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 `@src/locales/es/messages.po` around lines 877 - 880, Add Spanish translations for the new notification-related msgid entries so users see localized text: update the msgstr for msgid "Enable Notifications" (and the other empty entries around lines referenced, e.g., the entries at 2244-2247) with appropriate Spanish translations (for example "Activar notificaciones" or context-accurate variants) in src/locales/es/messages.po so both occurrences are no longer empty.src/locales/fi/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing Finnish translations for the new web push settings text.
Both
msgstrvalues are empty, so the UI falls back to English for Finnish users.🌐 Proposed fix
#: src/lib/settings/definitions/allSettings.ts msgid "Enable Notifications" -msgstr "" +msgstr "Ota ilmoitukset käyttöön" #: src/lib/settings/definitions/allSettings.ts msgid "Show desktop notifications for mentions and DMs. On servers that support push (soju.im/webpush) this also wakes this device when the app is closed." -msgstr "" +msgstr "Näytä työpöytäilmoitukset maininnoista ja yksityisviesteistä. Palvelimilla, jotka tukevat push-ilmoituksia (soju.im/webpush), tämä myös herättää laitteen, kun sovellus on suljettu."Also applies to: 2244-2247
🤖 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 `@src/locales/fi/messages.po` around lines 877 - 880, Populate the empty Finnish translation entries for the new web push settings by replacing the blank msgstr values for the msgid "Enable Notifications" (and the related web push settings msgid pair mentioned) with the correct Finnish strings; locate the msgid "Enable Notifications" and the other empty msgid occurrences in src/locales/fi/messages.po and add the appropriate Finnish translations (ensure plural/forms if any match the original msgid usage) so the UI no longer falls back to English.src/locales/fr/messages.po-2244-2246 (1)
2244-2246:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing French translation for notification setting description.
The
msgstrfield is empty for the notification behavior description. This help text explains when notifications will wake the device and should be localized for French users.Consider adding the French translation explaining the notification behavior.
🤖 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 `@src/locales/fr/messages.po` around lines 2244 - 2246, Add a French translation for the notification description msgid used in src/lib/settings/definitions/allSettings.ts by populating the empty msgstr with an accurate localized sentence; the msgid is "Show desktop notifications for mentions and DMs. On servers that support push (soju.im/webpush) this also wakes this device when the app is closed." — update the msgstr to a clear French equivalent that explains desktop notifications for mentions/DMs and that on push-enabled servers (soju.im/webpush) they will wake the device when the app is closed.src/locales/it/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnglish fallback text in Italian locale for "Enable Notifications".
The compiled Italian message bundle contains
"dL7xtl":["Enable Notifications"]which is English text rather than an Italian translation. This suggests the source Italian .po file has an empty or missingmsgstrfor this entry.The notification setting should display in Italian for Italian users, e.g., "Abilita notifiche" or equivalent. Please verify and update the Italian .po source file to include the proper translation before this .mjs file is regenerated.
🤖 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 `@src/locales/it/messages.mjs` at line 1, The Italian bundle contains an English fallback for the key "dL7xtl" in src/locales/it/messages.mjs; open the Italian .po source entry for that message id and add the proper Italian translation (e.g., "Abilita notifiche") into the msgstr for the corresponding msgid, then re-run the translation compile/regeneration step to produce an updated messages.mjs so "dL7xtl" maps to the Italian text instead of "Enable Notifications".src/locales/fr/messages.po-877-879 (1)
877-879:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing French translation for "Enable Notifications" setting.
The
msgstrfield is empty, which will cause this user-facing notification setting to display in English for French users. The PR description claims to add localized strings across 18 locales including French, but this translation is incomplete.Consider adding the French translation, e.g.,
msgstr "Activer les notifications".🤖 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 `@src/locales/fr/messages.po` around lines 877 - 879, The French .po entry for the setting key "Enable Notifications" has an empty msgstr; update the msgstr for the msgid "Enable Notifications" in src/locales/fr/messages.po to the proper French translation (for example, "Activer les notifications") so the UI shows the localized string for French users.src/locales/it/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill missing Italian translations for new notification strings.
Line 879 and Line 2246 have empty
msgstr, so Italian users will see fallback English for these new settings labels/descriptions.Also applies to: 2244-2247
🤖 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 `@src/locales/it/messages.po` around lines 877 - 880, Open src/locales/it/messages.po and provide Italian translations for the missing msgstr entries for the new notification strings—specifically set an appropriate Italian msgstr for msgid "Enable Notifications" and for the other empty msgstr block around lines 2244-2247 (the new notification label/description pair); ensure the translations are accurate, preserve punctuation and placeholders if any, and save the file so those msgid keys no longer fall back to English.src/locales/ja/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNew Japanese catalog entries are still in English.
The generated
messagespayload includes English text for the new notification label/description (dL7xtl,DZ6N9L), so JA users won’t get localized copy for this feature.🤖 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 `@src/locales/ja/messages.mjs` at line 1, Update the Japanese locale JSON so the new keys representing the notifications are translated: replace the English strings for the identifiers "dL7xtl" (currently "Enable Notifications") and "DZ6N9L" (currently the long English description about desktop notifications and wake-on-push) with their proper Japanese translations inside the exported messages JSON string in messages (export const messages=JSON.parse(...)); ensure you update only those two entries' values (preserving any interpolation tokens) so the rest of the JSON stays unchanged.src/locales/ko/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKorean compiled catalog contains untranslated English for new settings text.
The new notification entries (
dL7xtl,DZ6N9L) are still English in the KO catalog, so this feature is not fully localized for Korean users.🤖 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 `@src/locales/ko/messages.mjs` at line 1, The KO catalog still contains English strings for keys "dL7xtl" and "DZ6N9L"; locate the messages export (messages JSON string) and replace the English text for those keys with proper Korean translations following the existing message array format (keep surrounding array wrappers and any placeholders), i.e. update the values for "dL7xtl" and "DZ6N9L" to their Korean equivalents so the compiled catalog is fully localized.src/locales/ja/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate Japanese translations for the new push-notification strings.
Line 879 and Line 2246 are empty
msgstrentries, which causes English fallback for these new settings in the JA locale.Also applies to: 2244-2247
🤖 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 `@src/locales/ja/messages.po` around lines 877 - 880, Populate the empty Japanese translations in src/locales/ja/messages.po for the new push-notification strings so they don't fall back to English; specifically provide a proper msgstr for msgid "Enable Notifications" (and the other push-notification-related msgid entries around the second gap at lines ~2244-2247) by replacing the empty msgstr values with the correct Japanese translations.src/locales/ko/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill missing Korean translations before merge.
Both new entries have empty translations (
msgstr ""), so Korean users will see English fallback text in these notification settings (Line 877 and Line 2244).Also applies to: 2244-2247
🤖 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 `@src/locales/ko/messages.po` around lines 877 - 880, Two msgstr entries in src/locales/ko/messages.po are empty (one for msgid "Enable Notifications" and another around the block at lines ~2244-2247); fill both msgstr values with the correct Korean translations so users don't see English fallbacks—update the msgstr for the "Enable Notifications" msgid and locate the other empty msgstr near line 2244 in the same file and provide its Korean translation as well.src/locales/pl/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNew Polish notification strings are still in English.
The catalog payload still contains English values for the new notification entries (e.g., “Enable Notifications” and the desktop notifications description), so these UI strings won’t be localized for
pl.🤖 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 `@src/locales/pl/messages.mjs` at line 1, The Polish messages catalog contains untranslated English strings—update the entries for the notification keys (e.g., the message id "dL7xtl" currently "Enable Notifications" and the longer desktop-notification description under "DZ6N9L") to their proper Polish translations; locate these keys inside the exported messages JSON in src/locales/pl/messages.mjs and replace the English text values with the correct Polish strings (ensuring you preserve any interpolation placeholders like [\"mentions and DMs\", [\"soju.im/webpush\"]] or similar structure).src/locales/nl/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDutch translations are missing for new notification strings.
msgstris empty for both new strings (Line 877 and Line 2244), which will leave these labels/descriptions untranslated in the Dutch locale.Also applies to: 2244-2247
🤖 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 `@src/locales/nl/messages.po` around lines 877 - 880, The Dutch locale is missing translations for the new settings keys from src/lib/settings/definitions/allSettings.ts — fill the empty msgstr entries for the msgid "Enable Notifications" (and the other new msgid at the second location referenced around lines 2244-2247) with correct Dutch translations (e.g., "Meldingen inschakelen" or the appropriate phrasing), preserving the existing PO file formatting, escaping, and context so the labels/descriptions render in the nl locale.src/locales/pl/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill missing Polish translations for new notification strings.
msgstris empty in both entries (Line 879 and Line 2246), so these labels/descriptions will appear untranslated for Polish users.Also applies to: 2244-2247
🤖 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 `@src/locales/pl/messages.po` around lines 877 - 880, The Polish .po entries for the new notification labels are missing translations—locate the msgid "Enable Notifications" (and the other new notification-related msgid entries in the same .po file) and fill each corresponding msgstr with the correct Polish translation(s); ensure both occurrences of "Enable Notifications" and the related notification strings have appropriate Polish text in their msgstr fields, preserving existing formatting and any surrounding comments or context markers.src/locales/ro/messages.mjs-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRomanian bundle still contains English copy for new notification messages.
The new notification entries in this payload are not localized (e.g.,
"Enable Notifications"and the new desktop/webpush description), causing mixed-language UI inro.🤖 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 `@src/locales/ro/messages.mjs` at line 1, The Romanian messages bundle contains untranslated English strings for the new notification entries; update the JSON payload exported in the messages constant by replacing the English values for the notification keys (e.g., the "Enable Notifications" entry at key "dL7xtl" and the desktop/webpush description at key "DZ6N9L") with proper Romanian translations, preserving any placeholders/URLs/formatting exactly as in the original (for example keep "(soju.im/webpush)" and any HTML-like tags or placeholders intact) so the JSON.parse string remains valid and nothing else is modified.src/locales/pt/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Portuguese translations for the newly added notification entries.
Both new entries have empty
msgstr(Line 879 and Line 2246), which leaves the PT locale with untranslated UI copy.Also applies to: 2244-2247
🤖 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 `@src/locales/pt/messages.po` around lines 877 - 880, Add Portuguese translations for the untranslated msgid entries by filling the empty msgstr for "Enable Notifications" and the other new notification msgid present in the PT messages.po file; locate the entries by searching for the exact msgid strings (e.g., "Enable Notifications" and the second new notification msgid) and provide appropriate Portuguese translations in their corresponding msgstr fields, ensuring proper punctuation and capitalization to match the existing locale style.src/locales/ro/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate Romanian translations for new notification strings.
Both new entries are committed with empty
msgstr, so Romanian users will see source English for these settings.Also applies to: 2244-2247
🤖 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 `@src/locales/ro/messages.po` around lines 877 - 880, Populate the missing Romanian translations by replacing the empty msgstr values for the new notification strings (e.g., msgid "Enable Notifications") with their proper Romanian translations; update the same for the other empty entries referenced (also at the second group of entries) so that users see Romanian text instead of English in settings like Enable Notifications.src/locales/ru/messages.po-877-880 (1)
877-880:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Russian translations for the two newly introduced notification messages.
These entries are currently empty, which leaves English text in the Russian locale for the new notification toggle/description.
Also applies to: 2244-2247
🤖 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 `@src/locales/ru/messages.po` around lines 877 - 880, Fill in Russian translations for the empty PO entries: set msgstr for msgid "Enable Notifications" and for the companion notification description entry in the same locales/ru/messages.po file (the other newly added notification msgid elsewhere in the file); ensure the translations are proper Russian strings replacing the empty msgstr values so the Russian locale no longer falls back to English.src/locales/tr/messages.po-877-879 (1)
877-879:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Turkish translations for new web-push notification labels.
These
msgstrvalues are empty, so users on Turkish locale will get English fallback text in Settings.🌐 Suggested fix
#: src/lib/settings/definitions/allSettings.ts msgid "Enable Notifications" -msgstr "" +msgstr "Bildirimleri Etkinleştir" #: src/lib/settings/definitions/allSettings.ts msgid "Show desktop notifications for mentions and DMs. On servers that support push (soju.im/webpush) this also wakes this device when the app is closed." -msgstr "" +msgstr "Bahsetmeler ve DM'ler için masaüstü bildirimlerini göster. Push destekleyen sunucularda (soju.im/webpush), uygulama kapalıyken bu cihazı da uyandırır."Also applies to: 2244-2246
🤖 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 `@src/locales/tr/messages.po` around lines 877 - 879, The msgstr entries for the new web-push notification labels are empty (e.g., the msgid "Enable Notifications" and the other two notification msgid strings referenced later), so Turkish users see English fallbacks; locate the msgid "Enable Notifications" (and the two related notification msgid entries around the other reported lines) in the .po file and provide correct Turkish translations by filling each corresponding msgstr with the appropriate Turkish text, ensuring you keep msgid unchanged and preserve PO file quoting/escaping conventions.src/locales/sv/messages.po-877-879 (1)
877-879:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill missing Swedish translations for new notification strings.
Both new entries have empty
msgstr, so Swedish users will see English fallback text in the Notifications settings UI.🌐 Suggested fix
#: src/lib/settings/definitions/allSettings.ts msgid "Enable Notifications" -msgstr "" +msgstr "Aktivera aviseringar" #: src/lib/settings/definitions/allSettings.ts msgid "Show desktop notifications for mentions and DMs. On servers that support push (soju.im/webpush) this also wakes this device when the app is closed." -msgstr "" +msgstr "Visa skrivbordsaviseringar för omnämnanden och privata meddelanden. På servrar som stöder push (soju.im/webpush) väcks också den här enheten när appen är stängd."Also applies to: 2244-2246
🤖 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 `@src/locales/sv/messages.po` around lines 877 - 879, Fill the missing Swedish translations in src/locales/sv/messages.po by providing appropriate Swedish text for the empty msgstr entries for the notification-related msgids (e.g., msgid "Enable Notifications" and the other notification msgid occurrences noted in the review); update each msgstr to the correct Swedish translation and keep the surrounding PO entry format intact so the Notifications settings UI shows Swedish text instead of the English fallback.
🧹 Nitpick comments (2)
src/components/ui/UserSettings.tsx (1)
700-703: ⚡ Quick winConvert this to a single-line rationale comment.
Keep this as one line to match the repo’s TS/TSX comment style rule.
As per coding guidelines,
src/**/*.{ts,tsx}: "Comments must explain why, never what ... Keep comments to one line and write in project context, not change context."🤖 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 `@src/components/ui/UserSettings.tsx` around lines 700 - 703, Replace the multi-line rationale comment above the notifications toggle in the UserSettings component with a single-line comment that explains why the toggle exists (e.g., that it manages soju.im/webpush subscriptions and prompts for browser permission) and keep the content in project context; locate the comment near the notifications toggle logic in src/components/ui/UserSettings.tsx (the block referencing "notifications toggle" and "soju.im/webpush") and condense it to one line to match the repo's TS/TSX comment style.src/lib/irc/IRCClient.ts (1)
601-603: ⚡ Quick winKeep the capability rationale as a single-line comment.
This comment block should be collapsed to one line to match the TS/TSX comment convention used in this repo.
As per coding guidelines, `src/**/*.{ts,tsx}`: "Comments must explain why, never what ... Keep comments to one line and write in project context, not change context."♻️ Proposed edit
- // soju.im/webpush: lets us register a browser PushManager - // subscription with the server so DMs/highlights wake the device - // via the platform push service even when the tab is closed. + // Request soju.im/webpush so servers can wake this client for DMs/highlights while the tab is closed.🤖 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 `@src/lib/irc/IRCClient.ts` around lines 601 - 603, Collapse the three-line comment about soju.im/webpush into a single-line comment that explains why the capability exists (e.g., "soju.im/webpush: register browser PushManager so DMs/highlights wake device via platform push service when tab is closed") and replace the multi-line block near the PushManager registration area in IRCClient.ts (around the soju.im/webpush comment) with that single-line comment to conform to the project's one-line TS/TSX comment guideline.
🤖 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 `@src/components/ui/UserSettings.tsx`:
- Around line 706-720: The current code fires registerWebPush/unregisterWebPush
without awaiting and runs them for all servers (which may be disconnected);
change the logic to only operate on connected servers and await the operations
(either sequentially with await inside the for..of or gather promises and await
Promise.all). Specifically, in the wantNotifications branch, after permission
=== "granted", filter servers by a connection flag (e.g., srv.connected or
srv.isConnected) and by srv.vapidKey and capabilities before calling
registerWebPush(srv.id, srv.vapidKey), and await those calls; similarly, when
wantNotifications === false, filter by the same connection flag and capabilities
then await unregisterWebPush(srv.id). Ensure you handle errors from each awaited
call (try/catch or Promise.allSettled) so failures don’t fail silently.
In `@src/locales/en/messages.po`:
- Around line 876-878: The i18n pipeline failed due to untranslated strings
(specifically the msgid "Enable Notifications"); run the extraction and
compilation scripts (npm run i18n:extract && npm run i18n:compile) locally to
regenerate the locale files, verify the msgid "Enable Notifications" is present
and translated in the generated locale files, and commit the updated files so
the i18n catalog check passes.
---
Minor comments:
In `@src/locales/cs/messages.po`:
- Around line 877-880: The Czech translations are missing for the new
notification strings—locate the msgid entries (e.g., "Enable Notifications" and
the other notification msgid(s) referenced in the diff) in
src/locales/cs/messages.po and fill their corresponding msgstr fields with
proper Czech translations (ensure both occurrences noted in the review are
updated so users don't see English fallbacks); verify translation accuracy and
save the .po file so the build picks up the new Czech strings.
In `@src/locales/de/messages.po`:
- Around line 877-880: Add German translations for the new web push settings in
the de messages.po file by replacing the empty msgstr values for the
untranslated msgid entries (including msgid "Enable Notifications" and the other
new web-push-related msgid's in that section) with appropriate German text
(e.g., "Benachrichtigungen aktivieren" for "Enable Notifications"), ensure the
translations are UTF-8 and msgfmt-compatible, and save the updated
src/locales/de/messages.po so the UI renders in German.
In `@src/locales/es/messages.mjs`:
- Line 1: The messages export contains English text for new
web-push/notification entries (notably the key "DZ6N9L" and the app-slogan key
"ObsidianIRC - Bringing IRC to the future") so Spanish users see English; open
the messages JSON (export const messages) and replace those English values with
the correct Spanish translations (e.g., translate the "DZ6N9L" web push
description and the slogan value) and save the updated JSON string so the
messages constant returns fully localized Spanish text.
In `@src/locales/es/messages.po`:
- Around line 877-880: Add Spanish translations for the new notification-related
msgid entries so users see localized text: update the msgstr for msgid "Enable
Notifications" (and the other empty entries around lines referenced, e.g., the
entries at 2244-2247) with appropriate Spanish translations (for example
"Activar notificaciones" or context-accurate variants) in
src/locales/es/messages.po so both occurrences are no longer empty.
In `@src/locales/fi/messages.po`:
- Around line 877-880: Populate the empty Finnish translation entries for the
new web push settings by replacing the blank msgstr values for the msgid "Enable
Notifications" (and the related web push settings msgid pair mentioned) with the
correct Finnish strings; locate the msgid "Enable Notifications" and the other
empty msgid occurrences in src/locales/fi/messages.po and add the appropriate
Finnish translations (ensure plural/forms if any match the original msgid usage)
so the UI no longer falls back to English.
In `@src/locales/fr/messages.po`:
- Around line 2244-2246: Add a French translation for the notification
description msgid used in src/lib/settings/definitions/allSettings.ts by
populating the empty msgstr with an accurate localized sentence; the msgid is
"Show desktop notifications for mentions and DMs. On servers that support push
(soju.im/webpush) this also wakes this device when the app is closed." — update
the msgstr to a clear French equivalent that explains desktop notifications for
mentions/DMs and that on push-enabled servers (soju.im/webpush) they will wake
the device when the app is closed.
- Around line 877-879: The French .po entry for the setting key "Enable
Notifications" has an empty msgstr; update the msgstr for the msgid "Enable
Notifications" in src/locales/fr/messages.po to the proper French translation
(for example, "Activer les notifications") so the UI shows the localized string
for French users.
In `@src/locales/it/messages.mjs`:
- Line 1: The Italian bundle contains an English fallback for the key "dL7xtl"
in src/locales/it/messages.mjs; open the Italian .po source entry for that
message id and add the proper Italian translation (e.g., "Abilita notifiche")
into the msgstr for the corresponding msgid, then re-run the translation
compile/regeneration step to produce an updated messages.mjs so "dL7xtl" maps to
the Italian text instead of "Enable Notifications".
In `@src/locales/it/messages.po`:
- Around line 877-880: Open src/locales/it/messages.po and provide Italian
translations for the missing msgstr entries for the new notification
strings—specifically set an appropriate Italian msgstr for msgid "Enable
Notifications" and for the other empty msgstr block around lines 2244-2247 (the
new notification label/description pair); ensure the translations are accurate,
preserve punctuation and placeholders if any, and save the file so those msgid
keys no longer fall back to English.
In `@src/locales/ja/messages.mjs`:
- Line 1: Update the Japanese locale JSON so the new keys representing the
notifications are translated: replace the English strings for the identifiers
"dL7xtl" (currently "Enable Notifications") and "DZ6N9L" (currently the long
English description about desktop notifications and wake-on-push) with their
proper Japanese translations inside the exported messages JSON string in
messages (export const messages=JSON.parse(...)); ensure you update only those
two entries' values (preserving any interpolation tokens) so the rest of the
JSON stays unchanged.
In `@src/locales/ja/messages.po`:
- Around line 877-880: Populate the empty Japanese translations in
src/locales/ja/messages.po for the new push-notification strings so they don't
fall back to English; specifically provide a proper msgstr for msgid "Enable
Notifications" (and the other push-notification-related msgid entries around the
second gap at lines ~2244-2247) by replacing the empty msgstr values with the
correct Japanese translations.
In `@src/locales/ko/messages.mjs`:
- Line 1: The KO catalog still contains English strings for keys "dL7xtl" and
"DZ6N9L"; locate the messages export (messages JSON string) and replace the
English text for those keys with proper Korean translations following the
existing message array format (keep surrounding array wrappers and any
placeholders), i.e. update the values for "dL7xtl" and "DZ6N9L" to their Korean
equivalents so the compiled catalog is fully localized.
In `@src/locales/ko/messages.po`:
- Around line 877-880: Two msgstr entries in src/locales/ko/messages.po are
empty (one for msgid "Enable Notifications" and another around the block at
lines ~2244-2247); fill both msgstr values with the correct Korean translations
so users don't see English fallbacks—update the msgstr for the "Enable
Notifications" msgid and locate the other empty msgstr near line 2244 in the
same file and provide its Korean translation as well.
In `@src/locales/nl/messages.po`:
- Around line 877-880: The Dutch locale is missing translations for the new
settings keys from src/lib/settings/definitions/allSettings.ts — fill the empty
msgstr entries for the msgid "Enable Notifications" (and the other new msgid at
the second location referenced around lines 2244-2247) with correct Dutch
translations (e.g., "Meldingen inschakelen" or the appropriate phrasing),
preserving the existing PO file formatting, escaping, and context so the
labels/descriptions render in the nl locale.
In `@src/locales/pl/messages.mjs`:
- Line 1: The Polish messages catalog contains untranslated English
strings—update the entries for the notification keys (e.g., the message id
"dL7xtl" currently "Enable Notifications" and the longer desktop-notification
description under "DZ6N9L") to their proper Polish translations; locate these
keys inside the exported messages JSON in src/locales/pl/messages.mjs and
replace the English text values with the correct Polish strings (ensuring you
preserve any interpolation placeholders like [\"mentions and DMs\",
[\"soju.im/webpush\"]] or similar structure).
In `@src/locales/pl/messages.po`:
- Around line 877-880: The Polish .po entries for the new notification labels
are missing translations—locate the msgid "Enable Notifications" (and the other
new notification-related msgid entries in the same .po file) and fill each
corresponding msgstr with the correct Polish translation(s); ensure both
occurrences of "Enable Notifications" and the related notification strings have
appropriate Polish text in their msgstr fields, preserving existing formatting
and any surrounding comments or context markers.
In `@src/locales/pt/messages.po`:
- Around line 877-880: Add Portuguese translations for the untranslated msgid
entries by filling the empty msgstr for "Enable Notifications" and the other new
notification msgid present in the PT messages.po file; locate the entries by
searching for the exact msgid strings (e.g., "Enable Notifications" and the
second new notification msgid) and provide appropriate Portuguese translations
in their corresponding msgstr fields, ensuring proper punctuation and
capitalization to match the existing locale style.
In `@src/locales/ro/messages.mjs`:
- Line 1: The Romanian messages bundle contains untranslated English strings for
the new notification entries; update the JSON payload exported in the messages
constant by replacing the English values for the notification keys (e.g., the
"Enable Notifications" entry at key "dL7xtl" and the desktop/webpush description
at key "DZ6N9L") with proper Romanian translations, preserving any
placeholders/URLs/formatting exactly as in the original (for example keep
"(soju.im/webpush)" and any HTML-like tags or placeholders intact) so the
JSON.parse string remains valid and nothing else is modified.
In `@src/locales/ro/messages.po`:
- Around line 877-880: Populate the missing Romanian translations by replacing
the empty msgstr values for the new notification strings (e.g., msgid "Enable
Notifications") with their proper Romanian translations; update the same for the
other empty entries referenced (also at the second group of entries) so that
users see Romanian text instead of English in settings like Enable
Notifications.
In `@src/locales/ru/messages.po`:
- Around line 877-880: Fill in Russian translations for the empty PO entries:
set msgstr for msgid "Enable Notifications" and for the companion notification
description entry in the same locales/ru/messages.po file (the other newly added
notification msgid elsewhere in the file); ensure the translations are proper
Russian strings replacing the empty msgstr values so the Russian locale no
longer falls back to English.
In `@src/locales/sv/messages.po`:
- Around line 877-879: Fill the missing Swedish translations in
src/locales/sv/messages.po by providing appropriate Swedish text for the empty
msgstr entries for the notification-related msgids (e.g., msgid "Enable
Notifications" and the other notification msgid occurrences noted in the
review); update each msgstr to the correct Swedish translation and keep the
surrounding PO entry format intact so the Notifications settings UI shows
Swedish text instead of the English fallback.
In `@src/locales/tr/messages.po`:
- Around line 877-879: The msgstr entries for the new web-push notification
labels are empty (e.g., the msgid "Enable Notifications" and the other two
notification msgid strings referenced later), so Turkish users see English
fallbacks; locate the msgid "Enable Notifications" (and the two related
notification msgid entries around the other reported lines) in the .po file and
provide correct Turkish translations by filling each corresponding msgstr with
the appropriate Turkish text, ensuring you keep msgid unchanged and preserve PO
file quoting/escaping conventions.
---
Nitpick comments:
In `@src/components/ui/UserSettings.tsx`:
- Around line 700-703: Replace the multi-line rationale comment above the
notifications toggle in the UserSettings component with a single-line comment
that explains why the toggle exists (e.g., that it manages soju.im/webpush
subscriptions and prompts for browser permission) and keep the content in
project context; locate the comment near the notifications toggle logic in
src/components/ui/UserSettings.tsx (the block referencing "notifications toggle"
and "soju.im/webpush") and condense it to one line to match the repo's TS/TSX
comment style.
In `@src/lib/irc/IRCClient.ts`:
- Around line 601-603: Collapse the three-line comment about soju.im/webpush
into a single-line comment that explains why the capability exists (e.g.,
"soju.im/webpush: register browser PushManager so DMs/highlights wake device via
platform push service when tab is closed") and replace the multi-line block near
the PushManager registration area in IRCClient.ts (around the soju.im/webpush
comment) with that single-line comment to conform to the project's one-line
TS/TSX comment guideline.
🪄 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
Run ID: 78fa61cf-c92c-4c57-9843-16eb9e55753c
📒 Files selected for processing (46)
public/sw.jssrc/components/ui/UserSettings.tsxsrc/lib/irc/IRCClient.tssrc/lib/settings/definitions/allSettings.tssrc/lib/webpush.tssrc/locales/cs/messages.mjssrc/locales/cs/messages.posrc/locales/de/messages.mjssrc/locales/de/messages.posrc/locales/en/messages.mjssrc/locales/en/messages.posrc/locales/es/messages.mjssrc/locales/es/messages.posrc/locales/fi/messages.mjssrc/locales/fi/messages.posrc/locales/fr/messages.mjssrc/locales/fr/messages.posrc/locales/it/messages.mjssrc/locales/it/messages.posrc/locales/ja/messages.mjssrc/locales/ja/messages.posrc/locales/ko/messages.mjssrc/locales/ko/messages.posrc/locales/nl/messages.mjssrc/locales/nl/messages.posrc/locales/pl/messages.mjssrc/locales/pl/messages.posrc/locales/pt/messages.mjssrc/locales/pt/messages.posrc/locales/ro/messages.mjssrc/locales/ro/messages.posrc/locales/ru/messages.mjssrc/locales/ru/messages.posrc/locales/sv/messages.mjssrc/locales/sv/messages.posrc/locales/tr/messages.mjssrc/locales/tr/messages.posrc/locales/uk/messages.mjssrc/locales/uk/messages.posrc/locales/zh-TW/messages.mjssrc/locales/zh-TW/messages.posrc/locales/zh/messages.mjssrc/locales/zh/messages.posrc/protocol/isupport.tssrc/store/handlers/connection.tssrc/types/index.ts
Comment on lines
+706
to
+720
| if (wantNotifications) { | ||
| const permission = await requestNotificationPermission(); | ||
| if (permission === "granted") { | ||
| for (const srv of servers) { | ||
| if (srv.vapidKey && srv.capabilities?.includes("soju.im/webpush")) { | ||
| void registerWebPush(srv.id, srv.vapidKey); | ||
| } | ||
| } | ||
| } | ||
| } else if (wantNotifications === false) { | ||
| for (const srv of servers) { | ||
| if (srv.capabilities?.includes("soju.im/webpush")) { | ||
| void unregisterWebPush(srv.id); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle push subscription calls as awaited tasks on connected servers only.
This currently dispatches registration/unregistration as fire-and-forget across all servers, which can fail silently and attempt work against disconnected servers.
🛠️ Proposed fix
+ const pushServers = servers.filter(
+ (srv) =>
+ srv.isConnected && srv.capabilities?.includes("soju.im/webpush"),
+ );
+
if (wantNotifications) {
const permission = await requestNotificationPermission();
if (permission === "granted") {
- for (const srv of servers) {
- if (srv.vapidKey && srv.capabilities?.includes("soju.im/webpush")) {
- void registerWebPush(srv.id, srv.vapidKey);
- }
- }
+ const tasks = pushServers
+ .filter((srv) => !!srv.vapidKey)
+ .map((srv) => registerWebPush(srv.id, srv.vapidKey!));
+ await Promise.allSettled(tasks);
}
} else if (wantNotifications === false) {
- for (const srv of servers) {
- if (srv.capabilities?.includes("soju.im/webpush")) {
- void unregisterWebPush(srv.id);
- }
- }
+ const tasks = pushServers.map((srv) => unregisterWebPush(srv.id));
+ await Promise.allSettled(tasks);
}🤖 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 `@src/components/ui/UserSettings.tsx` around lines 706 - 720, The current code
fires registerWebPush/unregisterWebPush without awaiting and runs them for all
servers (which may be disconnected); change the logic to only operate on
connected servers and await the operations (either sequentially with await
inside the for..of or gather promises and await Promise.all). Specifically, in
the wantNotifications branch, after permission === "granted", filter servers by
a connection flag (e.g., srv.connected or srv.isConnected) and by srv.vapidKey
and capabilities before calling registerWebPush(srv.id, srv.vapidKey), and await
those calls; similarly, when wantNotifications === false, filter by the same
connection flag and capabilities then await unregisterWebPush(srv.id). Ensure
you handle errors from each awaited call (try/catch or Promise.allSettled) so
failures don’t fail silently.
Comment on lines
+876
to
+878
| #: src/lib/settings/definitions/allSettings.ts | ||
| msgid "Enable Notifications" | ||
| msgstr "Enable Notifications" |
Contributor
There was a problem hiding this comment.
Pipeline failure: i18n catalog check failed.
The GitHub Actions i18n check is failing with: "Untranslated strings detected in source." This indicates the i18n extraction and compilation process may not have completed correctly.
Please run:
npm run i18n:extract && npm run i18n:compileand commit any changes to the src/locales/ files to resolve the pipeline failure.
🤖 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 `@src/locales/en/messages.po` around lines 876 - 878, The i18n pipeline failed
due to untranslated strings (specifically the msgid "Enable Notifications"); run
the extraction and compilation scripts (npm run i18n:extract && npm run
i18n:compile) locally to regenerate the locale files, verify the msgid "Enable
Notifications" is present and translated in the generated locale files, and
commit the updated files so the i18n catalog check passes.
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
Client support for the
soju.im/webpushIRCv3 extension — wakes the device for DMs and channel highlights via the platform push service even when the tab is closed.soju.im/webpushcapability and reads theVAPID=ISUPPORT token onto server state (server.vapidKey).src/lib/webpush.ts: subscribes the browserPushManageragainst the server's VAPID key and sendsWEBPUSH REGISTER <endpoint> p256dh=…;auth=…; resubscribes if the server's key rotated;unregisterWebPushtears it down + sendsWEBPUSH UNREGISTER.public/sw.js):pushhandler parses the one-IRC-line payload and shows a notification —nick in #channel · <network>for highlights,nick · <network>for DMs (network from the manifest). Suppressed when a client window is focused (the in-app notifier covers that). Plus anotificationclickthat focuses/opens the app.enableNotifications, which previously existed in state but had no UI control). Turning it on is the user-gesture opt-in: prompts for permission and registers push on every connected push-capable server; turning it off unregisters.readyfor servers that already have permission granted.Server side (obbyircd
webpushmodule + hosted-backend VAPID/encrypt-send) lives in the ObbyIRCd / hosted-backend repos; this PR is the client half.Test plan
WEBPUSH REGISTERis sent and a subscription row is stored.WEBPUSH UNREGISTER.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes