Skip to content

fix(ui): humanize provider retry delay in the banner copy - #3402

Closed
me2seeks wants to merge 1 commit into
apache:mainfrom
me2seeks:fix/desktop-retry-delay-format
Closed

fix(ui): humanize provider retry delay in the banner copy#3402
me2seeks wants to merge 1 commit into
apache:mainfrom
me2seeks:fix/desktop-retry-delay-format

Conversation

@me2seeks

@me2seeks me2seeks commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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 as 4h 28m 3s (#3393).

providerRetryScheduled now 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

  • New conversation-copy.test.ts pins both locales across second/minute/hour/day scales and fails without the change.
  • ui suite on this head: 201 pass, biome lint/format clean. One pre-existing failure, composer-plus-menu › a loading catalog holds the row still, reproduces on the base commit (9a661a183) without this change — unrelated.
  • Not run: hosted checks (no CI on fork branches).

AI use

  • Generative tooling made a substantive contribution

Tool(s) and scope: Maka (AI agent) prepared the change end to end — implementation, tests, and PR text. The commit carries a Generated-by: Maka trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[] = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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: ' ',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed at efd19dbf. No findings.

The problem is small but real: a provider's Retry-After can be hours or days, and rendering that as a raw second count gives the reader a long number with no sense of scale.

The shape is right in the way that matters most for a copy change — it stays a copy change. conversation-copy.ts gains one pure formatter that decomposes into d/h/m/s, the locales supply only unit strings, and chat-turn.tsx remains the single call site, still doing Math.max(1, Math.ceil(delayMs / 1000)) as before. No countdown, no timer, no retry policy migrated into the UI, and no second formatter competing with an existing one.

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, NaN and Infinity reach the formatter only if called directly — the sole consumer clamps first and the runtime's retry metadata is a finite positive delay — so we did not raise those as findings.

Verification: the full packages/ui suite passes (202 tests / 29 suites), the focused conversation-copy test passes, and Biome plus git diff --check are clean on the changed files.

On CI: the run on this head is action_required at 0s — a fork-PR approval gate, not a failure, but it means nothing has actually executed. Worth a maintainer kicking it off; that is the only thing outstanding from our side.

@Astro-Han

Copy link
Copy Markdown
Contributor

Heads up on CI: the workflow run for this PR was sitting in action_required (fork PRs need a maintainer to approve the run), so it had never actually executed. I approved it — it has now run on efd19dbfa0fa862da71777b6b1d527ea48ead849 and came back red.

Failing step: Check ASF source headers

1 file(s) are missing the ASF license header. Run `npm run write:asf-headers`:
  packages/ui/src/__tests__/conversation-copy.test.ts

Fix:

npm run write:asf-headers

Worth flagging: this step runs early in the job, so everything after it — build, typecheck, and the whole test suite — was reported as skipped rather than passing. The red header check is not evidence that the rest is fine; those steps never ran.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@me2seeks
me2seeks force-pushed the fix/desktop-retry-delay-format branch from 0e8c328 to 3528853 Compare August 23, 2026 15:01

@ARE404 ARE404 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 messages line in conversation-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 test gate 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: '通过显式技能发送的历史消息暂不支持编辑并重发',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@Astro-Han

Astro-Han commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for digging this out and for the concrete reproduction — the 13565 秒后重试(2/10) example makes the problem obvious.

I am closing this one because #3611 covers the same change and goes further: it adds the same formatRetryDelay(seconds, locale) to packages/ui/src/conversation-copy.ts, wires it into both the zh and en providerRetryScheduled, and additionally fixes the CLI side in pi-transcript.ts. #3611 is already approved, and I have rebased it onto the latest main; it is waiting on CI.

One thing you have that #3611 does not: the unit tests in packages/ui/src/__tests__/conversation-copy.test.ts. #3611 ships no tests. If you would like to send those cases as a small standalone PR, I will prioritise reviewing it.

Note that your #3400 (the countdown, for #3393) is a separate concern — I am not touching it, and it stays in normal review.

简体中文

感谢你把这个问题挖出来并给了完整的复现(13565 秒后重试(2/10) 那个例子很有说服力)。

这条我准备关掉,因为 #3611 已经覆盖了同样的改动并且走得更远:同样在 packages/ui/src/conversation-copy.ts 里加 formatRetryDelay(seconds, locale)、同样接进中英两处 providerRetryScheduled,另外还一并修了 CLI 侧的 pi-transcript.ts#3611 目前已经 approve,我也帮它 rebase 到了最新 main,正在等 CI。

有一点你这边是有而 #3611 没有的:packages/ui/src/__tests__/conversation-copy.test.ts 里的单测。#3611 没带测试,如果你愿意把这几个用例单独提一条小 PR 补上去,我这边优先跟。

另外你的 #3400(倒计时,对应 #3393)和这条是两回事,那条我不动,继续正常 review。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop retry banner shows raw seconds for long waits: '13565 秒后重试'

4 participants