Skip to content

fix(ui): give the turn elapsed clock one start authority - #5337

Merged
Astro-Han merged 10 commits into
apache:mainfrom
Astro-Han:fix/ui-unify-turn-elapsed-authority
Sep 15, 2026
Merged

Astro-Han merged 10 commits into
apache:mainfrom
Astro-Han:fix/ui-unify-turn-elapsed-authority

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

One Turn now has one start fact, and states its elapsed once.

The elapsed clock and the settled duration both read turn.startedAt — the Turn's own recorded start — instead of the running clock inventing a second start from the optimistic message. The optimistic send timestamp was a real second authority: it fed the pre-Turn cue (pendingRunningStartedAt), and the desktop publisher stamped ts: Date.now() on every publish, so each transient update moved it. A turn that ran for minutes could show a running clock that kept restarting from the latest edit — reading like "the time of the last tool" rather than the turn.

The settled elapsed also appeared twice on the same turn: the process disclosure header and the footer meta. It now lives only in the disclosure header, which already owns the running cue; the footer meta keeps model · cost.

What changes:

  • The pre-Turn cue (chat view, before the Turn reaches the transcript) keeps showing a waiting state but with no clock: elapsed time appears only once turn.startedAt reaches the client.
  • A transient message's send time is written once; an update no longer moves it (mergeTransientMessageProjection), so the optimistic row's own timestamp stops drifting too.
  • The footer meta drops the duplicated elapsed; the process disclosure header is its single home.
  • The process disclosure drops cursor: pointer, and <summary> joins the native-cursor convention, which keeps the hand for links only.
  • A failed or interrupted tool no longer renames the disclosure header or forces it open; the disclosure keeps its own label and the row states itself.
  • The running cue has no spinner. It is the disclosure's own summary row: one line, the activity text, and the elapsed once the Turn's start is known.
  • That row is rendered for every Turn (empty when the Turn produced no process content), so the cue and the settled row are one thing and the transcript does not shift when a Turn settles.

Behavior changes:

  • Between send and the Turn reaching the transcript there is no elapsed clock. The timer starts when the Turn does.
  • Every Turn now has one status row: the running cue while it runs, then 执行过程 · 用时 X. The row is the elapsed's only home, so a Turn with no process content states its elapsed there rather than in the footer meta.

Review focus

The cue is the disclosure's own summary row, so a Turn whose content has no process item still needs a row to hold it: chat-turn.tsx injects an empty ProcessingBlock for the last segment, and that row stays after settlement. A row that appeared while live and disappeared at settlement was the first attempt; it shifted the bottom-pinned transcript, which SubmittedPromptSettlesWithoutReversing guards. Dropping the row for a plain streamed answer (no tools, no reasoning) instead deletes the cue for the most common turn shape — turn-running-spinner and chat-view-empty-compaction fail on exactly that. Before the Turn reaches the transcript the chat view renders the same empty row, so there is one cue at a time and no footer cue.

Verification

  • packages/ui: npm --workspace @maka/ui run build, then node --test --test-concurrency=4 "dist/**/*.test.js" — 471 pass. Rewritten chat-view-empty-compaction case: the pending cue renders with no .maka-turn-elapsed before the Turn exists, then reads 2s from turn.startedAt. New turn-running-spinner case: the elapsed is stated once, in the disclosure header, and the footer meta carries only the model.
  • apps/desktop Storybook: tsc -p tsconfig.storybook.json --noEmit clean. New fixture PromptSentBeforeTurnLands renders the pending-send window and its play asserts the cue has no .maka-turn-elapsed; captured from main and this branch for the comparison above.
  • apps/desktop: build:main, tsc -p tsconfig.renderer.json --noEmit, tsc -p tsconfig.main.json --noEmit, then the affected main suites (transient-message-projection, chat-view-optimistic-render, streaming-handoff, message-queue-ui-state, workhub-send-visibility, quote-companion-retry) — 125 pass. New case: an update carrying a later ts must not move the transient send time.
  • npm run format and npm run lint clean.

Before / after

The pending-send window, on its own fixture (Product/Shell Official AppShellPromptSentBeforeTurnLands), cropped to the transcript. Left is main, right is this PR. On main the cue is a spinner in a message frame, clocking from the client's send stamp; here it is the same disclosure row the process uses, with no clock until the Turn's recorded start arrives.

The pending-send cue: a spinner and a client-clock reading on main, the disclosure row in this PR

Two notes on the fixture set: this window had no fixture, which is why this one exists; and dark is pixel-identical to light under prefers-color-scheme here, so there is one comparison rather than two. The stable row also changes every Turn that previously had no process disclosure — such a Turn now shows one status row (empty until it runs), which the existing fixtures will pick up.

The dropped attention label, on the same fixture set's failure story (left main, right this PR):

A failed tool: "Needs attention" and a forced-open disclosure on main; the ordinary header in this PR

Root cause

The Turn start lives in the persisted transcript (turn.startedAt), reachable only after the message is admitted and re-materialized. The view covered that gap with the client clock, then kept the placeholder as a peer fact instead of a stand-in — and the publisher kept re-stamping it.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Maka authored the change, the comments, and the test updates from the issue/PR discussion.

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
  • No

The running elapsed and the settled duration must read one Turn start fact.
The optimistic send timestamp was a second authority: it fed the pre-Turn cue
and every transient update re-stamped it to Date.now(), so the running clock
restarted mid-turn instead of measuring from the turn's own start.

- Drop that client-side start; the pre-Turn cue now waits without a clock until
  turn.startedAt reaches the client, which is also what the settled duration
  measures from.
