diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c2420d7..991de39 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -16,6 +16,19 @@ +## Before / After + + + + +| | Before | After | +| --- | --- | --- | +| Light | | | +| Dark | | | + +- [ ] No visual change (refactor, tests, docs). Say why in "What was done?". + + ## Breaking Changes diff --git a/.github/scripts/screenshots-check.py b/.github/scripts/screenshots-check.py new file mode 100755 index 0000000..3565ef5 --- /dev/null +++ b/.github/scripts/screenshots-check.py @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +"""Fails a PR that changes anything under Sources/ (components, design tokens, the asset +catalog) or a package manifest unless its description shows Before / After screenshots or +declares that nothing visible changed: + + - a heading whose text mentions "Before / After", or mentions "Screenshots" and is not + about tests (any heading level, any case), and under it, + up to the next heading of the same or a higher level, at least two different images; or + - the ticked task "No visual change". + +It reads the description as GitHub renders it (body_html), so comments, code blocks, list +indentation and every other markdown rule are already applied: what counts is what a reader +sees. A file renamed out of Sources/ counts as a UI change, and so does the package manifest +(Package.swift, Package@swift-*.swift), whose platform floor decides which branches the +components compile. Struck-out text and images do not count. Whether the images really show +light and dark is left to the reviewer. + +Usage: + REPO=dashpay/DashUIKit PR_NUMBER=31 GH_TOKEN=… screenshots-check.py + screenshots-check.py --files-json --pr-json +Env: GITHUB_STEP_SUMMARY (optional). +""" +import json +import os +import re +import secrets +import subprocess +import sys +from html.parser import HTMLParser +from urllib.parse import urlsplit + +# "Before / After" anywhere in the heading, or "Screenshots" anywhere in a heading that is +# not about tests ("Screenshot tests", "Screenshot_testing"; "testnet" and "TestFlight" are +# fine). +BEFORE_AFTER = re.compile(r"before\W*(and|vs\.?|to)?\W*after", re.I) +SCREENSHOTS = re.compile(r"screenshot", re.I) +TESTS = re.compile(r"(?: N + self.heading_text = "" + self.section_level = 0 # inside a Before / After section: its heading level + self.sections = [] # the image sources of each such section, in order + self.in_task = False # inside an
  • whose checkbox is ticked + self.task_text = "" + self.ticked = [] + self.struck = 0 # inside / : text a reader sees as crossed out + + def handle_starttag(self, tag, attrs): + attrs = dict(attrs) + level = int(tag[1]) if re.fullmatch(r"h[1-6]", tag) else 0 + if level: + if self.section_level and level <= self.section_level: + self.section_level = 0 + self.heading_level, self.heading_text = level, "" + elif tag in ("img", "video") and self.section_level and not self.struck: + src = attrs.get("data-canonical-src") or attrs.get("src") or "" + emoji = "emoji" in (attrs.get("class") or "").split() + if src.startswith(("https://", "http://")) and not emoji and not is_badge(src): + self.sections[-1].append(src) + elif tag in ("del", "s", "strike"): + self.struck += 1 + elif tag == "input" and attrs.get("type") == "checkbox" and "checked" in attrs: + self.in_task, self.task_text = True, "" + + def handle_endtag(self, tag): + if tag in ("del", "s", "strike") and self.struck: + self.struck -= 1 + if self.heading_level and tag == "h%d" % self.heading_level: + # A matching heading starts a section, unless it is nested in an open one, + # which then simply goes on. + if is_section(self.heading_text.strip()) and not self.section_level: + self.section_level = self.heading_level + self.sections.append([]) + self.heading_level = 0 + elif tag == "li" and self.in_task: + self.ticked.append(self.task_text.strip()) + self.in_task = False + + def handle_data(self, data): + if self.heading_level and not self.struck: + self.heading_text += data + if self.in_task and not self.struck: + self.task_text += data + + +def is_section(heading): + return bool(BEFORE_AFTER.search(heading) or (SCREENSHOTS.search(heading) and not TESTS.search(heading))) + + +def split(src): + try: + return urlsplit(src) + except ValueError: # e.g. "http://[bad": not a URL GitHub can show + return None + + +def is_badge(src): + parts = split(src) + if parts is None: + return True + host, segments = parts.hostname or "", parts.path.split("/") + folders = [s for s in segments if s != "badge"] if host in BRANCH_HOSTS else segments + return (any(host == h or host.endswith("." + h) for h in NOT_SCREENSHOT_HOSTS) + or host.split(".")[0] in ("badge", "badges") + or any(s in BADGE_SEGMENTS for s in folders) or segments[-1].lower() in ("badge", "badges") + or bool(BADGE_SVG.fullmatch(segments[-1]))) + + +def distinct(sources): + """Different images: the same upload is often linked with different queries.""" + seen = [] + for src in sources: + parts = split(src) + if parts is None: + continue + parts = parts._replace(fragment="") + if parts.hostname in UPLOAD_HOSTS or (parts.hostname == "github.com" and parts.path.startswith("/user-attachments/")): + parts = parts._replace(query="") + key = parts.geturl() + if key not in seen: + seen.append(key) + return seen + + +def gh(path, accept=None, paginate=False): + cmd = ["gh", "api"] + (["--paginate", "--slurp"] if paginate else []) + [path] + if accept: + cmd[2:2] = ["-H", "Accept: " + accept] + r = subprocess.run(cmd, capture_output=True, text=True) + if r.returncode: + print("::error::Reading the PR from the GitHub API failed: %s" % r.stderr.strip().replace("\n", " ")[-400:]) + sys.exit(2) + data = json.loads(r.stdout) + return [item for page in data for item in page] if paginate else data + + +def shown(path): + """A path as it is printed in the summary list: on one line, and not rendered as markdown. + Workflow commands in it are stopped by summary(), not here.""" + return "`%s`" % path.replace("\r", "\\r").replace("\n", "\\n").replace("`", "'") + + +def summary(line): + # The lines carry PR-controlled text. The runner reads "::" commands at a line start and + # the legacy "##[" anywhere, and no escape covers every byte it may skip: stop command + # parsing around the text instead. The summary file is only rendered, as it is. + token = secrets.token_hex(16) + print("::stop-commands::%s\n%s\n::%s::" % (token, line, token)) + target = os.environ.get("GITHUB_STEP_SUMMARY") + if target: + with open(target, "a") as f: + f.write(line + "\n") + + +def main(): + args = sys.argv[1:] + if len(args) == 4 and args[0] == "--files-json" and args[2] == "--pr-json": + files = json.load(open(args[1])) + pr = json.load(open(args[3])) + elif not args: + repo, number = os.environ["REPO"], os.environ["PR_NUMBER"] + # Read live: a re-run reuses the event payload, whose description may be stale. + files = gh("repos/%s/pulls/%s/files?per_page=100" % (repo, number), paginate=True) + pr = gh("repos/%s/pulls/%s" % (repo, number), accept="application/vnd.github.full+json") + else: + sys.exit(__doc__) + + paths = [] + for f in files: + paths.append(f["filename"]) + if f.get("previous_filename"): + paths.append(f["previous_filename"]) + ui = [p for p in paths if p.startswith(UI_SOURCES) or MANIFEST.fullmatch(p)] + if not ui and len(files) >= FILES_CAP and (pr.get("changed_files") or 0) > len(files): + # GitHub lists only the first 3000: treat the rest as possibly UI. + ui = ["(more than %d changed files; the rest are not listed)" % FILES_CAP] + + if not ui: + summary("No UI files changed: no screenshots needed.") + return 0 + + page = Description() + page.feed(pr.get("body_html") or "") + page.close() + # One section must hold the two images; images from separate sections do not add up. + images = max((distinct(section) for section in page.sections), key=len, default=[]) + listed = "\n".join(" - " + shown(p) for p in ui[:10]) + ("\n - and %d more" % (len(ui) - 10) if len(ui) > 10 else "") + + if len(images) >= 2: + summary("UI change with %d different screenshots under Before / After." % len(images)) + return 0 + if any(NO_VISUAL_CHANGE.match(t) for t in page.ticked): + summary("Author declared no visual change for:\n" + listed) + return 0 + + print("::error::This PR changes UI files but shows no Before / After screenshots " + "(found %d different image(s) under a Before / After or Screenshots heading)." % len(images)) + summary("UI change without Before / After screenshots. Changed UI files:\n" + listed) + summary('Add at least two images (before and after, light and dark where it differs) under the ' + '"Before / After" heading of the description, or tick "No visual change" and say why. ' + "Editing the description re-runs this check.") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/tests/screenshots-test.py b/.github/scripts/tests/screenshots-test.py new file mode 100755 index 0000000..eabfaf0 --- /dev/null +++ b/.github/scripts/tests/screenshots-test.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Pins screenshots-check.py: each case feeds it saved API answers (the changed files, and +the PR with the description rendered as GitHub's body_html) and expects a pass or a failure. +Runs without the network. Run from the repository root.""" +import json +import os +import re +import secrets +import subprocess +import sys +import tempfile + +SCRIPT = os.path.join(os.getcwd(), ".github", "scripts", "screenshots-check.py") +UI = [{"filename": "Sources/DashUIKit/Components/SearchBar.swift"}] +IMG = 'x' +A, B = IMG % ("1111", "1111"), IMG % ("2222", "2222") + + +def h(level, text): + return '
    %s
    ' % (level, text, level) + + +def task(text, ticked): + box = '' % ( + "Completed task" if ticked else "Incomplete task", ' checked=""' if ticked else "") + return '
    • %s %s
    ' % (box, text) + + +CASES = [ + ("no Sources/ files", "pass", [{"filename": ".github/workflows/ci.yml"}], ""), + ("two images under h2 Before / After", "pass", UI, h(2, "Before / After") + "

    %s %s

    " % (A, B)), + ("emoji and h3", "pass", UI, h(3, "📸 Before / After") + "
    %s%s
    " % (A, B)), + ("an older Screenshots heading", "pass", UI, h(2, "Screenshots") + "

    %s

    %s

    " % (A, B)), + ("subheadings inside the section", "pass", UI, h(2, "Before / After") + h(3, "Light") + A + h(3, "Dark") + B), + ("one image", "fail", UI, h(2, "Before / After") + A), + ("the same image with two signed queries", "fail", UI, h(2, "Before / After") + + ''), + ("images under another heading", "fail", UI, h(2, "Before / After") + "

    none

    " + h(2, "How Has This Been Tested?") + A + B), + ("images above the section", "fail", UI, A + B + h(2, "Before / After")), + ("the empty template cells", "fail", UI, h(2, "Before / After") + ''), + ("data-canonical-src counts", "pass", UI, h(2, "Before / After") + + ''), + ("No visual change ticked", "pass", UI, h(2, "Before / After") + task('No visual change (refactor, tests, docs). Say why in "What was done?".', True)), + ("No visual change unticked", "fail", UI, task("No visual change (refactor, tests, docs).", False)), + ("another task ticked", "fail", UI, task("I have performed a self-review of my own code", True)), + ("empty description", "fail", UI, None), + ("Before & After heading", "pass", UI, h(2, "Before & After") + A + B), + ("emoji and badges are not screenshots", "fail", UI, h(2, "Before / After") + + ''), + ("two videos", "pass", UI, h(2, "Before / After") + + ''), + ("query is the identity on other hosts", "pass", UI, h(2, "Before / After") + + ''), + ("a newline in a path stays inside the stop block", "fail", [{"filename": "Sources/a\n::error::forged.swift"}], ""), + ("a Screenshot tests heading is not the section", "fail", UI, h(2, "Screenshot tests") + A + B), + ("Screenshots with a parenthetical", "pass", UI, h(2, "📸 Screenshots (light and dark)") + A + B), + ("a workflow badge and codecov are not screenshots", "fail", UI, h(2, "Before / After") + + ''), + ("one upload with and without ?raw=true", "fail", UI, h(2, "Before / After") + + ''), + ("a malformed URL does not crash the ticked box", "pass", UI, h(2, "Before / After") + + '' + task("No visual change", True)), + ("a struck-out tick does not count", "fail", UI, task("No visual change", True)), + ("Package.swift alone is a UI change", "fail", [{"filename": "Package.swift"}], ""), + ("one image in each of two sections", "fail", UI, h(2, "Screenshots") + A + h(2, "Other") + "

    x

    " + h(2, "Before / After") + B), + ("a Screenshots heading nested in Before / After", "pass", UI, h(2, "Before / After") + A + h(3, "Screenshots") + h(3, "Dark") + B), + ("Screenshots with more words in the heading", "pass", UI, h(2, "UI screenshots: light / dark") + A + B), + ("a numbered Screenshots heading", "pass", UI, h(2, "1. Screenshots & video") + A + B), + ("a struck-out image does not count", "fail", UI, h(2, "Before / After") + A + "%s" % B), + ("an image in does not count", "fail", UI, h(2, "Before / After") + A + "%s" % B), + ("a struck-out heading is not the section", "fail", UI, h(2, "Before / After") + A + B), + ("a plural Screenshots tests heading is not the section", "fail", UI, h(2, "Screenshots tests") + A + B), + ("a hyphenated Screenshot-testing heading is not the section", "fail", UI, h(2, "Screenshot-testing") + A + B), + ("Screenshot_tests is not the section", "fail", UI, h(2, "Screenshot_tests") + A + B), + ("a Screenshot of the test run is not the section", "fail", UI, h(2, "Before / After") + "

    x

    " + + h(2, "How Has This Been Tested?") + h(3, "Screenshot of the test run") + A + B), + ("Screenshots of the latest build", "pass", UI, h(2, "Screenshots of the latest build") + A + B), + ("Screenshots (tests) is not the section", "fail", UI, h(2, "Screenshots (tests)") + A + B), + ("Screenshot/snapshot tests is not the section", "fail", UI, h(2, "Screenshot/snapshot tests") + A + B), + ("Screenshots anywhere in the heading", "pass", UI, h(2, "iOS screenshots") + A + B), + ("Light / dark screenshots", "pass", UI, h(2, "Light / dark screenshots") + A + B), + ("Screenshots (testnet)", "pass", UI, h(2, "Screenshots (testnet)") + A + B), + ("Screenshots from TestFlight", "pass", UI, h(2, "Screenshots from TestFlight") + A + B), + ("Screenshots (tested on iPhone 16)", "pass", UI, h(2, "Screenshots (tested on iPhone 16)") + A + B), + ("screenshots in a Badge folder count", "pass", UI, h(2, "Before / After") + + ''), + ("a goreportcard badge is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("a Codacy badge is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("a Read the Docs badge is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("a badge in a repo's badges/ folder is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("a badge on a subdomain of a badge host is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("LFS screenshots on a badge branch count", "pass", UI, h(2, "Before / After") + + '' + + ''), + ("screenshots on a badge branch count", "pass", UI, h(2, "Before / After") + + '' + + ''), + ("GitLab badges are not screenshots", "fail", UI, h(2, "Before / After") + + ''), + ("a /badges leaf is not a screenshot", "fail", UI, h(2, "Before / After") + A + + ''), + ("coveralls and netlify badges are not screenshots", "fail", UI, h(2, "Before / After") + + ''), + ("badge SVG variants and badge hosts are not screenshots", "fail", UI, h(2, "Before / After") + + '' + + ''), + ("extensionless /badge leaves are not screenshots", "fail", UI, h(2, "Before / After") + + ''), + ("a host that only starts with badge is not a badge", "pass", UI, h(2, "Before / After") + + ''), + ("screenshots in a folder named after a badge count", "pass", UI, h(2, "Before / After") + + ''), + ("UI: Screenshots", "pass", UI, h(2, "UI: Screenshots") + A + B), + ("other badges are not screenshots", "fail", UI, h(2, "Before / After") + + '' + + ''), + ("a screenshot of badge UI counts", "pass", UI, h(2, "Before / After") + + ''), + ("a version-specific manifest is a UI change", "fail", [{"filename": "Package@swift-6.2.swift"}], ""), + ("a manifest backup is not a UI change", "pass", [{"filename": "Package.swift.orig"}], ""), + ("a legacy ##[ marker in a path stays inside the stop block", "fail", [{"filename": "Sources/##[error]forged.swift"}], ""), + ("renamed out of Sources/", "fail", [{"filename": "Legacy/Toast.swift", "previous_filename": "Sources/DashUIKit/Components/Toast.swift"}], ""), +] + + +def commands_on(stdout): + """The output with the ::stop-commands:: blocks cut out: what the runner still parses.""" + return re.sub(r"^::stop-commands::(\w+)$.*?^::\1::$", "", stdout, flags=re.M | re.S) + + +def run(files, body_html, changed_files=None): + with tempfile.TemporaryDirectory() as d: + fj, pj = os.path.join(d, "files.json"), os.path.join(d, "pr.json") + json.dump(files, open(fj, "w")) + json.dump({"body_html": body_html, "changed_files": changed_files or len(files)}, open(pj, "w")) + env = dict(os.environ, GITHUB_STEP_SUMMARY="") + return subprocess.run([sys.executable, SCRIPT, "--files-json", fj, "--pr-json", pj], + capture_output=True, text=True, env=env) + + +def main(): + failures = 0 + over = [{"filename": "docs/%d.md" % i} for i in range(3000)] + cases = CASES + [("over the files cap, no Sources/ seen", "fail", over, "", 3500), + ("over the files cap, with screenshots", "pass", over, h(2, "Before / After") + A + B, 3500)] + for case in cases: + name, want, files, html = case[:4] + r = run(files, html, case[4] if len(case) > 4 else None) + # A failure must be the check's own verdict, not a crash that also exits 1. + ok = ("Traceback" not in r.stderr) and ((r.returncode == 0) if want == "pass" + else (r.returncode == 1 and "::error::" in commands_on(r.stdout))) + if name.startswith(("a newline in a path", "a legacy ##[")) and "forged" in commands_on(r.stdout): + ok = False + print(("ok " if ok else "FAIL ") + name) + if not ok: + failures += 1 + # The child's output is PR-like text: defuse workflow commands before it reaches the log. + token = secrets.token_hex(16) + out = (r.stdout + r.stderr).strip().replace("\n", "\n ") + print(" expected %s, got exit %d\n::stop-commands::%s\n %s\n::%s::" % (want, r.returncode, token, out, token)) + print("%d case(s) failed" % failures if failures else "All cases passed") + return 1 if failures else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f48e98..ea14628 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,7 +72,8 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - id: checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false @@ -82,9 +83,16 @@ jobs: run: .github/scripts/conventions.sh # Breaks a copy of the tree once per rule, so a rule that stops matching fails here. + # It needs a clean baseline, so it runs only when the check above passed. - name: Test the conventions script run: .github/scripts/tests/conventions-test.sh + # The PR-description check runs from the base branch, so its rules are tested here, + # also when a conventions step above failed. + - name: Test the Before / After screenshots check + if: ${{ !cancelled() && steps.checkout.outcome == 'success' }} + run: python3 .github/scripts/tests/screenshots-test.py + actionlint: name: Lint CI config (actionlint, shellcheck) runs-on: ubuntu-latest diff --git a/.github/workflows/screenshots.yml b/.github/workflows/screenshots.yml new file mode 100644 index 0000000..f189736 --- /dev/null +++ b/.github/workflows/screenshots.yml @@ -0,0 +1,51 @@ +name: PR description + +# pull_request_target runs this file and the script as the base branch has them, so a PR +# cannot edit its own check. The job checks out the base branch only and runs no code from +# the PR: the script reads the PR's changed files and its rendered description through the +# API. Keep it that way: never check out or run the PR's head here. +# A stacked PR gets the check its base branch has; a feat/* base cut before this landed has +# none until it is merged up from master or the PR is retargeted. +# `edited` re-runs it when the author adds screenshots, so it lives apart from CI to keep +# body edits from rebuilding. +on: + pull_request_target: + types: + - opened + - edited + - synchronize + - reopened + - ready_for_review + +permissions: + contents: read + pull-requests: read + +concurrency: + # github.ref is the base branch under pull_request_target; key on the PR. + group: screenshots-${{ github.event.pull_request.number }} + # A new commit replaces the run for the old one; a body edit must not cancel a running + # check and leave the required check pending. + cancel-in-progress: ${{ github.event.action == 'synchronize' }} + +jobs: + screenshots: + name: Before / After screenshots + # `edited` also fires on closed PRs. + if: github.event.pull_request.state == 'open' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + # The script is all it needs; skip the sources and the asset catalog. + sparse-checkout: .github/scripts + sparse-checkout-cone-mode: false + + - name: Check the description + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: python3 .github/scripts/screenshots-check.py diff --git a/CLAUDE.md b/CLAUDE.md index 1c5c7ac..1f5fa6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,6 +61,10 @@ Sources/DashUIKit/ - **UIKit-only files** are wrapped in `#if canImport(UIKit)` (e.g. `SearchBar`, `Toast`, `AddressFieldView`, `DashSwitch`). - **Localization:** user-facing strings use `NSLocalizedString(_, bundle: .module, comment:)`. +- **PR screenshots:** a PR that changes anything under `Sources/` or a package manifest + (`Package.swift`, `Package@swift-*.swift`) shows Before / After screenshots (light and + dark) in its description, or ticks "No visual change" and says why. + CI (`Before / After screenshots`) enforces it. - Components are **stateless/value-driven** — state (`@Binding`) and callbacks live with the host. Don't add view models or persistence here. - CI enforces the mechanical ones with `.github/scripts/conventions.sh`: `DashIcon` ↔