Files

T

2026-05-23 18:23:48 +02:00

33 KiB

Raw Blame History

Edge Case Hunter Review Prompt - Story 1-5 Group 1 (Core Logic)

Story: 1-5-gm-control-ui-scryingpoolstrip-actionpopover-and-av-tile-integration
Group: Core Logic (Group 1 of 4)
Files: ScryingPoolController.js, VisibilityManager.js, and their tests
Diff lines: 804
Project path: /home/morr/work/foundryvtt/video-view-manager

YOUR ROLE: Edge Case Hunter

You are an Edge Case Hunter code reviewer. You receive the diff below AND read access to the project repository at /home/morr/work/foundryvtt/video-view-manager. Your job is to find edge cases, boundary conditions, and unusual scenarios that the code doesn't handle properly.

Rules:

You have read access to the project directory
You have access to the diff below
You have NO access to the spec file (blind to requirements)
You MUST find at least 5 edge case issues
Focus on: null/undefined handling, empty collections, concurrent access, extreme values, error paths, timing issues, state transitions

Method:

Read the diff carefully
Explore the project codebase to understand the broader context
Walk every branching path in the code
Check boundary conditions for all inputs and loops
Identify unhandled edge cases

Output Format:

Output findings as a Markdown list. Each finding:

- **EC-XX: [One-line title]** — [Code location] — [Edge case scenario] — [Impact]

Classify by type:

EC-NULL: Missing null/undefined checks
EC-BOUNDARY: Off-by-one, empty collections, extreme values
EC-CONCURRENCY: Race conditions, async issues
EC-STATE: Invalid state transitions, inconsistent state
EC-ERROR: Unhandled exceptions, error swallowing
EC-TIMING: Timeout issues, ordering problems

DIFF TO REVIEW

diff --git a/src/core/ScryingPoolController.js b/src/core/ScryingPoolController.js new file mode 100644 index 0000000..fc013f0 --- /dev/null +++ b/src/core/ScryingPoolController.js @@ -0,0 +1,181 @@ +/**

- ScryingPoolController — Orchestrates GM visibility actions with optimistic state updates.
- Handles: GM authorization, latest-revision-wins guard, last-intent guard, PendingOp
- lifecycle, optimistic setVisibility, socket emit, and echo reconciliation.
- Import rule: may only import from src/contracts/ and src/utils/.
- Constructors are side-effect free — call init() from module.js Hooks.once('ready').
- @module core/ScryingPoolController
*/

