-
Notifications
You must be signed in to change notification settings - Fork 0
[FTF-198] Send close code on react-web-cli panel unmount #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| // 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(); | ||
|
|
@@ -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
|
||
| // 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(); | ||
|
|
@@ -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
|
||
| // 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 | ||
|
|
@@ -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
|
||
| // SKIPPED: This test has timing issues with fake timers | ||
| // The 30-second timeout doesn't trigger consistently with vi.advanceTimersByTime | ||
| vi.useFakeTimers(); | ||
|
|
@@ -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
|
||
| // 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 | ||
|
|
@@ -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
|
||
| // 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 | ||
|
|
@@ -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" | ||
| />, | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| }); | ||
|
Comment on lines
+2572
to
+2574
|
||
|
|
||
| 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"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||||||||||||||
|
|
@@ -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
|
||||||||||||||||||||||||||||||
| 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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- WebSocket closes with code 4001 and reason "user-closed-panel"
handleWebSocketClosedoesn't recognize this as user-initiated- It enters the non-recoverable error path
- Shows error overlay/UI and triggers state updates
- 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 lineThis 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
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).
| 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
-
Explicit termination (via
terminateSession()method):- Uses code
4001and reason"user-closed-panel" - Signals to the server that the session should be terminated immediately
- Intended for when the user actively closes the panel
- Uses code
-
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)
- Currently uses normal close (
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new tests render
AblyCliTerminalwithablyApiKey, but the component props interface in this package expectssignedConfig+signature(and there is noablyApiKeyprop). This will fail type-checking and likely won’t exercise the real auth/connection path. Update the tests to use the existingrenderTerminalhelper/fixtures (DEFAULT_SIGNED_CONFIG/DEFAULT_SIGNATURE/TEST_WEBSOCKET_URL) so they compile and match the component’s supported API.