Skip to content

fix(socket): preserve queued once listeners#1978

Merged
senamakel merged 1 commit into
tinyhumansai:mainfrom
aqilaziz:fix-1932-socket-once-pending-listeners
May 17, 2026
Merged

fix(socket): preserve queued once listeners#1978
senamakel merged 1 commit into
tinyhumansai:mainfrom
aqilaziz:fix-1932-socket-once-pending-listeners

Conversation

@aqilaziz
Copy link
Copy Markdown
Contributor

@aqilaziz aqilaziz commented May 17, 2026

Summary

  • preserve once() semantics for listeners queued before the socket exists
  • flush queued one-shot listeners with socket.once() instead of socket.on()
  • add a regression test for queued once() listeners

Closes #1932

Tests

  • pnpm --filter openhuman-app exec vitest run --config test/vitest.config.ts src/services/__tests__/socketService.test.ts
  • pnpm --filter openhuman-app compile
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • Fixed socket event listener behavior so one-time listeners registered before a connection are preserved as single-invocation handlers after the socket connects, preventing unexpected repeated invocations and improving real-time event reliability.

Review Change Stack

@aqilaziz aqilaziz requested a review from a team May 17, 2026 01:01
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9d626def-bfd8-4e37-ae55-db64e680ad7d

📥 Commits

Reviewing files that changed from the base of the PR and between 8c95ad0 and ed8019d.

📒 Files selected for processing (2)
  • app/src/services/__tests__/socketService.test.ts
  • app/src/services/socketService.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/src/services/tests/socketService.test.ts
  • app/src/services/socketService.ts

📝 Walkthrough

Walkthrough

SocketService now records queued listeners with an once boolean and, when a socket is created, registers each queued listener using socket.once() for one-time handlers or socket.on() for persistent handlers. on() and once() set the queued listener's once flag accordingly. Tests verify both queued once and on handlers are attached with the correct socket methods after connection.

Changes

Socket listener queue once() behavior fix

Layer / File(s) Summary
Listener queue type and flush logic
app/src/services/socketService.ts
Introduce PendingSocketListener interface with once flag; change pendingListeners to PendingSocketListener[]; flush logic registers queued listeners with socket.once() when once: true, otherwise socket.on().
Listener registration methods
app/src/services/socketService.ts
on() queues listeners with once: false; once() queues listeners with once: true, preserving intended one-time vs persistent behavior when flushed.
Test coverage for queued listener behavior
app/src/services/__tests__/socketService.test.ts
Add tests that queue a once() listener and an on() listener before connection, then verify the created socket receives the queued handlers via the correct methods (once vs on) and not the other.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 I queued a flag, tucked it neat and small,
So one-shot calls now truly fire once and fall.
No echoes left to chase the day,
The socket listens just the one right way. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely summarizes the main change: preserving once() listener semantics when they are queued before socket connection.
Linked Issues check ✅ Passed The code changes directly address all requirements in #1932: add once flag to pending listeners, flush with socket.once() when once is true, and provide regression test.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the bug described in #1932; no unrelated modifications or scope creep detected.

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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

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

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 17, 2026
@aqilaziz aqilaziz force-pushed the fix-1932-socket-once-pending-listeners branch from 8c95ad0 to ed8019d Compare May 17, 2026 01:25
@aqilaziz
Copy link
Copy Markdown
Contributor Author

Follow-up update: I added coverage for the queued regular on() listener path as well. The previous Coverage Gate failure is fixed now, and all current CI checks are green. This PR is ready for maintainer review/merge.

@senamakel
Copy link
Copy Markdown
Member

@coderabbitai can u explain this pr in detail?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

Tip

For best results, initiate chat on the files or code changes.

@senamakel Sure! Here's a detailed breakdown of PR #1978.


🐛 The Bug (Issue #1932)

In app/src/services/socketService.ts, the SocketService class wraps a socket.io-client socket. When you call socketService.on(event, callback) or socketService.once(event, callback) before the socket is connected, those listeners can't be attached immediately — so they get queued in a pendingListeners array.

When the socket later connects and is created, the service flushes that queue by iterating over it and calling socket.on(event, callback) for every entry — regardless of whether the original registration was .on() or .once().

The problem: Listeners registered with .once() (which should fire only the first time the event occurs) were being converted to .on() listeners (which fire every time). This caused:

  • Duplicate state mutations
  • Redundant API calls
  • Potential UI flicker
sequenceDiagram
    participant App
    participant SocketService
    participant PendingQueue
    participant Socket

    App->>SocketService: once("message", handler)
    SocketService->>PendingQueue: queue {event, handler} ← no "once" flag!
    App->>SocketService: connect(...)
    SocketService->>Socket: io(...) creates socket
    SocketService->>PendingQueue: flush queue
    PendingQueue-->>Socket: socket.on("message", handler) ← BUG: should be socket.once()
    Note over Socket: handler now fires on EVERY "message", not just the first
Loading

✅ The Fix

The PR introduces a once boolean flag on each entry in pendingListeners via a new PendingSocketListener type:

// Before
type PendingSocketListener = { event: string; callback: Function }

// After
type PendingSocketListener = { event: string; callback: Function; once: boolean }

When queueing:

  • socketService.on(event, cb) stores { event, callback: cb, once: false }
  • socketService.once(event, cb) stores { event, callback: cb, once: true }

When flushing on connect:

// Before
pendingListeners.forEach(({ event, callback }) => socket.on(event, callback));

// After
pendingListeners.forEach(({ event, callback, once }) => {
  if (once) socket.once(event, callback);
  else       socket.on(event, callback);
});

This preserves the original intent of each registration.


🧪 The Tests

Two new Vitest unit tests were added to socketService.test.ts:

Test Verifies
once() before connect After connecting, the socket's .once() is called with the queued listener — and .on() is not called with it
on() before connect After connecting, the socket's .on() is called with the queued listener — and .once() is not called with it

Both tests:

  1. Call socketService.disconnect() to clear any existing socket
  2. Register a listener before calling connect()
  3. Trigger connect(...) to create the socket
  4. Inspect the mock socket to confirm the correct method was used

📁 Files Changed

File Change
app/src/services/socketService.ts +15 / -5 lines — adds once flag to PendingSocketListener, updates queue and flush logic
app/src/services/__tests__/socketService.test.ts +54 / -0 lines — adds two regression tests

In short: a small but impactful correctness fix ensuring one-shot event handlers stay one-shot, even when registered before the socket connection is established. 🐇

@senamakel senamakel merged commit 992d46d into tinyhumansai:main May 17, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: socketService.ts once() listeners become permanent on() when queued before connection

2 participants