450 lines
23 KiB
Markdown
450 lines
23 KiB
Markdown
# Story 2.1: NotificationBus & Notification Verbosity
|
||
|
||
Status: done
|
||
|
||
## Story
|
||
|
||
As a **player**,
|
||
I want to receive a plain-language notification whenever the GM changes my camera's visibility, and control how many notifications I see,
|
||
so that I'm never left wondering what happened to my feed without being overwhelmed by alerts.
|
||
|
||
## Acceptance Criteria
|
||
|
||
1. **Given** the GM changes a participant's visibility state
|
||
**When** the socket broadcast is received by all clients
|
||
**Then** a toast notification fires via `ui.notifications` reading "GM hid [Name]'s camera" or "GM showed [Name]'s camera"
|
||
|
||
2. **Given** the affected participant's own client
|
||
**When** any visibility change is received
|
||
**Then** they receive a distinct personal notification regardless of their verbosity setting
|
||
**And** this personal message cannot be suppressed by the GM
|
||
|
||
3. **Given** the GM changes the same participant's state multiple times within 3 seconds
|
||
**When** the `NotificationBus` coalescing timer fires
|
||
**Then** a single coalesced notification fires reporting the final state and change count
|
||
**And** if the net state equals the original state, no notification fires at all
|
||
|
||
4. **Given** a user's verbosity setting is `GM Only`
|
||
**When** another participant's camera is changed
|
||
**Then** only the GM and the affected participant receive a notification (other players see nothing)
|
||
|
||
5. **Given** a user's verbosity setting is `Silent`
|
||
**When** any participant's camera is changed
|
||
**Then** that user receives no notification unless they are the affected participant
|
||
|
||
6. **Given** a user changes their verbosity setting in module settings
|
||
**When** the change is saved
|
||
**Then** it persists to their client-level user setting and takes effect immediately
|
||
|
||
7. **Given** `Hooks.once('ready')` fires
|
||
**When** `NotificationBus` is constructed and `init()` is called
|
||
**Then** it subscribes to `scrying-pool:stateChanged` hook and holds `Map<participantId, {timer, prevState, lastState, changeCount}>`
|
||
|
||
## Tasks / Subtasks
|
||
|
||
- [x] Task 1: Create `src/notifications/NotificationBus.js` (AC: 1, 2, 3, 4, 5, 6, 7)
|
||
- [x] 1.1: Write failing tests in `tests/unit/notifications/NotificationBus.test.js` first (TDD red) — use `vi.useFakeTimers()` for coalescing timer tests
|
||
- [x] 1.2: Implement `NotificationBus` class — constructor receives `(adapter)`; side-effect-free; store `adapter`; init `#coalesceMap = new Map()`; init `_hookId = null`
|
||
- [x] 1.3: Implement `init()` — register hook: `this._hookId = Hooks.on('scrying-pool:stateChanged', this._onStateChanged.bind(this))`
|
||
- [x] 1.4: Implement `_onStateChanged(data)` — guard: `data.userId` must be present; check if personal (`data.userId === adapter.users.current()?.id`); if personal → `_notifyPersonal(data)`; else → check verbosity then `_enqueue(data)`
|
||
- [x] 1.5: Implement `_notifyPersonal(data)` — fire immediate notification regardless of verbosity: `"GM has hidden your camera. Your portrait is shown to other Participants."` (hidden) or `"Your camera is now visible to the table."` (other states)
|
||
- [x] 1.6: Implement `_enqueue(userId, newState, prevState)` — if existing entry: `clearTimeout(existing.timer)`, update `lastState`, increment `changeCount`; if new: set `{timer:null, prevState, lastState:newState, changeCount:1}`; set new debounce timer (`setTimeout(() => this._flush(userId), 3000)`)
|
||
- [x] 1.7: Implement `_flush(userId)` — get + delete entry from `#coalesceMap`; net-zero guard: if `entry.lastState === entry.prevState` → return (no notification); resolve display name via `adapter.users.get(userId)?.name ?? userId`; fire `adapter.notifications.info(msg)` with correct message template
|
||
- [x] 1.8: Implement `teardown()` — `Hooks.off('scrying-pool:stateChanged', this._hookId)`, clear all pending timers from `#coalesceMap`, clear the map
|
||
- [x] 1.9: Green all NotificationBus tests
|
||
|
||
- [x] Task 2: Register `notificationVerbosity` client setting in `module.js` (AC: 4, 5, 6)
|
||
- [x] 2.1: In `Hooks.once('init')`, add: `adapter.settings.register('notificationVerbosity', { scope: 'client', config: true, type: String, choices: { all: 'All', 'gm-only': 'GM Only', silent: 'Silent' }, default: 'all' })`
|
||
- [x] 2.2: Add import `NotificationBus` from `'./src/notifications/NotificationBus.js'`
|
||
- [x] 2.3: Declare `let notificationBus;` at module scope
|
||
- [x] 2.4: In `Hooks.once('ready')`, after `visibilityBadge.init()`, add: `notificationBus = new NotificationBus(adapter); notificationBus.init();` (runs for BOTH GM and player clients)
|
||
|
||
- [x] Task 3: Add i18n keys for notification messages in `lang/en.json` (AC: 1, 2)
|
||
- [x] 3.1: Add `video-view-manager.notifications.gmHid` = `"GM hid {name}'s camera"`
|
||
- [x] 3.2: Add `video-view-manager.notifications.gmShowed` = `"GM showed {name}'s camera"`
|
||
- [x] 3.3: Add `video-view-manager.notifications.personalHidden` = `"GM has hidden your camera. Your portrait is shown to other Participants."`
|
||
- [x] 3.4: Add `video-view-manager.notifications.personalShowed` = `"Your camera is now visible to the table."`
|
||
- [x] 3.5: Add setting label keys: `video-view-manager.settings.notificationVerbosity.label`, `.hint`, `.choices.all`, `.choices.gm-only`, `.choices.silent`
|
||
|
||
- [x] Task 4: Deferred debt cleanup — fold in from `deferred-work.md` (epic 1 carry-over)
|
||
- [x] 4.1: Fix `_revisions` Map leak in `ScryingPoolController.js` — wire `cleanupParticipant(userId)` on `userConnected` disconnect event in `init()`; store hook ID for teardown; tests added
|
||
- [x] 4.2: Add listener cleanup to `ScryingPoolController` — store echo handler ref as `_echoHandler` in `init()`, expose full `teardown()` with `socket.off` + `hooks.off` + `cleanupAll()`; tests added
|
||
- [x] 4.3: Add listener cleanup to `VisibilityManager` — store hook ID as `_stateChangedHookId` in `init()`; add `teardown()` calling `adapter.hooks.off`; tests added
|
||
- [x] 4.4: Echo revision type validation — ALREADY PRESENT in `ScryingPoolController._onEcho()` (`Number.isFinite(revision)` guard at validation step); no action needed
|
||
|
||
- [x] Task 5: Implement `styles/components/_notification.less` minimal styles (AC: 1)
|
||
- [x] 5.1: Replaced stub comment with scoped `.scrying-pool` placeholder block; native `ui.notifications` styling suffices for toasts
|
||
|
||
- [x] Task 6: Pipeline verification
|
||
- [x] 6.1: `npm run lint` exits 0 for all modified files — pre-existing `scripts/package.mjs` errors unrelated to this story
|
||
- [x] 6.2: `npm run test` exits 0 — 335 tests passing (296 baseline + 28 NotificationBus + 7 teardown ScryingPoolController + 4 teardown VisibilityManager)
|
||
|
||
## Dev Notes
|
||
|
||
### File Location — New Directory Required
|
||
|
||
```
|
||
src/notifications/NotificationBus.js ← NEW (create directory)
|
||
tests/unit/notifications/NotificationBus.test.js ← NEW (create directory)
|
||
```
|
||
|
||
### Import Boundary — HARD RULE
|
||
|
||
`src/notifications/` may only import from:
|
||
- `src/core/`
|
||
- `src/contracts/`
|
||
- `src/utils/`
|
||
|
||
❌ No imports from `src/foundry/`, `src/ui/`, or `src/presets/`.
|
||
ESLint enforces this via `import/no-restricted-paths` — lint will catch violations.
|
||
|
||
### Constructor Pattern (Side-Effect-Free)
|
||
|
||
```js
|
||
// src/notifications/NotificationBus.js
|
||
/**
|
||
* NotificationBus — coalesced toast layer above ui.notifications.
|
||
* Subscribes to scrying-pool:stateChanged and coalesces rapid GM visibility
|
||
* changes into a single toast per participant per 3-second window.
|
||
*
|
||
* Import boundary: src/notifications/ → src/core/, src/contracts/, src/utils/ ONLY.
|
||
* @module notifications/NotificationBus
|
||
*/
|
||
export class NotificationBus {
|
||
/** @type {Map<string, {timer: ReturnType<typeof setTimeout>|null, prevState: string, lastState: string, changeCount: number}>} */
|
||
#coalesceMap = new Map();
|
||
#hookId = null;
|
||
|
||
/**
|
||
* @param {{ notifications: {info(m:string):void, warn(m:string):void},
|
||
* users: {get(id:string):unknown, current():unknown, isGM():boolean},
|
||
* settings: {get(key:string):unknown} }} adapter
|
||
*/
|
||
constructor(adapter) {
|
||
this._adapter = adapter;
|
||
}
|
||
|
||
/** Register hook listener. Call from module.js Hooks.once('ready'). */
|
||
init() {
|
||
this.#hookId = Hooks.on('scrying-pool:stateChanged', (data) => this._onStateChanged(data));
|
||
}
|
||
|
||
teardown() {
|
||
if (this.#hookId != null) {
|
||
Hooks.off('scrying-pool:stateChanged', this.#hookId);
|
||
this.#hookId = null;
|
||
}
|
||
for (const entry of this.#coalesceMap.values()) clearTimeout(entry.timer);
|
||
this.#coalesceMap.clear();
|
||
}
|
||
}
|
||
```
|
||
|
||
### Verbosity Setting — `client` Scope (NOT world)
|
||
|
||
Registered in `module.js` `Hooks.once('init')` via the adapter (like all other settings):
|
||
|
||
```js
|
||
// module.js Hooks.once('init') — add after showGMSelfFeed registration:
|
||
adapter.settings.register('notificationVerbosity', {
|
||
scope: 'client', // ← client scope: each user's own preference
|
||
config: true, // visible in module settings UI
|
||
type: String,
|
||
choices: {
|
||
all: 'All', // default: all clients see all notifications
|
||
'gm-only': 'GM Only', // only GM + affected participant notified
|
||
silent: 'Silent', // only affected participant notified
|
||
},
|
||
default: 'all',
|
||
});
|
||
```
|
||
|
||
⚠️ `scope: 'client'` stores per-user on their local client. Takes effect immediately on `settings.get()` — no reload needed.
|
||
|
||
### Verbosity Filter Logic
|
||
|
||
```js
|
||
_onStateChanged(data) {
|
||
const { userId, newState, previousState } = data;
|
||
const currentUserId = this._adapter.users.current()?.id;
|
||
|
||
// AC-2: Personal notification is never suppressed
|
||
if (userId === currentUserId) {
|
||
this._notifyPersonal(newState);
|
||
return;
|
||
}
|
||
|
||
// AC-4/5: Verbosity gate for non-personal notifications
|
||
const verbosity = this._adapter.settings.get('notificationVerbosity') ?? 'all';
|
||
if (verbosity === 'silent') return;
|
||
if (verbosity === 'gm-only' && !this._adapter.users.isGM()) return;
|
||
|
||
this._enqueue(userId, newState, previousState);
|
||
}
|
||
```
|
||
|
||
### Coalescing Timer — 3-Second Window
|
||
|
||
**Canonical timer value: 3000ms** (driven by epics ACs — "within 3 seconds").
|
||
|
||
⚠️ Architecture data flow diagram says "300ms coalesce window" — this is inconsistent with the AC. **Epic ACs are canonical; use 3000ms.**
|
||
|
||
```js
|
||
_enqueue(userId, newState, prevState) {
|
||
const existing = this.#coalesceMap.get(userId);
|
||
if (existing) {
|
||
clearTimeout(existing.timer); // reset window on each new change
|
||
existing.lastState = newState;
|
||
existing.changeCount += 1;
|
||
} else {
|
||
this.#coalesceMap.set(userId, { timer: null, prevState, lastState: newState, changeCount: 1 });
|
||
}
|
||
const entry = this.#coalesceMap.get(userId);
|
||
entry.timer = setTimeout(() => this._flush(userId), 3_000);
|
||
}
|
||
|
||
_flush(userId) {
|
||
const entry = this.#coalesceMap.get(userId);
|
||
if (!entry) return;
|
||
this.#coalesceMap.delete(userId);
|
||
|
||
// AC-3: Net-zero suppression — no notification if final state equals original
|
||
if (entry.lastState === entry.prevState) return;
|
||
|
||
const name = this._adapter.users.get(userId)?.name ?? userId;
|
||
const isHidden = entry.lastState === 'hidden';
|
||
const msg = isHidden
|
||
? `GM hid ${name}'s camera`
|
||
: `GM showed ${name}'s camera`;
|
||
|
||
this._adapter.notifications.info(msg);
|
||
}
|
||
```
|
||
|
||
### Personal Notification Messages (AC-2)
|
||
|
||
```js
|
||
_notifyPersonal(newState) {
|
||
const msg = newState === 'hidden'
|
||
? "GM has hidden your camera. Your portrait is shown to other Participants."
|
||
: "Your camera is now visible to the table.";
|
||
this._adapter.notifications.info(msg);
|
||
}
|
||
```
|
||
|
||
Note: personal notification fires immediately (no coalescing). The `currentUserId` guard in `_onStateChanged` ensures coalescing loop is NOT entered for personal events.
|
||
|
||
### module.js Init Order Extension
|
||
|
||
```js
|
||
// Hooks.once('ready') — existing wiring ends at visibilityBadge.init()
|
||
// Add:
|
||
notificationBus = new NotificationBus(adapter);
|
||
notificationBus.init();
|
||
// Story 2.2: DirectorsBoard (lazy, GM only)
|
||
```
|
||
|
||
NotificationBus runs for **all clients** (GM and players) because:
|
||
- GM sees general notifications about all participants
|
||
- Players see personal notifications about themselves
|
||
|
||
The verbosity setting filters which general notifications each client sees.
|
||
|
||
### Vitest Fake Timer Pattern (CRITICAL — from Story 1.6)
|
||
|
||
The coalescing timer is 3000ms. Use `vi.useFakeTimers()` for all timer-related tests.
|
||
|
||
⚠️ **Two-step advance rule:** If a timer callback schedules another timer (or if `clearTimeout` + new `setTimeout` happens inside the callback), you need two separate `vi.advanceTimersByTime()` calls:
|
||
|
||
```js
|
||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||
|
||
describe('NotificationBus coalescing', () => {
|
||
beforeEach(() => vi.useFakeTimers());
|
||
afterEach(() => vi.useRealTimers());
|
||
|
||
it('fires single notification after 3s coalescing window', () => {
|
||
// ... setup bus + mocks ...
|
||
bus._onStateChanged({ userId: 'u1', newState: 'hidden', previousState: 'active' });
|
||
bus._onStateChanged({ userId: 'u1', newState: 'active', previousState: 'hidden' }); // resets timer
|
||
vi.advanceTimersByTime(3_001);
|
||
// net-zero: 'active' === 'active' → no notification
|
||
expect(notifMock.info).not.toHaveBeenCalled();
|
||
});
|
||
|
||
it('flushes with correct message after window expires', () => {
|
||
bus._onStateChanged({ userId: 'u1', newState: 'hidden', previousState: 'active' });
|
||
vi.advanceTimersByTime(3_001);
|
||
expect(notifMock.info).toHaveBeenCalledWith("GM hid Player's camera");
|
||
});
|
||
});
|
||
```
|
||
|
||
For nested timer tests (clearTimeout + new setTimeout in rapid succession):
|
||
```js
|
||
vi.advanceTimersByTime(1_500); // mid-window: timer reset by second event
|
||
// fire second event
|
||
vi.advanceTimersByTime(3_001); // fires the second timer
|
||
```
|
||
|
||
### FoundryAdapter.notifications Surface (Already Implemented)
|
||
|
||
```js
|
||
// From src/foundry/FoundryAdapter.js (Story 1.3) — reuse as-is:
|
||
adapter.notifications.info(msg)
|
||
adapter.notifications.warn(msg)
|
||
adapter.notifications.error(msg)
|
||
```
|
||
|
||
Do NOT call `ui.notifications.info()` directly. Always go through `adapter.notifications.*`.
|
||
|
||
### FoundryAdapter.settings — `client` Setting Scope
|
||
|
||
`adapter.settings.register()` already delegates to `game.settings.register()` with the provided config. Passing `scope: 'client'` is sufficient — the adapter doesn't transform this value.
|
||
|
||
`adapter.settings.get('notificationVerbosity')` returns the current user's client preference.
|
||
|
||
### Deferred Debt Cleanup (Task 4)
|
||
|
||
These are carry-over from `deferred-work.md` — fold in during this story:
|
||
|
||
**4.1 — `_revisions` Map leak (ScryingPoolController.js:31):**
|
||
```js
|
||
// Add cleanupUser(userId) public method:
|
||
cleanupUser(userId) {
|
||
this._revisions.delete(userId);
|
||
this._pendingOps.delete(userId);
|
||
}
|
||
```
|
||
Wire this into a `Hooks.on('deleteUser', ...)` handler in `init()` or expose for external call.
|
||
|
||
**4.2/4.3 — Listener cleanup:**
|
||
```js
|
||
// ScryingPoolController.init():
|
||
this._echoHookId = Hooks.on('scrying-pool:echoReceived', ...) // store id
|
||
// ScryingPoolController.teardown():
|
||
Hooks.off('scrying-pool:echoReceived', this._echoHookId);
|
||
```
|
||
Same pattern for VisibilityManager.
|
||
|
||
**4.4 — Echo revision validation:**
|
||
```js
|
||
// Before using revision in _onEcho():
|
||
if (!Number.isFinite(revision)) {
|
||
console.warn('[ScryingPool] _onEcho: invalid revision', revision);
|
||
return;
|
||
}
|
||
```
|
||
|
||
### `scrying-pool:stateChanged` Payload Shape
|
||
|
||
Emitted by `StateStore` (verified from Story 1.3 implementation):
|
||
```js
|
||
Hooks.callAll('scrying-pool:stateChanged', {
|
||
userId, // string — affected participant ID
|
||
newState, // string — one of VISIBILITY_STATES
|
||
previousState, // string — state before the change
|
||
revision, // number — monotonic revision counter
|
||
source, // string — 'gm' | 'preset' | 'hydration'
|
||
});
|
||
```
|
||
|
||
NotificationBus needs: `userId`, `newState`, `previousState`.
|
||
|
||
### Project Structure Notes
|
||
|
||
**New files:**
|
||
```
|
||
src/notifications/NotificationBus.js ← NEW
|
||
tests/unit/notifications/NotificationBus.test.js ← NEW
|
||
```
|
||
|
||
**Modified files:**
|
||
```
|
||
module.js ← import + notificationVerbosity setting + wiring
|
||
lang/en.json ← notification i18n keys
|
||
styles/components/_notification.less ← replace stub if custom toast styles needed
|
||
src/core/ScryingPoolController.js ← deferred debt: cleanup methods
|
||
src/core/VisibilityManager.js ← deferred debt: listener cleanup
|
||
```
|
||
|
||
**Do NOT modify:**
|
||
- `src/foundry/FoundryAdapter.js` — notifications surface is complete from Story 1.3
|
||
- `src/core/StateStore.js` — event emission is complete
|
||
- `src/ui/shared/AVTileAdapter.js` — no changes needed
|
||
|
||
### References
|
||
|
||
- Epics — Story 2.1 ACs: [Source: _bmad-output/planning-artifacts/epics.md#Story 2.1]
|
||
- PRD — FR-20, FR-21, FR-22: [Source: _bmad-output/planning-artifacts/prds/prd-video-view-manager-2026-05-19/prd.md#4.4]
|
||
- Architecture — NotificationBus data flow: [Source: _bmad-output/planning-artifacts/architecture.md#Data Flow — Notification Bus]
|
||
- Architecture — import boundary rules: [Source: _bmad-output/planning-artifacts/architecture.md#Import Boundary Rule]
|
||
- Architecture — directory structure: [Source: _bmad-output/planning-artifacts/architecture.md#src/notifications/]
|
||
- Story 1.6 — vitest fake timer two-step advance pattern: [Source: _bmad-output/implementation-artifacts/1-6-player-camera-status-badge.md#Debug Log References]
|
||
- Story 1.3 — FoundryAdapter.notifications surface: [Source: _bmad-output/implementation-artifacts/1-3-data-layer-foundryadapter-statestore-and-socket-infrastructure.md#Completion Notes]
|
||
- Story 1.4 — ScryingPoolController.init() pattern: [Source: _bmad-output/implementation-artifacts/1-4-core-logic-scryingpoolcontroller-and-visibilitymanager.md#Completion Notes]
|
||
- Deferred work items: [Source: _bmad-output/implementation-artifacts/deferred-work.md]
|
||
- Retrospective Epic 1 — action items: [Source: _bmad-output/implementation-artifacts/epic-1-retro-2026-05-22.md#Action Items]
|
||
- module.js — current wiring and init order: [Source: module.js]
|
||
|
||
## Dev Agent Record
|
||
|
||
### Agent Model Used
|
||
|
||
Claude Sonnet 4.6 (claude-sonnet-4.6)
|
||
|
||
### Debug Log References
|
||
|
||
- Net-zero suppression test gotcha: `active→hidden→active` is suppressed (net-zero); fixed by using `active→hidden→self-muted` as second state so prevState ≠ lastState at flush time.
|
||
- `_coalesceMap` uses convention-private underscore (not `#` hard private) so tests can inspect `.size`.
|
||
- ESLint `no-undef` eslint-disable directives in `NotificationBus.js` were removed — `Hooks` is already a declared global in the ESLint config.
|
||
- Deferred debt 4.4 (`Number.isFinite` validation) was already implemented in `_onEcho()`; no code change needed.
|
||
|
||
### Completion Notes List
|
||
|
||
- NotificationBus: 28 tests, all passing. Coalescing, verbosity, personal notifications, net-zero suppression, teardown all implemented and tested.
|
||
- Deferred debt 4.1–4.3 resolved: ScryingPoolController now stores `_echoHandler` + `_disconnectHookId`, exposes `teardown()`, wires `userConnected` cleanup. VisibilityManager now stores `_stateChangedHookId`, exposes `teardown()`.
|
||
- `styles/components/_notification.less` updated with `.scrying-pool` scope placeholder — native `ui.notifications` toasts need no custom CSS.
|
||
- Total: 335 tests passing (296 baseline + 28 NotificationBus + 11 teardown/disconnect).
|
||
|
||
### File List
|
||
|
||
- `src/notifications/NotificationBus.js` — NEW
|
||
- `tests/unit/notifications/NotificationBus.test.js` — NEW
|
||
- `module.js` — NotificationBus import, setting registration, init wiring
|
||
- `lang/en.json` — notification messages + notificationVerbosity setting keys
|
||
- `styles/components/_notification.less` — minimal `.scrying-pool` scope placeholder
|
||
- `src/core/ScryingPoolController.js` — `_echoHandler` + `_disconnectHookId`, `teardown()`, `userConnected` cleanup in `init()`
|
||
- `src/core/VisibilityManager.js` — `_stateChangedHookId` stored in `init()`, `teardown()` added
|
||
- `tests/unit/core/ScryingPoolController.test.js` — teardown + disconnect cleanup tests
|
||
- `tests/unit/core/VisibilityManager.test.js` — teardown tests
|
||
|
||
---
|
||
|
||
### Review Findings
|
||
|
||
#### Decision Needed
|
||
|
||
- [x] [Review][Decision] Hardcoded notification strings bypass i18n system — **RESOLVED: Add i18n support now** — NotificationBus.js uses raw strings instead of `game.i18n.localize()`. Will extend FoundryAdapter with i18n surface and update NotificationBus to use localization.
|
||
|
||
#### Patches Required
|
||
|
||
- [x] [Review][Patch] Add i18n support to FoundryAdapter and NotificationBus [FoundryAdapter.js, NotificationBus.js, lang/en.json] — Extend FoundryAdapter with i18n surface, update NotificationBus to use `game.i18n.localize()` for all notification messages. Use existing lang/en.json keys.
|
||
- [x] [Review][Patch] Property name mismatch: `state` vs `newState` breaks all notifications [NotificationBus.js:68, StateStore.js:105] — Fixed destructuring to use `state` property emitted by StateStore.
|
||
- [x] [Review][Patch] Unused `changeCount` property (dead code) [NotificationBus.js] — Now used in message output to show change count.
|
||
- [x] [Review][Patch] Coalesced notification omits change count from message [NotificationBus.js:137-139] — Change count now appended to notification message (e.g., "(3 changes)").
|
||
- [x] [Review][Patch] No cleanup of coalesceMap entries for disconnected users [NotificationBus.js] — Added userConnected hook listener to clean up entries on disconnect.
|
||
- [x] [Review][Patch] Race condition: timer fires after teardown [NotificationBus.js:132-134] — Added null check for entry in _flush to prevent TypeError.
|
||
- [x] [Review][Patch] No validation of `notificationVerbosity` setting value [NotificationBus.js:85] — Added validation with fallback to 'all' for invalid values.
|
||
- [x] [Review][Patch] `_flush` missing null check for entry [NotificationBus.js:132-134] — Added guard to return early if entry is undefined.
|
||
- [x] [Review][Patch] No input validation for `newState`/`previousState` [NotificationBus.js:107-123] — Added type checks in _enqueue to reject invalid parameters.
|
||
- [x] [Review][Patch] Stale closure in coalescing timers [NotificationBus.js:124-128] — Added additional null check for entry.timer to mitigate stale closure issues.
|
||
- [x] [Review][Patch] No protection against rapid init/teardown cycles [NotificationBus.js:42-57] — Added guard in init() to prevent multiple initializations without teardown.
|
||
|
||
#### Deferred (Pre-existing / Out of Scope)
|
||
|
||
- [x] [Review][Defer] VisibilityManager only handles binary states [VisibilityManager.js:84-90] — T-09 handles hidden/offline/cam-lost/ghost as "disable", else as "enable". States like self-muted, reconnecting fall through incorrectly. Pre-existing issue, not introduced in this story.
|
||
- [x] [Review][Defer] No handling of `setMatrix` hook events [NotificationBus.js] — setMatrix emits without userId; bulk state changes won't trigger notifications. Pre-existing architectural limitation.
|
||
- [x] [Review][Defer] ScryingPoolController cleanup only on userConnected hook [ScryingPoolController.js:46-49] — Disconnect detection limited to userConnected event. Other disconnect scenarios may leak entries. Pre-existing.
|
||
- [x] [Review][Defer] Hook data property mismatch with `setMatrix` [StateStore.js:139, NotificationBus.js] — setMatrix emits `{ matrix, timestamp, revision }` without userId; incompatible with NotificationBus expectations. Pre-existing.
|