🐛 Fix baseline download from builds page using OAuth authentication

[Robdel12]

Summary

• Fix getDefaultBranch is not defined error when downloading baselines from the TDD dashboard builds page
• Enable OAuth-based baseline downloads for users logged in via the dashboard, allowing them to download from any project they have membership to (not just the linked project)
• Add proper dependency injection in downloadBaselines() method
• Move SDK-API.md to docs/internal/ and sync with vizzly repo

Changes

• src/server/routers/baseline.js: Use OAuth via authService.authenticatedRequest() to call the new /api/cli/:projectSlug/builds/:buildId/tdd-baselines endpoint when organizationSlug and projectSlug are provided
• src/tdd/tdd-service.js:
  • Fix dependency injection by destructuring getDefaultBranch, output, colors, etc. from this._deps
  • Add processDownloadedBaselines() method to handle pre-fetched API responses
  • Fix downloadHotspots() dependency injection
• tests/tdd/tdd-service.test.js: Add tests for dependency injection fix
• docs/internal/SDK-API.md: Move from root and update with latest API docs

Test plan

• Run npm test - all 1590 tests pass
• Manual test: Start TDD server, login via dashboard, download baselines from builds page
• Verify OAuth flow works with vizzly PR #174 deployed

Fixes #154

[claude]

Code Review - PR #155

Overview

This PR fixes the getDefaultBranch is not defined error and enables OAuth-based baseline downloads. The implementation is solid overall, with good separation of concerns and proper dependency injection fixes.

Positive Aspects ✅

1. Proper Dependency Injection: The fix for getDefaultBranch and other dependencies in downloadBaselines() and downloadHotspots() methods correctly uses destructuring from this._deps. This follows the established DI pattern in the codebase.

2. OAuth Integration: The fallback strategy (OAuth first, then token-based) is well-designed and allows users to download from any project they have access to, not just linked projects.

3. Code Reuse: The processDownloadedBaselines() method properly extracts the common baseline processing logic, avoiding duplication.

4. Test Coverage: Good unit tests for the dependency injection fix (lines 1041-1093 in tdd-service.test.js).

5. Documentation: The SDK-API.md updates are comprehensive and well-organized.

Issues and Concerns

1. Missing Test Coverage for OAuth Flow ⚠️

The new OAuth authentication path in baseline.js:182-216 has no tests. Consider adding:

• Test for successful OAuth download with organizationSlug and projectSlug
• Test for OAuth fallback when auth fails with 401
• Test for OAuth errors that should NOT fallback (non-auth errors)

// Example test to add
it('downloads baselines via OAuth when organizationSlug and projectSlug provided', async () => {
  let authServiceCalled = false;
  let mockAuthService = {
    authenticatedRequest: async (path, options) => {
      authServiceCalled = true;
      return { build: { id: 'build-123', status: 'completed' }, screenshots: [] };
    }
  };
  
  let handler = createBaselineRouter({
    screenshotHandler: createMockScreenshotHandler(),
    tddService: createMockTddService({ processDownloadedBaselines: true }),
    authService: mockAuthService,
  });
  
  let req = createMockRequest('POST', { 
    buildId: 'build-123',
    organizationSlug: 'my-org',
    projectSlug: 'my-project'
  });
  let res = createMockResponse();
  
  await handler(req, res, '/api/baselines/download');
  
  assert.ok(authServiceCalled, 'Should use OAuth');
  assert.strictEqual(res.statusCode, 200);
});

2. Error Handling Logic Issue 🐛

Lines 217-224 in baseline.js have a potential bug:

if (
  !oauthError.message?.includes('Not authenticated') &&
  !oauthError.message?.includes('401')
) {
  throw oauthError;
}

This logic is inverted. It should throw when the error IS an auth error, not when it isn't. The comment says "fall through to other methods" for auth errors, but the code does the opposite.

Recommendation: Fix the logic:

// If OAuth fails with NON-auth error, throw it (don't fall through)
// Only fall through for auth errors (401, not authenticated)
if (
  oauthError.message?.includes('Not authenticated') ||
  oauthError.message?.includes('401')
) {
  output.debug('baseline', `OAuth auth failed, trying other methods: ${oauthError.message}`);
} else {
  // Non-auth error (network, API error, etc.) - don't fall through
  throw oauthError;
}

3. Incomplete Code Duplication 🔄

The processDownloadedBaselines() method (lines 701-973) duplicates ~270 lines from downloadBaselines(). This is substantial duplication. While the PR description mentions this is for separation of auth concerns, consider:

• Extract shared logic into smaller helper methods (e.g., _downloadScreenshotBatch(), _validateBaselineBuild())
• Both methods could call these helpers to reduce duplication
• Current approach makes maintenance harder (bug fixes need to be applied in two places)

4. Hotspot Download Skipped Silently ℹ️

Line 932 in processDownloadedBaselines() has a comment:

// Download hotspots (skip if no API key - hotspots require separate auth)
// Note: When using project token, hotspots won't be downloaded
// This is acceptable as hotspots are optional noise filtering

But there's no actual call to downloadHotspots() like there is in downloadBaselines() (line 640). This means OAuth users won't get hotspot data at all, even if they have a token. Consider:

• Try downloading hotspots if this.config.apiKey exists
• Add a comment explaining why OAuth path can't download hotspots

5. Minor: Inconsistent Variable Names

• Line 751: buildDetails is assigned from baselineBuild but they're the same object
• This adds confusion - just use baselineBuild throughout

6. Security: Error Message Exposure 🔒

Lines 199-202 expose the full error in the thrown message:

if (!apiResponse) {
  throw new Error(`Build ${buildId} not found or API returned null`);
}

Consider whether this exposes internal API details. If the build doesn't exist or user lacks access, the error should be user-friendly.

Performance Considerations

1. Batch Processing: Good use of batching (line 781, batchSize=5) to avoid overwhelming the network
2. SHA Deduplication: Excellent optimization to skip re-downloading unchanged files (lines 807-814)

Documentation

The SDK-API.md changes are excellent and comprehensive. The hotspot analysis documentation is particularly well-written.

Recommendations

Must Fix Before Merge:

1. Fix the inverted error handling logic in baseline.js:219-224

Strongly Recommended:
2. Add test coverage for OAuth flow
3. Address the code duplication with helper methods

Nice to Have:
4. Consider supporting hotspot download for OAuth users
5. Simplify variable naming in processDownloadedBaselines()

Summary

This is a solid fix that addresses the immediate issue and adds valuable OAuth functionality. The main concern is the inverted error handling logic which could cause non-auth errors to be silently swallowed. Once that's fixed and tests are added, this will be ready to merge.

Approval Status: Conditionally approved pending fix of error handling logic.