fix(server): SP3 — bind member to connection_id on WS join; rewrite broadcast routing#341
Merged
Merged
Conversation
…up broadcast routing Implements Step SP3 of the SyncPlay implementation plan. Changes: - SyncPlayManager.createGroup() and joinGroup() now accept an optional connection_id parameter and store it in the member record (replacing the prior hard-coded null). - A private connectionToMember reverse-map is maintained so that ConnectionPool::get($connectionId)->send(<flat frame>) is used for all group broadcasts instead of the incorrect sendToSession() wrapper format. - SyncPlayManager.onConnectionClose($connectionId) is called from WebSocketServer::onClose() to automatically vacate the member from their group and trigger host re-election if needed. - WS handlers (handleGroupCreate, handleGroupJoin) now pass \$connection->getId() as the connection_id when creating / joining. - 5 new unit tests covering connection_id binding, onClose leave, and broadcast delivery/exclusion with ≥2 connections. Gate: phpstan level 9 clean, PSR-12 clean, SyncPlay test suite 129/129
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium |
| Documentation | 5 minor |
| CodeStyle | 3 minor |
| Complexity | 3 medium |
🟢 Metrics 24 complexity · 2 duplication
Metric Results Complexity 24 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
1 participant
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 Step SP3 of the SyncPlay implementation plan.
What changed
src/Session/SyncPlay/SyncPlayManager.phpcreateGroup()andjoinGroup()now accept an optional?string $connectionId = null5th parameter and store it in the member record (previously alwaysnull).connectionToMember: array<string,string>reverse-map maintained on join/leave/create.broadcastToGroup()rewritten to useConnectionPool::getInstance()->get($connectionId)->send(<flat frame>)delivering flat canonical SyncPlay frames{type, ...data, timestamp}instead of the{type, data, timestamp}wrapper thatsendToSession()produced.leaveGroup()cleans up theconnectionToMemberentry when a member departs.onConnectionClose(string $connectionId): voidmethod for WS close handling.src/Server/WebSocket/WebSocketServer.phponClose()now calls$this->syncPlayManager->onConnectionClose($wsConnection->getId())before removing the connection from the pool, ensuring disconnects auto-vacate SyncPlay groups and trigger host re-election.tests/Unit/Session/SyncPlay/SyncPlayManagerTest.php5 new tests:
testCreateGroupStoresConnectionIdOnMemberRecordtestJoinGroupStoresConnectionIdOnMemberRecordtestOnConnectionCloseRemovesMemberFromGrouptestBroadcastToGroupDeliversToAllConnectedMembers(≥2 connections, verifiessend()called on each with correct flat frame)testBroadcastToGroupExcludesSpecifiedMemberIds(verifies exclusion logic)Gate status
./vendor/bin/phpstan analyze src/ --level=9 --no-progress./vendor/bin/phpcs --standard=PSR12 -n src/./vendor/bin/phpunit tests/Unit/Session/SyncPlay/./vendor/bin/phpunit(full suite)ArrTransportInterfacenot found inWorkermanArrTransportTest.php(unrelated to SP3)Notes
origin/master(SP1 and SP2 already merged ✅).Connection::sendFlat()and SyncPlay protocol spec).connectionToMembermap enables O(1) member lookup on connection close without scanning all groups.