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.

[zarvox32]

Review by Claude (Opus 4.7), driven by the PR author's collaborator. Posted on their behalf.

What the code actually does

• c → still shows your own scoresheet directly (unchanged for active players); spectators get the picker.
• shift+c → opens a transient picker (scoresheet_player_select) listing every active player + Back; selecting a player renders their scoresheet via status_box.
• Adds include_spectators=True to d, c, and shift+c so spectators can inspect dice/scoresheets.

Test results

• Existing yahtzee suite: 28 passed (tests/test_yahtzee.py).
• CLI simulate yahtzee --bots 3 --validate-keybinds: game runs to completion, 30/30 keybinds validate.
• Hand-driven smoke test of all paths (own sheet, picker → select, picker → back, spectator → picker, re-press while open): all behave correctly.

Issues

1. PR description contradicts the implementation

The summary says "Modified _action_view_scoresheet to display a transient menu… instead of instantly rendering a scoresheet." That is not what the code does. The code keeps c showing your own sheet directly and adds a separate shift+c keybind for the picker. The Testing Notes ("Press c and verify the target player list is shown") will mislead reviewers — they'll press c, see their own sheet, and assume the PR is broken. Either the description should be rewritten, or the code should match it (single c that always opens the picker).

2. No tests added

70 lines of new behavior, zero test coverage. Per CLAUDE.md, testing is priority 3.

3. Localization gaps

• "action-player-not-found" (spoken via speak_l) — properly localized. ✓
• "yahtzee-check-all-scoresheets" — added to yahtzee.ftl, but the view_all_scoresheets action is Visibility.HIDDEN, so this label is never displayed. Dead string.
• Localization.get(user.locale, "back") for the picker's Back item — properly localized. ✓
• define_keybind("shift+c", "View all scoresheets", [...]) — the keybind's user-facing label is a hardcoded English string, bypassing Fluent. CLAUDE.md is explicit: "Every announcement must go through Fluent… No hardcoded English strings reaching players." (Note: pre-existing "View dice" and "View scoresheet" labels have the same issue — the PR propagated the pattern rather than introducing it, but the new one shouldn't ship that way either.)

4. Two chord keybinds for one feature is overhead

For screen-reader users, fewer chords is better. The single-keybind design implied by the PR description (one c → picker, with your own name listed first) is simpler — fewer mental modes — and would let you delete _action_view_all_scoresheets, the shift+c keybind, the dead ftl entry, and the description/code mismatch in one stroke. shift+c may also collide with screen reader navigation chords for some users.

Recommendation

Pick one design and align code, description, and tests:

• Option A (preferred): Match the PR description — a single c keybind that always opens the picker, with your own name in the list. Drops complexity.
• Option B: Keep the two-keybind design, but rewrite the description and Testing Notes accordingly, localize "View all scoresheets", and either use or remove yahtzee-check-all-scoresheets.

Either way, please add tests covering the picker flow, the spectator path, and the Back item.

[mohammad-aloufi]

I can't work with this anymore.

Claude just confuses the hell out of me and is wasting my time at this point.

It suggested separate keybinds for viewing your own sheet vs getting a list of players to view their sheets which I did implement, and it was a good idea actually. One I should've done from the start.
When I added it, it suddenly reverted and thought the first idea is better and that shift + C collides with screen reader keybinds which is nonsense.

It's also confusing the testing notes that I wrote when I first submitted the PR vs the comments which cover the current progress.

Other than that, I should've added tests for sure, but I honestly can't work with this at this point.

I won't close the PR from my end, but I won't be updating it either

[zarvox32]

That is understandable. I’ll see how much I can understand of this manually, and get input from Rory. I’ll ask claude to look at it as a last resort. But it has definitely messed up here.