<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=638882) - [Work on this issue](https://contributors.gitlab.com/manage-issue?action=work&projectId=278964&issueIid=638882) </details> <!--IssueSummary end--> ## Summary `removeListenerSystemColorSchemeChange` in `app/assets/javascripts/lib/utils/css_utils.js` doesn't remove the listener that `listenSystemColorSchemeChange` registered. Every component that subscribes to system color scheme changes keeps its `change` listener on the media query for the lifetime of the page. ## Why it doesn't work The two helpers each build a fresh arrow function: ```js export function listenSystemColorSchemeChange(onEvent) { window.matchMedia(PREFERS_DARK) .addEventListener('change', (event) => handleColorSchemeChange(onEvent, event)); } export function removeListenerSystemColorSchemeChange(onEvent) { window.matchMedia(PREFERS_DARK) .removeEventListener('change', (event) => handleColorSchemeChange(onEvent, event)); } ``` `removeEventListener` matches on function identity, and the arrow passed to it is a different object from the one `addEventListener` received, so the registered listener stays on the media query. Measured in jsdom with a stubbed media query: three subscriptions give three listeners, and three matching `removeListenerSystemColorSchemeChange` calls leave all three attached. ## Affected callers All of these call the remove helper on teardown, and the listener stays registered: - `app/assets/javascripts/glql/components/presenters/bar_chart/single_dimension_bar_chart.vue` - `app/assets/javascripts/explore/analytics_dashboards/components/visualizations/heat_map_chart.vue` - `app/assets/javascripts/search/results/components/zoekt_blob_results.vue` - `ee/app/assets/javascripts/security_dashboard/components/shared/charts/mttr_over_time_chart.vue` - `ee/app/assets/javascripts/security_dashboard/components/shared/charts/vulnerabilities_by_age_chart.vue` - `ee/app/assets/javascripts/security_dashboard/components/shared/charts/vulnerabilities_by_identifier_chart.vue` - `ee/app/assets/javascripts/security_dashboard/components/shared/charts/open_vulnerabilities_over_time.vue` - `app/assets/javascripts/editor/source_editor.js` (added in !268588) Each remount adds a listener, and all of them run on an OS appearance change. The charts re-read severity colors, the Source Editor calls `monaco.editor.setTheme`. ## Why the tests don't catch it `spec/frontend/lib/utils/css_utils_spec.js:78` asserts only that `removeEventListener` was called: ```js expect(mockMedia.removeEventListener).toHaveBeenCalledWith('change', expect.any(Function)); ``` `expect.any(Function)` passes whether or not the right function was handed over. The callers' specs mock both helpers, so they can't see it either. ## Fix Two parts, and the first on its own is not enough. **1. Keep the registered wrapper so it can be removed again.** ```js const colorSchemeChangeHandlers = new WeakMap(); export function listenSystemColorSchemeChange(onEvent) { const handler = (event) => handleColorSchemeChange(onEvent, event); colorSchemeChangeHandlers.set(onEvent, handler); window.matchMedia(PREFERS_DARK).addEventListener('change', handler); } export function removeListenerSystemColorSchemeChange(onEvent) { const handler = colorSchemeChangeHandlers.get(onEvent); if (!handler) return; colorSchemeChangeHandlers.delete(onEvent); window.matchMedia(PREFERS_DARK).removeEventListener('change', handler); } ``` **2. Give `SourceEditor` its own handler.** The map is keyed by the callback, so two subscriptions sharing one callback overwrite each other's entry: both listeners get registered, only the second can be removed. The seven Vue components are fine, because `this.setColorScheme` and friends are bound per component instance. `SourceEditor` passes the shared module function `setupEditorTheme`, so it needs a per-editor handler: ```js this.onColorSchemeChange = () => setupEditorTheme(); listenSystemColorSchemeChange(this.onColorSchemeChange); ``` `css_utils_spec.js` should also assert that `removeEventListener` receives the same function `addEventListener` was given, instead of `expect.any(Function)`. ## Verification I prototyped both parts locally to size it up: - 5736 tests pass across `css_utils_spec`, all seven callers' specs and `spec/frontend/editor`, with no spec changes needed - with both parts applied, three `SourceEditor`s register three listeners and disposing all three leaves zero ## Note on the Source Editor `SourceEditor.dispose()` isn't called anywhere in `app/` or `ee/app/` today. All four construction sites keep only the `EditorInstance`, and `source_editor.vue`'s `this.editor.dispose()` reaches the Monaco editor through the `EditorInstance` proxy, because `EditorInstance` defines no `dispose` of its own. So the Source Editor won't benefit from this fix until disposal is wired up as well. The seven Vue components will, since they call the helper from `beforeDestroy`/`destroyed`. ## Origin Introduced in !165217 (merged 2024-09-12), which added both helpers. The arrow wrapper came out of a review suggestion to extract `handleColorSchemeChange`; the suggestion spelled out the `listen` half only, and the remove half was written to match it. Unchanged since. ## References - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165217 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/268588