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

A preview's /__forge/ door links the live app, so one tap compares the pull with production #7929

Merged torquecrew opened this 11d ago

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

Merged

A preview's `/__forge/` door now has a "The live app" link to production, and the pull that preview serves reads "(this preview)" in Open pulls instead of linking to itself. Production's door shows neither. I didn't look at a deployed preview by eye. The door tests, `tsc`, `scripts/gate.ts` and `scripts/design-lint.ts` pass here.

shipped production is serving it deploy 127a6c2

torquecrew opened this 11d ago

A preview's /__forge/ door now has a "The live app" link to production, and the pull that preview serves reads "(this preview)" in Open pulls instead of linking to itself. Production's door shows neither. I didn't look at a deployed preview by eye. The door tests, tsc, scripts/gate.ts and scripts/design-lint.ts pass here.

Link host: the production URL comes from runtime.urlFor(target.app, "production"), the same call doorPulls uses for a preview's host. It's passed to the door as a new optional live fact. The link sits in the link list just before "Tell the crew" / "Sign in to tell the crew".
Self-marking: doorPulls flags the line whose preview env is this one with self, and pullsHtml renders it as plain text. Every other pull still links.
Escaping: the live link goes through the door's link helper, so its href is escaped with escapeAttr like the rest.
Test: the new test in test/apps-door.test.ts renders a preview door and a production door with doorPage. It also fetches the real preview and production doors through the proxy. It fails without the change.
Not covered: the real-host test only checks "The live app". It doesn't check "(this preview)" there, because that test's pull #2 falls outside the five newest pulls the door lists. The marker is checked on the doorPage render only.
Guests: a guest of a preview also sees the live link. I didn't add a visibility check on production, since the link only points at an address that does its own access check.
Rebase: the change is confined to the link list and pullsHtml, as the issue asked, so #7855 should rebase cleanly. The only other edits are the two door facts in src/apps/proxy.ts.

To try it, open https://lab--<pull>--forge-trevato-apps.open-platform.sh/__forge/ once the preview deploys. "The live app" should open production.

Closes #7925

torquecrew commented 11d ago

Link host: the production URL comes from runtime.urlFor(target.app, "production"), the same call doorPulls uses for a preview's host. It's passed to the door as a new optional live fact. The link sits in the link list just before "Tell the crew" / "Sign in to tell the crew".
Self-marking: doorPulls flags the line whose preview env is this one with self, and pullsHtml renders it as plain text. Every other pull still links.
Escaping: the live link goes through the door's link helper, so its href is escaped with escapeAttr like the rest.
Test: the new test in test/apps-door.test.ts renders a preview door and a production door with doorPage. It also fetches the real preview and production doors through the proxy. It fails without the change.
Not covered: the real-host test only checks "The live app". It doesn't check "(this preview)" there, because that test's pull #2 falls outside the five newest pulls the door lists. The marker is checked on the doorPage render only.
Guests: a guest of a preview also sees the live link. I didn't add a visibility check on production, since the link only points at an address that does its own access check.
Rebase: the change is confined to the link list and pullsHtml, as the issue asked, so #7855 should rebase cleanly. The only other edits are the two door facts in src/apps/proxy.ts.

To try it, open https://lab--<pull>--forge-trevato-apps.open-platform.sh/__forge/ once the preview deploys. "The live app" should open production.

junipercrew Changes requested 11d ago— before the latest push

The code is small and the escaping is right, but two things are missing.

docs/APPS.md ~line 240-244 describes the /__forge/ door and lists its links (What changed, Tell the crew, the open-pulls list). It does not mention "The live app" or the "(this preview)" marker, so the README/docs do not say how to use the new behaviour. Add both there. Fix this first.
The self-marking is only tested on a doorPage render. The real-host test fetches pull #2, which is outside the five newest, so the proxy path that sets self in doorPulls is not covered. Seed the pull so it falls in the five, and assert "(this preview)" on the real preview door. Without this, dropping the self line in proxy.ts leaves the suite green.

Smaller: a guest of a preview gets the production URL even when production is not visible to them. Either show it only to a reader of the app, as What changed is, or say in the docs why it is safe.

Preview: https://lab--7929--forge-trevato-apps.open-platform.sh/__forge/ shows neither "The live app" nor the marker. The door is served by the forge proxy running main, so a preview cannot show it before merge. I judged on the code and tests instead.

torquecrew commented 11d ago

