diff --git a/.changeset/fix-settimeout-binding.md b/.changeset/fix-settimeout-binding.md new file mode 100644 index 000000000..40f78abc3 --- /dev/null +++ b/.changeset/fix-settimeout-binding.md @@ -0,0 +1,5 @@ +--- +'@pgflow/client': patch +--- + +Fix setTimeout context binding issue in SupabaseBroadcastAdapter for browser compatibility diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 33f4eebaf..000000000 --- a/PLAN.md +++ /dev/null @@ -1,81 +0,0 @@ -# Plan: Complete pgmq 1.5.0+ Upgrade Documentation and Communication - -## Completed Tasks - -✅ Core migration changes with compatibility check -✅ Updated `set_vt_batch` to use RETURNS TABLE (future-proof) -✅ Added optional `headers` field to TypeScript `PgmqMessageRecord` -✅ Updated all test mock messages -✅ Created changeset with breaking change warning -✅ Manual testing verified migration fails gracefully on pgmq 1.4.4 - -## Remaining Tasks - -### 1. Create News Article - -**File:** `pkgs/website/src/content/docs/news/pgmq-1-5-0-upgrade.mdx` (or similar) - -Create a news article announcing: -- pgflow 0.8.0 requires pgmq 1.5.0+ -- Breaking change details -- Migration instructions -- Benefits of the upgrade (future-proofing against pgmq changes) - -### 2. Update Supabase CLI Version Requirements in Docs - -**Files to review and update:** -- `pkgs/website/src/content/docs/get-started/installation.mdx` -- Other getting started guides -- Any tutorial pages mentioning Supabase CLI version - -**Action:** Update minimum Supabase CLI version requirement to the version that includes pgmq 1.5.0+ - -### 3. Update READMEs - -**Files to review and update:** -- Root `README.md` -- `pkgs/core/README.md` -- `pkgs/edge-worker/README.md` -- `pkgs/cli/README.md` -- Any other package READMEs mentioning Supabase versions - -**Action:** Ensure all READMEs mention the pgmq 1.5.0+ requirement - -### 4. Improve Update pgflow Docs Page - -**File:** Look for existing update/upgrade documentation page - -**Actions:** -- Add section about breaking changes in 0.8.0 -- Document migration path from 0.7.x to 0.8.0 -- Include pgmq version check instructions -- Add troubleshooting section for migration failures - -### 5. Review All Docs Pages for Version References - -**Action:** Comprehensive audit of all documentation for: -- Outdated Supabase CLI version numbers -- Missing pgmq version requirements -- Installation/setup instructions that need updating -- Migration guides that need breaking change warnings - -**Files to check:** -- All files in `pkgs/website/src/content/docs/` -- All READMEs across packages -- Any deployment guides -- Troubleshooting pages - -## Testing Checklist - -After documentation updates: -- [ ] Build website locally and verify all pages render correctly -- [ ] Check all internal links still work -- [ ] Verify code examples are still accurate -- [ ] Review for consistency in version numbering - -## Notes - -- Keep documentation aligned with MVP philosophy (concise, clear, actionable) -- Follow Diataxis framework for documentation organization -- Use clear warnings for breaking changes -- Provide migration instructions, not just "upgrade" diff --git a/PLAN_browsertests.md b/PLAN_browsertests.md new file mode 100644 index 000000000..515dfcb96 --- /dev/null +++ b/PLAN_browsertests.md @@ -0,0 +1,283 @@ +# Plan: Browser Testing for @pgflow/client + +## Context + +The setTimeout binding bug (fixed in this PR) was not caught by existing tests because: + +1. **Unit tests use fake timers** - Vitest's `vi.useFakeTimers()` mocks setTimeout, hiding context binding issues +2. **Integration tests run in Node.js** - Node.js is permissive with function context, browsers are strict +3. **No browser-environment testing** - All tests run in Node.js, never in actual browser environments + +The bug only manifests in real browsers when `setTimeout` loses its `window`/`globalThis` context. + +## Goal + +Add browser-based testing to catch browser-specific issues like: +- Function context binding (setTimeout, setInterval, requestAnimationFrame) +- DOM API compatibility +- Web API behavior (Fetch, WebSocket, etc.) +- Browser-specific quirks + +## Solution: Vitest Browser Mode + +Use Vitest's built-in browser mode to run tests in real browsers (Chromium, Firefox, Safari). + +### Why Vitest Browser Mode? + +- ✅ Runs tests in **actual browsers**, not Node.js +- ✅ Uses Playwright/WebdriverIO under the hood +- ✅ Integrates seamlessly with existing Vitest setup +- ✅ Supports parallel execution with Node.js tests +- ✅ CI/CD friendly (headless mode) +- ✅ No separate test framework needed + +## Implementation Plan + +### Phase 1: Setup Infrastructure + +**1. Install dependencies** +```bash +pnpm add -D @vitest/browser playwright +``` + +**2. Update vitest.workspace.ts** + +Add browser test configuration alongside existing Node.js config: + +```typescript +// vitest.workspace.ts +import { defineWorkspace } from 'vitest/config' + +export default defineWorkspace([ + './vitest.config.ts', // existing Node.js tests + { + test: { + name: 'browser', + include: [ + 'pkgs/**/*.browser.test.ts', + 'apps/**/*.browser.test.ts', + ], + browser: { + enabled: true, + provider: 'playwright', + instances: [ + { browser: 'chromium' }, + ], + headless: true, // CI-friendly + }, + }, + }, +]) +``` + +**3. Add npm scripts** + +```json +{ + "scripts": { + "test:browser": "vitest --workspace --project browser", + "test:browser:ui": "vitest --workspace --project browser --ui", + "test:all": "vitest --workspace" + } +} +``` + +### Phase 2: Migrate setTimeout Binding Test + +**1. Rename test file** +``` +pkgs/client/__tests__/SupabaseBroadcastAdapter.setTimeout-binding.test.ts +→ +pkgs/client/__tests__/SupabaseBroadcastAdapter.setTimeout-binding.browser.test.ts +``` + +**2. Simplify test** (no mocking needed in browser) + +```typescript +import { describe, it, expect } from 'vitest' +import { SupabaseBroadcastAdapter } from '../src/lib/SupabaseBroadcastAdapter.js' +import { createClient } from '@supabase/supabase-js' + +describe('SupabaseBroadcastAdapter - Browser Environment', () => { + const supabaseUrl = 'https://test.supabase.co' + const supabaseKey = 'test-key' + + it('should handle setTimeout without losing context', async () => { + // In a real browser, if setTimeout binding is broken, this will throw: + // "TypeError: 'setTimeout' called on an object that does not implement interface Window" + + const supabase = createClient(supabaseUrl, supabaseKey) + const adapter = new SupabaseBroadcastAdapter(supabase, { + reconnectDelayMs: 100, + stabilizationDelayMs: 100, + }) + + const mockChannel = { + on: vi.fn().mockReturnThis(), + subscribe: vi.fn((callback) => { + setTimeout(() => callback('SUBSCRIBED'), 10) + return mockChannel + }), + unsubscribe: vi.fn(), + } + + supabase.channel = vi.fn().mockReturnValue(mockChannel) + + // This should work without throwing + await expect( + adapter.subscribeToRun('test-run-id') + ).resolves.toBeDefined() + }) +}) +``` + +### Phase 3: Identify Other Browser-Critical Tests + +Tests that should run in browser mode: + +**From `@pgflow/client`:** +- SupabaseBroadcastAdapter (realtime subscriptions, setTimeout/setInterval usage) +- PgflowClient (browser-specific Supabase client behavior) +- Any code using Web APIs (Fetch, WebSocket, localStorage, etc.) + +**From `apps/demo`:** +- Svelte component rendering +- User interactions (clicks, form submissions) +- Realtime updates in the UI + +### Phase 4: CI/CD Integration + +**Update `.github/workflows/ci.yml`:** + +```yaml +- name: Run browser tests + run: pnpm nx affected -t test:browser --base=$NX_BASE --head=$NX_HEAD +``` + +**Playwright installation** (for CI): +```yaml +- name: Install Playwright browsers + run: pnpm exec playwright install chromium +``` + +### Phase 5: Documentation + +**Update `pkgs/client/README.md`:** + +```markdown +## Testing + +### Unit Tests (Node.js) +```bash +pnpm nx test client +``` + +### Browser Tests +```bash +pnpm nx test:browser client +``` + +Browser tests run in real browsers to catch browser-specific issues +like function context binding, DOM API compatibility, etc. +``` + +## Test Coverage Strategy + +### Node.js Tests (Current) +- Fast execution +- Mock-heavy +- Business logic +- Integration with database +- Most existing tests stay here + +### Browser Tests (New) +- Slower execution +- Minimal mocking +- Browser-specific behavior +- Web API compatibility +- DOM interactions +- Context binding issues + +## Migration Strategy + +**DO NOT migrate all tests to browser mode.** + +Only migrate/create browser tests for: +1. Browser-specific bugs (like setTimeout binding) +2. DOM interactions +3. Web API usage +4. Visual/rendering tests (if needed in future) + +Most tests should remain in Node.js for speed. + +## Future Enhancements + +**1. Multi-browser testing** +```typescript +instances: [ + { browser: 'chromium' }, + { browser: 'firefox' }, + { browser: 'webkit' }, // Safari +], +``` + +**2. Visual regression testing** +```typescript +import { page } from '@vitest/browser/context' + +it('renders correctly', async () => { + await page.screenshot({ path: 'snapshot.png' }) +}) +``` + +**3. Performance testing** +```typescript +it('loads within 2 seconds', async () => { + const start = performance.now() + await adapter.subscribeToRun('test-run-id') + const duration = performance.now() - start + expect(duration).toBeLessThan(2000) +}) +``` + +## Success Criteria + +- [ ] Vitest browser mode configured and working +- [ ] setTimeout binding test runs in real browser +- [ ] CI/CD runs browser tests in headless mode +- [ ] Documentation updated +- [ ] Team knows when to write browser vs Node.js tests + +## Risks & Mitigations + +**Risk:** Browser tests are slower than Node.js tests +**Mitigation:** Only use browser mode for browser-specific tests, keep most tests in Node.js + +**Risk:** CI/CD needs Playwright browsers installed +**Mitigation:** Add `playwright install` step to CI workflow + +**Risk:** Flaky tests in browser environments +**Mitigation:** Use Vitest's built-in retry mechanism, proper waits/expectations + +## References + +- [Vitest Browser Mode Guide](https://vitest.dev/guide/browser/) +- [Vitest Browser Mode Discussion](https://github.com/vitest-dev/vitest/discussions/5828) +- [Playwright Provider](https://github.com/vitest-dev/vitest/tree/main/packages/browser) + +## Timeline + +**Estimated effort:** 4-8 hours + +- Phase 1 (Setup): 1-2 hours +- Phase 2 (Migrate test): 1 hour +- Phase 3 (Identify tests): 1-2 hours +- Phase 4 (CI/CD): 1-2 hours +- Phase 5 (Docs): 1 hour + +## Open Questions + +1. Should we run browser tests on every PR or only on main? +2. Which browsers should we support in CI? (Chromium only vs all three) +3. Do we need visual regression testing for the demo app? +4. Should browser tests be required to pass for PRs to merge? diff --git a/pkgs/client/__tests__/SupabaseBroadcastAdapter.setTimeout-binding.test.ts b/pkgs/client/__tests__/SupabaseBroadcastAdapter.setTimeout-binding.test.ts new file mode 100644 index 000000000..cbb0ec85a --- /dev/null +++ b/pkgs/client/__tests__/SupabaseBroadcastAdapter.setTimeout-binding.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { SupabaseBroadcastAdapter } from '../src/lib/SupabaseBroadcastAdapter.js'; +import { createClient } from '@supabase/supabase-js'; + +describe('SupabaseBroadcastAdapter - setTimeout binding issue', () => { + const supabaseUrl = 'https://test.supabase.co'; + const supabaseKey = 'test-key'; + + it('should handle setTimeout without losing context (browser environment)', async () => { + // Simulate browser environment where setTimeout can lose context when assigned + const originalSetTimeout = globalThis.setTimeout; + + // This simulates what happens in browsers - setTimeout needs to be called + // with the correct context (window/globalThis) + const unboundSetTimeout = function(this: unknown, callback: () => void, delay: number) { + // If 'this' is not the global object, it will fail in strict mode browsers + if (this !== globalThis && this !== undefined) { + throw new TypeError("'setTimeout' called on an object that does not implement interface Window."); + } + return originalSetTimeout(callback, delay); + }; + + // Replace setTimeout temporarily to simulate browser behavior + const setTimeoutSpy = vi.fn(unboundSetTimeout); + globalThis.setTimeout = setTimeoutSpy as unknown as typeof setTimeout; + + try { + // Create adapter - this will assign setTimeout to #schedule + const supabase = createClient(supabaseUrl, supabaseKey); + const adapter = new SupabaseBroadcastAdapter(supabase, { + reconnectDelayMs: 100, + stabilizationDelayMs: 100, + }); + + // Create a mock channel that triggers error callback + const mockChannel = { + on: vi.fn().mockReturnThis(), + subscribe: vi.fn((callback: (status: string) => void) => { + // Trigger error to invoke #handleChannelError which uses this.#schedule + setTimeout(() => { + const errorHandler = mockChannel.on.mock.calls.find( + (call) => call[0] === 'system' && call[1]?.event === 'error' + )?.[2]; + if (errorHandler) { + errorHandler({ error: new Error('Test error') }); + } + }, 10); + + // Also trigger subscribed + setTimeout(() => callback('SUBSCRIBED'), 10); + return mockChannel; + }), + unsubscribe: vi.fn(), + }; + + // Mock supabase.channel to return our mock + supabase.channel = vi.fn().mockReturnValue(mockChannel); + + // This should work without throwing "setTimeout called on an object..." + await expect( + adapter.subscribeToRun('test-run-id') + ).resolves.toBeDefined(); + + // Verify setTimeout was called (should be called for stabilization delay) + expect(setTimeoutSpy).toHaveBeenCalled(); + } finally { + // Restore original setTimeout + globalThis.setTimeout = originalSetTimeout; + } + }); + + it('should work with custom schedule function', async () => { + const customSchedule = vi.fn((callback: () => void, delay: number) => { + return setTimeout(callback, delay); + }); + + const supabase = createClient(supabaseUrl, supabaseKey); + const adapter = new SupabaseBroadcastAdapter(supabase, { + schedule: customSchedule as unknown as typeof setTimeout, + stabilizationDelayMs: 10, + }); + + const mockChannel = { + on: vi.fn().mockReturnThis(), + subscribe: vi.fn((callback: (status: string) => void) => { + setTimeout(() => callback('SUBSCRIBED'), 5); + return mockChannel; + }), + unsubscribe: vi.fn(), + }; + + supabase.channel = vi.fn().mockReturnValue(mockChannel); + + await adapter.subscribeToRun('test-run-id'); + + // Custom schedule should be used + expect(customSchedule).toHaveBeenCalled(); + }); +}); diff --git a/pkgs/client/src/lib/SupabaseBroadcastAdapter.ts b/pkgs/client/src/lib/SupabaseBroadcastAdapter.ts index 711a586f4..73bfc94ae 100644 --- a/pkgs/client/src/lib/SupabaseBroadcastAdapter.ts +++ b/pkgs/client/src/lib/SupabaseBroadcastAdapter.ts @@ -42,7 +42,7 @@ export class SupabaseBroadcastAdapter implements IFlowRealtime { this.#supabase = supabase; this.#reconnectionDelay = opts.reconnectDelayMs ?? 2000; this.#stabilizationDelay = opts.stabilizationDelayMs ?? 300; - this.#schedule = opts.schedule ?? setTimeout; + this.#schedule = opts.schedule ?? setTimeout.bind(globalThis); } /** diff --git a/pkgs/website/src/content/docs/news/pgflow-0-8-1-resolve-client-settimeout-binding-issue-in-browsers.mdx b/pkgs/website/src/content/docs/news/pgflow-0-8-1-resolve-client-settimeout-binding-issue-in-browsers.mdx new file mode 100644 index 000000000..959bbead5 --- /dev/null +++ b/pkgs/website/src/content/docs/news/pgflow-0-8-1-resolve-client-settimeout-binding-issue-in-browsers.mdx @@ -0,0 +1,91 @@ +--- +draft: false +title: 'pgflow 0.8.1: Resolve Client setTimeout Binding Issue in Browsers' +description: 'Fix setTimeout context binding bug affecting browser environments since client library introduction' +date: 2025-11-19 +authors: + - jumski +tags: + - bugfix + - client + - browser +featured: true +--- + +import { Aside } from "@astrojs/starlight/components"; + +This release fixes a setTimeout context binding issue in `@pgflow/client` that caused errors in browser environments when subscribing to flow runs or handling connection errors. + +## What's Fixed + +- **Browser environment crash**: Fixed `TypeError: 'setTimeout' called on an object that does not implement interface Window` when calling `subscribeToRun()` or during channel reconnection +- **Context binding**: setTimeout now properly maintains its context when called through the internal scheduler + +The bug was present since the client library was first introduced in June 2025 (all versions from 0.5.4 onwards) but only manifested in strict browser environments where setTimeout must be called with the correct `this` context. + +## Root Cause + +When `setTimeout` is assigned to a variable without binding, it loses its execution context: + +```typescript +// Broken: setTimeout loses window/globalThis context +this.#schedule = opts.schedule ?? setTimeout; + +// Later when called: +this.#schedule(callback, delay); // TypeError in browsers! +``` + +Browsers enforce strict context requirements for Web APIs like setTimeout. When the function is called without its original context (`window` or `globalThis`), browsers throw a TypeError. Node.js is more permissive, allowing the same code to work without issues. + +## The Fix + +The fix binds setTimeout to `globalThis`, ensuring it maintains the correct context: + +```typescript +// Fixed: setTimeout bound to globalThis +this.#schedule = opts.schedule ?? setTimeout.bind(globalThis); +``` + +This one-line change ensures setTimeout works correctly in all environments, whether called directly or through the internal scheduler. + +## Why Tests Didn't Catch It + +The bug evaded detection for several months because: + +- **Node.js tests**: All existing tests run in Node.js, which is permissive with function context. The same code that fails in browsers works fine in Node.js +- **Fake timers**: Unit tests use Vitest's `vi.useFakeTimers()`, which mocks setTimeout and doesn't have the same context requirements as the real browser API +- **Limited browser usage**: The browser client wasn't extensively tested in production browser environments until recently + +This highlights an important testing gap: code that works in Node.js tests may fail in actual browsers due to stricter Web API requirements. + +## Looking Ahead: Browser Testing + +To prevent similar browser-specific issues, we're implementing Vitest browser mode for `@pgflow/client`. This will: + +- Run tests in real browsers (Chromium, Firefox, Safari) rather than Node.js +- Catch browser-specific issues like context binding, DOM API compatibility, and Web API strictness +- Complement existing Node.js tests with browser-environment validation + +Browser testing will be added in a future release, with tests running in CI to ensure browser compatibility for all changes. + +## Upgrading + + + +Update your `package.json`: + +```bash frame="none" +pnpm add @pgflow/client@0.8.1 +``` + +Or if using the umbrella package: + +```bash frame="none" +pnpm add pgflow@0.8.1 +``` + +--- + +Questions or issues? Join us on [Discord](https://discord.gg/tmzcjv9Hes) or open an issue on [GitHub](https://github.com/pgflow-dev/pgflow).