[kraleppa]

No description provided.

[kraleppa]

@claude review

[github-actions]

Claude finished @kraleppa's task in 1m 7s —— View job

---

🦀 Review

Severity: 🔴 blocking · 🟡 should-fix · 🟢 nit

• 🟡 85% lib/voyager_web/components/shell.ex:157: the indicator links to /node/:node/tracing/:id, but no such route exists, so clicking it gives a 404. The path is also built by hand with URI.encode/1, which leaves / in node names unescaped. Use ~p"/node/#{node}/tracing" for now and drop the future-tense comment.

<verdict: 0 blocking, 1 should-fix, 0 nit>

[Copilot]

🟡 Changes recommended

Session creation can crash an open NodeInfoLive page, and the new tracing indicator links to a nonexistent session-viewer route.

3 open issues

• Handle session_created broadcasts in NodeInfoLive · New
• Add accessible label to compact tracing link · New
• Add route for tracing session viewer URLs · New

What changed in this PR

Adds an active tracing-session indicator to Voyager’s app and settings topbars, keeping tracing activity visible across pages.

Changes:

• Adds a LiveView hook that tracks the running session through Tracer broadcasts.
• Displays tracing status, the traced function, trace counts, and a tooltip with session navigation.
• Adds tests for initial state, broadcast updates, formatting, and topbar integration.

File	Description
test/​voyager_web/​hooks/​tracing_status_hook_test.exs	Tests indicator visibility, updates, formatting, and integration.
lib/​voyager_web/​router.ex	Registers the tracing hook for app and settings sessions.
lib/​voyager_web/​hooks/​tracing_status_hook.ex	Maintains the active tracing-session assign.
lib/​voyager_web/​components/​shell.ex	Renders the indicator, tooltip, counts, and session link.
lib/​voyager_web/​components/​layouts.ex	Passes tracing state into both topbars.

_{Review effort: Balanced}

[hhubert6]

For me it looks better aligned to the left and with more spacing around traced mfa

Tracer service essentially operates on three states: empty, loaded/created, running.
So I'm wondering if the indicator should reflect that meaning it could show loaded session and not only running.

[kraleppa]

Tracer service essentially operates on three states: empty, loaded/created, running. So I'm wondering if the indicator should reflect that meaning it could show loaded session and not only running.

I agree other states could be shown, but it depends on what the chip should mean. The issue scopes it to running, so I assumed that it warns about potentially heavy tracing is on. If we also show idle/stopped sessions, it becomes an "open session" indicator that stays until the user closes the session.

I think both are useful, so I went with the second and kept the first visible:

• running: green with a pulse, as before
• ready / stopped: muted grey chip, stays until the session is closed
• stopped on error (node down, write failure): warning color, with the reason in the tooltip

Sessions that aren't configured yet stay hidden, since there's nothing to show or link to.

cc @hhubert6

[Copilot]

🟡 Changes recommended

Synchronous trace-status calls can block page mounting, full-trace subscriptions add unnecessary load across open views, and the session indicator links to a placeholder rather than a session viewer.

3 open issues

• Link the indicator to a session viewer or disable navigation · New
• Publish lightweight status updates instead of full trace batches · New
• Avoid blocking page mounts on the trace-start RPC · New

3 resolved since last review

• Handle session_created broadcasts in NodeInfoLive
• Add route for tracing session viewer URLs
• Add accessible label to compact tracing link

_{Review effort: Balanced}

[hhubert6]

Review of a91371e8. Finding 1 should be fixed before merge; 2-4 are nice to have.

Must fix

1. The chip can link to the wrong node and send the user to the connect page (lib/voyager_web/components/shell.ex:177)

• What this is: A session now stays open after it stops, and it survives a disconnect. The chip builds its link from the session's node, not the connected node.
• Problem: Trace on node A, disconnect, then connect to node B. Pages on B still show A's chip, and the tooltip doesn't name a node. The link is /node/a%40localhost/tracing, and NodeSessionHook redirects it to /. Reproduced in a test against this commit.
• Fix: In topbar, pass node_path(@session, "tracing") into tracing_indicator as the link. Put a Node row back in the tooltip; it was removed because the node name sits right next to the chip, which is no longer always true. The settings page has no connected session, so its link can stay as it is and will still redirect in this case.
• If we skip it: Users see a stopped session from another node as if it belonged to this one, and clicking it takes them out of the node pages. Nothing in lib starts a session yet, so this appears once the Tracing page lands.

Nice to have

2. The chip can stay on "Tracing" after the Tracer restarts (lib/voyager/services/tracer.ex:202)

• What this is: A Writer crash restarts the Tracer, which clears the session without telling any page.
• Problem: Open pages keep a pulsing green chip with a frozen count until the user navigates.
• Fix: Add broadcast_session(nil) to handle_continue(:interrupt_running). The hook already clears the chip on that message.
• If we skip it: After a rare crash, the chip says a trace is running when it isn't.

3. The chip ignores the Erlang syntax setting (lib/voyager_web/components/shell.ex:234)

• What this is: Other pages follow TermSyntax, but the chip always uses Elixir style.
• Problem: On an Erlang node, process pages show lists:map/2 and the chip shows :lists.map/2.
• Fix: Branch on TermSyntax.current() in format_traced_mfa. For :erlang, return "#{module}:#{function}/#{arity || "_"}".
• If we skip it: Only the display is inconsistent.

4. Hook tests still send the message by hand (test/voyager_web/hooks/tracing_status_hook_test.exs:116)

• What this is: The test named "consumes an activated session's broadcast" builds {:session_updated, ...} itself. An earlier version of this test kept passing with an outdated message name while the real message crashed NodeInfoLive.
• Problem: If the Tracer's message changes and the hook isn't updated, the message reaches NodeInfoLive and crashes it again, while this test stays green. tracer_test.exs does check the name, which lowers the risk but doesn't test the hook.
• Fix: Add {:ok, _} = Tracer.create(:ui) to that test. The same call passed when run against this commit.
• If we skip it: A future rename can bring the crash back without any hook test failing.

[kraleppa]

1. The chip can link to the wrong node and send the user to the connect page (lib/voyager_web/components/shell.ex:177)

We could, but it would make our URLs inconsistent: a session traced on node A would live at /node/B/tracing/:id, and the Tracing sessions list on B would need to call that out. Our UI ties every tracing session to the node it traced, so finding A's session in B's list would be misleading.

I think sessions from a previously connected node need their own handling. Either /node/A/tracing/:id should stay reachable while you're connected to B, or the node should be dropped from the session URL entirely, with a clear notice on every page while such a session is active.

Still, I'd prefer if the link details would be discussed during the implementation of the actual tracing view. Currently its temporary because those views does not exist.

But what can be done in this PR is e.g. changing color of this chip in such cases to make sure that user will understand that given tracing session comes from different node.