chore(core): fix connection tests to support sse

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 18, 2025 6:09am

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(core): fix missing transport for inline tests #1287 : 2 dependent PRs (#1250 , #1289 )
• chore(core): fix test for sending message to specific connection sse race condition #1286
• fix(cloudflare-workers): fix alarms #1285
• chore(core): fix broadcast race condition #1284
• chore: list sideEffects #1283
• chore(core): fix connection tests to support sse #1282 👈 (View in Graphite)
• fix(cloudflare-workers): fix aborts on sse streams on cloudflare workers #1281
• fix(cloudflare-workers): fix aborting sse streams #1280
• chore: remove unused FakeEventSource #1279
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: Fix Connection Tests to Support SSE

I've reviewed this pull request that addresses SSE (Server-Sent Events) connection test reliability issues. The changes demonstrate good awareness of SSE timing characteristics and implement appropriate test robustness improvements.

Tasks

• Read repository guidelines (CLAUDE.md)
• Analyze changed files for SSE connection test fixes
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage improvements
• Provide comprehensive feedback

---

✅ Strengths

Improved Test Reliability

• Excellent addition of vi.waitFor() wrappers around event assertions that previously had race conditions
• The SSE-specific timeout increase (10 seconds) for Cloudflare Workers shows platform-aware testing
• Good use of TODO comments documenting known SSE race conditions

Enhanced Test Utilities

• Addition of setCount action in counter.ts:11-16 provides deterministic state setting, which is better for testing than increment-based logic
• Proper cleanup with await connection.dispose() maintained throughout

---

⚠️ Areas for Improvement

Code Quality & Best Practices

1. Inconsistent Array Assertions (actor-conn.ts:96,106,133,160)

   // Current - less reliable
   expect(receivedEvents).includes(1);
   
   // Better - use proper jest matchers
   expect(receivedEvents).toContain(1);

   The .includes() method doesn't provide clear test failure messages and isn't idiomatic for test assertions.

2. TODO Comments Need Tracking (actor-conn.ts:92,181)

   • Race condition TODOs should have GitHub issue references for tracking resolution
   • Consider if these can be mitigated further with current changes

Test Structure

3. Magic Numbers (actor-conn-state.ts:182,277)

   // Consider extracting to constants
   const SSE_DISCONNECT_TIMEOUT_CF = 10_000;
   const SSE_POLLING_INTERVAL = 100;

4. Event Race Condition Pattern

   • The pattern in actor-conn.ts:93-97 could be extracted to a helper function since it's used multiple times:

   async function waitForSSEConnection(connection, expectedEvent) {
     await vi.waitFor(async () => {
       await connection.setCount(expectedEvent);
       expect(receivedEvents).toContain(expectedEvent);
     });
   }

---

🔒 Security Assessment

• ✅ No security concerns identified - Changes are limited to test improvements
• Test timeouts are reasonable and won't cause resource exhaustion

---

⚡ Performance Considerations

• ✅ Good: Increased timeouts are SSE-specific and documented
• ✅ Good: Test intervals (100ms) are reasonable for avoiding CPU spinning
• The changes appropriately balance test reliability vs execution time

---

🧪 Test Coverage Analysis

• ✅ Excellent: Tests now properly handle SSE timing characteristics
• ✅ Good: Connection lifecycle edge cases are better covered
• Suggestion: Consider adding a test that explicitly validates SSE ping behavior using the SSE_PING_INTERVAL constant

---

📋 Recommended Actions

1. High Priority: Replace .includes() with .toContain() for better test assertions
2. Medium Priority: Extract SSE timeout constants to reduce magic numbers
3. Low Priority: Consider creating helper functions for common SSE test patterns

This is a solid improvement that addresses real SSE timing issues in the test suite. The changes show good understanding of the underlying technology constraints while maintaining test reliability.

---

Job Run

[pkg-pr-new]

More templates

• example-ai-agent
• example-better-auth-external-db
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• counter
• example-crdt
• example-database
• example-sqlite
• example-elysia
• example-express
• example-game
• example-hono
• example-hono-react
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1282

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1282

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1282

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1282

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1282

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1282

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1282

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1282

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1282

commit: 082fdc5

[graphite-app]

Merge activity

• Sep 24, 6:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 24, 6:07 AM UTC: CI is running for this pull request on a draft pull request (#1294) due to your merge queue CI optimization settings.
• Sep 24, 6:08 AM UTC: Merged by the Graphite merge queue via draft PR: #1294.