- Write a transient message's send time once, so an update no longer moves it.
- Drop cursor:pointer from the process disclosure, and cover <summary> in the
  native-cursor convention (the hand stays for links).

Generated-by: Maka
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 15, 2026
A settled turn stated its elapsed twice: the process disclosure header and the
footer meta. The header already owns the running cue, so keep the elapsed there
and leave the footer meta to model · cost.

Generated-by: Maka
The window between sending and the Turn reaching the transcript is where the
running cue read a client-side start. Give that state its own fixture and pin
that the cue shows no clock until the Turn's own start arrives.

Generated-by: Maka
A failed or interrupted tool turned the process disclosure header into
"需要处理" and forced it open, special-casing one row the lifecycle already
states. Keep the disclosure's own mechanism: the label follows running or the
recorded duration, and the row shows its own error state.

Generated-by: Maka
The cue is now the disclosure's own summary row: one line, the activity text,
and the elapsed once the Turn's start is known. No spinner, and no separate
pre-Turn shape — the pending send renders the same row.

The row is rendered for every Turn (empty when nothing ran yet), so the cue
never moves between owners and settlement does not shift the transcript.

Generated-by: Maka
The empty status row carries no entries, so the stream callbacks it was
passed can never fire. Trim the comments added by this branch to what a
future reader still needs.

Generated-by: Maka
TurnRunningStatus had no consumer outside its own module; only a unit test
imported it. Render those assertions through TurnView instead.

Generated-by: Maka
The process header no longer asks whether a tool row owns the spinner, so
toolTrowHasVisibleSpinner has no caller left. Knip flagged the stale export.

Generated-by: Maka
The pending cue is no longer a message footer, so the handoff test asserts the
process row instead of the footer frame.

Generated-by: Maka
Every Turn now owns a process disclosure, so the geometry scene has many more
of them. Re-resolve the first collapsed one each time instead of walking an
index-based list that a click invalidates.

Generated-by: Maka

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

Independent agent review. Reviewed at 7190e7e3b6fed09d1442751ae91993adad749ae1. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.

No P0–P2 findings.

"One start authority" is a structural fact, not a description

The claim is checkable, so I checked it rather than reading the summary: pendingRunningStartedAt has zero occurrences repository-wide — the second authority is removed, not merely unused — and every elapsed-clock site in chat-turn.tsx now reads turn.startedAt. That is what makes the stated bug impossible rather than unlikely: a clock with one input cannot be restarted by a second one.

The data-level half is the two lines in mergeTransientMessageProjection. current is the existing projection and update the incoming one, and ts: current.ts sits immediately after ...update, so it overrides the incoming timestamp; nothing further in the merge touches ts. The send time is written once, as described. Ablating that single line turns keeps a transient message send time when a later update carries a new timestamp red, so it is pinned by a test named for exactly what it protects.

toolTrowHasVisibleSpinner is deleted outright, and it has no remaining references anywhere in the tree — dead code following the spinner removal rather than a behaviour change hiding in a deletion.

The removed tests are replaced, one contract at a time

Three assertions disappear, and each pinned a behaviour this PR deliberately changes — so the question is whether the new behaviour is pinned in turn. It is, and the replacements name their contracts:

removed replaced by
keeps activity in the answer footer before the session or Turn arrives keeps activity in the process row …, asserting .maka-turn-footer is now null
disclosure force-opened and renamed on failure (open === true, /Needs attention/) summary.textContent === 'Execution process' plus explicit open/closed assertions
the pending Turn clock ticks from send time … the pending Turn waits without a clock until the Turn start time reaches the client, asserting .maka-turn-elapsed is null

Two additions carry properties that had no test before: assert.deepEqual(mergeTransientMessageProjection(first, later), { ...later, ts: 2 }), and querySelectorAll('.maka-processing-summary').length === 1 for "stated once". This is intentional semantic replacement, not coverage loss.

I also want to note the "Review focus" section as written: it records the first attempt, names the two suites that failed on it, and explains why the current shape is what it is. A reviewer can check that reasoning instead of rediscovering it.

One suite failure that is not yours

npm --workspace @maka/ui run test:dist gives me 471 tests, 470 pass, 1 failinherits the confirmed target and exposes live sequence progress, in tool-activity-presentation.test.ts, a file this PR does not touch.

It is not a regression here, and I traced where it comes from rather than leaving it ambiguous:

  • It fails at this PR's merge base 72cd8b1f5 as well.
  • It passes on current origin/main (bb5b2b32a).
  • The commit that fixes it is 99098aafb#5315, which merged after this branch was cut.

So this branch carries a state that main has since repaired. Merging or rebasing onto current main should take the suite to 471/471, which is presumably why the description reports 471 passing. Nothing to change in this PR's own code.

transient-message-projection and streaming-handoff in the desktop main build: 31/31, and build:main compiles with zero TypeScript errors.

Scope of what I checked

I read the CSS and story changes rather than rendering them, so the cursor: pointer removal and the <summary> native-cursor convention are verified as source, not as appearance. I did not run Electron or Storybook, so the transcript-shift property the description discusses — that the cue and settled row are one row so the bottom-pinned transcript does not move at settlement — is guarded by SubmittedPromptSettlesWithoutReversing in the author's runs, not in mine.

This PR is currently a draft.

Code review, CI status and merge readiness are separate. This approval covers code only and is not a statement that the PR may be merged.

@Astro-Han
Astro-Han marked this pull request as ready for review September 15, 2026 09:10
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 15, 2026
@Astro-Han
Astro-Han merged commit 3f297e9 into apache:main Sep 15, 2026
2 of 3 checks passed
@Astro-Han
Astro-Han deleted the fix/ui-unify-turn-elapsed-authority branch September 15, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants