Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions packages/react-web-cli/src/AblyCliTerminal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@
expect(onConnectionStatusChangeMock).toHaveBeenCalledWith("disconnected");
});

test.skip("shows installation tip after 6 seconds during connection attempts", async () => {

Check warning on line 777 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 777 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test has timing issues with fake timers in CI environments
// The 6-second delay doesn't advance consistently with vi.advanceTimersByTime
vi.useFakeTimers();
Expand Down Expand Up @@ -830,7 +830,7 @@
});
});

test.skip("shows installation tip during reconnection after 6 seconds", async () => {

Check warning on line 833 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 833 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test has timing issues with fake timers in CI environments
// The 6-second delay doesn't advance consistently with vi.advanceTimersByTime
vi.useFakeTimers();
Expand Down Expand Up @@ -913,7 +913,7 @@
expect(onConnectionStatusChangeMock).toHaveBeenCalledWith("connecting");
});

test.skip("manual reconnect resets attempt counter after max attempts reached - skipped due to CI timing issues", async () => {

Check warning on line 916 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 916 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test has timing issues in CI environments
// Manual reconnect state transitions don't complete reliably with mocked timers
// Set up max attempts reached state
Expand Down Expand Up @@ -1211,7 +1211,7 @@
);
});

test.skip("connection timeout triggers error after 30 seconds", async () => {

Check warning on line 1214 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 1214 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test has timing issues with fake timers
// The 30-second timeout doesn't trigger consistently with vi.advanceTimersByTime
vi.useFakeTimers();
Expand Down Expand Up @@ -1614,7 +1614,7 @@
setItemMock.mockRestore();
});

test.skip("prompt detection correctly handles ANSI color codes", async () => {

Check warning on line 1617 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 1617 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test depends on React fiber internal structure
// React's internal structure is not stable across versions and breaks this test
// Create a mock component and socket
Expand Down Expand Up @@ -1652,7 +1652,7 @@
expect(instance.isSessionActive).toBe(true);
});

test.skip("onConnectionStatusChange only reports status for the primary terminal in split-screen mode", async () => {

Check warning on line 1655 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / setup

Disabled test - if you want to skip a test temporarily, use .todo() instead

Check warning on line 1655 in packages/react-web-cli/src/AblyCliTerminal.test.tsx

View workflow job for this annotation

GitHub Actions / e2e-cli

Disabled test - if you want to skip a test temporarily, use .todo() instead
// SKIPPED: This test verifies implementation details that are subject to change
// Core functionality is covered by other unit and integration tests
// The environment is not stable enough for this internal implementation test
Expand Down Expand Up @@ -2557,3 +2557,76 @@
expect(hasTestCmd).toBe(true);
}, 15_000);
});

describe("AblyCliTerminal - Unmount cleanup", () => {
test("closes socket normally on unmount (no special code for resume support)", async () => {
mockClose.mockClear();

const { unmount } = render(
<AblyCliTerminal
websocketUrl="wss://test.ably.com"
ablyApiKey="test-key"
/>,
Comment on lines +2565 to +2569
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new tests render AblyCliTerminal with ablyApiKey, but the component props interface in this package expects signedConfig + signature (and there is no ablyApiKey prop). This will fail type-checking and likely won’t exercise the real auth/connection path. Update the tests to use the existing renderTerminal helper/fixtures (DEFAULT_SIGNED_CONFIG / DEFAULT_SIGNATURE / TEST_WEBSOCKET_URL) so they compile and match the component’s supported API.

Copilot uses AI. Check for mistakes.
);

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 50));
});
Comment on lines +2572 to +2574
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests rely on setTimeout(..., 50) to wait for the socket to open. Elsewhere in this file you use waitFor assertions on mockSocketInstance.readyState === WebSocket.OPEN, which is less timing/flakiness-prone. Consider switching these waits to waitFor to reduce intermittent failures in CI.

Copilot uses AI. Check for mistakes.

expect(mockSocketInstance).toBeTruthy();
expect(mockSocketInstance.readyState).toBe(WebSocket.OPEN);

unmount();

// Should close without special code (allows grace period for resume)
expect(mockClose).toHaveBeenCalledWith();
});

test("does not call close if socket already closing", async () => {
mockClose.mockClear();

const { unmount } = render(
<AblyCliTerminal
websocketUrl="wss://test.ably.com"
ablyApiKey="test-key"
/>,
);

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 50));
});

mockSocketInstance.readyState = WebSocket.CLOSING;

unmount();

expect(mockClose).not.toHaveBeenCalled();
});

test("terminateSession() sends close code 4001 for immediate cleanup", async () => {
mockClose.mockClear();
const terminalRef = React.createRef<AblyCliTerminalHandle>();

render(
<AblyCliTerminal
ref={terminalRef}
websocketUrl="wss://test.ably.com"
ablyApiKey="test-key"
/>,
);

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 50));
});

expect(mockSocketInstance).toBeTruthy();
expect(mockSocketInstance.readyState).toBe(WebSocket.OPEN);

// Call terminateSession explicitly
act(() => {
terminalRef.current?.terminateSession();
});

expect(mockClose).toHaveBeenCalledWith(4001, "user-closed-panel");
});
});
21 changes: 20 additions & 1 deletion packages/react-web-cli/src/AblyCliTerminal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ export interface AblyCliTerminalHandle {
setSplitPosition: (percent: number) => void;
/** Read current split state. */
getSplitState: () => { isSplit: boolean; splitPosition: number };
/** Terminate the session immediately. Call this before unmounting when user explicitly closes the panel. */
terminateSession: () => void;
}

// Use shared debug logging
Expand Down Expand Up @@ -340,6 +342,23 @@ const AblyCliTerminalInner = (
setSplitPosition(clamped);
},
getSplitState: () => ({ isSplit, splitPosition }),
terminateSession: () => {
debugLog(
"[AblyCLITerminal] terminateSession called - closing with code 4001",
);
if (
socketReference.current &&
socketReference.current.readyState < WebSocket.CLOSING
) {
socketReference.current.close(4001, "user-closed-panel");
}
if (
secondarySocketReference.current &&
secondarySocketReference.current.readyState < WebSocket.CLOSING
) {
secondarySocketReference.current.close(4001, "user-closed-panel");
Comment on lines +353 to +359
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terminateSession() closes with reason "user-closed-panel", but handleWebSocketClose only treats "user-closed-primary"/"user-closed-secondary"/"manual-reconnect" as user-initiated. As a result, a 4001 close here will run the non-recoverable path and may briefly render a disconnect/error UI and trigger state updates just before unmount. Either include this reason in the user-initiated branch (and explicitly clear stored session IDs there) or adjust the close handling so explicit panel termination doesn’t surface as an error.

Suggested change
socketReference.current.close(4001, "user-closed-panel");
}
if (
secondarySocketReference.current &&
secondarySocketReference.current.readyState < WebSocket.CLOSING
) {
secondarySocketReference.current.close(4001, "user-closed-panel");
socketReference.current.close(4001, "user-closed-primary");
}
if (
secondarySocketReference.current &&
secondarySocketReference.current.readyState < WebSocket.CLOSING
) {
secondarySocketReference.current.close(4001, "user-closed-secondary");

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai what do you think about above review comment, can you explain a bit more

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sacOO7, let me explain the issue Copilot identified:

The Problem

When terminateSession() is called (typically during unmount), it closes the WebSocket with:

  • Code: 4001
  • Reason: "user-closed-panel"

However, in the handleWebSocketClose function, there's a check for user-initiated closes:

const userClosedTerminal =
  event.reason === "user-closed-primary" ||
  event.reason === "user-closed-secondary" ||
  event.reason === "manual-reconnect";

if (userClosedTerminal) {
  // Exit early - no reconnection, no error UI
  return;
}

Since "user-closed-panel" is not in this list, the close event continues through the handler. Then it hits this check:

const NON_RECOVERABLE_CLOSE_CODES = new Set<number>([
  4001, // Policy violation
  // ...
]);

if (NON_RECOVERABLE_CLOSE_CODES.has(event.code)) {
  // Shows error UI, sets manual reconnect prompt, etc.
}

The Consequence

When the component unmounts and terminateSession() is called:

  1. WebSocket closes with code 4001 and reason "user-closed-panel"
  2. handleWebSocketClose doesn't recognize this as user-initiated
  3. It enters the non-recoverable error path
  4. Shows error overlay/UI and triggers state updates
  5. All this happens right before the component unmounts (unnecessary work)

The Solution

Instead of changing the close reason (as Copilot suggested), I'd recommend adding "user-closed-panel" to the user-initiated check. This preserves the semantic clarity of the close reason while fixing the handling:

const userClosedTerminal =
  event.reason === "user-closed-primary" ||
  event.reason === "user-closed-secondary" ||
  event.reason === "manual-reconnect" ||
  event.reason === "user-closed-panel";  // Add this line

This way:

  • The server still receives code 4001 with reason "user-closed-panel" for immediate cleanup (as per your PR objectives)
  • The client recognizes it as an intentional user action
  • No error UI or unnecessary state updates during unmount
  • The reason remains semantically meaningful ("user-closed-panel" is clearer than "user-closed-primary")

Would you like me to search for the exact location in the file where this change should be made?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denissellu can you take a look

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}
},
}),
[
enableSplitScreen,
Expand Down Expand Up @@ -2382,7 +2401,7 @@ const AblyCliTerminalInner = (
socketReference.current &&
socketReference.current.readyState < WebSocket.CLOSING
) {
// close websocket
// Normal close (no 4001) so server grace period allows resume
debugLog("[AblyCLITerminal] Closing WebSocket on unmount.");
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling socketReference.current.close() on unmount will produce a normal CloseEvent (likely code 1000, empty reason). In handleWebSocketClose, that path is treated as recoverable and schedules auto-reconnect, which can continue after the component has unmounted (since grResetState() does not cancel timers unless already cancelled). Consider ensuring unmount close is treated as a user-initiated/no-reconnect close (e.g., close with an explicit reason that handleWebSocketClose short-circuits on, or set an isUnmounting flag checked by the close handler / cancel reconnect timers before closing).

Suggested change
debugLog("[AblyCLITerminal] Closing WebSocket on unmount.");
debugLog("[AblyCLITerminal] Closing WebSocket on unmount.");
// Prevent auto-reconnect and handler side effects after unmount
grCancelReconnect();
socketReference.current.onclose = null;
socketReference.current.onerror = null;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai what do you think about above review comment, can you explain a bit more

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sacOO7 Great question! Let me break down Copilot's concern and the broader context here.

Two Different Scenarios

Based on the code, there are two distinct ways the WebSocket gets closed:

  1. Explicit termination (via terminateSession() method):

    • Uses code 4001 and reason "user-closed-panel"
    • Signals to the server that the session should be terminated immediately
    • Intended for when the user actively closes the panel
  2. Component unmount (cleanup in useEffect):

    • Currently uses normal close (code 1000, no reason)
    • Allows the server to keep the session alive with a grace period for potential resumption
    • Happens when the component is unmounted (e.g., page navigation, React re-render)

The Problem Copilot Identified

When the component unmounts and calls socketReference.current.close() without arguments, it produces a CloseEvent with code 1000 (normal closure) and an empty reason.

Looking at your handleWebSocketClose function (around line 1645), it checks for specific reasons to avoid reconnecting:

const userClosedTerminal =
  event.reason === "user-closed-primary" ||
  event.reason === "user-closed-secondary" ||
  event.reason === "manual-reconnect";

Since the unmount close has no reason and code 1000 is not in your NON_RECOVERABLE_CLOSE_CODES set, the close event could potentially fall through to the reconnection logic. This means:

  • Reconnection timers might be scheduled
  • The component is already unmounted, so any callbacks would reference stale DOM/refs
  • Resource leak and potential errors

Current Mitigation

I notice your unmount cleanup does call grResetState() at the end:

grResetState(); // Ensure global state is clean
clearConnectionTimeout();

Key question: Does grResetState() properly cancel any pending reconnection timers? If yes, this concern is mitigated. If not, there's a potential issue.

Recommended Solutions

Option 1: Add a specific close reason for unmount to prevent reconnection logic from firing:

// Close with a reason that handleWebSocketClose recognizes as non-reconnectable
socketReference.current.close(1000, "component-unmount");

Then update handleWebSocketClose to recognize it:

const userClosedTerminal =
  event.reason === "user-closed-primary" ||
  event.reason === "user-closed-secondary" ||
  event.reason === "manual-reconnect" ||
  event.reason === "component-unmount";

Option 2: Explicitly cancel reconnection and remove event listeners before closing:

grCancelReconnect(); // Cancel any pending reconnect timers
if (socketReference.current) {
  // Note: Since you use addEventListener, you'd need to remove listeners
  // rather than set onclose = null
  socketReference.current.close();
}
grResetState();

The design intent (normal close to allow resume) is good, but you need to ensure the close handler doesn't trigger unwanted reconnection attempts after unmount. Let me know if you'd like me to search for the implementation of grResetState to verify it cancels timers properly!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denissellu can you take a look

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

socketReference.current.close();
}
Expand Down
Loading