refactor(breakout-room): Redesign sidebar UI for moderator and participants#24691
Open
AtilaU19 wants to merge 8 commits intobigbluebutton:v4.0.x-releasefrom
Open
refactor(breakout-room): Redesign sidebar UI for moderator and participants#24691AtilaU19 wants to merge 8 commits intobigbluebutton:v4.0.x-releasefrom
AtilaU19 wants to merge 8 commits intobigbluebutton:v4.0.x-releasefrom
Conversation
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:
WalkthroughAdds a large breakout-room UI refactor with new components (RunningBreakoutRoom, ParticipantBreakoutRoom, SidebarCreateBreakout, SidebarRoomAssignment, SidebarCreateBreakoutContainer), new styled-component sets, and sidebar/modal rendering changes. Introduces breakout window management functions (setBreakoutWindowRef, closeBreakoutWindow). Implements a "call moderator" end-to-end flow: GraphQL action and mutation, action adapter producing a Redis message, new Redis/Scala message types and handler (BreakoutRoomCallModeratorMsgHdlr) that inserts a system chat message, server routing updates, and client-side rendering for the new chat message type. Also replaces certain SQL comparisons with IS DISTINCT FROM and adds locale keys and an icon. Sequence Diagram(s)mermaid UI->>Hasura: breakoutRoomCallModerator(breakoutRoomId) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/component.tsx (1)
128-330:⚠️ Potential issue | 🔴 CriticalRemove or reuse the legacy
BreakoutRoomcomponent to unblock TypeScript compile.
BreakoutRoomis now dead code after the new container branching, and TypeScript fails on Line 129 withTS6133. This must be resolved before merge (delete the unused component + now-unused dependencies, or wire it back intentionally).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/component.tsx` around lines 128 - 330, The BreakoutRoom React component (const BreakoutRoom) is dead/unreferenced causing TS6133; either remove the entire BreakoutRoom declaration and unused symbols (breakoutRoomEndAll, breakoutRoomTransfer, breakoutRoomRequestJoinURL, stopMediaOnMainRoom, panelRef, showChangeTimeForm, requestedBreakoutRoomId and any related handlers like transferUserToMeeting, requestJoinURL, useEffect that opens windows) or reconnect it by exporting/using it from the new container branch; ensure you also remove now-unused imports/hooks (useMutation calls and any styles/intl usage only used by this component) so TypeScript no longer flags unused variables.bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx (1)
94-103:⚠️ Potential issue | 🔴 CriticalMove state updates to useEffect.
Lines 94-103 perform state mutations (
setWaiting,requestJoinURL) directly in render, violating React rules. Although the!waitingguard prevents infinite loops, side effects must be triggered fromuseEffect, not during render.Suggested fix
- // request join url if free join is enabled and user is not assigned to any room - if (defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId) { - const selectedBreakout = breakouts.find( - ({ breakoutRoomMeetingId }) => breakoutRoomMeetingId === defaultSelectedBreakoutId, - ); - if (!selectedBreakout?.joinURL && !waiting) { - requestJoinURL(defaultSelectedBreakoutId); - setWaiting(true); - } - } + useEffect(() => { + if (defaultSelectedBreakoutId !== breakouts[0]?.breakoutRoomMeetingId) return; + const selectedBreakout = breakouts.find( + ({ breakoutRoomMeetingId }) => breakoutRoomMeetingId === defaultSelectedBreakoutId, + ); + if (!selectedBreakout?.joinURL && !waiting) { + requestJoinURL(defaultSelectedBreakoutId); + setWaiting(true); + } + }, [defaultSelectedBreakoutId, breakouts, waiting]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx` around lines 94 - 103, The conditional that calls requestJoinURL(...) and setWaiting(true) must be moved out of render into a useEffect: create a useEffect in the BreakoutJoinConfirmationGraphql component that computes selectedBreakout (using breakouts.find(...) or a memo) and, when defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId && !selectedBreakout?.joinURL && !waiting, calls requestJoinURL(defaultSelectedBreakoutId) and then setWaiting(true); include the appropriate dependencies (defaultSelectedBreakoutId, breakouts, waiting, requestJoinURL) so the effect runs when those values change.
🧹 Nitpick comments (3)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx (1)
92-93:requestedBreakoutRoomIdflow is currently dead code.
setRequestedBreakoutRoomIdis never invoked, so this effect never executes meaningful work. Either wire it to a join action or remove it.Also applies to: 145-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx` around lines 92 - 93, The state requestedBreakoutRoomId and its setter setRequestedBreakoutRoomId are unused, leaving the useEffect that watches requestedBreakoutRoomId (around lines 145-157) as dead code; either remove both the state and the dependent useEffect, or wire setRequestedBreakoutRoomId into the room-join flow (e.g., call setRequestedBreakoutRoomId(roomId) from the join handler such as the component's joinBreakoutRoom / handleJoinClick function) so the effect runs as intended and performs the join logic, and ensure any cleanup or error handling inside that useEffect is kept consistent with the join flow.bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts (2)
61-82: Consider adding the standardappearanceproperty for future compatibility.The
-moz-appearanceis a vendor prefix. Adding the standard property ensures compatibility as browsers adopt the unprefixed version.Suggested improvement
-moz-appearance: textfield; + appearance: textfield;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts` around lines 61 - 82, In TimerInput styled component the rule uses the vendor-prefixed -moz-appearance but lacks the standard appearance property; add appearance: textfield (or appropriate value) alongside -moz-appearance: textfield in the TimerInput definition so the unprefixed property is present for future browser support and consistent behavior across engines.
229-238: Hardcoded color breaks theme consistency.The
#e8f0febackground color is hardcoded while the rest of the file uses palette variables. Consider defining this as a CSS variable in the palette for theme consistency, especially if dark mode support is planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts` around lines 229 - 238, InfoBanner currently uses a hardcoded background "#e8f0fe"; replace this with a palette variable from your theme (e.g., use the existing CSS variable or palette token used across the file) to keep theme consistency and enable dark-mode theming — update the styled component InfoBanner to reference the theme variable (or add a new palette entry like neutral/info background in your palette and reference that) instead of the hardcoded hex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx`:
- Around line 173-183: The code is mutating the breakouts prop by calling
breakouts.sort(...); replace the in-place sort with a non-mutating copy before
sorting (e.g., use [...breakouts] or breakouts.slice()) so the mapping uses a
sorted copy; update the expression where breakouts.sort((a, b) => a.sequence -
b.sequence) is used in the breakout-join-confirmation component to avoid side
effects on the breakouts prop.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx`:
- Around line 111-113: The computed breakoutStartedAt (from
breakoutProps?.startedAt) can be NaN and will poison the countdown; update the
initialization and the calcRemaining logic so you first validate the start time
(e.g. use Date.parse/built Date and check isFinite/Number.isNaN) and only
compute remainingTime when breakoutStartedAt is a valid timestamp; if invalid,
set breakoutStartedAt to null/undefined or 0 and ensure calcRemaining (and any
code that sets remainingTime) returns a safe fallback (like 0 or a paused state)
instead of performing arithmetic with NaN; adjust symbols:
breakoutDurationInSeconds, breakoutStartedAt, calcRemaining, and remainingTime
accordingly.
- Around line 186-203: The code currently calls notify and starts the cooldown
before chatSendMessage resolves; change it to invoke chatSendMessage(...) and
attach .then(() => { notify(intl.formatMessage(intlMessages.callModeratorSent),
'success', 'user'); setCallModeratorCooldown(true); setTimeout(() =>
setCallModeratorCooldown(false), CALL_MODERATOR_COOLDOWN_MS); }) and add a
.catch(err => {/* handle/log error and optionally notify failure */}) so success
UI and cooldown are only applied after chatSendMessage resolves; reference
chatSendMessage, notify, setCallModeratorCooldown, CALL_MODERATOR_COOLDOWN_MS,
intl.formatMessage, and intlMessages.callModeratorSent.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 134-135: The forEach callbacks currently use expression bodies
that implicitly return a value (e.g. b.participants.filter((p) =>
!p.isAudioOnly).forEach((p) => alreadyAssigned.add(p.userId))); change those to
block-bodied callbacks to avoid implicit returns: replace the arrow callbacks
with a block form that calls alreadyAssigned.add(...) inside braces (for example
.forEach((p) => { alreadyAssigned.add(p.userId); })); apply the same change to
the other occurrence around the code that similarly iterates participants (the
second instance at lines referenced in the comment).
- Line 113: breakoutStartedAt can become NaN when breakoutProps?.startedAt is
missing/invalid, which causes calcRemaining() to return NaN; update the code so
calcRemaining() first checks that breakoutStartedAt is a finite number (e.g.,
Number.isFinite(breakoutStartedAt) or isFinite(...)) and return a safe default
(0 or null) when it's not finite, and likewise validate/guard the other similar
calculation block around lines 216-224; reference the breakoutStartedAt variable
and the calcRemaining() function to locate where to add the finite check and
early return.
- Around line 316-323: The optimistic pending move added before calling
breakoutRoomMoveUser must be rolled back if the mutation fails: catch the
promise rejection from breakoutRoomMoveUser(...) and in the error handler remove
the optimistic pending move (use the same cleanup function/path the success
handler at line 170 uses, passing droppedUserId,
fromBreakoutRoomMeetingId/fromRoomMeetingId and
toBreakoutRoomMeetingId/toRoomId) so the pending state is cleared; also log the
error for visibility. Ensure you call the rollback/removal with the same
identifiers used when creating the pending move so it unambiguously reverts the
optimistic update.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Around line 163-165: The seconds state (seconds, setSeconds) is never
incorporated into the breakout duration logic so user input is dropped; update
all places that build or validate the duration (e.g., wherever hours and minutes
are combined — look for functions/handlers like the create/submit handler,
duration computation or validateDuration) to include seconds when computing
total seconds or milliseconds and in validation checks (e.g., min/max duration
comparisons), and ensure the UI's seconds value is passed through to any
payloads or state used to create the breakout (references: hours, minutes,
seconds, and the create/submit handler or validateDuration function).
- Around line 159-160: The expression computing captureSharedNotesByDefault is
using the wrong feature flag: it currently ANDs
BREAKOUT_SETTINGS.captureSharedNotesByDefault with
isImportPresentationWithAnnotationsEnabled; replace that second operand with the
shared-notes feature flag (e.g. isSharedNotesEnabled or the project’s
equivalent) so captureSharedNotesByDefault =
BREAKOUT_SETTINGS.captureSharedNotesByDefault && <shared-notes-flag>; update any
imports/usages of isImportPresentationWithAnnotationsEnabled in this expression
to the correct shared-notes flag.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/container.tsx`:
- Around line 66-76: The notify and logger.error side effects inside the render
path should be moved into a useEffect so they only run when the error state
changes: remove the direct calls from the component body that check usersError
|| meetingGroupError and instead add a useEffect that depends on [usersError,
meetingGroupError, intl] and calls notify(intl.formatMessage(...)) and
logger.error({ logCode: 'subscription_Failed', extraInfo: { error: usersError ||
meetingGroupError } }) when an error is present; keep the existing early return
(return null) in the render but ensure the side effects are executed only inside
the useEffect to avoid duplicate toasts/logs.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx`:
- Around line 54-56: The current drag payload parsing uses data.split('-') which
breaks when user IDs contain hyphens; change the parsing to deserialize a stable
format (e.g. JSON) instead of splitting. Replace the lines reading
ev.dataTransfer.getData('text') and splitting into [userId, from] with code that
does const { userId, from } = JSON.parse(ev.dataTransfer.getData('text')); and
then call moveUser(userId, Number(from), roomNumber). Also ensure the
dragstart/producer side sets the payload with JSON.stringify({ userId, from })
so moveUser receives the correct identifiers.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts`:
- Around line 395-418: Remove the redundant initial "border: none" declaration
in the StartButton styled component (it is overridden by "border: 2px solid
${colorGrayLighter}" later); update the StartButton definition to omit the first
border line so the intended visible border styling (and hover border-color)
remains effective while leaving border-radius, padding, disabled and hover rules
unchanged.
---
Outside diff comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx`:
- Around line 94-103: The conditional that calls requestJoinURL(...) and
setWaiting(true) must be moved out of render into a useEffect: create a
useEffect in the BreakoutJoinConfirmationGraphql component that computes
selectedBreakout (using breakouts.find(...) or a memo) and, when
defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId &&
!selectedBreakout?.joinURL && !waiting, calls
requestJoinURL(defaultSelectedBreakoutId) and then setWaiting(true); include the
appropriate dependencies (defaultSelectedBreakoutId, breakouts, waiting,
requestJoinURL) so the effect runs when those values change.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/component.tsx`:
- Around line 128-330: The BreakoutRoom React component (const BreakoutRoom) is
dead/unreferenced causing TS6133; either remove the entire BreakoutRoom
declaration and unused symbols (breakoutRoomEndAll, breakoutRoomTransfer,
breakoutRoomRequestJoinURL, stopMediaOnMainRoom, panelRef, showChangeTimeForm,
requestedBreakoutRoomId and any related handlers like transferUserToMeeting,
requestJoinURL, useEffect that opens windows) or reconnect it by exporting/using
it from the new container branch; ensure you also remove now-unused
imports/hooks (useMutation calls and any styles/intl usage only used by this
component) so TypeScript no longer flags unused variables.
---
Nitpick comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx`:
- Around line 92-93: The state requestedBreakoutRoomId and its setter
setRequestedBreakoutRoomId are unused, leaving the useEffect that watches
requestedBreakoutRoomId (around lines 145-157) as dead code; either remove both
the state and the dependent useEffect, or wire setRequestedBreakoutRoomId into
the room-join flow (e.g., call setRequestedBreakoutRoomId(roomId) from the join
handler such as the component's joinBreakoutRoom / handleJoinClick function) so
the effect runs as intended and performs the join logic, and ensure any cleanup
or error handling inside that useEffect is kept consistent with the join flow.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts`:
- Around line 61-82: In TimerInput styled component the rule uses the
vendor-prefixed -moz-appearance but lacks the standard appearance property; add
appearance: textfield (or appropriate value) alongside -moz-appearance:
textfield in the TimerInput definition so the unprefixed property is present for
future browser support and consistent behavior across engines.
- Around line 229-238: InfoBanner currently uses a hardcoded background
"#e8f0fe"; replace this with a palette variable from your theme (e.g., use the
existing CSS variable or palette token used across the file) to keep theme
consistency and enable dark-mode theming — update the styled component
InfoBanner to reference the theme variable (or add a new palette entry like
neutral/info background in your palette and reference that) instead of the
hardcoded hex.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bigbluebutton-html5/public/fonts/BbbIcons/bbb-icons.woffis excluded by!**/*.woffbigbluebutton-html5/public/fonts/BbbIcons/bbb-icons.woff2is excluded by!**/*.woff2
📒 Files selected for processing (18)
bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsxbigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/breakout-observer/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/service.tsbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/queries.tsbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/container.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/styles.tsbigbluebutton-html5/public/locales/en.jsonbigbluebutton-html5/public/stylesheets/bbb-icons.css
...ts/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx
Outdated
Show resolved
Hide resolved
...utton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx
Outdated
Show resolved
Hide resolved
...utton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsx
Outdated
Show resolved
Hide resolved
...luebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx
Outdated
Show resolved
Hide resolved
...luebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx
Outdated
Show resolved
Hide resolved
...ports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+163
to
+165
| const [hours, setHours] = useState(0); | ||
| const [minutes, setMinutes] = useState(DEFAULT_SIDEBAR_BREAKOUT_TIME); | ||
| const [seconds, setSeconds] = useState(0); |
Contributor
There was a problem hiding this comment.
Seconds input is ignored in breakout duration creation.
seconds is editable in the UI but not included in duration computation or validation, so user input is silently dropped.
🐛 Proposed fix
- const durationTime = (hours * 60) + minutes;
+ const durationInSecondsTotal = (hours * 3600) + (minutes * 60) + seconds;
+ const durationTime = Math.ceil(durationInSecondsTotal / 60);
...
- const totalMinutes = durationTime;
- if (totalMinutes < MIN_BREAKOUT_TIME) return;
+ const totalMinutes = durationTime;
+ if (durationInSecondsTotal < (MIN_BREAKOUT_TIME * 60)) return;
...
- const canStart = durationTime >= MIN_BREAKOUT_TIME;
+ const canStart = durationInSecondsTotal >= (MIN_BREAKOUT_TIME * 60);Also applies to: 207-207, 235-236, 301-302, 316-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`
around lines 163 - 165, The seconds state (seconds, setSeconds) is never
incorporated into the breakout duration logic so user input is dropped; update
all places that build or validate the duration (e.g., wherever hours and minutes
are combined — look for functions/handlers like the create/submit handler,
duration computation or validateDuration) to include seconds when computing
total seconds or milliseconds and in validation checks (e.g., min/max duration
comparisons), and ensure the UI's seconds value is passed through to any
payloads or state used to create the breakout (references: hours, minutes,
seconds, and the create/submit handler or validateDuration function).
...ports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/container.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+54
to
+56
| const data = ev.dataTransfer.getData('text'); | ||
| const [userId, from] = data.split('-'); | ||
| moveUser(userId, Number(from), roomNumber); |
Contributor
There was a problem hiding this comment.
Drag payload parsing is unsafe for IDs containing -.
Splitting by - can corrupt userId/from and move the wrong user when IDs include hyphens.
🐛 Proposed fix
const drop = (roomNumber: number) => (ev: React.DragEvent) => {
ev.preventDefault();
const data = ev.dataTransfer.getData('text');
- const [userId, from] = data.split('-');
+ const separator = data.lastIndexOf('-');
+ if (separator <= 0) return;
+ const userId = data.slice(0, separator);
+ const from = data.slice(separator + 1);
moveUser(userId, Number(from), roomNumber);
setSelectedId('');
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx`
around lines 54 - 56, The current drag payload parsing uses data.split('-')
which breaks when user IDs contain hyphens; change the parsing to deserialize a
stable format (e.g. JSON) instead of splitting. Replace the lines reading
ev.dataTransfer.getData('text') and splitting into [userId, from] with code that
does const { userId, from } = JSON.parse(ev.dataTransfer.getData('text')); and
then call moveUser(userId, Number(from), roomNumber). Also ensure the
dragstart/producer side sets the payload with JSON.stringify({ userId, from })
so moveUser receives the correct identifiers.
...5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx (1)
94-103:⚠️ Potential issue | 🟠 MajorMove join-URL request side-effects out of render.
The code at lines 95-103 executes
requestJoinURL()andsetWaiting(true)during render, outside of any effect. This causes duplicate mutation requests and violates React render purity. In Strict Mode, the component renders twice, triggering the mutation twice.Move this logic into a
useEffectwith appropriate dependency tracking:♻️ Proposed fix
- // request join url if free join is enabled and user is not assigned to any room - if (defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId) { - const selectedBreakout = breakouts.find( - ({ breakoutRoomMeetingId }) => breakoutRoomMeetingId === defaultSelectedBreakoutId, - ); - if (!selectedBreakout?.joinURL && !waiting) { - requestJoinURL(defaultSelectedBreakoutId); - setWaiting(true); - } - } + // request join url if free join is enabled and user is not assigned to any room + useEffect(() => { + if (!breakouts.length) return; + if (defaultSelectedBreakoutId !== breakouts[0].breakoutRoomMeetingId) return; + const selectedBreakout = breakouts.find( + ({ breakoutRoomMeetingId }) => breakoutRoomMeetingId === defaultSelectedBreakoutId, + ); + if (!selectedBreakout?.joinURL && !waiting) { + requestJoinURL(defaultSelectedBreakoutId); + setWaiting(true); + } + }, [breakouts, defaultSelectedBreakoutId, waiting]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx` around lines 94 - 103, The render currently calls requestJoinURL(...) and setWaiting(true) directly which causes side-effects during render; move this logic into a useEffect so the mutation runs only when needed. Create a useEffect that watches defaultSelectedBreakoutId, breakouts (or breakouts[0].breakoutRoomMeetingId), waiting, and requestJoinURL, check the same condition (defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId and selectedBreakout?.joinURL is falsy and !waiting) inside the effect, then call requestJoinURL(defaultSelectedBreakoutId) and setWaiting(true) from within the effect body. Ensure you compute selectedBreakout the same way (using breakouts.find(...)) inside the effect or memoize it so dependencies are accurate.
♻️ Duplicate comments (1)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx (1)
315-327:⚠️ Potential issue | 🟠 MajorRollback optimistic move state when move mutation fails.
pendingMovesis set beforebreakoutRoomMoveUser, but there is no rejection handler. On failure, the pending user can stay stuck in the UI indefinitely.♻️ Proposed fix
breakoutRoomMoveUser({ variables: { userId: droppedUserId, fromBreakoutRoomMeetingId: fromRoomMeetingId, toBreakoutRoomMeetingId: toRoomId, }, - }); + }).catch(() => { + setPendingMoves((prev) => { + const next = new Map(prev); + const pending = next.get(droppedUserId); + if (pending?.toRoomId === toRoomId) next.delete(droppedUserId); + return next; + }); + }); }, [breakoutRoomMoveUser]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 315 - 327, pendingMoves is updated optimistically before calling breakoutRoomMoveUser, but there's no error handling to roll that change back; update the call to breakoutRoomMoveUser (the promise returned by breakoutRoomMoveUser) to handle rejection and remove the optimistic entry from pendingMoves via setPendingMoves (e.g., clone prev map and delete droppedUserId) on failure, and optionally log or surface the error so the UI doesn't leave the user stuck; ensure you reference droppedUserId when rolling back the Map entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@akka-bbb-apps/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutRoomCallModeratorMsgHdlr.scala`:
- Around line 18-37: The code currently resolves breakout room and senderUser
but does not verify membership; before calling ChatMessageDAO.insertSystemMsg in
BreakoutRoomCallModeratorMsgHdlr (where you use state.breakout, room, and
RegisteredUsers.findWithUserId), add a membership check that ensures
msg.header.userId (or senderUser.id) is present in room.assignedUsers (or use
the existing RightsManagementTrait helper if available) and abort/log and do not
insert the system message if the user is not assigned to that breakout room;
only proceed to build msgMeta and call ChatMessageDAO.insertSystemMsg when the
membership check passes.
In
`@akka-bbb-apps/src/main/scala/org/bigbluebutton/core/running/MeetingActor.scala`:
- Around line 544-545: The BreakoutRoomCallModeratorReqMsg branch doesn't record
user activity; before or when calling handleBreakoutRoomCallModeratorMsg from
the case m: BreakoutRoomCallModeratorReqMsg in MeetingActor.scala, invoke the
same user-activity update used by other inactivity-aware message handlers (i.e.,
call the existing function that records the user's last-activity timestamp used
elsewhere in MeetingActor) so the moderator-call action resets the user's
inactivity timer; keep the activity update consistent with other cases and then
proceed to set state = handleBreakoutRoomCallModeratorMsg(m, state).
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Around line 296-309: The sidebar transition runs immediately after calling
createBreakoutRoom; change this so the layoutContextDispatch calls
(ACTIONS.SET_SIDEBAR_CONTENT_IS_OPEN and ACTIONS.SET_SIDEBAR_CONTENT_PANEL with
PANELS.BREAKOUT) only occur after the createBreakoutRoom operation succeeds —
either await the createBreakoutRoom promise (or move the dispatches into the
mutation's onCompleted handler) and perform the two layoutContextDispatch calls
there so failures do not trigger the panel change.
In
`@bigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx`:
- Around line 568-574: The metadata parsing can yield null from JSON.parse which
makes meta.roomName access unsafe; update the parsing closure (the const meta =
(() => { ... })()) to normalize the parsed value to a plain object by checking
the parsed result (e.g., const parsed = JSON.parse(...); return parsed && typeof
parsed === 'object' ? parsed : {} ) and keep the existing catch returning {} so
callModeratorMsg (and any other usages of meta.roomName) always read from an
object.
---
Outside diff comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsx`:
- Around line 94-103: The render currently calls requestJoinURL(...) and
setWaiting(true) directly which causes side-effects during render; move this
logic into a useEffect so the mutation runs only when needed. Create a useEffect
that watches defaultSelectedBreakoutId, breakouts (or
breakouts[0].breakoutRoomMeetingId), waiting, and requestJoinURL, check the same
condition (defaultSelectedBreakoutId === breakouts[0].breakoutRoomMeetingId and
selectedBreakout?.joinURL is falsy and !waiting) inside the effect, then call
requestJoinURL(defaultSelectedBreakoutId) and setWaiting(true) from within the
effect body. Ensure you compute selectedBreakout the same way (using
breakouts.find(...)) inside the effect or memoize it so dependencies are
accurate.
---
Duplicate comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 315-327: pendingMoves is updated optimistically before calling
breakoutRoomMoveUser, but there's no error handling to roll that change back;
update the call to breakoutRoomMoveUser (the promise returned by
breakoutRoomMoveUser) to handle rejection and remove the optimistic entry from
pendingMoves via setPendingMoves (e.g., clone prev map and delete droppedUserId)
on failure, and optionally log or surface the error so the UI doesn't leave the
user stuck; ensure you reference droppedUserId when rolling back the Map entry.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutApp2x.scalaakka-bbb-apps/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutRoomCallModeratorMsgHdlr.scalaakka-bbb-apps/src/main/scala/org/bigbluebutton/core/pubsub/senders/ReceivedJsonMsgHandlerActor.scalaakka-bbb-apps/src/main/scala/org/bigbluebutton/core/running/MeetingActor.scalabbb-common-message/src/main/scala/org/bigbluebutton/common2/msgs/BreakoutMsgs.scalabbb-common-message/src/main/scala/org/bigbluebutton/common2/msgs/GroupChatMsg.scalabbb-graphql-actions/src/actions/breakoutRoomCallModerator.tsbbb-graphql-server/bbb_schema.sqlbbb-graphql-server/metadata/actions.graphqlbbb-graphql-server/metadata/actions.yamlbigbluebutton-html5/imports/ui/components/breakout-join-confirmation/breakout-join-confirmation-graphql/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/container.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/mutations.tsbigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsxbigbluebutton-html5/imports/ui/core/enums/chat.tsbigbluebutton-html5/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/container.tsx
- bigbluebutton-html5/public/locales/en.json
...s/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutRoomCallModeratorMsgHdlr.scala
Outdated
Show resolved
Hide resolved
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/running/MeetingActor.scala
Outdated
Show resolved
Hide resolved
...ports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx
Show resolved
Hide resolved
...l5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx (1)
332-339:⚠️ Potential issue | 🟠 MajorAdd error handling to rollback the optimistic move state when the mutation fails.
The optimistic pending move is added at line 326-330 before the mutation is called, but there is no error handler. If the mutation fails, the pending state remains indefinitely, leaving the UI in an incorrect state.
🛠️ Proposed fix
breakoutRoomMoveUser({ variables: { userId: droppedUserId, fromBreakoutRoomMeetingId: fromRoomMeetingId, toBreakoutRoomMeetingId: toRoomId, }, - }); + }).catch(() => { + setPendingMoves((prev) => { + const next = new Map(prev); + const pending = next.get(droppedUserId); + if (pending?.toRoomId === toRoomId) next.delete(droppedUserId); + return next; + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 332 - 339, The optimistic pending move added immediately before calling breakoutRoomMoveUser must be rolled back if the mutation fails: add an error handler to the breakoutRoomMoveUser call (either await it in a try/catch or append .catch / use the mutation's onError) and on failure remove the optimistic pending move for the droppedUserId by dispatching the inverse update (i.e., remove the pending move that moved droppedUserId from fromBreakoutRoomMeetingId to toBreakoutRoomMeetingId) so the UI state is restored; ensure you use the same state updater or action used to add the pending move so the rollback mirrors the optimistic change.bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx (1)
207-207:⚠️ Potential issue | 🟠 MajorMinimum duration check is still bypassable with seconds.
Using
seconds > 0 ? 1 : 0rounds up duration, so00:04:01is treated as 5 minutes and passes both validation and start enablement. Validate minimum against total seconds, and only round for API payload conversion.🐛 Suggested fix
- const durationTime = (hours * 60) + minutes + (seconds > 0 ? 1 : 0); + const durationInSecondsTotal = (hours * 3600) + (minutes * 60) + seconds; + const durationTime = Math.ceil(durationInSecondsTotal / 60); ... - const totalMinutes = durationTime; - if (totalMinutes < MIN_BREAKOUT_TIME) return; + const totalMinutes = durationTime; + if (durationInSecondsTotal < (MIN_BREAKOUT_TIME * 60)) return; ... - const canStart = durationTime >= MIN_BREAKOUT_TIME; + const canStart = durationInSecondsTotal >= (MIN_BREAKOUT_TIME * 60);Also applies to: 235-236, 316-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx` at line 207, Compute totalSeconds = hours*3600 + minutes*60 + seconds and use that value for the minimum-duration validation and for enabling start; do not use durationTime = (hours*60)+minutes+(seconds>0?1:0) for validation since it rounds up and can bypass the minimum check. Keep the rounding only when creating the API payload by converting totalSeconds to minutes with Math.ceil(totalSeconds/60) (update any uses of durationTime and the validation logic that reference hours, minutes, seconds), and apply the same fix to the other occurrences around the duration calculation (the other durationTime usages at the noted spots).
🧹 Nitpick comments (2)
bigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx (1)
567-593: LGTM — past review concern addressed.The metadata parsing now correctly guards against
nulland non-object values fromJSON.parse. The defensive pattern used here is more robust than similar cases elsewhere in the switch statement.One optional refinement for improved type safety:
💡 Optional: Add explicit type annotation
const meta = (() => { try { const parsed = JSON.parse(message.messageMetadata); return parsed && typeof parsed === 'object' ? parsed : {}; } catch { return {}; } - })(); + })() as { roomName?: string };This provides better IDE autocompletion and catches typos like
meta.roomnameat compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx` around lines 567 - 593, Add an explicit type for the parsed metadata to improve type safety: in the ChatMessageType.BREAKOUT_CALL_MODERATOR branch, annotate the `meta` variable (the result of parsing `message.messageMetadata`) with an interface or type such as { roomName?: string } (or a broader Record<string, unknown>) so accesses like `meta.roomName` are checked by the compiler; keep the same try/catch parsing logic but cast/declare the parsed value to that type before using it in `callModeratorMsg` (used with `intlMessages.breakoutCallModerator`, `message.senderName`, and `roomName`).bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx (1)
586-598: Consider fixing types instead of using@ts-ignore.The
@ts-ignorecomments suppress TypeScript errors forStyled.MegaphoneBtnandStyled.FinishBtn. This hides potential type mismatches and reduces type safety. Consider updating the styled component type definitions to properly accept theicon,label, andonClickprops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 586 - 598, Styled.MegaphoneBtn and Styled.FinishBtn are using `@ts-ignore` because their styled-component types don’t accept the props (icon, label, onClick, data-test); remove the ignores by updating the component prop types: declare a proper props interface (e.g., MegaphoneBtnProps/FinishBtnProps) that includes icon: string, label: string, onClick: () => void, and data-test?: string (or extend the underlying Button props if wrapping a Button), apply that interface to the styled component definition (or use styled(Button)<Props>), and then remove the `@ts-ignore` lines so TypeScript validates the props correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 129-141: Remove the unnecessary "return undefined;" statements
from all forEach callbacks that iterate over breakouts and participants (e.g.,
the forEach that populates seqToMeetingId with b.sequence /
b.breakoutRoomMeetingId and the nested forEach inside
b.participants.filter(...).forEach that adds to alreadyAssigned), as well as the
other forEach blocks in this component with the same pattern; simply let the
callbacks perform their side effects without returning a value so the lint rule
lint/suspicious/useIterableCallbackReturn is satisfied and behavior remains
unchanged.
- Around line 269-273: The inner setTimeout that calls setEditingTime(null) can
update state after unmount; fix by storing its timer id in a ref (e.g.,
innerTimeoutRef) when you create the inner setTimeout inside the
debounceTimerRef callback, clear that inner timeout wherever you clear
debounceTimerRef (and in the component cleanup) and also clear it before setting
a new inner timeout; ensure breakoutRoomSetTime and setEditingTime logic remains
the same but the inner timer is tracked and cleared to avoid state updates on
unmounted components.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Line 177: The createBreakoutRoom mutation is unguarded and lacks error
handling; add an in-flight guard and explicit error handling around the mutation
call to prevent duplicate submissions and surfacing failures. Introduce a local
loading flag (e.g., isCreating) in the component state and set it true before
calling createBreakoutRoom(BREAKOUT_ROOM_CREATE, ...) and false in finally;
early-return if isCreating is true to block rapid clicks. Wrap the mutation call
in try/catch, inspect the mutation result for errors, and update component state
or display a user-facing error (e.g., setCreateError or a toast) on failure
instead of silently failing. Apply the same pattern to the other mutation usages
referenced (the blocks around lines indicated: the second create handler at
~296-308 and the handler at ~503-505) so all create-breakout mutation flows use
the isCreating guard, try/catch, result error check, and UI feedback.
---
Duplicate comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 332-339: The optimistic pending move added immediately before
calling breakoutRoomMoveUser must be rolled back if the mutation fails: add an
error handler to the breakoutRoomMoveUser call (either await it in a try/catch
or append .catch / use the mutation's onError) and on failure remove the
optimistic pending move for the droppedUserId by dispatching the inverse update
(i.e., remove the pending move that moved droppedUserId from
fromBreakoutRoomMeetingId to toBreakoutRoomMeetingId) so the UI state is
restored; ensure you use the same state updater or action used to add the
pending move so the rollback mirrors the optimistic change.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Line 207: Compute totalSeconds = hours*3600 + minutes*60 + seconds and use
that value for the minimum-duration validation and for enabling start; do not
use durationTime = (hours*60)+minutes+(seconds>0?1:0) for validation since it
rounds up and can bypass the minimum check. Keep the rounding only when creating
the API payload by converting totalSeconds to minutes with
Math.ceil(totalSeconds/60) (update any uses of durationTime and the validation
logic that reference hours, minutes, seconds), and apply the same fix to the
other occurrences around the duration calculation (the other durationTime usages
at the noted spots).
---
Nitpick comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 586-598: Styled.MegaphoneBtn and Styled.FinishBtn are using
`@ts-ignore` because their styled-component types don’t accept the props (icon,
label, onClick, data-test); remove the ignores by updating the component prop
types: declare a proper props interface (e.g., MegaphoneBtnProps/FinishBtnProps)
that includes icon: string, label: string, onClick: () => void, and data-test?:
string (or extend the underlying Button props if wrapping a Button), apply that
interface to the styled component definition (or use styled(Button)<Props>), and
then remove the `@ts-ignore` lines so TypeScript validates the props correctly.
In
`@bigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx`:
- Around line 567-593: Add an explicit type for the parsed metadata to improve
type safety: in the ChatMessageType.BREAKOUT_CALL_MODERATOR branch, annotate the
`meta` variable (the result of parsing `message.messageMetadata`) with an
interface or type such as { roomName?: string } (or a broader Record<string,
unknown>) so accesses like `meta.roomName` are checked by the compiler; keep the
same try/catch parsing logic but cast/declare the parsed value to that type
before using it in `callModeratorMsg` (used with
`intlMessages.breakoutCallModerator`, `message.senderName`, and `roomName`).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
akka-bbb-apps/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutRoomCallModeratorMsgHdlr.scalaakka-bbb-apps/src/main/scala/org/bigbluebutton/core/running/MeetingActor.scalabigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsxbigbluebutton-html5/imports/ui/components/chat/chat-graphql/chat-message-list/page/chat-message/component.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- akka-bbb-apps/src/main/scala/org/bigbluebutton/core/apps/breakout/BreakoutRoomCallModeratorMsgHdlr.scala
- akka-bbb-apps/src/main/scala/org/bigbluebutton/core/running/MeetingActor.scala
Comment on lines
+129
to
+141
| breakouts.forEach((b) => { | ||
| seqToMeetingId.set(b.sequence, b.breakoutRoomMeetingId); | ||
| return undefined; | ||
| }); | ||
|
|
||
| const alreadyAssigned = new Set<string>(); | ||
| breakouts.forEach((b) => { | ||
| b.participants.filter((p) => !p.isAudioOnly).forEach((p) => { | ||
| alreadyAssigned.add(p.userId); | ||
| return undefined; | ||
| }); | ||
| return undefined; | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary return undefined; statements from forEach callbacks.
Biome flags these as lint/suspicious/useIterableCallbackReturn. The return undefined; statements are unnecessary since forEach callbacks should not return any value.
♻️ Proposed fix
breakouts.forEach((b) => {
seqToMeetingId.set(b.sequence, b.breakoutRoomMeetingId);
- return undefined;
});
const alreadyAssigned = new Set<string>();
breakouts.forEach((b) => {
b.participants.filter((p) => !p.isAudioOnly).forEach((p) => {
alreadyAssigned.add(p.userId);
- return undefined;
});
- return undefined;
});Also applies to lines 155-158, 169-177, and 218-223 where the same pattern exists.
📝 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
| breakouts.forEach((b) => { | |
| seqToMeetingId.set(b.sequence, b.breakoutRoomMeetingId); | |
| return undefined; | |
| }); | |
| const alreadyAssigned = new Set<string>(); | |
| breakouts.forEach((b) => { | |
| b.participants.filter((p) => !p.isAudioOnly).forEach((p) => { | |
| alreadyAssigned.add(p.userId); | |
| return undefined; | |
| }); | |
| return undefined; | |
| }); | |
| breakouts.forEach((b) => { | |
| seqToMeetingId.set(b.sequence, b.breakoutRoomMeetingId); | |
| }); | |
| const alreadyAssigned = new Set<string>(); | |
| breakouts.forEach((b) => { | |
| b.participants.filter((p) => !p.isAudioOnly).forEach((p) => { | |
| alreadyAssigned.add(p.userId); | |
| }); | |
| }); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 129-129: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
[error] 135-135: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
[error] 136-136: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`
around lines 129 - 141, Remove the unnecessary "return undefined;" statements
from all forEach callbacks that iterate over breakouts and participants (e.g.,
the forEach that populates seqToMeetingId with b.sequence /
b.breakoutRoomMeetingId and the nested forEach inside
b.participants.filter(...).forEach that adds to alreadyAssigned), as well as the
other forEach blocks in this component with the same pattern; simply let the
callbacks perform their side effects without returning a value so the lint rule
lint/suspicious/useIterableCallbackReturn is satisfied and behavior remains
unchanged.
Comment on lines
+269
to
+273
| debounceTimerRef.current = setTimeout(() => { | ||
| const newMinutes = Math.max(1, Math.ceil(clamped / 60)); | ||
| breakoutRoomSetTime({ variables: { timeInMinutes: newMinutes } }); | ||
| setTimeout(() => setEditingTime(null), 500); | ||
| }, DEBOUNCE_DELAY); |
Contributor
There was a problem hiding this comment.
Inner setTimeout can cause state update on unmounted component.
The nested setTimeout at line 272 is not tracked or cleaned up. If the component unmounts after DEBOUNCE_DELAY fires but before the 500ms inner timeout completes, setEditingTime(null) will attempt to update state on an unmounted component.
🛠️ Proposed fix using a ref to track the inner timeout
+ const innerTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+
// Add cleanup for inner timer in the unmount effect
useEffect(() => () => {
if (debounceTimerRef.current) clearTimeout(debounceTimerRef.current);
+ if (innerTimerRef.current) clearTimeout(innerTimerRef.current);
}, []);
const commitTimeChange = useCallback((newTotalSeconds: number) => {
const clamped = Math.max(60, newTotalSeconds);
setEditingTime(clamped);
if (debounceTimerRef.current) clearTimeout(debounceTimerRef.current);
debounceTimerRef.current = setTimeout(() => {
const newMinutes = Math.max(1, Math.ceil(clamped / 60));
breakoutRoomSetTime({ variables: { timeInMinutes: newMinutes } });
- setTimeout(() => setEditingTime(null), 500);
+ innerTimerRef.current = setTimeout(() => setEditingTime(null), 500);
}, DEBOUNCE_DELAY);
}, [breakoutRoomSetTime]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`
around lines 269 - 273, The inner setTimeout that calls setEditingTime(null) can
update state after unmount; fix by storing its timer id in a ref (e.g.,
innerTimeoutRef) when you create the inner setTimeout inside the
debounceTimerRef callback, clear that inner timeout wherever you clear
debounceTimerRef (and in the component cleanup) and also clear it before setting
a new inner timeout; ensure breakoutRoomSetTime and setEditingTime logic remains
the same but the inner timer is tracked and cleared to avoid state updates on
unmounted components.
| const [, setLeastOneUserIsValid] = useState(false); | ||
| const [roomPresentations, setRoomPresentations] = useState<RoomPresentations>([]); | ||
|
|
||
| const [createBreakoutRoom] = useMutation(BREAKOUT_ROOM_CREATE); |
Contributor
There was a problem hiding this comment.
Protect createBreakoutRoom from duplicate submits and silent failures.
The mutation call has no in-flight guard and no error handling. Rapid clicks can fire duplicate create requests; rejected requests currently fail without user-facing/state handling.
♻️ Suggested fix
- const [createBreakoutRoom] = useMutation(BREAKOUT_ROOM_CREATE);
+ const [createBreakoutRoom, { loading: isCreatingBreakout }] = useMutation(BREAKOUT_ROOM_CREATE);
...
- const handleCreateRoom = useCallback(() => {
+ const handleCreateRoom = useCallback(async () => {
+ if (isCreatingBreakout) return;
const totalMinutes = durationTime;
if (totalMinutes < MIN_BREAKOUT_TIME) return;
...
- createBreakoutRoom({
- variables: {
- record,
- captureNotes,
- captureSlides,
- durationInMinutes: totalMinutes,
- sendInviteToModerators: inviteMods,
- rooms: roomsArray,
- },
- }).then(() => {
+ try {
+ await createBreakoutRoom({
+ variables: {
+ record,
+ captureNotes,
+ captureSlides,
+ durationInMinutes: totalMinutes,
+ sendInviteToModerators: inviteMods,
+ rooms: roomsArray,
+ },
+ });
layoutContextDispatch({ type: ACTIONS.SET_SIDEBAR_CONTENT_IS_OPEN, value: true });
layoutContextDispatch({ type: ACTIONS.SET_SIDEBAR_CONTENT_PANEL, value: PANELS.BREAKOUT });
- });
+ } catch (error) {
+ logger.error({ logCode: 'breakout_create_rooms_failed', extraInfo: { error } }, 'Failed to create breakout rooms');
+ }
}, [
- numberOfRooms, durationTime, freeJoin, record, captureNotes,
+ numberOfRooms, durationTime, freeJoin, record, captureNotes,
captureSlides, inviteMods, roomPresentations,
+ isCreatingBreakout, layoutContextDispatch,
]);
...
- disabled={!canStart}
+ disabled={!canStart || isCreatingBreakout}Also applies to: 296-308, 503-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`
at line 177, The createBreakoutRoom mutation is unguarded and lacks error
handling; add an in-flight guard and explicit error handling around the mutation
call to prevent duplicate submissions and surfacing failures. Introduce a local
loading flag (e.g., isCreating) in the component state and set it true before
calling createBreakoutRoom(BREAKOUT_ROOM_CREATE, ...) and false in finally;
early-return if isCreating is true to block rapid clicks. Wrap the mutation call
in try/catch, inspect the mutation result for errors, and update component state
or display a user-facing error (e.g., setCreateError or a toast) on failure
instead of silently failing. Apply the same pattern to the other mutation usages
referenced (the blocks around lines indicated: the second create handler at
~296-308 and the handler at ~503-505) so all create-breakout mutation flows use
the isCreating guard, try/catch, result error check, and UI feedback.
|
This pull request has conflicts ☹ |
Collaborator
Author
Screencast.from.03-03-2026.16.17.35.webm |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx (1)
181-181:⚠️ Potential issue | 🟠 MajorProtect
createBreakoutRoomfrom duplicate submits and add error handling.The mutation lacks an in-flight guard and error handling. Rapid clicks can fire duplicate create requests, and failures are silently ignored.
🐛 Proposed fix
- const [createBreakoutRoom] = useMutation(BREAKOUT_ROOM_CREATE); + const [createBreakoutRoom, { loading: isCreatingBreakout }] = useMutation(BREAKOUT_ROOM_CREATE);Then update
handleCreateRoom:const handleCreateRoom = useCallback(() => { + if (isCreatingBreakout) return; const totalMinutes = durationTime; if (totalMinutes < MIN_BREAKOUT_TIME) return; // ... rest of validation ... - createBreakoutRoom({ + createBreakoutRoom({ variables: { /* ... */ }, - }).then(() => { + }).then(() => { layoutContextDispatch({ type: ACTIONS.SET_SIDEBAR_CONTENT_IS_OPEN, value: true }); layoutContextDispatch({ type: ACTIONS.SET_SIDEBAR_CONTENT_PANEL, value: PANELS.BREAKOUT }); + }).catch((error) => { + logger.error({ + logCode: 'breakout_create_rooms_failed', + extraInfo: { error }, + }, 'Failed to create breakout rooms'); }); - }, [/* deps */]); + }, [/* deps */, isCreatingBreakout]);And disable the button during creation:
<Styled.StartButton - disabled={!canStart} + disabled={!canStart || isCreatingBreakout} onClick={handleCreateRoom}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx` at line 181, The BREAKOUT_ROOM_CREATE mutation hook (createBreakoutRoom) needs an in-flight guard and error handling to prevent duplicate submits and surface failures: update the component to track a local "creating" state (e.g., useState<boolean>) that is set true before calling createBreakoutRoom and false in both the success and catch/finally paths, early-return if creating is true inside handleCreateRoom to block duplicate clicks, call createBreakoutRoom with try/catch (or .catch) to handle errors and log/display them (e.g., set an error state or call existing notification/logger), and bind the "creating" state to the Create button's disabled prop so the button is disabled while the mutation is inflight; reference createBreakoutRoom, BREAKOUT_ROOM_CREATE, and handleCreateRoom when making these changes.bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx (3)
257-259:⚠️ Potential issue | 🟡 MinorTrack and clear the inner timeout to avoid post-unmount state updates.
setEditingTime(null)is scheduled in a nested timer that is never cleaned up.🛠️ Proposed fix
const debounceTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + const innerTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); ... useEffect(() => () => { if (debounceTimerRef.current) clearTimeout(debounceTimerRef.current); + if (innerTimerRef.current) clearTimeout(innerTimerRef.current); }, []); ... debounceTimerRef.current = setTimeout(() => { const newMinutes = Math.max(1, Math.ceil(clamped / 60)); breakoutRoomSetTime({ variables: { timeInMinutes: newMinutes } }); - setTimeout(() => setEditingTime(null), 500); + if (innerTimerRef.current) clearTimeout(innerTimerRef.current); + innerTimerRef.current = setTimeout(() => setEditingTime(null), 500); }, DEBOUNCE_DELAY);Also applies to: 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 257 - 259, The nested setTimeout that calls setEditingTime(null) isn't being tracked/cleared, causing possible post-unmount state updates; add a ref (e.g., editingTimerRef) or reuse debounceTimerRef to store the inner timeout id whenever you schedule the nested timer (the one that invokes setEditingTime), and update the existing cleanup in the useEffect teardown to clear that inner timeout as well (clearTimeout(editingTimerRef.current) and null it) so setEditingTime is never called after unmount; update locations that schedule the nested timer (the code surrounding setEditingTime(...) and debounceTimerRef) accordingly.
336-349:⚠️ Potential issue | 🟠 MajorRollback optimistic move when
breakoutRoomMoveUserfails.Pending move state is added optimistically but not reverted on mutation rejection.
🛡️ Proposed fix
breakoutRoomMoveUser({ variables: { userId: droppedUserId, fromBreakoutRoomMeetingId: fromRoomMeetingId, toBreakoutRoomMeetingId: toRoomId, }, - }); + }).catch((error) => { + setPendingMoves((prev) => { + const next = new Map(prev); + const pending = next.get(droppedUserId); + if (pending?.toRoomId === toRoomId) next.delete(droppedUserId); + return next; + }); + // keep visible for observability + // eslint-disable-next-line no-console + console.error('breakoutRoomMoveUser failed', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 336 - 349, The optimistic pending move added via setPendingMoves (creating a new Map and setting droppedUserId) must be rolled back if breakoutRoomMoveUser fails: after calling breakoutRoomMoveUser({...}) attach error handling (use the mutation's .catch or the mutation hook's onError callback) that calls setPendingMoves(prev => { const next = new Map(prev); next.delete(droppedUserId); return next; }) so the pending move is removed on rejection; keep the optimistic add as-is but ensure the rollback uses the same droppedUserId key and setPendingMoves updater to restore state consistently.
133-145:⚠️ Potential issue | 🟠 MajorRemove
return undefinedfromforEachcallbacks to unblock lint.These callbacks still return values, triggering
lint/suspicious/useIterableCallbackReturn.♻️ Proposed fix
breakouts.forEach((b) => { seqToMeetingId.set(b.sequence, b.breakoutRoomMeetingId); - return undefined; }); const alreadyAssigned = new Set<string>(); breakouts.forEach((b) => { b.participants.filter((p) => !p.isAudioOnly).forEach((p) => { alreadyAssigned.add(p.userId); - return undefined; }); - return undefined; }); ... newPending.forEach((val, key) => { if (!next.has(key)) next.set(key, val); - return undefined; }); ... breakouts.forEach((b) => { b.participants .filter((p) => !p.isAudioOnly) .forEach((p) => { ids.add(p.userId); - return undefined; }); - return undefined; }); ... pendingMoves.forEach((move, uId) => { if (move.toRoomId === breakout.breakoutRoomMeetingId && !existingIds.has(uId)) { pendingIncoming.push({ userId: uId, name: move.userName }); } - return undefined; });Also applies to: 159-162, 173-181, 222-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 133 - 145, In the shown forEach callbacks (the breakouts.forEach that calls seqToMeetingId.set and the nested participants.forEach that adds to alreadyAssigned) remove the trailing "return undefined" so the callbacks don't return a value; similarly, find and delete any other "return undefined" statements inside forEach callbacks in this component (other breakouts.forEach/participants.forEach uses) to satisfy linting (look for uses of breakouts, seqToMeetingId, alreadyAssigned, and participants in the file and remove the explicit returns inside those forEach arrow functions).
🧹 Nitpick comments (9)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.ts (2)
103-135: Extract shared action-button styles to avoid drift.
CallModeratorBtnandReturnBtnduplicate the same base button rules (width, font size, padding, radius, span truncation). Pull the shared block into one reusable style fragment.♻️ Proposed refactor
-import styled from 'styled-components'; +import styled, { css } from 'styled-components'; @@ +const ActionButtonBase = css` + width: 100%; + font-size: 1rem; + padding: 0.75rem 1.5rem; + border-radius: 1rem; + + & > span { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } +`; + // `@ts-ignore` - as Button comes from JS, we can't provide its props const CallModeratorBtn = styled(Button)` - width: 100%; - font-size: 1rem; - padding: 0.75rem 1.5rem; - border-radius: 1rem; - - & > span { - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - } + ${ActionButtonBase} `; @@ // `@ts-ignore` - as Button comes from JS, we can't provide its props const ReturnBtn = styled(Button)` - width: 100%; - font-size: 1rem; - padding: 0.75rem 1.5rem; + ${ActionButtonBase} background-color: ${colorWhite}; color: ${colorText}; border: 1px solid ${colorGrayLighter}; - border-radius: 1rem; - - & > span { - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - } &:hover { background-color: ${colorBlueAux}; } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.ts` around lines 103 - 135, CallModeratorBtn and ReturnBtn duplicate base button styles; extract the shared rules (width, font-size, padding, border-radius and the & > span truncation) into a reusable styled-component fragment or mixin (e.g., baseActionButtonStyles) and apply it to both CallModeratorBtn and ReturnBtn, leaving only the unique rules (ReturnBtn's background-color, color, border and &:hover) in ReturnBtn; ensure the shared fragment is imported/defined in the same module and used via styled(Button)` ${baseActionButtonStyles} ` to avoid style drift.
102-117: Replace@ts-ignorewith a typed cast for the Button component.Lines 102 and 116 use broad
@ts-ignoresuppression. Since the Button component is written in JavaScript without TypeScript definitions, use a type assertion instead to preserve type safety for the styled component itself. For example:const CallModeratorBtn = styled(Button as React.ComponentType<any>)` width: 100%; font-size: 1rem; padding: 0.75rem 1.5rem; border-radius: 1rem; & > span { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } `;This approach suppresses the error at the import point only, allowing TypeScript to still catch issues within the styled component's CSS logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.ts` around lines 102 - 117, Replace the broad `@ts-ignore` comments applied to the JS Button by casting Button to a React component type when creating the styled components (e.g., for CallModeratorBtn and ReturnBtn) so TypeScript still validates the styled wrapper; locate the styled(Button) usages for CallModeratorBtn and ReturnBtn and change them to use a type assertion like Button as React.ComponentType<any> before passing to styled.bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts (1)
229-238: Hardcoded color breaks theme consistency.
#e8f0feis hardcoded while other components use palette variables. This may not adapt well to dark mode or custom themes.♻️ Suggested fix
Consider using a palette variable or deriving from
colorPrimarywith opacity:export const InfoBanner = styled.div` display: flex; align-items: flex-start; gap: 0.5rem; - background: `#e8f0fe`; + background: ${colorPrimary}1a; /* ~10% opacity */ border-radius: 0.75rem; padding: 0.75rem; margin: 0.5rem 0; position: relative; `;Alternatively, add a dedicated palette variable for info banner backgrounds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts` around lines 229 - 238, The InfoBanner styled component currently hardcodes the background color "#e8f0fe"; update the InfoBanner definition to use the theme/palette instead (e.g., a dedicated palette variable like palette.infoBackground or derive from palette.colorPrimary with opacity) so the banner respects dark mode and custom themes; locate the export const InfoBanner = styled.div and replace the hardcoded background with the palette reference or an rgba/alpha derivation based on palette.colorPrimary.bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx (2)
211-211: Clarify the seconds rounding behavior.The current implementation rounds up: any non-zero seconds adds 1 minute to duration. This is likely intentional since the mutation accepts
durationInMinutes, but consider adding a brief comment explaining this design choice.📝 Suggested documentation
+ // Round up: if any seconds are specified, add 1 minute (backend accepts minutes only) const durationTime = (hours * 60) + minutes + (seconds > 0 ? 1 : 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx` at line 211, The duration calculation for creating a breakout room currently rounds any non-zero seconds up to the next minute by computing durationTime = (hours * 60) + minutes + (seconds > 0 ? 1 : 0); add a concise inline comment next to the durationTime declaration explaining that seconds are intentionally rounded up because the backend/mutation expects durationInMinutes and partial minutes should count as a full minute; reference the durationTime constant in component.tsx so future readers understand this design decision.
313-316:useCallbackdependency array may be incomplete.
layoutContextDispatch,durationInSeconds,createdTime,timeSync, and functions likegetCaptureFilename,getRoomPresentationare used insidehandleCreateRoombut not listed in dependencies. While some may be stable, explicitly listing them (or wrapping inner functions withuseCallback) prevents subtle bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx` around lines 313 - 316, The useCallback for handleCreateRoom has an incomplete dependency array; update it to include all external values used inside (e.g., layoutContextDispatch, durationInSeconds, createdTime, timeSync, getCaptureFilename, getRoomPresentation and any other helpers or context values referenced) OR wrap those helper functions (getCaptureFilename, getRoomPresentation) and unstable values in their own useCallback/useMemo so they are stable; then add the stable identifiers to the handleCreateRoom dependency array to ensure React sees all dependencies (locate handleCreateRoom in the component and adjust its dependency list or stabilize the inner functions accordingly).bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx (2)
31-35: Consider addingresetRoomsto dependency array.The
useEffectdepends onresetRoomsbeing called but it's not in the dependency array. IfresetRoomswere ever recreated by the parent, this could lead to stale closure issues.♻️ Suggested fix
useEffect(() => { if (numberOfRooms) { resetRooms(numberOfRooms); } - }, [numberOfRooms]); + }, [numberOfRooms, resetRooms]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx` around lines 31 - 35, The effect that calls resetRooms when numberOfRooms changes currently lists only numberOfRooms in its dependency array; update the useEffect dependency array to include resetRooms (i.e., [numberOfRooms, resetRooms]) or ensure resetRooms is memoized (e.g., via useCallback in the parent) so the closure stays correct; locate the useEffect in the component that references numberOfRooms and resetRooms and update the dependency list accordingly.
57-65: Add defensive validation for malformed drag data.While
lastIndexOf('-')handles IDs containing hyphens (improvement over the previoussplit('-')), there's no guard against malformed data. IfseparatorIndexis-1or0, theuserIdbecomes empty or invalid.🛡️ Proposed defensive fix
const drop = (roomNumber: number) => (ev: React.DragEvent) => { ev.preventDefault(); const data = ev.dataTransfer.getData('text'); const separatorIndex = data.lastIndexOf('-'); + if (separatorIndex <= 0) return; const userId = data.substring(0, separatorIndex); const from = data.substring(separatorIndex + 1); moveUser(userId, Number(from), roomNumber); setSelectedId(''); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx` around lines 57 - 65, The drop handler (function drop) doesn't validate the dragged data and can produce an empty or invalid userId when separatorIndex is -1 or 0; update drop to defensively check that separatorIndex > 0, that userId (substring(0, separatorIndex)) is non-empty, and that the parsed from value is a valid number before calling moveUser(userId, Number(from), roomNumber); if validation fails, bail out (optionally log or return) but still clear selection with setSelectedId('') to keep UI consistent.bigbluebutton-html5/public/locales/en.json (1)
35-35: Please confirm these locale additions follow the Transifex workflow.The new keys look correct, but this repository’s localization process may require managing locale updates through Transifex instead of direct PR edits.
Based on learnings,
public/locales/*.jsonupdates are expected to be handled through Transifex synchronization.Also applies to: 814-816, 1489-1504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/public/locales/en.json` at line 35, This change touches locale keys (e.g., "app.chat.breakoutCallModerator" in public/locales/en.json and other blocks around lines 814-816 and 1489-1504) which must be managed via the project's Transifex workflow rather than direct edits; revert the JSON edits in this PR, then add or update these keys through the Transifex project (or ask the i18n admin to create the keys), and once Transifex sync has run, pull the generated locale files back into the repo so the keys are present in public/locales/*.json.bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx (1)
267-304: Consider adding defensive checks for invalid numeric values.While the input elements use
type="number"with min/max constraints (which prevents invalid user input), adding explicit guards againstNaNandInfinityincommitTimeChangeandhandleTimerInputChangeis reasonable defensive programming. This protects against potential edge cases and makes the intent clearer.Suggested guards
const commitTimeChange = useCallback((newTotalSeconds: number) => { + if (!Number.isFinite(newTotalSeconds)) return; const clamped = Math.max(60, newTotalSeconds); setEditingTime(clamped);const handleTimerInputChange = useCallback(( field: 'h' | 'm' | 's', value: number, ) => { + if (!Number.isFinite(value)) return; let newH = hours;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx` around lines 267 - 304, Add defensive numeric validation to commitTimeChange and handleTimerInputChange: in commitTimeChange (used by adjustTime) verify newTotalSeconds is a finite number (not NaN or Infinity), coerce to a Number, and bail or clamp to a safe minimum (e.g., 60) before using it and before scheduling the debounce callback; likewise in handleTimerInputChange ensure parsed/received value and computed totalSeconds are finite numbers and fall back to safe defaults (0 or minimum allowed) before calling commitTimeChange. Reference commitTimeChange, handleTimerInputChange, and adjustTime when making these checks so invalid inputs are normalized early and never passed into setTimeout, setEditingTime, or breakoutRoomSetTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 319-334: The drag payload currently uses delimiter-split text in
dragStart and handleDrop which breaks if fields contain '::'; update dragStart
to serialize an object {visUserId, fromRoomId, userName} via JSON.stringify and
set it under a structured MIME type (e.g. 'application/json'), and update
handleDrop to read ev.dataTransfer.getData('application/json'), JSON.parse it
inside a try/catch, validate presence of visUserId/fromRoomId/userName before
destructuring and early-return on invalid data or same-room moves; replace the
split('::') logic and ensure errors are logged or ignored gracefully.
---
Duplicate comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 257-259: The nested setTimeout that calls setEditingTime(null)
isn't being tracked/cleared, causing possible post-unmount state updates; add a
ref (e.g., editingTimerRef) or reuse debounceTimerRef to store the inner timeout
id whenever you schedule the nested timer (the one that invokes setEditingTime),
and update the existing cleanup in the useEffect teardown to clear that inner
timeout as well (clearTimeout(editingTimerRef.current) and null it) so
setEditingTime is never called after unmount; update locations that schedule the
nested timer (the code surrounding setEditingTime(...) and debounceTimerRef)
accordingly.
- Around line 336-349: The optimistic pending move added via setPendingMoves
(creating a new Map and setting droppedUserId) must be rolled back if
breakoutRoomMoveUser fails: after calling breakoutRoomMoveUser({...}) attach
error handling (use the mutation's .catch or the mutation hook's onError
callback) that calls setPendingMoves(prev => { const next = new Map(prev);
next.delete(droppedUserId); return next; }) so the pending move is removed on
rejection; keep the optimistic add as-is but ensure the rollback uses the same
droppedUserId key and setPendingMoves updater to restore state consistently.
- Around line 133-145: In the shown forEach callbacks (the breakouts.forEach
that calls seqToMeetingId.set and the nested participants.forEach that adds to
alreadyAssigned) remove the trailing "return undefined" so the callbacks don't
return a value; similarly, find and delete any other "return undefined"
statements inside forEach callbacks in this component (other
breakouts.forEach/participants.forEach uses) to satisfy linting (look for uses
of breakouts, seqToMeetingId, alreadyAssigned, and participants in the file and
remove the explicit returns inside those forEach arrow functions).
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Line 181: The BREAKOUT_ROOM_CREATE mutation hook (createBreakoutRoom) needs an
in-flight guard and error handling to prevent duplicate submits and surface
failures: update the component to track a local "creating" state (e.g.,
useState<boolean>) that is set true before calling createBreakoutRoom and false
in both the success and catch/finally paths, early-return if creating is true
inside handleCreateRoom to block duplicate clicks, call createBreakoutRoom with
try/catch (or .catch) to handle errors and log/display them (e.g., set an error
state or call existing notification/logger), and bind the "creating" state to
the Create button's disabled prop so the button is disabled while the mutation
is inflight; reference createBreakoutRoom, BREAKOUT_ROOM_CREATE, and
handleCreateRoom when making these changes.
---
Nitpick comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.ts`:
- Around line 103-135: CallModeratorBtn and ReturnBtn duplicate base button
styles; extract the shared rules (width, font-size, padding, border-radius and
the & > span truncation) into a reusable styled-component fragment or mixin
(e.g., baseActionButtonStyles) and apply it to both CallModeratorBtn and
ReturnBtn, leaving only the unique rules (ReturnBtn's background-color, color,
border and &:hover) in ReturnBtn; ensure the shared fragment is imported/defined
in the same module and used via styled(Button)` ${baseActionButtonStyles} ` to
avoid style drift.
- Around line 102-117: Replace the broad `@ts-ignore` comments applied to the JS
Button by casting Button to a React component type when creating the styled
components (e.g., for CallModeratorBtn and ReturnBtn) so TypeScript still
validates the styled wrapper; locate the styled(Button) usages for
CallModeratorBtn and ReturnBtn and change them to use a type assertion like
Button as React.ComponentType<any> before passing to styled.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx`:
- Around line 267-304: Add defensive numeric validation to commitTimeChange and
handleTimerInputChange: in commitTimeChange (used by adjustTime) verify
newTotalSeconds is a finite number (not NaN or Infinity), coerce to a Number,
and bail or clamp to a safe minimum (e.g., 60) before using it and before
scheduling the debounce callback; likewise in handleTimerInputChange ensure
parsed/received value and computed totalSeconds are finite numbers and fall back
to safe defaults (0 or minimum allowed) before calling commitTimeChange.
Reference commitTimeChange, handleTimerInputChange, and adjustTime when making
these checks so invalid inputs are normalized early and never passed into
setTimeout, setEditingTime, or breakoutRoomSetTime.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsx`:
- Line 211: The duration calculation for creating a breakout room currently
rounds any non-zero seconds up to the next minute by computing durationTime =
(hours * 60) + minutes + (seconds > 0 ? 1 : 0); add a concise inline comment
next to the durationTime declaration explaining that seconds are intentionally
rounded up because the backend/mutation expects durationInMinutes and partial
minutes should count as a full minute; reference the durationTime constant in
component.tsx so future readers understand this design decision.
- Around line 313-316: The useCallback for handleCreateRoom has an incomplete
dependency array; update it to include all external values used inside (e.g.,
layoutContextDispatch, durationInSeconds, createdTime, timeSync,
getCaptureFilename, getRoomPresentation and any other helpers or context values
referenced) OR wrap those helper functions (getCaptureFilename,
getRoomPresentation) and unstable values in their own useCallback/useMemo so
they are stable; then add the stable identifiers to the handleCreateRoom
dependency array to ensure React sees all dependencies (locate handleCreateRoom
in the component and adjust its dependency list or stabilize the inner functions
accordingly).
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsx`:
- Around line 31-35: The effect that calls resetRooms when numberOfRooms changes
currently lists only numberOfRooms in its dependency array; update the useEffect
dependency array to include resetRooms (i.e., [numberOfRooms, resetRooms]) or
ensure resetRooms is memoized (e.g., via useCallback in the parent) so the
closure stays correct; locate the useEffect in the component that references
numberOfRooms and resetRooms and update the dependency list accordingly.
- Around line 57-65: The drop handler (function drop) doesn't validate the
dragged data and can produce an empty or invalid userId when separatorIndex is
-1 or 0; update drop to defensively check that separatorIndex > 0, that userId
(substring(0, separatorIndex)) is non-empty, and that the parsed from value is a
valid number before calling moveUser(userId, Number(from), roomNumber); if
validation fails, bail out (optionally log or return) but still clear selection
with setSelectedId('') to keep UI consistent.
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.ts`:
- Around line 229-238: The InfoBanner styled component currently hardcodes the
background color "#e8f0fe"; update the InfoBanner definition to use the
theme/palette instead (e.g., a dedicated palette variable like
palette.infoBackground or derive from palette.colorPrimary with opacity) so the
banner respects dark mode and custom themes; locate the export const InfoBanner
= styled.div and replace the hardcoded background with the palette reference or
an rgba/alpha derivation based on palette.colorPrimary.
In `@bigbluebutton-html5/public/locales/en.json`:
- Line 35: This change touches locale keys (e.g.,
"app.chat.breakoutCallModerator" in public/locales/en.json and other blocks
around lines 814-816 and 1489-1504) which must be managed via the project's
Transifex workflow rather than direct edits; revert the JSON edits in this PR,
then add or update these keys through the Transifex project (or ask the i18n
admin to create the keys), and once Transifex sync has run, pull the generated
locale files back into the repo so the keys are present in
public/locales/*.json.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/participant-room/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/styles.tsbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/sidebar-room-assignment.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/sidebar-create-breakout/styles.tsbigbluebutton-html5/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- bigbluebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/styles.ts
...luebutton-html5/imports/ui/components/breakout-room/breakout-room/running-room/component.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/component.tsx`:
- Around line 698-715: The sidebar branch should respect isOpen and mirror modal
dismiss behavior: only render the Styled.SidebarPanelContent when
renderInSidebar && isOpen, and change the SidebarHeaderContainer rightButton
onClick to conditionally call setUpdateUsersWhileRunning(false) when isUpdate is
true, otherwise call setIsOpen(false); apply the same change to the other
identical sidebar return (the block around the other
Styled.SidebarPanelContent), ensuring innerContent remains unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/component.tsxbigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/queries.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/queries.ts
bigbluebutton-html5/imports/ui/components/breakout-room/create-breakout-room/component.tsx
Show resolved
Hide resolved
|
This pull request has conflicts ☹ |
…all Moderator and codeRabbit fixes
…or room cards in both create and running room views
|
🚨 Automated tests failed |
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.
What does this PR do?
Replaces the legacy breakout room modals with a unified sidebar experience for both moderators (creation & management) and participants (status & actions).
Closes Issue(s)
Closes #24432
More
Screencast.from.27-02-2026.14.50.51.webm