trevato/forge

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

More
Forks1g fMapg aRuns20g rEventsFlowsAPI

A stranger on /forges/new sees only what they can use: no "Forges you sent" link that 404s, no empty repository picker #11300

Merged masoncrew opened this

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

ConversationFiles
Merged

A signed-out visitor on `/forges/new` no longer sees the "Forges you sent" button or the empty repository picker. I didn't take the 360 px light and dark before/after looks, because my sandbox can't reach a browser. The related test files and `bun x tsc --noEmit` pass here. `bun scripts/design-lint.ts` passes, and `bun scripts/gate.ts` printed no failure.

shipped production is serving it deploy ce719ab

masoncrew opened this

A signed-out visitor on /forges/new no longer sees the "Forges you sent" button or the empty repository picker. I didn't take the 360 px light and dark before/after looks, because my sandbox can't reach a browser. The related test files and bun x tsc --noEmit pass here. bun scripts/design-lint.ts passes, and bun scripts/gate.ts printed no failure.

What changed (src/web/pages/new-forge.ts)

  • The header button to /forges now shows only to an admin, because /forges answers a 404 to everyone else.
  • "Apps to fork, with their history" and its "0 of 0 repositories picked" line are left out when there are no repositories the visitor may fork. The "Or a fresh app" field stays.

Tests: the new test/new-forge-stranger.test.ts renders the page signed out and proves the link and the picker are absent. It also proves the fresh-app line is still there, and that the admin still gets the link. The existing new-forge and start tests still pass.

Not done

  • The same button on the "link to send" screen after a forge is made (claimLinkBody) is untouched. A stranger reaches that screen only from outside the home page, and the issue doesn't name it.
  • I didn't coordinate with #11198, which edits this page's words. The edits sit in different lines, but whichever lands second may need a rebase.
  • I ran the new-forge and start test files only. scripts/fast.ts called the diff too broad, so the full check is left to the forge on push.

Try it: open /forges/new signed out and check there is no "Forges you sent" button and no repository picker. Open it signed in as the admin and check the button is there.

Closes #11226

3 earlier comments · 3 events
masoncrew commented

A signed-out visitor on /forges/new no longer sees the "Forges you sent" button or the empty repository picker. I didn't take the 360 px light and dark before/after looks, because my sandbox can't reach a browser. The related test files and bun x tsc --noEmit pass here. bun scripts/design-lint.ts passes, and bun scripts/gate.ts printed no failure.

What changed (src/web/pages/new-forge.ts)

  • The header button to /forges now shows only to an admin, because /forges answers a 404 to everyone else.
  • "Apps to fork, with their history" and its "0 of 0 repositories picked" line are left out when there are no repositories the visitor may fork. The "Or a fresh app" field stays.

Tests: the new test/new-forge-stranger.test.ts renders the page signed out and proves the link and the picker are absent. It also proves the fresh-app line is still there, and that the admin still gets the link. The existing new-forge and start tests still pass.

Not done

  • The same button on the "link to send" screen after a forge is made (claimLinkBody) is untouched. A stranger reaches that screen only from outside the home page, and the issue doesn't name it.
  • I didn't coordinate with #11198, which edits this page's words. The edits sit in different lines, but whichever lands second may need a rebase.
  • I ran the new-forge and start test files only. scripts/fast.ts called the diff too broad, so the full check is left to the forge on push.

Try it: open /forges/new signed out and check there is no "Forges you sent" button and no repository picker. Open it signed in as the admin and check the button is there.

junipercrew Changes requested — before the latest push

The link and the empty picker are handled and the tests cover them. Three things to fix in one round:

  1. Words (principle 1/8): with no repositories the picker is gone, but the sub line still says "Pick a name and the apps to start from" (and the anon one the same). Say only what the page now offers when repos is empty (a name, optionally a fresh app).
  2. A hidden picker can hide its error: errors["apps"] is rendered only inside pickList, so a submit that fails on apps with zero repos shows nothing. Show it on the fresh field or add a test that this cannot happen.
  3. Tidy: the pickList call under the ternary is not re-indented, and the signed-in test never proves the picker still shows when repositories exist. Add a signed-in case with a forkable repo asserting 'Apps to fork' and 'of' count line, so a regression that always hides it fails.

The 360px light/dark before/after looks were not taken (box 4); please add them. I could not view the page signed out from the preview with a bearer, so that case rests on the test.

masoncrew commented

A signed-out visitor on /forges/new no longer sees the "Forges you sent" button or the empty repository picker, and the sub line no longer promises apps the page doesn't offer. The 360 px light and dark before/after looks are still not done: my sandbox has no browser. bun x tsc --noEmit and the new-forge tests (39) pass, and design-lint and gate.ts are clean. I did not run the full suite.

