Open
Conversation
Ovgodd
force-pushed
the
feat/pip-support
branch
4 times, most recently
from
d071c97 to
851ca16
Compare
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
WalkthroughThis pull request introduces a Picture-in-Picture (PiP) feature for video conferencing. It adds a new React-based PiP infrastructure including a document PiP portal component, PiP-specific UI components (control bar, view, lateral menu, options menu), hooks (useDocumentPiP, useRoomPiP), and context management (RoomPiPProvider, RoomPiPContext). Supporting changes include TypeScript type definitions for react-aria overlays, portal-aware hook utilities, modifications to existing components (Menu, Popover, TooltipWrapper, OptionsButton, VideoConference) to adapt to PiP contexts, and localization strings in four languages. The feature integrates with the browser's documentPictureInPicture API. Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/primitives/TooltipWrapper.tsx (1)
9-49: PiP tooltips ignoretooltipTypedelay.In the external-document branch,
VisualOnlyTooltip(seesrc/frontend/src/primitives/VisualOnlyTooltip.tsx, around lines 22-40) shows immediately on hover/focus, sotooltipType="delayed"no longer delays in PiP. If the delay is meant to stay consistent, pass it through and implement the delay inVisualOnlyTooltip.Suggested wiring change (requires adding a delay prop to VisualOnlyTooltip)
- isExternalDocument ? ( - <VisualOnlyTooltip tooltip={tooltip}>{children}</VisualOnlyTooltip> - ) : ( + isExternalDocument ? ( + <VisualOnlyTooltip + tooltip={tooltip} + delayMs={ + tooltipType === 'instant' + ? INSTANT_TOOLTIP_DELAY_MS + : DELAYED_TOOLTIP_DELAY_MS + } + > + {children} + </VisualOnlyTooltip> + ) : (
🤖 Fix all issues with AI agents
In `@src/frontend/`@types/react-aria-overlays.d.ts:
- Around line 1-4: Add declarations for the missing types in the module
augmentation: declare PortalProviderContextValue as an object with getContainer:
() => HTMLElement | null and PortalProviderProps as an object with getContainer:
() => HTMLElement | null and children: React.ReactNode; keep the existing
exported symbols useUNSAFE_PortalContext(): PortalProviderContextValue and
UNSAFE_PortalProvider: (props: PortalProviderProps) => JSX.Element so imports
like useOverlayPortalContainer compile.
In
`@src/frontend/src/features/rooms/livekit/components/controls/Options/OptionsButton.tsx`:
- Around line 16-33: The outside-click pointerdown handler inside the useEffect
(referencing wrapperRef, isInPiP, isOpen) duplicates PipOptionsMenu's click
handling and can close the menu before PipOptionsMenu's item actions run; remove
this useEffect block (or disable its registration when PipOptionsMenu is
present) so outside-click logic is consolidated in PipOptionsMenu, ensuring only
PipOptionsMenu handles closing on clicks and avoiding conflicting pointerdown vs
click ordering.
In
`@src/frontend/src/features/rooms/livekit/components/controls/Options/PipOptionsMenu.tsx`:
- Around line 59-68: The menu needs Escape-key dismissal: inside the
PipOptionsMenu component's existing effect (where isOpen and setIsOpen are
used), add a keydown listener that, when isOpen is true and event.key ===
'Escape', calls setIsOpen(false); ensure the listener is registered only while
the menu is open and always cleaned up in the effect's cleanup to remove the
handler; reference the Button trigger (id "room-options-trigger") and the
isOpen/setIsOpen state so the handler closes the same menu instance.
- Around line 25-50: The effect currently only closes the menu when a menu item
is clicked; update the click handler in the useEffect (handleMenuItemClick) to
also close the menu when the click is outside the menu wrapper: if
wrapperRef.current exists and neither wrapper.contains(target) nor
wrapper.querySelector('button')?.contains(target) are true then call
setIsOpen(false) (use requestAnimationFrame if you want the same timing
behavior), keeping the existing doc.addEventListener(..., true) and the cleanup
removeEventListener so outside clicks on the PiP background will close the menu.
In `@src/frontend/src/features/rooms/livekit/components/DocumentPiPPortal.tsx`:
- Around line 39-58: syncCssVariables is scanning every computed style property
each time (in applyVarsFrom) which is expensive; update it to cache CSS variable
names per source Document (keyed by source.documentElement or source.baseURI)
and reuse that cache on subsequent PiP opens, and only read values via
source.defaultView.getComputedStyle for those cached variable names and set them
on target.documentElement; reference the syncCssVariables function and its inner
applyVarsFrom to implement the cache lookup/population and the reduced per-open
loop so you avoid iterating all computed style properties on each call.
- Around line 126-140: DocumentPiPPortal and useDocumentPiP both register
pagehide/beforeunload on pipWindow causing duplicate handlers; change
useDocumentPiP to accept an optional onClose callback and move the
pagehide/beforeunload registration there (using pipWindow from the hook), then
remove the useEffect that registers those events from DocumentPiPPortal (and let
the hook call onClose which should reset containerRef.current,
setContainer(null) and propagate the original onClose). Ensure you update the
hook signature (useDocumentPiP) and all call sites in DocumentPiPPortal to pass
the existing onClose prop and remove the duplicate event listener effect that
references pipWindow, containerRef, setContainer.
In `@src/frontend/src/features/rooms/livekit/components/PipControlBar.tsx`:
- Around line 65-73: PipControlsRight's absolute positioning with a fixed right
offset (right: '1.35rem') can overlap centered controls in PipControlsCenter on
narrow PiP windows; update the layout so controls don't collide by adding a
minimum width or switching to a responsive flex layout: modify the styled
component PipControlsRight (and/or the parent PipControls container) to either
enforce a minWidth (e.g., minWidth on PipControlsRight or min-width on
PipControls container) or remove absolute positioning and use flexbox with
justify-content: space-between / gap or media query breakpoints to change
positioning when the container becomes too narrow, ensuring PipControlsCenter
remains centered and elements don't overlap.
In `@src/frontend/src/features/rooms/livekit/components/PipView.tsx`:
- Around line 31-44: The early-return logic in PipView can look asymmetric
because when pickTrackForPip(tracks) returns undefined but hasMultipleTiles is
true the component will render the grid (handled by GridLayout) even if tracks
are placeholders; add a short inline comment in PipView next to the if
(!trackRef && !hasMultipleTiles) return null explaining this intentional
behavior — that placeholder-only scenarios are expected to fall through to the
GridLayout path and that pickTrackForPip may return a placeholder or undefined
while GridLayout will render placeholders correctly.
In `@src/frontend/src/features/rooms/livekit/hooks/useDocumentPiP.ts`:
- Around line 28-39: The openPiP function currently awaits pip.requestWindow
without handling rejections; wrap the call in a try-catch so any errors from
pip.requestWindow are caught, avoid setting pipWindow on failure, and return
null on error. Specifically, in useDocumentPiP's openPiP, surround the await
pip.requestWindow({ width, height }) with try-catch, call setPipWindow only on
success, and handle/log the caught error (e.g., with console.error or existing
logger) before returning null so no unhandled promise rejection occurs.
- Around line 28-39: openPiP can race when called multiple times before
requestWindow resolves; introduce a ref (e.g., pendingPiPRef) to store the
in-flight Promise and use it to short-circuit subsequent calls, update
setPipWindow only once the Promise resolves, clear the ref on resolve/reject,
and also re-check pipWindow (and pipWindow.closed) after await to avoid creating
duplicate windows; update references in the openPiP callback (and any cleanup)
to use pendingPiPRef alongside the existing pipWindow and setPipWindow logic.
- Around line 23-26: The useMemo-based detection of documentPictureInPicture in
useDocumentPiP recalculates on remount; replace the useMemo call that defines
isSupported with a more idiomatic SSR-safe pattern (either a module-level
constant or a useState with a lazy initializer) so the window check (typeof
window === 'undefined') runs only once and the result is stable for the hook
lifecycle; locate the isSupported declaration inside the useDocumentPiP hook and
change it to useState(() => typeof window !== 'undefined' &&
'documentPictureInPicture' in window) or move that evaluation to a top-level
constant and have the hook read that constant.
In `@src/frontend/src/features/rooms/livekit/prefabs/VideoConference.tsx`:
- Around line 200-208: The inline style block in VideoConference.tsx uses
hardcoded "magic" values (358px, 80px, 16px, 3rem) tied to isSidePanelOpen;
extract these into named constants or CSS variables and use them instead to
avoid layout coupling: add descriptive CSS variables (e.g.,
--lk-side-panel-width, --lk-top-offset, --lk-right-offset, --lk-side-panel-gap)
in the module or global stylesheet and replace the numeric literals in the style
inset expressions and transition comment reference the style object in
VideoConference.tsx so the component reads those variables (or imports
constants) rather than hardcoding pixels.
In `@src/frontend/src/locales/nl/rooms.json`:
- Around line 235-238: The Dutch locale still has English PiP labels; update the
pictureInPicture translations by replacing the values for the keys
"pictureInPicture.enter" and "pictureInPicture.exit" with Dutch text (e.g.,
"Beeld-in-beeld" and "Beeld-in-beeld sluiten" or preferred Dutch phrasing),
ensuring the JSON structure and quoting remain valid.
In `@src/frontend/src/primitives/useOverlayPortalContainer.tsx`:
- Around line 18-24: The function useOverlayBoundaryElement contains a redundant
conditional returning portalContainer or undefined; simplify it by returning
portalContainer directly (or portalContainer ?? undefined) instead of the if
block — update the return inside useMemo in useOverlayBoundaryElement which
calls useOverlayPortalContainer to just return portalContainer and keep the
dependency array [portalContainer].
WalkthroughThis pull request introduces Picture-in-Picture (PiP) functionality to a LiveKit-based video conferencing frontend. The implementation includes a new document PiP portal system (DocumentPiPPortal) that manages rendering content into a separate PiP window with synchronized styling, cross-document theme and CSS variable propagation, and lifecycle management. New React hooks (useDocumentPiP, useRoomPiP, RoomPiPProvider) provide PiP window and state management at the room level. Multiple UI components render PiP-specific controls (PipControlBar, PipLateralMenu, PictureInPictureMenuItem, PipOptionsMenu) and content (PipView). Core primitive components (Menu, Popover, TooltipWrapper, VisualOnlyTooltip) are updated to detect and adapt behavior for PiP contexts using portal container awareness. Localization entries for en, de, fr, and nl add PiP feature labels. TypeScript declarations for Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/frontend/`@types/react-aria-overlays.d.ts:
- Around line 1-4: The ambient module declaration for '@react-aria/overlays'
declares unresolved types (PortalProviderContextValue, PortalProviderProps) and
unused exports (useUNSAFE_PortalContext, UNSAFE_PortalProvider); remove this
dead declaration file entirely to prevent TypeScript compilation errors and
because the package and symbols are not used or present in dependencies.
In
`@src/frontend/src/features/rooms/livekit/components/controls/Options/PipOptionsMenu.tsx`:
- Around line 24-43: The effect in PipOptionsMenu.tsx currently only closes the
menu when a menu item is clicked; extend the handleMenuItemClick (or rename to
handleDocumentClick) to also detect outside clicks by checking if the click
target is not contained within wrapperRef.current (and not the trigger button)
and call setIsOpen(false) via requestAnimationFrame for consistency; ensure the
listener is still attached to doc (ownerDocument) and cleaned up in the effect
return so outside clicks close the PiP options menu as documented.
In
`@src/frontend/src/features/rooms/livekit/components/controls/PipLateralMenu.tsx`:
- Around line 33-34: The toggle handler uses the current isOpen value directly
which can lead to stale state; update handlePress to use a functional state
updater: call setIsOpen(prev => !prev) instead of setIsOpen(!isOpen), leaving
handleClose as is; update the function referenced as handlePress in
PipLateralMenu.tsx to use the functional form to ensure correctness under
concurrent/batched updates.
- Around line 49-60: The Dialog in PipLateralMenu lacks an accessible name;
update the Dialog usage in the PipLateralMenu component to include an aria-label
(for example "PIP lateral menu" or "Picture-in-picture controls") so screen
readers can identify the dialog context; add the aria-label prop directly on the
Dialog element in PipLateralMenu to provide a clear, concise label (e.g.,
aria-label="PIP lateral menu").
In `@src/frontend/src/features/rooms/livekit/hooks/RoomPiPProvider.tsx`:
- Around line 1-13: Import the ReactNode type explicitly and use it in the
RoomPiPProvider props instead of referencing React.ReactNode; specifically add
an import like "import type { ReactNode } from 'react'" at the top and change
the props annotation from "children: React.ReactNode" to "children: ReactNode"
in the RoomPiPProvider declaration to avoid relying on the global React
namespace.
In `@src/frontend/src/features/rooms/livekit/hooks/useDocumentPiP.ts`:
- Around line 28-38: The openPiP function may throw if pip.requestWindow
rejects; update useDocumentPiP.openPiP to wrap the await pip.requestWindow(...)
call in a try/catch, catch any error from requestWindow, optionally log it
(e.g., via console.warn or existing logger), ensure setPipWindow is only called
on success and return null on failure so callers (like DocumentPiPPortal) don't
receive an unhandled rejection; reference the openPiP async function, the
pip.requestWindow call, and the setPipWindow state updater when making the
change.
In `@src/frontend/src/locales/nl/rooms.json`:
- Around line 235-238: Replace the English strings under the JSON object
"pictureInPicture" (keys "enter" and "exit") with native Dutch phrasing: set
"enter" to "Beeld-in-beeld" and "exit" to "Beeld-in-beeld sluiten" (or consult a
native speaker for preferred variant), keeping the same keys unchanged.
In `@src/frontend/src/primitives/TooltipWrapper.tsx`:
- Around line 32-49: TooltipWrapper currently bypasses tooltipType delays for
external-document tooltips because VisualOnlyTooltip is rendered without a
delay; compute the effective delay (use tooltipType to choose between
INSTANT_TOOLTIP_DELAY_MS and DELAYED_TOOLTIP_DELAY_MS) in TooltipWrapper and
pass it as a prop (e.g., delayMs) into VisualOnlyTooltip so it can honor the
same timing as the TooltipTrigger path; update VisualOnlyTooltip to accept and
apply delayMs. Reference: TooltipWrapper (useOverlayPortalContainer,
tooltipType, INSTANT_TOOLTIP_DELAY_MS, DELAYED_TOOLTIP_DELAY_MS) and
VisualOnlyTooltip.
Comment on lines
+33
to
+34
| const handlePress = () => setIsOpen(!isOpen) | ||
| const handleClose = () => setIsOpen(false) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a functional state update when toggling the menu.
This avoids stale state under concurrent/batched updates.
Suggested change
- const handlePress = () => setIsOpen(!isOpen)
+ const handlePress = () => setIsOpen((open) => !open)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handlePress = () => setIsOpen(!isOpen) | |
| const handleClose = () => setIsOpen(false) | |
| const handlePress = () => setIsOpen((open) => !open) | |
| const handleClose = () => setIsOpen(false) |
🤖 Prompt for AI Agents
In
`@src/frontend/src/features/rooms/livekit/components/controls/PipLateralMenu.tsx`
around lines 33 - 34, The toggle handler uses the current isOpen value directly
which can lead to stale state; update handlePress to use a functional state
updater: call setIsOpen(prev => !prev) instead of setIsOpen(!isOpen), leaving
handleClose as is; update the function referenced as handlePress in
PipLateralMenu.tsx to use the functional form to ensure correctness under
concurrent/batched updates.
Comment on lines
+1
to
+13
| import { useCallback, useMemo, useState } from 'react' | ||
| import { RoomPiPContext } from './roomPiPContext' | ||
|
|
||
| /** | ||
| * Context Provider that manages Picture-in-Picture state at the room level. | ||
| * Handles open/closed state, browser support detection, and exposes open/close/toggle functions. | ||
| * Components access PiP state via the useRoomPiP hook. | ||
| */ | ||
| export const RoomPiPProvider = ({ | ||
| children, | ||
| }: { | ||
| children: React.ReactNode | ||
| }) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer explicit ReactNode import for types.
Avoid relying on a global React namespace, which is TS-config dependent.
♻️ Suggested refactor
-import { useCallback, useMemo, useState } from 'react'
+import { useCallback, useMemo, useState } from 'react'
+import type { ReactNode } from 'react'
@@
}: {
- children: React.ReactNode
+ children: ReactNode
}) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { useCallback, useMemo, useState } from 'react' | |
| import { RoomPiPContext } from './roomPiPContext' | |
| /** | |
| * Context Provider that manages Picture-in-Picture state at the room level. | |
| * Handles open/closed state, browser support detection, and exposes open/close/toggle functions. | |
| * Components access PiP state via the useRoomPiP hook. | |
| */ | |
| export const RoomPiPProvider = ({ | |
| children, | |
| }: { | |
| children: React.ReactNode | |
| }) => { | |
| import { useCallback, useMemo, useState } from 'react' | |
| import type { ReactNode } from 'react' | |
| import { RoomPiPContext } from './roomPiPContext' | |
| /** | |
| * Context Provider that manages Picture-in-Picture state at the room level. | |
| * Handles open/closed state, browser support detection, and exposes open/close/toggle functions. | |
| * Components access PiP state via the useRoomPiP hook. | |
| */ | |
| export const RoomPiPProvider = ({ | |
| children, | |
| }: { | |
| children: ReactNode | |
| }) => { |
🤖 Prompt for AI Agents
In `@src/frontend/src/features/rooms/livekit/hooks/RoomPiPProvider.tsx` around
lines 1 - 13, Import the ReactNode type explicitly and use it in the
RoomPiPProvider props instead of referencing React.ReactNode; specifically add
an import like "import type { ReactNode } from 'react'" at the top and change
the props annotation from "children: React.ReactNode" to "children: ReactNode"
in the RoomPiPProvider declaration to avoid relying on the global React
namespace.
2f81be0 to
67dd60c
Compare
|
utility hook to cleanly open/close the pip window
mount react content into pip window with styles
connecting pip and room
add labels for PiP in each supported language
prevent black screen after closing and reopening pip
avoid layout jumps and prepare for more buttons
route menus/tooltips to the pip document
keep pip options layout reusable and isolated
centralize pip controls in reusable component
close options menu when selecting transcription or
add jsdoc comments to pip components and hooks
Prevent PiP actions from affecting main panel state and reduce PiP-only noise.
Show a main tile with a bottom-right thumbnail in PiP, like Google Meet.
Use PiP document ResizeObserver so controls hide into "..." while resizing.
PiP-only reactions strip, store flag, and overflow menu wired to PiP state.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
1cd8f0b to
40da9c0
Compare
Reuse a single ResizeObserver hook in the PiP control bar.
Render all participants with a focus mode and animated adaptive grid.
Start PiP in a taller shape closer to a typical video call layout.
Ovgodd
force-pushed
the
feat/pip-support
branch
3 times, most recently
from
77f7726 to
28dd02c
Compare
Labels for PiP window, stage, toolbar, reactions, and SR open/close.
Cross-doc focus helpers, Escape stack, lang/title sync, landmarks, SR announce.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
28dd02c to
12f607d
Compare
PiP-native options popover; split reactions, keyboard nav, and pagination.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
12f607d to
946da58
Compare
Set a single clipping owner per PiP layout and use a consistent 4px radius.
Force-close PiP on unmount and reset PiP state so next room starts clean.
Forward PiP keydown events and register PiP-specific handlers.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
5ca3976 to
74dc656
Compare
Render the shared toast queue in PiP.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
74dc656 to
d7fb31e
Compare
Group banner and toasts in one stack, cap visibility, fix chat dedup.
Ovgodd
force-pushed
the
feat/pip-support
branch
from
8296fc5 to
1080407
Compare
Extract OverflowItems, replace `any`, convert function declarations to const arrows.
Replace nested ternary logic, use globalThis instead of window, and switch side-panel enum re-exports to export-from for clearer and safer patterns. Made-with: Cursor
Flip when top overflows; clamp left/right so PiP tooltips stay visible.
|
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.
Purpose
Introduce a Document Picture-in-Picture (PiP) experience that reuses the existing room controls and side panels,
Proposal
Implement a dedicated PiP window using a React portal that mounts the desired React tree into the PiP document. The PiP view supports single or grid layouts, provides the control ba and notifications