-
Notifications
You must be signed in to change notification settings - Fork 383
compose_box: Add start video call button in compose #1916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9f3ca6c to
e6c9afb
Compare
5ab42a9 to
b2bb501
Compare
|
Thanks. It looks like you still have a TODO item before this is ready to be reviewed, so marking it as draft for now. |
4cd6e12 to
576234b
Compare
|
All the TODO are done @gnprice! |
|
Thanks. Before this can be reviewed, you'll need to revise it to have clear and coherent commits. See the instructions we discussed on a previous PR thread: #1830 (review) |
e6835e8 to
8a417e9
Compare
|
Thanks @gnprice for suggesting the refinement in commit history to make sure it becomes minimal and coherent. The same has been implemented. |
|
Thanks. Some of these commits aren't coherent. For example this commit f314a74: can't be right, because it says it involved One useful step for spotting this kind of issue is to run our tests at every commit. If they fail, the commit needs fixing. It's straightforward to do that by hand. But a command which can be a useful shortcut for it is this: |
c3d1581 to
82d2840
Compare
|
Thank you @chrisbobbe for your patience, I have pushed a lot of new changes, PTAL! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a video call button to the compose box that allows users to insert video call links into their messages. The implementation supports multiple video chat providers (Jitsi, Zoom, and BigBlueButton) based on realm configuration.
Key changes:
- Adds a new video icon button to the compose box attachment toolbar
- Implements video call URL generation for Jitsi (default), Zoom (with OAuth), and BigBlueButton providers
- Adds event handling for Zoom token authentication flow
Reviewed changes
Copilot reviewed 14 out of 34 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/widgets/compose_box.dart | Core implementation including ComposeCall class and _AddComposeCallUrlButton widget with provider-specific logic |
| lib/widgets/icons.dart | Adds video icon constant |
| lib/model/store.dart | Adds hasZoomToken state management and HasZoomTokenEvent handling |
| lib/model/realm.dart | Adds video chat provider configuration fields |
| lib/api/route/video_call.dart | New API routes for creating Zoom and BigBlueButton calls |
| lib/api/model/model.dart | Adds RealmAvailableVideoChatProviders, VideoCallResponse, and RealmVideoChatProvider models |
| lib/api/model/events.dart | Adds HasZoomTokenEvent for OAuth flow |
| lib/api/model/initial_snapshot.dart | Adds video chat provider configuration fields to initial snapshot |
| lib/generated/l10n/*.dart | Adds localized strings for video/voice call tooltips and error messages |
| assets/l10n/app_en.arb | Source English localization strings |
| assets/icons/video.svg | Video icon SVG asset |
| assets/icons/ZulipIcons.ttf | Updated icon font with video icon |
| test/widgets/compose_box_test.dart | Basic test for Jitsi video call functionality |
| test/example_data.dart | Test fixtures for video chat provider configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| final connection = store.connection; | ||
| final result = await createBigBlueButtonCall( | ||
| connection, meetingName: "Null", //TODO: Fetch message stream title |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meeting name is hardcoded to "Null" which is likely not the intended value. The TODO comment indicates this should be the stream title, but implementing this is important for BigBlueButton calls to have meaningful meeting names.
| "@errorCouldNotEditMessageTitle": { | ||
| "description": "Error title when an exception prevented us from opening the compose box for editing a message." | ||
| }, | ||
| "errorCouldNotAppendCallUrl": "Fail to get call URL", |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message has incorrect grammar. It should read "Failed to get call URL" instead of "Fail to get call URL".
| "errorCouldNotAppendCallUrl": "Fail to get call URL", | |
| "errorCouldNotAppendCallUrl": "Failed to get call URL", |
82d2840 to
3827b62
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Here's a review of the API/model changes:
1c07e3f icons: Add 'video', from the Figma
585d056 api: Add realmVideoChatProvider to InitialSnapshot
ac3c1c0 api: Add realmAvailableVideoChatProviders to InitialSnapshot
2bd7232 api: Add hasZoomToken to InitialSnapshot
3e8e1de api: Add jitsi server url configurations to InitialSnapshot
340df70 realm: Add getters for realmAvailableVideoChatProviders
9c46324 realm: Add getters for jitsiServerUrl and realmVideoChatProvider
37a9d94 store: Add getter for hasZoomToken
0671977 api: Add routes to fetch zoom and bigBlueButton call url
f35502a api: Add event hasZoomToken
leaving the last commit for a later round of review:
3827b62 compose_box: Add Add video call button
lib/api/model/model.dart
Outdated
| /// For docs, search for "realm_video_chat_provider:" | ||
| /// in <https://zulip.com/api/register-queue>. | ||
| @JsonEnum(fieldRename: FieldRename.snake, valueField: "apiValue") | ||
| enum RealmVideoChatProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It seems like this definition should go after both RealmEmojiItem and UserStatus. Can we define these "helper" definitions in the same order as their corresponding fields in InitialSnapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put RealmVideoChatProvider just after UserSettings since that was the only correlation that i was able to make from register-queue api docs since InitialSnapshot follow that api docs order.
lib/api/model/initial_snapshot.dart
Outdated
|
|
||
| final String realmName; | ||
|
|
||
| final int realmVideoChatProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api: Add realmVideoChatProvider to InitialSnapshot
| final int realmVideoChatProvider; | |
| final RealmVideoChatProvider realmVideoChatProvider; |
(And rerun dart run build_runner build and edit test/example_data.dart)
lib/api/model/model.dart
Outdated
| jitsi(apiValue: 1), | ||
| zoomUser(apiValue: 3), | ||
| bigBlueButton(apiValue: 4), | ||
| zoomServer(apiValue: 5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some of these names, let's match the API documentation a bit more: jitsiMeet, zoomUserOAuth, zoomServerToServerOAuth.
lib/api/model/model.dart
Outdated
| factory RealmAvailableVideoChatProviders.fromJson(Map<String, dynamic> json) => | ||
| _$RealmAvailableVideoChatProvidersFromJson(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: two-space indent
lib/api/model/initial_snapshot.dart
Outdated
|
|
||
| final int realmVideoChatProvider; | ||
|
|
||
| final String? realmJitsiServerUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| final String? realmJitsiServerUrl; | |
| final String? realmJitsiServerUrl; // TODO(server-8) |
lib/api/route/video_call.dart
Outdated
| return connection.get('createBigBlueButtonCall', VideoCallResponse.fromJson, | ||
| '/calls/bigbluebutton/create', {'meeting_name': meetingName, 'voice_only': voiceOnly}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return connection.get('createBigBlueButtonCall', VideoCallResponse.fromJson, | |
| '/calls/bigbluebutton/create', {'meeting_name': meetingName, 'voice_only': voiceOnly}); | |
| return connection.get( | |
| 'createBigBlueButtonCall', VideoCallResponse.fromJson, '/calls/bigbluebutton/create', { | |
| 'meeting_name': RawParameter(meetingName), | |
| if (voiceOnly != null) 'voice_only': voiceOnly, | |
| }); |
lib/api/route/video_call.dart
Outdated
| /// GET /api/v1/calls/bigbluebutton/create | ||
| Future<VideoCallResponse> createBigBlueButtonCall(ApiConnection connection, { | ||
| required String meetingName, | ||
| required bool voiceOnly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API doc says this isn't required; let's make it optional:
| required bool voiceOnly, | |
| bool? voiceOnly |
lib/api/model/events.dart
Outdated
| case 'presence': return PresenceEvent.fromJson(json); | ||
| case 'reaction': return ReactionEvent.fromJson(json); | ||
| case 'heartbeat': return HeartbeatEvent.fromJson(json); | ||
| case 'has_zoom_token': return HasZoomTokenEvent.fromJson(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this just after case 'user_settings':, I think; similarly to the ordering choices in RealmStore. (And reorder the HasZoomTokenEvent class definition correspondingly.)
lib/model/store.dart
Outdated
| case HasZoomTokenEvent(): | ||
| assert(debugLog("server event: has_zoom_token")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this just after the case UserSettingsUpdateEvent():.
|
|
||
| /// A Zulip event of type `hasZoomToken`: https://zulip.com/api/get-events#has_zoom_token | ||
| @JsonSerializable(fieldRename: FieldRename.snake) | ||
| class HasZoomTokenEvent extends Event { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit-message nit:
api: Add event `hasZoomToken`
The event is named HasZoomTokenEvent in our code; hasZoomToken isn't a name we use for the event.
3827b62 to
cf770f7
Compare
|
Thank you @chrisbobbe for the Review, have pushed some new changes as well as a reply to your comment in review #1824(comment). PTAL. Regarding the test case I have open a discussion on CZO: #mobile-dev-help > compose call button test @ 💬 Edit: |
cf770f7 to
c01022c
Compare
c01022c to
121e1de
Compare
|
I've also added test for Zoom video call insertion in the final commit. PTAL! @chrisbobbe |
|
We do want to support all the expected platforms—Jitsi Meet, Big Blue Button, and the two Zoom configurations. It looks like the last commit is meant to add support for all of them all at once, though, and it feels kind of overwhelming to review all of them together. Would you make separate commits for supporting each platform, please, starting with the simplest, which I think is Jitsi? If I could see each change on its own, it would make my reviews more efficient and help us be more confident in the result. |
# Conflicts: # lib/api/model/initial_snapshot.g.dart # test/example_data.dart
…vider Rearranged `realmEnableReadReceipts` in proper order under class `RealmStoreImpl` which was out of order previously. # Conflicts: # lib/model/realm.dart
121e1de to
42665e3
Compare
|
Thank you @chrisbobbe for the patience and advice. I have expanded the final commit into several parts for each meeting application configuration. PTAL! Edit: Fixing the failing CI Test. |
Fixes #1824
Chat thread:
Design (Outdated)
Light Theme
XRecorder_20251015_04.mp4
Dark Theme
XRecorder_20251015_05.mp4