core:frontend:OneMoreTime: Improve robustness

[joaomariolago]

• Avoid double starts
• Avoid unecessary resume and stops
• Avoid empty action running
• Make sure to kill timeouts when stop/dispose to avoid double invoking

Summary by Sourcery

Improve robustness of OneMoreTime class by adding state management, preventing double starts, and handling edge cases more gracefully

Bug Fixes:

• Prevent running actions when no action is defined
• Avoid potential double-start scenarios
• Ensure proper cleanup of timeouts when stopping or disposing

Enhancements:

• Add state tracking to prevent multiple concurrent executions
• Implement more robust timeout and task management
• Enhance error handling and lifecycle management

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request enhances the robustness of the OneMoreTime class by adding state management to prevent double starts and unnecessary resume/stop calls, implementing timeout clearing to avoid double invocations, handling cases where the action is not defined, and ensuring proper disposal when the observed object is destroyed. These changes improve the reliability and resource management of the OneMoreTime component.

Sequence diagram for OneMoreTime.start()

sequenceDiagram
  participant OMT as OneMoreTime
  OMT->OMT: Check isDisposed, isPaused, isRunning
  alt Not disposed, not paused, not running
    alt action is not defined
      OMT->OMT: console.warn('OneMoreTime: Started without an action, stopping execution')
    else action is defined
      OMT->OMT: this.isRunning = true
      OMT->OMT: await this.action()
      alt Error during action
        OMT->OMT: await new Promise((resolve) => setTimeout(resolve, this.options.errorDelay))
      end
      OMT->OMT: this.isRunning = false
      alt Not paused and not disposed
        OMT->OMT: this.timeoutId = setTimeout(() => this.start(), this.options.delay)
      end
    end
  end

Loading

Sequence diagram for OneMoreTime.resume()

sequenceDiagram
  participant OMT as OneMoreTime
  OMT->OMT: Check isDisposed or not isPaused
  alt Not disposed and isPaused
    OMT->OMT: this.isPaused = false
    OMT->OMT: this.start()
  end

Loading

Updated class diagram for OneMoreTime

classDiagram
  class OneMoreTime {
    -options: OneMoreTimeOptions
    -action: Function
    -isDisposed: boolean
    -isPaused: boolean
    -isRunning: boolean
    -timeoutId: ReturnType<typeof setTimeout>
    +constructor(options: OneMoreTimeOptions)
    -killTask(): void
    -watchDisposeWith(): void
    -softStart(): void
    +start(): Promise<void>
    +stop(): void
    +resume(): void
    +Symbol.dispose(): void
  }
  note for OneMoreTime "Added isRunning and timeoutId attributes for state management and timeout handling."
  note for OneMoreTime "Added killTask method to clear the timeout."
  note for OneMoreTime "Added checks for isRunning, isDisposed and isPaused in start, stop and resume methods."

Loading

File-Level Changes

Change	Details	Files
Introduces state management to prevent double starts and unnecessary resume/stop calls.	Added isRunning flag to track the execution state. Modified start to check isRunning and prevent concurrent executions. Modified stop and resume to check isDisposed and isPaused flags. Set isRunning to true at the beginning of the start method and to false in a finally block.	core/frontend/src/one-more-time.ts
Implements a mechanism to clear timeouts to avoid double invocations and improve resource management.	Added timeoutId to store the timeout ID. Implemented killTask to clear the timeout using clearTimeout and reset timeoutId. Called killTask in stop and [Symbol.dispose] to clear any pending timeouts.	core/frontend/src/one-more-time.ts
Handles cases where the action is not defined to prevent errors.	Added a check in start to ensure an action is defined before proceeding. Logs a warning message if start is called without an action.	core/frontend/src/one-more-time.ts
Ensures proper disposal when the observed object is destroyed.	The stop method was removed from the observer callback. The isDisposed flag is set to true when the observed object is destroyed. The killTask method is called when the observed object is destroyed.	core/frontend/src/one-more-time.ts

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!
• Generate a plan of action for an issue: Comment @sourcery-ai plan on
  an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @joaomariolago - I've reviewed your changes - here's some feedback:

Overall Comments:

• Consider adding a state diagram to clarify the component's lifecycle and state transitions.
• It might be beneficial to use a more descriptive name than killTask to better reflect its purpose.

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (complexity): Consider consolidating duplicate state guard checks into a helper function and inlining the simple killTask logic where it’s used to reduce code duplication and improve readability .

Consider consolidating duplicate state guard checks into a helper function and inlining the simple killTask logic where it’s used. For example:

1. Create a helper to check if execution should proceed:

   private canExecute(): boolean {
     return !this.isDisposed && !this.isPaused;
   }

2. Use it in your methods:

   async start(): Promise<void> {
     if (!this.canExecute() || this.isRunning) return;
     if (!this.action) {
       console.warn('OneMoreTime: Started without an action, stopping execution');
       return;
     }
     this.isRunning = true;
     try {
       await this.action();
     } catch (error) {
       console.error('Error in self-calling promise:', error);
       this.options.onError?.(error);
       await new Promise(resolve => setTimeout(resolve, this.options.errorDelay));
     } finally {
       this.isRunning = false;
     }
     if (this.canExecute()) {
       this.timeoutId = setTimeout(() => this.start(), this.options.delay);
     }
   }

3. Inline the simple killTask logic in stop and disposal:

   stop(): void {
     if (!this.canExecute()) return;
     this.isPaused = true;
     if (this.timeoutId) {
       clearTimeout(this.timeoutId);
       this.timeoutId = undefined;
     }
   }
   
   [Symbol.dispose](): void {
     this.isDisposed = true;
     if (this.timeoutId) {
       clearTimeout(this.timeoutId);
       this.timeoutId = undefined;
     }
   }

These changes reduce the repetition of guard conditions and simplify the overall control flow while preserving all functionality.

[joaoantoniocardoso]

great!