97 lines
9.0 KiB
Markdown
97 lines
9.0 KiB
Markdown
# Call to Play Code & Architecture Review Report
|
|
|
|
I have conducted a thorough review of the commits (`e141229` through `2c204ac`) on branch `calltoplay`, referencing [`FABLE_5_FINDINGS.md`](file:///pantheon/pfs/git/rust/pfs/lanspread/FABLE_5_FINDINGS.md) and [`CALL_TO_PLAY_FIXES_PLAN.md`](file:///pantheon/pfs/git/rust/pfs/lanspread/CALL_TO_PLAY_FIXES_PLAN.md).
|
|
|
|
---
|
|
|
|
## 1. Plan Implementation & Deviation Assessment
|
|
|
|
The plan outlined in [`CALL_TO_PLAY_FIXES_PLAN.md`](file:///pantheon/pfs/git/rust/pfs/lanspread/CALL_TO_PLAY_FIXES_PLAN.md) is **faithfully and elegantly implemented across all 5 code commits**, with zero regression to core invariants.
|
|
|
|
| Commit | Scope | Plan Requirements | Code Verification | Status |
|
|
|---|---|---|---|---|
|
|
| [`be7ad2e`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/call_to_play.rs) | Atomic Batch Merge & Store Semantics | - Transactional history merge<br>- Single-pass $O(N)$ compaction<br>- Conflicting ID batch rejection<br>- `NeedHistory` for missing roots<br>- Rebuild event IDs from retained events<br>- Capacity cap applies to unresolved history only | - `merge_batch_at` in [call_to_play.rs](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/call_to_play.rs#L83)<br>- `deduplicate_batch`<br>- `unresolved_event_count`<br>- Tests cover revival & conflict behavior | **Faithful** |
|
|
| [`e5d70ae`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/services/stream.rs) | Live Replication & Acknowledgement | - Protocol v7 bump<br>- `CallToPlayAck` return outcomes<br>- Remove unreliable source-IP equality check<br>- Enforce envelope vs actor ID matching<br>- Async handshake resync on delivery failure | - Protocol updated in [`lanspread-proto`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-proto/src/lib.rs)<br>- `handle_call_to_play_events` in [stream.rs](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/services/stream.rs#L124)<br>- `delivery_resync_reason` in [call_to_play.rs](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/call_to_play.rs#L472) | **Faithful** |
|
|
| [`9c34efa`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-tauri-deno-ts/src/lib/callToPlay.ts) | Terminal Outcome Retention | - 15-minute terminal outcome display (`TERMINAL_RETENTION_MS`) for `Running` and `Cancelled`<br>- Post-15-min compaction to tombstones<br>- Frontend status reduction, sorting, and badge exclusion<br>- Peer CLI scenario S49 | - `compact_history` in [call_to_play.rs](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-peer/src/call_to_play.rs#L340)<br>- Reducer logic in [callToPlay.ts](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-tauri-deno-ts/src/lib/callToPlay.ts)<br>- Roster/chat retained | **Faithful** |
|
|
| [`8d3affe`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-tauri-deno-ts/src/lib/callToPlay.ts) | Extend from Current Deadline | - `extendDeadline`: $\max(\text{now}, \text{currentDeadline}) + \text{duration}$ | - Implemented in `extendDeadline` in [callToPlay.ts](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-tauri-deno-ts/src/lib/callToPlay.ts#L12) | **Faithful** |
|
|
| [`2c204ac`](file:///pantheon/pfs/git/rust/pfs/lanspread/crates/lanspread-tauri-deno-ts/src/hooks/useCallToPlay.ts) | Startup & Error Guidance | - Replace missing folder prompt with LAN connecting state<br>- Map specific store errors (`Obsolete`, `NeedHistory`, `HistoryFull`) to human guidance | - Implemented in `useCallToPlay.ts` and `callToPlayPublishErrorMessage` | **Faithful** |
|
|
|
|
### Implementation Refinements Over the Initial Plan
|
|
1. **Tombstone Representation**: Rather than instantiating a separate tombstone data structure, `compact_history` retains **only** the `Start` or `Cancel` event (`event.id == terminal.event_id`) after the 15-minute terminal retention window expires. `terminal_tombstone_call_ids` uses unrooted terminal events to reject any incoming obsolete history. This is cleaner and more memory-efficient than allocating explicit tombstone markers.
|
|
2. **Unresolved History Cap (`unresolved_event_count`)**: The active event cap ($4,096$) is enforced strictly against *unresolved* calls (`Create` without `Start`/`Cancel`). As soon as a creator emits `Start` or `Cancel`, that call's events no longer count against the active cap. This guarantees a user can always settle (Start or Cancel) an open call even when the store is full.
|
|
|
|
---
|
|
|
|
## 2. Holistic Architecture Review
|
|
|
|
```
|
|
+------------------------+
|
|
| Frontend (TS/Tauri) |
|
|
| Event Reducer & Hooks |
|
|
+-----------+------------+
|
|
| publish_call_to_play / snapshot
|
|
v
|
|
+------------------------+
|
|
| CallToPlayStore |
|
|
| (Atomic Batch Merge) |
|
|
+-----------+------------+
|
|
|
|
|
+-------------------+-------------------+
|
|
| (Local immediate) | (Async broadcast)
|
|
v v
|
|
+-------------------+ +-------------------+
|
|
| Local UI Emitter | | Peer QUIC Stream |
|
|
+-------------------+ | (Protocol Ver 7) |
|
|
+---------+---------+
|
|
| CallToPlayEvents
|
|
v
|
|
+-------------------+
|
|
| CallToPlayAck |
|
|
| (Applied/NeedHist)|
|
|
+-------------------+
|
|
```
|
|
|
|
### Architectural Soundness
|
|
1. **Event-Sourced LAN Replication vs Server-Authoritative State**:
|
|
Maintaining an event-sourced replication model with deterministic reduction is optimal for LAN Spread. Decentralized peer-to-peer LAN parties lack guaranteed central servers. Using atomic batch merges ($O(N)$ compaction) completely eliminates the $O(N^2)$ quadratic slowdown of the previous per-event insertion model.
|
|
|
|
2. **Network Identity Model**:
|
|
Removing source-IP equality comparisons fixes a major real-world bug on multi-homed hardware (Ethernet + Wi-Fi / VPN / virtual bridges). Validating that `envelope peer_id` is present in the mDNS peer roster and verifying `event.actor_id == envelope peer_id` accurately matches the trusted-LAN threat model without making false cryptographic guarantees.
|
|
|
|
3. **Asynchronous Healing**:
|
|
Local updates succeed instantly for the local user without blocking on network delivery (`task_tracker.spawn(...)`). If a remote peer rejects an event with `NeedHistory` or `NeedHandshake`, an asynchronous full `Hello`/`HelloAck` resync is scheduled. This isolates local UI responsiveness from network transport delays.
|
|
|
|
4. **Lifecycle & Memory Management**:
|
|
The 3-tier lifecycle (`Open` $\rightarrow$ `Time's up` [5-min recovery] $\rightarrow$ `Running`/`Cancelled` [15-min display] $\rightarrow$ `Tombstone`) strikes the right balance between retaining full chat/roster history for late joiners and preventing unbounded memory growth.
|
|
|
|
---
|
|
|
|
## 3. User Experience (UX) Analysis
|
|
|
|
```
|
|
UX Flow Comparison (Add Time Action)
|
|
|
|
BEFORE: [10-min Call] -- (Filled at min 2) --> Click "+5 mins" --> Deadline set to (2+5) = 7 mins! (SHORTENED!)
|
|
AFTER: [10-min Call] -- (Filled at min 2) --> Click "+5 mins" --> Deadline set to max(2, 10)+5 = 15 mins! (EXTENDED!)
|
|
```
|
|
|
|
1. **Intuitive "+5 minutes" Extension**:
|
|
- *Previous behavior*: Setting deadline to `now + 5` inadvertently shortened calls that reached capacity early.
|
|
- *Current behavior*: `Math.max(now, currentDeadline) + 5` preserves existing remaining time when extending early, and correctly grants 5 new minutes to an overdue call.
|
|
|
|
2. **Startup & Connection Guidance**:
|
|
- *Previous behavior*: Attempting an action during startup raised misleading errors about missing game folders.
|
|
- *Current behavior*: Shows `"Call to Play is still connecting to the LAN. Try again in a moment."` while `actorId` is initializing, clearing automatically upon connection.
|
|
|
|
3. **Clear Terminal Receipts (`Running` and `Cancelled`)**:
|
|
- *Previous behavior*: Starting or canceling a call caused it to disappear or act erratically, hiding game chat.
|
|
- *Current behavior*: `Running` displays as a clear green success receipt card, and `Cancelled` displays as a read-only historical card. Roster and chat remain accessible for 15 minutes, sorted below active calls and excluded from badge counts.
|
|
|
|
---
|
|
|
|
## Conclusion & Recommendation
|
|
|
|
The commits are **clean, robust, and fully faithful to the findings and plan**. The architectural choices are sound for a LAN environment, and the UX is intuitive and frictionless.
|
|
|
|
No further code changes are needed; the implementation is ready for merge.
|