feat: Elixir upgrade to 1.19.5 (OTP 26) + PostHog analytics#17
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR upgrades the Elixir/OTP toolchain to 1.19.5, integrates PostHog analytics throughout the application, refactors tournament rendering components into a localized module, removes the rate-limiting infrastructure, updates multiple core dependencies, and optimizes the Docker build configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/mtg_friends_web/live/tournament_live/round.ex (2)
28-58:⚠️ Potential issue | 🟠 MajorRedundant DB query — round is fetched twice on
:editaction
get_pairing_id_from_number/3(line 41→206) fetches the round viaRounds.get_round_from_round_number_str!, and thengenerate_socket/4(line 44→62) fetches the same round again. On every edit-pairing navigation, this doubles the DB load for that round (including preloaded pairings).Consider restructuring so the round is fetched once and passed to both the pairing lookup and socket generation:
Sketch
defp apply_action(socket, :edit, %{...}) do case UserAuth.ensure_can_manage_tournament_id(...) do {:ok, socket} -> - with {pairing_number, ""} when pairing_number > 0 <- Integer.parse(pairing_number_str), - {:ok, pairing_id} <- - get_pairing_id_from_number(tournament_id, round_number, pairing_number) do + round = Rounds.get_round_from_round_number_str!(tournament_id, round_number) + with {pairing_number, ""} when pairing_number > 0 <- Integer.parse(pairing_number_str), + {:ok, pairing_id} <- get_pairing_id(round, pairing_number) do socket |> assign(:selected_pairing_id, pairing_id) - |> generate_socket(tournament_id, round_number, :edit) + |> generate_socket_from_round(round, :edit)
92-96: 🧹 Nitpick | 🔵 TrivialSilent no-op when timer
withdoesn't match — consider adding anelseor simplifyingThe
withblock on lines 92–96 has noelseclause, so iftimer_referenceisnil, it silently falls through returningnil(the result ofMap.get). This works but the intent is obscured. A simpleifwould be clearer:Suggested simplification
- with timer_reference <- Map.get(socket.assigns, :timer_reference), - false <- is_nil(timer_reference) do - {:ok, ref} = timer_reference - :timer.cancel(ref) - end + case Map.get(socket.assigns, :timer_reference) do + {:ok, ref} -> :timer.cancel(ref) + _ -> :ok + end
| defp maybe_capture_round_finished( | ||
| %Round{status: previous_status}, | ||
| attrs, | ||
| %Round{} = updated_round | ||
| ) do | ||
| with :finished <- normalize_status(Map.get(attrs, :status) || Map.get(attrs, "status")), | ||
| true <- previous_status != :finished do | ||
| Analytics.capture_round_finished( | ||
| updated_round.tournament_id, | ||
| updated_round.id, | ||
| updated_round.number | ||
| ) | ||
| end | ||
|
|
||
| :ok | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reading updated_round.status instead of re-inspecting attrs.
The current implementation reads the new status from attrs rather than the canonical updated_round.status. For the only call site today (%{status: :finished}), this is equivalent, but it's more fragile: a changeset that coerces or normalises the value would cause a mismatch.
♻️ Suggested refactor
defp maybe_capture_round_finished(
%Round{status: previous_status},
- attrs,
+ _attrs,
%Round{} = updated_round
) do
- with :finished <- normalize_status(Map.get(attrs, :status) || Map.get(attrs, "status")),
- true <- previous_status != :finished do
+ with :finished <- normalize_status(updated_round.status),
+ true <- previous_status != :finished do
f65fdb5 to
66c783b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/mtg_friends_web/live/tournament_live/tournament_edit_form_component.ex (2)
134-136:⚠️ Potential issue | 🟠 MajorAtom table exhaustion risk with
String.to_atom/1on user input.Using
String.to_atom/1ontournament_params["game_code"]from user input can cause atom table exhaustion (atoms are never garbage collected). This is a potential denial-of-service vector.Use
String.to_existing_atom/1instead, which only converts to atoms that already exist in the runtime.🛡️ Proposed fix
defp handle_event("validate", %{"tournament" => tournament_params}, socket) do selected_game_code = - tournament_params["game_code"] |> String.to_atom() + tournament_params["game_code"] |> String.to_existing_atom()As per coding guidelines: "Don't use
String.to_atom/1on user input due to memory leak risk"
145-147:⚠️ Potential issue | 🟠 MajorSame atom exhaustion risk here.
String.to_atom/1on user-provided format string poses the same DoS risk.🛡️ Proposed fix
false -> - tournament_params["format"] |> String.to_atom() + tournament_params["format"] |> String.to_existing_atom()
| # 5. Verify the updated points appear correctly on the new page | ||
| {:ok, round_view, _html} = live(conn, ~p"/tournaments/#{tournament.id}/rounds/1") | ||
|
|
||
| assert render(round_view) =~ "3 pts" | ||
| assert render(round_view) =~ "1 pts" |
There was a problem hiding this comment.
Replace raw HTML assertions with element-based checks.
Line 273-274 asserts on raw HTML strings, which makes LiveView tests brittle and violates the project’s test guidelines. Prefer has_element?/2 (ideally with stable IDs or data-testid attributes) instead of render/1 string matching.
✅ Example adjustment (update selector to match your markup)
- assert render(round_view) =~ "3 pts"
- assert render(round_view) =~ "1 pts"
+ assert has_element?(round_view, "span", "3 pts")
+ assert has_element?(round_view, "span", "1 pts")As per coding guidelines: “Never test against raw HTML. Always use element/2, has_element/2…” and “Instead of relying on testing text content, which can change, favor testing for the presence of key elements.”
1 participant
Also patches issues from
mix credoand cleans up service utilities that are placed outside services.Removes dead code too.
Summary by CodeRabbit
New Features
Chores