Skip to content

fix(forkchoice-viz): expose all role overlaps per block #341

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.

Sign up for GitHub

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

Open
conache wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(forkchoice-viz): expose all role overlaps per block #341

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0944d58
Add halo nodes indicating overlapping node roles
conache May 1, 2026
b42961c
Add suggested changes
conache May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 98 additions & 27 deletions crates/net/rpc/static/fork_choice.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@
cursor: pointer;
}

.halo {
fill: none;
stroke-width: 2;
pointer-events: none;
}
Comment on lines +81 to +85
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 1, 2026

Choose a reason for hiding this comment

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

P2 Halo ring area is not hoverable

The halo circles have pointer-events: none, and the node-hit circle's radius stays at NODE_RADIUS — not extended to cover the outermost halo at NODE_RADIUS + 18. Any cursor position strictly within a halo ring (between NODE_RADIUS and NODE_RADIUS + 18) falls outside every pointer-receiving element, so the tooltip won't appear and the cursor won't show as pointer there. Updating the hit target's radius to match the outermost halo would fix this — in the JS where node-hit is appended:

.attr("r", NODE_RADIUS + MAX_HALO_OFFSET);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/net/rpc/static/fork_choice.html
Line: 81-85

Comment:
**Halo ring area is not hoverable**

The halo circles have `pointer-events: none`, and the `node-hit` circle's radius stays at `NODE_RADIUS` — not extended to cover the outermost halo at `NODE_RADIUS + 18`. Any cursor position strictly within a halo ring (between `NODE_RADIUS` and `NODE_RADIUS + 18`) falls outside every pointer-receiving element, so the tooltip won't appear and the cursor won't show as `pointer` there. Updating the hit target's radius to match the outermost halo would fix this — in the JS where `node-hit` is appended:
```js
.attr("r", NODE_RADIUS + MAX_HALO_OFFSET);
```

How can I resolve this? If you propose a fix, please make it concise.


.node-inner {
stroke: none;
pointer-events: none;
Expand Down Expand Up @@ -182,6 +188,21 @@
default: "#666"
};

const ROLE_LABELS = {
finalized: "finalized",
justified: "justified",
safeTarget: "safe target",
head: "head"
};

// Concentric halos drawn outside the primary ring, one per secondary role.
const HALO_OFFSETS = [6, 12, 18];
const MAX_HALO_OFFSET = HALO_OFFSETS[HALO_OFFSETS.length - 1];
// Reserve label space below the outermost halo so the gap between the
// outermost circle and the label stays constant across all nodes,
// regardless of how many halos are drawn.
const LABEL_DY = NODE_RADIUS + MAX_HALO_OFFSET + 14;

const container = document.getElementById("chart-container");
const tooltip = document.getElementById("tooltip");
const emptyMsg = document.getElementById("empty-message");
Expand All @@ -204,12 +225,16 @@
return root.length > 10 ? root.slice(0, 10) : root;
}

function nodeColor(node, data) {
if (node.root === data.head) return COLORS.head;
if (node.root === data.safe_target) return COLORS.safeTarget;
if (node.root === data.justified.root) return COLORS.justified;
if (node.slot <= data.finalized.slot) return COLORS.finalized;
return COLORS.default;
// Returns the roles that apply to a block, in natural priority order
// (strongest commitment first). The first entry drives the primary color;
// the rest become halos around it.
function nodeRoles(node, data) {
const roles = [];
if (node.slot <= data.finalized.slot) roles.push("finalized");
if (node.root === data.justified.root) roles.push("justified");
if (node.root === data.safe_target) roles.push("safeTarget");
if (node.root === data.head) roles.push("head");
return roles;
}

function weightRatio(node, validatorCount) {
Expand Down Expand Up @@ -304,13 +329,18 @@
d.x += centerOffset;
});

const flatNodes = allDescendants.map(d => ({
...d.data,
x: d.x,
y: d.y,
_color: nodeColor(d.data, data),
_ratio: weightRatio(d.data, data.validator_count)
}));
const flatNodes = allDescendants.map(d => {
const roles = nodeRoles(d.data, data);
return {
...d.data,
x: d.x,
y: d.y,
_color: roles.length > 0 ? COLORS[roles[0]] : COLORS.default,
_ratio: weightRatio(d.data, data.validator_count),
// Colors of secondary roles, in priority order (after the primary).
_haloColors: roles.slice(1).map(r => COLORS[r])
};
});

const links = [];
Comment on lines +335 to 345
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 1, 2026

Choose a reason for hiding this comment

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

P2 nodeRoles called twice per node during layout

Inside the flatNodes map, roles is already computed via nodeRoles(d.data, data), but the call to nodeColor(d.data, data) on the next property invokes nodeRoles a second time internally. The primary color can be derived directly from the already-computed array:

_color: roles.length > 0 ? COLORS[roles[0]] : COLORS.default,
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/net/rpc/static/fork_choice.html
Line: 340-350

Comment:
**`nodeRoles` called twice per node during layout**

Inside the `flatNodes` map, `roles` is already computed via `nodeRoles(d.data, data)`, but the call to `nodeColor(d.data, data)` on the next property invokes `nodeRoles` a second time internally. The primary color can be derived directly from the already-computed array:
```js
_color: roles.length > 0 ? COLORS[roles[0]] : COLORS.default,
```

How can I resolve this? If you propose a fix, please make it concise.

hierarchy.links().forEach(link => {
Expand Down Expand Up @@ -338,17 +368,38 @@
// requiring the user to move the mouse.
let hoveredRoot = null;

function tooltipHtml(d, total) {
const pct = total ? parseFloat(((d.weight / total) * 100).toFixed(2)) : 0;
return `<span class="tt-label">root:</span> ${truncateRoot(d.root)}<br>` +
`<span class="tt-label">slot:</span> ${d.slot}<br>` +
`<span class="tt-label">proposer:</span> ${d.proposer_index}<br>` +
`<span class="tt-label">weight:</span> ${d.weight}${total != null ? `/${total} (${pct}%)` : ''}`;
function tooltipHtml(d, data) {
const roles = nodeRoles(d, data);
const lines = [
`<span class="tt-label">root:</span> ${truncateRoot(d.root)}`,
`<span class="tt-label">slot:</span> ${d.slot}`,
`<span class="tt-label">proposer:</span> ${d.proposer_index}`,
];

if (roles.length > 0) {
const roleSpans = roles
.map(r => `<span style="color: ${COLORS[r]}">${ROLE_LABELS[r]}</span>`)
.join(", ");
lines.push(`<span class="tt-label">status:</span> ${roleSpans}`);
}

// Skip the weight line for purely-finalized blocks: their fork-choice
// weight is 0 by design and reading "weight: 0" alongside "finalized" is
// misleading. Any non-finalized role still gets a meaningful number.
const isPureFinalized = roles.length === 1 && roles[0] === "finalized";
if (!isPureFinalized) {
const total = data.validator_count;
const pct = total ? parseFloat(((d.weight / total) * 100).toFixed(2)) : 0;
const suffix = total != null ? `/${total} (${pct}%)` : "";
lines.push(`<span class="tt-label">weight:</span> ${d.weight}${suffix}`);
}

return lines.join("<br>");
}

function showTooltip(event, d) {
function showTooltip(event, d, data) {
hoveredRoot = d.root;
tooltip.innerHTML = tooltipHtml(d, currentData?.validator_count);
tooltip.innerHTML = tooltipHtml(d, data);
tooltip.style.opacity = 1;
tooltip.style.left = (event.clientX + 14) + "px";
tooltip.style.top = (event.clientY - 14) + "px";
Expand Down Expand Up @@ -466,21 +517,33 @@
.attr("r", NODE_RADIUS)
.attr("stroke", d => d._color);

// Invisible hit target so hover works regardless of fill level.
// Halo rings, one per secondary role. Always rendered with a transparent
// stroke when no role applies so transitions can animate stroke color
// smoothly when overlap appears or disappears.
HALO_OFFSETS.forEach((offset, i) => {
nodeEnter.append("circle")
.attr("class", `halo halo-${i}`)
.attr("r", NODE_RADIUS + offset)
.attr("stroke", d => d._haloColors[i] || "transparent");
});

// Invisible hit target so hover works regardless of fill level. Sized to
// cover the outermost halo so the cursor still triggers the tooltip when
// it sits between rings.
nodeEnter.append("circle")
.attr("class", "node-hit")
.attr("r", NODE_RADIUS);
.attr("r", NODE_RADIUS + MAX_HALO_OFFSET);

nodeEnter.append("text")
.attr("class", "node-label")
.attr("dy", NODE_RADIUS + 14)
.attr("dy", LABEL_DY)
.text(d => truncateRoot(d.root));

const nodeMerged = nodeEnter.merge(nodeGroups);

nodeMerged
.on("mouseover", function (event, d) { showTooltip(event, d); })
.on("mousemove", function (event, d) { showTooltip(event, d); })
.on("mouseover", function (event, d) { showTooltip(event, d, data); })
.on("mousemove", function (event, d) { showTooltip(event, d, data); })
.on("mouseout", hideTooltip);

nodeMerged
Expand All @@ -507,13 +570,21 @@
.duration(100)
.attr("stroke", d => d._color);

HALO_OFFSETS.forEach((_, i) => {
nodeMerged.select(`.halo-${i}`)
.transition()
.delay(TRANSITION_DURATION)
.duration(100)
.attr("stroke", d => d._haloColors[i] || "transparent");
});
Comment on lines +573 to +579
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 1, 2026

Choose a reason for hiding this comment

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

P2 Halo strokes update without a transition

node-inner and node-outer both use .transition().delay(TRANSITION_DURATION).duration(100) when their color changes, but the halo update here calls .attr(...) directly with no transition. When a block gains or loses a role between polls, the halo color flips instantaneously while the primary ring animates. Adding the same transition keeps them in sync:

nodeMerged.select(`.halo-${i}`)
  .transition()
  .delay(TRANSITION_DURATION)
  .duration(100)
  .attr("stroke", d => d._haloColors[i] || "transparent");
Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/net/rpc/static/fork_choice.html
Line: 576-579

Comment:
**Halo strokes update without a transition**

`node-inner` and `node-outer` both use `.transition().delay(TRANSITION_DURATION).duration(100)` when their color changes, but the halo update here calls `.attr(...)` directly with no transition. When a block gains or loses a role between polls, the halo color flips instantaneously while the primary ring animates. Adding the same transition keeps them in sync:
```js
nodeMerged.select(`.halo-${i}`)
  .transition()
  .delay(TRANSITION_DURATION)
  .duration(100)
  .attr("stroke", d => d._haloColors[i] || "transparent");
```

How can I resolve this? If you propose a fix, please make it concise.


nodeMerged.select("text")
.text(d => truncateRoot(d.root));

// Keep the tooltip live while the user holds the mouse still over a node.
if (hoveredRoot) {
const hovered = layout.nodes.find(n => n.root === hoveredRoot);
if (hovered) tooltip.innerHTML = tooltipHtml(hovered, data.validator_count);
if (hovered) tooltip.innerHTML = tooltipHtml(hovered, data);
}

// Auto-scroll to head node
Expand Down