feat: Click-to-Call telephony + internal video chat session sharing#3370
Open
jeanfbrito wants to merge 73 commits into
Open
feat: Click-to-Call telephony + internal video chat session sharing#3370jeanfbrito wants to merge 73 commits into
jeanfbrito wants to merge 73 commits into
Conversation
Register Rocket.Chat as OS handler for callto: and tel: URL schemes on Windows, macOS, and Linux. When a telephony link is clicked in any app, RC launches or focuses and dispatches a typed IPC event to the server webview with the parsed phone number. - Register callto/tel schemes in electron-builder.json (all platforms) - Add parseTelephonyLink() with number normalization and callto:// support - Add performTelephonyCall() with multi-server dialog + remember choice - Expose onTelephonyCallRequested callback on RocketChatDesktop API - Persist telephonyPreferredServer via selectPersistableValues - IPC listener registered before onReady to avoid cold-start race
Tests cover parseTelephonyLink (tel:/callto: protocols, number normalization, callto:// double-slash format, extension syntax, edge cases) and performTelephonyCall (0/1/2+ servers, preferred server persistence, dialog remember checkbox).
…duplicate IPC listener - Pass getRootWindow() as first argument to dialog.showMessageBox so the server selection prompt appears as a modal sheet attached to the main window (consistent with all other dialogs in the codebase) - Add idempotency guard to listenToTelephonyRequests to prevent duplicate IPC handler registration during hot-reload dev cycles
Add GitNexus section with impact analysis, query, and context tools. Gitignore .gitnexus index directory.
Dispatch WEBVIEW_GIT_COMMIT_HASH_CHANGED from server info response. Match supportedVersions exceptions using sha:<hash> prefix against the server's git commit hash for per-build version overrides.
Add TelephonyServer component to Settings > General tab with a Select dropdown to choose which server handles tel:/callto: links. Hidden when only one server exists. "Auto (ask each time)" option clears the preference and reverts to dialog behavior.
electron-builder v26 rejects MimeType as a direct child of linux.desktop — only desktopActions and entry are valid properties. Move it inside desktop.entry where it belongs.
Replace hardcoded English strings in the telephony dialog with i18n t() calls and add telephonySelectServer translation keys to all 22 locale files.
…selection Replace dialog.showMessageBox with an in-app modal that shows server favicons, names and hostnames — matching the sidebar appearance. Scales to many servers via a scrollable list and includes a "remember this choice" checkbox. Also hardens the telephony flow: - Mutex prevents concurrent tel: links from opening duplicate modals - 120s timeout on modal promise prevents hanging if renderer crashes - 10s timeout on webContents polling prevents infinite loop if server is removed between selection and view creation
Drop Margins wrapper from the modal and the Tile container from rows. Title→message margin x8→x4, message→list x16→x12, rows now use paddingBlock x6 / paddingInline x8 instead of Tile padding x12.
…ver modal Adds 20 tests across three new spec files covering the telephony deep-link runtime path that was previously only validated by deepLinks/main.spec.ts. - src/telephony/renderer/preload.spec.ts (6 tests): IPC bridge state machine — listenToTelephonyRequests guard, pendingPayload buffer/replay, callback replacement, ipcRenderer.on registration. - src/ui/components/SettingsView/features/TelephonyServer.spec.tsx (8 tests): Settings dropdown — hide when servers.length <= 1, option generation (auto + per-server), value binding to telephonyPreferredServer, dispatch of TELEPHONY_PREFERRED_SERVER_SET (null for auto, URL string otherwise), hostname fallback when server title missing. - src/ui/components/TelephonyServerSelectModal/index.spec.tsx (6 tests): Modal flow — visibility gating on dialogs.telephonyServerSelect.isOpen, ServerItem rendering per server, dispatch payload shape on click with rememberChoice on/off, close dispatch with null payload, rememberChoice reset after close. Adds @testing-library/react, @testing-library/jest-dom, and @testing-library/dom (peer) as devDependencies. Fuselage Select and Dialog are mocked at module level since they rely on React-Aria and native <dialog>.showModal() respectively, which don't drive cleanly in @kayahr/jest-electron-runner's renderer environment. Spec paths follow the existing renderer testMatch convention: src/<module>/<subdir>/<file>.spec.tsx — a flat src/telephony/preload.spec.ts would be silently dropped by jest's testMatch globs.
tel:%2B15551234 left %2B encoded, producing phoneNumber '%2B15551234' instead of '+15551234'. decodeURIComponent runs before strip pass; malformed escapes return null (treated same as other invalid input).
…tive Git commit hashes are conventionally case-insensitive. SHA-bb83777 should match same as sha-bb83777.
- TelephonyServer: extract hostname via safeHostname helper to prevent settings page crash on malformed server URLs (new URL() throws). - TelephonyServerSelectModal: associate 'Remember this choice' label with checkbox via htmlFor/id for assistive tech. - ServerItem: render Tile as native button (is='button' type='button') so keyboard users get Tab focus and Enter/Space activation.
* feat(telephony): add global shortcut to dial clipboard number * fix(telephony): harden global shortcut handling * refactor(telephony): share dialpad opener * test(telephony): stabilize shortcut notification click
…at/Rocket.Chat.Electron into feat/telephony-deeplink
Updated the AvailableBrowsers, TelephonyGlobalShortcut, TelephonyServer, and ThemeAppearance components to include a marginBlock of 'x16' on the Field components for improved spacing and layout consistency.
…at/Rocket.Chat.Electron into feat/telephony-deeplink
Add `isTelephonyEnabled` setting (default off) to gate the telephony feature end-to-end: - New persisted `isTelephonyEnabled` boolean with action, reducer, and selector entry; surfaces as a master toggle in Settings > General. - `TelephonyServer` and `TelephonyGlobalShortcut` controls remain visible but disabled while the master toggle is off. - Global shortcut config selector returns the disabled config when the master toggle is off, so the existing watcher auto-unregisters any active accelerator on toggle-off. - `tel:`/`callto:` deep links short-circuit when the master toggle is off. - OS-level protocol registration for `tel`/`callto` moves out of the unconditional startup loop into a new reactive `setupTelephonyProtocolHandlers`, which calls `setAsDefaultProtocolClient` / `removeAsDefaultProtocolClient` in response to toggle changes. `rocketchat:` continues to register at startup unchanged. The macOS `Info.plist` and Linux `.desktop` files declared by electron-builder will still list the app as a candidate handler for `tel`/`callto`, but it will never be set as default unless the user opts in at runtime.
DMV-1 calls for a first-run prompt warning the user that Teams, Zoom,
or Skype may already own the tel:/callto: handler and that they must
confirm Rocket.Chat as default in OS settings themselves. Windows 10/11
hash-protects UserChoice so `setAsDefaultProtocolClient` only registers
the app as a candidate; without the prompt, users have no way to know
the OS silently kept the prior default.
Trigger every off->on transition of the master `isTelephonyEnabled`
toggle (not literal first run — the toggle is opt-in and is the natural
moment of user intent). Seed the transition tracker via a synchronous
`select` before subscribing, so returning users who reopen the app
with telephony already enabled are not re-prompted.
- New `TelephonyDefaultHandlerPromptModal` Fuselage modal (title, two
body paragraphs, "Open System Settings" + "Got it" buttons), mounted
in Shell next to the existing telephony modal.
- Three new void actions (`_OPEN`, `_CLOSE`, `_OPEN_SETTINGS_CLICKED`)
and a `telephonyDefaultHandlerPrompt` sub-reducer in `dialogs.ts`.
- Main-process `setupTelephonyDefaultHandlerPrompt` watches the master
toggle and dispatches OPEN on each off->on flip. Listens for the
settings-button click and routes per platform:
- Windows: `shell.openExternal('ms-settings:defaultapps')`
- macOS: opens FaceTime preferences (where tel: default lives)
- Linux: spawns `gnome-control-center default-apps` or
`kcmshell5/6 componentchooser` based on `XDG_CURRENT_DESKTOP`;
unknown DEs log a tip and rely on the modal's verbal instructions.
- i18n keys under `telephony.defaultHandlerPrompt`.
- 13 new tests covering transition detection, idempotency, teardown,
and each platform branch.
The renderer Jest project's `testMatch` requires at least one subdirectory between `src/<module>/` and the spec, so `src/app/PersistableValues.spec.ts` was silently skipped. Moved the file under `src/app/__tests__/`, fixed the relative import, and expanded the migration assertion to cover `isTelephonyEnabled`. Documented the constraint in `CLAUDE.md` so future renderer specs are placed correctly.
…s UI Split telephony, video-call and screen-capture controls out of the General settings tab into a dedicated Voice & Video tab so the telephony stack has room to grow without crowding the rest of the settings. Diagnostics UI now collapses by default behind an Accordion with a status Tag summary in the title (pass/issues/warnings/checking). Per-check rows use Tag variants for status (primary/danger/warning) with flexShrink guards so the badge does not collapse to an ellipsis on narrow widths. Check labels were rewritten in user-facing terms (Click-to-call, Click-to-conference) and platform names are humanized (darwin -> macOS). Long handler paths are stripped from the inline details (full path still ships in the copy-diagnostics JSON) so the row layout stays clean.
…stration Adds telephony/get-diagnostics IPC channel and runtime module, wires TelephonyDiagnostics into the settings panel, and registers Rocket.Chat in the Windows RegisteredApplications/Capabilities surface so Default Apps exposes it for tel and callto.
…skip darwin
Detects per-user vs per-machine installs from process.execPath and opens
ms-settings:defaultapps?registeredApp{User,Machine}=Rocket.Chat so the
Default Apps page lands on the app-specific surface. macOS has no
equivalent settings pane, so the open-settings handler is now a no-op
and the modal hides body2 plus the Open Settings button on darwin.
…ntry If a watcher synchronously dispatches an action that re-triggers the same subscription, the recursive call previously saw a stale prev value. Capture prev into a local before assigning curr to it, so the recursive watcher invocation observes the freshly applied state.
The TelephonyServerSelectModal kept `rememberChoice` local state alive across close/reopen cycles when the modal closed via a Redux state update (e.g., external dispatch) rather than the local close handlers, leaking the prior `true` value into the next dispatched payload. Add a useEffect keyed on `isVisible` that resets `rememberChoice` when the modal becomes hidden. The existing in-handler resets stay in place for stores that do not propagate state changes (notably the stub reducers in unit tests). Fixes the failing `rememberChoice resets when the dialog is closed by state update` spec that blocked all 6 PR #3325 CI jobs.
…link # Conflicts: # .gitignore # src/app/main/app.ts # src/app/selectors.ts # src/deepLinks/main.spec.ts # src/deepLinks/main.ts # src/i18n/en.i18n.json # src/servers/supportedVersions/main.ts # src/store/rootReducer.ts # src/telephony/actions.ts # src/telephony/reducers.ts # src/ui/components/SettingsView/GeneralTab.tsx # src/ui/components/SettingsView/features/TelephonyServer.tsx
A deeplink targeting a workspace without VoIP never registers an onTelephonyCallRequested callback, so the buffered pendingPayload would sit in the frame indefinitely and could surface a stale number on a later unrelated remount. Drop the payload silently after a 120s TTL; the timer is cleared on flush so a consumed payload never re-fires.
extractClipboardPhoneNumber returned the raw trimmed clipboard text, so pasted content like "Call (800) 555-0199 now" reached the dial pad with surrounding words and formatting intact. Strip everything that is not a dialable character ([^\d+*#]) and keep + only as a leading prefix; still require at least 3 digits.
Internal conferences opened via openInternalVideoChatWindow ran in a webview hardcoded to the isolated `persist:jitsi-session` partition, so they did not share cookies or localStorage with the server webview they were opened from. Electron scopes cookies/localStorage to the (partition, origin) pair, so a same-origin conference loaded unauthenticated (the login token lives in localStorage under the server origin). Load the call webview in the originating server's partition (`persist:<serverUrl>`, resolved from the caller webContents) so the call shares the main webview's session. Because session-level handlers are per-session, sharing the session means the call window's handlers would otherwise clobber the main webview's: - Screen sharing now uses a single unified display-media handler that routes by originating frame (in-call requests open the picker in the call window; main-app requests fall back to the server-view picker), and the plain server-view handler is restored when the call closes. - The teardown permission-handler reset is skipped on a shared session so it can't disable permissions on the live main webview. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the POC session-share fix correct for all users. - Thread per-window state (ActiveCall record) instead of reading the mutable pendingVideoCallPartition global at lifecycle points that span async ticks; narrow the global to renderer-handshake reads only. - Restore the server-view display-media AND permission handlers from every teardown path (closed, cleanup, render-process-gone) so a shared session can't leave the main webview's screen sharing or permission prompts disabled after a call ends. Restore is idempotent. - Fix isSharedSession staleness: snapshot the per-window record at webview attach and evaluate routing at display-media request time. - Guard terminal state null-out with identity (=== capturedCall) so a stale prior-window teardown can't wipe a newer call's state. - Serialize the open-window handler via a promise-chain mutex to close a rapid-double-open race that orphaned a window with leaked handlers. - Retain persist:jitsi-session as the unresolved-server fallback so such calls keep stable isolated storage. Add ipc.main.spec.ts covering restore (shared/fallback/destroyed/ idempotent), partition assignment, render-process-gone, the null-out guard, and the serialization race.
Brings the internal-video-chat session-sharing fix (PR #3359) into the telephony deep-link feature branch, plus the master advancement #3359 carried (telephony branch was 14 commits behind master). Conflict resolution (integrate both sides, never pick-a-side): - PersistableValues.ts: chained the migration versions — kept 4.14.0 (telephony: isTelephonyEnabled, telephonyPreferredServer, telephonyGlobalShortcutConfig) and layered master's 4.15.0 (e2ePdfPreviewSizeLimit) on top so the version chain stays sequential. - GeneralTab.tsx: adopted master's new layout + settings entries (ScreenCaptureFallback, InternalVideoChatWindow, VideoCallWindowPersistence, E2ePdfPreviewSizeLimit, ClearPermittedScreenCaptureServers) while keeping <TelephonyServer />. - AvailableBrowsers / ThemeAppearance / TelephonyServer: adopted master's SettingField wrapper convention. TelephonyServer preserves telephony behavior master lacked (disabled={!isTelephonyEnabled}, safeHostname). - yarn.lock: union of both dependency additions, normalized by install. Fuselage 0.78 migration (master upgrade broke the telephony settings): - Accordion.Item -> standalone AccordionItem in VoiceVideoTab.tsx and TelephonyDiagnostics.tsx. Verified: tsc --noEmit clean, yarn lint pass, videoCallWindow ipc.main.spec.ts 11/11 passing.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
WalkthroughThis PR adds telephony settings, diagnostics, deep-link routing, Windows default-app registration, QA tooling, and localization. It also updates video-call session partitioning and shared-session teardown behavior. ChangesTelephony feature, QA assets, and platform registration
Windows associations, installers, and deployment docs
Video-call shared-session partition lifecycle
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/videoCallWindow/ipc.ts (1)
225-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard all per-call globals against stale teardown.
Lines 607-612 only protect
activeCall; a stale first window’sclosedhandler can still clearvideoCallWindow,videoCallCredentials, andvideoCallProviderNameafter a newer call is active. Capture the owned window and clear globals only if they still belong to that captured call/window.Proposed direction
videoCallWindow = new BrowserWindow({ width, height, @@ // Capture per-window so a later call opening (which resets the module // state) can't change which server's handlers this window restores. + const capturedWindow = videoCallWindow; const capturedCall = activeCall; @@ - // Clear credentials and provider on close - videoCallCredentials = null; - videoCallProviderName = null; + if (activeCall === capturedCall) { + videoCallCredentials = null; + videoCallProviderName = null; + } @@ - videoCallWindow = null; - isVideoCallWindowDestroying = false; + if (videoCallWindow === capturedWindow) { + videoCallWindow = null; + isVideoCallWindowDestroying = false; + } videoCallWindowDestructionCount++; // Only clear `activeCall` if it still belongs to this window — a // stale prior-window teardown must not wipe a freshly-set newer call. if (activeCall === capturedCall) activeCall = null;Also applies to: 588-613
🤖 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/videoCallWindow/ipc.ts` around lines 225 - 235, The cleanup logic only guards `activeCall` against stale teardown callbacks but leaves `videoCallWindow`, `videoCallCredentials`, and `videoCallProviderName` unprotected. A stale first window's closed handler can still clear these globals after a newer call is active. Capture the current `videoCallWindow` reference at the start of the teardown (similar to how `capturedCall` is captured for `activeCall`), and then add conditional checks before clearing `videoCallWindow`, `videoCallCredentials`, and `videoCallProviderName` in the setTimeout block to ensure they are only cleared if they still belong to the captured window from this teardown. This prevents a stale first window's cleanup from interfering with a newer active call's window and credentials.
🟡 Minor comments (10)
docs/windows-default-app-associations.md-25-27 (1)
25-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced blocks (MD040).
Both fenced blocks are missing a language tag, which triggers markdownlint warnings.
Suggested fix
-``` +```text HKLM\SOFTWARE\Policies\Microsoft\Windows\System!DefaultAssociationsConfiguration-
+text
%ProgramFiles%\Rocket.Chat\resources\RocketChatDefaultAppAssociations.xmlAlso applies to: 38-40
🤖 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 `@docs/windows-default-app-associations.md` around lines 25 - 27, The fenced code blocks in the markdown file are missing language identifiers which triggers markdownlint warnings (MD040). Add the language tag `text` after the opening triple backticks for both code blocks - the first block containing the registry path (HKLM\SOFTWARE\Policies\Microsoft\Windows\System!DefaultAssociationsConfiguration) and the second block containing the file path (%ProgramFiles%\Rocket.Chat\resources\RocketChatDefaultAppAssociations.xml). Change each opening ``` to ```text to satisfy the markdownlint requirement.Source: Linters/SAST tools
src/i18n/de-DE.i18n.json-162-162 (1)
162-162: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove leading whitespace in dialog button label.
Line 162 has
"cancel": " Abbrechen"(leading space), which can render inconsistent button text.Suggested fix
- "cancel": " Abbrechen", + "cancel": "Abbrechen",🤖 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/i18n/de-DE.i18n.json` at line 162, The "cancel" key in the German translation file (de-DE.i18n.json) contains a leading whitespace character before "Abbrechen" which causes rendering inconsistencies in dialog button labels. Remove the leading space from the "cancel" value so it reads exactly "Abbrechen" without any preceding whitespace.qa/telephony-deeplink/README.md-13-23 (1)
13-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the desktop-QA workflow entrypoint reference.
Please add a short instruction here to start Desktop PR/branch QA from
skills/desktop-qa-flows/SKILL.mdbefore running this pack.As per coding guidelines, Desktop PR/branch/release-candidate QA passes should use
skills/desktop-qa-flows/SKILL.mdas the workflow entrypoint.🤖 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 `@qa/telephony-deeplink/README.md` around lines 13 - 23, The Quick Start section in the README.md file is missing a reference to the desktop-QA workflow entrypoint. Add a new instruction at the beginning of the Quick Start steps that directs users to start Desktop PR/branch QA by consulting skills/desktop-qa-flows/SKILL.md as the workflow entrypoint, before proceeding with the remaining steps for the telephony-deeplink testing pack.Source: Coding guidelines
src/ui/components/SettingsView/features/TelephonyServer.spec.tsx-52-198 (1)
52-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest coverage gap: missing assertion for disabled state when
isTelephonyEnabledis false.The test suite covers the core server-selection behavior, but the new
disabled={!isTelephonyEnabled}binding on the<Select>component (added in the implementation file) is not tested. Add a test case to verify that the Select is disabled whenisTelephonyEnabledisfalse.✅ Suggested test case
it('disables Select when isTelephonyEnabled is false', () => { const store = makeStore({ servers: [ { url: 'https://chat.alpha.com', title: 'Alpha' }, { url: 'https://chat.beta.com', title: 'Beta' }, ], telephonyPreferredServer: null, isTelephonyEnabled: false, }); render( <Provider store={store}> <TelephonyServer /> </Provider> ); const select = screen.getByTestId<HTMLSelectElement>('telephony-select'); expect(select).toBeDisabled(); });Note: Verify that
PartialStatetype definition in the test includesisTelephonyEnabledfield, or extend it to include this state slice.🤖 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/ui/components/SettingsView/features/TelephonyServer.spec.tsx` around lines 52 - 198, Add a new test case within the TelephonyServer test suite after the existing test cases to verify that the Select component is disabled when isTelephonyEnabled is false. The test should create a store using makeStore with isTelephonyEnabled set to false, render the TelephonyServer component wrapped in a Provider, retrieve the select element using screen.getByTestId with identifier 'telephony-select', and assert that the select element has the disabled attribute using toBeDisabled matcher. Ensure the makeStore function call includes the isTelephonyEnabled property and verify that the PartialState type definition includes this field or extend it accordingly.Source: Learnings
src/telephony/diagnostics.ts-361-367 (1)
361-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid broad
'rocket'matching in Linux handler detection.At Line 361, any desktop ID containing
"rocket"is treated as Rocket.Chat, so unrelated handlers can incorrectly pass and skipExec=verification.💡 Suggested fix
- const desktopIdLooksRocketChat = trimmed.toLowerCase().includes('rocket'); + const desktopIdLooksRocketChat = /rocket[.\-]?chat/i.test(trimmed);Consider adding a regression test for a non-Rocket.Chat desktop ID containing
"rocket"to lock this 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/telephony/diagnostics.ts` around lines 361 - 367, The condition at line 361 using trimmed.toLowerCase().includes('rocket') is too broad and will match any desktop ID containing the word "rocket", causing unrelated handlers to incorrectly pass validation and skip the commandLaunchesRocketChat verification. Replace the broad substring check in the desktopIdLooksRocketChat assignment with a more specific pattern that uniquely identifies actual Rocket.Chat desktop files, such as checking for the exact string "rocketchat" or matching against the specific filename pattern that Rocket.Chat uses. Additionally, add a regression test case that verifies a non-Rocket.Chat desktop ID containing the word "rocket" (for example, "my-rocket-launcher") correctly fails validation instead of passing.src/telephony/main.spec.ts-685-697 (1)
685-697: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove Linux desktop env cleanup into
afterEachfor deterministic isolation.
XDG_CURRENT_DESKTOPis deleted inside individual tests; if a test exits early, the env var can leak into subsequent cases.Proposed fix
afterEach(() => { teardownTelephonyDefaultHandlerPrompt(); + delete process.env.XDG_CURRENT_DESKTOP; Object.defineProperty(process, 'platform', { value: originalPlatform, writable: true, configurable: true, });Also applies to: 871-930
🤖 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/telephony/main.spec.ts` around lines 685 - 697, The XDG_CURRENT_DESKTOP environment variable is being deleted inside individual tests, which can leak into subsequent test cases if a test exits early. Move the cleanup of XDG_CURRENT_DESKTOP into the afterEach block where process.platform and process.execPath are already being restored. Store the original value of process.env.XDG_CURRENT_DESKTOP before tests run and restore it in afterEach using Object.defineProperty or direct assignment, ensuring deterministic cleanup that executes regardless of whether individual tests complete or exit prematurely.src/i18n/pt-BR.i18n.json-162-162 (1)
162-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove accidental leading whitespace from the cancel label.
"cancel": " Cancelar"adds an extra leading space in the dialog action text.Proposed fix
- "cancel": " Cancelar", + "cancel": "Cancelar",🤖 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/i18n/pt-BR.i18n.json` at line 162, The "cancel" key in the Portuguese-Brazilian translation has an unintended leading whitespace in its value. Remove the extra space before "Cancelar" so the value reads "Cancelar" instead of " Cancelar" to ensure the dialog action text displays correctly without the extra space.src/ui/components/SettingsView/features/TelephonyDiagnostics.tsx-86-90 (1)
86-90: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard clipboard access before writing diagnostics JSON.
Lines 86-90 assume
navigator.clipboard.writeTextalways exists/succeeds. In restricted contexts this can throw/reject and break the interaction path.As per coding guidelines, "Use optional chaining with fallbacks for platform-specific APIs:
process.getuid?.() ?? 1000. Only mock when defensive coding isn't possible."Suggested fix
- const handleCopy = useCallback(() => { - if (!diagnostics) return; - void navigator.clipboard.writeText(JSON.stringify(diagnostics, null, 2)); - setCopied(true); - setTimeout(() => setCopied(false), 2000); - }, [diagnostics]); + const handleCopy = useCallback(async () => { + if (!diagnostics || !navigator.clipboard?.writeText) { + return; + } + try { + await navigator.clipboard.writeText(JSON.stringify(diagnostics, null, 2)); + setCopied(true); + setTimeout(() => setCopied(false), 2000); + } catch { + // no-op: keep UI stable when clipboard is unavailable/denied + } + }, [diagnostics]);🤖 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/ui/components/SettingsView/features/TelephonyDiagnostics.tsx` around lines 86 - 90, The handleCopy function assumes navigator.clipboard.writeText always exists and succeeds without guarding against restricted contexts where it may be unavailable or throw errors. Add optional chaining to safely check if navigator.clipboard exists before accessing writeText, use a try-catch or .catch() to handle potential Promise rejections from writeText, and provide a fallback mechanism (such as using the deprecated document.execCommand approach or notifying the user) when the clipboard API is unavailable or fails.Source: Coding guidelines
src/i18n/zh.i18n.json-35-35 (1)
35-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the zh translation key with the key used by the component.
Line 35 defines
reservedAccelerator, but the UI readssettings.options.telephonyShortcut.reservedByApp. This makes the reserved-shortcut error fall back to a key string in zh.💡 Suggested fix
- "reservedAccelerator": "{{accelerator}} 已被 Rocket.Chat 或您的操作系统保留。" + "reservedByApp": "{{accelerator}} 已被 Rocket.Chat 或您的操作系统保留。"🤖 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/i18n/zh.i18n.json` at line 35, The translation key `reservedAccelerator` in the zh.i18n.json file does not match the key that the component expects, which is `settings.options.telephonyShortcut.reservedByApp`. Update the key structure in the zh.i18n.json file to use the correct nested key path `settings.options.telephonyShortcut.reservedByApp` instead of `reservedAccelerator`, while keeping the Chinese translation value the same. This ensures the component can properly find and display the translated error message instead of falling back to the key string.src/ui/components/SettingsView/features/TelephonyGlobalShortcut.tsx-209-214 (1)
209-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly show registration errors for the currently configured shortcut.
Right now any stale
telephonyGlobalShortcutRegistrationStatus.erroris rendered, even if the user has cleared or changed the shortcut. Scope this to the active config to avoid misleading error state.💡 Suggested fix
- {telephonyGlobalShortcutRegistrationStatus.error && ( + {telephonyGlobalShortcutConfig.enabled && + telephonyGlobalShortcutRegistrationStatus.accelerator === + telephonyGlobalShortcutConfig.accelerator && + telephonyGlobalShortcutRegistrationStatus.error && ( <FieldRow> <FieldHint color='danger'> {telephonyGlobalShortcutRegistrationStatus.error} </FieldHint> </FieldRow> )}🤖 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/ui/components/SettingsView/features/TelephonyGlobalShortcut.tsx` around lines 209 - 214, The FieldRow with FieldHint component that renders telephonyGlobalShortcutRegistrationStatus.error is displaying stale errors even after the shortcut has been changed or cleared by the user. Add a conditional check before rendering the error to ensure it only displays when the error corresponds to the currently active or configured shortcut, rather than showing any residual error from a previous configuration.
🧹 Nitpick comments (4)
src/videoCallWindow/main/ipc.main.spec.ts (1)
512-542: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExtend this race test to assert the fresh window still owns lifecycle IPC.
This fires the first window’s stale
closedhandler after the second open, but only provesactiveCallsurvived. Add an assertion throughvideo-call-window/close-requestedorvideo-call-window/open-screen-pickerso the test catches stale teardown clearing the freshvideoCallWindowpointer too.Example assertion
fire(firstWindow.listeners, 'closed'); await new Promise((r) => realSetTimeout(r, 80)); + const closeRequested = handleRegistry.get( + 'video-call-window/close-requested' + ); + expect(closeRequested).toBeDefined(); + await expect(closeRequested!()).resolves.toEqual({ success: true }); + // activeCall must still be B: firing the SECOND window's restore resolves // server B (not A, not undefined).🤖 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/videoCallWindow/main/ipc.main.spec.ts` around lines 512 - 542, The test currently only verifies that activeCall survived the stale teardown but does not verify that the fresh window still owns the lifecycle IPC. After the existing assertions that check the second window's restore behavior, add an additional assertion using either the video-call-window/close-requested or video-call-window/open-screen-picker IPC handler to confirm that the fresh secondWindow still owns the lifecycle, which would catch if stale teardown incorrectly cleared the videoCallWindow pointer.src/app/__tests__/PersistableValues.spec.ts (1)
4-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case that preserves an existing shortcut config.
This test currently won’t catch overwrite regressions on
telephonyGlobalShortcutConfig(orisTelephonyEnabled=true) during>=4.14.0migration.✅ Suggested test addition
describe('PersistableValues migrations', () => { it('adds telephony shortcut config without losing a persisted telephony server', () => { @@ }); + + it('keeps existing telephony settings when already present', () => { + const before = { + isTelephonyEnabled: true, + telephonyPreferredServer: 'https://chat.example.com', + telephonyGlobalShortcutConfig: { + enabled: true, + accelerator: 'CommandOrControl+Shift+D', + }, + } as unknown as Parameters<(typeof migrations)['>=4.14.0']>[0]; + + expect(migrations['>=4.14.0'](before)).toEqual({ + isTelephonyEnabled: true, + telephonyPreferredServer: 'https://chat.example.com', + telephonyGlobalShortcutConfig: { + enabled: true, + accelerator: 'CommandOrControl+Shift+D', + }, + }); + }); });🤖 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/app/__tests__/PersistableValues.spec.ts` around lines 4 - 17, The current test for the >=4.14.0 migration only validates the case where telephonyGlobalShortcutConfig and isTelephonyEnabled don't exist in the input, which means it won't catch overwrite regressions. Add another test case using it() that creates a before object containing pre-existing telephonyGlobalShortcutConfig with custom values and/or isTelephonyEnabled set to true, then verify that calling migrations['>=4.14.0'] preserves those existing values rather than overwriting them.src/telephony/__tests__/dialpad.spec.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this to a main-process spec filename.
This suite targets main-process logic, so
dialpad.main.spec.tsshould be used to match the repository naming convention.As per coding guidelines, main-process specs use
*.main.spec.tsfile naming.🤖 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/telephony/__tests__/dialpad.spec.ts` at line 1, Rename the test file from dialpad.spec.ts to dialpad.main.spec.ts to align with the repository naming convention for main-process test suites. Since this file tests main-process logic (as evidenced by the import from deepLinks/actions), it should follow the *.main.spec.ts naming pattern used throughout the codebase for main-process specifications.Source: Coding guidelines
src/telephony/__tests__/msiInjection.spec.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
*.main.spec.tsnaming for this suite.This test covers installer/main-process behavior and should be renamed to
msiInjection.main.spec.tsfor convention consistency.As per coding guidelines, main-process specs use
*.main.spec.tsfile naming.🤖 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/telephony/__tests__/msiInjection.spec.ts` at line 1, Rename the test file from msiInjection.spec.ts to msiInjection.main.spec.ts to follow the naming convention for test suites that cover main-process behavior. This aligns with the coding guidelines that require main-process specs to use the .main.spec.ts suffix for consistency.Source: Coding guidelines
🤖 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 `@build/msiProjectCreated.js`:
- Around line 206-242: The `WriteTelephonyCapabilities` custom action uses `On
Error Resume Next` which suppresses errors, but then never checks `Err.Number`
after any of the `shell.RegWrite` operations. Add error checking logic after the
registry write calls to detect and handle failures immediately. After each
critical `RegWrite` call (or after each batch of related writes), check if
`Err.Number` is non-zero and raise an error with details about which registry
operation failed if one occurs. This ensures that if any telephony capability
registration fails, the action fails fast rather than silently continuing with
an incomplete configuration.
In `@qa/scripts/export-qase-csv.mjs`:
- Around line 62-65: The frontmatter parsing and line comparison logic is not
handling Windows CRLF line endings properly. The regex pattern for YAML
frontmatter parsing and the string comparisons in the extractSection function
(such as comparing lines with `## ${heading}`) will fail or produce incorrect
results when the content contains CRLF line endings because splitting by '\n'
alone leaves '\r' characters. Normalize the line endings by replacing all CRLF
sequences with LF before processing the content, or update the regex pattern and
string comparisons to account for optional carriage return characters at the end
of lines to ensure cross-platform compatibility.
- Line 7: The script export-qase-csv.mjs imports the yaml package with the
statement import YAML from 'yaml' on line 7, but this package is not declared in
the project's package.json file as either a dependency or devDependency. Add the
yaml package to the project's package.json dependencies or devDependencies
section to resolve the module resolution failure at runtime and comply with the
QA script guidelines of using only Node built-ins and existing project
dependencies.
In `@qa/scripts/validate-flows.mjs`:
- Around line 9-15: The REQUIRED_QASE_FIELDS array is missing the qase_id field,
which violates the flow contract requirement that every flow must include a
qase_id (even if set to null). Add qase_id to the REQUIRED_QASE_FIELDS array
definition to enforce this requirement, and apply the same fix to the other
similar array mentioned in the comment (around lines 149-157) that also
validates qase metadata fields.
- Line 55: The regex pattern in the frontmatterMatch constant only matches Unix
line endings (\n) and will fail to parse frontmatter in files with Windows line
endings (\r\n), causing those flows to be incorrectly reported as missing
frontmatter. Update the regex pattern to handle both types of line endings by
modifying the delimiter matching to account for optional carriage returns before
newlines in both the opening --- delimiter, the content separator, and closing
--- delimiter.
In `@src/app/PersistableValues.ts`:
- Around line 223-226: In the `>=4.14.0` migration where
`telephonyGlobalShortcutConfig` is being set, modify the logic to preserve any
existing `telephonyGlobalShortcutConfig` values from the previous settings
instead of always resetting to the hardcoded defaults of enabled: false and
accelerator: null. Check if `telephonyGlobalShortcutConfig` already exists in
the source settings, and if it does, keep those values; only use the default
configuration if the setting doesn't already exist for this user.
In `@src/i18n/ar.i18n.json`:
- Around line 27-36: The Arabic locale file for telephonyShortcut is using the
outdated `reservedAccelerator` key while other locales have been updated to use
split keys `reservedByApp` and `reservedByOS` to distinguish between
app-reserved and OS-reserved keyboard accelerators. Replace the single
`reservedAccelerator` entry within the telephonyShortcut object with two new
keys: `reservedByApp` and `reservedByOS`, each with appropriate Arabic
translations that clearly indicate whether the shortcut is reserved by
Rocket.Chat or by the operating system.
In `@src/i18n/es.i18n.json`:
- Around line 293-302: The telephonyShortcut object in the Spanish translation
file uses the outdated `reservedAccelerator` key, but the schema has been
updated to split this into separate keys for different conflict types. Replace
the single `reservedAccelerator` translation with two new keys: `reservedByApp`
and `reservedByOS`, each with appropriate Spanish translations that specify
whether the accelerator is reserved by Rocket.Chat itself or by the operating
system.
In `@src/screenSharing/serverViewScreenSharing.ts`:
- Around line 140-148: The initializeProvider() function's error handler does
not clear the cached initialization state when initialization fails, which
prevents subsequent calls from retrying. In the catch block of the
initializeProvider().catch() chain, after logging the error and invoking the
callback, add logic to reset or clear any cached provider state (such as a
module-level or class-level initialization flag or cache variable) so that
future requests can attempt initialization again instead of reusing the failed
state.
In `@src/telephony/dialpad.ts`:
- Around line 120-131: In the dialpad.ts file, the webContents.send() call is
vulnerable to a race condition where the WebContents object can be destroyed
after getTelephonyWebContents returns a valid reference but before send is
executed. Add a guard check for the destroyed property on webContents before
calling the send method to verify the object is still alive, similar to the
pattern used in videoCallWindow/ipc.ts and
screenSharing/serverViewScreenSharing.ts. This will prevent send from throwing
an error on a stale handle.
In `@src/telephony/main.spec.ts`:
- Line 1: Rename the test file from its current filename to follow the
main-process spec naming convention. The file currently uses the pattern
`*.spec.ts` but should be renamed to `*.main.spec.ts` as per the repository's
test-file contract. Specifically, rename `main.spec.ts` to `main.main.spec.ts`
in the src/telephony directory to ensure proper test discovery and maintain
consistency with the coding guidelines for main-process specifications.
In `@src/telephony/main.ts`:
- Around line 331-379: The openSystemDefaultAppsSettings function has unhandled
promise rejections and missing error listeners on child processes. Add a
.catch() handler to the shell.openExternal() promise call in the Windows
platform block to catch async rejections. For the GNOME spawn call that directly
chains .unref(), attach an error listener before calling .unref() to handle
missing executable errors. Similarly, for the kcmshell6 spawn call invoked
within the KDE error handler, add an error listener before .unref() is called.
These changes ensure all async failures and child process errors are properly
caught instead of causing unhandled runtime errors.
In `@src/ui/components/SettingsView/features/TelephonyDiagnostics.tsx`:
- Around line 68-75: The fetchDiagnostics useCallback function uses a
try/finally block but lacks a catch block to handle errors from the invoke call,
causing unhandled promise rejections when the IPC call fails. Add a catch block
between the try and finally statements that captures any error thrown by
invoke('telephony/get-diagnostics') and sets an appropriate error state (such as
through setError or a similar error state setter) to provide controlled UI error
handling. Ensure this same fix is applied to the other similar function in the
same file at lines 78-84.
In `@src/ui/components/SettingsView/features/TelephonyGlobalShortcut.spec.tsx`:
- Around line 82-87: Both Object.defineProperty calls in the beforeAll and
afterAll blocks need to include configurable: true in their property descriptor
objects. In the beforeAll hook where process.platform is initially set to
'linux', add configurable: true to the descriptor. Similarly, in the afterAll
hook where the platform is being restored to originalPlatform, add configurable:
true to the descriptor as well. This will allow the property to be redefinable
in the teardown phase instead of throwing a TypeError when attempting to restore
the original value.
In `@src/ui/main/serverView/index.ts`:
- Around line 129-131: The console.log statement logging 'Permission request'
with permission and details parameters is running unconditionally in production,
exposing potentially sensitive information from web content. Either remove this
console.log call entirely or wrap it in a conditional check for development mode
using process.env.NODE_ENV === 'development'. This will ensure detailed
permission logs only appear during development and not in production builds,
making it consistent with other development-only logging patterns in the file.
- Around line 151-161: The isProtocolAllowed function can throw an error when
parsing a malformed URL, but there is no error handling around this call in the
openExternal case block. This means if an error is thrown, the callback function
is never invoked, which violates the Electron permission handler contract and
leaves the permission request unresolved. Wrap the isProtocolAllowed call in a
try-catch block, ensuring that in the catch block you invoke callback with a
false value to indicate the permission is denied, and in the try block invoke
callback with the allowed result as currently done.
In `@src/videoCallWindow/ipc.ts`:
- Around line 120-140: The `restoreServerViewHandler` function does not handle
the potential rejection from `getRootWindow()` which can fail during teardown or
before-quit scenarios, creating an unhandled rejection path since callers
intentionally drop the promise with `void`. Wrap the `getRootWindow()` call and
the subsequent permission handler setup in a try-catch block or use proper error
handling, ensuring that when the root window is unavailable, only the
`setupServerViewPermissionHandler` call is skipped while the display-media
restoration continues to run. Apply the same error handling pattern to the other
locations at lines 192-194, 588-597, and 778-782 where `getRootWindow()` is
called.
- Around line 435-451: The protocol validation for http and https must occur
before the g.co external-open check. Currently, the code checks for g.co
hostnames first, allowing non-http/https protocols like ftp to bypass the
allowedProtocols check. Additionally, the regex pattern in the hostname match
method is too permissive and will match hostnames like evilg.co. Move the
allowedProtocols check to happen before the g.co hostname check, and update the
regex pattern from its current form to only match g.co as an exact domain or as
a subdomain of g.co by using a regex that ensures g.co is either at the start of
the hostname or preceded by a dot.
---
Outside diff comments:
In `@src/videoCallWindow/ipc.ts`:
- Around line 225-235: The cleanup logic only guards `activeCall` against stale
teardown callbacks but leaves `videoCallWindow`, `videoCallCredentials`, and
`videoCallProviderName` unprotected. A stale first window's closed handler can
still clear these globals after a newer call is active. Capture the current
`videoCallWindow` reference at the start of the teardown (similar to how
`capturedCall` is captured for `activeCall`), and then add conditional checks
before clearing `videoCallWindow`, `videoCallCredentials`, and
`videoCallProviderName` in the setTimeout block to ensure they are only cleared
if they still belong to the captured window from this teardown. This prevents a
stale first window's cleanup from interfering with a newer active call's window
and credentials.
---
Minor comments:
In `@docs/windows-default-app-associations.md`:
- Around line 25-27: The fenced code blocks in the markdown file are missing
language identifiers which triggers markdownlint warnings (MD040). Add the
language tag `text` after the opening triple backticks for both code blocks -
the first block containing the registry path
(HKLM\SOFTWARE\Policies\Microsoft\Windows\System!DefaultAssociationsConfiguration)
and the second block containing the file path
(%ProgramFiles%\Rocket.Chat\resources\RocketChatDefaultAppAssociations.xml).
Change each opening ``` to ```text to satisfy the markdownlint requirement.
In `@qa/telephony-deeplink/README.md`:
- Around line 13-23: The Quick Start section in the README.md file is missing a
reference to the desktop-QA workflow entrypoint. Add a new instruction at the
beginning of the Quick Start steps that directs users to start Desktop PR/branch
QA by consulting skills/desktop-qa-flows/SKILL.md as the workflow entrypoint,
before proceeding with the remaining steps for the telephony-deeplink testing
pack.
In `@src/i18n/de-DE.i18n.json`:
- Line 162: The "cancel" key in the German translation file (de-DE.i18n.json)
contains a leading whitespace character before "Abbrechen" which causes
rendering inconsistencies in dialog button labels. Remove the leading space from
the "cancel" value so it reads exactly "Abbrechen" without any preceding
whitespace.
In `@src/i18n/pt-BR.i18n.json`:
- Line 162: The "cancel" key in the Portuguese-Brazilian translation has an
unintended leading whitespace in its value. Remove the extra space before
"Cancelar" so the value reads "Cancelar" instead of " Cancelar" to ensure the
dialog action text displays correctly without the extra space.
In `@src/i18n/zh.i18n.json`:
- Line 35: The translation key `reservedAccelerator` in the zh.i18n.json file
does not match the key that the component expects, which is
`settings.options.telephonyShortcut.reservedByApp`. Update the key structure in
the zh.i18n.json file to use the correct nested key path
`settings.options.telephonyShortcut.reservedByApp` instead of
`reservedAccelerator`, while keeping the Chinese translation value the same.
This ensures the component can properly find and display the translated error
message instead of falling back to the key string.
In `@src/telephony/diagnostics.ts`:
- Around line 361-367: The condition at line 361 using
trimmed.toLowerCase().includes('rocket') is too broad and will match any desktop
ID containing the word "rocket", causing unrelated handlers to incorrectly pass
validation and skip the commandLaunchesRocketChat verification. Replace the
broad substring check in the desktopIdLooksRocketChat assignment with a more
specific pattern that uniquely identifies actual Rocket.Chat desktop files, such
as checking for the exact string "rocketchat" or matching against the specific
filename pattern that Rocket.Chat uses. Additionally, add a regression test case
that verifies a non-Rocket.Chat desktop ID containing the word "rocket" (for
example, "my-rocket-launcher") correctly fails validation instead of passing.
In `@src/telephony/main.spec.ts`:
- Around line 685-697: The XDG_CURRENT_DESKTOP environment variable is being
deleted inside individual tests, which can leak into subsequent test cases if a
test exits early. Move the cleanup of XDG_CURRENT_DESKTOP into the afterEach
block where process.platform and process.execPath are already being restored.
Store the original value of process.env.XDG_CURRENT_DESKTOP before tests run and
restore it in afterEach using Object.defineProperty or direct assignment,
ensuring deterministic cleanup that executes regardless of whether individual
tests complete or exit prematurely.
In `@src/ui/components/SettingsView/features/TelephonyDiagnostics.tsx`:
- Around line 86-90: The handleCopy function assumes
navigator.clipboard.writeText always exists and succeeds without guarding
against restricted contexts where it may be unavailable or throw errors. Add
optional chaining to safely check if navigator.clipboard exists before accessing
writeText, use a try-catch or .catch() to handle potential Promise rejections
from writeText, and provide a fallback mechanism (such as using the deprecated
document.execCommand approach or notifying the user) when the clipboard API is
unavailable or fails.
In `@src/ui/components/SettingsView/features/TelephonyGlobalShortcut.tsx`:
- Around line 209-214: The FieldRow with FieldHint component that renders
telephonyGlobalShortcutRegistrationStatus.error is displaying stale errors even
after the shortcut has been changed or cleared by the user. Add a conditional
check before rendering the error to ensure it only displays when the error
corresponds to the currently active or configured shortcut, rather than showing
any residual error from a previous configuration.
In `@src/ui/components/SettingsView/features/TelephonyServer.spec.tsx`:
- Around line 52-198: Add a new test case within the TelephonyServer test suite
after the existing test cases to verify that the Select component is disabled
when isTelephonyEnabled is false. The test should create a store using makeStore
with isTelephonyEnabled set to false, render the TelephonyServer component
wrapped in a Provider, retrieve the select element using screen.getByTestId with
identifier 'telephony-select', and assert that the select element has the
disabled attribute using toBeDisabled matcher. Ensure the makeStore function
call includes the isTelephonyEnabled property and verify that the PartialState
type definition includes this field or extend it accordingly.
---
Nitpick comments:
In `@src/app/__tests__/PersistableValues.spec.ts`:
- Around line 4-17: The current test for the >=4.14.0 migration only validates
the case where telephonyGlobalShortcutConfig and isTelephonyEnabled don't exist
in the input, which means it won't catch overwrite regressions. Add another test
case using it() that creates a before object containing pre-existing
telephonyGlobalShortcutConfig with custom values and/or isTelephonyEnabled set
to true, then verify that calling migrations['>=4.14.0'] preserves those
existing values rather than overwriting them.
In `@src/telephony/__tests__/dialpad.spec.ts`:
- Line 1: Rename the test file from dialpad.spec.ts to dialpad.main.spec.ts to
align with the repository naming convention for main-process test suites. Since
this file tests main-process logic (as evidenced by the import from
deepLinks/actions), it should follow the *.main.spec.ts naming pattern used
throughout the codebase for main-process specifications.
In `@src/telephony/__tests__/msiInjection.spec.ts`:
- Line 1: Rename the test file from msiInjection.spec.ts to
msiInjection.main.spec.ts to follow the naming convention for test suites that
cover main-process behavior. This aligns with the coding guidelines that require
main-process specs to use the .main.spec.ts suffix for consistency.
In `@src/videoCallWindow/main/ipc.main.spec.ts`:
- Around line 512-542: The test currently only verifies that activeCall survived
the stale teardown but does not verify that the fresh window still owns the
lifecycle IPC. After the existing assertions that check the second window's
restore behavior, add an additional assertion using either the
video-call-window/close-requested or video-call-window/open-screen-picker IPC
handler to confirm that the fresh secondWindow still owns the lifecycle, which
would catch if stale teardown incorrectly cleared the videoCallWindow pointer.
🪄 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: f14384e8-f200-44d1-8ea4-a961d030dc10
⛔ Files ignored due to path filters (3)
qa/supported-versions/exports/qase-import.csvis excluded by!**/*.csvqa/telephony-deeplink/exports/qase-import.csvis excluded by!**/*.csvyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (116)
.gitignoreAGENTS.mdCLAUDE.mdbuild/RocketChatDefaultAppAssociations.xmlbuild/installer.nshbuild/msiProjectCreated.jsdocs/enterprise-deployment.mddocs/windows-default-app-associations.mdelectron-builder.jsonpackage.jsonqa/AGENTS.mdqa/README.mdqa/flow-template.mdqa/scripts/export-qase-csv.mjsqa/scripts/validate-flows.mjsqa/supported-versions/README.mdqa/supported-versions/exports/README.mdqa/supported-versions/flows/01-sha-prefixed-exception.mdqa/supported-versions/results/README.mdqa/telephony-deeplink/README.mdqa/telephony-deeplink/exports/README.mdqa/telephony-deeplink/flows/01-settings-discovery.mdqa/telephony-deeplink/flows/02-enable-disable-gating.mdqa/telephony-deeplink/flows/03-default-handler-prompt.mdqa/telephony-deeplink/flows/04-diagnostics-panel.mdqa/telephony-deeplink/flows/05-single-workspace-links.mdqa/telephony-deeplink/flows/06-multi-workspace-picker.mdqa/telephony-deeplink/flows/07-preferred-server-persistence.mdqa/telephony-deeplink/flows/08-global-shortcut.mdqa/telephony-deeplink/flows/09-macos-cold-launch.mdqa/telephony-deeplink/flows/10-windows-default-apps.mdqa/telephony-deeplink/flows/11-windows-msi-policy.mdqa/telephony-deeplink/flows/12-linux-protocols.mdqa/telephony-deeplink/flows/13-localization-layout.mdqa/telephony-deeplink/flows/14-negative-cases.mdqa/telephony-deeplink/results/README.mdqa/telephony-deeplink/scripts/README.mdqa/telephony-deeplink/test-links.htmlskills/desktop-qa-flows/SKILL.mdsrc/app/PersistableValues.tssrc/app/__tests__/PersistableValues.spec.tssrc/app/main/app.main.spec.tssrc/app/main/app.tssrc/app/selectors.tssrc/deepLinks/main.spec.tssrc/deepLinks/main.tssrc/i18n/ar.i18n.jsonsrc/i18n/de-DE.i18n.jsonsrc/i18n/en.i18n.jsonsrc/i18n/es.i18n.jsonsrc/i18n/fi.i18n.jsonsrc/i18n/fr.i18n.jsonsrc/i18n/hu.i18n.jsonsrc/i18n/it-IT.i18n.jsonsrc/i18n/ja.i18n.jsonsrc/i18n/nb-NO.i18n.jsonsrc/i18n/nn.i18n.jsonsrc/i18n/no.i18n.jsonsrc/i18n/pl.i18n.jsonsrc/i18n/pt-BR.i18n.jsonsrc/i18n/ru.i18n.jsonsrc/i18n/se.i18n.jsonsrc/i18n/sv.i18n.jsonsrc/i18n/tr-TR.i18n.jsonsrc/i18n/uk-UA.i18n.jsonsrc/i18n/zh-CN.i18n.jsonsrc/i18n/zh-TW.i18n.jsonsrc/i18n/zh.i18n.jsonsrc/ipc/channels.tssrc/main.spec.tssrc/main.tssrc/screenSharing/serverViewScreenSharing.tssrc/servers/supportedVersions/main.tssrc/store/index.tssrc/store/ipc.tssrc/store/rootReducer.tssrc/telephony/__tests__/defaultAssociationsXml.spec.tssrc/telephony/__tests__/diagnostics.spec.tssrc/telephony/__tests__/dialpad.spec.tssrc/telephony/__tests__/msiInjection.spec.tssrc/telephony/acceleratorDisplay.tssrc/telephony/actions.tssrc/telephony/common.tssrc/telephony/diagnostics.tssrc/telephony/dialpad.tssrc/telephony/ipc.tssrc/telephony/links.tssrc/telephony/main.spec.tssrc/telephony/main.tssrc/telephony/preload.tssrc/telephony/reducers.tssrc/telephony/renderer/preload.spec.tssrc/telephony/shortcuts.tssrc/ui/actions.tssrc/ui/components/SettingsView/SettingsView.tsxsrc/ui/components/SettingsView/VoiceVideoTab.tsxsrc/ui/components/SettingsView/features/Telephony.spec.tsxsrc/ui/components/SettingsView/features/Telephony.tsxsrc/ui/components/SettingsView/features/TelephonyDiagnostics.tsxsrc/ui/components/SettingsView/features/TelephonyGlobalShortcut.spec.tsxsrc/ui/components/SettingsView/features/TelephonyGlobalShortcut.tsxsrc/ui/components/SettingsView/features/TelephonyServer.spec.tsxsrc/ui/components/SettingsView/features/TelephonyServer.tsxsrc/ui/components/SettingsView/features/__tests__/TelephonyDiagnostics.spec.tsxsrc/ui/components/Shell/index.tsxsrc/ui/components/TelephonyDefaultHandlerPromptModal/index.spec.tsxsrc/ui/components/TelephonyDefaultHandlerPromptModal/index.tsxsrc/ui/components/TelephonyServerSelectModal/ServerItem.tsxsrc/ui/components/TelephonyServerSelectModal/index.spec.tsxsrc/ui/components/TelephonyServerSelectModal/index.tsxsrc/ui/main/serverView/index.tssrc/ui/reducers/dialogs.tssrc/ui/reducers/isTelephonyEnabled.tssrc/videoCallWindow/ipc.tssrc/videoCallWindow/main/ipc.main.spec.tssrc/videoCallWindow/video-call-window.ts
Hardening fixes from CodeRabbit review (verified against code, false positives rejected): Security / stability: - videoCallWindow/ipc.ts: validate URL protocol (http/https) before the g.co external-open escape hatch (was reachable via ftp://g.co/...); tighten host match to exact g.co / *.g.co (was overmatching evilg.co); guard getRootWindow() rejection in restoreServerViewHandler so teardown can't produce an unhandled rejection (display-media restore still runs). - serverView/index.ts: gate the 'Permission request' debug log behind NODE_ENV==='development' (was logging payloads in production); wrap isProtocolAllowed() in try/catch so the openExternal permission callback always fires even on malformed URLs. - telephony/main.ts: handle shell.openExternal() promise rejection with .catch(); attach 'error' listeners to gnome/kcmshell spawns before unref() so a missing executable can't throw unhandled. - screenSharing/serverViewScreenSharing.ts: reset cached init state on provider init failure so later requests can retry instead of reusing a rejected promise until restart. - telephony/dialpad.ts: guard webContents.send against a destroyed handle. Data integrity: - PersistableValues.ts: preserve persisted telephonyGlobalShortcutConfig in the >=4.14.0 migration instead of resetting it to defaults on upgrade. UI / i18n: - TelephonyDiagnostics.tsx: catch rejected get-diagnostics IPC and set a controlled empty state instead of leaking an unhandled rejection. - i18n/ar, i18n/es: split reservedAccelerator into reservedByApp / reservedByOS to match the runtime keys (other locales fall back to en). QA tooling / installer: - validate-flows.mjs: enforce qase_id PRESENCE (key may be null per the flow contract) rather than truthiness, which would have rejected every existing flow; handle CRLF frontmatter delimiters. - export-qase-csv.mjs: normalize CRLF on read for cross-platform parsing. - package.json: declare yaml as a devDependency (used by export-qase-csv). - msiProjectCreated.js: fail fast on telephony RegWrite errors in the WriteTelephonyCapabilities custom action. Tests: - main.spec.ts: mock shell.openExternal as a resolved promise. - dialpad.spec.ts / deepLinks/main.spec.ts: add isDestroyed() to webContents mocks for the new destroyed-handle guard. - TelephonyGlobalShortcut.spec.tsx: add configurable:true so the process.platform restore in afterAll doesn't throw. Rejected: - Rename telephony/main.spec.ts -> main.main.spec.ts: false positive. jest testMatch already routes src/**/main.spec.ts to the main-process project; the rename would break discovery. Verified: tsc clean, lint clean, full suite 456 passed / 2 skipped / 0 failed, validate-flows passes all 14 telephony flows.
…deo call window
The standalone internal video-chat window is its own BrowserWindow with no
window.opener, so the web app's window.open/opener trick can't reach the main
window — it just spawns another window. Add an IPC path so the conference page
can ask the main app window to focus itself and navigate to an in-app route.
Caller (video-chat window):
- window.videoCallWindow.openInMainWindow(path) — validates that path is an
in-app relative route ("/..."), rejecting absolute/protocol-relative/scheme
URLs, then invokes 'video-call-window/open-in-main-window'.
Main process:
- New handler resolves the target server webview (caller's own server, else the
active currentView.url), shows/restores/focuses the main window, and emits
'navigate-to-route' (payload: path) to that server webview's webContents. It
intentionally does NOT loadURL — that would hard-reload the SPA. No-ops safely
with a warning when no window/webview is found, and re-validates the path.
Receiver (server webview is contextIsolated, so a raw send lands in the preload,
not the page):
- New navigateToRoute preload relay listens on 'navigate-to-route' and forwards
to a RocketChatDesktop.onNavigateToRoute(callback) the web client registers,
buffering the latest path if it arrives before registration (mirrors the
telephony relay).
Also: Cmd/Ctrl+Shift+D ("Toggle Developer Tools") now targets the focused window
(falling back to the main window), so it can open DevTools for the video call
window instead of always the main one.
Web-repo follow-up (out of scope here): the web client must call
RocketChatDesktop.onNavigateToRoute(path => router.navigate(path)) for the route
change to take effect.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the standalone video call window asks the main window to navigate, the handler resolved the target server from the caller webContents and fell back to whichever server was active in the main window. In a multi-workspace setup that could navigate a *different* server than the one the call belongs to. Resolve in priority order: caller's own server, then the active call's origin server (authoritative, via activeCall.serverWebContentsId), then the active view as a last-resort guess (now logged as ambiguous). Also log in the preload bridge when a non-relative path is rejected, for parity with the main-process handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The internal video-chat window is a BrowserWindow created by the main process,
so the renderer's own window.close() can't close it. Expose a close() method on
the window.videoCallWindow bridge that asks the main process to do it.
- Preload: close: () => ipcRenderer.send('video-call-window/close') (no payload).
- Main: ipcMain.on('video-call-window/close') resolves the window from the sender
(with a hostWebContents fallback for the webview-guest sender) and calls
win.close() when it's live. Resolving from the sender means a renderer can only
close its own window.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ference Clicking "join" again from the main window while the video call window was already open tore the window down and recreated it. When the requested conference URL matches the one already open, focus the existing window instead (restore if minimized, show, focus) and return early, leaving activeCall, provider, credentials and partition untouched. A different URL still closes + recreates as before. Track the conference URL on activeCall so the decision uses lifecycle state rather than the renderer-handshake globals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wser
The internal video-chat window didn't route external links to the system
browser like the main window does, so target="_blank" / window.open links from
the conference chat (which run in the webview guest, whose setWindowOpenHandler
was unset) spawned a new Electron window instead.
Set the guest webview's window-open handler on attach: http(s) popups return
{ action: 'deny' } and open via the system browser (openExternal), smb:// is
denied, anything else stays in-app. Also add a will-navigate handler that sends
external-scheme target="_self" navigations (mailto:, tel:, custom schemes) to
the browser, while leaving http(s) self-navigations in the webview so the
conference's own flows (auth redirects, etc.) keep working.
Extract the shared deny/openExternal policy into a helper reused by the host
window's existing handler so host and guest behave identically.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- media/openExternal permission branches in serverView now catch rejections and deny instead of leaving the request hanging - video call window-open policy denies popups by default, allowing only about:/blob: in-app schemes (closes javascript:/data:/file:/smb:) - install the media permission handler on the conference webview's partition session (isolated fallback only) so mic/cam requests route through handleMediaPermissionRequest instead of Electron's default - resolve the partition / set activeCall only inside the window-creating branch, after URL validation and the g.co redirect, so a bailed-out open can't leave stale state for teardown to misread - swallow the fire-and-forget open-in-main-window invoke rejection - drop no-op awaits on synchronous getNormalBounds()/getURL() - add regression tests for popup scheme policy and the fallback-session permission handler
# Conflicts: # src/ui/main/serverView/index.ts # src/videoCallWindow/ipc.ts
# Conflicts: # package.json # yarn.lock
Linux installer download |
macOS installer download |
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
2 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.
Click-to-Call + Internal Video Chat Session Sharing
Part 1 — Click-to-Call: Dial Phone Numbers Directly from Any App
Summary
You can now click phone number links (
tel:andcallto:) in any application — browser, email client, CRM, PDF viewer — and have Rocket.Chat open with the number ready to dial. The feature is fully opt-in: a master toggle in Settings > Voice & Video controls everything. When enabled, a new global keyboard shortcut lets you dial whatever phone number is on your clipboard from anywhere on your system.What's New
Added
tel:andcallto:links, in all common formats — international, dashed, parenthesizedImproved
Fixed
%2Bfor+) now decode correctly before being passed to the dial padPlatform Notes
tel:links. Rocket.Chat'sInfo.plistdeclarestel/calltoso macOS lists it as a candidate even before the toggle is enabled — but it only becomes the default handler once the user enables TelephonysetAsDefaultProtocolClientcall fortel/calltoonly fires when the toggle is on. Works from Outlook, Edge, Chrome, and other appsxdg-openon both X11 and Wayland. The.desktopMimeType list is baked at install time, so the OS may list the app as a candidate before the toggle is enabledPart 2 — Internal Video Chat Window: Share Main Webview Session (merged from #3359)
Problem
Internal conferences opened via
openInternalVideoChatWindowran in a webview hardcoded to the isolatedpersist:jitsi-sessionpartition, so they did not share cookies or localStorage with the server webview they were opened from. Electron scopes cookies/localStorage to the(partition, origin)pair, so a same-origin conference loaded unauthenticated (the login token lives in localStorage under the server origin).Fix
Load the call webview in the originating server's partition (
persist:<serverUrl>, resolved from the callerwebContents) so the call shares the main webview's session.Because session-level handlers are per-session, sharing the session means the call window's handlers would otherwise clobber the main webview's:
Production hardening
Per-window state instead of a mutable global
The call's partition was tracked in a module-level global read at several lifecycle points that span async ticks (cleanup, screen-share routing, teardown restore). A second call opening could mutate it mid-flight. State is now captured per window in an
ActiveCallrecord ({ partition, isSharedSession, serverWebContentsId }); every lifecycle decision reads the per-window value. The global is narrowed to renderer-handshake reads only.Handler restore on every teardown path
Restore is now idempotent and fires from every teardown path:
closed,cleanupVideoCallWindow, and a newrender-process-gonelistener. It restores both the server-view display-media handler and the permission handler (extracted into an exportedsetupServerViewPermissionHandlershared by the install site and the restore site). Fallback (isolated) sessions are skipped.Screen-share routing evaluated at request time
The per-window record is snapshotted at attach and the routing branch is evaluated at display-media request time; the call-window identity check stays live.
State null-out guarded by identity
Terminal teardowns now null only their own state (
if (activeCall === capturedCall) activeCall = null), so a stale prior-window teardown can't wipe a newer call's record.Open-window race serialized
The open handler is serialized through a promise-chain mutex, so each open runs to completion before the next begins, closing a rapid-double-open race that orphaned a window with leaked handlers.
Unresolved-server fallback
When the originating server can't be resolved, the call falls back to
persist:jitsi-session(stable isolated storage), andisSharedSessionisfalseso none of the shared-session restore logic engages.Additional lifecycle bridges
videoCallWindow.close()bridge — the call renderer can ask the main process to close its own window (a renderer can't close a main-process-created window directly).ActiveCallcarries the conferenceurlto support this.open-in-main-windowbridge — the standalone call window can navigate the main app window to an in-app route (e.g./channel/...) and bring it to the front. Routes are sanitized (relative-only) on both the preload and main sides.target="_blank"/window.openand external-protocol self-navigations from the conference go to the system browser instead of spawning Electron popups.Review fixes (post-#3359 review)
about:/blob:schemes and routing externalhttp(s)to the system browser, so a compromised frame can't openjavascript:/data:/file:Electron windows.activeCall/partition resolution moved after URL validation and theg.coexternal redirect, so a bailed-out open can't leave stale state for a later teardown to misread.media/openExternalbranches catch rejections and deny instead of leaving the request hanging; the preloadopen-in-main-windowinvoke swallows its rejection.awaits on synchronousgetNormalBounds()/getURL().Files changed (video-call)
src/videoCallWindow/ipc.ts— per-window state, idempotent restore from all teardown paths, request-time routing, null-out guards, open-handler serialization, webview-partition permission handler, popup-scheme policy, deferred partition assignment.src/ui/main/serverView/index.ts— extracted/exportedsetupServerViewPermissionHandler.src/videoCallWindow/video-call-window.ts— receives the partition from the main process instead of hardcoding it.src/ipc/channels.ts—partitionadded to therequest-urlresponse type.src/screenSharing/serverViewScreenSharing.ts—handleServerViewDisplayMediaRequestfor routing main-app requests back to the server-view picker.src/videoCallWindow/preload/index.ts—openInMainWindow/closebridges with relative-route sanitization.src/servers/preload/navigateToRoute.ts— buffered single-listenernavigate-to-routebridge wired into preload startup.src/ui/main/menuBar.ts— DevTools targets the focused window before falling back to the root window.src/videoCallWindow/main/ipc.main.spec.ts,src/servers/preload/navigateToRoute.spec.ts— regression tests.Merge integration notes
The video-call branch (#3359) was based on a newer
masterthan the telephony branch (#3325, 14 commits behind). Merging therefore also synced telephony up to currentmaster. Conflicts were resolved by integrating both sides:PersistableValues.ts— chained the migration versions: kept4.14.0(telephony:isTelephonyEnabled,telephonyPreferredServer,telephonyGlobalShortcutConfig) and layeredmaster's4.15.0(e2ePdfPreviewSizeLimit) on top so the migration chain stays sequential.GeneralTab.tsx— adoptedmaster's new layout and settings entries (ScreenCaptureFallback,InternalVideoChatWindow,VideoCallWindowPersistence,E2ePdfPreviewSizeLimit,ClearPermittedScreenCaptureServers) while keeping<TelephonyServer />.AvailableBrowsers.tsx/ThemeAppearance.tsx/TelephonyServer.tsx— adoptedmaster'sSettingFieldwrapper convention.TelephonyServerpreserves telephony behaviormasterlacked (disabled={!isTelephonyEnabled},safeHostname).Accordion.Item→ standaloneAccordionIteminVoiceVideoTab.tsxandTelephonyDiagnostics.tsx(the Fuselage upgrademastercarried removed the compound API).Later merges onto this branch
serverView/index.tsandvideoCallWindow/ipc.tswere resolved by keeping the deferred partition placement (fix OAuth not working #4) on top of the upstream protocol-validation / exactg.cohost-match guards.mastertest-coverage sync —master's coverage tooling (Codecov config, Phase 1–3 unit/RTL tests) was merged in to keep the branch current. Conflicts inpackage.json/yarn.lockover@testing-library/*were resolved by keeping the newer stack (react ~16.3.2,dom ~10.4.1,jest-dom ~6.9.1) and addingmaster'suser-event ~14.5.2;yarn.lockwas regenerated viayarn install.Verification
tsc --noEmit: clean (post-merge)yarn lint: clean on changed video-call filesvideoCallWindow/main/ipc.main.spec.ts: passing on the merged treemaster's newly merged RTL coverage tests under@testing-library/reactv16): left to CI on this PRRelated
Summary by CodeRabbit
tel:/callto:with dialpad integration, server selection, and a global shortcut.tel:/callto:associations (MSI flag) and bundled association metadata.