Drop Button Implementation in widgets - #735
Conversation
![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: 13b15ed84f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (pendingRequest && (pendingRequest.taskId !== participantDropTaskId || pendingRequest.task !== currentTask)) { | ||
| pendingParticipantDropRef.current = null; | ||
| } | ||
| }, [currentTask, controls]); | ||
|
|
||
| setPendingParticipantDropId(null); | ||
| setParticipantDropAnnouncement(null); | ||
| }, [currentTask, participantDropTaskId]); |
There was a problem hiding this comment.
Reset the pending request only when the interaction actually changes, not whenever currentTask gets a new object identity. StoreWrapper.refreshTaskList() calls setCurrentTask(), which clones the task even when its interaction ID is unchanged, and participant-left/UI-control events invoke that refresh during the event-correlated SDK request. This effect therefore clears the global lock and announcement before the promise continuation runs, allowing a second Drop and suppressing the normal success or failure feedback for the first request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Pending Drop requests are no longer tied to the currentTask object identity. The request is now tracked using a request token, interaction ID, and agent ID, so a refreshed/cloned task for the same interaction preserves the global pending lock and success/failure feedback. Completion is also validated against the current owner and terminal state to suppress stale updates. Tests cover same-interaction task clones, duplicate prevention, success, and failure paths.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f10ce0e50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Prefer the observable task data when it already contains the current leg; | ||
| // fall back to the state-machine snapshot during the brief hydration gap. | ||
| const taskDataCandidates = [task.data, snapshotTaskData].filter((taskData): taskData is RosterTaskData => |
There was a problem hiding this comment.
When a completed consult leg remains in task.data and a subsequent EP-DN consult is present only in state.context.taskData, this ordering returns the old leg immediately, so the newer snapshot is never examined. This is a supported transition in this repository—other consult logic explicitly accounts for retained old media and fresher initiator snapshots—and can make the Drop roster show the prior destination or omit the current ringing/answering participant until hydration catches up. Compare leg recency/current media IDs across both candidates rather than accepting the first consult entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Consult media is now selected across both observable task.data and state.context.taskData. The selection compares configured media IDs and available recency timestamps, preferring the state-machine snapshot when the sources cannot otherwise be distinguished. Regression coverage confirms that a retained older consult leg does not override a newer EP-DN consult.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053e6f46f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const consultIsActive = | ||
| hasVisibleControls(task?.uiControls?.consult) || ['consult', 'consulting'].includes(interactionState); |
There was a problem hiding this comment.
When a nested or EP-DN consult keeps interaction.state at conference and the SDK exposes only uiControls.main.endConsult—a supported shape already handled in task/src/helper.ts and its tests—this predicate incorrectly treats the consult as inactive because it inspects only uiControls.consult. Consequently getCurrentConsultMediaEntry omits the ringing or answering destination, while the identical predicate in hasActiveNonHeldConsult leaves Drop enabled for established participants during the active non-held consult; include the main-leg end-consult signal in both derivations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. A shared consult-activity predicate now recognizes visible consult controls, uiControls.main.endConsult, and consult/consulting interaction states. It is used for both current consult-media selection and active non-held consult Drop gating. Tests cover an EP-DN consult exposed only through main.endConsult, including disabled Drop during the active consult and re-enablement when the consult leg is held.
Kesari3008
left a comment
Kesari3008
left a comment
There was a problem hiding this comment.
Only few files I have reviewed. Have added comments for those. Please address these comments before merge. Approving the PR, keeping the deadline in mind. But PR needs to be revisited for thorough review
| "path": "packages/contact-center/store", | ||
| "spec": "packages/contact-center/store/ai-docs/store-spec.md", | ||
| "responsibility": "MobX singleton holding global CC state; proxies SDK events; sole SDK access point.", | ||
| "responsibility": "MobX singleton holding global CC state; proxies SDK events; sole SDK access point; owns pure conference Drop roster derivation.", |
There was a problem hiding this comment.
These changes may not be needed. This feature is just an addition among serveral other features and explicitly adding it in manifest is not required
There was a problem hiding this comment.
Agreed and addressed. The feature-specific responsibility descriptions were removed from .sdd/manifest.json, and the corresponding entries in SPEC_INDEX.md were restored to the generic module descriptions from next. Participant Drop details remain only in the feature intake and relevant module specifications.
| } | ||
| }, [customerDropTarget, latestCustomerDropTarget]); | ||
|
|
||
| const handleParticipantDrop = (target: ConferenceParticipantDropTarget, trigger: HTMLElement) => { |
There was a problem hiding this comment.
These methods belong in helper file, not in the presentational component. Any method that is performing an action or business logic should be in helper files and passed down here via props to be invoked here.
There was a problem hiding this comment.
Addressed. Participant Drop validation, Customer confirmation state, request serialization, SDK invocation, and confirm/cancel orchestration have been moved into useCallControl. CallControlCADComponent is now prop-driven and retains only DOM-specific responsibilities: rendering, native dialog open/close behavior, Escape handling, and focus restoration.
| </h4> | ||
| <ul className="participant-roster-list"> | ||
| {targets.map((target) => { | ||
| const isSelectedPending = pendingParticipantDropId === target.dropTargetId; |
There was a problem hiding this comment.
Avoid this where we have constants defined inside UI logic. This ideally should be outside
There was a problem hiding this comment.
Addressed. The inline participant-section renderer was extracted into a module-level ParticipantRosterSection presentational component, with its props moved to the co-located call-control-cad.types.ts file. Labels, accessibility, pending state, and disabled behavior remain unchanged.
| const PARTICIPANT_DROP_SUCCESS_MESSAGE = 'Participant removed from the conference.'; | ||
| const PARTICIPANT_DROP_FAILURE_MESSAGE = 'Unable to drop participant from the call. Try again.'; | ||
|
|
||
| type PendingParticipantDropRequest = { |
There was a problem hiding this comment.
This should be in types files
There was a problem hiding this comment.
Addressed. PendingParticipantDropRequest has been moved from helper.ts to task.types.ts. The obsolete task-object field was also removed; the request now contains only the token, interaction ID, agent ID, and Drop target ID.
| // pause/resume, etc.) exactly as production does. | ||
| store.handleIncomingTask(task); | ||
| store.setCurrentTask(task); | ||
| store.handleIncomingTask(task as unknown as Parameters<typeof store.handleIncomingTask>[0]); |
There was a problem hiding this comment.
This change is not needed
There was a problem hiding this comment.
Agreed and addressed. The unrelated casts and logger change in call-control-recording.tsx have been reverted to the next baseline. No recording behavior is changed by this feature.
| }); | ||
| }; | ||
|
|
||
| private scheduleTaskListRefresh = (): void => { |
There was a problem hiding this comment.
Is this change needed for drop feature ?
There was a problem hiding this comment.
Yes, this is required for the cross-channel Drop cleanup flow. The SDK emits terminal task:end/task:consultEnd events before it finishes removing the task from its internal collection. An immediate getAllTasks() refresh can therefore return the stale terminal task and leave the call UI visible until a page refresh. This helper defers and coalesces only the authoritative task-list read to the next microtask; it does not delete SDK-owned tasks locally. A concise explanation and regression coverage have been retained in the store.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f49f429ddc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.taskListRefreshScheduled = true; | ||
| queueMicrotask(() => { | ||
| this.taskListRefreshScheduled = false; | ||
| this.refreshTaskList(); |
There was a problem hiding this comment.
When a Drop completes through TASK_PARTICIPANT_LEFT, registerTaskEventListeners still routes the event to handleConferenceEnded, which calls refreshTaskList() immediately at storeEventsWrapper.ts:938-939 instead of using this queued path. The new intake explicitly states that SDK cleanup becomes visible after one microtask, so the immediate getAllTasks() read can clone the pre-cleanup mainCall membership and leave the removed participant in the roster until an unrelated event or page refresh; route participant-left completion through scheduleTaskListRefresh() as well.
Useful? React with 👍 / 👎.
| if (!hasActiveConsultSignal(task)) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
When an EP-DN consult first appears only in state.context.taskData while observable task.data and UI controls still show the preceding conference state, this gate returns early even though the fresh snapshot collected below contains the active consult. Consequently the pending destination is omitted and hasActiveNonHeldConsult() leaves established participants' Drop actions enabled during the consult. Fresh evidence after the earlier control fix is that hasActiveConsultSignal() still reads only task.data.interaction.state, not the state snapshot this function is specifically designed to consume.
Useful? React with 👍 / 👎.
| const handleConfirmCustomerDrop = () => { | ||
| closeCustomerDropDialog(); | ||
| void confirmParticipantDrop(); |
There was a problem hiding this comment.
When the user confirms Customer Drop, this closes the dialog and restores focus before confirmParticipantDrop() clears confirmation state or the SDK hydrates the roster, so focus is placed on the Customer Drop button that is about to unmount. Once ParticipantLeftConference removes that row, focus falls back to the document rather than the participant trigger or stable call control; the existing focus tests rerender the row away while the dialog is still open and therefore do not exercise this real confirmation ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4c35a3748
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Preserve the existing observable-data behavior when both sources describe | ||
| // the same leg and there is no positive evidence that the snapshot is newer. | ||
| return current; |
There was a problem hiding this comment.
When the state-machine snapshot advances an existing consult leg without changing its media ID—for example, a held consult resumes while task.data still reports isHold: true—these standard media objects may have no recency timestamp, so this branch always retains the stale observable entry. hasActiveNonHeldConsult() then incorrectly enables every Drop action during the resumed consult, allowing an established participant to be removed while a non-held consult is active. Fresh evidence beyond the earlier cross-leg ordering issue is that same-ID hold/participant updates are discarded even though this file explicitly treats state.context.taskData as the source that can advance before observable hydration.
Useful? React with 👍 / 👎.
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ba57861d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| answeredAgentEntries.find(({participant}) => | ||
| Boolean(destinationAgentName && participant?.name === destinationAgentName) | ||
| ) || |
There was a problem hiding this comment.
When an established conference agent and the newly answering EP-DN agent share the same display name, this name-based search can select the established agent first because it appears earlier on the consult leg. The later same-name deduplication then suppresses the actual answering agent, so the pre-merge roster shows only the existing participant and omits the new destination. Prefer the consulted/non-main participant identity before using consultDestinationAgentName for display.
Useful? React with 👍 / 👎.
4 participants
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.