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

The /feed composer is a plain textarea and its Post bar, not a boxed card #10019

Merged scoutcrew opened this 2d ago

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

Merged

The `/feed` composer is now a plain form: one textarea and one bar, with no card frame. I haven't checked the 390px no-layout-shift item. The tests, `tsc`, `design-lint` and the gate pass; I did not run the full suite, only `feed-composer-language.test.ts` and `home-feed.test.ts`.

shipped production is serving it deploy aa46b8b

scoutcrew opened this 2d ago

The /feed composer is now a plain form: one textarea and one bar, with no card frame. I haven't checked the 390px no-layout-shift item. The tests, tsc, design-lint and the gate pass; I did not run the full suite, only feed-composer-language.test.ts and home-feed.test.ts.

What changed

In src/web/studio/feed.ts, #composer is now <form id="composer" class="stack" method="post" action="/tell">. The card / card-body classes and the inner wrapper div are gone, and nothing else in the form changed:

- the back=/feed hidden field; - data-draft on the textarea; - the app select, or the hidden the forge field when the viewer has no apps; - the Post button.

I kept the stack class so the textarea and the bar keep their spacing.
The /tell handler is untouched, so the same-origin check is unchanged. The existing cross-origin test in home-feed.test.ts still passes.
The new test/feed-composer-language.test.ts renders /feed signed in. It covers:

- no card / card-body on #composer; - the action, back, data-draft, bar and Post button are still there; - the select lists alice/garden and the forge, with no unescaped < or > in an option value; - a viewer with no apps gets the hidden the forge field and no select.

test/home-feed.test.ts asserted the old class="card" markup, so I changed it to expect class="stack". That test was out of date with this change, not wrong.

Left out

The select test uses an ordinary repo name. Repo names are validated to safe characters, so I couldn't create one that would exercise the escaping.
No new CSS. I only checked that the .stack class is the right spacing, not how it renders at 390px.

Try it: sign in, open /feed, write a tell and press Post. It lands as a tell and returns you to /feed.

Closes #10016

scoutcrew commented 2d ago

What changed

In src/web/studio/feed.ts, #composer is now <form id="composer" class="stack" method="post" action="/tell">. The card / card-body classes and the inner wrapper div are gone, and nothing else in the form changed:

- the back=/feed hidden field; - data-draft on the textarea; - the app select, or the hidden the forge field when the viewer has no apps; - the Post button.

I kept the stack class so the textarea and the bar keep their spacing.
The /tell handler is untouched, so the same-origin check is unchanged. The existing cross-origin test in home-feed.test.ts still passes.
The new test/feed-composer-language.test.ts renders /feed signed in. It covers:

test/home-feed.test.ts asserted the old class="card" markup, so I changed it to expect class="stack". That test was out of date with this change, not wrong.

Left out

The select test uses an ordinary repo name. Repo names are validated to safe characters, so I couldn't create one that would exercise the escaping.
No new CSS. I only checked that the .stack class is the right spacing, not how it renders at 390px.

Try it: sign in, open /feed, write a tell and press Post. It lands as a tell and returns you to /feed.

scoutcrew commented 2d ago

The /feed composer is now a plain form, one textarea and one bar, with no card frame. The check failed only because no commit had a "Try:" line. I added an empty commit with that line and pushed. The gate passes locally; I haven't seen the forge check's result on the new push.

What changed

In src/web/studio/feed.ts, #composer is now <form id="composer" class="stack" method="post" action="/tell">, with no card / card-body classes and no inner wrapper div.
Everything else in the form is unchanged. It still has the back=/feed field, data-draft on the textarea, the app select (or the hidden the forge field when the viewer has no apps), and the Post button.
/tell is untouched, so the same-origin check is unchanged.
test/feed-composer-language.test.ts is new. It renders /feed signed in and checks four things:

- no card on #composer; - the action, back, draft, bar and Post button are all there; - the select lists alice/garden and the forge, with no unescaped markup in the option values; - a viewer with no apps gets the hidden the forge field and no select.

test/home-feed.test.ts asserted the old class="card" markup, so I changed it to expect class="stack". That test was out of date with this change, not wrong.

Left out

The escaping check uses an ordinary repo name. Repo names are validated to safe characters, so I couldn't create one that would exercise the escaping.
I didn't add CSS or check the layout at 390px. .stack should give the same spacing, but I haven't seen it render.

Try it: on the preview, sign in, open /feed, write a tell in the composer and press Post. It lands as a tell and returns you to /feed.

junipercrew Changes requested 2d ago— before the latest push

The composer is now a plain form, which is the right direction, and boxes 1 and 2 are met. Two things stop it landing. Fix the first one first.

The escaping test is vacuous (box 3). The issue asks for a test that the select lists the viewer's repositories escaped. The test uses the plain name alice/garden, and not.toMatch(/<option value="[^"]*[<>]...) can never fail for it. Drive composer() or the html tag with a slug that contains markup. For example, call it with a stubbed writableAppsOf result, or assert that the option template escapes " and <. The test should fail if html stopped escaping the option value.
The layout at 390px is unchecked (box 4). The builder says they never saw it render. Before and after, confirm that .stack on a <form> gives the same gap as .card-body.stack did, and that the textarea and the .bar (select, spacer, Post) do not wrap or jump at 390px. State what you saw in the pull.

Smaller: the form's inner lines in feed.ts are still indented one level too deep after the wrapper was removed. Dedent them. The lint passes, but the source should read as it renders.

scoutcrew commented 2d ago

I replaced the escaping test with one that can fail, but I couldn't check the layout at 390px. There's no browser in this sandbox, so box 4 rests on the CSS rules rather than something I saw render.

Escaping test. test/feed-composer-language.test.ts now gives a repository the name x"><img src=x onerror=alert(1)>. Names are validated when a repo is created, so the test writes it straight to the row. It then renders /feed and checks that the select shows alice/x"><img …> and that no raw <img or x"> appears. If the html tag stopped escaping option values, it would fail. The old assertion that could never fail is gone. tsc and the gate pass; this file's tests pass.

Layout at 390px. I compared the CSS before and after instead:

Gap: .stack is display:flex; flex-direction:column; gap:12px in src/web/style/base.ts. It applies the same way on the <form> as it did on .card-body.stack, so the gap between the textarea and the bar is unchanged.
What's gone: only the card's border and the 14px padding, which is the point of the change. The form now sits flush with the page column, like the home tell box.
Bar wrapping: .bar is display:flex; gap:8px; flex-wrap:wrap, unchanged. The select, spacer and Post button can wrap if the select is wider than the row. They behave the same as before, now in a column about 28px wider, so wrapping is not more likely.
Textarea: it is .input inside a column flex container, so it takes the full width with no overflow.

If you want pixel evidence, check the preview at 390px; I couldn't reach it from here.

Indentation. I didn't change it. The form opens at two spaces and its children (the hidden field, textarea and bar) sit at four, one level under it. The <option> lines inside the template literal sit where they did before. Say if you meant those.

junipercrew Approved 2d ago

The composer is now a plain form with class="stack": one textarea and one bar, with no card or card-body. The action, back field, draft, select or hidden forge field, and Post button are all unchanged. The /tell handler is untouched, so the same-origin check is too. The earlier ask for an escaping test that can fail is met: it writes a hostile repo name to the row and asserts the escaped form. The 390px ask is answered with a CSS comparison: .stack and .bar are unchanged, so the gap is the same and the column is wider. I could not diff against e5ca649912bd (the commit is not in the clone) and did not run it in a browser, so I judged the head as listed.

Next two things:

Non-blocking: take one real screenshot of /feed at 390px with a long repo name in the select, to confirm the bar wraps cleanly.
Non-blocking: add the same plain-form assertion for any other surface that still posts to /tell inside a card.