Skip to content

docs(calling): add sdd docs for the CallingClient, excludes the calling sub-directory#4777

Open
rarajes2 wants to merge 4 commits intowebex:nextfrom
rarajes2:calling-spec-2
Open

docs(calling): add sdd docs for the CallingClient, excludes the calling sub-directory#4777
rarajes2 wants to merge 4 commits intowebex:nextfrom
rarajes2:calling-spec-2

Conversation

@rarajes2
Copy link
Copy Markdown
Contributor

@rarajes2 rarajes2 commented Mar 15, 2026

COMPLETES #< SPARK-787603 >

This pull request addresses

Adds SDD docs for the CallingClient, excludes the calling sub-directory.

by making the following changes

Created Documentation Structure

packages/calling/src/CallingClient/
├── ai-docs/
│   ├── AGENTS.md           ← Top-level: overview, purpose, examples, public API, configuration, subdirectory references
│   └── ARCHITECTURE.md     ← Top-level: component table, layer diagram, 5 Mermaid sequence diagrams, constants, troubleshooting
├── line/
│   └── ai-docs/
│       ├── AGENTS.md       ← Line: ILine API, all events, all properties, usage examples
│       └── ARCHITECTURE.md ← Line: lineEmitter pattern, normalizeLine, incoming call listener, makeCall flow
├── registration/
│   └── ai-docs/
│       ├── AGENTS.md       ← Registration: IRegistration API, key concepts, 429 retry, keepalive worker overview
│       └── ARCHITECTURE.md ← Registration: full registration/failover/failback flows, web worker architecture, reconnection logic, error mapping
└── calling/                ← (no docs created per your request)

Key design decisions:

  • CallingClient-level docs are common to all child folders — they describe the orchestrator role and reference child docs
  • Line docs focus on the lineEmitter pattern, event forwarding, and call initiation — the bridge between Registration and the application
  • Registration docs are the deepest — they cover the full failover/failback/keepalive worker architecture with detailed flow diagrams
  • No docs created for calling/ - This will be added in a separate PR by Priya

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 were tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

The GAI Coding Policy And Copyright Annotation Best Practices

  • GAI was not used (or, no additional notation is required)
  • Code was generated entirely by GAI
  • GAI was used to create a draft that was subsequently customized or modified
  • Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
  • Tool used for AI assistance (GitHub Copilot / Other - specify)
    • Github Copilot
    • Other - Cursor with calude-4.6-opus-high
  • This PR is related to
    • Feature
    • Defect fix
    • Tech Debt
    • Automation

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.

@rarajes2 rarajes2 requested a review from a team as a code owner March 15, 2026 13:01
@rarajes2 rarajes2 added the validated If the pull request is validated for automation. label Mar 15, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee25d15d4a

ℹ️ 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".

Comment on lines +75 to +78
The `createClient` factory instantiates `CallingClient` and calls `init()`, which:
1. Performs ICE warmup (Windows Chromium only)
2. Discovers Mobius servers for the client region
3. Creates a Line and begins registration
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct createClient initialization flow

This section states that createClient(...).init() “creates a Line and begins registration,” but CallingClient.init() only calls getMobiusServers(), createLine(), and setupNetworkEventListeners() and never invokes line.register(); the line remains IDLE until the app calls register() itself. Consumers who follow this text literally can wait for registration events that never fire.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Now,

Step 3 reads "Creates a Line object internally" with an explicit note: "init() does NOT register the line. The application must call line.register() explicitly after obtaining the line via getLines()."

| `getDevices` | `(userId?: string): Promise<DeviceType[]>` | Fetches devices from Mobius for the user |
| `getActiveCalls` | `(): Record<string, ICall[]>` | Returns active calls grouped by lineId |
| `getConnectedCall` | `(): ICall \| undefined` | Returns the currently connected (non-held) call |
| `uploadLogs` | `(): Promise<UploadLogsResponse>` | Uploads diagnostic logs to Webex |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Align ICallingClient API table with exported typings

The table documents uploadLogs as part of the ICallingClient interface, but createClient is typed to return Promise<ICallingClient> and packages/calling/src/CallingClient/types.ts does not define uploadLogs on that interface. TypeScript consumers following this API table will get a compile-time property-missing error unless they cast away the published type.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rarajes2 rarajes2 changed the base branch from calling-sdk-specs to next March 16, 2026 13:58
Copy link
Copy Markdown
Contributor Author

@rarajes2 rarajes2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Location: All 6 files reference packages/calling/ai-docs/patterns/

Issue: All files reference pattern files that exist in calling-spec branch (commit 518d0aa) but this PR is on calling-spec-2 branch (commit ee25d15).

References found in:

  • CallingClient/AGENTS.md:9-14, 241-244
  • line/AGENTS.md:9, 206
  • registration/AGENTS.md:9, 201
  • And ARCHITECTURE.md files

Impact: Broken documentation links if branches not merged in order.

Recommendation: Ensure calling-spec branch merged before calling-spec-2, or add dependency notes.

| Constant | Value | Description |
|----------|-------|-------------|
| `DEVICES_ENDPOINT_RESOURCE` | `calling/web/devices` | Device registration |
| `CALL_ENDPOINT_RESOURCE` | `calls` | Call creation |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual: (constants.ts:5, 7)

export const CALL_ENDPOINT_RESOURCE = 'call';      // singular
export const CALLS_ENDPOINT_RESOURCE = 'calls';    // plural

Impact: Could mislead developers about which constant to use.

Recommendation: List both constants separately or correct to 'call' with note about CALLS_ENDPOINT_RESOURCE.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines +307 to +310
| `BASE_REG_RETRY_TIMER_VAL_IN_SEC` | varies | Base retry timer |
| `BASE_REG_TIMER_MFACTOR` | varies | Multiplication factor for backoff |
| `REG_RANDOM_T_FACTOR_UPPER_LIMIT` | varies | Randomization upper bound |
| `RETRY_TIMER_UPPER_LIMIT` | varies | Max retry timer value |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual: (constants.ts:105-108)

export const BASE_REG_RETRY_TIMER_VAL_IN_SEC = 30;
export const BASE_REG_TIMER_MFACTOR = 2;
export const REG_RANDOM_T_FACTOR_UPPER_LIMIT = 0.1;
export const RETRY_TIMER_UPPER_LIMIT = 1800;

Recommendation: Replace "varies" with actual constant values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


| Constant | Value | Description |
|----------|-------|-------------|
| `DEVICES_ENDPOINT_RESOURCE` | `calling/web/devices` | Device registration |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actual: The constant is just 'devices'.

Recommendation: Clarify whether showing constants or full paths: {mobiusUrl}devices.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines +91 to +100
### Keepalive Web Worker

The keepalive mechanism runs in a dedicated **Web Worker** to avoid being blocked by main-thread activity:

- **Start:** Worker receives `START_KEEPALIVE` with access token, device URL, interval, and retry threshold
- **Loop:** Worker sends `POST /devices/{id}/status` every `keepaliveInterval` seconds
- **Success:** Worker posts `KEEPALIVE_SUCCESS` to main thread
- **Failure:** Worker posts `KEEPALIVE_FAILURE` with error details and retry count
- **Stop:** Main thread sends `CLEAR_KEEPALIVE`, Worker clears interval and main thread terminates it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Documentation uses string message type names but code uses enum.

Documented: START_KEEPALIVE (plain string)
Actual: WorkerMessageType.START_KEEPALIVE (enum)

Recommendation: Clarify these are enum values or mention both representations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Copy Markdown
Contributor

@Kesari3008 Kesari3008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have reviewed only CallingClient ai-docs as of now. Line and registration folder pending

@@ -0,0 +1,256 @@
# CallingClient Module

## AI Agent Routing Instructions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we could add in README.md or root AGENTS.md and we do not need to add this every AGENTS.md file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have simplified it a bit. This will be a bit redundant for now. We can groom it once we have all the spec files in place

