Skip to content

Conversation

@Robdel12
Copy link
Contributor

Summary

  • Adds vizzly preview step to the static-site SDK E2E workflow
  • Uploads the test-site as a preview after cloud mode tests complete
  • Tests the preview command in real CI conditions

Test plan

  • CI runs the static-site SDK E2E job
  • Preview upload step succeeds after vizzly run
  • Preview URL is generated and linked to the build

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@claude
Copy link

claude bot commented Jan 20, 2026

PR Review

Overview

This PR adds the vizzly preview command to the static-site SDK E2E workflow, testing the preview upload functionality in real CI conditions after cloud mode tests complete.

Code Quality: ✅ Good

Strengths:

  • Clean, focused change that adds a single step to test preview functionality
  • Follows existing workflow patterns (working directory, token usage)
  • Uses the same token secret that's already established for cloud mode tests

Potential Issues: ⚠️ Minor Concerns

1. Missing Git Context Environment Variables

The preview command runs after the cloud mode tests, but it's missing the git context environment variables that were set in the previous step. The cloud mode step includes:

  • VIZZLY_COMMIT_MESSAGE
  • VIZZLY_COMMIT_SHA

While the preview command itself (src/commands/preview.js:1-459) doesn't directly use these variables, it relies on the build ID from the session file created by the previous vizzly run command. If the session file stores git context, this might be fine. However, for consistency and completeness, consider adding these same environment variables to the preview step.

2. Working Directory Path Resolution

The command runs from ./clients/static-site and references ../../test-site. Verification:

  • ✅ Confirmed: test-site/ exists at repo root with HTML files for a test SaaS marketing site
  • ✅ Path resolution is correct: from clients/static-site../../test-site = repo root

3. Error Handling

The preview step has no error handling. If the preview upload fails:

  • Will the entire workflow fail?
  • Should it be marked with continue-on-error: true since this is a new feature being tested?

Consider whether preview upload failure should block the workflow or just be reported.

Security: ✅ Good

  • Uses existing secret token (VIZZLY_STATIC_SITE_CLIENT_TOKEN)
  • No new credentials exposed
  • Path validation in preview.js (isPathSafe function) prevents command injection
  • Symlink protection prevents path traversal attacks

Performance: ✅ Good

  • Sequential execution is appropriate (preview depends on build from previous step)
  • Timeout already set at job level (8 minutes)

Test Coverage: ℹ️ Info

  • This PR IS the test - it adds E2E coverage for the preview command in CI
  • No unit tests found for preview command itself (search shows no tests/commands/preview.spec.js)
  • Consider adding unit tests for the preview command separately if not already covered

Documentation: ✅ Covered

  • PR description clearly explains the purpose
  • Test plan is clear and actionable

Recommendations

Priority: Medium

  1. Add git context environment variables (VIZZLY_COMMIT_MESSAGE, VIZZLY_COMMIT_SHA) for consistency with cloud mode step
  2. Decide on error handling strategy (fail vs. continue-on-error)

Priority: Low
3. Consider capturing preview URL as output for potential use in subsequent steps
4. Add unit tests for preview command if not already present

Verdict: ✅ Approve with Suggestions

This is a solid addition to the E2E test suite. The change is minimal, focused, and follows existing patterns. The suggestions above would make it more robust, but the PR is functional as-is for testing the preview command in CI.

Note: The preview command implementation (src/commands/preview.js) shows good security practices with path validation, symlink protection, and command injection prevention. Nice work on that front!

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing
Copy link

vizzly-testing bot commented Jan 21, 2026

🐻 Vizzly — 14 changes need review

Status Count
Passed 5
Changed 14
Auto-approved 5
Changes needing review (14)

viewer-overlay-mode · Firefox · 1920×1080 · 1.6% diff

viewer-overlay-mode

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

filter-failed-only

search-homepage · Firefox · 1920×1080 · 0.2% diff

search-homepage

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

bulk-accept-dialog

search-no-results · Firefox · 1920×1080 · 0.2% diff

search-no-results

viewer-slide-mode · Firefox · 1920×1080 · 0.2% diff

viewer-slide-mode

...and 8 more in Vizzly.

Review changes →


CLI Reporter · test-preview-e2e · a3bdd0c0

@vizzly-testing
Copy link

🐻 Vizzly — 1 change needs review

Status Count
Changed 1
Auto-approved 4
Changes needing review (1)

vizzly-help · 1202×1430 · 556.0% diff

vizzly-help

Review changes →


CLI TUI · test-preview-e2e · a3bdd0c0

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.

2 participants