Review comments

assets/js/dashboard/stats/behaviours/funnel.js

@@ -11,6 +11,7 @@ import LazyLoader from '../../components/lazy-loader'
 Chart.register(ChartDataLabels);
 
 // TODO: still need to update the state nicely if a funnel gets deleted
+// TODO: refactor to a function component
 
 export default class Funnel extends React.Component {
   constructor(props) {
@@ -50,6 +51,7 @@ export default class Funnel extends React.Component {
   fetchFunnel() {
     const funnel = this.getFunnel()
     if (typeof funnel === 'undefined') {
+      // TODO: clear local storage for funnels
       this.setState({ loading: false, error: { message: "Failed to locate funnel" } })
     } else {
       api.get(`/api/stats/${encodeURIComponent(this.props.site.domain)}/funnels/${funnel.id}`, this.props.query)

lib/plausible/funnels.ex

@@ -190,6 +190,7 @@ defmodule Plausible.Funnels do
         if current_visitors == 0 or total_visitors == 0 do
           "0.00"
         else
+          # XXX: Talk to Vini
           (current_visitors / total_visitors * 100)
           |> Decimal.from_float()
           |> Decimal.round(2)

lib/plausible/goal/schema.ex

@@ -90,6 +90,7 @@ defimpl Jason.Encoder, for: Plausible.Goal do
   end
 end
 
+# TODO: consider Phoenix.HTML.Safe
 defimpl String.Chars, for: Plausible.Goal do
   def to_string(%{page_path: page_path}) when is_binary(page_path) do
     "Visit " <> page_path

lib/plausible_web/controllers/api/stats_controller.ex

@@ -846,6 +846,7 @@ defmodule PlausibleWeb.Api.StatsController do
     )
   end
 
+  # TODO: tests
   def funnel(conn, %{"id" => funnel_id} = params) do
     site = conn.assigns[:site]
 

lib/plausible_web/live/funnel_settings.ex

@@ -2,6 +2,8 @@ defmodule PlausibleWeb.Live.FunnelSettings do
   use Phoenix.LiveView
   use Phoenix.HTML
 
+  use Plausible.Funnel
+
   alias Plausible.{Sites, Goals, Funnels}
 
   def mount(
@@ -38,7 +40,7 @@ defmodule PlausibleWeb.Live.FunnelSettings do
         goals={@goals}
       />
     <% else %>
-      <div :if={Enum.count(@goals) >= 2}>
+      <div :if={Enum.count(@goals) >= Funnel.min_steps()}>
         <.live_component
           module={PlausibleWeb.Live.FunnelSettings.List}
           id="funnels-list"
@@ -47,7 +49,8 @@ defmodule PlausibleWeb.Live.FunnelSettings do
         />
         <button type="button" class="button mt-6" phx-click="add-funnel">+ Add funnel</button>
       </div>
-      <div :if={Enum.count(@goals) < 2}>
+
+      <div :if={Enum.count(@goals) < Funnel.min_steps()}>
         <div class="rounded-md bg-yellow-100 p-4 mt-8">
           <p class="text-sm leading-5 text-gray-900 dark:text-gray-100">
             You need to define at least two goals to create a funnel. Go ahead and <%= link(

lib/plausible_web/live/funnel_settings/form.ex

@@ -11,6 +11,7 @@ defmodule PlausibleWeb.Live.FunnelSettings.Form do
     {:ok,
      assign(socket,
        form: assigns.form,
+       # XXX: assigns goals at the top level/mount?
        goals: assigns.goals,
        site: assigns.site
      )}

lib/plausible_web/live/funnel_settings/input_picker.ex

@@ -2,6 +2,8 @@ defmodule PlausibleWeb.Live.FunnelSettings.InputPicker do
   use Phoenix.LiveComponent
   alias Phoenix.LiveView.JS
 
+  ## FIXME: rename to ComboBox
+
   @max_options_displayed 15
 
   def update(assigns, socket) do
@@ -96,6 +98,7 @@ defmodule PlausibleWeb.Live.FunnelSettings.InputPicker do
   attr(:suggestions, :list, default: [])
   attr(:target, :any)
 
+  # FIXME: bug when skipping via Tab
   def dropdown(assigns) do
     ~H"""
     <ul

test/plausible/funnels_test.exs

@@ -91,6 +91,7 @@ defmodule Plausible.FunnelsTest do
              )
   end
 
+  # But really? XXX: ref: Robert's question
   test "a goal can only appear once in a funnel", %{steps: [g1 | _], site: site} do
     {:error, _changeset} =
       Funnels.create(
@@ -113,7 +114,7 @@ defmodule Plausible.FunnelsTest do
              funnels_list
   end
 
-  test "funnels can be evaluated per site within a time range against an interim definition", %{
+  test "funnels can be evaluated per site within a time range against an ephemeral definition", %{
     site: site,
     goals: [g1, g2, g3 | _]
   } do