Add optional listen param to dial() to support starting calls using a single method.

[jpsantosbh]

Description

Make dial() async again to support listen parameters similar to the realtime-sdk API. This is an old request from the DevEx team, and we need this before the first public release of @signalwire/client SDK.

The old way still allowed developers to choose between the "dial(), add listeners, start()" or "dial(listen:{...})" approach.

Type of change

• Internal refactoring
• Bug fix (bugfix - non-breaking)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

import { SignalWire } from '@signalwire/client'

// 1. Create client
const client = await SignalWire({
  host: 'your-space.signalwire.com',
  token: 'your-access-token'
})

// 2. Dial with event listeners - call starts automatically!
const call = await client.dial({
  to: '/public/conference-room',
  audio: true,
  video: true,
  listen: {
    'call.joined': (params) => {
      console.log('Call joined!', params)
    },
    'member.joined': (params) => {
      console.log('Member joined:', params.member.name)
    }
  }
})

// Call is already started and ready to use
console.log('Call ID:', call.id)

In case of new feature or breaking changes, please include code snippets.

[jpsantosbh]

This is an attempt to simplify the e2e tests.
This allows for both waiting for a specific event and waiting for a particular change of state.

[LittorWired]

It might be helpful for debugging purposes if page.evalulate() call that awaits or resolves a promise should also have a timeout otherwise we could still potentially have "hanging" promises

[jpsantosbh]

That is a valid point, especially when the promise resolution depends on external events.
But for method calls, the SDK has its internal timeout, and page.evaluate has a default timeout.

[iAmmar7]

Just curious and trying to understand if I am missing something - If the page.evaluate has the default timeout. Why did we even create the new utilities?

[jpsantosbh]

IMO, we don't need the utilities; they are a nice-to-have.

What we need is a test to execute a single action each time it runs in the page scope.
If inside the page.evaluate, we have only await callObj!.audioMute() like here when this page.evaluate fail we now exactly why.

[LittorWired]

We could take advantage of test.step to improve readibility of the test

[LittorWired]

we might want some unit tests for the _callState

[jpsantosbh]

Now you really made me think... I always thought test code is self-testable 🤔

You can still write a faulty test, but that means the code being tested has a "pairing" fault... And if that is the case, the problem most likely is a misunderstanding of the requirements, meaning that all attempts to test will embed the same fault.

I never thought to test a test, but maybe I'm wrong...

But in this case, what do you mean by " unit tests for the _callState"? What function/implementation do you think I should cover?

[LittorWired]

Just that the __callState instance has state and some methods that might be worthwhile to write unit tests so that:

1. That the state and methods work as intended
2. There are no unintended regressions or bugs introduced in the future should this test utility change

[LittorWired]

I guess I was thinking that if these utilities have some level of complexity and are critical to the e2e tests then they might be worthwhile having their own set of basic tests to minimize the chances that the e2e tests don't face errors due to the test code

[jpsantosbh]

Ok, let me try that

[iAmmar7]

Just highlighting:

• the Realtime-API does not have listen param with event names. It simply has callback handlers. For eg; onCallReceived, onPlaybackStarted, etc. Ref: https://developer.signalwire.com/sdks/realtime-sdk/voice/call#events
• similarly the Realtime-API provide event handlers for all the APIs. It also allows attaching child listener on parent. For eg; on parent, the listener callback can be onPlaybackStarted, but on the child (playback API), the same listener can be attached as onStarted. https://developer.signalwire.com/sdks/realtime-sdk/voice/call-playback#events

Based on the above points, I am not sure if that's what the DevExp need. But I will leave this decision up to you.

[iAmmar7]

Can we avoid any, if possible?

[jpsantosbh]

In this case, I want any, this is to handle ANY object passed to it.

[iAmmar7]

Pretty weird, I am not sure why we even need these aliases?

[iAmmar7]

@claude, can you check why we are using the aliases while creating the Playwright test server using Vite? The e2e-client package is a part of the monorepo, which has the @signalwire/client package already. Do we even need these aliases?

[jpsantosbh]

@iAmmar7 I discovered why... This PR was made on top of the SDK version with type export issues. #1299

