✨ Static Site SDK: UX improvements and --dry-run option #151
Conversation
- Exit early with helpful message when no TDD server or API token found - Add smart default concurrency based on CPU cores (half cores, min 2, max 8) - Add progress with ETA during screenshot capture - Show total completion time at end of run The --concurrency flag already existed; now the default is smarter.
Sitemap: - Fix sitemap index files returning child sitemap URLs as pages - Now follows sitemap index to parse actual child sitemaps - Extracts real page URLs instead of sitemap-0.xml etc. Progress display: - Interactive terminals: single updating progress line with ETA - Non-interactive (CI): traditional per-task logging - Shows completion summary with total time and error count
- Add --dry-run flag to print discovered pages without capturing screenshots - Shows pages grouped by source (sitemap vs HTML scan) - Displays task count, viewports, and concurrency settings - Useful for debugging page discovery and include/exclude patterns Also includes tests for: - Sitemap XML parsing (standard and index formats) - Config schema validation and smart concurrency defaults - Dry-run mode behavior with various options
This comment has been minimized.
This comment has been minimized.
Pull Request ReviewThis PR significantly improves the static-site SDK's user experience with smart defaults, better error handling, and helpful debugging tools. Overall, the implementation is solid with excellent test coverage. Here's my detailed feedback: ✅ StrengthsCode Quality
UX Improvements
🔍 Issues & Concerns1. Potential Race Condition in Progress Display (Medium Priority)Location: Issue: When running with high concurrency, multiple tasks may fail simultaneously. The writeProgress('') + process.stdout.write sequence could interleave with concurrent success messages, leading to garbled output. Recommendation: Wrap the entire error logging block in a mutex/lock, or accumulate errors and print them after clearing the progress line once. 2. ETA Calculation May Be Inaccurate Initially (Low Priority)Location: Issue: Early in execution, avgTime can be highly variable. The first task might take 5s (cold browser), while subsequent ones take 500ms, leading to wildly inaccurate ETAs initially. Recommendation: Consider using an exponential moving average or wait until N tasks complete before showing ETA. 3. Sitemap Child Resolution Has Security Implications (Medium Priority)Location: Issue: A malicious sitemap could contain URLs like https://evil.com/../../../etc/passwd. While join() normalizes paths, the .split('/').pop() approach is fragile. Recommendation:
4. Redundant Dynamic Imports (Low Priority)Location: Issue: Dynamic imports are already available at the top of the file. These redundant dynamic imports add overhead. Recommendation: Import dirname and existsSync at the top of the file instead. 5. Silent Failures in Sitemap Parsing (Low Priority)Location: Issue: Silent failures make debugging difficult. Users won't know if a child sitemap failed to parse due to permissions, corrupted XML, or other issues. Recommendation: Add optional debug logging when VIZZLY_LOG_LEVEL is set to debug. 6. getDefaultConcurrency() Performance (Low Priority)Location: Issue: cpus().length is called every time getDefaultConcurrency() executes. While cheap, this could be cached. Recommendation: Cache the result in a module-level constant. 💡 Suggestions1. Consider Exponential Backoff for Failed Child SitemapsIf a child sitemap is temporarily unavailable (network file system, etc.), might be worth retrying with backoff instead of silently skipping. 2. Add --verbose Option for Dry RunThe dry-run output is great, but for large sites with 1000+ pages, it could be overwhelming. Consider adding a verbose flag to show all pages vs just a summary. 3. Progress Bar Library for Better UXConsider using a library like cli-progress or ora for more robust progress display handling (handles terminal resizing, concurrent writes, etc.). 📋 Test Coverage AnalysisExcellent work on testing! New test files provide comprehensive coverage:
Suggestion: Add edge case tests for:
🔒 Security Assessment
🎯 SummaryRecommendation: Approve with minor changes This is a high-quality PR that delivers real value. The UX improvements are well-thought-out and the test coverage is exemplary. Address the path traversal concern (Issue #3) before merging, and consider the race condition fix (Issue #1) for a follow-up PR. Priority Fixes:
Nice-to-haves:
Great work! The static-site SDK is significantly better with these changes. |
- Fix race condition in progress display by queuing errors - Improve ETA accuracy: wait for 5+ samples, use recent 20 for average - Add path traversal protection for sitemap child resolution - Move dynamic imports to top level in sitemap.js - Cache CPU count for concurrency default - Add security tests for sitemap path traversal
Summary
Improves the static-site SDK with better UX, smarter defaults, and debugging tools:
--dry-runoption to preview discovered pages without capturing screenshotsChanges
Better Error Handling
Smart Concurrency
Math.max(2, Math.min(8, Math.floor(cores / 2)))Progress Display
Sitemap Index Support
Dry Run Option
Output shows:
Test plan
npm test(154 tests passing)CI=true