Skip to content

test(desktop): make the transcript scroll regression line real - #4796

Merged
Astro-Han merged 3 commits into
mainfrom
test/desktop-transcript-scroll-cost-ratchets
Sep 4, 2026
Merged

test(desktop): make the transcript scroll regression line real#4796
Astro-Han merged 3 commits into
mainfrom
test/desktop-transcript-scroll-cost-ratchets

Conversation

@Astro-Han

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

Copy link
Copy Markdown
Contributor

Summary

The Desktop transcript had no working performance regression line. native-transcript-perf.spec.ts looked like one, but every test in it was gated on MAKA_TRANSCRIPT_PERF=1 / MAKA_TRANSCRIPT_STRESS=1, and those strings appeared nowhere else in the repository — not in ci.yml, not in desktop-nightly.yml, not in any script. The suite had been permanently skipped since it was written.

What it would have measured was the harness, not the product. base.css gave every element under [data-maka-e2e-fixture="true"] a transition-duration: 0.01ms !important. The initial transition-property is all, so that is a live transition on every animatable property of every element — and base.css also sets * { scrollbar-color: var(--border-strong) transparent } from a relative oklch(...) colour that Chromium re-serialises on each style recalculation, so something actually changes on every recalc. A near-zero duration does not mean "no transition": Chromium still creates the transition objects and still fires transitionrun / transitionstart / transitionend.

Measured on this branch's build, one wheel sweep over the ten mounted Turns of the prompt-rail fixture: 2,481 transitionrun events with 0.01ms, 0 with transition: none. The timing numbers PRs #4206 and #4259 reported off this harness were mostly that storm, and the containment deltas they showed were how much of it containment happened to hide.

This PR does three things:

  1. transition: none !important replaces transition-duration: 0.01ms !important in both the [data-maka-e2e-fixture] block and the prefers-reduced-motion: reduce block (they are deliberately kept identical). It matches the animation: none !important already on the line above, and it serves the declaration's stated purpose — a fixture render is a settled state, not an entry frame — strictly better: with 0.01ms, sampling right after a gesture found 3 and then 10 transitions still unfinished; with none it is 0 in every sample.

  2. native-transcript-perf.spec.ts is deleted and replaced by transcript-scroll-cost.spec.ts, three count assertions that run in ordinary CI with no env var. MAKA_TRANSCRIPT_PERF and MAKA_TRANSCRIPT_STRESS are gone from the repository, and PROMPT_RAIL_PROMPT_COUNT is a constant again.

  3. Gestures are relative input — a real wheel through CDP Input.dispatchMouseEvent, which is also what the product's own history paging listens for (use-chat-scroll.ts). The deleted suite drove scrolling by writing absolute scrollTop values per frame, which erases whatever scroll-anchoring correction the browser applied since the previous frame, so the probe fought the scroller and produced displacement artifacts that read as product bugs.

What is asserted, and why only these three

Each candidate was measured before it was kept.

Counter Kept Measured
transitionrun / animationstart during one gesture, and unfinished animations after it yes 2,481 → 0 across the CSS change; 0 in 6/6 runs on the fixed code
Turns the browser is actually skipping (content-visibility) > 0 yes 1–8 depending on position; 0 with containment disabled
mounted Turns ≤ DESKTOP_TRANSCRIPT_ACTIVE_RANGE_MAX_TURNS across a full 120-Turn traversal yes 10, at every page and after returning to latest
style recalculation count during one gesture no 315–332 with the storm, 156–225 without. A threshold that separates them sits ~25% above the clean arm's own spread. Too tight for a shared runner, and too loose to catch anything smaller than the storm.
forced layout count during one gesture no 440–460 in both arms. No signal at all.
exactly one scroll write per ResizeObserver delivery while pinned no Already covered, at unit level and more precisely, by the nine tests in packages/ui/src/__tests__/transcript-scroll-authority.test.ts. An e2e restatement would be duplicate coverage.

No millisecond number is asserted anywhere.

Timing as a nightly trend: not done, and why

The brief suggested moving timing to desktop-nightly.yml as a trend. That workflow runs no tests — it packages, signs, notarizes, verifies and publishes the nightly release artifacts, triggered by a successful npm publication. Adding an e2e perf run to it would put a perf harness in the path of release publication and would still need somewhere to keep a rolling median that nothing in this repository currently has. The honest outcome is that the timing telemetry is deleted rather than relocated: it produced a console.log nobody read, in a job that never ran. If a trend line is wanted later it should be its own scheduled workflow with its own storage, and that is a separate PR.

One finding worth recording for #4206 / #4259

element.checkVisibility({ contentVisibilityAuto: true }) is not usable for measuring containment from a test. The flag it reads is updated during rendering, so a synchronous call after a scroll reports every Turn visible. On this fixture it returned 0 skipped Turns in every position where contentvisibilityautostatechange reported between 1 and 8 — including for a control element deliberately parked 2,800px off-screen. Any measurement of containment built on checkVisibility reads as "containment never engages". The new spec uses the event.

Containment does engage, but it has little to work with: the Desktop's active range is 10 Turns ≈ 2,900px, about 4.5 viewports, so 1–4 Turns are skipped at a 700px viewport height. That is worth knowing before further sub-turn containment work is justified on transcript-scroll cost.

Refs #4206, #4259, #4793

Verification

  • npm exec -w @maka/desktop -- playwright test --config e2e/playwright.config.ts transcript-scroll-cost — 3 passed, 6 consecutive runs, ~13s each. No flake.
  • Full Desktop e2e suite from apps/desktop: 88 passed, 1 skipped, 0 failed (6.2m).
  • Mutation-tested, both directions:
    • restoring transition-duration: 0.01ms !importanta scroll through the fixture transcript starts no transitions fails with Expected: 0, Received: 2481; the other two still pass.
    • content-visibility: visible on .maka-transcript-turnthe browser skips the Turns the reader has scrolled past fails with Expected: > 0, Received: 0; the other two still pass.
  • npm run format, npm run lint, npm --workspace @maka/desktop run typecheck: clean.
  • CI Desktop e2e (Linux, xvfb, 4 workers): green as of f293969257.
  • Not run: Windows/macOS packaging, storybook visual smoke (untouched by this change).

What CI caught that six local runs did not

Both are recorded here because they are the point of the PR: local wall-clock never reproduced either, and the fix is a wait, not a timing threshold.

The first CI run failed on skill-draft-lifecycle.spec.ts › cancelling a failed revision restores the complete pre-edit draft, in the slash-command menu, with the option detaching mid-click. It passes on main, three times locally, and on the rerun. Pre-existing flake, unrelated to motion.

The rerun then failed on this PR's own paging back through the whole history keeps the mounted range bounded, and the trace showed two things:

  • returnToLatest probed the affordance with await isVisible(), which answers about the current instant and does not wait. On a loaded runner that falls through before the button renders — into a fallback that clicked the last prompt-rail tick, inherited from the deleted suite and therefore never once executed by any run. Waiting for the button, and dropping the fallback, is the fix.
  • That was not what failed here, though: the trace shows the button visible, the click landing, and the following toHaveCount timing out behind it. Coming back from the far end is a range reload — the Host resolves a new window around the tail and the renderer mounts it — and the suite's 10s expect timeout is sized for UI already on screen. That one step now gets 30s.

Neither is a millisecond assertion; both are waits on a state the product either reaches or does not.

One more local flake, chased down and cleared: session-workbar.spec.ts › browser address field tracks the workbar column width failed once on the fake backend's echo (create browser session), which has nothing to do with motion. It passes on main, in isolation, with the whole file, and in the clean full-suite run above.

AI use

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

Tool(s) and scope: Claude Code (Opus). Investigated the defect, ran the A/B measurements under a real Electron build, and authored the CSS change, the replacement spec and this description. Every number quoted above was measured, not estimated; each retained assertion was mutation-tested and each rejected one was rejected on its measured instability. Commits carry Generated-by: Claude Code.

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 only user-facing surface touched is the prefers-reduced-motion: reduce block: a reader with that OS setting now gets no transition at all instead of a 0.01ms one. That is the same thing to look at and strictly less work to produce; it is listed here because it is a real rule change on a real user path, not because anything moves on screen.

`transition-duration: 0.01ms !important` is not "no transition". The
initial `transition-property` is `all`, so the declaration gave every
element under `[data-maka-e2e-fixture]` a live transition on every
animatable property, and `* { scrollbar-color: ... }` resolves from a
relative `oklch()` that Chromium re-serialises on each style
recalculation — so something changed on every recalc and the transitions
fired. Measured on a real Electron build, one wheel sweep over the ten
mounted Turns of the prompt-rail fixture produced 2,481 `transitionrun`
events; with `transition: none` it produces 0.

`none` also serves the reason the declaration exists — a fixture render
is a settled state, not an entry frame — strictly better: sampling right
after a gesture found 3 and then 10 transitions still unfinished under
0.01ms, and 0 under `none`.

Both variants change together, as the comment on the first one requires.

Generated-by: Claude Code
… ratchets

`MAKA_TRANSCRIPT_PERF` and `MAKA_TRANSCRIPT_STRESS` appeared nowhere in
the repository except their own reads, so every test in
native-transcript-perf.spec.ts had been skipped since it was written —
no workflow set either variable. What the suite would have measured was
the fixture's transition storm rather than the product, and its gestures
wrote absolute `scrollTop` values per frame, erasing the browser's
scroll-anchoring correction between frames.

