Add avatar upload functionality #184
Conversation
…g in DeviceCheck component
WalkthroughAdds a session-scoped avatar upload and UI; threads an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant DeviceCheck as DeviceCheck (UI)
participant Room as Room (container)
participant VideoGrid as VideoGrid (render)
note right of DeviceCheck #E8F5E9: avatar state + media refs
User->>DeviceCheck: upload avatar file
DeviceCheck->>DeviceCheck: validate → create blob URL → set `avatar`
DeviceCheck->>DeviceCheck: revoke previous blob URL (if any)
User->>DeviceCheck: toggle video/audio
DeviceCheck->>Browser: if videoOn/audioOn → getUserMedia
alt stream returned
Browser-->>DeviceCheck: MediaStream
DeviceCheck->>DeviceCheck: store tracks in refs/state
DeviceCheck->>VideoElement: attach stream for preview
else error/denied
Browser-->>DeviceCheck: error
DeviceCheck->>User: show toast/error
end
DeviceCheck->>Room: render with `avatar` prop
Room->>VideoGrid: forward `avatar`
VideoGrid->>VideoGrid: if camOff → render `avatar` or fallback icon
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
🧹 Nitpick comments (6)
frontend/components/RTC/DeviceCheck.tsx (6)
115-118: Revoke avatar blob URL on unmount/changeAvoid leaking object URLs when the user changes/leaves.
useEffect(() => { getCamRef.current = getCam; }); +useEffect(() => { + return () => { + if (avatar?.startsWith("blob:")) URL.revokeObjectURL(avatar); + }; +}, [avatar]);
39-76: Reduce prompts: request audio+video in a single getUserMedia when both are onTwo sequential getUserMedia calls can double‑prompt and complicate device selection. Request both in one call when needed.
- // request camera stream only if videoOn is true - if (videoOn) { - try { - const videoStream = await navigator.mediaDevices.getUserMedia({ video: true }); - videoTrack = videoStream.getVideoTracks()[0] || null; - } catch (err) { - console.warn("Camera access denied or unavailable:", err); - toast.error("Camera Error", { description: "Could not access camera" }); - } - } - // Request microphone stream only if audioOn is true - if (audioOn) { - try { - const audioStream = await navigator.mediaDevices.getUserMedia({ audio: true }); - audioTrack = audioStream.getAudioTracks()[0] || null; - } catch (err) { - console.warn("Microphone access denied or unavailable:", err); - toast.error("Microphone Error", { description: "Could not access microphone" }); - } - } + // Request in one call when possible to reduce prompts + if (videoOn || audioOn) { + try { + const constraints: MediaStreamConstraints = { + video: !!videoOn, + audio: audioOn ? { echoCancellation: true, noiseSuppression: true, autoGainControl: true } : false, + }; + const stream = await navigator.mediaDevices.getUserMedia(constraints); + videoTrack = stream.getVideoTracks()[0] || null; + audioTrack = stream.getAudioTracks()[0] || null; + } catch (err) { + console.warn("Media access denied or unavailable:", err); + toast.error("Device Access Error", { description: "Could not access camera and/or microphone" }); + } + }
89-109: Consider also watching microphone permissions and device changesAdd microphone to the Permissions watcher where supported, and listen to navigator.mediaDevices.devicechange to re‑probe devices when hardware changes.
Example:
try { const mic = await navigator.permissions.query({ name: "microphone" as PermissionName }); mic.onchange = () => mic.state === "granted" && getCamRef.current(); } catch {} navigator.mediaDevices?.addEventListener?.("devicechange", () => getCamRef.current());
2-2: Type‑only import; drop default React importYou only need React for the event type. Prefer a type‑only import to avoid an unnecessary runtime binding. Based on learnings.
-import React, { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; +import type { ChangeEvent } from "react"; -const handleAvatarChange = (e: React.ChangeEvent<HTMLInputElement>) => { +const handleAvatarChange = (e: ChangeEvent<HTMLInputElement>) => {Also applies to: 30-30
120-130: Optionally reset avatar on leave to match “resets when leaving the session”If “leaves the session” means leaving the meeting, clear the avatar here (and revoke blob URL).
const handleOnLeave = () => { setJoined(false); try { localAudioTrack?.stop(); } catch {} try { localVideoTrack?.stop(); } catch {} setLocalAudioTrack(null); setLocalVideoTrack(null); + if (avatar?.startsWith("blob:")) URL.revokeObjectURL(avatar); + setAvatar(null); };
171-191: UI looks good; small a11y/perf nitNice placeholder UX. Consider adding decoding="async" to the img for quicker paint; optional.
- <img - src={avatar} - alt="Avatar" - className="h-24 w-24 rounded-full object-cover border border-white/20" - /> + <img + src={avatar} + alt="Avatar" + decoding="async" + className="h-24 w-24 rounded-full object-cover border border-white/20" + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/components/RTC/DeviceCheck.tsx(5 hunks)
🔇 Additional comments (2)
frontend/components/RTC/DeviceCheck.tsx (2)
25-27: LGTM: track refs + cleanup prevent leaksUsing refs for current tracks and stopping them on cleanup is solid and avoids dangling device streams.
Also applies to: 104-109
132-140: Avatar not passed to Room component during callThe review identifies a real implementation gap: DeviceCheck uses an avatar in the preview UI, but doesn't pass it to the Room component, which has no avatar prop or fallback UI when video is off.
The suggested change is technically correct—adding
avatar={avatar}to the Room component props would require:
- Adding
avatar?: string | nullto theRoomPropsinterface- Accepting it in the Room function signature
- Using it in the video-off rendering logic (likely within VideoGrid)
However, I cannot verify from the codebase whether avatar display during active calls is an actual acceptance requirement. You should manually confirm:
- Whether the feature spec requires showing avatars when participants disable their camera during the call
- How the VideoGrid component should integrate avatar display when video is off
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/components/RTC/DeviceCheck.tsx (1)
29-36: Harden avatar uploads: validate MIME/size, block SVG, use blob URLs, and revoke themCurrent code accepts image/*, reads as Data URL, and lacks validation. This risks large in-memory strings and potential SVG-based surprises. Validate type/size, avoid SVG, prefer object URLs, and revoke old URLs to prevent leaks.
Apply:
-// Handle avatar upload -const handleAvatarChange = (e: React.ChangeEvent<HTMLInputElement>) => { - const file = e.target.files?.[0]; - if (!file) return; - const reader = new FileReader(); - reader.onloadend = e => setAvatar(e.target?.result as string); - reader.readAsDataURL(file); -}; +// Handle avatar upload (validates + uses object URL) +const handleAvatarChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const file = e.target.files?.[0]; + if (!file) return; + const allowed = new Set(["image/png", "image/jpeg", "image/webp", "image/gif"]); + const MAX_MB = 5; + if (!allowed.has(file.type)) { + toast.error("Unsupported file type", { description: "Use PNG, JPEG, WebP, or GIF." }); + e.currentTarget.value = ""; + return; + } + if (file.size > MAX_MB * 1024 * 1024) { + toast.error("File too large", { description: `Max size is ${MAX_MB}MB.` }); + e.currentTarget.value = ""; + return; + } + const url = URL.createObjectURL(file); + setAvatar(prev => { + if (prev?.startsWith("blob:")) URL.revokeObjectURL(prev); + return url; + }); +};And tighten the input:
-<input - type="file" - accept="image/*" - onChange={handleAvatarChange} - className="hidden" -/> +<input + type="file" + accept="image/png,image/jpeg,image/webp,image/gif" + onChange={handleAvatarChange} + className="hidden" />Also applies to: 184-189
🧹 Nitpick comments (6)
frontend/components/RTC/Room.tsx (2)
12-18: Avoid duplicate “ensureRemoteStream” logic; keep a single sourceYou import ensureRemoteStream but also define ensureRemoteStreamLocal with overlapping behavior. Prefer one to prevent drift. If the local helper is preferred, drop the import; otherwise, reuse the util and delete the local helper.
Example (drop unused import):
-import { - ensureRemoteStream, - detachLocalPreview, +import { + detachLocalPreview, stopProvidedTracks, teardownPeers, toggleCameraTrack } from "./webrtc-utils";Also applies to: 81-96
218-221: Don’t request screen-share audio if unusedgetDisplayMedia asks for audio but you never attach the audio track, causing extra permission prompts. Request video-only.
-const screenStream = await navigator.mediaDevices.getDisplayMedia({ - video: true, - audio: true -}); +const screenStream = await navigator.mediaDevices.getDisplayMedia({ + video: true, + audio: false +});frontend/components/RTC/DeviceCheck.tsx (3)
115-118: Revoke object URLs on unmount/change to prevent memory leaksAdd a cleanup effect to revoke the current blob URL when the component unmounts or when avatar changes.
useEffect(() => { getCamRef.current = getCam; }); + +// Revoke blob URL when replaced/unmounted +useEffect(() => { + return () => { + if (avatar?.startsWith("blob:")) { + try { URL.revokeObjectURL(avatar); } catch {} + } + }; +}, [avatar]);
45-64: Optional: consolidate getUserMedia calls to reduce promptsWhen both toggles are on, one combined call is simpler and can reduce permission prompts; split only if needed.
Sketch:
- let videoTrack: MediaStreamTrack | null = null; - let audioTrack: MediaStreamTrack | null = null; - if (videoOn) { /* get video */ } - if (audioOn) { /* get audio */ } + let videoTrack: MediaStreamTrack | null = null; + let audioTrack: MediaStreamTrack | null = null; + if (videoOn || audioOn) { + const stream = await navigator.mediaDevices.getUserMedia({ video: videoOn, audio: audioOn }); + videoTrack = videoOn ? stream.getVideoTracks()[0] ?? null : null; + audioTrack = audioOn ? stream.getAudioTracks()[0] ?? null : null; +}
89-110: Optional: watch microphone permission tooYou watch “camera” permission; mirroring for “microphone” improves UX when users grant mic mid-flow. Guard with feature detection as you did.
frontend/components/RTC/VideoGrid.tsx (1)
68-72: Minor a11y/UX polish for avatar imagesConsider setting alt to a more descriptive string (e.g.,
${name || "You"}’s avatar) and add draggable={false} to prevent accidental drag on desktop.Example:
-<img src={avatar} alt="Avatar" className="absolute inset-0 h-full w-full object-cover" /> +<img src={avatar} alt={`${name || "You"}'s avatar`} draggable={false} className="absolute inset-0 h-full w-full object-cover" />Also applies to: 201-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/components/RTC/DeviceCheck.tsx(6 hunks)frontend/components/RTC/Room.tsx(3 hunks)frontend/components/RTC/VideoGrid.tsx(4 hunks)
🔇 Additional comments (2)
frontend/components/RTC/Room.tsx (1)
22-30: Avatar prop wiring looks goodProp is typed, plumbed through Room, and forwarded to VideoGrid correctly. No concerns.
Also applies to: 32-40, 756-768
frontend/components/RTC/VideoGrid.tsx (1)
33-34: Avatar fallback rendering is correct and consistentProp is typed and consumed correctly; avatar shows when camOff, with sensible default icon fallback. Nicely done.
Also applies to: 46-48, 57-78, 188-210
…ar display styling.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/components/RTC/DeviceCheck.tsx (1)
52-101: LGTM! Media acquisition refactored cleanly.The separate video/audio acquisition logic with individual error handling provides better UX by showing specific error messages for camera vs. microphone failures.
Optional: The outer
try-catch(lines 95-100) is now largely redundant since all operations within are wrapped in their own try-catches. Consider removing it for clarity:const getCam = async () => { - try { localAudioTrackRef.current?.stop(); localVideoTrackRef.current?.stop(); let videoTrack: MediaStreamTrack | null = null; let audioTrack: MediaStreamTrack | null = null; // ... rest of function if (!videoOn && !audioOn && videoRef.current) { videoRef.current.srcObject = null; } - } catch (e: any) { - const errorMessage = e?.message || "Could not access camera/microphone"; - toast.error("Device Access Error", { - description: errorMessage - }); - } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/components/RTC/DeviceCheck.tsx(6 hunks)frontend/components/RTC/VideoGrid.tsx(4 hunks)
🔇 Additional comments (3)
frontend/components/RTC/VideoGrid.tsx (1)
33-33: LGTM! Avatar prop integration is clean.The optional
avatarprop is correctly typed and threaded through the component to support avatar display when the camera is off.Also applies to: 46-47
frontend/components/RTC/DeviceCheck.tsx (2)
103-132: LGTM! Effects properly structured with cleanup.The refactored effects cleanly separate concerns:
- Permission watching with cleanup (lines 103-123)
- Media acquisition on toggle (lines 125-127)
- Latest getCam reference for callbacks (lines 129-131)
The
getCamRefpattern ensures the permission watcher always calls the currentgetCamclosure, avoiding stale dependencies.
186-206: LGTM! Avatar preview UI is well-designed.The avatar display when video is off is clear and user-friendly:
- Circular avatar with standard Tailwind sizing (
h-24 w-24)- Intuitive upload/change button
- File input restricted to validated MIME types matching the handler
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/components/RTC/DeviceCheck.tsx (2)
2-2: Remove unnecessary React default import.With React 19's automatic JSX transform, the default
Reactimport is no longer required. Only the named hook imports are needed.Apply this diff:
-import React, { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react";Based on learnings
129-131: Consider adding a comment to clarify the no-dependency pattern.This effect intentionally runs on every render to keep
getCamRef.currentpointing to the latestgetCamclosure. This avoids stale closures in the permission watcher (line 110), which is set up once with empty deps. The pattern is correct but unconventional.Optionally add a clarifying comment:
+// Update ref to latest getCam to avoid stale closures in permission watcher useEffect(() => { getCamRef.current = getCam; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/components/RTC/DeviceCheck.tsx(6 hunks)frontend/components/RTC/VideoGrid.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/components/RTC/VideoGrid.tsx
🔇 Additional comments (5)
frontend/components/RTC/DeviceCheck.tsx (5)
133-139: Cleanup effect correctly handles blob URL lifecycle.The effect properly revokes blob URLs on avatar changes and unmount, addressing the previous review concern. Once the inline revocation in
handleAvatarChangeis removed, this will be the single, correct cleanup point.
52-101: LGTM: getCam refactoring correctly gates device requests.The refactoring properly:
- Requests video/audio only when respective toggle is on
- Handles device errors independently
- Stops old tracks before acquiring new ones
- Clears the video stream when both devices are off
This improves efficiency and user experience.
194-214: LGTM: Avatar UI is accessible and user-friendly.The implementation:
- Shows avatar/icon only when video is off (correct per requirements)
- Uses accessible label + hidden input pattern
- Provides clear user guidance ("Upload Avatar" vs "Change Avatar")
- File accept attribute matches validation rules
Well done!
33-44: LGTM: Validation is thorough and user-friendly.The validation logic properly:
- Restricts to safe, common image formats
- Enforces reasonable 5MB size limit
- Provides clear, actionable error messages
- Resets input to allow re-upload after errors
Excellent error handling!
161-161: LGTM: Avatar prop correctly passed to Room.The avatar is properly threaded through the component hierarchy as
string | null, enabling display in VideoGrid when the camera is off.
|
@lismarreiros join in another room and check the states are updating or not |
2 participants
…g in DeviceCheck component
🌟 Pre-submission Checklist
#pull-requestchannel to ensure no one else is working on this issue#pull-requestchannelSummary
Brief description of what this PR accomplishes.
Type of Changes
Testing Completed
Development Setup Verification
Code Quality
Related Issues
Closes #168
Screenshots/Videos
Additional Notes
Any additional information or context about the changes.
Note: For faster PR review and approval, ensure you're active in our Discord server!
Summary by CodeRabbit
New Features
Improvements