VIDCS-3340: Add pinning feature #41

maikthomas · 2025-02-04T16:19:43Z

What is this PR doing?

Add part 1 of pinning feature.
What this PR includes:

Overlay button on subscriber video tile to pin/unpin subscriber
pinning limit - 3 on desktop, 1 on mobile, and disabled button when limit reached
pinned subscribers are large unless screenshare is present, pinned subscribers are never hidden

NOT INCLUDED IN THIS PR:

Pinning from participant list
any changes we may make to layout manager proportions for pinned subscribers

How should this be manually tested?

run with multiple participants
ensure you can pin/unpin up to the maximum number
pinned subscribers should never be hidden
pinned subscribers should be small when screenshare is present

What are the relevant tickets?

Resolves VIDCS-3340

This reverts commit 6a5bbfd.

maikthomas · 2025-02-13T10:27:44Z

frontend/.env.example

+
+# Option to skip the waiting room. Useful when doing development work on the meeting room
+VITE_BYPASS_WAITING_ROOM=


I added this to help speed up development testing. Let me know if you think it's ok to add this

I'm okay with adding this option in. Looks like you added it to the file twice so maybe remove one of them?

Sounds good to me! Should we give it an initial value?

maikthomas · 2025-02-13T10:29:34Z

frontend/src/Context/SessionProvider/session.tsx

@@ -126,6 +131,8 @@ export type SessionProviderProps = {
  children: ReactNode;
 };

+const MAX_PIN_COUNT = isMobile() ? 1 : 3;


I know we talked about using small viewport instead of mobile for this stuff, but the number of subscribers we display is currently governed by isMobile too. I would say let's change this in both places in a follow-up PR if that's what we decide to do.

maikthomas · 2025-02-13T11:24:26Z

frontend/src/components/MeetingRoom/LayoutToggleButton/LayoutToggleButton.tsx

+    !isMobile() && (
+      <div>


ignore this, it's to make tests pass after I loaded tailwind into the setup test and it realised the tests were wrong.

Anyway, @cpettet's PR changes this component so when that PR is merged in I'll deal with the conflicts and this will probably not be changed by my PR at all.

I was just about to say, wait I remember lots of changes around here in a different PR 😆

maikthomas · 2025-02-13T11:26:04Z

frontend/src/components/MeetingRoom/PinButton/PinButton.tsx

+    return `Pin ${participantName}'s video`;
+  };
+
+  const onClick = (clickEvent: MouseEvent<HTMLButtonElement>) => {


You'll see this here and in the Subscriber component. Trust me when I say I tried everything to avoid adding this hack. In the end it was innevitable, even though we could use tailwind hover states to avoid some of this, the MUI toolbar open logic is not css based. Also tailwind struggled when the tooltip itself was hovered and de-applied the hover css to the area underneath.

At least I can say that this approach works.

maikthomas · 2025-02-13T11:29:04Z

frontend/src/Context/SessionProvider/session.tsx

+  const pinSubscriber = useCallback(
+    (id: string) => {
+      setSubscriberWrappers((previousSubscriberWrappers) => {
+        const subscribers = previousSubscriberWrappers
+          .map((subscriberWrapper) => {
+            if (subscriberWrapper.id === id) {
+              return {
+                ...subscriberWrapper,
+                isPinned: !subscriberWrapper.isPinned,
+              };
+            }
+            return subscriberWrapper;
+          })
+          .sort(sortByDisplayPriority(activeSpeakerId)); // Sorting by display priority will place this pinned participant above the rest
+        return subscribers;
+      });
+    },
+    [activeSpeakerId]
+  );
+


This logic is untested right now (other than the integration test).
We plan to refactor this context provider in a way that will allow us to better test this internal logic.

maikthomas · 2025-02-13T11:30:22Z

frontend/src/components/RedirectToWaitingRoom/RedirectToWaitingRoom.tsx

-  const bypass = searchParams.get('bypass');
-  const canEnter = !hasAccess && bypass !== 'true';
-  return canEnter ? (
+  const bypass =
+    searchParams.get('bypass') === 'true' || import.meta.env.VITE_BYPASS_WAITING_ROOM === 'true';
+  const mustEnterWaitingRoom = !hasAccess && !bypass;
+  return mustEnterWaitingRoom ? (
    <Navigate


Added the env var but also refactored slightly. I found canEnter confusing as it means canEnterWaiting room, whereas can enter IMO makes more sense to be about the room.

maikthomas · 2025-02-13T11:31:06Z

frontend/src/components/ScreenSharingButton/ScreenSharingButton.tsx

+    !isMobile() && (
+      <div>
+        <Tooltip title={title} aria-label="add">


Same comment here as for LayoutButton - tests were not working once tailwind was enabled and this will all be overwritten when the mobile menu bar PR is merged and I pull in those changes. Don't worry about this for now.

maikthomas · 2025-02-13T11:31:40Z

frontend/src/components/Subscriber/Subscriber.tsx

@@ -61,13 +66,30 @@ const Subscriber = ({
    }
  }, [subscriberWrapper, isScreenShare]);

+  const handlePinClick = (clickEvent: MouseEvent<HTMLButtonElement>) => {


Again, nothing I could do to avoid this unfortunately.

since this is repeated in two components, could it be broken out to a separate hook? 🤔

maikthomas · 2025-02-13T11:33:06Z

frontend/src/test/setup.ts

@@ -1,3 +1,4 @@
+import '../index.css';


This makes vitest understand at least some of the tailwind rules, like className="hidden" means toBeVisible() is false.
It cannot do stuff like hover states though, probably more due to JSDOM not implementing CSS

maikthomas · 2025-02-13T11:33:52Z

frontend/src/utils/helpers/getLayoutBoxes/getLayoutElements.ts

  sessionHasScreenshare: boolean,
  layoutMode: LayoutMode,
  activeSpeakerId: string | undefined,
  index: number
 ) => {
  // We display subscriber as large if it is screenshare or active speaker given that layout mode is active speaker and
  // screenshare is not being displayed
-  return (


I didn't attempt to simplify this at all, I think it's better to keep it readable and obvious as to which cases do what.

sonarqubecloud · 2025-02-13T17:16:44Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

behei-vonage

impressive job 👏 I left two comments for now, I might take another look again since the PR is just massive 😱

behei-vonage · 2025-02-13T17:36:10Z

frontend/src/components/MeetingRoom/PinButton/PinButton.tsx

+
+  const getTooltipText = () => {
+    if (isDisabled) {
+      return `You can't pin any more tiles`;


nit: maybe instead of a current message, we could say the Current ${MAX_PIN} pin limit has been reached or something like that?

behei-vonage · 2025-02-13T17:40:33Z

frontend/src/components/Subscriber/Subscriber.tsx

@@ -61,13 +66,30 @@ const Subscriber = ({
    }
  }, [subscriberWrapper, isScreenShare]);

+  const handlePinClick = (clickEvent: MouseEvent<HTMLButtonElement>) => {


since this is repeated in two components, could it be broken out to a separate hook? 🤔

cpettet

Looks good so far! Just have a few comments/questions. Let me know what you think!

cpettet · 2025-02-18T16:34:18Z

frontend/.env.example

+
+# Option to skip the waiting room. Useful when doing development work on the meeting room
+VITE_BYPASS_WAITING_ROOM=


Sounds good to me! Should we give it an initial value?

cpettet · 2025-02-18T18:03:43Z

frontend/src/Context/SessionProvider/session.tsx

+  }, [subscriberWrappers]);
+
+  /**
+   * Marks a subscriber as pinned, and moves it to the top of the display order.


Nit, doesn't this function work for unpinning, too? Could we update the jsdoc?

cpettet · 2025-02-18T18:05:07Z

frontend/src/utils/sortByDisplayPriority/sortByDisplayPriority.ts

@@ -17,6 +17,12 @@ const sortByDisplayPriority =
    if (wrapperB.isScreenshare) {
      return 1;
    }
+    if (wrapperA.isPinned) {


Could we add onto the jsdoc that we want pinned users to have priority? Since we have several prioritized subscribers, we should delineate the order, too.

cpettet · 2025-02-18T19:49:49Z

frontend/src/components/MeetingRoom/PinButton/PinButton.tsx

+  handleClick: (clickEvent: MouseEvent<HTMLButtonElement>) => void;
+};
+
+const PinButton = ({


Could we get some jsdoc for this component?

cpettet · 2025-02-18T21:18:18Z

frontend/src/types/session.ts

@@ -8,4 +8,5 @@ export type SubscriberWrapper = {
  subscriber: Subscriber;
  isScreenshare: boolean;
  id: string;
+  isPinned: boolean;


Should we add to the jsdoc, too?

cpettet · 2025-02-18T22:15:08Z

frontend/src/Context/SessionProvider/session.tsx

+        const subscribers = previousSubscriberWrappers
+          .map((subscriberWrapper) => {
+            if (subscriberWrapper.id === id) {
+              return {
+                ...subscriberWrapper,
+                isPinned: !subscriberWrapper.isPinned,
+              };
+            }
+            return subscriberWrapper;
+          })
+          .sort(sortByDisplayPriority(activeSpeakerId)); // Sorting by display priority will place this pinned participant above the rest
+        return subscribers;


Could we break the state function into a helper? I know we plan on refactoring later, but it might make sense to separate this logic so we can add the test now. Hopefully, the tests we write for this helper, could still be reused.

cpettet · 2025-02-18T22:18:47Z

frontend/src/components/Subscriber/Subscriber.tsx

  const hasVideo = subscriberWrapper.subscriber?.stream?.hasVideo;
  const initials = subscriberWrapper.subscriber?.stream?.initials;
  const username = subscriberWrapper.subscriber?.stream?.name ?? '';
  const hasAudio = subscriberWrapper.subscriber.stream?.hasAudio;
  const audioIndicatorStyle =
    'rounded-xl absolute top-3 right-3 bg-darkGray-55 h-6 w-6 items-center justify-center flex m-auto';
-
+  const pinStyle = `flex rounded-xl absolute top-3 left-3 h-6 w-6 items-center justify-center m-auto`;


Can we move this into the PinButton component?

If it's forward-thinking for the participant list changes, feel free to resolve this and my other comments.

cpettet · 2025-02-18T22:25:37Z

frontend/src/components/Subscriber/Subscriber.tsx

    >
+      {!isScreenShare && (
+        <PinButton
+          color="white"


Can we move this to the PinButton directly?

maikthomas added the WIP Work in progress label Feb 4, 2025

github-actions bot changed the base branch from main to develop February 4, 2025 16:19

maikthomas added 17 commits February 12, 2025 15:05

Add pinning button

73e29f7

fix lint

af8b7b0

missing types

c377a5b

fix test by not rendering on mobile

131530b

don't render layout button on mobile

421da1b

Add PushPinOffIcon

324898f

Add hover logic

0764ff4

remove tile hover

251f106

Add env util to bypass waiting room

532f6a4

Disable button after max pin amount

588cc42

Fix bypass util

fe80415

unused import

4f1ed55

add integraton test

9abc563

Fix RedirectToWaitingRoom tests

025df68

Fix pinning test

7bb4a42

Revert "remove tile hover"

82fe65b

This reverts commit 6a5bbfd.

change tests to new conditional rendering approach

a620a98

maikthomas force-pushed the mthomas/VIDCS-3340-pinning branch from c37e048 to a620a98 Compare February 12, 2025 14:05

maikthomas added 10 commits February 12, 2025 15:08

fix lint

3cc351f

Add sortByDisplayPriority tests

a0fe14a

Add test for pin off icon

861bc85

Move click check to util file

ed4f8e7

Update tooltip fix

b75acf4

Add comment to env var

004edb1

Add comment

c369a64

Simplify subscriber pin filter to map

106af24

Add same hack for hover state on moved element to subscriber tile

422213a

remove unused tailwind group

87faf55

maikthomas commented Feb 13, 2025

View reviewed changes

maikthomas removed the WIP Work in progress label Feb 13, 2025

maikthomas added 2 commits February 13, 2025 18:05

Merge branch 'develop' into mthomas/VIDCS-3340-pinning

3581c91

Update .env.example

7f8abf3

behei-vonage reviewed Feb 13, 2025

View reviewed changes

cpettet reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VIDCS-3340: Add pinning feature #41

VIDCS-3340: Add pinning feature #41

maikthomas commented Feb 4, 2025 •

edited

Loading

maikthomas Feb 13, 2025

behei-vonage Feb 13, 2025

cpettet Feb 18, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

behei-vonage Feb 13, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

behei-vonage Feb 13, 2025

maikthomas Feb 13, 2025

maikthomas Feb 13, 2025

sonarqubecloud bot commented Feb 13, 2025

behei-vonage left a comment

behei-vonage Feb 13, 2025

behei-vonage Feb 13, 2025

cpettet left a comment

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025

cpettet Feb 18, 2025


		# Option to skip the waiting room. Useful when doing development work on the meeting room
		VITE_BYPASS_WAITING_ROOM=

VIDCS-3340: Add pinning feature #41

Are you sure you want to change the base?

VIDCS-3340: Add pinning feature #41

Conversation

maikthomas commented Feb 4, 2025 • edited Loading

What is this PR doing?

How should this be manually tested?

What are the relevant tickets?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 13, 2025

Quality Gate passed

behei-vonage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpettet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maikthomas commented Feb 4, 2025 •

edited

Loading