feat: reconnect and resync socket presence - #5
Draft
DJAscendance wants to merge 6 commits into
Draft
Conversation
Add a correlated JOIN protocol and make SocketManager the single owner of the join/reconnect lifecycle (issue CybertownRevival#69, stacked on the presence foundation). - join-protocol: each JOIN carries a joinId; ROOM_STATE/JOIN:error are matched on BOTH room and joinId so a stale/superseded response can never settle a newer attempt. joinRoomOverSocket now returns a cancelable JoinHandle. - reconnect-coordinator (new, dependency-free): owns a logical room intent that survives transport drops. A mid-join disconnect cancels the transport attempt but keeps the intent pending; reconnect starts a fresh correlated attempt with the same presenceId and resolves the original caller. A newer room supersedes the older intent; leaving/clearing cancels it. Exposes a replayable readiness phase so transport connectivity alone never implies room-ready. - socket.ts: thin adapter wiring instance connect/disconnect into the coordinator (v4: reconnect fires on the manager; connect re-fires on the instance). Defers the wire JOIN until connected and drops/volatile-emits room-tagged AV so nothing buffered offline flushes into a later room. - server.js: echo joinId on ROOM_STATE/JOIN:error; room-tag AV/AV:new/AV:del; guard every presence-mutating path on socket ownership; handle every repeated-JOIN transition deterministically (same/other room, changed presenceId, new-socket rebind); preserve transforms on an in-place rebind; reject AV whose room != the socket's current room; never throw on a malformed payload; validate joinId as a bounded string. - presence.ts: isPresenceEventForRoom rejects untagged/mismatched presence events.
Wire the world/chat UI to the reconnect lifecycle so a dropped socket recovers without a page refresh (issue CybertownRevival#69). - WorldBrowserPage: correlate the persistent ROOM_STATE reconcile by joinId so a stale attempt can't re-reconcile; drop presence (AV/AV:new/AV:del) events whose room != the active room; route AV emits through the room-tagged, offline-safe sendAv; on a reconnect 'resynced' transition, re-announce the current viewpoint once so a restarted socket server relearns our stationary position. - Chat: drive input liveness from the room-readiness lifecycle instead of raw transport - input is disabled on disconnect and re-enabled only after a successful resync (a matching authoritative ROOM_STATE), never on mere reconnect. Gate initial enable on roomReady so a Chat mounted mid-resync stays disabled and learns the state on 'resynced'. One disconnected/reconnected message per outage/recovery; lifecycle subscription cleaned up on teardown.
Extend the dependency-free harness (no new test framework) with three suites, run in deterministic order via tests/run-all.js (any failure exits non-zero): - presence.test.ts: correlated joinId echo/rejection, stale room+joinId responses ignored, and isPresenceEventForRoom room-guarding. - reconnect-coordinator.test.ts: drives the real coordinator via a fake socket + injectable isConnected/joinId - interrupted-initial-join recovery resolving the original caller, offline JOIN deferral, latest-room-wins (A interrupted then B offline), clear-while-offline prevents rejoin, invalid-auth no-retry-loop, stale ROOM_STATE can't flip readiness, connected-but-resyncing stays unready, one disconnected/resynced per cycle, flat lifecycle subscriptions. - server-presence.test.ts: boots the real server.js on an ephemeral port with signed JWTs (unconditional subprocess teardown) - joinId echo, malformed payload safety, duplicate-JOIN idempotency, room-move announce, new-socket rebind transform preservation, stale old-socket cannot delete/AV:del/AV a replacement presence, and AV room-tag rejection. All suites green (49 tests); npm run build clean under Node 14.
Local Copilot review adjudication - fixes for the three reproduced/cheap items; two findings were adjudicated as no-change (documented in the PR report). - server.js: an invalid joinId JOIN:error now echoes the client's raw joinId instead of undefined, so the client correlates and fails the attempt fast rather than waiting out the timeout. - socket.ts: leaveRoom guards against a missing coordinator (never-started socket), matching the defensive pattern of the readiness getters. - reconnect-coordinator.ts: handleDisconnect emits 'disconnected' at most once per outage (guard against a redundant disconnect while already disconnected). - tests: cover the raw-joinId echo and the single-disconnect-per-outage guard.
Browser QA of the socket-service-restart scenario surfaced a real defect: after
reconnect, a stationary 3D user's position was never restored on peers (lasting
origin/no-position), violating the A6 recovery goal.
Root cause + fixes (narrow, reconnect/resync only):
- sendInitialViewpoint emitted the transform under ({detail:{pos,rot}}),
but the server (msg.pos/msg.rot) and onPresenceMoved (event.pos/event.rot) read
TOP-LEVEL only, so the viewpoint resend was silently ignored by peers and never
stored server-side. Add presence.avTransformPayload(pos,rot) -> {pos,rot} and
send it top-level, matching the movement watchers.
- The one-shot resync viewpoint went through sendAv's volatile emit, so a busy or
backgrounded reconnecting tab could drop it. Add sendAv(payload,{reliable}) and
send the recovery viewpoint reliably; high-frequency movement stays volatile.
Tests: avTransformPayload shape unit test; a server-presence protocol test proving
a top-level-pos viewpoint reaches peers and is stored for late joiners. 53/53 pass,
build clean. Verified in-browser: stationary peer position now recovers after a
socket restart even when the reconnecting sender is a backgrounded tab.
DJAscendance
added a commit
that referenced
this pull request
Jul 27, 2026
The beta database records 20260717120000_dedupe_role_rows.ts as applied (migrations id 39, batch 5) - it came across in the mysqldump of the local dev database, which had local-testing applied. The file itself never made it onto beta, because beta was built from upstream/master plus PRs CybertownRevival#409-CybertownRevival#412 and fork PR #5, none of which carry it. Knex validates that every completed migration still exists on disk, so beta could not run ANY migration command: The migration directory is corrupt, the following files are missing: 20260717120000_dedupe_role_rows.ts Both migrate:list and migrate:latest fail. Nothing had broken yet only because compose.beta.yml's ct-api command does not migrate at start-up. Restored byte-for-byte from its single authoritative blob (0eb926a - identical at PR #1 head 4616466, at the fork merge a1420a2, and on fork master). Not renamed, not edited, and NOT re-run: beta already records it as applied, and a repaired migrate:latest against a database in beta's migration state reports "Already up to date" with the migrations row count unchanged. Verified against a disposable MySQL 5.7 seeded to beta's migration state: without the file -> migrate:list and migrate:latest both error as above with the file -> migrate:list succeeds, "No Pending Migration files Found"
DJAscendance
added a commit
that referenced
this pull request
Jul 27, 2026
The reconciliation matrix, the finalized PR CybertownRevival#410/CybertownRevival#412/#5 and role invariants, the seed-architecture finding behind the Home Chat Guest decision, and the list of historical implementation deliberately rejected. Also records five follow-up lanes found during this work and deliberately not fixed here, each with a reproducer: 1. db:init cannot build an empty database - 20260309032638_add_voting_tables inserts a vote_list row referencing place_id 1 before the place seeds run 2. NODE_ENV=test has no entry in knexfile, so 12 of 20 suites die at import 3. v-html bound to member-authored content in MessageBoard, Inbox and MayorElection 4. spa `npm run lint` is already failing on beta, and auto-fixes when run; server.js is outside its scope and has 322 pre-existing problems 5. lost-update race across the self-transacting TransactionRepository helpers
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
There was a problem hiding this comment.
🟡 Not ready to approve
Chat lifecycle handling currently never enables chat on the initial "ready" transition (only on "resynced"), and SocketManager.start() can return undefined despite being typed as Promise<void>.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Implements a reconnect + authoritative resync lifecycle for socket-driven presence, adding correlated JOIN attempts (joinId), room-tagged presence events, and server-side ownership/room-guard enforcement so reconnects and offline navigation can’t apply stale presence mutations.
Changes:
- Added
ReconnectCoordinatorstate machine to preserve intended room across disconnects and rejoin/resync on reconnect. - Updated join protocol and server JOIN/AV handling for joinId correlation, room tagging, and socket-ownership guards.
- Added integration-style server presence tests plus coordinator/unit tests, and updated the test runner to execute all suites.
File summaries
| File | Description |
|---|---|
| spa/tests/server-presence.test.ts | Boots real server.js and asserts room-tagged presence protocol + ownership guards over the wire. |
| spa/tests/run-all.js | Deterministic, dependency-free runner for all compiled test suites. |
| spa/tests/reconnect-coordinator.test.ts | Unit tests for reconnect/join intent lifecycle and joinId correlation behavior. |
| spa/tests/presence.test.ts | Extends tests for correlated JOIN, cancellation, room-tag filtering, and AV transform payload shape. |
| spa/src/socket.ts | Introduces coordinator wiring, adds sendAv, and exposes room readiness/lifecycle state. |
| spa/src/reconnect-coordinator.ts | New dependency-free coordinator implementing reconnect/resync join lifecycle. |
| spa/src/presence.ts | Adds room-tag filtering helper and a canonical top-level AV transform payload builder. |
| spa/src/pages/world-browser/WorldBrowserPage.vue | Applies room-tag filtering, joinId-gated ROOM_STATE reconcile, and reliable viewpoint resend after resync. |
| spa/src/join-protocol.ts | Adds joinId correlation, cancellable join handles, and joinId validation helpers. |
| spa/src/components/Chat.vue | Gates chat input on authoritative room readiness lifecycle and switches gesture sending to sendAv. |
| spa/server.js | Adds JOIN payload validation, joinId echoing, room tagging, presence ownership guards, and transform preservation on rebind. |
| spa/package.json | Runs the new tests/run-all.js runner after compiling test TS. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+1165
to
1177
| onSocketLifecycle(event: string): void { | ||
| if (event === "disconnected") { | ||
| if (!this.chatEnabled) return; // already down - don't repeat | ||
| this.chatEnabled = false; | ||
| this.setTimers(false); | ||
| this.systemMessage("Reconnecting to chat server..."); | ||
| } else if (event === "resynced") { | ||
| if (this.chatEnabled) return; // already up - don't repeat | ||
| this.chatEnabled = true; | ||
| this.setTimers(true); | ||
| this.systemMessage("Reconnected to chat server."); | ||
| } | ||
| }, |
Comment on lines
187
to
189
| public start(): Promise<void> { | ||
| if (this.socket) return; | ||
| debugMsg("starting socket..."); |
Copilot review of #5. onSocketLifecycle handled "disconnected" and "resynced" and ignored "ready". But the coordinator emits `recovered ? "resynced" : "ready"`, so a FIRST successful join only ever emits "ready" -- "resynced" means a rejoin that recovered an existing room. mounted() enables chat only when $socket.roomReady is already true, so a Chat that mounted before the room came up had no path to enabled at all: input stayed hidden behind v-show="chatEnabled", and startNewChat, canAdmin, getRole, getXpAmount, joinedChat and the timers never ran. Whether it broke depended purely on whether the join beat the mount, which is why it would present as chat intermittently not working rather than as a clean failure. The comment above the mounted() check asserted "onSocketLifecycle flips it on the resynced transition if we mounted mid-resync", which is precisely the wrong assumption. The mount-time bundle is now activateRoom(), called from mounted() when the room is already ready and from the "ready" event when it was not. Guarded by roomActivated so both paths cannot double-run: startNewChat clears this.messages, and re-running it would wipe the visible history. That is also why "ready" on an already-activated Chat -- a rejoin that could not be resynced -- restores input without calling activateRoom, matching what the "resynced" branch has always been careful to do. "failed" is now handled too. Without it the user keeps the "Reconnecting to chat server..." line from the disconnect forever, which stops being true the moment the coordinator gives up. Beyond the finding, but leaving a message that has become a lie is not a state worth preserving. socket.ts: start() returns Promise.resolve() on the already-started path instead of a bare `return`. The signature promises a Promise, so it was handing back undefined and any caller chaining .then() would throw. The one current caller awaits, and `await undefined` is fine, which is why this survived -- not why it was safe. joinRoom and onLifecycle now guard this.coordinator, which only exists after start(). leaveRoom, roomReady, lifecyclePhase, pendingJoinId, currentRoom and sendAv already did. onLifecycle logs an error rather than silently returning a no-op unsubscribe: a dropped subscription means the subscriber never learns the room came up, which is the same failure this commit is fixing, and it should not be silent. Verified: 53/53 spa tests pass (29 presence, 13 reconnect-coordinator, 11 server-presence). eslint compared against a stashed baseline error-class by error-class: identical, none introduced -- two doublequote violations I added were fixed before this commit. Deliberately not done: no test covers the mount-before-ready ordering. The existing suite drives the coordinator directly and does not mount Chat, so asserting this needs component-level mounting that the harness does not currently do. Worth adding, but it is a test-infrastructure change rather than part of this fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the reconnect and authoritative resynchronization phase for the socket-presence work related to CybertownRevival#69.
This is a stacked follow-up to the presence foundation in CybertownRevival#412.
Dependency
This PR is intentionally based on:
fix/socket-presence-foundation8cb4622b60de84d7c4db166ab3ae61c0c7a565b6It contains only the five reconnect/resync commits above that foundation.
What changed
joinId.Validation
npm test: 53/53 passingnpm run build: passing under Node 14Browser and X_ITE QA
All dedicated reconnect scenarios passed:
QA found one real defect involving the stationary viewpoint resend payload. It was fixed, regression-tested, and reverified in the browser.
Scope
This does not implement:
Relationship
Related to CybertownRevival#69.
Depends on CybertownRevival#412.