✨ Add tab pool architecture to storybook SDK

[Robdel12]

Summary

Port the robust browser management from static-site SDK to fix timeout issues in CI:

• Tab pooling - Reuses browser tabs instead of creating/destroying per story
• Tab recycling - Closes and recreates tabs after 10 uses to prevent memory leaks
• Automatic retry - On timeout/crash, closes bad tab and retries with fresh one
• Tab state reset - Clears cookies/localStorage between uses
• Progress tracking - Shows ETA and completion percentage in TTY mode
• CI-optimized flags - Same hardened browser args as static-site

Also fixes the services.get is not a function bug - the dist wasn't rebuilt after the source was fixed.

Test plan

• All 104 unit tests pass
• Run storybook SDK in CI to verify no more timeouts
• Verify cloud mode works with VIZZLY_TOKEN set

[claude]

Code Review - Tab Pool Architecture

This is a solid architectural improvement that addresses real CI timeout issues. The implementation is well-structured and follows the codebase's functional patterns.

Strengths

Architecture & Design

• Excellent functional design: Using closures for the tab pool is idiomatic and aligns with the codebase's service-oriented approach
• Smart tab recycling: The 10-use recycling threshold is a practical solution to prevent memory leaks
• Automatic retry logic: Handling timeouts by closing bad tabs and retrying with fresh ones is robust
• Dependency injection: The deps parameter pattern in tasks.js makes testing straightforward

Code Quality

• Well-documented: Clear JSDoc comments explain the why behind design decisions
• Clean separation: pool.js (resource management) and tasks.js (orchestration) have clear boundaries
• Performance optimization: Set-based concurrency control is O(1) vs the old array-based O(n) approach
• UX improvements: Progress tracking with ETA and TTY-aware output is excellent

[claude]

Areas for Improvement

1. Missing Test Coverage - HIGH PRIORITY

Issue: The new pool.js and tasks.js files (552 lines total) have zero test coverage.

Impact: Core functionality like tab recycling, retry logic, and pool draining is untested. This is risky for CI-critical code.

Recommendation: Add test files:

• clients/storybook/tests/pool.test.js - Test tab lifecycle, recycling, drain behavior
• clients/storybook/tests/tasks.test.js - Test task generation, retry logic, progress tracking

Critical test cases needed:

• Pool: Tab reuse and recycling after 10 uses
• Pool: Drain resolves pending acquires with null
• Pool: Reset clears cookies/localStorage between uses
• Pool: Handle tab.close() failures gracefully
• Tasks: Retry timeout errors exactly once
• Tasks: Don't retry non-timeout errors
• Tasks: Handle pool.acquire() returning null after drain
• Tasks: Progress tracking and ETA calculation

Location references:

• Pool recycling logic: clients/storybook/src/pool.js:106-133
• Retry logic: clients/storybook/src/tasks.js:232-261
• Drain handling: clients/storybook/src/tasks.js:276-282

[claude]

2. Potential Race Condition in Pool Release

Issue: In pool.js:100-148, the release() function has a potential race when recycling tabs.

Location: clients/storybook/src/pool.js:117-132

Scenario:

1. Tab recycling triggers (useCount >= recycleAfter)
2. Old tab closes successfully
3. createTabEntry() fails (network issue, browser crash)
4. totalTabs-- reduces count
5. But a waiting worker was already queued and won't get a tab

Fix: Notify waiting workers when recycling fails - in the catch block around line 129-132 that does totalTabs--, also notify waiting workers by resolving with null if any are waiting.

---

3. Silent Reset Failures

Issue: resetTab() swallows all errors, which could mask serious issues.

Location: clients/storybook/src/pool.js:79-91

Concern: If resetTab() fails repeatedly, tabs accumulate stale state but continue being used. This could cause flaky screenshots.

Recommendation: Add debug logging for reset failures to provide visibility when troubleshooting.

[claude]

4. Retry Logic Only Covers Timeouts

Issue: The retry logic only handles timeout-like errors but not other transient failures.

Location: clients/storybook/src/tasks.js:237-241

Current detection: Only retries timeout, Timeout, Target closed, and Protocol error

Potential gaps:

• Network errors: net::ERR_CONNECTION_REFUSED
• Storybook loading errors: Story iframe fails to load
• Memory pressure: out of memory

Consider: Expanding retry logic or making it configurable to handle more transient error types.

---

5. Hard-Coded Pool Recycle Threshold

Issue: DEFAULT_RECYCLE_AFTER = 10 is not configurable.

Location: clients/storybook/src/pool.js:11

Concern: Large Storybooks with heavy stories might need more aggressive recycling (every 5 uses). Small ones might tolerate 20 uses.

Recommendation: Consider exposing this via configuration for advanced users who need to tune performance.