At some point, CC added this as a workaround for the issue, but it can now be removed.
#1301

[iAmmar7]

dial is now going to start the call as well?

[jpsantosbh]

Only if the user opted to pass the listeners.
But yes, dial now can be used to dial the address, not just build a session then later start the call.

[iAmmar7]

await' has no effect on the type of this expression.ts(80007)

You probably don't need the await here.

[jpsantosbh]

What if the developer using the SDK passes an async function to listen to the event?

[iAmmar7]

We might be able to move this to the CallSession class so that you don't need to pass the callSession as a parameter.

[jpsantosbh]

I agree, we can do that later

[iAmmar7]

I think you can avoid any here:

Considering the listen type in Dial and Reattach is Partial<CallSessionEvents>, you may need to export the CallSessionEvents type from the SDK.

Suggested change

	const listenHandlers: any = {}
	const listenHandlers: Partial<CallSessionEvents> = {}

[iAmmar7]

The import should be relative to avoid build issues.

[jpsantosbh]

good catch

[claude]

Claude finished @jpsantosbh's task —— View job

---

Pull Request Review

Reviewing PR #1289: Add optional listen param to dial() to support starting calls using a single method.

Review Tasks:

• Read and analyze core implementation files
• Review WSClient.ts changes and dial() method modifications
• Examine interface changes and type definitions
• Analyze test coverage and quality
• Review e2e utilities and test implementations
• Check for security and performance concerns
• Evaluate existing review comments and concerns
• Provide comprehensive feedback and recommendations

Summary

This PR successfully implements the optional listen parameter for the dial() method, allowing developers to start calls with event listeners attached in a single method call. The implementation follows good patterns and includes comprehensive test coverage.

Code Quality Assessment

✅ Strengths

1. Clean API Design: The listen parameter integration is well-designed and maintains backward compatibility
2. Comprehensive Testing: Excellent test coverage with 962 lines of well-structured unit tests covering both success and error scenarios
3. Error Handling: Robust error handling with proper cleanup on failures
4. Type Safety: Good TypeScript integration with proper type definitions

⚠️ Areas for Improvement

Critical Issues

1. Missing Changeset - The changeset-bot correctly identified that this PR needs a changeset since it adds new functionality

2. Import Path Issue (packages/client/src/unified/interfaces/wsClient.ts:30):

   import { CallSessionEvents } from 'packages/client/src/utils/interfaces'  // ❌ Absolute path

   Should be:

   import { CallSessionEvents } from '../../utils/interfaces/fabric'  // ✅ Relative path

Minor Issues

3. Unnecessary await (WSClient.ts:469):

   await handler(params)  // The await may not be necessary for sync handlers

   Consider checking if handler is async before awaiting.

4. Type Usage: Several places use any type that could be more specific:

   • CallStateManager.ts: Multiple any types could be replaced with specific interfaces
   • E2E utilities: Some handler parameters could be better typed

Questionable Design Decisions

5. E2E Test Aliases: The Vite aliases in e2e tests seem unnecessary since the packages are in the same monorepo. This adds complexity without clear benefit.

6. CallStateManager Complexity: The CallStateManager utility in e2e tests has significant logic that might warrant its own unit tests (as suggested in review comments).

Security & Performance

• ✅ Security: No security concerns identified. Proper error isolation in event handlers prevents failures from affecting other handlers
• ✅ Performance: Event listener wrapping is efficient and errors are properly contained
• ✅ Memory Management: Proper cleanup on failures with callSession.destroy()

Test Coverage

Excellent - The test suite is comprehensive:

• Unit tests for both dial() and reattach() methods
• Error handling scenarios including cleanup failures
• Event listener functionality and error isolation
• Integration scenarios with complete flow testing
• Edge cases like storage unavailability

Addressing Review Comments

Most existing review comments have valid concerns:

1. ✅ Import path issue: Confirmed - should use relative imports
2. ✅ await unnecessarily: Confirmed - conditional await would be better
3. ✅ Type improvements: any types could be more specific
4. ✅ Missing changeset: Required for version tracking

Recommendations

High Priority

1. Add changeset for version bump tracking
2. Fix import path to use relative imports
3. Consider conditional await for sync vs async handlers

Medium Priority

4. Improve type safety by replacing any with specific types
5. Consider simplifying e2e test setup by removing unnecessary aliases

Low Priority

6. Add unit tests for CallStateManager utility if it's intended to be reusable

Overall Assessment

This is a well-implemented feature with excellent test coverage and clean API design. The core functionality is solid, but there are a few technical debt items that should be addressed, particularly the missing changeset and import path issue.

Recommendation: Approve after addressing the critical issues (changeset, import paths).

[claude]

Claude finished @jpsantosbh's task —— View job

---

Pull Request Review

Reviewing PR #1289: Add optional listen param to dial() to support starting calls using a single method.

Review Tasks:

• Read and analyze core implementation files
• Review WSClient.ts changes and dial() method modifications
• Examine interface changes and type definitions
• Analyze test coverage and quality
• Review e2e utilities and test implementations
• Check for security and performance concerns
• Evaluate existing review comments and concerns
• Provide comprehensive feedback and recommendations

Summary

This PR successfully implements the optional listen parameter for the dial() method, allowing developers to start calls with event listeners attached in a single method call. The implementation follows good patterns and includes comprehensive test coverage.

Code Quality Assessment

✅ Strengths

1. Clean API Design: The listen parameter integration is well-designed and maintains backward compatibility
2. Comprehensive Testing: Excellent test coverage with 962 lines of well-structured unit tests covering both success and error scenarios
3. Error Handling: Robust error handling with proper cleanup on failures
4. Type Safety: Good TypeScript integration with proper type definitions

⚠️ Areas for Improvement

Critical Issues

1. Missing Changeset - The changeset-bot correctly identified that this PR needs a changeset since it adds new functionality

2. Import Path Issue (packages/client/src/unified/interfaces/wsClient.ts:30):

   import { CallSessionEvents } from 'packages/client/src/utils/interfaces'  // ❌ Absolute path

   Should be:

   import { CallSessionEvents } from '../../utils/interfaces/fabric'  // ✅ Relative path

Minor Issues

3. Unnecessary await (WSClient.ts:469):

   await handler(params)  // The await may not be necessary for sync handlers

   Consider checking if handler is async before awaiting.

4. Type Usage: Several places use any type that could be more specific:

   • CallStateManager.ts: Multiple any types could be replaced with specific interfaces
   • E2E utilities: Some handler parameters could be better typed

Questionable Design Decisions

5. E2E Test Aliases: The Vite aliases in e2e tests seem unnecessary since the packages are in the same monorepo. This adds complexity without clear benefit.

6. CallStateManager Complexity: The CallStateManager utility in e2e tests has significant logic that might warrant its own unit tests (as suggested in review comments).

Security & Performance

• ✅ Security: No security concerns identified. Proper error isolation in event handlers prevents failures from affecting other handlers
• ✅ Performance: Event listener wrapping is efficient and errors are properly contained
• ✅ Memory Management: Proper cleanup on failures with callSession.destroy()

Test Coverage

Excellent - The test suite is comprehensive:

• Unit tests for both dial() and reattach() methods
• Error handling scenarios including cleanup failures
• Event listener functionality and error isolation
• Integration scenarios with complete flow testing
• Edge cases like storage unavailability

Addressing Review Comments

Most existing review comments have valid concerns:

1. ✅ Import path issue: Confirmed - should use relative imports
2. ✅ await unnecessarily: Confirmed - conditional await would be better
3. ✅ Type improvements: any types could be more specific
4. ✅ Missing changeset: Required for version tracking

Recommendations

High Priority

1. Add changeset for version bump tracking
2. Fix import path to use relative imports
3. Consider conditional await for sync vs async handlers

Medium Priority

4. Improve type safety by replacing any with specific types
5. Consider simplifying e2e test setup by removing unnecessary aliases

Low Priority

6. Add unit tests for CallStateManager utility if it's intended to be reusable

Overall Assessment

This is a well-implemented feature with excellent test coverage and clean API design. The core functionality is solid, but there are a few technical debt items that should be addressed, particularly the missing changeset and import path issue.

Recommendation: Approve after addressing the critical issues (changeset, import paths).

[claude]

Claude finished @jpsantosbh's task —— View job

---

Pull Request Review

Reviewing PR #1289: Add optional listen param to dial() to support starting calls using a single method.

Review Tasks:

• Read repository guidelines and setup instructions
• Analyze core implementation changes in WSClient.ts
• Review new CallStateManager utility and its tests
• Examine test coverage and quality improvements
• Check security and performance considerations
• Evaluate how previous review feedback has been addressed
• Provide comprehensive feedback and recommendations

Summary

This PR successfully implements the optional listen parameter for the dial() method, allowing developers to start calls with event listeners attached in a single method call. The implementation has been significantly improved since the last review with the addition of comprehensive test coverage and addressing most previous feedback.

Code Quality Assessment

✅ Strengths

1. Clean API Design: The listen parameter integration is well-designed and maintains backward compatibility
2. Comprehensive Testing: Excellent test coverage with 962 lines of well-structured unit tests covering both success and error scenarios
3. Error Handling: Robust error handling with proper cleanup on failures and event listener error isolation
4. Type Safety: Good TypeScript integration with proper type definitions
5. New CallStateManager: Well-tested utility with 286 lines of comprehensive unit tests
6. Changeset Added: Proper changeset has been added for version tracking

⚠️ Issues Identified

Critical Issues

1. Import Path Issue (packages/client/src/unified/WSClient.ts:30):

   import { CallSessionEvents } from 'packages/client/src/utils/interfaces'  // ❌ Absolute path

   Should be:

   import { CallSessionEvents } from '../../utils/interfaces/fabric'  // ✅ Relative path

   This has been flagged by previous reviewers and remains unaddressed.

Minor Issues

2. Conditional await (WSClient.ts:469):

   await handler(params)  // May not be necessary for sync handlers

   While the PR author correctly pointed out that user handlers might be async, a conditional check could improve performance for sync handlers.

3. Type Usage in CallStateManager (CallStateManager.ts):

   • Uses any types throughout which could be more specific
   • While the author mentioned wanting any for flexibility, using union types or generics would be safer

4. Typo in Changeset: "envent listeners" should be "event listeners"

Security & Performance

• ✅ Security: No security concerns identified. Error isolation in event handlers prevents failures from affecting other handlers
• ✅ Performance: Event listener wrapping is efficient with proper error containment
• ✅ Memory Management: Proper cleanup on failures with callSession.destroy()

Test Coverage Analysis

Excellent - The test suite now includes:

• ✅ Unit tests for both dial() and reattach() methods (962 lines)
• ✅ Error handling scenarios including cleanup failures
• ✅ Event listener functionality and error isolation
• ✅ CallStateManager utility tests (286 lines covering all edge cases)
• ✅ Integration scenarios with complete flow testing
• ✅ Edge cases like storage unavailability

Addressing Previous Review Feedback

Progress Made:

1. ✅ Changeset added: Now includes proper changeset for version tracking
2. ✅ CallStateManager tests added: Comprehensive unit tests addressing reviewer concerns
3. ✅ Test utility improvements: Better structured e2e test utilities

Still Outstanding:

1. ❌ Import path: Critical relative import issue remains unaddressed
2. ❌ Conditional await: Performance optimization suggestion not implemented
3. ❌ Type safety: any types could be more specific in places

Recommendations

High Priority (Should be addressed before merge)

1. Fix import path in WSClient.ts line 30 to use relative imports
2. Fix typo in changeset: "envent" → "event"

Medium Priority

3. Consider conditional await for better performance with sync handlers
4. Improve type safety in CallStateManager by using more specific types

Low Priority

5. Code organization: Consider moving attachEventListeners to CallSession class as suggested by reviewer

Overall Assessment

This is a well-implemented feature with substantial improvements since the last review. The addition of comprehensive test coverage (both unit and e2e) significantly strengthens the implementation. The core functionality is solid and the API design is clean.

The main blocker is the import path issue which could cause build problems. Once that's resolved, this PR should be ready for merge.

Recommendation: Address the import path issue and typo, then approve.

---