fix: add missing ngOnDestroy cleanup in 4 UI components (event listener & timer leaks)#867
Open
GaneshPatil7517 wants to merge 2 commits intoHSF:mainfrom
Open
Conversation
Add proper lifecycle cleanup to prevent event listener accumulation and memory leaks when components are destroyed: - MakePictureComponent: reset onfullscreenchange, remove click/touchstart listeners, exit fullscreen, and remove ss-mode class on destroy - ZoomControlsComponent: clear recursive zoom timeout on destroy - LoaderComponent: store and call unsubscribe function from ErrorMessageService on destroy - CartesianGridConfigComponent: unsubscribe RxJS subscriptions on destroy Also fix ErrorMessageService.subscribeToError() to return the unsubscribe function from ActiveVariable.onUpdate(). Resolves HSF#865
There was a problem hiding this comment.
Pull request overview
This PR addresses UI resource leaks in phoenix-ui-components by adding missing teardown logic (timeouts, RxJS subscriptions, and global DOM handlers/listeners) so components don’t accumulate handlers across navigation and re-instantiation.
Changes:
ErrorMessageService.subscribeToError()now returns an unsubscribe function so callers can detach callbacks.- Added
ngOnDestroycleanup for recursive zoom timeouts and error subscription teardown. - Added
ngOnDestroycleanup for screenshot/fullscreen handlers and cartesian grid shifting subscriptions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/phoenix-ng/projects/phoenix-ui-components/lib/services/error-message-service.ts | Return the underlying ActiveVariable.onUpdate() unsubscribe function from subscribeToError(). |
| packages/phoenix-ng/projects/phoenix-ui-components/lib/components/ui-menu/zoom-controls/zoom-controls.component.ts | Clear hold-to-zoom recursive timeout chain on component destroy. |
| packages/phoenix-ng/projects/phoenix-ui-components/lib/components/ui-menu/view-options/cartesian-grid-config/cartesian-grid-config.component.ts | Unsubscribe grid-shifting subscriptions on destroy. |
| packages/phoenix-ng/projects/phoenix-ui-components/lib/components/ui-menu/make-picture/make-picture.component.ts | Add ngOnDestroy cleanup for fullscreen handler, document listeners, fullscreen state, and CSS class. |
| packages/phoenix-ng/projects/phoenix-ui-components/lib/components/loader/loader.component.ts | Persist and invoke the error subscription unsubscribe function during destroy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/phoenix-ng/projects/phoenix-ui-components/lib/services/error-message-service.ts
Show resolved
Hide resolved
...lib/components/ui-menu/view-options/cartesian-grid-config/cartesian-grid-config.component.ts
Outdated
Show resolved
Hide resolved
...projects/phoenix-ui-components/lib/components/ui-menu/make-picture/make-picture.component.ts
Outdated
Show resolved
Hide resolved
...projects/phoenix-ui-components/lib/components/ui-menu/make-picture/make-picture.component.ts
Show resolved
Hide resolved
- MakePictureComponent: store onfullscreenchange handler reference and only reset if it still belongs to this component; store setTimeout id and clear it in ngOnDestroy to prevent re-adding listeners after destroy; only exit fullscreen and remove ss-mode class when ssMode was active - CartesianGridConfigComponent: revert ngOnDestroy since shiftCartesianGridByPointer() calls onClose() immediately after creating subscriptions the subscriptions are designed to outlive the dialog and self-unsubscribe on stopShifting - ErrorMessageService: add @returns JSDoc for unsubscribe function
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #865
Several UI components attach global event listeners, set global handlers, or create recursive timeouts but never clean them up when destroyed. This causes event listener accumulation and memory leaks when users navigate between sections.
Changes:
MakePictureComponent Added OnDestroy implementation following the existing SSModeComponent pattern:
ZoomControlsComponent Added OnDestroy to clear the recursive setTimeout chain used for hold-to-zoom. Previously, destroying the component mid-zoom left the timeout chain firing indefinitely.
LoaderComponent Store the unsubscribe function returned by ErrorMessageService.subscribeToError() and call it in ngOnDestroy. Previously every instantiation added a callback to ErrorMessageService that was never removed.
CartesianGridConfigComponent Added OnDestroy to unsubscribe originChangedSub and stopShiftingSub. These RxJS subscriptions only self-unsubscribed on a user event (stopShifting), so destroying the dialog mid-interaction leaked them.
ErrorMessageService Fixed subscribeToError() to return the unsubscribe function from ActiveVariable.onUpdate() (it was previously discarded).
Testing: All 56 phoenix-ng test suites pass (176 tests).