[claude]

6. Browser Args Now Always Use CI Flags

Issue: Removed the isCI() check, so CI-optimized flags now run in local dev too.

Location: clients/storybook/src/browser.js:63-70

Impact: Local dev now uses --disable-gpu, --disable-web-security, etc. This is probably fine (even good for consistency), but worth documenting.

Recommendation: Add a comment explaining this intentional change for consistency between local and CI environments.

---

7. Progress Output Race Condition (Minor)

Issue: The output coordinator queues errors but could drop them if progress updates stop.

Location: clients/storybook/src/tasks.js:180-186

Scenario: If the last few tasks error out but no more progress updates come, errors sit in pendingErrors until flush(). The flush() call at tasks.js:346 handles this correctly, but the flow is subtle.

Recommendation: Document the flush requirement more clearly in the comment.

[claude]

Minor Suggestions

1. ETA Calculation Edge Case

Location: clients/storybook/src/tasks.js:303

When config.concurrency = 1 and remaining = 1, you still divide by concurrency. This is mathematically correct but worth a comment explaining the parallelism calculation.

2. Magic Number for ETA Samples

Location: clients/storybook/src/tasks.js:223

Consider extracting 5 and 0.1 as named constants with explanation of why these values balance early feedback vs accuracy.

3. Formatting Consistency

The test file changes show mostly import reordering and formatting fixes. These are good hygiene but make the diff larger. Consider whether these should be in a separate formatting commit.

[claude]

Security Review

No security concerns identified. The changes:

• Don't introduce new user input paths
• Don't modify authentication/authorization
• Use Puppeteer APIs safely (no command injection)
• Tab state reset properly clears cookies/localStorage

---

Test Plan Validation

The PR's test plan says:

• All 104 unit tests pass
• Run storybook SDK in CI to verify no more timeouts
• Verify cloud mode works with VIZZLY_TOKEN set

Recommendation:

1. Add the missing unit tests for pool.js and tasks.js
2. Before merging, complete the CI and cloud mode validation
3. Consider adding a smoke test that intentionally triggers a timeout to verify retry logic works

[claude]

Overall Assessment

Verdict: Approve with changes

This PR solves a real problem (CI timeouts) with a well-architected solution. The code quality is high and follows repository patterns well. The main blocker is missing test coverage for critical new code.

Before merging:

1. ⚠️ MUST: Add comprehensive tests for pool.js and tasks.js
2. ⚠️ MUST: Fix the race condition in pool recycling
3. ⚠️ SHOULD: Complete the CI validation testing
4. 💡 CONSIDER: Making retry logic and recycle threshold configurable

Estimated test additions: ~200-300 lines of test code to match the 552 lines of new functionality.

Great work on the architectural improvement! The tab pooling pattern is exactly what this codebase needs for reliable CI operation.

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

filter-failed-only · Firefox · 1920×1080 · 1.3% diff

bulk-accept-dialog · Firefox · 1920×1080 · 10.3% diff

viewer-zoomed-100 · Firefox · 1920×1080 · 0.4% diff

filter-failed-only · Firefox · 375×704 · 14.2% diff

bulk-accept-dialog · Firefox · 375×667 · 208.9% diff

fullscreen-viewer · Firefox · 375×667 · 162.1% diff

Review changes

CLI TUI - Processing...

Build in progress...

---

_{feat/storybook-tab-pool · 701874ce}

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

filter-failed-only · Firefox · 1920×1080 · 1.3% diff

bulk-accept-dialog · Firefox · 1920×1080 · 10.3% diff

viewer-zoomed-100 · Firefox · 1920×1080 · 0.4% diff

filter-failed-only · Firefox · 375×704 · 14.2% diff

bulk-accept-dialog · Firefox · 375×667 · 208.9% diff

fullscreen-viewer · Firefox · 375×667 · 162.1% diff

Review changes

CLI TUI - Processing...

Build in progress...

---

_{feat/storybook-tab-pool · 701874ce}

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

filter-failed-only · Firefox · 1920×1080 · 1.3% diff

bulk-accept-dialog · Firefox · 1920×1080 · 10.3% diff

viewer-zoomed-100 · Firefox · 1920×1080 · 0.4% diff

filter-failed-only · Firefox · 375×704 · 14.2% diff

bulk-accept-dialog · Firefox · 375×667 · 208.9% diff

fullscreen-viewer · Firefox · 375×667 · 162.1% diff

Review changes

CLI TUI - 1 change needs review

Status	Count
Passed	4
Changed	1
Auto-approved	4

Changes needing review (1)

vizzly-help · 1202×1430 · 1.4% diff

Review changes

---

_{feat/storybook-tab-pool · 701874ce}