Skip to content

feat(ui-voip): pre-fill media-call widget from desktop telephony deeplink#40751

Open
jeanfbrito wants to merge 5 commits into
developfrom
feat/telephony-deeplink-consumer
Open

feat(ui-voip): pre-fill media-call widget from desktop telephony deeplink#40751
jeanfbrito wants to merge 5 commits into
developfrom
feat/telephony-deeplink-consumer

Conversation

@jeanfbrito
Copy link
Copy Markdown
Member

@jeanfbrito jeanfbrito commented Jun 1, 2026

Jira: DMV-36 — Desktop deeplink core improvements

Proposed changes

Consumer side of the Desktop telephony deeplink feature. The Rocket.Chat Desktop app forwards tel:/callto: deeplinks and global-shortcut phone numbers to the workspace via window.RocketChatDesktop.onTelephonyCallRequested. This PR consumes that signal and pre-populates the media-call dial-pad with the number — the user still presses Call.

What's included

  • useDesktopTelephonyListener — subscribes to onTelephonyCallRequested and routes by widget state:
    • closed → open the widget pre-filled with the number
    • new → widget already open/idle → set the number
    • in-progress call → ignored
  • usePeerAutocomplete — reflect an externally-selected { number } peer in the visible input. The input value was derived from userId only, so a number set programmatically left the field empty. The new sync fires on peerInfo identity change, so manual typing is preserved.
  • @rocket.chat/desktop-api — declare optional onTelephonyCallRequested on IRocketChatDesktop (payload { phoneNumber, rawUri }). Implemented in Rocket.Chat.Electron#3325.

Cold-start

When the Desktop app is closed at deeplink time, the event fires before the workspace mounts the listener. Buffering + replay-last-pending on subscribe is handled on the Desktop side; this consumer registers once on mount and handles the event whenever it arrives.

How to test

  1. Build/run Desktop from Rocket.Chat.Electron#3325.
  2. Trigger a tel:/callto: deeplink or the global shortcut.
  3. Media-call widget opens with the number in the dial-pad input; press Call.

Further comments

  • @rocket.chat/ui-voip is a private package → no changeset; @rocket.chat/desktop-api is published → changeset included (minor).
  • New unit tests cover the listener's state routing and the number→input sync (incl. manual-edit preservation).
  • Implementation refinement vs the ticket AC: the listener lives in packages/ui-voip (co-located with the media-call provider) rather than apps/meteor/client, and routes by widget state (toggleWidget/selectPeer) instead of toggleWidget only.

Summary by CodeRabbit

  • New Features

    • Desktop app integration: tel:/callto: deeplinks and global shortcuts now open the voice widget.
  • Behavior Changes

    • Incoming phone numbers pre-fill the dialer; manual edits are preserved and update the selected number.
    • Telephony requests are routed by widget state and ignored during active calls.
    • Outgoing flow prevents call attempts when the destination number is empty.
    • Widget teardown preserves user-driven “idle compose” state when calls clear.
  • Tests

    • Added comprehensive tests for telephony handling, number sync, and session-state behaviors.

…link

Consume `window.RocketChatDesktop.onTelephonyCallRequested` (forwarded
`tel:`/`callto:` deeplinks and global-shortcut numbers from the Desktop
app) and pre-populate the media-call dial-pad with the number. The user
still starts the call.

- useDesktopTelephonyListener: routes by widget state — closed opens the
  widget pre-filled, new sets the number, an in-progress call is ignored.
- usePeerAutocomplete: reflect an externally-selected `{ number }` peer in
  the visible input (value is derived from userId only, so the field would
  otherwise stay empty). Fires on peerInfo identity change, preserving
  manual typing.
- desktop-api: declare optional `onTelephonyCallRequested` on
  IRocketChatDesktop. Implemented in Rocket.Chat.Electron#3325.

Cold-start replay (app closed when the deeplink fires) is handled on the
Desktop side: the contract is replay-last-pending on subscribe.
@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Jun 1, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 1, 2026

🦋 Changeset detected

Latest commit: 7808b77

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rocket.chat/desktop-api Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6f0e4ce7-0ffb-4dfd-bdbc-bda0c5ff7474

📥 Commits

Reviewing files that changed from the base of the PR and between 2d663cb and 7808b77.

📒 Files selected for processing (1)
  • .changeset/telephony-call-requested-desktop-api.md
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: CodeQL-Build
  • GitHub Check: Hacktron Security Check
  • GitHub Check: CodeQL-Build
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.

Applied to files:

  • .changeset/telephony-call-requested-desktop-api.md
🔇 Additional comments (1)
.changeset/telephony-call-requested-desktop-api.md (1)

2-5: LGTM!


Walkthrough

Adds a desktop API callback and changes to media/voip hooks/providers so desktop telephony requests spawn or prefill a media-call PeerInfo; autocomplete syncs external numbers into the filter and media session teardown preserves user-driven "new" state.

Changes

Desktop Telephony Call Request Integration

Layer / File(s) Summary
Desktop API telephony callback contract
.changeset/telephony-call-requested-desktop-api.md, packages/desktop-api/src/index.ts
IRocketChatDesktop gains optional onTelephonyCallRequested(callback) accepting { phoneNumber, rawUri }; changeset declares minor bump.
Desktop telephony listener hook and tests
packages/ui-voip/src/providers/useDesktopTelephonyListener.ts, packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
New useDesktopTelephonyListener registers the desktop callback and routes incoming phone numbers as PeerInfo to toggleWidget when session is closed or selectPeer when new; tests cover registration, routing, ignored active states, pending delivery handling, and absent-bridge safety.
Autocomplete filter external number sync
packages/ui-voip/src/context/usePeerAutocomplete.ts, packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
usePeerAutocomplete syncs peerInfo.number into the input filter on peerInfo identity changes and updates selected { number } on manual edits/keypad for number-based peers; tests validate sync, identity semantics, and edit preservation.
MediaCallViewProvider listener integration
packages/ui-voip/src/providers/MediaCallViewProvider.tsx
Provider imports and invokes useDesktopTelephonyListener with sessionState, toggleWidget, and selectPeer to wire desktop telephony handling into the media call lifecycle; outgoing-call flow short-circuits when a number peer's trimmed input is empty.
Media session teardown preserves 'new' state
packages/ui-voip/src/providers/useMediaSession.ts, packages/ui-voip/src/providers/useMediaSession.spec.tsx
Reducer adds call_cleared action; effects dispatch call_cleared when call-derived state is gone so call info clears but a user-driven new widget state is preserved; tests cover lifecycle resets and state retention.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

type: feature

Suggested reviewers

  • tassoevan
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding desktop telephony deeplink integration to pre-fill the media-call widget with phone numbers.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • DMV-36: Request failed with status code 401

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the type: feature Pull requests that introduces new feature label Jun 1, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eea5bfd81a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/ui-voip/src/context/usePeerAutocomplete.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/ui-voip/src/context/usePeerAutocomplete.ts (1)

42-45: 💤 Low value

Consider removing the inline comment.

Per coding guidelines, code comments should be avoided in TypeScript implementation. The effect's behavior can be understood from the conditional and effect dependencies.

🤖 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 `@packages/ui-voip/src/context/usePeerAutocomplete.ts` around lines 42 - 45,
Remove the inline explanatory comment inside usePeerAutocomplete that describes
the effect behavior (the block referencing peerInfo, value and number)—leave the
effect and its conditionals intact (the logic that mirrors an
externally-selected phone number into the input when peerInfo identity changes)
but delete the comment text so only the implementation (including references to
peerInfo, value and number) remains.
packages/ui-voip/src/providers/useDesktopTelephonyListener.ts (1)

11-23: 💤 Low value

Consider removing the documentation comment.

Per coding guidelines, code comments should be avoided in TypeScript implementation. The hook's behavior can be documented externally or inferred from the type signature and implementation.

🤖 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 `@packages/ui-voip/src/providers/useDesktopTelephonyListener.ts` around lines
11 - 23, Remove the large documentation block comment above the
useDesktopTelephonyListener hook; delete the multi-line JSDoc-style comment that
describes deeplink/global-shortcut handling and routing behavior so the
TypeScript implementation contains no large explanatory comments, leaving only
the hook export (useDesktopTelephonyListener) and related runtime code
(including references to window.RocketChatDesktop.onTelephonyCallRequested)
intact; if any short clarification is still required for maintainers keep it as
a one-line comment next to the hook definition rather than the current
multi-line doc block.
🤖 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.

