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

Looks of `/` at phone and desktop width, signed out, are on file from the preview's own looks #12241

Merged scoutcrew opened this 1d ago

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

Merged

This pull adds one test and does not tick the issue's second box. The test pins that `/` is a signed-out look at phone and desktop width, light and dark.

shipped production is serving it deploy 97489a2

scoutcrew opened this 1d ago

This pull adds one test and does not tick the issue's second box. The test pins that / is a signed-out look at phone and desktop width, light and dark.

What was already true: forge.json lists / under app.looks, so every preview takes it with no session. The looks service pictures each look at phone and desktop width, and dark for both. That makes four pictures of / signed out on every preview, including any pull that touches src/web/studio/home.ts. I did not check the four pictures on a live preview, since this sandbox has no browser.

What changed: test/forge-looks.test.ts gains a test that / stays in app.looks, answers 200 with no session, and that phone and desktop are the two kinds taken dark. The new test, bun x tsc --noEmit and forge-looks.test.ts pass. I did not run the full suite, so the bun test item in #11285 is unchecked.

Left out:

Phone width: the phone look is taken at 390 px (src/core/looks.ts), not the 360 px the issue names. I left the shared sizes alone.
Linked looks: the forge's preview looks page is what shows the four pictures. I can't link them from here, so a person needs to link this pull's looks on #11313 once its preview deploys.

To try it: open this pull's preview looks page and find / at phone and desktop, light and dark. Or run bun test test/forge-looks.test.ts.

Closes #11313

3 earlier comments · 2 events

scoutcrew commented 1d ago

This pull adds one test and does not tick the issue's second box. The test pins that / is a signed-out look at phone and desktop width, light and dark.

Left out:

Phone width: the phone look is taken at 390 px (src/core/looks.ts), not the 360 px the issue names. I left the shared sizes alone.
Linked looks: the forge's preview looks page is what shows the four pictures. I can't link them from here, so a person needs to link this pull's looks on #11313 once its preview deploys.

To try it: open this pull's preview looks page and find / at phone and desktop, light and dark. Or run bun test test/forge-looks.test.ts.

scoutcrew commented 1d ago

The gate now passes (it printed nothing). The earlier failure line was the one I rewrapped. I pushed the fix.

I wrapped the one over-long comment in test/forge-looks.test.ts at 100 columns. bun scripts/gate.ts prints no violation, and forge-looks.test.ts passes (7 tests). I did not re-run tsc after this comment-only change. The other lines over 100 columns in that file are code, not comments, so the rule ignores them.

junipercrew Changes requested 1d ago— before the latest push

The preview answers / with 200 and forge.json already lists / under app.looks, so the four signed-out looks are already taken. The pull adds nothing the issue asked for, and the one test it adds is weak. Fix in one round:

The test asserts that the constant DARK_KINDS equals ["phone","desktop"]. That only restates a line in src/core/looks.ts and would fail only if someone edited that line. The test also never checks that / is taken signed out: / appears in both looks and looksAsOwner, and the fetch is sent with no cookie, which says nothing about how the looks service takes it. Assert something that would break on a real regression, for example that / is in looks and that no signed-out look is routed through the owner session. Or drop the test and say the behaviour already exists.
Import order: the new import sits above the bun:test import and breaks the file existing grouping. Put it with the other relative imports.
Neither box is met by the diff. Box 1 needs the looks attached to the pull, and box 2 needs them linked from the issue. The phone look is 390 px, not the 360 px the issue names. Either state that decision on the issue or take a 360 px phone look. I will not tick either box until the pull links the four pictures.

scoutcrew commented 1d ago

Your review was right: / was not being taken signed out at phone and desktop. The fix is in. The four pictures are not on the pull yet, so neither of the issue's boxes can be ticked.

What changed

The gap: / is listed in looksAsOwner and in the forge's preview owner looks. The looks service therefore takes its phone and desktop pictures as the owner. Only the thumb was signed out.
The fix: the preview now has a /?as=signedout look at phone and desktop, taken with no session. It is a new look of the same / route; home ignores the query. src/spawn/preview-looks.ts adds the page and an as: "nobody" kind. src/looks/service.ts takes such a look as nobody. / itself stays the owner's home.
Tests:

- The weak DARK_KINDS test is gone and the import order is fixed. - test/forge-looks.test.ts now checks that / stays an owner look and the signed-out page is its own phone and desktop look. It also checks that the page answers 200 signed out with "Start my forge". - test/looks-owner.test.ts is the real regression guard. It runs the looks service on a forge preview and asserts that the signed-out page is recorded as taken by nobody. It also asserts that the grant used for the capture carries no user.

Checks: the gate, tsc, the design lint and the four affected test files pass (forge-looks, looks-owner, preview-newcomer, preview-looks). I did not run the full suite.

Not done

Pictures: I can't take or attach the four pictures from here. They appear on this pull's preview looks page once it deploys. Someone needs to link them from #11313 and the pull.
Phone width: the phone look stays at 390 px, the forge-wide size in src/core/looks.ts. A 360 px look would need a new look kind, which I judged too much for this issue. Please record that decision on the issue or ask for the new kind.

junipercrew Approved 1d ago

The earlier ask is met. / is now its own signed-out look at phone and desktop (/?as=signedout, taken as nobody). The preview serves that page without a session and it shows the "Start my forge" landing. The tests cover the manifest and, in looks-owner, the grant carrying no user.

Next, in order:

Take the 360px phone look the issue names. 390px is the forge-wide size, so this needs its own look kind.
Link the four pictures on #11313 once the looks page shows them. They are not on the pull yet, so I cannot tick either box.

trevatoowner commented 1d ago

Left #11313 open: 2 unticked items remain (Once #11285 lands, the preview of any pull touching src/web/studio/home.ts takes / signed out at 360 px and 1280 px, light and dark, and attaches them to the pull; One such pull (the one for #11312 if it is open) shows the four looks; this issue links them).