fix(cc-task): fix hold/resume not working after recording pause/resume#667
fix(cc-task): fix hold/resume not working after recording pause/resume#667bhabalan wants to merge 6 commits intowebex:nextfrom
Conversation
… name mismatch in cleanup Recording pause/resume callbacks called onRecordingToggle without a null check, throwing TypeError when the optional prop wasn't provided. This crashed the SDK event emitter, preventing subsequent hold/resume events from firing. Additionally, cleanup used CONTACT_RECORDING_PAUSED/RESUMED (different string values) instead of TASK_RECORDING_PAUSED/RESUMED, so old callbacks were never removed on re-renders.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
The SDK fires TASK_HOLD events before updating internal task data, so getAllTasks() returns stale isHold values. This caused the hold/resume UI to not update after recording pause/resume. Track isHeld locally in useCallControl (same pattern as isRecording), driven by holdCallback/resumeCallback events, synced from task data on currentTask changes, and override controlVisibility.isHeld with this event-driven state.
bhabalan
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae62a5ca97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| // findHoldStatus may fail if task data is incomplete | ||
| } |
There was a problem hiding this comment.
If findHoldStatus(currentTask, 'mainCall', agentId) throws (which this block explicitly anticipates for incomplete task payloads), the catch path leaves isHeld unchanged from the previous task. Because controlVisibility is later overridden with this local isHeld, a task switch from a held call can keep the next call stuck in a held UI state (e.g., Hold button sends resume). Please set a safe fallback (setIsHeld(false)) in the catch/else path so stale state does not leak across task updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed — the sync useEffect now uses controlVisibilityBase?.isHeld ?? false, so if getControlsVisibility returns undefined or errors out, isHeld falls back to false. No stale state leaks across task switches.
…ncerns - Add 3 tests verifying controlVisibility.isHeld is driven by hold/resume callbacks rather than stale task data from getAllTasks() - Fix sync useEffect dependency to use interactionId (not currentTask ref) to prevent overwriting event-driven state on refreshTaskList - Replace empty catch block with logger.warn for better observability
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28233f59af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| } | ||
| } | ||
| }, [currentTask?.data?.interactionId, agentId]); |
There was a problem hiding this comment.
The new hold sync effect only depends on currentTask?.data?.interactionId, so it runs once per interaction and never retries when the same interaction is refreshed with newer data. In the exact incomplete-payload case this block already anticipates (the findHoldStatus try/catch), a later refreshTaskList update for the same interaction ID will not re-sync isHeld, and the stale local value will keep overriding getControlsVisibility. Please include hold-relevant task data (or currentTask) in the dependencies so local state can recover when the task object is hydrated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentionally keeping interactionId as the dependency rather than currentTask. The root cause of this bug is that the SDK fires TASK_HOLD/TASK_RESUME events before updating its internal task data, so refreshTaskList → getAllTasks() returns stale isHold: false. Adding currentTask back to the deps would re-run the sync on every refreshTaskList and overwrite the event-driven state — reintroducing the original bug. The event callbacks (holdCallback/resumeCallback) are the source of truth during a call; this effect only seeds the initial value on task switch.
| getConferenceParticipants, | ||
| Participant, | ||
| findMediaResourceId, | ||
| findHoldStatus, |
There was a problem hiding this comment.
dont we have this already in getControlsVisibility ?
There was a problem hiding this comment.
Good catch — removed the separate findHoldStatus import. The sync useEffect now reads from controlVisibilityBase.isHeld (which already computes this via getControlsVisibility), eliminating the duplication. See 3bcf6af.
rsarika
left a comment
rsarika
left a comment
There was a problem hiding this comment.
check if can use the value in getControlsVisibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e64aac84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| // Override isHeld with event-driven local state to avoid stale task data from getAllTasks() | ||
| const controlVisibility = useMemo(() => ({...controlVisibilityBase, isHeld}), [controlVisibilityBase, isHeld]); |
There was a problem hiding this comment.
Overriding only controlVisibility.isHeld creates inconsistent UI state whenever local isHeld diverges from task-derived hold status (the stale-payload race this patch is handling). getControlsVisibility(...) already computed holdResume.isEnabled/end.isEnabled from the old hold value, while the button icon/action uses the overridden isHeld in cc-components (call-control.utils.ts uses controlVisibility.isHeld for icon/tooltip but controlVisibility.holdResume.isEnabled for disabled). In conference+consult-completed flows this can render a “Resume” hold button that stays disabled, blocking recovery.
Useful? React with 👍 / 👎.
…f findHoldStatus - Remove findHoldStatus import per reviewer feedback; reuse value already computed by getControlsVisibility via controlVisibilityBase - Add null safety (optional chaining + fallback) for controlVisibilityBase - Update override test to verify event-driven callbacks override base value
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bcf6afb82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| setIsHeld(controlVisibilityBase?.isHeld ?? false); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [currentTask?.data?.interactionId, agentId]); |
There was a problem hiding this comment.
This effect only depends on currentTask?.data?.interactionId and agentId, so it never runs when the same interaction is refreshed with newer task data (for example, after refreshTaskList hydration), leaving local isHeld stale while it continues to override getControlsVisibility. I re-checked the current head and confirmed controlVisibilityBase is recomputed from currentTask each render, but this sync path ignores those updates because exhaustive deps are explicitly suppressed.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| // Override isHeld with event-driven local state to avoid stale task data from getAllTasks() | ||
| const controlVisibility = useMemo(() => ({...controlVisibilityBase, isHeld}), [controlVisibilityBase, isHeld]); |
There was a problem hiding this comment.
Overriding only controlVisibility.isHeld leaves all other hold-derived flags (holdResume.isEnabled, end.isEnabled, etc.) based on stale task data, so the UI can become internally inconsistent when local hold state diverges from task-derived state. Fresh evidence on current head: in cc-components/src/components/task/CallControl/call-control.utils.ts, the hold icon/tooltip use controlVisibility.isHeld while disabled state still uses controlVisibility.holdResume.isEnabled, which can render a “Resume” button that remains disabled in consult/conference paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a0cc4bebb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| // Override isHeld with event-driven local state to avoid stale task data from getAllTasks() | ||
| const controlVisibility = useMemo(() => ({...controlVisibilityBase, isHeld}), [controlVisibilityBase, isHeld]); |
There was a problem hiding this comment.
The new controlVisibility override only replaces isHeld ({...controlVisibilityBase, isHeld}), but holdResume.isEnabled, end.isEnabled, and other flags in controlVisibilityBase were already computed from the old task-derived hold value. In consult/conference states where enablement depends on isHeld, this creates internally inconsistent UI (e.g., the button renders "Resume" from isHeld but stays disabled from stale holdResume.isEnabled), which can block recovery actions after hold/resume events.
Useful? React with 👍 / 👎.
2 participants
COMPLETES https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7507
This pull request addresses
Hold followed by recording Pause or Resume stops working — the hold/resume button stays stuck as "Hold the call" even though the SDK successfully processes the hold request.
by making the following changes
Fix 1: Track hold state locally via TASK_HOLD/TASK_RESUME events
Root cause: The SDK fires TASK_HOLD events before updating its internal task data. When
refreshTaskListruns in response and callsgetAllTasks(), the returned data still hasisHold: false. SincecontrolVisibility.isHeldreads from this stale task data, the UI never reflects the held state.Fix: Track
isHeldlocally inuseCallControlusinguseState, driven byholdCallback/resumeCallbackevents (same pattern already used forisRecording). The local state is synced from task data oncurrentTaskchanges and overridescontrolVisibility.isHeld.Fix 2: Guard optional onRecordingToggle callback
onRecordingToggleis an optional prop but was called unconditionally inpauseRecordingCallbackandresumeRecordingCallback, unlike other callbacks (onHoldResume,onEnd,onWrapUp) which all have null checks.Fix 3: Fix event name mismatch in useEffect cleanup
Registration used
TASK_EVENTS.TASK_RECORDING_PAUSED('task:recordingPaused') but cleanup usedTASK_EVENTS.CONTACT_RECORDING_PAUSED('ContactRecordingPaused'). These are different string values, soremoveTaskCallbacknever matched the registered listeners.Vidcast: https://app.vidcast.io/share/355ef012-b9ce-4b84-b35a-cc0f3cc5e303
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging