Skip to content

SignalR Service Improvements

Note: This is a temporary planning document. Delete after improvements are implemented.

Overview

The SignalRService in the web application has several areas that could be improved for better testability, reliability, and maintainability. These improvements were identified during PR #2238 when fixing test failures caused by timer subscription leaks.

File: src/services/web/src/app/core/services/signal-r/signal-r.service.ts

Problem Statement

The current implementation has:

  1. Null safety issues - Multiple places access _hubConnection without null checks, causing runtime errors when the connection is undefined
  2. Testing challenges - Manual subscription array management requires tests to access internal implementation details
  3. Tight coupling - HubConnection is created internally, making it impossible to inject mocks
  4. Mixed concerns - Connection management, event handling, and subscription tracking are all in one large service

1. Add Null Safety to _hubConnection Access (Priority: High)

Current code (problematic):

// Line 148 - _isConnected getter
private get _isConnected(): boolean {
  return this._hubConnection.state === HubConnectionState.Connected;
}

// Line 596 - timer subscription
map(() => this._hubConnection.state)

Recommended fix:

private get _isConnected(): boolean {
  return this._hubConnection?.state === HubConnectionState.Connected;
}

// In the timer pipe:
map(() => this._hubConnection?.state ?? HubConnectionState.Disconnected)

Other locations to check:

  • Line 575: this._hubConnection.connectionId
  • Line 589: this._hubConnection.connectionId
  • Line 626-627: this._hubConnection.state in _connect()

2. Use takeUntil Pattern Instead of Manual Subscription Arrays (Priority: High)

Current approach:

private _otherSubs: Subscription[] = [];
private _hubSubs: Subscription[] = [];

// In _createHubConnection:
this._otherSubs.push(
  this._isFullyAuthenticated$.subscribe(...)
);

ngOnDestroy(): void {
  this._otherSubs.forEach((sub) => sub.unsubscribe());
  this._hubSubs.forEach((sub) => sub.unsubscribe());
}

Recommended approach:

private readonly _destroy$ = new Subject<void>();

// In subscriptions:
this._isFullyAuthenticated$
  .pipe(takeUntil(this._destroy$))
  .subscribe(...)

ngOnDestroy(): void {
  this._disconnect();
  this._destroy$.next();
  this._destroy$.complete();
}

Benefits:

  • Automatic cleanup - no need to manually track subscriptions
  • Tests don't need to access internal _otherSubs/_hubSubs arrays
  • Standard Angular pattern that's widely understood
  • Prevents subscription leaks if a subscription is accidentally not added to the array

3. Extract HubConnection Factory for Testability (Priority: Medium)

Create injectable factory:

// hub-connection.factory.ts
@Injectable({ providedIn: 'root' })
export class HubConnectionFactory {
  create(url: string, accessTokenFactory: () => Promise<string>): HubConnection {
    return new HubConnectionBuilder()
      .withAutomaticReconnect()
      .withUrl(url, { accessTokenFactory })
      .build();
  }
}

Update service to use factory:

constructor(
  private _hubConnectionFactory: HubConnectionFactory,
  // ... other deps
) {}

private async _createHubConnection(impersonatingUser: IUser | null = null) {
  await this._hubConnection?.stop();
  this._hubConnection = this._hubConnectionFactory.create(
    this._createUrlString(impersonatingUser),
    () => firstValueFrom(this._authService.getAccessTokenSilently())
  );
  // ... setup event handlers
}

Benefits:

  • Tests can provide a mock factory returning a mock HubConnection
  • Removes direct dependency on HubConnectionBuilder
  • Easier to test connection creation logic

4. Add Destroyed Flag to Guard Timers (Priority: Medium)

Add flag and guard:

private _isDestroyed = false;

ngOnDestroy(): void {
  this._isDestroyed = true;
  // ... rest of cleanup
}

// In timer subscription:
timer(5000, 5000)
  .pipe(
    filter(() => !this._isDestroyed && !!this._hubConnection),
    map(() => this._hubConnection.state),
    // ... rest
  )

5. Consider Extracting Connection State Management (Priority: Low)

For larger refactoring, consider separating concerns:

// signalr-connection-manager.service.ts
@Injectable({ providedIn: 'root' })
export class SignalRConnectionManager {
  private readonly _state$ = new BehaviorSubject<ConnectionState>('disconnected');
  readonly state$ = this._state$.asObservable();

  connect(): Promise<void> { ... }
  disconnect(): Promise<void> { ... }

  get isConnected(): boolean {
    return this._state$.value === 'connected';
  }
}

// signalr-subscription-manager.service.ts
@Injectable({ providedIn: 'root' })
export class SignalRSubscriptionManager {
  subscribeToProject(projectId: string): Promise<void> { ... }
  unsubscribeFromProject(projectId: string): Promise<void> { ... }
  // ... other subscription methods
}

Benefits:

  • Single responsibility principle
  • Easier to test each concern independently
  • Clearer code organization

Implementation Plan

Phase 1: Quick Wins ✅ COMPLETED

  • Add null checks to all _hubConnection access points
  • Add filter guard in timer to prevent null access

Phase 2: Subscription Cleanup Refactor ✅ COMPLETED

  • Replace _otherSubs/_hubSubs arrays with takeUntil pattern
  • Update tests to remove internal array access
  • Verify all subscriptions are properly cleaned up

Phase 3: Testability Improvements

  • Create HubConnectionFactory service
  • Update SignalRService to use factory
  • Add comprehensive unit tests with mocked HubConnection

Phase 4: Architecture Improvements (Optional)

  • Extract SignalRConnectionManager
  • Extract SignalRSubscriptionManager
  • Update consumers to use new services

Testing Considerations

After improvements, tests should be able to:

  1. Mock the HubConnection via the factory
  2. Not access internal subscription arrays - cleanup happens automatically
  3. Test connection state transitions without race conditions
  4. Verify subscription/unsubscription calls on the mock hub
  • src/services/web/src/app/core/services/signal-r/signal-r.service.ts
  • src/services/web/src/app/core/services/signal-r/signal-r.service.spec.ts

References