Ref(html5): Re-introduce raise hand audio notification and full user options for raised hand users list#24214
Conversation
…options for raised hand users list
WalkthroughThis change adds a raise-hand notification system. A new RaiseHandNotifier presentational component detects newly raised hands and can play audio and send push notifications. A RaiseHandNotifierContainer subscribes to RAISED_HAND_USERS, maps current user context and settings, and supplies props to the notifier. App now imports and conditionally renders the notifier when Sequence DiagramsequenceDiagram
participant App
participant RaiseHandNotifierContainer
participant GraphQL as GraphQL Subscription
participant RaiseHandNotifier
participant AudioManager
participant NotificationService
App->>RaiseHandNotifierContainer: render (if isRaiseHandEnabled)
RaiseHandNotifierContainer->>GraphQL: subscribe to RAISED_HAND_USERS
GraphQL-->>RaiseHandNotifierContainer: RaisedHandUsers[]
RaiseHandNotifierContainer->>RaiseHandNotifier: props (users, settings, user context)
RaiseHandNotifier->>RaiseHandNotifier: diff current vs previous users
alt New raised-hand(s) detected (exclude current user) and user has presenter/moderator role
opt audio alerts enabled
RaiseHandNotifier->>AudioManager: playAlertSound(outputDeviceId or 'default')
AudioManager-->>RaiseHandNotifier: played
end
opt push alerts enabled
RaiseHandNotifier->>NotificationService: notify(text)
NotificationService-->>RaiseHandNotifier: sent
end
end
RaiseHandNotifier->>RaiseHandNotifier: update previous users ref
RaiseHandNotifier-->>App: (no visible UI returned / null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/container.tsx (1)
43-43: Redundant fallback forcurrentUserId.The fallback
?? ''on line 43 is redundant because line 34 already guards against!currentUser. SincecurrentUseris guaranteed to exist at this point, the nullish coalescing can be removed for clarity.Apply this diff to simplify:
- currentUserId={currentUser.userId ?? ''} + currentUserId={currentUser.userId}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bigbluebutton-html5/imports/ui/components/app/component.jsx(4 hunks)bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx(1 hunks)bigbluebutton-html5/imports/ui/components/raisehand-notifier/container.tsx(1 hunks)bigbluebutton-html5/imports/ui/components/user-list/user-list-content/raised-hands/component.tsx(2 hunks)bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts(2 hunks)bigbluebutton-html5/imports/ui/services/audio-manager/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx (1)
bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts (1)
RaisedHandUser(99-110)
bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts (1)
bigbluebutton-html5/imports/ui/Types/user.ts (1)
User(61-111)
bigbluebutton-html5/imports/ui/components/user-list/user-list-content/raised-hands/component.tsx (1)
bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts (2)
RaisedHandUsersSubscriptionResponse(112-114)RAISED_HAND_USERS(116-134)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/container.tsx (1)
bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts (2)
RaisedHandUsersSubscriptionResponse(112-114)RAISED_HAND_USERS(116-134)
bigbluebutton-html5/imports/ui/services/audio-manager/index.js (1)
bigbluebutton-html5/imports/ui/components/audio/audio-modal/container.jsx (1)
outputDeviceId(82-82)
bigbluebutton-html5/imports/ui/components/app/component.jsx (2)
bigbluebutton-html5/imports/ui/components/app/container.jsx (1)
isRaiseHandEnabled(97-97)bigbluebutton-html5/imports/ui/components/actions-bar/container.jsx (1)
isRaiseHandEnabled(95-95)
🔇 Additional comments (2)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/container.tsx (2)
25-31: Verify error handling behavior: should rendering continue after subscription failure?The error handling correctly logs the issue and sets the connection status, but the component continues to render with an empty
raiseHandUsersarray. Please confirm whether this is the intended behavior or if the component should returnnullwhen the subscription fails.If rendering should stop on error, apply this diff:
if (usersError) { connectionStatus.setSubscriptionFailed(true); logger.error({ logCode: 'raisehand_notifier_container_subscription_error', extraInfo: { usersError }, }, 'Error on requesting raise hand data'); + return null; }
1-10: LGTM: Clean component structure and proper hook usage.The imports, subscription setup, and render logic are well-structured. The use of
useCurrentUserwith a selector function anduseDeduplicatedSubscriptionwith proper typing follows best practices. The boolean coercion with!!ensures type safety when passing props to the presentational component.Also applies to: 13-23, 36-42
| const newRaisedHandUsers = raiseHandUsers.filter( | ||
| (user) => !previousUserIdsSet.has(user.userId), | ||
| ); | ||
| const hasNewRaisedHand = newRaisedHandUsers.length > 0; | ||
| const isAllowed = isModerator || isPresenter; | ||
|
|
||
| if (hasNewRaisedHand && isAllowed) { | ||
| if (raiseHandAudioAlert) { | ||
| AudioService.playAlertSound(getRaiseHandSoundUrl()); | ||
| } | ||
|
|
||
| if (raiseHandPushAlert) { | ||
| const nonCurrentUser = newRaisedHandUsers | ||
| .filter((user) => user.userId !== currentUserId); | ||
| const names = nonCurrentUser | ||
| .map((user) => user.name) | ||
| .filter((name): name is string => !!name && name.length > 0); | ||
| const message = buildRaisedHandMessage(names); | ||
|
|
||
| if (message) { | ||
| notify(message, 'info', 'hand'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid audible alert for presenter self-raise
We're still playing the alert sound when the only new raised hand belongs to the current presenter. The PR objective calls out filtering presenter self-raises, so the audible notification should be skipped as well; otherwise the presenter hears the chime whenever they raise their own hand. Please filter the current user out before deciding to play the sound and reuse that filtered list for the toast names. Based on PR objectives.
- const hasNewRaisedHand = newRaisedHandUsers.length > 0;
+ const newRaisedHandUsersExcludingSelf = newRaisedHandUsers.filter(
+ (user) => user.userId !== currentUserId,
+ );
+ const hasNewRaisedHand = newRaisedHandUsersExcludingSelf.length > 0;
@@
- if (raiseHandAudioAlert) {
+ if (raiseHandAudioAlert && newRaisedHandUsersExcludingSelf.length > 0) {
AudioService.playAlertSound(getRaiseHandSoundUrl());
}
if (raiseHandPushAlert) {
- const nonCurrentUser = newRaisedHandUsers
- .filter((user) => user.userId !== currentUserId);
- const names = nonCurrentUser
+ const names = newRaisedHandUsersExcludingSelf
.map((user) => user.name)🤖 Prompt for AI Agents
In bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx
around lines 64 to 87, filter out the current user before deciding there are new
raised hands and reuse that filtered list for both the audible alert and the
push/toast names: compute newRaisedHandUsers as you do, then derive
nonCurrentNewRaisedHandUsers = newRaisedHandUsers.filter(u => u.userId !==
currentUserId); use nonCurrentNewRaisedHandUsers.length > 0 (together with
isAllowed) to decide whether to play the alert sound and to build the names for
the push notification so the presenter’s own raise is ignored for both sound and
toast.
| // @ts-ignore | ||
| const { raiseHandAudioAlerts, raiseHandPushAlerts } = useSettings(SETTINGS.APPLICATION); |
There was a problem hiding this comment.
Remove @ts-ignore and add proper typing.
The @ts-ignore directive suppresses TypeScript errors without explanation, which makes the code fragile and hides potential type issues. Please resolve the underlying type mismatch by either:
- Adding proper type definitions to
useSettingsfor the APPLICATION settings shape, or - Using a typed assertion with explicit interface definition
Apply this approach to properly type the settings:
- // @ts-ignore
- const { raiseHandAudioAlerts, raiseHandPushAlerts } = useSettings(SETTINGS.APPLICATION);
+ const settings = useSettings(SETTINGS.APPLICATION) as {
+ raiseHandAudioAlerts?: boolean;
+ raiseHandPushAlerts?: boolean;
+ };
+ const { raiseHandAudioAlerts, raiseHandPushAlerts } = settings;Or better yet, define a proper interface for APPLICATION settings and use it consistently across the codebase.
📝 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.
| // @ts-ignore | |
| const { raiseHandAudioAlerts, raiseHandPushAlerts } = useSettings(SETTINGS.APPLICATION); | |
| const settings = useSettings(SETTINGS.APPLICATION) as { | |
| raiseHandAudioAlerts?: boolean; | |
| raiseHandPushAlerts?: boolean; | |
| }; | |
| const { raiseHandAudioAlerts, raiseHandPushAlerts } = settings; |
🤖 Prompt for AI Agents
In bigbluebutton-html5/imports/ui/components/raisehand-notifier/container.tsx
around lines 32-33, remove the `// @ts-ignore` and fix the typing by declaring
an interface for the APPLICATION settings (including raiseHandAudioAlerts and
raiseHandPushAlerts) and use that interface with useSettings (or pass it as a
generic) so the returned object is correctly typed; update any related type
exports so the APPLICATION shape is reused across the codebase and replace the
ignored line with a typed call or a typed assertion to eliminate the ts-ignore.
|
IMO it's not necessary to include the user's name here! 
|
ramonlsouza
left a comment
ramonlsouza
left a comment
There was a problem hiding this comment.
We're still playing the alert sound when the only new raised hand belongs to the current presenter. The PR objective calls out filtering presenter self-raises, so the audible notification should be skipped as well
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx (1)
76-77: Remove redundant filtering after fixing line 66.After fixing the filter at line 66 to exclude the current user, these lines become unnecessary since
newRaisedHandUsersalready excludescurrentUserId.Apply this diff to remove the redundant filter:
if (raiseHandPushAlert) { - const nonCurrentUser = newRaisedHandUsers - .filter((user) => user.userId !== currentUserId); - const names = nonCurrentUser + const names = newRaisedHandUsers .map((user) => user.name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx(1 hunks)bigbluebutton-html5/imports/ui/components/user-list/user-list-content/user-participants/user-list-participants/user-actions/component.tsx(1 hunks)bigbluebutton-html5/public/locales/en.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T17:52:37.432Z
Learnt from: guiiBecker
Repo: bigbluebutton/bigbluebutton PR: 24084
File: bigbluebutton-html5/public/locales/pt.json:323-323
Timestamp: 2025-10-15T17:52:37.432Z
Learning: Translation issues in locale files (bigbluebutton-html5/public/locales/*.json) should be updated in Transifex, the translation management platform used by the BigBlueButton project, rather than being fixed directly in pull requests.
Applied to files:
bigbluebutton-html5/public/locales/en.json
🧬 Code graph analysis (1)
bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx (1)
bigbluebutton-html5/imports/ui/core/graphql/queries/users.ts (1)
RaisedHandUser(99-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-package (bbb-html5)
🔇 Additional comments (6)
bigbluebutton-html5/public/locales/en.json (1)
717-717: LGTM! Proper placement and naming.The new translation key
app.actionsBar.reactions.lowUserHandis well-placed within the reactions namespace and follows the existing convention established byapp.actionsBar.reactions.lowHandon the previous line.bigbluebutton-html5/imports/ui/components/user-list/user-list-content/user-participants/user-list-participants/user-actions/component.tsx (1)
160-160: Verify translation key synchronization across locale files.The old translation key
app.statusNotifier.lowerHandDescOneUseris not referenced in code and has been successfully replaced. However, the old key remains in all 30+ locale files, while the new keyapp.actionsBar.reactions.lowUserHandis currently only defined in en.json. Per the project's guidelines, translation management should be coordinated through Transifex rather than updated directly in pull requests. Ensure this translation change is properly synchronized across all locale files through the Transifex platform.bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx (4)
1-14: LGTM!Imports and type definitions are well-structured. The shared
RaisedHandUsertype promotes consistency across the codebase.
16-25: LGTM!Internationalization setup correctly handles singular and plural cases.
27-53: LGTM!Helper functions are well-implemented. The message builder correctly handles empty, singular, and plural cases.
99-102: LGTM!Returning
nullis appropriate for a notification-only component with side effects.
| const newRaisedHandUsers = raiseHandUsers.filter( | ||
| (user) => !previousUserIdsSet.has(user.userId), | ||
| ).filter((user) => isPresenter && user.userId !== currentUserId); |
There was a problem hiding this comment.
Critical: Broken filter logic prevents notifications for non-presenters.
Line 66 uses isPresenter && user.userId !== currentUserId which is incorrect. The isPresenter boolean refers to whether the current user is a presenter, not the user being filtered. When the current user is not a presenter (e.g., a moderator-only), the condition evaluates to false for all users, removing everyone from newRaisedHandUsers and completely disabling notifications.
Apply this diff to filter out only the current user's own raise:
const newRaisedHandUsers = raiseHandUsers.filter(
(user) => !previousUserIdsSet.has(user.userId),
-).filter((user) => isPresenter && user.userId !== currentUserId);
+).filter((user) => user.userId !== currentUserId);This relates to the past review comment which suggested filtering presenter self-raises, but the implementation introduced a logic error.
📝 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.
| const newRaisedHandUsers = raiseHandUsers.filter( | |
| (user) => !previousUserIdsSet.has(user.userId), | |
| ).filter((user) => isPresenter && user.userId !== currentUserId); | |
| const newRaisedHandUsers = raiseHandUsers.filter( | |
| (user) => !previousUserIdsSet.has(user.userId), | |
| ).filter((user) => user.userId !== currentUserId); |
🤖 Prompt for AI Agents
bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx lines
64-66: the filter currently uses "isPresenter && user.userId !== currentUserId"
which wrongly ties per-user filtering to whether the current user is a
presenter, causing all users to be filtered out when the current user is not a
presenter; replace that filter so it only excludes the current user's own raise
(i.e., drop the isPresenter check and filter by user.userId !== currentUserId)
so notifications for other users work regardless of the current user's presenter
status.
| }, [ | ||
| intl, | ||
| isModerator, | ||
| isPresenter, | ||
| raiseHandAudioAlert, | ||
| raiseHandPushAlert, | ||
| raiseHandUsers, | ||
| ]); |
There was a problem hiding this comment.
Add missing dependency to useEffect.
The currentUserId is used within the effect (lines 66, 77) but is not included in the dependency array. This could cause stale closure issues if currentUserId changes.
Apply this diff to add the missing dependency:
}, [
+ currentUserId,
intl,
isModerator,📝 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.
| }, [ | |
| intl, | |
| isModerator, | |
| isPresenter, | |
| raiseHandAudioAlert, | |
| raiseHandPushAlert, | |
| raiseHandUsers, | |
| ]); | |
| }, [ | |
| currentUserId, | |
| intl, | |
| isModerator, | |
| isPresenter, | |
| raiseHandAudioAlert, | |
| raiseHandPushAlert, | |
| raiseHandUsers, | |
| ]); |
🤖 Prompt for AI Agents
In bigbluebutton-html5/imports/ui/components/raisehand-notifier/component.tsx
around lines 90 to 97, the useEffect depends on currentUserId (used at lines ~66
and ~77) but currentUserId is missing from the dependency array; update the
dependency array to include currentUserId so the effect re-runs when it changes,
ensuring closures see the latest value.
Automated tests Summary✅ All the CI tests have passed! |
4 participants
What does this PR do?
Closes Issue(s)
Closes #24175
How to test
More
Simple notification
