Enable viewing other players' Yahtzee scoresheets

[mohammad-aloufi]

Summary

Lets a Yahtzee player view any other active player's scoresheet without leaving the game. Adds a separate keybind for the picker so the common case (viewing your own sheet) stays a single keystroke.

Keybind layout

• c — show your own scoresheet immediately (active players). Spectators have no own sheet, so c falls back to the picker.
• shift+c — open a transient player picker listing every active player + Back; selecting a player renders their scoresheet.
• d (view dice), c, and shift+c are all now include_spectators=True, so spectators can inspect dice and scoresheets too.

Code changes

• New _action_view_all_scoresheets that opens the scoresheet_player_select transient display with the active players + Back.
• New _show_player_scoresheet helper that renders a target player's sheet to the viewer (viewer's locale, target's data).
• _action_view_scoresheet (own sheet) now routes spectators to the picker.
• _handle_transient_display_selection handles scoresheet_player_select locally before delegating to the base class (kind-first dispatch, not super-then-state).
• Speech feedback ("Player not found.") when the picker is opened with no active players or a stale selection ID — adheres to CLAUDE.md "silence is a bug".
• Removed dead Fluent entry yahtzee-check-all-scoresheets; the view_all_scoresheets action is hidden, so its label is never rendered.

Tests

server/tests/test_yahtzee_scoresheet_select.py (9 tests):

• c on an active player shows their own scoresheet directly (not the picker).
• shift+c opens the picker with all active players + Back item.
• Picker excludes spectators from the target list.
• Selecting a player renders that player's sheet via status_box.
• Selecting Back closes the picker without opening a sheet.
• Spectator pressing c falls back to the picker.
• Empty active-players list speaks "Player not found." rather than failing silently.
• Stale selection ID after picker open speaks "Player not found.".
• Re-pressing shift+c while a sheet is open reopens the picker.

Testing notes

• Existing yahtzee suite (28) + new tests (9) all pass: uv run pytest tests/test_yahtzee.py tests/test_yahtzee_scoresheet_select.py
• CLI sim runs to completion and all 30 keybinds validate: uv run cli.py simulate yahtzee --bots 3 --validate-keybinds

[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.

[zarvox32]

I've taken some time away from the project since this situation occurred. I blindly followed what claude was saying, and that is entirely on me for causing your frustration. I won't hide the fact that I don't know what I'm doing, but the worst thing that I can do is give up.
Below is a comment that claude drafted, and a solution to the problem. Yeah more vibe coding. But hopefully this will suffice. Again this was completely on me for not checking if any of what it was doing made sense.

---

Mohammad — an apology owed, from both Claude and me.

Looking back at the two reviews together:

• The first review (Claude) proposed c → own sheet + shift+c → picker as one of two valid fast-path designs. You picked it. The second review (Claude) then argued against that exact design. A flat contradiction. Both reviews even used "Option A" to mean opposite things across the two posts.
• "shift+c may collide with screen reader navigation chords" was speculation Claude offered with no concrete chord behind it. You'd know better than the review and you were right to call it nonsense.
• Claude framed your stale Testing Notes as a code/description contradiction. They were just from before the design change. That should have been a one-line "please update the description," not a top-level item.

My piece in this: I posted both reviews without checking whether they were internally consistent before they reached you. I should have caught the flip and the unsubstantiated screen-reader claim before they went out. I'm sorry — that's on me, not on you.

The current implementation is the design Claude's first review endorsed. The second review's keybind-design pushback is withdrawn; what you shipped stays.

The cleanup is mine, not yours. I'll push a follow-up commit to your branch covering: updated PR description, localized "View all scoresheets" label, deleting the now-unused yahtzee-check-all-scoresheets Fluent entry (picking the smaller change over unhiding the action — say the word if you'd rather the other call), and tests for the picker / spectator / Back paths. You stay the PR author; the merge is yours. No obligation to respond.

[mohammad-aloufi]

It's alright. Most of us use AI. I didn't know that Claude is that bad at code review though.
It does seem like it doesn't take the whole conversation into account or something.

Anyways, yeah feel free to go with those changes.