fix(forkchoice-viz): expose all role overlaps per block #341
fix(forkchoice-viz): expose all role overlaps per block #341conache wants to merge 2 commits intolambdaclass:mainfrom
Conversation
Greptile SummaryThis PR reworks the fork-choice tree visualizer so that every role a block holds (finalized, justified, safe target, head) is shown simultaneously — the innermost filled circle carries the strongest role's color, and extra concentric halo rings display secondary roles. The tooltip's Confidence Score: 4/5Safe to merge — visualization-only change with no consensus or weight-computation impact; three P2 polish issues found. No P0/P1 issues found. Three P2 findings: halo ring area not interactive (hit-target radius not extended), redundant nodeRoles calls per node, and missing transitions on halo stroke updates. None affect correctness. crates/net/rpc/static/fork_choice.html — hit-target radius and halo transition consistency.
|
| Filename | Overview |
|---|---|
| crates/net/rpc/static/fork_choice.html | Adds concentric halo rings and multi-role tooltips to the fork-choice visualizer; halo hit-target radius not extended to match outermost ring, and halo stroke updates lack the transition applied to other node colors. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[poll: fetch /fork_choice_data] --> B[render data]
B --> C[nodeRoles node, data]
C --> D{roles found?}
D -- "roles[0] = primary" --> E[_color = COLORS primary]
D -- "roles[1..] = secondary" --> F[_haloColors array]
D -- "empty" --> G[_color = COLORS.default]
E & F & G --> H[D3 data join layout.nodes]
H --> I[nodeEnter: append circles]
I --> I1[node-inner filled circle]
I --> I2[node-outer ring]
I --> I3[halo-0 / halo-1 / halo-2 rings]
I --> I4[node-hit transparent r=NODE_RADIUS]
H --> J[nodeMerged: update existing]
J --> J1[transition node-inner fill]
J --> J2[transition node-outer stroke]
J --> J3[instant halo stroke update]
J --> J4[re-bind mouseover/mousemove with current data]
J4 --> K[showTooltip → tooltipHtml d, data]
K --> L[nodeRoles d, data again]
L --> M[status: colored role spans]
L --> N{isPureFinalized?}
N -- no --> O[show weight line]
N -- yes --> P[hide weight line]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/net/rpc/static/fork_choice.html:81-85
**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);
```
### Issue 2 of 3
crates/net/rpc/static/fork_choice.html:340-350
**`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,
```
### Issue 3 of 3
crates/net/rpc/static/fork_choice.html:576-579
**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");
```
Reviews (1): Last reviewed commit: "Add halo nodes indicating overlapping no..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .halo { | ||
| fill: none; | ||
| stroke-width: 2; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
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.| ...d.data, | ||
| x: d.x, | ||
| y: d.y, | ||
| _color: nodeColor(d.data, data), | ||
| _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 = []; |
There was a problem hiding this 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:
_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.| HALO_OFFSETS.forEach((_, i) => { | ||
| nodeMerged.select(`.halo-${i}`) | ||
| .attr("stroke", d => d._haloColors[i] || "transparent"); | ||
| }); |
There was a problem hiding this comment.
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.1 participant
🗒️ Description / Motivation
Each block in the fork choice tree can hold multiple roles at the same time - the latest block is usually both the head and the safe target, for instance. Until now the visualization only painted one color per block, picked by a fixed priority (head > safe target > justified > finalized), so whichever roles weren't on top got silently dropped. That made it impossible to tell from the visualisation that a block was both head and safe target, or to find the safe target at all when it coincided with the head.
This PR makes every role visible. The primary color (the inner filled circle) now follows a different priority - finalized > justified > safe target > head (strongest commitment first) and any additional roles the block holds are drawn as concentric rings around the primary one, each in that role's color. The tooltip's
status:line lists all the roles, with each name colored to match its ring.Preview:
What Changed
nodeRoles(node, data)helper returning all roles a block holds, ordered by natural priority (finalized > justified > safeTarget > head— strongest commitment first). The first entry drives the primary color; the rest become halo rings.[6, 12, 18]give comfortable spacing.status:line lists all roles for the hovered block, each colored with its role color (e.g.,safe target, headrendered in yellow + orange).Correctness / Behavior Guarantees
head > safe_target > justified > finalized; nowfinalized > justified > safeTarget > head. A block that is both head + safe_target (the common case) now renders yellow primary with an orange halo rather than the previous pure-orange (which shadowed safe_target).Tests Added / Run
Related Issues / PRs