amadan

oficina / docs Public

← Discussions

worktree-toolbar-flash: the toolbar repaints itself on every keystroke, and 21 of 21 writes change nothing closed

On branch worktree-toolbar-flash

claude driven by paul@keymail.dev session 4a30c557 · claude-code/2.1.252 · DOCS-T1

The toolbar flashes when you switch tabs, and it is rewriting itself on every keystroke too. Opening this before the first commit.

Measured, not guessed

Paul reported a flash when tabbing between two tabs on the same document. Driving it in a real browser — one genuinely hidden tab, an edit made in the other, then revealed — the content update turns out to be innocent: the editable node is never replaced, one text node is swapped, and the change is visible 33ms after reveal.

The flash is the toolbar. On reveal, 23 DOM mutations land, 21 of them attribute writes on visible controls: buttons flipping disabled and aria-pressed, and the Style, Font and Size comboboxes relabelling.

Then the part that decides the fix. Recording each mutation's old value against its new one:

21 of 21 writes are redundant. Zero changed value. Every one writes the string that was already there — aria-pressed: "false" -> "false", disabled: "" -> "".

Why

fixtures/toolbar.mjs's plugin update loops every button and unconditionally assigns button.disabled and setAttribute('aria-pressed', …), then re-enables every select and combobox. It is correct about what the state should be; it just says so again every time, whether or not anything moved.

setAttribute with an identical value still invalidates style and still records a mutation, so a bar of controls repaints for nothing.

And this is not a tab-switch bug that happens to show up elsewhere — that loop runs on every ProseMirror state update, which means every keystroke. Tabbing back just makes it visible, because the whole bar repaints in one frame instead of being lost in the noise of typing.

An earlier measurement of the same thing, before the toolbar redesign, showed 12 mutations. It is 23 now. The cost grows with every control added, which is the shape of a defect that gets worse quietly.

The fix

Write only when the value differs. That is the whole change: compare before assigning, for disabled, for aria-pressed, and for the select and combobox properties the same loop sets.

It cannot change behaviour — the resulting DOM is identical, because the writes it skips were writing what was already there. What it removes is the repaint.

What I want the guard to be

Not "count mutations on tab switch", which is a symptom at one moment. The honest guard is: after a no-op state update, the toolbar performs zero attribute writes. That fails against today's code, passes after, and keeps failing if someone later adds a control that writes unconditionally — which is exactly how this grew from 12 to 23.

Not in scope

Whether the toolbar should disable at all when the editor loses focus. Every write measured here is redundant, so that question does not arise for this flash; if disabling on blur is also wrong, it is a separate change with its own argument.

claude driven by paul@keymail.dev session 4a30c557 · claude-code/2.1.252 · DOCS-T1

Whole-branch review: approve with changes, and the one change was in the test, not the product.

The product fix stands as measured

The reviewer went after the thing that would actually hurt — a write that looks redundant but carries a side effect — and checked every call site the new setAttr/setProp helpers now guard:

  • input.value on the search field: skipping an identical assignment is strictly safer, because assigning the same string still resets the caret to the end in Chromium.
  • select.value with a value that is not in the option list: the browser refuses the assignment and leaves the old value. The fake reproduces that refusal, so a bad value stays loud instead of being silently swallowed by the guard.
  • tabIndex routing traced through the one control rendered without a tabindex attribute: behaviourally identical.

Browser drive re-run: 44/44. Gate exit 0.

The defect: my report's central claim was false

I wrote that the structural check was "false-positive-prone but never false-passing". The reviewer disproved it by construction. Add this to the menu's sync loop, on the every-state-update path:

if (!name.includes('https://')) el.title = 'seen'

and all three tests pass. It is exactly the defect this branch exists to fix — title is attribute-reflected, so an identical-value write invalidates style and records a mutation on every keystroke — and it would have shipped invisibly.

Two blind spots covered each other's hole. stripComments doesn't parse string literals, so the // inside 'https://' blanked the rest of the line before the regex saw it. And the recording fake logged only setAttribute/removeAttribute plus three defined accessors, so a bare .title = landed on a plain object silently.

The fix

Each fake element is now wrapped in a Proxy whose set trap logs every property write before forwarding it. The reviewer's exact probe now fails the no-op test, naming all three menu rows, with from: 'seen' -> 'seen' legible in the log. As a bonus it catches el['disabled'], el.ariaPressed, and compound assignments like ||=, which the regex's bare-= shape also missed.

The structural check still passes against the probe, as predicted — which is the argument for keeping both nets rather than replacing one. The false claim is corrected where it was written down, in makeDOM's header and above stripComments.

Accepted, not fixed

  • A third surface added to that plugin is caught by neither net: not in GUARDED, not mounted by the fake. A mechanical derivation of GUARDED sweeps in modules that legitimately write off the update path, so the hardcoded list plus its comment stays.
  • dataset, style and classList writes are outside both nets even with the Proxy — they are method calls, not property sets.
  • toolbar-roving's setStop is off the no-op path.
  • Toolbar sync adds aria-pressed="false" to undo/redo although show.html deliberately omits it. Pre-existing, arguably an ARIA wart, worth removing another day.

Merging.

claude driven by paul@keymail.dev session 4a30c557 · claude-code/2.1.252 · DOCS-T1

Merged as f5fd252 and live.

The chain from the measurement to production, closed link by link rather than assumed:

  • The measurement that mattered was taken on this bundle: 0 attribute writes on a tab-switch reveal, down from 23, with content still landing in 35ms.
  • Rebuilding the bundle from fixtures/ produces a file byte-identical to the one committed, so nothing was measured that then failed to ship.
  • Production serves those exact bytes — fetched /static/editor.5dc0309cb2a5ef6b.js from the live host, 336107 bytes, cmp clean against the local file.
  • x-carlos-version: d20a813 on the real host, read off the response rather than from carlos channels, which reports what a channel points at and not what the box is serving.

The bundle's name is content-hashed, and the previously deployed one was editor.d8d3d5d91b1c09c4.js. A returning tab therefore cannot serve the old JavaScript out of cache — the filename it asks for no longer exists in that cache.

Closing. Discussion 31 carries the locale work; the standing items recorded in the review reply (the third-surface gap, dataset/style/classList, the undo/redo aria-pressed wart) are noted there and in the branch report, not lost with this thread.