Skip to content

fix(dashboard): show shell elapsed line only past a 10s threshold - #8847

Merged
chenmingwei23 merged 1 commit into
mainfrom
fix/shell-elapsed-threshold
Sep 6, 2026
Merged

fix(dashboard): show shell elapsed line only past a 10s threshold#8847
chenmingwei23 merged 1 commit into
mainfrom
fix/shell-elapsed-threshold

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

The "Running · Ns" status line under a shell tool pill now waits until the command has run for 10 seconds before it appears, and is removed the moment the command completes.

Since #7916 the line was sticky within the turn: it appeared when the shell tool started and, on completion, froze into the elapsed total instead of collapsing. That kept the transcript height monotonic above a bottom-pinned reader, but it left a permanent line under every shell call in the turn — and because the elapsed clock only ticks once a second and freezes at its last tick, a grep or git show that returned in 40 ms showed a meaningless 0s (zh-CN: 0秒). A turn with a dozen quick shell calls became a wall of 0秒 rows.

The threshold is what preserves the point of #7916 rather than reverting it: a short call now adds no line and removes none, so the per-tool step above a pinned reader is gone for the common case, and the one collapse that remains happens only at the end of a command that genuinely ran long — which is the only command a reader was watching the clock on anyway.

Changes

  • website/src/pages/chat/ToolCallLine.tsxSHELL_ACTIVITY_MIN_SECS = 10; showShellActivity is liveShellActivity && elapsedSeconds >= threshold. The sticky latch (shellActivityShownRef) and the frozen-label branch are removed; the label is always Running · Ns because the line now only exists while the command is live. The clock stays anchored to the tool's own start (execution_started_at, then the log ts), so a row that re-mounts mid-command (virtualizer, reload) 25 s in shows the line immediately rather than waiting another ten ticks.
  • website/src/test/ToolCallLine.shellActivityThreshold.test.tsx (replaces ToolCallLine.stickyActivity.test.tsx) — hidden under the threshold; appears on the tick that reaches 10 s with the live total; shows at once when mounted past the threshold; removed on completion; never shown for a tool already done at mount.
  • website/src/test/ToolCallLine.test.tsx — the row-slide test is back to pinning the collapse (eases to zero, then unmounts).
  • website/scripts/capture-shell-elapsed-threshold.mjs — self-checking Playwright harness against the built SPA: every still below is asserted against the DOM ([data-testid="shell-activity"] count and text) before it is written.
  • website/scripts/lib/tool-row-scene.mjs — the mid-turn shell-tool scene and page boot shared by this harness and capture-tool-row-slide.mjs (which is migrated onto it, behaviour unchanged).

Evidence

Captured against the built SPA, real time (500 ms per frame). The GIF is cropped to the transcript column; the corner stamp is seconds since the tool started.

timeline

Moment DOM assertion Still
1 s in no status line 01
6 s in no status line 02
11.5 s in one line, Running · 11s 03
13.5 s in one line, Running · 13s 04
tool_result no status line 05

past threshold

Verification

  • npx tsc -b, npx eslint on the touched files, npx jscpd . (0 clones), npm run i18n:check, check-unit-literals.mjs — all green locally.
  • vitest is left to CI on this head (the dev host runs the gateway; concurrent local suites have stalled it).
  • Harness run: 5/5 DOM assertions passed on the built SPA (temp-screenshots/shell-elapsed-threshold/assertions.json).

Pattern harvest

