Skip to content

Read the margin layout the resize scheduled before measuring the sidebar page - #153

Merged
max-sixty merged 1 commit into
mainfrom
fix/ci-33340968793
Aug 31, 2026
Merged

Read the margin layout the resize scheduled before measuring the sidebar page#153
max-sixty merged 1 commit into
mainfrom
fix/ci-33340968793

Conversation

@leaf-agent

Copy link
Copy Markdown
Collaborator

Problem

ci on 597ff63 failed in run 33340968793 with 32 tests red. Almost all of them are the standing board and already owned; one is not, and it is a race rather than a stale reading — tests/test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back:

assert tighter["marginRight"] <= tighter["viewportWidth"] + 1
E   assert 1258 <= (1234 + 1)

The test narrows the window to exactly what the sidebar strip, a 720px column and the rail claim need, then asks that the widest margin row still fits. margin-layout.js answers a width change through a ResizeObserver on the body and a requestAnimationFrame, so the rows are re-measured and re-condensed one whole rendering turn after the resize event resized waits for. The read in between is a read of the rows at the width they have just left: the widest .lf-margin-item was still 228px wide instead of the 103px lf-condensed gives it, and hung 24px past the window.

It is a race, not a fixture that is wrong about the layer. Running the same probe twice on the same tree, main at 2d46d4a, the first run had the row condensed at 103px and the second had it at 228px reaching 1258 — same code, same width, different frame.

Solution

margins_laid_out(page) runs the layout the resize scheduled, and the sidebar test calls it after each of its two resizes, before reading. Both readings are affected: the tight one asserts the row's right edge, and the roomy one reads --strip-r for the width to narrow to, which is the rail claim that same pass writes.

It runs the pending work rather than waiting a frame for it, because tests/CLAUDE.md is explicit that "a fixed number of animation frames only guesses when that record will be delivered under load". Whether the observer schedules the pass at all is test_render_margin.py's subject, not that of a test reading the layout it produces — and that file already drove it this exact way inline, so this is its recipe named once in the harness and read from both call sites.

Testing

On this branch rebased onto 521dfec:

  • uv run pytest tests — 740 passed, 6 skipped. That is the gate this PR's CI runs.
  • uv run pytest tests/test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back --run-nightly -n0 — passes, three runs in a row. It reproduced red on main at 2d46d4a before the change, with the 1258 above.
  • uv run pytest tests/test_render_controls.py tests/test_render_pages.py tests/test_render_margin.py --run-nightly -n2 (measured at 2d46d4a) — 125 passed, 3 failed, all three owned elsewhere and failing identically on that base: test_coarse_pointer_resize_reach_stays_reachable_without_trapping_scroll (Stop the thread panel wearing the UA dialog's padding #145), test_a_seat_conversation_leaves_the_pick_it_is_about_live (ci-fix: two nightly tests guard a seat no shipped widget still has #147), and test_every_ring_the_layer_draws_is_shown_whole_somewhere_in_the_corpus (Update the option-group tests for the removed seat, and ring the add field #148, whose four input.lf-ui [outline none, shadow none] stops are exactly what that PR describes).
  • ruff check and ruff format --check over tests/ pass. pre-commit needs root to install in this sandbox, so its other hooks went unrun here; CI's lint job covers them.
Where the other 31 failures in that run stand

The whole run's failure set, and its owner:

count failures owner
1 test_the_public_catalog_is_a_visual_index_of_full_page_routes #134 / #133
4 the versions-menu group in test_render_anchors.py #137
19 the durable-draft group, test_c_in_a_seated_conversation…, test_a_questions_digits_are_drawn_whole, test_a_label_in_a_retired_slot…, test_the_ring_says_where_the_reader_is_standing, test_every_ring… #148
2 test_a_seat_conversation_leaves_the_pick_it_is_about_live, test_the_ring_holds_on_a_seat… #147 / #151
1 test_coarse_pointer_resize_reach_stays_reachable_without_trapping_scroll #145
1 test_the_render_gate_reports_code_the_reader_cannot_tell_from_its_block #149
1 test_the_responsive_action_shelf_keeps_primary_actions_in_reach #150
1 test_a_coined_class_cannot_reach_the_chromes_rules #152
1 test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back this PR
1 test_a_folding_reference_hands_its_hold_to_the_next_card unreproduced, below

test_a_folding_reference_hands_its_hold_to_the_next_card is the one new failure at 597ff63 — the run before it, 33340823726 on 650db7e, had the other 31 and not this. It asserts that the card after a folding one holds still, and reported the successor moving from 401.7px to 362.6px after the fold finished. It does not reproduce on today's tree: five runs at -n0 and one whole-file run at -n2 on 2d46d4a all pass. That leaves it unclassified rather than transient — -n0 also drops the load some races need — so it is left rather than guessed at. #144 landed between those two runs and is the panel change nearest to it.

Two failures on origin/main at 521dfec are newer than the run this PR is for and belong to aa41f7a ("Keep chord hints stable through progress"), which changed the key line's chord row to gm1–4page-map items: test_render_margin.py::test_g_addresses_the_page_map_prefix_in_its_announced_order and test_one_margin_item_owns_a_targets_controls_information_and_more_actions both wait for m\s*page-map items. Both fail on that commit with this branch's changes stashed, so they are not this PR's and are not touched here.


Automated fix for failed run

@leaf-agent leaf-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnosis holds and the fix works: I reran test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back with --run-nightly -n0 on this branch and it passes, and the two new call sites look like the only one-shot geometry reads in the suite that follow a resize — the other post-resize margin assertions in test_render_margin.py are retrying expect(...) calls, which wait for the observer's own pass, and the 700px and print readings later in this test sit under @media screen and (max-width: 899px) { .lf-living-margin ... display: none }, so no margin row is in play there.

One point on the shape, which is worth raising now that the recipe is a harness helper in render_support's __all__ rather than one inline call: it awaits a module load inside page.evaluate, which tests/CLAUDE.md names as the unbounded case — "page.evaluate takes no timeout in any binding, so a promise awaited inside it — an animation's finished, a module's load, a listener's next call — is a wait nothing bounds... it spends the job's whole step, and the share of the suite already handed to that worker never runs." test_render_conversations.py carries the same lesson in a comment from a real 45-minute step. Inline suggestion below moves the bound to the driver side, which is what that paragraph prefers; it keeps the single round trip and the layout still runs in a frame.

Comment thread tests/render_harness.py
Comment on lines +1358 to +1361
page.evaluate(
"() => import('/runtime/margin-layout.js')"
".then(({layoutMarginRows}) => layoutMarginRows())"
)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import is awaited inside page.evaluate, so nothing bounds it: if /runtime/margin-layout.js stalls on the way out of the server, this doesn't fail in thirty seconds naming its test — it holds the worker for the rest of the job step. wait_for_function puts the deadline on the driver side, which is the shape tests/CLAUDE.md prefers ("stating synchronous readiness inside the page and polling it with wait_for_probe, whose driver-side wait carries SERVED_TIMEOUT_MS"), and render_checks_model is already imported here.

Suggested change
page.evaluate(
"() => import('/runtime/margin-layout.js')"
".then(({layoutMarginRows}) => layoutMarginRows())"
)
page.wait_for_function(
"() => import('/runtime/margin-layout.js')"
".then(({layoutMarginRows}) => (layoutMarginRows(), true))",
timeout=render_checks_model.SERVED_TIMEOUT_MS,
)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actioned in #154, against main — this merged about three minutes after the review, so the unbounded await is on the default branch rather than on a branch that can still absorb the change.

The suggestion is taken as written: wait_for_function with render_checks.SERVED_TIMEOUT_MS, one round trip, layout still inside a frame. The docstring now carries the reason the bound is driver-side, since the helper is exported and the next caller will not have this thread. #154 is green on the everyday suite and on the sidebar test at --run-nightly -n0.

@max-sixty
max-sixty merged commit d836303 into main Aug 31, 2026
6 checks passed
@max-sixty
max-sixty deleted the fix/ci-33340968793 branch August 31, 2026 00:50
max-sixty pushed a commit that referenced this pull request Aug 31, 2026
…the job (#154)

## Problem

The review on #153
([review](#153 (review)))
raised this while that PR was in flight; it merged before the point was
actioned, so the shape is on `main` now. `margins_laid_out` awaits the
`/runtime/margin-layout.js` import inside `page.evaluate`, which
`tests/CLAUDE.md` names as the unbounded case: "`page.evaluate` takes no
timeout in any binding, so a promise awaited inside it — an animation's
`finished`, a module's load, a listener's next call — is a wait nothing
bounds. It does not fail in thirty seconds naming its test: it spends
the job's whole step, and the share of the suite already handed to that
worker never runs."

That is the class #84 and #95 bound at eleven other sites; this helper
was added after them and reintroduces it at three call sites —
`test_render_pages.py` twice and `test_render_margin.py` once — all of
them on the render suite's ordinary path.

## Solution

Poll the import from the driver with `wait_for_function` against
`render_checks.SERVED_TIMEOUT_MS`, the same patience the payload's own
probes carry. The deadline moves to the driver side, which is the shape
`tests/CLAUDE.md` prefers; the round trip count and the frame the layout
runs in are unchanged. The docstring now says why the wait is bounded
from outside, so the next reader does not have to re-derive it.

## Testing

On this branch, rebased on `6d68a1b`:

- `uv run pytest tests` — 740 passed, 6 skipped. That is the gate CI
runs on this PR.
- `uv run pytest
tests/test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back
--run-nightly -n0` — passes. This is the test #153 fixed, and the one
the helper was written for.
- `uv run pytest tests/test_render_margin.py tests/test_render_pages.py
--run-nightly -n2` — 62 passed, 2 failed. Both failures are pre-existing
and untouched here (below).
- `ruff check` and `ruff format --check` over the changed file pass.

<details><summary>The two nightly failures, and why they are not this
change</summary>


`test_render_margin.py::test_g_addresses_the_page_map_prefix_in_its_announced_order`
and
`test_one_margin_item_owns_a_targets_controls_information_and_more_actions`
both wait for `m\s*page-map items` on the key line and read
`gm1–9page-map items` / `gm1–4page-map items` instead. I reran both at
`-n0` with this diff stashed, on `6d68a1b` unmodified: they fail
identically. They belong to `aa41f7a` ("Keep chord hints stable through
progress"), which changed the chord row to carry the item count, and
#153's body already named them on its own base. No open issue tracks
them yet; the next `ci` on `main` should route them to `tend-ci-fix`.

</details>

---------

Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
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