Skip to content
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

Use the same debounced restart for all watchers #2818

Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Nov 1, 2024

Motivation

I noticed another issue with our restart watchers: they don't share the same debounced function. This matters because currently if two separate watchers are triggered in sequence, then we restart twice.

This is of course not what we want. We want to restart a single time within the debounce time, no matter how many watchers have been triggered.

Implementation

The core of the issue is that if we have multiple debounced functions, they each have their own timeout object with a different ID. That leads to this scenario:

  1. Rebase watcher fires its debounced restart
  2. After rebase is complete, the lockfile was modified so that watcher is fired too
  3. Because the debounced restart function of the lockfile is not the same as the rebase watcher, it won't clear the timeout of the rebase invocation
  4. This triggers two consecutive restarts for no reason

The fix is to ensure that we only have one single debounced restart function that is used by all of the watchers. This guarantees that, no matter which watcher is fired, it will always clear the previous restart timeout and avoid doing it multiple times.

@vinistock vinistock added vscode This pull request should be included in the VS Code extension's release notes bugfix This PR will fix an existing bug labels Nov 1, 2024 — with Graphite App
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @vinistock and the rest of your teammates on Graphite Graphite

@vinistock vinistock requested review from andyw8 and st0012 November 1, 2024 19:40
@vinistock vinistock marked this pull request as ready for review November 1, 2024 19:44
@vinistock vinistock requested a review from a team as a code owner November 1, 2024 19:44
@vinistock vinistock self-assigned this Nov 1, 2024
@vinistock vinistock merged commit fbe2350 into main Nov 4, 2024
38 checks passed
@vinistock vinistock deleted the 11-01-use_the_same_debounced_restart_for_all_watchers branch November 4, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants