feat: establish terminal reconnection foundation #18693
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 <[email protected]>
- 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 <[email protected]>
- Add parentheses around arrow function parameter - Fix indentation Co-authored-by: BrunoQuaresma <[email protected]>
- 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 <[email protected]>
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 <[email protected]>
417b053 to
dd7adda
Compare
There was a problem hiding this comment.
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,
};
};| /** | ||
| * Initial delay in milliseconds | ||
| */ | ||
| initialDelay: number; |
There was a problem hiding this comment.
Just a suggestion, but I find a suffix helpful for interval-based vars, like initialDelayMs.
| */ | ||
| onRetry: () => Promise<void>; | ||
| /** | ||
| * Maximum number of retry attempts |
There was a problem hiding this comment.
The comments on most of these vars seem redundant, they are mostly just repeating the var name 😅
| /** | ||
| * Maximum number of retry attempts | ||
| */ | ||
| maxAttempts: number; |
There was a problem hiding this comment.
Does there need to be a max number of attempts? Any reason we cannot keep trying?
| /** | ||
| * Initial delay in milliseconds | ||
| */ | ||
| initialDelay: number; |
There was a problem hiding this comment.
This is probably just a matter of opinion so feel free to ignore, but I was at first confused with initialDelay because we use it in every loop (multiplied), not just the initial one, so I wonder if we could call this delay (or delayMs) instead.
| /** | ||
| * Whether a retry is currently in progress (manual or automatic) | ||
| */ | ||
| isRetrying: boolean; |
There was a problem hiding this comment.
Without having seen the planned usage yet, my assumption is that when users pass in callback they will already have a variable for whether the callback is running, so I wonder if we need isRetrying since it gives info that the caller already has?
For example the terminal will already have a connection status state it can use, making this one redundant, since you could setTerminalState("connecting") inside onRetry(), and then use terminalState instead of isRetrying.
Actually, the caller will always need to have their own state anyway, so they know when to call start to begin the retry.
| /** | ||
| * Current delay for the next automatic retry (null if not scheduled) | ||
| */ | ||
| currentDelay: number | null; |
There was a problem hiding this comment.
Will we need to display the current delay? I imagine it will just be something like "reconnecting in N seconds.." and "reconnecting (attempt N)" so all we really need is the countdown and attempt.
| @@ -49,7 +49,7 @@ const TerminalPage: FC = () => { | |||
| // updates. | |||
| const [terminal, setTerminal] = useState<Terminal>(); | |||
| const [connectionStatus, setConnectionStatus] = | |||
| useState<ConnectionStatus>("initializing"); | |||
| useState<ConnectionStatus>("connecting"); | |||
There was a problem hiding this comment.
nbd but this feels like it should be part of the other PRs since this one is just about the retry hook
| } | ||
| } | ||
| } | ||
| }, 100); // Update every 100ms for smooth countdown |
There was a problem hiding this comment.
Is this too fast maybe? I imagine we will want to round this to a second in the UI, or do we plan on showing millisecond precision?
| startRetrying: () => void; | ||
| /** | ||
| * Stop the retry process and reset state | ||
| */ | ||
| stopRetrying: () => void; |
There was a problem hiding this comment.
Instead of the imperative start and stop, would it make sense to control this with a boolean so it could be more declarative? Sort of like how we use React Query. Hard to say exactly how it should look without the usage though. I know you have other PRs but I have not seen them yet.
| expect(result.current.timeUntilNextRetry).toBe(null); | ||
| }); | ||
|
|
||
| it("should stop retrying when stopRetrying is called", async () => { |
There was a problem hiding this comment.
It might be good to add a test that starts a retry that was previous stopped or unmounted or succeeded. In the terminal case, I imagine you could get many disconnects over time and the retry will have to be restarted multiple times.
Implements:
Related to: coder/internal#659