fix(cc): review comments and cleanup state machine by arun3528 · Pull Request #2 · adhmenon/webex-js-sdk

arun3528 · 2026-01-07T12:19:14Z

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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios where tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

arun3528 · 2026-01-14T06:47:36Z

-          // Backend may send hold success without a preceding HOLD_INITIATED (e.g. remote hold)
+          // Backend may send hold success without a preceding HOLD_INITIATED (e.g. remote hold) for consult cases
+          // TODO:  check if we need this state as we already have consulting and intermediate state
          [TaskEvent.HOLD_SUCCESS]: {


remove this , we have intermediate state for consult and conference

Kesari3008

Guards and actions file needs some cleanup. It's not a blocker for this PR if we need to proceed with merging it with Adhwaith's PR but it is something would be needed as some of them aren't really required

Kesari3008 · 2026-01-14T08:07:06Z

  data: WebSocketPayload;
 };

+type TaskWithStateMachine = ITask & {


Move this to types file and this is applicable for any other similar occurrences of defining tyopes and interfaces in the same file

its not. needed moved to task class

Kesari3008 · 2026-01-14T08:21:27Z

I believe this comment is remaining to be discussed as I still feel we don't need to handle RONA, Failed, END and WRAPUP events here as state machine already has these transitions addressed. I even checked the handlers for these events and I see that apart from logging we are not doing much there

can you elaborate more on this ,

we just do when we get OUTBOUND on OFFERED state , and do the following

[TaskEvent.OUTBOUND_FAILED]: { target: TaskState.TERMINATED, actions: ['updateTaskData', 'markEnded', 'emitTaskReject'], },

Kesari3008 · 2026-01-14T10:25:32Z

      const payload = JSON.parse(event) as WebSocketMessage;

      // Filter out keepalive messages
      if (payload?.keepalive === 'true' || payload?.keepalive === true) {


There was a previous comment on this line that why do we need to check for string and boolean value here ?

i have seen it coming up as a string , for secruity if they end of changing to boolan both gets handled

Kesari3008 · 2026-01-14T10:32:23Z


    // Some consult/conference events can arrive on a child interactionId while the task we track
    // is keyed by the main/parent interactionId. Resolve to an existing task when possible.
    const interactionId = message.data.interactionId;


What is the difference between the 3 interaction IDs here and task object ? I believe one would be the current task/interaction and other would be the associated task for consult or conference flows so why do we have 3 interactions/task being fetched here

@adhmenon do you know why we have 3 intraction

Kesari3008 · 2026-01-14T13:21:36Z


+  // ============================================
  // Recording Guards
  recordingActive: ({context}: GuardParams): boolean => {


I don't see these guards being used in state machine but in the methods: pauseRecording and resumeRecording. We can directly us context itself in those method in the conditions instead of creating new guards

Kesari3008 · 2026-01-14T13:27:20Z

+   * This prevents all agents in a conference from becoming consultInitiator when one agent
+   * starts a new consult.
+   */
+  setConsultInitiator: assign(({event}: TaskActionArgs) => {


If we already have consultInitiator being set in context, why do we need these actions: setConsultInitiator and determineConsultInitiator?

this is what sets the values in the context ,
determineConsultInitiator is a helper becuase when a task update happend then also we need to check if there is initator or not that happens in deriveTaskDataUpdates

Kesari3008 · 2026-01-14T13:29:10Z

Are these getting used ? We can remove them if not needed

no its not , removed it

Kesari3008 · 2026-01-14T13:46:16Z

Won't this change now that we don't have OFFERED_CONSULT state

yep removed it

Kesari3008 · 2026-01-14T13:49:56Z

    context;
  const {recordingControlsAvailable, recordingInProgress} = context;

  const isHeld = serverHold ?? state === TaskState.HELD;


Same question here, why can't we use context to identify task states ? Do we really need to create so many conditions in this file ?

serverHold is from the context object .

state === TaskState.HELD is just a backup when conext is not set

adhmenon

Tested changes locally and it works.
Approving.

Kesari3008 · 2026-01-29T17:43:42Z

-  EventObject
->;
-
 type RecordingStateUpdate = Partial<


This should be in types file

Kesari3008 · 2026-01-29T17:44:13Z

  }),

-  setHoldInitiated: assign({}),
  handleTransferInit: assign({}),


We are still keeping these even when not used

Kesari3008 · 2026-01-29T18:01:34Z


  // State categories for cleaner logic
-  const isOffered = state === TaskState.OFFERED || state === TaskState.OFFERED_CONSULT;
+  const isOffered = state === TaskState.OFFERED;


Why do we even need this variable, just use the state

Kesari3008 · 2026-01-29T18:33:37Z

+      return;
+    }
+
+    const eventTaskData = Task.extractTaskDataFromEvent(event) as any;


I still see any being used in code

Kesari3008 · 2026-01-29T18:34:50Z

+      return;
+    }
+
+    const isAutoAnsweringFromEvent = eventTaskData?.isAutoAnswering;


Why is this extra check required?

arun3528 · 2026-01-30T08:03:05Z

@Kesari3008 have update the PR with the review comments addredded , unit test are passing as well

arun3528 marked this pull request as ready for review January 7, 2026 12:20

arun3528 force-pushed the cleanup-statemachine branch from 0b9598a to 53ebb39 Compare January 12, 2026 11:00

fix(cc): review comments and cleanup state machine

c63d7d7

arun3528 force-pushed the cleanup-statemachine branch from 53ebb39 to c63d7d7 Compare January 13, 2026 16:15

arun3528 added 3 commits January 13, 2026 22:48

Merge branch 'task-conference' into cleanup-statemachine

81a6459

test(cc): update the unit test

0d84eeb

Merge branch 'task-conference' into cleanup-statemachine

6094bc6

arun3528 commented Jan 14, 2026

View reviewed changes

fix: cleanup review comments

36ec884

Kesari3008 suggested changes Jan 14, 2026

View reviewed changes

arun3528 added 2 commits January 19, 2026 10:01

fix: review comments and unit test

e1fd7e8

Merge branch 'task-conference' into cleanup-statemachine

aa27b45

adhmenon approved these changes Jan 21, 2026

View reviewed changes

Kesari3008 approved these changes Jan 29, 2026

View reviewed changes

fix: final review comments fixed

f877286

arun3528 force-pushed the cleanup-statemachine branch from a5f23da to f877286 Compare January 30, 2026 08:02

adhmenon merged commit fa8af3e into adhmenon:task-conference Jan 30, 2026
9 of 10 checks passed

Conversation

arun3528 commented Jan 7, 2026

COMPLETES #< INSERT LINK TO ISSUE >

This pull request addresses

by making the following changes

Change Type

The following scenarios where tested

I certified that

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kesari3008 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adhmenon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arun3528 commented Jan 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kesari3008 left a comment •

edited

Loading