fix(scripts): address visual-diff PR review comments#2578
Conversation
Introduce a Playwright-based before/after screenshot workflow with CI reporting so PRs that change console UI can attach and refresh visual evidence automatically. Co-authored-by: Cursor <cursoragent@cursor.com>
Reload route config after git checkout, honor --routes in diff, restore the original branch on failure, dedupe Supabase status helpers, and remove an unused import. Co-authored-by: Cursor <cursoragent@cursor.com>
Correct pull_request trigger types list syntax and register visual-diff.config.ts as a knip entry so dynamic imports pass deadcode. Co-authored-by: Cursor <cursoragent@cursor.com>
Harden CLI parsing, HTTP readiness checks, backend restarts across git checkouts, single login per capture, and diff directory creation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThis PR tightens the visual-diff workflow permissions and label matching, adds a Supabase status parsing utility and knip entry wiring, and hardens ChangesVisual Diff Workflow and Script Refinements
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant runPipeline
participant ensureBackendStack
participant stopBackendStack
participant captureScreenshots
participant Backend
runPipeline->>ensureBackendStack: ensureBackendStack(force)
ensureBackendStack->>Backend: isHttpReady(/ok, requireSuccess)
alt force or not ready
ensureBackendStack->>stopBackendStack: stopBackendStack()
stopBackendStack->>Backend: remove marker, terminate processes
ensureBackendStack->>Backend: restart backend
end
runPipeline->>captureScreenshots: captureScreenshots(forceFrontend, forceBackend)
captureScreenshots->>ensureBackendStack: ensureBackendStack(forceBackend)
captureScreenshots->>captureScreenshots: login once on first auth route
Compact metadata:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1c2b316. Configure here.
There was a problem hiding this comment.
Risk: medium. Cursor Bugbot skipped and reported 2 potential issues (empty diff false success, git restore dependency mismatch). Human review is needed before merge; assigned WcaleNieWolny and Dalanir.
Sent by Cursor Approval Agent: Pull Request Approver External
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/visual-diff.yml:
- Around line 17-20: The `pull-requests: write` permission is currently set at
the workflow-wide level in the top-level permissions section, granting it to all
jobs even though only the visual-diff job needs it. Remove `pull-requests:
write` from the top-level permissions block and keep only `contents: read` there
for read-only access. Then add a new `permissions` section under the
`jobs.visual-diff` job configuration to explicitly grant `pull-requests: write`
only to that job, reducing the token exposure surface by limiting write
permissions to only where needed.
In `@scripts/supabase-worktree-status.ts`:
- Around line 25-30: The spawnSync call in the getSupabaseStatus function lacks
a timeout option, which allows it to block indefinitely if the Bun or Supabase
command hangs during startup. Add a timeout property to the options object
passed to spawnSync to establish a maximum wait time for the command execution,
preventing the function from blocking the visual-diff and Playwright backend
startup processes.
In `@scripts/visual-diff.ts`:
- Around line 92-95: The requireArg function should reject option tokens (values
starting with -- or -) when validating flag arguments. Modify the validation
check in requireArg to not only ensure a value exists, but also verify that the
value does not start with a dash character, which would indicate it is another
flag rather than an actual argument value. If the value starts with a dash,
throw an error indicating that the flag requires a non-flag value.
- Around line 88-117: The thresholdPercent variable initialization on line 88
from the VISUAL_DIFF_THRESHOLD_PERCENT environment variable lacks range
validation, and the CLI argument validation around line 115-116 only checks if
the value is numeric but not if it falls within a valid range (should be between
0 and 100). Add validation after parsing the threshold value from the
environment variable to ensure it is within acceptable bounds, and enhance the
existing Number.isFinite check in the --threshold argument handler to also
validate that thresholdPercent is within the safe range of 0 to 100 before
accepting it.
- Around line 257-262: The waitForExit function uses a Promise chain with
.then() (specifically `sleep(timeoutMs).then(() => hasExited())`) which does not
align with the repository's async/await style requirement. Refactor the
Promise.race call to use async/await syntax instead of the .then() chain,
ensuring the function properly awaits the sleep operation and calls hasExited()
asynchronously without relying on Promise chaining methods.
- Around line 232-240: The current implementation pipes both child.stdout and
child.stderr to logStream without specifying the end option, which causes the
stream to close prematurely when the first source finishes writing. This results
in lost logs from the second stream. Additionally, logStream is never explicitly
closed, creating a resource leak. Fix this by passing { end: false } as the
second argument to both the child.stdout.pipe() and child.stderr.pipe() calls to
prevent automatic stream closure, then add a listener to the child process close
or exit event to explicitly call logStream.end() once the child process has
fully terminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7bf92ef-d3a5-43c3-be6c-a911ea957023
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/visual-diff.yml.gitignoreAGENTS.mdknip.jsonpackage.jsonplaywright/visual-diff.config.tsscripts/serve-backend-playwright.tsscripts/supabase-worktree-status.tsscripts/visual-diff.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Keep PR visual-diff workflow and script improvements while adopting main lockfile updates. Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2811213d-81ea-42e2-856c-b8ec3d435974) |
Merging this PR will degrade performance by 32.16%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | /updates manifest response with metadata |
114.7 µs | 169 µs | -32.16% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/visual-diff-pr-comments (7fd43d2) with main (b8f27fe)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Visual diff failedVisual diff did not produce a report. Check the workflow logs and artifacts. Commit: Open |
Harden CLI arg parsing and threshold validation, prevent premature log stream closure, fail when no screenshots are compared, restore deps after git checkout, scope workflow write permissions, and add Supabase status timeout. Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f5be1c02-7aed-462f-aa94-b4ebef683ae7) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/visual-diff.ts (1)
720-734: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
throwinsidefinallytrips Biome'snoUnsafeFinallyand is redundant.The
throwat line 728 is nested in a try that's immediately caught by the enclosingcatch(same block), so it never escapes thefinally— but Biome's lint (lint/correctness/noUnsafeFinally) still flags any throw lexically inside afinallyregardless of nested try/catch, per the static analysis hint. This will fail CI lint. Since the error is only ever logged via the very nextcatch, drop thethrow/catchround-trip and log directly.🩹 Proposed fix
try { const currentRef = git(['rev-parse', 'HEAD']) if (currentRef !== originalRef) { git(['checkout', '--force', originalRef]) const install = spawnSync('bun', ['install'], { cwd: repoRoot, stdio: 'inherit', env: process.env }) - if ((install.status ?? 1) !== 0) - throw new Error(`bun install failed after restoring git ref ${originalRef}`) + if ((install.status ?? 1) !== 0) + console.error(`[visual-diff] bun install failed after restoring git ref ${originalRef}`) } } catch (error) { console.error(`[visual-diff] failed to restore git ref ${originalRef}:`, error) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/visual-diff.ts` around lines 720 - 734, The cleanup in visual-diff’s finally block still contains a throw inside the finally scope, which triggers Biome’s noUnsafeFinally lint. In the restore/ref checkout logic around currentRef/originalRef, remove the throw/catch round-trip after bun install and handle the failure by logging it directly in the same branch, so no throw remains lexically inside finally.Source: Linters/SAST tools
knip.json (1)
44-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
playwright/visual-diff.config.ts!entry fromknip.json.
It appears twice in the sameentryarray; keep only one copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knip.json` around lines 44 - 50, The knip.json entry array contains a duplicate exclusion for playwright/visual-diff.config.ts!, so remove the extra copy and keep only one instance in the same list. Update the entry configuration near the repeated playwright/visual-diff.config.ts! symbol so the array has unique values only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/visual-diff.yml:
- Around line 46-48: Add a brief inline comment in the workflow permissions
block that explains why this least-privilege grant is needed, especially the
purpose of pull-requests: write. Locate the permissions section in
visual-diff.yml and document the scope so the intent of the GitHub Actions
permissions is clear to auditors. Keep the comment short and directly tied to
the workflow’s PR-related behavior.
---
Outside diff comments:
In `@knip.json`:
- Around line 44-50: The knip.json entry array contains a duplicate exclusion
for playwright/visual-diff.config.ts!, so remove the extra copy and keep only
one instance in the same list. Update the entry configuration near the repeated
playwright/visual-diff.config.ts! symbol so the array has unique values only.
In `@scripts/visual-diff.ts`:
- Around line 720-734: The cleanup in visual-diff’s finally block still contains
a throw inside the finally scope, which triggers Biome’s noUnsafeFinally lint.
In the restore/ref checkout logic around currentRef/originalRef, remove the
throw/catch round-trip after bun install and handle the failure by logging it
directly in the same branch, so no throw remains lexically inside finally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1803a559-1bef-4aa0-9a3d-4871625d07e3
📒 Files selected for processing (4)
.github/workflows/visual-diff.ymlknip.jsonscripts/supabase-worktree-status.tsscripts/visual-diff.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_20387296-67b1-4304-826a-e91e792e5990) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8bc2c2d1-34e4-4c02-80a3-c4cc9f80c6bd) |
Co-authored-by: Cursor <cursoragent@cursor.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_032deefe-2fe0-4ddf-9292-bbacfcbee099) |
|
1 participant
Summary (AI generated)
scripts/visual-diff.tsbased on CodeRabbit and Bugbot feedback from feat(frontend): add visual diff tooling for UI PR reviews #2576.--phase,--base,--head,--threshold,--routes).toJson().Motivation (AI generated)
PR #2576 introduced the visual diff system but review bots flagged correctness and robustness issues. This PR closes the remaining feedback without changing the overall workflow design.
Business Impact (AI generated)
More reliable visual diff captures and CI comments reduce false negatives/positives during UI PR review.
Test Plan (AI generated)
bun run lint:deadcodebun scripts/visual-diff.ts capturevalidates missing--phasebun run visual:capture:before/after/difflocally on a small UI tweakvisual-changelabel and confirm workflow posts the sticky commentGenerated with AI
Made with Cursor
Summary by CodeRabbit