Replaced with three assertions that run in ordinary CI, drive the
scroller with real relative wheel input through CDP, and assert counts
rather than milliseconds: no transition starts during a gesture, the
browser skips Turns the reader scrolled past, and the mounted range
stays within DESKTOP_TRANSCRIPT_ACTIVE_RANGE_MAX_TURNS across a full
120-Turn traversal.

Style-recalculation and layout counts were measured and rejected: recalc
runs 315-332 with the storm and 156-225 without, leaving no threshold
both stable on a shared runner and tight enough to catch anything
smaller; layout count is 440-460 in both arms. The single-scroll-writer
property is already covered by transcript-scroll-authority's unit tests.

`checkVisibility({ contentVisibilityAuto: true })` is not usable here —
its flag updates during rendering, so it reported 0 skipped Turns in
every position where `contentvisibilityautostatechange` reported 1 to 8.

Generated-by: Claude Code
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 4, 2026
Two defects the first CI run surfaced, neither reproducible locally.

`isVisible()` answers about the current instant and does not wait, so on
a loaded runner the probe falls through before the affordance renders.
The branch it fell through to — clicking the last prompt-rail tick — was
inherited from a suite that never ran, so nothing had ever executed it.
Wait for the button instead, and drop the untested fallback.

Coming back from the far end of the history is a range reload rather
than a scroll: the Host resolves a new window around the tail and the
renderer mounts it. The suite's 10s expect timeout is sized for UI
already on screen, and the CI trace shows the click landing and the
assertion timing out behind it with four workers competing. Give that
one step 30s.

Generated-by: Claude Code
@Astro-Han
Astro-Han marked this pull request as ready for review September 4, 2026 17:40

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

Approving at exact head f2939692. No findings. Both checks pass.

The premise checks out

MAKA_TRANSCRIPT_PERF and MAKA_TRANSCRIPT_STRESS appear on main in exactly two files — the spec that gates on them and its seed helper. Not in ci.yml, not in desktop-nightly.yml, not in any script. So the claim is accurate: nothing could enable that suite, and it has been skipped since it was written. A regression line that cannot run is worse than none, because its presence is what stops anyone from building one.

The CSS change is safe, and the reasoning behind it is the interesting part

transition-duration: 0.01ms !important is not "effectively no transition". With the initial transition-property: all, it is a live transition on every animatable property of every element — and Chromium still constructs the transition objects and still fires transitionrun / transitionstart / transitionend. Pair that with * { scrollbar-color: … } computed from a relative oklch(...) that gets re-serialised on each style recalculation, and something changes on every recalc. 2,481 transitionrun events in one wheel sweep over ten Turns is the measured result.

I checked the one risk this introduces: no production code listens for transitionend, transitionrun, transitionstart or onTransitionEnd anywhere in packages/ui/src or apps/desktop/src. So replacing the duration with transition: none cannot strand a handler waiting for an event that no longer fires.

That matters most for the prefers-reduced-motion: reduce block, which is a real user path rather than a fixture. For a reader who asked for less motion, none is strictly closer to what they asked for than a near-zero duration that still runs the machinery.

The three assertions that were kept, and the three that were not

The table of rejected candidates is the most useful part of this PR, because each rejection is measured rather than argued:

  • style recalculation count — 315–332 with the storm against 156–225 without. A threshold separating them sits about 25% above the clean arm's own spread: too tight for a shared runner, too loose to catch anything smaller than the storm itself.
  • forced layout count — 440–460 in both arms. No signal at all.
  • one scroll write per ResizeObserver delivery — already covered at unit level, more precisely, by the nine tests in transcript-scroll-authority.test.ts. An e2e restatement would be duplicate coverage.

Rejecting a counter because it has no signal, and rejecting another because a unit test already pins it better, is the part that usually gets skipped. No millisecond number is asserted anywhere, which is the right call for a shared runner.

Two findings in the description worth keeping visible

checkVisibility({ contentVisibilityAuto: true }) cannot measure containment from a test. The flag it reads is updated during rendering, so a synchronous call after a scroll reports everything visible — it returned 0 skipped Turns in every position where contentvisibilityautostatechange reported 1 to 8, including for a control element parked 2,800px off-screen. Any containment measurement built on it reads as "containment never engages". Using the event instead is the fix, and this is worth knowing before anyone writes another such probe.

The timing numbers in #4206 and #4259 were measured through the storm. That does not make those changes wrong, but their reported deltas were substantially "how much of the transition storm containment happened to hide" rather than product cost. Stating it here is the honest thing to do.

On deleting the telemetry rather than relocating it

desktop-nightly.yml runs no tests — it packages, signs, notarizes, verifies and publishes. Putting a perf harness there would place it in the path of release publication and would still need a rolling median this repository has nowhere to keep. Deleting a console.log nobody read, in a job that never ran, is the accurate outcome; a trend line deserves its own scheduled workflow and its own storage, which is a separate change.

