Skip to content

feat: establish terminal reconnection foundation #18693

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

Merged
merged 20 commits into from
Jul 3, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Jul 1, 2025

Adds a new hook called useWithRetry as part of coder/internal#659

- Update ConnectionStatus type: replace 'initializing' with 'connecting'
- Create useRetry hook with exponential backoff logic
- Add comprehensive tests for useRetry hook
- Export useRetry from hooks index

Implements:
- Initial delay: 1 second
- Max delay: 30 seconds
- Backoff multiplier: 2
- Max retry attempts: 10

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
@BrunoQuaresma BrunoQuaresma changed the title feat: Phase 1 - Terminal reconnection foundation feat: terminal reconnection foundation Jul 1, 2025
@BrunoQuaresma BrunoQuaresma changed the title feat: terminal reconnection foundation feat: establish terminal reconnection foundation Jul 1, 2025
blink-so bot and others added 5 commits July 1, 2025 14:34
- Fix startRetrying to immediately perform first retry
- Adjust retry scheduling conditions
- Fix delay calculation for exponential backoff

Still debugging test failures
- Fix attemptCount to represent attempts started, not completed
- Fix exponential backoff delay calculation
- Fix retry scheduling conditions for proper max attempts handling
- All 10 useRetry tests now pass
- No regressions in existing test suite

Implements correct behavior:
- attemptCount increments when retry starts
- Exponential backoff: 1s, 2s, 4s, 8s, 16s, 30s (capped)
- Respects maxAttempts limit
- Manual retry cancels automatic retries
- State resets properly on success

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Add parentheses around arrow function parameter
- Fix indentation

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Replace setTimeout/setInterval with window.setTimeout/window.setInterval
- Replace clearTimeout/clearInterval with window.clearTimeout/window.clearInterval
- Fixes TypeScript error: Type 'Timeout' is not assignable to type 'number'
- Ensures proper browser environment timer types

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
Convert useRetry hook from multiple useState calls to a single useReducer
for cleaner state management. This improves code clarity and makes state
transitions more predictable.

Changes:
- Replace 5 useState calls with single useReducer
- Add RetryState interface and RetryAction union type
- Implement retryReducer function for all state transitions
- Update all state access to use state object
- Replace setState calls with dispatch calls throughout

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
@BrunoQuaresma BrunoQuaresma requested review from code-asher and a team July 1, 2025 16:13
@blink-so blink-so bot force-pushed the feature/terminal-reconnection branch from 417b053 to dd7adda Compare July 1, 2025 16:31
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

I found the state hard to follow, is that just me? I think the code could be significantly simpler (simpler to me at least). For example (not tested):

import { useEffect, useRef, useState } from "react";
import { useEffectEvent } from "./hookPolyfills";

export type RetryState = "idle" | "retry" | "countdown";

interface RetryOptions {
	delayMs: number;
	enabled: boolean;
	maxDelayMs: number;
	multiplier: number;
	onRetry: () => Promise<void>;
}

export const useRetry = ({ delayMs, enabled, maxDelayMs, multiplier, onRetry }: RetryOptions) => {
	const [retryState, setRetryState] = useState<RetryState>("idle");
	const [attempt, setAttempt] = useState(0);
	const [countdown, setCountdown] = useState(0);
	const onRetryEvent = useEffectEvent(onRetry);

	useEffect(() => {
		setRetryState(enabled ? "countdown" : "idle");
	}, [enabled]);

	useEffect(() => {
		switch (retryState) {
			case "idle":
				setAttempt(0);
				break;
			case "retry":
				let aborted = false;
				onRetryEvent().then(() => {
					if (!aborted) {
						setRetryState("idle");
					}
				}).catch(() => {
					if (!aborted) {
						setRetryState("countdown");
						setAttempt(attempt + 1); // probably better to set earlier or together with the state
					}
				});
				return () => {
					aborted = true;
				};
			case "countdown":
				const delay = Math.min(delayMs * multiplier ** (attempt - 1), maxDelayMs);
				const timer = setTimeout(() => setRetryState("retry"), delay);
				const start = Date.now();
				const interval = setInterval(() => setCountdown(Math.max(0, delay - (Date.now() - start))), 1000);
				return () => {
					clearTimeout(timer);
					clearInterval(interval);
				};
		}
	}, [attempt, retryState, delayMs, multiplier, maxDelayMs, onRetryEvent]);

	return {
		attempt,
		// countdown will be null if the next retry is not scheduled.
		countdown: retryState === "countdown" ? countdown : null,
	};
};