Rule candidate: review-prompt
Pattern: per-item status chrome (elapsed row, spinner text) rendered unconditionally on every tool call instead of gated on the signal being worth showing — a UI review prompt can ask "does this row earn its place for the common short case?"

  • A "sticky within the turn" fix (perf(chat): reachable archived history and stable phone scrolling #7916) that keeps a per-item row for layout stability trades one problem (jump above a pinned reader) for another (a permanent, often-0s row under every call). A threshold that suppresses the row for the common short case keeps the stability win without the noise — prefer conditioning the extra chrome on the signal being worth it over showing it unconditionally.
  • Anchor an elapsed clock to the tool's own recorded start (execution_started_at, then log ts), never to mount time, so a row re-mounting mid-command (virtualizer, reload) reflects real elapsed rather than restarting the count.
  • Extract a shared harness scene (scripts/lib/tool-row-scene.mjs) when a second capture script needs the same page/boot setup, so jscpd stays at 0 clones and the two harnesses cannot drift.

@CrysisDeu
CrysisDeu requested a review from a team September 6, 2026 00:22
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 6, 2026 00:22
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of e34e89e06416ad1d9c05feb74d272bc367b15d17 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Threshold-gating the elapsed row fixes the 0s-wall at its cause while keeping #7916's stability for the common short-call case; the one reintroduced collapse is explicit, argued, and pinned by test.

[DESIGN-REVIEWED] e34e89e

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e34e89e06416ad1d9c05feb74d272bc367b15d17 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e34e89e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e34e89e06416ad1d9c05feb74d272bc367b15d17: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of e34e89e06416ad1d9c05feb74d272bc367b15d17 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks complete. The one pre-existing consumer of the shell-activity testid (capture-tool-status-row-align.mjs) seeds its shell tool 75 seconds in the past (capture/tool-status-row-align.tsx:106), so it stays past the new threshold and does not break. The extraction of tool-row-scene.mjs is mandated by the jscpd pretest gate documented in website/AGENTS.md, the capture-script + committed-screenshot evidence pattern has 60+ siblings in the repo, and the fix itself is a net deletion of state (the shellActivityShownRef latch and frozen-label branch) plus one gate on an elapsed clock that already existed and was already anchored to execution_started_at.

First-Principles-Verdict: PASS

Removes the reported wall of frozen "0s" rows by deleting the sticky latch and gating on real elapsed time; every rider is CI-gate-derived or repo convention.

What this change ships

Intent: stop every quick shell call from leaving a permanent, meaningless "0s" status row in the transcript. FIX.

  1. Shell status line waits until the command has run 10s — justified (the fix; cause-level: deletes the unconditional render)
  2. Line disappears at completion instead of freezing at its total — justified (deletes the frozen-row state perf(chat): reachable archived history and stable phone scrolling #7916 introduced)
  3. Frozen elapsed-only label variant removed; label is always "Running · Ns" — justified consequence of 2
  4. Sticky latch (shellActivityShownRef) deleted — justified (this is the subtraction, shipped)
  5. New self-checking capture harness for the threshold — justified, conventional (60+ sibling capture-*.mjs scripts)
  6. Shared scene lib scripts/lib/tool-row-scene.mjs, slide harness migrated onto it — justified (jscpd pretest gate in website/AGENTS.md; 2 consumers, counted)
  7. Committed stills/GIF/assertions.json under temp-screenshots/ — justified, conventional (existing directory, many siblings)
  8. Sticky-behavior test replaced by threshold test — justified (pins the new invariant)

The clock anchoring the PR leans on (execution_started_at, then log ts) pre-exists at ToolCallLine.tsx:364-368, so the re-mount claim adds no new surface. The one external shell-activity consumer (capture-tool-status-row-align.mjs) seeds its tool 75s in the past and survives the threshold — 0 broken consumers, grepped data-testid="shell-activity" across website/.

[FIRST-PRINCIPLES-REVIEWED] e34e89e

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed e34e89e06416ad1d9c05feb74d272bc367b15d17 — this comment is updated in place on each push.

Review details

The only shipped-code change is the one-line render edit in ToolCallLine.tsx:990; everything else is test harness, tests, and screenshot artifacts.

I verified CANDIDATE 1's mechanism against the code: StatusRow (ToolCallLine.tsx:135) wraps the child in AnimatePresence, so when showShellActivity flips false on completion the child stays mounted through the exit height/opacity ease (SLIDE_DURATION 0.22s / opacity 0.15s), and the child now unconditionally renders Running · {elapsedLabel}. The behavior the candidate describes is real: for ~220ms a completed command's collapsing, fading row reads "Running · Ns" where the prior conditional showed only the elapsed label.

But this fails the bar for a reported defect. The text appears solely on an element that is simultaneously fading to opacity: 0 and easing to zero height — the residual of a state that is vanishing, not new false information presented on a stable row. In the prior sticky design the non-live branch rendered a frozen, persistent row (removing "Running" mattered there); in the new design that branch is reachable only during the brief exit transition, which is why the author removed the conditional and updated the row-slide test to expect /Running ·/ during collapse. During a snap/hot-stream transcript the duration is 0 and it never appears at all. This is a deliberate, tested transition artifact on a disappearing element, not a defect at 80+ confidence. Dropped.

No grounded Step 2 finding surfaced.

No findings.

[OPUS-REVIEWED] e34e89e

Verdict parsed from the review's SHA-scoped output markers for commit e34e89e06416ad1d9c05feb74d272bc367b15d17.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e34e89e06416ad1d9c05feb74d272bc367b15d17: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of e34e89e06416ad1d9c05feb74d272bc367b15d17 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The change is purely the shell "Running · Ns" status line: it now waits 10 s to appear and vanishes on completion (previously it froze into the elapsed total). The blind read shows the reader correctly understood the timer, the counting, and the done state — but explicitly flagged that the elapsed total disappears on completion and they couldn't tell whether that was deliberate or lost information. All introduced states are covered by the five stills plus the committed GIF, and the removal animates (height ease pinned by test), so there are no evidence gaps and no block predicates.

UX-Verdict: CONCERNS

The threshold works and reads cleanly, but completing a long command now deletes the one number the user was watching: its duration.

Watch

  • Duration loss at completion: the diff removes the frozen-total branch (liveShellActivity ? … : elapsedLabel → live-only), so a command that ran 10+ s drops its elapsed total the instant it finishes, and the duration is shown nowhere else in the row. The blind reader hit exactly this on shot-05: "the app doesn't tell me how long it took — the timer just vanishes" and "I can't tell if the disappearing is deliberate or if I've just lost the information about how long it took." Frequency: every long command (the only ones users watch the clock on) × impact: minor data loss plus a beat of doubt × persistence: every completion. Smallest fix: for past-threshold commands only, freeze the line into the bare total on completion instead of collapsing it — that also removes the one remaining pinned-reader step this PR still accepts, while short calls stay line-free.

[UX-REVIEWED] e34e89e

@CrysisDeu
CrysisDeu force-pushed the fix/shell-elapsed-threshold branch from 9751565 to 007fbd8 Compare September 6, 2026 06:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
…, drop it on completion

Since #7916 the 'Running · Ns' line under a shell tool pill was sticky within
the turn: it appeared on start and froze into an elapsed total on completion,
leaving a permanent (often '0s') line under every shell call. Gate the line on
elapsedSeconds >= SHELL_ACTIVITY_MIN_SECS (10) and only while live, so short
calls add and remove no line while long ones collapse once at the end.

Replaces the sticky latch/frozen-label branch; tests renamed to
ToolCallLine.shellActivityThreshold.test.tsx; shared scene extracted to
website/scripts/lib/tool-row-scene.mjs.
@CrysisDeu
CrysisDeu force-pushed the fix/shell-elapsed-threshold branch from 007fbd8 to e34e89e Compare September 6, 2026 07:11
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition: fixed (in e34e89e06416ad1d9c05feb74d272bc367b15d17)

  • Drop the theme option from openToolRowScene (website/scripts/lib/tool-row-scene.mjs:87) and inline 'light'

0 of 2 callers pass it.

Agreed — both call sites (capture-tool-row-slide.mjs:52, capture-shell-elapsed-threshold.mjs:45) pass only { base, slotKey, scene }. The theme parameter is removed; 'light' is inlined at the boot-route stub and the mc-theme init-script write, and the init script now takes the bare slotKey instead of a [slotKey, theme] tuple. eslint + jscpd (0 clones) green on the touched scripts.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition: fixed (in e34e89e06416ad1d9c05feb74d272bc367b15d17)

  • Drop export on TOOL_ROW_PROJECT and TOOL_ROW_LAST_PURPOSE (tool-row-scene.mjs:18,24)

0 external consumers; keep them module-local.

Agreed — both constants are referenced only inside tool-row-scene.mjs (fixture project path, and the purpose text openToolRowScene waits for). Both export keywords are removed; the two consumers import only TOOL_ROW_VIEW, makeToolRowScene, openToolRowScene, which are unchanged.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition: rebutted — deliberate product decision; the code stays as shipped.

  • Duration loss at completion: a past-threshold command drops its elapsed total the instant it finishes

Smallest fix: for past-threshold commands only, freeze the line into the bare total on completion instead of collapsing it.

The observation is accurate — the total does vanish on completion — and that is the design this PR states in its title ("drop it on completion") and Summary, not an oversight. Reasoning:

  1. The line is a liveness affordance, not a duration record. Its job is to answer "is this still running, and for how long already?" while the user is waiting. The moment tool_result lands, the answer the user was waiting for is the result itself; a frozen Running · 14s14s row then becomes stale chrome under a finished call.
  2. Nothing users relied on is lost. Before perf(chat): reachable archived history and stable phone scrolling #7916 the line disappeared on completion too. perf(chat): reachable archived history and stable phone scrolling #7916's freeze-into-total was introduced as a layout-stability device (keep transcript height monotonic above a pinned reader), not as a duration feature — see the PR Summary. This PR keeps the stability win where it matters (no per-call step for the common short case) and accepts one animated collapse at the end of a long command.
  3. Freezing for past-threshold commands reintroduces the class this PR removes, just with a higher bar: a permanent row under every long shell call for the rest of the session. The threshold gates when the line earns its place; a completed command no longer does.
  4. If elapsed duration of finished calls is wanted as a feature, it is a different design: it should apply to every tool kind (not only shell), live in the result header/details, and be decided on its own — it is out of scope for a fix whose purpose is removing 0s noise.

The collapse is animated (height eases to zero, pinned by ToolCallLine.test.tsx) so the removal reads as intentional rather than as lost content. Recorded here so a maintainer who prefers the freeze-into-total variant can request it as a follow-up.

@chenmingwei23
chenmingwei23 merged commit 383f0e5 into main Sep 6, 2026
65 checks passed
@chenmingwei23
chenmingwei23 deleted the fix/shell-elapsed-threshold branch September 6, 2026 07:55
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026
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.

2 participants