Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
BrunoQuaresma merged 20 commits intomainfromfeature/terminal-reconnection
Jul 3, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma commentedJul 1, 2025
edited
Loading

Adds a new hook calleduseWithRetry as part ofcoder/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 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>
@BrunoQuaresmaBrunoQuaresma changed the titlefeat: Phase 1 - Terminal reconnection foundationfeat: terminal reconnection foundationJul 1, 2025
@BrunoQuaresmaBrunoQuaresma changed the titlefeat: terminal reconnection foundationfeat: establish terminal reconnection foundationJul 1, 2025
blink-sobotand others added5 commitsJuly 1, 2025 14:34
- 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>
@BrunoQuaresmaBrunoQuaresma requested review fromcode-asher anda teamJuly 1, 2025 16:13
@blink-soblink-sobotforce-pushed thefeature/terminal-reconnection branch from417b053 todd7addaCompareJuly 1, 2025 16:31
Copy link
Member

@code-ashercode-asher left a comment
edited
Loading

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,};};

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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-sobotand others added8 commitsJuly 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 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>
@BrunoQuaresma
Copy link
CollaboratorAuthor

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

Copy link
Member

@code-ashercode-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 whilefn is in flight.

BrunoQuaresmaand others added4 commitsJuly 3, 2025 18:13
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>
@BrunoQuaresma
Copy link
CollaboratorAuthor

@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>
@BrunoQuaresmaBrunoQuaresma merged commit369bccd intomainJul 3, 2025
30 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the feature/terminal-reconnection branchJuly 3, 2025 20:49
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 3, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@code-ashercode-ashercode-asher approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@BrunoQuaresma@code-asher

[8]ページ先頭

©2009-2025 Movatter.jp