Skip to content

Conversation

hhubert6
Copy link
Contributor

No description provided.

@hhubert6 hhubert6 linked an issue Oct 14, 2025 that may be closed by this pull request
@hhubert6 hhubert6 marked this pull request as ready for review October 15, 2025 10:12
Copy link
Contributor

@GuzekAlan GuzekAlan left a comment

Choose a reason for hiding this comment

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

Looks good

Comment on lines +28 to +29
@spec update_dead_liveviews_setting!(boolean()) :: {:ok, boolean()} | {:error, term()}
def update_dead_liveviews_setting!(new_value) when is_boolean(new_value) do
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to have ! and ok error tuple

end

defp clear_tracing(socket) do
def clear_tracing(socket) do
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: private and public functions should be grouped together

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you handle DeadViewModeEntered event like you do in node_traces_live.ex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I forgot

nil

{:ok, socket} ->
LvProcess.new(pid, socket)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LvProcess.new(pid, socket)
pid
|> LvProcess.new(socket)

Comment on lines +44 to +50
dead_lv_processes =
StatesStorage.get_all_states()
|> Enum.filter(fn {pid, %LvState{}} -> not Process.alive?(pid) end)
|> Enum.map(&elem(&1, 1))
|> Enum.map(&(LvProcess.new(&1.pid, &1.socket) |> LvProcess.set_alive(false)))

{:ok, %{dead_grouped_lv_processes: LiveViewDiscovery.group_lv_processes(dead_lv_processes)}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dead_lv_processes =
StatesStorage.get_all_states()
|> Enum.filter(fn {pid, %LvState{}} -> not Process.alive?(pid) end)
|> Enum.map(&elem(&1, 1))
|> Enum.map(&(LvProcess.new(&1.pid, &1.socket) |> LvProcess.set_alive(false)))
{:ok, %{dead_grouped_lv_processes: LiveViewDiscovery.group_lv_processes(dead_lv_processes)}}
dead_grouped_lv_processes =
StatesStorage.get_all_states()
|> Enum.filter(fn {pid, %LvState{}} -> not Process.alive?(pid) end)
|> Enum.map(&elem(&1, 1))
|> Enum.map(&(LvProcess.new(&1.pid, &1.socket) |> LvProcess.set_alive(false)))
|> LiveViewDiscovery.group_lv_processes()
{:ok, %{dead_grouped_lv_processes: dead_grouped_lv_processes}}

Comment on lines +18 to +26
LiveViews listed below are not active anymore and they will be removed in a short time (usually within 2 seconds).
If you want to keep them for a longer time you may do so by disabling
<b>Garbage Collection</b>
in <.link navigate={RoutesHelper.settings()} class="underline cursor-pointer">settings</.link>. But be aware that this will lead to increased memory usage.
<% else %>
You have <b>Garbage Collection</b>
disabled which means that LiveViews listed below will not be removed automatically.
This will lead to increased memory usage. You can enable it
in <.link navigate={RoutesHelper.settings()} class="underline cursor-pointer">settings</.link>.
Copy link
Member

Choose a reason for hiding this comment

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

I'd wrap these texts in <p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whole if expression is in <p> tag. You want me to add one more inside each expression case?

Copy link
Member

Choose a reason for hiding this comment

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

I'd consider splitting dead and active liveviews section do LiveComponents

@kraleppa
Copy link
Member

I'd also like to redesign it a bit - this is my proposition, but we can wait for the designs.

I'd change the title to Dead LiveViews

I wouldn't display Dead LiveViews at all with garbage collection enabled, since having LiveView displayed for 2s is not that helpful

I think that the warning text is too long in both variants and we should probably use component similar to our flash but implementation for warning. Maybe you could decrease number of text by moving some stuff to tooltip

Imo this color in warning is too gold(ish) - I'd change it to swm-yellow-80 or -60

From left to right: Dead LiveViews inactive variant, variant with Garbage collection on, variant with garbage collection off
LiveDebugger-1 0 0 drawio

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.

Add "Dead LiveViews" section to DiscoveryLive

3 participants