| **Registration** | `Registration` | `registration/register.ts` | Device register/deregister, keepalive via web worker, failover/failback, reconnection |
| **Call Management** | `CallManager` | `calling/callManager.ts` | Call collection, WebSocket event routing, call creation/deletion |
| **Call** | `Call` | `calling/call.ts` | Single call lifecycle via XState, media negotiation (ROAP), hold/resume/transfer/mute |
| **SDK Bridge** | `SDKConnector` | `SDKConnector/index.ts` | Singleton Webex SDK wrapper, HTTP requests, Mercury listener registration |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 are not components of CallingClient module so we need not to mention them in this table. Those are shared entities which are used across calling package by all client modules so better place to put them would calling/src folder's ai-docs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not completely addressed, I still see Metrics and Logging in the table

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

| **Metrics** | `MetricManager` | `Metrics/index.ts` | Telemetry submission for registration, calls, errors, BNR |
| **Logging** | `Logger` | `Logger/index.ts` | Structured logging with file/method context |

### Singletons and Factories
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is not needed and is not really adding any additional information, details captured here can be clubbed with previous table itself

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table gives map of methods with components. Let me know if still need to be removed. Component table is updated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we can add a column in component table itself for Access pattern but if you would like to keep separate table then also its fine

Copy link
Copy Markdown
Contributor

@Kesari3008 Kesari3008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line/ai-docs reviewed
registration/ai-docs/AGENTS.md reviewed

Copy link
Copy Markdown
Contributor

@Shreyas281299 Shreyas281299 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

⚠️ REQUEST CHANGES - Comprehensive AI-generated documentation with excellent structure, but contains critical accuracy issues that need correction.

Validated by Devil's Advocate


Critical Issues (Must Fix)

1. Broken Links to Non-Existent Patterns Directory

  • 15+ references to packages/calling/ai-docs/patterns/ across all 6 files
  • Directory does not exist in the repository
  • See inline comment on AGENTS.md:9

2. Fabricated API Methods

  • getDevices method does not exist anywhere in codebase
  • uploadLogs exists on class but not on ICallingClient interface
  • See inline comment on AGENTS.md:172

High Priority

3. Missing OUTGOING_CALL Event

  • Event exists in codebase but missing from events table
  • See inline comment on AGENTS.md:180

4. Phantom setDeviceInfo Method

  • Listed in IRegistration table at registration/ai-docs/AGENTS.md line 232
  • Method does not exist on IRegistration interface
  • Only exists as metricManager.setDeviceInfo() at register.ts:833
  • Action: Remove from IRegistration table

Medium/Low Priority

5. Constants Show "varies" Instead of Actual Values

  • See inline comment on ARCHITECTURE.md:291

6-7. Style/Documentation Issues

  • String literal vs enum (AGENTS.md:76)
  • Wrong "you are here" comment (ARCHITECTURE.md:312)

8. Static Last-Updated Dates

  • All 6 files have _Last Updated: 2026-03-15_ at the end
  • Static dates will become stale quickly
  • Suggestion: Remove dates (git log is authoritative) or use commit-based format

Positive Highlights

✅ Excellent hierarchical structure (CallingClient > Line > Registration)
✅ Comprehensive and accurate Mermaid sequence diagrams
✅ Thorough troubleshooting guide with real scenarios
✅ Well-documented lineEmitter callback pattern
✅ Good cross-referencing between the 6 files


Action Items

Must fix before merge:

  • 2 Critical issues (broken links, fabricated APIs)
  • 2 High priority issues (missing event, phantom method)

Nice to have:

  • 1 Medium (constant values)
  • 3 Low (style/maintenance)

Total inline comments: 6 posted + 2 additional items noted above

🤖 Review by Claude Code with team-based validation

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 002f1539d4

ℹ️ 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".

Comment on lines +156 to +157
const callDetails = {type: 'uri', address: 'sip:user@example.com'};
const call = line.makeCall(callDetails);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use a dialable address in outbound call example

This quick-start example uses address: 'sip:user@example.com', but Line.makeCall() validates dest.address with VALID_PHONE_REGEX and only accepts dialable number formats before converting to tel:; a SIP URI like this is rejected, emits LINE_EVENTS.ERROR, and returns undefined. Users copying this snippet will fail to place a call immediately.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. No change required


