Skip to content

fix: store idle watcher interval ID and listeners for proper cleanup#5789

Open
manvirsingh01 wants to merge 1 commit intosugarlabs:masterfrom
manvirsingh01:fix/idle-watcher-cleanup
Open

fix: store idle watcher interval ID and listeners for proper cleanup#5789
manvirsingh01 wants to merge 1 commit intosugarlabs:masterfrom
manvirsingh01:fix/idle-watcher-cleanup

Conversation

@manvirsingh01
Copy link

Description

Fixes #5768

The idle watcher in _initIdleWatcher() creates a setInterval and attaches window event listeners, but neither the interval ID nor the handler reference was preserved. This prevented cleanup on reinitialisation and caused duplicate intervals to accumulate.

Changes

  • Store the interval ID in this._idleWatcherIntervalId
  • Store the event handler in this._resetIdleTimer
  • Store the event names in this._idleWatcherEvents
  • Add _destroyIdleWatcher() to clear interval and remove listeners
  • Guard _initIdleWatcher() to destroy any existing watcher first

Testing

  • All 3114 existing Jest tests pass
  • No new runtime errors introduced

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor

@manvirsingh01 Pull from updated master.

The idle watcher in _initIdleWatcher() creates a setInterval and
attaches window event listeners, but neither the interval ID nor the
handler reference was preserved. This prevented cleanup on
reinitialisation and caused duplicate intervals to accumulate.

Changes:
- Store the interval ID in this._idleWatcherIntervalId
- Store the event handler in this._resetIdleTimer
- Store the event names in this._idleWatcherEvents
- Add _destroyIdleWatcher() to clear interval and remove listeners
- Guard _initIdleWatcher() to destroy any existing watcher first

Fixes sugarlabs#5768
@manvirsingh01 manvirsingh01 force-pushed the fix/idle-watcher-cleanup branch from 749ec81 to 1b01d5d Compare February 19, 2026 08:01
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor

@manvirsingh01 Cleanup looks correct. One question: is _destroyIdleWatcher() also called during Activity teardown/unload?

@parthdagia05
Copy link
Contributor

@manvirsingh01 Cleanup looks correct. One question: is _destroyIdleWatcher() also called during Activity teardown/unload?

@manvirsingh01 I also had the same thought , if Activity instances can be torn down, should _destroyIdleWatcher() be invoked during the teardown lifecycle to ensure listeners don’t persist?

@manvirsingh01
Copy link
Author

Hey @vanshika2720 @parthdagia05, thanks for the review!

Good catch — right now,
_destroyIdleWatcher() is only called inside
_initIdleWatcher() itself as a safety check (so if the watcher gets re-initialized, it cleans up the old one first before setting up a new one).

It's not hooked into any teardown/unload flow yet, mainly because Music Blocks doesn't really have a formal "shutdown" step — the app just lives as a single page. So there's no natural place to call it on exit today.

That said, I totally agree it's good practice to clean up on unload. I can add a beforeunload listener that calls
_destroyIdleWatcher() as a safety net — want me to include that in this PR?

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.

[Performance] Idle watcher interval not cleared causing resource waste

3 participants