Improve checkout screenshot fixtures - #1868
Conversation
# Conflicts: # scripts/screenshots.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughScreenshot automation now centralizes temporary database and Stripe setup in ChangesScreenshot automation
Quantity selector styling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScreenshotRunner
participant StripeMock
participant AppServer
ScreenshotRunner->>StripeMock: Start mock
ScreenshotRunner->>AppServer: Start with database and Stripe environment
AppServer->>StripeMock: Activate mock Stripe settings
ScreenshotRunner->>StripeMock: Stop mock on failed startup
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 409cb5372e
ℹ️ 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.
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)
123-139: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the Stripe mock when startup throws.
startStripeMock()acquires a process before temporary-directory creation, child spawning, and health polling. If any of those operations throws, the existing cleanup paths are bypassed and the mock process remains running. Wrap pre-return startup in cleanup handling and add a regression test for startup failure.🤖 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 123 - 139, Wrap the startup sequence in scripts/screenshots.ts, beginning with startStripeMock() and covering temporary-directory creation, child spawning, and health polling, in cleanup handling that stops the Stripe mock whenever startup throws. Preserve the existing cleanup behavior for successful startup and add a regression test that simulates startup failure and verifies the mock process is terminated.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@scripts/screenshots.ts`:
- Around line 123-139: Wrap the startup sequence in scripts/screenshots.ts,
beginning with startStripeMock() and covering temporary-directory creation,
child spawning, and health polling, in cleanup handling that stops the Stripe
mock whenever startup throws. Preserve the existing cleanup behavior for
successful startup and add a regression test that simulates startup failure and
verifies the mock process is terminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9d7d113-efab-402f-86ef-41654aebb2b2
📒 Files selected for processing (1)
scripts/screenshots.ts
|
Addressed CodeRabbit’s startup-cleanup review in f3cf318. Screenshot startup now runs behind |
There was a problem hiding this comment.
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)
123-136: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake startup and shutdown cleanup failure-safe.
Once the temporary directory and child process are acquired, exceptions from process startup or health checking can bypass the cleanup branch; the wrapper at Line 183 through Line 186 only stops Stripe, so app resources can leak. Also, if
removeTreeat Line 167 rejects,stripeMock.stop()at Line 168 is skipped. Compose all cleanup tasks with nestedfinally/failAfterCleanupswhile preserving the original error.Also applies to: 162-186
🤖 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 123 - 136, The startAppServer lifecycle must clean up the temporary directory and child process when startup or health checks fail, and must still stop Stripe if removeTree fails. Update the cleanup flow around startAppServer and its wrapper to use nested finally blocks or failAfterCleanups, ensuring every acquired resource is released while the original startup or shutdown error is preserved.
🤖 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.
Outside diff comments:
In `@scripts/screenshots.ts`:
- Around line 123-136: The startAppServer lifecycle must clean up the temporary
directory and child process when startup or health checks fail, and must still
stop Stripe if removeTree fails. Update the cleanup flow around startAppServer
and its wrapper to use nested finally blocks or failAfterCleanups, ensuring
every acquired resource is released while the original startup or shutdown error
is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a300f934-6dad-4c3c-a08d-b57cb5e9bb47
📒 Files selected for processing (3)
scripts/screenshots.tsscripts/screenshots/server.tstest/scripts/screenshots-server.test.ts
|
Addressed CodeRabbit’s follow-up lifecycle review in a912079. Startup now registers Stripe mock, temporary directory, and app child cleanup in acquisition order; failures run every cleanup in reverse order while preserving the startup error, and normal shutdown takes ownership of the same cleanup stack. Tests cover reverse-order failure cleanup, ownership transfer on success, and continuing after one cleanup fails. A paid mock checkout and |
1 participant
What changed
Why
The screenshot fixtures disabled online payments for every scenario. This made most paid checkout examples look like offline bookings, and child add-on quantity boxes stretched wider than the main ticket quantity box on mobile.
Checks
deno task precommitRisk
The fake Stripe settings and Stripe mock exist only for each throwaway screenshot run. They are not used by production or test databases.
Summary by CodeRabbit
Bug Fixes
Tests