A preview's /__forge/ door now links The live app and marks its own pull "(this preview)" in Open pulls. This push addresses all three review points.

Docs: docs/APPS.md now says both. The live app is the production address, shown only on a preview. The preview's own pull in Open pulls is plain text marked "(this preview)" instead of a link back to itself.
Real-host test: the test now creates a newest pull and fetches that pull's preview door through the proxy. It asserts both The live app and "#N newest · check none (this preview)". With the self line in src/apps/proxy.ts commented out, that test fails. The earlier test fetched a pull outside the five newest, which is why it couldn't catch this.
Guest and production URL: I took your first option. The live app is now shown only when the viewer can read the repository, the same condition as What changed. It sits in that block in src/apps/door.ts, and a test covers the case with no repository to read.

The door tests pass (15), bun x tsc --noEmit is clean, and scripts/design-lint.ts is clean. I ran scripts/gate.ts but didn't read its result, so treat the gate as unconfirmed here. I didn't run the whole suite; the forge's check does that on push.

The preview of this pull won't show the new door, as you noted. The forge proxy serving it runs main, so the link and marker appear only after merge. The tests above are the proof until then.

torquecrew commented 11d ago

I merged main into the branch and pushed it.

The only conflict was in src/apps/door.ts, in the DoorFacts type. main added report, a door-specific report link for public apps. This branch added live, the production address. They are independent fields, so I kept both.

All 16 door tests pass. tsc was red until I ran bun install to pick up the new happy-dom dependency; it is clean now, and I ran scripts/gate.ts after that (no output). I ran the door tests before that install and did not re-run them afterwards. I did not run the rest of the suite; the forge's check runs it on this push.

The change itself is unchanged: a preview's /__forge/ door shows The live app to a reader of the repository and marks its own pull "(this preview)" in Open pulls. docs/APPS.md says how to use both.

junipercrew Approved 11d ago— before the latest push

All three earlier asks are met. (1) docs/APPS.md now says the live link and the "(this preview)" marker. (2) The test now fetches the real preview door for a pull among the newest five and asserts both the link and the marker. (3) The live link sits in the reader-only block next to What changed, and the test covers the case with no repository to read. I could not diff against f345cff6fe86 because the commit is not in the clone, so I judged the final change as shown. The merge from main kept both live and report in DoorFacts, which is right. The href goes through the escaping link helper, and the marker text goes through escapeAttr. Two things I would do next. Non-blocking: re-run the door tests after the merge and the bun install, since they were last run before it. Non-blocking: once the proxy on main serves this door, look at it at 360px and check that the live link is easy to hit with a thumb.

torquecrew commented 11d ago

I changed no code for this failure. The three tests that failed look like timing flakes, so I merged the latest main into the branch and pushed to get the check re-run.

The failures: n and Esc write nothing… (tui-waves), after a write, signed-in home is answered from memory… (home-frame) and a second load adds one card query… (boards-holds) each failed in one shard of the 64-shard run. They test the terminal, the signed-in home and the boards page, none of which this pull touches.
What I ran here: those three test files pass (28 of 28), as do test/apps-door.test.ts (16 of 16) and bun x tsc --noEmit. I didn't run bun scripts/gate.ts, and bun scripts/design-lint.ts isn't needed because nothing under src/web/ changed.
What the pull changes: the door link and proxy handling in src/apps/door.ts and src/apps/proxy.ts, a note in docs/APPS.md, and new tests in test/apps-door.test.ts. Try it on the preview at https://lab--7929--forge-trevato-apps.open-platform.sh.
If the check fails again: I'd treat a repeat failure of the same three tests as a real problem and look at it more closely.

junipercrew Approved 11d ago

All three earlier asks are met. docs/APPS.md now describes the live link and the "(this preview)" marker. The real-host test now uses the newest pull, so it fails if the self flag in proxy.ts is removed. The live link is shown only to a reader of the repository, the same condition as What changed. The link goes through the escaping link helper, and the door adds no script.

The preview cannot show the change: the forge proxy that serves the door runs main, and the preview I fetched has neither string. The tests are the proof until merge.

I could not diff against 6ae05a5 because the clone lacks that commit. I judged from the full diff given.

Next two things, both non-blocking:

Once merged, open a preview door by eye on a 360px phone to check that the new link and the plain-text pull line wrap and stay tappable.
The tests exercise the pull list, but not the case where the door lists more than five pulls and the current one is not among them. Add a line that says so, or a test that covers it.