Nitpick comments:
In `@packages/ui-voip/src/context/usePeerAutocomplete.ts`:
- Around line 42-45: Remove the inline explanatory comment inside
usePeerAutocomplete that describes the effect behavior (the block referencing
peerInfo, value and number)—leave the effect and its conditionals intact (the
logic that mirrors an externally-selected phone number into the input when
peerInfo identity changes) but delete the comment text so only the
implementation (including references to peerInfo, value and number) remains.

In `@packages/ui-voip/src/providers/useDesktopTelephonyListener.ts`:
- Around line 11-23: Remove the large documentation block comment above the
useDesktopTelephonyListener hook; delete the multi-line JSDoc-style comment that
describes deeplink/global-shortcut handling and routing behavior so the
TypeScript implementation contains no large explanatory comments, leaving only
the hook export (useDesktopTelephonyListener) and related runtime code
(including references to window.RocketChatDesktop.onTelephonyCallRequested)
intact; if any short clarification is still required for maintainers keep it as
a one-line comment next to the hook definition rather than the current
multi-line doc block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 022fec20-56e0-4386-b159-c3051581c5d5

📥 Commits

Reviewing files that changed from the base of the PR and between 0ef1002 and eea5bfd.

📒 Files selected for processing (7)
  • .changeset/telephony-call-requested-desktop-api.md
  • packages/desktop-api/src/index.ts
  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/context/usePeerAutocomplete.ts
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: CodeQL-Build
  • GitHub Check: Hacktron Security Check
  • GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/context/usePeerAutocomplete.ts
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/desktop-api/src/index.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
🧠 Learnings (8)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.

Applied to files:

  • .changeset/telephony-call-requested-desktop-api.md
📚 Learning: 2026-02-26T19:22:29.385Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:29.385Z
Learning: For TSX files in the UI VOIP package, ensure that when a media session state is 'unavailable', the voiceCall action is excluded from the actions object passed to CallHistoryActions so it does not render in the menu. This filtering should occur upstream (before getItems is called) to avoid tooltips or UI hints for unavailable actions. If there are multiple actions with availability states, implement a centralized helper to filter actions based on session state.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
📚 Learning: 2026-05-05T12:34:29.042Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 40331
File: packages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx:69-69
Timestamp: 2026-05-05T12:34:29.042Z
Learning: In Rocket.Chat’s `packages/ui-voip` UI (e.g., media/call widgets), voice/media calls are only supported in Direct Message (DM) rooms. Rocket.Chat models a DM as a “room” with exactly two participants, so handlers like `onClickDirectMessage` are the correct destination—even when the UI text/element says “Open in room” (e.g., on the shared screen card/`StreamCard`). During review, don’t flag a “DM vs room” mismatch for these cases; they intentionally map to the same destination.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx
  • packages/ui-voip/src/context/usePeerAutocomplete.ts
  • packages/ui-voip/src/providers/MediaCallViewProvider.tsx
  • packages/desktop-api/src/index.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.ts
  • packages/desktop-api/src/index.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

Applied to files:

  • packages/ui-voip/src/context/usePeerAutocomplete.ts
  • packages/desktop-api/src/index.ts
  • packages/ui-voip/src/providers/useDesktopTelephonyListener.ts
🔇 Additional comments (7)
.changeset/telephony-call-requested-desktop-api.md (1)

1-6: LGTM!

packages/desktop-api/src/index.ts (1)

66-66: LGTM!

packages/ui-voip/src/providers/useDesktopTelephonyListener.ts (1)

24-49: LGTM!

packages/ui-voip/src/providers/useDesktopTelephonyListener.spec.tsx (1)

1-107: LGTM!

packages/ui-voip/src/context/usePeerAutocomplete.spec.tsx (1)

142-200: LGTM!

packages/ui-voip/src/providers/MediaCallViewProvider.tsx (1)

16-16: LGTM!

Also applies to: 47-48

packages/ui-voip/src/context/usePeerAutocomplete.ts (1)

46-50: ⚡ Quick win

Update filter when peerInfo switches away from { number }

  • usePeerAutocomplete sets filter only when peerInfo has number and leaves filter unchanged when peerInfo becomes { userId } or undefined (lines 46-50), which can leave a stale phone-number term driving peerAutocomplete(debouncedFilter).
  • usePeerAutocomplete.spec.tsx covers numbernumber updates and “preserve manual typing while peerInfo is unchanged”, but there’s no test for numberuserId / undefined, so the intended UX here isn’t locked in.
Proposed fix (if you want the dropdown/search term cleared when not syncing a number)
	useEffect(() => {
		if (peerInfo && 'number' in peerInfo) {
			setFilter(peerInfo.number);
+		} else {
+			setFilter('');
		}
	}, [peerInfo]);

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 7 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/ui-voip/src/context/usePeerAutocomplete.ts
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 65.45455% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.80%. Comparing base (b92bcc7) to head (7808b77).
⚠️ Report is 12 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #40751      +/-   ##
===========================================
+ Coverage    69.76%   69.80%   +0.04%     
===========================================
  Files         3327     3328       +1     
  Lines       123134   123175      +41     
  Branches     21963    21996      +33     
===========================================
+ Hits         85902    85983      +81     
+ Misses       33873    33835      -38     
+ Partials      3359     3357       -2     
Flag Coverage Δ
unit 70.54% <65.45%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jeanfbrito
Copy link
Copy Markdown
Member Author

@coderabbitai Keeping the explanatory comments in usePeerAutocomplete.ts and useDesktopTelephonyListener.ts. The "avoid comments in implementation" rule comes from .cursor/rules/playwright.mdc, which is scoped to Playwright tests; the **/*.{ts,tsx,js} glob over-matches src here. Both comments document non-obvious behavior — the number-input sync and the listener’s widget-state routing — so they earn their place.

When a number peer is pre-filled (e.g. from a desktop telephony deeplink),
editing the dial-pad input only changed local filter state, leaving
sessionState.peerInfo.number stale. onCall then dialed the original number
instead of the one shown. Sync the selected peer on onChangeFilter/onKeypadPress
for number-dial peers so the call dials what the user sees.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Tip

For best results, initiate chat on the files or code changes.

@jeanfbrito That's a fair and well-reasoned correction — thank you for tracing it back to the source. The rule in .cursor/rules/playwright.mdc is indeed scoped to Playwright test authoring, and applying it to general src/**/*.{ts,tsx,js} implementation files was an over-reach on my part. The comments you kept document genuinely non-obvious invariants (the identity-gated sync in usePeerAutocomplete and the widget-state routing logic in useDesktopTelephonyListener), so they're appropriate and useful here. Totally agree — keeping them is the right call.


✏️ Learnings added
Learnt from: jeanfbrito
URL: https://github.com/RocketChat/Rocket.Chat/pull/40751

Timestamp: 2026-06-01T18:56:35.183Z
Learning: In the Rocket.Chat repository, the "avoid explanatory comments in implementation files" guideline originates from `.cursor/rules/playwright.mdc` and is scoped **only to Playwright test files**. The `**/*.{ts,tsx,js}` glob in that rule over-matches general `src/` TypeScript/JavaScript source files. Do not flag explanatory comments in `src/**/*.{ts,tsx}` implementation files as violating this rule; comments documenting non-obvious behavior in source files are acceptable and encouraged.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai coderabbitai Bot added type: bug and removed type: feature Pull requests that introduces new feature labels Jun 1, 2026
On cold start the dial pad opened pre-filled then immediately closed, and the
delivered number could be dropped. Two causes: the freshly-initialized
media-call instance emits a no-active-call state right after init, which reset
the widget from 'new' back to 'closed'; and the deeplink number is delivered
consume-once, before the dial pad can act.

- useDesktopTelephonyListener: register the handler once at mount and store the
  delivered number, applying it via a separate effect when the widget is idle.
  Delivery is consume-once, so registration must not be gated on subsystem
  readiness — only acting on the number is.
- useMediaSession: a no-active-call instance emit no longer tears down an idle
  'new' compose widget (e.g. a deeplink-prefilled dial pad); only real call
  teardown resets it.
@coderabbitai coderabbitai Bot added type: feature Pull requests that introduces new feature and removed type: bug labels Jun 1, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4667ffa5be

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/ui-voip/src/context/usePeerAutocomplete.ts
Clearing the dial-pad input re-selects a number peer with an empty number (the
input and selected peer are mirrored on purpose). Guard onCall so an empty or
whitespace-only number no longer requests media or attempts a SIP call — it is
a no-op, consistent with the existing empty-peer case.
'@rocket.chat/desktop-api': minor
---

Add `onTelephonyCallRequested(callback)` to the `IRocketChatDesktop` type definition. Desktop clients can expose this method to forward `tel:`/`callto:` deeplink and global-shortcut phone numbers to the media-call widget. Implemented in Rocket.Chat.Electron#3325.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changeset reads a little too technical. I recommend rephrasing it to focus on the feature it enables instead of implementation details.

For instance:
"Adds deeplink and shortcut handling for voice calls by integrating with the desktop app API to allow phone links and global shortcuts triggered outside the app to open the voice widget seamlessly. Related to Rocket.Chat.Electron#3325."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — reworded in 7808b77 using your suggested text. Thanks!

return;
}

window.RocketChatDesktop.onTelephonyCallRequested(({ phoneNumber }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see a way to remove this listener on unmount. Is this a concern?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair question — I looked into it and I believe it's safe as-is:

  • The desktop side is a single slot, not a listener list. In the Electron implementation (Rocket.Chat.Electron#3325, src/telephony/preload.ts), onTelephonyCallRequested does telephonyCallback = callback — registration overwrites, it never accumulates. So repeated registrations can't leak or double-fire.
  • The hook lives for the app lifetime. It's mounted by MediaCallViewProvider, which sits in the root provider tree (MeteorProvider.tsx), so in practice it never unmounts. If it ever did remount, the new registration overwrites the stale slot, so it self-heals.
  • Registering once at mount is deliberate. The desktop app buffers a cold-start deeplink and delivers it consume-once, synchronously inside the first registration (see the hook's docblock). An unsubscribe/resubscribe cycle wouldn't protect anything that the overwrite semantics don't already cover.

That said, the Electron PR is still open — if you'd prefer the API to return an unsubscribe function for hygiene, I can add it on both sides. Let me know!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the changes to this file really needed? 🤔
Having the number (instead of number and filter) selected seems more inline the way we already pre-fill the widget when opening via DM rooms.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered the chip-only approach, but I think the changes are needed — number peers and userId peers have different display contracts in the current widget:

  • filter == selected number is already the invariant for number peers on develop. When a user dials manually (types a number, picks the first option), onChangeValue selects { number } but doesn't clear the filter — so the input keeps showing the selected number. The sync effect just extends that same invariant to numbers selected externally (deeplink), instead of leaving the input empty only in that one path.
  • DM-room prefill is a userId peer — a different shape. There the chip carries the display (name/avatar) and the input is a search box for finding someone else. For a number peer the input is the dial surface; an empty input with the number only in the chip would make the number un-editable in place (wrong extension, missing prefix), which is a common need for tel: links.
  • updateNumberFilter also fixes a bug that exists on develop today, independent of deeplinks: type 123 → select the first option → type 4 → input shows 1234 but Call dials 123. Keeping the peer in sync with edits closes that stale-dial gap for the manual flow too.

Happy to walk through it if you'd like — but dropping the sync would reintroduce the empty-input inconsistency and the stale-dial edit bug for number peers.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7808b77471

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const updateNumberFilter = (next: string) => {
setFilter(next);
if (peerInfo && 'number' in peerInfo) {
onSelectPeer({ number: next });
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear empty number peers for transfer too

When the transfer modal has a number peer selected, clearing the autocomplete now re-selects { number: '' } here. Fresh evidence beyond the earlier call-flow fix is that TransferModal.confirm still treats any truthy peer with a number property as valid and calls onConfirm('sip', { id: peer.number, ... }), so a user can select/enter a SIP destination, clear the field, and submit an empty transfer target instead of seeing the required-field error. Consider clearing the peer or skipping this re-select when next.trim() is empty.

Useful? React with 👍 / 👎.

@ggazzo ggazzo added this to the POC-VOICE milestone Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add-to-poc type: feature Pull requests that introduces new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants