Repository navigation
fix(packets): resolve path hops with the observer that heard them (#2097) - #2099
Merged
Merged
Conversation
…a-clawbot#2097) Every 1-byte path prefix on the network is shared: measured, 254 prefixes cover all 2043 nodes, nine of them on a single prefix. HopResolver already reports such a hop as ambiguous with its candidate list, and HopDisplay already renders a warning badge with a candidate popover. Neither reached the packets page. HopResolver.resolve() takes six parameters. packets.js passed one, so observerId was undefined, packetIata was null, nodeInRegion() never ran, no candidate was flagged regional, globalFallback stayed false, and HopDisplay computed a badge count of 0. One name was printed as certain out of nine candidates, chosen without the geographic filter that was already written. Observed on live data: a hop resolved to a repeater 126 km from the transmitter and 185 km from the observer, while the only candidate with a recorded edge to the next hop in the path sat at the transmitter's own location. resolveHops() now takes the observer and caches under the per-observer key that renderHop() has always read and nothing ever wrote. resolveHopsForPackets() groups a multi-packet call by observer so each group gets its own filtering, and the six multi-packet call sites use it. One site was the bug in its purest form: it wrote the result under hopNameCache[k + ':' + pkt.observer_id] after resolving without that observer, so the key promised something the value was not. The structural guard fails if any call passes the hops alone; verified by reverting one call, which names it in the failure. No browser validation yet; deploying to staging next.
…he list
Both found by looking at the previous commit on staging rather than at its
tests.
The badge is a sibling of its pill, so a path read "A [8] -> B [6] -> C" and
nothing said which name the 8 qualified. It qualifies the one on its left. Pill
and badge now render inside a single nowrap .hop-group, which puts the separator
outside the pair. Only a hop that carries a badge is wrapped: wrapping every hop
would change the layout of every path to no purpose.
The markup predates this work, but the previous commit is what made it visible:
before it, the badge essentially never appeared on the packets page.
In the list, every 1-byte hop getting its own badge turned a row into a line of
warning triangles. renderPath now takes { summary: true } for the three list
call sites, which renders the names with no per-hop badge and appends one
indicator reading "N of M hops have more than one candidate". The two detail-pane
call sites are unchanged and keep a badge per hop, because that is where the
candidate list actually gets read.
HopDisplay gains opts.badge === false for that, and the hop keeps its
hop-ambiguous class either way so CSS can still mark it.
The new .hop-path-warn mirrors the .hop-conflict-btn rule directly above it,
including its var(--status-yellow, #f59e0b) fallback form.
Suite green. Browser check on staging next.
…wbot#2097) The geographic filter in HopResolver has never worked on this network, and the previous commits only made that visible rather than causing it. nodeInRegion() looks the observer's IATA code up in /api/iata-coords. Measured on the live deployment: 0 of 42 observers have their code in that table. All ten codes in use are missing (ANR BRU GNE HEP KJK LGG MST NRW OBL OST) while the 54 entries it does hold are AMS, APC, ATL, AUS and the like. So every candidate came back with regional=false and distKm=undefined, no candidate was ever excluded, and the displayed name was simply the first in index order. 27 of 42 observers report their own lat/lon, and resolve() has always taken observerLat/observerLon as the anchor at the receiving end: when nothing later in the path is resolved, the observer is the next known position, which is what pickByAffinity needs to score candidates. resolveHops now looks the observer up in the existing observerMap and passes it. The test pins the behaviour with iataCoords deliberately empty, which is the live state: with FAR-AWAY listed first, the resolver picks it without an anchor and rules it out with one. It stays reported as ambiguous with all candidates listed, because a better pick is not a certain one. ensureHopResolver() seeds observerMap from the observer list it already fetches, so the anchor is available if a render precedes loadObservers(). The broken IATA table is a separate defect and gets its own issue: it is a network-wide lookup that resolves nothing, and hop resolution may not be its only consumer.
Caught on staging, not in the tests: the detail pane resolves through its own HopResolver.resolve call, and the previous commit gave it the observer id while still passing null for the position. So the path that shows the candidate badges was the one path with no anchor, and it kept picking the first candidate. observerPosition() is now the single lookup and both resolve paths use it. The new guard fails if any call passes an observer id and drops its position; verified by reverting the detail call, which names it in the failure. That is the check that would have caught this before the deploy.
Since Kpa-clawbot#2097 renderHop wraps a hop that carries a badge in a `.hop-group` span. The node full view marks the current node by replacing the first `class="` in that HTML, which now hit the wrapper, so the selected hop lost its outline whenever it had a conflict or unreliable badge. Match `class="hop ` so only the pill is marked. The Kpa-clawbot#1146 E2E counted every nested button as a non-hop child of the chain, so the two wrapped hops read as 16 of 18. Count only direct button children. Locally the Kpa-clawbot#1146 suite went from 7 passed / 4 failed to 11 / 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
CI failure addressed (commit The
Locally the suite went from 7 passed / 4 failed to 11 / 0. The full Playwright suite was not run locally; this CI run is the check for that. |
…guity # Conflicts: # test-all.sh
…ides it .path-hops clips at its right edge. The per-path ambiguity indicator was appended after the last hop, so in a narrow path column it was the thing clipped: every hop still fitted, no +N pill was added, and the warning was invisible. With Kpa-clawbot#2090's packed columns this made the Kpa-clawbot#1128 "every overflowing .path-hops has a +N pill" E2E fail. The indicator now leads the hops and does not shrink, so the edge clips hops, which the +N pill counts. The +N popover leaves it out of the hop list, as it already did with the pill itself. Locally, against master merged in: Kpa-clawbot#1128 packets layout fails 4/1 without this change and passes 5/0 with it; Kpa-clawbot#1146 11/0, Kpa-clawbot#1128 multi-viewport 15/0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Second CI failure addressed (commits After #2090 merged, the PR's merge ref failed
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2097.
Every 1-byte path prefix on this network is shared. Measured on the live deployment: 254 prefixes cover all 2043 nodes, nine of them on a single prefix, and not one node has a prefix to itself. The packets page printed one of those nine as a certainty.
It could already have said otherwise.
hop-resolver.jsmarks such a hopambiguousand returns its full candidate list;hop-display.jsrenders a warning badge with a count and a candidate popover;renderHop(h, observerId)reads a per-observer cache key. None of it fired, becauseHopResolver.resolve()takes six parameters andpackets.jspassed one.Without
observerId,packetIatais null,nodeInRegion()never runs, no candidate is flaggedregional,globalFallbackstays false, andhop-display.jscomputes a badge count of 0. The whole chain stayed silent while the data said it was a guess.What this changes
Resolution carries the observer.
resolveHops()takes it and writes the per-observer cache key thatrenderHop()has always read and nothing ever wrote.resolveHopsForPackets()groups a multi-packet call by observer so each group is filtered on its own. One site was the bug in miniature: it wrote its result underhopNameCache[k + ':' + pkt.observer_id]after resolving without that observer, so the key promised something the value was not.The observer's position is the anchor.
resolve()has always acceptedobserverLat/observerLonas the anchor at the receiving end: when nothing later in the path is resolved, the observer is the next known position, which is whatpickByAffinityneeds.observerPosition()is the single lookup, used by both resolve paths.This matters more than the IATA route, which turns out to be dead on this network.
nodeInRegion()looks the observer's code up in/api/iata-coords; 0 of 42 observers have their code in that table (ANR BRU GNE HEP KJK LGG MST NRW OBL OSTare all missing, while its 54 entries areAMS,APC,ATLand the like). So the 300 km filter has never excluded anything here. 27 of 42 observers report lat/lon directly, and that works. Filed separately as #2098, since it is a network-wide lookup that resolves nothing and hop resolution may not be its only consumer.Each badge belongs to its pill. The badge is a sibling after the pill, so a path read
A [8] → B [6] → Cwith nothing to say which name the 8 qualified. Pill and badge now render inside onewhite-space: nowrap.hop-group, leaving the separator outside the pair. Only a hop that carries a badge is wrapped; wrapping every hop would change the layout of every path for nothing. That markup predates this work, but these commits are what made it visible — before them the badge essentially never appeared on this page.The list summarises, the detail pane does not. Every 1-byte hop getting its own badge turned a table row into a line of warning triangles.
renderPath()takes{ summary: true }for the three list call sites: names with no per-hop badge, and one indicator reading "N of M hops have more than one candidate". The two detail-pane sites are unchanged and keep a badge per hop, because that is where the candidates actually get read.HopDisplaygainsopts.badge === falsefor it, and the hop keeps itshop-ambiguousclass either way so CSS can still mark it.Scope
Deliberately not included: chaining the next resolved hop to narrow a candidate set to one.
pickByAffinityalready scores on graph edges and would do it; it needs neighbouring context this call does not yet supply, and that belongs in its own change tohop-resolver.js. What is here makes the uncertainty visible and picks a defensible candidate rather than the first in index order.Verification
30 unit tests in
tests/unit/test-issue-2097-hop-ambiguity-badge.js, registered intest-all.sh. Full frontend suite exits 0.Four of them are structural guards over
packets.js, and each was mutation-checked by reverting the change it protects:HopResolver.resolve()call passes the hops alonerenderPathwithoutsummaryThe behavioural test for the anchor runs with
iataCoordsdeliberately empty, which is the live state, and lists the distant candidate first: without an anchor the resolver keeps candidate order and picks it, with one it does not, and the hop stays reported as ambiguous with all candidates listed.Browser validated on a staging deployment of this branch, against live data:
.hop-groupOn the packet that prompted the issue, the first hop now resolves to a repeater 10.6 km from the transmitter instead of one 126 km away, and the second hop resolves to the only candidate that has a recorded neighbour edge matching the next hop in the path.
Note for reviewers
The last commit exists because staging disagreed with the tests. After the anchor landed, the detail pane still picked the distant candidate: it resolves through its own
HopResolver.resolvecall, which had the observer id but a null position. The guard that now fails on exactly that shape is what would have caught it before the deploy.