✨ Add git detection API to plugin services

[Robdel12]

Summary

• Adds services.git.detect() to the plugin API for correct git info detection
• Replaces fragile dynamic imports in storybook/static-site plugins with clean API
• Handles CI environments correctly (GitHub Actions PR merge commits, GitLab, etc.)
• Graceful fallback for older CLI versions, with peer dep warning

Problem

The storybook and static-site plugins were using a fragile dynamic import to access the CLI's git utilities:

gitUtils = await import('@vizzly-testing/cli/dist/utils/git.js').catch(() => null);

This was breaking in some environments, causing incorrect git info (wrong branch, wrong commit SHA) in PR builds.

Solution

Add a proper plugin API for git detection:

// Plugin can now do:
let gitInfo = await services.git.detect({ buildPrefix: 'Storybook' });
// Returns: { branch, commit, message, prNumber, buildName }

The CLI handles all the complexity of CI environment detection (GitHub Actions uses GITHUB_HEAD_REF for PRs, reads event payload for correct commit SHA, etc.).

Compatibility

Plugin	CLI	Result
Old	Old	✅ Works
Old	New	✅ Works (new API is additive)
New	New	✅ Uses proper git detection
New	Old	✅ Falls back to env vars (with peer dep warning)

Test plan

• CLI build passes
• CLI tests pass
• Storybook plugin builds and tests pass
• Static-site plugin builds and tests pass
• Verified services.git.detect() returns correct values locally
• Verified CI env var simulation returns correct branch/PR number

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

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

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

viewer-zoomed-100 · Firefox · 1920×1080 · 0.1% diff

search-homepage · Firefox · 375×696 · 0.7% diff

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

bulk-accept-dialog · Firefox · 375×667 · 159.2% diff

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{feat/plugin-git-api · 8f920eca}

[claude]

Code Review

I've reviewed this PR and overall it's a well-designed improvement to the plugin API. The change from fragile dynamic imports to a proper API is the right approach. Here's my detailed feedback:

✅ Strengths

1. Excellent API Design

• Clean, stable interface in src/plugin-api.js:35-62 that properly abstracts git detection
• Good encapsulation using Object.freeze() to prevent modifications
• Clear JSDoc documentation explaining purpose and return values
• Backwards-compatible fallback strategy works well

2. Correct CI Environment Handling

• Properly delegates to existing battle-tested utils/git.js and utils/ci-env.js
• Handles GitHub Actions PR merge commit issue correctly (reading from event payload)
• Supports wide range of CI providers through existing infrastructure

3. Clean Migration Path

• Both plugins (clients/storybook/src/index.js:115-135 and clients/static-site/src/index.js:169-191) handle old and new CLI versions gracefully
• Fallback to environment variables ensures compatibility
• Peer dependency bump clearly signals version requirement

4. Code Quality

• Removes fragile dynamic import hackery (good riddance to those try/catch chains!)
• Simplifies plugin code significantly (-15 lines in storybook, -13 in static-site)
• More readable and maintainable

⚠️ Issues & Concerns

1. Missing Test Coverage (Critical)

The new services.git.detect() API has zero test coverage. This is a public API that plugins depend on, and it should have comprehensive tests covering:

• ✅ Returns correct git info in normal git repo
• ✅ Handles GitHub Actions PR context (merge commit SHA vs head SHA)
• ✅ Handles missing git repo gracefully
• ✅ Respects environment variable overrides (VIZZLY_*)
• ✅ Correctly formats buildName with and without prefix
• ✅ Returns proper structure { branch, commit, message, prNumber, buildName }

Recommendation: Add tests/unit/plugin-api.spec.js with comprehensive coverage before merging.

2. Inconsistent Error Handling

src/plugin-api.js:45-61 calls multiple async git functions but doesn't handle errors. What happens if:

• Not in a git repo?
• Git command fails?
• CI environment vars are malformed?

Looking at utils/git.js, the underlying functions return null on failure, but the plugin API doesn't document this behavior.

Recommendation: Either:

• Document that null values are possible in the return object
• Add try/catch and return safe defaults explicitly
• Consider adding a success: boolean field to the return object

3. Silent Peer Dependency Issues

The fallback in plugins (clients/storybook/src/index.js:126-135) silently uses environment variables when services.git?.detect is missing, but there's no warning to users that they should upgrade the CLI.

Recommendation: Add a one-time warning when falling back:

if (services.git?.detect) {
  // Use CLI's git detection
} else {
  logger.warn('⚠️  Using old CLI version. Upgrade to @vizzly-testing/cli@>=0.25.0 for improved git detection.');
  // Fallback...
}

4. Type Safety Concerns

The PR description mentions "peer dep warning" but I don't see where this warning would be shown. The peerDependencies in package.json will cause npm/yarn to warn during install, but:

• Not all package managers enforce peer deps
• Users might miss the warning
• Runtime detection would be more reliable

5. Potential Race Condition

src/plugin-api.js:46-50 uses Promise.all() which is good, but detectPullRequestNumber() at line 51 is synchronous (only reads env vars). This works but looks inconsistent. Consider documenting why it's not included in the Promise.all().

🎯 Suggestions

1. Integration Tests

Since this is cross-package functionality (CLI + plugins), consider adding integration tests that:

• Start a build using the plugin with the new API
• Verify correct git info is captured in the build metadata
• Test in simulated CI environments

2. Documentation

The CLAUDE.md should be updated to mention the new services.git.detect() API in the plugin section:

**Plugin Architecture:**
...
Plugins receive access to:
- `services.git.detect()` - Git detection with CI environment support (v0.25.0+)
- `services.testRunner` - Build lifecycle management
- `services.serverManager` - Server control

3. Consider Async PR Number Detection

Future enhancement: Some CI providers might need async operations to detect PR numbers (e.g., calling GitHub API). Consider making detectPullRequestNumber() async now to avoid breaking change later.

🔒 Security

No security concerns. The changes reduce attack surface by removing dynamic imports and standardizing on official APIs.

🎨 Code Style

Follows project conventions well. Consistent with the service-oriented architecture described in CLAUDE.md.

📊 Performance

No performance concerns. Git detection happens once per build, not in hot paths.

Summary

This is a solid improvement that solves a real problem (fragile dynamic imports breaking git detection). The main blocker is missing test coverage for the new public API. Once tests are added and the error handling is clarified, this should be good to merge.

Recommended actions before merge:

1. Add comprehensive unit tests for services.git.detect()
2. Document null-handling behavior or add explicit error handling
3. Add runtime warning when falling back to old behavior
4. Update CLAUDE.md to document the new plugin API

Approval Status: Request changes (tests required)

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

viewer-zoomed-100 · Firefox · 1920×1080 · 0.1% diff

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

bulk-accept-dialog · Firefox · 375×667 · 159.3% diff

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

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

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

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{feat/plugin-git-api · abbb534f}

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 7 changes need review

Status	Count
Passed	12
Changed	7
Auto-approved	12

Changes needing review (7)

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

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

bulk-accept-dialog · Firefox · 375×667 · 159.3% diff

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

viewer-toggle-mode · Firefox · 1920×1080 · 0.1% diff

filter-failed-only · Firefox · 375×667 · 14.2% diff

...and 1 more in Vizzly.

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{feat/plugin-git-api · bd8371ca}

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - 6 changes need review

Status	Count
Passed	13
Changed	6
Auto-approved	13

Changes needing review (6)

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

viewer-zoomed-100 · Firefox · 1920×1080 · 0.0% diff

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

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

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

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

Review changes

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{feat/plugin-git-api · ce8d33bb}