Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ChartComponent bug, correctly display metrics history #469

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

pojiro
Copy link
Contributor

@pojiro pojiro commented Feb 20, 2025

The bug I found is #468, I confirmed it occurs on phoenix_live_view 1.0.3 and 1.0.4.

So first I reproduced the issue with mix test with 1.0.4 by c375bb5.
Then make a fix commit, 5dc6fbe.

Fix commit doesn't effect mix test results on phoenix_live_view 1.0.0, so we can remove the c375bb5 if we need to remove the commit.

Thanks.

@@ -50,7 +50,14 @@ defmodule Phoenix.LiveDashboard.ChartComponent do
<div class="card-body">
<!-- We don't add a phx-update="stream" because js expects only new data -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, could this comment be improved? (I'm not sure how it should be written, though.)

@josevalim josevalim requested a review from SteffenDE February 20, 2025 04:55
@SteffenDE
Copy link
Contributor

It's this: phoenixframework/phoenix_live_view@65bdbc4

I think we should just revert 946a1fd. Temporary assigns are completely fine to use here and also make the code easier, especially since stream features are not needed.

@pojiro
Copy link
Contributor Author

pojiro commented Feb 20, 2025

@SteffenDE Thanks for your response!
So what should i do? Just delete two commits and make one revert commit for this branch and force push it?

@SteffenDE
Copy link
Contributor

@pojiro you can just replace the chart component file with the old version that uses temporary assigns:

defmodule Phoenix.LiveDashboard.ChartComponent do
  use Phoenix.LiveDashboard.Web, :live_component

  @impl true
  def mount(socket) do
    {:ok, socket, temporary_assigns: [data: []]}
  end

  @impl true
  def update(assigns, socket) do
    socket = assign(socket, assigns)
    validate_assigns!(socket.assigns)
    {:ok, socket}
  end

  defp validate_assigns!(assigns) do
    validate_positive_integer_or_nil!(assigns[:bucket_size], :bucket_size)
    validate_positive_integer_or_nil!(assigns[:prune_threshold], :prune_threshold)
    :ok
  end

  defp validate_positive_integer_or_nil!(nil, _field), do: nil

  defp validate_positive_integer_or_nil!(value, field) do
    unless is_integer(value) and value > 0 do
      raise ArgumentError,
            "#{inspect(field)} must be a positive integer, got: #{inspect(value)}"
    end

    value
  end

  @impl true
  def render(assigns) do
    ~H"""
    <div class={chart_size(@full_width)}>
      <div id={"chart-#{@id}"} class="card">
        <div class="card-body">
          <div phx-hook="PhxChartComponent" id={"chart-#{@id}-datasets"} hidden>
            <span :for={{x, y, z} <- @data} data-x={x || @label} data-y={y} data-z={z}></span>
          </div>
          <div class="chart"
              id={"chart-ignore-#{@id}"}
              phx-update="ignore"
              data-label={@label}
              data-metric={@kind}
              data-title={@title}
              data-tags={Enum.join(@tags, "-")}
              data-unit={@unit}
              data-prune-threshold={@prune_threshold}
              {bucket_size(@bucket_size)}
            >
          </div>
        </div>
        <Phoenix.LiveDashboard.PageBuilder.hint :if={@hint} text={@hint} />
      </div>
    </div>
    """
  end

  defp chart_size(_full_width = true), do: "col-12 charts-col"
  defp chart_size(_full_width = false), do: "col-xl-6 col-xxl-4 col-xxxl-3 charts-col"

  defp bucket_size(nil), do: %{}
  defp bucket_size(integer) when is_integer(integer), do: %{data_bucket_size: to_string(integer)}
end

and then maybe run mix format lib/phoenix/live_dashboard/components/chart_component.ex. Then we should be good to go :)

This issue occurred in liveview version 1.0.2 .. 1.0.4 and was caused by
phoenixframework/phoenix_live_view@65bdbc4

Fixed by replacing `stream` with `temporary_assigns`, stream features
are not required in this case.
@pojiro
Copy link
Contributor Author

pojiro commented Feb 20, 2025

@SteffenDE I did it!

@SteffenDE SteffenDE merged commit c939321 into phoenixframework:main Feb 21, 2025
4 checks passed
@SteffenDE
Copy link
Contributor

@pojiro thank you! 🙌🏻

@pojiro pojiro deleted the patch-1 branch February 21, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants