fix(hooks): detach WebSocket handlers on explicit disconnect

[6figpsolseeker]

Summary

• disconnect() in usePriceStream called ws.close() without detaching the onclose handler first. Because close() is async, the onclose callback still fires on the next event loop tick — and its body schedules a setTimeout(connect, ...) for auto-reconnect.
• Result: a zombie reconnect timer wakes up after the app backgrounded or the component unmounted, causing either:
  • A setState on an unmounted component (React warning), or
  • A duplicate WebSocket when the app subsequently foregrounded
• The fix detaches all four handlers (onopen, onmessage, onerror, onclose) before calling close(). For unexpected closes (network drop while foregrounded), the handler stays attached and auto-reconnect still works as designed.

Reproduction (before fix)

1. Open the trade screen, WebSocket connects
2. Background the app (or unmount the screen) — disconnect() runs, wsRef.current = null
3. The async onclose callback fires ~immediately afterward
4. It runs reconnectTimerRef.current = setTimeout(connect, baseDelay + jitter) — a timer in the background-ed app
5. Either: timer fires while still backgrounded → connect() opens a new WS even though the user isn't using the app
6. Or: app foregrounds → handleAppState('active') ALSO calls connect() → double connection

What changed

File	Change
src/hooks/usePriceStream.ts	Detach onopen/onmessage/onerror/onclose before close() in disconnect()
__tests__/hooks/usePriceStream.test.ts	New regression test asserting all four handlers are null after unmount, and no second WebSocket instance is created

Test plan

• All 19 usePriceStream tests pass (18 existing + 1 new regression test)
• Full hook test suite passes (134/134 across 12 suites)
• Manual: open trade screen, background app, foreground after 5 seconds → verify only one active WebSocket connection (check Helius dashboard)
• Manual: navigate away from trade screen mid-connection → verify no React "setState on unmounted" warnings in dev console

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed issue where connection cleanup could trigger unexpected reconnection behavior.

• Tests

  • Added regression test to verify proper cleanup of connections when components unmount.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds explicit WebSocket event handler cleanup to the usePriceStream hook's disconnect method and introduces a regression test to verify proper handler detachment. Previously, handlers remained attached after manual disconnection, leaving onclose logic capable of running post-cleanup.

Changes

Cohort / File(s)	Summary
WebSocket Handler Cleanup src/hooks/usePriceStream.ts	Modified disconnect callback to explicitly nullify onopen, onmessage, onerror, and onclose event handlers before calling close(), preventing stale handler execution after manual cleanup.
Regression Test __tests__/hooks/usePriceStream.test.ts	New test verifies that unmount() detaches all WebSocket event handlers (setting them to null), closes the socket, and prevents late onclose events from triggering reconnect behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A bundle of handlers, so tangled they grew,
Now neatly detached when disconnect is due,
No ghostly onclose to haunt us at night,
WebSockets clean up, everything right! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'fix(hooks): detach WebSocket handlers on explicit disconnect' directly and clearly describes the main change: detaching WebSocket event handlers in the disconnect function to fix reconnection issues.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

__tests__/hooks/usePriceStream.test.ts (1)

34-36: Consider: Mock doesn't simulate async onclose invocation.

The real WebSocket.close() fires onclose asynchronously after the close handshake. The mock sets closed = true synchronously without invoking onclose. This is fine for the current test since you verify handlers are nulled before close() returns, but if you want stronger proof against the race condition, you could optionally enhance the mock:

close() {
  this.closed = true;
  // Simulate async onclose delivery
  setTimeout(() => this.onclose?.(), 0);
}

This would let you verify that the reconnect timer doesn't fire even when onclose is dispatched post-disconnect.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@__tests__/hooks/usePriceStream.test.ts` around lines 34 - 36, Mock
WebSocket.close() currently sets this.closed synchronously but doesn't simulate
the async onclose callback; update the mock implementation of close() in the
test (the mock class's close method and its onclose handler) to mark closed and
schedule an asynchronous invocation of this.onclose (e.g., via setTimeout 0) so
tests exercise the real-world race where onclose fires after close handshake and
ensure the reconnect timer logic in usePriceStream is still correct.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@__tests__/hooks/usePriceStream.test.ts`:
- Around line 34-36: Mock WebSocket.close() currently sets this.closed
synchronously but doesn't simulate the async onclose callback; update the mock
implementation of close() in the test (the mock class's close method and its
onclose handler) to mark closed and schedule an asynchronous invocation of
this.onclose (e.g., via setTimeout 0) so tests exercise the real-world race
where onclose fires after close handshake and ensure the reconnect timer logic
in usePriceStream is still correct.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28d2f5d5-1f11-40f7-bcfd-fdedd5336af7

📥 Commits

Reviewing files that changed from the base of the PR and between df5b0fd and a11a26f.

📒 Files selected for processing (2)

• __tests__/hooks/usePriceStream.test.ts
• src/hooks/usePriceStream.ts