docs(calling): add sdd patterns for calling package#4776
docs(calling): add sdd patterns for calling package#4776rarajes2 merged 6 commits intowebex:nextfrom
Conversation
|
@codex review |
![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: 518d0aa0f6
ℹ️ 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 const createClient = ( | ||
| webex: WebexSDK, | ||
| config?: CallingClientConfig | ||
| ): ICallingClient => new CallingClient(webex, config); | ||
|
|
There was a problem hiding this comment.
This snippet documents createClient as synchronous and encourages a follow-up client.init() call, but the real factory is async and already calls init() internally (packages/calling/src/CallingClient/CallingClient.ts:840-847). Consumers following this pattern will either use the API incorrectly without await or call initialization twice, which can duplicate setup work (line creation/listener registration) and produce hard-to-debug behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This needs to be fixed. Good point codex
| CallSettings | ||
| ├── WxCallBackendConnector (Webex Calling) | ||
| ├── UcmBackendConnector (UCM) | ||
| └── BroadworksBackendConnector (Broadworks) |
There was a problem hiding this comment.
The architecture map states that CallSettings has a BroadworksBackendConnector, but the module only contains WxCallBackendConnector and UcmBackendConnector implementations (packages/calling/src/CallSettings/*). Keeping this entry will push readers/LLMs toward imports and abstractions that do not exist in the package, causing dead-end implementation paths and compile errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point. Should be removed.
| it('should wait for timer', async () => { | ||
| jest.useFakeTimers(); | ||
|
|
||
| call.startKeepalive(); |
There was a problem hiding this comment.
This test example calls call.startKeepalive(), but there is no such public method on ICall (the interface exposes postStatus() for keepalive-related behavior in packages/calling/src/CallingClient/calling/types.ts). Copying this snippet into real tests will fail type-checking and mislead contributors about the supported call API surface.
Useful? React with 👍 / 👎.
| - **MUST** use `SDKConnector` for all Webex SDK interactions (requests, Mercury listeners) | ||
| - **MUST** separate backend-specific logic into connector classes (WxCall, UCM, Broadworks) | ||
| - **MUST** keep types co-located in `types.ts` within each module | ||
| - **MUST** export public API through `src/api.ts` only |
There was a problem hiding this comment.
This is only for the typeDoc documentation generation. See if needs modification
| export const createClient = ( | ||
| webex: WebexSDK, | ||
| config?: CallingClientConfig | ||
| ): ICallingClient => new CallingClient(webex, config); | ||
|
|
There was a problem hiding this comment.
This needs to be fixed. Good point codex
| CallSettings | ||
| ├── WxCallBackendConnector (Webex Calling) | ||
| ├── UcmBackendConnector (UCM) | ||
| └── BroadworksBackendConnector (Broadworks) |
There was a problem hiding this comment.
Good point. Should be removed.
| ### Line Errors | ||
|
|
||
| ```typescript | ||
| this.lineEmitter(LINE_EVENTS.ERROR, undefined, lineError); |
There was a problem hiding this comment.
Mention deviceInfo as optional 2nd param instead of undefined
|
|
||
| For client and device-level errors (device registration, Mobius discovery, etc.). | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
u can directly point to file/code block instead of copying the whole code again docs? same for all the places we have written whole code in docs. instead small examples
| - **MUST** mock `@webex/internal-media-core` at the top of call-related test files | ||
| - **MUST** maintain ≥85% line/function/statement coverage and ≥80% branch coverage | ||
| - **MUST** clean up mocks in `beforeEach`/`afterEach` with `jest.clearAllMocks()` | ||
| - **NEVER** test implementation details — test observable behavior and event emissions |
There was a problem hiding this comment.
should we add an example for this line ? like what observable reviews to want our focus to be on? so the Agent has better context?
| // webex.internal.services.getMobiusClusters → jest.fn() | ||
| // webex.logger → mock logger with all levels | ||
| // webex.people.list → jest.fn() | ||
| ``` |
There was a problem hiding this comment.
Any Info about why we have not mentioned rest of the parameters & methods here?
There was a problem hiding this comment.
Added note: "For the full list of mocked properties, refer to src/common/testUtil.ts". The doc shows the most commonly used properties as a quick reference.
| let callManager: ICallManager; | ||
|
|
||
| beforeEach(() => { | ||
| callManager = getCallManager(webex, defaultServiceIndicator); |
There was a problem hiding this comment.
import {ServiceIndicator} from '../../common/types';
const defaultServiceIndicator = ServiceIndicator.CALLING;
let's add these lines in the example as well
| call.on(CALL_EVENT_KEYS.ALERTING, alertingSpy); | ||
|
|
||
| // Trigger the action that causes the event | ||
| callManager.dequeueWsEvents(incomingCallEvent); |
There was a problem hiding this comment.
dequeueWsEvents Method is private. so i'm not sure if accessing it by dot is correct.
There was a problem hiding this comment.
Fixed. Replaced the event testing example that used callManager.dequeueWsEvents() with a pattern that triggers events through the public API (call.dial() + flushPromises) and verifies observable behavior.
|
|
||
| ```typescript | ||
| // Single response | ||
| webex.request.mockReturnValueOnce({ |
There was a problem hiding this comment.
as webex.request returns an promise, i guess it should be mockResolvedValue,
There was a problem hiding this comment.
Fixed. Changed all mockReturnValueOnce → mockResolvedValueOnce and mockReturnValue → mockResolvedValue in the mock response configuration examples.
| }); | ||
|
|
||
| // Persistent response | ||
| webex.request.mockReturnValue({statusCode: 200, body: {}}); |
| it('should wait for timer', async () => { | ||
| jest.useFakeTimers(); | ||
|
|
||
| call.startKeepalive(); |
There was a problem hiding this comment.
it's an private method right? so i guess we should not use directly '.' here
There was a problem hiding this comment.
We don't need this file. Patterns docs would be referred for something that we would be creating repeatedly in the package. State machine is something created once and that too depends on if we have something like call or task to manage it's states. When we create global ai-docs for SDK, that's where the patten for state-machine will be appliacable
| ### Call Events | ||
|
|
||
| ```typescript | ||
| export type CallEventTypes = { |
There was a problem hiding this comment.
Critical: Incomplete CallEventTypes — This type map is missing two entries that exist in the source at src/Events/types.ts:213,219:
[LINE_EVENT_KEYS.INCOMING_CALL]: (callObj: ICall) => void; // line 213
[CALLING_CLIENT_EVENT_KEYS.ALL_CALLS_CLEARED]: () => void; // line 219This also hides the fact that CallEventTypes crosses enum boundaries — it uses keys from three different enums (CALL_EVENT_KEYS, LINE_EVENT_KEYS, CALLING_CLIENT_EVENT_KEYS), which is architecturally significant for LLM consumers.
| export enum WEBSOCKET_KEYS { | ||
| CALL_PROGRESS = 'callprogress', | ||
| CALL_CONNECTED = 'callconnected', | ||
| CALL_DISCONNECTED = 'callconnected', |
There was a problem hiding this comment.
Critical: Source code bug reproduced without flagging — CALL_DISCONNECTED = 'callconnected' is a copy-paste bug in the source (src/Events/types.ts:304) where both CALL_CONNECTED and CALL_DISCONNECTED map to the same string 'callconnected'.
The doc should flag this rather than silently propagating it. Suggestion: add an inline comment like:
CALL_DISCONNECTED = 'callconnected', // ⚠️ Bug in source — should likely be 'calldisconnected'Also consider filing a separate issue to fix the source.
There was a problem hiding this comment.
Noted, will create a ticket for this.
|
|
||
| emit<E extends keyof T>(event: E, ...args: Parameters<T[E]>): boolean { | ||
| const timestamp = new Date().toUTCString(); | ||
| Logger.info(`${timestamp} ${LOG_PREFIX.EVENT}: ${event.toString()} - event emitted`); |
There was a problem hiding this comment.
Inaccurate: emit() log message is wrong — The actual source at src/Events/impl/index.ts:23-31 is:
Logger.info(
`${timestamp} ${LOG_PREFIX.EVENT}: ${event.toString()} - event emitted with parameters -> ${args}`,
{ file: 'Events/impl/index.ts', method: 'emit' }
);Two differences: (a) missing with parameters -> ${args} suffix, (b) missing {file, method} second argument. An LLM consumer would generate code with the wrong Logger.info signature.
|
|
||
| --- | ||
|
|
||
| ## Event Key Enums |
There was a problem hiding this comment.
Missing enums in this section:
-
LINE_EVENT_KEYS(src/Events/types.ts:16-18) — A separate enum fromLINE_EVENTSwithINCOMING_CALL = 'incoming_call'(vsLINE_EVENTS.INCOMING_CALL = 'line:incoming_call'). This distinction matters becauseCallEventTypesusesLINE_EVENT_KEYS, notLINE_EVENTS. -
COMMON_EVENT_KEYS(src/Events/types.ts:9-14) — Used inCallHistoryEventTypesandVoicemailEventTypestype maps shown in this doc, but the enum definition is never shown. -
MEDIA_CONNECTION_EVENT_KEYS(src/Events/types.ts:184-187) — Event-related enum that is completely absent.
|
|
||
| ```typescript | ||
| // Emit with callId | ||
| this.emit(CALL_EVENT_KEYS.ALERTING, this.correlationId); |
There was a problem hiding this comment.
Unverified example — grep -rn "emit.*ALERTING" packages/calling/src/ returns zero results. CALL_EVENT_KEYS.ALERTING is defined in the type map (types.ts:205) but is never actually emitted anywhere in the codebase. Consider replacing with a verified example like CALL_EVENT_KEYS.PROGRESS or CALL_EVENT_KEYS.CONNECT.
| ### Emitting from a Line | ||
|
|
||
| ```typescript | ||
| lineEmitter: (event: LINE_EVENTS, deviceInfo?: IDeviceInfo, lineError?: LineError) => void; |
There was a problem hiding this comment.
Misleading simplification — The actual lineEmitter at src/CallingClient/line/index.ts:190-215 uses a switch statement with important logic:
- For
REGISTERED: callsthis.normalizeLine(deviceInfo)then emitsthis(theILineinstance), notdeviceInfo - For
ERROR: only emits iflineErroris truthy - For
UNREGISTERED/RECONNECTED/RECONNECTING: emits with no args
The doc's simplified view suggests lineEmitter is a pass-through, which is incorrect.
|
|
||
| ```typescript | ||
| private listenForWsEvents() { | ||
| this.sdkConnector.registerListener<MobiusCallEvent>( |
There was a problem hiding this comment.
Doc doesn't match actual code — The actual listenForWsEvents at src/CallingClient/calling/callManager.ts:130 uses a raw string, not the enum:
// Actual code:
this.sdkConnector.registerListener('event:mobius', async (event) => {
this.dequeueWsEvents(event);
});The doc shows MOBIUS_EVENT_KEYS.SERVER_EVENT_INCLUSIVE and <MobiusCallEvent> type parameter, neither of which exist in the real code. Either document reality or note this is the recommended pattern.
There was a problem hiding this comment.
Done. The actual code should have used the enum but for now keeping it par with code
| ### Session Listener Pattern in CallingClient | ||
|
|
||
| ```typescript | ||
| private registerSessionsListener() { |
There was a problem hiding this comment.
Heavily oversimplified — The actual registerSessionsListener at src/CallingClient/CallingClient.ts:665-691 contains critical business logic the doc omits:
// Actual code filters by session type:
const sessionArr = event?.data.userSessions.userSessions;
if (sessionArr.length === 1) {
if (sessionArr[0].sessionType !== SessionType.WEBEX_CALLING) {
return; // ← silently drops non-WEBEX_CALLING sessions
}
}
for (let i = 0; i < sessionArr.length; i += 1) {
if (sessionArr[i].sessionType !== SessionType.WEBEX_CALLING) {
sessionArr.splice(i, 1); // ← mutates the array
}
}The doc shows a trivial if (event) { this.emit(...) }. At minimum add a comment noting the filtering, since this is a reference for LLMs that will generate code based on it.
| | Client | Factory | Interface | Purpose | | ||
| |--------|---------|-----------|---------| | ||
| | `CallingClient` | `createClient()` | `ICallingClient` | Line registration, call management | | ||
| | `CallHistory` | `createCallHistoryClient()` | `ICallHistory` | Call records | | ||
| | `CallSettings` | `createCallSettingsClient()` | `ICallSettings` | DND, forwarding, voicemail settings | | ||
| | `ContactsClient` | `createContactsClient()` | `IContacts` | Contact management | | ||
| | `Voicemail` | `createVoicemailClient()` | `IVoicemail` | Voicemail operations | |
There was a problem hiding this comment.
It would be nice to have another column as description, just a small text telling what these are.
There was a problem hiding this comment.
The client modules table was moved out of the patterns file (per Priya's feedback that it belongs in architecture-level ai-docs, not patterns). The pattern file now focuses on reusable patterns only.
| ## High-Level Architecture | ||
|
|
||
| ``` | ||
| ┌──────────────────────────────────────────────────────────────────┐ |
There was a problem hiding this comment.
We should be using memaid diagrams here
There was a problem hiding this comment.
High-Level Architecture and Call Lifecycle were moved out of the patterns file per Priya's feedback. The event-patterns.md now uses a mermaid sequence diagram for the WebSocket event flow. Error-handling-patterns.md uses a mermaid flowchart for the error flow overview.
|
|
||
| ## Call Lifecycle | ||
|
|
||
| ``` |
There was a problem hiding this comment.
We should be using memaid diagrams here
There was a problem hiding this comment.
Not required in this file. Removed.
There was a problem hiding this comment.
Callback patterns, is something I feel we can add here as well
|
|
||
| ## Singleton Pattern | ||
|
|
||
| ### SDKConnector |
There was a problem hiding this comment.
For each of these can we add more description
| ```typescript | ||
| // PascalCase | ||
| CallingClient.ts | ||
| CallManager.ts | ||
| SDKConnector.ts |
There was a problem hiding this comment.
Aren't these file names? I understand class names would look similar but seems misleading. Let's may be have a table to say file name and class name accordingly
| enum CALL_EVENT_KEYS { ... } | ||
| enum ERROR_TYPE { ... } | ||
| enum CallDirection { ... } | ||
| enum RegistrationStatus { ... } |
There was a problem hiding this comment.
Why is this of 3 different casings?
There was a problem hiding this comment.
Added explanation table showing the three casing conventions, what they're used for, and examples. Added guidance for new enums.
| export {CallHistory, CallSettings, CallingClient, ContactsClient, Voicemail}; | ||
|
|
||
| // Types | ||
| export {ContactGroup, Contact, CallForwardSetting, VoicemailSetting}; |
There was a problem hiding this comment.
Should it be export type?
| export {ContactGroup, Contact, CallForwardSetting, VoicemailSetting}; | |
| export type {ContactGroup, Contact, CallForwardSetting, VoicemailSetting}; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85d8f7f050
ℹ️ 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".
| responsePayload, | ||
| ERROR_LAYER.CALL_CONTROL, | ||
| this.correlationId, | ||
| {file: CALL_FILE, method: 'dial'}, | ||
| () => this.retryOperation() // optional retry callback |
There was a problem hiding this comment.
handleCallErrors with the real parameter order
This example uses the wrong argument order (responsePayload is passed second) and omits the required trailing file argument, while handleCallErrors in packages/calling/src/common/Utils.ts is defined as (emitterCb, errorLayer, retryCb, correlationId, err, caller, file) and returns a Promise<boolean>. If copied into code, this will either fail type-checking or invoke the handler with incorrect semantics, so generated error handling logic will be broken.
Useful? React with 👍 / 👎.
| const {abort} = handleCallingClientErrors(responsePayload, { | ||
| file: CALLING_CLIENT_FILE, | ||
| method: 'getMobiusServers', | ||
| }); |
There was a problem hiding this comment.
handleCallingClientErrors as async boolean API
The snippet destructures {abort} from handleCallingClientErrors(...), but the actual helper in packages/calling/src/common/Utils.ts returns Promise<boolean> and requires an emitter callback as its second argument. Following this example will produce invalid callsites (wrong arguments and wrong return handling), and callers may skip emitting CallingClientError events because the callback path is missing.
Useful? React with 👍 / 👎.
| const callHistory = createCallHistoryClient(webex, config); | ||
| const callSettings = createCallSettingsClient(webex, config); | ||
| const contacts = createContactsClient(webex, config); | ||
| const voicemail = createVoicemailClient(webex, config); |
There was a problem hiding this comment.
These examples pass config into createCallHistoryClient/createCallSettingsClient/createContactsClient/createVoicemailClient, but those factories are typed to take (webex, logger[, ...]) in their respective source files. Copying this pattern from the docs will produce type errors and lead contributors/LLMs to wire the wrong initialization inputs for every non-CallingClient module.
Useful? React with 👍 / 👎.
| | `log.info()` | Normal operations, method entry/exit, state transitions | Starting registration, call connected | | ||
| | `log.warn()` | Recoverable issues, degraded behavior, fallbacks | ICE warmup failed (will retry), missing optional config | | ||
| | `log.error()` | Unrecoverable failures, critical errors | Device registration failed, unhandled exception | |
There was a problem hiding this comment.
Aren't there more log types? Can we get them also into the patterns?
riteshfyi
left a comment
riteshfyi
left a comment
There was a problem hiding this comment.
Everything Looks Good, just a minor suggestion, Approval for testing-patterns.md
| describe('Call Tests', () => { | ||
| const deviceId = '55dfb53f-bed2-36da-8e85-cee7f02aa68e'; | ||
| const dest = {type: CallType.URI, address: 'tel:5003'}; | ||
| let callManager: ICallManager; |
There was a problem hiding this comment.
should we mention where to get more info about the call Managar?
There was a problem hiding this comment.
I think that information is covered in the File Structure section and we also have Related section with other docs reference
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a628f5dff
ℹ️ 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 enum ServiceIndicator { | ||
| CALLING = 'calling', | ||
| CONTACTCENTER = 'contactcenter', |
There was a problem hiding this comment.
ServiceIndicator enum member names
The enum example documents ServiceIndicator.CONTACTCENTER, but the exported enum in packages/calling/src/common/types.ts uses CONTACT_CENTER (and also includes GUEST_CALLING). If contributors or LLM-generated code copy this snippet for branching or comparisons, they will reference a non-existent member and hit type-check failures.
Useful? React with 👍 / 👎.
| ```typescript | ||
| // SCREAMING_SNAKE_CASE | ||
| const MAX_RETRY_COUNT = 3; | ||
| const DEFAULT_SESSION_TIMER = 180; |
There was a problem hiding this comment.
DEFAULT_SESSION_TIMER with the actual value
This snippet states DEFAULT_SESSION_TIMER = 180, but the real constant is 1000 * 60 * 10 in packages/calling/src/CallingClient/constants.ts. Reusing the documented value in keepalive logic or tests would drastically shorten the interval and produce unrealistic behavior (or excessive status calls).
Useful? React with 👍 / 👎.
| dial(localAudioStream: LocalMicrophoneStream): Promise<void>; | ||
| answer(localAudioStream: LocalMicrophoneStream): Promise<void>; | ||
| end(): void; | ||
| doHoldResume(): void; | ||
| sendDigit(tone: string): Promise<void>; |
There was a problem hiding this comment.
ICall method returns with the exported API
The interface pattern shows dial, answer, and sendDigit returning Promise<void>, but the public ICall contract in packages/calling/src/CallingClient/calling/types.ts declares these methods as void. This mismatch will mislead consumers into Promise-based call flows that conflict with the package typings and create avoidable type errors.
Useful? React with 👍 / 👎.
Kesari3008
left a comment
Kesari3008
left a comment
There was a problem hiding this comment.
Approving with few comments that still need to be taken care of
| const voicemail = createVoicemailClient(webex, logger); | ||
| ``` | ||
|
|
||
| **Important differences from `createClient`**: The non-CallingClient factories (`createCallHistoryClient`, `createCallSettingsClient`, `createContactsClient`, `createVoicemailClient`) are **synchronous** and take a `LoggerInterface` as their second argument — not a `CallingClientConfig`. `LoggerInterface` is defined as `{ level: LOGGER }` (where `LOGGER` is the string enum from `src/Logger/types.ts`). `createCallSettingsClient` additionally accepts an optional `useProdWebexApis?: boolean` third argument. |
There was a problem hiding this comment.
Refer to other modules as just other client modules, calling them non-callingclient doesn't seem very appropriate
|
|
||
| This document describes the typed event system used throughout the `@webex/calling` package. | ||
|
|
||
| The package implements a fully type-safe event architecture built on Node.js `EventEmitter` (from `events`) augmented with compile-time type enforcement via `typed-emitter`. At its core is the `Eventing<T>` generic base class (`src/Events/impl/index.ts`), which every event-emitting class in the package extends — including `CallingClient`, `Call`, `CallManager`, `Line`, `CallHistory`, and `Voicemail`. Each of these classes is parameterized with a dedicated event type map (e.g., `CallingClientEventTypes`, `CallEventTypes`, `LineEventTypes`, `CallHistoryEventTypes`, `VoicemailEventTypes`) defined in `src/Events/types.ts`, which statically associates string-valued enum keys to their corresponding callback signatures. All event keys are defined as TypeScript `enum` members across `CALL_EVENT_KEYS`, `LINE_EVENTS`, `LINE_EVENT_KEYS`, `CALLING_CLIENT_EVENT_KEYS`, `COMMON_EVENT_KEYS`, `MOBIUS_EVENT_KEYS`, and `MEDIA_CONNECTION_EVENT_KEYS` — raw string keys are never used directly. |
There was a problem hiding this comment.
Can we put these points in bullet points?
|
|
||
| ## Eventing Base Class | ||
|
|
||
| All event-emitting classes extend this generic typed emitter (`src/Events/impl/index.ts`). |
There was a problem hiding this comment.
Add source file locations like this with a title or something. Like Source File:
|
|
||
| ### Emitting from a Line | ||
|
|
||
| The `lineEmitter` method (`src/CallingClient/line/index.ts`) is **not** a simple pass-through — it contains important business logic via a `switch` statement: |
There was a problem hiding this comment.
Add an example code for calling lineEMitter as well
|
|
||
| ## Event Emission Pattern | ||
|
|
||
| Events are emitted using `this.emit()` with the enum constant as the event name and the typed payload as arguments. Because every emitting class extends `Eventing<T>`, the compiler enforces that the enum key and payload match the class's event type map at every call site. The `Eventing.emit()` override logs every emission with a UTC timestamp before delegating to the underlying `EventEmitter`. Emission happens across five classes — `Call` (the most prolific emitter, covering call lifecycle and error events), `Line` (registration state and incoming calls), `CallManager` (incoming call signaling and all-calls-cleared), `CallingClient` (client-level errors and session info), and `CallHistory` (session, viewed, and deleted events). `Voicemail` defines `VoicemailEventTypes` in its type map but does not currently contain any `this.emit()` call sites in its implementation. |
There was a problem hiding this comment.
This could be in bullet points
|
|
||
| --- | ||
|
|
||
| ## Event Key Enums |
There was a problem hiding this comment.
This comment is missed I believe
|
|
||
| --- | ||
|
|
||
| ## Pattern for Adding New Events |
There was a problem hiding this comment.
Provide references here in this summary for sections above where you have provided details about how each of this step is done
|
|
||
| ### serviceErrorCodeHandler | ||
|
|
||
| Utility that maps HTTP status codes to structured response objects (not `ERROR_TYPE` values). Used by voicemail, call settings, contacts, and call history modules — **not** for the call/line/client error hierarchy. |
There was a problem hiding this comment.
Why are we saying not ERROR_TYPE, directly reference the ERROR code. I believe AI resolved these comments. This section needs manual updating so please check
| ## Error Flow Overview | ||
|
|
||
| ```mermaid | ||
| flowchart TD |
There was a problem hiding this comment.
ServiceErrorCodeHandler is mentioned but we do not cover how it's related to rest of the error flow so adding a link/relation would be good here
|
|
||
| --- | ||
|
|
||
| ## Error Handler Utilities |
There was a problem hiding this comment.
We have handler for registration and keepalive errors as well right, add a reference for those as well
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bbd9a144e
ℹ️ 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".
| ```typescript | ||
| // In types.ts | ||
| export interface ICallingClient extends Eventing<CallingClientEventTypes> { | ||
| getLine(): ILine | undefined; |
There was a problem hiding this comment.
getLine() in client contract snippet
The interface example declares getLine(): ILine | undefined, but the actual public contract exposes getLines(): Record<string, ILine> and does not define getLine (packages/calling/src/CallingClient/types.ts). If readers/LLMs copy this snippet, they will generate call sites that fail type-checking against the exported SDK API.
Useful? React with 👍 / 👎.
| const line = callingClient.getLines()[0]; | ||
|
|
||
| line.on(LINE_EVENTS.REGISTERED, (lineInfo: ILine) => { | ||
| console.log('Line registered:', lineInfo.deviceId); |
There was a problem hiding this comment.
ILine field in the REGISTERED listener example
This listener logs lineInfo.deviceId, but LINE_EVENTS.REGISTERED emits an ILine and ILine does not have a deviceId property (packages/calling/src/CallingClient/line/types.ts); it provides getDeviceId()/mobiusDeviceId instead. Copying this example into strict TypeScript code will produce a property-does-not-exist error.
Useful? React with 👍 / 👎.
| ERROR_LAYER.CALL_CONTROL | ||
| ); | ||
| this.emit(CALL_EVENT_KEYS.CALL_ERROR, callError); | ||
| uploadLogs(this.webex); |
There was a problem hiding this comment.
uploadLogs instead of webex
The snippet calls uploadLogs(this.webex), but uploadLogs is typed as uploadLogs(metaData?: LogsMetaData, throwError?: boolean) and obtains the Webex instance internally from SDKConnector (packages/calling/src/common/Utils.ts). Using this example directly leads to incorrect typing and teaches an invalid API contract for log uploads.
Useful? React with 👍 / 👎.
Thank you for your contribution! |
7 participants
COMPLETES #< SPARK-787602 >
This pull request addresses
Adds SDD patterns for the calling package.
by making the following changes
Created:
packages/calling/ai-docs/patterns/typescript-patterns.mdIprefix on types,Iprefix on interfaces, enum patterns, discriminated unions)architecture-patterns.mdevent-patterns.mdEventing<T>base class instead of observable storesstate-machine-patterns.mderror-handling-patterns.mdExtendedError→CallError/LineError/CallingClientErrorhierarchytesting-patterns.mdEvery pattern document was derived from reading the actual source code, not generated generically. The code examples reference real classes, enums, and interfaces from the codebase.
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.