fix: handle_event(next_round) dead-code early returns#2836
fix: handle_event(next_round) dead-code early returns#2836immortal71 wants to merge 13 commits intoOWASP:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes handle_event("next_round", ...) in the Player LiveView so the if/else expression becomes the function’s final expression, preventing unintended fallthrough execution (DB fetch + PubSub broadcast) when the handler should have returned early.
Changes:
- Moves
Game.find/1,Endpoint.broadcast/3, and the final{:noreply, ...}return inside the outerelsebranch ofif round_open?(game)to eliminate dead-code early returns. - Ensures the no-op branch (
round_open? && !can_continue_round?) can return without triggering unnecessary DB queries and broadcasts.
| {:ok, updated_game} = Game.find(game.id) | ||
|
|
||
| CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) | ||
| CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) | ||
|
|
||
| {:noreply, assign(socket, :game, updated_game)} | ||
| {:noreply, assign(socket, :game, updated_game)} |
There was a problem hiding this comment.
With Game.find/1 + broadcast now only executed in the outer else branch, the round_open?/can_continue_round? branch updates the DB (round_open: false) but returns assign(socket, :game, game) without reloading the updated game. This means the caller’s LiveView state can remain stale until a later PubSub update, and the scheduled :proceed_to_next_round handler will compute rounds_played + 1 from socket.assigns.game (which is now guaranteed stale after this handler). Consider either reloading/broadcasting in the can_continue_round? branch too, or changing handle_info(:proceed_to_next_round, ...) to fetch the latest game from the DB before applying rounds_played updates so it can’t regress/overwrite newer state.
|
@sydseter is this good to go |
- Collapse nested with in handle_params to single chain with correct
{:noreply, redirect} return type (removes 2 dead error branches)
- Replace with/else in handle_info(game:updated) with case; add
coveralls-ignore on unreachable {:error} arm
- In handle_event(next_round) can_continue? branch: reload game from DB
before returning so LiveView state reflects round_open: false
(addresses Copilot stale-state review comment)
- Simplify {:error, changeset} arms in toggle_continue_vote and
toggle_vote to {:error, _} + Logger.debug; add coveralls-ignore
immortal71
force-pushed
the
fix/2632-next-round-dead-code
branch
from
8677e80 to
2d5efa3
Compare
immortal71
force-pushed
the
fix/2632-next-round-dead-code
branch
from
032e3b2 to
ed193f0
Compare
|
finally, is this good to go...? |
| # coveralls-ignore-start | ||
| {:error, _reason} -> | ||
| {:noreply, redirect(socket, to: "/error")} | ||
| # coveralls-ignore-stop |
| case Player.find(socket.assigns.player.id) do | ||
| {:ok, updated_player} -> | ||
| {:noreply, socket |> assign(:game, updated_game) |> assign(:player, updated_player)} | ||
| # coveralls-ignore-start |
| # coveralls-ignore-start | ||
| {:error, _reason} -> | ||
| {:noreply, socket} | ||
| # coveralls-ignore-stop |
| CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) | ||
| {:noreply, assign(socket, :game, updated_game)} | ||
| else | ||
| # coveralls-ignore-start |
| # coveralls-ignore-start | ||
| Logger.warning("Unauthorized vote attempt: player_id: #{player.id}, dealt_card_id: #{dealt_card_id}, game_id: #{game.id}") | ||
| {:noreply, socket |> put_flash(:error, "Invalid card selection")} | ||
| # coveralls-ignore-stop |
|
@immortal71 there are also some conflicts |
…I controller conflicts)
|
@sydseter is this good to go ? |
|
@immortal71 Please see my comments on your changes. Thank you for your time. |
okayyyyyy |
|
@sydseter done !! |
Fixes #2632
The else branch of the outer if round_open? block was closed before Game.find and Endpoint.broadcast, so those three lines executed unconditionally on every call regardless of which branch was taken.
Fix: move Game.find, Endpoint.broadcast, and {:noreply, assign(...)} inside the else branch so the outer if/else is the function's last expression and each branch returns its own value properly.