🐛 Keep TDD server running after tests for review

[Robdel12]

Summary

Fixes #159 - tdd run no longer generates a static report, but now keeps the server running so users can review changes interactively.

Changes:

• After tdd run completes with comparisons, server stays alive for review
• Shows "Press Enter to stop server" prompt after the results summary
• Ctrl+C gracefully stops the server with correct exit code
• Added static mode infrastructure to reporter (for potential future static HTML reports)

Before:

→ Review changes: http://localhost:47392
# Server immediately stops - URL is inaccessible

After:

→ Review changes: http://localhost:47392
→ Press Enter to stop server
# Server stays running until user presses Enter or Ctrl+C

Test plan

• Run vizzly tdd run "npx playwright test ..." with tests that produce screenshots
• Verify server stays running after tests complete
• Verify dashboard is accessible at the URL
• Verify Enter key stops server
• Verify Ctrl+C stops server gracefully
• Verify no comparisons = server stops immediately (no prompt)

[claude]

Code Review

Thanks for tackling issue #159! The approach of keeping the server running for interactive review is a good user experience improvement. Here's my feedback:

---

Code Quality & Design

✅ Good:

• Clean implementation with proper signal handling for graceful shutdown
• Good use of process.stdin for waiting on user input
• Exit code properly preserved from test results in signal handlers
• Static mode infrastructure is well-structured and follows existing patterns
• Checks for hasComparisons before prompting is smart - avoids unnecessary prompts

⚠️ Issues Found:

1. Stdin Not Restored After User Input (cli.js:455-459)

await new Promise(resolve => {
  process.stdin.setRawMode?.(false);
  process.stdin.resume();
  process.stdin.once('data', resolve);
});

Problem: After the user presses Enter, process.stdin remains in resumed state and isn't properly cleaned up. This could interfere with subsequent operations or leave dangling listeners.

Fix: Pause stdin after receiving input:

await new Promise(resolve => {
  process.stdin.setRawMode?.(false);
  process.stdin.resume();
  process.stdin.once('data', () => {
    process.stdin.pause();
    resolve();
  });
});

2. Signal Handlers May Fire After Cleanup (cli.js:438-444)

process.once('SIGINT', () => {
  handleCleanup().then(() => process.exit(result?.exitCode || 0));
});

Problem: Signal handlers are registered before await cleanup() at line 462. If a signal fires during the stdin wait, both the signal handler AND the normal cleanup path will execute, causing double cleanup.

Fix: Remove signal listeners before normal cleanup or check cleanup state:

await cleanup();

// Remove signal handlers to prevent double cleanup
process.removeAllListeners('SIGINT');
process.removeAllListeners('SIGTERM');

process.exit(result?.exitCode || 0);

3. Potential Race Condition in Reporter (use-tdd-queries.js:8-9)

const staticMode = isStaticMode();
const staticData = staticMode ? window.VIZZLY_REPORTER_DATA : undefined;

Problem: The useReportData hook computes staticMode on every render. If window.VIZZLY_STATIC_MODE could change during the component lifecycle, this could cause inconsistent behavior between the data source and refetch settings.

Recommendation: Although unlikely to change in practice, consider using useMemo to ensure consistency within the hook's lifecycle:

const staticMode = useMemo(() => isStaticMode(), []);
const staticData = useMemo(() => staticMode ? window.VIZZLY_REPORTER_DATA : undefined, [staticMode]);

---

Test Coverage

❌ Missing Tests:

• No tests for the new stdin waiting behavior
• No tests verifying server stays running after comparisons
• No tests for signal handling during stdin wait
• No tests for the "no comparisons" case (should skip prompt)

Recommended Test Cases:

1. tdd run with comparisons → server should wait for Enter
2. tdd run with no comparisons → server should stop immediately
3. Ctrl+C during stdin wait → should cleanup and exit with correct code
4. Multiple SIGINT/SIGTERM → should not double-cleanup

---

Performance & Security

No significant concerns - the changes are minimal and don't introduce performance issues or security vulnerabilities.

---

Additional Considerations

Documentation Update Needed

The PR mentions "Documentation still says 'Each tdd run generates an HTML report'" - this documentation should be updated to reflect the new interactive behavior.

Static Report Future

The PR adds "static mode infrastructure for potential future static HTML reports." Consider:

• Should there be a --static flag to generate HTML reports instead of keeping server running?
• Would users want both behaviors available?

Edge Cases

1. What if stdin is not a TTY? (e.g., running in CI)

   • process.stdin.setRawMode?.() uses optional chaining which is good
   • But process.stdin.resume() will still be called - verify this works correctly in non-interactive environments

2. What if user closes terminal without pressing Enter?

   • Signal handlers should catch this, but verify behavior

---

Summary

Blockers (must fix before merge):

1. Fix stdin cleanup (pause after input)
2. Fix double cleanup potential from signal handlers
3. Add test coverage for new behavior

Nice to have:

• Use useMemo in reporter hook for consistency
• Document the new behavior vs old static report behavior
• Consider adding --static flag for HTML report generation

Overall Assessment: Good solution to the problem, but needs cleanup handling fixes and test coverage before merge.