- Notifications
You must be signed in to change notification settings - Fork1k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 indexImplements:- Initial delay: 1 second- Max delay: 30 seconds- Backoff multiplier: 2- Max retry attempts: 10Co-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 backoffStill 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 suiteImplements 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 successCo-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
- Add parentheses around arrow function parameter- Fix indentationCo-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 typesCo-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
Convert useRetry hook from multiple useState calls to a single useReducerfor cleaner state management. This improves code clarity and makes statetransitions 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 throughoutCo-authored-by: BrunoQuaresma <3165839+BrunoQuaresma@users.noreply.github.com>
417b053
todd7adda
Compare
code-asher left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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";exporttypeRetryState="idle"|"retry"|"countdown";interfaceRetryOptions{delayMs:number;enabled:boolean;maxDelayMs:number;multiplier:number;onRetry:()=>Promise<void>;}exportconstuseRetry=({ delayMs, enabled, maxDelayMs, multiplier, onRetry}:RetryOptions)=>{const[retryState,setRetryState]=useState<RetryState>("idle");const[attempt,setAttempt]=useState(0);const[countdown,setCountdown]=useState(0);constonRetryEvent=useEffectEvent(onRetry);useEffect(()=>{setRetryState(enabled ?"countdown" :"idle");},[enabled]);useEffect(()=>{switch(retryState){case"idle":setAttempt(0);break;case"retry":letaborted=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":constdelay=Math.min(delayMs*multiplier**(attempt-1),maxDelayMs);consttimer=setTimeout(()=>setRetryState("retry"),delay);conststart=Date.now();constinterval=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,};};
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@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 configurationCo-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 patternsCo-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 functionalityCo-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 checksCo-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 behaviorThis 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 whilefn
is in flight.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Remove the MAX_ATTEMPTS constant and associated logic to allow unlimitedretry 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 maintainingall existing functionality like cancellation, cleanup, and proper statemanagement.🤖 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 unmountThis fixes a memory leak where in-flight async operations could schedulenew 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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
- 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>
369bccd
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Adds a new hook called
useWithRetry
as part ofcoder/internal#659