Driving the gesture through CDP Input.dispatchMouseEvent rather than writing absolute scrollTop per frame is also right for the reason given: absolute writes erase whatever anchoring correction the browser applied since the previous frame, so the probe fights the scroller and manufactures displacement that reads as a product bug.

This is a test change, so the merge decision remains a human's.

简体中文

f2939692 上批准。没有发现问题。 两项检查均通过。

前提核实成立

MAKA_TRANSCRIPT_PERFMAKA_TRANSCRIPT_STRESSmain 上恰好出现在两个文件里——那个以它们为开关的 spec,以及它的 seed helper。不在 ci.yml,不在 desktop-nightly.yml,不在任何脚本里。 所以这个声称是准确的:没有任何东西能启用那个套件,它从写出来那天起就一直被跳过。 一条跑不起来的回归防线比没有更糟——因为它的存在本身,会让人不再去建一条真的。

CSS 改动是安全的,而它背后的推理才是有意思的部分

transition-duration: 0.01ms !important 并不等于「实际上没有 transition」。由于初始的 transition-property: all,它是每个元素每个可动画属性上的一个活的 transition——而 Chromium 仍然会构造 transition 对象,仍然会触发 transitionrun / transitionstart / transitionend。再叠加 * { scrollbar-color: … } 由一个相对 oklch(...) 计算而来、每次样式重算都会被重新序列化,于是每一次重算都真的有东西在变。实测结果是:一次滚轮扫过十个 Turn,2,481 个 transitionrun 事件。

我核了这个改动引入的唯一风险:packages/ui/srcapps/desktop/src 的生产代码里,没有任何地方监听 transitionendtransitionruntransitionstartonTransitionEnd 所以把 duration 换成 transition: none,不可能把某个处理器晾在一个不再触发的事件上。

这一点对 prefers-reduced-motion: reduce 那个块最要紧,因为那是真实用户路径而非 fixture。 对一个主动要求减少动效的读者来说,none 严格地比「一个仍然把整套机制跑起来的近零时长」更接近他所要求的东西。

保留的三条断言,与没保留的三条

被否掉的候选那张表是这个 PR 里最有用的部分,因为每一次否决都是量出来的,不是论证出来的:

  • 样式重算次数——有风暴时 315–332,无风暴时 156–225。能分开两者的阈值大约落在干净那一臂自身离散度之上 25%:对共享 runner 来说太紧,而要抓比风暴本身更小的东西又太松。
  • 强制布局次数——两臂都是 440–460。完全没有信号。
  • 每次 ResizeObserver 投递对应一次 scroll 写入——已经在单元层由 transcript-scroll-authority.test.ts 的九条测试更精确地覆盖了。e2e 上再陈述一遍就是重复覆盖。

因为「没有信号」而否掉一个计数器,又因为「单元测试已经钉得更准」而否掉另一个,这一步通常会被跳过。任何地方都没有断言毫秒数,对共享 runner 而言这是正确的选择。

描述里有两条发现值得保持可见

checkVisibility({ contentVisibilityAuto: true }) 无法从测试中测量 containment。 它所读的标志是在渲染期间更新的,所以滚动之后的同步调用会报告一切可见——contentvisibilityautostatechange 报告跳过 1 到 8 个 Turn 的每一个位置上,它都返回 0,包括一个被故意停在屏幕外 2,800px 的对照元素。任何建立在它之上的 containment 测量,读出来都会是「containment 从不生效」。改用事件是正确的修法,而这件事值得在有人再写一个这样的探针之前就知道。

#4206#4259 里的时间数字是隔着这场风暴测出来的。 这并不使那些改动本身有错,但它们报告的差值,相当程度上是「containment 恰好遮住了多少 transition 风暴」,而不是产品成本。 在这里说清楚是诚实的做法。

关于删除遥测而不是搬迁它

desktop-nightly.yml 不跑任何测试——它做打包、签名、公证、校验与发布。把一个 perf harness 放进去,等于把它放进发布路径,而且仍然需要一个这个仓库无处安放的滚动中位数。 删掉一个「没人读的 console.log,在一个从不运行的 job 里」才是准确的结果;趋势线应当有它自己的定时工作流与自己的存储,那是另一个改动。

用 CDP Input.dispatchMouseEvent 驱动手势、而不是每帧写绝对 scrollTop,理由也是对的:绝对写入会抹掉浏览器自上一帧以来施加的任何锚定修正,于是探针与滚动容器互相对抗,制造出读起来像产品缺陷的位移。

这是一次测试改动,合并与否仍由人决定。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han merged commit 2e829be into main Sep 4, 2026
3 checks passed
@Astro-Han
Astro-Han deleted the test/desktop-transcript-scroll-cost-ratchets branch September 4, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants