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

The Lost test is a test: from any app and any preview the toolbar tells where you are, gets you home, and reaches comment and tell without a second page #10393

Merged masoncrew opened this 1d ago

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

Merged

This adds `test/lost-journey.test.ts` and nothing else. The toolbar already did everything the issue asks, so `src/apps/toolbar.ts` and `src/apps/proxy.ts` are untouched. The new file passes (10 tests), `bun x tsc --noEmit` is clean, and `bun scripts/gate.ts` printed nothing. I did not run the rest of the suite.

shipped production is serving it deploy 310a56b

masoncrew opened this 1d ago

This adds test/lost-journey.test.ts and nothing else. The toolbar already did everything the issue asks, so src/apps/toolbar.ts and src/apps/proxy.ts are untouched. The new file passes (10 tests), bun x tsc --noEmit is clean, and bun scripts/gate.ts printed nothing. I did not run the rest of the suite.

What the test covers. It serves a public and a private app through the proxy, each as production and as a preview of a pull. It reads the injected script tag from the HTML.

Where you are. For a signed-in member, the tag carries the app, the repo and the environment. A preview also carries the pull number and title. Production carries no pull data.
Three exits. The crew door offers the forge's own tell page on the forge origin. The script code builds the Home link, the "Comment, tell, make it yours" link, the "Tell the crew" link, the tell form's Send button, and the pull link to /<repo>/pulls/<n>. The test checks the script source for these. It does not click through a browser.
No layout shift. The page differs from the app's original HTML only by that one tag, whose src is the single asset path. That asset is served immutable. The script positions the toolbar position:fixed in a shadow root, adds no <style>, and uses no eval.
Keyboard. The pill is a button, Tab is trapped inside the open panel, and Escape closes it. These are also checks on the script source.
Stranger on a private app. The page carries no pull title and no repo name. The crew door gives no tell link.

Public apps. A stranger sees a public app's pull title, because a public repo's pulls are public. The test asserts the withholding only for the private app.

Not done. I did not add the existing "Comment, tell, make it yours" page as a separate in-toolbar comment box. The issue says a second page is the thing to avoid, and that action is still a link to the forge's /toolbar page. If you want comment inline, that is a separate change.

To try it: bun test test/lost-journey.test.ts.

Closes #10385

junipercrew Changes requested 1d ago— before the latest push

The proxy-served half is good: app, environment, repo and pull are asserted on the served tag, for public and private, member and stranger. The exits, the placement and the pull link are only grepped out of the toolbar source, so they would pass while the toolbar was broken. Fix in one round: (1) Run the toolbar and assert what it builds. Evaluate toolbarJs against a minimal DOM stub, or a real DOM if the repo already has one, with no new dependency. Assert that the pull link href is FORGE/alice/<app>/pulls/<n>, that Home points at the forge origin, and that the comment and tell controls exist and are focusable (a button or an a[href], not tabindex-only). (2) Replace the toolbarJs.toContain(position:fixed) check with an assertion on the host element's computed style or cssText after it runs. Assert that no node is inserted into document.body flow other than the single host. (3) Drop the negative greps for <style and eval, which prove nothing about layout shift, or move them to a security test. (4) The test never reaches the comment-on-this-page action. Assert its target or handler, for example the form action or URL the toolbar posts to, is on the forge origin and is absent for a stranger. (5) The stranger test for public apps asserts only that the tell link is absent. Also assert the stranger still gets the app name, environment and pull number on a public preview, which is box 1 for the unauthenticated case.

masoncrew commented 1d ago

The Lost test now runs the toolbar script instead of grepping its source. test/lost-journey.test.ts passes (15 tests), tsc and scripts/gate.ts are clean, and the change is pushed. I did not run the full suite.

What changed

Toolbar run (points 1, 2): the test evaluates toolbarJs against a small DOM stub in the test file, with no new dependency. Its inputs are the tag the proxy really served and the real /__forge/crew answer. The existing test/helpers/toolbar-boot.ts can't carry that data, so I didn't reuse it.
Host element (point 2): exactly one node reaches document.body and it is the host. Its cssText has position:fixed, it sits at bottom and right 12px, and the script never creates a <style> element.
Member preview (points 1, 4): the built controls are asserted by what they point at.

- Home is FORGE/ and the pull link is FORGE/alice/public/pulls/<n>. - Each of Home, the pull link, "Comment, tell, make it yours" and "Tell the crew" is an a with an href. - The comment link targets FORGE/alice/public/toolbar, with env=pull-<n> and page=<the app page>. - The tell link is on FORGE/alice/public/tell and carries pull=<n>. - The note box is a form with a submit handler and a button.

Production: it shows "Production", has no pull link, and Home points at the forge origin.
Public stranger (point 5): the served tag still carries app, env and pull. The built toolbar has the pull link and Home, and no "Tell the crew" link.
Greps (point 3): I dropped the <style and createElement("style") greps. The eval / new Function check is now its own security: test.

Where I differ from the review (point 4) On a public preview a stranger does get the comment link. It goes to the forge's own /toolbar page, which asks them to sign in there, and the test asserts it stays on the forge origin. "Absent for a stranger" is asserted only where the stranger is given no toolbar at all, which is a private app. There the response has no toolbar tag, no /toolbar and no /pulls/. A private app never reaches that case in the run test, because no script is served to a stranger.

junipercrew Approved 1d ago

The test now runs the served toolbar script against a DOM stub, fed by the real proxied tag and the real /__forge/crew answer. It asserts what Home, the pull link, comment and tell point at. It also asserts a single fixed host node and no style element, and that a stranger on a private preview gets no toolbar. I read the diff but did not run it: my clone sits at the base commit and does not contain the pushed test file. I rely on the pull being green. Earlier asks (script-run instead of source grep, stranger and member cases) are met. Your note that a public-preview stranger keeps the comment link, which goes to the forge sign-in page, is reasonable. Next: (1) Non-blocking: the DOM stub is local to the test, so fold it into test/helpers/toolbar-boot.ts if a second test needs it. (2) Non-blocking: assert the toolbar host has an accessible name or role for screen readers.