@BrunoQuaresma
Copy link
Collaborator Author

@code-asher thanks a lot for the review! I think I have a better sense on how to simplify the hook usage and make code more understandable.

blink-so bot and others added 8 commits July 2, 2025 13:36
- Created useWithRetry hook with simple interface (call, retryAt, isLoading)
- Implements exponential backoff with configurable options
- Includes comprehensive tests covering all scenarios
- Added usage examples for different configurations
- Follows existing code patterns and uses constants for configuration

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove example file as requested
- Fix circular dependency issue using useRef pattern
- Ensure proper cleanup and timer management
- Implementation follows existing codebase patterns

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove useRetry.ts and useRetry.test.ts files
- Update hooks index.ts to remove useRetry export
- useWithRetry provides simpler interface for retry functionality

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Remove options parameter - hook now uses fixed configuration
- Update max attempts to 10 (from 3)
- Update max delay to 10 minutes (from 8 seconds)
- Remove countdown logic - no interval ref needed
- Consolidate state into single RetryState object
- Calculate delay inline where it's used
- Remove separate executeFunction - logic moved into call function
- Only use functions for reusable logic (clearTimer)
- Update tests to match new implementation
- Run formatting and linting checks

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Do not reset attemptCount when no more attempts are possible
- Add attemptCount to UseWithRetryResult interface
- Return attemptCount in hook result for tracking
- Update tests to verify attemptCount preservation
- Add comprehensive test for attemptCount behavior

This allows consumers to track how many attempts were made,
even after all retry attempts have been exhausted.

Co-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
@BrunoQuaresma BrunoQuaresma requested a review from code-asher July 2, 2025 14:38
@BrunoQuaresma
Copy link
Collaborator Author

@code-asher I just came up with a better solution, I guess. Please let me know what you think 🙏

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

I like the idea to externalize the countdown!

The new hook looks great to me, I do have one concern/question about unmounting while fn is in flight.

BrunoQuaresma and others added 4 commits July 3, 2025 18:13
Remove the MAX_ATTEMPTS constant and associated logic to allow unlimited
retry attempts. The hook now retries indefinitely with exponential backoff
(capped at 10 minutes delay) until the operation succeeds or is cancelled.

Update tests to verify the new unlimited retry behavior while maintaining
all existing functionality like cancellation, cleanup, and proper state
management.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…me retryAt to nextRetryAt

- Remove attemptCount from RetryState interface as it's not needed externally
- Rename retryAt to nextRetryAt for better clarity
- Simplify all setState calls to only manage isLoading and nextRetryAt
- Keep attempt tracking local to executeAttempt function for delay calculation

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…EffectEvent

- Update all test references from retryAt to nextRetryAt to match the new API
- Add useEffectEvent import to stabilize function reference and prevent unnecessary re-renders
- Update callback dependency from fn to stableFn for better performance
- All tests now pass with the updated API

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add mountedRef to track component mount state and prevent:
- setState calls after component unmount
- Scheduling new retry timeouts when async operations complete after unmount

This fixes a memory leak where in-flight async operations could schedule
new retries even after the component was unmounted.

Changes:
- Add mountedRef.current checks before all setState calls
- Add mountedRef.current checks before scheduling timeouts
- Set mountedRef.current = false in cleanup
- Add test to verify fix prevents retries after unmount

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@BrunoQuaresma BrunoQuaresma requested a review from code-asher July 3, 2025 18:35
@BrunoQuaresma
Copy link
Collaborator Author

@code-asher I tried to implement all the remaining points. Please let me know if there is anything missing.

- Change call() return type from Promise<void> to void
- Add guard to prevent multiple concurrent calls when isLoading is true
- Add test case to verify no duplicate calls during loading state
- Update executeAttempt to use default parameter instead of explicit 0

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@BrunoQuaresma BrunoQuaresma requested a review from code-asher July 3, 2025 20:27
@BrunoQuaresma BrunoQuaresma merged commit 369bccd into main Jul 3, 2025
35 checks passed
@BrunoQuaresma BrunoQuaresma deleted the feature/terminal-reconnection branch July 3, 2025 20:49
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants