From 7248a779e35bcd74904e0b38c0ef0320bebafa87 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:02:19 +0100 Subject: [PATCH 01/16] review: read worktrees, rounds and old files from git HIDDEN moves from devserver.py to gitread.py. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- scripts/review/devserver.py | 4 +- scripts/review/gitread.py | 69 +++++++++++++++++++++++++ scripts/review/test_gitread.py | 93 ++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 scripts/review/gitread.py create mode 100644 scripts/review/test_gitread.py diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index 239ef3b..bc2987e 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -21,14 +21,16 @@ import os import re import secrets +import sys import time from functools import partial from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, HERE) +from gitread import HIDDEN # noqa: E402 PREFIX = "/__review/" ASSETS = {"review.js": "text/javascript; charset=utf-8", "review.css": "text/css; charset=utf-8"} -HIDDEN = (".git", ".review", ".reels", "node_modules", "__pycache__") MAX_BODY = 32 * 1024 MAX_NOTE = 4000 NOTE_ID = re.compile(r"^\d{8}-\d{6}-[0-9a-f]{6}$") diff --git a/scripts/review/gitread.py b/scripts/review/gitread.py new file mode 100644 index 0000000..7d7f3c4 --- /dev/null +++ b/scripts/review/gitread.py @@ -0,0 +1,69 @@ +"""Read worktrees, rounds and old files from git. Never uses a shell.""" +import re +import subprocess + +HIDDEN = (".git", ".review", ".reels", "node_modules", "__pycache__") + +_BRANCH_RE = re.compile(r"^[A-Za-z0-9._/-]{1,100}$") + + +def _git(cwd, *args): + # safe.directory: the container user differs from the mount owner in some setups. + return subprocess.run( + ["git", "-c", "safe.directory=*", *args], + cwd=cwd, capture_output=True, text=True, timeout=5, + ) + + +def worktrees(main): + out = _git(main, "worktree", "list", "--porcelain").stdout + result = [] + for block in out.split("\n\n"): + path = branch = None + for line in block.splitlines(): + if line.startswith("worktree "): + path = line[len("worktree "):] + elif line.startswith("branch refs/heads/"): + branch = line[len("branch refs/heads/"):] + if path and branch: + result.append({"branch": branch, "path": path}) + return result + + +def rounds(main, branch, base="master"): + res = _git(main, "log", "--reverse", "--format=%H%x00%s", base + ".." + branch, "--") + if res.returncode != 0: + return [] + result = [] + for line in res.stdout.splitlines(): + sha, _, subject = line.partition("\x00") + result.append({"n": len(result) + 1, "sha": sha, "subject": subject}) + return result + + +def dirty(path): + out = _git(path, "status", "--porcelain").stdout + for line in out.splitlines(): + name = line[3:].split(" -> ")[-1].strip('"') + if line.startswith("??") and name.split("/")[0] in HIDDEN: + continue + return True + return False + + +def show(main, rev, path): + res = _git(main, "show", rev + ":" + path) + return res.stdout if res.returncode == 0 else None + + +def valid_branch(name): + if not _BRANCH_RE.match(name) or ".." in name or name.startswith("-"): + return False + return _git(None, "check-ref-format", "--branch", name).returncode == 0 + + +def valid_path(path): + if not path or path.startswith("/"): + return False + parts = path.split("/") + return not any(p == ".." or p in HIDDEN for p in parts) diff --git a/scripts/review/test_gitread.py b/scripts/review/test_gitread.py new file mode 100644 index 0000000..61e50e9 --- /dev/null +++ b/scripts/review/test_gitread.py @@ -0,0 +1,93 @@ +import os +import subprocess +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import gitread + + +def git(cwd, *args): + subprocess.run( + ["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], + cwd=cwd, check=True, capture_output=True, + ) + + +class GitReadTest(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.tmp = tempfile.TemporaryDirectory() + root = os.path.realpath(cls.tmp.name) + cls.main = os.path.join(root, "main") + cls.wt = os.path.join(root, "wt") + os.mkdir(cls.main) + git(cls.main, "init", "-b", "master") + with open(os.path.join(cls.main, "a.txt"), "w") as f: + f.write("old\n") + git(cls.main, "add", "a.txt") + git(cls.main, "commit", "-m", "base") + git(cls.main, "worktree", "add", "-b", "t/a", cls.wt) + for i, text in enumerate(["one", "two"], 1): + with open(os.path.join(cls.wt, "a.txt"), "w") as f: + f.write(text + "\n") + git(cls.wt, "commit", "-am", "round " + text) + + @classmethod + def tearDownClass(cls): + cls.tmp.cleanup() + + def test_worktrees_lists_main_first(self): + result = gitread.worktrees(self.main) + self.assertEqual(result[0], {"branch": "master", "path": self.main}) + self.assertIn({"branch": "t/a", "path": self.wt}, result) + + def test_rounds_are_numbered_oldest_first(self): + result = gitread.rounds(self.main, "t/a") + self.assertEqual([r["n"] for r in result], [1, 2]) + self.assertEqual([r["subject"] for r in result], ["round one", "round two"]) + self.assertEqual(len(result[0]["sha"]), 40) + + def test_rounds_of_master_are_empty(self): + self.assertEqual(gitread.rounds(self.main, "master"), []) + + def test_dirty_sees_an_edit_and_a_new_file(self): + self.assertFalse(gitread.dirty(self.wt)) + os.makedirs(os.path.join(self.wt, ".review")) + with open(os.path.join(self.wt, ".review", "x.json"), "w") as f: + f.write("{}") + self.assertFalse(gitread.dirty(self.wt)) + path = os.path.join(self.wt, "a.txt") + with open(path) as f: + text = f.read() + with open(path, "w") as f: + f.write("changed\n") + self.assertTrue(gitread.dirty(self.wt)) + with open(path, "w") as f: + f.write(text) + self.assertFalse(gitread.dirty(self.wt)) + new = os.path.join(self.wt, "new.txt") + with open(new, "w") as f: + f.write("x") + self.assertTrue(gitread.dirty(self.wt)) + os.remove(new) + + def test_show_returns_old_text_and_none_for_missing(self): + self.assertEqual(gitread.show(self.main, "master", "a.txt"), "old\n") + self.assertEqual(gitread.show(self.main, "t/a", "a.txt"), "two\n") + self.assertIsNone(gitread.show(self.main, "master", "missing.txt")) + + def test_valid_branch_refuses(self): + for bad in ["../x", "a b", "a;b", "-x", ""]: + self.assertFalse(gitread.valid_branch(bad), bad) + self.assertTrue(gitread.valid_branch("t/a")) + + def test_valid_path_refuses(self): + for bad in ["../x", "/etc/passwd", ".git/config", "a/../b"]: + self.assertFalse(gitread.valid_path(bad), bad) + self.assertTrue(gitread.valid_path("posts/index.html")) + + +if __name__ == "__main__": + unittest.main() From 6fee3e199a8bb7f65eba55583a257c0aac225189 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:04:11 +0100 Subject: [PATCH 02/16] review: harden gitread against option injection and failed git Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- scripts/review/gitread.py | 27 +++++++++++++++++++-------- scripts/review/test_gitread.py | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/scripts/review/gitread.py b/scripts/review/gitread.py index 7d7f3c4..f31b2da 100644 --- a/scripts/review/gitread.py +++ b/scripts/review/gitread.py @@ -9,10 +9,13 @@ def _git(cwd, *args): # safe.directory: the container user differs from the mount owner in some setups. - return subprocess.run( - ["git", "-c", "safe.directory=*", *args], - cwd=cwd, capture_output=True, text=True, timeout=5, - ) + try: + return subprocess.run( + ["git", "-c", "safe.directory=*", *args], + cwd=cwd, capture_output=True, text=True, timeout=5, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, ValueError): + return subprocess.CompletedProcess(args, 1, "", "") def worktrees(main): @@ -31,7 +34,9 @@ def worktrees(main): def rounds(main, branch, base="master"): - res = _git(main, "log", "--reverse", "--format=%H%x00%s", base + ".." + branch, "--") + if base.startswith("-") or branch.startswith("-"): + return [] + res = _git(main, "log", "--reverse", "--format=%H%x00%s", "--end-of-options", base + ".." + branch, "--") if res.returncode != 0: return [] result = [] @@ -42,8 +47,10 @@ def rounds(main, branch, base="master"): def dirty(path): - out = _git(path, "status", "--porcelain").stdout - for line in out.splitlines(): + res = _git(path, "status", "--porcelain") + if res.returncode != 0: + return True # fail closed: an unknown state counts as dirty + for line in res.stdout.splitlines(): name = line[3:].split(" -> ")[-1].strip('"') if line.startswith("??") and name.split("/")[0] in HIDDEN: continue @@ -52,7 +59,9 @@ def dirty(path): def show(main, rev, path): - res = _git(main, "show", rev + ":" + path) + if rev.startswith("-"): + return None + res = _git(main, "show", "--end-of-options", rev + ":" + path) return res.stdout if res.returncode == 0 else None @@ -65,5 +74,7 @@ def valid_branch(name): def valid_path(path): if not path or path.startswith("/"): return False + if any(ord(c) < 0x20 or ord(c) == 0x7F for c in path): + return False parts = path.split("/") return not any(p == ".." or p in HIDDEN for p in parts) diff --git a/scripts/review/test_gitread.py b/scripts/review/test_gitread.py index 61e50e9..bed60c3 100644 --- a/scripts/review/test_gitread.py +++ b/scripts/review/test_gitread.py @@ -88,6 +88,29 @@ def test_valid_path_refuses(self): self.assertFalse(gitread.valid_path(bad), bad) self.assertTrue(gitread.valid_path("posts/index.html")) + def test_show_refuses_option_like_rev(self): + out = os.path.join(self.main, "x") + self.assertIsNone(gitread.show(self.main, "--output=" + out, "a")) + self.assertFalse(os.path.exists(out)) + + def test_rounds_refuses_option_like_base_or_branch(self): + out = os.path.join(self.main, "x") + self.assertEqual(gitread.rounds(self.main, "t/a", base="--output=" + out), []) + self.assertEqual(gitread.rounds(self.main, "--output=" + out), []) + self.assertFalse(os.path.exists(out)) + + def test_dirty_is_true_when_git_fails(self): + with tempfile.TemporaryDirectory() as d: + self.assertTrue(gitread.dirty(d)) + self.assertTrue(gitread.dirty("/nonexistent-folder-xyz")) + + def test_valid_path_refuses_control_characters_and_empty(self): + for bad in ["a\x00b", "", "a\nb", "a\x7fb", "a\x1fb"]: + self.assertFalse(gitread.valid_path(bad), repr(bad)) + + def test_show_of_bad_path_returns_none(self): + self.assertIsNone(gitread.show(self.main, "master", "a\x00b")) + if __name__ == "__main__": unittest.main() From 503c486f9ba411dd427435031e4029ea2200892b Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:06:41 +0100 Subject: [PATCH 03/16] review: serve the chosen branch and list its rounds Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- docker-compose.dev.yml | 19 +++-- scripts/review/Dockerfile | 4 + scripts/review/devserver.py | 113 +++++++++++++++++++++------- scripts/review/gitread.py | 8 ++ scripts/review/test_devserver.py | 125 ++++++++++++++++++++++++++++++- scripts/review/test_gitread.py | 6 ++ 6 files changed, 237 insertions(+), 38 deletions(-) create mode 100644 scripts/review/Dockerfile diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index bb7a199..ebbb65a 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -2,19 +2,23 @@ # login. GitHub Pages still serves t7d.dev from gh-pages. This stack never touches it. # # mkdir -p .review/notes # first: else Docker makes the folder as root -# echo REVIEW_HOST= > .env # the host name of the review site (.env and .review/ are gitignored) +# # .env (gitignored): REVIEW_HOST=, REVIEW_MAIN=, +# # REVIEW_WORKTREES= # docker-compose -p t7d-review -f docker-compose.dev.yml up -d # docker-compose -p t7d-review -f docker-compose.dev.yml down # -# The clone is read-only in the container. Only .review/notes is writable: one JSON file per note. -# A checkout of another branch in this folder shows that branch at once, with its name in the note tab. +# The clone and the worktrees are read-only in the container, at their host paths (git needs the same paths). Only .review/notes is writable: one JSON file per note. +# The page shows the branch of ?branch=, else of the cookie, else of the clone. The image adds only the package git. services: t7d-review: - image: python:3.12-slim + build: + context: scripts/review + dockerfile: Dockerfile + image: t7d-review:local container_name: t7d-review restart: unless-stopped user: "1001:1001" - command: [python3, /repo/scripts/review/devserver.py, --root, /repo, --notes, /repo/.review/notes, --host, 0.0.0.0, --port, "8000"] + command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --notes, "${REVIEW_MAIN}/.review/notes", --host, 0.0.0.0, --port, "8000"] environment: PYTHONDONTWRITEBYTECODE: "1" read_only: true @@ -25,8 +29,9 @@ services: - /tmp:size=8m,mode=1777 logging: { driver: json-file, options: { max-size: "10m", max-file: "3" } } volumes: - - .:/repo:ro - - ./.review/notes:/repo/.review/notes:rw + - ${REVIEW_MAIN}:${REVIEW_MAIN}:ro + - ${REVIEW_WORKTREES:?set REVIEW_WORKTREES in .env}:${REVIEW_WORKTREES}:ro + - ${REVIEW_MAIN}/.review/notes:${REVIEW_MAIN}/.review/notes:rw networks: [proxy] # no ports: the lab proxy routes to it labels: - traefik.enable=true diff --git a/scripts/review/Dockerfile b/scripts/review/Dockerfile new file mode 100644 index 0000000..56ff572 --- /dev/null +++ b/scripts/review/Dockerfile @@ -0,0 +1,4 @@ +FROM python:3.12-slim +RUN apt-get update \ + && apt-get install -y --no-install-recommends git \ + && rm -rf /var/lib/apt/lists/* diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index bc2987e..66e5952 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -8,7 +8,11 @@ script can reach this server. Nothing else in the CSP changes, so the review site keeps the rules of the real site. - /__review/reload: server-sent events. The stamp changes when a file of the site changes, is added or is deleted. The page then loads again. -- /__review/api/state: the branch and the commit of the clone, and the number of open notes. +- The branch comes from ?branch=, else the cookie review_branch, else the branch of the main clone. A page with a + valid ?branch= sets the cookie. An unknown branch gives the main clone. Pages come from the worktree of that branch. +- /__review/api/state: the branch and the commit of the chosen worktree, and the number of open notes. +- /__review/api/versions?branch=: the branches, the rounds of the branch since master, and if the worktree has changes. +- /__review/api/text?branch=&rev=&path=: the HTML of a page at that revision, or null. - /__review/api/notes: GET lists the notes, open and done. POST writes one note as /.json. The POST needs the header X-T7d-Review and a JSON body: a plain form from another site cannot send either. A handled note goes to /done/ with an "answer" field. The page shows the answer under "My notes". @@ -23,13 +27,18 @@ import secrets import sys import time +import urllib.parse from functools import partial +from http.cookies import SimpleCookie from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer HERE = os.path.dirname(os.path.abspath(__file__)) sys.path.insert(0, HERE) +import gitread # noqa: E402 from gitread import HIDDEN # noqa: E402 PREFIX = "/__review/" +COOKIE = "review_branch" +BASE = "master" ASSETS = {"review.js": "text/javascript; charset=utf-8", "review.css": "text/css; charset=utf-8"} MAX_BODY = 32 * 1024 MAX_NOTE = 4000 @@ -68,27 +77,9 @@ def stamp(root): return "%.3f-%d" % (newest, count) -def git_state(root): - """{branch, commit} of the clone from .git, without the git binary (the container has none).""" - g = os.path.join(root, ".git") - try: - head = read(os.path.join(g, "HEAD")).strip() - except OSError: - return {"branch": None, "commit": None} - if not head.startswith("ref: "): - return {"branch": None, "commit": head[:7]} - ref = head[5:] - commit = None - try: - commit = read(os.path.join(g, ref)).strip() - except OSError: - try: - for line in read(os.path.join(g, "packed-refs")).splitlines(): - if line.rstrip().endswith(" " + ref): - commit = line.split()[0] - except OSError: - pass - return {"branch": ref.rsplit("refs/heads/", 1)[-1], "commit": commit[:7] if commit else None} +def git_state(path): + """{branch, commit} of the checkout at path.""" + return gitread.head(path) def inject(html): @@ -151,6 +142,35 @@ def hidden(self): parts = self.path.split("?", 1)[0].split("/") return any(p in HIDDEN for p in parts) + def pick(self): + """Choose the branch of this request. Sets self.branch, self.wt (its worktree path), self.names, + self.cookie_branch (a valid ?branch= to remember) and self.bad_branch (a ?branch= that is invalid or unknown).""" + main = gitread.head(self.root)["branch"] + found = {w["branch"]: w["path"] for w in gitread.worktrees(self.root)} + if main: + found[main] = self.root + self.names = ([main] if main else []) + [b for b in found if b != main] + queried = urllib.parse.parse_qs(urllib.parse.urlsplit(self.path).query).get("branch") + self.cookie_branch, self.bad_branch = None, False + want = None + if queried: + if gitread.valid_branch(queried[0]) and queried[0] in found: + want = self.cookie_branch = queried[0] + else: + self.bad_branch = True + else: + try: + value = SimpleCookie(self.headers.get("Cookie") or "")[COOKIE].value + except (KeyError, ValueError): + value = None + if value and gitread.valid_branch(value) and value in found: + want = value + self.branch = want or main + self.wt = found.get(want) or self.root + + def query(self, name): + return urllib.parse.parse_qs(urllib.parse.urlsplit(self.path).query).get(name, [None])[0] + def send_json(self, code, obj): body = json.dumps(obj).encode() self.send_response(code) @@ -164,11 +184,15 @@ def send_page(self, path, code=200): self.send_response(code) self.send_header("Content-Type", "text/html; charset=utf-8") self.send_header("Content-Length", str(len(body))) + if self.cookie_branch: + self.send_header("Set-Cookie", "%s=%s; Path=/; SameSite=Strict" % (COOKIE, self.cookie_branch)) self.end_headers() self.wfile.write(body) def do_GET(self): url = self.path.split("?", 1)[0] + self.pick() + self.directory = self.wt # SimpleHTTPRequestHandler serves from this folder if url.startswith(PREFIX): return self.review_get(url[len(PREFIX):]) if self.hidden(): @@ -190,7 +214,7 @@ def do_GET(self): return super().do_GET() def send_generated(self, url): - body = gen_feeds().build(self.root)[url.lstrip("/")].encode() + body = gen_feeds().build(self.wt)[url.lstrip("/")].encode() self.send_response(200) self.send_header("Content-Type", GENERATED[url]) self.send_header("Content-Length", str(len(body))) @@ -198,7 +222,7 @@ def send_generated(self, url): self.wfile.write(body) def not_found(self): - page = os.path.join(self.root, "404.html") + page = os.path.join(self.wt, "404.html") if os.path.isfile(page): return self.send_page(page, 404) self.send_error(404) @@ -215,11 +239,45 @@ def review_get(self, name): if name == "reload": return self.reload() if name == "api/state": - return self.send_json(200, {**git_state(self.root), "open": len(self.list_notes("open"))}) + return self.send_json(200, {**git_state(self.wt), "open": len(self.list_notes("open"))}) + if name == "api/versions": + return self.versions() + if name == "api/text": + return self.text() if name == "api/notes": return self.send_json(200, {"notes": self.list_notes("open") + self.list_notes("done")}) self.send_error(404) + def versions(self): + if self.bad_branch: + return self.send_json(400, {"error": "unknown branch"}) + return self.send_json(200, {"branch": self.branch, "branches": self.names, "base": BASE, + "rounds": gitread.rounds(self.root, self.branch, BASE) if self.branch else [], + "working": gitread.dirty(self.wt)}) + + def text(self): + if self.bad_branch: + return self.send_json(400, {"error": "unknown branch"}) + rev, path = self.query("rev"), self.query("path") + shas = [r["sha"] for r in gitread.rounds(self.root, self.branch, BASE)] if self.branch else [] + if rev not in ("working", BASE, *shas): + return self.send_json(400, {"error": "unknown rev"}) + rel = (path or "").lstrip("/") if (path or "").startswith("/") else None + if rel is not None and (rel == "" or rel.endswith("/")): + rel += "index.html" + if rel is None or not gitread.valid_path(rel): + return self.send_json(400, {"error": "bad path"}) + try: + if rev == "working": + full = os.path.realpath(os.path.join(self.wt, rel)) + inside = full.startswith(os.path.realpath(self.wt) + os.sep) + html = read(full) if inside and os.path.isfile(full) else None + else: + html = gitread.show(self.root, rev, rel) + except UnicodeDecodeError: + html = None # a binary file + return self.send_json(200, {"html": html}) + def reload(self): self.send_response(200) self.send_header("Content-Type", "text/event-stream") @@ -227,7 +285,7 @@ def reload(self): last, beat = None, 0.0 try: while True: - now = stamp(self.root) + now = stamp(self.wt) if now != last or time.time() - beat > 15: self.wfile.write(("data: %s\n\n" % now).encode()) self.wfile.flush() @@ -266,7 +324,8 @@ def do_POST(self): note = clean_note(json.loads(self.rfile.read(n))) except ValueError as e: return self.send_json(400, {"error": str(e)}) - note.update(git_state(self.root)) + self.pick() + note.update(git_state(self.wt)) note["time"] = time.strftime("%Y-%m-%dT%H:%M:%S%z") nid = time.strftime("%Y%m%d-%H%M%S-") + secrets.token_hex(3) os.makedirs(self.notes, exist_ok=True) diff --git a/scripts/review/gitread.py b/scripts/review/gitread.py index f31b2da..9116be4 100644 --- a/scripts/review/gitread.py +++ b/scripts/review/gitread.py @@ -58,6 +58,14 @@ def dirty(path): return False +def head(path): + """{branch, commit} of the checkout at path. A value is None if git cannot tell.""" + branch = _git(path, "symbolic-ref", "--short", "-q", "HEAD") + commit = _git(path, "rev-parse", "--short=7", "HEAD") + return {"branch": branch.stdout.strip() or None if branch.returncode == 0 else None, + "commit": commit.stdout.strip() or None if commit.returncode == 0 else None} + + def show(main, rev, path): if rev.startswith("-"): return None diff --git a/scripts/review/test_devserver.py b/scripts/review/test_devserver.py index 2103617..31a9627 100644 --- a/scripts/review/test_devserver.py +++ b/scripts/review/test_devserver.py @@ -1,6 +1,7 @@ """Tests of the review server. Run: python3 -m unittest scripts/review/test_devserver.py""" import json import os +import subprocess import sys import tempfile import threading @@ -13,6 +14,11 @@ sys.path.insert(0, os.path.dirname(__file__)) import devserver # noqa: E402 +def git(cwd, *args): + return subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, + capture_output=True, text=True).stdout.strip() + + PAGE = ('T

Hi

') @@ -52,10 +58,14 @@ def setUp(self): self.root = tempfile.mkdtemp() os.makedirs(os.path.join(self.root, "a")) for rel, body in (("index.html", PAGE), ("404.html", PAGE.replace("Hi", "Gone")), ("a/index.html", PAGE), - ("x.css", "p{}"), (".git/HEAD", "ref: refs/heads/feat/x\n"), (".git/refs/heads/feat/x", "abcdef1234\n")): + ("x.css", "p{}"), ): os.makedirs(os.path.dirname(os.path.join(self.root, rel)), exist_ok=True) with open(os.path.join(self.root, rel), "w") as f: f.write(body) + git(self.root, "init", "-b", "feat/x") + git(self.root, "add", "-A") + git(self.root, "commit", "-m", "start") + self.commit = git(self.root, "rev-parse", "--short=7", "HEAD") self.notes = os.path.join(self.root, ".review", "notes") h = partial(type("H", (devserver.Handler,), {"root": self.root, "notes": self.notes}), directory=self.root) self.srv = ThreadingHTTPServer(("127.0.0.1", 0), h) @@ -67,9 +77,9 @@ def tearDown(self): self.srv.shutdown() self.srv.server_close() - def get(self, path): + def get(self, path, cookie=None): try: - r = urllib.request.urlopen(self.base + path) + r = urllib.request.urlopen(urllib.request.Request(self.base + path, headers={"Cookie": cookie} if cookie else {})) return r.status, r.headers, r.read().decode() except urllib.error.HTTPError as e: return e.code, e.headers, e.read().decode() @@ -115,7 +125,7 @@ def test_hidden_folders_are_not_served(self): def test_state_has_the_branch(self): s = json.loads(self.get("/__review/api/state")[2]) - self.assertEqual((s["branch"], s["commit"], s["open"]), ("feat/x", "abcdef1", 0)) + self.assertEqual((s["branch"], s["commit"], s["open"]), ("feat/x", self.commit, 0)) def test_a_note_is_a_file_and_in_the_list(self): code, d = self.post({"note": "The title is too big", "path": "/a/"}) @@ -136,5 +146,112 @@ def test_a_post_without_the_header_or_json_is_refused(self): self.assertFalse(os.path.exists(self.notes) and os.listdir(self.notes)) +class Branches(unittest.TestCase): + """A main clone on master with the worktree t/a: two rounds, a new page and an edit that is not committed.""" + + @classmethod + def setUpClass(cls): + cls.tmp = tempfile.TemporaryDirectory() + base = os.path.realpath(cls.tmp.name) + cls.main, cls.wt = os.path.join(base, "main"), os.path.join(base, "wt") + os.mkdir(cls.main) + git(cls.main, "init", "-b", "master") + cls.write(cls.main, "index.html", PAGE.replace("Hi", "Master home")) + git(cls.main, "add", "-A") + git(cls.main, "commit", "-m", "base") + git(cls.main, "worktree", "add", "-b", "t/a", cls.wt) + cls.write(cls.wt, "index.html", PAGE.replace("Hi", "Round one")) + git(cls.wt, "commit", "-am", "round one") + cls.write(cls.wt, "new/index.html", PAGE.replace("Hi", "New page")) + git(cls.wt, "add", "-A") + git(cls.wt, "commit", "-m", "round two") + cls.shas = git(cls.main, "log", "--reverse", "--format=%H", "master..t/a").split() + h = partial(type("H", (devserver.Handler,), {"root": cls.main, "notes": os.path.join(cls.main, ".review", "notes")}), + directory=cls.main) + cls.srv = ThreadingHTTPServer(("127.0.0.1", 0), h) + cls.srv.daemon_threads = True + threading.Thread(target=cls.srv.serve_forever, daemon=True).start() + cls.base = "http://127.0.0.1:%d" % cls.srv.server_port + + @classmethod + def tearDownClass(cls): + cls.srv.shutdown() + cls.srv.server_close() + cls.tmp.cleanup() + + @staticmethod + def write(root, rel, body): + os.makedirs(os.path.dirname(os.path.join(root, rel)), exist_ok=True) + with open(os.path.join(root, rel), "w") as f: + f.write(body) + + def get(self, path, cookie=None): + try: + r = urllib.request.urlopen(urllib.request.Request(self.base + path, headers={"Cookie": cookie} if cookie else {})) + return r.status, r.headers, r.read().decode() + except urllib.error.HTTPError as e: + return e.code, e.headers, e.read().decode() + + def test_page_comes_from_the_chosen_worktree(self): + self.assertIn("Master home", self.get("/")[2]) + self.assertIn("Round one", self.get("/?branch=t/a")[2]) + self.assertIn("Round one", self.get("/", cookie="review_branch=t/a")[2]) + self.assertIn("New page", self.get("/new/?branch=t/a")[2]) + self.assertEqual(self.get("/new/")[0], 404) + + def test_branch_query_sets_the_cookie(self): + headers = self.get("/?branch=t/a")[1] + self.assertEqual(headers["Set-Cookie"], "review_branch=t/a; Path=/; SameSite=Strict") + self.assertIsNone(self.get("/")[1]["Set-Cookie"]) + + def test_invalid_branch_falls_back_to_main(self): + for bad in ("nope", "..%2Fx", "-x", "t/a%00"): + code, headers, body = self.get("/?branch=" + bad) + self.assertEqual(code, 200, bad) + self.assertIn("Master home", body, bad) + self.assertIsNone(headers["Set-Cookie"], bad) + self.assertIn("Master home", self.get("/", cookie="review_branch=nope")[2]) + + def test_versions_lists_rounds_and_working(self): + v = json.loads(self.get("/__review/api/versions?branch=t/a")[2]) + self.assertEqual((v["branch"], v["branches"], v["base"], v["working"]), ("t/a", ["master", "t/a"], "master", False)) + self.assertEqual([(r["n"], r["sha"], r["subject"]) for r in v["rounds"]], + [(1, self.shas[0], "round one"), (2, self.shas[1], "round two")]) + v = json.loads(self.get("/__review/api/versions", cookie="review_branch=t/a")[2]) + self.assertEqual(v["branch"], "t/a") + self.assertEqual(json.loads(self.get("/__review/api/versions")[2])["rounds"], []) + self.write(self.wt, "index.html", PAGE.replace("Hi", "Edited")) + try: + self.assertTrue(json.loads(self.get("/__review/api/versions?branch=t/a")[2])["working"]) + finally: + git(self.wt, "checkout", "index.html") + self.assertEqual(self.get("/__review/api/versions?branch=nope")[0], 400) + + def test_text_returns_old_html_and_null_for_a_new_page(self): + def text(rev, path, branch="t/a"): + code, _, body = self.get("/__review/api/text?branch=%s&rev=%s&path=%s" % (branch, rev, path)) + self.assertEqual(code, 200) + return json.loads(body)["html"] + self.assertIn("Master home", text("master", "/")) + self.assertIn("Round one", text(self.shas[0], "/")) + self.assertIn("Round one", text("working", "/")) + self.assertIsNone(text("master", "/new/")) + self.assertIsNone(text(self.shas[0], "/new/")) + self.assertIn("New page", text(self.shas[1], "/new/")) + self.assertIn("New page", text("working", "/new/index.html")) + self.assertIsNone(text("working", "/missing/")) + + def test_text_refuses_a_foreign_sha_and_a_bad_path(self): + url = "/__review/api/text?branch=t/a&rev=%s&path=%s" + for rev, path in (("deadbeef", "/"), ("-x", "/"), ("master", "/../x"), ("master", "/.git/config"), + ("working", "/.git/config"), ("master", "index.html"), ("master", ""), ("master", "/%00")): + code, _, body = self.get(url % (rev, path)) + self.assertEqual(code, 400, (rev, path)) + self.assertIn("error", json.loads(body)) + self.assertEqual(self.get("/__review/api/text?branch=t/a&path=/")[0], 400) + # a SHA of t/a is not valid for master + self.assertEqual(self.get("/__review/api/text?branch=master&rev=%s&path=/" % self.shas[0])[0], 400) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/review/test_gitread.py b/scripts/review/test_gitread.py index bed60c3..39b7086 100644 --- a/scripts/review/test_gitread.py +++ b/scripts/review/test_gitread.py @@ -73,6 +73,12 @@ def test_dirty_sees_an_edit_and_a_new_file(self): self.assertTrue(gitread.dirty(self.wt)) os.remove(new) + def test_head_gives_branch_and_short_commit(self): + h = gitread.head(self.wt) + self.assertEqual(h["branch"], "t/a") + self.assertEqual(h["commit"], gitread.rounds(self.main, "t/a")[-1]["sha"][:7]) + self.assertEqual(gitread.head(self.tmp.name), {"branch": None, "commit": None}) + def test_show_returns_old_text_and_none_for_missing(self): self.assertEqual(gitread.show(self.main, "master", "a.txt"), "old\n") self.assertEqual(gitread.show(self.main, "t/a", "a.txt"), "two\n") From 97e7d7c23ca6609b5f4854a041de3abdfdd32024 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:10:10 +0100 Subject: [PATCH 04/16] review: refuse encoded hidden paths and symlinks, offer only real worktrees Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- docker-compose.dev.yml | 2 +- scripts/review/devserver.py | 35 ++++++++-- scripts/review/test_devserver.py | 113 +++++++++++++++++++++++++++++-- 3 files changed, 138 insertions(+), 12 deletions(-) diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index ebbb65a..cdb7c5e 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -18,7 +18,7 @@ services: container_name: t7d-review restart: unless-stopped user: "1001:1001" - command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --notes, "${REVIEW_MAIN}/.review/notes", --host, 0.0.0.0, --port, "8000"] + command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --notes, "${REVIEW_MAIN}/.review/notes", --worktrees, "${REVIEW_WORKTREES}", --host, 0.0.0.0, --port, "8000"] environment: PYTHONDONTWRITEBYTECODE: "1" read_only: true diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index 66e5952..f90dfd1 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -130,6 +130,7 @@ def num(v): class Handler(SimpleHTTPRequestHandler): root = "." notes = ".review/notes" + worktrees = None # the folder that holds the site worktrees, or None for any def end_headers(self): self.send_header("Cache-Control", "no-cache") @@ -139,14 +140,31 @@ def log_message(self, fmt, *args): pass # quiet: the reload stream would fill the log def hidden(self): - parts = self.path.split("?", 1)[0].split("/") + parts = urllib.parse.unquote(self.path.split("?", 1)[0]).split("/") return any(p in HIDDEN for p in parts) + def servable(self, path): + """True if path is a file or folder inside the chosen worktree, with no hidden folder in its real path. + A symlink in the worktree cannot lead out of it or into .git.""" + base = os.path.realpath(self.wt) + real = os.path.realpath(path) + if not real.startswith(base + os.sep): + return real == base + return not any(p in HIDDEN for p in os.path.relpath(real, base).split(os.sep)) + + def allowed(self, path): + """A worktree is offered if it is a folder, and below self.worktrees when that is set.""" + if not os.path.isdir(path): + return False + if not self.worktrees: + return True + return os.path.realpath(path).startswith(os.path.realpath(self.worktrees) + os.sep) + def pick(self): """Choose the branch of this request. Sets self.branch, self.wt (its worktree path), self.names, self.cookie_branch (a valid ?branch= to remember) and self.bad_branch (a ?branch= that is invalid or unknown).""" main = gitread.head(self.root)["branch"] - found = {w["branch"]: w["path"] for w in gitread.worktrees(self.root)} + found = {w["branch"]: w["path"] for w in gitread.worktrees(self.root) if self.allowed(w["path"])} if main: found[main] = self.root self.names = ([main] if main else []) + [b for b in found if b != main] @@ -200,6 +218,8 @@ def do_GET(self): if url in GENERATED: return self.send_generated(url) path = self.translate_path(self.path) + if not self.servable(path): + return self.not_found() if os.path.isdir(path): if not url.endswith("/"): self.send_response(301) @@ -207,6 +227,8 @@ def do_GET(self): self.end_headers() return path = os.path.join(path, "index.html") + if not self.servable(path): + return self.not_found() if path.endswith(".html"): return self.send_page(path) if os.path.isfile(path) else self.not_found() if not os.path.isfile(path): @@ -269,9 +291,8 @@ def text(self): return self.send_json(400, {"error": "bad path"}) try: if rev == "working": - full = os.path.realpath(os.path.join(self.wt, rel)) - inside = full.startswith(os.path.realpath(self.wt) + os.sep) - html = read(full) if inside and os.path.isfile(full) else None + full = os.path.join(self.wt, rel) + html = read(full) if self.servable(full) and os.path.isfile(full) else None else: html = gitread.show(self.root, rev, rel) except UnicodeDecodeError: @@ -340,11 +361,13 @@ def main(): ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) ap.add_argument("--root", default=os.path.normpath(os.path.join(HERE, "..", ".."))) ap.add_argument("--notes", default=None, help="the notes folder (default: /.review/notes)") + ap.add_argument("--worktrees", default=os.environ.get("REVIEW_WORKTREES"), + help="offer only the worktrees below this folder (default: env REVIEW_WORKTREES, else any)") ap.add_argument("--host", default="127.0.0.1") ap.add_argument("--port", type=int, default=8000) a = ap.parse_args() root = os.path.abspath(a.root) - handler = partial(type("H", (Handler,), {"root": root, "notes": os.path.abspath(a.notes or os.path.join(root, ".review", "notes"))}), + handler = partial(type("H", (Handler,), {"root": root, "worktrees": a.worktrees and os.path.abspath(a.worktrees), "notes": os.path.abspath(a.notes or os.path.join(root, ".review", "notes"))}), directory=root) srv = ThreadingHTTPServer((a.host, a.port), handler) srv.daemon_threads = True diff --git a/scripts/review/test_devserver.py b/scripts/review/test_devserver.py index 31a9627..fb910ce 100644 --- a/scripts/review/test_devserver.py +++ b/scripts/review/test_devserver.py @@ -1,6 +1,7 @@ """Tests of the review server. Run: python3 -m unittest scripts/review/test_devserver.py""" import json import os +import shutil import subprocess import sys import tempfile @@ -163,15 +164,24 @@ def setUpClass(cls): cls.write(cls.wt, "index.html", PAGE.replace("Hi", "Round one")) git(cls.wt, "commit", "-am", "round one") cls.write(cls.wt, "new/index.html", PAGE.replace("Hi", "New page")) + os.symlink("/etc/hostname", os.path.join(cls.wt, "leak.txt")) + os.symlink("/etc", os.path.join(cls.wt, "leakdir")) + os.symlink(os.path.join(cls.main, ".git", "config"), os.path.join(cls.wt, "gitlink.html")) + os.symlink(".git", os.path.join(cls.wt, "gitself.html")) git(cls.wt, "add", "-A") git(cls.wt, "commit", "-m", "round two") cls.shas = git(cls.main, "log", "--reverse", "--format=%H", "master..t/a").split() - h = partial(type("H", (devserver.Handler,), {"root": cls.main, "notes": os.path.join(cls.main, ".review", "notes")}), + cls.srv, cls.base = cls.serve() + + @classmethod + def serve(cls, worktrees=None): + h = partial(type("H", (devserver.Handler,), {"root": cls.main, "worktrees": worktrees, + "notes": os.path.join(cls.main, ".review", "notes")}), directory=cls.main) - cls.srv = ThreadingHTTPServer(("127.0.0.1", 0), h) - cls.srv.daemon_threads = True - threading.Thread(target=cls.srv.serve_forever, daemon=True).start() - cls.base = "http://127.0.0.1:%d" % cls.srv.server_port + srv = ThreadingHTTPServer(("127.0.0.1", 0), h) + srv.daemon_threads = True + threading.Thread(target=srv.serve_forever, daemon=True).start() + return srv, "http://127.0.0.1:%d" % srv.server_port @classmethod def tearDownClass(cls): @@ -179,6 +189,13 @@ def tearDownClass(cls): cls.srv.server_close() cls.tmp.cleanup() + def get_from(self, base, path, cookie=None): + try: + r = urllib.request.urlopen(urllib.request.Request(base + path, headers={"Cookie": cookie} if cookie else {})) + return r.status, r.headers, r.read().decode() + except urllib.error.HTTPError as e: + return e.code, e.headers, e.read().decode() + @staticmethod def write(root, rel, body): os.makedirs(os.path.dirname(os.path.join(root, rel)), exist_ok=True) @@ -252,6 +269,92 @@ def test_text_refuses_a_foreign_sha_and_a_bad_path(self): # a SHA of t/a is not valid for master self.assertEqual(self.get("/__review/api/text?branch=master&rev=%s&path=/" % self.shas[0])[0], 400) + def test_encoded_hidden_folders_are_not_served(self): + for p in ("/%2egit/HEAD", "/%2Egit/config", "/%2ereview/notes/n.json", "/%2egit?branch=t/a", + "/%2egit/HEAD?branch=t/a", "/a/%2e%2e/%2egit/HEAD"): + self.assertEqual(self.get(p)[0], 404, p) + + def test_symlinks_do_not_leave_the_worktree(self): + for p in ("/leak.txt?branch=t/a", "/leakdir/hostname?branch=t/a", "/gitlink.html?branch=t/a", + "/gitself.html?branch=t/a"): + self.assertEqual(self.get(p)[0], 404, p) + + def test_text_working_does_not_follow_a_symlink_to_hidden_or_outside(self): + for path in ("/gitlink.html", "/gitself.html", "/leak.txt", "/leakdir/hostname"): + code, _, body = self.get("/__review/api/text?branch=t/a&rev=working&path=" + path) + self.assertEqual((code, json.loads(body)["html"]), (200, None), path) + + def test_a_removed_worktree_is_not_offered(self): + gone = os.path.join(os.path.dirname(self.wt), "gone") + git(self.main, "worktree", "add", "-b", "t/gone", gone) + shutil.rmtree(gone) + try: + v = json.loads(self.get("/__review/api/versions")[2]) + self.assertNotIn("t/gone", v["branches"]) + code, headers, body = self.get("/?branch=t/gone") + self.assertIn("Master home", body) + self.assertIsNone(headers["Set-Cookie"]) + self.assertIn("Master home", self.get("/", cookie="review_branch=t/gone")[2]) + finally: + git(self.main, "worktree", "prune") + git(self.main, "branch", "-D", "t/gone") + + def test_a_worktree_outside_the_configured_folder_is_not_offered(self): + srv, base = self.serve(worktrees=os.path.join(os.path.dirname(self.wt), "only-here")) + try: + v = json.loads(self.get_from(base, "/__review/api/versions")[2]) + self.assertEqual(v["branches"], ["master"]) + self.assertIn("Master home", self.get_from(base, "/?branch=t/a")[2]) + finally: + srv.shutdown() + srv.server_close() + srv, base = self.serve(worktrees=os.path.dirname(self.wt)) + try: + self.assertIn("Round one", self.get_from(base, "/?branch=t/a")[2]) + finally: + srv.shutdown() + srv.server_close() + + def test_reload_stamps_the_chosen_worktree(self): + def first_stamp(path): + r = urllib.request.urlopen(self.base + path) + line = r.readline().decode() + r.close() + return line + before = first_stamp("/__review/reload?branch=t/a") + self.write(self.main, "main-only.txt", "x") + self.assertEqual(first_stamp("/__review/reload?branch=t/a"), before) + self.write(self.wt, "wt-only.txt", "x") + try: + self.assertNotEqual(first_stamp("/__review/reload?branch=t/a"), before) + finally: + os.remove(os.path.join(self.wt, "wt-only.txt")) + os.remove(os.path.join(self.main, "main-only.txt")) + + def test_state_reports_the_worktree(self): + s = json.loads(self.get("/__review/api/state?branch=t/a")[2]) + self.assertEqual((s["branch"], s["commit"]), ("t/a", self.shas[1][:7])) + s = json.loads(self.get("/__review/api/state")[2]) + self.assertEqual(s["branch"], "master") + + def test_feeds_and_404_come_from_the_chosen_worktree(self): + post = 'Wt post ยท Tim Conrad' + self.write(self.wt, "thoughts/w/index.html", post) + self.write(self.wt, "404.html", PAGE.replace("Hi", "Wt gone")) + try: + self.assertIn("Wt post", self.get("/thoughts/feed.xml?branch=t/a")[2]) + self.assertNotIn("Wt post", self.get("/thoughts/feed.xml")[2]) + code, _, body = self.get("/nope/?branch=t/a") + self.assertEqual(code, 404) + self.assertIn("Wt gone", body) + finally: + shutil.rmtree(os.path.join(self.wt, "thoughts")) + os.remove(os.path.join(self.wt, "404.html")) + + def test_query_wins_over_the_cookie(self): + self.assertIn("Master home", self.get("/?branch=master", cookie="review_branch=t/a")[2]) + self.assertIn("Round one", self.get("/", cookie="review_branch=t/a")[2]) + if __name__ == "__main__": unittest.main() From 9f7c96c64271968d3004d9178e356ad565803fe1 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:12:24 +0100 Subject: [PATCH 05/16] review: a word diff for the visible text Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- scripts/review/test_textdiff.mjs | 127 +++++++++++++++++++++++ scripts/review/textdiff.js | 171 +++++++++++++++++++++++++++++++ 2 files changed, 298 insertions(+) create mode 100644 scripts/review/test_textdiff.mjs create mode 100644 scripts/review/textdiff.js diff --git a/scripts/review/test_textdiff.mjs b/scripts/review/test_textdiff.mjs new file mode 100644 index 0000000..9dc05be --- /dev/null +++ b/scripts/review/test_textdiff.mjs @@ -0,0 +1,127 @@ +import test from "node:test"; +import assert from "node:assert"; +import { createRequire } from "node:module"; + +const { blocks, diffWords, diffBlocks } = createRequire(import.meta.url)("./textdiff.js"); + +test("diffWords marks a changed word", () => { + assert.deepStrictEqual(diffWords("a b c", "a x c"), [ + { op: "eq", text: "a " }, + { op: "del", text: "b" }, + { op: "add", text: "x" }, + { op: "eq", text: " c" }, + ]); +}); + +test("diffWords gives one eq for equal input", () => { + assert.deepStrictEqual(diffWords("a b c", "a b c"), [{ op: "eq", text: "a b c" }]); +}); + +test("diffWords gives one add from empty text", () => { + assert.deepStrictEqual(diffWords("", "a b"), [{ op: "add", text: "a b" }]); +}); + +test("diffWords gives one del to empty text", () => { + assert.deepStrictEqual(diffWords("a b", ""), [{ op: "del", text: "a b" }]); +}); + +const words = (n, tag) => Array.from({ length: n }, (_, i) => `${tag}${i}`).join(" "); + +test("diffWords: 10k words with a few edits finishes in under 500 ms", () => { + const a = words(10000, "w"); + const b = a.replace("w100 ", "x ").replace("w5000 ", "").replace("w9000", "w9000 y z"); + const t0 = performance.now(); + const ops = diffWords(a, b); + assert.ok(performance.now() - t0 < 500); + assert.ok(ops.some((o) => o.op === "add") && ops.some((o) => o.op === "del")); + const rebuilt = ops.filter((o) => o.op !== "del").map((o) => o.text).join(""); + assert.strictEqual(rebuilt, b); +}); + +test("diffWords: two fully different 10k texts finish in under 500 ms", () => { + const a = words(10000, "a"); + const b = words(10000, "b"); + const t0 = performance.now(); + const ops = diffWords(a, b); + assert.ok(performance.now() - t0 < 500); + assert.deepStrictEqual(ops, [{ op: "del", text: a }, { op: "add", text: b }]); +}); + +const blk = (key, text) => ({ key, text }); + +test("diffBlocks pairs a changed paragraph as chg and keeps a new one as add", () => { + const a = [blk("p0", "one two three four five"), blk("p1", "stay the same")]; + const b = [ + blk("p0", "one two three four six"), + blk("p1", "stay the same"), + blk("p2", "completely new text here"), + ]; + const ops = diffBlocks(a, b); + assert.deepStrictEqual(ops.map((o) => o.op), ["chg", "eq", "add"]); + assert.strictEqual(ops[0].a.key, "p0"); + assert.strictEqual(ops[0].b.text, "one two three four six"); + assert.ok(ops[0].words.some((w) => w.op === "del" && w.text === "five")); + assert.strictEqual(ops[2].b.key, "p2"); +}); + +test("diffBlocks keeps del and add apart when less than 40 % is shared", () => { + const ops = diffBlocks([blk("p0", "alpha beta gamma delta")], [blk("p0", "alpha one two three")]); + assert.deepStrictEqual(ops.map((o) => o.op), ["del", "add"]); +}); + +// A minimal fake element tree. It supports only what blocks() uses. +function el(tagName, children = [], parent = null) { + const node = { + tagName: tagName.toUpperCase(), children, parent, attrs: {}, + get textContent() { + return this.children.map((c) => (typeof c === "string" ? c : c.textContent)).join(""); + }, + closest(sel) { + const parts = sel.split(",").map((s) => s.trim()); + for (let n = this; n; n = n.parent) { + if (parts.some((p) => (p.startsWith("[") ? p.slice(1, -1) in n.attrs : p === n.tagName.toLowerCase()))) return n; + } + return null; + }, + all(sel) { + const tags = sel.split(",").map((s) => s.trim().toUpperCase()); + const out = []; + const walk = (n) => n.children.forEach((c) => { + if (typeof c === "string") return; + if (tags.includes(c.tagName)) out.push(c); + walk(c); + }); + walk(this); + return out; + }, + querySelectorAll(sel) { return this.all(sel); }, + querySelector(sel) { return this.all(sel)[0] || null; }, + }; + children.forEach((c) => { if (typeof c !== "string") c.parent = node; }); + return node; +} + +test("blocks reads main, numbers tags, and skips review UI and papercuts", () => { + const bar = el("p", ["review bar text"]); + bar.attrs["data-review-ui"] = ""; + const inWidget = el("p", ["widget text"]); + const widget = el("papercuts-widget", [inWidget]); + const quote = el("blockquote", [el("p", ["quoted"])]); + const main = el("main", [ + el("h1", ["Title"]), + el("p", ["First line\n here"]), + el("p", [" "]), + el("p", ["Second"]), + quote, + bar, + widget, + ]); + const outside = el("p", ["outside main"]); + const body = el("body", [outside, main]); + assert.deepStrictEqual(blocks(body), [ + { key: "h10", text: "Title" }, + { key: "p0", text: "First line here" }, + { key: "p1", text: "Second" }, + { key: "p2", text: "quoted" }, + ]); +}); diff --git a/scripts/review/textdiff.js b/scripts/review/textdiff.js new file mode 100644 index 0000000..5f5b248 --- /dev/null +++ b/scripts/review/textdiff.js @@ -0,0 +1,171 @@ +// Word diff for the review site. Sets window.reviewDiff in a browser and module.exports in node. +(function () { + "use strict"; + + var BLOCK_SELECTOR = "h1, h2, h3, h4, h5, h6, p, li, blockquote, figcaption, td, th, pre"; + var SKIP_SELECTOR = "[data-review-ui], papercuts-widget"; + // Myers stops after this many edited tokens. The block pair then becomes "del all + add all". + var MAX_EDITS = 1000; + // Two blocks pair as a change when more than this share of their words is equal. + var PAIR_RATIO = 0.4; + + function tokens(s) { + return s.split(/(\s+)/).filter(function (t) { return t !== ""; }); + } + + function push(out, op, text) { + var last = out[out.length - 1]; + if (last && last.op === op) last.text += text; + else out.push({ op: op, text: text }); + } + + // Myers shortest edit script. Returns a list of {op, text} per token, or null if more than MAX_EDITS. + function myers(a, b) { + var n = a.length, m = b.length, max = Math.min(n + m, MAX_EDITS); + var off = max + 1; + var v = new Int32Array(2 * max + 3); + var trace = []; + var found = -1; + for (var d = 0; d <= max && found < 0; d++) { + trace.push(v.slice()); + for (var k = -d; k <= d; k += 2) { + var x; + if (k === -d || (k !== d && v[off + k - 1] < v[off + k + 1])) x = v[off + k + 1]; + else x = v[off + k - 1] + 1; + var y = x - k; + while (x < n && y < m && a[x] === b[y]) { x++; y++; } + v[off + k] = x; + if (x >= n && y >= m) { found = d; break; } + } + } + if (found < 0) return null; + var ops = []; + var px = n, py = m; + for (var dd = found; dd > 0; dd--) { + var vv = trace[dd]; + var kk = px - py; + var prevK = (kk === -dd || (kk !== dd && vv[off + kk - 1] < vv[off + kk + 1])) ? kk + 1 : kk - 1; + var prevX = vv[off + prevK], prevY = prevX - prevK; + while (px > prevX && py > prevY) { px--; py--; ops.push({ op: "eq", text: a[px] }); } + if (px === prevX) { py--; ops.push({ op: "add", text: b[py] }); } + else { px--; ops.push({ op: "del", text: a[px] }); } + } + while (px > 0 && py > 0) { px--; py--; ops.push({ op: "eq", text: a[px] }); } + return ops.reverse(); + } + + function diffWords(a, b) { + var ta = tokens(a), tb = tokens(b); + var lo = 0; + while (lo < ta.length && lo < tb.length && ta[lo] === tb[lo]) lo++; + var ha = ta.length, hb = tb.length; + while (ha > lo && hb > lo && ta[ha - 1] === tb[hb - 1]) { ha--; hb--; } + var mid = myers(ta.slice(lo, ha), tb.slice(lo, hb)); + if (!mid) { + mid = ta.slice(lo, ha).map(function (t) { return { op: "del", text: t }; }) + .concat(tb.slice(lo, hb).map(function (t) { return { op: "add", text: t }; })); + } + var out = []; + ta.slice(0, lo).forEach(function (t) { push(out, "eq", t); }); + mid.forEach(function (o) { push(out, o.op, o.text); }); + ta.slice(ha).forEach(function (t) { push(out, "eq", t); }); + return out; + } + + // LCS over block texts. Returns a list of {op, a?, b?}. + function lcsBlocks(a, b) { + var lo = 0; + while (lo < a.length && lo < b.length && a[lo].text === b[lo].text) lo++; + var ha = a.length, hb = b.length; + while (ha > lo && hb > lo && a[ha - 1].text === b[hb - 1].text) { ha--; hb--; } + var n = ha - lo, m = hb - lo, w = m + 1; + var t = new Uint32Array((n + 1) * w); + for (var i = n - 1; i >= 0; i--) { + for (var j = m - 1; j >= 0; j--) { + t[i * w + j] = a[lo + i].text === b[lo + j].text + ? t[(i + 1) * w + j + 1] + 1 + : Math.max(t[(i + 1) * w + j], t[i * w + j + 1]); + } + } + var out = []; + for (var p = 0; p < lo; p++) out.push({ op: "eq", a: a[p], b: b[p] }); + var x = 0, y = 0; + while (x < n || y < m) { + if (x < n && y < m && a[lo + x].text === b[lo + y].text) { + out.push({ op: "eq", a: a[lo + x], b: b[lo + y] }); x++; y++; + } else if (y >= m || (x < n && t[(x + 1) * w + y] >= t[x * w + y + 1])) { + out.push({ op: "del", a: a[lo + x] }); x++; + } else { + out.push({ op: "add", b: b[lo + y] }); y++; + } + } + for (var q = ha; q < a.length; q++) out.push({ op: "eq", a: a[q], b: b[hb + q - ha] }); + return out; + } + + function wordCount(s) { + var m = s.match(/\S+/g); + return m ? m.length : 0; + } + + // Share of equal words, relative to the longer of the two blocks. Returns {ratio, words}. + function similarity(x, y) { + var words = diffWords(x.text, y.text); + var shared = 0; + words.forEach(function (o) { if (o.op === "eq") shared += wordCount(o.text); }); + var longest = Math.max(wordCount(x.text), wordCount(y.text)); + return { ratio: longest ? shared / longest : 0, words: words }; + } + + function diffBlocks(a, b) { + var raw = lcsBlocks(a, b); + var out = []; + var i = 0; + while (i < raw.length) { + if (raw[i].op === "eq") { out.push(raw[i++]); continue; } + var dels = [], adds = []; + while (i < raw.length && raw[i].op !== "eq") { + if (raw[i].op === "del") dels.push(raw[i].a); else adds.push(raw[i].b); + i++; + } + var next = 0; + dels.forEach(function (d) { + for (var k = next; k < adds.length; k++) { + var s = similarity(d, adds[k]); + if (s.ratio > PAIR_RATIO) { + for (var r = next; r < k; r++) out.push({ op: "add", b: adds[r] }); + out.push({ op: "chg", a: d, b: adds[k], words: s.words }); + next = k + 1; + return; + } + } + out.push({ op: "del", a: d }); + }); + for (var r2 = next; r2 < adds.length; r2++) out.push({ op: "add", b: adds[r2] }); + } + return out; + } + + // Visible text of main (else body) as blocks. Uses only querySelector(All), closest and textContent. + function blocks(doc) { + var root = (doc.querySelector && (doc.querySelector("main") || doc.querySelector("body"))) || doc; + var counts = {}; + var out = []; + Array.prototype.forEach.call(root.querySelectorAll(BLOCK_SELECTOR), function (el) { + if (el.closest(SKIP_SELECTOR)) return; + // The inner blocks carry the text of a container such as blockquote > p. + if (el.querySelector(BLOCK_SELECTOR)) return; + var text = (el.textContent || "").replace(/\s+/g, " ").trim(); + if (!text) return; + var tag = el.tagName.toLowerCase(); + var idx = counts[tag] || 0; + counts[tag] = idx + 1; + out.push({ key: tag + idx, text: text }); + }); + return out; + } + + var api = { blocks: blocks, diffWords: diffWords, diffBlocks: diffBlocks }; + if (typeof module !== "undefined" && module.exports) module.exports = api; + if (typeof window !== "undefined") window.reviewDiff = api; +})(); From 312e6132f27f498d99f0aefe55574abebdfd84d0 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:22:12 +0100 Subject: [PATCH 06/16] review: a version picker and the changes of each round in the page The note tab goes away. A bar at the bottom of the page holds the branch select, the "from" and "to" versions and the switch "Show changes". The page marks new words with ins and old words with del, and shows a removed block struck through. textdiff.js reads dt and dd too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- scripts/review/check_review.py | 242 ++++++++++++++++++++++ scripts/review/devserver.py | 12 +- scripts/review/review.css | 77 +++---- scripts/review/review.js | 344 +++++++++++++++++++++---------- scripts/review/test_devserver.py | 4 + scripts/review/test_textdiff.mjs | 20 +- scripts/review/textdiff.js | 15 +- 7 files changed, 549 insertions(+), 165 deletions(-) create mode 100644 scripts/review/check_review.py diff --git a/scripts/review/check_review.py b/scripts/review/check_review.py new file mode 100644 index 0000000..b8c05a2 --- /dev/null +++ b/scripts/review/check_review.py @@ -0,0 +1,242 @@ +#!/usr/bin/env python3 +"""Browser check of the review bar: the version picker and the marks of a round. + + uv run -q --no-project --with playwright==1.63.0 python3 scripts/review/check_review.py [--shots DIR] + +Makes a temporary git repo (master and the worktree t/a with two rounds), starts devserver.py on a free port and +opens the pages in Chromium at 1440, 390 and 320 px, in the light and the dark theme. Exit code 1 if a check fails. +Round 1 changes the heading. Round 2 changes one word, removes one paragraph, adds one and adds the page /new/. +""" +import argparse +import os +import shutil +import socket +import subprocess +import sys +import tempfile +import time +import urllib.request + +from playwright.sync_api import sync_playwright + +HERE = os.path.dirname(os.path.abspath(__file__)) +REPO = os.path.normpath(os.path.join(HERE, "..", "..")) +CSP = ("default-src 'none'; style-src 'self'; script-src 'self'; img-src 'self'; font-src 'self'; " + "base-uri 'none'; form-action 'none'") +P1_OLD = "The first paragraph has a quick word in it." +P1_NEW = "The first paragraph has a slow word in it." +P2 = "This second paragraph goes away in round two." +P3 = "The third paragraph stays the same." +P4 = "Round two adds this closing text." +SIZES = ((1440, 900), (390, 844), (320, 640)) +MARKS = "ins.rv-add, del.rv-del, .rv-gone, .rv-newpage" + + +def page(title, paras): + body = "\n".join("

%s

" % p for p in paras) + return ('' + '' + '%s' + '' + '' + '

%s

\n%s\n
') % (CSP, title, title, body) + + +def git(cwd, *args): + subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, + capture_output=True) + + +def write(root, rel, text): + os.makedirs(os.path.dirname(os.path.join(root, rel)), exist_ok=True) + with open(os.path.join(root, rel), "w", encoding="utf-8") as f: + f.write(text) + + +def fixture(base): + """The main clone (master) and the worktree of t/a below base/wts. Returns (main, wts).""" + main, wts = os.path.join(base, "main"), os.path.join(base, "wts") + os.makedirs(os.path.join(main, "assets")) + for f in ("site.css", "theme.js", "favicon.svg"): + shutil.copy(os.path.join(REPO, "assets", f), os.path.join(main, "assets", f)) + shutil.copytree(os.path.join(REPO, "assets", "fonts"), os.path.join(main, "assets", "fonts")) + write(main, "index.html", page("Fixture", [P1_OLD, P2, P3])) + git(main, "init", "-b", "master") + git(main, "add", "-A") + git(main, "commit", "-m", "base") + wt = os.path.join(wts, "t-a") + git(main, "worktree", "add", "-b", "t/a", wt) + write(wt, "index.html", page("Fixture page", [P1_OLD, P2, P3])) + git(wt, "commit", "-am", "A longer heading") + write(wt, "index.html", page("Fixture page", [P1_NEW, P3, P4])) + write(wt, "new/index.html", page("New", ["A page that round two adds."])) + git(wt, "add", "-A") + git(wt, "commit", "-m", "Change a word, remove and add a paragraph") + return main, wts + + +def free_port(): + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +def start(main, wts): + port = free_port() + proc = subprocess.Popen([sys.executable, os.path.join(HERE, "devserver.py"), "--root", main, "--worktrees", wts, + "--port", str(port)], stdout=subprocess.DEVNULL) + base = "http://127.0.0.1:%d" % port + for _ in range(100): + try: + urllib.request.urlopen(base + "/__review/api/state", timeout=1) + return proc, base + except OSError: + time.sleep(0.1) + proc.kill() + raise SystemExit("devserver.py did not start") + + +class Check: + def __init__(self): + self.failed = [] + + def that(self, ok, what): + print(("ok " if ok else "FAIL ") + what) + if not ok: + self.failed.append(what) + + +def texts(pg, sel): + return pg.eval_on_selector_all(sel, "ns => ns.map(n => n.textContent)") + + +def no_side_scroll(pg): + return pg.evaluate("document.documentElement.scrollWidth <= document.documentElement.clientWidth") + + +def wait_for(pg, expr): + """Poll expr in the page. wait_for_function cannot run: the page CSP has no 'unsafe-eval'.""" + for _ in range(50): + if pg.evaluate(expr): + return + pg.wait_for_timeout(100) + raise AssertionError("timeout: " + expr) + + +def bar_ready(pg): + pg.wait_for_selector(".rv-bar[data-ready]", state="attached") + + +def run(base, shots): + c = Check() + with sync_playwright() as p: + browser = p.chromium.launch() + for w, h in SIZES: + for scheme in ("light", "dark"): + tag = "%d %s" % (w, scheme) + ctx = browser.new_context(viewport={"width": w, "height": h}, color_scheme=scheme) + ctx.set_default_timeout(5000) + pg = ctx.new_page() + errors = [] + pg.on("console", lambda m, e=errors: m.type == "error" and e.append(m.text)) + pg.on("pageerror", lambda x, e=errors: e.append(str(x))) + + # master has no rounds: only the branch select + pg.goto(base + "/?branch=master") + bar_ready(pg) + c.that(pg.is_visible(".rv-branch-pick") and not pg.is_visible(".rv-from") + and not pg.is_visible(".rv-to") and not pg.is_visible(".rv-switch"), + tag + ": on master the bar shows only the branch select") + c.that(pg.locator(".rv-approve").count() == 1 and pg.locator(".rv-approve").is_hidden(), + tag + ": the approve button is hidden") + c.that(pg.locator(".rv-tab, .rv-panel").count() == 0, tag + ": the note tab is gone") + + # the branch select opens t/a, with Round 2 against Round 1 + with pg.expect_navigation(): + pg.select_option(".rv-branch-pick", "t/a") + bar_ready(pg) + c.that("branch=t%2Fa" in pg.url or "branch=t/a" in pg.url, tag + ": the branch select loads t/a") + c.that(pg.input_value(".rv-to") == pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value"), + tag + ": to is the newest round") + c.that(pg.eval_on_selector(".rv-from", "s => s.options[s.selectedIndex].text").startswith("Round 1"), + tag + ": from is Round 1") + c.that(texts(pg, "ins.rv-add") == ["slow", P4], tag + ": ins marks the new word and the new paragraph (%r)" % texts(pg, "ins.rv-add")) + c.that(texts(pg, "del.rv-del") == ["quick"], tag + ": del marks the old word (%r)" % texts(pg, "del.rv-del")) + c.that(texts(pg, ".rv-gone") == [P2], tag + ": rv-gone holds the removed paragraph") + c.that(pg.evaluate("document.querySelector('.rv-gone').nextElementSibling.textContent") == P3, + tag + ": rv-gone is before the next kept paragraph") + c.that(pg.evaluate("[...document.querySelectorAll('[class^=rv-], [class*=\" rv-\"]')]" + ".filter(n => !n.closest('[data-review-ui]') && !n.matches('ins, del')).length") == 0, + tag + ": every review element outside the marks carries data-review-ui") + c.that(no_side_scroll(pg), tag + ": no horizontal scroll with marks") + small = pg.evaluate("[...document.querySelectorAll('.rv-bar select, .rv-bar label, .rv-bar button')]" + ".filter(n => n.offsetParent).map(n => n.getBoundingClientRect().height).filter(x => x < 40)") + c.that(small == [], tag + ": touch targets are 40 px or more (%r)" % small) + if shots: + pg.screenshot(path=os.path.join(shots, "marks-%d-%s.png" % (w, scheme)), full_page=True) + + # the switch off removes every mark and keeps the text of round 2; the choice stays after a reload + pg.click(".rv-switch") + c.that(pg.locator(MARKS).count() == 0, tag + ": switch off removes every mark") + c.that(texts(pg, "main p") == [P1_NEW, P3, P4], tag + ": switch off shows the text of round 2") + pg.reload() + bar_ready(pg) + c.that(not pg.is_checked(".rv-switch input") and pg.locator(MARKS).count() == 0, + tag + ": the switch stays off after a reload") + pg.click(".rv-switch") + wait_for(pg, "document.querySelectorAll('del.rv-del').length === 1") + c.that(texts(pg, "del.rv-del") == ["quick"] and texts(pg, ".rv-gone") == [P2], + tag + ": switch on shows the marks again") + + # to = Round 1 is an older round: the page shows round 1, marked against master + pg.select_option(".rv-from", "master") + to1 = pg.eval_on_selector(".rv-to", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value") + pg.select_option(".rv-to", to1) + wait_for(pg, "document.querySelector('main h1 ins.rv-add') !== null") + c.that(texts(pg, "main > p") == [P1_OLD, P2, P3], tag + ": to = Round 1 shows the text of round 1") + c.that(texts(pg, "ins.rv-add") == [" page"] and pg.locator("del.rv-del, .rv-gone").count() == 0, + tag + ": Round 1 against master marks only the heading (%r)" % texts(pg, "ins.rv-add")) + if shots and w != 1440: + pg.screenshot(path=os.path.join(shots, "round1-%d-%s.png" % (w, scheme))) + pg.select_option(".rv-to", pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value")) + wait_for(pg, "document.querySelectorAll('main > p').length === 3") + wait_for(pg, "document.querySelectorAll('.rv-gone').length === 1") + c.that(texts(pg, "ins.rv-add") == [" page", "slow", P4] and texts(pg, "del.rv-del") == ["quick"], + tag + ": back to Round 2 shows the page on disk, marked against master (%r)" % texts(pg, "ins.rv-add")) + + # a page new in "to" + pg.select_option(".rv-from", pg.eval_on_selector(".rv-from", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value")) + pg.goto(base + "/new/?branch=t/a") + bar_ready(pg) + c.that(texts(pg, ".rv-newpage") == ["New page in this round"], tag + ": a new page has one line") + c.that(pg.locator("ins.rv-add, del.rv-del, .rv-gone").count() == 0, tag + ": a new page has no other mark") + c.that(no_side_scroll(pg), tag + ": no horizontal scroll on the new page") + if shots and w == 390: + pg.screenshot(path=os.path.join(shots, "newpage-%d-%s.png" % (w, scheme))) + + c.that(errors == [], tag + ": no console error (%r)" % errors) + ctx.close() + browser.close() + return c + + +def main(): + ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + ap.add_argument("--shots", help="save screenshots to this folder") + a = ap.parse_args() + if a.shots: + os.makedirs(a.shots, exist_ok=True) + with tempfile.TemporaryDirectory() as tmp: + main_clone, wts = fixture(os.path.realpath(tmp)) + proc, base = start(main_clone, wts) + try: + c = run(base, a.shots) + finally: + proc.terminate() + proc.wait() + print("%d checks failed" % len(c.failed) if c.failed else "all checks passed") + sys.exit(1 if c.failed else 0) + + +if __name__ == "__main__": + main() diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index f90dfd1..bccce00 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -1,10 +1,10 @@ #!/usr/bin/env python3 -"""Review server for the site: live reload and notes. Only the review site uses it. GitHub Pages serves t7d.dev. +"""Review server for the site: live reload, the rounds of a branch and notes. Only the review site uses it. GitHub Pages serves t7d.dev. python3 scripts/review/devserver.py [--root .] [--notes .review/notes] [--host 127.0.0.1] [--port 8000] - Serves the repository root with Cache-Control: no-cache. A missing page gets /404.html with status 404. -- Every HTML page gets /__review/review.css and /__review/review.js. The page CSP gets connect-src 'self', so the +- Every HTML page gets /__review/review.css, /__review/textdiff.js and /__review/review.js. The page CSP gets connect-src 'self', so the script can reach this server. Nothing else in the CSP changes, so the review site keeps the rules of the real site. - /__review/reload: server-sent events. The stamp changes when a file of the site changes, is added or is deleted. The page then loads again. @@ -15,7 +15,7 @@ - /__review/api/text?branch=&rev=&path=: the HTML of a page at that revision, or null. - /__review/api/notes: GET lists the notes, open and done. POST writes one note as /.json. The POST needs the header X-T7d-Review and a JSON body: a plain form from another site cannot send either. A handled note goes to - /done/ with an "answer" field. The page shows the answer under "My notes". + /done/ with an "answer" field. - /llms.txt, /thoughts/feed.xml and /blog/feed.xml are made on each request by scripts/gen-feeds.py, as the deploy makes them for gh-pages. - /.git, /.review, /.reels and /node_modules are not served. """ @@ -39,7 +39,8 @@ PREFIX = "/__review/" COOKIE = "review_branch" BASE = "master" -ASSETS = {"review.js": "text/javascript; charset=utf-8", "review.css": "text/css; charset=utf-8"} +ASSETS = {"review.js": "text/javascript; charset=utf-8", "textdiff.js": "text/javascript; charset=utf-8", + "review.css": "text/css; charset=utf-8"} MAX_BODY = 32 * 1024 MAX_NOTE = 4000 NOTE_ID = re.compile(r"^\d{8}-\d{6}-[0-9a-f]{6}$") @@ -91,7 +92,8 @@ def csp(m): return m.group(1) + policy + m.group(3) html = CSP.sub(csp, html, count=1) link = '' % PREFIX - script = '' % PREFIX + # textdiff.js first: deferred scripts run in order, and review.js uses window.reviewDiff. + script = '\n' % (PREFIX, PREFIX) html = html.replace("", link + "\n" + script + "\n", 1) if "" in html else link + script + html return html diff --git a/scripts/review/review.css b/scripts/review/review.css index e04788a..f16bb90 100644 --- a/scripts/review/review.css +++ b/scripts/review/review.css @@ -1,49 +1,36 @@ -/* The note tab of the review site (scripts/review/devserver.py). Black and white, as the site (DESIGN.md). */ -.rv-tab { - position: fixed; right: 0; top: 50%; z-index: 2147483000; - transform: translateY(-50%) rotate(180deg); writing-mode: vertical-rl; - padding: 0.9rem 0.45rem; border: var(--rule, 1.5px) solid var(--ink, #111); border-left: 0; - background: var(--bg, #fff); color: var(--ink, #111); - font: 600 0.8rem/1 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; cursor: pointer; +/* The review bar and the marks of a round (scripts/review/devserver.py). Black and white, as the site (DESIGN.md). */ +.rv-bar { + position: sticky; bottom: 0; z-index: 2147483000; + display: flex; flex-wrap: wrap; align-items: center; gap: 0.5rem 0.9rem; + padding: 0.5rem var(--pad, 1rem); border-top: var(--rule, 1.5px) solid var(--ink, #111); + background: var(--bg, #fff); color: var(--ink, #111); font: 0.9rem/1.3 var(--body, sans-serif); } -.rv-tab:focus-visible, .rv-panel button:focus-visible, .rv-text:focus-visible { outline: 2px solid var(--ink, #111); outline-offset: 2px; } -.rv-badge:not(:empty) { - display: inline-block; margin-top: 0.4rem; padding: 0.2rem 0.15rem; - background: var(--ink, #111); color: var(--bg, #fff); writing-mode: horizontal-tb; transform: rotate(180deg); +.rv-bar [hidden] { display: none !important; } +.rv-field { display: flex; flex: 1 1 11rem; align-items: center; gap: 0.45rem; min-width: 0; min-height: 40px; } +.rv-label { flex: none; min-width: 3.4rem; font: 500 0.72rem/1 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; color: var(--muted, #5c5c5c); } +.rv-field select { + flex: 1 1 auto; width: 100%; min-width: 0; min-height: 40px; padding: 0 0.5rem; + border: var(--rule, 1.5px) solid var(--ink, #111); border-radius: 0; + background: var(--bg, #fff); color: var(--ink, #111); font: inherit; text-overflow: ellipsis; } -.rv-panel { - position: fixed; right: 2.4rem; bottom: 1rem; z-index: 2147483001; - width: min(24rem, calc(100vw - 3.4rem)); max-height: calc(100vh - 2rem); overflow: auto; - padding: 1rem; border: var(--rule, 1.5px) solid var(--ink, #111); - background: var(--bg, #fff); color: var(--ink, #111); font: 0.9rem/1.4 var(--body, sans-serif); - box-shadow: 0.35rem 0.35rem 0 var(--ink, #111); +.rv-switch { display: inline-flex; align-items: center; gap: 0.5rem; min-height: 40px; cursor: pointer; white-space: nowrap; } +.rv-switch input { width: 1.2rem; height: 1.2rem; margin: 0; accent-color: var(--ink, #111); cursor: pointer; } +.rv-approve { + min-height: 40px; padding: 0 1rem; border: var(--rule, 1.5px) solid var(--ink, #111); + background: var(--ink, #111); color: var(--bg, #fff); font: 600 0.75rem var(--mono, monospace); text-transform: uppercase; cursor: pointer; } -.rv-panel[hidden], .rv-box[hidden], .rv-panel [hidden] { display: none; } -.rv-head { display: flex; align-items: baseline; gap: 0.6rem; margin-bottom: 0.6rem; } -.rv-head strong { font: 600 1.05rem/1.1 var(--display, sans-serif); text-transform: uppercase; } -.rv-branch { flex: 1; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; font: 0.72rem var(--mono, monospace); color: var(--muted, #5c5c5c); } -.rv-text { - display: block; box-sizing: border-box; width: 100%; padding: 0.5rem; resize: vertical; - border: var(--rule, 1.5px) solid var(--ink, #111); background: var(--bg, #fff); color: var(--ink, #111); font: inherit; +.rv-status:empty { display: none; } +.rv-status { min-width: 0; overflow-wrap: anywhere; color: var(--muted, #5c5c5c); font-size: 0.82rem; } +.rv-bar select:focus-visible, .rv-bar input:focus-visible, .rv-bar button:focus-visible { outline: 2px solid var(--ink, #111); outline-offset: 2px; } + +/* the marks: they change no width, so long words wrap */ +ins.rv-add { color: inherit; background: none; text-decoration: underline 2px; text-underline-offset: 0.18em; } +del.rv-del, .rv-gone { color: inherit; text-decoration: line-through; opacity: 0.55; } +del.rv-del + ins.rv-add { margin-left: 0.25em; } +/* no margin on .rv-gone: the page spaces its blocks (.prose is a grid with a gap) */ +.rv-gone, .rv-newpage, .rv-missing { max-width: 100%; overflow-wrap: anywhere; } +.rv-newpage, .rv-missing { margin: 0.75rem 0; } +.rv-newpage { + padding: 0.4rem 0; border-bottom: var(--rule, 1.5px) solid var(--ink, #111); + font: 500 0.75rem/1.3 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; } -.rv-row { display: flex; flex-wrap: wrap; align-items: center; gap: 0.5rem; margin-top: 0.6rem; } -.rv-btn, .rv-x { - min-height: 2.2rem; padding: 0.3rem 0.7rem; border: var(--rule, 1.5px) solid var(--ink, #111); - background: var(--bg, #fff); color: var(--ink, #111); font: 600 0.75rem var(--mono, monospace); text-transform: uppercase; cursor: pointer; -} -.rv-send { background: var(--ink, #111); color: var(--bg, #fff); } -.rv-btn:disabled { opacity: 0.5; cursor: wait; } -.rv-link { border: 0; background: none; color: var(--ink, #111); text-decoration: underline; font: inherit; cursor: pointer; } -.rv-part, .rv-status { min-width: 0; overflow-wrap: anywhere; color: var(--muted, #5c5c5c); font-size: 0.82rem; } -.rv-list-head { margin-top: 1rem; padding-top: 0.6rem; border-top: 1px solid var(--soft, #d9d9d9); font: 600 0.72rem var(--mono, monospace); text-transform: uppercase; } -.rv-list { margin: 0.4rem 0 0; padding: 0; list-style: none; } -.rv-item, .rv-empty { padding: 0.45rem 0; border-bottom: 1px solid var(--soft, #d9d9d9); font-size: 0.82rem; } -.rv-item span { display: block; overflow-wrap: anywhere; } -.rv-meta { font: 0.7rem var(--mono, monospace); color: var(--muted, #5c5c5c); } -.rv-answer { margin-top: 0.25rem; padding-left: 0.6rem; border-left: 3px solid var(--ink, #111); } -.rv-done .rv-note { color: var(--muted, #5c5c5c); } -.rv-box { - position: fixed; z-index: 2147482999; pointer-events: none; box-sizing: border-box; - border: 3px solid #e0201b; background: rgba(224, 32, 27, 0.08); -} -.rv-picking, .rv-picking * { cursor: crosshair !important; } diff --git a/scripts/review/review.js b/scripts/review/review.js index 426b4d9..a6b640b 100644 --- a/scripts/review/review.js +++ b/scripts/review/review.js @@ -1,133 +1,259 @@ -/* The review site (scripts/review/devserver.py): live reload and the note tab. Never on t7d.dev. */ +/* The review site (scripts/review/devserver.py): live reload, the branch picker and the changes of a round. + Never on t7d.dev. Needs window.reviewDiff from /__review/textdiff.js. */ (function () { 'use strict'; var API = '/__review/api/'; + var diff = window.reviewDiff; + /* the branch of this page: ?branch= of the address, else the server uses the cookie */ + var asked = new URLSearchParams(location.search).get('branch'); + function q(branch) { return branch ? 'branch=' + encodeURIComponent(branch) : ''; } /* live reload: the first stamp is the page's own; a new stamp loads the page again */ - var first = null, es = new EventSource('/__review/reload'); + var first = null, es = new EventSource('/__review/reload?' + q(asked)); es.onmessage = function (e) { if (first === null) first = e.data; else if (e.data !== first) location.reload(); }; - function el(tag, cls, text) { + /* an element of the bar: blocks() of textdiff.js skips everything with data-review-ui */ + function ui(tag, cls, text) { var n = document.createElement(tag); + n.setAttribute('data-review-ui', ''); if (cls) n.className = cls; if (text) n.textContent = text; return n; } - function theme() { - var t = document.documentElement.dataset.theme; - if (t === 'light' || t === 'dark') return t; - return matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; - } - /* a CSS selector for a part of the page: its id, else the tag path with :nth-of-type */ - function selectorOf(node) { - var parts = []; - for (var n = node; n && n.nodeType === 1 && n !== document.body && parts.length < 8; n = n.parentElement) { - if (n.id) { parts.unshift('#' + CSS.escape(n.id)); return parts.join(' > '); } - var tag = n.tagName.toLowerCase(), i = 1; - for (var s = n.previousElementSibling; s; s = s.previousElementSibling) if (s.tagName === n.tagName) i++; - parts.unshift(tag + ':nth-of-type(' + i + ')'); + function field(label, cls) { + var l = ui('label', 'rv-field ' + (cls || '')), s = ui('select', cls ? 'rv-' + cls : ''); + l.append(ui('span', 'rv-label', label), s); + return { label: l, select: s }; + } + + var bar = ui('div', 'rv-bar'); + bar.setAttribute('role', 'region'); bar.setAttribute('aria-label', 'Review'); + var branchField = field('Branch', 'branch-pick'), fromField = field('From', 'from'), toField = field('To', 'to'); + var sw = ui('label', 'rv-switch'), swBox = ui('input'); + swBox.type = 'checkbox'; sw.append(swBox, ui('span', '', 'Show changes')); + var approve = ui('button', 'rv-approve', 'Approve'); + approve.type = 'button'; approve.hidden = true; /* Task 6 of the review rounds connects it */ + var status = ui('span', 'rv-status'); status.setAttribute('role', 'status'); + bar.append(branchField.label, fromField.label, toField.label, sw, approve, status); + var versionParts = [fromField.label, toField.label, sw]; + + /* the choice per branch in localStorage; "last" is the newest version when it was saved */ + function load(branch) { + try { return JSON.parse(localStorage.getItem('review-bar:' + branch)) || {}; } catch (e) { return {}; } + } + function save(branch, s) { + try { localStorage.setItem('review-bar:' + branch, JSON.stringify(s)); } catch (e) { /* no storage: the choice is not kept */ } + } + + var texts = {}; /* rev -> promise of the HTML of this page at rev, or null */ + function text(branch, rev) { + if (!(rev in texts)) { + texts[rev] = fetch(API + 'text?' + q(branch) + '&rev=' + encodeURIComponent(rev) + '&path=' + encodeURIComponent(location.pathname)) + .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) + .then(function (d) { return d.html; }); } - return 'body > ' + parts.join(' > '); + return texts[rev]; } + function parse(html) { return new DOMParser().parseFromString(html, 'text/html'); } - var tab = el('button', 'rv-tab', 'Note'), badge = el('span', 'rv-badge'); - tab.type = 'button'; tab.setAttribute('aria-haspopup', 'dialog'); tab.appendChild(badge); - var panel = el('div', 'rv-panel'); panel.setAttribute('role', 'dialog'); panel.setAttribute('aria-label', 'Note on this page'); panel.hidden = true; - var head = el('div', 'rv-head'), title = el('strong', '', 'Note on this page'), branch = el('span', 'rv-branch'), close = el('button', 'rv-x', 'Close'); - close.type = 'button'; head.append(title, branch, close); - var text = el('textarea', 'rv-text'); text.rows = 5; text.maxLength = 4000; text.placeholder = 'What must change? The note keeps the page, the width and the theme.'; - var partRow = el('div', 'rv-row'), pick = el('button', 'rv-btn', 'Point at a part'), part = el('span', 'rv-part'), unpick = el('button', 'rv-link', 'Remove'); - pick.type = unpick.type = 'button'; unpick.hidden = true; partRow.append(pick, part, unpick); - var sendRow = el('div', 'rv-row'), send = el('button', 'rv-btn rv-send', 'Save note'), status = el('span', 'rv-status'); - send.type = 'button'; status.setAttribute('role', 'status'); sendRow.append(send, status); - var listHead = el('div', 'rv-list-head', 'My notes'), list = el('ol', 'rv-list'); - panel.append(head, text, partRow, sendRow, listHead, list); - var box = el('div', 'rv-box'); box.hidden = true; + /* ---- the version on screen: the page on disk, or the body of an older version ---- */ + var own = null; /* the nodes of the page on disk while an older version is on screen */ + var shown = null; /* the rev of the older version on screen, or null */ + function pageNodes() { + return Array.prototype.filter.call(document.body.childNodes, function (n) { return n !== bar; }); + } + function showOwn() { + if (!own) return; + pageNodes().forEach(function (n) { n.remove(); }); + own.forEach(function (n) { document.body.insertBefore(n, bar); }); + own = null; shown = null; + } + function showOther(rev, html) { + if (shown === rev) return; + if (!own) own = pageNodes(); + pageNodes().forEach(function (n) { n.remove(); }); + var nodes; + if (html === null) { + nodes = [ui('p', 'rv-missing wrap', 'This page is not in this version.')]; + } else { + var body = parse(html).body; + /* the scripts of the page ran once already */ + Array.prototype.forEach.call(body.querySelectorAll('script'), function (s) { s.remove(); }); + nodes = Array.prototype.map.call(body.childNodes, function (n) { return document.adoptNode(n); }); + } + nodes.forEach(function (n) { document.body.insertBefore(n, bar); }); + shown = rev; + } - var target = null, selection = ''; + /* ---- the marks ---- */ + var fallbacks = []; /* {el, nodes}: blocks with inline markup that got plain marked text */ + function unmark() { + var parents = []; + Array.prototype.forEach.call(document.querySelectorAll('ins.rv-add'), function (n) { + if (parents.indexOf(n.parentNode) < 0) parents.push(n.parentNode); + n.replaceWith.apply(n, Array.prototype.slice.call(n.childNodes)); + }); + Array.prototype.forEach.call(document.querySelectorAll('del.rv-del, .rv-gone, .rv-newpage'), function (n) { + if (n.parentNode && parents.indexOf(n.parentNode) < 0) parents.push(n.parentNode); + n.remove(); + }); + fallbacks.forEach(function (f) { f.el.replaceChildren.apply(f.el, f.nodes); }); + fallbacks = []; + parents.forEach(function (p) { p.normalize(); }); + } - function state() { - fetch(API + 'state').then(function (r) { return r.json(); }).then(function (s) { - branch.textContent = (s.branch || 'detached') + (s.commit ? ' ยท ' + s.commit : ''); - badge.textContent = s.open ? String(s.open) : ''; - }).catch(function () { branch.textContent = 'no server'; }); - } - function showList() { - fetch(API + 'notes').then(function (r) { return r.json(); }).then(function (d) { - list.textContent = ''; - if (!d.notes.length) { list.appendChild(el('li', 'rv-empty', 'No notes yet.')); return; } - d.notes.slice(0, 20).forEach(function (n) { - var li = el('li', 'rv-item rv-' + n.status); - li.append(el('span', 'rv-meta', (n.status === 'done' ? 'Done' : 'Open') + ' ยท ' + (n.path || '') + ' ยท ' + (n.time || '').slice(0, 16).replace('T', ' ')), - el('span', 'rv-note', n.note)); - if (n.answer) li.appendChild(el('span', 'rv-answer', n.answer)); - list.appendChild(li); - }); - }).catch(function () { list.textContent = ''; list.appendChild(el('li', 'rv-empty', 'The notes cannot load.')); }); - } - function open() { - panel.hidden = false; tab.setAttribute('aria-expanded', 'true'); state(); showList(); - setTimeout(function () { text.focus(); }, 0); - } - function shut() { panel.hidden = true; tab.setAttribute('aria-expanded', 'false'); stopPick(); tab.focus(); } + /* Every text node of el, and for each character of the block text (blank runs as one space, trimmed) its node and + offset. The block text is the text that blocks() of textdiff.js gives. */ + function textMap(el) { + var nodes = [], at = [], text = '', blank = false; + var walk = document.createTreeWalker(el, NodeFilter.SHOW_TEXT); + for (var t = walk.nextNode(); t; t = walk.nextNode()) { + nodes.push(t); + for (var i = 0; i < t.data.length; i++) { + if (/\s/.test(t.data[i])) { if (!blank && text) { text += ' '; at.push({ node: t, off: i }); } blank = true; } + else { text += t.data[i]; at.push({ node: t, off: i }); blank = false; } + } + } + if (text.slice(-1) === ' ') { text = text.slice(0, -1); at.pop(); } + return { nodes: nodes, at: at, text: text }; + } + function del(textOf) { var d = document.createElement('del'); d.className = 'rv-del'; d.textContent = textOf; return d; } + function ins() { var n = document.createElement('ins'); n.className = 'rv-add'; return n; } - /* the text that is selected on the page, read before the tap on the tab can clear it */ - tab.addEventListener('pointerdown', function () { selection = String(getSelection() || '').trim().slice(0, 1000); }); - tab.addEventListener('click', function () { if (panel.hidden) open(); else shut(); }); - close.addEventListener('click', shut); - document.addEventListener('keydown', function (e) { if (e.key === 'Escape' && !panel.hidden) { if (picking) stopPick(); else shut(); } }); + /* Put the word marks into the text nodes of el, so links and inline markup stay. */ + function markWords(el, words) { + var map = textMap(el); + var live = words.filter(function (w) { return w.op !== 'del'; }).map(function (w) { return w.text; }).join(''); + if (live !== map.text || !map.at.length) return markPlain(el, words); + var edits = [], pos = 0; + words.forEach(function (w) { + if (w.op === 'eq') pos += w.text.length; + else if (w.op === 'add') { edits.push({ from: pos, to: pos + w.text.length }); pos += w.text.length; } + else edits.push({ from: pos, removed: w.text }); + }); + /* from the end to the start: a split keeps the head of a text node in that node, so earlier offsets stay true */ + for (var k = edits.length - 1; k >= 0; k--) { + var e = edits[k]; + if (e.removed !== undefined) { + var p = e.from < map.at.length ? map.at[e.from] : { node: map.at[map.at.length - 1].node, off: map.at[map.at.length - 1].off + 1 }; + var rest = p.node.splitText(p.off); + rest.parentNode.insertBefore(del(e.removed), rest); + } else { + var s = map.at[e.from], last = map.at[e.to - 1]; + var i0 = map.nodes.indexOf(s.node), i1 = map.nodes.indexOf(last.node); + for (var i = i1; i >= i0; i--) { + var node = map.nodes[i]; + var a = i === i0 ? s.off : 0, b = i === i1 ? last.off + 1 : node.data.length; + if (a >= b) continue; + var mid = node.splitText(a); + if (b - a < mid.data.length) mid.splitText(b - a); + var wrap = ins(); + mid.parentNode.insertBefore(wrap, mid); + wrap.appendChild(mid); + } + } + } + } + /* The text of el does not match the block text: replace it with the marked text, and keep the old nodes for unmark. */ + function markPlain(el, words) { + fallbacks.push({ el: el, nodes: Array.prototype.slice.call(el.childNodes) }); + el.replaceChildren(); + words.forEach(function (w) { + if (w.op === 'eq') el.append(w.text); + else if (w.op === 'del') el.append(del(w.text)); + else { var n = ins(); n.textContent = w.text; el.append(n); } + }); + } - /* point at a part: the outline follows the pointer, a click takes that part */ - var picking = false; - function ours(n) { return n === tab || tab.contains(n) || panel.contains(n) || n === box; } - function frame(n) { - var r = n.getBoundingClientRect(); - box.style.left = r.left + 'px'; box.style.top = r.top + 'px'; box.style.width = r.width + 'px'; box.style.height = r.height + 'px'; - box.hidden = false; - } - function onMove(e) { var n = document.elementFromPoint(e.clientX, e.clientY); if (n && !ours(n)) frame(n); } - function onPick(e) { - var n = document.elementFromPoint(e.clientX, e.clientY); - if (!n || ours(n)) return; - e.preventDefault(); e.stopPropagation(); - var r = n.getBoundingClientRect(); - target = { selector: selectorOf(n), text: (n.innerText || n.alt || '').trim().replace(/\s+/g, ' ').slice(0, 300), - rect: { x: Math.round(r.left + scrollX), y: Math.round(r.top + scrollY), w: Math.round(r.width), h: Math.round(r.height) } }; - part.textContent = target.text ? 'โ€œ' + target.text.slice(0, 60) + 'โ€' : target.selector; - unpick.hidden = false; stopPick(); frame(n); panel.hidden = false; text.focus(); - } - function stopPick() { - if (!picking) return; - picking = false; document.documentElement.classList.remove('rv-picking'); pick.textContent = 'Point at a part'; - document.removeEventListener('pointermove', onMove, true); document.removeEventListener('click', onPick, true); - if (!target) box.hidden = true; - } - pick.addEventListener('click', function () { - if (picking) return stopPick(); - picking = true; document.documentElement.classList.add('rv-picking'); pick.textContent = 'Click a part (Esc stops)'; - panel.hidden = true; - document.addEventListener('pointermove', onMove, true); document.addEventListener('click', onPick, true); - }); - unpick.addEventListener('click', function () { target = null; part.textContent = ''; unpick.hidden = true; box.hidden = true; }); + function root() { return document.querySelector('main') || document.body; } + function mark(fromHtml) { + if (fromHtml === null) { + var line = ui('p', 'rv-newpage', 'New page in this round'); + root().insertBefore(line, root().firstChild); + return; + } + var live = diff.blockNodes(document), byKey = {}; + live.forEach(function (b) { byKey[b.key] = b.el; }); + var ops = diff.diffBlocks(diff.blocks(parse(fromHtml)), live.map(function (b) { return { key: b.key, text: b.text }; })); + var gone = [], lastEl = null; + function gonesBefore(el) { + gone.forEach(function (t) { var g = ui('div', 'rv-gone', t); el.parentNode.insertBefore(g, el); }); + gone = []; + } + ops.forEach(function (o) { + if (o.op === 'del') { gone.push(o.a.text); return; } + var el = byKey[o.b.key]; + gonesBefore(el); + if (o.op === 'chg') markWords(el, o.words); + else if (o.op === 'add') markWords(el, [{ op: 'add', text: o.b.text }]); + lastEl = el; + }); + gone.forEach(function (t) { + var g = ui('div', 'rv-gone', t); + if (lastEl) { lastEl.after(g); lastEl = g; } else root().appendChild(g); + }); + } - send.addEventListener('click', function () { - var note = text.value.trim(); - if (!note) { status.textContent = 'Write the note first.'; text.focus(); return; } - send.disabled = true; status.textContent = 'Saving โ€ฆ'; - var body = { note: note, path: location.pathname + location.hash, title: document.title, theme: theme(), selection: selection, - viewport: { w: innerWidth, h: innerHeight, dpr: devicePixelRatio }, scroll: { x: Math.round(scrollX), y: Math.round(scrollY) }, - target: target, agent: navigator.userAgent }; - fetch(API + 'notes', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-T7d-Review': '1' }, body: JSON.stringify(body) }) - .then(function (r) { return r.json().then(function (d) { if (!r.ok) throw new Error(d.error || r.status); return d; }); }) - .then(function (d) { - status.textContent = 'Saved (' + d.id + ').'; - text.value = ''; selection = ''; target = null; part.textContent = ''; unpick.hidden = true; box.hidden = true; - state(); showList(); - }) - .catch(function (err) { status.textContent = 'Not saved: ' + err.message + '. The text stays here.'; }) - .then(function () { send.disabled = false; }); + /* ---- the bar ---- */ + var v = null, versions = [], choice = {}, run = 0; + function onDisk() { return versions[versions.length - 1].value; } + function render() { + var mine = ++run; + unmark(); + status.textContent = ''; + var to = choice.to, from = choice.from; + var shownReady = to === onDisk() ? Promise.resolve(showOwn()) : text(v.branch, to).then(function (html) { + if (mine === run) showOther(to, html); + }); + return shownReady.then(function () { + if (mine !== run || !choice.show || from === to) return; + return text(v.branch, from).then(function (html) { if (mine === run) mark(html); }); + }).catch(function () { if (mine === run) status.textContent = 'The changes cannot load.'; }); + } + function remember() { save(v.branch, { show: choice.show, from: choice.from, to: choice.to, last: onDisk() }); } + function fill(select, list, value) { + select.replaceChildren(); + list.forEach(function (o) { var n = ui('option', '', o.label); n.value = o.value; select.appendChild(n); }); + select.value = value; + } + + function start(data) { + v = data; + fill(branchField.select, v.branches.map(function (b) { return { value: b, label: b }; }), v.branch); + versions = [{ value: v.base, label: v.base }].concat(v.rounds.map(function (r) { + return { value: r.sha, label: 'Round ' + r.n + ': ' + r.subject }; + })); + if (v.working) versions.push({ value: 'working', label: 'Working' }); + if (versions.length < 2) { + versionParts.forEach(function (n) { n.hidden = true; }); + return Promise.resolve(); + } + var values = versions.map(function (o) { return o.value; }); + var saved = load(v.branch), same = saved.last === onDisk(); + choice.show = saved.show !== false; + choice.to = same && values.indexOf(saved.to) >= 0 ? saved.to : onDisk(); + choice.from = same && values.indexOf(saved.from) >= 0 ? saved.from : values[Math.max(0, values.indexOf(choice.to) - 1)]; + fill(fromField.select, versions, choice.from); + fill(toField.select, versions, choice.to); + swBox.checked = choice.show; + return render(); + } + + branchField.select.addEventListener('change', function () { + location.href = location.pathname + '?branch=' + encodeURIComponent(branchField.select.value) + location.hash; }); + fromField.select.addEventListener('change', function () { choice.from = fromField.select.value; remember(); render(); }); + toField.select.addEventListener('change', function () { choice.to = toField.select.value; remember(); render(); }); + swBox.addEventListener('change', function () { choice.show = swBox.checked; remember(); render(); }); - function mount() { document.body.append(tab, panel, box); state(); } + function mount() { + document.body.appendChild(bar); + versionParts.forEach(function (n) { n.hidden = true; }); + fetch(API + 'versions?' + q(asked)) + .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) + .then(function (data) { versionParts.forEach(function (n) { n.hidden = false; }); return start(data); }) + .catch(function () { status.textContent = 'The review server does not answer.'; }) + .then(function () { bar.setAttribute('data-ready', ''); }); + } if (document.body) mount(); else document.addEventListener('DOMContentLoaded', mount); })(); diff --git a/scripts/review/test_devserver.py b/scripts/review/test_devserver.py index fb910ce..cef1a54 100644 --- a/scripts/review/test_devserver.py +++ b/scripts/review/test_devserver.py @@ -29,6 +29,7 @@ def test_adds_assets_and_connect_src_only(self): out = devserver.inject(PAGE) self.assertIn('href="/__review/review.css"', out) self.assertIn('src="/__review/review.js" defer', out) + self.assertLess(out.index('src="/__review/textdiff.js" defer'), out.index('src="/__review/review.js"')) self.assertIn("default-src 'none'; script-src 'self'; base-uri 'none'; connect-src 'self'", out) self.assertNotIn("unsafe-inline", out) @@ -100,6 +101,9 @@ def test_pages_get_the_tab_and_no_cache(self): self.assertEqual(headers["Cache-Control"], "no-cache") self.assertIn("/__review/review.js", body) self.assertEqual(self.get("/x.css")[2], "p{}") + code, headers, body = self.get("/__review/textdiff.js") + self.assertEqual((code, headers["Content-Type"]), (200, "text/javascript; charset=utf-8")) + self.assertIn("window.reviewDiff", body) def test_missing_page_is_the_404_page(self): code, _, body = self.get("/nope/") diff --git a/scripts/review/test_textdiff.mjs b/scripts/review/test_textdiff.mjs index 9dc05be..aac515f 100644 --- a/scripts/review/test_textdiff.mjs +++ b/scripts/review/test_textdiff.mjs @@ -2,7 +2,7 @@ import test from "node:test"; import assert from "node:assert"; import { createRequire } from "node:module"; -const { blocks, diffWords, diffBlocks } = createRequire(import.meta.url)("./textdiff.js"); +const { blocks, blockNodes, diffWords, diffBlocks } = createRequire(import.meta.url)("./textdiff.js"); test("diffWords marks a changed word", () => { assert.deepStrictEqual(diffWords("a b c", "a x c"), [ @@ -125,3 +125,21 @@ test("blocks reads main, numbers tags, and skips review UI and papercuts", () => { key: "p2", text: "quoted" }, ]); }); + +test("blocks reads the status and dates of a thought (dt and dd)", () => { + const main = el("main", [el("dl", [el("dt", ["Status"]), el("dd", ["Draft"])])]); + assert.deepStrictEqual(blocks(main), [ + { key: "dt0", text: "Status" }, + { key: "dd0", text: "Draft" }, + ]); +}); + +test("blockNodes gives the element of each block, in the order and with the keys of blocks", () => { + const p = el("p", ["One"]); + const h = el("h2", ["Two"]); + const main = el("main", [p, el("p", [" "]), h]); + const nodes = blockNodes(main); + assert.deepStrictEqual(nodes.map((n) => [n.key, n.text]), blocks(main).map((b) => [b.key, b.text])); + assert.strictEqual(nodes[0].el, p); + assert.strictEqual(nodes[1].el, h); +}); diff --git a/scripts/review/textdiff.js b/scripts/review/textdiff.js index 5f5b248..b58aa73 100644 --- a/scripts/review/textdiff.js +++ b/scripts/review/textdiff.js @@ -2,7 +2,7 @@ (function () { "use strict"; - var BLOCK_SELECTOR = "h1, h2, h3, h4, h5, h6, p, li, blockquote, figcaption, td, th, pre"; + var BLOCK_SELECTOR = "h1, h2, h3, h4, h5, h6, p, li, dt, dd, blockquote, figcaption, td, th, pre"; var SKIP_SELECTOR = "[data-review-ui], papercuts-widget"; // Myers stops after this many edited tokens. The block pair then becomes "del all + add all". var MAX_EDITS = 1000; @@ -146,8 +146,8 @@ return out; } - // Visible text of main (else body) as blocks. Uses only querySelector(All), closest and textContent. - function blocks(doc) { + // Visible text of main (else body) as blocks with their elements. Uses only querySelector(All), closest and textContent. + function blockNodes(doc) { var root = (doc.querySelector && (doc.querySelector("main") || doc.querySelector("body"))) || doc; var counts = {}; var out = []; @@ -160,12 +160,17 @@ var tag = el.tagName.toLowerCase(); var idx = counts[tag] || 0; counts[tag] = idx + 1; - out.push({ key: tag + idx, text: text }); + out.push({ key: tag + idx, text: text, el: el }); }); return out; } - var api = { blocks: blocks, diffWords: diffWords, diffBlocks: diffBlocks }; + // The same blocks as {key, text}, for a diff. + function blocks(doc) { + return blockNodes(doc).map(function (b) { return { key: b.key, text: b.text }; }); + } + + var api = { blocks: blocks, blockNodes: blockNodes, diffWords: diffWords, diffBlocks: diffBlocks }; if (typeof module !== "undefined" && module.exports) module.exports = api; if (typeof window !== "undefined") window.reviewDiff = api; })(); From 3cf1e294fcfdc3ee70ee7d1cbae236d56d988ebe Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:32:15 +0100 Subject: [PATCH 07/16] review: a compact bar on phones, an older round changes only main On a narrow screen the bar is one row: the comparison and the switch. A tap on the comparison opens the selects. scroll-padding-bottom keeps find-in-page and focus above the bar. An older round now replaces only the content of main, so the header keeps its listeners. All nodes of the older version are moved: a copy of the list comes first. A failed text request is not kept, so a second try loads it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- scripts/review/check_review.py | 109 +++++++++++++++++++++++++++++---- scripts/review/review.css | 25 ++++++++ scripts/review/review.js | 61 +++++++++++++----- 3 files changed, 166 insertions(+), 29 deletions(-) diff --git a/scripts/review/check_review.py b/scripts/review/check_review.py index b8c05a2..7210e8f 100644 --- a/scripts/review/check_review.py +++ b/scripts/review/check_review.py @@ -9,6 +9,7 @@ """ import argparse import os +import re import shutil import socket import subprocess @@ -29,6 +30,7 @@ P3 = "The third paragraph stays the same." P4 = "Round two adds this closing text." SIZES = ((1440, 900), (390, 844), (320, 640)) +WIDE = 960 # from this width the bar is one row with every select; below it the selects open on a tap MARKS = "ins.rv-add, del.rv-del, .rv-gone, .rv-newpage" @@ -38,8 +40,11 @@ def page(title, paras): '' '%s' '' - '' - '

