Skip to content

♻️ Refactor static site SDK to use functional tab pool + work queue#150

Merged
Robdel12 merged 2 commits intomainfrom
refactor/functional-tab-pool
Dec 22, 2025
Merged

♻️ Refactor static site SDK to use functional tab pool + work queue#150
Robdel12 merged 2 commits intomainfrom
refactor/functional-tab-pool

Conversation

@Robdel12
Copy link
Contributor

Summary

  • Add functional tab pool (pool.js) using closures for browser tab management
  • Add task-based processing (tasks.js) with dependency injection for testability
  • Replace page-level concurrency with task-level (page × viewport = task)
  • Migrate from Vitest to Node native test runner (matching main CLI)
  • Rename all .spec.js to .test.js
  • Remove vitest dependency
  • Add tests for viewport helpers (formatViewport, setViewport, getCommonViewports)
  • Add tests for pattern matching edge cases

Key Architecture Changes

Before: Page-level concurrency where each page processed viewports sequentially
After: Task-level concurrency where each (page, viewport) tuple is an independent task

Benefits:

  • True tab control - Concurrency now means "max tabs" not "max pages"
  • Tab reuse - No create/destroy overhead per screenshot (tabs are pooled)
  • Even work distribution - Tasks processed as capacity allows via work queue
  • Functional - No classes, just functions and closures
  • Testable - Dependency injection enables unit testing without module mocking

Coverage

Metric Value
Lines 81.89%
Branches 91.81%
Functions 73.44%

Test plan

  • All 118 tests pass
  • Lint passes (biome)
  • Format passes (biome)
  • Manual test with real static site build

- Add functional tab pool (pool.js) using closures for tab management
- Add task-based processing (tasks.js) with dependency injection
- Replace page-level concurrency with task-level (page×viewport)
- Migrate from Vitest to Node native test runner
- Rename all .spec.js to .test.js
- Remove vitest dependency
- Add tests for viewport helpers and pattern edge cases

Key improvements:
- True tab control: concurrency now means "max tabs" not "max pages"
- Tab reuse: no create/destroy overhead per screenshot
- Even work distribution: tasks processed as capacity allows
- Functional: no classes, just functions and closures

Coverage: 81.89% lines, 91.81% branches, 73.44% functions
@blacksmith-sh

This comment was marked as outdated.

@claude

This comment was marked as outdated.

- Fix concurrency test to import mapWithConcurrency from tasks.js
- Export mapWithConcurrency and add proper JSDoc documentation
- Document pool drain() null behavior in acquire() JSDoc
- Add tab cleanup (resetTab) before release to prevent cross-contamination
- Add debug logging to isTddModeAvailable for troubleshooting
- Fix mapWithConcurrency error propagation through Promise.all
- Update pool tests for async release() method
@blacksmith-sh
Copy link
Contributor

blacksmith-sh bot commented Dec 22, 2025

Found 4 test failures on Blacksmith runners:

Failures

Test View Logs
[firefox-mobile] › tests/reporter/specs/
review-workflow.spec.js:40:3 › Review Workflow › user can open a screenshot and see the
fullscreen viewer
View Logs
[firefox-mobile] › tests/reporter/specs/
review-workflow.spec.js:79:3 › Review Workflow › user can close fullscreen viewer with
Escape key
View Logs
Review Workflow/user can close fullscreen viewer with Escape key View Logs
Review Workflow/user can open a screenshot and see the fullscreen viewer View Logs

Fix in Cursor

@Robdel12 Robdel12 merged commit a573296 into main Dec 22, 2025
32 of 36 checks passed
@Robdel12 Robdel12 deleted the refactor/functional-tab-pool branch December 22, 2025 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant