Repository navigation
port(upstream#1967): keep the selected hash size in Hash Issues links - #44
Merged
Merged
Conversation
Red commit: `5a5ecb6` (local browser: 14 passed, 8 behavior assertion failures before the fix). Hash Issues links now restore `bytes=1|2|3` for the selected control and its matrix/collision data. Missing or malformed values default to one byte. Selector clicks, section/top links, tab-bar changes, filters and theme refreshes retain the chosen view through the existing URL helper. Fixes Kpa-clawbot#1914. - E2E assertion added: `test-issue-1306-collisions-terminology-e2e.js:242`. The existing CI-selected harness passes 23 checks, including distinct nonempty collision rows for each byte size. Its original assertions remain. - Browser verified: `http://127.0.0.1:55634` with the local fixture API, plus reviewed matrix/risk screenshots. Region refresh passed; area coverage skips because the fixture has no areas. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers; URL helpers pass 18. Three independent reviews found no blocking issues; their coverage suggestion is included in `fb482fe`. - Added work parses URL state and updates six links. Rendering and bulk requests are reused; no backend, configuration, dependency or CI-list changes. - A broader smoke run timed out at Live autocomplete (Kpa-clawbot#1110); full-suite success is not established. ## Preflight overrides - The external `run-all.sh` is absent. Corresponding scope, PII, syntax, whitespace and CSS checks passed; no SQL, migration or image changes require those gates. - Red browser evidence is local. Upstream CI execution remains a separate approval gate, as discussed in Kpa-clawbot#1922. (cherry picked from commit eb1d733) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent review flagged that the URL sync ran first inside refreshHashViews, ahead of the button states, titles and both render calls, so a throwing history.replaceState (Safari throttles it; it is unavailable on an opaque origin) would take the whole tab down rather than just the URL update. I could not reproduce that outcome: with a stub that throws for every 'bytes=' URL, both the old and the new ordering render the tab fully with zero page errors, because the existing 'newHash !== location.hash' guard means the deep-link path never calls replaceState at all. So this is not a fix for an observed break — it is the ordering the code should have had either way: the rendered views are the feature, the URL sync is a convenience, and nothing in the render path depends on it. Extracted into syncHashUrl() and called last, with the reasoning in a comment. No test is added: a test asserting a behavioural difference here would be asserting one I could not demonstrate. The existing 23 assertions still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
3 participants
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.
Split out of #25 (commit
0500c0cdthere). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
eb1d733998ac9c2f3254b7a309a67593c78d66d9git cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.public/analytics.js,test-issue-1306-collisions-terminology-e2e.js.Problem
On Analytics → Hash Issues the selected hash size (1/2/3 byte) was not kept: section and ↑top links, tab switches, filter changes and theme refreshes all reset the view to 1 byte.
Change
refreshHashViewswritesbytesinto the URL hash viaURLState.updateHashParams(withreplaceState) and rewrites the section links to carry it. Each render readsbytesback from the URL; missing or invalid values fall back to 1.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5xss-gate-diffchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-linttest-issue-1375-scope-stats-fetch.jsExactly oneapi('/scope-stats'call exists (the fixed loader) — found 2test-issue-1648-m4-emoji-scan.jsmap.js has 1 emoji/misc-icon hit(s):test-a11y-axe-routes-coverage.jsaxe ROUTES missing analytics tabs (issue #1706): areas, foreign-traffic, wardrivingtest-frontend-helpers.jsfavStar returns empty star for non-favorite: The expression evaluated to a falsy value:,favStar returns filled star for favorite: The expression evaluated to a falsy value:Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Local Go server (built from master) on the committed E2E fixture DB (freshened, migrated and seeded exactly like CI), serving this branch's
public/, compared side by side with the same server serving master'spublic/. Analytics → Hash Issues, clicked the 2-byte selector, then dispatchedtheme-refresh. master: URL stays#/analytics?tab=collisions, section links carry nobytes, and after the refresh the view is back to "1-Byte Hash Usage Matrix". this branch: URL becomes…&bytes=2, the three section links carrybytes=2, after the refresh the 2-byte button is still active ("2-Byte Hash Usage Matrix"), and a direct deep link#/analytics?tab=collisions&bytes=3opens with the 3-byte view selected.Not run:
test-issue-1306-collisions-terminology-e2e.js(Playwright) have not run anywhere for this fork.eslint(not installed locally; CI installs it on the fly).-race/tests for modules this PR does not touch (unchanged code, identical to master).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34751735213 on
1a5df04a. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code