🐛 Don't fail CI when Vizzly API returns 5xx errors#190
Conversation
When Vizzly's infrastructure has issues (Cloudflare 530, server errors), the CLI now warns the user but doesn't fail their CI. The user's tests still ran - only the visual comparison couldn't happen. - Add status code to API error context for checking - Gracefully handle 5xx errors in run, upload, status, finalize commands - Add dependency injection to status command for testability - Add tests verifying 5xx errors don't exit(1) but 4xx errors still do
This comment has been minimized.
This comment has been minimized.
Code ReviewThis PR addresses a real user pain point by gracefully handling 5xx errors from Vizzly's infrastructure so that users' CI pipelines don't fail due to temporary API unavailability. The implementation is solid overall, with excellent test coverage and consistent patterns across all affected commands. ✅ Strengths
🔍 Code Quality Observations1. Missing test for API client context (Non-blocking)The new Suggested test: it('includes status code in error context', async () => {
let client = createApiClient({
token: 'test-token',
baseUrl: 'https://api.test',
});
mockFetch.mock.mockImplementation(async () => ({
ok: false,
status: 500,
headers: new Map(),
text: async () => 'Internal Server Error',
}));
await assert.rejects(
() => client.request('/api/test'),
error => {
assert.strictEqual(error.context?.status, 500);
return true;
}
);
});2. Edge case: Status 599 (Non-blocking)The condition 3. Type safety consideration (Non-blocking)The pattern 4. Inconsistent return in status.js (Very minor)In 🔒 Security ConsiderationsNo security concerns. The change is purely defensive - gracefully degrading when the API is unavailable. ⚡ Performance ConsiderationsNo performance impact. This only affects the error path. 🧪 Test CoverageExcellent coverage:
Minor gap:
📋 SuggestionsPriority: Low
✨ Overall AssessmentThis is a high-quality PR that solves a real problem with a clean, well-tested implementation. The suggested improvements are minor and don't block merging. The code follows the repository's patterns and conventions nicely. Recommendation: Approve with optional suggestions for follow-up |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
- Add unit tests for error.context.status in API client - Make statusCommand return value consistent with other commands
Vizzly - Visual Test ResultsCLI Reporter - 14 changes need review
Changes needing review (14)fullscreen-viewer · Firefox · 375×667 · 158.5% diff search-no-results · Firefox · 1920×1080 · 0.2% diff filter-failed-only · Firefox · 1920×1080 · 1.2% diff search-homepage · Firefox · 1920×1080 · 0.3% diff viewer-slide-mode · Firefox · 1920×1080 · 0.2% diff viewer-toggle-mode · Firefox · 1920×1080 · 0.7% diff ...and 8 more in Vizzly. CLI TUI - Processing...Build in progress...
|
Vizzly - Visual Test ResultsCLI Reporter - 14 changes need review
Changes needing review (14)fullscreen-viewer · Firefox · 375×667 · 158.5% diff search-no-results · Firefox · 1920×1080 · 0.2% diff filter-failed-only · Firefox · 1920×1080 · 1.2% diff search-homepage · Firefox · 1920×1080 · 0.3% diff viewer-slide-mode · Firefox · 1920×1080 · 0.2% diff viewer-toggle-mode · Firefox · 1920×1080 · 0.7% diff ...and 8 more in Vizzly. CLI TUI - 1 change needs review
Changes needing review (1)vizzly-help · 1202×1430 · 556.8% diff
|
Summary
Motivation
Users were seeing their CI fail with errors like:
This is outside the user's control - their tests ran fine, only the visual comparison couldn't happen because Vizzly's API was temporarily unavailable.
Changes
src/api/client.js- Addstatusto error contextsrc/commands/run.js- Handle 5xx gracefullysrc/commands/upload.js- Handle 5xx gracefullysrc/commands/status.js- Handle 5xx gracefully + add DI for testabilitysrc/commands/finalize.js- Handle 5xx gracefullyTest plan
npm testpasses (1817 tests)npm run lintpasses