fix: address bugs B1–B8 from IMPROVEMENTS.md (#213)

* fix: address bugs B1–B8 from IMPROVEMENTS.md

B1 (App.svelte): store media query handler in named var so removeEventListener removes the correct reference
B2 (Menu.svelte): move ClipboardJS into onMount, narrow selector to [data-clipboard-text], destroy on cleanup
B3 (Board.svelte): guard accepts callback against missing card during store mid-update
B4 (Board.svelte): guard drop handler against missing card during store mid-update
B5 (api.js): throw on non-OK HTTP status in requestJson so callers can distinguish failure
B6 (firestore.js): optional-chain column/owner in normaliseCard to survive partial Firestore writes
B7 (firestore.js): replace private _document field access with Date.now() fallback
B8 (store.js): cancel stale passwordValid promises with a generation flag to prevent stale results

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: handle requestJson errors in Board.svelte callers; remove bugs section

- getBoard (onMount): wrap in try/catch so HTTP 404 navigates to /not-found
  instead of propagating as an unhandled rejection
- updateBoard (board subscriber): use .catch() since the call is fire-and-forget
  and a sync try/catch cannot catch async errors
- Remove resolved Bugs section from IMPROVEMENTS.md

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: d0x2f

IMPROVEMENTS.md

@@ -4,187 +4,6 @@ Findings from a comprehensive review of the retro.tools frontend. Covers Svelte
 
 ---
 
-## Bugs
-
-### B1 — Memory leak in dark mode media query listener (`src/App.svelte:20–34`)
-
-**Severity: High**
-
-The `change` listener on `prefersDarkScheme` is added as an anonymous arrow function on line 20, but the `onMount` cleanup on line 33 tries to remove `darkModeChangeListener` — a different function reference used for the colorMode subscription. The anonymous media-query listener is never removed, leaking on every unmount.
-
-```js
-// Current (line 20): listener added as anonymous function
-prefersDarkScheme.addEventListener("change", (e) => { ... });
-
-// Current (line 33): removes the WRONG function
-return () => {
-  unsubscribeDarkModeChange();
-  prefersDarkScheme.removeEventListener("change", darkModeChangeListener); // wrong ref
-};
-```
-
-**Fix:** Store the handler in a named variable and reference it in both `addEventListener` and `removeEventListener`.
-
-```js
-const handleSchemeChange = (e) => { ... };
-prefersDarkScheme.addEventListener("change", handleSchemeChange);
-return () => {
-  unsubscribeDarkModeChange();
-  prefersDarkScheme.removeEventListener("change", handleSchemeChange);
-};
-```
-
----
-
-### B2 — ClipboardJS instance never stored or destroyed (`src/components/Menu.svelte:42`)
-
-**Severity: High**
-
-`new ClipboardJS("button")` runs at module scope (outside any lifecycle hook), so the instance is created but never stored or `.destroy()`-ed when the component unmounts. The `"button"` selector also matches all buttons in the document, including those not yet mounted.
-
-**Fix:** Move into `onMount`, store the instance, destroy it in the cleanup:
-
-```js
-import { onMount } from "svelte";
-let clipboard;
-onMount(() => {
-  clipboard = new ClipboardJS("[data-clipboard-text]");
-  return () => clipboard.destroy();
-});
-```
-
----
-
-### B3 — Null dereference in drag `accepts` callback (`src/Board.svelte:62`)
-
-**Severity: High**
-
-```js
-accepts: (el, target) => {
-  return target.dataset.rankId !== $cards.find((c) => c.id === el.dataset.cardId).column;
-},
-```
-
-If the card is not found in `$cards` (e.g. the store is mid-update), `.column` throws a `TypeError` and crashes the drag handler.
-
-**Fix:** Guard against a missing card:
-
-```js
-accepts: (el, target) => {
-  const card = $cards.find((c) => c.id === el.dataset.cardId);
-  return card != null && target.dataset.rankId !== card.column;
-},
-```
-
----
-
-### B4 — Null dereference in drag `drop` handler (`src/Board.svelte:80`)
-
-**Severity: High**
-
-Same pattern as B3. `const card = $cards.find(...)` is used immediately without a null check. If the card is absent, subsequent property access throws.
-
-**Fix:**
-
-```js
-drake.on("drop", async (el, target) => {
-  const card = $cards.find((c) => c.id === el.dataset.cardId);
-  if (!card) return;
-  ...
-});
-```
-
----
-
-### B5 — `requestJson` doesn't check HTTP status (`src/api.js:16–18`)
-
-**Severity: Medium**
-
-```js
-async function requestJson(input, init) {
-  return (await fetch(input, init)).json();
-}
-```
-
-On a 4xx or 5xx response, `.json()` either throws a parse error (if the body isn't JSON) or silently decodes the error body as if it were valid data. Callers have no way to distinguish success from failure.
-
-**Fix:**
-
-```js
-async function requestJson(input, init) {
-  const response = await fetch(input, init);
-  if (!response.ok) throw new Error(`HTTP ${response.status}`);
-  return response.json();
-}
-```
-
----
-
-### B6 — Missing null guards in `normaliseCard` (`src/firestore.js:115–116`)
-
-**Severity: Medium**
-
-`data.column.id` and `data.owner.id` are accessed directly without checking that `column` or `owner` exist. If a Firestore document is missing either field (due to a partial write, schema migration, or corrupted data), this throws inside the `onSnapshot` callback and breaks all card subscriptions silently.
-
-**Fix:**
-
-```js
-column: data.column?.id ?? null,
-owner: data.owner?.id === get(uid),
-```
-
----
-
-### B7 — Private Firestore SDK field access (`src/firestore.js:124`)
-
-**Severity: Medium**
-
-```js
-document._document.createTime.timestamp.seconds * 1000
-```
-
-`_document` is an internal, undocumented Firestore SDK field. It can silently change or disappear on any SDK update. The fallback is also non-obvious.
-
-**Fix:** Use `Date.now()` as the fallback rather than reaching into private internals:
-
-```js
-created_at: new Date(data?.created_at?.toMillis() ?? Date.now()),
-```
-
----
-
-### B8 — Race condition in async derived store (`src/store.js:70–76`)
-
-**Severity: Medium**
-
-```js
-export const passwordValid = derived(
-  [board, password],
-  ([$board, $password], set) => {
-    checkBoardPassword($board, $password).then(set);
-  },
-  null,
-);
-```
-
-If `board` or `password` changes rapidly, multiple async calls can be in-flight simultaneously. Whichever resolves last wins — which may not be the most recent call. This can leave `passwordValid` set to a stale result.
-
-**Fix:** Cancel the previous promise by tracking a generation counter:
-
-```js
-export const passwordValid = derived(
-  [board, password],
-  ([$board, $password], set) => {
-    let cancelled = false;
-    checkBoardPassword($board, $password).then((v) => { if (!cancelled) set(v); });
-    return () => { cancelled = true; };
-  },
-  null,
-);
-```
-
----
-
 ## Security
 
 ### S1 — Weak encryption: no key derivation, no authentication (`src/encryption.js:1–20`)

src/App.svelte

@@ -17,20 +17,19 @@
     );
 
     const prefersDarkScheme = window.matchMedia('(prefers-color-scheme: dark)');
-    prefersDarkScheme.addEventListener('change', (e) => {
+    const handleSchemeChange = (e) => {
       const systemPreference = e.matches;
       const appPreference = window.localStorage.getItem('darkModePreference');
-
-      // Update color theme if the user hasn't set an app preference
       if (appPreference) {
         $darkMode = appPreference === 'dark';
       } else {
         $darkMode = systemPreference;
       }
-    });
+    };
+    prefersDarkScheme.addEventListener('change', handleSchemeChange);
     return () => {
       unsubscribeDarkModeChange();
-      prefersDarkScheme.removeEventListener('change', darkModeChangeListener);
+      prefersDarkScheme.removeEventListener('change', handleSchemeChange);
     };
   });
 </script>

src/Board.svelte

@@ -57,10 +57,8 @@
     copy: true,
     moves: (el) => el.dataset.drag !== 'false',
     accepts: (el, target) => {
-      return (
-        target.dataset.rankId !==
-        $cards.find((c) => c.id === el.dataset.cardId).column
-      );
+      const card = $cards.find((c) => c.id === el.dataset.cardId);
+      return card != null && target.dataset.rankId !== card.column;
     },
   });
 
@@ -78,6 +76,7 @@
     const rankId = target.dataset.rankId;
     const cardId = el.dataset.cardId;
     const card = $cards.find((c) => c.id === cardId);
+    if (!card) return;
     const originalRankId = card.column;
 
     el.parentNode.removeChild(el);
@@ -129,7 +128,13 @@
   }
 
   onMount(async () => {
-    const b = await getBoard(boardId);
+    let b;
+    try {
+      b = await getBoard(boardId);
+    } catch {
+      navigate('/not-found');
+      return;
+    }
 
     if (b.error == 'Not Found') {
       navigate('/not-found');
@@ -150,10 +155,8 @@
     let previousBoard = { ...$board };
     if ($board.owner || $board.open_permission) {
       unsubscribeLocalBoard = board.subscribe((b) => {
-        try {
-          if (!compareBoards(previousBoard, b)) updateBoard(b);
-        } catch (err) {
-          error('error.updating_settings', err);
+        if (!compareBoards(previousBoard, b)) {
+          updateBoard(b).catch((err) => error('error.updating_settings', err));
         }
         previousBoard = { ...b };
       });

src/api.js

@@ -14,7 +14,9 @@ async function request(input, init) {
 }
 
 async function requestJson(input, init) {
-  return (await fetch(input, init)).json();
+  const response = await fetch(input, init);
+  if (!response.ok) throw new Error(`HTTP ${response.status}`);
+  return response.json();
 }
 
 export async function createAuthToken() {

src/components/Menu.svelte

@@ -1,5 +1,6 @@
 <script>
   import ClipboardJS from 'clipboard';
+  import { onMount } from 'svelte';
   import { _ } from 'svelte-i18n';
   import { fly } from 'svelte/transition';
   import {
@@ -39,7 +40,11 @@
     if ($board.data?.timer_end_at != null) showTimer = true;
   });
 
-  new ClipboardJS('button');
+  let clipboard;
+  onMount(() => {
+    clipboard = new ClipboardJS('[data-clipboard-text]');
+    return () => clipboard.destroy();
+  });
 
   const preventDefault = (e) => {
     e.preventDefault();

src/firestore.js

@@ -112,17 +112,14 @@ function normaliseCard(document) {
     ...data,
     id: document.id,
     votes: data?.votes?.length ?? 0,
-    column: data.column.id,
-    owner: data.owner.id === get(uid),
+    column: data.column?.id ?? null,
+    owner: data.owner?.id === get(uid),
     voted:
       (data?.votes ?? []).find((voteDoc) => voteDoc.id === get(uid)) !==
       undefined,
     reactions,
     reacted,
-    created_at: new Date(
-      data?.created_at?.toMillis() ?? // Added 16 Mar 2023
-        document._document.createTime.timestamp.seconds * 1000 // Bit of a hack
-    ),
+    created_at: new Date(data?.created_at?.toMillis() ?? Date.now()),
   };
 }
 

src/store.js

@@ -70,7 +70,13 @@ export const colors = derived(colorMode, ($colorMode) => Colors[$colorMode]);
 export const passwordValid = derived(
   [board, password],
   ([$board, $password], set) => {
-    checkBoardPassword($board, $password).then(set);
+    let cancelled = false;
+    checkBoardPassword($board, $password).then((v) => {
+      if (!cancelled) set(v);
+    });
+    return () => {
+      cancelled = true;
+    };
   },
   null
 );