docs(calling): Spark 787604 call flow specs#4780
docs(calling): Spark 787604 call flow specs#4780Kesari3008 wants to merge 11 commits intowebex:nextfrom
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: bc9c0e0e1d
ℹ️ 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".
packages/calling/src/Agents.md
Outdated
| @@ -0,0 +1,120 @@ | |||
| # Calling Package - Agent Specification | |||
There was a problem hiding this comment.
This document is added as Agents.md, but the repository’s agent-discovery convention uses the exact filename AGENTS.md; on case-sensitive environments this file will not be discovered, so the new guidance under packages/calling/src is effectively ignored. Renaming it to AGENTS.md is required for the instructions to take effect.
Useful? React with 👍 / 👎.
packages/calling/AGENTS.md
Outdated
|
|
||
| | Module | Scope / Keywords | Location | ai-docs | | ||
| |---|---|---|---| | ||
| | **CallingClient** | line, register, call, dial, answer, hold, transfer, mute, media | `src/CallingClient/` | [`src/CallingClient/ai-docs/`](src/CallingClient/ai-docs/AGENTS.md) | |
There was a problem hiding this comment.
The routing table directs readers to src/CallingClient/ai-docs/AGENTS.md, but this commit only adds CallingClient docs under src/CallingClient/calling/ai-docs/. Following the documented Quick Start workflow therefore fails at the first module lookup for CallingClient tasks, preventing agents from loading the intended context.
Useful? React with 👍 / 👎.
packages/calling/AGENTS.md
Outdated
| 1. **Classify the task** - Determine if it's a new module, new method, bug fix, feature enhancement, or architecture question. | ||
| 2. **Load context** - Use the [Module Routing Table](#module-routing-table) to find and read the target module's `ai-docs/AGENTS.md` and `ARCHITECTURE.md`. | ||
| 3. **Load rules** - Read [`ai-docs/RULES.md`](ai-docs/RULES.md) for coding standards. | ||
| 4. **Load patterns** - Read relevant patterns from [`ai-docs/patterns/`](ai-docs/patterns/). |
There was a problem hiding this comment.
The Quick Start workflow mandates loading ai-docs/patterns/, but this commit does not include a patterns/ directory or any of the referenced pattern files. As written, agents following the prescribed process hit dead links and cannot complete the documented workflow without manual guesswork.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73e787eba1
ℹ️ 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".
|
|
||
| Before generating code, read: | ||
| 1. [`../../patterns/typescript-patterns.md`](../../patterns/typescript-patterns.md) - Type conventions, factory pattern, singleton pattern | ||
| 2. [`../../patterns/event-driven-patterns.md`](../../patterns/event-driven-patterns.md) - Event and WebSocket patterns |
There was a problem hiding this comment.
This template sends agents to ../../patterns/event-driven-patterns.md, but that file is not in packages/calling/ai-docs/patterns (the existing file is event-patterns.md), so the documented workflow breaks at the context-loading step. The same broken target also appears in templates/new-method/00-master.md, so generated tasks that follow either master template will fail to load the intended event guidance.
Useful? React with 👍 / 👎.
| * ModuleName module implementation. | ||
| * [Description] | ||
| */ | ||
| export class ModuleName implements IModuleName { |
There was a problem hiding this comment.
The "Simple Module (no events)" example defines ModuleName as implements IModuleName, but earlier in the same template IModuleName is defined as extending Eventing<ModuleNameEventTypes>. If an engineer or agent follows this template literally, the generated class will not satisfy the interface contract (missing event-emitter members) and will fail TypeScript checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49e6e882e2
ℹ️ 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".
|
|
||
| class ICallerId { | ||
| <<interface>> | ||
| +fetchCallerDetails(callerInfo) Promise~DisplayInformation~ |
There was a problem hiding this comment.
The class diagram currently documents ICallerId.fetchCallerDetails(callerInfo) as returning Promise<DisplayInformation>, but the real interface in src/CallingClient/calling/CallerId/types.ts returns DisplayInformation synchronously. If implementers follow this spec, they can incorrectly introduce async behavior into caller-ID resolution even though Call relies on an immediate return plus callback-based enrichment, which can break expected call-flow timing.
Useful? React with 👍 / 👎.
|
|
||
| | Event Key | Payload | Emitted When | | ||
| |-----------|---------|--------------| | ||
| | `CALL_EVENT_KEYS.ALERTING` | `correlationId` | Remote side is alerting | |
There was a problem hiding this comment.
This emitted-events table says CALL_EVENT_KEYS.ALERTING is published to consumers, but Call does not emit that key anywhere (call-progress signaling emits CALL_EVENT_KEYS.PROGRESS instead). Consumers or agents that wire UI transitions to ALERTING based on this spec will wait on an event that never arrives, causing incorrect call-state handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This event is never used, so maybe we can remove this
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd0725c08d
ℹ️ 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.metricManager.submitCallMetric( | ||
| METRIC_EVENT.CALL, | ||
| 'newMethod', | ||
| METRIC_TYPE.BEHAVIORAL, | ||
| this.callId, |
There was a problem hiding this comment.
This template is routed for adding methods to many existing modules (e.g. CallingClient, Line, CallHistory), but the core implementation example hardcodes Call-specific members (submitCallMetric, this.callId, this.correlationId, createCallError, CALL_EVENT_KEYS.CALL_ERROR). If an engineer/agent follows it for non-Call modules, the generated code will not match those module contracts and can fail type checks or wire the wrong metrics/events.
Useful? React with 👍 / 👎.
|
|
||
| | Event | Enum Key | Payload | When Emitted | | ||
| |-------|----------|---------|-------------| | ||
| | `alerting` | `CALL_EVENT_KEYS.ALERTING` | `CorrelationId` | Outgoing call alerting at remote | |
There was a problem hiding this comment.
The emitted-events table states that CALL_EVENT_KEYS.ALERTING is emitted, but call.ts never publishes that event (it emits PROGRESS for call progress). This can cause consumers to subscribe to an event that never fires and stall UI state transitions. Fresh evidence: this newly added AGENTS spec repeats the same incorrect ALERTING contract in its own event table.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfd7a8f2e7
ℹ️ 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".
| - On each tick, `Call` sends `POST /devices/{deviceId}/calls/{callId}/status`. | ||
| - Success resets keepalive retry tracking and schedules the next keepalive cycle. | ||
| - Failure increments `callKeepaliveRetryCount` and schedules retry via `RetryCallBack`. | ||
| - After max retries (4), the call is force-disconnected by sending `E_SEND_CALL_DISCONNECT`. |
There was a problem hiding this comment.
This section says the call is force-disconnected after 4 keepalive retries, but the implementation does not do that in the retry-limit path: callKeepaliveRetryCallback exits when callKeepaliveRetryCount === MAX_CALL_KEEPALIVE_RETRY_COUNT and does not send E_SEND_CALL_DISCONNECT (call.ts). This mismatch matters when repeated keepalive failures (for example 500/503 flows that use retry callbacks) occur, because readers and generated changes will assume a disconnect transition/event that never happens.
Useful? React with 👍 / 👎.
rarajes2
left a comment
rarajes2
left a comment
There was a problem hiding this comment.
Reviewed AGENTS.md files under src/
|
|
||
| ### 2. Mobius Event Intake and Routing | ||
| - Subscribes to `event:mobius` via `SDKConnector` and processes signaling/media events. | ||
| - Routes each event to the correct `Call` object based on `correlationId` and `callId` matching. |
There was a problem hiding this comment.
I think it should be based on mobiusEvent
There was a problem hiding this comment.
It is based on mobiusEvent, that's what the first line is about but if the call object already exisits we do a match using these ids. We maintain a callCollection so we need to fetch the call object from ther and that is done based on coorrelationId
|
|
||
| ## CallManager | ||
|
|
||
| ### Purpose |
There was a problem hiding this comment.
| ### Purpose | |
| ### Key Capabilities |
There was a problem hiding this comment.
This is a sub heading not the main headings. Documentation does have Overview and Key Capabilities at the start and when we are decribing each of the class present inide this folder that's where it says purpose to describe the reason behind the class existence. if you still feel we need to change this, let me know
| `CallManager` is a **singleton** that serves as the central hub for all call-related operations. It: | ||
| - Maintains the collection of active `Call` objects keyed by `correlationId` | ||
| - Listens for Mobius WebSocket events (`event:mobius`) via the `SDKConnector` | ||
| - Routes incoming Mobius events to the correct `Call` instance |
There was a problem hiding this comment.
It's not incorrect but I have updated to add one more step where it talks about checking eventType before matching the call object
| - Emits `ALL_CALLS_CLEARED` when the last call is removed from the collection | ||
| - Emits `INCOMING_CALL` to signal the Line about new incoming calls | ||
|
|
||
| ### Singleton Pattern |
There was a problem hiding this comment.
Should we keep it here or patterns/ should take care of this ?
There was a problem hiding this comment.
Patterns does have detail for singleton patterns. Here it's just mentioned in context of how complete CallManager module is designed so that's why can have it here to give reference on how this object is consumed
|
|
||
| | Property | Type | Description | | ||
| |----------|------|-------------| | ||
| | `callCollection` | `Record<CorrelationId, ICall>` | Active calls keyed by client-side correlation ID | |
There was a problem hiding this comment.
Q. Why are we not keeping the callManger under Line. Each Line could have one CallManager which will subsequently have map of calls. Just curious about the current implementation
~~ NOT IN THE SCOPE OF THIS PR ~~
There was a problem hiding this comment.
Its a singleton instance, same instance gets used across all the lines even we create multiple lines
packages/calling/src/Agents.md
Outdated
| | Module | Main Class | Factory Function | Interface | Description | | ||
| |--------|-----------|-----------------|-----------|-------------| | ||
| | **CallingClient** | `CallingClient` | `createClient()` | `ICallingClient` | Core module for line registration and call management | | ||
| | **CallHistory** | `CallHistory` | `createCallHistoryClient()` | `ICallHistory` | Retrieval, update, and deletion of call history records | | ||
| | **CallSettings** | `CallSettings` | `createCallSettingsClient()` | `ICallSettings` | Call waiting, DND, call forwarding, voicemail settings | | ||
| | **Contacts** | `ContactsClient` | `createContactsClient()` | `IContacts` | Contact and contact group management | | ||
| | **Voicemail** | `Voicemail` | `createVoicemailClient()` | `IVoicemail` | Voicemail list, content, summary, read/unread, delete, transcripts | |
There was a problem hiding this comment.
Should cover NoiseReductionEffect, Logger etc as well. Please check
There was a problem hiding this comment.
This file will be removed, it got added initially when I creating .md file
packages/calling/src/Agents.md
Outdated
| Each `ai-docs/` folder contains: | ||
| - **Agents.md** - Module purpose, key capabilities, and high-level behavior | ||
| - **Architecture.md** - Low-level specifications including state machines, events, APIs, and architectural diagrams |
There was a problem hiding this comment.
Should this info be inside the README.md ?
packages/calling/src/Agents.md
Outdated
| | Mercury WebSocket| | ROAP Media (via | | ||
| | (event:mobius) | | @webex/internal- | | ||
| | | | media-core) | | ||
| +------------------+ +-------------------+ |
There was a problem hiding this comment.
could be mermaid flowchart
packages/calling/src/Agents.md
Outdated
| |--------|---------| | ||
| | **Errors** | Hierarchical error classes: `ExtendedError` -> `CallingClientError`, `LineError`, `CallError` | | ||
| | **Events** | `Eventing<T>` base class (typed `EventEmitter`) and all event enums/types | | ||
| | **Logger** | Logging singleton with configurable levels (`ERROR`, `WARN`, `LOG`, `INFO`, `TRACE`) | |
There was a problem hiding this comment.
Logger methods' order seems incorrect here
There was a problem hiding this comment.
Order doesn't matter here because we are just trying to show what all levels are there to be configued but in detailed information about logging it is mentioned by order
| @@ -0,0 +1,175 @@ | |||
| # CallerId Sub-Module - Agent Specification | |||
There was a problem hiding this comment.
Do we need to create separate AGENTS.md for CallerId since it's pretty small ?
There was a problem hiding this comment.
That's why I have not added ARCHITECTURE.md for this. But method details and how this component is designed has to be added somewhere so keeping AGENTS.md file for that reason
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
| | Component | Primary Responsibility | Key Interactions | | ||
| |-----------|------------------------|------------------| | ||
| | `CallManager` | Owns active call collection, resolves/routs backend events | `SDKConnector` (`event:mobius`), `Call`, `Line` | | ||
| | `Call` | Executes call lifecycle operations and state machines | `Mobius` REST APIs, `RoapMediaConnection`, `CallerId`, app listeners | |
There was a problem hiding this comment.
Do you think it would be better to mention this is a Call
There was a problem hiding this comment.
I didn't understand this, could you please elaborate
There was a problem hiding this comment.
I mean to say this is a class can we mention that.
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
packages/calling/src/CallingClient/calling/ai-docs/ARCHITECTURE.md
Outdated
Show resolved
Hide resolved
| K --> L[Subscribe EFFECT_ADDED to onEffectEnabled] | ||
| K --> M[Subscribe EFFECT_REMOVED to onEffectDisabled] |
There was a problem hiding this comment.
- Doc Claims: "Subscribe EFFECT_ADDED to onEffectEnabled" and "Subscribe EFFECT_REMOVED to onEffectDisabled"
- Actual State:
call.ts:2786subscribes EffectAdded tothis.registerEffectListener(which is a different method that checks for noise reduction effect and then subscribes Enabled/Disabled). EffectRemoved is not subscribed at all.
…4-Call-Flow-Specs
…webex-js-sdk into SPARK-787604-Call-Flow-Specs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be44aad880
ℹ️ 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".
| ## Step 5: Export from `src/api.ts` | ||
|
|
||
| ```typescript | ||
| // Add to src/api.ts | ||
|
|
There was a problem hiding this comment.
This template routes new-module work to export only through src/api.ts, but @webex/calling’s runtime and type entrypoint is src/index.ts (main/types resolve to dist/.../index), so following this guidance for a public module will compile internally yet ship an API that consumers cannot import. Please include src/index.ts export steps/checks alongside src/api.ts so generated modules are actually reachable to SDK users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1602b12c88
ℹ️ 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".
| export type CallEventTypes = { | ||
| [CALL_EVENT_KEYS.ALERTING]: (callId: CallId) => void; | ||
| [CALL_EVENT_KEYS.CALL_ERROR]: (error: CallError) => void; | ||
| [CALL_EVENT_KEYS.CALLER_ID]: (display: CallerIdDisplay) => void; |
There was a problem hiding this comment.
This change removes CALL_EVENT_KEYS.ALERTING from the exported CallEventTypes map while CALL_EVENT_KEYS.ALERTING remains in the public enum, creating an inconsistent API contract. Because ICall/ICallManager event methods are typed from CallEventTypes, existing TypeScript consumers using call.on(CALL_EVENT_KEYS.ALERTING, ...) will now fail to compile even though the enum still advertises the key. Either keep the mapping for compatibility (with deprecation) or remove the enum key in the same change as an explicit breaking update.
Useful? React with 👍 / 👎.
| ```typescript | ||
| // In src/api.ts - add new public types | ||
| export { NewType } from './ModuleName/types'; | ||
| ``` |
There was a problem hiding this comment.
This template tells authors to export newly added public types only from src/api.ts, but @webex/calling publishes src/index.ts as its main/types entrypoint and index.ts does not re-export api.ts. Following this guidance for a new public method/type will produce code that appears complete internally yet is not importable by SDK consumers from @webex/calling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abe58dc416
ℹ️ 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".
| |--------|-----------|-------------| | ||
| | `scheduleCallKeepaliveInterval` | `private scheduleCallKeepaliveInterval = (): void` | Schedules periodic `postStatus()` call to Mobius | | ||
| | `callKeepaliveRetryCallback` | `private callKeepaliveRetryCallback = (interval: number): void` | Retries keepalive POST after error with given interval | | ||
| | `handleCallKeepaliveError` | `private handleCallKeepaliveError = async (err: unknown): Promise<void>` | Handles keepalive errors, increments retry count, force-disconnects after max retries | |
There was a problem hiding this comment.
This row says keepalive handling force-disconnects after max retries, but the implementation does not do that on retry exhaustion: callKeepaliveRetryCallback returns immediately when callKeepaliveRetryCount === MAX_CALL_KEEPALIVE_RETRY_COUNT (call.ts), and disconnect is only triggered in a separate abort path inside handleCallKeepaliveError. Keeping this statement as-is will cause agents/readers to design behavior and tests around a disconnect transition that is not guaranteed to occur.
Useful? React with 👍 / 👎.
|
There are some comments that are not addressed. Please address them before merging. |
3 participants
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-787604
This pull request addresses
Adding specs(.md files) for calling folder.
by making the following changes
Added AGENTS.md and ARCHITECTURE.md under calling folder for call.ts and callManager.ts. Covers call flows (incoming, outgoing, hold/resume, transfer, disconnect), state machine design, transitions, and events, call management.
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.