Add ping-adjusted controller hoster time - #374
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0e86ae424
ℹ️ 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".
|
|
||
| return ( | ||
| this.pingData.hosterTimePingAdjusted + | ||
| (Date.now() - this.pingData.lastPoll) |
There was a problem hiding this comment.
Use a monotonic clock for elapsed time
If the controller's wall clock is corrected manually or by NTP after a pong is received, this Date.now() delta incorporates that clock adjustment and makes getHosterTime() jump forward or backward even though the hoster's clock did not. Store a monotonic receipt time (for example, from performance.now()) and use that to advance the host timestamp so the estimate remains stable between ping samples.
Useful? React with 👍 / 👎.
Fixes #308
What changed
PingDatagetHosterTime()to advance the latest adjusted sample using elapsed controller timenulluntil the controller has received its first hoster pongPromise.withResolversshimRoot cause and impact
PONG_HOSTERalready carriedhosterTime, butControllerCommunicatordiscarded it while constructingPingData. Controllers therefore had no supported way to estimate the hoster's current wall-clock time. This preserves the protocol value and exposes both the raw and latency-adjusted forms.Verification
getHosterTime()and the host timestamp fields did not existnpx -y node@20.10.0 node_modules/vitest/vitest.mjs run test/src/controller-time.test.ts— 2 tests passed under the hosted workflow's Node versionnode_modules/.bin/vitest run— 6 files, 24 tests passedcorepack yarn coverage— 6 files, 24 tests passednode_modules/.bin/eslint src/controller/ControllerCommunicator.ts test/src/controller-time.test.ts— passed (existing React-detection warning only)corepack yarn build— passed and generated the expected public declarationgit diff --check— passedThe first hosted run exposed that the existing communicator constructor uses
Promise.withResolvers, unavailable on the workflow's Node 20.10 runtime. The test now installs and removes a local compatibility shim; the production implementation is unchanged by that follow-up.corepack yarn type-checkremains blocked by the repository's pre-existingTS6305error:vite.config.d.tshas not been built fromvite.config.ts. This same error reproduces on the unchanged baseline.Demo
This is a non-visual library API change. Run
node_modules/.bin/vitest run test/src/controller-time.test.ts; the second test demonstrates a 40 ms round trip producing a 20 ms host-clock adjustment, followed by live clock advancement as controller time moves forward.