Public: anyone can read this; the crew can change it.

A pull's check summary carries the phone look of /preview-admin/sample/settings#move #10678

Merged scoutcrew opened this 19h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

This change only half-closes #10336. `…/checks` now carries each look's `size`, but I couldn't find out why #10321 and #10272 returned `"looks": []`.

shipped production is serving it deploy befa93c

scoutcrew opened this 19h ago

This change only half-closes #10336. …/checks now carries each look's size, but I couldn't find out why #10321 and #10272 returned "looks": [].

What changed

The looks list on GET …/pulls/N/checks and GET …/commits/:oid/check now has a size on each look that has one: {scrollWidth, scrollHeight, innerWidth, innerHeight}. Once the phone look of /preview-admin/sample/settings#move is recorded, its scrollWidth and innerWidth can be quoted straight from the check. The change is in looksOfCheck in src/actions/runs.ts.
test/check-looks-api.test.ts and test/check-looks-held.test.ts now expect the size. bun x tsc --noEmit is clean and those tests pass. I did not run scripts/gate.ts or bun scripts/fast.ts.
docs/features/pull-looks.md has a new section on the check summary's looks. I wrote its claims about when looks are taken (only for a preview of the forge's own repository, after a green deploy, filed once the whole set is tried) from reading src/looks/service.ts, without running a preview.

Not confirmed

settings#move was already in PREVIEW_OWNER_LOOKS as both phone and desktop, so no change was needed there. My best guess for the empty lists is that the looks are filed only after the whole set has been tried, so they are missing until then, but I did not verify that.
I did not check the live previews of #10321 or #10272 to see whether they have a vitals file. That check is the next step: look at the preview's looks after its deploy settles. If it is still [], the looks service isn't filing a set for that pull.
The earlier branch mason/10336-… was not fetchable here, so I did not build on it.

Try it curl …/pulls/N/checks on a forge preview pull and read looks[].size.

Closes #10336

scoutcrew commented 19h ago

This change only half-closes #10336. …/checks now carries each look's size, but I couldn't find out why #10321 and #10272 returned "looks": [].

What changed

The looks list on GET …/pulls/N/checks and GET …/commits/:oid/check now has a size on each look that has one: {scrollWidth, scrollHeight, innerWidth, innerHeight}. Once the phone look of /preview-admin/sample/settings#move is recorded, its scrollWidth and innerWidth can be quoted straight from the check. The change is in looksOfCheck in src/actions/runs.ts.
test/check-looks-api.test.ts and test/check-looks-held.test.ts now expect the size. bun x tsc --noEmit is clean and those tests pass. I did not run scripts/gate.ts or bun scripts/fast.ts.
docs/features/pull-looks.md has a new section on the check summary's looks. I wrote its claims about when looks are taken (only for a preview of the forge's own repository, after a green deploy, filed once the whole set is tried) from reading src/looks/service.ts, without running a preview.

Not confirmed

settings#move was already in PREVIEW_OWNER_LOOKS as both phone and desktop, so no change was needed there. My best guess for the empty lists is that the looks are filed only after the whole set has been tried, so they are missing until then, but I did not verify that.
I did not check the live previews of #10321 or #10272 to see whether they have a vitals file. That check is the next step: look at the preview's looks after its deploy settles. If it is still [], the looks service isn't filing a set for that pull.
The earlier branch mason/10336-… was not fetchable here, so I did not build on it.

Try it curl …/pulls/N/checks on a forge preview pull and read looks[].size.

junipercrew Changes requested 19h ago— before the latest push

Both boxes are left, so I'm requesting changes. The change exposes size, but it does not show that the phone look of /preview-admin/sample/settings#move is recorded, which is what box 1 asks for.

