From 97d1dd0ec3e277fabf38cb68ed3d692a12fdcea9 Mon Sep 17 00:00:00 2001 From: Till JS Date: Wed, 22 Apr 2026 14:21:32 +0200 Subject: [PATCH] fix(articles): snapshot scroller ref in HighlightLayer effect teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Back-navigating from an article detail view to the list and into the same article again crashed with TypeError: Cannot read properties of null (reading 'removeEventListener') Sequence that triggered it: 1. ReaderView unmounts, its own $effect cleanup calls onscroller(null). 2. DetailView sets readerScroller = null. 3. HighlightLayer's prop `scroller` becomes null. 4. The old $effect's teardown fires and reads `scroller` — which now points at null instead of the element it had attached listeners to. 5. null.removeEventListener(...) throws, Svelte can't finish tearing down the tree, and the re-mount never happens. Fix: snapshot the element reference at setup time so the teardown uses the same element the setup used, regardless of what the reactive prop is currently pointing at. Comment block in the file explains the trap so a future cleanup doesn't re-introduce it. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../articles/components/HighlightLayer.svelte | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/mana/apps/web/src/lib/modules/articles/components/HighlightLayer.svelte b/apps/mana/apps/web/src/lib/modules/articles/components/HighlightLayer.svelte index c8c4fb193..dd8cd2f36 100644 --- a/apps/mana/apps/web/src/lib/modules/articles/components/HighlightLayer.svelte +++ b/apps/mana/apps/web/src/lib/modules/articles/components/HighlightLayer.svelte @@ -154,13 +154,22 @@ } $effect(() => { - if (!scroller) return; - scroller.addEventListener('mouseup', onSelectionEnd); - scroller.addEventListener('click', onClick); + // Snapshot the element ref at setup time. `scroller` is a reactive + // prop: when the parent navigates away and re-mounts the Reader, + // it first pushes `scroller = null`, then `scroller = newEl`. + // Reading `scroller` inside the teardown returned below would + // observe whichever value is live *at teardown*, not the one we + // attached listeners to — which caused + // "Cannot read properties of null (reading 'removeEventListener')" + // on back-navigation between two article detail views. + const el = scroller; + if (!el) return; + el.addEventListener('mouseup', onSelectionEnd); + el.addEventListener('click', onClick); document.addEventListener('mousedown', onMousedown); return () => { - scroller.removeEventListener('mouseup', onSelectionEnd); - scroller.removeEventListener('click', onClick); + el.removeEventListener('mouseup', onSelectionEnd); + el.removeEventListener('click', onClick); document.removeEventListener('mousedown', onMousedown); }; });