+import { createPendingOp } from '../contracts/pending-op.js'; +import { createSocketIntentMessage, SOCKET_EVENTS } from '../contracts/socket-message.js'; + +/**

- Orchestrates GM visibility actions: auth, optimistic state, socket emit, echo reconciliation.
*/ +export class ScryingPoolController {
/**
- @param {import('./StateStore.js').StateStore} stateStore
- @param {{ emit(event: string, payload: object): void, registerPendingOp(op: object, event: string, payload: object): void, confirmPendingOp(opId: string): void, setReady(handler: object): void }} socketHandler
- @param {{ users: { isGM(): boolean }, socket: { on(event: string, handler: (...args: unknown[]) => void): void }, hooks: { on(event: string, handler: (...args: unknown[]) => void): void, callAll(event: string, data: unknown): void } }} adapter
*/
constructor(stateStore, socketHandler, adapter) {
this._stateStore = stateStore;
this._socketHandler = socketHandler;
this._adapter = adapter;
/** @type {Map<string, import('../contracts/pending-op.js').PendingOp>} participantId → PendingOp */
this._pendingOps = new Map();
/** @type {Map<string, number>} participantId → last-confirmed revision */
this._revisions = new Map();
}
/**
- Registers the socket echo listener.
- Called from module.js Hooks.once('ready') — NOT from constructor.
*/
init() {
this._adapter.socket.on(
```
 SOCKET_EVENTS.VISIBILITY_UPDATED,
```

 (payload) => this._onEcho(/** @type {any} */ (payload))

);
}
/**
- Returns the last confirmed revision for a participant (0 if unknown).
- @param {string} participantId
- @returns {number}
*/
getRevision(participantId) {
return this._revisions.get(participantId) ?? 0;
}
/**
- Returns true if a pending op is currently in-flight for the given participant.
- @param {string} participantId
- @returns {boolean}
*/
hasPendingOp(participantId) {
return this._pendingOps.has(participantId);
}
/**
- Cleans up a pending operation by userId.
- Called by SocketHandler timeout callback via composite handler in module.js.
- @param {string} userId
*/
cleanupPendingOp(userId) {
this._pendingOps.delete(userId);
}
/**
- Processes a GM visibility toggle request.
- Guards: isGM, latest-revision-wins, last-intent (idempotent).
- @param {string} source - Who triggered the action (e.g. 'ui', 'preset').
- @param {string} participantId - Target userId.
- @param {string} targetState - Desired VisibilityState.
- @param {string} opId - Unique operation ID (supplied by caller — Story 1.5 UI).
- @param {number} baseRevision - StateStore revision at time of intent.
*/
action(source, participantId, targetState, opId, baseRevision) {
// 0. Input validation
if (!participantId || typeof participantId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid participantId');

```
 return;
```
}
if (!targetState || typeof targetState !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid targetState');

```
 return;
```
}
if (!opId || typeof opId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid opId');

```
 return;
```
}
if (typeof baseRevision !== 'number' || !Number.isFinite(baseRevision) || baseRevision < 0) {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid baseRevision');

```
 return;
```
}
// 1. Authorization
if (!this._adapter.users.isGM()) {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: non-GM call rejected');

```
 return;
```
}
// 2. Latest-revision-wins guard
const currentRevision = this._revisions.get(participantId) ?? 0;
if (baseRevision < currentRevision) return;
// 3. Last-intent guard (idempotent)
const currentState = this._stateStore.getState(participantId);
if (currentState === targetState) return;
// 4. Register PendingOp
const previousState = currentState ?? 'never-connected';
const pendingOp = createPendingOp(opId, participantId, targetState, previousState);
this._pendingOps.set(participantId, pendingOp);
// 5. Optimistic state update
this._stateStore.setVisibility(participantId, targetState);
// 6. Socket emit
const msg = createSocketIntentMessage(opId, participantId, targetState, baseRevision);
this._socketHandler.emit(msg.event, msg.payload);
// 7. Start acknowledgement timer
this._socketHandler.registerPendingOp(pendingOp, msg.event, msg.payload);
// 8. Notify UI subscribers
try {

 this._adapter.hooks.callAll('scrying-pool:controllerAction', { participantId, targetState, source, opId });

} catch (hookErr) {

 console.error('[ScryingPool] ScryingPoolController.action: hook emission failed', hookErr);

}
}
/**
- Processes an authoritative echo from the socket server.
- Confirms the pending op, updates revision, and sets the authoritative state.
- @private
- @param {{ opId: string, userId: string, state: string, revision?: number }} payload
*/
_onEcho(payload) {
// Validate payload fields
if (!payload || typeof payload !== 'object') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: invalid payload');

```
 return;
```
}
const { opId, userId, state, revision } = payload;
if (!opId || typeof opId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid opId');

```
 return;
```
}
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid userId');

```
 return;
```
}
if (!state || typeof state !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid state');

```
 return;
```
}
this._socketHandler.confirmPendingOp(opId);
this._revisions.set(userId, revision ?? 0);
this._pendingOps.delete(userId);
this._stateStore.setVisibility(userId, state);
try {

 this._adapter.hooks.callAll('scrying-pool:controllerAction', {

```
   participantId: userId,
```
```
   targetState: state,
```
```
   source: 'echo',
```
```
   opId,
```
```
 });
```
} catch (hookErr) {

 console.error('[ScryingPool] ScryingPoolController._onEcho: hook emission failed', hookErr);

}
} +} diff --git a/src/core/VisibilityManager.js b/src/core/VisibilityManager.js new file mode 100644 index 0000000..0e465f2 --- /dev/null +++ b/src/core/VisibilityManager.js @@ -0,0 +1,104 @@ +/**
- VisibilityManager — WebRTC strategy applier and SocketHandler revert handler.
- Listens to scrying-pool:stateChanged hook events (emitted by StateStore) and
- applies the appropriate webrtcMode strategy:
- - 'track-disable' + non-null adapter.webrtc → call disableTrack / enableTrack
- - 'css-fallback' / 'unsupported' / null webrtc → no-op (CSS handled by RoleRenderer)
- Also implements onRevert(pendingOp) for SocketHandler timeout callbacks.
- Import rule: may only import from src/contracts/ and src/utils/.
- Constructors are side-effect free — call init() from module.js Hooks.once('ready').
- @module core/VisibilityManager
*/

+/**

- Applies webrtcMode strategy on state changes and reverts failed operations.
*/ +export class VisibilityManager {
/**
- @param {import('./StateStore.js').StateStore} stateStore
- @param {{ settings: { get(key: string): unknown }, webrtc: { disableTrack(userId: string): void, enableTrack(userId: string): void } | null, notifications: { warn(msg: string): void }, hooks: { on(event: string, handler: (...args: unknown[]) => void): void } }} adapter
*/
constructor(stateStore, adapter) {
this._stateStore = stateStore;
this._adapter = adapter;
}
/**
- Registers the Hooks.on('scrying-pool:stateChanged') listener.
- Called from module.js Hooks.once('ready') — NOT from constructor.
*/
init() {
this._adapter.hooks.on('scrying-pool:stateChanged', (data) => this._onStateChanged(/** @type {any} */ (data)));
}
/**
- Handles a state change by applying the webrtcMode strategy.
- css-fallback / unsupported → no-op (CSS applied by RoleRenderer in Story 1.5).
- track-disable + non-null webrtc → disable/enable the participant's track.
- Always safe with null adapter.webrtc (OQ-1 spike result for v14).
- @private
- @param {{ userId: string, state: string }} data
*/
_onStateChanged(data) {
const { userId, state } = data;
// Input validation
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager._onStateChanged: invalid userId');

```
 return;
```
}
if (!state || typeof state !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager._onStateChanged: invalid state');

```
 return;
```
}
const mode = this._adapter.settings.get('webrtcMode');
if (mode !== 'track-disable' || !this._adapter.webrtc) return;
if (state === 'hidden') {

 this._adapter.webrtc.disableTrack(userId);

} else {

 this._adapter.webrtc.enableTrack(userId);

}
}
/**
- Called by SocketHandler after retry exhaustion — reverts the optimistic state
- and notifies the GM that the operation could not be confirmed.
- @param {{ userId: string, previousState: string, opId: string }} pendingOp
*/
onRevert(pendingOp) {
// Input validation
if (!pendingOp || typeof pendingOp !== 'object') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid pendingOp');

```
 return;
```
}
const { userId, previousState } = pendingOp;
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid userId in pendingOp');

```
 return;
```
}
if (!previousState || typeof previousState !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid previousState in pendingOp');

```
 return;
```
}
try {

 this._stateStore.setVisibility(userId, previousState);

} catch (err) {

 console.error('[ScryingPool] VisibilityManager.onRevert: setVisibility failed', err);

}
try {
```
 this._adapter.notifications.warn(
```

   `[ScryingPool] Visibility change for ${userId} could not be confirmed — reverting to ${previousState}`

```
 );
```
} catch (err) {

 console.error('[ScryingPool] VisibilityManager.onRevert: notification failed', err);

}
} +} diff --git a/tests/unit/core/ScryingPoolController.test.js b/tests/unit/core/ScryingPoolController.test.js new file mode 100644 index 0000000..eb4f4ad --- /dev/null +++ b/tests/unit/core/ScryingPoolController.test.js @@ -0,0 +1,277 @@ +// @ts-nocheck +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { ScryingPoolController } from '../../../src/core/ScryingPoolController.js'; +import { createFoundryAdapterMock } from '../../helpers/foundryAdapterMock.js'; +import { StateStore } from '../../../src/core/StateStore.js';

+/** @returns {{ emit: Function, registerPendingOp: Function, confirmPendingOp: Function, setReady: Function }} */ +function makeSocketHandler() {

return {
emit: vi.fn(),
registerPendingOp: vi.fn(),
confirmPendingOp: vi.fn(),
setReady: vi.fn(),
}; +}

+/** @returns {StateStore} */ +function makeStateStore() {

const settingsMock = {
get: vi.fn().mockReturnValue({ _version: 1, matrix: {} }),
set: vi.fn().mockResolvedValue(undefined),
register: vi.fn(),
};
return new StateStore(settingsMock); +}

+describe('ScryingPoolController', () => {

let adapter;
let stateStore;
let socketHandler;
let controller;
let hooksStub;
beforeEach(() => {
hooksStub = { callAll: vi.fn(), on: vi.fn(), once: vi.fn(), off: vi.fn() };
vi.stubGlobal('Hooks', hooksStub);
adapter = createFoundryAdapterMock({
```
 users: { isGM: () => true },
```
```
 hooks: hooksStub
```
});
adapter.socket.on = vi.fn();
stateStore = makeStateStore();
socketHandler = makeSocketHandler();
controller = new ScryingPoolController(stateStore, socketHandler, adapter);
});
afterEach(() => {
vi.unstubAllGlobals();
vi.clearAllMocks();
});
// ── AC-1: Construction ────────────────────────────────────────────────────
describe('constructor (AC-1)', () => {
it('initialises _pendingOps as an empty Map', () => {

 expect(controller._pendingOps).toBeInstanceOf(Map);

 expect(controller._pendingOps.size).toBe(0);

});
it('initialises _revisions as an empty Map', () => {

 expect(controller._revisions).toBeInstanceOf(Map);

 expect(controller._revisions.size).toBe(0);

});
it('does NOT register socket listener in constructor (side-effect free)', () => {

 expect(adapter.socket.on).not.toHaveBeenCalled();

});
});
// ── AC-1: init() ─────────────────────────────────────────────────────────
describe('init() (AC-1)', () => {
it('registers socket echo listener for scrying-pool.visibility.updated', () => {
```
 controller.init();
```

 expect(adapter.socket.on).toHaveBeenCalledWith(

```
   'scrying-pool.visibility.updated',
```
```
   expect.any(Function)
```
```
 );
```
});
});
// ── AC-2: action() happy path ─────────────────────────────────────────────
describe('action() happy path (AC-2)', () => {
it('stores a PendingOp in _pendingOps keyed by participantId', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(controller._pendingOps.has('user-1')).toBe(true);

 expect(controller._pendingOps.get('user-1')).toMatchObject({

```
   opId: 'op-1',
```
```
   userId: 'user-1',
```
```
   targetState: 'hidden',
```
```
 });
```
});
it('calls stateStore.setVisibility with the target state (optimistic update)', () => {

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
it('calls socketHandler.emit with VISIBILITY_SET event and correct payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(socketHandler.emit).toHaveBeenCalledWith(

```
   'scrying-pool.visibility.set',
```

   expect.objectContaining({ opId: 'op-1', userId: 'user-1', targetState: 'hidden', baseRevision: 0 })

```
 );
```
});
it('calls socketHandler.registerPendingOp with the PendingOp, event, and payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(socketHandler.registerPendingOp).toHaveBeenCalledWith(

   expect.objectContaining({ opId: 'op-1', userId: 'user-1', targetState: 'hidden' }),

```
   'scrying-pool.visibility.set',
```

   expect.objectContaining({ opId: 'op-1' })

```
 );
```
});
it('fires Hooks.callAll scrying-pool:controllerAction with correct payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(hooksStub.callAll).toHaveBeenCalledWith(

```
   'scrying-pool:controllerAction',
```

   expect.objectContaining({ participantId: 'user-1', targetState: 'hidden', source: 'ui', opId: 'op-1' })

```
 );
```
});
it('sets previousState to null-coalesced "never-connected" when participant is new', () => {

 controller.action('ui', 'new-user', 'hidden', 'op-1', 0);

 const op = controller._pendingOps.get('new-user');

 expect(op.previousState).toBe('never-connected');

});
});
// ── AC-5: non-GM authorization ────────────────────────────────────────────
describe('action() non-GM authorization (AC-5)', () => {
it('warns and silently drops the action when adapter.users.isGM() is false', () => {

 const nonGmAdapter = createFoundryAdapterMock({

```
   users: { isGM: () => false },
```
```
   hooks: hooksStub
```
```
 });
```
```
 nonGmAdapter.socket.on = vi.fn();
```

 const playerController = new ScryingPoolController(stateStore, socketHandler, nonGmAdapter);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});

 playerController.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(warnSpy).toHaveBeenCalledWith('[ScryingPool]', expect.stringContaining('non-GM'));

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

 expect(socketHandler.registerPendingOp).not.toHaveBeenCalled();

 expect(hooksStub.callAll).not.toHaveBeenCalled();

```
 warnSpy.mockRestore();
```
});
});
// ── AC-3: latest-revision-wins guard ─────────────────────────────────────
describe('action() latest-revision-wins guard (AC-3)', () => {
it('silently drops action when baseRevision < confirmed revision', () => {

 controller._revisions.set('user-1', 5);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 3); // 3 < 5 → stale

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

 expect(hooksStub.callAll).not.toHaveBeenCalled();

});
it('allows action when baseRevision equals confirmed revision (not stale)', () => {

 controller._revisions.set('user-1', 5);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 5); // 5 == 5 → not stale

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
it('allows action with baseRevision=0 when no revision confirmed yet', () => {

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

```
 expect(setSpy).toHaveBeenCalled();
```
});
});
// ── AC-4: last-intent guard ───────────────────────────────────────────────
describe('action() last-intent guard (AC-4)', () => {
it('silently drops action when participant is already in targetState', () => {

 // Seed the state store with the current state

 stateStore.setVisibility('user-1', 'hidden');

 vi.clearAllMocks(); // reset all mock call counts

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 0);

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

});
it('allows action when targetState differs from current state', () => {

 stateStore.setVisibility('user-1', 'active');

```
 vi.clearAllMocks();
```

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-3', 0);

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
});
// ── AC-11: echo reconciliation (_onEcho) ──────────────────────────────────
describe('_onEcho() echo reconciliation (AC-11)', () => {
// Helper: call init() and return the captured echo handler
function getEchoHandler() {
```
 controller.init();
```

 return adapter.socket.on.mock.calls[0][1];

}
it('calls socketHandler.confirmPendingOp with the opId', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(socketHandler.confirmPendingOp).toHaveBeenCalledWith('op-1');

});
it('stores the echo revision in _revisions for the userId', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 7 });

 expect(controller._revisions.get('user-1')).toBe(7);

});
it('calls stateStore.setVisibility with the authoritative state', () => {
```
 const echoHandler = getEchoHandler();
```

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'active', revision: 2 });

 expect(setSpy).toHaveBeenCalledWith('user-1', 'active');

});
it('fires Hooks.callAll scrying-pool:controllerAction with source: echo', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(hooksStub.callAll).toHaveBeenCalledWith(

```
   'scrying-pool:controllerAction',
```

   expect.objectContaining({ source: 'echo', participantId: 'user-1', targetState: 'hidden', opId: 'op-1' })

```
 );
```
});
it('removes the participant from _pendingOps after echo', () => {
```
 // Register a pending op first
```

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(controller._pendingOps.has('user-1')).toBe(true);

```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(controller._pendingOps.has('user-1')).toBe(false);

});
it('defaults revision to 0 when echo payload omits revision field', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden' }); // no revision

 expect(controller._revisions.get('user-1')).toBe(0);

});
}); +}); diff --git a/tests/unit/core/VisibilityManager.test.js b/tests/unit/core/VisibilityManager.test.js new file mode 100644 index 0000000..36df70f --- /dev/null +++ b/tests/unit/core/VisibilityManager.test.js @@ -0,0 +1,218 @@ +// @ts-nocheck +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { VisibilityManager } from '../../../src/core/VisibilityManager.js'; +import { createFoundryAdapterMock } from '../../helpers/foundryAdapterMock.js'; +import { StateStore } from '../../../src/core/StateStore.js';