fix: use ?? for autoReconnect default; add onClose and onError callbacks

[osr21]

Summary

Fixes two bugs in src/client.ts reported in #43.

---

Bug 1: autoReconnect: false is silently ignored

Root cause: The constructor used || (logical OR), which treats false as falsy and always falls back to true:

// Before — always evaluates to true, even when caller passes false
this.autoReconnect = args!.autoReconnect || true;

Fix: Replace with ?? (nullish coalescing), which only falls back on null or undefined:

// After — respects explicit false
this.autoReconnect = args!.autoReconnect ?? true;

---

Bug 2: No onClose or onError callbacks exposed to callers

RealTimeDataClientArgs only offered onConnect, onMessage, and onStatusChange. Users had no way to react to disconnect events or errors — they could not implement application-level watchdogs, structured logging, or custom back-off strategies.

Fix: Added two new optional callbacks to both the interface and the class:

// New interface additions
onClose?: (client: RealTimeDataClient, event: CloseEvent) => void;
onError?: (client: RealTimeDataClient, error: ErrorEvent) => void;

Both callbacks fire before the existing reconnect logic, so callers can:

• Log the disconnect reason and error code
• Update external state / UI
• Implement custom exponential back-off (by setting autoReconnect: false and calling connect() themselves on a timer)

---

Example usage after this fix

const client = new RealTimeDataClient({
  autoReconnect: false, // now actually respected
  onClose: (c, event) => {
    console.warn(`Disconnected (code ${event.code}): ${event.reason}`);
    // custom back-off
    setTimeout(() => c.connect(), 5_000);
  },
  onError: (c, err) => {
    console.error("WS error:", err);
    metrics.increment("ws.error");
  },
});

---

Testing

• Verified that passing autoReconnect: false now prevents automatic reconnection on close/error
• Verified onClose fires with the correct CloseEvent (code + reason) before the reconnect gate
• Verified onError fires with the ErrorEvent before the reconnect gate
• No existing behaviour changed when autoReconnect is omitted (defaults to true) or when onClose/onError are not provided

Fixes #43

---

Note

Medium Risk
Changes core connection teardown and auto-reconnect behavior; fixes are targeted but affect all consumers relying on reconnect semantics.

Overview
Fixes autoReconnect: false being ignored by defaulting with ?? instead of ||, and exposes optional onClose / onError hooks so apps can log disconnects and implement custom backoff.

Reconnect lifecycle is tightened: each connect() stamps a connectionId so stale sockets don’t trigger a second reconnect when both error and close fire; user callbacks are wrapped in try/catch so thrown handlers don’t block auto-reconnect. onClose still runs (and status goes DISCONNECTED) even when the socket is stale, so close code/reason aren’t dropped.

Constructor uses optional args?. so new RealTimeDataClient() is safe. disconnect() only closes when the socket exists and isn’t already closing/closed.

^{Reviewed by Cursor Bugbot for commit 5ae00c6. Bugbot is set up for automated code reviews on this repo. Configure here.}

[graphite-app]

If the user's onUserError callback throws an exception, execution stops and the auto-reconnect logic on lines 155-157 never runs. This breaks automatic reconnection.

Fix: Wrap the callback invocation in a try-catch block:

if (this.onUserError) {
    try {
        this.onUserError(this, err);
    } catch (callbackError) {
        console.error('Error in onError callback:', callbackError);
    }
}

Suggested change

	if (this.onUserError) {
	this.onUserError(this, err);
	}
	if (this.onUserError) {
	try {
	this.onUserError(this, err);
	} catch (callbackError) {
	console.error('Error in onError callback:', callbackError);
	}
	}

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[osr21]

Thanks for the bot reviews — both findings are valid and have been addressed in the latest commit.

---

Graphite: throwing onError callback aborts reconnect

Correct. If the user-supplied onError callback throws, execution halts before the if (this.autoReconnect) { this.connect(); } block, silently breaking reconnection with no indication why. The same risk exists symmetrically in onClose.

Fix: wrapped both onUserError and onUserClose invocations in try/catch. Callback exceptions are logged to console.error but no longer propagate into the handler.

---

Cursor Bugbot: stale onclose fires after onError → second reconnect

Also correct, and a subtler race. The sequence:

1. Server drops the connection → onError fires
2. connect() runs → this.ws is replaced with a new WebSocket
3. The old socket (still alive in memory) finishes closing → its onclose handler fires — still pointing at this.onClose
4. onClose runs as if the new session disconnected: onUserClose is called spuriously, notifyStatusChange(DISCONNECTED) fires, and connect() is called a second time

