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

The New forge form offers the size, records it only when it is not the default, and shows an unknown size beside the field #5683

Merged opened this

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

Merged

The New forge form now has an optional size select. Small is preselected and sends nothing, so the plan stays as it is today. The four new tests plus `test/new-forge.test.ts`, `test/ssh-site.test.ts` and `test/spawn-fresh-forks.test.ts` pass (29 tests), `tsc --noEmit` is clean and `design-lint` passes. I did not run the whole suite.

shipped production is serving it deploy d79f016

opened this

The New forge form now has an optional size select. Small is preselected and sends nothing, so the plan stays as it is today. The four new tests plus test/new-forge.test.ts, test/ssh-site.test.ts and test/spawn-fresh-forks.test.ts pass (29 tests), tsc --noEmit is clean and design-lint passes. I did not run the whole suite.

  • Form: /forges/new (src/web/pages/new-forge.ts) has a "Size (optional)" select with small, medium and large, each with its one-line description from sizesView(). It is built with selectField unchanged.
  • Recording: medium or large is passed to the plan and ends up in the child's parent.json.
  • Unknown size: a posted unknown size re-renders the form with a 422. The message "size must be small, medium or large" sits in a sibling <span class="error" id="f-size-err"> after the select, and every other value is kept. The size is validated in planSpawn, after the other field checks.
  • Tests: test/new-forge-size.test.ts covers the four cases.

- Select: present with small chosen. - Small: the post carries no size. - Medium: it reaches parent.json. - Unknown size: a 422 with the message and the kept values. - How "no size" is checked: it spies on spawnDoor's arguments, because the plan page never prints the plan's size.

I did not attach the before-and-after screenshot of the form that the issue asks for; this sandbox can't take one.

To try it, open /forges/new, pick medium, press See the plan, then Spawn.

Closes #5680

commented

The New forge form now has an optional size select. Small is preselected and sends nothing, so the plan stays as it is today. The four new tests plus test/new-forge.test.ts, test/ssh-site.test.ts and test/spawn-fresh-forks.test.ts pass (29 tests), tsc --noEmit is clean and design-lint passes. I did not run the whole suite.

  • Form: /forges/new (src/web/pages/new-forge.ts) has a "Size (optional)" select with small, medium and large, each with its one-line description from sizesView(). It is built with selectField unchanged.
  • Recording: medium or large is passed to the plan and ends up in the child's parent.json.
  • Unknown size: a posted unknown size re-renders the form with a 422. The message "size must be small, medium or large" sits in a sibling <span class="error" id="f-size-err"> after the select, and every other value is kept. The size is validated in planSpawn, after the other field checks.
  • Tests: test/new-forge-size.test.ts covers the four cases.

- Select: present with small chosen. - Small: the post carries no size. - Medium: it reaches parent.json. - Unknown size: a 422 with the message and the kept values. - How "no size" is checked: it spies on spawnDoor's arguments, because the plan page never prints the plan's size.

I did not attach the before-and-after screenshot of the form that the issue asks for; this sandbox can't take one.

To try it, open /forges/new, pick medium, press See the plan, then Spawn.

Changes requested — before the latest push

The page works in the preview: the select renders server-side with small chosen, and medium or large reaches the plan. Two things block it.

  1. Both doors (fix first). The terminal form src/tui/screens/newforge.ts has no size field, so a person on the terminal cannot choose a size. Add it there through the same spawnDoor call, with the same rule (small is not sent), and add a test. The README paragraph on the terminal New forge (around line 456) and the one on spawn size (line 606) should say the form offers it.
  1. The error is not wired like the design system's field. The message is a loose <span class="error"> outside the .field div. The select has no aria-describedby or aria-invalid, and the field has no data-invalid. A screen reader user is never told which field failed. selectField takes no error option. Extend it the way formField does (describedby, aria-invalid, data-invalid, error span inside the field) and pass the error through, instead of hand-placing markup next to it.

Minor: the labels read "small: For a notes app..." with a capital after the colon. Test the 422 field wiring once item 2 is done.

commented

Both blockers are fixed and pushed. The size tests, test/tui-newforge.test.ts, test/new-forge.test.ts, test/ssh-site.test.ts and test/spawn-fresh-forks.test.ts pass, tsc --noEmit is clean and design-lint passes. I didn't run the whole suite.

  • Terminal form: src/tui/screens/newforge.ts has a "Size" field after "Follows this forge's code". It goes through the same spawnDoor call with the page's rule: empty or small sends nothing, medium or large is sent. An unknown size shows the door's refusal beside the field. Two new tests in test/tui-newforge.test.ts: medium reaches parent.json, and an unknown size is refused with the message on screen. There is no terminal test that small sends no size; the page test covers that rule.
  • selectField error: it now takes error the way field does. data-invalid goes on the .field div, and the select gets aria-invalid and aria-describedby="f-size-hint f-size-err". The error span sits inside the field, and the hint now has an id. The hand-placed span is gone from new-forge.ts. The 422 test checks all of this wiring.
  • Label case: the labels now read "small: for a notes app…".
  • README: the terminal New forge paragraph and the spawn-size paragraph say both forms offer it.
  • selectField callers: I didn't change the select's markup for callers with no hint or error beyond adding the hint id. test/ssh-site.test.ts still passes, as do the other callers' tests that ran. I changed one assertion in my own test/new-forge-size.test.ts, because it matched the old exact <select> tag.

I still haven't attached the before-and-after screenshot of the form; this sandbox can't take one.

commented

I fixed the two failing boards tests and pushed. I did not run the full suite: my full run timed out and gave no result. The boards, size, terminal New forge, ssh-site, web-ui, studio-shipping and webspec tests pass locally.

The cause was the selectField change from the review round, which test/boards-page.test.ts matches on exactly.

  • Select tag: the <select> opening tag had been reflowed onto several lines, which broke the check that the chart select has id then name. I put it back as <select class="input" id=… name=…, with the aria attributes on the next line, so that test passes unchanged.
  • Hint id: selectField now gives the hint an id so aria-describedby can point at it. The "Column to aggregate" hint check in the same file expected the bare <span class="hint">. I updated that one assertion, because it was over-specific; nothing else about the hint changed.

I also adjusted the select check in test/new-forge-size.test.ts to allow the new line break.

Approved

Works in the preview: /forges/new serves the Size select with small selected, hint linked by aria-describedby, built with the existing selectField (no new class, no JS). Small sends nothing so plans hash as before; medium/large reach parent.json; an unknown size is a 422 beside the field with values kept. The terminal form has the same field and rule, README covers both, and tests cover each behaviour. Two things next: (1) attach the before/after screenshot the issue asked for once someone can take one; (2) the new size select adds a long option label on a 360px phone - check it does not wrap or overflow, and consider shortening the label to the size name with the description in the hint. Note the selectField change touches other callers (hint id), fine and tested.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs