fix(ui,cli): count down provider retry wait from the event timestamp - #3400
fix(ui,cli): count down provider retry wait from the event timestamp#3400me2seeks wants to merge 2 commits into
Conversation
…pache#3393) A long provider-mandated Retry-After (e.g. a subscription quota window reset of ~4.5h from kimi-k3 / OpenCode Go) made the retry indicator pin the original delay for the whole sleep: both the TUI activity strip and the desktop banner rendered the static delayMs snapshot while the 1s ticker re-rendered everything around them. Render the remaining wait as delayMs - (now - retry.ts) instead: - TUI strip reuses the shared duration formatter, so hours-long waits read '4h 28m 3s' rather than a raw five-digit second count - desktop banner follows the elapsed-clock determinism contract: first paint and frozen fixtures show the provider's delay, live mounts tick the countdown once per second The wait itself is unchanged; whether to cap extreme Retry-After values in the runtime retry loop is a separate policy question.
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head 5eb038d3ddb75cb0d8eea8a614ac2c6da8a407a1.
The current main UI and TUI both render the original delayMs, so the bug still exists. The implementation correctly derives remaining time from the event timestamp, clamps at zero, preserves the frozen-fixture contract, and cleans up the one-second interval. The focused UI build and new UI tests passed locally (2/2). I found no P0-P2 product defect.
Non-blocking test-quality gap: provider-retry-countdown.test.tsx proves the timestamp calculation by rerendering with an older event, but it never advances the mounted interval. The test would still pass if the interval were deleted. Please consider advancing fake timers on one mounted banner so the live ticking behavior is pinned directly.
Merge readiness: not ready yet. This exact head has no hosted checks and still requires independent human review.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for fixing the frozen retry presentation without moving retry authority out of Runtime. I reviewed exact head 5eb038d3ddb75cb0d8eea8a614ac2c6da8a407a1. Reusing the TUI heartbeat and keeping the Desktop timer local to the banner are both appropriately scoped. I left two P2 presentation-contract issues below.
AI-assisted review disclosure: Codex performed the exact-head analysis, and an independent reviewer agent adversarially checked clock semantics, remote Runtime Host paths, timer lifecycle, accessibility, and regression coverage. I verified the source paths, live state, and final severity.
| ? 0 | ||
| : nowMs === undefined | ||
| ? retry.delayMs | ||
| : Math.max(0, retry.delayMs - (nowMs - retry.ts)); |
There was a problem hiding this comment.
Thanks for deriving the remaining delay from the event rather than storing another countdown state. [P2] retry.ts is produced by the Runtime Host clock, while nowMs comes from the Client clock. On the supported remote-Host path, ordinary clock skew can therefore make a multi-hour retry display 0s immediately or show substantially more time than the Host will actually wait. Could the Host projection provide an authoritative remaining value, or otherwise establish a single clock domain before the Client starts ticking it down? The same calculation in the TUI has the same boundary.
| useEffect(() => { | ||
| if (retry.phase !== 'scheduled' || !isTimeDrivenMotionEnabled(rootRef.current)) return; | ||
| setNowMs(Date.now()); | ||
| const tick = window.setInterval(() => setNowMs(Date.now()), ELAPSED_TICK_MS); |
There was a problem hiding this comment.
Thanks for keeping the countdown visually live. [P2] Because this banner is a role="status" live region, changing its title every second can cause screen readers to announce every tick—potentially for hours during a quota wait. Could the changing visual countdown be hidden from the live region while exposing a stable accessible status, following the pattern already used by the running-turn indicator? A focused accessibility regression would help keep the timer from reintroducing this.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks @me2seeks — reviewed at exact head 5eb038d3ddb75cb0d8eea8a614ac2c6da8a407a1. The countdown itself is correct: the elapsed portion is subtracted properly, the clamps prevent negative values, and both test files pin Date via mock timers so there is no wall-clock flakiness on slow machines.
One P2 and one P3 below, both about scope rather than about the calculation being wrong.
The P2 is that the fix does not reach users who have reduced motion enabled — for them the banner still shows the full original delay, frozen, which is the #3393 symptom this PR sets out to remove. The determinism contract in the comment is right for frozen fixtures and SSR; the issue is that a real accessibility preference is currently on the same side of that gate.
The P3 is that UI and CLI each carry their own copy of delayMs - (now - ts), and the two have already diverged on the floor: the UI clamps to 1 s, the CLI to 0 s, and each side's new test pins its own value.
The clock-domain question and the role="status" announcement cadence are already covered in earlier reviews on this PR; not repeating them here.
This review was AI-assisted. It is not a substitute for independent human review by a committer.
| // progress instead of a frozen number. | ||
| const [nowMs, setNowMs] = useState<number | undefined>(undefined); | ||
| useEffect(() => { | ||
| if (retry.phase !== 'scheduled' || !isTimeDrivenMotionEnabled(rootRef.current)) return; |
There was a problem hiding this comment.
[P2] This gate covers both the interval and the initial setNowMs(Date.now()), so under reduced motion nowMs stays undefined for the lifetime of the banner.
remainingMs then falls through to retry.delayMs — the full original delay, never decremented. For a provider-mandated wait that can span hours, a reduced-motion user sees exactly the frozen number that #3393 describes, which is the symptom this PR is fixing everywhere else.
isTimeDrivenMotionEnabled returns false whenever the element or an ancestor matches [data-maka-reduced-motion="true"], and the Banner carrying rootRef is itself that root, so this is reachable through the ordinary preference rather than only in fixtures. It is the only writer of nowMs in this component, and there is no fallback in the remainingMs expression.
The determinism contract in the comment above is sound for frozen fixtures and SSR — the gap is that a genuine accessibility preference currently shares a gate with them. Taking the initial measurement outside the gate and leaving only the setInterval behind it would give reduced-motion users a correct static value that reflects the time already waited, while still not animating:
useEffect(() => {
if (retry.phase !== 'scheduled') return;
setNowMs(Date.now());
if (!isTimeDrivenMotionEnabled(rootRef.current)) return;
const tick = window.setInterval(() => setNowMs(Date.now()), ELAPSED_TICK_MS);
return () => window.clearInterval(tick);
}, [retry.phase, retry.ts]);
| Math.max(1, Math.ceil(props.retry.delayMs / 1_000)), | ||
| props.retry.attempt, | ||
| props.retry.maxAttempts, | ||
| Math.max(1, Math.ceil(remainingMs / 1_000)), |
There was a problem hiding this comment.
[P3] The same delayMs - (now - ts) calculation now exists here and in pi-transcript.ts, and the two copies have already drifted apart at the boundary: this side clamps with Math.max(1, …) so an expired countdown reads 1s, while formatRetryCountdown on the CLI side clamps with Math.max(0, …) and reads 0s.
Each side's new test pins its own value, so the divergence is now locked in by tests rather than flagged by them — the same wait renders differently depending on where the user is looking.
Worth extracting the remaining-time computation (and one agreed floor) into a shared helper, e.g. in @maka/core, so the next change to the formula cannot land on only one surface.
…ry countdown (apache#3393) Review follow-up on apache#3400: - Clock domains (Astro-Han): counting down from the event's ts mixed the Runtime Host clock with the client clock, so remote-Host clock skew could zero out or inflate a multi-hour wait. The scheduled event now carries remainingMs — a duration, therefore skew-free; Runtime sets it at scheduling and the Host projection recomputes it from the snapshot's stored schedule time on mid-wait re-projection (reconnect no longer restarts the countdown). Both clients stamp a local receipt time when the event lands and tick against that single domain. - Accessibility (Astro-Han): the banner is a role=status live region, so a title changing every second would be announced every second, for hours. The ticking text is now aria-hidden and the region exposes a stable label (reason + waiting attempt), following the running-turn indicator's pattern. - Test quality (jackwener): the mounted banner is now driven with mocked Date + setInterval, proving the one-second interval itself ticks (10s -> 9s -> 7s), plus an a11y regression pinning the stable label against the moving text. Suites: runtime 3002, runtime-host 1041, cli 339, ui 188 — all green; biome lint/format clean.
Fixes #3393.
Problem
When a provider returns a long
Retry-After(e.g. a subscription quota window reset — kimi-k3 / OpenCode Go returns ~4.5h when its 5h quota is exhausted), the retry indicator pins the original delay for the entire sleep:Retrying in 16083s (2/10)never counts down, indistinguishable from a hung process. The TUI's 1s heartbeat re-renders the strip and the siblingWorking… <elapsed>counter ticks, but the retry line renders a staticdelayMssnapshot.Fix
Render the remaining wait as
delayMs - (now - retry.ts)in both clients:pi-transcript.ts): reuses the shared duration formatter, so hours-long waits read4h 28m 3sinstead of a raw five-digit second count; floors at0s.chat-turn.tsx): follows the same determinism contract as the turn's elapsed clock — first paint and frozen fixtures show the provider's original delay; live mounts tick the countdown once per second.Scope
Presentation layer only: the wait itself is unchanged. Honoring a multi-hour
Retry-Afterfrom a subscription quota window is intentional (the turn resumes when quota resets); whether the runtime should additionally cap extreme values is a separate policy question noted in the issue.Tests
AI use
Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push.
Generated-by: Makatrailer will be added to the branch commit on its next update (local worktree had uncommitted changes, history rewrite skipped this round).