Result: onUserClose sees phantom disconnects on a live session, and the client can enter a double-reconnect loop.

Fix: immediately before calling connect() in the error path, set this.ws.onclose = null on the dying socket so its natural close event is silently swallowed:

if (this.autoReconnect) {
    this.ws.onclose = null; // prevent stale close from firing onUserClose again
    this.connect();
}

This is the same pattern used by most production WS client libraries (e.g. reconnecting-websocket) to break the close-cascade.

[graphite-app]

Runtime crash when args is undefined

The constructor uses non-null assertion (args!) but args is typed as optional (args?: RealTimeDataClientArgs). When new RealTimeDataClient() is called without arguments, args will be undefined, causing args!.onClose and args!.onError to throw:

Cannot read property 'onClose' of undefined

This crashes the application during instantiation.

Fix: Add null check:

this.onUserClose = args?.onClose;
this.onUserError = args?.onError;

Note: Lines 77-82 have the same pre-existing bug with other properties, but since those weren't modified in this PR, the new callback assignments are the immediate issue introduced here.

Suggested change

	this.onUserClose = args!.onClose;
	this.onUserError = args!.onError;
	this.onUserClose = args?.onClose;
	this.onUserError = args?.onError;

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[osr21]

Fixed in the latest commit — replaced all eight args!. accesses in the constructor with args?.:

this.host = args?.host || DEFAULT_HOST;
this.pingInterval = args?.pingInterval || DEFAULT_PING_INTERVAL;
this.autoReconnect = args?.autoReconnect ?? true;
this.onCustomMessage = args?.onMessage;
this.onConnect = args?.onConnect;
this.onStatusChange = args?.onStatusChange;
this.onUserClose = args?.onClose;
this.onUserError = args?.onError;

The existing fallback values (|| DEFAULT_HOST, || DEFAULT_PING_INTERVAL, ?? true) already handle the undefined case correctly, so new RealTimeDataClient() with no arguments now works as expected. The pre-existing lines (host, pingInterval, autoReconnect, onMessage, onConnect, onStatusChange) were fixed at the same time since they had the same crash risk.

[osr21]

Good catch. The ws.onclose = null approach overcorrected — it stopped the double-reconnect but silently dropped the DISCONNECTED status notification and onUserClose on error-driven disconnects, which is arguably worse.

Replaced in the latest commit with an isReconnecting guard flag:

private isReconnecting = false;

private onError = async (err: ErrorEvent) => {
    // ... callback with try/catch ...
    if (this.autoReconnect && !this.isReconnecting) {
        this.isReconnecting = true;
        this.connect(); // this.ws replaced here
    }
};

private onClose = async (message: CloseEvent) => {
    this.notifyStatusChange(ConnectionStatus.DISCONNECTED); // always fires
    // ... onUserClose with try/catch ...
    if (this.autoReconnect && !this.isReconnecting) {
        this.isReconnecting = true;
        this.connect(); // only if onError did not already reconnect
    }
};

public connect() {
    this.isReconnecting = false; // reset for next cycle
    // ...
}

The sequence on an error-driven disconnect is now:

1. onError fires → sets isReconnecting = true → calls connect()
2. Old socket closes naturally → onClose fires → delivers DISCONNECTED + onUserClose ✓
3. onClose sees isReconnecting === true → skips connect() → no double-reconnect ✓
4. connect() resets isReconnecting = false → next cycle works normally ✓

[osr21]

Summary of all fixes applied to this PR

This PR grew across several review iterations. Here is a full log of every issue raised and how it was addressed, in order.

---

Fix 1 — autoReconnect: false silently ignored (original PR)

Root cause: || true treats false as falsy.

// Before
this.autoReconnect = args!.autoReconnect || true; // always true
// After
this.autoReconnect = args?.autoReconnect ?? true; // respects explicit false

---

Fix 2 — No onClose / onError callbacks (original PR)

Added both to RealTimeDataClientArgs and the class, wired through the internal handlers.

---

Fix 3 — Throwing callback aborts autoReconnect (Graphite review)

If a user-supplied onError or onClose callback threw, execution halted before the reconnect branch. Wrapped both in try/catch:

try {
    if (this.onUserError) this.onUserError(this, err);
} catch (callbackError) {
    console.error("Error in onError callback:", callbackError);
}

---

Fix 4 — Stale onClose causes double-reconnect after error (Cursor Bugbot review)

Problem: onError → connect() replaces this.ws, but the dying socket still had onclose = this.onClose. Its natural close fired a spurious onUserClose + second connect().

First attempt: set this.ws.onclose = null before connect(). This stopped the double-reconnect but silently dropped the DISCONNECTED status notification and onUserClose on error-driven disconnects (flagged by Cursor Bugbot in follow-up review).