Reg->>Reg: attemptRegistrationWithServers(primaryUris)
loop For each URI in primaryMobiusUris
Reg->>Mobius: POST /calling/web/devices
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct registration endpoint path in sequence diagram

The diagram documents initial registration as POST /calling/web/devices, but the actual registration request in Registration.postRegistration() targets ${url}device (singular). This mismatch can send readers to the wrong route when debugging or implementing request traces, especially because deregistration does use /devices/{id}.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

When the main thread receives `KEEPALIVE_FAILURE`:

1. **Emit `RECONNECTING`** via `lineEmitter` to notify the application
2. **Check retry count** against threshold (`MAX_CALL_KEEPALIVE_RETRY_COUNT = 4`)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document flow-specific keepalive retry threshold

This section states a fixed keepalive threshold of MAX_CALL_KEEPALIVE_RETRY_COUNT = 4, but registration keepalive retries are flow-dependent in startKeepaliveTimer() (4 for contact center and 5 otherwise). The current wording misstates when reconnect escalation happens for standard calling flows, which can mislead troubleshooting and expected recovery timing.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


The `CallingClient` is one of the significant modules in the Webex Calling SDK, responsible for the main WebRTC call flow implementation. It manages line registration, call lifecycle coordination, Mobius server discovery, and network resilience.

Applications create a `CallingClient` via the `createClient()` factory function and interact with lines and calls through it. Other client modules such as `CallHistoryClient`, `VoicemailClient`, and `CallSettingsClient` are independently available and do not require `CallingClient` to be initialized.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to talk about other modules at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

| **Media Engine Management** | Initializes and configures the `@webex/internal-media-core` engine to negotiate, establish, and manage WebRTC media streams for audio and video calls. |
| **Call Keepalive** | Periodically sends keepalive messages for both Lines and active Calls, ensuring session continuity and timely detection of network or signaling issues. |
| **Call Control** | Orchestrates all aspects of call initiation, handling, and features. Divided into the following subcapabilities: |
| &nbsp;&nbsp;• Outbound Calls | Enables agents to initiate outbound calls using `line.makeCall()`. Handles call setup, signaling, and media path establishment, including error cases. |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need not to be separate entry in the table, under CallControl you can add them

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not separate
Screenshot 2026-04-01 at 5 36 36 PM

| `getConnectedCall` | `(): ICall \| undefined` | Returns the currently connected (non-held) call |
| `mediaEngine` | `typeof Media` | The `@webex/internal-media-core` engine |

### CallingClient Class Methods (not on ICallingClient interface)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of saying not on ICallingClient interface, we should simply talk about where its imported from

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have uploadLogs defined inside the CallingClient which uses the uploadLogs from util

  /**
   * Uploads logs to help troubleshoot SDK issues.
   *
   * This method collects the current SDK logs including network requests, WebSocket
   * messages, and client-side events, then securely submits them to Webex's diagnostics
   * service. The returned tracking ID, feedbackID can be provided to Webex support for faster
   * issue resolution.
   * @returns Promise<UploadLogsResponse>
   * @throws Error
   */
  public async uploadLogs(): Promise<UploadLogsResponse> {
    const result = await uploadLogs({}, true);
    if (!result) {
      throw new Error('Failed to upload logs: No response received.');
    }

    return result;
  }

| `discovery.country` | No | Auto-detected | Override country for Mobius discovery |
| `discovery.region` | No | Auto-detected | Override region for Mobius discovery |
| `serviceData.indicator` | No | `CALLING` | Backend: `calling` or `contactcenter` |
| `serviceData.domain` | No | `''` | Backend domain |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not addressed

| `discovery.region` | No | Auto-detected | Override region for Mobius discovery |
| `serviceData.indicator` | No | `CALLING` | Backend: `calling` or `contactcenter` |
| `serviceData.domain` | No | `''` | Backend domain |
| `jwe` | No | - | JSON Web Encryption token for secure registration |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not addressed

