feat(gui): the UI explains itself - #50
Merged
Merged
Conversation
This was referenced Aug 21, 2026
Behnam-RK
force-pushed
the
fix/open-minimized-launch-marker
branch
from
August 22, 2026 06:47
fbd9da4 to
6e8a1b5
Compare
Three changes to the same problem: controls that ask the user to already know what they do. Per-key documentation anchors. The (?) beside a setting resolved to one of four section anchors shared by roughly forty keys, so every click landed in the same place. The help renderer now gives each documented key an id on its table row (first occurrence wins, since several keys appear in both the field reference and the presets comparison), and config.docAnchorFor derives each key's anchor from the key rather than restating forty of them by hand. TestEveryTunableDocAnchorResolves accepts row anchors and therefore fails by name when a key loses its row, and TestKeyAnchorSlugMatchesTheRenderer pins the two slug rules together across the package boundary they cannot import across. The advanced table's rows are qualified in full to match every other table. Short action titles with one caption line. Block / Unblock / Switch VPN… / Pause / Guard down / Panic…, and the sentence each title used to carry inline now lives in a single caption line under the row, following the pointer and the keyboard focus and falling back to the posture headline. This matters most for Pause, whose old title was the only place the real-IP warning appeared. The menubar keeps its long titles — a menu has no caption line to delegate to — and so does the guided empty state's panic button. The (?) tooltip is the key's own help text rather than a description of the button, which wasted the one surface that could answer without navigating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Behnam-RK
force-pushed
the
feat/ui-explains-itself
branch
from
August 22, 2026 07:11
142f5ef to
80dc5ba
Compare
…ide it First review round on #50, thirteen findings. The per-key anchor replaced the section anchor instead of joining it, which broke the reader it was not written for. `config schema` prints the anchor for someone who will open docs/usage/config.md on GitHub, and row ids exist only in the HTML tools/helpgen renders — so every CLI user got a fragment resolving nowhere, to fix a granularity problem only the app had. It also cost the app its middle step on version skew: a CLI newer than the bundled help found no row id and fell all the way back to the top of a forty-key reference. Tunable now carries both, the CLI prints the heading anchor, the app prefers the row and falls back to the section, and the Go test checks each field against the anchors that field is allowed to name. The caption line had four defects, all in the direction of saying something untrue. It was one truncated line, so every long caption lost its tail — which is exactly where routineHint appends the password expectation, so Pause read "Will ask for your pass…" and dropped the warning; it wraps to a reserved second line now, the same trade the panic caption already made. The hint was captured at hover-enter and never refreshed, so a daemon going unreachable under a parked pointer left the caption and the tooltip disagreeing — the one thing the wrapper exists to prevent. Hover outranked focus unconditionally, so tabbing with the pointer parked elsewhere moved the focus ring and the Space key while the caption described another button; focus supersedes a parked pointer now, and moving the pointer takes it back. And a control replaced under a stationary pointer never received a hover-exit, so the caption described a button that no longer existed until the mouse moved. The Cancel button stopped distinguishing an automatic redial window from a manual switch window, and hovering it hides the posture headline that carries that distinction — so the caption names which window it closes. The ? tooltip was a duplicate: schemaField and schemaToggle already put the key's help on the control, and schemaToggleWithCaption shows it visibly. It now says what the button does, which is the affordance a pointer user had lost — "opens the documentation" survived only in the accessibility label. Plus stale text: two comments asserting the old design in the functions whose behaviour changed, two checklist titles a tester would file as failures ("Resume now", "Switching VPN…"), and a CHANGELOG entry omitting the window controls' renames. The checklist also gained the three cases this round found — the truncated tail, focus versus a parked pointer, and a caption outliving its control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round on #50, six findings, and the medium is a test I broke last round. TestKeyAnchorSlugMatchesTheRenderer read DocAnchor, which after the split is a *section* anchor and therefore never among the rendered row anchors — so every iteration hit `continue` and the test could not fail, while its doc comment claimed to be the pin against exactly the divergence it had stopped checking. The reviewer demonstrated it by diverging anchorSlug and watching the test pass. Reading DocKeyAnchor instead was not enough, and the negative test is what showed that too: gating on "is this anchor in the bundle" skips precisely the keys that have diverged, since a wrong slug is by definition not among the rendered ones. The derivations are now compared directly — whether the anchor exists is the resolution test's question — and a `checked == 0` guard fails the build if this ever goes inert a third time. Verified by tampering: it now names blockedCountries and pollInterval. The Swift fixture had the slug rule wrong in the same direction: it mapped "." to "-" and asserted key-vpn-endpointrefresh, which the pipeline never emits — Go drops dots, so the real anchor is key-vpnendpointrefresh. Nothing failed because the test only exercises decoding and ordering, but the fixture is the app-side reference for a cross-language contract and it stated that contract wrongly. It now mirrors the Go rule explicitly. Focus clearing hover left the pointer unable to take the caption back without leaving the control and re-entering, because onHover fires only on enter and exit — which is not what "move the pointer and it takes over again" means, and that is the step the checklist asks for. onContinuousHover re-arms on movement. Also: openHelp(docAnchor:) was left with no callers and without the resolution step the new path exists for, so the shorter-looking overload would silently restore landing at the top of the page; a HelpIndex comment still attributed row anchors to docAnchor, which is the collapse schema.go warns against; and the checklist still asserted the ? tooltip shows the key's help, which this PR deliberately moved off that button. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ming it is Third review round on #50, four findings. KeyAnchor's comment promised that prefixing with `key-` made collision with a heading anchor impossible. It does not, and the counterexample is already in the tree: docs/usage/cli.md has a "## Key flags" heading, which slugs to `key-flags` — exactly what a row for a key named `flags` would claim. Verified in-package: Anchor("Key flags") == KeyAnchor("flags"). Two elements with one id is invalid HTML and the browser jumps to whichever came first, so the reader lands on the heading and the contextual link is silently wrong, which is the outcome the comment called impossible. Render now resolves it in the heading's favour — that being the anchor markdown itself generates and the one written links depend on — and *notes* the row rather than skipping it quietly, so Unsupported fails the bundle build naming the page. Verified by adding a colliding row: TestBundleBuilds fails with "a key row and a heading both claim the anchor key-flags". Heading anchors are pre-scanned because a heading can appear after the row it collides with, while r.Headings is filled in the same single pass. The resolution test accepted a DocKeyAnchor that matched a *heading*, which defeated its own promise: a key whose row was deleted would still pass whenever some heading slugged to the same fragment — and such headings exist — so the app would deep-link to a heading instead of the test failing by name. It checks key rows only now, while DocAnchor keeps checking headings only. onContinuousHover re-armed on any `.active` phase, and `.active` is also reported when a tracking area is rebuilt. These controls re-measure constantly — a window's countdown retitles "Cancel (m:ss left)" every second, re-placing the row — so a parked pointer could silently reclaim the caption a second after the keyboard took it, defeating "most recent interaction wins" in precisely the scenario the new checklist step describes. It compares the reported location and ignores a report at an unchanged point. Also the comment above the four section anchors still called them a fallback for keys documented in prose and pointed at a function this PR renamed — a description true only of the intermediate design that was reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth review round on #50, nine findings, all low — and the first three are all last round's `onContinuousHover` fix, which is the signal that it was the wrong shape rather than nearly right. The location it reports is in the view's *local* space, so a banner appearing above the action row moves a control under a stationary mouse and reads as movement — silently taking the caption back from the keyboard, the exact thing the guard was added to prevent. `lastHoverPoint` was also never cleared in onDisappear, so the middle slot swapping identity (Pause → Cancel → Pause) had the returning control compare against a point captured before the swap and treat it as movement. And it wrote `@State` on every mouse-move event over any action control, invalidating the whole Overview body — including ActionRow's re-measure of five controls — at pointer-move frequency, for a value never read while rendering. So the re-arm is gone. Focus supersedes a parked pointer, and the pointer takes the caption back by *entering* a control, the same one or another. Every version of "the mouse moved a little" read a proxy for movement rather than movement: `.active` fires when a tracking area is merely established, and these controls re-measure every second because a window's countdown retitles them. Jiggling inside the control you are already on aims at nothing new, so nothing is lost — and the checklist now says re-entering is what hands it back. Shortening the titles also cost VoiceOver the explanation, since the caption carrying it is accessibilityHidden to avoid being read twice — leaving "Cancel" with no way to know which window it closes. Each control now carries its sentence as an accessibility hint, which is where a consequence belongs. The heading pre-scan added last round accepted any trimmed line starting with #, so `#nospace`, seven hashes, and a shell comment inside a fenced block all registered as phantom headings — and one that slugged onto a real row's anchor would refuse that row and fail the whole app build naming a heading nobody can find. It goes through headingRe outside code fences now, verified against a fenced "# Key vpn.redialWindow". Also: a dead `anchors` map in the test, which existed only to be passed back to the check whose laxity was just removed; three leftover doc lines describing the deleted openHelp overload's behaviour above the new one; HelpBundle.bundled() re-read and fully decoded on every ? click, now decoded once and kept; and a docdrift comment describing the advanced table as using bare field names, which this PR fully qualified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifth review round on #50, five findings, and the medium is last round's own fix leaking through an edge the comment claimed it covered. The heading pre-scan detected fences on the trimmed line but headings on the raw one, while the main pass matches the trimmed form — and this renderer honours markdown's up-to-three leading spaces. So " ## Key flags" rendered as a heading the pre-scan never saw, the colliding row was claimed anyway, and the build stayed green with two elements sharing one id: exactly the failure the guard exists to turn into a named error, and the comment asserted the two passes agreed. Confirmed by probe before fixing, and pinned by a test that fails if the indented line ever stops rendering as a heading, so it cannot quietly stop testing anything. claimKey's comment promised a key anchors to "its definition". It implements "the first row on the page wins", which lands on the definition only because config.md is ordered that way — an assumption about the document that the function cannot check, and that nothing noticed: a duplicate is refused silently, because refusing loudly would fail the build on every legitimately repeated key, and the resolution test passes as long as *some* row carries the anchor. Move the Presets comparison above the field tables and every ? for those keys would deep-link to a presets row. The comment now says what the rule actually is, and a new test asserts each key's first row falls under one of the definition headings — verified to fail, naming the key and the section it would land in. Also three pieces of misplaced prose. The four-paragraph rationale for openHelp(preferring:) had ended up documenting the cache property inserted between it and the function, leaving the function undocumented and the property's own line stranded after it. Two paragraphs inside onChange(of: focusedAction) both justified the same single statement, the first written for the mechanism removed last round. And the checklist listed Panic… among the row's captioned titles, where hovering it changes nothing by design — a tester following it would have recorded a false failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixth review round on #50, two findings, both in what I added. claimedKeys was a set, so claimKey could not tell "the same key documented in two tables" — benign, and the reason the refusal is deliberately silent — from "two different keys whose slugs collide", which is a silently wrong deep link. Anchor lowercases and drops dots, so a rename changing only case (vpn.armAtBoot / vpn.armatboot) or removing a dot (vpn.pauseMax / vpnpauseMax) produces one; hyphens survive, so the pair I first reached for was not actually a collision, which the test told me before the comment did. Since the "Renamed keys" table precedes the definition tables and lists the OLD name, such a rename would hand the anchor to the old row and send every ? for the live key there, with a green build. It is now anchor -> owning key, and a different owner is reported the way a heading collision already was. Both directions are pinned: the collision is reported, a legitimately repeated key still is not. The ordering test added last round used a looser row rule than the renderer: first-cell code span rather than a *lone* code span. So a row written "| `vpn.redialWindow` (renamed) | … |" — the natural way to annotate that table — would have been counted as the key's first row and failed the build even though claimKey had anchored the definition correctly: a spurious failure on the one test that exists to catch a silent one. It now mirrors help.rowKey, and it reads the page out of DocKeyAnchor rather than hardcoding config.md, so a tunable documented on another bundled page is checked against that page instead of reported as having no row. Verified both ways — fails on a reordered reference, stays quiet on an annotated cell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventh review round on #50, five findings, and the first removes a whole class rather than an instance. rowKey fired on any table row opening with a lone code span, on every bundled page — so concepts/modes.md minted key-1006410 and key-fc007 from its private-range table and usage/cli.md minted key-panic and key-run from its subcommand tables, contradicting Rendered.Keys' own description ("one per documented config key"). Verified by dumping what the renderer actually emits. The cost was not untidiness: the collision guard aborts the whole app build, and cli.md already has a "## Key flags" heading, so adding a row named `flags` to one of its flag tables would have failed `task gui:build` complaining about a key row on a page with no config keys in it. A table now defines keys only if its first column is headed "Field". That also settles definition-versus-summary structurally, which two earlier rounds had settled by argument: config.md heads its four defining tables "Field" and its presets and retired tables "Key", so a summary row can no longer claim an anchor however early it appears. The ordering test stays as a second line — two Field-headed tables naming one key is the case the renderer still cannot settle — and its doc comment no longer claims to be the only guard. Verified both ways: a second Field table moved above Fields fails it by name; a Key-headed table moved there does not. That test was also re-implementing the renderer's row rule and getting it wrong again: it counted any pipe-prefixed line, with no separator row and no header check, so a pseudo-table or prose line above the definitions would have failed the build over a row claimKey ignores. It now applies the same two conditions renderTable does. Two caption fixes. The resting text was PostureUI.humanPosture, which is exactly the headline the hero renders in title2 a hundred points above — so in the normal state, pointer off the row, the pane said "Traffic cut" twice; it is a prompt now. And Block/Unblock described actions a greyed-out button will not perform, which mattered little as a tooltip and matters now that it is the row's primary visible explanation; they say why they are disabled, as the Pause branch already did. On the advanced table's prose I did not expand the bare names: the table is what was requalified, its anchors derive from that, and qualifying eight keys mid- sentence hurts the paragraph. The shorthand is now stated explicitly instead, so it reads as deliberate rather than as drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… once Eighth review round on #50, five findings. Changing the resting caption to a prompt last round left four places claiming it falls back to the posture headline: cli.md, the CHANGELOG, ActionCaption's own API doc, and the manual checklist — where it matters most, since an operator moving the pointer off the row would see the prompt and mark a passing build as failed. All four now say what ships, and the checklist also picks up the disabled-button wording added last round. switchMenu wrapped each of its two shapes in its own `captioned("switch", …)`. state.profiles arrives asynchronously, so the Button→Menu swap happens after launch: the outgoing branch's onDisappear cleared the caption state for an id the incoming branch owns, and the incoming one gets no hover-enter under a stationary pointer — so pointing at "Switch VPN…" while profiles loaded dropped the caption to the resting prompt until the mouse moved, and did the same to a keyboard user's hint. One wrapper around both shapes means the modifiers never disappear, only their content changes. docKeyAnchorFor derived the row's page from the key's *section* anchor, coupling two things that need not agree: point one key's DocAnchor at a section on another page and its row anchor named that page too, so the resolution test failed claiming nothing there carries the anchor — when the key does have a row, in config.md. The only escape was to declare the key documented in prose, which is the wrong statement about it. The row page is now named once, where the rows actually are; verified by pointing a key's section at concepts/modes.md and watching the row anchor stay put. Also the menubar item list in cli.md still advertised Block now/Unblock, which AppDelegate explicitly does not add and says why. Pre-existing, but in the hunk this PR edits, and it now lists what the menu really contains — including Pause and hold-the-line, which it had omitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ninth review round on #50, five findings, all low. Focus outranking a parked pointer was implemented by setting hoveredAction to nil, which throws the pointer's position away rather than deprioritising it. Tab into the row and back out to anything outside it and both inputs were empty, so the caption fell to the resting prompt while the pointer was physically on a button, until it moved off and back on. ActionCaption now takes which input the user reached for last and orders the two candidates by it — which satisfies both directions at once, where clearing could only ever satisfy one. Three tests pin it, including the parked-pointer-survives-blur case that was broken. Two leftover parameters. actionCaption still took the Snapshot it stopped reading when the resting text became a prompt, implying the caption is snapshot-derived when it is not; Swift warns about neither. And docKeyAnchorFor took the section anchor only to ignore it — the argument the comment right above it explains must not be consulted, which left the next edit one `_` away from reintroducing the coupling that comment exists to prevent. Two comment blocks had been spliced by successive edits: the claimKey paragraph ran straight into an orphaned "anchor -> the key that claimed it" with no declaration between them, and the headingAnchors block ended in a dangling fragment. Both rewritten as one argument each — and the split TestKeyRowsAnchorToTheirDefinitionSection is greppable again, which is the point of naming a test in a comment. The rewrite also removed two false claims I had left in them. The collision example cited vpn.pause-max, which does not collide (hyphens survive Anchor; only dots are dropped) — the real pairs are a case-only rename and one that removes a dot. And both that comment and the collision test still described the "Renamed keys" table stealing an anchor by appearing first, which last round's header gate made impossible: that table is headed "Old name", so it can never claim one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tenth review round on #50 found no correctness defect in the shipped code — the first clean pass on this branch. Two documentation findings, both mine. The manual step verifying that a hovered caption tracks a live hint asked the tester to stop the daemon. That cannot work: `state.isLive` goes false, Overview renders its guided "stopped" layout, and the action row and its caption line are gone before anything could be observed. `routineHint` keys on `controlIsReachable`, which is the control socket rather than the posture, so the socket has to be the thing that changes — `control.enabled=false` and a restart. As written this was the only coverage for that fix and would have been silently skipped by anyone following it. The README still advertised Block now / Unblock as menubar items, which this PR's own cli.md hunk now states they deliberately are not, and which AppDelegate has never added. Doubly stale, since the Overview button is "Block" now. It lists what the dropdown actually has, and says where Block went and why. Also softened the new cache comment rather than letting it overstate: keeping one decoded HelpBundle is right for resolving a target, but HelpView builds its own in a @State initialiser, so this is not the app's only decode and the comment should not imply it is. Sharing the two is worth doing and is not this change's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleventh review round on #50, two findings, both low, and the second is the cost of round eight's fix. Wrapping switchMenu once — so the caption modifiers survive the Button→Menu swap when profiles arrive — put `.focused($focusedAction, equals:)` on the if/else rather than on the control inside it. `.focused(_:equals:)` binds focus for a focusable *view*, and a _ConditionalContent wrapper is not one. If it is a no-op there, tabbing onto "Switch VPN…" leaves focusedAction nil, so the caption shows the resting prompt for the single control in the row a keyboard user is most likely to reach — while the checklist asks the tester to confirm focus drives the caption across the whole row. I could not confirm on-host either way, so rather than depend on whether it happens to work, the binding now goes on the concrete Menu and Button inside each branch, and `captioned` skips its own for that caller. The skip is a ViewModifier rather than an `if` in the builder, so it does not change the view's identity — which is exactly what wrapping once was protecting. The id is one constant shared by the wrapper and both branches so they cannot drift. Also cli.md still listed Panic… among the row's captioned titles. testing.md was corrected six rounds ago to say the opposite — that hovering Panic changes nothing, correctly, since it sits below the row with its own fixed caption — so two docs disagreed about one behaviour and a tester following cli.md would file the correct behaviour as a bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing it Twelfth review round on #50, three findings, all in what I added. The ordering test matched its header with a literal regex on the raw cell, while renderTable strips inline markup and case-folds. So `| **Field** |`, `` | `Field` | `` or `| FIELD |` make the *renderer* mint row anchors from a table this test stops treating as a key table — the guarantee it exists to pin silently stops covering it, and the checked == 0 fatal does not help because it fires only when every key falls out, not a subset. It now applies the same rule; verified against a **Field**-headed table, which the old pattern skipped in silence. Copied rather than called, deliberately: internal/help's own tests import this package, so importing help from here would close an import cycle. The comment says so, since a copied rule is exactly the thing that drifts. The caption reserved two lines, which was enough at a comfortable width and not at a narrow one — the pane resizes, ActionRow is built to wrap, and the longest caption is ~137 characters, so the ellipsis landed back on the password clause precisely when the window was small. That clause is what the checklist requires to be readable, so it reserves three now, and the checklist asks for the narrow case explicitly. And FocusBinding's comment claimed a modifier avoids the identity change an inline `if` would cause. It does not — `if let` in a ViewModifier body produces _ConditionalContent either way. It is safe only because `focusable:` is a compile-time constant at each call site, which is the actual constraint to keep: make it state-dependent and the branch swap destroys the wrapper's onHover and onDisappear state, which is the caption loss switchMenu was restructured to fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
My own review pass rather than a reviewer agent's, aimed at the two defect classes this branch has actually produced: claims that no longer match the code, and guards that cannot fail. The app decodes `config schema --json` by field name, and `docKeyAnchor` is read with decodeIfPresent so an older CLI degrades to the section anchor. That tolerance is what would make a Go-side rename invisible: every key silently loses its row anchor, the ? goes back to landing on section headings, and nothing fails — least of all the Swift test, which builds its fixture by hand. Now pinned, and verified by renaming each tag in turn: both are caught, 43 keys each. Written against schemaEntry rather than config.Tunable, because that is the wire shape — the CLI wrapper embeds the Tunable and adds `preset`. My first attempt marshalled the Tunable alone, which both missed a name the app decodes and failed for a reason that was not a defect; running it is what showed that. Three claims corrected. The CHANGELOG still said the caption "wraps to a second line", which stopped being true when it went to three reserved lines; it now also records the most-recent-input rule and the VoiceOver hint, neither of which it mentioned. actionCaption's doc still called itself one line high. And captioned's doc comment had been spliced again by the focusable change, running one paragraph into the next without a break. The checklist gains the case round nine's fix was for and the list never asked for: with the pointer parked on a button, Tab *out* of the row and the caption must return to describing what the pointer is on, rather than falling to the resting prompt. That was the bug — focus outranks a parked pointer, it does not erase where the pointer is — and nothing on the list would have caught it coming back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A parked pointer could take the caption back from the keyboard about a second after tabbing. `aim = .pointer` was set on every `onHover(true)`, and that fires when a tracking area is re-established under a stationary mouse — which these controls cause constantly, since a window's countdown retitles "Cancel (m:ss left)" every second. So the focus ring and the Space key sat on one button while the caption described another, breaking exactly what ActionCaptionTests.theKeyboardWinsAfterTabbing and the checklist promise. It is the same layout churn the movement re-arm was deleted for, arriving through the other door; `aim` now moves only when this control was not already the hovered one. The wire test I added an hour ago claimed to cover "every name in ConfigSchema.swift's CodingKeys" and omitted three, all omitempty. The consequential one is restartReason: rename that tag and appliesLive — which is `(restartReason ?? "").isEmpty` — returns true for every key, so Settings tells the user a restart-required key applies live, and nothing fails. The omitempty names are now asserted per tunable that has a value, both directions, with a guard that fails if any of them stops being carried at all. Verified: 21 keys catch a restartReason rename. Two manual steps could not be performed as written. Opening a switch window "from the menubar" with the pointer parked on Pause requires moving the pointer to reach the menubar, which fires the hover-exit that clears the state the step exists to observe — it now says to trigger it from an already-focused terminal, and adds the countdown-tick case the bug above would have failed. And the password-clause step implied waiting was enough; refreshServiceState() is not on the 1-second timer, which polls the state file and repaints only, so it takes an action sequence. The comment in the code claimed the same wrong thing and now says where controlIsReachable actually changes. Also dropped the menubar half of the passwordless Block/Unblock check: they are deliberately not in the menubar, which this branch had already corrected in the README and cli.md while leaving the standing checklist asking for a check that cannot pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three rounds of review over the branch, and what they found was not the renderer or the schema wiring but the sentences and the state machine the shortened titles now depend on. A contextual help link undid its own scroll. The anchor is one-shot: it is spent by the navigation it triggered and cleared on the next runloop turn, which re-evaluates the view and asks for the same page with no fragment a moment later. That differed from the loaded URL only by the dropped fragment, so it loaded again and put the reader back at the top of a forty-key reference. HelpNavigation.shouldLoad now decides this in the testable target, keyed on the fragment rather than on equality: an anchored target is an explicit "scroll here" and is always honoured — including a repeat of the anchor already showing, since the reader may have scrolled away and clicking the same search hit again is how you ask — while a bare target naming the page on screen is suppressed. Unblock described three different releases as one. It is offered while the guard holds a downed tunnel and while egress is cut by a standing block, and the daemon's handler branches on AutoArm && !tunnelUp && !standby without looking at why egress was cut — so a full block over a downed tunnel drops to STANDBY exactly like the guard case does. "Resumes monitoring" was the opposite of what happens there. PostureUI.unblockConsequence asks the tunnel question before the posture question, and claims neither manual nor automatic for the block itself: postureName derives "full-block" from blocked alone, so nothing on the wire tells the two apart. The sentences are also short enough to fit the caption's three-line reservation, which the first attempt at this overran by 40% — putting the truncation back on the password clause the reservation exists to protect. The caption followed a button that moved under a resting hand. The guard against a re-established tracking area only caught the same control re-firing; Cancel replacing Pause arrives with a new id, so aim went back to the pointer while the focus ring sat elsewhere. The signal the earlier attempt lacked was named in its own comment — it compared the hover point in local space, which a banner shifting the row makes move under a stationary mouse. NSEvent.mouseLocation does not, so ActionCaption.hoverIsAim asks whether the hand went anywhere. A control inserted already holding focus also seeds its own hint now: onChange does not fire for a view born matching the value it observes, so Cancel kept Pause's sentence. Panic lost its explanation when its title was shortened. It is the one destructive control on the pane and it had neither tooltip nor accessibility hint, only an adjacent label no pointer user and no VoiceOver user reaches from the button. It now carries the sentence both ways, with the visible copy hidden from VoiceOver — the arrangement the action row already used. Two fixtures were also lying about the contracts they exist to pin: the Swift anchor-slug helper still diverged from Go on spaces and non-ASCII digits, and the Snapshot builder wrote an empty tunnel array for nil, so the test for an absent tunnel list exercised the empty one another test already covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pointer-movement guard added last round asked the right question against the wrong reference. It compared the mouse's position to where it was at the previous hover event, and `onHover` fires on enter and exit only — so that reading is the last boundary crossing, not where the hand actually is. Both directions were wrong because of it. A hand that drifted a few points inside a control while reading produced no hover event, so the next control arriving underneath compared against a stale point, read as movement, and took the caption off the button the focus ring was on: the exact failure the guard exists to prevent, surviving it. And a fast flick across the gap between two buttons dispatches the exit and the following enter from one mouse-moved event, so both sample the same location, which read as stillness and silently refused to hand the caption back to the pointer at all. The reference is now the moment the keyboard took over — the mouse's position when focus moved, in screen coordinates — which is the baseline the question was always about: has the hand gone anywhere since? It is cleared once the pointer proves it has, so a later entry that happens to land on the same pixel is not swallowed. Opening the Help pane on a deep link also loaded it twice. Honouring every anchored request, which is what lets a repeat click scroll back to an anchor the reader has scrolled away from, does not distinguish a repeat from the duplicate `NSViewRepresentable` produces by design: `updateNSView` runs immediately after `makeNSView`, in the same turn, while the anchor is still pending because it is cleared asynchronously. The second load cancelled the first mid-flight. The coordinator now records the anchor it served and drops that record on the bare follow-up call, which is the one call that can only arrive after the anchor was cleared — so the same-turn duplicate is refused and a genuine repeat still navigates. And headsAKeyTable, which mirrors the renderer's "Field" header gate, still stripped less than stripInline does: a header cell written as a link would make the renderer anchor that table while this test quietly stopped covering it, with the checked == 0 fatal no help because it only fires when every key falls out at once. Left alone, deliberately: focus arriving on a control that was inserted under the focus ring flips aim to the keyboard without the user touching a key. Unlike its mirror image above, the caption then describes the control that actually holds first responder — which is where the Space key goes, so the caption and the keyboard still agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestVPNHoldsFullBlockOnProbeError asserted a literal seven-call sequence, which made it depend on a race it does not test. fakeMonitor.Once cancels the context while handing out the last result, and the run loop then has both ctx.Done() and a 1ms geo ticker ready at its next select — which picks uniformly among ready cases, so one extra probe cycle is a coin flip whenever processing the previous one took longer than the interval. On a fast machine it never does; under -race on a loaded CI runner it does, and the job failed with one extra "apply-guard apply-fullblock" pair. The comment in fakeMonitor states the assumption that breaks, in as many words: "let the loop process it, then exit on ctx.Done next select". Nothing guarantees that. What the test is for is unchanged and is now stated directly: after the startup guard and the escalation, the calls are whole lift-and-recut pairs ending in cleanup, so a probe error never leaves the guard standing where a full block should be. An extra probe cycle satisfies that as fully as the expected number did; a lifted block fails it at any count, including the sequences that end on a lift, that stop escalating, or that never reach cleanup — each checked against the new assertion before this landed. Not a CHANGELOG entry: no user-visible behavior changes, and the run loop is untouched. The alternative fix, having the loop decline to start a lift-and-probe once the context is cancelled, is a real improvement to shutdown — it would stop momentarily lifting FULL BLOCK on the way out — but it belongs in its own change, not smuggled in under a flaky test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ding A geo tick taken while FULL BLOCK stands is not a passive read. When no provider addresses resolved — so the tunnel-scoped pass cannot be built — probe's fallback path lifts the guard, looks up the exit country, and re-cuts. That is the sanctioned bounded leak, and it is justified by a FULL BLOCK that must be able to observe its way out. None of that justification survives a shutdown. Cleanup is deferred and about to remove every rule, so the reading can change nothing, and the lift paid for it opens egress through the exact forbidden-country exit the block exists for. The loop could take one on the way down: with the context cancelled and a geo tick pending, select had two ready cases and chose between them at random, and the ticker is ready whenever the previous tick's work outran the interval. So the geo tick now returns when the context is done, which is what the ctx.Done() case beside it already does — the change is only that the answer no longer depends on which of the two select picked. It is the same rule the manualBlock and panic-disarm branches state a few lines below: when recovery cannot act on what it learns, it must not pay a lift to learn it. TestShutdownTakesNoProbeTick reaches the state on purpose rather than hoping for it, stalling inside the monitor past the interval so the tick is pending rather than merely possible. Verified in both directions: with the check disabled it failed on the first iteration, reporting the extra apply-guard/apply-fullblock pair. The privileged counterpart is in testing.md, because the Go test can only show the decision — that the rules never leave FULL BLOCK for the teardown needs pfctl. 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
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.
Three changes to one problem: controls that ask the reader to already know what they do.
Per-key documentation anchors
The ? beside a setting already deep-linked, but
internal/config/schema.godefined only four anchors —#fields,#control-block,#vpn-block,#advanced-tunables-vpnadvanced— shared by roughly forty keys. Every click landed in the same four places, which is why it felt like "always opens the configuration doc".internal/help/markdown.gogives each documented key anidon its table row, derived from the row's leading code span (vpn.redialWindow→key-vpnredialwindow). First occurrence wins: eight keys appear in both the field reference and the presets comparison, and duplicate HTML ids are invalid besides.config.docAnchorForderives each key's anchor from the key rather than restating forty of them by hand — the same argument the defaults table is built on.TestEveryTunableDocAnchorResolvesnow accepts row anchors, so a key that loses its documentation row fails the build by name instead of silently degrading into a link to the top of a section.TestKeyAnchorSlugMatchesTheRendererpinsconfig.anchorSlugtohelp.KeyAnchor. The two derive the same fragment from opposite ends of the same link and cannot import each other (internal/helpimportsinternal/configto check its own work), so without this a divergence would break every contextual link at once while both packages' tests still passed.vpn.advanced.redialBudgetrather thanredialBudget), matching the other three tables. This was the only change needed in the docs — the tables are otherwise untouched, which was the point of choosing row anchors over restructuring.53 unique key anchors in
usage/config.md; everyTunableresolves.Short action titles, one caption line
The Overview's row is now Block / Unblock / Switch VPN… / Pause / Guard down / Panic…, with a single caption line beneath it carrying the sentence each title used to hold inline. It follows the pointer and the keyboard focus and falls back to the posture headline; the same string is still the tooltip, so the two cannot drift.
This matters most for Pause, whose old title
"Pause — use my real IP"was the only place that warning appeared. A tooltip alone would not do: it needs a hover and a delay, so the warning would arrive after the decision to click.ActionCaption(in DezhbanCore, with tests) owns the fallback rule, including that the line never goes blank — an empty caption collapses its own height and reflows the row under the pointer.Two places deliberately keep long titles, for the same stated reason: the menubar dropdown (a menu has no caption line to delegate to) and the guided empty states' panic button (no action row there either).
The (?) tooltip
Was
"Read about <label> in the documentation"— a description of the button, wasting the one surface that could answer the question without navigating. It is now the key's own one-line help. The generic phrasing survives as the accessibility label, where it is correct.Verification
go build ./...,go vet ./...,go test ./...— pass.swift test— 197 tests pass (3 new inActionCaptionTests).build-app.shproduces a bundle whoseusage-config.htmlcarries the row ids, anddezhban config schema --jsonemits e.g."docAnchor": "usage/config.md#key-blockedcountries".On-host checks added to docs/contribute/testing.md: that the ? lands on the key's own row (including one under Advanced) and shows the key's help as its tooltip; that hovering Pause puts the real-IP warning in the caption line before the click; that keyboard focus drives the caption too; and that the caption never blanks or changes height.
🤖 Generated with Claude Code