docs(bugs): mark N116/N117/N120/N124/N125/N128 FIXED
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+6
-6
@@ -605,7 +605,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]
|
||||
- **Status:** **FIXED** (`19feca49`) — body-level MutationObserver rescans all `[data-video-fit]` tiles each mutation (mirrors `useRemoteAllMuted`); no longer drops still-speaking participants.
|
||||
- **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.
|
||||
- **Root Cause:** Speaker state is derived from the delta (mutation batch) rather than the full current DOM state. Compare with `useRemoteAllMuted.syncState()` in the same file, which correctly re-scans all `[data-muted]` elements on every mutation rather than looking only at the mutated ones.
|
||||
- **Fix:** Replace the per-batch iteration with a full re-scan of all observed tiles on each callback: iterate all elements in `videoContainers`, check each for the `::before` speaking indicator, and build the new `Set` from currently-speaking tiles — not just the mutated ones.
|
||||
@@ -615,7 +615,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]
|
||||
- **Status:** **FIXED** (`19feca49`) — replaced the static NodeList + per-tile observers with a single body-level observer, so mid-call tiles are covered.
|
||||
- **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.
|
||||
- **Root Cause:** Observing a static snapshot of tiles does not compose with EC's dynamically-updating DOM. `useRemoteAllMuted` avoids this entirely by watching `doc.body` with `{ subtree: true, childList: true }`, which automatically picks up new tiles without re-querying.
|
||||
- **Fix:** Replace the static-NodeList + per-tile-observer approach with a single body-level observer (same as `useRemoteAllMuted`), and re-scan all `[data-video-fit]` tiles on each relevant mutation.
|
||||
@@ -649,7 +649,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]
|
||||
- **Status:** **FIXED** (`ce8a03ab`) — CDN single-sourced as `DECORATION_CDN` in `avatarDecorations.ts`; `syncDecorations.mjs` extracts it by regex and fails hard if renamed.
|
||||
- **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.
|
||||
- **Root Cause:** `syncDecorations.mjs` is a plain `.mjs` script that cannot directly `import` from a `.ts` source file at runtime, so the constant was copied instead of shared.
|
||||
- **Fix:** Extract the CDN URL into a shared `.mjs` config file (e.g. `scripts/decorationConfig.mjs`) that `syncDecorations.mjs` imports directly. Have `avatarDecorations.ts` read the same value at build time (via a Vite define/import, or by making the script write the constant into `avatarDecorations.ts` rather than hardcoding it). Alternatively, add a CI step that `grep`s both files and fails if the URLs differ.
|
||||
@@ -659,7 +659,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]
|
||||
- **Status:** **FIXED** (`ce8a03ab`) — `process.exit(1)` on a genuine missing patch target; idempotent already-applied path still exits 0.
|
||||
- **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.
|
||||
- **Root Cause:** The mismatch branch uses `console.warn` (exit 0) rather than `process.exit(1)`, treating a broken build pre-requisite as a non-fatal advisory.
|
||||
- **Fix:** Replace the `console.warn(...)` + implicit exit-0 with `console.error(...)` followed by `process.exit(1)`. This causes `npm install` (and CI) to fail loudly, forcing the developer to update the patch target string before the build can proceed. The "already applied" branch (line 15) correctly exits 0 and does not need to change.
|
||||
@@ -796,7 +796,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
|
||||
**N124 — Denoise shim `cleanup()` leaks the noise gate `AudioWorkletNode` processor thread when `USE_GATE=true`**
|
||||
|
||||
- **File:** `build/lotus-denoise.js`, lines 235–244 and 267–281
|
||||
- **Status:** **OPEN** [Claude_Found]
|
||||
- **Status:** **FIXED** (`ce8a03ab`) — `cleanup()` now disconnects `gateNode` (guarded), releasing the gate processor thread.
|
||||
- **Issue:** When the noise gate is active (`USE_GATE=true`), `processStream` creates a `gateNode` (`AudioWorkletNode`) and wires it as `source → gateNode → mlNode → dest`. The `cleanup()` closure inside the inner `.then()` callback calls `source.disconnect()` and `mlNode.disconnect()` but never `gateNode.disconnect()`. `gateNode` is declared with `var` inside the outer `if (USE_GATE)` block — hoisted via `var` to the enclosing `.then()` function scope — and IS accessible in the inner callback via closure, but is simply absent from `cleanup()`. The AudioWorklet processor thread for the orphaned gate node continues running on the audio rendering thread until the EC iframe is destroyed. If EC's LiveKit client calls `getUserMedia` more than once within a session (e.g., a device switch mid-call), a new orphaned gate processor accumulates on each call, each consuming audio-thread CPU indefinitely.
|
||||
- **Root Cause:** `gateNode` is in closure scope but missing from the `cleanup()` body.
|
||||
- **Fix:** Add to `cleanup()`:
|
||||
@@ -811,7 +811,7 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
|
||||
**N125 — Denoise shim `postMessage` uses wildcard `'*'` target origin**
|
||||
|
||||
- **File:** `build/lotus-denoise.js`, lines 294–306 and 317–320
|
||||
- **Status:** **OPEN** [Claude_Found]
|
||||
- **Status:** **FIXED** (`ce8a03ab`) — `postMessage` targets the parentUrl-derived origin (fallback to own origin) instead of `*`.
|
||||
- **Issue:** Both `lotus-denoise-status` `postMessage` calls use `'*'` as the `targetOrigin` argument, broadcasting the message to any frame that currently contains the EC iframe as a child regardless of its origin. If the Lotus EC widget URL is ever embedded by a third-party page (possible since it is same-origin and publicly routable), that page receives the denoise status payload (`{ type, active, model, nativeNS, gate }`). Using `'*'` violates the MDN/W3C `postMessage` security recommendation.
|
||||
- **Root Cause:** The shim has no reference to the parent origin at the point these calls are made. The `parentUrl` widget URL parameter — already present in `window.location.search` and parsed into `params` at line 27 — provides the correct target origin.
|
||||
- **Fix:** Extract `parentUrl` from `params` and use it as the target origin:
|
||||
|
||||
Reference in New Issue
Block a user