fix(ui): humanize provider retry delay in the banner copy - #3402
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head ef7097b50a91c7dc5284f00180db0ffe15a95fa4.
I confirmed the current main copy still renders provider waits as raw seconds, and the patch fixes that at the presentation boundary without changing retry policy. The d/h/m/s formatter is bounded, locale-specific, and the focused UI build plus new locale test passed locally (1/1). I found no P0-P2 correctness issue.
Merge readiness: not ready yet. This head has no hosted check result and still requires an independent human review; both gates should be present on this exact head before merge.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for this — the five-digit second count really does read as a hang, and fixing it per locale rather than with a generic duration formatter is the right call for copy that sits inside a sentence.
I verified the fix itself and it behaves as described. Two [P2]s and two [P3]s below, none of them about the desktop change being wrong.
The one I would like you to look at first is the premise in the PR description:
The TUI strip already humanizes the same wait as
4h 28m 3s(#3393).
At current main it does not. renderMakaPiActivityStrip (packages/cli/src/pi-transcript.ts:1407) still renders the retry countdown as Retrying in ${Math.max(1, Math.ceil(retry.delayMs / 1_000))}s, so the same quota window that produced 13565 秒后重试 in the desktop banner produces Retrying in 13565s in the TUI strip. What #3393 humanized is the elapsed counter on the line below (Working… <elapsed>, via formatElapsedDuration), which is a different string. That is line 1403-1407 and it is the only provider-retry render in the CLI, so the surface is not covered anywhere else.
That matters twice over, because formatElapsedDuration (pi-transcript.ts:1415) is the algorithm this PR just re-implemented — same [['d', 86_400], ['h', 3_600], ['m', 60]] table, same loop, same if (remaining > 0 || parts.length === 0) tail, same join(' '). The new formatProviderRetryDelay is that function with the unit strings lifted into a parameter.
I want to be fair about the cost of de-duplicating: formatElapsedDuration lives in packages/cli and the new one in packages/ui, so reuse is not a local import — the algorithm would have to be hoisted somewhere both can reach. The zh unit strings genuinely differ, so what is shared is the ladder, not the copy. That is real work, which is why this is a [P2] and not a merge blocker.
Nothing here blocks: no [P0]/[P1]. If you would rather land the desktop fix now and take the TUI strip and the shared ladder as a follow-up, that seems entirely reasonable to me — I would just ask that the PR description be corrected either way, since the sentence about the TUI currently describes a state the repository is not in, and the next person to touch this will trust it.
Review assisted by AI (Claude Opus 5). The findings above were verified against the files at this head; the reviewer is accountable for them.
| */ | ||
| function formatProviderRetryDelay(seconds: number, units: ProviderRetryDelayUnits): string { | ||
| let remaining = Math.max(0, Math.floor(seconds)); | ||
| const parts: string[] = []; |
There was a problem hiding this comment.
[P2] This duplicates formatElapsedDuration in packages/cli/src/pi-transcript.ts:1415 exactly — same unit table, same loop, same zero/empty tail, same separator. The only real difference is that the unit strings became a parameter so zh can supply its own.
Simpler equivalent solution, spelled out: hoist this ladder to a package both ui and cli can import, taking the unit strings and separator as the parameter you already designed here, then have formatElapsedDuration and formatProviderRetryDelay both call it. That also makes the [P2] on the TUI strip a one-line change instead of a third copy.
Acknowledging the cost honestly: this is a cross-package move, not a local import, so it is more work than it looks. Not a blocker — but with the ladder now written twice, the third copy is the one that will drift.
| minute: '分', | ||
| hour: '小时', | ||
| day: '天', | ||
| separator: ' ', |
There was a problem hiding this comment.
[P3] separator: ' ' is shared by both locales, so zh renders 4小时 28分 3秒后重试. Chinese typography does not normally space between a number-unit group and the next one — 4小时28分3秒后重试 is the conventional form. Since separator is already per-locale in this interface, zh can just carry '' and en keep ' '; the test expectations would move with it.
Cosmetic, and a native reader should overrule me if they disagree.
| assert.equal(zh(1, 2, 10), '1秒后重试(2/10)'); | ||
| assert.equal(en(1, 2, 10), 'Retrying in 1s (2/10)'); | ||
| assert.equal(zh(45, 2, 10), '45秒后重试(2/10)'); | ||
| assert.equal(en(45, 2, 10), 'Retrying in 45s (2/10)'); |
There was a problem hiding this comment.
[P3] The description says short delays “keep the familiar seconds-only form (45秒后重试)”, but the previous zh copy was ${seconds} 秒后重试 — with a space before 秒. This assertion pins '45秒后重试(2/10)', so the short-delay zh string does change, just subtly.
I think dropping the space is the better copy and I am not asking you to restore it. Only flagging that the description says this path is unchanged when it is not, so a reader diffing screenshots is not left confused.
Separately, and to the test's credit: pinning both locales across second/minute/hour/day scales is exactly the right shape here — it fails if the ladder regresses, rather than restating the implementation.
d18d0c2 to
efd19db
Compare
|
Reviewed at The problem is small but real: a provider's The shape is right in the way that matters most for a copy change — it stays a copy change. Checked and clear: 0, 1, 45 and 75 seconds; the 59/60 and 3599/3600 carry boundaries; mixed hour-and-day combinations; zh/en agreement; and that sub-second values still round up in the existing consumer rather than displaying "0 seconds". Negative, Verification: the full On CI: the run on this head is |
|
Heads up on CI: the workflow run for this PR was sitting in Failing step: Fix: Worth flagging: this step runs early in the job, so everything after it — build, typecheck, and the whole test suite — was reported as The review conclusion from our side is unchanged and still stands on this head. Once the header is added and CI is green we'll approve. |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 0e8c3288c7922da3fda3f24834358ce8f178f324 — required test is completed / success bound to that SHA. Both earlier [P2]s are still open. No new P0–P2.
A note on why this is a fresh review rather than a follow-up: the two earlier reviews are bound to ef7097b5, and the only change since is an ASF header added to the test file — the production formatter is unchanged. Worth flagging for anyone auditing this thread: the inline comments API re-anchors commit_id to the current head, so those comments look current; only original_commit_id shows they were written against the older revision.
[P2] The PR description states the TUI already humanizes this wait. It does not.
The body says:
The TUI strip humanizes the same wait as
4h 28m 3s(#3393).
The current TUI does not. pi-transcript.ts:1446 still renders the scheduled-retry branch as:
`Retrying in ${Math.max(1, Math.ceil(retry.delayMs / 1_000))}s (${retry.attempt}/${retry.maxAttempts})`So for one delayMs of 16,083,000, Desktop now shows 4h 28m 3s while the TUI shows Retrying in 16083s — the exact illegible five-digit count this PR exists to remove, still present on the other surface.
formatElapsedDuration (pi-transcript.ts:1454) does humanize, but it is called only for Working… ${formatElapsedDuration(metadata.turnElapsedMs)} — turn elapsed time, not provider retry. So the sentence is not merely imprecise; it describes a state the repository is not in.
This is a P2 rather than a nit because a PR description outlives the review: the next person deciding whether the retry-countdown work is finished will read that line and conclude the TUI is covered. Either narrow the description to Desktop, or wire the CLI branch up too. If only the description changes, the TUI's raw-seconds output should be tracked as remaining work rather than left described as done.
[P2] The d/h/m/s ladder is now implemented twice.
formatProviderRetryDelay (conversation-copy.ts:101) and formatElapsedDuration (pi-transcript.ts:1454) are the same algorithm: the same [day 86_400, hour 3_600, minute 60] table, the same floor-and-modulo loop, the same if (remaining > 0 || parts.length === 0) tail, the same join. The differences are the unit strings, the separator, and whether the input arrives as seconds or milliseconds — parameters, not semantics.
This is not a "looks similar" observation. It has a live consequence today (the two surfaces disagree on the same value) and a latent one: when someone later adjusts the ladder — adds a seconds threshold, changes rounding at a boundary — they will change one copy, and the surfaces will drift silently.
The de-duplication is real work and that is why this is P2, not a blocker: the two live in packages/cli and packages/ui, so sharing means hoisting the ladder into a module both can depend on. Note that packages/core/src/provider-retry-countdown.ts shares the remaining-time calculation, not the display ladder, so it does not already cover this. Keeping the copy in each caller and sharing only the ladder would close both findings at once — the CLI branch would then humanize by construction, and the description would become true rather than needing to be narrowed.
Two earlier [P3]s (spacing around Chinese units, spacing change on short second values) remain observable in the current diff. They are unchanged and are not re-raised here.
AI-assisted review. The description claim, the CLI render path, and the duplication were each verified against the source at this exact head. Under CONTRIBUTING.md §Review this does not count as the required independent human review.
A subscription quota window hands the runtime an hour-scale Retry-After,
and the desktop banner rendered it as a raw five-digit second count
('13565 秒后重试(2/10)') that reads as a frozen hang. Format the delay
in d/h/m/s units per locale ('4小时 28分 3秒后重试', 'Retrying in 4h 28m
3s') so the countdown stays legible and keeps ticking every second,
unlike the goal chip's minute-granularity ladder.
Generated-by: Maka
0e8c328 to
3528853
Compare
ARE404
left a comment
There was a problem hiding this comment.
NO-GO (pending) at head 3528853836adc6ee0b0081789e58ce8834e2a5c3, on the same feature line as #3400.
Gate (this PR standalone): test ✅ green on this exact head (no package.json/lockfile change → audit correctly not triggered).
Findings: 1 × [P2] (inline on conversation-copy.ts:101), 1 × [P3] (inline on :520). No P0/P1.
Why not an unconditional GO:
- [P2] above — it should reuse the shared formatter from #3400 rather than ship a parallel one, and it's currently not stacked on #3400 (sibling branch off main). Both PRs edit the same
messagesline inconversation-copy.ts, so if they merge independently they will conflict as a pair; this PR should be rebased on #3400. - #3400 itself is currently NO-GO (its
testgate fails on the protocol epoch guard — see review on #3400). This PR's user-facing intent (humanized countdown) only makes sense once the #3400 countdown lands.
The math and behavior are correct (I independently verified the formatter output and the PR's tests). No APPROVE from me (read scope); flagging to orchestrator — this should be sequenced after #3400's P1 fix and re-based to reuse the shared formatter before a MEMBER approves. Not merging.
| * frozen hang, so the banner counts down in d/h/m/s units that keep moving | ||
| * every second (unlike the goal chip's minute-granularity ladder). | ||
| */ | ||
| function formatProviderRetryDelay(seconds: number, units: ProviderRetryDelayUnits): string { |
There was a problem hiding this comment.
[P2] This adds a second, local copy of the d/h/m/s humanizer, but the same feature line (#3400) deliberately extracted/shared that computation into @maka/core precisely because "the two client copies drifted apart at the expiry floor" (see the #3393 commit message and provider-retry-countdown.ts). I verified this formatProviderRetryDelay is byte-for-byte identical to formatElapsedDuration (CLI) across a 0–200k second sweep, so today they agree — but keeping two parallel formatters reintroduces the exact divergent-copy risk this work set out to remove. Please reuse the shared formatter (ideally export one canonical duration formatter from @maka/core and have both the CLI strip and this banner call it) instead of a second local implementation.
There was a problem hiding this comment.
Cross-reference so this isn't read in isolation (see #3400 thread): the correct sequence for this feature line is #3400 lands first, then #3402 rebases on top of it. This PR is currently a sibling branch off main; both PRs edit the same messages providerRetryScheduled line in conversation-copy.ts, so merging this independently of #3400 will conflict as a pair. When rebased onto #3400, this banner should reuse the shared d/h/m/s formatter (the one #3400 establishes in @maka/core) instead of the local formatProviderRetryDelay copy — per the [P2] on line 101. Note #3400 is currently NO-GO: its test job fails on the protocol epoch guard (current origin/main epoch is 44; the author must rebase to main and bump from 44). Treat this PR's green standalone test as necessary, not sufficient, for the line to merge.
| }, | ||
| messages: { | ||
| you: '你', assistant: 'Maka', processing: '正在处理…', continuing: '继续中…', workingPhrases: ['正在琢磨…', '正在推敲…', '正在盘算…', '正在钻研…', '正在忙活…', '正在梳理…', '正在打磨…', '正在鼓捣…', '正在酝酿…', '正在攻坚…', '正在权衡…', '正在拾掇…'], providerRetryScheduled: (seconds, attempt, maxAttempts) => `${seconds} 秒后重试(${attempt}/${maxAttempts})`, providerRetryStarted: (attempt, maxAttempts) => `正在重试(${attempt}/${maxAttempts})`, providerRetryReason: { network: '网络中断', provider_capacity: '模型服务暂时满载', provider_unavailable: '模型服务暂时不可用', rate_limit: '触发模型速率限制', timeout: '请求超时', unknown: '模型请求失败' }, safeResumePending: '正在验证…', safeResume: '安全恢复', thinking: '深度思考', truncated: '已截断', copied: '已复制', copying: '复制中', copyFailed: '复制失败', copy: '复制', editMessage: '编辑并重发', editMessageDisabledRunning: '当前回答仍在进行中,结束后再编辑', editMessageDisabledAttachments: '包含附件的历史消息暂不支持编辑并重发', editMessageDisabledQuotes: '包含引用的历史消息暂不支持编辑并重发', editMessageDisabledTransformedText: '通过显式技能发送的历史消息暂不支持编辑并重发', | ||
| you: '你', assistant: 'Maka', processing: '正在处理…', continuing: '继续中…', workingPhrases: ['正在琢磨…', '正在推敲…', '正在盘算…', '正在钻研…', '正在忙活…', '正在梳理…', '正在打磨…', '正在鼓捣…', '正在酝酿…', '正在攻坚…', '正在权衡…', '正在拾掇…'], providerRetryScheduled: (seconds, attempt, maxAttempts) => `${formatProviderRetryDelay(seconds, PROVIDER_RETRY_DELAY_UNITS_ZH)}后重试(${attempt}/${maxAttempts})`, providerRetryStarted: (attempt, maxAttempts) => `正在重试(${attempt}/${maxAttempts})`, providerRetryReason: { network: '网络中断', provider_capacity: '模型服务暂时满载', provider_unavailable: '模型服务暂时不可用', rate_limit: '触发模型速率限制', timeout: '请求超时', unknown: '模型请求失败' }, safeResumePending: '正在验证…', safeResume: '安全恢复', thinking: '深度思考', truncated: '已截断', copied: '已复制', copying: '复制中', copyFailed: '复制失败', copy: '复制', editMessage: '编辑并重发', editMessageDisabledRunning: '当前回答仍在进行中,结束后再编辑', editMessageDisabledAttachments: '包含附件的历史消息暂不支持编辑并重发', editMessageDisabledQuotes: '包含引用的历史消息暂不支持编辑并重发', editMessageDisabledTransformedText: '通过显式技能发送的历史消息暂不支持编辑并重发', |
There was a problem hiding this comment.
[P3] Minor copy note: this bundles a display change beyond pure humanization — the Chinese banner drops the space (45 秒后重试 → 45秒后重试) and, for range-y totals, produces tokens like 4小时 28分 3秒后重试 where the embedded spaces around CJK units read a little loose right before 后重试 (4小时28分3秒后重试 would read tighter). Also worth a sentence in the PR description that the zh string format changed alongside the humanization. Not blocking.
|
Thanks for digging this out and for the concrete reproduction — the I am closing this one because #3611 covers the same change and goes further: it adds the same One thing you have that #3611 does not: the unit tests in Note that your #3400 (the countdown, for #3393) is a separate concern — I am not touching it, and it stays in normal review. 简体中文感谢你把这个问题挖出来并给了完整的复现( 这条我准备关掉,因为 #3611 已经覆盖了同样的改动并且走得更远:同样在 有一点你这边是有而 #3611 没有的: |
Summary
With a long provider
Retry-After(subscription quota window reset, e.g. kimi-k3 / OpenCode Go 5h quota), the desktop retry banner rendered a raw five-digit second count —13565 秒后重试(2/10)— illegible and visually indistinguishable from a hang. The TUI strip humanizes the same wait as4h 28m 3s(#3393).providerRetrySchedulednow formats in d/h/m/s units per locale:4小时 28分 3秒后重试(2/10)/Retrying in 4h 28m 3s (2/10). One-second granularity is kept so the banner visibly ticks every second — the goal chip's minute-granularity ladder would reintroduce the frozen look between minute boundaries. Short delays keep the familiar seconds-only form.Independent of #3400 (countdown ticking); either merge order works.
Fixes #3401
Verification
conversation-copy.test.tspins both locales across second/minute/hour/day scales and fails without the change.composer-plus-menu › a loading catalog holds the row still, reproduces on the base commit (9a661a183) without this change — unrelated.AI use
Tool(s) and scope: Maka (AI agent) prepared the change end to end — implementation, tests, and PR text. The commit carries a
Generated-by: Makatrailer.Checklist
Does this PR entail a change in behavior?