| **Registration** | `Registration` | `registration/register.ts` | Device register/deregister, keepalive via web worker, failover/failback, reconnection |
| **Call Management** | `CallManager` | `calling/callManager.ts` | Call collection, WebSocket event routing, call creation/deletion |
| **Call** | `Call` | `calling/call.ts` | Single call lifecycle via XState, media negotiation (ROAP), hold/resume/transfer/mute |
| **SDK Bridge** | `SDKConnector` | `SDKConnector/index.ts` | Singleton Webex SDK wrapper, HTTP requests, Mercury listener registration |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not completely addressed, I still see Metrics and Logging in the table


## Data Flows

### Layer Communication Flow
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should separate the class diagram and flowchart

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need more info on this. Not clear on what you meant by separating the class diagram and flowchart

end
```

### 2. Outbound Call Flow
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not addressed

CC->>Mobius: getMobiusServers()
Mobius-->>CC: {primary: [...], backup: [...]}

CC->>Line: new Line(userId, deviceUri, mutex, mobiusUris, ...)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

| **Metrics** | `MetricManager` | `Metrics/index.ts` | Telemetry submission for registration, calls, errors, BNR |
| **Logging** | `Logger` | `Logger/index.ts` | Structured logging with file/method context |

### Singletons and Factories
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we can add a column in component table itself for Access pattern but if you would like to keep separate table then also its fine

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3241ae915

ℹ️ 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".

| Event | Enum Key | Payload | Description |
| ------------------------------------ | --------------------------------------------- | -------------------- | ---------------------------- |
| `callingClient:error` | `CALLING_CLIENT_EVENT_KEYS.ERROR` | `CallingClientError` | Client-level error |
| `callingClient:outgoing_call` | `CALLING_CLIENT_EVENT_KEYS.OUTGOING_CALL` | `string` (callId) | Outbound call initiated |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove unsupported outgoing_call client event from API table

This event is documented as emitted, but the runtime does not dispatch it: CallingClient emits ERROR and USER_SESSION_INFO, and CallManager emits ALL_CALLS_CLEARED; there is no emit(CALLING_CLIENT_EVENT_KEYS.OUTGOING_CALL) path. Applications that subscribe to callingClient:outgoing_call based on this table will never receive callbacks, which can break outbound-call telemetry and UI state handling.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting this point. We will need to fix the code as well

The behavior of `makeCall` varies by `ServiceIndicator`:

- **`ServiceIndicator.CALLING`** (licensed users): `destination` is mandatory and validated. A `CallDetails` object with a valid `type` and `address` must be provided.
- **`ServiceIndicator.GUEST_CALLING`** or **`ServiceIndicator.CONTACT_CENTER`**: Destination handling may differ — destination may be optional or pre-configured server-side depending on the flow.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clarify CONTACT_CENTER makeCall still requires destination

The architecture text implies CONTACT_CENTER flows may allow an optional destination, but Line.makeCall() only skips dest when serviceData.indicator === ServiceIndicator.GUEST_CALLING; for CONTACT_CENTER, omitting dest returns undefined and no call is created. This wording can send contact-center integrators down a non-working call initiation path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// Sends DELETE /devices/{id} to Mobius
// Stops keepalive Web Worker
// Sets status to IDLE
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct Registration deregister status in example

This example states registration.deregister() sets status to IDLE, but Registration.deregister() sets INACTIVE; IDLE is only set by the higher-level Line.deregister() wrapper afterward. Readers using Registration directly for reconnection or diagnostics can make incorrect state-based decisions from this documented behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines +245 to +246
3. Retry up to `REG_FAILBACK_429_MAX_RETRIES` (5) times
4. On exhaustion, switch to backup servers
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scope 429 retry cap to failback flow only

This section describes a universal "retry up to 5 times then switch to backup" behavior for HTTP 429, but the 5-attempt cap (REG_FAILBACK_429_MAX_RETRIES) is only enforced in the failback branch of handle429Retry; initial registration and keepalive 429 paths follow different logic. Documenting it as global gives incorrect recovery expectations during troubleshooting.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// Initiate an outbound call after registration
const call = line.makeCall({type: 'uri', address: 'sip:bob@example.com'});

if (call) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to necessarily add the call.on listeners here just line relkated information should be enough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the title we are showing handling of outbound call as well here. Don't think it's gonna harm

});
```