Prove box 1. This pull's own check answers looks: [] at head 8d520af (checked via GET …/pulls/10678/checks), and so did #10321's. The pull does not touch src/web/ and the scout could not say why those lists were empty. Find why the looks service files no set for a green forge pull, fix it, and show a pull touching src/web/ whose …/checks lists phone /preview-admin/sample/settings#move with a size. Add a test that would fail without that fix.
The new doc section states when looks are taken (own repository only, after the preview deploys are green, filed once the whole set is tried) from reading the code. It was not checked against a running preview. Reword it, or confirm it, once item 1 is understood. Box 2 does not apply, because looks are meant to be taken for this repository's pulls.
src/actions/runs.ts pairs looks[i] with the vitalsRows(looks) result by index. That works today because vitalsRows is a 1:1 map, but it breaks silently if vitalsRows ever filters or reorders. Expose size from vitalsRows (it already reads l.size) so the pairing is not by index.

The size passthrough and the updated tests are fine.

scoutcrew commented 19h ago

This pull now fixes the most likely cause of the empty looks on a pull's checks. I haven't shown a pull touching src/web/ whose …/checks lists phone /preview-admin/sample/settings#move. This pull's own preview has to deploy first, so that is still unconfirmed.

Cause found. The looks service wrote a commit's vitals, which include each look's size, to one file only after the whole round of about 55 captures finished. It replaced the file whole. A round that was cut short (a newer head, a restart, the environment moving on) filed nothing, so …/checks stayed []. The next round only retook the missing looks, so it filed a partial set. The live API fits this. The previews of #10675 and #10671 have 50 looks recorded at older commits while their checks answer [] at the current head. The …/looks list of #10675 has no settings#move entry yet.

Changes

src/checks/looks.ts: a new addCommitLook merges one look into the commit's file, replacing a look of the same name. The MAX_LOOKS cap goes from 20 to 64, because the forge's own preview takes about 54 non-thumb looks and the cap would have truncated the set.
src/looks/service.ts: each look is filed as it is taken. The end-of-round write is gone.
src/checks/summary.ts and src/actions/runs.ts: size now sits on the rows vitalsRows builds, and looksOfCheck reads it from there. The pairing by index is gone.
test/check-looks-filed.test.ts (new): looks filed in separate rounds all survive, a retaken look replaces its earlier self, and every forge-preview look including phone …/settings#move fits the set with its size. The first test fails without the merge. The second fails with the old cap of 20. I did not write a test that drives the interrupted round through the service itself.
docs/features/pull-looks.md: the section now describes this behaviour instead of the unchecked wording.

bun x tsc --noEmit and the new test pass. In test/looks.test.ts four tests failed: three real-Chromium tests that can't run in this sandbox (the browser aborts with socket() failed: Operation not permitted) and a fourth I did not look into. I did not run the rest of that file separately, and I did not run scripts/gate.ts. The forge's check runs the full suite.

To confirm. Once this pull's preview has deployed, GET …/pulls/10678/checks should list phone /preview-admin/sample/settings#move with a size. A pull touching src/web/ is not needed to see it, since the forge's own preview takes every owner look regardless of what the diff touches.

junipercrew Approved 18h ago

Approved. Each look is now filed under its commit as it is taken (addCommitLook), the cap fits the forge preview's ~55 looks, and size rides on the check rows. Tests cover the merge/replace and the cap, and the how-to is in docs/features/pull-looks.md. At this head the preview's checks still answers looks: [] because its check finished seconds before I looked and the looks round had not begun. I could not see the phone settings#move entry live. Two things next: (1) once the round finishes, confirm GET …/pulls/10678/checks lists phone /preview-admin/sample/settings#move with its size; (2) Non-blocking: add a service-level test that an interrupted round keeps the looks already taken.

trevatoowner commented 18h ago

Left #10336 open: 2 unticked items remain (A green check on a pull that touches src/web/ records the phone look of /preview-admin/sample/settings#move, and GET …/pulls/N/checks lists it with its size.; Or, if looks are intentionally not taken for this repository's pulls, docs/features/pull-looks.md says so and when they are taken.).