feat(video-conf): persistent-chat call flow, behind a new setting (3/4) - #41657
Open
rodrigok wants to merge 37 commits into
Open
feat(video-conf): persistent-chat call flow, behind a new setting (3/4)#41657rodrigok wants to merge 37 commits into
rodrigok wants to merge 37 commits into
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: ee6a162 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
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 |
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a disabled-by-default conference-window mode. It updates server ringing, client call routing, popup behavior, ongoing-call discovery, roomless calling, window lifecycle handling, persistent chat settings, tests, documentation, and translations. ChangesConference window and call state
Client routing and call surfaces
Feature support
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to With the new setting enabled, calls can select inconsistent ringing behavior, leave membership or notifications out of sync after partial failures, and fail without reliable client error handling. The feature remains opt-in and existing access controls are preserved, but these bounded correctness and reliability risks should be fixed or explicitly accepted before merge. Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant VideoConfProvider
participant VideoConfManager
participant VideoConferenceService
participant ConferenceWindow
Caller->>VideoConfProvider: Start call
VideoConfProvider->>ConferenceWindow: Open conference preflight
ConferenceWindow->>VideoConfManager: Join call
VideoConfManager->>VideoConferenceService: Create or join conference
VideoConferenceService-->>VideoConfManager: Ring or join state
VideoConfManager-->>VideoConfProvider: call/join
VideoConfProvider->>ConferenceWindow: Open or focus call window
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Errors 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/persistent-chat/conference-window #41657 +/- ##
==========================================================================
+ Coverage 69.88% 71.66% +1.78%
==========================================================================
Files 4327 2073 -2254
Lines 174961 126641 -48320
Branches 31128 18062 -13066
==========================================================================
- Hits 122265 90759 -31506
+ Misses 47577 35162 -12415
+ Partials 5119 720 -4399
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
rodrigok
force-pushed
the
feat/persistant-chat
branch
4 times, most recently
from
August 19, 2026 22:33
9d8f48f to
a1b8a88
Compare
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 21, 2026 01:29
9cfd23d to
912cdda
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 25, 2026 14:08
4917d51 to
ea0c48f
Compare
rodrigok
force-pushed
the
feat/persistant-chat
branch
3 times, most recently
from
August 25, 2026 19:42
67f0e14 to
3cdaf7a
Compare
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 26, 2026 17:11
9b1ef23 to
cbea4e5
Compare
rodrigok
changed the base branch from
feat/persistent-chat/server
to
feat/persistent-chat/conference-window
August 26, 2026 17:57
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 26, 2026 18:02
cbea4e5 to
3c2f007
Compare
rodrigok
changed the title
rodrigok
changed the title
rodrigok
commented
Aug 26, 2026
rodrigok
commented
Aug 26, 2026
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 26, 2026 20:03
898fd52 to
f2fc8e1
Compare
rodrigok
force-pushed
the
feat/persistant-chat
branch
from
August 26, 2026 20:11
f2fc8e1 to
605ff02
Compare
The feature doc predated the setting and described persistent chat as the switch for the whole experience. It now names `VideoConf_Conference_Window_Enabled`, says it is off by default, defines the "with/without the call window" wording it then uses throughout, and keeps `VideoConf_Enable_Persistent_Chat` to the one thing it still means. The changeset leads with the same, and the removed embedded-provider join path is recorded with the other follow-ups so the LiveKit work can find it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w is on The window opens for every provider, but ringing waited on the `embedded` capability that nothing declares yet — and with the window on the client no longer runs its own ring loop either, so a direct call rang nobody at all. The ring now waits for the caller's arrival whenever the call has a preflight to wait for: an embedded provider, or any provider with the window enabled. The callee becomes a member at creation on the same condition, since that is what gives the ring someone to reach and the missed call a history entry, and the creation-time push is held back so one call cannot buzz a phone twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Develop rewrote `useRoomList` around a `groups` array, so the flag lands differently: `Incoming_Calls` is a dynamic group, and reporting no incoming call while the window is on empties it — which is what leaves it out — with the room falling to whichever group it would otherwise be in. `RoomList.tsx` goes back to develop's version; it had no business in this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mode was registered a PR earlier, which changed what workspaces already running persistent chat get: a thread instead of the discussion per call they have today. It belongs here instead, next to the flag, because a thread off the call message is only meaningful inside the call window's chat panel. `getPersistentChatMode` answers `main_room` whenever the window is off, whatever the setting was left at, so turning the window off puts a workspace back exactly where it was rather than leaving it on a mode it can no longer see. The setting's `enableQuery` says the same in the admin UI.
…kspace An external provider opened with a live `window.opener` can navigate the tab the user came from — a login screen being the obvious thing to imitate. `noopener` in the features is the ordinary way to say that, but it makes `window.open` return null, and the handle is what watches the window for closing, which is how leaving a call gets reported. So the window is opened blank, cut loose while it is still same-origin and can be reached, and only then sent to the provider. Our own conference window keeps its opener: the conference page posts navigation requests back through it, and it is our origin at both ends. Also stops the caller's arrival pushing the callee twice. Ringing on arrival pushes them, and the same arrival flips the call to STARTED, which pushed everyone in the room — one call, two notifications, moments apart. Docs: the stage layout, the call bar and the MatrixRTC comparison describe the native provider, not this, and move with it. What is left names components that exist here.
…eb address The banner pointed a conference discussion back into its call, from before the navbar listed ongoing calls — which is now where a call is found, from anywhere, without a room-shaped special case. It cost a `video-conference.list` request per discussion opened and took the room's announcement slot to do it. Out entirely: `RoomBody` is byte-identical to develop again. Also refuses a call URL that isn't `http:` or `https:`. A `javascript:` or `data:` "URL" is not somewhere to go but something to run, and the window opened for an external provider is our own blank one until it navigates — so it would have run as this origin. No provider's call has ever been either scheme, so the check turns nothing legitimate away. The provider's address is otherwise passed through as given: `URL` normalises, and rewriting a provider's URL has bitten this code before.
…ll-back respects the flag `asCallUrl` resolved against this page, so anything relative took our origin and read as an in-product conference — a provider's own path, or the empty string a call with no URL arrives as, would have opened the workspace at some arbitrary route inside the call window. It now parses without a base, so only an absolute `http:`/`https:` address is accepted. Handing a relative one to a blank window would resolve it against this origin too, so refusing is the only honest answer; our own conference URLs are built with `absoluteUrl` and are unaffected. Calling back from a video-conference message block dispatched the outgoing popup unconditionally, so the "Calling…" popup the call window replaces came back through that one path. It now starts the call in the window, like every other way of placing one.
…e who is calling The user-card entry appeared for anyone once the call window was on, including a user with no username — and `im.create` speaks usernames, so clicking it ended in a warning toast rather than a call. It is offered only where a room can actually be created. An incoming call in a room the member can't see carries no title and no room, so the dialog was announced as "Incoming call from" and nothing at all, while naming the caller on screen right below it. The label falls back to the caller, named the way the body names them so the two agree. Also covers the provider-unavailable path in `useVideoCallRoomAction`, which is the reason capabilities are loaded before anything opens: a window opening on top of the answer is a worse way to learn the provider is misconfigured.
The sweep asked whether the provider was embedded, so a Jitsi call was outside it — even with the conference window on, where that call runs in a page of ours and renews its lease like any other. A window that died without reporting held its members present until the 24-hour TTL noticed, and nothing covered the gap between a preflight's join landing and the conference page installing its own watcher. Same fact the ring gate already needed, so it is one predicate now: `runsInOurCallWindow` — embedded provider, or the conference window, because the window is ours whoever runs the media. `ringsOnCallerArrival` was that fact named after one of its consequences; the three ring sites say what follows in their own comments instead. A call handed off to the provider's own page is still skipped: nothing of ours is there to renew anything, so every lease would look expired and the sweep would end the call after three minutes.
…call a refusal a blocked popup The arrival ring asked the setting a second time, so an admin toggling the call window between a call's creation and its caller walking in could decide differently than the creation did. It asks the call instead: a call that rang at creation never put its callee in `users` — they enter by answering — so there is nobody unasked for the arrival to reach, and the question answers itself. The test for that case built a callee who *was* a member, which is the shape of a call created with the window on. It now builds the call a creation-time ring actually leaves behind. A call URL we refuse — no address yet, or not a web address — was reported as a blocked popup, advice and all: allowing popups cannot make it open, and the modal's retry refused it again. Nothing opened, so nothing is claimed.
… ring only a ringing call Sweeping on the setting was unsafe. It says what a call opened *now* would do, and the sweep meets calls opened before it: one created while the window was off was handed to the provider's own page and never heartbeats — yet joining stamps `lastSeenAt` for every provider, so its members' leases expire three minutes after an admin flips the setting, and a call still running in Jitsi is ended under the people in it. Back to the provider's capability, with the reason written down. The gap it leaves — a URL provider's call held in our window waits on the 24-hour TTL — needs the call to carry how it was opened, not the sweep guessing from a setting that can have changed since. The arrival ring now also requires the call to still be `CALLING`. Asking only whether a member is unasked rang someone added to a call already under way with `ring: false`, when the creator turned up again. A call waiting for its caller is the only one still `CALLING`, so the call's own state separates the two. The fixtures for that ring said `STARTED`, which such a call never is at that moment; they now say what `startDirect` actually leaves behind.
70 manual cases under `docs/features/video-conference-persistent-chat/qa/`, importable as Qase CSV V1 (one row per case, numbered steps in `steps_actions`/`steps_result`, suites declared before the cases). Split along `VideoConf_Conference_Window_Enabled` the way the code is, since half of what this PR has to prove is that the setting off changes nothing: a regression suite for the room popups, the 1:1 ring handshake, the sidebar Incoming Calls group and the absence of any joinable polling; then the flag-on flows, persistent chat's two modes, permissions, security and the handful of things only a person can check. Every expected result traces to something on this branch rather than to the feature's design notes, so the qa README also records where that document is ahead of the code — there is no ongoing-calls sidebar group, a call row has no accept button, no panel is open by default — and what deliberately has no cases: the embedded provider that arrives with the native one, and the three behaviours gated on it (busy presence, the presence sweep, server-side single-call enforcement). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grows `video-conference-call-window.spec.ts` from the one caller-side test on the branch to thirteen, covering the flag-on journeys that only make sense with two browser contexts and a window of their own: placing a call and cancelling it, confirming the preflight, naming a group conference, the ring waiting for the caller's arrival, answering through the join preflight, declining from the row and joining anyway, the standalone layout and its one panel slot, one call window reused rather than stacked, adding participants without touching a room, and the conference route refusing a call URL that is not a web address. A second describe pins the flag-off side of the promise: no ongoing-calls button and no `video-conference.joinable` polling at all. Two page objects rather than inline selectors: `ConferenceWindow` for the separate window (preflight, top bar, the members and chat panels, the add-people modal, the state pages), and an `OngoingCalls` fragment for the navbar button and its dropdown, hung off `HomeChannel` beside the other fragments. Written against the Qase set in `docs/features/video-conference-persistent-chat/qa/`, whose README now records which case ids are automated, why the timed ones are left to a person, and three places where a case's expected result does not match the code on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite was written against text, Fuselage's internal `data-testid` and a
`.last()` on an ambiguous landmark, because there was nothing else to hold on to.
`572b7397` gave the components the roles and names they were missing, so the
selectors can say what they mean:
- the call row is a `link` named after the call, so the ongoing-calls rows are
addressed as rows rather than as their title text;
- the dropdown is a `region` named `Ongoing calls`, replacing `getByTestId('dropdown')`;
- the preflight heading is an `h2`, so it is a `heading` rather than loose text;
- the call timer is a `timer`, so what is asserted is a clock reading a time
rather than the whole bar containing something time-shaped;
- the members panel is a named `list`, which is what scopes a member's name and
status to the panel instead of to the window;
- the chat panel's title has one accessible name — `Chat in <room>` — instead of
an icon in the middle of it, so the two `toContainText` halves become one
lookup by name;
- the add-people picker is labelled, so it is not the modal's only combobox by
luck;
- the chat-access notice is a `status`, leaving the call's own header the only
`banner`, so `topBar` drops its `.last()`.
Left alone deliberately: `findCallAnywhere` stays a page-wide text lookup,
because it exists for `toHaveCount(0)` and "this name appears nowhere" is a
stronger thing to prove than "no row is named that"; and the ringing mark and the
joined count stay scoped to the list rather than to their row, which the row's
name would now allow, because that would prove more than the tests set out to.
Unrun: this environment has no workspace and no EE licence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`getByRole('banner')` was unqualified because there was nothing to qualify it
with. Now that the call's header is named, matching on the name is what keeps
the locator correct while a modal — whose own header is also a banner — is open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tests in `video-conference-call-window.spec.ts` were written without ever being run, and CI run 33428535519 found three separate reasons for it. The Playwright DOM snapshots and network logs from that run are what each of the changes below is based on. **A wrong premise, fixed in the test.** `should honour the ring choice on a direct call` assumed the callee is a member who was simply never rung. They are not a member at all: both ringing types in `ee/server/configuration/videoConference.ts` require `allowRinging`, so unchecking *Ring people* makes `getTypeForRoom` fall through to a plain group conference even in a two-person DM — the trace has `video-conference.start` answering `type: "videoconference"` — and `startDirect`, which is the only thing that puts a callee on the roster before they answer, never runs. The test now proves what the choice actually did: the roster is the caller alone, and the callee, sitting in the room with the call's message in front of them, is never rung. **Two product bugs, parked rather than papered over.** The call window never learns what anyone else does to the call. It reads membership from `video-conference.info` and refreshes it only from the `video-conference/<callId>/updated` stream, which does not reach it: a callee joined and five seconds later the caller's window had issued no request at all and still read `Ringing`, and a decline the server wrote — and the caller's own main app page read back 1.4s later over `notify-room` — never moved the caller's call window, whose timer was still ticking in the same snapshot. So the event fires and the user is reachable. The caller-side steps move into a `test.fixme` that names the bug; what each test can still prove on its own side stays. With persistent chat off, the chat panel opens a thread instead of the room. `useConferenceEmbedded` reads `VideoConf_Persistent_Chat_Mode` alone — default `thread` — and checks neither `VideoConf_Enable_Persistent_Chat` nor the provider's `persistentChat` capability, both of which the server does check. The panel comes up headed `Thread in <channel>`. The layout case now identifies the panel by its composer, which is the claim it was making, and the title has a `test.fixme` of its own. Qase case 13 is corrected to what the code does, and the QA README gains a section for the two bugs and the cases parked on them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The conference window's only word on what a call is doing is one `video-conference/<callId>/updated` subscription, and the server refuses a subscription it cannot authorise rather than queueing it. Neither side reports the refusal and nothing asks again, so a window that subscribes at the wrong moment sits on whatever it first read — the caller's window still saying "Ringing" after the callee has joined or declined, with no request going out. Two ways it subscribes at the wrong moment, both closed here: - The window opens on `/conference/new`, before the conference exists, and `new` is a call id this page can be handed (`ConferenceRoute` falls through to it whenever the `rid` the start screen needs is missing). `allowRead` looks the conference up by that id, finds nothing and refuses. It now waits for a real id, and subscribes as soon as it has one. - A subscription is only good while the connection under it is. One that was refused, or lost with the socket, left the window watching nothing in silence. It now subscribes per connection and re-reads the conference when a connection is re-established, since whatever moved while this window was away was announced to nobody here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`VideoConf_Persistent_Chat_Mode` says which of the two homes a workspace wants its call chat in; it does not say where a given call's chat is. The server takes three answers before it will thread one: persistent chat enabled, the mode, and a provider that declares the `persistentChat` capability — `autoFollowCallThread` refuses on any of them, as `maybeCreateDiscussion` does for the other mode. The call window read the mode alone. Its registered default is `thread`, so with the window on, every call opened its chat as a thread — including calls whose provider does no persistent chat and workspaces with the feature off. What that looks like is a panel titled "Thread in <channel>" over a thread nobody is subscribed to, with `capabilities.persistentChat: false` in the very response the panel was built from. `info.capabilities` was already in the payload the hook reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…agrees with the server The panel is titled after the room again when persistent chat is off, so the case that caught it can run. The other `fixme` stays: two holes in the window's subscription are closed, but the run recorded no WebSocket frames, so nothing proves either of them is what dropped it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Joining was announced only for embedded providers, and the list of calls to join is how the whole window flow is reached. So with a provider that hands the call to its own page, nothing told the app behind the window that its user had arrived anywhere: their own other sessions went on believing they were free, and the guard that asks before swapping one call for another — the guard whose whole purpose is that a user is in one call at a time — never fired, because the list it reads still said they were in nothing. The room's members waited out the same 20-second poll to learn there was a call worth joining. A call handed to a provider's page is still held in a window of ours, so the arrival is announced whenever the window is what runs it, whatever runs the media. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`app/utils/client/lib/SDKClient` is now `client/lib/SDKClient` (#41994). The manager's own import moved with it in that refactor; its spec — which is new here — still named the old path, in the import and in the `jest.mock` beside it. A `jest.mock` on a path that resolves to nothing mocks nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**A scripted `?callUrl=` was opened.** `/conference?callUrl=…` takes its address from whoever wrote the link and hands it to `window.open`. The window flow guards this with `asCallUrl`; the path without the window did not, so a `javascript:` or `data:` "URL" — not somewhere to go, but something to run — ran in a window we opened. Refused now, resolved against this origin so a provider that answers with a relative address still passes: turning those away is a different question, and would break calls that work today. **The enhanced-navigation row still offered accept/reject.** Its twin in the sidebar drops them when the call window is on, because a ringing call is answered from the list of running calls instead. Nothing renders this row yet, which is exactly how it would have shipped ungated. **An arrival and a ring told the conference but not the room.** Declining, adding and ending all announce both; joining and ringing announced only the conference, so the call's own block in the room went on showing the roster from before someone arrived or was rung. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s new shape The chat-panel case reads "when persistent chat is off" and never said so: the suite shares one workspace, and off being the registered default is not the same as off when the test runs. Set explicitly, so a setting someone else moved fails as itself rather than as the bug this case covers. The People panel is now a group of two lists — the dividers between them are not list items — so the page object scopes to the group. Dialog locators match names exactly, as the e2e guide asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o is asking Driven against a live workspace, the theory this was `fixme`'d on turns out to be wrong: the call window subscribes, the `<callId>/updated` frame arrives, and the conference is read again. The stream works. What the CI traces show is a window that subscribed once and heard nothing after — which is what a *refused* subscription looks like. `allowRead` resolves the user from the connection and refuses when there is none, and a refusal is never retried: the stream is dropped from the cache and the consumer is left listening to nothing until its effect happens to run again. A call window opens fresh and authenticates as it loads, so asking before the session is restored is its normal condition rather than an edge case. So the subscription waits for the user id, as it already waits for a real call id and a live connection. The e2e case that caught this runs again rather than staying `fixme`'d on a diagnosis that no longer holds — if something else is also wrong, that is the fastest way to find out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ither way Two CI failures, both mine. The `?callUrl=` guard rendered an error screen where the page had always sent the user home once the call was out of its hands — a new answer to a question that already had one — and it accepted a *relative* address, which is not a call either: `?callUrl=/admin/settings` would have opened the workspace itself in a call window. It now parses with no base, like `asCallUrl` does, and refuses anything that is not absolute `http(s)` while still going home. The caller-follows-the-call case goes back to `fixme`, with what is now known written down: the stream works (proved against a live workspace), it is the call window specifically that does not react — the main app refetches 14ms after a decline, the window never does — and waiting for the user id was not the cause. What differs is how the window got there, `/conference/new` first, and the run recorded no WebSocket frames to see the DDP side of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The caller-follows-the-call case runs again, instrumented: the window records its DDP frames and attaches them when the assertion fails. A window that was refused its subscription, one that never asked for it, and one that asked and then heard nothing look identical from the outside, and traces carry no WebSocket frames — so the test keeps them itself. Driven against a live workspace on this branch, with the window setting on and LiveKit as the provider, the chain works end to end: the window subscribes and is ready before and after joining, an add made from another session delivers `<callId>/updated`, and the conference is read again each time. What is still different about CI is the provider — the tester app answers with a *relative* join URL, so the window renders an iframe that loads the client a second time inside itself, which an embedded provider never does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The window read the call and *then* subscribed, and the gap between the two is not empty: subscribing is a round trip. Anything announced while it was in flight reached nobody here, and a conference event is announced once — so the window went on showing the call as it was, waiting for a next event that might never come. Its own DDP frames say so. From the run that failed: the `sub` for `<callId>/updated` went out two seconds after the call had been read, the callee declined in between, and the window kept the pre-decline roster for the rest of the test. The subscription was fine; there was simply nothing listening yet when the one event went past. So the call is read again once the window is watching, once per subscription — after the first read settles, because invalidating a read still in flight achieves nothing: the fetch already running is the one that returns, stale answer and all. That last part is why the first version of this fix did not work, and the test now covers it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tream The last capture filtered on the stream name, which hid the answer: a `ready` and a `nosub` carry only the subscription's id, and those two are the whole difference between a subscription the server refused and one it accepted before going silent. Frames are capped at 400 characters each, so keeping all of them costs little and cannot leave out the line that matters. What the previous run did establish, written into the test: the window *does* subscribe — the `sub` goes out on a socket that has already received two `notify-user` events, so it is alive and authenticated — and nothing for that stream ever comes back. The catch-up read added alongside it cannot help here either, since it covers what changed during the subscribe round trip while the decline lands a couple of seconds after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The route option that pins it is the one link in that chain no unit test reaches, and what it guards against is a colour, so the window's own case now weighs two of its palette tokens: light text over a dark surface, which is what a light theme inverted. Weighed as luminance rather than compared to hex values, so tuning the palette doesn't fail the case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Proposed changes (including videos or screenshots)
The behavioural half: the part that rewires the existing call flow rather than adding alongside it. Where #41956 was unmounted by construction, everything here is reachable — so it ships dark.
One new workspace setting,
VideoConf_Conference_Window_Enabled(defaultfalse), gates every change in this PR. With it off, the client behaves exactly as it does on the base branch. Turning it on switches the workspace to the new call experience.The important consequence:
VideoConf_Enable_Persistent_Chatkeeps meaning only what it means today — a discussion per call. Workspaces already using persistent chat see no change until they opt into the new flag as well. Several of these behaviours were gated on that older setting on the source branch, which would have flipped those workspaces silently; they are moved onto the new flag here.A second setting,
VideoConf_Persistent_Chat_Mode(thread|main_room), decides where a call's chat lives once the window is on. With the window off the server forcesmain_room, which is what it does today.What turning the flag on changes
With the flag off: no popups suppressed, no popout window, no window watcher, no navbar item, no
video-conference.joinablepolling, and the sidebar group and its row actions stay exactly as they are today.Security
Opening a provider URL in a window of our own means handling a URL that came from a provider record.
asCallUrlaccepts only an absolutehttp(s)URL — parsed with no base, so a relative string cannot be resolved against our origin andjavascript:/data:cannot get through — and the window is opened blank withopenercleared before navigating, so the provider's page has no handle on ours.45 files, +3,354 / −147. Includes the feature's documentation (
docs/features/video-conference-persistent-chat/) and a second e2e spec that drives the flag-on flow.Issue(s)
https://rocketchat.atlassian.net/browse/NV-64
Steps to test or reproduce
Flag off (the default — this is the important half):
develop.video-conference.joinable.Flag on:
VideoConf_Persistent_Chat_Modevalues and check where the call's chat lands.Further comments
video-conference-ring.spec.tsis unchanged and proves the flag-off path;video-conference-call-window.spec.tsenables the setting and drives the new flow.enableQuerytying the new setting toVideoConf_Enable_Persistent_Chat. The dependency is not real — with persistent chat off, the conference chat falls back to the call's own room — and chaining them would both hide the new experience behind an unrelated opt-in and change what persistent-chat workspaces see, which is the thing this PR must not do.useJoinableCallsreads the setting itself rather than taking anenabledprop, so no future caller can reintroduce the 20-second poll by forgetting to pass it.TimedVideoConfPopup'shasPopupis not gated symmetrically. Preserving the flag-off path byte-for-byte would have preserved a latent throw (focusFirston an empty scope), reachable before this work whenever an incoming call arrived for a room minimongo did not yet have. A flag-off test hit it, so it is fixed rather than reproduced.isCalling/ring), the sidebar group, the open-call hook, the joinable-calls query and the popups, with mutation checks confirming each gate's test fails when the gate is removed.packages/jwt's HS256 helpers and the unconsumedMediaCallViewContextsurface are deliberately not here — they have no callers until the LiveKit provider (feat(video-conf): LiveKit as a native video conference provider (4/4) #42000), and travel with it.Summary by CodeRabbit
New Features
Documentation