arrrrr... doch pub/private keys...
This commit is contained in:
@@ -0,0 +1,129 @@
|
||||
# Call to Play review findings evaluation
|
||||
|
||||
The implementation is fundamentally sound, but I would make four focused
|
||||
follow-ups before calling it finished. Two address real behavioral gaps; two
|
||||
complete the new lifecycle cleanly.
|
||||
|
||||
## Worth fixing before finish
|
||||
|
||||
### 1. Propagate rootless terminal tombstones
|
||||
|
||||
This is the most important finding.
|
||||
|
||||
After 15 minutes, a peer retains only the `Start` or `Cancel` event. A fresh
|
||||
peer receiving that tombstone during handshake currently rejects it as missing
|
||||
its `Create` root, and the handshake path merely logs the missing root.
|
||||
|
||||
That means the fresh peer has no protection against a stale peer later
|
||||
resurrecting the finished call. It only self-heals if a tombstone-holder is
|
||||
still online afterward.
|
||||
|
||||
I would:
|
||||
|
||||
- Add an explicit handshake merge policy that permits rootless `Start` and
|
||||
`Cancel` events as non-visible tombstones.
|
||||
- Keep local and live orphan actions returning `NeedHistory`.
|
||||
- Never expose a rootless tombstone as a nomination in the UI.
|
||||
- Verify that a later stale complete history is classified obsolete.
|
||||
- Add store and handshake tests for this sequence.
|
||||
|
||||
This restores the intended "tombstone prevents resurrection for the rest of the
|
||||
session" invariant. I would fix the semantics, not merely downgrade the
|
||||
repeated warning.
|
||||
|
||||
### 2. Distinguish missing game directory from peer startup
|
||||
|
||||
Kimi is correct here. The peer is only started after a valid directory reaches
|
||||
`update_game_directory`. Without one, "still connecting -- try again" never
|
||||
becomes true.
|
||||
|
||||
I would:
|
||||
|
||||
- Model directory readiness as `checking | missing | ready`, rather than
|
||||
passing only a boolean that conflates hydration with a known missing
|
||||
directory.
|
||||
- Pass that prerequisite state into `useCallToPlay`.
|
||||
- Show folder guidance only for the confirmed `missing` state.
|
||||
- Preserve the current connecting message for `checking` or
|
||||
`ready-but-peer-starting`.
|
||||
- Test both states and the transition after a valid directory is selected.
|
||||
|
||||
That finishes the original finding's full intent without returning to
|
||||
misleading folder advice during normal startup.
|
||||
|
||||
### 3. Add a local Launch action to Running receipts
|
||||
|
||||
Fable's UX point is persuasive. Participants currently reach the key moment
|
||||
and see only that the game is running.
|
||||
|
||||
I would add a local-only Launch button when:
|
||||
|
||||
- The call is `running`.
|
||||
- The game is installed and launchable locally.
|
||||
- No conflicting operation prevents launch.
|
||||
|
||||
This would not violate the read-only terminal invariant: launching the local
|
||||
game does not mutate the replicated call. I would use a dedicated play
|
||||
callback rather than the generic primary action, so a button labelled "Launch"
|
||||
cannot unexpectedly initiate an install or update.
|
||||
|
||||
### 4. Make terminal receipts visually static and correct the spec
|
||||
|
||||
The terminal receipt details are small but real:
|
||||
|
||||
- `MiniBubbles` continue advancing pending-ready countdowns after a call is
|
||||
Running or Cancelled.
|
||||
- Terminal cards still draw empty seats.
|
||||
- The specification still says "one row per active call" and documents the old
|
||||
ticker rank, despite terminal rows and expired-first ordering.
|
||||
|
||||
I would:
|
||||
|
||||
- Freeze participant readiness at `terminalAt`, or render terminal
|
||||
participants without countdown tags.
|
||||
- Suppress empty roster slots on terminal cards.
|
||||
- Change Ready ticker text to name the creator.
|
||||
- Update the ticker specification to match the actual visible-call and ranking
|
||||
behavior.
|
||||
|
||||
## Coverage to add alongside those fixes
|
||||
|
||||
I would close the acknowledgement-heal test gap, but without introducing a full
|
||||
React test stack or waiting five real minutes:
|
||||
|
||||
- Exercise `NeedHistory` from a live orphan action.
|
||||
- Apply the full history through the handshake merge path.
|
||||
- Assert the receiver is revived exactly once.
|
||||
- Cover the new rootless-tombstone handshake followed by stale-history
|
||||
rejection.
|
||||
- Rename the IP test so its name honestly describes what it proves; the current
|
||||
helper no longer accepts a source address.
|
||||
|
||||
## Findings not worth pursuing now
|
||||
|
||||
- **Backend/frontend semantic duplication:** a real maintenance risk, but
|
||||
eliminating it would require an architectural rewrite. Shared conformance
|
||||
fixtures could be a later improvement.
|
||||
- **Full-store cloning per merge:** acceptable under the 4,096 unresolved-event
|
||||
cap.
|
||||
- **Substring-matched frontend errors:** brittle, but currently pinned by
|
||||
tests; a proper fix requires a typed peer-to-Tauri error contract and is
|
||||
disproportionate for finishing this branch.
|
||||
- **Missing component-test infrastructure:** inspection plus pure reducer tests
|
||||
is adequate here; I would not add a UI test framework solely for these
|
||||
controls.
|
||||
- **Time's-up calls in the badge:** intentional. These calls remain unresolved
|
||||
and visible, even for non-creators.
|
||||
- **The direct-reply observation:** there is nothing to fix; the CLI already
|
||||
uses the reply channel.
|
||||
|
||||
## Recommended finish scope
|
||||
|
||||
The recommended finish scope is:
|
||||
|
||||
1. Tombstone propagation.
|
||||
2. Accurate directory/startup diagnosis.
|
||||
3. A Running-card Launch action.
|
||||
4. Terminal-receipt polish and specification corrections.
|
||||
5. Targeted replication tests.
|
||||
|
||||
Reference in New Issue
Block a user