fix: add a 60-second Node-side timeout to each story capture - #41
Merged
Conversation
A capture that hangs — due to a browser transport failure, a Node runtime regression (e.g. nodejs/node#61098 affecting Firefox on Node 24), or an unresponsive engine — now reports "Capture timed out" instead of freezing the UI indefinitely. The in-page 15s waitForStory timeout only fires if the Playwright transport is healthy; if the transport itself breaks, page.evaluate() never returns and Node never sees the timeout. AbortSignal.any() combines the existing user-cancel signal with AbortSignal.timeout() so the runner surfaces a capture-error regardless of cause. Closes #39 Assisted-by: claude-code/claude-opus-4-6
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe story runner adds a configurable 60-second capture timeout. Hanging captures produce a ChangesStory capture timeout
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VisualTestRunner
participant CaptureSession
participant TimeoutTimer
VisualTestRunner->>TimeoutTimer: start configured timeout
VisualTestRunner->>CaptureSession: capture with combined cancellation signal
TimeoutTimer-->>VisualTestRunner: abort timeout signal
VisualTestRunner->>VisualTestRunner: report capture-error with timeout message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
AbortSignal.timeout() doesn't interrupt a never-resolving capture mock — the signal aborts but the await never yields. Promise.race against a setTimeout rejection catches the hang regardless of whether the underlying operation respects the signal. Remove unused imports (afterEach, beforeEach, CAPTURE_TIMEOUT_MS). Use a real 50ms timeout in the test instead of fake timers. Add captureTimeoutMs option for test injection. Assisted-by: claude-code/claude-opus-4-6
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/storyproof/test/runner.test.ts (1)
790-796: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the regression test verify timeout cancellation.
The fake capture never resolves and ignores
request.signal. This test passes even if the timeout only rejectsPromise.raceand leaves the real capture active. Record the signal passed tocaptureand assert that itsabortevent fires.🤖 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 `@packages/storyproof/test/runner.test.ts` around lines 790 - 796, Update the regression test’s createCaptureSession mock and capture setup to record the signal passed to capture, then assert that the signal’s abort event fires when the timeout occurs. Keep the never-resolving capture behavior so the test verifies cancellation of the underlying capture rather than only rejection of Promise.race.
🤖 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 `@packages/storyproof/src/node/runner.ts`:
- Around line 404-417: Update the timeout setup in the capture Promise.race flow
to store the setTimeout handle, then clear it in a finally block after the race
settles, regardless of whether capturePromise resolves, rejects, or times out.
Preserve the existing timeout error and capture result behavior.
- Around line 399-417: The capture timeout currently rejects without cancelling
session.capture(). In the capture flow around session.capture and Promise.race,
create a timeout AbortController, pass AbortSignal.any([run.controller.signal,
timeoutController.signal]) as the capture request signal, and abort
timeoutController in the timeout callback before rejecting; preserve the
existing timeout error behavior.
- Around line 73-74: Validate the optional captureTimeoutMs input during
VisualTestRunner construction, accepting only finite numbers greater than zero
and rejecting zero, negative, NaN, and Infinity values before they reach
setTimeout. Preserve the existing default timeout behavior when the option is
omitted.
---
Nitpick comments:
In `@packages/storyproof/test/runner.test.ts`:
- Around line 790-796: Update the regression test’s createCaptureSession mock
and capture setup to record the signal passed to capture, then assert that the
signal’s abort event fires when the timeout occurs. Keep the never-resolving
capture behavior so the test verifies cancellation of the underlying capture
rather than only rejection of Promise.race.
🪄 Autofix
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 Plus
Run ID: 667fd673-bfaa-4b1f-949d-36cf11bebb0a
📒 Files selected for processing (3)
.changeset/capture-timeout.mdpackages/storyproof/src/node/runner.tspackages/storyproof/test/runner.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signal the capture via AbortSignal.any() so a timed-out capture tears down its browser context instead of leaking it. Keep Promise.race so the runner continues without awaiting the hung capture. Clear the setTimeout handle in a finally block after the race settles so successful captures don't accumulate 60 s timers. Wrap test assertions in try/finally so the temp directory cleans up on assertion failure. Assisted-by: claude-code/claude-opus-4-6
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Use a dedicated timeoutController so the catch block can distinguish
a timeout abort from a user cancel — capture.ts returning
{status:"cancelled"} on a timeout no longer hides the error message.
- Abort the entire run after 3 consecutive capture timeouts instead of
crawling through every remaining story at 60 s each.
- Race session.close() against a 5 s deadline so a wedged transport
can't re-hang the run after captures have finished.
- Single timer drives both the abort signal and the race rejection.
- Un-export CAPTURE_TIMEOUT_MS (no external consumer).
Assisted-by: claude-code/claude-opus-4-6
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
failRun sets all targets to capture-error but didn't abort the controller — concurrent workers in runPool kept running and could publish results after the run-wide failure. Now aborts the controller and clears completed candidates for the failed run, matching cancelActiveRun's cleanup. Assisted-by: claude-code/claude-opus-4-6
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The Promise.race abort waiter only rejected when the per-capture timeoutController fired. When the run controller aborted (from the 3-consecutive-timeout path), a concurrent worker's race stayed unsettled because its abort listener filtered on timeoutController. Now rejects on any abort of the combined signal. Assisted-by: claude-code/claude-opus-4-6
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 `@packages/storyproof/src/node/runner.ts`:
- Around line 303-306: Update the Promise.race cleanup around session.close() to
store the five-second timeout handle and clear it in a finally block, ensuring
the fallback timer is canceled when session.close() resolves first. Preserve the
existing timeout fallback and error-swallowing behavior.
🪄 Autofix
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 Plus
Run ID: 5beca9a1-236e-4d9c-b7b2-4744d25744f8
📒 Files selected for processing (2)
packages/storyproof/src/node/runner.tspackages/storyproof/test/runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/storyproof/test/runner.test.ts
The 5 s setTimeout in the session.close() race wasn't cleared when close() resolved first, leaving a live timer handle on the event loop. Assisted-by: claude-code/claude-opus-4-6
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
AbortSignal.timeout()to eachsession.capture()call, combined with the existing user-cancel signal viaAbortSignal.any()Closes #39
Why
The in-page 15 s
waitForStorytimeout only fires if the Playwright transport is healthy. When the transport itself breaks (e.g. Node 24's duplexPair stream change affecting Firefox's Juggler transport — nodejs/node#61098),page.evaluate()never returns and the runner stays in "running" state forever. The UI freezes with no error message.A Node-side timeout is the outermost layer the addon controls. Everything inside it — browser launch, page navigation, in-page evaluation, screenshot — is covered.
Test
Unit test with fake timers: a capture that never resolves is aborted after
CAPTURE_TIMEOUT_MSand reportscapture-errorwith a "timed out" message.Assisted-by: claude-code/claude-opus-4-6
Summary by cubic
Add a 60-second Node-side timeout to each
session.capture()so hung captures fail with a clear error instead of freezing the UI. Abort the run after 3 timeouts and ensure workers stop cleanly; bound shutdown so a broken transport can’t re-hang. Addresses #39.Promise.race; combine signals withAbortSignal.any()so timed-out captures abort and tear down their browser context; clear the timer.capture-errorwith a "timed out" message, keepcancelledfor user aborts.session.close()with a 5 s race and clear its fallback timer;captureTimeoutMsis configurable.Written for commit 6adef29. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features