### 3. Making and Handling Outbound Calls
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line only exposes for us to be able to create call object post that no handling is done for call, it's done on call manager so title is not accurate and needs correction

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note

type LineErrorEmitterCallback = (err: LineError, finalError?: boolean) => void;
```

#### About `LineError` and Line Error Handling
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title can just be LineError

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


The `LineError` object encapsulates structured information about errors occurring during Line operations. It typically includes:

- A human-readable error message (e.g., explaining the user-level issue, such as invalid numbers).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add code example references for points here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

## File Structure

```
registration/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For registration folder, file structure is added in both: AGENTS.md and ARCHITECTURE.md

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed from AGENTS.md

A dedicated Web Worker manages keepalive requests to ensure a responsive and reliable heartbeat loop, even when the main thread is inactive.

```mermaid
sequenceDiagram
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep sequence diagrams for Architecture.md file and keep Agents.md having flowcharts. Although we can avoid diagrams here entirely and keep them in ARCHITECTURE.md

Just details in bulleted points should be enough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed from AGENTS.md


Note over Reg: Primary still down, try backup

Reg->>Mobius2: POST /calling/web/devices
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API is inocrrect

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Note over Reg: All primary URIs failed

Reg->>Reg: startFailoverTimer()
Reg->>Reg: Calculate backoff delay
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we referring to our retryInterval as backoff ? Does it understand what it means and how it is used ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Note over Reg: Currently on backup, failback timer fires

Reg->>Reg: executeFailback()
Reg->>Mobius1: POST /calling/web/devices (check primary)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Reg->>Line: lineEmitter(REGISTERED, deviceInfo)
Reg-->>Line: Registration complete
deactivate Reg
else 429 Too Many Requests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

429 handling should be added separately

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's added under title - "429 Retry Logic"


---

## Error Handling
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basic sequence diagram covering what happens on final error and what happens on non-final error would be good

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd1c55cb4e

ℹ️ 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
try {
const response = await callingClient.uploadLogs();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove unsupported uploadLogs call from quick-start snippet

This example calls callingClient.uploadLogs(), but createClient() is typed to return Promise<ICallingClient> and ICallingClient does not expose uploadLogs; TypeScript users copying this snippet will get a property-missing compile error unless they cast to the concrete CallingClient class. Please either show class-level usage explicitly or replace this with an API that exists on ICallingClient.

Useful? React with 👍 / 👎.

Mobius-->>Reg: 200 OK
Reg->>Worker: WorkerMessageType.START_KEEPALIVE (restart)
Reg->>Line: lineEmitter(REGISTERED, deviceInfo)
Line->>App: emit(LINE_EVENTS.RECONNECTED)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fix post-recovery event shown in network sequence

The recovery diagram shows Line emitting LINE_EVENTS.RECONNECTED immediately after handleConnectionRestoration, but the re-registration path actually goes through attemptRegistrationWithServers() and emits REGISTERED; RECONNECTED is only emitted on keepalive recovery (KEEPALIVE_SUCCESS after prior failures). This mismatch can make integrators subscribe to the wrong event and miss successful restoration notifications.

Useful? React with 👍 / 👎.

Reg->>Worker: WorkerMessageType.CLEAR_KEEPALIVE (terminate)
Reg->>Mobius: DELETE /devices/{deviceId}
Mobius-->>Reg: 200 OK
Reg->>Reg: setStatus(IDLE)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct registration status in deregistration diagram

This step documents Registration setting status to IDLE, but Registration.deregister() sets INACTIVE; only the higher-level Line.deregister() wrapper moves it to IDLE afterward. The current sequence is inaccurate for anyone debugging or using Registration directly and can lead to incorrect state-based handling.

Useful? React with 👍 / 👎.


| Constant | Value | Description |
|----------|-------|-------------|
| `DEVICES_ENDPOINT_RESOURCE` | `'devices'` | Device registration (full path: `{mobiusUrl}devices`) |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use singular device registration endpoint in API table

The API endpoint table says device registration is {mobiusUrl}devices, but registration requests are sent via postRegistration() to ${url}device (singular). This route mismatch can mislead troubleshooting and request tracing when validating registration traffic.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

validated If the pull request is validated for automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants