feat: add +10s seek forward button to admin control bar#511
Conversation
- Adds ⏩ +10s button next to volume controls - Reads current media_position from HA state and seeks to pos + 10s - Only visible during PLAYING and REVEAL phases - Adds tooltips to all admin control bar buttons Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "seek forward" capability for the media player, allowing admins to skip 10 seconds ahead during the playing and reveal phases. The implementation includes a new websocket action, backend service logic, and a UI button in the admin panel. Review feedback identifies a bug in how media position is calculated from Home Assistant's state, a missing method in the MediaPlayerProtocol, the need for input validation on the websocket data, and a requirement to use internationalization keys for UI tooltips.
| current_pos = state.attributes.get("media_position", 0) or 0 | ||
| new_pos = current_pos + seconds |
There was a problem hiding this comment.
In Home Assistant, the media_position attribute is a snapshot of the position at the time recorded in media_position_updated_at. If the player is currently playing, simply adding seconds to the stale media_position will result in an inaccurate seek (it will effectively seek forward from where the song was when the state last updated, not from the current real-time position). You should calculate the current position by accounting for the time elapsed since the last update.
current_pos = state.attributes.get("media_position", 0) or 0
if state.state == "playing":
updated_at = state.attributes.get("media_position_updated_at")
if updated_at:
import homeassistant.util.dt as dt_util
current_pos += (dt_util.utcnow() - updated_at).total_seconds()
new_pos = current_pos + seconds| async def seek_forward(self, seconds: int) -> bool: | ||
| """Seek media player forward by given seconds (#498).""" | ||
| if self._media_player_service: | ||
| return await self._media_player_service.seek_forward(seconds) |
There was a problem hiding this comment.
The MediaPlayerProtocol defined in custom_components/beatify/game/protocols.py is missing the seek_forward method. Since _media_player_service is typed as MediaPlayerProtocol, this call will fail static type checking and violates the service contract. Please update the protocol definition to include this new method.
| """Handle admin seek_forward action (#498).""" | ||
| if game_state.phase not in (GamePhase.PLAYING, GamePhase.REVEAL): | ||
| return | ||
| seconds = data.get("seconds", 10) |
There was a problem hiding this comment.
It is safer to validate that seconds is an integer before passing it to the game state, similar to how the year is validated in the submit handler. This prevents potential type errors if a client sends a non-integer value.
| seconds = data.get("seconds", 10) | |
| seconds = data.get("seconds", 10) | |
| if not isinstance(seconds, int): | |
| seconds = 10 |
| <button id="admin-seek-forward" class="btn btn-secondary btn-compact" title="Seek forward 10 seconds">⏩ +10s</button> | ||
| <button id="admin-vol-down" class="btn btn-secondary btn-compact" title="Volume down">🔉</button> | ||
| <button id="admin-vol-up" class="btn btn-secondary btn-compact" title="Volume up">🔊</button> | ||
| <button id="admin-end-game-playing" class="btn btn-danger btn-compact" title="End game">End Game</button> |
There was a problem hiding this comment.
The new title attributes should use the project's internationalization system (e.g., data-i18n-title) instead of hardcoded English strings to remain consistent with the rest of the UI. Additionally, the Stop button is missing a tooltip, which contradicts the PR summary stating tooltips were added to all buttons.
HA caches media_position and only updates at media_position_updated_at. Without adjusting for elapsed time since last update, seeking would jump backward to the stale position then add 10s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mholzi
left a comment
mholzi
left a comment
There was a problem hiding this comment.
Code Review: LGTM (after fix)
HTML/JS/WebSocket all follow existing patterns correctly. Pushed fix for stale media_position bug: now adjusts for elapsed time since media_position_updated_at to prevent seeking backward. Phase gate (PLAYING/REVEAL) is correct.
1 participant
Summary
media_positionfrom HA state and seeks toposition + 10stitletooltips to all admin control bar buttonsCloses #498
Test plan
🤖 Generated with Claude Code