Final fix: isReconnecting guard flag:

private isReconnecting = false;

private onError = async (err: ErrorEvent) => {
    // ... try/catch callback ...
    if (this.autoReconnect && !this.isReconnecting) {
        this.isReconnecting = true;
        this.connect(); // this.ws replaced here
    }
};

private onClose = async (message: CloseEvent) => {
    this.notifyStatusChange(ConnectionStatus.DISCONNECTED); // always fires ✓
    // ... try/catch callback ...
    if (this.autoReconnect && !this.isReconnecting) {
        this.isReconnecting = true;
        this.connect(); // skipped if onError already reconnected ✓
    }
};

public connect() {
    this.isReconnecting = false; // reset for next cycle
    ...
}

Sequence on error-driven disconnect:

1. onError fires → isReconnecting = true → connect() (new socket assigned)
2. Old socket closes naturally → onClose fires → DISCONNECTED + onUserClose delivered ✓
3. onClose sees isReconnecting === true → skips connect() → no double-reconnect ✓
4. connect() resets flag → next cycle works normally ✓

---

Fix 5 — Runtime crash when called without arguments (Graphite review)

args is typed args?: RealTimeDataClientArgs (optional) but all constructor assignments used args!. (non-null assertion). Calling new RealTimeDataClient() with no arguments crashed immediately with Cannot read properties of undefined.

Replaced all eight args!. accesses with args?.:

// Before — crashes if args is undefined
this.host          = args!.host          || DEFAULT_HOST;
this.pingInterval  = args!.pingInterval  || DEFAULT_PING_INTERVAL;
this.autoReconnect = args!.autoReconnect ?? true;
this.onCustomMessage = args!.onMessage;
this.onConnect       = args!.onConnect;
this.onStatusChange  = args!.onStatusChange;
this.onUserClose     = args!.onClose;
this.onUserError     = args!.onError;

// After — safe, fallbacks handle undefined
this.host          = args?.host          || DEFAULT_HOST;
this.pingInterval  = args?.pingInterval  || DEFAULT_PING_INTERVAL;
this.autoReconnect = args?.autoReconnect ?? true;
this.onCustomMessage = args?.onMessage;
this.onConnect       = args?.onConnect;
this.onStatusChange  = args?.onStatusChange;
this.onUserClose     = args?.onClose;
this.onUserError     = args?.onError;

---

Final state of src/client.ts

Behaviour	Before this PR	After this PR
autoReconnect: false	❌ ignored (|| true)	✅ respected (?? true)
onClose callback	❌ not available	✅ fires on every disconnect
onError callback	❌ not available	✅ fires on every WS error
Throwing callback	❌ aborts reconnect silently	✅ logged, reconnect continues
Error-driven disconnect	❌ double-reconnect race	✅ single reconnect, DISCONNECTED fires
No-args constructor	❌ runtime crash	✅ works, defaults applied

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 3c9994e. Configure here.}

[graphite-app]

Calling disconnect() before connect() will throw an error because this.ws is uninitialized. The class uses a definite assignment assertion (ws!: WebSocket) at line 92, which tells TypeScript the property will be assigned, but disconnect() can be called before any connection is established.

public disconnect() {
    this.autoReconnect = false;
    if (this.ws) {
        this.ws.close();
    }
}

Alternatively, check if the WebSocket is in an appropriate state before closing:

public disconnect() {
    this.autoReconnect = false;
    if (this.ws && this.ws.readyState !== WebSocket.CLOSED && this.ws.readyState !== WebSocket.CLOSING) {
        this.ws.close();
    }
}

Suggested change

	public disconnect() {
	this.autoReconnect = false;
	this.ws.close();
	public disconnect() {
	this.autoReconnect = false;
	if (this.ws && this.ws.readyState !== WebSocket.CLOSED && this.ws.readyState !== WebSocket.CLOSING) {
	this.ws.close();
	}

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[osr21]

Fixed — applied the readyState guard suggestion. this.ws is declared with ws!: WebSocket (definite assignment assertion), so calling disconnect() before connect() dereferences an undefined value and throws immediately. The readyState check also prevents calling close() on a socket that is already CLOSING or CLOSED:

public disconnect() {
    this.autoReconnect = false;
    if (this.ws && this.ws.readyState !== WebSocket.CLOSED && this.ws.readyState !== WebSocket.CLOSING) {
        this.ws.close();
    }
}

This is consistent with the guards already in place on subscribe() and unsubscribe(), which both check this.ws.readyState !== WebSocket.OPEN before sending.