Skip to content

✨ Add VIZZLY_BUILD_NAME environment variable support#195

Merged
Robdel12 merged 2 commits intomainfrom
feat/build-name-env-var
Jan 30, 2026
Merged

✨ Add VIZZLY_BUILD_NAME environment variable support#195
Robdel12 merged 2 commits intomainfrom
feat/build-name-env-var

Conversation

@Robdel12
Copy link
Copy Markdown
Contributor

Summary

  • Add VIZZLY_BUILD_NAME environment variable for setting build names in CI without CLI flags
  • Makes CI configuration cleaner - no need to interpolate values in CLI args

Changes

  • Add getBuildName() function to environment-config.js
  • Wire into config-loader.js to set config.build.name from environment
  • Add tests for new functionality
  • Update CLAUDE.md documentation

Usage

# Instead of
vizzly run "npm test" --build-name "PR #${PR_NUMBER} - ${BRANCH_NAME}"

# You can now use
export VIZZLY_BUILD_NAME="PR #${PR_NUMBER} - ${BRANCH_NAME}"
vizzly run "npm test"

Test plan

  • Unit tests pass for environment-config.js
  • Unit tests pass for config-loader.js
  • Lint passes

- Add getBuildName() to environment-config.js
- Wire into config-loader.js to set build.name from env var
- Update CLAUDE.md with new env var documentation
- Add tests for new functionality
@vizzly-testing

This comment has been minimized.

@claude
Copy link
Copy Markdown

claude Bot commented Jan 30, 2026

Code Review

Summary

This PR cleanly adds support for the VIZZLY_BUILD_NAME environment variable, making CI configuration cleaner by eliminating the need to interpolate values in CLI arguments. The implementation follows established patterns in the codebase and includes comprehensive test coverage.

✅ Strengths

  1. Consistency: The implementation follows the exact same pattern used for other environment variables like VIZZLY_PARALLEL_ID and VIZZLY_TOKEN, maintaining excellent consistency across the codebase.

  2. Proper Priority Order: Correctly respects the configuration priority chain:

    • CLI flags (highest) → Environment variables → Config file → Defaults (lowest)
    • Test at config-loader.test.js:148 validates this correctly
  3. Comprehensive Test Coverage:

    • Tests for the new getBuildName() function in environment-config.test.js
    • Tests for config loading behavior in config-loader.test.js
    • Tests for priority order (CLI override > env var)
    • Test cleanup properly added to setup/teardown
  4. Documentation: CLAUDE.md updated with the new environment variable, maintaining alphabetical order in the list

  5. Clean Implementation: Minimal, focused changes with no over-engineering

🔍 Code Quality

src/utils/environment-config.js

  • Simple getter function matching the established pattern
  • Proper JSDoc comments
  • Correctly added to getAllEnvironmentConfig() for completeness

src/utils/config-loader.js

  • Clean import addition
  • Proper truthiness check (if (envBuildName)) consistent with envParallelId
  • Applied before CLI overrides, after file config (correct priority)

Tests

  • Good coverage of both positive and negative cases
  • Environment cleanup properly handled
  • Clear, descriptive test names

🎯 Suggestions

Minor: Environment Variable Truthiness Check Inconsistency

There's a subtle inconsistency in how environment variables are checked in config-loader.js:

// Line 116-122
if (envApiKey) {
  config.apiKey = envApiKey;
  output.debug('config', 'using token from environment');
}
if (envApiUrl !== 'https://app.vizzly.dev') config.apiUrl = envApiUrl;
if (envBuildName) config.build.name = envBuildName;
if (envParallelId) config.parallelId = envParallelId;

Observation:

  • envApiKey gets a debug log when set, but envBuildName and envParallelId don't
  • envApiUrl has special handling (only overrides if non-default)

Suggestion: Consider adding a debug log for envBuildName for consistency with envApiKey. This would help with troubleshooting CI configurations:

if (envBuildName) {
  config.build.name = envBuildName;
  output.debug('config', 'using build name from environment');
}

This is a minor nice-to-have, not a blocker. The current implementation is functionally correct.

🛡️ Security, Performance & Best Practices

  • Security: ✅ No concerns. Build names are user-provided strings with no execution context.
  • Performance: ✅ No impact. Simple string assignment.
  • Best Practices: ✅ Follows established patterns, proper test coverage, clean code.

📝 Test Coverage

Test coverage is excellent:

  • ✅ Environment variable undefined case
  • ✅ Environment variable set case
  • ✅ CLI override priority
  • ✅ Integration with getAllEnvironmentConfig()
  • ✅ Proper cleanup in test setup/teardown

✨ Verdict

LGTM! This is a well-implemented, focused feature that improves the CI/CD experience. The code is clean, follows existing patterns, and has comprehensive test coverage. The minor suggestion above is optional and doesn't block approval.

Great work on maintaining consistency with the existing codebase patterns! 🚀

@vizzly-testing
Copy link
Copy Markdown

Vizzly - Visual Test Results

CLI Reporter - 5 changes need review
Status Count
Passed 14
Changed 5
Auto-approved 14
Changes needing review (5)

fullscreen-viewer · Firefox · 1920×1080 · 5.7% diff

fullscreen-viewer

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

bulk-accept-dialog

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

viewer-overlay-mode

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

fullscreen-viewer

bulk-accept-dialog · Firefox · 375×892 · 1.5% diff

bulk-accept-dialog

Review changes

CLI TUI - Processing...

Build in progress...


feat/build-name-env-var · 4d5fc79c

@Robdel12 Robdel12 merged commit 693fa63 into main Jan 30, 2026
28 of 29 checks passed
@Robdel12 Robdel12 deleted the feat/build-name-env-var branch January 30, 2026 06:38
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