Conversation
Covers the home page, the theme-preview page, one nav page per top-level section, the MCP metadata panel, and mobile navigation, in light and dark mode. Runs on every PR inside a pinned Playwright Docker image so committed baselines stay pixel-consistent across runs. Baselines can only be regenerated in CI, never copied in from a local run, via an /update-snapshots PR comment (gated to collaborators with write access). A second workflow folds new/changed/deleted baselines into the PR description as an image table, using GitHub's own raw blob URLs since the PNGs are already committed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical workflow defects, missing baselines, and nondeterministic metadata prevent reliable approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Playwright visual regression coverage for layouts, themes, navigation, metadata, and mobile behavior, plus CI automation for baselines and PR screenshot summaries.
Changes:
- Adds desktop and mobile visual tests with theme fixtures.
- Adds pinned CI and authorized snapshot regeneration workflows.
- Adds automated screenshot summary generation.
File summaries
| File | Description |
|---|---|
tests/visual/README.md |
Documents visual testing and baseline updates. |
tests/visual/pages.spec.ts |
Defines representative page and metadata tests. |
tests/visual/mobile.spec.ts |
Tests mobile layouts and navigation. |
tests/visual/fixtures.ts |
Provides theme and metadata helpers. |
scripts/update-visual-baselines.sh |
Regenerates and commits snapshots. |
scripts/update-pr-screenshot-summary.mjs |
Updates PR descriptions. |
scripts/pr-screenshot-summary.mjs |
Generates screenshot summaries. |
playwright.config.ts |
Configures Playwright projects and serving. |
package.json |
Adds Playwright scripts and dependencies. |
package-lock.json |
Locks package versions. |
.gitignore |
Ignores Playwright output. |
.github/workflows/update-visual-baselines.yaml |
Handles authorized baseline updates. |
.github/workflows/pr-screenshot-summary.yaml |
Publishes screenshot summaries. |
.github/workflows/on-pr.yaml |
Invokes visual regression checks. |
.github/workflows/_visual-regression.yaml |
Runs visual regression CI. |
Review details
Suppressed comments (6)
.github/workflows/pr-screenshot-summary.yaml:11
- The baseline update job pushes with
GITHUB_TOKEN, and GitHub suppresses workflow-triggering events caused by that token. Consequently, its commit will not produce thesynchronizeevent this workflow relies on, so the promised screenshot summary will not be added after/update-snapshots; invoke the summary update from the trusted baseline workflow or use a token/event path that can trigger it.
types: [opened, synchronize, edited]
.github/workflows/pr-screenshot-summary.yaml:13
- The
pathsfilter is evaluated against changed files, so apull_request.editedevent caused only by editing the PR body has no matching snapshot path and will be skipped. That means the claimed self-healing behavior cannot restore the summary after a human overwrites it. Remove the path filter for this workflow and gate the job in anif, or split body-edit handling into a separate trigger.
types: [opened, synchronize, edited]
paths:
- 'tests/visual/**/*.spec.ts-snapshots/**'
.github/workflows/update-visual-baselines.yaml:43
- The collaborators API returns repository permissions as
pushormaintainfor ordinary write-capable collaborators, notwrite. As written, the documented/update-snapshotscommand will reject the people who should be allowed to run it; acceptpush,maintain, andadmin(or use an equivalent permission check).
if [[ "$PERMISSION" != "admin" && "$PERMISSION" != "write" ]]; then
.github/workflows/update-visual-baselines.yaml:106
- The snapshot-regeneration job has the same nondeterministic
install-thv.shdependency: it can use a different latest CLI or registry response from the comparison job, so/update-snapshotscan generate baselines that immediately fail the check. Use the same pinned CLI and metadata source in both jobs.
run: ./scripts/install-thv.sh
tests/visual/README.md:19
toHaveScreenshot()performs a pixel-level comparison with configurable tolerances; it is not a byte-for-byte comparison. This wording overstates the matcher and can mislead contributors about what a visual failure means.
**Local baselines won't match CI.** `toHaveScreenshot()` compares byte-for-byte,
and font rendering differs by host OS/GPU even on the same Chromium build. CI
tests/visual/fixtures.ts:47
generatedAtmakes every metadata sidecar change whenever--update-snapshotsrewrites a PNG, even when the PNG bytes are identical. Because the update script treats any change under the snapshot directory as a reason to commit and push, repeating/update-snapshotscreates needless baseline commits and reruns PR checks. Keep sidecars deterministic by removing this field or writing metadata only when its stable contents change.
generatedAt: new Date().toISOString(),
- Files reviewed: 13/15 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
actions/checkout marks the repo safe in the runner's git config, but a run: step inside container: executes via docker exec into a separate environment that never got that exception. Docusaurus's build shells out to git for last-update metadata, which failed with "detected dubious ownership" before this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mirrors sep's toolhive-cloud-ui-e2e.yml auto-fix flow: when the check fails on a same-repo PR branch, a follow-up job regenerates baselines against the same tree the check tested and pushes the result, unless the last commit was already a baseline update (avoids chaining) or the PR is from a fork (no write secrets there). A notify job comments the outcome either way. /update-snapshots remains as the fork/manual fallback. Also fixes a real bug found while testing this: the 'tests/visual/**/*.spec.ts-snapshots/**' pathspec silently matched zero files in both git status and git diff --name-status (confirmed against a real git repo), which would have made the diff-detection in both update-visual-baselines.sh and pr-screenshot-summary.mjs always report "nothing changed". Replaced with the plain 'tests/visual/*.spec.ts-snapshots/*' pattern, verified against a throwaway repo to match files in both snapshot directories. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The auto-fix job's if: lacked always(), so GitHub's implicit
"every dependency must succeed" gate silently skipped it whenever
visual-regression (one of its needs) failed — exactly the case it
exists to react to. Confirmed live: eligibility check correctly
returned eligible=true, but the job still showed as skipped.
Also: the notify job and update-visual-baselines.yaml's comment-result
job call `gh pr comment` with no checkout step in that job, so gh had
no git remote to auto-detect the repo from ("not a git repository").
Added --repo explicitly to every such call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
❌ No baseline auto-fix could be pushed. Either regenerating reproduced no diff (consistent with a real, non-snapshot failure — no baseline change will fix that) or the auto-fix step itself failed. See the failing-tests list in the Visual regression job for what actually broke. |
Discovered live: a github-actions[bot]/GITHUB_TOKEN-authored push triggers a check run that GitHub now holds for manual approval, even on a same-repo branch. Note it in the visual regression README so it's not a mystery next time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kantord
marked this pull request as ready for review
September 18, 2026 17:58
Checked the live PR: GitHub renders these at whatever width the PR body happens to be (~711px measured), well under our 1280px capture width, so the browser downscales by ~44%. That softens small body text far more than it would cloud-ui's sparser dashboard UI, which is why these looked blurry there but not in cloud-ui's own screenshots. deviceScaleFactor alone doesn't fix it — toHaveScreenshot() defaults to scale: 'css', which downsamples back to CSS pixel dimensions regardless of deviceScaleFactor (confirmed: dimensions stayed 1280x3524 until scale: 'device' was added). With both set, the PNG is genuinely higher-resolution, so GitHub's inevitable downscale is supersampling real data instead of stretching a 1x image. Baselines will regenerate at the new (larger) dimensions via the usual check-fails auto-fix flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
|
/update-snapshots |
Capture navigation sidebars instead of full content pages, use neutral metadata masks, and pin ToolHive metadata generation so intentional docs churn does not create noisy baselines.
danbarr
previously approved these changes
Sep 18, 2026
danbarr
dismissed
their stale review
September 18, 2026 21:26
Need to fix workflow security issues first
Run screenshot summary updates from the trusted default-branch workflow and treat PR revisions as read-only Git data. Generate baselines without write access, then validate and publish snapshot-only artifacts in a separate trusted job.
danbarr
approved these changes
Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Visual regression screenshots
Auto-generated by CI from
scripts/pr-screenshot-summary.mjs— edits inside this block are overwritten on the next push.🟢 New
//toolhive/guides-cli//toolhive/guides-mcp/context7/ai-gateway/connector-gateway/toolhive/concepts/platform/toolhive/theme-previewDescription
Adds targeted Playwright visual regression coverage for the shared docs layout, themes, and navigation described in #1163:
The check compares committed baselines in a pinned Playwright container on every pull request. Content changes on the full-page canaries will produce a diff; masked metadata values and content outside the sidebar viewport will not. Baseline updates are deliberate rather than automatic. After this workflow is present on the default branch, a collaborator with write access can accept an intentional change with
/update-snapshotsand review the generated before-and-after summary.Type of change
Related issues/PRs
Closes #1163
Screenshots
Generated automatically in the visual regression screenshot summary above.
Submitter checklist
Content and formatting