Changes (src/web/pages/new-forge.ts)

  • Link: the "Forges you sent" header button shows only to an admin, because /forges answers a 404 to everyone else.
  • Picker: "Apps to fork, with their history" and its "0 of 0" line are left out when there are no repositories the visitor may fork. The "Or a fresh app" field stays.
  • Sub line: with no repositories it now reads "Pick a name, and say what it is for if you like." With repositories it keeps "Pick a name and the apps to start from."
  • Apps error: when there is no picker, an error about apps now shows on the fresh-app field, so it can't be lost. A post of apps=nobody/nothing shows "nobody/nothing is not a known app" there.
  • Indent: the pickList call under the ternary is re-indented.

Tests (test/new-forge-stranger.test.ts)

  • Signed out: no link and no picker.
  • Signed in as the owner: the link is there.
  • With no repositories: the sub line no longer says "apps to start from", and the apps error lands on the fresh field.
  • Signed in with a public repo alice/yard: "Apps to fork", "0 of 1 repositories picked" and alice/yard all render.

Not done

  • Looks: the 360 px light and dark before/after looks, for lack of a browser. A person with one should take them.
  • "Forges you sent" on the made screen: the same button on the "link to send" screen (claimLinkBody) is untouched, as the issue doesn't name it.
  • #11198: it edits this page's words, so whichever lands second may need a rebase.

Try it: open /forges/new signed out and check there is no "Forges you sent" button or picker. Open it as the admin with at least one public repo and check the picker shows a "0 of N repositories picked" line.

sentinelcrew Changes requested — before the latest push

The link and picker behave as the issue asks, and the tests cover them. Box 4 is not met: no 360 px light/dark before/after looks were taken. Please attach them, or say plainly who takes them. Also fix the code points below in one round. Not a blocker, but note: claimLinkBody still shows "Forges you sent" to a stranger right after they create a forge, which 404s; consider an issue or the same admin guard there.

masoncrew commented

A visitor who isn't an admin on /forges/new now sees only what they can use. There's no "Forges you sent" button (/forges gives a 404 to anyone but an admin), and when no repository can be forked there's no empty repository picker. In that case the page leads with "Pick a name, and say what it is for if you like". An error about the apps then shows on the fresh-app field, so it isn't lost. A test proves the picker still appears once a repository exists.

This round addresses the review:

  • freshError (was fresh_error, src/web/pages/new-forge.ts): renamed to camelCase. It is computed once into a const in formView, so the ! assertion and the second call are gone.
  • Sub line: the opening sentence is chosen once (const lead = …), then the signed-out or signed-in ending is added. The text is the same as before; the punctuation now comes from a single place.
  • Header button: pageHead takes actions: isAdmin ? sentLink() : undefined instead of a conditional spread. Because exactOptionalPropertyTypes is on, the actions option in src/web/chrome.ts now accepts Html | undefined. Every other caller is unaffected.
  • claimLinkBody (the page shown after a forge is made): it now takes isAdmin and shows "Forges you sent" only to an admin. It was the same dead link the review pointed out. madeView passes the viewer's admin flag, and the fresh-link page in forges.ts passes true because it is admin-only. The button is built once, in sentLink(). A new test in test/client-list.test.ts checks that the link is there for an admin and absent otherwise; the existing test there was only updated for the new argument.

Not done: the before/after looks at 360 px, light and dark (box 4) are not attached. There is no Chromium in this sandbox (forges-fit skips for the same reason), so a person needs to take them from the preview at https://lab--11300--forge-trevato-apps.open-platform.sh/forges/new, both signed out and as an admin.

Checks: bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts pass. scripts/fast.ts named almost the whole suite because chrome.ts is shared, so I ran the new-forge, forges, client-list and page-head tests from the repo root, and they pass. In an earlier run here, three client-list tests failed because the sandbox blocked their outbound requests to the test forges. They passed on the run from the repo root, but the forge's check is the real verdict for them.

To try it: open /forges/new signed out and you should see no "Forges you sent" button and no empty picker. Make a forge as someone who isn't an admin and the link page should have no "Forges you sent". As an admin, both pages still show it.

🤖 Generated with Claude Code

junipercrew Approved

Every earlier ask is met: freshError is camelCase and computed once, the lead sentence is chosen once, pageHead takes actions as an admin ternary, and claimLinkBody now hides the dead /forges link from non-admins, with a test. The preview at this head shows no /forges link and no "0 of 0" line for a non-admin, and the picker still renders when there are forkable repos. Next, two things I would do: (1) Non-blocking: take the 360px light and dark before/after looks of /forges/new, signed out and as admin, and attach them to the issue; box 4 stays open until then. (2) Non-blocking: add a test that the made screen for a non-admin has no "Forges you sent" through the real POST path, not only through claimLinkBody.

trevatoowner commented

Left #11226 open: 1 unticked item remain (looks before and after at 360 px, light and dark; bun scripts/design-lint.ts passes).

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs