refactor(call): extract VideoCall.jsx into focused custom hooks - #1200
Merged
Conversation
Decompose the 1622-line VideoCall.jsx monolith into 11 focused hooks, reducing it to a ~290-line orchestrator. No behavior changes — all 459 component tests pass across chromium, firefox, and webkit. New hooks: - useDisplayNameSync: mirror Empirica name → Daily displayName - useDailyIdTracking: store dailyId/dailyIdHistory on player - useCallStartSignaling: signal server to start recording - useVisibilityTracking: log page blur/focus for debugging - useTrackMonitor: poll for ended tracks, auto-recover - usePermissionMonitor: watch Permissions API for revocations - useCallLifecycle: join/leave Daily room + Firefox stall detection - useDeviceErrors: device errors + fatal + network with priority dedup - useGesturePrompt: gesture-gated operations + computed prompt state - useDeviceRecovery: devicechange auto-recovery (W4) - useDeviceAlignment: consolidated 3x ~120-line alignment functions Part of #1199 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the VideoCall.jsx component by extracting large chunks of call/device lifecycle logic into focused custom hooks, aiming to keep behavior the same while improving maintainability and reuse.
Changes:
- Extracts multiple concerns (join/leave, device alignment, gesture gating, permissions, track monitoring, visibility tracking, etc.) into dedicated hooks under
client/src/call/hooks/. - Consolidates device alignment logic (camera/mic/speaker) into a single
useDeviceAlignmenthook using the sharedfindMatchingDeviceutility. - Reduces
VideoCall.jsxto an orchestrator that wires hooks into UI components/modals.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/call/VideoCall.jsx | Replaces in-component monolith logic with hook composition; keeps UI orchestration. |
| client/src/call/hooks/useDisplayNameSync.js | Syncs Empirica name/title into Daily displayName. |
| client/src/call/hooks/useDailyIdTracking.js | Tracks dailyId + structured dailyIdHistory on player at join time. |
| client/src/call/hooks/useCallStartSignaling.js | Signals stage callStarted on joined-meeting, with retry when stage becomes available. |
| client/src/call/hooks/useVisibilityTracking.js | Logs visibility/focus/blur events into player state for debugging/analytics. |
| client/src/call/hooks/useTrackMonitor.js | Polls local track readyState and attempts auto-recovery if ended. |
| client/src/call/hooks/usePermissionMonitor.js | Uses Permissions API to detect mid-call revocations and route into device error UI. |
| client/src/call/hooks/useCallLifecycle.js | Joins/leaves Daily room and implements Firefox blur-stall detection behavior. |
| client/src/call/hooks/useDeviceErrors.js | Centralizes Daily error listeners + deduped per-device error state. |
| client/src/call/hooks/useGesturePrompt.js | Tracks gesture-gated operations and derives unified prompt/modal state. |
| client/src/call/hooks/useDeviceRecovery.js | Listens for devicechange to auto-switch after not-found errors. |
| client/src/call/hooks/useDeviceAlignment.js | Aligns Daily devices to stored Empirica preferences and exposes picker switch handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| try { | ||
| if (!callObject.isDestroyed?.()) { | ||
| await devices.setSpeaker(targetId); | ||
| setSpeakerError((prev) => (prev?.dailyErrorType === "not-found" ? null : prev)); |
There was a problem hiding this comment.
setSpeakerError((prev) => ...) has the same issue as the camera/mic path: setSpeakerError from useDeviceErrors does not accept functional updaters, so this won’t reliably clear a stale not-found picker error (and can even set state to a function). Clear conditionally by inspecting the current speakerError value and calling setSpeakerError(null) only when it’s a not-found error, or adjust the useDeviceErrors API to handle updater functions.
Suggested change
| setSpeakerError((prev) => (prev?.dailyErrorType === "not-found" ? null : prev)); | |
| if (errorValues?.speakerError?.dailyErrorType === "not-found") { | |
| setSpeakerError(null); | |
| } |
Comment on lines
+27
to
+34
| const allDevices = await navigator.mediaDevices.enumerateDevices(); | ||
| const isSpeaker = deviceError.type === "speaker-error"; | ||
| const isCamera = deviceError.type === "camera-error"; | ||
| const relevantDevices = allDevices.filter((d) => { | ||
| if (isCamera) return d.kind === "videoinput"; | ||
| if (isSpeaker) return d.kind === "audiooutput"; | ||
| return d.kind === "audioinput"; | ||
| }); |
There was a problem hiding this comment.
useDeviceRecovery treats any deviceError that isn’t camera-error or speaker-error as a microphone failure (audioinput). If the active error is fatal-devices-error (which useDeviceErrors stores in cameraError) and happens to have dailyErrorType: 'not-found', this hook will auto-switch the mic incorrectly. Consider explicitly handling only camera-error / mic-error / speaker-error (early-return otherwise) and branching on deviceError.type === 'mic-error' for the mic case.
Comment on lines
+52
to
+98
| // Track blur events during join to detect stalled joins | ||
| const handleBlurDuringJoin = () => { | ||
| if (joiningMeetingRef.current) { | ||
| blurredDuringJoinRef.current = true; | ||
| // If page blurs during join, show prompt after short delay (not 5s) | ||
| setTimeout(() => { | ||
| if (joiningMeetingRef.current && blurredDuringJoinRef.current) { | ||
| console.warn("[VideoCall] Join stalled - tab blurred during join"); | ||
| setJoinStalled(true); | ||
| } | ||
| }, 500); | ||
| } | ||
| }; | ||
| window.addEventListener("blur", handleBlurDuringJoin); | ||
|
|
||
| // Fallback stall detection - if join takes > 5s and tab was blurred, prompt user | ||
| const stallTimer = setTimeout(() => { | ||
| if (joiningMeetingRef.current && blurredDuringJoinRef.current) { | ||
| console.warn( | ||
| "[VideoCall] Join appears stalled due to tab blur - prompting user" | ||
| ); | ||
| setJoinStalled(true); | ||
| } | ||
| }, 5000); | ||
|
|
||
| const joinRoom = async () => { | ||
| const meetingState = callObject.meetingState?.(); | ||
| if (meetingState === "joined-meeting" || joiningMeetingRef.current) | ||
| return; | ||
|
|
||
| const joinStartTime = Date.now(); | ||
| joinStartTimeRef.current = joinStartTime; | ||
| // Check if page is ALREADY unfocused when join starts | ||
| const alreadyUnfocused = !document.hasFocus(); | ||
| blurredDuringJoinRef.current = alreadyUnfocused; | ||
|
|
||
| // If page is already unfocused, show prompt quickly (Firefox suspends WebRTC when unfocused) | ||
| if (alreadyUnfocused) { | ||
| setTimeout(() => { | ||
| if (joiningMeetingRef.current) { | ||
| console.warn( | ||
| "[VideoCall] Join stalled - page was unfocused at start" | ||
| ); | ||
| setJoinStalled(true); | ||
| } | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
The 500ms setTimeout callbacks (blur-during-join and already-unfocused paths) aren’t tracked/cleared on cleanup. If the component unmounts or roomUrl changes while a join is in progress, these timers can still fire and call setJoinStalled(true) after unmount. Consider storing these timeout IDs and clearing them in the cleanup, and/or setting joiningMeetingRef.current = false in cleanup so the callbacks bail out reliably.
Comment on lines
+92
to
+100
| // Log to Sentry for monitoring browser policy trends | ||
| Sentry.captureMessage("Setup operation requires user gesture", { | ||
| level: "info", | ||
| tags: { | ||
| operation, | ||
| browser: navigator.userAgent, | ||
| }, | ||
| extra: { error: error?.message, details }, | ||
| }); |
There was a problem hiding this comment.
Sentry.captureMessage is adding navigator.userAgent as a tag (tags: { browser: navigator.userAgent }). Full UA strings are high-cardinality and can significantly increase Sentry index volume/cost; tags are best kept to low-cardinality values. Consider tagging a coarse browser name (e.g., "Chrome"/"Firefox"/"Safari") and putting the full UA string into extra/contexts instead.
Comment on lines
+20
to
+43
| const checkTracks = async () => { | ||
| try { | ||
| const [audioTrack, videoTrack] = await Promise.all([ | ||
| callObject.localAudio?.(), | ||
| callObject.localVideo?.(), | ||
| ]); | ||
|
|
||
| if (audioTrack?.readyState === "ended") { | ||
| Sentry.addBreadcrumb({ | ||
| category: "track-monitor", | ||
| message: "Audio track ended — attempting auto-recovery", | ||
| level: "warning", | ||
| }); | ||
| await callObject.setInputDevicesAsync({ audioDeviceId: true }); | ||
| } | ||
|
|
||
| if (videoTrack?.readyState === "ended") { | ||
| Sentry.addBreadcrumb({ | ||
| category: "track-monitor", | ||
| message: "Video track ended — attempting auto-recovery", | ||
| level: "warning", | ||
| }); | ||
| await callObject.setInputDevicesAsync({ videoDeviceId: true }); | ||
| } |
There was a problem hiding this comment.
The track polling loop only checks callObject.isDestroyed?.() once when the effect mounts. If the call object is destroyed later, the interval can keep firing and attempt localAudio/localVideo/setInputDevicesAsync, causing avoidable warnings/errors. Add an isDestroyed guard inside checkTracks (and/or stop the interval when the call transitions to a destroyed state).
Comment on lines
+161
to
+163
| console.log("Disabled AGC via setInputDevicesAsync"); | ||
| } catch (agcErr) { | ||
| console.warn("Failed to disable AGC:", agcErr); |
There was a problem hiding this comment.
This block documents that setInputDevicesAsync({ audioSource: constraintsObject }) is known not to work (expects a MediaStreamTrack and may silently no-op), but the code still logs "Disabled AGC via setInputDevicesAsync" on success. Since Daily may not throw on a no-op, that log can be misleading during debugging. Consider removing this call until implemented properly, or changing the log to reflect that it’s only an attempted (and currently ineffective) configuration.
Suggested change
| console.log("Disabled AGC via setInputDevicesAsync"); | |
| } catch (agcErr) { | |
| console.warn("Failed to disable AGC:", agcErr); | |
| console.log( | |
| "Attempted to disable AGC via setInputDevicesAsync (this configuration may currently be a no-op; see comment above)." | |
| ); | |
| } catch (agcErr) { | |
| console.warn("Failed to attempt AGC disable via setInputDevicesAsync:", agcErr); |
| try { | ||
| if (!callObject.isDestroyed?.()) { | ||
| await callObject.setInputDevicesAsync({ [inputDeviceKey]: targetId }); | ||
| setError((prev) => (prev?.dailyErrorType === "not-found" ? null : prev)); |
There was a problem hiding this comment.
setError((prev) => ...) passes a function into setCameraError/setMicError, but those setters (from useDeviceErrors) expect an error object (or null) and implement their own dedupe logic. This will either set state to a function or (more likely) fail to clear a stale not-found picker error. Use setError(null) when the current error is dailyErrorType === 'not-found' (you already have cameraError/micError available via errorValues), or update useDeviceErrors setters to explicitly support functional updaters.
Suggested change
| setError((prev) => (prev?.dailyErrorType === "not-found" ? null : prev)); | |
| // Clear stale "not-found" picker errors after successful alignment. | |
| let currentError = null; | |
| if (deviceType === "camera") { | |
| currentError = errorValues?.cameraError ?? null; | |
| } else if (deviceType === "microphone") { | |
| currentError = errorValues?.micError ?? null; | |
| } else if (deviceType === "speaker") { | |
| currentError = errorValues?.speakerError ?? null; | |
| } | |
| if (currentError?.dailyErrorType === "not-found") { | |
| setError(null); | |
| } |
Deliberation
|
||||||||||||||||||||||||||||
| Project |
Deliberation
|
| Branch Review |
refactor/videocall-extract-hooks
|
| Run status |
|
| Run duration | 09m 06s |
| Commit |
|
| Committer | James Houghton |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
1
|
Skipped
|
0
|
Passing
|
21
|
| View all changes introduced in this branch ↗︎ | |
- Fix functional updater bug: useDeviceErrors setters don't support React-style functional updaters, so replace setError(prev => ...) with conditional setError(null) using the actual error values - Add explicit type guard in useDeviceRecovery to only handle camera-error/mic-error/speaker-error (avoid mis-routing) - Track and clear inline setTimeout IDs in useCallLifecycle cleanup to prevent state updates after unmount - Add isDestroyed guard inside useTrackMonitor polling interval - Replace high-cardinality navigator.userAgent Sentry tag with coarse browser name (Chrome/Safari/Firefox/Edge/Other) - Fix misleading AGC success log (the call is currently a no-op) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This was referenced Mar 4, 2026
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
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.
Summary
VideoCall.jsxmonolith into 11 focused custom hooks, reducing it to a ~290-line orchestratoralignInputDevicefunctionNew hooks
useDisplayNameSyncuseDailyIdTrackinguseCallStartSignalinguseVisibilityTrackinguseTrackMonitorusePermissionMonitoruseCallLifecycleuseDeviceErrorsuseGesturePromptuseDeviceRecoveryuseDeviceAlignmentTest plan
Part of #1199
🤖 Generated with Claude Code