Repaint the key line when a disclosure changes, not when one is restated - #112
Conversation
leaf-agent
left a comment
leaf-agent
left a comment
There was a problem hiding this comment.
The observer guard is right, and it holds up under the reads I could construct against it: if no record's oldValue differs from the attribute's current value, then the attribute never held a different value at any point in the batch, so a toggle-and-return inside one batch still repaints on its return leg's record. Putting it on the observer rather than on paintCoreControls also covers living-margin's per-row aria-expanded restatements, which ride lf-actions the same way. reflectShortcuts in keyboard/scopes.js already guards its own write the same way (if (scope.el.getAttribute("aria-keyshortcuts") !== shortcuts)), so the shape is the file's own.
One finding, in the new test.
probe.applied can never increment, so the assertion is not paints <= applied + 1 but paints <= 1. The listener is registered bare, which binds it to window in the bubble phase, and every dispatcher of lf-actions sends a plain new Event("lf-actions") on document — non-bubbling, so the bubble-phase window listener is never invoked. render_harness.py's own ticked() and every runtime subscriber (decisions/view.js, living-margin.js, updates.js, requests.js) use document.addEventListener for exactly this reason. The PR's own bug-back line records it: applied: 0.
That matters because the tolerance term is what the test needs on the runner this PR is about. syncDecisions is subscribed to lf-actions and ends in paintKeys() or paintHere() either way, so every heartbeat — TICK_MS, 2000ms — repaints the key line on a page nobody is touching. The window is 90 frames, which is ~1.5s at 60fps and stays under one heartbeat on a desk, but on 4 vCPU under -n 2 rAF only has to fall to ~45fps for the window to cross 2s. Then paints is 1 or more against a tolerance of 1 and the test fails on a page at rest, with the fix in place.
Measured, on this branch's merged tree
Chromium confirms the dispatch semantics directly — document.dispatchEvent(new Event("lf-actions")) against three listeners:
{'win_bubble': 0, 'win_capture': 1, 'doc': 1}
Running the test as written, with a document-scoped counter added beside the existing one and nothing else changed:
| window | paints |
applied (window) |
applied (document) |
result |
|---|---|---|---|---|
frames >= 90 |
0 | 0 | 0 | passes |
frames >= 400 |
3 | 0 | 3 | fails, assert 3 <= (0 + 1) |
At 400 frames the page is at rest and correct — three heartbeats, three repaints, one apiece. paints tracks the state exactly; only the counter it is compared against is missing. With the one-word change the same 400-frame window passes at 3 <= 4.
The test stays non-vacuous with the change: reverting the observer to new MutationObserver(() => paintHere()) and re-running at 90 frames gives {'frames': 90, 'paints': 90, 'applied': 0}, failing as it should — the same numbers the PR reports.
Not blocking on this, but worth a note against tests/CLAUDE.md's "If the mechanism is a watcher or lease that acts only after a grace period, the test must hold a window derived from that product constant plus scheduling room": with the counter fixed, the assertion is a ratio rather than a bound, so it holds for any number of heartbeats and the 90-frame figure stops carrying weight. It is only the broken counter that makes the window size load-bearing.
I could not run the full nightly suite here — this sandbox has no browser until #106 lands, so I installed the chromium shell into the session to run the one test above. The 33m45s number in the description is unverified from here.
|
The 1490 passed, 6 skipped, 4 failed, and none of the four is this change. Two of them are The fourth is worth a line because it lands in the file this PR edits its test into. Chunks, and one note on the docsEach chunk
|
paintCoreControls writes what the More control currently says on every paint, `aria-expanded` among it. The runtime watches `open` and `aria-expanded` over the whole document, because those two attributes are how both spellings of a disclosure keep which way they stand, and a record arrives for a write that says what the attribute already said. So the paint delivered its own write back to itself and asked for another frame, and the page repainted for as long as it was open. Nothing on screen says so, and the runner was not slow. Between the last green run and `adf9e30`, which added that write, every `tests/test_render_*` file roughly doubled in wall time while the rest of the suite got slightly faster — 1327s to 2473s against 285s to 231s, over the tests both runs reached. A CPU profile put a second of self time in `renderLine` where the run before it had under a millisecond, and an untouched page mutated the key line 4100 times in two seconds against 37. Read the old value, which is what tells a restatement from a change. A real toggle still arrives, including one that lands back where it started, because the record for its return leg carries the other value.
The probe's `lf-actions` listener was registered bare, which binds to
`window` in the bubble phase, and `lf-actions` is a non-bubbling `Event`
dispatched on `document`. So `applied` never incremented and the
assertion was really `paints <= 1` — a bound that holds on a desk, where
90 frames is under one 2000ms heartbeat, but crosses a heartbeat on a
loaded runner and fails on a page at rest.
With the listener on `document` the assertion is the ratio it was meant
to be: over 400 frames the page reports `{'paints': 3, 'applied': 3}`,
one repaint per heartbeat, and passes.
The paragraph beside the watch says it repaints for both spellings and stops there, which is the sentence a contributor reads before writing the next paint-time attribute write. It is the one that has to carry the distinction now: the record is the write, and only the comparison is the state.
7b982dd to
dc2f1db
Compare
|
The
Measured on
That also bears on #111, which fixes the same test by reading both boxes in one How the per-frame swap was traced, and the other failuresA Same-chunk wall time on 4 vCPU, with and without the guard, on Independent chunked nightly inventory on |
2 participants
Problem
The nightly suite did not get slow because the runner was slow.
adf9e30closed a repaint loop in the runtime, and every browser test on every page has been paying for it since — which took the run from 33 minutes to over 45 and put five pushes over the old bound.paintCoreControlswritesaria-expandedon the key line's More control every paint.disclosureWatchobservesopenandaria-expandedover the whole document, because those two attributes are how both spellings of a disclosure keep which way they stand, and it repaints for either. A write that restates a value still delivers a record, so the paint fed itself and the page repainted for as long as it was open.Evidence that the cause is in the page, not the machine
Comparing per-test wall time from the
-vlogs of the last green run (33262000771) againstadf9e30's (33264501463), over the tests both runs reached:adf9e30tests/test_render_*A slow runner does not speed the file tests up while doubling the browser ones.
bc4596d, the commit before, ran the whole suite in 33m33s (33264423236);adf9e30and every push after it hit the wall.Two local readings name the mechanism. A CPU profile of one page under
adf9e30puts 1055ms of self time inrenderLineand 1199ms inpageSelection, against under a millisecond each atbc4596d. Counting mutations on.lf-keylineover two seconds of a page nobody is touching gives 4100 atadf9e30and 37 atbc4596d— a repaint per frame against one per heartbeat.Solution
Reading
oldValueis what tells a restatement from a change. A real toggle still arrives, including one that lands back where it started, because the record for its return leg carries the other value.The guard belongs on the observer rather than on the one write that tripped it: the observer is what makes any paint-time write of those two attributes re-enter the paint, and the loop it creates is silent — it never turns a check red, it only spends the run's time.
plugins/leaf/skills/leaf/CLAUDE.mddescribes that watch and stopped at "repaints for both", which is the sentence a contributor reads before writing the next paint-time attribute write; it carries the distinction now.test_a_page_at_rest_repaints_the_key_line_only_when_the_state_movesis new and holds the shape. It reads the page's own frames against its own state applications: every application repaints the line and says so throughlf-actions, the heartbeat's re-application included, so a line repainting more often than the state moves is repainting for a reason the page has not got. Counting frames rather than seconds keeps the contrast the same size on a loaded runner as on a desk. Its second commit fixes the listener —lf-actionsis a non-bubblingEventondocument, so a bareaddEventListenerbound towindowand never counted, leaving the assertion atpaints <= 1: true on a desk inside one heartbeat, false on a slower runner that crosses one.c14bb42raised the push bound to 90 minutes on the reading that the runner was slow but advancing. That is your call and this PR leaves it alone — but the measured time below is what the bound is now sitting over, so it may be worth a second look.The two failures the old bound was hiding are already fixed on main:
0a4dc85for the visual-action proxies a scroll stop was taking away, and3d04843for the text-entry key test. This PR is only the loop.Testing
uv run pytest tests --run-nightlyon 4 vCPU with the suite's own-n 2, againstfcf32c2plus this change: 1491 passed, 6 skipped, 3 failed in 1973s (32m53s) — back at the 33 minutes the suite ran at beforeadf9e30, with more tests in it than that run had.All three failures reproduce on plain
origin/mainwithout this branch, and all three arece73388's:test_example_renders[gallery]and[parallel-workstreams]report<main> draws 96px of inset and shows 128px below what it holds: its last block is a <lf-tabs> reserving 32px against a neighbour it hasn't got, andtest_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpusreports the find box's ring under the margin preview's close button.ce73388's own ci run is red on them. They are not this branch's and are not fixed here.Bug-back, run both ways: with the observer change reverted, the new test reports
{'frames': 90, 'paints': 90, 'applied': 0}— a repaint on every frame of an untouched page.pre-commit run --filesover the changed files passes. The comment below carries an independent chunked run of the same suite from another session, with the same inventory.Automated fix for failed run