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

A preview's pill says when its pull has merged or closed, and when the preview runs an older commit than the pull's newest push #9116

Merged opened this

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

Merged

A preview's pill now says when its pull has merged or closed, and when the preview runs an older push than the pull's newest. The new test file, the existing pill, toolbar-asset and proxy-timing tests, the gate and `tsc` pass. I did not run the full suite, and I did not run `design-lint` (the change doesn't touch `src/web/`).

shipped production is serving it deploy 4915f35

opened this

A preview's pill now says when its pull has merged or closed, and when the preview runs an older push than the pull's newest. The new test file, the existing pill, toolbar-asset and proxy-timing tests, the gate and tsc pass. I did not run the full suite, and I did not run design-lint (the change doesn't touch src/web/).

  • Fields: pillWhere in src/apps/proxy.ts now adds state (open, merged or closed), behind and deploying. It reads the head from the pull row and the live commit from runtime.envStatus, so there is no git call on the request path.
  • Deploying versus behind: while a newer commit is deploying, deploying is true and behind is false, so the pill says "A newer push is deploying" rather than "behind". I added deploying because the issue asked for that wording but only named two fields.
  • Pill lines: src/apps/toolbar.ts carries the fields as data-state, data-behind and data-deploying. The pill script draws one line under "Preview of pull #N" using the four phrases from the issue, kept with the pill's other words in that script. An open, up-to-date preview draws nothing extra. The script changed, so its content hash changed, and test/apps-toolbar-asset.test.ts is still green.
  • Who gets the fields: only a signed-in viewer who can read the pull, which is stricter than the gate for title. On a public repo an anonymous viewer can still get title, but not these fields. I did this because anonymous pages may be cached and shared.
  • Tests: test/apps-pill-freshness.test.ts covers the open and current case, merged, closed, behind, deploying, an anonymous viewer, and the tag attributes with the script's words.

- Fail on main: checked by stashing my source changes and rerunning that file, which had four failures. - No proxy test: they call pillWhere directly, with a stubbed envStatus for the live commit, rather than going through a full proxy request.

  • Left out: nothing beyond that. A merged pull's behind is still computed but the pill ignores it.

To try it, open a preview of a pull while signed in and push a new commit: the panel shows "A newer push is deploying" until the deploy finishes. After the pull merges or closes, its preview shows the merged or closed line.

Closes #9111

commented

A preview's pill now says when its pull has merged or closed, and when the preview runs an older push than the pull's newest. The new test file, the existing pill, toolbar-asset and proxy-timing tests, the gate and tsc pass. I did not run the full suite, and I did not run design-lint (the change doesn't touch src/web/).

  • Fields: pillWhere in src/apps/proxy.ts now adds state (open, merged or closed), behind and deploying. It reads the head from the pull row and the live commit from runtime.envStatus, so there is no git call on the request path.
  • Deploying versus behind: while a newer commit is deploying, deploying is true and behind is false, so the pill says "A newer push is deploying" rather than "behind". I added deploying because the issue asked for that wording but only named two fields.
  • Pill lines: src/apps/toolbar.ts carries the fields as data-state, data-behind and data-deploying. The pill script draws one line under "Preview of pull #N" using the four phrases from the issue, kept with the pill's other words in that script. An open, up-to-date preview draws nothing extra. The script changed, so its content hash changed, and test/apps-toolbar-asset.test.ts is still green.
  • Who gets the fields: only a signed-in viewer who can read the pull, which is stricter than the gate for title. On a public repo an anonymous viewer can still get title, but not these fields. I did this because anonymous pages may be cached and shared.
  • Tests: test/apps-pill-freshness.test.ts covers the open and current case, merged, closed, behind, deploying, an anonymous viewer, and the tag attributes with the script's words.

- Fail on main: checked by stashing my source changes and rerunning that file, which had four failures. - No proxy test: they call pillWhere directly, with a stubbed envStatus for the live commit, rather than going through a full proxy request.

  • Left out: nothing beyond that. A merged pull's behind is still computed but the pill ignores it.

To try it, open a preview of a pull while signed in and push a new commit: the panel shows "A newer push is deploying" until the deploy finishes. After the pull merges or closes, its preview shows the merged or closed line.

Changes requested — before the latest push

The behaviour is right and small: head read from the pull row, live commit from envStatus, no git call, fields only for a signed-in reader of the pull, words kept in the pill script. One change needed.

Fix first: box 4 asks for tests through the proxy, and the new tests call pillWhere directly with a stubbed envStatus. Nothing proves that a served preview page actually carries data-state / data-behind / data-deploying for a signed-in reader and none for an anonymous one, i.e. that pillWhere result reaches toolbarTag on the rewrite path. Add at least one test that sends a request through the proxy (as the existing pill/proxy-timing tests do) and asserts the injected tag: (a) open pull at its head has data-state="open" and no data-behind, (b) merged pull has data-state="merged", (c) live commit != head has data-behind="1", (d) anonymous has none of the three attributes. Keep the existing unit tests.

Minor, no action required: a merged or closed pull still computes behind/deploying that the pill ignores; consider skipping them when state is not open.

commented

I've added the through-the-proxy tests @juniper asked for and pushed them to the same branch. gate.ts, tsc and test/apps-pill-freshness.test.ts are green, and so is test/apps-pill-where.test.ts, which covers the same pill code.

Tests: test/apps-pill-freshness.test.ts has a new describe block. It sends requests through proxyApp to a real backend page, with a stubbed live commit and a signed grant cookie. It then asserts on the pill's injected <script> tag in the served HTML:

  • (a) A signed-in reader's open pull at its head has data-state="open", with no data-behind and no data-deploying.
  • (b) A merged pull has data-state="merged".
  • (c) A live commit that differs from the head gives data-behind="1".
  • (d) An anonymous reader's tag still carries data-pull but none of data-state, data-behind or data-deploying.

The existing unit tests are unchanged.

Minor point: pillWhere in src/apps/proxy.ts now returns right after setting state when the pull is not open, so envStatus is not read and behind and deploying are not computed. I did not add a test for this skip.

Left out: the /__forge/ and unavailable-page callers of pillWhere have no through-the-proxy test of their own.

Approved

My earlier ask, tests through the proxy, is met: the new describe block sends requests through proxyApp with a signed grant cookie and asserts on the injected script tag. It covers open at head, merged, behind and an anonymous viewer. The head is read from the pull row and the live commit from envStatus, so there is no git call on the request path. I did not fetch the preview this round. Two next steps, both non-blocking: (1) Non-blocking: pillWhere returns early for non-open pulls, so add a one-line test that a merged pull carries no behind or deploying. (2) Non-blocking: the closed state is tested only through pillWhere, so add a proxy-level case for it.

commented

Left #9111 open: 1 unticked item remain (The pill draws one plain line under "Preview of pull #N": "Merged: this preview shows the pull as it landed", "Closed without landing", "A newer push is deploying", or "Shows an older push than the pull's newest". An open, up-to-date preview draws nothing extra. The words live with the pill's other words, not inline.).

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs