switch live gameplay websocket traffic to binary#3514
Draft
scamiv wants to merge 7 commits intoopenfrontio:mainfrom
Draft
switch live gameplay websocket traffic to binary#3514scamiv wants to merge 7 commits intoopenfrontio:mainfrom
scamiv wants to merge 7 commits intoopenfrontio:mainfrom
Conversation
Contributor
WalkthroughA binary protocol layer was added for gameplay messages. Transport and server now detect JSON vs binary at runtime, initialize a binary context on game start, and encode/decode gameplay intents/turns in compact binary frames alongside existing JSON control messages. Changes
Sequence DiagramsequenceDiagram
participant ClientApp as Client App
participant ClientTrans as Client Transport
participant WSNet as WebSocket Network
participant ServerGame as Game Server
rect rgba(0,150,255,0.5)
Note over ClientApp,ServerGame: Game start via JSON
ServerGame->>WSNet: send JSON "start" (gameStartInfo)
WSNet->>ClientTrans: deliver "start" (string)
ClientTrans->>ClientTrans: activateBinaryGameplay(gameStartInfo)
ClientTrans->>ClientApp: binary mode active
end
rect rgba(0,200,100,0.5)
Note over ClientApp,ServerGame: Gameplay (binary)
ClientApp->>ClientTrans: ClientIntentMessage
ClientTrans->>ClientTrans: encodeBinaryClientGameplayMessage()
ClientTrans->>WSNet: send Uint8Array (binary)
WSNet->>ServerGame: receive binary intent
ServerGame->>ServerGame: decodeBinaryClientGameplayMessage()
ServerGame->>ServerGame: process intent
end
rect rgba(200,100,0,0.5)
Note over ClientApp,ServerGame: Turn result (binary)
ServerGame->>ServerGame: endTurn()
ServerGame->>ServerGame: encodeBinaryServerGameplayMessage()
ServerGame->>WSNet: send Uint8Array (turn)
WSNet->>ClientTrans: deliver binary turn
ClientTrans->>ClientTrans: decodeBinaryServerGameplayMessage()
ClientTrans->>ClientApp: update game state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/BinaryCodec.ts`:
- Around line 823-831: The helper is incorrectly classifying every "intent" as
binary which causes Transport.serializeMessage to later throw for intents that
have no opcode (e.g., kick_player and update_game_config); update
isBinaryGameplayClientMessage to only return true for intent messages that map
to a binary opcode (use intentTypeToOpcode(message.intent.type) !== undefined or
explicitly exclude "kick_player" and "update_game_config"), so that
isBinaryGameplayClientMessage, BinaryClientGameplayMessage, ClientMessage,
intentTypeToOpcode, Transport.serializeMessage, and callers like
start/GameServer behave correctly.
In `@src/server/GameServer.ts`:
- Around line 326-347: The binary handler currently lets
decodeBinaryClientGameplayMessage throw into the outer catch (which only logs)
so malformed binary frames bypass checkRateLimit and don't get disconnected;
wrap the decodeBinaryClientGameplayMessage call in its own try/catch inside the
client.ws.on("message",...) branch, and on any decode/parsing error call
this.kickClient(client.clientID, KICK_REASON_INVALID_MESSAGE) and return
(optionally log the parse error), ensuring checkRateLimit is only called for
successfully decoded messages and malformed binary frames are kicked
immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e55bffb-89ed-4fc2-a9d5-60b0dea54660
📒 Files selected for processing (6)
src/client/Transport.tssrc/core/BinaryCodec.tssrc/core/BinaryProtocol.tssrc/server/GameServer.tstests/core/BinaryCodec.test.tstests/server/GameServerBinaryProtocol.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
Author
|
Follow-up
Tighten binary gameplay error handling to match the old JSON path. Malformed binary gameplay frames are now treated as invalid protocol messages and kicked with Changes
Follow-up Limit SuggestionBinary gameplay frames are about If we want byte-based rate limits to apply roughly the same effective message count pressure as before reduce the byte ceilings by about
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/server/GameServerBinaryProtocol.test.ts (2)
141-144: Add a comment explaining the magic number.
message[1] === 2checks for the binary turn message type, but this is not obvious. A quick comment would help future readers.📝 Suggested comment
const binaryTurn = clientA.ws.sent.find( (message): message is Uint8Array => - message instanceof Uint8Array && message[1] === 2, + // byte[1] === 2 is the binary "turn" message type + message instanceof Uint8Array && message[1] === 2, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/GameServerBinaryProtocol.test.ts` around lines 141 - 144, Add a short explanatory comment next to the predicate in the find call that checks message[1] === 2 (the arrow function used to locate binaryTurn in clientA.ws.sent) to state that index 1 encodes the message type and that value 2 represents the "binary turn" message type; update the comment near the binaryTurn declaration or inside the predicate to clarify the magic number for future readers.
262-263: Add a comment explaining why this payload is malformed.The byte sequence
[99, 1, 0, 0]triggers a decode failure, but the reason is not clear. A brief comment helps readers understand what makes this invalid (wrong magic byte? invalid message type?).📝 Suggested comment
clientA.ws.sent.length = 0; + // Invalid magic byte (99) causes decode to fail clientA.ws.emit("message", new Uint8Array([99, 1, 0, 0]), true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/GameServerBinaryProtocol.test.ts` around lines 262 - 263, Add a one-line inline comment above the clientA.ws.emit call explaining why the payload [99, 1, 0, 0] is malformed: note which field is invalid (e.g., the first byte (99) is not the expected magic/version/header value or the header indicates an invalid/unsupported message type/length), so the decoder fails; reference the exact payload and the clientA.ws.emit call so readers immediately see why this test triggers a decode error.
🤖 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/server/GameServerBinaryProtocol.test.ts`:
- Around line 141-144: Add a short explanatory comment next to the predicate in
the find call that checks message[1] === 2 (the arrow function used to locate
binaryTurn in clientA.ws.sent) to state that index 1 encodes the message type
and that value 2 represents the "binary turn" message type; update the comment
near the binaryTurn declaration or inside the predicate to clarify the magic
number for future readers.
- Around line 262-263: Add a one-line inline comment above the clientA.ws.emit
call explaining why the payload [99, 1, 0, 0] is malformed: note which field is
invalid (e.g., the first byte (99) is not the expected magic/version/header
value or the header indicates an invalid/unsupported message type/length), so
the decoder fails; reference the exact payload and the clientA.ws.emit call so
readers immediately see why this test triggers a decode error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23a19b12-8b7e-4a54-a91c-a99e795f4ff9
📒 Files selected for processing (2)
src/server/GameServer.tstests/server/GameServerBinaryProtocol.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/GameServer.ts
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
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.
Description:
Switch live gameplay WebSocket traffic from JSON to binary while keeping setup and reconnect messages as JSON.
JSON remains for:
join,rejoinlobby_info,prestart,starterror,winnerBinary is now used for:
intent,hash,pingturn,desyncChanges
src/core/BinaryProtocol.tsandsrc/core/BinaryCodec.tsstartTesting
npx tsc --noEmitnpx vitest run tests/core/BinaryCodec.test.ts tests/server/GameServerBinaryProtocol.test.tsOut of scope/todo:
startPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME