Publish acceptance-tested screenshots for the website - #1924
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a specification-evidence pipeline that selects tagged Cucumber cases, captures deterministic screenshots, builds validated manifests, uploads evidence artifacts in CI, and refactors shared screenshot and checksum utilities. ChangesSpecification evidence pipeline
Shared checksum utility
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CucumberAfterHook
participant captureScenarioEvidence
participant captureCurrentScenarioEvidence
participant Chromium
participant EvidenceWorld
CucumberAfterHook->>captureScenarioEvidence: run in evidence mode
captureScenarioEvidence->>captureCurrentScenarioEvidence: load and execute capture
captureCurrentScenarioEvidence->>Chromium: navigate and capture prepared page
Chromium-->>captureCurrentScenarioEvidence: PNG bytes
captureCurrentScenarioEvidence->>EvidenceWorld: attach PNG evidence
CucumberAfterHook-->>CucumberAfterHook: clean up scenario resources
sequenceDiagram
participant runEvidenceSpecs
participant runSpecs
participant CucumberMessages
participant buildEvidenceBundle
participant EvidenceOutput
runEvidenceSpecs->>runSpecs: execute selected evidence cases
runSpecs-->>runEvidenceSpecs: catalog and Cucumber messages
runEvidenceSpecs->>buildEvidenceBundle: build validated bundle
buildEvidenceBundle->>CucumberMessages: resolve attachments and scenarios
buildEvidenceBundle->>EvidenceOutput: write manifest.json and assets
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/screenshots.ts (1)
196-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated readiness/capture/write pipeline.
capture()(Lines 209-213) andcaptureScenario()(Lines 265-272) both now implement the identicalwaitForScreenshotPage→capturePreparedPage→Deno.writeFilesequence, differing only in theelementSelector/fullPageargs and the logging/social-variant follow-up. Worth extracting a small shared helper (e.g.captureAndWrite(page, outputPath, elementSelector?, fullPage?)) to keep the two capture paths from silently diverging as this pipeline evolves.♻️ Suggested helper extraction
+const captureAndWrite = async ( + page: Page, + outputPath: string, + elementSelector?: string, + fullPage?: boolean, +): Promise<Rgb> => { + await waitForScreenshotPage(page); + const screenshot = await capturePreparedPage(page, elementSelector, fullPage); + await Deno.writeFile(outputPath, screenshot.png); + return screenshot.background; +};Then both
capture()andcaptureScenario()callcaptureAndWrite(...)instead of repeating the three-line sequence.Also applies to: 256-282
🤖 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/screenshots.ts` around lines 196 - 213, Extract the shared wait, screenshot capture, and file-writing pipeline from capture and captureScenario into a helper such as captureAndWrite, accepting page, outputPath, elementSelector, and fullPage as needed. Replace the duplicated sequences in both functions with calls to this helper while preserving each function’s existing arguments, return values, logging, and follow-up behavior.
🤖 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/spec-evidence.yml:
- Around line 23-24: Pin every third-party action in
.github/workflows/spec-evidence.yml to its reviewed immutable full commit SHA:
update actions/checkout at lines 23-24, actions/cache at lines 29-30 and 46-48,
and actions/upload-artifact at lines 64-65, replacing mutable version tags while
preserving the existing workflow behavior.
In `@scripts/screenshots/capture.ts`:
- Line 1: Add mock-based unit tests covering the screenshot branches in capture,
app-server, and browser: verify crop behavior, startup timeout and build-failure
handling, and environment-variable plus Nix fallback behavior. Mock Playwright,
subprocess/server startup, and relevant environment or filesystem dependencies
so these tests exercise the branches directly without launching real browsers or
processes.
In `@scripts/specs/evidence/capture.ts`:
- Line 24: Review the sequential capture flow in captureCurrentScenarioEvidence
and its After-hook timeout configuration in hook.ts, ensuring the total
declaration×profile capture budget remains within EVIDENCE_HOOK_TIMEOUT_MS as
EVIDENCE_CAPTURES grows. Adjust the per-capture timeout or execution strategy as
needed while preserving each page operation’s timeout guarantee.
- Around line 109-116: Remove the waitUntil: "networkidle" option from the
page.goto call in the evidence navigation flow, allowing the existing
waitForScreenshotPage(page) readiness check to control synchronization while
preserving the navigation URL and blocked-request assertion.
- Around line 149-153: Extract the duplicated Chromium screenshot launch setup
from the screenshot entry points into a shared helper, reusing
browserLaunchOptions, chromiumExecutable, and the CDPScreenshotNewSurface
disable flag. Update the launch flow in capture.ts and the corresponding flow in
scripts/screenshots.ts to call the helper so both remain aligned.
In `@scripts/specs/evidence/declarations.ts`:
- Around line 8-21: Remove the unused CSS custom properties from the :root
declaration in SERVICING_STUDIO_CSS: --border-radius, --color-bg,
--color-bg-secondary, --color-link, --color-secondary-accent, --color-table,
--color-text, --color-text-secondary, and --font-family. Keep --color-secondary,
--color-shadow, and --color-accent unchanged because they are consumed by
var(...) references.
---
Outside diff comments:
In `@scripts/screenshots.ts`:
- Around line 196-213: Extract the shared wait, screenshot capture, and
file-writing pipeline from capture and captureScenario into a helper such as
captureAndWrite, accepting page, outputPath, elementSelector, and fullPage as
needed. Replace the duplicated sequences in both functions with calls to this
helper while preserving each function’s existing arguments, return values,
logging, and follow-up behavior.
🪄 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 Plus
Run ID: 7ef0850f-e49d-4c15-b77f-84a7fe86b3c1
📒 Files selected for processing (40)
.github/workflows/spec-evidence.ymlAGENTS.mdREADME.mddeno.jsonscripts/checksum.tsscripts/run-spec-evidence.tsscripts/screenshots.tsscripts/screenshots/app-server.tsscripts/screenshots/browser.tsscripts/screenshots/capture.tsscripts/screenshots/image.tsscripts/screenshots/profile.tsscripts/screenshots/readiness.tsscripts/specs/evidence/browser.tsscripts/specs/evidence/capture.tsscripts/specs/evidence/declarations.tsscripts/specs/evidence/hook.tsscripts/specs/evidence/manifest.tsscripts/specs/evidence/resolve.tsscripts/specs/evidence/run.tsscripts/specs/evidence/schema.tsscripts/specs/evidence/style.tsscripts/specs/run.tsscripts/specs/selection.tsscripts/specs/types.tsscripts/static-cdn.tsspecs/servicing/hold-and-cost.featuretest/scripts/checksum.test.tstest/scripts/screenshots-capture.test.tstest/scripts/specs/evidence-capture.test.tstest/scripts/specs/evidence-fixture.tstest/scripts/specs/evidence-hook.test.tstest/scripts/specs/evidence-manifest.test.tstest/scripts/specs/evidence-resolve.test.tstest/scripts/specs/evidence-schema.test.tstest/scripts/specs/evidence-style.test.tstest/scripts/specs/run-execution.test.tstest/specs/steps/servicing-hold.tstest/specs/support/hooks.tstest/specs/support/world.ts
|
Re: outside-diff comment on Extracted |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/scripts/screenshots-capture.test.ts`:
- Around line 134-178: Split the combined chromiumExecutable test into four
independent tests covering the environment override, Nix-path fallback, absent
executable, and filesystem-error propagation. Move each case’s Deno.env and
Deno.stat stubs into its own test with isolated setup and cleanup, while
preserving the existing expectations and behavior.
🪄 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 Plus
Run ID: 50b924f4-f0ae-4522-b42d-0c9ca4e7003b
📒 Files selected for processing (6)
.github/workflows/spec-evidence.ymlscripts/browser-options.tsscripts/screenshots.tsscripts/specs/evidence/capture.tsscripts/specs/evidence/declarations.tstest/scripts/screenshots-capture.test.ts
💤 Files with no reviewable changes (1)
- scripts/specs/evidence/declarations.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398302f6d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42c22b3a0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex's second review of PR #1924 left two unresolved threads (both on commit 42c22b3): P1 — capturePreparedPage had no direct unit coverage. Add direct tests for its whole decision tree (no-element path, fullPage passthrough, viewport-resize math for tall and short elements, viewport-null and box-null error cases, viewport restoration in finally when the screenshot throws) plus direct coverage of readBodyBackground's inner evaluate fn (both branches: valid style and missing style). The mock page invokes the inner fns Playwright would normally run in the browser, so the lines they hold are exercised in-process too and the coverage gate stays green. P2 — serveHandler in src/serve-app.ts runs its memoized initialize() the first time a request hits the capture's loopback server, which flips the N+1 guard to notify-only. specs:evidence runs scenarios serially (parallel: 0), so a flipped guard would leak into the next scenario and silence its N+1 checks. captureScenarioEvidence's cleanup now restores the default throw mode, with regression tests that set the guard to notify-only, run a capture (success and failure), and verify the next N+1 violation throws via the shared reportGuardViolation path. The regression tests fail without the reset, confirming they catch the leak they claim to.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a1b4396fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b9122663c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5357447dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
stefan-burke
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/specs/evidence/schema.ts`:
- Around line 139-146: Remove the duplicated repository literal by defining or
retaining a single exported REPOSITORY constant in schema.ts, using it in
EvidenceManifestSchema, and updating manifest.ts to import and reuse that
constant instead of defining its own.
In `@test/scripts/specs/evidence-server.test.ts`:
- Around line 6-21: Extend the lifecycle test around defineLoopbackServer and
server.close() to verify that the closed server no longer accepts connections
after close resolves. Keep the existing request and cleanup assertions, and make
the post-close connection attempt assert the expected rejection or unavailable
state.
🪄 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 Plus
Run ID: 43af015e-62f7-4326-b53e-dca867bbaeec
📒 Files selected for processing (34)
.github/workflows/spec-evidence.ymlAGENTS.mdREADME.mddeno.jsonscripts/browser-options.tsscripts/screenshots.tsscripts/screenshots/capture.tsscripts/screenshots/readiness.tsscripts/specs/evidence/capture-flow.tsscripts/specs/evidence/capture.tsscripts/specs/evidence/declarations.tsscripts/specs/evidence/execute.tsscripts/specs/evidence/git.tsscripts/specs/evidence/manifest.tsscripts/specs/evidence/run.tsscripts/specs/evidence/schema.tsscripts/specs/evidence/server.tsscripts/specs/run.tsscripts/specs/types.tstest/scripts/browser-options.test.tstest/scripts/screenshots-browser.contract.tstest/scripts/screenshots-browser.test.tstest/scripts/screenshots-capture.test.tstest/scripts/screenshots-fixture.tstest/scripts/screenshots-image.test.tstest/scripts/screenshots-profile.test.tstest/scripts/screenshots-readiness.test.tstest/scripts/specs/evidence-capture.test.tstest/scripts/specs/evidence-execute.test.tstest/scripts/specs/evidence-git.test.tstest/scripts/specs/evidence-manifest.test.tstest/scripts/specs/evidence-schema.test.tstest/scripts/specs/evidence-server.test.tstest/scripts/specs/evidence-style.test.ts
1 participant
What changed
deno task specs:evidenceto run declared Cucumber stories one at a time and capture the real rendered page before its test data is removed.main, refreshes it monthly, and keeps each artifact for 90 days.The website consumer and first imported servicing screenshot are live in
chobbledotcom/tickets-sitecommitsdf6af7band1cf1f77.Why
Website screenshots can now be tied to successful acceptance stories instead of separate setup scripts that can drift from tested behaviour. The website still controls captions, placement, and social images, and its normal build stays offline.
Verification
maincommit865202feand resolved the combined type-check task without dropping either command.deno task precommitpassed after the merge, including type checks, duplication checks, builds, all tests, and 100% coverage.deno task test:screenshot-contractpassed both real-browser contracts.deno task specs:evidencepassed: 1 scenario and 6 steps.deno task precommit:mutationfound no changedsrcfiles.Setup after merge
Add a fine-grained
TICKETS_EVIDENCE_TOKENsecret to the website repository with Actions read access tochobbledotcom/tickets. GitHub limits its built-in workflow token to the website repository, so it cannot download the app artifact.Summary by CodeRabbit
deno task specs:evidencetask to run evidence-declared specs and generate versioned evidence outputs.reports/evidence/manifest.json+ assets contract.