Add opt-in game message delivery confirmations - #375
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca0e7ecd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| options, | ||
| ); | ||
| const { messageId } = envelope.__tpgCoreDelivery; | ||
| const connectionDisposer = player.addConnectionListener((hasConnection) => { |
There was a problem hiding this comment.
Remove settled delivery disposers from the player
Each confirmed targeted send calls player.addConnectionListener, which appends its MobX reaction disposer to SmartPlayerModel.destructors. The delivery cleanup invokes that disposer but never removes it from the array, so repeated confirmed sends or broadcasts permanently retain one stopped reaction function per recipient per message. Long-running games using confirmations regularly will therefore accumulate unbounded per-player memory; use a listener path that does not register in destructors, or remove the disposer from that collection when delivery settles.
Useful? React with 👍 / 👎.
Closes #295
What changed
Promise.withResolverstest shim so the new communicator tests run on the Node 20 CI targetVerification
npx -y node@20.10.0 ./node_modules/vitest/vitest.mjs run --coverage— passed (32 tests) in the exact hosted Node versioncorepack yarn vitest run test/src/message-delivery.test.ts test/src/player-models.test.ts— passed (14 tests)corepack yarn vitest run— passed (32 tests)corepack yarn build— passed, including declaration generationcorepack yarn lint:check— passedgit diff --check— passedcorepack yarn type-check— repository baseline failure: TS6305 forvite.config.d.tsbecause the root config includesvite.config.tswhile referencingtsconfig.node.json; unrelated to this patchDemo
Non-visual library change; screenshots and screencasts are not applicable. Reproduce with
corepack yarn vitest run test/src/message-delivery.test.ts, which exercises controller-to-host, host-to-controller, multi-recipient broadcast, timeout, disconnect, spoofed acknowledgement, and cleanup behavior.