Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR plausible#5886

Type: Clean (correct implementation)

Original PR Title: Update goal settings design
Original PR Description: ### Changes

  • Replace the Add goal button in goal settings with a dropdown button to directly select the goal type. This way, a modal opens with the correct form for the selected goal type. The tabs in the modal have been removed.
  • Add a new pill component to show the goal type in the table in a more distinct way. The settings_badge component is replaced with the pill component. The pill component that was used in plan_box.ex is renamed to highlight_pill.
  • Replaced Belongs to funnel text with a funnel icon in the goal settings list.
  • Some small tweaks like increasing the search bar width, the padding of the table cells, and adding a header to the goal settings list.

Tests

  • Automated tests have been added

Dark mode


PR Type

Enhancement


Description

  • Replace "Add goal" button with dropdown to select goal type directly

  • Remove modal tabs and auto-select form based on dropdown choice

  • Replace settings_badge component with new pill component supporting multiple colors

  • Update goal settings table with pill badges for goal types and funnel icon

  • Increase search bar width and adjust table cell padding for better UX


Diagram Walkthrough

flowchart LR
  A["Add goal button"] -->|replaced with| B["Dropdown menu"]
  B -->|options| C["Pageview"]
  B -->|options| D["Custom event"]
  B -->|options| E["Scroll depth"]
  C -->|auto-selects| F["Form without tabs"]
  D -->|auto-selects| F
  E -->|auto-selects| F
  G["settings_badge"] -->|replaced with| H["pill component"]
  H -->|supports colors| I["gray, indigo, yellow, green"]
Loading

File Walkthrough

Relevant files
Refactoring
1 files
plan_box.ex
Rename pill component to highlight_pill                                   
+2/-2     
Enhancement
6 files
generic.ex
Add pill component with color variants                                     
+44/-11 
layout.ex
Replace settings_badge with pill component                             
+3/-1     
goal_settings.ex
Add goal_type state and pass to form                                         
+11/-4   
form.ex
Remove tabs and use goal_type parameter                                   
+26/-110
list.ex
Replace button with dropdown and update goal type display
+86/-28 
sites.ex
Use pill component for pending invitation badge                   
+2/-2     
Formatting
2 files
prima_dropdown.ex
Adjust dropdown item padding and width                                     
+1/-1     
shared_link_settings.ex
Add flex gap wrapper for consistent spacing                           
+48/-46 
Tests
2 files
form_test.exs
Update tests to use goal type dropdown instead of tabs     
+76/-59 
goal_settings_test.exs
Update tests for dropdown-based goal type selection           
+25/-2   

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Action Logging: New user-triggered critical actions like adding goals via the dropdown (event
"add-goal") and saving goals appear without explicit audit logging in the added
code diff.

Referred Code
def handle_event("add-goal", %{"goal-type" => goal_type}, socket) do
  socket =
    socket
    |> assign(form_goal: nil, goal_type: goal_type)
    |> Modal.open("goals-form-modal")

  {:noreply, socket}
end

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Edge Cases: The new flow derives form type from "goal_type" strings and assigns without
visible validation or fallback for unexpected values, which may cause incorrect form
rendering.

Referred Code
form_type =
  if assigns.goal do
    case assigns.goal do
      %{page_path: p, scroll_threshold: s} when not is_nil(p) and s > -1 -> "scroll"
      %{page_path: p} when not is_nil(p) -> "pageviews"
      _ -> "custom_events"
    end
  else
    assigns[:goal_type] || "custom_events"
  end

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Validation: The new "add-goal" event accepts "goal-type" from the client without
visible server-side validation of allowed values before using it to control form
rendering.

Referred Code
def handle_event("add-goal", %{"goal-type" => goal_type}, socket) do
  socket =
    socket
    |> assign(form_goal: nil, goal_type: goal_type)
    |> Modal.open("goals-form-modal")

  {:noreply, socket}
end

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refactor duplicated code into component

Refactor the duplicated "Add goal" dropdown menu into a new function component
to eliminate code redundancy.

lib/plausible_web/live/goal_settings/list.ex [23-195]

-<div class="flex flex-col gap-4">
-  <%= if @searching? or Enum.count(@goals) > 0 do %>
-    <.filter_bar filter_text={@filter_text} placeholder="Search Goals">
-      <PrimaDropdown.dropdown id="add-goal-dropdown">
-        <PrimaDropdown.dropdown_trigger as={&button/1} mt?={false}>
-          Add goal <Heroicons.chevron_down mini class="size-4 mt-0.5" />
-        </PrimaDropdown.dropdown_trigger>
+defp add_goal_dropdown(assigns) do
+  ~H"""
+  <PrimaDropdown.dropdown {@rest}>
+    <PrimaDropdown.dropdown_trigger as={&button/1} mt?={false}>
+      Add goal <Heroicons.chevron_down mini class="size-4 mt-0.5" />
+    </PrimaDropdown.dropdown_trigger>
 
-        <PrimaDropdown.dropdown_menu>
-          <PrimaDropdown.dropdown_item
-            phx-click="add-goal"
-            phx-value-goal-type="pageviews"
-            x-data
-            x-on:click={Modal.JS.preopen("goals-form-modal")}
-          >
-            <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Pageview
-          </PrimaDropdown.dropdown_item>
-          <PrimaDropdown.dropdown_item
-            phx-click="add-goal"
-            phx-value-goal-type="custom_events"
-            x-data
-            x-on:click={Modal.JS.preopen("goals-form-modal")}
-          >
-            <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Custom event
-          </PrimaDropdown.dropdown_item>
-          <PrimaDropdown.dropdown_item
-            phx-click="add-goal"
-            phx-value-goal-type="scroll"
-            x-data
-            x-on:click={Modal.JS.preopen("goals-form-modal")}
-          >
-            <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Scroll depth
-          </PrimaDropdown.dropdown_item>
-        </PrimaDropdown.dropdown_menu>
-      </PrimaDropdown.dropdown>
-    </.filter_bar>
-  <% end %>
+    <PrimaDropdown.dropdown_menu>
+      <PrimaDropdown.dropdown_item
+        phx-click="add-goal"
+        phx-value-goal-type="pageviews"
+        x-data
+        x-on:click={Modal.JS.preopen("goals-form-modal")}
+      >
+        <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Pageview
+      </PrimaDropdown.dropdown_item>
+      <PrimaDropdown.dropdown_item
+        phx-click="add-goal"
+        phx-value-goal-type="custom_events"
+        x-data
+        x-on:click={Modal.JS.preopen("goals-form-modal")}
+      >
+        <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Custom event
+      </PrimaDropdown.dropdown_item>
+      <PrimaDropdown.dropdown_item
+        phx-click="add-goal"
+        phx-value-goal-type="scroll"
+        x-data
+        x-on:click={Modal.JS.preopen("goals-form-modal")}
+      >
+        <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Scroll depth
+      </PrimaDropdown.dropdown_item>
+    </PrimaDropdown.dropdown_menu>
+  </PrimaDropdown.dropdown>
+  """
+end
 
-  <%= if Enum.count(@goals) > 0 do %>
-    ...
-  <% else %>
-    ...
-    <PrimaDropdown.dropdown id="add-goal-dropdown-empty" class="mt-4">
-      <PrimaDropdown.dropdown_trigger as={&button/1} mt?={false}>
-        Add goal <Heroicons.chevron_down mini class="size-4 mt-0.5" />
-      </PrimaDropdown.dropdown_trigger>
+def render(assigns) do
+  # ... (assigns setup)
+  ~H"""
+  <div class="flex flex-col gap-4">
+    <%= if @searching? or Enum.count(@goals) > 0 do %>
+      <.filter_bar filter_text={@filter_text} placeholder="Search Goals">
+        <.add_goal_dropdown id="add-goal-dropdown" />
+      </.filter_bar>
+    <% end %>
 
-      <PrimaDropdown.dropdown_menu>
-        <PrimaDropdown.dropdown_item
-          phx-click="add-goal"
-          phx-value-goal-type="pageviews"
-          x-data
-          x-on:click={Modal.JS.preopen("goals-form-modal")}
-        >
-          <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Pageview
-        </PrimaDropdown.dropdown_item>
-        <PrimaDropdown.dropdown_item
-          phx-click="add-goal"
-          phx-value-goal-type="custom_events"
-          x-data
-          x-on:click={Modal.JS.preopen("goals-form-modal")}
-        >
-          <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Custom event
-        </PrimaDropdown.dropdown_item>
-        <PrimaDropdown.dropdown_item
-          phx-click="add-goal"
-          phx-value-goal-type="scroll"
-          x-data
-          x-on:click={Modal.JS.preopen("goals-form-modal")}
-        >
-          <Heroicons.plus class={PrimaDropdown.dropdown_item_icon_class()} /> Scroll depth
-        </PrimaDropdown.dropdown_item>
-      </PrimaDropdown.dropdown_menu>
-    </PrimaDropdown.dropdown>
-  <% end %>
-</div>
+    <%= if Enum.count(@goals) > 0 do %>
+      ...
+    <% else %>
+      ...
+      <.add_goal_dropdown id="add-goal-dropdown-empty" class="mt-4" />
+    <% end %>
+  </div>
+  """
+end

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies significant code duplication introduced in the PR and proposes a valid refactoring into a reusable component, which improves code quality and maintainability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants