docs(bugs): mark N113/N114/N115/N122/N123/N126 FIXED

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-28 09:18:52 -04:00
parent 7013da70bc
commit adbda094e7
1 changed files with 50 additions and 22 deletions
@@ -519,6 +519,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N105 — Missing SW `notificationclick` handler: notification clicks broken when tab is closed**
+
 - **File:** `src/sw.ts` (handler entirely absent); `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 151–155 (`InviteNotifications`) and 277–284 (`MessageNotifications`)
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** All notification click handling is wired via `noti.onclick` in the main thread (`noti.onclick = () => { navigate(...); noti.close(); }`). This callback only fires while the originating tab is open and its JavaScript is running. When the browser has no open tabs for the app (or the tab is suspended/backgrounded), clicking an OS notification does nothing — there is no SW `notificationclick` handler to focus an existing window or open a new one and navigate to the correct room.
@@ -528,6 +529,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N106 — Decrypted E2EE message plaintext leaked to OS notification center**
+
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, line 343
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `MessageNotifications` component passes `mEvent.getContent().body` directly as the notification body: `body: (mEvent.getContent().body as string | undefined) ?? ''`. By the time `RoomEvent.Timeline` fires, `matrix-js-sdk` has already decrypted the event in memory. The fully decrypted plaintext is then handed to `new window.Notification()`, which stores it in the OS notification center. This plaintext is visible on the device lock screen (if notification previews are enabled), in the OS notification history, and may be read by any app with `READ_NOTIFICATIONS` permission (e.g., accessibility services, backup apps) — even when the room uses end-to-end encryption. The 120-character slice (`slice(0, 120)`) does not mitigate this.
@@ -537,6 +539,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N107 — SW has no `push` event handler: Web Push delivery is completely broken**
+
 - **File:** `src/sw.ts` (handler entirely absent)
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The service worker never registers a `push` event listener. If a Matrix push gateway (e.g., Sygnal) is ever configured and sends a Web Push notification, the SW silently discards the push event — no notification is shown, no in-app routing occurs. The absence of a `push` handler means the entire background-notification path (i.e., notifications when no tab is open) is non-functional, which is one of the primary requirements for a PWA.
@@ -546,6 +549,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N108 — No maskable icon in PWA manifest: Android adaptive icons display incorrectly**
+
 - **File:** `public/manifest.json`, lines 12–57
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The manifest lists nine `android-chrome-*.png` icons (36 × 36 through 512 × 512) but none include `"purpose": "maskable"`. Android 8+ adaptive icons apply a platform-defined shape mask (circle, squircle, teardrop, etc.) to PWA home-screen icons. Without a maskable-purpose icon, the OS either adds a white square background to prevent clipping or applies the mask directly to the regular icon, typically cropping the Lotus logo in a visually incorrect way.
@@ -555,13 +559,13 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N109 — Authenticated media URLs passed to `Notification` icon/badge: OS cannot fetch them (produces 401)**
+
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 333–339 and 270–273
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** When the homeserver requires authenticated media (Matrix spec v1.11+, path `/_matrix/client/v1/media/download/...`), `mxcUrlToHttp(mx, avatarMxc, useAuthentication, 96, 96, 'crop')` returns an authenticated URL. That URL is then passed directly as `icon` and `badge` to `new window.Notification()`. The OS/browser notification subsystem fetches `icon` and `badge` URLs directly — outside the page's JavaScript context — so the service worker's `fetch` handler never fires for them (the SW only intercepts fetches with a valid `event.clientId`, which these OS-initiated fetches lack). The homeserver returns HTTP 401, and the notification shows no icon or badge.
 - **Root Cause:** The SW auth-header injection is designed for page-initiated `/_matrix/client/v1/media/` fetches. It does not (and cannot) intercept fetches made by the OS notification subsystem. Room avatar URLs are passed to `Notification` without first converting them to an auth-agnostic form.
 - **Fix:** Before creating a `Notification`, fetch the avatar URL in-page (via the existing authenticated fetch path where the SW can inject headers), convert the response to a Blob URL (`URL.createObjectURL(blob)`), and pass the Blob URL as `icon`/`badge`. Alternatively, skip the avatar for notifications entirely and use the static app logo (already done for invite notifications via `LogoSVG`) to avoid the authenticated-media complexity.

-
 ## 🌸 Lotus Feature Internals Audit (Wave 2)

 > Deep audit of Lotus-specific hook internals, build scripts, and the avatar-decoration pipeline. All findings below are **[Claude_Found]**.
@@ -569,8 +573,9 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N113 — `addReminder`/`removeReminder` Read-Modify-Write Race Condition**
+
 - **File:** `src/app/hooks/useReminders.ts`, lines 52–68
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`7013da70`) — mutations compute from a ref synced to server echoes; writes serialized via a promise queue.
 - **Issue:** Both `addReminder` and `removeReminder` call `readReminders(mx)` — a synchronous read from the Matrix client's local account-data cache — and then fire `setAccountData` asynchronously. If two calls overlap before either write has committed and the local cache updated (e.g. a user quickly adds two reminders, or adds one while a removal is in flight), both calls read the same stale baseline and the second write silently overwrites the first. Example: adding R1 and R2 in quick succession → both calls read `[]`, write `[R1]` and `[R2]` respectively → only R2 survives, R1 is lost.
 - **Root Cause:** No optimistic locking, no serial queue, and the read source (`mx.getAccountData()`) does not reflect uncommitted in-flight writes.
 - **Fix:** Use the React `reminders` state (passed as a parameter or captured in a `useRef`) as the source of truth for mutations instead of re-reading from the client cache. Alternatively, serialize writes through a promise queue so each `addReminder`/`removeReminder` awaits the previous `setAccountData` before computing the next state.
@@ -578,8 +583,9 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N114 — `ReminderMonitor` Calls `removeReminder` Fire-and-Forget; Network Failure Silently Drops the Reminder**
+
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 399, 413–414
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`7013da70`) — toast fires once (firedRef); removal retried on later ticks via removingRef, released on failure.
 - **Issue:** Inside `ReminderMonitor.check()`, when a reminder fires the code immediately does `firedRef.current.add(key)` and then calls `removeReminder(r.eventId, r.timestamp)` without `await` and without a `.catch()` handler. If `removeReminder` fails (network error, 429 rate-limit, homeserver down), the reminder remains in account data but is permanently blocked from re-firing this session because its key is already in `firedRef`. The user's reminder is silently swallowed for the rest of the session; only a full page reload recovers it.
 - **Root Cause:** The promise returned by `removeReminder` is discarded. There is no error path that rolls back `firedRef.current` or reschedules the reminder for retry.
 - **Fix:** Make `check` an `async` function (or add a `.catch()` on the call), and only add to `firedRef` after `removeReminder` succeeds. On failure, omit the `firedRef` add so the reminder retries on the next poll tick.
@@ -587,8 +593,9 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N115 — `ReminderMonitor` 30 s Poll Interval Is Reset on Every `reminders` State Change, Delaying Near-Due Reminders**
+
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 394–428
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`7013da70`) — reminders/mDirects read via refs; dropped from effect deps so the 30s interval is created once.
 - **Issue:** `reminders` is listed in the `useEffect` dependency array (`}, [mx, reminders, setToast, removeReminder, mDirects]`). Every time a reminder is added, removed, or synced back from the server, React tears down the effect (clearing `setInterval`) and re-creates it, resetting the 30 s countdown from zero. A reminder due 1 s from now will not fire for up to 30 s if a reminder state change occurs 0.5 s before the due time — for instance, when the server's account-data echo arrives and updates `reminders`. In the worst case, rapid add/remove cycles can continuously defer the poll indefinitely (as long as new mutations keep arriving faster than 30 s).
 - **Root Cause:** `check()` closes over `reminders`, requiring it as a dependency; but the interval itself does not need to be recreated on every reminder change — only the closure does.
 - **Fix:** Store the latest `reminders` value in a `useRef` updated on each render, and read from the ref inside `check()`. Remove `reminders` from the `useEffect` dependency array. The interval is then created once per `mx`/handler change, and `check()` always sees the current snapshot via the ref.
@@ -596,6 +603,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N116 — `useCallSpeakers` Speaker Set Rebuilt From Mutation Batch Only — All Other Speaking Participants Are Dropped**
+
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 20–44
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `MutationObserver` callback builds a fresh `Set<string>` from only the tiles present in the current mutation batch, then calls `setSpeakers(s)`. If participant A has been speaking for 10 s but their tile has not mutated recently, and participant B's tile mutates for an unrelated reason (e.g. a class change), the batch contains only B's tile. Even if B is not speaking, `s` is empty and `setSpeakers(s)` replaces the entire state — A disappears from the speakers set despite still speaking. The result is a constantly-flickering or always-empty speakers indicator.
@@ -605,6 +613,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N117 — `useCallSpeakers` Static `querySelectorAll` NodeList Misses Video Tiles Added to EC DOM Mid-Call**
+
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 14–17
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `callEmbed.document?.querySelectorAll('[data-video-fit]')` returns a static `NodeList` snapshot at the instant the `useMemo` evaluates. When a new participant joins mid-call and EC renders their video tile, that tile is not in the captured list. No `MutationObserver` is ever attached to the new tile, so the new participant can never be detected as a speaker for the remainder of the call. `callMembers` is a memo dependency and does update on join/leave, but there is a timing gap: `callMembers` may change before EC has finished rendering the new tile inside the iframe, so `querySelectorAll` at that moment still does not find the new tile.
@@ -614,6 +623,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N118 — `useCallSpeakers` Relies on Three Layers of Undocumented EC Internal APIs**
+
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 15, 28–35
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Speaker detection depends on three private Element Call implementation details that are not part of any stable EC API contract and can silently break on any EC version bump:
@@ -627,6 +637,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N119 — `syncDecorations.mjs` Treats Network Errors the Same as 404 — CDN Outage Silently Wipes Entire Catalog**
+
 - **File:** `scripts/syncDecorations.mjs`, lines 39–46, 56–65
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `headCheck` catches all fetch exceptions (DNS failure, timeout, CORS error, TLS failure) and returns `{ ok: false, status: 0 }`. This is structurally identical to an HTTP 404 (`{ ok: false, status: 404 }`). The script classifies all non-ok results as "missing" and removes them from `avatarDecorations.ts`. If `drive.lotusguild.org` is temporarily unreachable when a developer runs `npm run sync:decorations`, every single decoration fails the HEAD check with `status: 0`, is marked missing, and is removed. The script writes an empty `avatarDecorations.ts`, logs "Done. Removed N entries from the catalog.", and exits 0 — permanently destroying the catalog in source control with no warning.
@@ -636,6 +647,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N120 — CDN URL Hard-Coded Separately in `syncDecorations.mjs` and `avatarDecorations.ts` — Can Drift**
+
 - **File:** `scripts/syncDecorations.mjs`, line 24; `src/app/features/lotus/avatarDecorations.ts`, lines 1–2
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The Nextcloud CDN base URL (including the embedded share token `bHswJ9pNKp2t26N`) is defined twice: as `const CDN` in the sync script and as `export const DECORATION_CDN` in the runtime catalog. If the CDN is migrated (new provider, new Nextcloud share, rotated token), a developer must update both files. Missing one means the sync script probes the old URL while the runtime client fetches from the new one (or vice versa), silently producing a catalog that references unreachable assets. There is no test or lint check that enforces parity.
@@ -645,6 +657,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 ---

 **N128 — `patch-folds.mjs` Emits `console.warn` Instead of `process.exit(1)` When Patch Target Is Not Found**
+
 - **File:** `scripts/patch-folds.mjs`, lines 21–23
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** When the target string `children: src(filled)` is not found in `node_modules/folds/dist/index.js` — because folds shipped an update that renamed or restructured this code path — the script logs `Warning: folds Icon patch target not found - may need updating.` and exits with code 0. The `postinstall` npm hook considers the install successful. The production build then ships the unpatched folds, where passing a non-function as `src` to `<Icon>` causes a runtime `TypeError: src is not a function` at any call site that relies on the guard. The failure is invisible at build and install time; it manifests only when the affected UI is rendered in production.
@@ -689,8 +702,8 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 - **Root Cause:** `localStorage.clear()` was chosen as a one-line logout implementation rather than selectively removing only the four session credential keys. No distinction is made between "end the session" and "factory reset."
 - **Fix:** Replace `window.localStorage.clear()` in both `logoutClient` (line 78) and `handleLogout` (line 133) with targeted removal of only the session credential keys:
  ```typescript
-  ['cinny_access_token', 'cinny_device_id', 'cinny_user_id', 'cinny_hs_base_url'].forEach(k =>
-    window.localStorage.removeItem(k)
+  ['cinny_access_token', 'cinny_device_id', 'cinny_user_id', 'cinny_hs_base_url'].forEach((k) =>
+    window.localStorage.removeItem(k),
  );
  ```
  Leave `settings`, draft keys, and all other preference keys intact. Reserve `window.localStorage.clear()` for the `clearLoginData()` path only.
@@ -710,10 +723,13 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 - **Root Cause:** The `useSyncState` callback is designed around a single happy-path terminal state (`PREPARED`). `SyncStatus` handles error states for the **post-PREPARED** reconnection UX, but does not replace the loading screen.
 - **Fix:** Extend the `useSyncState` callback to handle `SyncState.Error` and `SyncState.Stopped` by setting a separate `syncError` state, then render a dedicated error splash (parallel to the existing `loadState`/`startState` error dialog at lines 193–238) that shows a descriptive message and a Retry button that calls `startMatrix(mx)`:
  ```tsx
-  useSyncState(mx, useCallback((state) => {
+  useSyncState(
+    mx,
+    useCallback((state) => {
      if (state === 'PREPARED') setLoading(false);
      else if (state === 'ERROR' || state === 'STOPPED') setSyncError(true);
-  }, []));
+    }, []),
+  );
  ```

 ---
@@ -760,7 +776,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 **N122 — `setMediaState` promise hangs permanently when EC omits a `DeviceMute` state-echo**

 - **File:** `src/app/plugins/call/CallControl.ts`, lines 185–193
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`49d9410e`) — setMediaState resolves on EC transport ACK; dropped the single-slot mediaStatePromiseResolver.
 - **Issue:** The Promise returned by `setMediaState` can never resolve if EC does not emit a `DeviceMute` `fromWidget` state-update event in response to the host's mute command. After `await this.call.transport.send(ElementWidgetActions.DeviceMute, state)` resolves (EC has ACK'd the command), the function creates an inner Promise whose resolver is stored in `this.mediaStatePromiseResolver` — a field consumed only by `onMediaState` or by the NEXT call to `setMediaState`. If EC ACKs the command but does not subsequently fire a `DeviceMute` state-report back (the most likely trigger: the requested state already matches EC's current state and EC elides the echo, or EC is shutting down before broadcasting), the inner Promise is stranded forever. `applyState()` awaits this Promise at line 118 (`await this.setMediaState({...})`); the subsequent `this.setSound(this.sound)` and `this.emitStateUpdate()` calls at lines 122–123 are never reached. Because `forceState` (which calls `applyState`) is invoked fire-and-forget from `onCallJoined`, the practical result is that the initial deafen state and the first `StateUpdate` event emission are silently skipped on every call join when EC batches or omits the echo.
 - **Root Cause:** The single-slot `mediaStatePromiseResolver` architecture gates the mute operation's completion on an EC-originated event that is not guaranteed to fire for every host-initiated command.
 - **Fix:** Resolve the inner Promise directly when `transport.send()` returns — EC having replied already confirms the command was received and applied. Drop the `new Promise(...)` wrapper and return `data` immediately after `await transport.send()`. Keep `onMediaState` as the authoritative state-sync path (updating `this.state` and calling `emitStateUpdate`) but remove the `mediaStatePromiseResolver` field and its invocation from that handler entirely.
@@ -770,7 +786,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 **N123 — `focusCameraParticipant` tile click silently drops when EC spotlight layout isn't ready in 2 animation frames**

 - **File:** `src/app/plugins/call/CallControl.ts`, lines 396–401
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`49d9410e`) — MutationObserver waits for a spotlight videoTile (600ms timeout fallback) instead of a fixed 2-frame delay.
 - **Issue:** After clicking `spotlightButton` to enter spotlight mode, `focusCameraParticipant` waits exactly two `requestAnimationFrame` callbacks (~32 ms at 60 fps) before querying the EC document for the target tile. If EC's React tree has not committed new spotlight tile nodes within that window — which occurs regularly on slower devices, during animated layout transitions, or when EC is simultaneously decoding video streams — `findTile()` returns `undefined` and the focus action is silently dropped. The user sees EC switch to spotlight mode but the requested participant is never pinned. There is no retry, no surfaced error, and the only signal is a DEV-only `console.warn`.
 - **Root Cause:** The double-rAF heuristic is a timing approximation, not a DOM-readiness guarantee. EC's React reconciliation and layout commit can exceed 32 ms.
 - **Fix:** Replace the double-rAF with a `MutationObserver` on `this.document.body` (childList + subtree) that waits for a `[data-testid="videoTile"]` element to appear, then calls `applyFocus()` and disconnects. Add a 600 ms hard-timeout fallback that calls `applyFocus()` and disconnects regardless, so the click is always attempted at least once even when tile rendering is slow.
@@ -785,7 +801,9 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 - **Root Cause:** `gateNode` is in closure scope but missing from the `cleanup()` body.
 - **Fix:** Add to `cleanup()`:
  ```javascript
-  try { if (gateNode) gateNode.disconnect(); } catch (e) {}
+  try {
+    if (gateNode) gateNode.disconnect();
+  } catch (e) {}
  ```

 ---
@@ -807,7 +825,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 **N126 — PiP position restored from `localStorage` without type validation, silently producing `NaN` coordinates on corrupt data**

 - **File:** `src/app/components/CallEmbedProvider.tsx`, line 723
- **Status:** **OPEN** [Claude_Found]
+- **Status:** **FIXED** (`49d9410e`) — saved PiP position shape+finiteness validated before use; corrupt data falls back to default.
 - **Issue:** The saved PiP position is cast without runtime validation:
  ```typescript
  const savedPos = saved ? (JSON.parse(saved) as { left: number; top: number }) : null;
@@ -816,11 +834,21 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 - **Root Cause:** TypeScript `as` casts do not validate at runtime; the parsed value's shape is never checked.
 - **Fix:** Add an explicit shape-and-finite guard:
  ```typescript
-  const raw = saved ? (() => { try { return JSON.parse(saved); } catch { return null; } })() : null;
+  const raw = saved
+    ? (() => {
+        try {
+          return JSON.parse(saved);
+        } catch {
+          return null;
+        }
+      })()
+    : null;
  const savedPos =
    raw != null &&
-    typeof raw.left === 'number' && isFinite(raw.left) &&
-    typeof raw.top  === 'number' && isFinite(raw.top)
+    typeof raw.left === 'number' &&
+    isFinite(raw.left) &&
+    typeof raw.top === 'number' &&
+    isFinite(raw.top)
      ? (raw as { left: number; top: number })
      : null;
  ```