Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a GitHub Actions CI workflow and refactors the CLI into a Playwright-driven audit engine split across new modules for CLI parsing, config, audits, evaluation, reporting, and utilities; updates README, bumps package version, and replaces top-level script to delegate to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (argv)
participant Main as src/main
participant Browser as Playwright Browser
participant Page as Page (DOM)
participant Audits as src/audits
participant FS as Filesystem
CLI->>Main: parseArgs(argv)
Main->>Browser: launch(context: desktop/mobile, trace?)
Browser->>Page: newPage / navigate(url, waitUntil)
Main->>Page: wait for readySelector (optional)
Main->>Audits: run audit sequence (overflow, tap targets, styles, axe, perf, crops)
Audits->>Page: evaluate DOM / take screenshots / capture crops / run axe
Audits-->>Main: return findings, screenshots, crops, metrics
Main->>FS: write JSON report, screenshots, crops, optional HTML/trace
Main->>CLI: print summary, set exit code based on budgets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
ui-review.js (3)
488-494: Redundant field:overflowCropsis duplicated.The
overflowCropsarray is included both insideoverflow.crops(line 488) and as a top-level field (line 494). The HTML generator usesoverflow.crops, so the top-leveloverflowCropsappears unused and could be removed.♻️ Proposed fix
overflow: { ...overflow, crops: overflowCrops }, tapTargets: { ...tapTargets, crops: tapCrops }, viewportMeta, styles: { ...styles }, axe, screenshots: shots, - overflowCrops, networkIssues, consoleIssues, trace: tracePath,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui-review.js` around lines 488 - 494, Remove the redundant top-level overflowCrops property from the returned object in ui-review.js: keep the crops only inside the overflow object (overflow: { ...overflow, crops: overflowCrops }) and delete the separate overflowCrops field so that overflow.crops is the single source of truth; locate the object literal that includes overflow, tapTargets, viewportMeta, styles, axe, screenshots and remove the trailing overflowCrops entry.
192-204: Minor:spacingOkfield is alwaysfalsein output.Since elements are only pushed to
tinywhen!spacedEnough(line 192), thespacingOk: spacedEnoughfield on line 202 will always befalse. This is harmless but redundant—consider removing it or documenting its purpose for when spacing policies evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui-review.js` around lines 192 - 204, The object pushed into the tiny array includes spacingOk: spacedEnough but that push only happens inside the if (!spacedEnough) branch, so spacingOk is always false and redundant; update the push in the tiny array (the block that constructs the object with selector/rect/size/text) to remove the spacingOk field entirely (or alternatively set spacingOk: false explicitly) and keep only relevant data, referencing the tiny.push call and the spacedEnough variable to locate the change.
537-617: Consider escaping user-provided content in HTML output.The
report.url,report.config.readySelector, and screenshot paths are inserted directly into HTML without escaping. While this is a local CLI tool with user-provided URLs, defense-in-depth suggests escaping special characters (<,>,&,") to prevent accidental HTML injection if the report is shared.♻️ Proposed helper
const escapeHtml = (str) => String(str) .replace(/&/g, '&') .replace(/</g, '<') .replace(/>/g, '>') .replace(/"/g, '"');Then use
escapeHtml(report.url)in the template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui-review.js` around lines 537 - 617, The HTML template in generateHtml directly interpolates user-controlled values (e.g., report.url, report.config.* fields, screenshot paths, budget labels, trace values) causing possible HTML injection; add an escapeHtml helper (as suggested) and use it wherever values are injected into the template and inside map callbacks (screenshots, overflow.crops, tapTargets.crops, budgets entries, trace, config fields) so all dynamic strings are escaped before being concatenated into the html string; update generateHtml to call escapeHtml(...) for each interpolated report property and array item..github/workflows/ci.yml (2)
35-52: Consider adding explicit permissions for provenance publishing.When using
npm publish --provenance, GitHub Actions needsid-token: writepermission to generate the OIDC token. While this may work with default permissions, explicitly declaring it is clearer and more robust.♻️ Proposed fix
publish: needs: test runs-on: ubuntu-latest if: startsWith(github.ref, 'refs/tags/v') + permissions: + contents: read + id-token: write steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 35 - 52, The publish job currently runs npm publish --provenance but does not declare the OIDC permission; to fix, add explicit permissions so GitHub Actions can mint an id-token by adding permissions: id-token: write (either at the workflow top-level or under the publish job) so the publish job (publish) and the Publish to npm step (run: npm publish --provenance) can generate the required token; ensure the permission block is added to the same job definition that contains the Publish to npm step.
45-48: Playwright browsers may be unnecessary in the publish job.The publish job installs Playwright Chromium but doesn't run any tests—only
npm publish. Unless Playwright is needed at publish time (e.g., for a prepublish script), consider removing lines 47-48 to speed up the release workflow.♻️ Proposed fix
- name: Install dependencies run: npm ci - - name: Install Playwright browsers (chromium only) - run: npx playwright install chromium --with-deps - name: Publish to npm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 45 - 48, The publish workflow currently runs the step named "Install Playwright browsers (chromium only)" which executes the command `npx playwright install chromium --with-deps` even though the job only performs `npm publish`; remove that Playwright install step from the publish job to speed up CI (or, if a prepublish script truly requires Playwright, move the `npx playwright install chromium --with-deps` step into the job that runs tests or guard it behind a condition so it only runs when needed).package.json (1)
11-13: Scripts are well-structured; note/tmppaths are Unix-specific.The
smokescript uses/tmp/uxray-ci.jsonand/tmp/uxray-shots, which work on Linux/macOS CI runners. If local Windows development is expected, consider using a cross-platform temp directory or documenting this limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 11 - 13, The smoke script in package.json hardcodes /tmp/uxray-ci.json and /tmp/uxray-shots which breaks on Windows; update the "smoke" npm script so it doesn't use hardcoded /tmp paths—either (a) compute the temp dir at runtime (use a small Node wrapper that uses os.tmpdir() and runs ui-review.js with those paths), or (b) use environment temp variables (respect $TMPDIR / %TEMP%) or a project-local ./tmp directory (and ignore it in git); change the "smoke" script entry (and any callers like "ci") to use the new approach and document the behavior in README if you choose environment-variable fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui-review.js`:
- Around line 277-279: The split regex for deriving the class snippet is wrong:
change the `.split(/\\s+/)` usage in the computation of `cls` (the `const cls =
...` line that reads `typeof el.className === 'string' && el.className.trim() ?
...`) to use a real whitespace-regex `.split(/\s+/)` so it splits on whitespace
rather than the literal `\s`; ensure you update only that regex token to match
the correct pattern consistent with the other occurrence.
- Around line 326-361: captureCrops is using viewport-relative rects (from
getBoundingClientRect) as if they were document-relative when calling
page.screenshot({ clip }), and because captureCrops runs after scrolling the
page those rects are stale; fix by querying the current page scroll offset
inside captureCrops (e.g. await page.evaluate(() => ({ x: window.scrollX, y:
window.scrollY }))) and add those offsets to rect.x and rect.y before computing
clip (adjust the clip.x/clip.y calculations), then keep the existing bounds
checks with docSize and call page.screenshot({ clip }) with the corrected
coordinates so clip is document-relative; update references to rect, clip,
docSize and the page.screenshot call accordingly.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 35-52: The publish job currently runs npm publish --provenance but
does not declare the OIDC permission; to fix, add explicit permissions so GitHub
Actions can mint an id-token by adding permissions: id-token: write (either at
the workflow top-level or under the publish job) so the publish job (publish)
and the Publish to npm step (run: npm publish --provenance) can generate the
required token; ensure the permission block is added to the same job definition
that contains the Publish to npm step.
- Around line 45-48: The publish workflow currently runs the step named "Install
Playwright browsers (chromium only)" which executes the command `npx playwright
install chromium --with-deps` even though the job only performs `npm publish`;
remove that Playwright install step from the publish job to speed up CI (or, if
a prepublish script truly requires Playwright, move the `npx playwright install
chromium --with-deps` step into the job that runs tests or guard it behind a
condition so it only runs when needed).
In `@package.json`:
- Around line 11-13: The smoke script in package.json hardcodes
/tmp/uxray-ci.json and /tmp/uxray-shots which breaks on Windows; update the
"smoke" npm script so it doesn't use hardcoded /tmp paths—either (a) compute the
temp dir at runtime (use a small Node wrapper that uses os.tmpdir() and runs
ui-review.js with those paths), or (b) use environment temp variables (respect
$TMPDIR / %TEMP%) or a project-local ./tmp directory (and ignore it in git);
change the "smoke" script entry (and any callers like "ci") to use the new
approach and document the behavior in README if you choose environment-variable
fallback.
In `@ui-review.js`:
- Around line 488-494: Remove the redundant top-level overflowCrops property
from the returned object in ui-review.js: keep the crops only inside the
overflow object (overflow: { ...overflow, crops: overflowCrops }) and delete the
separate overflowCrops field so that overflow.crops is the single source of
truth; locate the object literal that includes overflow, tapTargets,
viewportMeta, styles, axe, screenshots and remove the trailing overflowCrops
entry.
- Around line 192-204: The object pushed into the tiny array includes spacingOk:
spacedEnough but that push only happens inside the if (!spacedEnough) branch, so
spacingOk is always false and redundant; update the push in the tiny array (the
block that constructs the object with selector/rect/size/text) to remove the
spacingOk field entirely (or alternatively set spacingOk: false explicitly) and
keep only relevant data, referencing the tiny.push call and the spacedEnough
variable to locate the change.
- Around line 537-617: The HTML template in generateHtml directly interpolates
user-controlled values (e.g., report.url, report.config.* fields, screenshot
paths, budget labels, trace values) causing possible HTML injection; add an
escapeHtml helper (as suggested) and use it wherever values are injected into
the template and inside map callbacks (screenshots, overflow.crops,
tapTargets.crops, budgets entries, trace, config fields) so all dynamic strings
are escaped before being concatenated into the html string; update generateHtml
to call escapeHtml(...) for each interpolated report property and array item.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e251f2e-9beb-4ea5-8b4b-78437008db9e
📒 Files selected for processing (4)
.github/workflows/ci.ymlREADME.mdpackage.jsonui-review.js
Comment on lines
+326
to
+361
| async function captureCrops(page, items, dir, prefix, limit = 8) { | ||
| if (!items || !items.length) return []; | ||
| ensureDir(dir); | ||
| const docSize = await page.evaluate(() => ({ | ||
| width: document.documentElement.scrollWidth, | ||
| height: document.documentElement.scrollHeight, | ||
| })); | ||
|
|
||
| const crops = []; | ||
| const targets = items.slice(0, limit); | ||
| for (let i = 0; i < targets.length; i += 1) { | ||
| const rect = targets[i].rect; | ||
| if (!rect || !rect.width || !rect.height) continue; | ||
| const clip = { | ||
| x: Math.max(0, rect.x - 4), | ||
| y: Math.max(0, rect.y - 4), | ||
| width: rect.width + 8, | ||
| height: rect.height + 8, | ||
| }; | ||
| const maxWidth = Math.max(0, docSize.width - clip.x); | ||
| const maxHeight = Math.max(0, docSize.height - clip.y); | ||
| clip.width = Math.min(clip.width, maxWidth); | ||
| clip.height = Math.min(clip.height, maxHeight); | ||
| if (clip.width <= 1 || clip.height <= 1) continue; | ||
| const cropPath = path.join(dir, `${prefix}-${i + 1}.png`); | ||
| try { | ||
| await page.screenshot({ path: cropPath, clip }); | ||
| crops.push(path.relative(process.cwd(), cropPath)); | ||
| } catch (err) { | ||
| // Skip invalid clip regions (outside viewport) to keep the run resilient. | ||
| // eslint-disable-next-line no-console | ||
| console.warn(`crop failed for ${prefix} index ${i}: ${err.message}`); | ||
| } | ||
| } | ||
| return crops; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the element coordinates are viewport-relative in detector functions
# and whether scroll position affects capture accuracy.
# Look for getBoundingClientRect usage in detector functions
rg -n 'getBoundingClientRect' ui-review.js -A2 -B2
# Check if there's any scroll position adjustment before screenshot
rg -n 'scrollIntoView|scrollTo|scrollY' ui-review.jsRepository: Agent-Hellboy/UXRay
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Find all calls to captureCrops
rg -n 'captureCrops' ui-review.js -B 5 -A 5
# Get broader context around the scroll position tracking
sed -n '300,330p' ui-review.js
# Check if there's coordinate adjustment happening in the detector functions
sed -n '100,180p' ui-review.jsRepository: Agent-Hellboy/UXRay
Length of output: 5184
Viewport-relative coordinates used as document-relative in screenshot clipping.
The rect values passed to captureCrops come from detectOverflow and detectTapTargets, which use getBoundingClientRect() — viewport-relative coordinates. However, page.screenshot({ clip }) expects document-relative coordinates. Additionally, captureCrops is called after captureScrollShots scrolls the page, so the rects captured at the initial viewport are no longer valid. This causes incorrect crop regions.
Adjust rect coordinates by adding the current scroll position before clipping, or capture rects again at screenshot time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui-review.js` around lines 326 - 361, captureCrops is using viewport-relative
rects (from getBoundingClientRect) as if they were document-relative when
calling page.screenshot({ clip }), and because captureCrops runs after scrolling
the page those rects are stale; fix by querying the current page scroll offset
inside captureCrops (e.g. await page.evaluate(() => ({ x: window.scrollX, y:
window.scrollY }))) and add those offsets to rect.x and rect.y before computing
clip (adjust the clip.x/clip.y calculations), then keep the existing bounds
checks with docSize and call page.screenshot({ clip }) with the corrected
coordinates so clip is document-relative; update references to rect, clip,
docSize and the page.screenshot call accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/audits.js`:
- Around line 149-156: The code currently drops any bgHex equal to '#000000'
which incorrectly excludes opaque black because toHex() strips alpha; instead
inspect the original style.backgroundColor to detect true transparency and only
skip fully transparent backgrounds. In elements.forEach where bgHex is computed
from toHex(style.backgroundColor), parse style.backgroundColor (e.g., check for
'rgba(...)' and parse the alpha value) and set a boolean like isTransparent =
(alpha === 0) (or treat 'transparent' explicitly); then use if (bgHex &&
!isTransparent) colorCounts.set(bgHex, (colorCounts.get(bgHex) || 0) + 1) so
opaque black backgrounds are counted while fully transparent ones are ignored.
In `@src/cli.js`:
- Around line 24-86: The CLI argument parsing loop in src/cli.js stores numeric
flags without validation (e.g., args.wait, args.width/height, args.steps,
args.budgetA11y, budgetTap, budgetOverflow, budgetConsole, budgetHttpErrors), so
sanitize and validate each parsed Number: parse with Number.parseInt (or Number)
into a temp var, check Number.isFinite and enforce sensible minima (e.g., wait
>= 0, width/height > 0, steps >= 1, budget values >= 0), then either clamp to
the minimum or reject by printing an error and exiting; update the branches that
set args.wait, args.width/height, args.steps and all args.budget* to perform
this validation before assigning to args so downstream callers (like
page.waitForTimeout and context creation in src/main.js) receive safe values.
In `@src/main.js`:
- Around line 38-140: Wrap the Playwright lifecycle in a try/finally: move the
main sequence (creating context, page navigation, audits, screenshots, trace
start, all awaits like page.goto, detectOverflow, captureScrollShots,
captureCrops, etc.) into a try block and put cleanup in finally; in the finally
ensure you stop tracing if trace is true and context exists (call await
context.tracing.stop({ path: tracePath }) only when tracing was started and
handle missing path), close the context/browser if they exist (await
context.close() or await browser.close()), and preserve/propagate any thrown
errors after cleanup so traces and browser are always cleaned up even on failure
(refer to symbols: browser, context, context.tracing.start/stop, tracePath,
await context.newPage(), and browser.close()).
In `@src/report.js`:
- Around line 41-117: The generateHtml function interpolates unescaped
user-controlled values (e.g., report.url, report.runAt, report.config.* fields,
report.budgets entries, screenshot/overflow/tapTargets paths, trace) into the
HTML template and must escape them before insertion; add a small helper like
htmlEscape(str) that replaces &, <, >, ", ' and ` with HTML entities and apply
it everywhere dynamic values are injected (wrap report.url, report.runAt,
report.config.viewport/mobile/waitUntil, each budgets entry label/value/limit,
and every item in arrays like report.desktop.screenshots,
report.desktop.overflow.crops, report.desktop.tapTargets.crops, report.mobile.*
equivalents, and trace fields) and use the escaped values in the template and
array maps so no raw user content is output unescaped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15746a5d-d839-4362-b9f9-80bc8c82094e
📒 Files selected for processing (8)
src/audits.jssrc/cli.jssrc/config.jssrc/evaluation.jssrc/main.jssrc/report.jssrc/utils.jsui-review.js
✅ Files skipped from review due to trivial changes (2)
- src/utils.js
- src/config.js
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/audits.js (1)
149-156:⚠️ Potential issue | 🟡 MinorOpaque black backgrounds are still excluded from
colorCounts.
toHex()removes alpha, sorgb(0,0,0)andrgba(0,0,0,0)both collapse to#000000. Line 156 therefore drops real black backgrounds and skewstopColors/colorCount.Suggested fix
const style = getComputedStyle(el); const fgHex = toHex(style.color); const bgHex = toHex(style.backgroundColor); + const rawBg = parseRgb(style.backgroundColor); const font = style.fontFamily.split(',')[0].replace(/['"]/g, '').trim(); if (fgHex) colorCounts.set(fgHex, (colorCounts.get(fgHex) || 0) + 1); - if (bgHex && bgHex !== '#000000' && bgHex !== '#00000000') colorCounts.set(bgHex, (colorCounts.get(bgHex) || 0) + 1); + if (bgHex && (!rawBg || rawBg.a !== 0)) colorCounts.set(bgHex, (colorCounts.get(bgHex) || 0) + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/audits.js` around lines 149 - 156, The code currently excludes any background that converts to "#000000" because toHex drops alpha, which causes opaque black ("rgb(0,0,0)") to be incorrectly skipped; in the elements.forEach block use the original style.backgroundColor (or parse it before converting) to detect fully transparent backgrounds (e.g. "transparent" or "rgba(...,0)") and only skip when alpha === 0, then call toHex and increment colorCounts for true opaque blacks; update the conditional around colorCounts.set for bgHex to check parsedAlpha/transparent instead of raw bgHex === '#000000' so real black backgrounds are counted while transparent ones remain excluded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/audits.js`:
- Around line 371-379: The missing-label audit currently only skips hidden
inputs and thereby flags button-like controls; update the filter in the inputs
loop (the block working with inputs, inputsMissingLabels and format(el)) to also
return early for input types that don't require labels by checking el.type
against ['submit','button','reset','image'] (and optionally skip elements with
role="button"), e.g. replace the single hidden check with a membership test so
submit/button/reset/image inputs are excluded from being pushed into
inputsMissingLabels.
- Around line 386-389: The submit-control check is wrong: it treats
[role="button"] as a submit control and misses implicit submit buttons like
<button> which have no type attribute; update the selector in the forms.forEach
block so it looks for actual submit controls by including implicit buttons and
explicit submit inputs but removing [role="button"]. For example, change the
querySelector call used when building hasSubmit to include
button[type="submit"], button:not([type]) and input[type="submit"] (and keep
using formsWithoutSubmit and format(form) as is) so real <button>Save</button>
is recognized and elements with role="button" do not suppress the warning.
- Around line 452-467: The current focusables list and loop count invisible or
non-rendered elements (display:none, visibility:hidden, zero-size) which causes
false missing focus indicator reports; update the selection/filter logic around
focusables (and the loop that uses el.focus(), visibleCount, and missing) to
exclude non-rendered elements before scoring: filter out elements where
getComputedStyle(el).display === 'none' || getComputedStyle(el).visibility ===
'hidden' || Number.parseFloat(getComputedStyle(el).opacity) === 0, and elements
with no layout boxes (el.getClientRects().length === 0 or el.offsetWidth === 0
&& el.offsetHeight === 0) or not isConnected; keep the existing disabled
exclusion and retain format(el) usage for reporting, so only rendered, connected
elements are focused and counted when updating visibleCount and missing.
- Around line 223-250: The crop rectangles used by captureCrops are
viewport-relative (from getBoundingClientRect) but captureScrollShots scrolls
the page and doesn't restore scroll, so captureCrops receives shifted rects; fix
by either resetting the page scroll to the top before taking crops (e.g., call
page.evaluate(() => window.scrollTo(0,0)) at the start of captureCrops) or
convert each stored rect to document-relative coordinates inside captureCrops by
adding the current scroll offsets (get them via page.evaluate(() => ({x:
window.scrollX, y: window.scrollY})) and add to rect.x/rect.y) before building
the clip and calling page.screenshot; update captureCrops (and ensure any
callers like captureScrollShots won't rely on preserved scroll) accordingly.
---
Duplicate comments:
In `@src/audits.js`:
- Around line 149-156: The code currently excludes any background that converts
to "#000000" because toHex drops alpha, which causes opaque black ("rgb(0,0,0)")
to be incorrectly skipped; in the elements.forEach block use the original
style.backgroundColor (or parse it before converting) to detect fully
transparent backgrounds (e.g. "transparent" or "rgba(...,0)") and only skip when
alpha === 0, then call toHex and increment colorCounts for true opaque blacks;
update the conditional around colorCounts.set for bgHex to check
parsedAlpha/transparent instead of raw bgHex === '#000000' so real black
backgrounds are counted while transparent ones remain excluded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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 (1)
README.md (1)
116-116:⚠️ Potential issue | 🟡 MinorUpdate reference to reflect the new file structure.
The reference to
ui-review.jsappears outdated. According to the PR summary, the top-level script now delegates tosrc/main, so this should reference the current entry point or launcher configuration.📝 Suggested update
-Tool runs headless; remove `headless: true` in `ui-review.js` if you want to watch it. +Tool runs headless; remove `headless: true` in the Playwright launch config (`src/main.js`) if you want to watch it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 116, Update the outdated README reference to ui-review.js: change the instruction that mentions removing headless: true in ui-review.js to point to the current launcher (the top-level script that delegates to src/main or the entrypoint function in src/main); locate README text referencing "ui-review.js" and replace it with the correct entrypoint name (e.g., the top-level script or src/main) and, if applicable, mention where to toggle headless behavior in the new config or launcher code instead of ui-review.js.
🧹 Nitpick comments (1)
scripts/smoke.js (1)
11-11: Replace hardcoded external URL with deterministic local target.Line 11 uses
https://example.com, creating an external network dependency that can cause flaky CI failures. Use a data: URL by default with an env override for flexibility.Proposed refactor
+const smokeUrl = process.env.UXRAY_SMOKE_URL + || `data:text/html;charset=utf-8,${encodeURIComponent('<!doctype html><html><body><main id="app">uxray smoke</main></body></html>')}`; + const args = [ path.join(__dirname, '..', 'ui-review.js'), - '--url', 'https://example.com', + '--url', smokeUrl, '--steps', '1',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke.js` at line 11, Replace the hardcoded external target '--url', 'https://example.com' in scripts/smoke.js with a deterministic local default (e.g., a data: URL) and allow override via an environment variable; modify the place where the CLI args array contains the '--url' entry so it uses process.env.SMOKE_URL || 'data:text/html,<html><body>ok</body></html>' (or similar) instead of the fixed https://example.com, ensuring the script falls back to the env value when provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/smoke.js`:
- Around line 20-21: The current call to spawnSync('node', args, { stdio:
'inherit' }) should be hardened for CI by adding a timeout (e.g., timeout: <ms>)
and explicit spawn error handling: pass a timeout option to spawnSync, check
result.error after the call and log the error to stderr (or
process.stderr.write) with context including result.error.code/message, and then
exit with a non-zero status; retain the existing exit behavior for result.status
when there is no error but ensure that a timeout or spawn failure causes a clear
error log and non-zero exit. Use the existing symbols spawnSync, args, result,
and process.exit when applying the change.
---
Outside diff comments:
In `@README.md`:
- Line 116: Update the outdated README reference to ui-review.js: change the
instruction that mentions removing headless: true in ui-review.js to point to
the current launcher (the top-level script that delegates to src/main or the
entrypoint function in src/main); locate README text referencing "ui-review.js"
and replace it with the correct entrypoint name (e.g., the top-level script or
src/main) and, if applicable, mention where to toggle headless behavior in the
new config or launcher code instead of ui-review.js.
---
Nitpick comments:
In `@scripts/smoke.js`:
- Line 11: Replace the hardcoded external target '--url', 'https://example.com'
in scripts/smoke.js with a deterministic local default (e.g., a data: URL) and
allow override via an environment variable; modify the place where the CLI args
array contains the '--url' entry so it uses process.env.SMOKE_URL ||
'data:text/html,<html><body>ok</body></html>' (or similar) instead of the fixed
https://example.com, ensuring the script falls back to the env value when
provided.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95a7f3d3-db8a-448b-b452-771ea9b083d6
📒 Files selected for processing (8)
.github/workflows/ci.ymlREADME.mdpackage.jsonscripts/smoke.jssrc/audits.jssrc/cli.jssrc/main.jssrc/report.js
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/report.js
- src/main.js
- src/cli.js
Comment on lines
+20
to
+21
| const result = spawnSync('node', args, { stdio: 'inherit' }); | ||
| process.exit(result.status === null ? 1 : result.status); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout/error-handling are currently absent/present in the smoke runner.
rg -n -C3 "spawnSync\\(|timeout|killSignal|result\\.error|process\\.exit\\(" scripts/smoke.jsRepository: Agent-Hellboy/UXRay
Length of output: 224
🏁 Script executed:
cat -n scripts/smoke.jsRepository: Agent-Hellboy/UXRay
Length of output: 808
Add timeout + explicit spawn error handling for CI reliability.
Line 20 runs a blocking child process without timeout. If the test or underlying Playwright process hangs, this can stall the CI job until the global workflow timeout. Additionally, line 21 does not check result.error, which means spawn failures (e.g., ENOENT, permission issues) go undiagnosed in logs.
Proposed fix
-const result = spawnSync('node', args, { stdio: 'inherit' });
-process.exit(result.status === null ? 1 : result.status);
+const result = spawnSync(process.execPath, args, {
+ stdio: 'inherit',
+ timeout: 180000,
+ killSignal: 'SIGKILL',
+});
+
+if (result.error) {
+ console.error('Smoke run failed to start:', result.error.message);
+ process.exit(1);
+}
+
+process.exit(typeof result.status === 'number' ? result.status : 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = spawnSync('node', args, { stdio: 'inherit' }); | |
| process.exit(result.status === null ? 1 : result.status); | |
| const result = spawnSync(process.execPath, args, { | |
| stdio: 'inherit', | |
| timeout: 180000, | |
| killSignal: 'SIGKILL', | |
| }); | |
| if (result.error) { | |
| console.error('Smoke run failed to start:', result.error.message); | |
| process.exit(1); | |
| } | |
| process.exit(typeof result.status === 'number' ? result.status : 1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/smoke.js` around lines 20 - 21, The current call to spawnSync('node',
args, { stdio: 'inherit' }) should be hardened for CI by adding a timeout (e.g.,
timeout: <ms>) and explicit spawn error handling: pass a timeout option to
spawnSync, check result.error after the call and log the error to stderr (or
process.stderr.write) with context including result.error.code/message, and then
exit with a non-zero status; retain the existing exit behavior for result.status
when there is no error but ensure that a timeout or spawn failure causes a clear
error log and non-zero exit. Use the existing symbols spawnSync, args, result,
and process.exit when applying the change.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Chores
CI