fix(store): pass task object directly to setTaskCallback/removeTaskCallback to prevent duplicate event listeners - #723
fix(store): pass task object directly to setTaskCallback/removeTaskCallback to prevent duplicate event listeners#723brain-frog wants to merge 8 commits into
Conversation
…to prevent duplicate event listeners - Changed setTaskCallback and removeTaskCallback signatures to accept ITask instead of taskId string, eliminating volatile store.taskList lookup that caused orphaned listeners during React 18 StrictMode mount/unmount cycles - Added diagnostic logging with optional chaining on logger - Updated useIncomingTask and useCallControl hooks to pass task objects - Fixed leaking spy in useIncomingTask test (mockRestore) - Added jest.restoreAllMocks() in useCallControl beforeEach - Added regression test for removal when task absent from store.taskList CAI-8283
![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: 340bf53809
ℹ️ 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 (!callback) return; | ||
| const task = this.store.taskList[taskId]; | ||
| if (!task) return; | ||
| setTaskCallback = (event: TASK_EVENTS, callback, task: ITask) => { |
There was a problem hiding this comment.
Existing @webex/cc-store consumers can still call setTaskCallback(event, cb, interactionId) because this singleton is a published/default package surface, and this change now treats that string as an ITask: the guard passes, the log prints an undefined id, and the call then fails at task.on(...) instead of registering the listener. Since this PR is described as non-breaking and only updates the in-repo callers, keep accepting the old string-id form (or add a separate new API) so already-published widgets/hosts do not crash after upgrading; the same compatibility issue applies to removeTaskCallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a36d733. setTaskCallback/removeTaskCallback are now (event, callback, taskId: string, task?: ITask). The required taskId restores the original string-based call path — if task is omitted, it resolves via store.taskList[taskId], so already-published external consumers still calling with just an interaction ID keep working. The optional task param lets in-repo callers (useIncomingTask, useCallControl) pass the task reference directly, which is what avoids the stale store.taskList lookup race this PR originally set out to fix. Added test coverage for both the task-object and taskId-only (legacy) call paths in store/tests/storeEventsWrapper.ts
…ack signature change - Added STORE-R-022 requirement for ITask-based signatures - Updated test strategy with regression test coverage - Updated task spec overview, data flow, pitfalls, and do's/don'ts - Reflects CAI-8283 fix for duplicate event listeners CAI-8283
|
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: d284c97064
ℹ️ 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".
| store.setTaskCallback(TASK_EVENTS.TASK_WRAPPEDUP, wrapupCallCallback, interactionId); | ||
| store.setTaskCallback(TASK_EVENTS.TASK_RECORDING_PAUSED, pauseRecordingCallback, interactionId); | ||
| store.setTaskCallback(TASK_EVENTS.TASK_RECORDING_RESUMED, resumeRecordingCallback, interactionId); | ||
| store.setTaskCallback(TASK_EVENTS.TASK_HOLD, holdCallback, currentTask); |
There was a problem hiding this comment.
When CallControl is driven by store.currentTask, this passes the store-exposed task into task.on, but setCurrentTask stores a cloned object (Object.assign(Object.create(Object.getPrototypeOf(task)), task)) rather than the SDK task instance held in taskList. The SDK/store task events are registered and emitted on the original task objects, so active-call callbacks such as onHoldResume, onEnd, and recording toggles can stop firing when listeners are attached to the clone; capture the original task reference for registration and reuse that same reference for cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looked into this — store.currentTask is indeed a clone (Object.assign(Object.create(Object.getPrototypeOf(task)), task) in setCurrentTask), not the original instance from taskList. However, since Task extends EventEmitter and Node's EventEmitter keeps its listener bucket in an own property (_events), that shallow Object.assign copies the _events object by reference, so the clone and the original share the same underlying listener storage. I verified this with a standalone repro using Node's real EventEmitter: registering via the clone and emitting on the original (and removing via the clone) both work correctly today.
Because of that, we're leaving useCallControl as-is rather than adding a store.taskList[interactionId] resolution step, to avoid extra complexity for a case that isn't actually broken. That said, this does rely on an EventEmitter implementation detail rather than a documented contract — if the SDK's Task class ever changes how it stores listeners (e.g., private class fields, a different emitter), this would need revisiting. Flagging this as known fragility rather than fixing further for now
- Fix removeTaskCallback guard test to assert on mockTask.off instead of mockTask.on so the assertion actually catches a regression - Verify setTaskCallback receives the correct task object (not just any function) in the useCallControl event-listener test - Update react-patterns.md example to pass the ITask object instead of the stale interactionId string to setTaskCallback/removeTaskCallback
cmullenx
left a comment
cmullenx
left a comment
There was a problem hiding this comment.
resolve conflicts and the codex comments
Resolve conflicts from upstream/next in storeEventsWrapper.ts, task/helper.ts, and their tests/specs, keeping this PR's task-object-based setTaskCallback/removeTaskCallback signature (instead of upstream's taskId+task fallback) while folding in upstream's unrelated feature work (participant drop roster, wxApp mute sync, DTMF keypad, etc.).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0dfa76f77
ℹ️ 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".
…Callback Addresses outstanding Codex review comment on PR webex#723: external/already- published @webex/cc-store consumers may still call setTaskCallback/removeTaskCallback with a string interactionId. Restore that as the required third argument, resolving to task ?? store.taskList[taskId], and make the ITask reference an optional fourth argument so in-repo callers (useIncomingTask, useCallControl) keep passing the task object directly to avoid the stale taskList lookup race this PR originally fixed. Update store/task tests and specs to cover both the task-object and legacy taskId-only call paths.
…k logs
Task/interaction data is classified as agent/customer PII in
ai-docs/SECURITY.md and AGENTS.md rule 8 prohibits logging PII. Match
the rest of storeEventsWrapper.ts (e.g. removeCCCallback), which keeps
log messages free of payload values and carries only {module, method}
context.
2 participants
COMPLETES # https://jira-eng-sjc12.cisco.com/jira/browse/CAI-8283
https://jira-eng-sjc12.cisco.com/jira/browse/CAI-8283
This pull request addresses
Getting duplicate task acceptances
< 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.