Skip to content

fix: add idle watcher interval cleanup and resource management#5769

Closed
DhyaniKavya wants to merge 1 commit intosugarlabs:masterfrom
DhyaniKavya:fix/idle-watcher-interval-cleanup
Closed

fix: add idle watcher interval cleanup and resource management#5769
DhyaniKavya wants to merge 1 commit intosugarlabs:masterfrom
DhyaniKavya:fix/idle-watcher-interval-cleanup

Conversation

@DhyaniKavya
Copy link
Contributor

issue : #5768

Problem:-
The idle watcher in js/activity.js creates a setInterval that runs every second to monitor inactivity and adjust the frame rate. The interval ID is not stored, and the activity reset event listeners are attached using a handler reference that cannot be removed.
As a result:
-the interval cannot be cleared if the activity is reinitialized
-duplicate intervals could accumulate in future lifecycle scenarios
-event listeners cannot be removed during teardown
-unnecessary resource usage may occur in long sessions or development reloads
-While this does not cause immediate user-visible errors, it represents incomplete lifecycle and resource management.

Solution:-
-This PR enables proper lifecycle cleanup while preserving all existing behavior:
-stores the idle watcher interval ID
-prevents duplicate intervals from being created
-stores the reset handler for listener removal
-adds a cleanup method to remove the interval and listeners
-does not change idle detection logic or behavior

Changes Made:-
-File modified:
->js/activity.js
->store interval ID for cleanup
->prevent duplicate interval creation
->store reset handler reference
->add cleanupIdleWatcher() method for lifecycle cleanup

Testing Performed:-
-Manual Verification
-Idle throttling activates after inactivity
-Frame rate restores on activity
-Playback and interaction behavior unchanged
-No console errors observed
-Automated Testing

Jest: ✅ all tests passed
Prettier: ✅ formatting verified
ESLint: ⚠️ pre-existing repository configuration warning (no new lint errors introduced)

Impact:-
-Improves resource and lifecycle management
-Prevents duplicate idle watcher intervals
-Enables safe cleanup for future modularization and testing
-Improves maintainability and long-session efficiency
-No breaking changes

Notes:-
This change is intentionally minimal and non-breaking. The cleanup method is opt-in and does not alter current behavior. It provides proper lifecycle handling for future use cases such as testing, development reloads, or modular integration.

Happy to adjust if maintainers prefer a different approach.

- Store interval ID in _idleWatcherIntervalId property
- Store reset handler in _resetIdleTimer for cleanup
- Prevent duplicate intervals with clearInterval check
- Add cleanupIdleWatcher() method to remove interval and event listeners
- No behavior changes to idle detection or FPS throttling
@github-actions
Copy link
Contributor

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

@Ady0333
Copy link
Contributor

Ady0333 commented Feb 17, 2026

@DhyaniKavya
One small thought, should cleanupIdleWatcher() also be called in the activity reset/stop path (for example, right before teardown)? That would ensure no intervals or listeners remain if the activity restarts or unloads.

@vanshika2720 what do you think?

Copy link
Contributor

@vanshika2720 vanshika2720 left a comment

Choose a reason for hiding this comment

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

@DhyaniKavya this is duplicate of PR #5770
please close this .

@vanshika2720
Copy link
Contributor

vanshika2720 commented Feb 17, 2026

@Ady0333 checkout PR #5770 .it is duplicate.
Yes, Adding cleanupIdleWatcher() to the reset/stop path would ensure proper teardown and avoid any potential residual listeners or intervals.
It would improve lifecycle safety.

@Ady0333
Copy link
Contributor

Ady0333 commented Feb 17, 2026

@vanshika2720
I also noticed there is another PR (#5770) that also touches the idle watcher cleanup. Since both are working around the same area in js/activity.js, do you think it would make sense to consolidate the changes to avoid overlap?

Just thinking from a maintainability point of view.

@DhyaniKavya
Copy link
Contributor Author

@vanshika2720 sure , i am closing this PR. Forgot to close it before. Thankyou

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