%s

\n%s\n
') % (CSP, title, title, body) + '' + '' + '
Fixture' + '
' + '

%s

\n%s\n
') % (CSP, title, title, body) def git(cwd, *args): @@ -57,10 +62,11 @@ def fixture(base): """The main clone (master) and the worktree of t/a below base/wts. Returns (main, wts).""" main, wts = os.path.join(base, "main"), os.path.join(base, "wts") os.makedirs(os.path.join(main, "assets")) - for f in ("site.css", "theme.js", "favicon.svg"): + for f in ("site.css", "theme.js", "site.js", "favicon.svg"): shutil.copy(os.path.join(REPO, "assets", f), os.path.join(main, "assets", f)) shutil.copytree(os.path.join(REPO, "assets", "fonts"), os.path.join(main, "assets", "fonts")) write(main, "index.html", page("Fixture", [P1_OLD, P2, P3])) + write(main, "long/index.html", page("Long", ["Paragraph %d of a page that scrolls." % i for i in range(60)])) git(main, "init", "-b", "master") git(main, "add", "-A") git(main, "commit", "-m", "base") @@ -127,6 +133,27 @@ def bar_ready(pg): pg.wait_for_selector(".rv-bar[data-ready]", state="attached") +def open_bar(pg): + """On a narrow screen the selects are in a part that opens on a tap.""" + if pg.is_visible(".rv-more-toggle") and pg.get_attribute(".rv-more-toggle", "aria-expanded") != "true": + pg.click(".rv-more-toggle") + + +def pick(pg, sel, value): + open_bar(pg) + pg.select_option(sel, value) + + +def small_targets(pg): + return pg.evaluate("[...document.querySelectorAll('.rv-bar select, .rv-bar label, .rv-bar button')]" + ".filter(n => n.offsetParent).map(n => n.getBoundingClientRect().height).filter(x => x < 40)") + + +def theme_now(pg): + return pg.evaluate("document.documentElement.dataset.theme || " + "(matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light')") + + def run(base, shots): c = Check() with sync_playwright() as p: @@ -144,6 +171,7 @@ def run(base, shots): # master has no rounds: only the branch select pg.goto(base + "/?branch=master") bar_ready(pg) + open_bar(pg) c.that(pg.is_visible(".rv-branch-pick") and not pg.is_visible(".rv-from") and not pg.is_visible(".rv-to") and not pg.is_visible(".rv-switch"), tag + ": on master the bar shows only the branch select") @@ -153,7 +181,7 @@ def run(base, shots): # the branch select opens t/a, with Round 2 against Round 1 with pg.expect_navigation(): - pg.select_option(".rv-branch-pick", "t/a") + pick(pg, ".rv-branch-pick", "t/a") bar_ready(pg) c.that("branch=t%2Fa" in pg.url or "branch=t/a" in pg.url, tag + ": the branch select loads t/a") c.that(pg.input_value(".rv-to") == pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value"), @@ -166,11 +194,10 @@ def run(base, shots): c.that(pg.evaluate("document.querySelector('.rv-gone').nextElementSibling.textContent") == P3, tag + ": rv-gone is before the next kept paragraph") c.that(pg.evaluate("[...document.querySelectorAll('[class^=rv-], [class*=\" rv-\"]')]" - ".filter(n => !n.closest('[data-review-ui]') && !n.matches('ins, del')).length") == 0, + ".filter(n => n !== document.documentElement && !n.closest('[data-review-ui]') && !n.matches('ins, del')).length") == 0, tag + ": every review element outside the marks carries data-review-ui") c.that(no_side_scroll(pg), tag + ": no horizontal scroll with marks") - small = pg.evaluate("[...document.querySelectorAll('.rv-bar select, .rv-bar label, .rv-bar button')]" - ".filter(n => n.offsetParent).map(n => n.getBoundingClientRect().height).filter(x => x < 40)") + small = small_targets(pg) c.that(small == [], tag + ": touch targets are 40 px or more (%r)" % small) if shots: pg.screenshot(path=os.path.join(shots, "marks-%d-%s.png" % (w, scheme)), full_page=True) @@ -188,24 +215,50 @@ def run(base, shots): c.that(texts(pg, "del.rv-del") == ["quick"] and texts(pg, ".rv-gone") == [P2], tag + ": switch on shows the marks again") + # one failed request: the same version loads when it is selected again + round1 = pg.eval_on_selector(".rv-to", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value") + pg.route(re.compile(r"/__review/api/text\?.*rev=master"), lambda route: route.abort(), times=1) + n_errors = len(errors) + pick(pg, ".rv-from", "master") + wait_for(pg, "document.querySelector('.rv-status').textContent !== ''") + c.that(pg.locator(MARKS).count() == 0, tag + ": a failed request shows no marks") + errors[n_errors:] = [e for e in errors[n_errors:] if "net::ERR_FAILED" not in e] + pick(pg, ".rv-from", round1) + wait_for(pg, "document.querySelectorAll('del.rv-del').length === 1") + pick(pg, ".rv-from", "master") + try: + wait_for(pg, "document.querySelector('main h1 ins.rv-add') !== null") + again = True + except AssertionError: + again = False + c.that(again and pg.text_content(".rv-status") == "", + tag + ": after a failed request the version loads when it is selected again") + # to = Round 1 is an older round: the page shows round 1, marked against master - pg.select_option(".rv-from", "master") - to1 = pg.eval_on_selector(".rv-to", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value") - pg.select_option(".rv-to", to1) + pick(pg, ".rv-to", round1) wait_for(pg, "document.querySelector('main h1 ins.rv-add') !== null") c.that(texts(pg, "main > p") == [P1_OLD, P2, P3], tag + ": to = Round 1 shows the text of round 1") c.that(texts(pg, "ins.rv-add") == [" page"] and pg.locator("del.rv-del, .rv-gone").count() == 0, tag + ": Round 1 against master marks only the heading (%r)" % texts(pg, "ins.rv-add")) + header = pg.evaluate("document.querySelector('header.nav') !== null") + before = theme_now(pg) + pg.evaluate("document.getElementById('theme-toggle').click()") + after = theme_now(pg) + pg.evaluate("document.getElementById('theme-toggle').click()") + c.that(header and after != before and theme_now(pg) == before, + tag + ": in an older round the theme toggle of the header still works (%s -> %s)" % (before, after)) + if pg.get_attribute(".rv-more-toggle", "aria-expanded") == "true": + pg.click(".rv-more-toggle") if shots and w != 1440: pg.screenshot(path=os.path.join(shots, "round1-%d-%s.png" % (w, scheme))) - pg.select_option(".rv-to", pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value")) + pick(pg, ".rv-to", pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value")) wait_for(pg, "document.querySelectorAll('main > p').length === 3") wait_for(pg, "document.querySelectorAll('.rv-gone').length === 1") c.that(texts(pg, "ins.rv-add") == [" page", "slow", P4] and texts(pg, "del.rv-del") == ["quick"], tag + ": back to Round 2 shows the page on disk, marked against master (%r)" % texts(pg, "ins.rv-add")) # a page new in "to" - pg.select_option(".rv-from", pg.eval_on_selector(".rv-from", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value")) + pick(pg, ".rv-from", pg.eval_on_selector(".rv-from", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value")) pg.goto(base + "/new/?branch=t/a") bar_ready(pg) c.that(texts(pg, ".rv-newpage") == ["New page in this round"], tag + ": a new page has one line") @@ -214,6 +267,38 @@ def run(base, shots): if shots and w == 390: pg.screenshot(path=os.path.join(shots, "newpage-%d-%s.png" % (w, scheme))) + # a long page: the closed bar is small, sticks to the bottom and opens on a tap + pg.goto(base + "/long/?branch=t/a") + bar_ready(pg) + box = pg.evaluate("(() => { const r = document.querySelector('.rv-bar').getBoundingClientRect();" + " return {top: r.top, bottom: r.bottom, h: r.height, vh: innerHeight}; })()") + c.that(abs(box["bottom"] - box["vh"]) < 2, tag + ": the bar sticks to the bottom of a long page (%r)" % box) + pad = pg.evaluate("parseFloat(getComputedStyle(document.documentElement).scrollPaddingBottom) || 0") + c.that(pad >= box["h"], tag + ": scroll-padding-bottom (%s px) is at least the bar height (%s px)" % (pad, box["h"])) + if w < WIDE: + c.that(box["h"] <= 0.15 * h, tag + ": the closed bar is at most 15%% of the screen (%s px)" % box["h"]) + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "false" and not pg.is_visible(".rv-from"), + tag + ": the selects are closed at first") + c.that(pg.text_content(".rv-more-toggle").strip() == "Round 2 vs Round 1", + tag + ": the closed bar shows the comparison (%r)" % pg.text_content(".rv-more-toggle")) + c.that(small_targets(pg) == [], tag + ": closed bar: touch targets are 40 px or more") + if shots: + pg.screenshot(path=os.path.join(shots, "long-closed-%d-%s.png" % (w, scheme))) + pg.click(".rv-more-toggle") + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "true" + and pg.is_visible(".rv-branch-pick") and pg.is_visible(".rv-from") and pg.is_visible(".rv-to"), + tag + ": a tap opens the selects") + c.that(small_targets(pg) == [] and no_side_scroll(pg), + tag + ": open bar: touch targets are 40 px or more, no horizontal scroll") + if shots: + pg.screenshot(path=os.path.join(shots, "long-open-%d-%s.png" % (w, scheme))) + pg.click(".rv-more-toggle") + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "false" and not pg.is_visible(".rv-from"), + tag + ": a second tap closes the selects") + else: + c.that(not pg.is_visible(".rv-more-toggle") and pg.is_visible(".rv-from"), + tag + ": a wide screen shows every select in one row") + c.that(errors == [], tag + ": no console error (%r)" % errors) ctx.close() browser.close() diff --git a/scripts/review/review.css b/scripts/review/review.css index f16bb90..77d145b 100644 --- a/scripts/review/review.css +++ b/scripts/review/review.css @@ -19,6 +19,31 @@ min-height: 40px; padding: 0 1rem; border: var(--rule, 1.5px) solid var(--ink, #111); background: var(--ink, #111); color: var(--bg, #fff); font: 600 0.75rem var(--mono, monospace); text-transform: uppercase; cursor: pointer; } +/* Find-in-page and focus stop above the sticky bar. The values are the height of the bar, closed and open. */ +html.rv-has-bar { scroll-padding-bottom: 4.5rem; } +html.rv-has-bar.rv-bar-open { scroll-padding-bottom: 17rem; } + +/* narrow screens: one row with the comparison and the switch, the selects open above it on a tap */ +.rv-more-toggle { + flex: 1 1 0; min-width: 0; min-height: 40px; padding: 0 0.6rem; overflow: hidden; + border: var(--rule, 1.5px) solid var(--ink, #111); background: var(--bg, #fff); color: var(--ink, #111); + font: 600 0.85rem/1 var(--body, sans-serif); text-align: left; white-space: nowrap; text-overflow: ellipsis; cursor: pointer; +} +.rv-more-toggle::after { content: " \25B4"; } +.rv-open .rv-more-toggle::after { content: " \25BE"; } +.rv-more { order: -1; flex: 1 0 100%; display: flex; flex-direction: column; gap: 0.5rem; min-width: 0; } +.rv-more .rv-field { flex: none; } +.rv-bar:not(.rv-open) .rv-more { display: none; } +@media (max-width: 959px) { + .rv-bar { gap: 0.5rem 0.6rem; padding: 0.5rem; } + .rv-switch { gap: 0.4rem; font-size: 0.85rem; } +} +/* wide screens: one row with every select, no toggle */ +@media (min-width: 960px) { + .rv-more-toggle { display: none; } + .rv-more, .rv-bar:not(.rv-open) .rv-more { display: contents; } + .rv-more .rv-field { flex: 1 1 11rem; } +} .rv-status:empty { display: none; } .rv-status { min-width: 0; overflow-wrap: anywhere; color: var(--muted, #5c5c5c); font-size: 0.82rem; } .rv-bar select:focus-visible, .rv-bar input:focus-visible, .rv-bar button:focus-visible { outline: 2px solid var(--ink, #111); outline-offset: 2px; } diff --git a/scripts/review/review.js b/scripts/review/review.js index a6b640b..19db622 100644 --- a/scripts/review/review.js +++ b/scripts/review/review.js @@ -34,8 +34,21 @@ var approve = ui('button', 'rv-approve', 'Approve'); approve.type = 'button'; approve.hidden = true; /* Task 6 of the review rounds connects it */ var status = ui('span', 'rv-status'); status.setAttribute('role', 'status'); - bar.append(branchField.label, fromField.label, toField.label, sw, approve, status); + /* On a narrow screen the bar is one row: the comparison and the switch. A tap on the comparison opens the selects. */ + var more = ui('div', 'rv-more'), moreToggle = ui('button', 'rv-more-toggle'); + more.id = 'rv-more'; more.append(branchField.label, fromField.label, toField.label); + moreToggle.type = 'button'; moreToggle.setAttribute('aria-controls', more.id); moreToggle.setAttribute('aria-expanded', 'false'); + bar.append(moreToggle, more, sw, approve, status); var versionParts = [fromField.label, toField.label, sw]; + function setOpen(open) { + bar.classList.toggle('rv-open', open); + document.documentElement.classList.toggle('rv-bar-open', open); + moreToggle.setAttribute('aria-expanded', String(open)); + } + moreToggle.addEventListener('click', function () { setOpen(!bar.classList.contains('rv-open')); }); + bar.addEventListener('keydown', function (e) { + if (e.key === 'Escape' && bar.classList.contains('rv-open')) { setOpen(false); moreToggle.focus(); } + }); /* the choice per branch in localStorage; "last" is the newest version when it was saved */ function load(branch) { @@ -50,38 +63,44 @@ if (!(rev in texts)) { texts[rev] = fetch(API + 'text?' + q(branch) + '&rev=' + encodeURIComponent(rev) + '&path=' + encodeURIComponent(location.pathname)) .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) - .then(function (d) { return d.html; }); + .then(function (d) { return d.html; }, function (err) { delete texts[rev]; throw err; }); /* keep no failure */ } return texts[rev]; } function parse(html) { return new DOMParser().parseFromString(html, 'text/html'); } - /* ---- the version on screen: the page on disk, or the body of an older version ---- */ - var own = null; /* the nodes of the page on disk while an older version is on screen */ + /* ---- the version on screen: the page on disk, or the content of main of an older version ---- + Only main changes, so the header, the nav and the footer keep the listeners of site.js. A page without main + changes its whole body. */ + var own = null; /* {el, nodes}: the content of the page on disk while an older version is on screen */ var shown = null; /* the rev of the older version on screen, or null */ - function pageNodes() { - return Array.prototype.filter.call(document.body.childNodes, function (n) { return n !== bar; }); + function contentOf(el) { + return Array.prototype.filter.call(el.childNodes, function (n) { return n !== bar; }); + } + function put(el, nodes) { + nodes.forEach(function (n) { el.insertBefore(n, el === document.body ? bar : null); }); } function showOwn() { if (!own) return; - pageNodes().forEach(function (n) { n.remove(); }); - own.forEach(function (n) { document.body.insertBefore(n, bar); }); + contentOf(own.el).forEach(function (n) { n.remove(); }); + put(own.el, own.nodes); own = null; shown = null; } function showOther(rev, html) { if (shown === rev) return; - if (!own) own = pageNodes(); - pageNodes().forEach(function (n) { n.remove(); }); + if (!own) { var el = document.querySelector('main') || document.body; own = { el: el, nodes: contentOf(el) }; } + contentOf(own.el).forEach(function (n) { n.remove(); }); var nodes; if (html === null) { nodes = [ui('p', 'rv-missing wrap', 'This page is not in this version.')]; } else { - var body = parse(html).body; + var doc = parse(html), from = own.el === document.body ? doc.body : (doc.querySelector('main') || doc.body); /* the scripts of the page ran once already */ - Array.prototype.forEach.call(body.querySelectorAll('script'), function (s) { s.remove(); }); - nodes = Array.prototype.map.call(body.childNodes, function (n) { return document.adoptNode(n); }); + Array.prototype.forEach.call(from.querySelectorAll('script'), function (s) { s.remove(); }); + /* a copy first: adoptNode takes each node out of the live childNodes list */ + nodes = Array.prototype.slice.call(from.childNodes).map(function (n) { return document.adoptNode(n); }); } - nodes.forEach(function (n) { document.body.insertBefore(n, bar); }); + put(own.el, nodes); shown = rev; } @@ -197,8 +216,13 @@ /* ---- the bar ---- */ var v = null, versions = [], choice = {}, run = 0; function onDisk() { return versions[versions.length - 1].value; } + function nameOf(value) { + for (var i = 0; i < versions.length; i++) if (versions[i].value === value) return versions[i].short; + return value; + } function render() { var mine = ++run; + moreToggle.textContent = nameOf(choice.to) + ' vs ' + nameOf(choice.from); unmark(); status.textContent = ''; var to = choice.to, from = choice.from; @@ -220,11 +244,12 @@ function start(data) { v = data; fill(branchField.select, v.branches.map(function (b) { return { value: b, label: b }; }), v.branch); - versions = [{ value: v.base, label: v.base }].concat(v.rounds.map(function (r) { - return { value: r.sha, label: 'Round ' + r.n + ': ' + r.subject }; + versions = [{ value: v.base, label: v.base, short: v.base }].concat(v.rounds.map(function (r) { + return { value: r.sha, label: 'Round ' + r.n + ': ' + r.subject, short: 'Round ' + r.n }; })); - if (v.working) versions.push({ value: 'working', label: 'Working' }); + if (v.working) versions.push({ value: 'working', label: 'Working', short: 'Working' }); if (versions.length < 2) { + moreToggle.textContent = v.branch; versionParts.forEach(function (n) { n.hidden = true; }); return Promise.resolve(); } @@ -248,6 +273,8 @@ function mount() { document.body.appendChild(bar); + document.documentElement.classList.add('rv-has-bar'); + moreToggle.textContent = 'Review'; versionParts.forEach(function (n) { n.hidden = true; }); fetch(API + 'versions?' + q(asked)) .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) From 3821416b5f72426130a4c906a38783470fe1e708 Mon Sep 17 00:00:00 2001 From: Tim Conrad <4759569+Tim-Conrad@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:37:42 +0100 Subject: [PATCH 08/16] review: papercuts in a local store instead of the note tab The server removes the notes API, --notes and show.py. With env PAPERCUTS_URL it adds the widget tag for the chosen branch and the relay origin to the CSP. A second container, the relay, writes .papercuts/papercuts.db. The skill and the hook now use papercuts. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFxuEteUiyeGMzLJ2VGnGc --- .claude/settings.json | 2 +- .claude/skills/handling-review-notes/SKILL.md | 20 --- .../skills/handling-review-papercuts/SKILL.md | 26 ++++ .gitignore | 1 + CLAUDE.md | 6 +- README.md | 16 +-- docker-compose.dev.yml | 43 +++++- scripts/review/devserver.py | 124 +++++------------- scripts/review/show.py | 78 ----------- scripts/review/test_devserver.py | 90 +++++++------ 10 files changed, 155 insertions(+), 251 deletions(-) delete mode 100644 .claude/skills/handling-review-notes/SKILL.md create mode 100644 .claude/skills/handling-review-papercuts/SKILL.md delete mode 100644 scripts/review/show.py diff --git a/.claude/settings.json b/.claude/settings.json index fd1d312..3511cbd 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "n=$(ls \"${CLAUDE_PROJECT_DIR:-.}\"/.review/notes/*.json 2>/dev/null | wc -l); if [ \"$n\" -gt 0 ]; then echo \"$n open review notes in .review/notes/ (skill handling-review-notes)\"; fi; true" + "command": "d=\"${CLAUDE_PROJECT_DIR:-.}/.papercuts/papercuts.db\"; if [ -f \"$d\" ]; then n=$(PAPERCUTS_STORE=local PAPERCUTS_DB=\"$d\" papercuts list 2>/dev/null | sed -n 's/^count: \\([0-9]*\\) of.*/\\1/p'); if [ \"${n:-0}\" -gt 0 ]; then echo \"$n open papercuts of the review site (skill handling-review-papercuts)\"; fi; fi; exit 0" } ] } diff --git a/.claude/skills/handling-review-notes/SKILL.md b/.claude/skills/handling-review-notes/SKILL.md deleted file mode 100644 index 41e339e..0000000 --- a/.claude/skills/handling-review-notes/SKILL.md +++ /dev/null @@ -1,20 +0,0 @@ ---- -name: handling-review-notes -description: Use when Tim mentions notes or feedback on the review site (the tab ยปNoteยซ), when the session start says there are open review notes, or when you change scripts/review/ or docker-compose.dev.yml. ---- - -# Handling review notes - -Tim writes notes on the review site: the tab ยปNoteยซ at the right edge. The review site is this clone, served by `scripts/review/devserver.py` in the container `t7d-review` (`docker-compose.dev.yml`, host name in `.env` as `REVIEW_HOST`). It shows the checked-out branch at once and loads the page again when a file changes. A note is a JSON file in `.review/notes/`. - -1. The SessionStart hook (`.claude/settings.json`) prints the number of open notes in `.review/notes/`. Tell Tim how many are open, and list them with `ls .review/notes/*.json`. -2. Read the note: `note`, `path`, `viewport`, `theme`, `scroll`, `target` (`selector`, `text`, `rect`), `selection`, `branch`, `commit`. -3. Run `uv run --no-project --with playwright python3 scripts/review/show.py ` (more ids in one call). Look at `.review/notes/.png`. The red frame is the part that Tim pointed at. -4. Do the work with the rules of this file: a branch, `DESIGN.md`, the look at three widths. -5. After the fix, run `show.py --suffix after` and compare the two pictures. -6. Add an `"answer"` field to the note: what changed, and the commit. Move the note to `.review/notes/done/`. Tim sees the answer under ยปMy notesยซ in the tab. - -- Keep notes and their pictures out of git. `.review/` and `.env` are ignored. -- A change to `devserver.py` needs `docker restart t7d-review`. A change to `review.js` or `review.css` needs only a page load. -- The server adds only `connect-src 'self'` to the CSP of a page. Keep it that way, so that the review site keeps the rules of the real site. -- Tests: `python3 -m unittest scripts/review/test_devserver.py`. diff --git a/.claude/skills/handling-review-papercuts/SKILL.md b/.claude/skills/handling-review-papercuts/SKILL.md new file mode 100644 index 0000000..063667d --- /dev/null +++ b/.claude/skills/handling-review-papercuts/SKILL.md @@ -0,0 +1,26 @@ +--- +name: handling-review-papercuts +description: Use when Tim mentions papercuts or feedback on the review site, when the session start says there are open papercuts, or when you change scripts/review/ or docker-compose.dev.yml. +--- + +# Handling review papercuts + +Tim writes papercuts on the review site with the papercuts widget. The review site is this clone, served by `scripts/review/devserver.py` in the container `t7d-review` (`docker-compose.dev.yml`, host name in `.env` as `REVIEW_HOST`). A second container, `t7d-review-papercuts`, is the relay. It writes the local store `.papercuts/papercuts.db` in this clone. Each papercut keeps the page, the branch, the commit and a picture. + +Set the store once per shell, in any worktree. `$REVIEW_MAIN` is the folder of this clone. + +```sh +export PAPERCUTS_STORE=local PAPERCUTS_DB=$REVIEW_MAIN/.papercuts/papercuts.db +``` + +1. The SessionStart hook (`.claude/settings.json`) prints the number of open papercuts. Tell Tim how many are open, and run `papercuts list`. +2. Run `papercuts show ` to read one papercut: the text, the page, the branch, the commit and the picture. +3. Do the work in a worktree for the branch of the papercut, in `~/code/worktrees/t7d/`. Follow the rules of this file: a branch, `DESIGN.md`, the look at three widths. +4. A fix is one commit on that branch. One commit is one round on the review site. Tim sees the round in the version picker. +5. Run `papercuts comment ""` if Tim needs an explanation. +6. Run `papercuts done --fix ""` after the commit. + +- Keep the store and its pictures out of git. `.papercuts/`, `.review/` and `.env` are ignored. +- A change to `devserver.py` needs `docker restart t7d-review`. A change to `review.js` or `review.css` needs only a page load. +- The server adds only `connect-src 'self'` to the CSP of a page, and the origin of the relay when `PAPERCUTS_URL` is set. Keep it that way, so that the review site keeps the rules of the real site. +- Tests: `python3 -m unittest discover -s scripts/review -p 'test_*.py'` and `node --test 'scripts/review/test_*.mjs'`. diff --git a/.gitignore b/.gitignore index 53916dc..f0768c8 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ __pycache__/ /thoughts/feed.xml /blog/feed.xml .review/ +.papercuts/ .env .preview/ diff --git a/CLAUDE.md b/CLAUDE.md index 27b526d..00df2bd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,11 +22,11 @@ t7d.dev: the personal site of Tim Conrad. Plain HTML, CSS and a little JavaScrip - `blog/` and `CVE/` only redirect old addresses (the thoughts were a blog until November 2026). Do not add pages there. -Show a changed page as a Claude artifact, not a local server: `python3 scripts/preview-artifact.py ` builds the bundle; publish `/index.html` with the Artifact tool and pass the other printed files in `files`. Republish the same `file_path` after each change, so the link stays. The review site (skill `handling-review-notes`) is where Tim leaves notes on his own. +Show a changed page as a Claude artifact, not a local server: `python3 scripts/preview-artifact.py ` builds the bundle; publish `/index.html` with the Artifact tool and pass the other printed files in `files`. Republish the same `file_path` after each change, so the link stays. The review site (skill `handling-review-papercuts`) is where Tim leaves papercuts on his own. - -Use the skill `handling-review-notes`. A hook prints the number of open notes at the start of a session. + +Use the skill `handling-review-papercuts`. A hook prints the number of open papercuts at the start of a session. diff --git a/README.md b/README.md index 088f814..c5737dd 100644 --- a/README.md +++ b/README.md @@ -43,16 +43,16 @@ It is plain HTML, CSS and a little JavaScript. There is no build step and no dep | `scripts/ship.sh` | Merges a PR, waits for the deploy and checks t7d.dev | | `scripts/preview-artifact.py` | Builds a page as a Claude artifact bundle, for a review before it is published | | `scripts/reels/` | Scripts that film the two games and cut the reels | -| `scripts/review/`, `docker-compose.dev.yml` | The review site: live reload and a note tab. Never published | +| `scripts/review/`, `docker-compose.dev.yml` | The review site: live reload and the papercuts widget. Never published | | `.github/workflows/deploy.yml` | Runs the deploy on every push to `master` | | `DESIGN.md` | How the site looks and sounds: colours, type, layout, components and the wording rules | | `CLAUDE.md` | Instructions for Claude Code in this repository | | `.claude/skills/updating-product-reels/` | Working guide for Claude Code: how to take the reels and pictures again | | `.claude/skills/adding-a-thought/` | Working guide for Claude Code: how to turn a link into a new thought, and how to revise one | | `.claude/skills/updating-game-timeline/` | Working guide for Claude Code: how to film the timeline clips of a game page | -| `.claude/skills/handling-review-notes/` | Working guide for Claude Code: how to handle Tim's notes from the review site | +| `.claude/skills/handling-review-papercuts/` | Working guide for Claude Code: how to handle Tim's papercuts from the review site | | `.claude/rules/pages.md` | Rules for a new page; Claude Code loads them only when it works on an HTML page, the sitemap or the deploy script | -| `.claude/settings.json` | A hook that prints the number of open review notes at the start of a session | +| `.claude/settings.json` | A hook that prints the number of open papercuts at the start of a session | | `scripts/check.py` | Checks the CSP rules and the sitemap of every page; CI runs it | ## Preview locally @@ -63,19 +63,19 @@ python3 -m http.server 8000 Then open . All paths are absolute, so serve from the repository root. -For live reload and the note tab, run `python3 scripts/review/devserver.py` instead (port 8000). +For live reload, run `python3 scripts/review/devserver.py` instead (port 8000). ## Review site -The review site serves this clone with live reload and a tab ยปNoteยซ at the right edge. A note keeps the page, the width, the theme and the part that you pointed at. It is a JSON file in `.review/notes/`, and git ignores that folder. The site files on `gh-pages` do not change: `scripts/deploy.sh` never publishes `scripts/`. +The review site serves this clone with live reload. Each page has the papercuts widget, and Tim picks a branch and a round. A papercut keeps the page, the branch, the commit and a picture. A relay container writes it to the local store `.papercuts/papercuts.db`, and git ignores that folder. The site files on `gh-pages` do not change: `scripts/deploy.sh` never publishes `scripts/`. To run it in a container behind a login proxy: -1. Make the notes folder: `mkdir -p .review/notes`. -2. Write the host name into `.env`: `REVIEW_HOST=`. +1. Make the store folder: `mkdir -p .papercuts`. +2. Write the settings into `.env`: `REVIEW_HOST`, `REVIEW_MAIN`, `REVIEW_WORKTREES`, `REVIEW_PAPERCUTS_HOST` and `PAPERCUTS_SRC` (the folder of the relay source with its Dockerfile). 3. Start the stack: `docker-compose -p t7d-review -f docker-compose.dev.yml up -d`. -`scripts/review/show.py ` opens the page of a note and frames its part in a picture. A handled note goes to `.review/notes/done/` with an `"answer"` field, which the tab shows under ยปMy notesยซ. Tests: `python3 -m unittest scripts/review/test_devserver.py`. +To read the papercuts, run `PAPERCUTS_STORE=local PAPERCUTS_DB=.papercuts/papercuts.db papercuts list`. Then run `papercuts done --fix ""` after the fix. Tests: `python3 -m unittest discover -s scripts/review -p 'test_*.py'`. ## Deploy diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index cdb7c5e..499ff9a 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -1,13 +1,14 @@ -# The review site: this clone with live reload and the note tab (scripts/review/devserver.py), in the lab behind its +# The review site: this clone with live reload and the papercuts widget (scripts/review/devserver.py), in the lab behind its # login. GitHub Pages still serves t7d.dev from gh-pages. This stack never touches it. # -# mkdir -p .review/notes # first: else Docker makes the folder as root +# mkdir -p .papercuts # first: else Docker makes the folder as root # # .env (gitignored): REVIEW_HOST=, REVIEW_MAIN=, # # REVIEW_WORKTREES= +# # REVIEW_PAPERCUTS_HOST=, PAPERCUTS_SRC= # docker-compose -p t7d-review -f docker-compose.dev.yml up -d # docker-compose -p t7d-review -f docker-compose.dev.yml down # -# The clone and the worktrees are read-only in the container, at their host paths (git needs the same paths). Only .review/notes is writable: one JSON file per note. +# The clone and the worktrees are read-only in the container, at their host paths (git needs the same paths). The review server writes nothing. Only the relay container writes, and only to .papercuts (the local store of papercuts). # The page shows the branch of ?branch=, else of the cookie, else of the clone. The image adds only the package git. services: t7d-review: @@ -18,9 +19,10 @@ services: container_name: t7d-review restart: unless-stopped user: "1001:1001" - command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --notes, "${REVIEW_MAIN}/.review/notes", --worktrees, "${REVIEW_WORKTREES}", --host, 0.0.0.0, --port, "8000"] + command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --worktrees, "${REVIEW_WORKTREES}", --host, 0.0.0.0, --port, "8000"] environment: PYTHONDONTWRITEBYTECODE: "1" + PAPERCUTS_URL: https://${REVIEW_PAPERCUTS_HOST:?set REVIEW_PAPERCUTS_HOST in .env} read_only: true cap_drop: [ALL] security_opt: [no-new-privileges:true] @@ -31,7 +33,6 @@ services: volumes: - ${REVIEW_MAIN}:${REVIEW_MAIN}:ro - ${REVIEW_WORKTREES:?set REVIEW_WORKTREES in .env}:${REVIEW_WORKTREES}:ro - - ${REVIEW_MAIN}/.review/notes:${REVIEW_MAIN}/.review/notes:rw networks: [proxy] # no ports: the lab proxy routes to it labels: - traefik.enable=true @@ -42,9 +43,39 @@ services: - traefik.http.services.t7d-review.loadbalancer.server.port=8000 - homepage.group=Apps - homepage.name=t7d.dev (Review) - - homepage.description=Working copy with live reload and notes + - homepage.description=Working copy with live reload and papercuts - homepage.href=https://${REVIEW_HOST} + t7d-review-papercuts: + build: ${PAPERCUTS_SRC:?set PAPERCUTS_SRC in .env} + container_name: t7d-review-papercuts + restart: unless-stopped + user: "1001:1001" + environment: + PAPERCUTS_STORE: local + PAPERCUTS_DB: /data/papercuts.db + PAPERCUTS_HOST: 0.0.0.0 + PAPERCUTS_ORIGINS: https://${REVIEW_HOST} + PAPERCUTS_REPOS: Tim-Conrad/Tim-Conrad.github.io + PAPERCUTS_PUBLIC_URL: https://${REVIEW_PAPERCUTS_HOST} + read_only: true + cap_drop: [ALL] + security_opt: [no-new-privileges:true] + mem_limit: 128m + tmpfs: + - /tmp + logging: { driver: json-file, options: { max-size: "10m", max-file: "3" } } + volumes: + - ${REVIEW_MAIN}/.papercuts:/data:rw + networks: [proxy] # no ports: the lab proxy routes to it + labels: + - traefik.enable=true + - traefik.http.routers.t7d-review-papercuts.rule=Host(`${REVIEW_PAPERCUTS_HOST}`) + - traefik.http.routers.t7d-review-papercuts.entrypoints=websecure + - traefik.http.routers.t7d-review-papercuts.tls=true + - traefik.http.routers.t7d-review-papercuts.middlewares=authelia@file,security-headers@file + - traefik.http.services.t7d-review-papercuts.loadbalancer.server.port=8000 + networks: proxy: external: true diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index bccce00..94f88f6 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 -"""Review server for the site: live reload, the rounds of a branch and notes. Only the review site uses it. GitHub Pages serves t7d.dev. +"""Review server for the site: live reload and the rounds of a branch. Only the review site uses it. GitHub Pages serves t7d.dev. - python3 scripts/review/devserver.py [--root .] [--notes .review/notes] [--host 127.0.0.1] [--port 8000] + python3 scripts/review/devserver.py [--root .] [--host 127.0.0.1] [--port 8000] - Serves the repository root with Cache-Control: no-cache. A missing page gets /404.html with status 404. - Every HTML page gets /__review/review.css, /__review/textdiff.js and /__review/review.js. The page CSP gets connect-src 'self', so the @@ -10,12 +10,11 @@ The page then loads again. - The branch comes from ?branch=, else the cookie review_branch, else the branch of the main clone. A page with a valid ?branch= sets the cookie. An unknown branch gives the main clone. Pages come from the worktree of that branch. -- /__review/api/state: the branch and the commit of the chosen worktree, and the number of open notes. +- /__review/api/state: the branch and the commit of the chosen worktree,. - /__review/api/versions?branch=: the branches, the rounds of the branch since master, and if the worktree has changes. - /__review/api/text?branch=&rev=&path=: the HTML of a page at that revision, or null. -- /__review/api/notes: GET lists the notes, open and done. POST writes one note as /.json. The POST needs - the header X-T7d-Review and a JSON body: a plain form from another site cannot send either. A handled note goes to - /done/ with an "answer" field. +- Env PAPERCUTS_URL (the relay of the papercuts widget): each page gets its script tag, and the origin of the URL goes into + script-src, connect-src and img-src of the CSP. Without it, the page has no widget. - /llms.txt, /thoughts/feed.xml and /blog/feed.xml are made on each request by scripts/gen-feeds.py, as the deploy makes them for gh-pages. - /.git, /.review, /.reels and /node_modules are not served. """ @@ -24,10 +23,10 @@ import json import os import re -import secrets import sys import time import urllib.parse +from html import escape as html_escape from functools import partial from http.cookies import SimpleCookie from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer @@ -41,9 +40,6 @@ BASE = "master" ASSETS = {"review.js": "text/javascript; charset=utf-8", "textdiff.js": "text/javascript; charset=utf-8", "review.css": "text/css; charset=utf-8"} -MAX_BODY = 32 * 1024 -MAX_NOTE = 4000 -NOTE_ID = re.compile(r"^\d{8}-\d{6}-[0-9a-f]{6}$") GENERATED = {"/llms.txt": "text/plain; charset=utf-8", "/thoughts/feed.xml": "application/rss+xml; charset=utf-8", "/blog/feed.xml": "application/rss+xml; charset=utf-8"} @@ -83,55 +79,44 @@ def git_state(path): return gitread.head(path) -def inject(html): - """The page with the review assets, and connect-src 'self' in its CSP.""" +def add_source(policy, directive, origin): + """The policy with origin in the directive: appended if the directive exists, else a new one that keeps 'self'.""" + m = re.search(r"(^|;\s*)%s\s+([^;]*)" % directive, policy) + if m: + return policy[:m.end()] + " " + origin + policy[m.end():] + return policy.rstrip("; ") + "; %s 'self' %s" % (directive, origin) + + +def inject(html, widget=None, branch="", commit=""): + """The page with the review assets, and connect-src 'self' in its CSP. + With widget (the URL of the papercuts relay), the page also gets the widget tag and the relay in the CSP.""" + origin = None + if widget: + u = urllib.parse.urlsplit(widget) + origin = "%s://%s" % (u.scheme, u.netloc) + def csp(m): policy = m.group(2) if "connect-src" not in policy: policy = policy.rstrip("; ") + "; connect-src 'self'" + if origin: + for d in ("script-src", "connect-src", "img-src"): + policy = add_source(policy, d, origin) return m.group(1) + policy + m.group(3) html = CSP.sub(csp, html, count=1) link = '' % PREFIX # textdiff.js first: deferred scripts run in order, and review.js uses window.reviewDiff. script = '\n' % (PREFIX, PREFIX) + if widget: + script += '\n' % ( + html_escape(widget.rstrip("/")), html_escape(branch), html_escape(commit)) html = html.replace("", link + "\n" + script + "\n", 1) if "" in html else link + script + html return html -def clean_note(d): - """The stored note from a POST body, or a ValueError. Only known fields, with limits.""" - if not isinstance(d, dict): - raise ValueError("the body is not a JSON object") - note = d.get("note") - if not isinstance(note, str) or not note.strip() or len(note) > MAX_NOTE: - raise ValueError("note: 1 to %d characters" % MAX_NOTE) - - def text(k, n): - v = d.get(k) - return v[:n] if isinstance(v, str) else None - - def num(v): - return round(float(v), 1) if isinstance(v, (int, float)) and not isinstance(v, bool) else None - - out = {"note": note.strip(), "path": text("path", 500), "title": text("title", 200), "theme": text("theme", 10), - "selection": text("selection", 1000), "agent": text("agent", 300)} - for k in ("viewport", "scroll"): - v = d.get(k) - out[k] = {a: num(v.get(a)) for a in ("w", "h", "x", "y", "dpr") if a in v} if isinstance(v, dict) else None - t = d.get("target") - if isinstance(t, dict): - r = t.get("rect") if isinstance(t.get("rect"), dict) else {} - out["target"] = {"selector": t["selector"][:500] if isinstance(t.get("selector"), str) else None, - "text": t["text"][:300] if isinstance(t.get("text"), str) else None, - "rect": {a: num(r.get(a)) for a in ("x", "y", "w", "h")}} - else: - out["target"] = None - return out - - class Handler(SimpleHTTPRequestHandler): root = "." - notes = ".review/notes" + widget = None # the URL of the papercuts relay, or None for no widget worktrees = None # the folder that holds the site worktrees, or None for any def end_headers(self): @@ -200,7 +185,8 @@ def send_json(self, code, obj): self.wfile.write(body) def send_page(self, path, code=200): - body = inject(read(path)).encode() + state = git_state(self.wt) + body = inject(read(path), self.widget, state.get("branch") or "", state.get("commit") or "").encode() self.send_response(code) self.send_header("Content-Type", "text/html; charset=utf-8") self.send_header("Content-Length", str(len(body))) @@ -263,13 +249,11 @@ def review_get(self, name): if name == "reload": return self.reload() if name == "api/state": - return self.send_json(200, {**git_state(self.wt), "open": len(self.list_notes("open"))}) + return self.send_json(200, git_state(self.wt)) if name == "api/versions": return self.versions() if name == "api/text": return self.text() - if name == "api/notes": - return self.send_json(200, {"notes": self.list_notes("open") + self.list_notes("done")}) self.send_error(404) def versions(self): @@ -317,59 +301,21 @@ def reload(self): except (BrokenPipeError, ConnectionResetError): pass # the page closed - def list_notes(self, status): - d = self.notes if status == "open" else os.path.join(self.notes, "done") - out = [] - try: - names = sorted(os.listdir(d), reverse=True) - except OSError: - return out - for f in names: - if not (f.endswith(".json") and NOTE_ID.match(f[:-5])): - continue - try: - n = json.loads(read(os.path.join(d, f))) - except (OSError, ValueError): - continue - out.append({"id": f[:-5], "status": status, "time": n.get("time"), "path": n.get("path"), - "note": n.get("note", "")[:200], "answer": n.get("answer") if status == "done" else None}) - return out - def do_POST(self): - if self.path.split("?", 1)[0] != PREFIX + "api/notes": - return self.send_error(404) - if not self.headers.get("X-T7d-Review") or "application/json" not in (self.headers.get("Content-Type") or ""): - return self.send_json(403, {"error": "the review page sends notes"}) - n = int(self.headers.get("Content-Length") or 0) - if n <= 0 or n > MAX_BODY: - return self.send_json(413, {"error": "the note is too large"}) - try: - note = clean_note(json.loads(self.rfile.read(n))) - except ValueError as e: - return self.send_json(400, {"error": str(e)}) - self.pick() - note.update(git_state(self.wt)) - note["time"] = time.strftime("%Y-%m-%dT%H:%M:%S%z") - nid = time.strftime("%Y%m%d-%H%M%S-") + secrets.token_hex(3) - os.makedirs(self.notes, exist_ok=True) - tmp = os.path.join(self.notes, "." + nid + ".tmp") - with open(tmp, "w", encoding="utf-8") as f: - json.dump({"id": nid, **note}, f, ensure_ascii=False, indent=1) - os.replace(tmp, os.path.join(self.notes, nid + ".json")) - self.send_json(201, {"id": nid}) + self.send_error(404) # the server has no write API def main(): ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) ap.add_argument("--root", default=os.path.normpath(os.path.join(HERE, "..", ".."))) - ap.add_argument("--notes", default=None, help="the notes folder (default: /.review/notes)") ap.add_argument("--worktrees", default=os.environ.get("REVIEW_WORKTREES"), help="offer only the worktrees below this folder (default: env REVIEW_WORKTREES, else any)") ap.add_argument("--host", default="127.0.0.1") ap.add_argument("--port", type=int, default=8000) a = ap.parse_args() root = os.path.abspath(a.root) - handler = partial(type("H", (Handler,), {"root": root, "worktrees": a.worktrees and os.path.abspath(a.worktrees), "notes": os.path.abspath(a.notes or os.path.join(root, ".review", "notes"))}), + handler = partial(type("H", (Handler,), {"root": root, "worktrees": a.worktrees and os.path.abspath(a.worktrees), + "widget": os.environ.get("PAPERCUTS_URL") or None}), directory=root) srv = ThreadingHTTPServer((a.host, a.port), handler) srv.daemon_threads = True diff --git a/scripts/review/show.py b/scripts/review/show.py deleted file mode 100644 index a8db480..0000000 --- a/scripts/review/show.py +++ /dev/null @@ -1,78 +0,0 @@ -#!/usr/bin/env python3 -"""Show where a review note points: open the page at the width and in the theme of the note, frame the part in red, -and save /.png next to the note (it stays out of git). - - uv run --no-project --with playwright python3 scripts/review/show.py [ ...] [--suffix after] - ---suffix after writes .after.png: run it after the fix and compare the two pictures. The script serves the -working tree itself (devserver.py on a free port), so it shows the files as they are now. -""" -import argparse -import json -import os -import sys -import threading -from functools import partial -from http.server import ThreadingHTTPServer - -HERE = os.path.dirname(os.path.abspath(__file__)) -sys.path.insert(0, HERE) -import devserver # noqa: E402 - -FRAME = """([sel, rect, scroll]) => { - let r = null; const n = sel && document.querySelector(sel); - if (n) { n.scrollIntoView({ block: 'center', behavior: 'instant' }); r = n.getBoundingClientRect(); } - else { scrollTo({ left: scroll.x || 0, top: scroll.y || 0, behavior: 'instant' }); - if (rect && rect.w) r = { left: rect.x - scrollX, top: rect.y - scrollY, width: rect.w, height: rect.h }; } - for (const x of document.querySelectorAll('.rv-tab, .rv-panel, .rv-box')) x.remove(); - if (!r) return n ? 'found' : 'no part'; - const b = document.createElement('div'); document.body.appendChild(b); - Object.assign(b.style, { position: 'fixed', zIndex: 2147483647, pointerEvents: 'none', border: '4px solid #e0201b', - left: (r.left - 6) + 'px', top: (r.top - 6) + 'px', width: (r.width + 12) + 'px', height: (r.height + 12) + 'px' }); - return n ? 'found' : 'by position'; -}""" - - -def main(): - ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) - ap.add_argument("ids", nargs="+") - ap.add_argument("--suffix", default="") - ap.add_argument("--notes", default=os.path.join(HERE, "..", "..", ".review", "notes")) - a = ap.parse_args() - from playwright.sync_api import sync_playwright - - root = os.path.normpath(os.path.join(HERE, "..", "..")) - notes = os.path.abspath(a.notes) - h = partial(type("H", (devserver.Handler,), {"root": root, "notes": notes}), directory=root) - srv = ThreadingHTTPServer(("127.0.0.1", 0), h) - srv.daemon_threads = True - threading.Thread(target=srv.serve_forever, daemon=True).start() - base = "http://127.0.0.1:%d" % srv.server_port - - with sync_playwright() as pw: - chrome = "/usr/bin/google-chrome" # the system Chrome fits every Playwright version, a cached build only one - browser = pw.chromium.launch(executable_path=chrome) if os.path.exists(chrome) else pw.chromium.launch() - for nid in a.ids: - path = next((p for p in (os.path.join(notes, nid + ".json"), os.path.join(notes, "done", nid + ".json")) if os.path.isfile(p)), None) - if not path: - print("%s: no such note" % nid) - continue - n = json.loads(devserver.read(path)) - vp, theme = n.get("viewport") or {}, n.get("theme") if n.get("theme") in ("light", "dark") else "light" - ctx = browser.new_context(viewport={"width": int(vp.get("w") or 1440), "height": int(vp.get("h") or 900)}, - device_scale_factor=min(float(vp.get("dpr") or 1), 2), color_scheme=theme) - ctx.add_init_script("try { localStorage.setItem('theme', %s) } catch (e) {}" % json.dumps(theme)) - page = ctx.new_page() - page.goto(base + (n.get("path") or "/"), wait_until="networkidle") - t = n.get("target") or {} - how = page.evaluate(FRAME, [t.get("selector"), t.get("rect"), n.get("scroll") or {}]) - out = os.path.join(os.path.dirname(path), nid + ("." + a.suffix if a.suffix else "") + ".png") - page.screenshot(path=out) - ctx.close() - print("%s: %s (%s, %s px, %s) -> %s" % (nid, n.get("note", "")[:60], n.get("path"), vp.get("w"), how, out)) - browser.close() - srv.shutdown() - - -if __name__ == "__main__": - main() diff --git a/scripts/review/test_devserver.py b/scripts/review/test_devserver.py index cef1a54..89148d3 100644 --- a/scripts/review/test_devserver.py +++ b/scripts/review/test_devserver.py @@ -1,6 +1,7 @@ """Tests of the review server. Run: python3 -m unittest scripts/review/test_devserver.py""" import json import os +import re import shutil import subprocess import sys @@ -38,21 +39,18 @@ def test_keeps_an_existing_connect_src(self): self.assertEqual(devserver.inject(page).count("connect-src"), 1) -class CleanNote(unittest.TestCase): - def test_keeps_known_fields_with_limits(self): - n = devserver.clean_note({"note": " Fix it ", "path": "/x/", "theme": "dark", "evil": "x", - "viewport": {"w": 390, "h": 844, "dpr": 3, "q": 1}, - "target": {"selector": "#a", "text": "t" * 999, "rect": {"x": 1, "y": 2, "w": 3, "h": True}}}) - self.assertEqual(n["note"], "Fix it") - self.assertNotIn("evil", n) - self.assertEqual(n["viewport"], {"w": 390.0, "h": 844.0, "dpr": 3.0}) - self.assertEqual(len(n["target"]["text"]), 300) - self.assertIsNone(n["target"]["rect"]["h"]) - - def test_rejects_an_empty_or_long_note(self): - for bad in ({"note": " "}, {"note": "x" * 4001}, {"note": 5}, ["note"]): - with self.assertRaises(ValueError): - devserver.clean_note(bad) +class Widget(unittest.TestCase): + def test_widget_tag_and_csp_only_with_the_env(self): + self.assertNotIn("papercuts", devserver.inject(PAGE).lower()) + self.assertNotIn("papercuts", devserver.inject(PAGE, widget="").lower()) + out = devserver.inject(PAGE, widget="https://p.example", branch="feat/x", commit="abc1234") + self.assertEqual(out.count("