fix: use numeric constant instead of WebSocket.OPEN

[adnaan]

Problem

In certain execution contexts (e.g., during WebSocket reconnection or after multiple connection cycles), the WebSocket global object's constants can become undefined even when the WebSocket connection is active.

This causes readyState comparisons like readyState === WebSocket.OPEN to fail:

• readyState = 1 (WebSocket is OPEN)
• WebSocket.OPEN = undefined
• 1 === undefined → false
• Result: Incorrect HTTP fallback despite WebSocket being available

Solution

Use the numeric constant 1 directly instead of WebSocket.OPEN for readyState comparisons.

Changes

• livetemplate-client.ts: Fixed isReady() and send() methods (2 locations)
• transport/websocket.ts: Fixed send() method (1 location)

Testing

• Verified delete operations now use WebSocket instead of falling back to HTTP
• All e2e tests passing

Impact

Fixes issue where operations would incorrectly fall back to HTTP in full test suites despite having an open WebSocket connection.

[claude]

Code Review - PR #3: Fix WebSocket.OPEN constant issue

Overview

This PR addresses a critical bug where WebSocket.OPEN and other WebSocket constants become undefined in certain execution contexts, causing incorrect HTTP fallback behavior despite having an active WebSocket connection.

---

✅ Code Quality & Best Practices

Strengths:

• Clear documentation: Inline comments explain the rationale (// WebSocket.OPEN = 1)
• Consistent application: The fix is applied uniformly across all 3 locations
• Follows conventions: Commit message follows Conventional Commits format as per CONTRIBUTING.md
• Targeted fix: Minimal, surgical change that doesn't introduce unnecessary complexity

Suggestions:

1. Consider defining constants - While the current approach works, consider defining named constants at the module level for better maintainability:

   // At the top of the file or in a constants file
   const WS_CONNECTING = 0;
   const WS_OPEN = 1;
   const WS_CLOSING = 2;
   const WS_CLOSED = 3;

   This would make the code more self-documenting while avoiding the WebSocket.OPEN issue.

2. TypeScript type safety - The getReadyState() method in transport/websocket.ts:192 returns number | undefined. Consider checking for all possible states explicitly if you want to make the code more defensive.

---

🐛 Potential Bugs & Issues

Critical observation:
The fix only addresses comparisons with WebSocket.OPEN (1). However, the WebSocket API has 4 states:

• CONNECTING = 0
• OPEN = 1
• CLOSING = 2
• CLOSED = 3

Potential issue in livetemplate-client.ts:425-431:

} else if (readyState !== undefined) {
  // WebSocket is connecting or closing, fall back to HTTP temporarily
  this.logger.warn(
    `WebSocket not ready (state: ${readyState}), using HTTP fallback`
  );

This catches all non-OPEN states (0, 2, 3). If WebSocket.CONNECTING, WebSocket.CLOSING, or WebSocket.CLOSED constants also become undefined in the same execution context, you won't be able to distinguish between these states if you need to in the future.

Recommendation: If you ever need to check for specific states (e.g., to handle CLOSING differently than CONNECTING), ensure you use numeric constants throughout:

if (readyState === 0) { // CONNECTING
  // handle connecting
} else if (readyState === 2 || readyState === 3) { // CLOSING or CLOSED
  // handle closing/closed
}

---

⚡ Performance Considerations

Good:

• Zero performance impact - numeric comparison is actually slightly faster than property access
• No additional allocations or overhead introduced

---

🔒 Security Concerns

No security issues identified. The change is purely a comparison operation and doesn't introduce any vulnerabilities.

---

🧪 Test Coverage

Observations:

1. ✅ The PR description mentions "All e2e tests passing"
2. ✅ The fix addresses a test-specific issue that was discovered in the test suite
3. ⚠️ Missing unit tests for this specific scenario

Recommendations:
Consider adding a test case that specifically validates the behavior when WebSocket global constants are undefined. This would prevent regression:

describe('WebSocket readyState handling', () => {
  it('should use WebSocket when readyState is 1 even if WebSocket.OPEN is undefined', () => {
    // Mock scenario where WebSocket.OPEN is undefined
    const originalOpen = WebSocket.OPEN;
    Object.defineProperty(WebSocket, 'OPEN', { value: undefined, configurable: true });
    
    // Test that isReady() and send() still work correctly
    // ... test implementation
    
    // Restore
    Object.defineProperty(WebSocket, 'OPEN', { value: originalOpen, configurable: true });
  });
});

---

📊 Overall Assessment

Score: 8.5/10

Pros:

• ✅ Fixes a real production issue
• ✅ Minimal, focused change
• ✅ Well-documented with inline comments
• ✅ Consistent application across codebase
• ✅ Good PR description explaining the problem and solution

Areas for improvement:

• Consider defining module-level constants for all WebSocket states
• Add specific unit test coverage for this edge case
• Consider whether other WebSocket state constants might have the same issue

---

🎯 Recommendation

APPROVE with minor suggestions

This is a solid fix for a legitimate bug. The code change is correct and addresses the immediate issue. The suggestions above are for future-proofing and improved maintainability, but they're not blockers for merging this PR.

The fix is production-ready as-is, especially given that e2e tests are passing.

[Copilot]

Pull request overview

This PR addresses a critical bug where WebSocket connection state checks fail due to WebSocket.OPEN becoming undefined in certain execution contexts (e.g., during reconnection cycles). The fix replaces symbolic constant references with the numeric value 1, ensuring reliable WebSocket state checking.

Key Changes:

• Replaced WebSocket.OPEN with numeric constant 1 to avoid undefined reference issues
• Added inline comments documenting the numeric constant meaning
• Applied fix consistently across WebSocket transport layer and client code

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
transport/websocket.ts	Updated send() method to use numeric constant 1 instead of WebSocket.OPEN
livetemplate-client.ts	Updated isReady() and send() methods to use numeric constant 1 for WebSocket state checks

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Consider adding a named constant for the WebSocket ready state to constants.ts (following the existing pattern in the codebase) instead of using the magic number 1. This would improve code maintainability and consistency across all three locations where this check is used.

Example addition to constants.ts:

export const WS_READY_STATE_OPEN = 1; // WebSocket.OPEN constant

Then import and use it:

import { WS_READY_STATE_OPEN } from "../constants";

// ...
if (this.socket && this.socket.readyState === WS_READY_STATE_OPEN) {

This approach provides the reliability of numeric constants while maintaining code clarity and following established codebase patterns.

[Copilot]

[nitpick] Consider using a named constant from constants.ts instead of the magic number 1. This follows the established pattern in the codebase (e.g., FOCUSABLE_INPUTS) and would improve maintainability. The constant should be shared across all three usages (lines 389, 417, and transport/websocket.ts:60).

[Copilot]

[nitpick] Same as line 389: consider using a named constant from constants.ts instead of the magic number 1 for consistency.