fix: frontend tests added #121
Conversation
…onResponse directly
📝 WalkthroughWalkthroughReplaces SSENotificationEvent/SSENotificationEventData with NotificationResponse across backend and frontend, simplifies backend notification SSE flow (removes handshake/heartbeat, uses auth dependency, adds ping=30), introduces QueuePriority enum, and updates generated types and frontend parsing/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as EventSource (browser)
participant Route as /notifications/stream (FastAPI)
participant SSE as SSEService
participant Redis as Redis Pub/Sub
participant Auth as AuthService
Client->>Route: GET /notifications/stream (EventSource)
Route->>Auth: resolve auth_service -> current_user
Route->>SSE: start notification_stream(current_user)
SSE->>Redis: subscribe to user's notification channel
Redis-->>SSE: RedisNotificationMessage
SSE->>Route: yield SSE event { event: "notification", data: NotificationResponse }
Route-->>Client: EventSource stream ping=30 / message event "notification"
Client->>Client: parse NotificationResponse and invoke callbacks (and optional Browser Notification)
Note right of SSE: logs open/close and ignores missing messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/lib/__tests__/formatters.test.ts">
<violation number="1" location="frontend/src/lib/__tests__/formatters.test.ts:43">
P3: This assertion hard-codes a Latin-digit year, but formatTimestamp uses `toLocaleString` with the default locale. In locales with non-Latin numerals, the output won’t contain "2025", so the test can fail depending on environment locale. Consider comparing against Intl.DateTimeFormat output for the current locale to keep the test deterministic.</violation>
<violation number="2" location="frontend/src/lib/__tests__/formatters.test.ts:149">
P3: These expectations assume western digit grouping and Latin numerals, but formatNumber uses `toLocaleString()` with the environment’s locale. In locales with different digit groupings or numerals, the substrings won’t appear and the test becomes flaky. Consider comparing to `Intl.NumberFormat(undefined)` output instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| it('formats with locale separators', () => { | ||
| const result = formatNumber(1234567); | ||
| expect(result).toContain('1'); |
There was a problem hiding this comment.
P3: These expectations assume western digit grouping and Latin numerals, but formatNumber uses toLocaleString() with the environment’s locale. In locales with different digit groupings or numerals, the substrings won’t appear and the test becomes flaky. Consider comparing to Intl.NumberFormat(undefined) output instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/__tests__/formatters.test.ts, line 149:
<comment>These expectations assume western digit grouping and Latin numerals, but formatNumber uses `toLocaleString()` with the environment’s locale. In locales with different digit groupings or numerals, the substrings won’t appear and the test becomes flaky. Consider comparing to `Intl.NumberFormat(undefined)` output instead.</comment>
<file context>
@@ -0,0 +1,178 @@
+
+ it('formats with locale separators', () => {
+ const result = formatNumber(1234567);
+ expect(result).toContain('1');
+ expect(result).toContain('234');
+ expect(result).toContain('567');
</file context>
| }); | ||
|
|
||
| it('accepts custom Intl options', () => { | ||
| expect(formatTimestamp(new Date(2025, 0, 15).toISOString(), { year: 'numeric' })).toContain('2025'); |
There was a problem hiding this comment.
P3: This assertion hard-codes a Latin-digit year, but formatTimestamp uses toLocaleString with the default locale. In locales with non-Latin numerals, the output won’t contain "2025", so the test can fail depending on environment locale. Consider comparing against Intl.DateTimeFormat output for the current locale to keep the test deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/__tests__/formatters.test.ts, line 43:
<comment>This assertion hard-codes a Latin-digit year, but formatTimestamp uses `toLocaleString` with the default locale. In locales with non-Latin numerals, the output won’t contain "2025", so the test can fail depending on environment locale. Consider comparing against Intl.DateTimeFormat output for the current locale to keep the test deterministic.</comment>
<file context>
@@ -0,0 +1,178 @@
+ });
+
+ it('accepts custom Intl options', () => {
+ expect(formatTimestamp(new Date(2025, 0, 15).toISOString(), { year: 'numeric' })).toContain('2025');
+ });
+
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/services/sse/sse_service.py`:
- Around line 202-226: The notification loop in sse_service.py is vulnerable to
malformed Redis messages causing a validation/serialization exception to bubble
out and terminate the SSE stream; wrap the per-message processing that converts
RedisNotificationMessage into NotificationResponse (the block after await
subscription.get(RedisNotificationMessage) through yield {"event":
"notification", ...}) in a try/except that catches serialization/validation
errors (and general exceptions), logs the error with context (user_id and raw
redis_msg), and continues the loop so the subscription remains open; keep the
existing finally that calls asyncio.shield(subscription.close()) and logs
"Notification stream closed".
In `@frontend/src/lib/__tests__/formatters.test.ts`:
- Around line 33-48: Tests in formatters.test.ts are brittle because they assume
US-style output; update the assertions for formatTimestamp and formatNumber to
compare against the actual Intl output for the current runtime locale instead of
hard-coded substrings. For formatTimestamp tests, build an expected string via
new Intl.DateTimeFormat(undefined, <same options passed to formatTimestamp>)
formatted with the same Date/ISO input and assert equality (or strict
containment of that result); for formatNumber tests, use new
Intl.NumberFormat(undefined, <same options>) to produce the expected string for
the number and assert equality. Ensure you reference the same options used by
formatTimestamp and formatNumber so the tests are deterministic across locales.
🧹 Nitpick comments (2)
frontend/src/lib/notifications/__tests__/stream.test.ts (2)
129-139: Consider moving global stub cleanup toafterEachfor robustness.If an assertion fails before
vi.unstubAllGlobals()on line 138, the global stub could theoretically leak to subsequent tests. While Vitest typically isolates tests well, moving cleanup toafterEachwould make this test more resilient.♻️ Suggested cleanup pattern
describe('NotificationStream', () => { let callback: ReturnType<typeof vi.fn>; beforeEach(() => { callback = vi.fn(); listenCallbacks = {}; mockAbort.mockClear(); notificationStream.disconnect(); }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); // ... other tests ... it('fires browser Notification when permission is granted', () => { const MockNotification = vi.fn(); Object.defineProperty(MockNotification, 'permission', { value: 'granted' }); vi.stubGlobal('Notification', MockNotification); notificationStream.connect(callback); listenCallbacks.onMessage!({ event: 'notification', data: JSON.stringify(validPayload()) }); expect(MockNotification).toHaveBeenCalledWith('Test', { body: 'Hello', icon: '/favicon.png' }); - vi.unstubAllGlobals(); }); });
113-119: Consider whether non-401 errors should set an error message for better UX.The implementation intentionally skips setting an error message for non-401 response errors (e.g., 500)—only 401 errors are assigned
error = 'Unauthorized'. This test correctly validates that behavior. However, the design choice to leaveerrornull for server errors means users get no visible feedback when the connection fails due to a 500 or similar error, only a silent disconnection. If user feedback is important for these errors, consider setting an error message or log to help diagnose connection issues.
| describe('formatTimestamp', () => { | ||
| it.each([null, undefined, 'garbage'] as const)('returns N/A for %j', (input) => { | ||
| expect(formatTimestamp(input as string | null | undefined)).toBe('N/A'); | ||
| }); | ||
|
|
||
| it('formats ISO string', () => { | ||
| expect(formatTimestamp(new Date(2025, 0, 15).toISOString())).not.toBe('N/A'); | ||
| }); | ||
|
|
||
| it('accepts custom Intl options', () => { | ||
| expect(formatTimestamp(new Date(2025, 0, 15).toISOString(), { year: 'numeric' })).toContain('2025'); | ||
| }); | ||
|
|
||
| it('accepts Date object', () => { | ||
| expect(formatTimestamp(new Date(2025, 0, 15))).not.toBe('N/A'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for locale/timezone pinning in test setup files
find . -type f \( -name "vitest.config.*" -o -name "setup*.ts" -o -name "setupTests.*" \) | head -20Repository: HardMax71/Integr8sCode
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for locale/timezone/Intl configuration in test-related files
rg -n --type=ts 'process\.env\.(TZ|LANG|LC_ALL)|Intl\.(DateTimeFormat|NumberFormat)' --glob='**/vitest*.ts' --glob='**/setup*.ts' --glob='**/test*.ts' 2>/dev/null | head -30Repository: HardMax71/Integr8sCode
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Examine the actual test file
wc -l frontend/src/lib/__tests__/formatters.test.tsRepository: HardMax71/Integr8sCode
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the relevant test file sections
cat -n frontend/src/lib/__tests__/formatters.test.ts | sed -n '1,60p'Repository: HardMax71/Integr8sCode
Length of output: 2272
🏁 Script executed:
#!/bin/bash
# Look for the formatNumber test at lines 147-152
cat -n frontend/src/lib/__tests__/formatters.test.ts | sed -n '140,160p'Repository: HardMax71/Integr8sCode
Length of output: 758
🏁 Script executed:
#!/bin/bash
# Find and examine the formatTimestamp and formatNumber implementations
fd -t f 'formatters\.ts' --exclude='*.test.ts' | head -5Repository: HardMax71/Integr8sCode
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check vitest configuration
cat frontend/vitest.config.tsRepository: HardMax71/Integr8sCode
Length of output: 1005
🏁 Script executed:
#!/bin/bash
# Examine the formatters implementation
cat -n frontend/src/lib/formatters.tsRepository: HardMax71/Integr8sCode
Length of output: 6310
🏁 Script executed:
#!/bin/bash
# Check vitest setup file for locale pinning
cat frontend/vitest.setup.ts 2>/dev/null || echo "File not found"Repository: HardMax71/Integr8sCode
Length of output: 2607
Make locale-dependent assertions deterministic.
Both formatTimestamp() and formatNumber() use toLocaleString() for locale-aware formatting, but the tests use brittle assertions that don't validate actual behavior. With no locale pinned in test setup, these tests will fail on non-en-US locales.
For line 43, toContain('2025') only checks substring presence—the function might include month/day depending on locale (e.g., ja-JP: "2025年1月15日"). For lines 147-152, toContain('1'), toContain('234'), toContain('567') assume digit sequences appear consecutively, but locale-specific grouping (de-DE uses "1.234.567", fr-FR uses "1 234 567") may break these assertions.
Compare against actual Intl output for the runtime locale instead:
🔧 Proposed test hardening
it('accepts custom Intl options', () => {
- expect(formatTimestamp(new Date(2025, 0, 15).toISOString(), { year: 'numeric' })).toContain('2025');
+ const date = new Date(2025, 0, 15);
+ const expected = date.toLocaleString(undefined, { year: 'numeric' });
+ expect(formatTimestamp(date, { year: 'numeric' })).toBe(expected);
});
it('formats with locale separators', () => {
- const result = formatNumber(1234567);
- expect(result).toContain('1');
- expect(result).toContain('234');
- expect(result).toContain('567');
+ const expected = new Intl.NumberFormat().format(1234567);
+ expect(formatNumber(1234567)).toBe(expected);
});🤖 Prompt for AI Agents
In `@frontend/src/lib/__tests__/formatters.test.ts` around lines 33 - 48, Tests in
formatters.test.ts are brittle because they assume US-style output; update the
assertions for formatTimestamp and formatNumber to compare against the actual
Intl output for the current runtime locale instead of hard-coded substrings. For
formatTimestamp tests, build an expected string via new
Intl.DateTimeFormat(undefined, <same options passed to formatTimestamp>)
formatted with the same Date/ISO input and assert equality (or strict
containment of that result); for formatNumber tests, use new
Intl.NumberFormat(undefined, <same options>) to produce the expected string for
the number and assert equality. Ensure you reference the same options used by
formatTimestamp and formatNumber so the tests are deterministic across locales.
|
Also fixing parts of backend if there's shitshow in codebase
Summary by cubic
Simplified the SSE notifications stream to emit plain NotificationResponse events and added frontend unit tests for interceptors, formatters, and the notification stream. Also updated OpenAPI and generated SDK types (including QueuePriority) and enabled ping keepalive for more stable connections.
Refactors
New Features
Written for commit f4ad102. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.