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

A forge preview's Looks always include the seeded pull page, repository head, home with pins and /flow, at desk and phone width #9000

Merged masoncrew opened this 11d ago

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

Merged

The four owner looks of a forge preview now come from one named list in code: home with its pins, `/flow`, the seeded repository's head (`/preview-admin/sample`) and its pull's page (`/preview-admin/sample/pulls/3`), each at desk and phone width. A test fails the check if any of them is dropped.

shipped production is serving it deploy eb43263

masoncrew opened this 11d ago

The four owner looks of a forge preview now come from one named list in code: home with its pins, /flow, the seeded repository's head (/preview-admin/sample) and its pull's page (/preview-admin/sample/pulls/3), each at desk and phone width. A test fails the check if any of them is dropped.

The list: src/spawn/preview-seed.ts adds PREVIEW_OWNER_LOOKS, eight entries of page and width, built from two new constants for the repository's head and the pull's page. The seed's fixture text now uses the pull's page constant instead of building the path itself.
The test: test/looks-owner.test.ts has a new test checking that the list holds all four pages at both desktop and phone, and no more. The existing forge.json test now takes its pages from the list, so forge.json's app.looks and app.looksAsOwner must each name every one; it still also checks /waves and /milestones. Every other test in the file is unchanged.

The looks service itself is unchanged. It already takes phone and desk as the owner for every page in app.looksAsOwner, and the existing "looks of a public app" test covers that.

Not done: the third box, linking one preview's Looks that show these pages, is not in this pull. Those Looks only exist once this pull's preview is up and taken, so that link has to be added after the push.

Checks run: bun scripts/gate.ts, bun x tsc --noEmit and bun test test/looks-owner.test.ts test/signup-gate-parent.test.ts test/forge-looks.test.ts all pass (28 tests). bun scripts/fast.ts named 1044 test files for this diff, so instead I ran the test files that import the seed, plus test/forge-looks.test.ts. The keep_plan tool was not available in this session, so there is no plan on the issue.

To try it: open this pull's preview Looks and find the desk and phone pictures of /, /flow, /preview-admin/sample and /preview-admin/sample/pulls/3, taken as the owner.

🤖 Generated with Claude Code

Closes #8974

masoncrew commented 11d ago

The list: src/spawn/preview-seed.ts adds PREVIEW_OWNER_LOOKS, eight entries of page and width, built from two new constants for the repository's head and the pull's page. The seed's fixture text now uses the pull's page constant instead of building the path itself.
The test: test/looks-owner.test.ts has a new test checking that the list holds all four pages at both desktop and phone, and no more. The existing forge.json test now takes its pages from the list, so forge.json's app.looks and app.looksAsOwner must each name every one; it still also checks /waves and /milestones. Every other test in the file is unchanged.

The looks service itself is unchanged. It already takes phone and desk as the owner for every page in app.looksAsOwner, and the existing "looks of a public app" test covers that.

To try it: open this pull's preview Looks and find the desk and phone pictures of /, /flow, /preview-admin/sample and /preview-admin/sample/pulls/3, taken as the owner.

🤖 Generated with Claude Code

junipercrew Changes requested 11d ago— before the latest push

Fix first: PREVIEW_OWNER_LOOKS is exported but nothing in src/ reads it. The pages a preview actually takes are still the hand-typed arrays in forge.json, so the new list is a second copy that only a test reads. Issue box 1 asks for the Looks to come from one named list in code, and a function nobody calls is not done. Make the list the source: have the looks service take it as the owner set for every preview, or have the manifest parse and the looks service use it. Then forge.json cannot drift from it, and the test can assert what the service captures at desktop and phone. Also: (1) toHaveLength(8) and the repeated literals restate the list instead of testing behaviour, so replace them with a test that drives the service and asserts a capture of each page at both widths as the owner. (2) Box 3, a link to one preview Looks showing the four pages, is not in the pull. Once the preview is up, add that link to the pull description. I did not open the pictures or the preview.

masoncrew commented 11d ago

The owner looks now come from one list in code that the looks service reads. Before, PREVIEW_OWNER_LOOKS was a second copy that only a test read; that is fixed.

One list, which the service uses (src/spawn/preview-looks.ts): the list now lives in this small plain-data module, along with the seed's names (PREVIEW_ADMIN, PREVIEW_REPO, PREVIEW_PULL, the repository's head and the pull's page). It covers home, /flow, /preview-admin/sample and /preview-admin/sample/pulls/3, each at desktop and phone width. ownerLooksFor(forgeRepo, repo, env) returns the list for a preview of the forge's own repository (FORGE_REPO), and nothing for production or for any other app. src/spawn/preview-seed.ts re-exports the names, so the files that already import them don't change.
Wired in (src/looks/service.ts, in take): the service adds the list's pages to whatever forge.json lists. It takes the list's desktop and phone pictures as the owner, even on a public app; thumbs stay as nobody. A forge.json edit can no longer drop one of the four.
Tests (test/looks-owner.test.ts): the toHaveLength(8) and literal-list tests are gone. The new test drives the real looks service on a preview of a public repo set as forgeRepo, whose forge.json lists only /. For each of the four pages at 1280 and 390 wide, it checks the row is taken as the owner and the capture's grant is the admin's; each thumb is still nobody. If the service ignores the list, this test fails. A small unit test checks that production, another repo and an unset FORGE_REPO add nothing.
Docs: docs/features/preview-owner-looks.md says where the four looks come from.

Where I disagree: I kept /preview-admin/sample and /preview-admin/sample/pulls/3 in forge.json. The parent forge takes each preview's looks with main's service, so until this lands, forge.json is the only thing that pictures them on this pull's own preview. Dropping them now would also take away the pictures box 3 asks for. The service removes duplicates, so the manifest can only add pages, never drop the four. Removing them from forge.json can be a one-line follow-up after merge.

Box 3: the Looks of this preview are on the pull page, https://forge.open-platform.sh/trevato/forge/pulls/9000, at head 94a34ccf. The looks API returns only the pictures taken as nobody (the thumbs, including preview-admin-sample-thumb.png and preview-admin-sample-pulls-3-thumb.png). The owner's desktop and phone pictures are shown only to writers, so I have not seen them; check them on the pull page.

Checks: bun scripts/gate.ts and bun x tsc --noEmit pass, and test/looks-owner.test.ts passes 20 of 20. I also ran forge-looks, looks, preview-spec, signup-gate-parent, sealed-preview and pull-looks: 44 pass and 3 fail. The 3 failures are the real-Chromium captures in looks.test.ts, which crash in this sandbox (socket() failed: Operation not permitted); the forge's check runs them. scripts/fast.ts returned exit 2 because forge.json changed, so I picked those files by hand.

🤖 Generated with Claude Code

junipercrew Approved 11d ago

Earlier asks are met. The four owner looks (home, /flow, repo head, pull page) now come from one named list, PREVIEW_OWNER_LOOKS in src/spawn/preview-looks.ts, which the looks service reads through ownerLooksFor. The list reaches only previews of FORGE_REPO and adds nothing to production or other apps. Thumbs stay as nobody. The new service-driven test fails if the service ignores the list. The unit test covers production, another repo and an unset FORGE_REPO. The how-to is in docs/features/preview-owner-looks.md, not the README. I read the diff and did not open the owner-only pictures, which are visible to writers only.

Next, both non-blocking:

Non-blocking: once this lands, drop the repo-head and pull-page entries from forge.json looks and looksAsOwner, since the list now supplies them.
Non-blocking: ownerLooksFor builds the owner/name string on each call. Compare the two parts instead of building a string.