[romchornyi]

Issue being fixed or feature implemented

The Before / After screenshots check (#34) told screenshots from other images with a badge denylist: badge hosts, badge / badges path segments, badge SVG names, and an exemption for GitHub hosts where badge can be a branch name. Every review round on #34 grew the list, and each rule misclassified in both directions. The last bot review on #34 reproduced three cases:

• a feat/badges branch dropped real screenshots;
• a .github/badge/coverage.svg badge counted as a screenshot;
• a GitHub Pages badge/ folder dropped screenshots.

The template asks authors to drag files into the table, and a dragged file is an upload. An author without a browser (gh pr create, an agent) cannot upload attachments; this repo's earlier PRs (#10, #14, #19, #20) show screenshots as repository files pinned to a commit instead.

What was done?

• Uploads and commit-pinned files count. An image or video counts when it is a file uploaded to GitHub, identified by its UUID in any form body_html can hold:

  • github.com/user-attachments/assets/<uuid> (the markdown form; GitHub leaves it in a <picture> and for some viewers);
  • the older github.com/<owner>/<repo>/assets/<user>/<uuid>;
  • the rendered private-user-images.githubusercontent.com/<user>/<n>-<uuid>[.<ext>]?jwt=…, the older user-images.githubusercontent.com/… and the oldest cloud.githubusercontent.com/assets/….

  Or, for images only, when it is a repository file pinned to a full 40-hex commit SHA (GitHub strips a hand-written <video> whose file is not an upload, so a video must be dragged in): raw.githubusercontent.com/<owner>/<repo>/<sha>/…, github.com/<owner>/<repo>/raw/<sha>/…, or …/blob/<sha>/…?raw=…. Such a file is keyed by commit and normalised path, so a fork, a .. segment or percent-encoding still counts once. It must be embedded (![](…)); a plain link does not count.

  The same upload in two forms counts once. data-canonical-src is read first, then src. Only https, and the whole path must match. A <picture> counts what a light-mode and a dark-mode reader see (GitHub's light / dark pattern): the first <source> before the <img> whose media applies, else the <img>.

• Never counted: badges, emoji, externally hosted images (directly or through camo), files on a branch or a short SHA, and user-attachments/files/ attachments.

• Removed: the whole badge machinery (NOT_SCREENSHOT_HOSTS, BADGE_*, BRANCH_HOSTS, is_badge()) and the emoji class check.

• Docs: the error message, the template comment and CLAUDE.md name both ways: drag the files in, or link files pinned to a full commit SHA.

• Heading regex: the Before / After gaps are bounded, so a long run of punctuation no longer makes the match quadratic.

• Known gap: an upload GitHub cannot resolve (a made-up UUID) still counts. A reader sees it as a broken image.

How Has This Been Tested?

• python3 .github/scripts/tests/screenshots-test.py: 118 cases, all pass.
  • The fixtures use the markup GitHub actually renders, with made-up UUIDs: a signed private-user-images <img>, and a dragged video as a <details><video> block.
  • Mutating fullmatch, the case folding, the src fallback, the attribute order or the user-images host each fails a case.
• actionlint is clean.
• Live PRs keep their results. DashUIKit feat(list): add List8View, a label ↔ avatar + value row #23 (three uploads) passes. ci: build, test and API-break check #29 and ci: require Before / After screenshots for UI changes #34 change no UI files. chore!: raise the deployment target to iOS 18 #15, ci: asset and convention checks #30, fix(search-bar): style the typed text and drive Cancel from focus #31 and refactor(scroll-view): read the offset from onScrollGeometryChange #32 fail.
• Earlier PRs, run against the new rule: feat(components): Criteria and SimpleSelect, plus the field and sheet work behind them #19 and feat(icons): add menu-identities and menu-voting #20 (commit-pinned files) pass. feat(transaction-view): optional amount accessory SF Symbol #10 (one image) and feat(bottom-sheet): add dismissal controls #14 (screenshots outside a Before / After heading) fail.
• GitHub's markdown API was used to check that a raw file in a <picture>'s srcset is not proxied through camo.
• Real videos: the uploaded videos in dashwallet-ios #1101–#1103 are recognised.
• Local /code-review high rounds were run until clean, before the PR and again after the bot review.

Before / After

• No visual change (refactor, tests, docs). Say why in "What was done?".

CI config only, no files under Sources/.

Breaking Changes

None. A description that passed with images hosted elsewhere or on a branch now needs uploads or commit-pinned files.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: dashpay/DashUIKit
• Branch: ci/screenshots-uploads-only
• File: .coderabbit.yaml
• Commit: 53aa5aa
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 443798f4-8417-4357-86f7-daa13024df39

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[romchornyi-pitcrew]

✅ Review posted for commit 53aa5aa · triage: normal · approve · took 2 min

[romchornyi-pitcrew]

🟡 1 major (inline) · 6 non-blocking recommendations · no blockers — reviewed at 4f707afd85

Level	Verdict
1 · Idea	✅ valid	Replacing the badge denylist with a positive "is this an upload" rule ends the misclassifications #34's review rounds kept finding
2 · Architecture	✅ right layer	Same script and workflow (pull_request_target, base-branch script, rendered body_html); the workflow itself is unchanged
3 · Completeness	✅ complete	Template comment, CLAUDE.md, error text and docstring follow. No other doc mentions the rule
4 · Code	🟡 1 major	The rule rejects the commit-pinned screenshots this repo's authors have used, including every CLI/agent-opened PR

Major (inline)

• DashUIKit/.github/scripts/screenshots-check.py

  Lines 55 to 57 in 4f707af

  	UPLOAD_HOSTS = ("private-user-images.githubusercontent.com", "user-images.githubusercontent.com")
  	RENDERED = re.compile(r"/\d+/\d+-(%s)\.\w+" % UUID)
  	MARKDOWN_FORM = re.compile(r"/user-attachments/assets/(%s)/?|/[^/]+/[^/]+/assets/\d+/(%s)/?" % (UUID, UUID))

  — commit-pinned raw.githubusercontent.com screenshots (the form used in #10, #14, #19, #20) no longer count, and CLI/agent authors have no way to upload.

6 non-blocking recommendations

1. <picture> light/dark counts once —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 88 in 4f707af

   key = upload(attrs.get("data-canonical-src")) or upload(attrs.get("src"))

   — GitHub's documented themed-image pattern (<source media="(prefers-color-scheme: dark)" srcset=…dark> + <img src=…light>) holds two uploads but counts one. Counting a <source srcset> upload inside a <picture> would let the template's "light and dark" be met that way. The PR documents the current choice; (reproduced: "found 1").
2. Extensionless rendered uploads —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 56 in 4f707af

   RENDERED = re.compile(r"/\d+/\d+-(%s)\.\w+" % UUID)

   — RENDERED requires \.\w+, so private-user-images…/1/2-<uuid> (no extension) is not recognised. Making the extension optional costs nothing. (reproduced; whether GitHub renders such a URL is not verified)
3. cloud.githubusercontent.com/assets/<user>/<n>/<uuid>.<ext> —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 55 in 4f707af

   UPLOAD_HOSTS = ("private-user-images.githubusercontent.com", "user-images.githubusercontent.com")

   — the pre-user-images upload host is a genuine GitHub upload but is not in UPLOAD_HOSTS, while the comment says every form counts. (not verified that such URLs still render)
4. Guidance says "images" only —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 212 to 215 in 4f707af

   	summary('Drag at least two images (before and after, light and dark where it differs) into the '
   	'"Before / After" section of the description (or a Screenshots section), so GitHub uploads them; links to images '
   	'hosted elsewhere do not count. Or tick "No visual change" and say why. '
   	"Editing the description re-runs this check.")

   — videos also pass, and the template comment says "at least two images". "images or videos" in both would tell an author with an animation change that a recording works.
5. upload() readability —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 128 to 138 in 4f707af

   	def upload(src):
   	"""The UUID of a file uploaded to GitHub, else None. The query only signs the URL."""
   	parts = split((src or "").strip())
   	if parts is None or parts.scheme != "https":
   	return None
   	pattern = RENDERED if parts.hostname in UPLOAD_HOSTS else MARKDOWN_FORM if parts.hostname == "github.com" else None
   	match = pattern and pattern.fullmatch(parts.path.lower())
   	return match and next(g for g in match.groups() if g)
   	def gh(path, accept=None, paginate=False):

   — the chained ternary and match and next(g for g in match.groups() if g) return None, a falsy match or a str by short-circuit. A {host: pattern} lookup with an explicit if not match: return None and match.group(1) or match.group(2) reads more plainly.
6. BEFORE_AFTER backtracks quadratically (outside this diff) —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 39 in 4f707af

   BEFORE_AFTER = re.compile(r"before\W*(and|vs\.?|to)?\W*after", re.I)

   — a heading of "before" followed by ~62k punctuation and no "after" took ~18 s locally. The 65,536-character body limit keeps it under the job's 5-minute timeout, so the worst case is a slow check for the author's own PR. before\W{0,20}(and|vs\.?|to)?\W{0,20}after bounds it. (reproduced by the security lane)

Review details

First review of this PR. Levels 1–3 inline. Level 4 ran the general lane (/code-review high) and a security lane, because the script parses the PR author's description in pull_request_target. The security lane found no reachable sink: upload keys are never printed, malformed src values exit 0/1 without a traceback, and the upload regexes are linear. The verifier pulled body_html of #10, #14, #19, #20 and #23 to check the major, and ran the reported inputs through upload(). Dropped:

• a made-up UUID counts as an upload: documented in the docstring as a known gap, and a reader sees a broken image;
• missing test cases for the major and item 1: folded into those items.

🤖 Reviewed with Claude Code

[romchornyi]

Re: the review on 4f707af — the major is answered in its thread. Recommendations 1, 2, 4, 5 and 6 are fixed in 9b4dcee: each <source> of a <picture> counts (one key per <source>); extensionless rendered uploads count; the guidance and template say images or videos; upload() is rewritten as screenshot() with explicit branches; the Before / After gaps are bounded (a 62k-punctuation heading takes ~1 ms). Recommendation 3 is fixed too: cloud.githubusercontent.com/assets/… counts. 98 cases pass.

[romchornyi-pitcrew]

✅ no blockers · no majors · 7 non-blocking recommendations — reviewed at 9b4dcee9ec

Level	Verdict
1 · Idea	✅ valid	Uploads plus commit-pinned repository files: positive rules, no denylist
2 · Architecture	✅ right layer	Unchanged
3 · Completeness	✅ complete	Template, CLAUDE.md, error text, docstring and PR body follow
4 · Code	✅ clean	All 98 cases pass at this head; #19 and #20 pass from their real body_html

Closed since the last review (bot review on 4f707af)

• Major: commit-pinned repository files count (raw.githubusercontent.com/…/<sha>/…, …/raw/<sha>/…, …/blob/<sha>/…?raw=…), keyed by commit and normalised path. Files on a branch and short SHAs still don't.
• A light / dark <picture> counts both variants.
• Extensionless rendered uploads and cloud.githubusercontent.com uploads count.
• The guidance and template say images or videos.
• upload() is rewritten as screenshot() with explicit branches.
• The Before / After regex gaps are bounded (~1 ms on the 62k-punctuation heading).

7 non-blocking recommendations

1. "link" vs "embed" —

   DashUIKit/.github/PULL_REQUEST_TEMPLATE.md

   Line 20 in 9b4dcee

   <!--- Required when the PR changes anything under Sources/, Package.swift or Package@swift-*.swift: CI checks for at least two images or videos here: drag the files in, or link repository files pinned to a full 40-character commit SHA. -->

   and

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 258 to 262 in 9b4dcee

   	summary('Drag at least two images or videos (before and after, light and dark where it differs) into the '
   	'"Before / After" section of the description (or a Screenshots section), so GitHub uploads them, or '
   	"link repository files pinned to a full commit SHA (`raw.githubusercontent.com/OWNER/REPO/SHA/...`). "
   	'Images hosted elsewhere or on a branch do not count. Or tick "No visual change" and say why. '
   	"Editing the description re-runs this check.")

   — the template and the failure hint say to link pinned files, but only embedded images and videos count; [before](https://raw…/<sha>/before.png) renders as <a href> and finds 0. "embed (![](…))" says what the check needs.
2. Any <source> in a <picture> counts —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 111 to 116 in 9b4dcee

   	elif tag == "source" and self.picture and self.section_level and not self.struck:
   	# srcset lists resolution variants of one image, of which a reader sees one: the
   	# first stands for the <source>.
   	key = screenshot(((attrs.get("srcset") or "").split(",")[0].split() or [""])[0], tag)
   	if key:
   	self.sections[-1].append(key)

   — a <source media="(min-width: 800px)"> plus the <img> is one image at two sizes, yet counts as two. Counting a <source> only when its media mentions prefers-color-scheme matches the light / dark intent. (reproduced)
3. <video><source src> —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 105 to 116 in 9b4dcee

   	elif tag in ("img", "video") and self.section_level and not self.struck:
   	# Camo keeps the original URL in data-canonical-src; either attribute may hold
   	# the upload.
   	key = screenshot(attrs.get("data-canonical-src"), tag) or screenshot(attrs.get("src"), tag)
   	if key:
   	self.sections[-1].append(key)
   	elif tag == "source" and self.picture and self.section_level and not self.struck:
   	# srcset lists resolution variants of one image, of which a reader sees one: the
   	# first stands for the <source>.
   	key = screenshot(((attrs.get("srcset") or "").split(",")[0].split() or [""])[0], tag)
   	if key:
   	self.sections[-1].append(key)

   — standard video markup with the file in a <source src> never counts, so two pinned .mp4 files written that way find 0. Reading <source src> inside a <video> would cover it. (reproduced)
4. Pinned files are not checked to exist —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 170 to 181 in 9b4dcee

   	pinned = PINNED.get(parts.hostname)
   	match = pinned.fullmatch(parts.path) if pinned else None
   	if not match:
   	return None
   	route, sha, path = match.groups()
   	# A blob page is an HTML page unless a raw= parameter (any value) asks for the file itself.
   	if route.lower() == "blob" and "raw" not in parse_qs(parts.query, keep_blank_values=True):
   	return None
   	path = posixpath.normpath("/" + unquote(path).lstrip("/"))
   	if path.rsplit(".", 1)[-1].lower() not in MEDIA[tag]:
   	return None
   	return sha.lower() + path

   — a made-up 40-hex SHA with two file names passes while both images are broken. Unlike uploads, repository files can be checked: one gh api repos/<o>/<r>/contents/<path>?ref=<sha> per key would close it. Same class as the documented made-up-UUID gap. (reproduced)
5. Path normalisation is lenient —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 178 in 9b4dcee

   path = posixpath.normpath("/" + unquote(path).lstrip("/"))

   — a trailing slash (…/a.png/, a 404) still counts, and a%2Fb.png folds into a/b.png. ?raw=false / ?raw=0 on a blob counts; GitHub redirected these to the raw file in the last review's probe, so this one likely renders. (reproduced: keys returned)
6. The empty () group —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 74 in 9b4dcee

   "raw.githubusercontent.com": re.compile(r"/[^/]+/[^/]+/()([0-9a-f]{40})/(.+)", re.I),

   — it exists only to line up route, sha, path with the github.com pattern. Named groups would drop the placeholder.
7. Fixture names a personal fork —

   DashUIKit/.github/scripts/tests/screenshots-test.py

   Lines 152 to 154 in 9b4dcee

   	("commit-pinned raw files count", "pass", UI, h(2, "Before / After")
   	+ '<img src="https://raw.githubusercontent.com/romchornyi/DashUIKit-fork/d0c798679389e6ba06b9e1bf26942b99483f6dde/pr19/before-light.png">'
   	+ '<img src="https://raw.githubusercontent.com/romchornyi/DashUIKit-fork/d0c798679389e6ba06b9e1bf26942b99483f6dde/pr19/after-light.png">'),

   — romchornyi/DashUIKit-fork/…/pr19/… where the other cases use o/r; o/r/<sha>/before-light.png covers the same thing.

Review details

Re-review of the bot review on 4f707af; the new diff is 9b4dcee. Levels 1–3 re-checked inline. Level 4 used the general lane (/code-review high) on the new commit, and the verifier ran the reported inputs through screenshot() and the parser. Dropped:

• "srcset may be proxied through camo": the author checked with GitHub's markdown API that a raw file in a <picture>'s srcset is rendered as written;
• a comma inside a srcset URL: unusual file names, and the <img> path is unaffected.

🤖 Reviewed with Claude Code

[romchornyi]

Re: the review on 9b4dcee — fixed in 53aa5aa:

1. link vs embed — the template, CLAUDE.md and the failure hint say to embed pinned images with ![](…); a plain link does not count.
2. <source> in a <picture> — a <picture> now counts what a light-mode and a dark-mode reader see: the first <source> (with a srcset, before the <img>) whose media is empty, all, screen or names that colour scheme, else the <img>. A screen-size <source> or a format variant is no longer a second image, and a <picture> with no <img> shows nothing.
3. <video><source src> — checked with GitHub's markdown API: a hand-written <video> is stripped unless its file is an upload, so that markup never reaches body_html. Commit-pinned files therefore count only as images, a <video> counts only an upload, and the hint says a video must be dragged in.
4. Fixture — uses o/r.

Items 4 and 5 (made-up SHA, lenient path normalisation) are left as they are: both show up as broken images in the description. 118 cases pass.

[romchornyi-pitcrew]

✅ no blockers · no majors · 6 non-blocking recommendations — reviewed at 53aa5aaa70

Level	Verdict
1 · Idea	✅ valid	Unchanged
2 · Architecture	✅ right layer	Unchanged
3 · Completeness	✅ complete	Template, CLAUDE.md, hint, error text and PR body follow the new rules
4 · Code	✅ clean	All 118 cases pass at this head; #19 and #20 still pass from their real body_html

Closed since the last review (bot review on 9b4dcee)

• The template, CLAUDE.md and the hint say to embed pinned images with ![](…); a plain link doesn't count.
• A <picture> counts what a light-mode and a dark-mode reader see, so a screen-size <source> or a format variant is no longer a second image.
• <video><source src>: GitHub strips hand-written <video> tags that don't point at uploads, so pinned files count only as images and a <video> counts only an upload.
• The fixture uses o/r.

6 non-blocking recommendations

All six need unusual <picture> markup, and none of them can turn an ordinary light / dark pair into a wrong result.

1. Catch-all media queries —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 171 to 190 in 53aa5aa

   	def seen(picture):
   	"""The images a <picture> shows to a light-mode and a dark-mode reader: the first <source>
   	whose media applies, else the <img>. Media with no query, "all" or "screen" applies to
   	every reader, a colour-scheme query to that scheme; any other query (a screen size) is
   	taken not to apply, so the image it would replace is the one counted. A type is taken as
   	supported: a format variant then simply stands for the image."""
   	shown = []
   	for scheme in ("light", "dark"):
   	if "img" not in picture:
   	return [] # a <picture> without an <img> shows nothing
   	key = picture["img"]
   	for media, source in picture["sources"]:
   	if media in ("", "all", "screen") or "prefers-color-scheme:%s" % scheme in media:
   	key = source
   	break
   	if key and key not in shown:
   	shown.append(key)
   	return shown

   — only screen or (min-width: 1px) applies to every reader, but seen() treats it as not applying. A colour-scheme <source> after it is counted although nobody sees it. (reproduced: [z, y])
2. Compound and negated colour-scheme queries —

   DashUIKit/.github/scripts/screenshots-check.py

   Line 183 in 53aa5aa

   if media in ("", "all", "screen") or "prefers-color-scheme:%s" % scheme in media:

   — the substring test applies (prefers-color-scheme: dark) and (max-width: 1px) to every dark reader. It also applies not all and (prefers-color-scheme: dark) to dark readers, which is the opposite of what it means. (reported by the general lane)
3. A struck-out <source> —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 117 to 121 in 53aa5aa

   	elif (tag == "source" and self.picture is not None and "img" not in self.picture
   	and self.section_level and not self.struck and attrs.get("srcset")):
   	# A browser skips a <source> without srcset, and one after the <img>.
   	media = re.sub(r"\s+", "", (attrs.get("media") or "").lower())
   	self.picture["sources"].append((media, screenshot(first(attrs.get("srcset")))))

   — it is skipped, but a browser still picks it. A struck <img> is kept as the end of the source list, so <source> should behave the same way. (reported by the general lane)
4. Nested <picture> —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 122 to 134 in 53aa5aa

   	elif tag == "picture":
   	self.picture = {"sources": []}
   	elif tag in ("del", "s", "strike"):
   	self.struck += 1
   	elif tag == "input" and attrs.get("type") == "checkbox" and "checked" in attrs:
   	self.in_task, self.task_text = True, ""
   	def handle_endtag(self, tag):
   	if tag in ("del", "s", "strike") and self.struck:
   	self.struck -= 1
   	if tag == "picture" and self.picture is not None:
   	if self.section_level:
   	self.sections[-1].extend(seen(self.picture))

   — a single dict replaced the depth counter, so an inner <picture> overwrites the outer one's sources. (reproduced: the outer dark source is lost)
5. Unclosed <picture> —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 132 to 135 in 53aa5aa

   	if tag == "picture" and self.picture is not None:
   	if self.section_level:
   	self.sections[-1].extend(seen(self.picture))
   	self.picture = None

   — its images are only flushed on </picture>, so a description that ends inside one loses them. close() could flush it. (reported by the general lane)
6. Readability —

   DashUIKit/.github/scripts/screenshots-check.py

   Lines 107 to 110 in 53aa5aa

   	image = tag == "img"
   	key = None if self.struck else (
   	screenshot(attrs.get("data-canonical-src"), image) or screenshot(attrs.get("src"), image)
   	or (screenshot(first(attrs.get("srcset"))) if image else None))

   and

   DashUIKit/.github/scripts/screenshots-check.py

   Line 179 in 53aa5aa

   if "img" not in picture:

   — screenshot(src, image) reads as "is pinned" rather than "may be pinned" (allow_pinned would say it), and the "img" not in picture return belongs above the per-scheme loop.

Review details

Re-review of the bot review on 9b4dcee. The new diff is 53aa5aa. Levels 1–3 re-checked inline. Level 4 used the general lane (/code-review high) on the new commit. The verifier ran items 1 and 4 through the parser. Nothing was dropped.

🤖 Reviewed with Claude Code