Enable viewing other players' Yahtzee scoresheets

[mohammad-aloufi]

Summary

This PR enhances the UX in Yahtzee by allowing players to check the scoresheets of any active participant in the game. Previously, the c keybind would automatically jump into displaying the scoresheet of the player whose turn it currently is. This introduces a transient menu when pressing c giving the flexibility to select and view the detailed scoresheet of any given active player.

Changes Made

• Modified _action_view_scoresheet to display a transient menu (kind="scoresheet_player_select") populated with all active players in the game instead of instantly rendering a scoresheet.
• Introduced a new _show_player_scoresheet helper method to build and render the target player's metrics.
• Intercepted menu selections by overriding _handle_transient_display_selection in YahtzeeGame.

Testing Notes

• Start a Yahtzee game in local/server environment.
• Press c at any point during active play and verify that the target player list is shown correctly.
• Ensure selecting a targeted player effectively renders and displays the intended scoresheet properly without locking UI components.

[zarvox32]

Review generated by Claude (Anthropic's coding assistant), posted at the repo owner's request.

Thanks for this — being able to inspect any player's sheet is a nice quality-of-life win for Yahtzee. Claude tested the PR end-to-end (real YahtzeeGame, real transient-display state machine, real _handle_transient_display_selection dispatch). The feature works as documented: picker shows all active players, excludes spectators, includes bots, viewer's locale is used for the rendered sheet, re-pressing c while a sheet is showing reopens the picker cleanly, and Enter on the sheet closes it via the inherited status_box path. Existing Yahtzee suite (28), event-handling/mixin suite (30), and a CLI sim with --test-serialization all pass.

A few things worth discussing.

UX concern

Common case is now slower. Pressing c used to show your own sheet in one keystroke. It's now c → arrow/letter → enter → sheet — three keystrokes for what's probably the most-frequent operation. The PR description doesn't acknowledge this trade-off. Two ways to keep the fast path:

• c → your sheet, shift+c → picker (cheap, two keybinds)
• c → picker but pre-positioned on the viewer's own row, so a single enter shows your own sheet (one extra keystroke instead of two-plus)

Worth picking one before merging.

Picker also blocks all other keybinds while open. Pre-existing behavior — event_handling_mixin.py:97-98 drops all keybind events while a transient display is open — but it means while the picker is up, the player can't press r to roll, s for scores, etc. They have to dismiss with Escape first. Combined with the extra navigation step, friction adds up.

Code-quality issues

1. super() then re-fetch state is a fragile pattern (game.py:629-641):

def _handle_transient_display_selection(self, player, selection_id):
    super()._handle_transient_display_selection(player, selection_id)
    state = self._get_transient_display_state(player)
    if not state:
        return
    if state.kind == "scoresheet_player_select":
        ...

This works today only because the base implementation doesn't mutate state for unknown kinds. If anyone later adds a new kind to the base that does mutate state, the override will dispatch on the wrong state. The robust pattern is to check kind first, handle locally, and delegate to super for unknown kinds:

state = self._get_transient_display_state(player)
if state and state.kind == "scoresheet_player_select":
    self._close_transient_display(player)
    target = self.get_player_by_id(selection_id)
    if target:
        self._show_player_scoresheet(player, target)
    return
super()._handle_transient_display_selection(player, selection_id)

2. Brittle blind cast (game.py:569): ytz_target: YahtzeePlayer = target # type: ignore. The signature is target: Player, but the body assumes YahtzeePlayer and would crash on .scores if a non-Yahtzee player were ever passed. Either narrow the type (target: YahtzeePlayer) or isinstance-guard.

3. get_player_by_id(selection_id) returning None is silent (game.py:639-641): If the selection ID is stale (e.g., player left between picker open and selection), the action does nothing — no speech feedback. Per CLAUDE.md "silence is a bug," a speak_l for the not-found case is warranted.

4. if not active_players: return is silent (game.py:552-553). Edge case (impossible during a real game), but same CLAUDE.md principle. Either add feedback or document why silence is fine here.

5. Trailing whitespace at lines 554 and 632.

Smaller observations

6. Spectators still can't view scoresheets. The c keybind is registered without include_spectators=True (existing behavior, unchanged here). Since "view any player's sheet" is a pure browse capability, this might have been a natural moment to widen it. Not a regression; just an opportunity not taken.

Testing

The PR description describes manual testing only. Claude wrote 8 targeted tests in server/tests/test_yahtzee_scoresheet_select.py covering the picker, spectator exclusion, target-vs-viewer name/locale handling, close path, re-open path, the keybind-blocking behavior (pinned as a known consequence so any future change is intentional), and the empty-list edge case. ~150 LOC, available to attach or push as a follow-up commit if you'd like the regression coverage.

[mohammad-aloufi]

Done.
Summary of fixes:

1. Keybind Reorganization:

   • c: Reverted to viewing your own scoresheet instantly.
   • Shift+c (C): Mapped to the new view_all_scoresheets action, which opens the player picker to view anyone's scoresheet.

2. Fixed Fragile super() Pattern:

   • Refactored _handle_transient_display_selection so it correctly checks if the menu state is our local scoresheet_player_select kind before delegating to the base class. This prevents our override from accidentally dispatching on unknown states that a base class might add in the future.

3. Added "Player Not Found" Feedback:

   • Added speech feedback ("Player not found.") if a player selects a user from the picker who has since disconnected or left the table. This replaces a silent failure with an explicit error message, adhering to the "silence is a bug" principle.

4. Added "No Active Players" Feedback:

   • Handled the edge case where the picker is triggered but active_players is empty. It now correctly speaks "Player not found." instead of silently dropping the action.

5. Cleaned Up Formatting:

   • Removed trailing whitespace on lines 554 and 632 (which was actually a blank line with trailing spaces before the items list generation).

6. Enabled Spectator Viewing:

   • Added include_spectators=True to the c, Shift+c, and d (View dice) keybinds.
   • Added logic so that if a spectator presses c (which tries to view an "own" scoresheet), it gracefully falls back to opening the picker instead, ensuring a seamless experience.