docs(ai-docs): task refactor migration — store layer (PR 2/4)#646
docs(ai-docs): task refactor migration — store layer (PR 2/4)#646akulakum wants to merge 26 commits intowebex:nextfrom
Conversation
Add store layer migration documentation: - 003: Store event wiring migration (30+ event handlers in storeEventsWrapper.ts) - 008: Store task-utils migration (~15 utility functions, findHoldStatus/findHoldTimestamp retention) Made-with: Cursor
![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: 08e96feca6
ℹ️ 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".
| // REMOVED from store/task-utils.ts — SDK tracks hold state in TaskContext: | ||
| // - task.uiControls.hold.isEnabled indicates holdable | ||
| // - task.uiControls.switchToConsult.isVisible indicates consult call is held | ||
| // No widget-side derivation needed. |
There was a problem hiding this comment.
This "After" snippet says findHoldStatus is removed and that hold state should be inferred from task.uiControls, but the same document elsewhere says findHoldStatus must be kept and explicitly warns that controls.hold.isEnabled is not a reliable held-state signal. Following this removal guidance would cause held calls to be misclassified during consult/transition states, which can regress TaskList status and held-state UI behavior.
Useful? React with 👍 / 👎.
| | `handleConsulting` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConsultAccepted` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConsultOffer` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConferenceStarted` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConferenceEnded` | Refresh + callbacks | **Simplify** — callbacks only | |
There was a problem hiding this comment.
The mapping marks handleConsulting and handleConferenceStarted as callback-only simplifications, but this conflicts with the earlier event table in the same doc that keeps these handlers to update consultStartTimeStamp, isQueueConsultInProgress, and currentConsultQueueId. If implementers follow this row, consult/conference transitions will stop resetting these store fields, breaking timer/reset behavior after those events.
Useful? React with 👍 / 👎.
|
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: 566a06c19d
ℹ️ 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".
|
|
||
| | Old Handler | Old Action | New Action | | ||
| |-------------|-----------|------------| | ||
| | `refreshTaskList` (15+ events) | Re-fetch ALL tasks from SDK | **Remove** — task.data is live; fire callbacks only | |
There was a problem hiding this comment.
This mapping says refreshTaskList can be removed for the whole 15+ event group, but AGENT_WRAPPEDUP is currently one of those registrations in storeEventsWrapper.ts and is used to clear wrapped-up tasks from store state. If migration work follows this row literally, wrap-up completion can leave stale interactions in taskList/currentTask until another full refresh path runs.
Useful? React with 👍 / 👎.
| | `handleConsultOffer` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConferenceStarted` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConferenceEnded` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleAutoAnswer` | Refresh + callbacks | **Simplify** — callbacks only | |
There was a problem hiding this comment.
Marking handleAutoAnswer as callback-only drops behavior that currently sets isDeclineButtonEnabled in the store; that flag is read by TaskList disable logic for auto-answering calls (disableDecline depends on !store.isDeclineButtonEnabled). Following this guidance would keep Decline disabled when auto-answer is active, changing user-facing call controls.
Useful? React with 👍 / 👎.
Move migration docs from ai-docs/migration/ to packages/contact-center/ai-docs/migration/ per reviewer feedback — this migration is specific to contact center. Made-with: Cursor
Remove findHoldTimestamp signature mismatch section from 008 — not a task-refactor change. Fix double --- separators in 003 and 008. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c05d0ccba0
ℹ️ 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".
| | `handleTaskAssigned` | Set current task | **Keep** — still need list management | | ||
| | `handleConsultCreated` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConsulting` | Refresh + callbacks | **Simplify** — callbacks only | | ||
| | `handleConsultAccepted` | Refresh + callbacks | **Simplify** — callbacks only | |
There was a problem hiding this comment.
This row marks handleConsultAccepted as callback-only, but store/src/storeEventsWrapper.ts::handleConsultAccepted currently performs essential state work beyond refreshes, including attaching TASK_MEDIA for browser devices (validated by store/tests/storeEventsWrapper.ts around the handleConsultAccepted media-listener tests). If migration follows this mapping literally, consult-accepted browser flows can lose media-track setup and regress call audio behavior.
Useful? React with 👍 / 👎.
| | `refreshTaskList` (15+ events) | Re-fetch ALL tasks from SDK | **Remove** — task.data is live; fire callbacks only | | ||
| | `handleTaskEnd` | Remove from list + cleanup | **Keep** — still need list management | | ||
| | `handleTaskAssigned` | Set current task | **Keep** — still need list management | | ||
| | `handleConsultCreated` | Refresh + callbacks | **Simplify** — callbacks only | |
There was a problem hiding this comment.
Mapping handleConsultCreated to callback-only drops the current consultStartTimeStamp update performed in store/src/storeEventsWrapper.ts::handleConsultCreated (and explicitly asserted in store/tests/storeEventsWrapper.ts), so AGENT_CONSULT_CREATED would no longer initialize consult timing state. That can leave consult timers stale or unset until a later event arrives, which changes user-visible consult timing behavior.
Useful? React with 👍 / 👎.
Removed serial numbers from filenames. Added event name rename table, constants to delete/keep, CONSULT_INITIATING gotcha, and fixed findHoldStatus contradiction in store-task-utils doc. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81c581c128
ℹ️ 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".
| | Delete | Reason | | ||
| |--------|--------| | ||
| | Local `TASK_EVENTS` enum (`store/src/store.types.ts`) | SDK exports this — delete local copy | | ||
| | `TASK_STATE_CONSULT`, `TASK_STATE_CONSULTING`, `TASK_STATE_CONSULT_COMPLETED` | SDK handles via state machine | |
There was a problem hiding this comment.
This row says TASK_STATE_CONSULT, TASK_STATE_CONSULTING, and TASK_STATE_CONSULT_COMPLETED should be deleted, but the same migration document also says findHoldStatus is kept with unchanged implementation, and that implementation in store/src/task-utils.ts still depends on those constants (via isConsultOnHoldMPC and consult-completed checks). If implementers delete these constants following this guidance before rewriting hold-state logic, the store layer will either fail to compile or regress held-state derivation.
Useful? React with 👍 / 👎.
| | `TASK_MEDIA` | `handleTaskMedia` | **Keep** (browser WebRTC setup) | | ||
| | `TASK_UI_CONTROLS_UPDATED` | **NEW** — `handleUIControlsUpdated` | **Add** — trigger widget re-renders | | ||
| | `TASK_WRAPUP` | `handleWrapup` | **Simplify** — no need to refresh | | ||
| | `AGENT_WRAPPEDUP` | `handleWrappedup` | **Keep refresh or add explicit task removal** — task must be removed from `taskList`/`currentTask` after wrapup completion to prevent stale UI | |
There was a problem hiding this comment.
The document first maps AGENT_WRAPPEDUP to SDK TASK_WRAPPEDUP and tells readers to import TASK_EVENTS from SDK, but this “new registration” table still uses AGENT_WRAPPEDUP. In a migration that actually switches to SDK enums/events, this mismatch can leave wrap-up completion unsubscribed (or non-compiling), which in turn risks stale taskList/currentTask cleanup after wrap-up.
Useful? React with 👍 / 👎.
Cross-validated against both CC Widgets and CC SDK task-refactor repos: - Fix event count (27 not 30+), add all handler detail (state mutations, side effects) - Document handleTaskRemove cleanup, pre-existing bugs (dead handleConsultEnd, listener leak) - Add store-only enum members to delete, new SDK events to evaluate - Fix contradictions between proposed and mapping tables (consolidated into single table) - Correct renamed event usage in After code examples - Fix function counts (remove 5 not 6), add ConsultStatus enum to deletions - Add participant role constants to keep list, findHoldTimestamp dual-signature note - Replace simplified getTaskStatus Before code with actual implementation - Add all 22 get*ButtonVisibility functions to deletion scope - Add review function dependencies, test file impact, barrel export notes Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af98aa7af8
ℹ️ 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".
| task.on(TASK_EVENTS.TASK_UI_CONTROLS_UPDATED, (uiControls) => { | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_UI_CONTROLS_UPDATED, interactionId, uiControls); | ||
| }); |
There was a problem hiding this comment.
This on registration uses an inline arrow function, but the cleanup section later removes TASK_UI_CONTROLS_UPDATED with this.handleUIControlsUpdated; those are different function references, so off(...) will not detach the listener. If implementers follow this pattern, removed/switched tasks can keep stale listeners and fire duplicate UI-control callbacks on future updates.
Useful? React with 👍 / 👎.
| this.fireTaskCallbacks(TASK_EVENTS.TASK_CONFERENCE_STARTED, interactionId); | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_PARTICIPANT_JOINED, interactionId); | ||
| }; |
There was a problem hiding this comment.
This shared handler fires both TASK_CONFERENCE_STARTED and TASK_PARTICIPANT_JOINED callbacks unconditionally, while the registration table binds the same handler to each event type; as a result, one incoming event can produce two different callback notifications and trigger widget side effects twice or at the wrong phase. The same duplication pattern appears in handleConferenceEnded for ended/participant-left callbacks.
Useful? React with 👍 / 👎.
Add callout note explaining getControlsVisibility lives in task/src/Utils/task-util.ts (hook layer), not the store. It appears in the store utils doc because it is the primary consumer of the 5 store functions being removed. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 850b0bacdd
ℹ️ 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".
| task.on(TASK_EVENTS.TASK_AUTO_ANSWERED, this.handleAutoAnswer); | ||
|
|
||
| // KEEP: Wrapup completion — task must be removed from list | ||
| task.on(TASK_EVENTS.TASK_WRAPPEDUP, (data) => { // renamed from AGENT_WRAPPEDUP |
There was a problem hiding this comment.
This TASK_WRAPPEDUP registration uses an inline arrow function, but the same document’s cleanup pattern relies on off(event, handler) matching the original handler reference; with an inline callback, handleTaskRemove cannot reliably detach the listener. If implementers follow this snippet, removed/re-hydrated tasks can retain stale wrapup listeners and trigger duplicate refreshTaskList()/callback executions on later wrapup events.
Useful? React with 👍 / 👎.
| this.fireTaskCallbacks(TASK_EVENTS.TASK_CONFERENCE_STARTED, interactionId); | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_PARTICIPANT_JOINED, interactionId); |
There was a problem hiding this comment.
This handler fires callbacks with interactionId, but the shown method signature does not receive interactionId and the snippet defines it as a standalone class handler. Implementing this literally leaves interactionId unresolved at runtime, so conference events can fail callback dispatch (or dispatch with the wrong task id), which breaks conference-related widget updates.
Useful? React with 👍 / 👎.
1. Add ordering constraint for consult state constants — TASK_STATE_CONSULT, TASK_STATE_CONSULTING, TASK_STATE_CONSULT_COMPLETED must not be deleted until findHoldStatus/isConsultOnHoldMPC are rewritten to use SDK TaskState. 2. Use stable named handler (handleUIControlsUpdated) for TASK_UI_CONTROLS_UPDATED event instead of inline arrow, so handleTaskRemove can correctly .off() it. 3. Fix conference handlers to accept eventType parameter and fire only the triggering event's callback, preventing duplicate notifications when both TASK_CONFERENCE_STARTED and TASK_PARTICIPANT_JOINED are wired to the same handler. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0df8e9c70b
ℹ️ 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".
| const interactionId = this.store.currentTask?.data.interactionId; | ||
| if (interactionId) { |
There was a problem hiding this comment.
handleUIControlsUpdated derives interactionId from this.store.currentTask, so when TASK_UI_CONTROLS_UPDATED fires for a non-selected task (multi-task/consult scenarios), callbacks are dispatched under the wrong task id. That can prevent the emitting task’s widgets from re-rendering and can incorrectly notify the current task instead; this handler should use the interaction id of the task that raised the event, not global current-task state.
Useful? React with 👍 / 👎.
|
|
||
| // KEEP: Conference state management (remove refreshTaskList, keep consult state reset) | ||
| // Pass event type so handler fires only the correct callback (not both) | ||
| task.on(TASK_EVENTS.TASK_PARTICIPANT_JOINED, () => this.handleConferenceStarted(TASK_EVENTS.TASK_PARTICIPANT_JOINED)); |
There was a problem hiding this comment.
This registration uses a new inline function, but the cleanup section requires off(event, handler) to receive the exact same handler reference. Without storing these wrapper callbacks, TASK_PARTICIPANT_JOINED/STARTED/ENDED/LEFT listeners cannot be detached on task removal, which can leave stale listeners and produce duplicate conference callbacks after task switches.
Useful? React with 👍 / 👎.
…sues All 4 issues stem from the tension between needing per-task context (interactionId) and stable handler references for .off() cleanup: 1. TASK_WRAPPEDUP inline arrow — now a bound handler stored in map 2. Conference handlers unresolved interactionId — now passed as parameter from bound handler wrappers 3. handleUIControlsUpdated wrong task ID — no longer reads currentTask; uses bound interactionId from the emitting task's registration closure 4. Conference inline wrappers not detachable — replaced with stored bound handlers that handleTaskRemove retrieves from the map Pattern: taskBoundHandlers Map<interactionId, Record<string, Function>> - Created per-task in registerTaskEventListeners - Retrieved in handleTaskRemove for exact .off() reference matching - Deleted after cleanup to prevent memory leaks Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d29b58c40
ℹ️ 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".
| | `INTERACTION_STATE_WRAPUP` | `store/src/constants.ts` | SDK handles via `TaskState.WRAPPING_UP` | | ||
| | `INTERACTION_STATE_POST_CALL` | `store/src/constants.ts` | SDK handles via `TaskState.POST_CALL` | | ||
| | `INTERACTION_STATE_CONNECTED` | `store/src/constants.ts` | SDK handles via `TaskState.CONNECTED` | | ||
| | `INTERACTION_STATE_CONFERENCE` | `store/src/constants.ts` | SDK handles via `TaskState.CONFERENCING` | |
There was a problem hiding this comment.
These rows schedule INTERACTION_STATE_WRAPUP/POST_CALL/CONNECTED/CONFERENCE for deletion, but the same document keeps findHoldStatus and only marks getConsultMPCState for later review. In the current store code, findHoldStatus still reaches getConsultMPCState, which still uses these interaction-state constants, so following this deletion guidance before rewriting that chain will break compilation and held-state derivation during migration.
Useful? React with 👍 / 👎.
| task.on(TASK_EVENTS.TASK_HOLD, () => { | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_HOLD, interactionId); | ||
| }); | ||
| task.on(TASK_EVENTS.TASK_RESUME, () => { | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_RESUME, interactionId); |
There was a problem hiding this comment.
This "After" snippet reintroduces inline callbacks for TASK_HOLD/TASK_RESUME, but the cleanup strategy in this same doc depends on removing listeners via stored handler references from taskBoundHandlers. If implementers copy this snippet literally, off(...) will not match these inline functions, leaving stale listeners attached and causing duplicate callback firing after task removal/re-registration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b91d8b78ea
ℹ️ 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".
| // KEEP: Task lifecycle events that need store-level management (class methods — no interactionId needed) | ||
| task.on(TASK_EVENTS.TASK_END, this.handleTaskEnd); | ||
| task.on(TASK_EVENTS.TASK_ASSIGNED, this.handleTaskAssigned); | ||
| task.on(TASK_EVENTS.TASK_REJECT, this.handleTaskReject); |
There was a problem hiding this comment.
This migration snippet wires TASK_REJECT directly to this.handleTaskReject, but the current store handler takes (task, reason) and today is intentionally registered with a task-bound wrapper ((reason) => this.handleTaskReject(task, reason) in store/src/storeEventsWrapper.ts, with tests asserting that task is forwarded). If implementers follow this line literally, reject events provide only the reason and lose the emitting task context, which can misfire onTaskRejected and remove/update the wrong interaction in multi-task flows.
Useful? React with 👍 / 👎.
| | `CONSULT_STATE_INITIATED` | `store/src/constants.ts` | SDK handles via context | | ||
| | `CONSULT_STATE_COMPLETED` | `store/src/constants.ts` | SDK handles via context | | ||
| | `CONSULT_STATE_CONFERENCING` | `store/src/constants.ts` | SDK handles via context | |
There was a problem hiding this comment.
These rows instruct deleting CONSULT_STATE_INITIATED, CONSULT_STATE_COMPLETED, and CONSULT_STATE_CONFERENCING without any ordering constraint, but store/src/task-utils.ts still imports and branches on all three inside getConsultMPCState (and getTaskStatus’s consult-completed path). Following this table as written before rewriting those helpers will break compilation and regress consult/hold derivation; it needs the same explicit "delete only after rewrite" guard used for other state constants in this doc.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307891e442
ℹ️ 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".
| const controls = currentTask?.uiControls ?? getDefaultUIControls(); | ||
| // All 17 controls come pre-computed from SDK. Zero store util calls needed. |
There was a problem hiding this comment.
This migration step bypasses getControlsVisibility and reads raw task.uiControls, but the existing control logic also applies widget-level gates from featureFlags and the conferenceEnabled prop in task/src/Utils/task-util.ts. Because those inputs are not part of task state, SDK-computed uiControls cannot enforce them, so following this guidance can make controls (for example conference/transfer/end-consult) visible or enabled even when an integrator has explicitly disabled them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708fad0bf4
ℹ️ 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".
| if (controls.wrapup.isVisible) return 'Wrap Up'; | ||
| if (controls.endConsult.isVisible) return 'Consulting'; | ||
| if (controls.exitConference.isVisible) return 'Conference'; |
There was a problem hiding this comment.
This rewrite changes getTaskStatus to return display labels ('Wrap Up', 'Consulting', 'Conference', etc.) instead of the existing machine-readable state values, which is a breaking API change for any downstream consumer of @webex/cc-store that compares status strings. Since this same document calls out that task-utils is barrel-exported, migration guidance should keep the existing return contract (or introduce a separate display-status helper) to avoid silent behavior regressions outside this repo.
Useful? React with 👍 / 👎.
| | `store/src/storeEventsWrapper.ts` | Refactor `registerTaskEventListeners` (see definitive table), update `handleTaskRemove` (fix listener mismatches + add `TASK_UI_CONTROLS_UPDATED`), simplify handlers (remove `refreshTaskList()` from all except `TASK_WRAPPEDUP`), wire `handleConsultEnd` to `TASK_CONSULT_END` | | ||
| | `store/src/store.ts` | No changes expected (observables stay) | | ||
| | `store/src/store.types.ts` | Delete local `TASK_EVENTS` enum; import from SDK (which includes `TASK_UI_CONTROLS_UPDATED`) | |
There was a problem hiding this comment.
This section instructs replacing the local TASK_EVENTS enum with SDK TASK_EVENTS but only lists store files, which is incomplete: task-layer consumers still use legacy members (AGENT_WRAPPEDUP, CONTACT_RECORDING_PAUSED, CONTACT_RECORDING_RESUMED in packages/contact-center/task/src/helper.ts). If implementers follow this file list literally, those references become invalid and the build breaks, so this step needs to explicitly include (or clearly sequence) the required task-package updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585851545d
ℹ️ 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".
| > | `featureFlags.webRtcEnabled` | accept, decline, muteUnmute, conference, muteUnmuteConsult, + telephony support (holdResume, endConsult) | Hide control when `webRtcEnabled` is `false` and channel is voice in browser | | ||
| > | `featureFlags.isEndCallEnabled` | end | Hide end button when `isEndCallEnabled` is `false` (phone device only) | | ||
| > | `featureFlags.isEndConsultEnabled` | endConsult | Hide end-consult when `isEndConsultEnabled` is `false` | | ||
| > | `conferenceEnabled` (widget prop) | conference, exitConference, mergeConference, consultTransferConsult | Hide all conference-related controls when `conferenceEnabled` is `false` | |
There was a problem hiding this comment.
The new applyFeatureGates table is still incomplete: it lists conferenceEnabled gating for conference/exitConference/mergeConference/consultTransferConsult, but omits mergeConferenceConsult, which is currently gated by conferenceEnabled in task/src/Utils/task-util.ts (getMergeConferenceConsultButtonVisibility). The webRtcEnabled row also omits controls like transfer that are currently hidden via isTransferVisibility. If implementers follow this table literally, integrator-disabled controls can reappear in production after migration.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9619b93a
ℹ️ 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".
| const controls = currentTask?.uiControls ?? getDefaultUIControls(); | ||
| // All 17 controls come pre-computed from SDK. Zero store util calls needed. |
There was a problem hiding this comment.
This migration step replaces getControlsVisibility(...) with raw task.uiControls and says all store-util control logic can be deleted, but many consumers still depend on derived booleans that are not part of SDK controls (for example controlVisibility.consultCallHeld/isConsultInitiated in packages/contact-center/task/src/Utils/timer-utils.ts and isHeld/isConferenceInProgress across packages/contact-center/cc-components/src/components/task/CallControl*). If implementers follow this section literally without migrating those consumers in the same step, they will either break ControlVisibility usage or lose consult/hold/conference timer and button behavior.
Useful? React with 👍 / 👎.
| > | ||
| > | Widget Prop | Controls Affected | Gate Logic | | ||
| > |-------------|-------------------|------------| | ||
| > | `featureFlags.webRtcEnabled` | accept, decline, muteUnmute, conference, muteUnmuteConsult, **transfer** (browser: `isTransferVisibility`), + telephony support (holdResume, endConsult) | Hide control when `webRtcEnabled` is `false` and channel is voice in browser | |
There was a problem hiding this comment.
The featureFlags.webRtcEnabled mapping still omits controls whose visibility currently depends on telephonySupported, especially consult and pause/resume recording (getControlsVisibility computes telephonySupported from webRtcEnabled and passes it to getConsultButtonVisibility and getPauseResumeRecordingButtonVisibility in task/src/Utils/task-util.ts). In browser voice scenarios with webRtcEnabled=false, following this table as written can leave those controls visible when they are currently hidden.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1109be968
ℹ️ 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".
| | `TASK_STATE_CONSULT` | `store/src/constants.ts` | SDK `TaskState.CONSULT_INITIATING` — **delete ONLY AFTER rewriting `findHoldStatus`** (see ordering note below) | | ||
| | `TASK_STATE_CONSULTING` | `store/src/constants.ts` | SDK `TaskState.CONSULTING` — **same ordering constraint** | |
There was a problem hiding this comment.
The table recommends deleting TASK_STATE_CONSULT by mapping it to TaskState.CONSULT_INITIATING, but TASK_STATE_CONSULT is currently used in findHoldStatus(task, mType, agentId) as a media-type sentinel (mType === 'consult'), not just a task-state value. If this guidance is followed literally, consult hold detection can stop matching and downstream controls/timers that depend on findHoldStatus(..., 'consult', ...) will regress in consult scenarios. The migration note should explicitly preserve a media-type constant (or rewrite that branch to use media-type enums) instead of replacing it with TaskState.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e64d50f0d7
ℹ️ 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".
| > | `featureFlags.webRtcEnabled` | accept, decline, muteUnmute, conference, muteUnmuteConsult, **transfer** (browser: `isTransferVisibility`), **consult**, **recording** (pause/resume), + telephony support (holdResume, endConsult). (Old logic: `telephonySupported` from `webRtcEnabled` drives `getConsultButtonVisibility`, `getPauseResumeRecordingButtonVisibility` in task-util.) | Hide control when `webRtcEnabled` is `false` and channel is voice in browser | | ||
| > | `featureFlags.isEndCallEnabled` | end | Hide end button when `isEndCallEnabled` is `false` (phone device only) | | ||
| > | `featureFlags.isEndConsultEnabled` | endConsult | Hide end-consult when `isEndConsultEnabled` is `false` | | ||
| > | `conferenceEnabled` (widget prop) | conference, exitConference, mergeConference, **mergeConferenceConsult**, consultTransferConsult | Hide all conference-related controls when `conferenceEnabled` is `false` | |
There was a problem hiding this comment.
This table adds consultTransferConsult to the conferenceEnabled gate, but current behavior does not gate that control on conference settings (getConsultTransferConsultButtonVisibility has no conferenceEnabled input, and getControlsVisibility calls it without that flag in task/src/Utils/task-util.ts). If implementers follow this row literally during migration, tenants with conferenceEnabled=false will lose consult-transfer-consult actions that currently remain available, causing a functional regression in consult flows.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5149061382
ℹ️ 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".
| | # | Function | Reason | SDK Replacement | Impact on Other Functions | | ||
| |---|----------|--------|-----------------|--------------------------| | ||
| | 1 | `getConsultStatus(task, agentId)` | Primary consumer `getControlsVisibility` is deleted | `task.uiControls` encodes all consult control states | `getConsultStatus()` **calls** `getTaskStatus()` (not the reverse). When we delete `getConsultStatus`, update `getTaskStatus` to use `task.uiControls` (see After code below). | | ||
| | 2 | `getIsConferenceInProgress(task)` | `task-util.ts` already uses `task?.data?.isConferenceInProgress` directly; function only used in tests | `task.uiControls.exitConference.isVisible` | None | |
There was a problem hiding this comment.
This mapping is semantically wrong: getIsConferenceInProgress currently derives actual conference state from participants, but task.uiControls.exitConference.isVisible only reflects whether the exit control is currently shown. In the same document, conferenceEnabled gating is described as hiding conference controls, which means a task can still be in conference while exitConference.isVisible is false; following this replacement would cause false negatives and regress conference-dependent behavior (timers/button logic).
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| | Widget Enum Member | Value | Note | | ||
| |---|---|---| | ||
| | `TASK_UNHOLD` | `'task:unhold'` | SDK uses `TASK_RESUME` instead | | ||
| | `TASK_CONSULT` | `'task:consult'` | No SDK equivalent; consult flow uses multiple events | |
There was a problem hiding this comment.
The spec file in this path: packages/contact-center/task/ai-docs/widgets/CallControl/ARCHITECTURE.md mentions about this event. That needs to be handled as well.
There was a problem hiding this comment.
Doc updated: added a “Docs to update” note that points to CallControl ARCHITECTURE.md (consult sequence ~line 169) and says to update it when we migrate event names.
| **Action:** Delete the local `TASK_EVENTS` enum from `store/src/store.types.ts` and import from SDK instead. SDK's `TASK_EVENTS` already includes all needed events including `TASK_UI_CONTROLS_UPDATED`. | ||
|
|
||
| ### Pre-existing Bug: Event Name Mismatches | ||
|
|
||
| The 5 renamed events above are currently hardcoded in `store.types.ts` with a TODO comment: `// TODO: remove this once cc sdk exports this enum`. During migration, replace the entire local enum with SDK's exported `TASK_EVENTS` enum. |
There was a problem hiding this comment.
Do we know if the SDK is exporting this already?
There was a problem hiding this comment.
Doc updated: it now states the SDK doesn’t export TASK_EVENTS yet and points to migration-overview “SDK Pending Exports”; we keep the local enum until the SDK exports it.
| **Bug 1: `handleConsultEnd` is dead code.** | ||
| A `handleConsultEnd` method exists (resets `isQueueConsultInProgress`, `currentConsultQueueId`, `consultStartTimeStamp`) but `TASK_CONSULT_END` is wired to `refreshTaskList()` instead. The method's consult state cleanup never runs. |
There was a problem hiding this comment.
While this is a dead code, it is being used in the UT. Which means the UT is dead too. The spec needs a plan to fix that too
There was a problem hiding this comment.
Doc updated: added a “Migration / test plan” that when we wire TASK_CONSULT_END to handleConsultEnd, we’ll update store UTs to assert the new wiring and consult state reset.
| A `handleConsultEnd` method exists (resets `isQueueConsultInProgress`, `currentConsultQueueId`, `consultStartTimeStamp`) but `TASK_CONSULT_END` is wired to `refreshTaskList()` instead. The method's consult state cleanup never runs. | ||
|
|
||
| **Bug 2: `handleTaskRemove` listener mismatch.** | ||
| `registerTaskEventListeners` wires `TASK_CONFERENCE_TRANSFERRED → this.refreshTaskList`. But `handleTaskRemove` calls `taskToRemove.off(TASK_EVENTS.TASK_CONFERENCE_TRANSFERRED, this.handleConferenceEnded)` — wrong handler reference. This listener is **never actually removed**, causing a listener leak. |
There was a problem hiding this comment.
More concerning part is that TASK_CONFERENCE_TRANSFERRED event doesn't have a UT at all today. We need a ticket for such gaps
There was a problem hiding this comment.
Doc updated and tracked in Jira: added a “Test gap” note and linked CAI-7758 for adding UTs for this and similar gaps.
https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7758
| **New:** SDK keeps `task.data` updated via state machine actions. The store can read `task.data` directly instead of re-fetching. `refreshTaskList()` should only be called for: | ||
| - Initial load / hydration | ||
| - Full page refresh recovery | ||
| - `TASK_WRAPPEDUP` (task must be removed from list — may be replaceable with explicit list removal) | ||
|
|
There was a problem hiding this comment.
I think the reason why we refreshTaskList is because the store doesn't listen to task.data directly to update the UI. While when we fetch the data manually from task object, it will show up. However, unless we set it again in store, it won't be observed and the UI won't re-render. How is that being tackled?
There was a problem hiding this comment.
Doc updated: added subsection “Why we can remove most refreshTaskList()” explaining SDK updates the same task reference, widgets re-render via fireTaskCallbacks/setTaskCallback, and we only keep refreshTaskList where the list itself changes.
There was a problem hiding this comment.
but still refreshTaskList needed right?
There was a problem hiding this comment.
we can enahance refreshTaskList to take task as param and update only given task
| reject: (reason: string) => this.handleTaskReject(task, reason), | ||
| outdialFailed: (reason: string) => this.handleOutdialFailed(reason), | ||
| uiControlsUpdated: (uiControls: TaskUIControls) => { | ||
| this.fireTaskCallbacks(TASK_EVENTS.TASK_UI_CONTROLS_UPDATED, interactionId, uiControls); |
There was a problem hiding this comment.
Even though it talks about fireTaskCallbacks a lot, it doesn't seem to have mentioned what it's definition would look like anywhere I believe. Can we check and include that?
There was a problem hiding this comment.
Doc updated: added “fireTaskCallbacks — Definition” with purpose, signature, and location (storeEventsWrapper.ts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cabccbca6
ℹ️ 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".
| |------|--------| | ||
| | `store/src/storeEventsWrapper.ts` | Refactor `registerTaskEventListeners` (see definitive table), update `handleTaskRemove` (fix listener mismatches + add `TASK_UI_CONTROLS_UPDATED`), simplify handlers (remove `refreshTaskList()` from all except `TASK_WRAPPEDUP`), wire `handleConsultEnd` to `TASK_CONSULT_END` | | ||
| | `store/src/store.ts` | No changes expected (observables stay) | | ||
| | `store/src/store.types.ts` | Delete local `TASK_EVENTS` enum; import from SDK (which includes `TASK_UI_CONTROLS_UPDATED`) | |
There was a problem hiding this comment.
This checklist item tells implementers to delete store/src/store.types.ts’s local TASK_EVENTS enum and import from SDK immediately, but the same document earlier states the SDK entry point does not yet export TASK_EVENTS. Following this row literally in the current migration state will produce unresolved imports and a broken build in store/task consumers; make this step explicitly conditional on the SDK export landing (or keep the local enum until then).
Useful? React with 👍 / 👎.
|
|
||
| The store's `task-utils.ts` contains 16 exported utility functions that inspect raw task data to derive state flags (consult status, hold status, conference state, participant info). Many of these become redundant when `task.uiControls` is the source of truth. This document maps which utils to keep, simplify, or remove. | ||
|
|
||
| **Barrel export:** `store/src/index.ts` has `export * from './task-utils'` — all 16 functions are publicly exported via `@webex/cc-store`. Removing functions will cause compile errors in any downstream consumer still importing them. |
There was a problem hiding this comment.
I don't think anyone will be using these utils. The only downstream we have today is Epic. So we can ask about these Utils usage in the space and eliminate them if they do not use it today.
Exported doesn't mean used!
There was a problem hiding this comment.
Doc updated: added a pre-migration step to confirm downstream usage (e.g. Epic) before removing utils — "exported doesn’t mean used."
| |--------|------|--------| | ||
| | Local `TASK_EVENTS` enum | `store/src/store.types.ts` | SDK exports this — delete local copy (covered in detail in [store-event-wiring-migration.md](./store-event-wiring-migration.md)) | | ||
| | `ConsultStatus` enum | `store/src/store.types.ts` | All consumers (`getConsultStatus`, `getControlsVisibility`) are being removed | | ||
| | `TASK_STATE_CONSULT` | `store/src/constants.ts` | **Dual use:** (1) Task state → SDK `TaskState.CONSULT_INITIATING`. (2) **Media-type sentinel** in `findHoldStatus(task, mType, agentId)` — line 328 uses `mType === TASK_STATE_CONSULT` to identify the consult *leg*, not task state. **Delete ONLY AFTER rewriting `findHoldStatus`**; when rewriting, preserve a media-type constant (or use media-type enums) for the consult leg — do **not** replace that branch with `TaskState`. See ordering note below. | |
There was a problem hiding this comment.
- TASK_STATE_CONSULT
- RELATIONSHIP_TYPE_CONSULT
- MEDIA_TYPE_CONSULT
All 3 of them resolve to the same string 'consult'
Is there anything we can do to tackle this?
There was a problem hiding this comment.
Doc updated: added “Consult string alias” in Constants to Keep — all three are documented as the same string 'consult'; we can consider one constant or document the alias; deletion/rewrite ordering is unchanged.
| | `RELATIONSHIP_TYPE_CONSULT` | `store/src/constants.ts` | Still used by `findMediaResourceId` (KEEP) | | ||
| | `MEDIA_TYPE_CONSULT` | `store/src/constants.ts` | Still used by `findMediaResourceId` (KEEP) | | ||
| | `AGENT` | `store/src/constants.ts` | Used by `getConferenceParticipants` (KEEP) for participant filtering | | ||
| | `CUSTOMER` | `store/src/constants.ts` | Used by `EXCLUDED_PARTICIPANT_TYPES` | | ||
| | `SUPERVISOR` | `store/src/constants.ts` | Used by `EXCLUDED_PARTICIPANT_TYPES` | | ||
| | `VVA` | `store/src/constants.ts` | Used by `EXCLUDED_PARTICIPANT_TYPES` | | ||
| | `EXCLUDED_PARTICIPANT_TYPES` | `store/src/constants.ts` | Used by `getConferenceParticipants` (KEEP) for participant filtering | |
There was a problem hiding this comment.
Why do some say (KEEP) and a few doesn't? All of these are supposed to be kept. Is my understanding correct?
Some of them might change based on my comment above about 3 constants resolving to same string 'consult'
There was a problem hiding this comment.
Yes — all entries in the “Constants to Keep” table are kept. Doc updated so the Reason column is consistent and the consult string alias (TASK_STATE_CONSULT / RELATIONSHIP_TYPE_CONSULT / MEDIA_TYPE_CONSULT) is called out; any consolidation would follow from that.
| ### fireTaskCallbacks — Definition | ||
|
|
||
| **Purpose:** Invoke all callbacks that were registered for a given task event via `setTaskCallback(event, cb, taskId)` (or equivalent), so widgets can re-render or react when that event occurs. | ||
|
|
||
| **Signature (conceptual):** | ||
| `fireTaskCallbacks(event: TASK_EVENTS, interactionId: string, payload?: unknown): void` | ||
|
|
||
| **Where it lives:** In the store-events layer — `packages/contact-center/store/src/storeEventsWrapper.ts`. After the migration, "After" handlers call `fireTaskCallbacks(...)` instead of (or in addition to) `refreshTaskList()` for events that only need to notify widgets. Implementation pattern: look up callbacks by event and optional `taskId`/`interactionId`, then invoke each with the payload. | ||
|
|
There was a problem hiding this comment.
This again doesn't give details of how the callbacks or fireTaskCalback will update the task data back into the store. That implementation information is required
There was a problem hiding this comment.
Doc updated: added an implementation note that fireTaskCallbacks does not write task data back. The SDK mutates the same task reference in place; the store holds that reference in taskList; when we call fireTaskCallbacks we only invoke widget callbacks, and widgets get the latest data by re-reading the same task from the store. So no “update back into the store” step is needed for in-place task updates.
| | 19 | `TASK_PARTICIPANT_LEFT` | `handleConferenceEnded` | **Simplify** | Same as #18 | | ||
| | 20 | `TASK_HOLD` | Fire callback only | **Simplify** | Remove `refreshTaskList()`. | | ||
| | 21 | `TASK_RESUME` | Fire callback only | **Simplify** | Remove `refreshTaskList()`. | | ||
| | 22 | `TASK_RECORDING_PAUSED` | Fire callback only | **Simplify + rename** | Was `CONTACT_RECORDING_PAUSED`. | |
There was a problem hiding this comment.
Only the call to refreshTaskList() is removed from those handlers. The handlers themselves are kept — they still fire callbacks so widgets can re-render. The doc table and “When refreshTaskList() is still required” section spell this out.
| | 20 | `TASK_HOLD` | Fire callback only | **Simplify** | Remove `refreshTaskList()`. | | ||
| | 21 | `TASK_RESUME` | Fire callback only | **Simplify** | Remove `refreshTaskList()`. | | ||
| | 22 | `TASK_RECORDING_PAUSED` | Fire callback only | **Simplify + rename** | Was `CONTACT_RECORDING_PAUSED`. | | ||
| | 23 | `TASK_RECORDING_RESUMED` | Fire callback only | **Simplify + rename** | Was `CONTACT_RECORDING_RESUMED`. | |
There was a problem hiding this comment.
Same as above: we remove only refreshTaskList() from the handler; we keep the handler and fire callbacks. Remove = remove refresh call, not the handler.
| **New:** SDK keeps `task.data` updated via state machine actions. The store can read `task.data` directly instead of re-fetching. `refreshTaskList()` should only be called for: | ||
| - Initial load / hydration | ||
| - Full page refresh recovery | ||
| - `TASK_WRAPPEDUP` (task must be removed from list — may be replaceable with explicit list removal) | ||
|
|
There was a problem hiding this comment.
but still refreshTaskList needed right?
| **New:** SDK keeps `task.data` updated via state machine actions. The store can read `task.data` directly instead of re-fetching. `refreshTaskList()` should only be called for: | ||
| - Initial load / hydration | ||
| - Full page refresh recovery | ||
| - `TASK_WRAPPEDUP` (task must be removed from list — may be replaceable with explicit list removal) | ||
|
|
There was a problem hiding this comment.
we can enahance refreshTaskList to take task as param and update only given task
|
|
||
| 1. **SDK keeps the same task reference up to date.** The state machine updates `task.data` (and `task.uiControls`) on the **same** `ITask` reference already held in the store's `taskList`. No re-fetch is needed for in-place updates. | ||
| 2. **Widget re-renders are driven by callbacks.** Widgets that registered via `setTaskCallback(event, cb, taskId)` are notified when the store calls `fireTaskCallbacks(event, interactionId, payload)`. Those callbacks cause the widget to re-run and re-render with the updated task from the store. | ||
| 3. **Re-fetch is only needed when the list itself changes.** We keep `refreshTaskList()` only where the **list** must change: e.g. initial load, full refresh, or `TASK_WRAPPEDUP` (task removed from the list). For all other events, the existing task reference is already updated by the SDK, and `fireTaskCallbacks` triggers UI updates. |
There was a problem hiding this comment.
this true w.r.t sdk perspective. but widgets still need to get that latest updated data. decide on how? whether with refresh task list or not
There was a problem hiding this comment.
Yes. refreshTaskList() is still needed for: initial load/hydration, full refresh recovery, and TASK_WRAPPEDUP (task must be removed from the list). Doc has a “When refreshTaskList() is still required” subsection. For other events we only remove the refresh call and keep firing callbacks.
Noted. Doc updated with a “Future consideration” that we could add an optional single-task refresh, e.g. refreshTaskList(task?), to update only the given task.
Doc clarifies: widgets get the latest data by re-reading the same task reference from the store (SDK mutates it in place). We don’t need refreshTaskList for that — we only need it when the list changes (init, full refresh, TASK_WRAPPEDUP). For other events, fireTaskCallbacks triggers re-renders and widgets read the updated task from the store.
| | `CONSULT_STATE_COMPLETED` | `store/src/constants.ts` | SDK handles via context — **same ordering constraint** | | ||
| | `CONSULT_STATE_CONFERENCING` | `store/src/constants.ts` | SDK handles via context — **same ordering constraint** | | ||
|
|
||
| ## Constants to Keep |
There was a problem hiding this comment.
we should remove all these, the task object should be source of truth. we should not have these kind of utils in widgets
There was a problem hiding this comment.
Agreed — task is the source of truth and we’re removing store task-utils. Doc states that. The “Constants to Delete” table and ordering constraint say to delete these only after findHoldStatus / related helpers are rewritten (or SDK/task exposes equivalent). Until then we keep the constants so we don’t break compilation or hold derivation.
| | `VVA` | `store/src/constants.ts` | Used by `EXCLUDED_PARTICIPANT_TYPES` | | ||
| | `EXCLUDED_PARTICIPANT_TYPES` | `store/src/constants.ts` | Used by `getConferenceParticipants` (KEEP) for participant filtering | | ||
|
|
||
| ## Ordering Constraint: Consult State Constants |
There was a problem hiding this comment.
below also should remove , the task object should be source of truth. we should not have these kind of utils in widgets
There was a problem hiding this comment.
Same as above: task as source of truth is the end state. We remove the store utils and constants once we have SDK/task equivalents. The doc’s ordering constraint says do not delete those constants until findHoldStatus (and related) are rewritten; then we can remove them.
| - **Via `isConsultOnHoldMPC`:** Line 303 — `[TASK_STATE_CONSULT, TASK_STATE_CONSULTING].includes(getConsultMPCState(...))` | ||
| - **Via `getConsultMPCState`:** Line 321 — `[TASK_STATE_CONSULT_COMPLETED].includes(getConsultMPCState(...))` | ||
|
|
||
| **Do NOT delete these 3 constants until `findHoldStatus` and `isConsultOnHoldMPC` are rewritten** to use SDK `TaskState` equivalents. Deleting them first will break compilation. When rewriting `findHoldStatus`, note that `TASK_STATE_CONSULT` is used there as a **media-type** sentinel (`mType === TASK_STATE_CONSULT`); preserve a media-type constant or media-type enum for the consult leg — do not replace that comparison with `TaskState`. |
There was a problem hiding this comment.
we should remove these as now task is source of truth
| | 15 | `findHoldStatus(task, mType, agentId)` | Hold status by media type | `task/src/Utils/task-util.ts` (`getControlsVisibility`), `getTaskStatus()` (via `getConsultMPCState` chain) | | ||
| | 16 | `findHoldTimestamp(task, mType)` | Hold timestamp for timers | Timer utils (store version takes `ITask`; see dual-signature note below) | | ||
|
|
||
| ### Important: `findHoldTimestamp` Dual Signatures |
…ENTS, task utils - Event wiring: when refreshTaskList() is required; remove call not handler; no write-back - fireTaskCallbacks note; future refreshTaskList(task?) - TASK_EVENTS: SDK exports from package index; delete local enum, import from @webex/contact-center - Task utils: task as source of truth; keep findHoldStatus/findHoldTimestamp until SDK equivalent - Constants/consult alias and pre-migration checks Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff04cbed4
ℹ️ 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".
| reject: (reason: string) => this.handleTaskReject(task, reason), | ||
| outdialFailed: (reason: string) => this.handleOutdialFailed(reason), | ||
| uiControlsUpdated: (uiControls: TaskUIControls) => { |
There was a problem hiding this comment.
The proposed TASK_OUTDIAL_FAILED binding only forwards reason to handleOutdialFailed, so it has no task/interaction context to perform the row #4 requirement of removing the failed task from store state. In current store/src/storeEventsWrapper.ts, handleOutdialFailed only fires onOutdialFailed and does not mutate taskList; with this migration’s broad refreshTaskList() removal, outdial-failed tasks can linger in UI/state until some unrelated full refresh event occurs.
Useful? React with 👍 / 👎.
|
|
||
| ## Decision: `findHoldStatus` and `findHoldTimestamp` retained for now | ||
|
|
||
| Reviewers may suggest removing these because "task is source of truth." They are **kept** in this migration because: (1) SDK `task.uiControls.hold.isEnabled` indicates whether the hold *action* is available, not whether a given leg is *currently held* (e.g. during consult, main call can be held while consult is active). (2) Timers and UI need per-leg hold state and hold timestamp, which are read from `task.data.interaction.media`; the store helpers centralize that derivation. Once the SDK or task layer exposes equivalent hold state (or widgets derive it in a single place from `task.data` only), these helpers can be removed and the "task as source of truth" end state is fully achieved. |
There was a problem hiding this comment.
(1) SDK
task.uiControls.hold.isEnabledindicates whether the hold action is available, not whether a given leg is currently held (e.g. during consult, main call can be held while consult is active).
This statement is incorrect. The UI Controls in the task object (task.uiControls) is recomputed every time the task.data changes. So, it should be considered for anything and everything in the UI
| **Implementation note — task data flow (no write-back):** | ||
| `fireTaskCallbacks` **does not** write or update task data back into the store. The flow is: (1) The SDK mutates the **same** `ITask` reference already held in the store's `taskList` (updates `task.data` and `task.uiControls` in place). (2) The store calls `fireTaskCallbacks(event, interactionId, payload)`, which only **invokes** the registered widget callbacks. (3) Widgets then **read** from the store (e.g. `store.taskList[interactionId]` or `store.currentTask`) and re-render with the updated task. So widgets get the latest data by re-reading that same reference after the callback; there is no separate "update task data back into the store" step. |
There was a problem hiding this comment.
How would the widgets read the data update? In the store, we have marked the cc object to be not observed in the constructor?
There was a problem hiding this comment.
Doc updated: added a short “How widgets see updates when the store does not observe the cc object” note. Because the store doesn’t observe cc (or task internals), MobX doesn’t re-run when the SDK mutates the task in place. Re-renders are callback-driven: when the store calls fireTaskCallbacks, the widget’s callback runs (e.g. setState or re-read), then the widget reads store.currentTask / store.taskList[interactionId] — the same task reference the SDK has already updated — so it gets the latest task.data and task.uiControls. So the callback triggers the re-render and read; we don’t rely on an observable on task internals
…a callback-driven re-render, not MobX observation of cc
| | `AGENT_WRAPPEDUP` | `'AgentWrappedUp'` | `TASK_WRAPPEDUP` | `'task:wrappedup'` | | ||
| | `AGENT_CONSULT_CREATED` | `'AgentConsultCreated'` | `TASK_CONSULT_CREATED` | `'task:consultCreated'` | | ||
| | `AGENT_OFFER_CONTACT` | `'AgentOfferContact'` | `TASK_OFFER_CONTACT` | `'task:offerContact'` | | ||
| | `CONTACT_RECORDING_PAUSED` | `'ContactRecordingPaused'` | `TASK_RECORDING_PAUSED` | `'task:recordingPaused'` | |
There was a problem hiding this comment.
These recording event names are redundant. I see in TASK_EVENTS, we already have task:recordingPaused and task:recordingResumed added. So we can just remove these names and this is not again something that needs to be done as aprt of task-refactor I believe
|
|
||
| | Widget Enum Member | Value | Note | | ||
| |---|---|---| | ||
| | `TASK_UNHOLD` | `'task:unhold'` | SDK uses `TASK_RESUME` instead | |
There was a problem hiding this comment.
Attach the actual event string here what TASK_RESUME and TASK_HOLD is
|
|
||
| The widget's local `TASK_EVENTS` enum (in `store/src/store.types.ts`) uses CC-level naming that differs from the SDK's task-level naming. | ||
|
|
||
| ### 5 Renamed Events |
There was a problem hiding this comment.
Some of the details here don't seem correct completely. Here is the complete list of what events are being emitted from SDK, based on this information update the events that need change:
From Task.ts
TASK_EVENTS.TASK_INCOMING
TASK_EVENTS.TASK_HYDRATE
TASK_EVENTS.TASK_OFFER_CONTACT
TASK_EVENTS.TASK_ASSIGNED
TASK_EVENTS.TASK_END
TASK_EVENTS.TASK_OFFER_CONSULT
TASK_EVENTS.TASK_CONSULT_CREATED
TASK_EVENTS.TASK_CONSULT_ACCEPTED
TASK_EVENTS.TASK_CONSULT_END
TASK_EVENTS.TASK_CONSULT_QUEUE_CANCELLED
TASK_EVENTS.TASK_CONSULT_QUEUE_FAILED
TASK_EVENTS.TASK_WRAPPEDUP
From voice/Voice.ts
TASK_EVENTS.TASK_HOLD
TASK_EVENTS.TASK_RESUME
TASK_EVENTS.TASK_RECORDING_STARTED
TASK_EVENTS.TASK_RECORDING_PAUSED
TASK_EVENTS.TASK_RECORDING_PAUSE_FAILED
TASK_EVENTS.TASK_RECORDING_RESUMED
TASK_EVENTS.TASK_RECORDING_RESUME_FAILED
TASK_EVENTS.TASK_PARTICIPANT_JOINED
TASK_EVENTS.TASK_PARTICIPANT_LEFT
TASK_EVENTS.TASK_CONFERENCE_STARTED
TASK_EVENTS.TASK_CONFERENCE_ENDED
TASK_EVENTS.TASK_CONFERENCE_FAILED
TASK_EVENTS.TASK_EXIT_CONFERENCE
TASK_EVENTS.TASK_TRANSFER_CONFERENCE
TASK_EVENTS.TASK_SWITCH_CALL
TASK_EVENTS.TASK_CONFERENCE_TRANSFER_FAILED
TASK_EVENTS.TASK_OUTDIAL_FAILED
| | `TASK_TRANSFER_CONFERENCE` | `'task:transferConference'` | Evaluate for conference flow | | ||
| | `TASK_CLEANUP` | `'task:cleanup'` | SDK internal — likely no widget action | | ||
|
|
||
| **Action:** Delete the local `TASK_EVENTS` enum from `store/src/store.types.ts` and import from SDK once the SDK exports it. **As of this migration, the SDK does not yet export `TASK_EVENTS` from its package entry point** (or status is TBD). For current status and the plan to replace the local enum once the SDK exports it, see [migration-overview.md](./migration-overview.md) → "SDK Pending Exports" (or equivalent section). Until then, keep the local enum and align event string values with the SDK where needed. |
There was a problem hiding this comment.
We are exporting TASK_EVENTS from the SDK. Check index.ts file: /Users/pkesari/Desktop/WorkProjects/WebRTCFrontEnd/wxcc-sdk/webex-js-sdk/packages/@webex/contact-center/src/index.ts
| | 26 | `TASK_POST_CALL_ACTIVITY` | `refreshTaskList` | Re-fetch all tasks | | ||
| | 27 | `TASK_MEDIA` | `handleTaskMedia` | Browser-only: `setCallControlAudio(new MediaStream([track]))` | | ||
|
|
||
| ### Pre-existing Bugs in Old Code |
There was a problem hiding this comment.
Would this be relevant for task-refactor specific migrations ?
|
|
||
| handleConferenceStarted = (eventType: TASK_EVENTS, interactionId: string) => { | ||
| runInAction(() => { | ||
| this.setIsQueueConsultInProgress(false); |
There was a problem hiding this comment.
Would we still need these after the migration ? What does these setters help us with today ?
|
|
||
| --- | ||
|
|
||
| ## Files to Modify |
There was a problem hiding this comment.
This section should be placed right after summary
| | 9 | `TASK_CONSULTING` | `handleConsulting` | `refreshTaskList()` + `setConsultStartTimeStamp(Date.now())` | | ||
| | 10 | `TASK_CONSULT_ACCEPTED` | `handleConsultAccepted` | `refreshTaskList()` + `setConsultStartTimeStamp(Date.now())` + set ENGAGED state + **registers `TASK_MEDIA` listener on consult task (browser only)** | | ||
| | 11 | `TASK_OFFER_CONSULT` | `handleConsultOffer` | `refreshTaskList()` | | ||
| | 12 | `TASK_AUTO_ANSWERED` | `handleAutoAnswer` | `setIsDeclineButtonEnabled(true)` + `refreshTaskList()` | |
There was a problem hiding this comment.
Why do we need isDeclineButtonEnabled property and getter and setters for this ? UIControls should get used in place of this and we are not adding this point in this migrationdocs that this needs to be replaced with uiCOntrols.decline.isEnabled
|
|
||
| --- | ||
|
|
||
| ### fireTaskCallbacks — Definition |
There was a problem hiding this comment.
This whole approach is not task-refactor migration related ? So why are we coming up with as part of this migration doc ? What is the problem that we are trying to solve here ? And if we do wanna address some of the refactoring in widgets to solve some existing problems then it would be good to separate the sections. Modifcations based on task-refactor changes and modifications for Widgets imporvements and fixing exiisting issues. Also I feel this is unnecessary, we are try to over complicate a simple approach of switching on and witching off the listeners with their callbacks associated
| |------|--------| | ||
| | `store/src/task-utils.ts` | Remove 5 functions, keep 7 (update `getTaskStatus` to use `task.uiControls`), review 4 | | ||
| | `store/src/store.types.ts` | Delete `ConsultStatus` enum (all consumers removed) | | ||
| | `store/src/constants.ts` | Delete 9 task/interaction/consult state constants; keep 7 participant/media constants | |
There was a problem hiding this comment.
This table says delete 9 constants, but the "After" getTaskStatus code still uses them as return values. Clarify that these deletions are deferred until a subsequent step where getTaskStatus is further rewritten to use SDK TaskState values.
4 participants
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7714
This pull request addresses
This is PR 2 of 4 in the task refactor migration documentation series. It covers the store layer changes — event wiring simplification and store utility function migration.
by making the following changes
Added 2 store layer migration documents under
packages/contact-center/ai-docs/migration/:store-event-wiring-migration.md: Documents how 30+ event handler subscriptions in
storeEventsWrapper.tschange:refreshTaskList()calls becomefireTaskCallbacks()(SDK keeps task.data in sync)TASK_UI_CONTROLS_UPDATEDsubscription added to trigger widget re-rendersstore.types.tsregisterTaskEventListenersstore-task-utils-migration.md: Identifies ~15 utility functions and their migration fate:
getConsultStatus,getIsConferenceInProgress,getConferenceParticipantsCount,getIsCustomerInCall,getIsConsultInProgressisIncomingTask,getTaskStatus,getConferenceParticipants,isInteractionOnHold,findMediaResourceId,findHoldTimestamp,findHoldStatusTaskState.CONSULT_INITIATINGvsCONSULTINGdistinctiongetTaskStatusenhancementCC SDK Task-Refactor Branch Reference
Change Type
Related PRs
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging