-
Notifications
You must be signed in to change notification settings - Fork 952
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
Conversation
- 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>
- 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>
417b053
to
dd7adda
Compare
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.
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,
};
};
@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. |
- 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>
@code-asher I just came up with a better solution, I guess. Please let me know what you think 🙏 |
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.
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.
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>
@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>
Adds a new hook called
useWithRetry
as part of coder/internal#659