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

fix: stop interval when window not focused - [INS-4571] #8082

Closed
wants to merge 2 commits into from

Conversation

CurryYangxx
Copy link
Member

@CurryYangxx CurryYangxx commented Oct 15, 2024

Changes:

  • add isWinsowFocused bridge
  • listen main window focus and blur event
  • get window focus states in the renderer, and when the window is blurred, we should stop getting git changes interval
  • add a flag isCheckingGitChanges to record the task status, if the last time task is not finished, skip this time.

Background:
We use a timer in the collection to periodically check the status of the git repository and update db, to display uncommitted changes in the dashboard page.
image

@CurryYangxx CurryYangxx requested a review from a team October 15, 2024 09:12
@CurryYangxx CurryYangxx changed the title fix: stop interval when window not focused fix: stop interval when window not focused - [INS-4571] Oct 15, 2024
Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the document.hasFocus() method and avoid the bridge here.
Also the checkGitChanges doesn't seem to work as expected.

return;
}
isCheckingGitChanges.current = true;
await checkGitChanges(workspaceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is updating the UI since it doesn't use the routes and the app doesn't re-render

Copy link
Member Author

@CurryYangxx CurryYangxx Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gatzjames do you mean the checkGitChanges function? We don't need to update the UI in this scenario, we just need to update the hasUncommitChange db field when the user is working on the collection page, and we use this field to show notifications on the other router page(dashboard page).

const loadingPush = gitPushFetcher.state === 'loading';
const loadingPull = gitPullFetcher.state === 'loading';
const loadingFetch = gitFetchFetcher.state === 'loading';
const loadingStatus = gitStatusFetcher.state === 'loading';

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need the bridge here. Could you share some more info on your approach?
Tried with document.hasFocus() and it works as expected if the app is not focused since we only have one tab in one window renderer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gatzjames I haven't used document.hasFocus before, I can try it and if it works I can use it instead. And I think listening to the BrowserWindow focus and 'blur' event is necessary here.

@CurryYangxx
Copy link
Member Author

After discussing with the team, we have some concern about using focus to control when the interval starts may introduce some side effects. Also after James's pr #8027, the performance of git status has been greatly improved.
So we decide to open another pr: #8086 to just make sure the previous task is completed before executing the next one .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants