From ed06e78d2e642d9246040dd108a1878650c09692 Mon Sep 17 00:00:00 2001 From: Jonathan Burdge Date: Fri, 9 Oct 2026 12:58:17 +0300 Subject: [PATCH] Make development previews visually distinct --- docs/development.md | 2 ++ scripts/pages_preview.py | 53 ++++++++++++++++++++++++++++++++--- tests/test_pages_preview.py | 49 ++++++++++++++++++++++++-------- tests/test_public_browser.mjs | 15 ++++++++++ 4 files changed, 104 insertions(+), 15 deletions(-) diff --git a/docs/development.md b/docs/development.md index 20c85aa..feb6471 100644 --- a/docs/development.md +++ b/docs/development.md @@ -125,6 +125,8 @@ The publisher deploys one combined site: - `/quickdash/pr-preview/pr-/` serves that PR’s latest published successful build. - `/quickdash/pr-preview/` lists open PRs, preview links, and the exact built commits. It labels a retained preview as **Previous successful build** when the current PR head has no successful build yet. +Preview dashboards and demos have a `[DEV PREVIEW] PR #` browser title, a persistent amber banner linking the PR and main dashboard, and a warm-colored header. The trusted publisher adds this presentation, including to retained previews whose artifacts have expired. Production keeps its original appearance. These visual cues identify a review page; they do not isolate its JavaScript from the shared Pages origin. + After deployment succeeds, the publisher creates or updates one bot comment in each PR conversation with a prominent **Open preview** link and the built commit, or the owner-approval instructions when deployment is blocked. It reuses the comment without writing when the text is unchanged, and only edits comments owned by `github-actions[bot]` with the publisher’s hidden marker. A preview from an older successful build is labelled explicitly. Closing or merging a PR removes its directory and changes its existing comment to **Preview closed** after deployment succeeds; pending comment cleanup persists across failed deployments and retries. Reopening the PR lets the next successful deployment reuse the comment. A **Pages preview** check on each current authorized, built PR commit also includes an **Open preview** link in its summary. A failed main build preserves production; a failed PR build preserves its previous authorized preview. Review both the ordinary `build` check and the built commit before assessing a preview. Previews are public and share the Pages origin with the main dashboard. Authorizing a preview means trusting its browser code: read-only data does not stop JavaScript from misleading users or reading same-origin browser storage and files a user imports into that page. The authorization gate is not a browser sandbox. The trusted publisher has `pull-requests: write` for comments and acknowledgment reactions; PR build jobs retain read-only permissions. diff --git a/scripts/pages_preview.py b/scripts/pages_preview.py index 06607a2..081904f 100644 --- a/scripts/pages_preview.py +++ b/scripts/pages_preview.py @@ -165,8 +165,45 @@ def download(self, candidate): f"repos/{self.repository}/actions/artifacts/{candidate['artifact_id']}/zip"]) -def install(site, candidate, source): +def preview_html(data, pr): + """Apply the current preview appearance to new or retained static pages.""" + number = int(pr["number"]) + page = data.decode("utf-8") + marker = "quickdash-dev-preview" + page = re.sub(f".*?", "", page, flags=re.S) + page, titles = re.subn(r"(]*>)(?:\[DEV PREVIEW\] PR #\d+ · )?", + lambda match: f"{match[1]}[DEV PREVIEW] PR #{number} · ", + page, count=1, flags=re.I) + if titles != 1: + raise ValueError("Preview HTML needs a document title") + banner = (f'') + # Escape '<' in the JSON string so embedded markup cannot end the script. + encoded = json.dumps(banner).replace("<", "\\u003c") + decoration = ''''''.replace("BANNER", encoded) + decoration = f"{decoration}" + return re.sub(r"", lambda match: match[0] + decoration, + page, count=1, flags=re.I).encode("utf-8") + + +def install(site, candidate, source, pr=None): files = read_bundle(source.download(candidate)) + if pr: + files = {name: preview_html(data, pr) if name.endswith(".html") else data + for name, data in files.items()} site.mkdir(parents=True, exist_ok=True) for name, data in files.items(): (site / name).write_bytes(data) @@ -213,12 +250,20 @@ def reconcile(site, source): if previous and not source.authorization(pr, previous["sha"])["allowed"]: shutil.rmtree(preview_dir / ("pr-" + number)) del state["previews"][number] - if not access["allowed"]: - continue try: + if number in state["previews"]: + # Refresh retained previews too, without depending on their + # build artifacts still being available or a new PR commit. + paths = [preview_dir / ("pr-" + number) / name for name in ("index.html", "demo.html")] + decorated = [preview_html(path.read_bytes(), pr) for path in paths] + for path, data in zip(paths, decorated): + if path.read_bytes() != data: + path.write_bytes(data) + if not access["allowed"]: + continue candidate = source.latest_build(pr) if candidate and not same_build(state["previews"].get(number), candidate): - state["previews"][number] = install(preview_dir / ("pr-" + number), candidate, source) + state["previews"][number] = install(preview_dir / ("pr-" + number), candidate, source, pr) except ValueError as error: # A malformed PR bundle must not prevent production/other PRs publishing. warnings.append(f"PR #{number}: {error}") diff --git a/tests/test_pages_preview.py b/tests/test_pages_preview.py index 1bf10e1..10e9201 100644 --- a/tests/test_pages_preview.py +++ b/tests/test_pages_preview.py @@ -14,6 +14,10 @@ from quickdash.io import parse_yaml +def page(label, name="index.html"): + return f"{label}
{label}:{name}
".encode() + + def archive(label="dashboard", extra=None, files=None): tar = io.BytesIO() with tarfile.open(fileobj=tar, mode="w") as out: @@ -21,7 +25,7 @@ def archive(label="dashboard", extra=None, files=None): root.type = tarfile.DIRTYPE out.addfile(root) for name in pages.SITE_FILES: - data = (files or {}).get(name, (label + ":" + name).encode()) + data = (files or {}).get(name, page(label, name) if name.endswith('.html') else (label + ":" + name).encode()) member = tarfile.TarInfo("./" + name) member.size = len(data) out.addfile(member, io.BytesIO(data)) @@ -64,6 +68,29 @@ def authorization(self, pr, sha=None): class PagesPreview(unittest.TestCase): + def test_preview_decoration_is_visible_idempotent_and_preserves_content(self): + original = page('Dashboard & demo') + rendered = pages.preview_html(original, pull(11)).decode() + self.assertIn('[DEV PREVIEW] PR #11 · Dashboard & demo', rendered) + self.assertIn('DEV PREVIEW', rendered) + self.assertIn('https://github.com/org/repo/pull/11', rendered) + self.assertIn('Main dashboard', rendered) + self.assertIn('
Dashboard & demo:index.html
', rendered) + self.assertEqual(pages.preview_html(rendered.encode(), pull(11)), rendered.encode()) + + def test_existing_previews_get_decoration_without_downloading_expired_artifacts(self): + source = Source() + with tempfile.TemporaryDirectory() as tmp: + site = Path(tmp) + pages.reconcile(site, source) + (site / 'pr-preview/pr-11/index.html').write_bytes(page('Stored preview')) + source.previews = {} + with patch.object(source, 'download') as download: + pages.reconcile(site, source) + download.assert_not_called() + self.assertIn('[DEV PREVIEW]', (site / 'pr-preview/pr-11/index.html').read_text()) + self.assertEqual((site / 'index.html').read_bytes(), page('production')) + def test_owners_are_explicit_nonempty_logins(self): with tempfile.TemporaryDirectory() as tmp: path = Path(tmp) / "OWNERS" @@ -121,7 +148,7 @@ def test_new_push_keeps_only_previously_approved_snapshot_until_new_approval(sel source.previews[11] = build(12, later, "new") state = pages.reconcile(site, source) self.assertEqual(state["previews"]["11"]["sha"], sha) - self.assertEqual((site / "pr-preview/pr-11/index.html").read_text(), "approved:index.html") + self.assertIn("
approved:index.html
", (site / "pr-preview/pr-11/index.html").read_text()) self.assertFalse(state["preview_access"]["11"]["allowed"]) approvals.append({"id": 21, "body": "/deploy " + later, "user": {"login": "trusted"}}) state = pages.reconcile(site, source) @@ -156,8 +183,8 @@ def test_reconcile_updates_main_and_previews_preserves_failed_builds_and_prunes_ with tempfile.TemporaryDirectory() as tmp: site = Path(tmp) first = pages.reconcile(site, source) - self.assertEqual((site / "index.html").read_text(), "production:index.html") - self.assertEqual((site / "pr-preview/pr-12/index.html").read_text(), "fork:index.html") + self.assertEqual((site / "index.html").read_text(), page("production").decode()) + self.assertIn("
fork:index.html
", (site / "pr-preview/pr-12/index.html").read_text()) self.assertEqual(set(first["previews"]), {"11", "12"}) # A main deploy preserves previews whose latest build is pending/failed/expired. @@ -165,8 +192,8 @@ def test_reconcile_updates_main_and_previews_preserves_failed_builds_and_prunes_ source.previews = {} source.open[0]["head"]["sha"] = "pending" pages.reconcile(site, source) - self.assertEqual((site / "index.html").read_text(), "new production:index.html") - self.assertEqual((site / "pr-preview/pr-11/index.html").read_text(), "first:index.html") + self.assertEqual((site / "index.html").read_text(), page("new production").decode()) + self.assertIn("
first:index.html
", (site / "pr-preview/pr-11/index.html").read_text()) self.assertIn("Previous successful build", (site / "pr-preview/index.html").read_text()) # Every event reconciles all open PRs, regardless of which run triggered it. @@ -174,8 +201,8 @@ def test_reconcile_updates_main_and_previews_preserves_failed_builds_and_prunes_ source.open = [pull(11, "next", title="")] source.previews = {11: build(30, "next", "updated")} final = pages.reconcile(site, source) - self.assertEqual((site / "index.html").read_text(), "new production:index.html") - self.assertEqual((site / "pr-preview/pr-11/index.html").read_text(), "updated:index.html") + self.assertEqual((site / "index.html").read_text(), page("new production").decode()) + self.assertIn("
updated:index.html
", (site / "pr-preview/pr-11/index.html").read_text()) self.assertFalse((site / "pr-preview/pr-12").exists()) self.assertEqual(final["previews"]["11"]["sha"], "next") self.assertNotIn("