From 80dc5ba23240f1e26a24f16cf0c9f0837b4f19ee Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Fri, 21 Aug 2026 11:45:30 +0330 Subject: [PATCH 01/19] feat(gui): the UI explains itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 20 +++ docs/contribute/testing.md | 21 ++- docs/usage/cli.md | 12 +- docs/usage/config.md | 32 ++-- .../Sources/DezhbanCore/ActionCaption.swift | 34 ++++ gui/macos/Sources/DezhbanCore/HelpIndex.swift | 21 ++- .../Sources/DezhbanMenu/OverviewView.swift | 145 +++++++++++++----- .../Sources/DezhbanMenu/SettingsView.swift | 8 +- .../DezhbanCoreTests/ActionCaptionTests.swift | 25 +++ .../ActionRowPackingTests.swift | 22 ++- internal/config/schema.go | 59 ++++++- internal/help/bundle.go | 8 +- internal/help/help_test.go | 34 ++++ internal/help/markdown.go | 83 +++++++++- 14 files changed, 446 insertions(+), 78 deletions(-) create mode 100644 gui/macos/Sources/DezhbanCore/ActionCaption.swift create mode 100644 gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index a21ca1c..9bb4705 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,26 @@ current as you land changes. ## [Unreleased] +### Changed + +- **Contextual help lands on the key you asked about.** The **?** beside a + setting used to open one of four section anchors shared by every key in that + section; it now scrolls to that key's own row in the configuration reference. + The help renderer gives each documented key a row anchor, and each key's + anchor is derived from the key rather than hand-written, so the two cannot + drift — a key that loses its documentation row now fails the build by name. + The **?** tooltip is the key's own one-line help instead of "Read about X in + the documentation". +- **Shorter action buttons, with one caption line under the row.** The Overview's + controls are now Block / Unblock / Switch VPN… / Pause / Guard down / Panic…, + and the sentence each title used to carry inline moved to a single caption + line beneath the row that follows the pointer and the keyboard focus (falling + back to the posture headline). The same sentence remains the tooltip. The + menubar's items are unchanged: a menu has no caption line to delegate to, and + a guided empty state's panic button keeps its full title for the same reason. +- **The advanced tunables table names its keys in full** (`vpn.advanced.redialBudget` + rather than `redialBudget`), matching every other table in the reference. + ### Fixed - **"Open minimized" now actually decides whether the window opens.** The app diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 68cc56b..180a221 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -999,6 +999,18 @@ task gui:build && open dist/Dezhban.app ### Actions +- [ ] **The action row explains itself before the click.** Titles are short + (Block / Unblock / Switch VPN… / Pause / Guard down / Panic…). Move the + pointer across the row: the caption line beneath it changes to that + control's sentence — in particular, hovering **Pause** must say it uses + your real ISP IP, which is the warning its old title carried. Moving off + the row restores the posture headline. Tab through the row with Full + Keyboard Access on and confirm focus drives the caption too. The line must + never go blank or change height, which would reflow the row under the + pointer. +- [ ] **The degraded states keep their long panic title.** With the CLI missing + or the service not installed, the panic button still reads "Panic — force + unblock…" — there is no caption line there to carry the explanation. - [ ] **Routine ops are passwordless with a live daemon.** Block/Unblock and the switch window complete over the control socket with **no** prompt, from both the menubar and Overview; the switch countdown ticks in both surfaces and @@ -1164,9 +1176,12 @@ traffic, so the check that matters is the one CI cannot run: with egress gone. shows this; the Go tests cannot. - [ ] Built from a checkout whose `docs/` was renamed under it, `task gui:build` **fails** rather than producing an app whose Help pane is missing a page. -- [ ] The **?** beside a Settings field opens Help scrolled to that key's own - heading — not the top of the configuration reference. Spot-check one field - per section, including one under Advanced. +- [ ] The **?** beside a Settings field opens Help scrolled to **that key's own + table row** — not to the section heading it shares with dozens of other + keys, and not to the top of the reference. Spot-check one field per + section, including one under Advanced (whose rows are anchored on the + fully-qualified `vpn.advanced.*` name). Its **tooltip** is the key's own + one-line help, not "Read about X in the documentation". - [ ] Against a CLI too old to know `config schema`, the **?** buttons are absent rather than present and inert. diff --git a/docs/usage/cli.md b/docs/usage/cli.md index cd15350..43838b5 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -622,7 +622,8 @@ Two surfaces, split by urgency: - **Menubar dropdown — the safety/glance core.** One status line (posture, exit country/provider), **Open Dezhban…**, **Block now/Unblock**, the VPN switch - window (Switching VPN… / Cancel with a live countdown) when in VPN mode, + window (Switching VPN… / Cancel VPN switch, with a live countdown) when in VPN + mode, **Panic — force unblock…**, Quit. These are the time-critical and lockout-recovery actions; they never depend on the main window opening. Items enable/disable from the current state. @@ -640,7 +641,14 @@ The main window's sidebar sections: banners above the grid — enforcement problems in red, failing exit checks in orange, first line only with the full text behind a disclosure — while an *expected* unknown exit stays a plain row. With profiles configured, - "Switching VPN…" becomes a menu so a switch window can target one by name. + "Switch VPN…" becomes a menu so a switch window can target one by name. + The action row's titles are deliberately short (Block, Unblock, Switch VPN…, + Pause, Guard down, Panic…) with a **single caption line beneath the row** + carrying the sentence each one used to hold inline — it follows the pointer + and the keyboard focus, and falls back to the posture headline. The same + sentence is also the control's tooltip, so the two can never disagree. The + menubar's items keep their long, self-explaining titles: a menu has no caption + line to delegate to. Degraded states are guided: CLI missing, service not installed, and daemon stopped each render an explanation with the one relevant action inline (Install service… / Guard up). diff --git a/docs/usage/config.md b/docs/usage/config.md index ec2c050..521f9a2 100644 --- a/docs/usage/config.md +++ b/docs/usage/config.md @@ -341,22 +341,22 @@ were on. Turning it off is fine; turning it off by accident is not. | Field | Default | What it controls | |---|---|---| -| `switchWindowMax` | `3m` | Hard cap on any MANUAL switch window (incl. `--for`). | -| `redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `switchWindowMax` so one trigger's budget never truncates the other's. | -| `commandFreshness` | `30s` | How recent a control command must be to be acted on (replay guard). | -| `windowDiscoveryInterval` | `1s` | How often the new server is looked for while a window is open. | -| `tunnelPruneAfter` | `60s` | How long a dynamically-detected tunnel must be gone before it's dropped. | -| `learnedEndpointTTL` | `720h` | How long an unused learned endpoint is kept. | -| `learnedMaxPerProfile` | `16` | Cap on learned endpoints per profile (LRU). | -| `promoteAfterRefreshes` | `3` | Consecutive sightings before a discovered endpoint is learned under normal guard. | -| `redialMinUptime` | `15s` | Backoff seed for the automatic redial window: a tunnel that was up for less than this, with no good exit confirmed during that uptime, still gets a window — but a shorter one for each consecutive fast drop, with a growing wait between them. The first drop after startup is exempt — uptime before the daemon started is unknowable. The wait between windows is cleared by a tunnel that proves itself (a confirmed exit, or an uptime past this value), not only by elapsing. `"0"` disables the backoff, so every qualifying drop gets a full window until the budget runs out. | -| `redialBudget` | `2m` | Total time automatic redial windows may leave the guard relaxed within `redialBudgetWindow`. Debited when a window opens and **credited back when it closes early**, so a redial that succeeded in three seconds costs three seconds — the budget measures the exposure actually taken, not the exposure offered. When it can no longer afford a window the guard simply holds and traffic stays cut. Not disablable (see below). | -| `redialBudgetWindow` | `15m` | The rolling period `redialBudget` is measured over. Each window's cost is returned as it falls out of the period, so a busy link recovers its allowance progressively rather than needing a full quiet stretch. Not disablable. | -| `endpointWarnThreshold` | `256` | Union size at which `doctor` warns about rule-list bloat. | -| `verifyInterval` | `1m` | How often the daemon re-reads the firewall to confirm the rules it believes are installed are still there, re-applying whatever is currently in force — the standing guard, a full block, or an open switch/redial window or pause — the instant they are not. Every other rule change dezhban makes is triggered by something the daemon itself did — this is the only one that notices a ruleset removed from OUTSIDE it (another firewall tool, `pfctl -F all`, `nft flush ruleset`, an OS ruleset reload). `"0"` disables the check, trusting the rules to stay put once applied. On Windows each check is a whole PowerShell invocation (one, not two: the group-existence test and the profile-default cross-check share a single script), so a very short interval has a real cost — the default is deliberately conservative. | -| `livenessRedial` | `false` | Lets a tunnel that reports up but has stopped passing traffic open an automatic redial window — see [ADR-0010](../adr/0010-tunnel-liveness.md). Off by default: an exit that censors the geo lookup produces the identical failure pattern as a genuinely hung tunnel, and turning this on lets that exit trigger a window on a tunnel that was never actually down. The diagnosis itself (`dezhban doctor`, the state file) is always on regardless of this key — only ACTING on it is gated. | -| `windowProtocols` | `[]` | Restrict a switch window to these protocols (e.g. `["udp"]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed protocol. | -| `windowPorts` | `[]` | Restrict a switch window to these ports (e.g. `[51820]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed port set (e.g. WireGuard on 51820). | +| `vpn.advanced.switchWindowMax` | `3m` | Hard cap on any MANUAL switch window (incl. `--for`). | +| `vpn.advanced.redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `switchWindowMax` so one trigger's budget never truncates the other's. | +| `vpn.advanced.commandFreshness` | `30s` | How recent a control command must be to be acted on (replay guard). | +| `vpn.advanced.windowDiscoveryInterval` | `1s` | How often the new server is looked for while a window is open. | +| `vpn.advanced.tunnelPruneAfter` | `60s` | How long a dynamically-detected tunnel must be gone before it's dropped. | +| `vpn.advanced.learnedEndpointTTL` | `720h` | How long an unused learned endpoint is kept. | +| `vpn.advanced.learnedMaxPerProfile` | `16` | Cap on learned endpoints per profile (LRU). | +| `vpn.advanced.promoteAfterRefreshes` | `3` | Consecutive sightings before a discovered endpoint is learned under normal guard. | +| `vpn.advanced.redialMinUptime` | `15s` | Backoff seed for the automatic redial window: a tunnel that was up for less than this, with no good exit confirmed during that uptime, still gets a window — but a shorter one for each consecutive fast drop, with a growing wait between them. The first drop after startup is exempt — uptime before the daemon started is unknowable. The wait between windows is cleared by a tunnel that proves itself (a confirmed exit, or an uptime past this value), not only by elapsing. `"0"` disables the backoff, so every qualifying drop gets a full window until the budget runs out. | +| `vpn.advanced.redialBudget` | `2m` | Total time automatic redial windows may leave the guard relaxed within `redialBudgetWindow`. Debited when a window opens and **credited back when it closes early**, so a redial that succeeded in three seconds costs three seconds — the budget measures the exposure actually taken, not the exposure offered. When it can no longer afford a window the guard simply holds and traffic stays cut. Not disablable (see below). | +| `vpn.advanced.redialBudgetWindow` | `15m` | The rolling period `redialBudget` is measured over. Each window's cost is returned as it falls out of the period, so a busy link recovers its allowance progressively rather than needing a full quiet stretch. Not disablable. | +| `vpn.advanced.endpointWarnThreshold` | `256` | Union size at which `doctor` warns about rule-list bloat. | +| `vpn.advanced.verifyInterval` | `1m` | How often the daemon re-reads the firewall to confirm the rules it believes are installed are still there, re-applying whatever is currently in force — the standing guard, a full block, or an open switch/redial window or pause — the instant they are not. Every other rule change dezhban makes is triggered by something the daemon itself did — this is the only one that notices a ruleset removed from OUTSIDE it (another firewall tool, `pfctl -F all`, `nft flush ruleset`, an OS ruleset reload). `"0"` disables the check, trusting the rules to stay put once applied. On Windows each check is a whole PowerShell invocation (one, not two: the group-existence test and the profile-default cross-check share a single script), so a very short interval has a real cost — the default is deliberately conservative. | +| `vpn.advanced.livenessRedial` | `false` | Lets a tunnel that reports up but has stopped passing traffic open an automatic redial window — see [ADR-0010](../adr/0010-tunnel-liveness.md). Off by default: an exit that censors the geo lookup produces the identical failure pattern as a genuinely hung tunnel, and turning this on lets that exit trigger a window on a tunnel that was never actually down. The diagnosis itself (`dezhban doctor`, the state file) is always on regardless of this key — only ACTING on it is gated. | +| `vpn.advanced.windowProtocols` | `[]` | Restrict a switch window to these protocols (e.g. `["udp"]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed protocol. | +| `vpn.advanced.windowPorts` | `[]` | Restrict a switch window to these ports (e.g. `[51820]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed port set (e.g. WireGuard on 51820). | ## Presets diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift new file mode 100644 index 0000000..502c200 --- /dev/null +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -0,0 +1,34 @@ +import Foundation + +/// What the single description line under the action row says. +/// +/// The row's buttons carry one- or two-word titles ("Pause", "Panic", "Guard +/// down"), so the sentence that used to live inside the title — "Pause — use my +/// real IP" — has to live somewhere the reader sees it *before* they click, not +/// only after a tooltip delay. That somewhere is one caption line beneath the +/// row, showing whatever the pointer or keyboard focus is on. +/// +/// Split out of the view for the same reason `PostureUI` and +/// `ActionRowPacking` are: the fallback rule has a right and a wrong answer, and +/// getting it wrong means a kill switch whose most dangerous button explains +/// itself to nobody. +public enum ActionCaption { + /// The caption to show. + /// + /// `hovered` is the hint of the control the pointer is over, `focused` the + /// hint of the keyboard-focused control. Hover wins when both are present: + /// the pointer is the thing the user is currently aiming, and focus often + /// lingers on whatever was last clicked. + /// + /// With neither, the posture headline stands in. Never empty and never a + /// placeholder like "—": the line must not collapse and reflow the row every + /// time the pointer crosses a gap between two buttons. + public static func text(hovered: String?, focused: String?, fallback: String) -> String { + for candidate in [hovered, focused] { + if let c = candidate, !c.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return c + } + } + return fallback + } +} diff --git a/gui/macos/Sources/DezhbanCore/HelpIndex.swift b/gui/macos/Sources/DezhbanCore/HelpIndex.swift index c8d8a0d..6d9c46f 100644 --- a/gui/macos/Sources/DezhbanCore/HelpIndex.swift +++ b/gui/macos/Sources/DezhbanCore/HelpIndex.swift @@ -27,12 +27,16 @@ public struct HelpPage: Codable, Identifiable, Hashable { /// material — reachable and searchable, but not part of the guided track. public let tutorial: Int public let headings: [HelpHeading] + /// Per-row anchors of the page's reference tables — one per documented + /// config key. Separate from `headings` because they are not headings: they + /// never appear in the sidebar outline, they only resolve deep links. + public let keys: [HelpHeading] /// The page stripped to words, so search runs in the app with no second /// pass over the HTML. public let text: String private enum CodingKeys: String, CodingKey { - case file, source, title, summary, tutorial, headings, text + case file, source, title, summary, tutorial, headings, keys, text } public init(from decoder: Decoder) throws { @@ -45,24 +49,31 @@ public struct HelpPage: Codable, Identifiable, Hashable { // common case: most pages are reference, not tutorial. tutorial = try c.decodeIfPresent(Int.self, forKey: .tutorial) ?? 0 headings = try c.decodeIfPresent([HelpHeading].self, forKey: .headings) ?? [] + // `omitempty` on the Go side, and absent entirely from a help bundle + // built before key anchors existed — so an older bundle decodes to a + // page with no key anchors rather than failing to decode at all. + keys = try c.decodeIfPresent([HelpHeading].self, forKey: .keys) ?? [] text = try c.decodeIfPresent(String.self, forKey: .text) ?? "" } public init(file: String, source: String, title: String, summary: String, - tutorial: Int = 0, headings: [HelpHeading] = [], text: String = "") { + tutorial: Int = 0, headings: [HelpHeading] = [], + keys: [HelpHeading] = [], text: String = "") { self.file = file self.source = source self.title = title self.summary = summary self.tutorial = tutorial self.headings = headings + self.keys = keys self.text = text } - /// True when this page has a heading with that fragment id — what a - /// contextual deep link depends on. + /// True when this page has a heading OR a documented-key row with that + /// fragment id — what a contextual deep link depends on. Keys count because + /// a `Tunable.docAnchor` names one directly ("usage/config.md#key-vpnredialwindow"). public func hasAnchor(_ anchor: String) -> Bool { - headings.contains { $0.anchor == anchor } + headings.contains { $0.anchor == anchor } || keys.contains { $0.anchor == anchor } } } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 60b9789..4f117b4 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -8,6 +8,15 @@ import DezhbanCore struct OverviewView: View { @EnvironmentObject var state: AppState @State private var busy = false + /// The action control the pointer is over, and the sentence it explains. + /// Carried together so a control can only clear the caption it actually put + /// there — see `captioned`. + @State private var hoveredAction: (id: String, hint: String)? + /// Last hint handed over by keyboard focus. Read only while + /// `focusedAction != nil`, so blurring the row falls back to the posture + /// headline instead of stranding the caption on a control nobody is on. + @State private var focusedHint = "" + @FocusState private var focusedAction: String? var body: some View { Group { @@ -58,7 +67,10 @@ struct OverviewView: View { Divider() - actionButtons(s) + VStack(alignment: .leading, spacing: 6) { + actionButtons(s) + actionCaption(s) + } Spacer(minLength: 12) @@ -272,6 +284,11 @@ struct OverviewView: View { private func actionButtons(_ s: Snapshot) -> some View { let blocked = s.blocked let guardHolds = PostureUI.guardHoldsDownedTunnel(s) + // Titles are one or two words; the sentence each one used to carry + // inline ("Pause — use my real IP") is now the caption line below, fed + // by hover and keyboard focus. Same strings still go to `.help`, so the + // tooltip and the caption can never say different things. + // // ActionRow, not HStack: an HStack given less width than its children's // ideal sum compresses every one of them, so at a narrow window all // five labels truncated at once ("Block n…", "Switchin…", "Guard…"). @@ -279,62 +296,114 @@ struct OverviewView: View { // of whatever line it lands on — which is also why the Spacer that used // to sit before it is gone. return ActionRow(trailingCount: 1) { - Button("Block now") { AppActions.routine(["block"], "block") } - .disabled(blocked) - .help(state.routineHint("Cuts all traffic and holds it until you unblock.")) - Button("Unblock") { AppActions.routine(["unblock"], "unblock") } - .disabled(!(blocked || guardHolds)) - .help(state.routineHint("Releases a manual block and resumes monitoring.")) + captioned("block", + state.routineHint("Cuts all traffic and holds it until you unblock.")) { + Button("Block") { AppActions.routine(["block"], "block") } + .disabled(blocked) + } + captioned("unblock", + state.routineHint("Releases a manual block and resumes monitoring.")) { + Button("Unblock") { AppActions.routine(["unblock"], "unblock") } + .disabled(!(blocked || guardHolds)) + } if let sw = s.switch, sw.open, sw.isPause { // `switch --cancel` deliberately refuses to touch a pause (see the // glossary's Pause entry) — `resume` is the only way to end one early. - Button("Resume now" + sw.leftSuffix(asOf: state.now)) { - AppActions.routine(["resume"], "resume the guard") + captioned("resume", + state.routineHint("Ends the pause early and re-arms the guard.")) { + Button("Resume" + sw.leftSuffix(asOf: state.now)) { + AppActions.routine(["resume"], "resume the guard") + } } - .help(state.routineHint("Ends the pause early and re-arms the guard.")) } else if let sw = s.switch, sw.open { - Button("\(sw.isAutoRedial ? "Cancel redial window" : "Cancel VPN switch")" - + sw.leftSuffix(asOf: state.now)) { - AppActions.routine(["switch", "--cancel"], "cancel the switch window") + captioned("cancel-window", + state.routineHint("Closes the window and restores the guard.")) { + Button("Cancel" + sw.leftSuffix(asOf: state.now)) { + AppActions.routine(["switch", "--cancel"], "cancel the switch window") + } } - .help(state.routineHint("Closes the window and restores the guard.")) } else { switchMenu - Button("Pause — use my real IP") { AppActions.routine(["pause"], "pause the guard") } - .disabled(!state.pauseIsEnabled) - .help(state.pauseIsEnabled - ? state.routineHint("Deliberately drops to your real ISP IP, then re-arms the guard automatically.") - : "Disabled — vpn.pauseMax is \"0\" in your config.") + captioned("pause", state.pauseIsEnabled + ? state.routineHint("Uses your real ISP IP instead of the VPN, then re-arms the guard automatically.") + : "Disabled — vpn.pauseMax is \"0\" in your config.") { + Button("Pause") { AppActions.routine(["pause"], "pause the guard") } + .disabled(!state.pauseIsEnabled) + } + } + captioned("stop", + "Stops dezhban. Asks for your password — it can’t stop itself while running.") { + Button("Guard down") { AppActions.privileged(["stop"], "take the guard down") } } - Button("Guard down") { AppActions.privileged(["stop"], "take the guard down") } - .help("Stops dezhban. Asks for your password — it can’t stop itself while running.") } .frame(maxWidth: .infinity, alignment: .leading) } + /// Wraps one action control so pointing at it, or focusing it with the + /// keyboard, puts `hint` in the caption line — and puts the same string in + /// the tooltip, so the two surfaces cannot drift apart. + /// + /// `id` is what distinguishes controls in the hover/focus state; it is never + /// shown. Hover is cleared only by the control that owns the current hint, + /// so the pointer leaving A after it has already entered B does not blank + /// out B's caption. + @ViewBuilder + private func captioned(_ id: String, _ hint: String, + @ViewBuilder content: () -> Content) -> some View { + content() + .help(hint) + .focused($focusedAction, equals: id) + .onHover { inside in + if inside { + hoveredAction = (id, hint) + } else if hoveredAction?.id == id { + hoveredAction = nil + } + } + .onChange(of: focusedAction) { _ in focusedHint = focusedAction == id ? hint : focusedHint } + } + + /// The one line that explains whatever the user is pointing at. Always + /// present and always one line high — a caption that vanished between two + /// buttons would reflow the row under the pointer. + private func actionCaption(_ s: Snapshot) -> some View { + Text(ActionCaption.text(hovered: hoveredAction?.hint, + focused: focusedAction == nil ? nil : focusedHint, + fallback: PostureUI.humanPosture(s))) + .font(.callout) + .foregroundStyle(.secondary) + .lineLimit(1) + .truncationMode(.tail) + .frame(maxWidth: .infinity, alignment: .leading) + .accessibilityHidden(true) + } + /// Opens a switch window, optionally targeted at a known VPN profile so the /// learned endpoint is attributed to it (`switch --no-wait --name `). /// Plain button when there are no profiles to pick from — a menu with a /// single "Any known VPN" entry would just be a worse button. @ViewBuilder private var switchMenu: some View { + let hint = state.routineHint("Briefly relaxes the guard so a new VPN can connect.") if let profiles = state.profiles, !profiles.profiles.isEmpty { - Menu("Switching VPN…") { - Button("Any known VPN") { - AppActions.routine(["switch", "--no-wait"], "open a switch window") - } - Divider() - ForEach(profiles.profiles) { p in - Button(p.name) { - AppActions.routine(["switch", "--no-wait", "--name", p.name], - "open a switch window for \(p.name)") + captioned("switch", hint) { + Menu("Switch VPN…") { + Button("Any known VPN") { + AppActions.routine(["switch", "--no-wait"], "open a switch window") + } + Divider() + ForEach(profiles.profiles) { p in + Button(p.name) { + AppActions.routine(["switch", "--no-wait", "--name", p.name], + "open a switch window for \(p.name)") + } } } } - .help(state.routineHint("Briefly relaxes the guard so a new VPN can connect.")) } else { - Button("Switching VPN…") { AppActions.routine(["switch", "--no-wait"], "open a switch window") } - .help(state.routineHint("Briefly relaxes the guard so a new VPN can connect.")) + captioned("switch", hint) { + Button("Switch VPN…") { AppActions.routine(["switch", "--no-wait"], "open a switch window") } + } } } @@ -346,11 +415,11 @@ struct OverviewView: View { state.showInLogs(title: "dezhban — panic", text: result.output) } } label: { - Label("Panic — force unblock…", systemImage: "exclamationmark.octagon.fill") + Label("Panic…", systemImage: "exclamationmark.octagon.fill") } .tint(.red) .fixedSize() - Text("Removes every dezhban firewall rule, even with dezhban not running.") + Text("Force unblock: removes every dezhban firewall rule, even with dezhban not running.") .font(.callout) .foregroundStyle(.secondary) // Wrap to a second line rather than truncate: this caption is @@ -429,6 +498,12 @@ struct OverviewView: View { .padding(.top, 4) // Panic stays reachable even from a degraded state — stale rules with // no daemon are exactly when the escape hatch matters. + // + // This one keeps its full title while the Overview's own panic + // button is just "Panic…": the rule is that a title may shed its + // explanation only where the explanation has somewhere else to + // live. Here there is no caption line and no action row — a lone + // "Panic…" in a guided empty state would explain itself to nobody. Button("Panic — force unblock…") { guard AppActions.confirmPanic() else { return } AppActions.capturedPrivileged(["panic"]) { result in diff --git a/gui/macos/Sources/DezhbanMenu/SettingsView.swift b/gui/macos/Sources/DezhbanMenu/SettingsView.swift index d3c02fb..2c24977 100644 --- a/gui/macos/Sources/DezhbanMenu/SettingsView.swift +++ b/gui/macos/Sources/DezhbanMenu/SettingsView.swift @@ -758,7 +758,13 @@ struct SettingsView: View { } .buttonStyle(.borderless) .foregroundStyle(.secondary) - .help("Read about \(tunable.label) in the documentation") + // The key's own one-liner, not "Read about X in the documentation": + // a tooltip that only describes the button wastes the one surface + // that could answer the question without navigating anywhere. The + // generic phrasing survives as the accessibility label, where it is + // the right thing — VoiceOver announces the control, and `help` is + // read out separately as its description. + .help(tunable.help) .accessibilityLabel("Documentation for \(tunable.label)") } } diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift new file mode 100644 index 0000000..6a792cc --- /dev/null +++ b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift @@ -0,0 +1,25 @@ +import Testing +@testable import DezhbanCore + +struct ActionCaptionTests { + @Test func hoverWins() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: "release the line", + fallback: "guard is up") == "cut everything") + } + + @Test func focusIsUsedWhenNothingIsHovered() { + #expect(ActionCaption.text(hovered: nil, + focused: "release the line", + fallback: "guard is up") == "release the line") + } + + /// The line must never go blank between two buttons: an empty caption + /// collapses its own height and reflows the row under the pointer. + @Test func fallbackFillsEveryGap() { + #expect(ActionCaption.text(hovered: nil, focused: nil, + fallback: "guard is up") == "guard is up") + #expect(ActionCaption.text(hovered: "", focused: " ", + fallback: "guard is up") == "guard is up") + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift index ff63cc0..6204f0e 100644 --- a/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift @@ -3,9 +3,16 @@ import Testing @testable import DezhbanCore struct ActionRowPackingTests { - /// The Overview's standard-state row, measured at macOS 13pt bordered-button - /// metrics: Block now / Unblock / Switching VPN… (a Menu, so + chevron) / - /// Pause — use my real IP / Guard down. + /// The Overview's standard-state row: Block / Unblock / Switch VPN… (a Menu, + /// so + chevron) / Pause / Guard down. + /// + /// The widths are the ones measured at macOS 13pt bordered-button metrics + /// when those controls still carried their long titles ("Block now", + /// "Pause — use my real IP"). They are deliberately kept: what is under test + /// is the packing arithmetic and the break points these numbers exercise, + /// not the current text metrics. Shrinking them to match today's shorter + /// titles would make every row fit on one line and quietly stop testing the + /// wrapping this type exists for. Treat them as a fixture, not a measurement. private let overview: [CGFloat] = [94, 82, 141, 168, 110] private let spacing: CGFloat = 10 private let gutter: CGFloat = 24 @@ -85,11 +92,12 @@ struct ActionRowPackingTests { } /// The middle slot is state-dependent and can grow a live countdown - /// ("Cancel redial window (12:34 left)"), so the break point moves at - /// runtime. Packing must follow it rather than assume a fixed arity. + /// ("Cancel (12:34 left)"), so the break point moves at runtime. Packing + /// must follow it rather than assume a fixed arity. Same fixture caveat as + /// `overview` above: the widths exercise the break, they do not measure it. @Test func aLongerMiddleSlotMovesTheBreakPoint() { - let short: [CGFloat] = [94, 82, 141, 110] // "Cancel VPN switch" → 471 natural - let long: [CGFloat] = [94, 82, 215, 110] // "Cancel redial window (12:34 left)" → 545 + let short: [CGFloat] = [94, 82, 141, 110] // no countdown → 471 natural + let long: [CGFloat] = [94, 82, 215, 110] // with a live countdown → 545 // One pane width, two different answers — which is the point: nothing // here may be decided from the control COUNT, only from the measurements. #expect(pack(short, 500, pinnedFrom: 3).count == 1) diff --git a/internal/config/schema.go b/internal/config/schema.go index 03557c9..4e178d7 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -1,6 +1,9 @@ package config -import "sort" +import ( + "sort" + "strings" +) // This file exists because a default stated in more than one place is a default // that will drift — and this one already had. Before it, every tunable's default @@ -100,8 +103,9 @@ type Tunable struct { // LiveAppliable reports whether a running daemon adopts this key in place. func (t Tunable) LiveAppliable() bool { return t.RestartReason == "" } -// Doc anchors. Keys are documented by section, so these are the four sections of -// docs/usage/config.md that between them cover every key. +// Doc anchors. Each key resolves to its own row in docs/usage/config.md — see +// docAnchorFor. These four section anchors are the declared fallback for a key +// the reference documents in prose rather than as a table row. const ( anchorFields = "usage/config.md#fields" anchorControl = "usage/config.md#control-block" @@ -475,11 +479,60 @@ func Tunables() []Tunable { for i, t := range tunables { t.Default = defaults[t.Key] t.RestartReason = restartReasonFor(t.Key) + t.DocAnchor = docAnchorFor(t.Key, t.DocAnchor) out[i] = t } return out } +// docAnchorFor upgrades a key's declared section anchor to the anchor of its own +// row in the reference. +// +// The reference documents each key as a table row opening with the key in a code +// span, and the help renderer gives every such row an id (help.KeyAnchor). So a +// contextual help link can land on the key the reader clicked rather than on a +// section heading that four dozen keys share, which is what the four constants +// above delivered on their own. +// +// Derived rather than hand-written, for the same reason defaults are: forty-odd +// anchors restated by hand is forty-odd chances to drift. keysDocumentedInProse +// names the exceptions, and TestEveryTunableDocAnchorResolves fails the build +// naming any key whose derived anchor does not exist — so a key that loses its +// row cannot silently fall back to landing somewhere plausible and wrong. +func docAnchorFor(key, declared string) string { + if keysDocumentedInProse[key] { + return declared + } + page, _, ok := strings.Cut(declared, "#") + if !ok || page == "" { + return declared + } + return page + "#key-" + anchorSlug(key) +} + +// keysDocumentedInProse are the keys docs/usage/config.md covers outside a table +// row, which therefore have no row anchor to land on. They keep their section +// anchor. Adding a row for one of these is an improvement — delete it from here +// when you do. +var keysDocumentedInProse = map[string]bool{} + +// anchorSlug mirrors help.Anchor's rule (GitHub's), applied to a config key. +// Duplicated deliberately rather than imported: internal/help renders the docs +// and would import this package to check its work, so depending on it here would +// close a cycle. TestKeyAnchorSlugMatchesTheRenderer pins the two together. +func anchorSlug(key string) string { + var b strings.Builder + for _, r := range strings.ToLower(key) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-': + b.WriteRune(r) + case r == ' ': + b.WriteByte('-') + } + } + return b.String() +} + // TunableByKey looks one key up. The bool is false for a key that is not // settable, which callers should treat as "unknown key", not "no metadata". func TunableByKey(key string) (Tunable, bool) { diff --git a/internal/help/bundle.go b/internal/help/bundle.go index 4c34bdb..603f052 100644 --- a/internal/help/bundle.go +++ b/internal/help/bundle.go @@ -19,6 +19,11 @@ type IndexEntry struct { Summary string `json:"summary"` Tutorial int `json:"tutorial,omitempty"` Headings []Heading `json:"headings"` + // Keys are the per-row anchors of the page's reference tables, so a + // contextual help link can resolve to one config key rather than to the + // section heading it shares with dozens of others. Omitted for the pages + // that document no keys. + Keys []Heading `json:"keys,omitempty"` // Text is the page stripped to words, so search can run entirely in the app // with no second pass over the HTML. Text string `json:"text"` @@ -57,7 +62,8 @@ func Build(docsDir, outDir string) ([]IndexEntry, error) { index = append(index, IndexEntry{ File: name, Source: page.Source, Title: page.Title, Summary: page.Summary, - Tutorial: page.Tutorial, Headings: rendered.Headings, Text: rendered.Text, + Tutorial: page.Tutorial, Headings: rendered.Headings, Keys: rendered.Keys, + Text: rendered.Text, }) } diff --git a/internal/help/help_test.go b/internal/help/help_test.go index 3505944..dce4f19 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -54,6 +54,11 @@ func TestBundleBuilds(t *testing.T) { // TestEveryTunableDocAnchorResolves ties the settings schema to the bundle. A // contextual help link is a promise that the section exists; a stale anchor // silently lands the reader at the top of a long reference page instead. +// +// Key anchors count alongside headings: a Tunable's DocAnchor names the key's +// own row (config.docAnchorFor), which is the whole point of the per-key grain. +// A key that loses its documentation row fails here by name rather than +// degrading into a link to the top of the section. func TestEveryTunableDocAnchorResolves(t *testing.T) { index := buildInto(t) @@ -63,6 +68,9 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { for _, h := range e.Headings { set[h.Anchor] = true } + for _, k := range e.Keys { + set[k.Anchor] = true + } anchors[e.Source] = set } @@ -83,6 +91,32 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { } } +// TestKeyAnchorSlugMatchesTheRenderer pins config.anchorSlug (which cannot +// import this package — internal/help imports internal/config to check its own +// work) to help.KeyAnchor. The two derive the same fragment id from opposite +// ends of the same link, and a divergence would break every contextual help +// link at once while both packages' own tests still passed. +func TestKeyAnchorSlugMatchesTheRenderer(t *testing.T) { + index := buildInto(t) + + rendered := map[string]bool{} + for _, e := range index { + for _, k := range e.Keys { + rendered[e.Source+"#"+k.Anchor] = true + } + } + for _, tun := range config.Tunables() { + if !rendered[tun.DocAnchor] { + continue // documented in prose; covered by the resolution test above + } + page, frag, _ := strings.Cut(tun.DocAnchor, "#") + if want := KeyAnchor(tun.Key); frag != want { + t.Errorf("%s: schema derived %q, renderer derives %q (page %s)", + tun.Key, frag, want, page) + } + } +} + // TestBundleIsSelfContained — the pane opens when the kill switch has cut every // byte of egress, so a page that reaches for a CDN would render broken at // exactly the moment it is needed. diff --git a/internal/help/markdown.go b/internal/help/markdown.go index 68a359b..a15b59a 100644 --- a/internal/help/markdown.go +++ b/internal/help/markdown.go @@ -39,6 +39,13 @@ type Heading struct { type Rendered struct { HTML string Headings []Heading + // Keys are the per-row anchors emitted for reference tables: one entry per + // table row whose first cell is a lone code span, which is how every field + // table in the docs is written. They are what lets a contextual help link + // land on `vpn.redialWindow` itself rather than on the section heading four + // dozen keys share. Kept separate from Headings because they are not + // headings: they never appear in the sidebar outline. + Keys []Heading // Text is the page with markup stripped, for substring search. Text string // Unsupported names constructs the renderer met and could not represent. @@ -47,7 +54,10 @@ type Rendered struct { } var ( - inlineCode = regexp.MustCompile("`([^`]+)`") + inlineCode = regexp.MustCompile("`([^`]+)`") + // A table cell that is one code span and nothing else — how every field + // table in the docs names the key a row documents. See rowKey. + loneCode = regexp.MustCompile("^`([^`]+)`$") inlineLink = regexp.MustCompile(`\[([^\]]*)\]\(([^)]+)\)`) inlineImage = regexp.MustCompile(`!\[([^\]]*)\]\(([^)]+)\)`) // Non-greedy and NOT [^*]+, so a bold span may contain italics: the docs @@ -118,6 +128,23 @@ func Render(source, markdown string) Rendered { return rendered } + // claimKey hands out the anchor for a key row, the FIRST time that key is + // seen on this page. A key documented in more than one table — several are, + // e.g. `pollInterval` in both the field reference and the presets + // comparison — must anchor to its definition, and duplicate ids are invalid + // HTML besides: a browser jumps to whichever came first, so the choice is + // made here rather than left to chance. + claimedKeys := map[string]bool{} + claimKey := func(key string) (string, bool) { + anchor := KeyAnchor(key) + if claimedKeys[anchor] { + return "", false + } + claimedKeys[anchor] = true + r.Keys = append(r.Keys, Heading{Text: key, Anchor: anchor}) + return anchor, true + } + lines := strings.Split(markdown, "\n") inCode, inList, inQuote := false, false, false listTag := "" @@ -196,7 +223,7 @@ func Render(source, markdown string) Rendered { if strings.HasPrefix(trimmed, "|") && i+1 < len(lines) && isTableSeparator(lines[i+1]) { closeList() closeQuote() - consumed := renderTable(&out, &text, lines[i:], renderInline) + consumed := renderTable(&out, &text, lines[i:], renderInline, claimKey) i += consumed - 1 continue } @@ -366,7 +393,14 @@ func isTableSeparator(line string) bool { } // renderTable emits one table and reports how many lines it consumed. -func renderTable(out *strings.Builder, text *strings.Builder, lines []string, renderInline func(string) string) int { +// +// A row whose first cell is a lone code span — `vpn.redialWindow`, which is how +// every field table in the docs opens a row — gets an `id` on its so a +// contextual help link can land on that key. Every other row is emitted exactly +// as before. claimKey decides the anchor and reports whether this row is the +// one that gets it — see Render, which resolves keys documented in two tables. +func renderTable(out *strings.Builder, text *strings.Builder, lines []string, + renderInline func(string) string, claimKey func(string) (string, bool)) int { header := splitRow(lines[0]) out.WriteString("
\n") for _, c := range header { @@ -382,8 +416,17 @@ func renderTable(out *strings.Builder, text *strings.Builder, lines []string, re if !strings.HasPrefix(s, "|") { break } - out.WriteString("") - for _, c := range splitRow(s) { + cells := splitRow(s) + anchor := "" + if key, ok := rowKey(cells); ok { + anchor, _ = claimKey(key) + } + if anchor != "" { + fmt.Fprintf(out, "", anchor) + } else { + out.WriteString("") + } + for _, c := range cells { out.WriteString("") text.WriteString(stripInline(c) + " ") } @@ -526,6 +569,36 @@ func stripInline(s string) string { return strings.TrimSpace(out) } +// rowKey reports the config key a table row documents, when its first cell is a +// lone code span and nothing else. The "nothing else" is deliberate: a row whose +// first cell is prose that happens to contain code (`"see `vpn.endpoints`"`) is +// not a definition of that key, and anchoring to it would send a help link to +// the wrong row — silently, which is the one failure mode this package refuses. +func rowKey(cells []string) (string, bool) { + if len(cells) == 0 { + return "", false + } + m := loneCode.FindStringSubmatch(strings.TrimSpace(cells[0])) + if m == nil { + return "", false + } + key := strings.TrimSpace(m[1]) + if key == "" { + return "", false + } + return key, true +} + +// KeyAnchor derives the fragment id a documented config key gets. +// +// Prefixed, so a key anchor can never collide with the heading anchor GitHub +// would derive from a heading of the same words — the two namespaces share one +// document and a silent collision would send a help link somewhere plausible +// and wrong. +func KeyAnchor(key string) string { + return "key-" + Anchor(key) +} + // Anchor derives the fragment id a heading gets, matching GitHub's rule so an // anchor written in the docs (and in a Tunable's DocAnchor) resolves in the // rendered page too. From b58808de79cab73bee2c37f021074e7d72fbfe90 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 11:06:17 +0330 Subject: [PATCH 02/19] fix(gui): keep the section anchor the CLI prints, and add the row beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 16 ++++-- docs/contribute/testing.md | 30 ++++++++---- docs/usage/cli.md | 3 +- .../Sources/DezhbanCore/ActionCaption.swift | 6 +++ .../Sources/DezhbanCore/ConfigSchema.swift | 31 +++++++++--- gui/macos/Sources/DezhbanMenu/AppState.swift | 18 +++++++ .../Sources/DezhbanMenu/OverviewView.swift | 49 +++++++++++++++++-- .../Sources/DezhbanMenu/SettingsView.swift | 25 +++++----- .../SettingsFieldsTests.swift | 34 ++++++++++--- internal/config/schema.go | 45 ++++++++++++----- internal/help/help_test.go | 46 ++++++++++++----- 11 files changed, 233 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bb4705..c5e8319 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,13 +20,19 @@ current as you land changes. The help renderer gives each documented key a row anchor, and each key's anchor is derived from the key rather than hand-written, so the two cannot drift — a key that loses its documentation row now fails the build by name. - The **?** tooltip is the key's own one-line help instead of "Read about X in - the documentation". + The **?** button now says what it does on hover ("Open the documentation for + …"), which is the affordance a pointer user was missing — the key's own one-line + help is already on the control beside it, and visible under the toggles that + carry a caption. - **Shorter action buttons, with one caption line under the row.** The Overview's controls are now Block / Unblock / Switch VPN… / Pause / Guard down / Panic…, - and the sentence each title used to carry inline moved to a single caption - line beneath the row that follows the pointer and the keyboard focus (falling - back to the posture headline). The same sentence remains the tooltip. The + with the window controls shortened to match — **Resume (m:ss left)** rather than + "Resume now", and one **Cancel** in place of "Cancel redial window" and "Cancel + VPN switch" (which window it closes is in the caption). The sentence each title + used to carry inline moved to a caption line beneath the row that follows the + pointer and the keyboard focus, falling back to the posture headline. The same + sentence remains the tooltip, and the caption wraps to a second line rather than + truncating, since its tail is where the password expectation is stated. The menubar's items are unchanged: a menu has no caption line to delegate to, and a guided empty state's panic button keeps its full title for the same reason. - **The advanced tunables table names its keys in full** (`vpn.advanced.redialBudget` diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 180a221..b67a01e 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1003,11 +1003,23 @@ task gui:build && open dist/Dezhban.app (Block / Unblock / Switch VPN… / Pause / Guard down / Panic…). Move the pointer across the row: the caption line beneath it changes to that control's sentence — in particular, hovering **Pause** must say it uses - your real ISP IP, which is the warning its old title carried. Moving off - the row restores the posture headline. Tab through the row with Full - Keyboard Access on and confirm focus drives the caption too. The line must - never go blank or change height, which would reflow the row under the - pointer. + your real ISP IP, which is the warning its old title carried — and the whole + sentence must be readable, including the password expectation at its tail, + which is what a single truncated line used to cut. Moving off the row restores + the posture headline. + + Tab through the row with Full Keyboard Access on and confirm focus drives the + caption too, **including with the pointer left resting on a different + button** — focus is meant to supersede a parked pointer; then move the pointer + and confirm it takes over again. The line must never go blank or change + height, which would reflow the row under the pointer. + + Two more, both about a caption outliving what it described. With the pointer + parked on **Pause**, open a switch window from the menubar: the button under + the pointer becomes **Cancel**, and the caption must stop describing Pause + without waiting for the mouse to move. And with the pointer parked on + **Block**, stop the daemon: the caption's password clause must follow the + tooltip's rather than keeping the answer it was given at hover-enter. - [ ] **The degraded states keep their long panic title.** With the CLI missing or the service not installed, the panic button still reads "Panic — force unblock…" — there is no caption line there to carry the explanation. @@ -1016,15 +1028,15 @@ task gui:build && open dist/Dezhban.app the menubar and Overview; the switch countdown ticks in both surfaces and matches. - [ ] **Pause and Resume, from both surfaces.** Pause opens with no password - (`control.allowPauseOps` default true); the app shows "Resume now (m:ss - left)" in place of the switch-window Cancel item, and the countdown agrees - between menubar and Overview. Resuming early re-arms the guard immediately. + (`control.allowPauseOps` default true); Overview shows "Resume (m:ss left)" + and the menubar "Resume now (m:ss left)" in place of the switch-window + Cancel item, and the countdown agrees between the two. Resuming early re-arms the guard immediately. Letting a pause expire re-arms it with no action needed. With `vpn.pauseMax: "0"`, Pause is disabled in both surfaces with a reason ("vpn.pauseMax is \"0\""), not just a silent no-op. - [ ] **Profile picker.** With `configs/dezhban.profiles.json`, Overview's details grid lists every configured profile and marks the one that - matched (`(active)`), matching `dezhban vpn list`; "Switching VPN…" + matched (`(active)`), matching `dezhban vpn list`; "Switch VPN…" becomes a menu with "Any known VPN" plus one item per profile, and picking a profile passes `--name ` (`dezhban vpn list` shows the learned endpoint attributed to it afterward). With no profiles diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 43838b5..16c7566 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -643,7 +643,8 @@ The main window's sidebar sections: *expected* unknown exit stays a plain row. With profiles configured, "Switch VPN…" becomes a menu so a switch window can target one by name. The action row's titles are deliberately short (Block, Unblock, Switch VPN…, - Pause, Guard down, Panic…) with a **single caption line beneath the row** + Pause, Guard down, Panic…, and Resume / Cancel while a window is open) with a + **caption line beneath the row** carrying the sentence each one used to hold inline — it follows the pointer and the keyboard focus, and falls back to the posture headline. The same sentence is also the control's tooltip, so the two can never disagree. The diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift index 502c200..56de545 100644 --- a/gui/macos/Sources/DezhbanCore/ActionCaption.swift +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -20,6 +20,12 @@ public enum ActionCaption { /// the pointer is the thing the user is currently aiming, and focus often /// lingers on whatever was last clicked. /// + /// That order is only safe because the caller clears `hovered` when focus + /// moves — most-recent-interaction wins. Preferring hover *unconditionally* + /// meant a pointer parked on one button outranked the keyboard indefinitely, + /// so tabbing moved the focus ring and the Space key while the caption kept + /// describing something else. + /// /// With neither, the posture headline stands in. Never empty and never a /// placeholder like "—": the line must not collapse and reflow the row every /// time the pointer crosses a gap between two buttons. diff --git a/gui/macos/Sources/DezhbanCore/ConfigSchema.swift b/gui/macos/Sources/DezhbanCore/ConfigSchema.swift index 50ef75c..c75a4c0 100644 --- a/gui/macos/Sources/DezhbanCore/ConfigSchema.swift +++ b/gui/macos/Sources/DezhbanCore/ConfigSchema.swift @@ -37,8 +37,14 @@ public struct ConfigTunable: Codable, Identifiable, Hashable { /// the config to Custom. public let preset: Bool public let help: String - /// Where this key is documented, as "#". + /// The *section* of the documentation covering this key, as "#". + /// A heading anchor, so it resolves in any markdown viewer as well as here. public let docAnchor: String + /// The key's own table row, as "#", or nil for a key documented + /// in prose. Row ids exist only in the rendered help, which is why this is + /// additional to `docAnchor` rather than a replacement — and why an older + /// bundle that predates row ids still lands the reader on the section. + public let docKeyAnchor: String? /// Why a running daemon cannot adopt this key in place; nil when it can. public let restartReason: String? @@ -46,17 +52,28 @@ public struct ConfigTunable: Codable, Identifiable, Hashable { private enum CodingKeys: String, CodingKey { case key, label, kind case defaultValue = "default" - case capKey, unit, disablable, advanced, preset, help, docAnchor, restartReason + case capKey, unit, disablable, advanced, preset, help, docAnchor, docKeyAnchor + case restartReason } /// True when a change to this key takes effect without restarting the daemon. public var appliesLive: Bool { (restartReason ?? "").isEmpty } - /// Where the Help pane should land for this key. Nil only if the schema - /// carried no anchor at all — Go's TestEveryTunableDocAnchorResolves keeps - /// that from shipping, and a control simply offers no link rather than a - /// dead one. - public var docTarget: HelpTarget? { HelpTarget(docAnchor: docAnchor) } + /// Where the Help pane should land for this key, best first. + /// + /// The key's row, then its section. Two entries rather than one because the + /// grain the app wants and the grain that always exists are different things: + /// a row id is present only in help rendered by a build that emits them, so a + /// CLI newer than the bundle would otherwise drop the reader at the top of a + /// forty-key reference. Falling back to the section is the middle step that + /// case needs. + /// + /// Empty only if the schema carried no anchor at all — Go's + /// TestEveryTunableDocAnchorResolves keeps that from shipping, and a control + /// then offers no link rather than a dead one. + public var docTargets: [HelpTarget] { + [docKeyAnchor, docAnchor].compactMap { $0.flatMap(HelpTarget.init(docAnchor:)) } + } /// Placeholder text for a text field: the label, then the real default. /// diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index f18f831..8eb5a69 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -225,6 +225,24 @@ final class AppState: ObservableObject { selectedSection = .help } + /// Opens the first of `targets` whose anchor actually exists in the bundle. + /// + /// Preference order, not alternatives: a key's own row first, then its + /// section. Resolving here rather than in the Help pane is what makes the + /// fallback a *section* rather than the top of the page — the pane's own + /// resolve() drops an unknown fragment and keeps the page, which for a + /// forty-key reference is not a useful place to land. + func openHelp(preferring targets: [HelpTarget]) { + guard !targets.isEmpty else { return } + // Loaded on demand rather than held: this runs on a click, and the pane + // keeps its own copy. `bundled()` returning nil (a bare SwiftPM binary with + // no help payload) leaves the preferred target, which is the right guess + // when nothing can be checked. + let index = HelpBundle.bundled() + helpTarget = targets.first { index?.resolve($0)?.anchor != nil } ?? targets[0] + selectedSection = .help + } + /// Routes a finished transcript into the Logs pane and /// navigates there — the window-side output surface for long actions. func showInLogs(title: String, text: String) { diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 4f117b4..7d0b2ec 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -316,8 +316,15 @@ struct OverviewView: View { } } } else if let sw = s.switch, sw.open { + // Which window, not just "the window". Hovering this button replaces + // the posture headline — the only other place the distinction + // appears — so a caption that says neither leaves the user cancelling + // something unnamed, and the three triggers being distinct is most of + // what the guard's rules rest on. captioned("cancel-window", - state.routineHint("Closes the window and restores the guard.")) { + state.routineHint(sw.isAutoRedial + ? "Closes the automatic redial window and restores the guard." + : "Closes the switch window you opened and restores the guard.")) { Button("Cancel" + sw.leftSuffix(asOf: state.now)) { AppActions.routine(["switch", "--cancel"], "cancel the switch window") } @@ -360,7 +367,35 @@ struct OverviewView: View { hoveredAction = nil } } - .onChange(of: focusedAction) { _ in focusedHint = focusedAction == id ? hint : focusedHint } + .onChange(of: focusedAction) { _ in + guard focusedAction == id else { return } + focusedHint = hint + // Focus supersedes a parked pointer. `.help` is recomputed every + // body pass while the captured hint is not, and hover used to win + // unconditionally — so tabbing with the pointer resting on another + // button moved the focus ring and the Space key while the caption + // went on describing whatever the mouse happened to be over. Most + // recent interaction wins; moving the pointer takes it straight back. + hoveredAction = nil + } + .onChange(of: hint) { newHint in + // The captured string has to track the live one. `state.routineHint` + // flips on `controlIsReachable`, which a poll updates — so a hint + // captured at hover-enter went stale under a stationary pointer and + // the caption said "No password needed" while the tooltip, recomputed + // each pass, said the opposite. That disagreement is the one thing + // this wrapper exists to prevent. + if hoveredAction?.id == id { hoveredAction = (id, newHint) } + if focusedAction == id { focusedHint = newHint } + } + .onDisappear { + // A control can be replaced under a stationary pointer — Pause + // becomes Cancel the moment a window opens — and the removed view + // never receives a hover-exit, so its caption described a button that + // no longer exists until the mouse next moved. + if hoveredAction?.id == id { hoveredAction = nil } + if focusedAction == id { focusedHint = "" } + } } /// The one line that explains whatever the user is pointing at. Always @@ -372,8 +407,14 @@ struct OverviewView: View { fallback: PostureUI.humanPosture(s))) .font(.callout) .foregroundStyle(.secondary) - .lineLimit(1) - .truncationMode(.tail) + // Two lines, always reserved. One line truncated the tail of every long + // caption, and the tail is where `routineHint` appends the password + // expectation — so Pause read "…Will ask for your pass…" and dropped the + // clause warning that a prompt is coming. Reserving the space keeps the + // row from reflowing as the pointer crosses it, which is why this was + // one line to begin with; the panic caption twelve lines below already + // made the same trade for the same reason. + .lineLimit(2, reservesSpace: true) .frame(maxWidth: .infinity, alignment: .leading) .accessibilityHidden(true) } diff --git a/gui/macos/Sources/DezhbanMenu/SettingsView.swift b/gui/macos/Sources/DezhbanMenu/SettingsView.swift index 2c24977..b0c6743 100644 --- a/gui/macos/Sources/DezhbanMenu/SettingsView.swift +++ b/gui/macos/Sources/DezhbanMenu/SettingsView.swift @@ -742,29 +742,30 @@ struct SettingsView: View { /// /// A tooltip has room for one sentence; the reason a setting exists, what it /// costs, and what happens when it is off often needs a page. This is the - /// bridge between the two — and it lands on the *heading*, from the key's - /// own `docAnchor`, so the answer is on screen rather than somewhere in a - /// long reference page. + /// bridge between the two — and it lands on the key's own table *row*, falling + /// back to its section, so the answer is on screen rather than somewhere in a + /// forty-key reference (`ConfigTunable.docTargets`). /// /// Absent when the schema is unavailable (a CLI too old to know /// `config schema`): a button that could only apologise is worse than none. @ViewBuilder private func docLink(_ key: String) -> some View { - if let tunable = schema?[key], !tunable.docAnchor.isEmpty { + if let tunable = schema?[key], !tunable.docTargets.isEmpty { Button { - state.openHelp(docAnchor: tunable.docAnchor) + state.openHelp(preferring: tunable.docTargets) } label: { Image(systemName: "questionmark.circle") } .buttonStyle(.borderless) .foregroundStyle(.secondary) - // The key's own one-liner, not "Read about X in the documentation": - // a tooltip that only describes the button wastes the one surface - // that could answer the question without navigating anywhere. The - // generic phrasing survives as the accessibility label, where it is - // the right thing — VoiceOver announces the control, and `help` is - // read out separately as its description. - .help(tunable.help) + // What the button does, because the key's own one-liner is already + // here: `schemaField`/`schemaToggle` put it on the control itself, and + // `schemaToggleWithCaption` shows it visibly underneath. Putting it on + // this button too hovered the same sentence twice and left nothing + // telling a pointer user that the control navigates — the only "opens + // the documentation" wording was the accessibility label, which a + // mouse never reaches. + .help("Open the documentation for \(tunable.label)") .accessibilityLabel("Documentation for \(tunable.label)") } } diff --git a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift index ce9e132..350a151 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift @@ -13,7 +13,8 @@ private func testSchema() -> ConfigSchema { {"key":"\(key)","label":"\(label)","kind":"\(kind)","default":"\(defaultValue)", \(capKey.map { "\"capKey\":\"\($0)\"," } ?? "") "disablable":\(disablable),"advanced":false,"preset":false, - "help":"help for \(key)","docAnchor":"usage/config.md#fields"} + "help":"help for \(key)","docAnchor":"usage/config.md#fields", + "docKeyAnchor":"usage/config.md#key-\(key.replacingOccurrences(of: ".", with: "-").lowercased())"} """ return try! JSONDecoder().decode(ConfigTunable.self, from: Data(json.utf8)) } @@ -251,13 +252,32 @@ struct ConfigSchemaTests { /// Every control's help link is only as good as the anchor it carries. Go's /// TestEveryTunableDocAnchorResolves proves the anchors exist in the - /// bundled pages; this proves the app turns them into a page and a heading - /// rather than dropping the fragment and landing at the top. - @Test func docAnchorBecomesADeepLink() { + /// bundled pages; this proves the app turns them into a page and a fragment + /// rather than dropping the fragment and landing at the top — and that it + /// offers the key's own row *before* the section, with the section still there + /// as the step to fall back to when a bundle predates row ids. + @Test func docAnchorsBecomeDeepLinksRowFirst() { let schema = testSchema() - let target = try! #require(schema["vpn.endpointRefresh"]?.docTarget) - #expect(target.source == "usage/config.md") - #expect(target.anchor == "fields") + let targets = try! #require(schema["vpn.endpointRefresh"]?.docTargets) + #expect(targets.count == 2) + #expect(targets[0].source == "usage/config.md") + #expect(targets[0].anchor == "key-vpn-endpointrefresh") + #expect(targets[1].anchor == "fields") + } + + /// A key documented in prose has no row to land on, so it offers the section + /// alone rather than a fabricated row id. Absent in the JSON, not empty: an + /// older CLI that does not know the field at all has to behave the same way. + @Test func aKeyWithoutARowOffersOnlyItsSection() { + let json = """ + {"key":"some.key","label":"Some key","kind":"text","default":"", + "disablable":false,"advanced":false,"preset":false, + "help":"","docAnchor":"usage/config.md#fields"} + """ + let tunable = try! JSONDecoder().decode(ConfigTunable.self, from: Data(json.utf8)) + #expect(tunable.docKeyAnchor == nil) + #expect(tunable.docTargets.count == 1) + #expect(tunable.docTargets[0].anchor == "fields") } /// The placeholder states the real default, which is the whole point: the diff --git a/internal/config/schema.go b/internal/config/schema.go index 4e178d7..aa4b46a 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -88,12 +88,25 @@ type Tunable struct { // Help is one line explaining what the key does and what it costs. Help string `json:"help"` - // DocAnchor points at the section of the bundled documentation that covers - // this key, as "#". Section-level rather than per-key because - // that is the granularity docs/usage/config.md actually has; a test asserts - // every anchor resolves in the generated help bundle. + // DocAnchor points at the *section* of the documentation that covers this + // key, as "#". A heading anchor, so it resolves everywhere + // markdown does — on GitHub, in an editor's preview, and in the app's bundled + // help. This is what a CLI surface prints, because it is the only one a reader + // outside the app can follow. DocAnchor string `json:"docAnchor"` + // DocKeyAnchor points at the key's own table row, as "#", or is + // empty for a key documented in prose rather than a row. + // + // Additional to DocAnchor, never a replacement for it. Row ids exist only in + // the HTML tools/helpgen renders; markdown viewers generate heading anchors + // only. Overwriting DocAnchor with this therefore handed every CLI user a + // fragment that resolves nowhere, to fix a granularity problem only the app + // had — and it removed the app's own middle step on version skew, where a CLI + // newer than the bundled help finds no row id and would otherwise fall all the + // way back to the top of a forty-key reference instead of to the section. + DocKeyAnchor string `json:"docKeyAnchor,omitempty"` + // RestartReason is why a running daemon cannot adopt this key in place, or "" // when it can. Derived from restartReasonFor — never restated here, so a key // cannot claim to be live in one table and restart-required in another. @@ -479,14 +492,14 @@ func Tunables() []Tunable { for i, t := range tunables { t.Default = defaults[t.Key] t.RestartReason = restartReasonFor(t.Key) - t.DocAnchor = docAnchorFor(t.Key, t.DocAnchor) + t.DocKeyAnchor = docKeyAnchorFor(t.Key, t.DocAnchor) out[i] = t } return out } -// docAnchorFor upgrades a key's declared section anchor to the anchor of its own -// row in the reference. +// docKeyAnchorFor derives the anchor of a key's own row in the reference, to sit +// alongside its declared section anchor rather than in place of it. // // The reference documents each key as a table row opening with the key in a code // span, and the help renderer gives every such row an id (help.KeyAnchor). So a @@ -494,26 +507,32 @@ func Tunables() []Tunable { // section heading that four dozen keys share, which is what the four constants // above delivered on their own. // +// Alongside, because a row id is not a markdown anchor: it exists only in the HTML +// tools/helpgen renders. Replacing the section anchor with it therefore broke every +// reader outside the app — `config schema` prints the anchor for someone who will +// open the file on GitHub — and cost the app its middle step when a CLI is newer +// than the bundled help. +// // Derived rather than hand-written, for the same reason defaults are: forty-odd // anchors restated by hand is forty-odd chances to drift. keysDocumentedInProse // names the exceptions, and TestEveryTunableDocAnchorResolves fails the build // naming any key whose derived anchor does not exist — so a key that loses its // row cannot silently fall back to landing somewhere plausible and wrong. -func docAnchorFor(key, declared string) string { +func docKeyAnchorFor(key, declared string) string { if keysDocumentedInProse[key] { - return declared + return "" } page, _, ok := strings.Cut(declared, "#") if !ok || page == "" { - return declared + return "" } return page + "#key-" + anchorSlug(key) } // keysDocumentedInProse are the keys docs/usage/config.md covers outside a table -// row, which therefore have no row anchor to land on. They keep their section -// anchor. Adding a row for one of these is an improvement — delete it from here -// when you do. +// row, which therefore have no row anchor to land on. They get an empty +// DocKeyAnchor and are reached through their section anchor alone. Adding a row +// for one of these is an improvement — delete it from here when you do. var keysDocumentedInProse = map[string]bool{} // anchorSlug mirrors help.Anchor's rule (GitHub's), applied to a config key. diff --git a/internal/help/help_test.go b/internal/help/help_test.go index dce4f19..d981a7e 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -55,10 +55,13 @@ func TestBundleBuilds(t *testing.T) { // contextual help link is a promise that the section exists; a stale anchor // silently lands the reader at the top of a long reference page instead. // -// Key anchors count alongside headings: a Tunable's DocAnchor names the key's -// own row (config.docAnchorFor), which is the whole point of the per-key grain. -// A key that loses its documentation row fails here by name rather than -// degrading into a link to the top of the section. +// Both anchors a Tunable carries are checked, because they are load-bearing for +// different readers. DocAnchor is a *heading* anchor and must resolve everywhere +// markdown does, since a CLI prints it for someone reading the file on GitHub. +// DocKeyAnchor names the key's own row (config.docKeyAnchorFor) and exists only in +// the rendered HTML; it is what gives the app's contextual help its per-key grain, +// and a key that loses its documentation row fails here by name rather than +// degrading into a link to the top of a long reference. func TestEveryTunableDocAnchorResolves(t *testing.T) { index := buildInto(t) @@ -74,19 +77,38 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { anchors[e.Source] = set } - for _, tun := range config.Tunables() { - page, frag, found := strings.Cut(tun.DocAnchor, "#") + headings := map[string]map[string]bool{} + for _, e := range index { + set := map[string]bool{} + for _, h := range e.Headings { + set[h.Anchor] = true + } + headings[e.Source] = set + } + + check := func(t *testing.T, key, field, value string, in map[string]map[string]bool) { + t.Helper() + page, frag, found := strings.Cut(value, "#") if !found { - t.Errorf("%s: DocAnchor %q has no fragment", tun.Key, tun.DocAnchor) - continue + t.Errorf("%s: %s %q has no fragment", key, field, value) + return } - set, ok := anchors[page] + set, ok := in[page] if !ok { - t.Errorf("%s: DocAnchor names %q, which is not a bundled page", tun.Key, page) - continue + t.Errorf("%s: %s names %q, which is not a bundled page", key, field, page) + return } if !set[frag] { - t.Errorf("%s: no heading in %s has the anchor %q", tun.Key, page, frag) + t.Errorf("%s: nothing in %s has the anchor %q (%s)", key, page, frag, field) + } + } + + for _, tun := range config.Tunables() { + // A heading, specifically: this is the one a CLI prints for a reader who + // will open the file on GitHub, where row ids do not exist. + check(t, tun.Key, "DocAnchor", tun.DocAnchor, headings) + if tun.DocKeyAnchor != "" { + check(t, tun.Key, "DocKeyAnchor", tun.DocKeyAnchor, anchors) } } } From 4e32060c53c71dacb51c08c7f342e2e6f8b6c1ca Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 11:20:32 +0330 Subject: [PATCH 03/19] test(help): make the slug pin able to fail, twice over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/contribute/testing.md | 13 +++++++-- gui/macos/Sources/DezhbanCore/HelpIndex.swift | 11 ++++++-- gui/macos/Sources/DezhbanMenu/AppState.swift | 10 +++---- .../Sources/DezhbanMenu/OverviewView.swift | 11 ++++++++ .../SettingsFieldsTests.swift | 19 +++++++++++-- internal/help/help_test.go | 27 ++++++++++++------- 6 files changed, 70 insertions(+), 21 deletions(-) diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index b67a01e..ed38c5b 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1192,8 +1192,17 @@ traffic, so the check that matters is the one CI cannot run: with egress gone. table row** — not to the section heading it shares with dozens of other keys, and not to the top of the reference. Spot-check one field per section, including one under Advanced (whose rows are anchored on the - fully-qualified `vpn.advanced.*` name). Its **tooltip** is the key's own - one-line help, not "Read about X in the documentation". + fully-qualified `vpn.advanced.*` name). Its **tooltip** says what the button + does ("Open the documentation for …"): the key's own one-line help is already + on the control beside it, so repeating it here left nothing telling a pointer + user that the button navigates at all. +- [ ] **A CLI newer than the bundled help still lands on the section.** The **?** + offers the key's row first and its section second, so an app bundle predating + row ids must land on the section heading rather than the top of the page. + Reproduce by running the built app against a help bundle from before this + change, or by checking that `dezhban config schema` prints a `docs:` anchor + that resolves as a heading on GitHub — that is the one a CLI reader follows, + and row ids exist only in the app's rendered help. - [ ] Against a CLI too old to know `config schema`, the **?** buttons are absent rather than present and inert. diff --git a/gui/macos/Sources/DezhbanCore/HelpIndex.swift b/gui/macos/Sources/DezhbanCore/HelpIndex.swift index 6d9c46f..c5f81f1 100644 --- a/gui/macos/Sources/DezhbanCore/HelpIndex.swift +++ b/gui/macos/Sources/DezhbanCore/HelpIndex.swift @@ -70,8 +70,15 @@ public struct HelpPage: Codable, Identifiable, Hashable { } /// True when this page has a heading OR a documented-key row with that - /// fragment id — what a contextual deep link depends on. Keys count because - /// a `Tunable.docAnchor` names one directly ("usage/config.md#key-vpnredialwindow"). + /// fragment id — what a contextual deep link depends on. Keys count because a + /// `ConfigTunable.docKeyAnchor` names one directly + /// ("usage/config.md#key-vpnredialwindow"). + /// + /// Not `docAnchor`: that is the key's *section*, kept separate on purpose so a + /// CLI can print an anchor resolving in any markdown viewer, and so this index + /// has a step to fall back to when a bundle predates row ids. Collapsing the two + /// is the regression internal/config/schema.go spends a paragraph warning + /// against. public func hasAnchor(_ anchor: String) -> Bool { headings.contains { $0.anchor == anchor } || keys.contains { $0.anchor == anchor } } diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index 8eb5a69..ec0d936 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -220,13 +220,13 @@ final class AppState: ObservableObject { /// way a `Tunable`'s docAnchor writes it ("usage/config.md#fields"). /// A docAnchor that names nothing bundled still opens the pane — better a /// reader lands in the documentation than on a dead control. - func openHelp(docAnchor: String) { - helpTarget = HelpTarget(docAnchor: docAnchor) - selectedSection = .help - } - /// Opens the first of `targets` whose anchor actually exists in the bundle. /// + /// The only entry point, deliberately. A second `openHelp(docAnchor:)` taking a + /// bare string survived this change with no callers, and it skipped the + /// resolution step this one exists for — so picking the shorter-looking overload + /// would have silently restored landing at the top of the page. + /// /// Preference order, not alternatives: a key's own row first, then its /// section. Resolving here rather than in the Help pane is what makes the /// fallback a *section* rather than the top of the page — the pane's own diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 7d0b2ec..0cd181c 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -367,6 +367,17 @@ struct OverviewView: View { hoveredAction = nil } } + // Re-arms hover on the next mouse *movement*, not only on crossing a + // boundary. `onHover` fires on enter and exit alone, so once focus + // cleared the hover state (see below) a pointer already sitting on this + // control could not take the caption back until it left and re-entered — + // which is not what "move the pointer and it takes over again" means, and + // is the step the checklist now asks a tester to perform. + .onContinuousHover { phase in + if case .active = phase, hoveredAction?.id != id { + hoveredAction = (id, hint) + } + } .onChange(of: focusedAction) { _ in guard focusedAction == id else { return } focusedHint = hint diff --git a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift index 350a151..b940b34 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift @@ -5,6 +5,20 @@ import Testing /// Builds a schema good enough to exercise the pane's use of it, without /// shelling out to the CLI. Kinds and labels match what `config schema --json` /// reports for these keys. +/// Mirrors Go's `config.anchorSlug` / `help.Anchor`: lowercase, then keep only +/// letters, digits and hyphens — so a dot is **dropped**, not turned into a hyphen. +/// +/// Spelled out here because the fixture is the app-side reference for a +/// cross-language contract, and it had the rule wrong: it mapped "." to "-" and +/// then asserted `key-vpn-endpointrefresh`, which the pipeline never emits. Nothing +/// failed, because the test only exercised decoding and ordering — but the next +/// person deriving an anchor from this fixture would have got a fragment resolving +/// nowhere. Go's TestKeyAnchorSlugMatchesTheRenderer pins the two derivations that +/// actually ship; this keeps the fixture honest about them. +private func goAnchorSlug(_ key: String) -> String { + key.lowercased().filter { $0.isLowercase && $0.isASCII || $0.isNumber || $0 == "-" } +} + private func testSchema() -> ConfigSchema { func tunable(_ key: String, _ label: String, _ kind: String, defaultValue: String = "", capKey: String? = nil, @@ -14,7 +28,7 @@ private func testSchema() -> ConfigSchema { \(capKey.map { "\"capKey\":\"\($0)\"," } ?? "") "disablable":\(disablable),"advanced":false,"preset":false, "help":"help for \(key)","docAnchor":"usage/config.md#fields", - "docKeyAnchor":"usage/config.md#key-\(key.replacingOccurrences(of: ".", with: "-").lowercased())"} + "docKeyAnchor":"usage/config.md#key-\(goAnchorSlug(key))"} """ return try! JSONDecoder().decode(ConfigTunable.self, from: Data(json.utf8)) } @@ -261,7 +275,8 @@ struct ConfigSchemaTests { let targets = try! #require(schema["vpn.endpointRefresh"]?.docTargets) #expect(targets.count == 2) #expect(targets[0].source == "usage/config.md") - #expect(targets[0].anchor == "key-vpn-endpointrefresh") + // Dots dropped, not hyphenated — see `goAnchorSlug`. + #expect(targets[0].anchor == "key-vpnendpointrefresh") #expect(targets[1].anchor == "fields") } diff --git a/internal/help/help_test.go b/internal/help/help_test.go index d981a7e..098f0ba 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -119,23 +119,30 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { // ends of the same link, and a divergence would break every contextual help // link at once while both packages' own tests still passed. func TestKeyAnchorSlugMatchesTheRenderer(t *testing.T) { - index := buildInto(t) - - rendered := map[string]bool{} - for _, e := range index { - for _, k := range e.Keys { - rendered[e.Source+"#"+k.Anchor] = true - } - } + checked := 0 for _, tun := range config.Tunables() { - if !rendered[tun.DocAnchor] { + // DocKeyAnchor, not DocAnchor: the latter is a *section* anchor, so reading it + // here made every iteration skip and the test could not fail — while its doc + // comment claimed to be the pin against exactly this divergence. + if tun.DocKeyAnchor == "" { continue // documented in prose; covered by the resolution test above } - page, frag, _ := strings.Cut(tun.DocAnchor, "#") + // Compared directly, with no "is it in the bundle" gate. Such a gate is the + // second way this went inert: a slug that has *diverged* is by definition not + // among the rendered anchors, so gating on presence skipped precisely the + // keys it was meant to catch. Whether the anchor exists is the resolution + // test's question; whether the two derivations agree is this one's. + page, frag, _ := strings.Cut(tun.DocKeyAnchor, "#") if want := KeyAnchor(tun.Key); frag != want { t.Errorf("%s: schema derived %q, renderer derives %q (page %s)", tun.Key, frag, want, page) } + checked++ + } + // A test that silently checks nothing is worse than no test: this one already + // went inert once, when the field it reads stopped being the key anchor. + if checked == 0 { + t.Fatal("compared no anchors — the schema and the renderer are no longer being pinned together") } } From 8431cbc3164fcf763903ef91b08836583def998f Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 11:39:35 +0330 Subject: [PATCH 04/19] fix(help): the key- prefix is not a collision guarantee, so stop claiming it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../Sources/DezhbanMenu/OverviewView.swift | 32 ++++++++++++----- internal/config/schema.go | 13 +++++-- internal/help/help_test.go | 20 ++++++++--- internal/help/markdown.go | 35 ++++++++++++++++--- 4 files changed, 81 insertions(+), 19 deletions(-) diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 0cd181c..9f9fc5e 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -16,6 +16,9 @@ struct OverviewView: View { /// `focusedAction != nil`, so blurring the row falls back to the posture /// headline instead of stranding the caption on a control nobody is on. @State private var focusedHint = "" + /// The last pointer position reported for each control, so `.active` phases + /// caused by a tracking area being rebuilt can be told from real movement. + @State private var lastHoverPoint: [String: CGPoint?] = [:] @FocusState private var focusedAction: String? var body: some View { @@ -367,16 +370,29 @@ struct OverviewView: View { hoveredAction = nil } } - // Re-arms hover on the next mouse *movement*, not only on crossing a - // boundary. `onHover` fires on enter and exit alone, so once focus - // cleared the hover state (see below) a pointer already sitting on this - // control could not take the caption back until it left and re-entered — - // which is not what "move the pointer and it takes over again" means, and - // is the step the checklist now asks a tester to perform. + // Re-arms hover on actual mouse *movement*, not merely on being hovered. + // `onHover` fires on enter and exit alone, so once focus cleared the hover + // state (see below) a pointer already sitting on this control could not + // take the caption back until it left and re-entered — which is not what + // "move the pointer and it takes over again" means, and is the step the + // checklist asks a tester to perform. + // + // The location is compared, because `.active` is also reported when a + // tracking area is (re)established — and these controls re-measure + // constantly: a window's countdown retitles "Cancel (m:ss left)" every + // second, which re-places the row. Acting on any `.active` therefore let + // a parked pointer silently reclaim the caption a second after the + // keyboard took it, defeating "most recent interaction wins" in exactly + // the scenario the checklist step describes. .onContinuousHover { phase in - if case .active = phase, hoveredAction?.id != id { - hoveredAction = (id, hint) + guard case .active(let point) = phase else { + lastHoverPoint[id] = nil + return } + defer { lastHoverPoint[id] = point } + guard let previous = lastHoverPoint[id] ?? nil else { return } + guard previous != point else { return } + if hoveredAction?.id != id { hoveredAction = (id, hint) } } .onChange(of: focusedAction) { _ in guard focusedAction == id else { return } diff --git a/internal/config/schema.go b/internal/config/schema.go index aa4b46a..983ebbd 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -116,9 +116,16 @@ type Tunable struct { // LiveAppliable reports whether a running daemon adopts this key in place. func (t Tunable) LiveAppliable() bool { return t.RestartReason == "" } -// Doc anchors. Each key resolves to its own row in docs/usage/config.md — see -// docAnchorFor. These four section anchors are the declared fallback for a key -// the reference documents in prose rather than as a table row. +// Doc anchors — the *section* of docs/usage/config.md that covers a group of keys, +// and every key's DocAnchor unconditionally. These are heading anchors, so they are +// what `dezhban config schema` prints for a reader who will open the file on GitHub, +// and the app's second choice after the key's own row (docKeyAnchorFor derives that +// separately, and it is additional rather than a replacement). +// +// Not a "fallback for keys documented in prose", which is what this said: that was +// true only of an intermediate design where the row anchor overwrote this one. +// keysDocumentedInProse is empty today, and even if it were not, these would still +// be every key's section. const ( anchorFields = "usage/config.md#fields" anchorControl = "usage/config.md#control-block" diff --git a/internal/help/help_test.go b/internal/help/help_test.go index 098f0ba..64d91ae 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -78,12 +78,18 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { } headings := map[string]map[string]bool{} + keyRows := map[string]map[string]bool{} for _, e := range index { - set := map[string]bool{} + hs := map[string]bool{} for _, h := range e.Headings { - set[h.Anchor] = true + hs[h.Anchor] = true + } + headings[e.Source] = hs + ks := map[string]bool{} + for _, k := range e.Keys { + ks[k.Anchor] = true } - headings[e.Source] = set + keyRows[e.Source] = ks } check := func(t *testing.T, key, field, value string, in map[string]map[string]bool) { @@ -108,7 +114,13 @@ func TestEveryTunableDocAnchorResolves(t *testing.T) { // will open the file on GitHub, where row ids do not exist. check(t, tun.Key, "DocAnchor", tun.DocAnchor, headings) if tun.DocKeyAnchor != "" { - check(t, tun.Key, "DocKeyAnchor", tun.DocKeyAnchor, anchors) + // Key rows only, not rows-or-headings. Accepting either defeated this + // function's own promise: a key whose table row is deleted would still + // pass whenever some heading on the page happened to slug to the same + // `key-…` fragment — and such headings exist ("Key flags" in cli.md) — + // so the app would silently deep-link to a heading instead of failing + // here by name. + check(t, tun.Key, "DocKeyAnchor", tun.DocKeyAnchor, keyRows) } } } diff --git a/internal/help/markdown.go b/internal/help/markdown.go index a15b59a..88eb70d 100644 --- a/internal/help/markdown.go +++ b/internal/help/markdown.go @@ -135,11 +135,36 @@ func Render(source, markdown string) Rendered { // HTML besides: a browser jumps to whichever came first, so the choice is // made here rather than left to chance. claimedKeys := map[string]bool{} + // Heading anchors are collected up front, because a heading can appear *after* + // the row that would collide with it while r.Headings is filled in the same + // single pass below. + // + // The `key-` prefix was assumed to make collision impossible. It does not: a + // heading of "Key flags" — docs/usage/cli.md has one — slugs to exactly + // `key-flags`, which is also what a row for a key named `flags` would claim. + // Two elements with one id is invalid HTML and a browser jumps to whichever came + // first, so the reader lands on the heading and the contextual link is silently + // wrong. The heading wins, being the anchor markdown itself generates and the one + // written links depend on, and the row is *noted* rather than quietly skipped: + // Unsupported fails the bundle build (bundle.go), so this becomes a build error + // naming the page instead of a link that misses. + headingAnchors := map[string]bool{} + for _, line := range strings.Split(markdown, "\n") { + if t := strings.TrimSpace(line); strings.HasPrefix(t, "#") { + if title := strings.TrimSpace(strings.TrimLeft(t, "#")); title != "" { + headingAnchors[Anchor(title)] = true + } + } + } claimKey := func(key string) (string, bool) { anchor := KeyAnchor(key) if claimedKeys[anchor] { return "", false } + if headingAnchors[anchor] { + note("a key row and a heading both claim the anchor " + anchor) + return "", false + } claimedKeys[anchor] = true r.Keys = append(r.Keys, Heading{Text: key, Anchor: anchor}) return anchor, true @@ -591,10 +616,12 @@ func rowKey(cells []string) (string, bool) { // KeyAnchor derives the fragment id a documented config key gets. // -// Prefixed, so a key anchor can never collide with the heading anchor GitHub -// would derive from a heading of the same words — the two namespaces share one -// document and a silent collision would send a help link somewhere plausible -// and wrong. +// Prefixed to keep key rows out of the way of most headings — but the prefix is +// not a guarantee, and it was documented as one. A heading of "Key flags", which +// docs/usage/cli.md has, slugs to `key-flags`, exactly what a row for a key named +// `flags` would claim: the two namespaces share one document and can meet. Render +// resolves that in the heading's favour and reports it, so the collision fails the +// bundle build rather than sending a help link somewhere plausible and wrong. func KeyAnchor(key string) string { return "key-" + Anchor(key) } From 972b81e4c267751a5ff6ec4323910efee19b73ca Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 11:56:54 +0330 Subject: [PATCH 05/19] fix(gui): drop the hover re-arm rather than patch it a third time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/contribute/testing.md | 11 ++++- docs/usage/config.md | 2 +- gui/macos/Sources/DezhbanMenu/AppState.swift | 20 ++++---- .../Sources/DezhbanMenu/OverviewView.swift | 46 ++++++++----------- internal/config/docdrift_test.go | 10 ++-- internal/help/help_test.go | 12 ----- internal/help/markdown.go | 18 +++++++- 7 files changed, 63 insertions(+), 56 deletions(-) diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index ed38c5b..ed81dae 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1010,8 +1010,10 @@ task gui:build && open dist/Dezhban.app Tab through the row with Full Keyboard Access on and confirm focus drives the caption too, **including with the pointer left resting on a different - button** — focus is meant to supersede a parked pointer; then move the pointer - and confirm it takes over again. The line must never go blank or change + button** — focus is meant to supersede a parked pointer. Then move the pointer + onto a control (the same one or another) and confirm it takes over again: + re-entering is what hands it back, deliberately, since jiggling inside the + control you are already on aims at nothing new. The line must never go blank or change height, which would reflow the row under the pointer. Two more, both about a caption outliving what it described. With the pointer @@ -1020,6 +1022,11 @@ task gui:build && open dist/Dezhban.app without waiting for the mouse to move. And with the pointer parked on **Block**, stop the daemon: the caption's password clause must follow the tooltip's rather than keeping the answer it was given at hover-enter. +- [ ] **VoiceOver still hears what each button does.** With VoiceOver on, move + through the action row: each control announces its short title *and* its + consequence as a hint — Cancel in particular must say whether it closes the + automatic redial window or one you opened, since the titles no longer carry + that and the caption line is hidden from VoiceOver to avoid reading it twice. - [ ] **The degraded states keep their long panic title.** With the CLI missing or the service not installed, the panic button still reads "Panic — force unblock…" — there is no caption line there to carry the explanation. diff --git a/docs/usage/config.md b/docs/usage/config.md index 521f9a2..b4589e1 100644 --- a/docs/usage/config.md +++ b/docs/usage/config.md @@ -342,7 +342,7 @@ were on. Turning it off is fine; turning it off by accident is not. | Field | Default | What it controls | |---|---|---| | `vpn.advanced.switchWindowMax` | `3m` | Hard cap on any MANUAL switch window (incl. `--for`). | -| `vpn.advanced.redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `switchWindowMax` so one trigger's budget never truncates the other's. | +| `vpn.advanced.redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `vpn.advanced.switchWindowMax` so one trigger's budget never truncates the other's. | | `vpn.advanced.commandFreshness` | `30s` | How recent a control command must be to be acted on (replay guard). | | `vpn.advanced.windowDiscoveryInterval` | `1s` | How often the new server is looked for while a window is open. | | `vpn.advanced.tunnelPruneAfter` | `60s` | How long a dynamically-detected tunnel must be gone before it's dropped. | diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index ec0d936..6a4e7f0 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -216,10 +216,6 @@ final class AppState: ObservableObject { } } - /// Opens the Help pane at a specific place in the documentation, named the - /// way a `Tunable`'s docAnchor writes it ("usage/config.md#fields"). - /// A docAnchor that names nothing bundled still opens the pane — better a - /// reader lands in the documentation than on a dead control. /// Opens the first of `targets` whose anchor actually exists in the bundle. /// /// The only entry point, deliberately. A second `openHelp(docAnchor:)` taking a @@ -232,13 +228,19 @@ final class AppState: ObservableObject { /// fallback a *section* rather than the top of the page — the pane's own /// resolve() drops an unknown fragment and keeps the page, which for a /// forty-key reference is not a useful place to land. + /// Decoded lazily on the first contextual help click and kept thereafter — see + /// `openHelp(preferring:)`. + private var cachedHelpBundle: HelpBundle? + func openHelp(preferring targets: [HelpTarget]) { guard !targets.isEmpty else { return } - // Loaded on demand rather than held: this runs on a click, and the pane - // keeps its own copy. `bundled()` returning nil (a bare SwiftPM binary with - // no help payload) leaves the preferred target, which is the right guess - // when nothing can be checked. - let index = HelpBundle.bundled() + // Decoded once and kept. `bundled()` reads the payload off disk and JSON- + // decodes every page's full text plus its per-row key list, which is not a + // thing to do synchronously on the main thread on every click of a `?`. + // Nil (a bare SwiftPM binary with no help payload) leaves the preferred + // target, which is the right guess when nothing can be checked. + if cachedHelpBundle == nil { cachedHelpBundle = HelpBundle.bundled() } + let index = cachedHelpBundle helpTarget = targets.first { index?.resolve($0)?.anchor != nil } ?? targets[0] selectedSection = .help } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 9f9fc5e..3121dea 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -16,9 +16,6 @@ struct OverviewView: View { /// `focusedAction != nil`, so blurring the row falls back to the posture /// headline instead of stranding the caption on a control nobody is on. @State private var focusedHint = "" - /// The last pointer position reported for each control, so `.active` phases - /// caused by a tracking area being rebuilt can be told from real movement. - @State private var lastHoverPoint: [String: CGPoint?] = [:] @FocusState private var focusedAction: String? var body: some View { @@ -362,6 +359,14 @@ struct OverviewView: View { @ViewBuilder content: () -> Content) -> some View { content() .help(hint) + // And as an accessibility hint, not only as `help`. Shortening the titles + // moved the explanation into a caption line that is + // `accessibilityHidden` (it would otherwise be read twice), so VoiceOver + // was left with "Cancel", "Pause", "Guard down" and no disambiguation — + // worst for Cancel, which no longer says whether it closes an automatic + // redial window or one the user opened. A hint is where a control's + // consequence belongs, and it is announced after the label. + .accessibilityHint(hint) .focused($focusedAction, equals: id) .onHover { inside in if inside { @@ -370,33 +375,20 @@ struct OverviewView: View { hoveredAction = nil } } - // Re-arms hover on actual mouse *movement*, not merely on being hovered. - // `onHover` fires on enter and exit alone, so once focus cleared the hover - // state (see below) a pointer already sitting on this control could not - // take the caption back until it left and re-entered — which is not what - // "move the pointer and it takes over again" means, and is the step the - // checklist asks a tester to perform. - // - // The location is compared, because `.active` is also reported when a - // tracking area is (re)established — and these controls re-measure - // constantly: a window's countdown retitles "Cancel (m:ss left)" every - // second, which re-places the row. Acting on any `.active` therefore let - // a parked pointer silently reclaim the caption a second after the - // keyboard took it, defeating "most recent interaction wins" in exactly - // the scenario the checklist step describes. - .onContinuousHover { phase in - guard case .active(let point) = phase else { - lastHoverPoint[id] = nil - return - } - defer { lastHoverPoint[id] = point } - guard let previous = lastHoverPoint[id] ?? nil else { return } - guard previous != point else { return } - if hoveredAction?.id != id { hoveredAction = (id, hint) } - } .onChange(of: focusedAction) { _ in guard focusedAction == id else { return } focusedHint = hint + // Focus supersedes a parked pointer, and the pointer takes the caption + // back by *entering* a control — the same one or another. There is + // deliberately no "the mouse moved a little" re-arm: it was tried, and + // every version of it read a proxy for movement rather than movement. + // `onContinuousHover`'s `.active` fires when a tracking area is merely + // established, and these controls re-measure constantly (a window's + // countdown retitles "Cancel (m:ss left)" every second); comparing the + // reported point did not help either, since it is in local space, so a + // banner appearing above the row moves the control under a stationary + // mouse and reads as movement. Jiggling the pointer inside the control + // you are already on aims at nothing new, so nothing is lost. // Focus supersedes a parked pointer. `.help` is recomputed every // body pass while the captured hint is not, and hover used to win // unconditionally — so tabbing with the pointer resting on another diff --git a/internal/config/docdrift_test.go b/internal/config/docdrift_test.go index ee7623e..77cf1ae 100644 --- a/internal/config/docdrift_test.go +++ b/internal/config/docdrift_test.go @@ -169,9 +169,13 @@ func parseDocDefaults(t *testing.T, path string) map[string]string { for rawLine := range strings.SplitSeq(string(data), "\n") { line := strings.TrimSpace(rawLine) if !strings.HasPrefix(line, "|") { - // A heading sets the key prefix for the tables under it: the - // advanced table lists bare field names (`switchWindowMax`) because - // its heading already says which block they live in. + // A heading sets the key prefix for the tables under it. The advanced + // table now names its keys in full (`vpn.advanced.switchWindowMax`), + // which is what lets its rows carry anchors — so this prefix is + // currently unused by every table in the reference. Kept because it is + // the mechanism that makes a bare-name table legal at all, and a + // future block may want one; a table that reverted to bare names + // would otherwise fail the drift check for the wrong reason. if strings.HasPrefix(line, "#") { prefix = "" if strings.Contains(line, "vpn.advanced") { diff --git a/internal/help/help_test.go b/internal/help/help_test.go index 64d91ae..89abbdd 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -65,18 +65,6 @@ func TestBundleBuilds(t *testing.T) { func TestEveryTunableDocAnchorResolves(t *testing.T) { index := buildInto(t) - anchors := map[string]map[string]bool{} - for _, e := range index { - set := map[string]bool{} - for _, h := range e.Headings { - set[h.Anchor] = true - } - for _, k := range e.Keys { - set[k.Anchor] = true - } - anchors[e.Source] = set - } - headings := map[string]map[string]bool{} keyRows := map[string]map[string]bool{} for _, e := range index { diff --git a/internal/help/markdown.go b/internal/help/markdown.go index 88eb70d..8e7ad76 100644 --- a/internal/help/markdown.go +++ b/internal/help/markdown.go @@ -148,10 +148,24 @@ func Render(source, markdown string) Rendered { // written links depend on, and the row is *noted* rather than quietly skipped: // Unsupported fails the bundle build (bundle.go), so this becomes a build error // naming the page instead of a link that misses. + // Through headingRe and outside code fences, so the pre-scan sees exactly what + // the main pass will call a heading. Accepting "any trimmed line starting with + // #" was looser: `#nospace`, seven hashes, and — worst — a shell comment inside a + // fenced block all registered as phantom headings, and one that happened to slug + // to a real row's anchor would refuse that row and fail the whole app build + // naming a heading nobody can find. headingAnchors := map[string]bool{} + scanInCode := false for _, line := range strings.Split(markdown, "\n") { - if t := strings.TrimSpace(line); strings.HasPrefix(t, "#") { - if title := strings.TrimSpace(strings.TrimLeft(t, "#")); title != "" { + if strings.HasPrefix(strings.TrimSpace(line), "```") { + scanInCode = !scanInCode + continue + } + if scanInCode { + continue + } + if m := headingRe.FindStringSubmatch(line); m != nil { + if title := strings.TrimSpace(m[2]); title != "" { headingAnchors[Anchor(title)] = true } } From 016c49b9b29a3cbb8e09085366734757bc907f9f Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 12:11:49 +0330 Subject: [PATCH 06/19] fix(help): an indented heading is still a heading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/contribute/testing.md | 6 +- gui/macos/Sources/DezhbanMenu/AppState.swift | 8 +- .../Sources/DezhbanMenu/OverviewView.swift | 24 +++--- internal/config/keyanchororder_test.go | 84 +++++++++++++++++++ internal/help/indentcollide_test.go | 26 ++++++ internal/help/markdown.go | 29 +++++-- 6 files changed, 151 insertions(+), 26 deletions(-) create mode 100644 internal/config/keyanchororder_test.go create mode 100644 internal/help/indentcollide_test.go diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index ed81dae..c4f69b0 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1000,8 +1000,10 @@ task gui:build && open dist/Dezhban.app ### Actions - [ ] **The action row explains itself before the click.** Titles are short - (Block / Unblock / Switch VPN… / Pause / Guard down / Panic…). Move the - pointer across the row: the caption line beneath it changes to that + (Block / Unblock / Switch VPN… / Pause / Guard down). Panic… is *not* part of + this row — it sits below with its own fixed caption, so hovering it changes + nothing, which is correct rather than a failure. Move the pointer across the + row: the caption line beneath it changes to that control's sentence — in particular, hovering **Pause** must say it uses your real ISP IP, which is the warning its old title carried — and the whole sentence must be readable, including the password expectation at its tail, diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index 6a4e7f0..b419d91 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -216,6 +216,10 @@ final class AppState: ObservableObject { } } + /// Decoded lazily on the first contextual help click and kept thereafter — see + /// `openHelp(preferring:)`. + private var cachedHelpBundle: HelpBundle? + /// Opens the first of `targets` whose anchor actually exists in the bundle. /// /// The only entry point, deliberately. A second `openHelp(docAnchor:)` taking a @@ -228,10 +232,6 @@ final class AppState: ObservableObject { /// fallback a *section* rather than the top of the page — the pane's own /// resolve() drops an unknown fragment and keeps the page, which for a /// forty-key reference is not a useful place to land. - /// Decoded lazily on the first contextual help click and kept thereafter — see - /// `openHelp(preferring:)`. - private var cachedHelpBundle: HelpBundle? - func openHelp(preferring targets: [HelpTarget]) { guard !targets.isEmpty else { return } // Decoded once and kept. `bundled()` reads the payload off disk and JSON- diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 3121dea..a9d97bf 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -378,23 +378,21 @@ struct OverviewView: View { .onChange(of: focusedAction) { _ in guard focusedAction == id else { return } focusedHint = hint - // Focus supersedes a parked pointer, and the pointer takes the caption - // back by *entering* a control — the same one or another. There is - // deliberately no "the mouse moved a little" re-arm: it was tried, and + // Focus supersedes a parked pointer. Hover used to win + // unconditionally, so tabbing with the pointer resting on another + // button moved the focus ring and the Space key while the caption went + // on describing whatever the mouse happened to be over. + // + // The pointer takes it back by *entering* a control — the same one or + // another. There is deliberately no "the mouse moved a little" re-arm: // every version of it read a proxy for movement rather than movement. // `onContinuousHover`'s `.active` fires when a tracking area is merely // established, and these controls re-measure constantly (a window's // countdown retitles "Cancel (m:ss left)" every second); comparing the - // reported point did not help either, since it is in local space, so a - // banner appearing above the row moves the control under a stationary - // mouse and reads as movement. Jiggling the pointer inside the control - // you are already on aims at nothing new, so nothing is lost. - // Focus supersedes a parked pointer. `.help` is recomputed every - // body pass while the captured hint is not, and hover used to win - // unconditionally — so tabbing with the pointer resting on another - // button moved the focus ring and the Space key while the caption - // went on describing whatever the mouse happened to be over. Most - // recent interaction wins; moving the pointer takes it straight back. + // reported point did not help either, being in local space, so a banner + // appearing above the row moved the control under a stationary mouse + // and read as movement. Jiggling inside the control you are already on + // aims at nothing new, so nothing is lost. hoveredAction = nil } .onChange(of: hint) { newHint in diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go new file mode 100644 index 0000000..42f7381 --- /dev/null +++ b/internal/config/keyanchororder_test.go @@ -0,0 +1,84 @@ +package config + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// TestKeyRowsAnchorToTheirDefinitionSection pins the document ordering that +// help.claimKey silently depends on. +// +// claimKey gives a key's row anchor to the FIRST table row naming that key on the +// page — it cannot tell a definition from a summary, and refusing a duplicate +// loudly would fail the build on every legitimately repeated key. So the guarantee +// that a contextual help link lands on the definition rather than on the Presets +// comparison is a property of how config.md is *arranged*. Nothing in the renderer +// or the resolution test notices if that changes: both stay green as long as some +// row carries the anchor. +// +// This is the test that notices. For every settable key, the first table row +// naming it must fall under one of the reference's definition headings. +func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { + path := filepath.Join("..", "..", "docs", "usage", "config.md") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + + // The sections that define keys, as opposed to comparing or retiring them. + definition := map[string]bool{ + "Fields": true, + "`control` block": true, + "`vpn` block": true, + "Advanced tunables (`vpn.advanced`)": true, + } + + firstCell := regexp.MustCompile("^\\|\\s*`([^`]+)`") + heading := regexp.MustCompile(`^#{2,6}\s+(.*)$`) + + seen := map[string]string{} // key -> heading it was first seen under + section := "" + inCode := false + for _, raw := range strings.Split(string(data), "\n") { + line := strings.TrimSpace(raw) + if strings.HasPrefix(line, "```") { + inCode = !inCode + continue + } + if inCode { + continue + } + if m := heading.FindStringSubmatch(line); m != nil { + section = strings.TrimSpace(m[1]) + continue + } + if m := firstCell.FindStringSubmatch(line); m != nil { + if _, dup := seen[m[1]]; !dup { + seen[m[1]] = section + } + } + } + + checked := 0 + for _, tun := range Tunables() { + if tun.DocKeyAnchor == "" { + continue // documented in prose, so no row to order + } + where, ok := seen[tun.Key] + if !ok { + t.Errorf("%s: no table row names this key, so it has no anchor to claim", tun.Key) + continue + } + if !definition[where] { + t.Errorf("%s: its first table row is under %q, so the help link would land there "+ + "rather than on its definition", tun.Key, where) + } + checked++ + } + if checked == 0 { + t.Fatal("checked no keys — this test is no longer pinning the document order") + } +} diff --git a/internal/help/indentcollide_test.go b/internal/help/indentcollide_test.go new file mode 100644 index 0000000..509efd2 --- /dev/null +++ b/internal/help/indentcollide_test.go @@ -0,0 +1,26 @@ +package help + +import ( + "strings" + "testing" +) + +// An indented heading is still a heading, so a key row that collides with it must +// still be refused — the pre-scan and the main pass have to agree about what +// counts. They did not: the scan matched the raw line, the renderer the trimmed +// one, and a heading with up to three leading spaces slipped through to produce +// two elements sharing one id and a green build. +func TestIndentedHeadingStillBlocksACollidingKeyRow(t *testing.T) { + md := " ## Key flags\n\n| Field | Default |\n|---|---|\n| `flags` | `x` |\n" + r := Render("probe.md", md) + + if !strings.Contains(r.HTML, `

Date: Sat, 22 Aug 2026 12:30:43 +0330 Subject: [PATCH 07/19] fix(help): tell a repeated key from two keys colliding on one anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/config/keyanchororder_test.go | 123 ++++++++++++++++--------- internal/help/markdown.go | 21 ++++- internal/help/slugcollide_test.go | 47 ++++++++++ 3 files changed, 145 insertions(+), 46 deletions(-) create mode 100644 internal/help/slugcollide_test.go diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go index 42f7381..69e2b29 100644 --- a/internal/config/keyanchororder_test.go +++ b/internal/config/keyanchororder_test.go @@ -8,6 +8,27 @@ import ( "testing" ) +// The sections of the reference that *define* keys, as opposed to comparing, +// renaming or retiring them. +var definitionSections = map[string]bool{ + "Fields": true, + "`control` block": true, + "`vpn` block": true, + "Advanced tunables (`vpn.advanced`)": true, +} + +// A *lone* code span in the first cell, exactly as help.rowKey requires — nothing +// else in the cell. +// +// Looser than that, this test recorded rows the renderer never claims: a row +// written "| `vpn.redialWindow` (renamed) | … |", the natural way to annotate the +// Renamed table, would have been reported as that key's first row and failed the +// build even though claimKey had correctly anchored the definition. A spurious +// failure on the one test that exists to catch a silent one. +var firstCellKey = regexp.MustCompile("^\\|\\s*`([^`]+)`\\s*\\|") + +var sectionHeading = regexp.MustCompile(`^#{2,6}\s+(.*)$`) + // TestKeyRowsAnchorToTheirDefinitionSection pins the document ordering that // help.claimKey silently depends on. // @@ -15,34 +36,69 @@ import ( // page — it cannot tell a definition from a summary, and refusing a duplicate // loudly would fail the build on every legitimately repeated key. So the guarantee // that a contextual help link lands on the definition rather than on the Presets -// comparison is a property of how config.md is *arranged*. Nothing in the renderer -// or the resolution test notices if that changes: both stay green as long as some -// row carries the anchor. +// comparison is a property of how the reference is *arranged*. Nothing in the +// renderer or the resolution test notices if that changes: both stay green as long +// as some row carries the anchor. // -// This is the test that notices. For every settable key, the first table row -// naming it must fall under one of the reference's definition headings. +// This is the test that notices. For every settable key, the first table row naming +// it must fall under one of the reference's definition headings. func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { - path := filepath.Join("..", "..", "docs", "usage", "config.md") - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("read %s: %v", path, err) + // The pages come from the anchors themselves rather than being hardcoded, so a + // tunable documented on some other bundled page is checked against that page + // instead of being reported as having no row at all. + pages := map[string]bool{} + for _, tun := range Tunables() { + if tun.DocKeyAnchor == "" { + continue + } + page, _, _ := strings.Cut(tun.DocKeyAnchor, "#") + pages[page] = true + } + if len(pages) == 0 { + t.Fatal("no tunable carries a row anchor — this test is no longer pinning anything") } - // The sections that define keys, as opposed to comparing or retiring them. - definition := map[string]bool{ - "Fields": true, - "`control` block": true, - "`vpn` block": true, - "Advanced tunables (`vpn.advanced`)": true, + // key -> the heading its first row appeared under, per page. + seen := map[string]map[string]string{} + for page := range pages { + path := filepath.Join("..", "..", "docs", filepath.FromSlash(page)) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + seen[page] = firstRowSections(string(data)) } - firstCell := regexp.MustCompile("^\\|\\s*`([^`]+)`") - heading := regexp.MustCompile(`^#{2,6}\s+(.*)$`) + checked := 0 + for _, tun := range Tunables() { + if tun.DocKeyAnchor == "" { + continue // documented in prose, so there is no row to order + } + page, _, _ := strings.Cut(tun.DocKeyAnchor, "#") + where, ok := seen[page][tun.Key] + if !ok { + t.Errorf("%s: no table row on %s names this key, so it has no anchor to claim", + tun.Key, page) + continue + } + if !definitionSections[where] { + t.Errorf("%s: its first table row on %s is under %q, so the help link would land "+ + "there rather than on its definition", tun.Key, page, where) + } + checked++ + } + if checked == 0 { + t.Fatal("checked no keys — this test is no longer pinning the document order") + } +} - seen := map[string]string{} // key -> heading it was first seen under +// firstRowSections maps each key to the heading under which its first table row +// appears, skipping fenced code the way the renderer does. +func firstRowSections(markdown string) map[string]string { + out := map[string]string{} section := "" inCode := false - for _, raw := range strings.Split(string(data), "\n") { + for _, raw := range strings.Split(markdown, "\n") { line := strings.TrimSpace(raw) if strings.HasPrefix(line, "```") { inCode = !inCode @@ -51,34 +107,15 @@ func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { if inCode { continue } - if m := heading.FindStringSubmatch(line); m != nil { + if m := sectionHeading.FindStringSubmatch(line); m != nil { section = strings.TrimSpace(m[1]) continue } - if m := firstCell.FindStringSubmatch(line); m != nil { - if _, dup := seen[m[1]]; !dup { - seen[m[1]] = section + if m := firstCellKey.FindStringSubmatch(line); m != nil { + if _, dup := out[m[1]]; !dup { + out[m[1]] = section } } } - - checked := 0 - for _, tun := range Tunables() { - if tun.DocKeyAnchor == "" { - continue // documented in prose, so no row to order - } - where, ok := seen[tun.Key] - if !ok { - t.Errorf("%s: no table row names this key, so it has no anchor to claim", tun.Key) - continue - } - if !definition[where] { - t.Errorf("%s: its first table row is under %q, so the help link would land there "+ - "rather than on its definition", tun.Key, where) - } - checked++ - } - if checked == 0 { - t.Fatal("checked no keys — this test is no longer pinning the document order") - } + return out } diff --git a/internal/help/markdown.go b/internal/help/markdown.go index 599029c..f401c61 100644 --- a/internal/help/markdown.go +++ b/internal/help/markdown.go @@ -143,7 +143,18 @@ func Render(source, markdown string) Rendered { // would quietly deep-link to the presets row instead, with the resolution test // still green because *some* row carries the anchor. TestKeyRowsAnchorToTheir // DefinitionSection pins the ordering this relies on. - claimedKeys := map[string]bool{} + // anchor -> the key that claimed it, not merely "claimed". The distinction is + // the whole point: refusing a *repeat* of the same key is benign and therefore + // silent (config.md documents several keys in more than one table, and failing + // the build on that would be absurd), but two *different* keys landing on one + // anchor is a silently wrong deep link. `Anchor` lowercases and strips `.`, `[` + // and `]`, so `vpn.pauseMax` and `vpn.pause-max` — or any rename that changes + // only punctuation or case — slug identically. The "Renamed keys" table precedes + // the definition tables and its first cell is the OLD name, so such a rename + // would hand the anchor to the old-name row and send every `?` for the live key + // there, with a green build: the resolution test only asks whether *some* row + // carries the fragment. Reported the same way a heading collision is. + claimedKeys := map[string]string{} // Heading anchors are collected up front, because a heading can appear *after* // the row that would collide with it while r.Headings is filled in the same // single pass below. @@ -187,14 +198,18 @@ func Render(source, markdown string) Rendered { } claimKey := func(key string) (string, bool) { anchor := KeyAnchor(key) - if claimedKeys[anchor] { + if owner, taken := claimedKeys[anchor]; taken { + if owner != key { + note("two different keys claim the anchor " + anchor + + " (" + owner + " and " + key + ")") + } return "", false } if headingAnchors[anchor] { note("a key row and a heading both claim the anchor " + anchor) return "", false } - claimedKeys[anchor] = true + claimedKeys[anchor] = key r.Keys = append(r.Keys, Heading{Text: key, Anchor: anchor}) return anchor, true } diff --git a/internal/help/slugcollide_test.go b/internal/help/slugcollide_test.go new file mode 100644 index 0000000..a8c7622 --- /dev/null +++ b/internal/help/slugcollide_test.go @@ -0,0 +1,47 @@ +package help + +import ( + "strings" + "testing" +) + +// Two different keys whose slugs collide is a silently wrong deep link, and has to +// be told apart from the same key appearing in two tables — which is normal and is +// refused quietly on purpose. +// +// Anchor lowercases and drops `.`, so the colliding pairs are a case-only rename +// (`vpn.armAtBoot` / `vpn.armatboot`, both `key-vpnarmatboot`) and one that removes +// a dot (`vpn.pauseMax` / `vpnpauseMax`). Hyphens survive, so `vpn.pause-max` is +// *not* one of them — checked, rather than assumed, when this test was written. +// +// The "Renamed keys" table lists the OLD name and precedes the definition tables, +// so the old row would take the anchor and every contextual link for the live key +// would land there. +func TestTwoKeysCollidingOnOneAnchorIsReported(t *testing.T) { + md := "## Fields\n\n| Field | Default |\n|---|---|\n" + + "| `vpn.armatboot` | old |\n| `vpn.armAtBoot` | new |\n" + r := Render("probe.md", md) + + if len(r.Unsupported) == 0 { + t.Fatal("a slug collision between two different keys was not reported") + } + if !strings.Contains(strings.Join(r.Unsupported, " "), "two different keys") { + t.Errorf("unexpected report: %v", r.Unsupported) + } +} + +// The same key in two tables stays silent: config.md does this legitimately, and +// failing the build on it would be absurd. +func TestTheSameKeyTwiceIsNotReported(t *testing.T) { + md := "## Fields\n\n| Field | Default |\n|---|---|\n| `pollInterval` | `5s` |\n\n" + + "## Presets\n\n| Field | strict |\n|---|---|\n| `pollInterval` | `1s` |\n" + r := Render("probe.md", md) + + if len(r.Unsupported) != 0 { + t.Errorf("a legitimately repeated key was reported: %v", r.Unsupported) + } + if strings.Count(r.HTML, `id="key-pollinterval"`) != 1 { + t.Errorf("expected exactly one anchored row, got %d", + strings.Count(r.HTML, `id="key-pollinterval"`)) + } +} From e97e67a1386299172f93a5537b64842b63f64bbf Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 12:50:42 +0330 Subject: [PATCH 08/19] fix(help): only a Field-headed table documents config keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/usage/config.md | 5 +- .../Sources/DezhbanMenu/OverviewView.swift | 22 ++++++-- internal/config/keyanchororder_test.go | 53 ++++++++++++++----- internal/help/keyscope_test.go | 51 ++++++++++++++++++ internal/help/markdown.go | 32 ++++++++--- 5 files changed, 138 insertions(+), 25 deletions(-) create mode 100644 internal/help/keyscope_test.go diff --git a/docs/usage/config.md b/docs/usage/config.md index b4589e1..b820b85 100644 --- a/docs/usage/config.md +++ b/docs/usage/config.md @@ -308,7 +308,10 @@ An optional block for behaviors that are otherwise recommended defaults. Omit it entirely to keep the defaults; set only the knobs you need. Every field below is reachable with `dezhban config set vpn.advanced.=` — the same validated write-and-reload path as any other key — not just by hand-editing the -file. `switchWindowMax`, `redialWindowMax`, `redialMinUptime`, `redialBudget`, +file. Prose in this section drops the `vpn.advanced.` prefix once it is +established; the table below always writes it in full, because that is what its +row anchors are derived from. +`switchWindowMax`, `redialWindowMax`, `redialMinUptime`, `redialBudget`, `redialBudgetWindow`, `verifyInterval`, `livenessRedial`, and `windowDiscoveryInterval` apply live; the rest (built into something the run loop constructs once at startup, or — for `windowProtocols`/`windowPorts` — diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index a9d97bf..d2c35b8 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -296,13 +296,20 @@ struct OverviewView: View { // of whatever line it lands on — which is also why the Spacer that used // to sit before it is gone. return ActionRow(trailingCount: 1) { - captioned("block", - state.routineHint("Cuts all traffic and holds it until you unblock.")) { + // A disabled control says why, rather than describing an action it will + // not perform. This text was tooltip-only before the row's titles were + // shortened; promoting it to the primary visible explanation is what makes + // "Releases a manual block…" under a greyed-out button worth fixing. The + // Pause branch below already worked this way. + captioned("block", blocked + ? "Disabled — traffic is already blocked." + : state.routineHint("Cuts all traffic and holds it until you unblock.")) { Button("Block") { AppActions.routine(["block"], "block") } .disabled(blocked) } - captioned("unblock", - state.routineHint("Releases a manual block and resumes monitoring.")) { + captioned("unblock", !(blocked || guardHolds) + ? "Disabled — there is no manual block or guard hold to release." + : state.routineHint("Releases a manual block and resumes monitoring.")) { Button("Unblock") { AppActions.routine(["unblock"], "unblock") } .disabled(!(blocked || guardHolds)) } @@ -419,9 +426,14 @@ struct OverviewView: View { /// present and always one line high — a caption that vanished between two /// buttons would reflow the row under the pointer. private func actionCaption(_ s: Snapshot) -> some View { + // The resting text is a prompt, not the posture. `PostureUI.humanPosture` is + // exactly `display.headline`, which the hero already renders in title2 a + // hundred points above — so with the pointer off the row, which is the normal + // state, the pane said "Traffic cut" twice. The only requirement on the + // fallback is that the line never collapses and reflows the row. Text(ActionCaption.text(hovered: hoveredAction?.hint, focused: focusedAction == nil ? nil : focusedHint, - fallback: PostureUI.humanPosture(s))) + fallback: "Point at a button to see what it does.")) .font(.callout) .foregroundStyle(.secondary) // Two lines, always reserved. One line truncated the tail of every long diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go index 69e2b29..33301e2 100644 --- a/internal/config/keyanchororder_test.go +++ b/internal/config/keyanchororder_test.go @@ -29,19 +29,27 @@ var firstCellKey = regexp.MustCompile("^\\|\\s*`([^`]+)`\\s*\\|") var sectionHeading = regexp.MustCompile(`^#{2,6}\s+(.*)$`) -// TestKeyRowsAnchorToTheirDefinitionSection pins the document ordering that -// help.claimKey silently depends on. +// A table's separator row, and a header whose first column is "Field" — the pair +// help.renderTable uses to decide that a table documents config keys. +var ( + tableSeparator = regexp.MustCompile(`^\|[\s:|-]+\|$`) + firstColumnIsField = regexp.MustCompile(`^\|\s*[Ff]ield\s*\|`) +) + +// TestKeyRowsAnchorToTheirDefinitionSection pins the document arrangement that +// help.claimKey depends on. // -// claimKey gives a key's row anchor to the FIRST table row naming that key on the -// page — it cannot tell a definition from a summary, and refusing a duplicate -// loudly would fail the build on every legitimately repeated key. So the guarantee -// that a contextual help link lands on the definition rather than on the Presets -// comparison is a property of how the reference is *arranged*. Nothing in the -// renderer or the resolution test notices if that changes: both stay green as long -// as some row carries the anchor. +// The renderer settles most of this itself now: only a table heading its first +// column "Field" defines keys, so config.md's presets and retired tables — headed +// "Key" — cannot claim an anchor however early they appear. What it still cannot +// settle is *two* Field-headed tables naming the same key, where the first one +// wins; refusing a duplicate loudly is not an option, since several keys are +// legitimately repeated. // -// This is the test that notices. For every settable key, the first table row naming -// it must fall under one of the reference's definition headings. +// So this is the remaining guard, and it is a second line rather than the only one. +// For every settable key, the first Field-table row naming it must fall under one of +// the reference's definition headings. Nothing else notices if that changes: the +// resolution test stays green as long as some row carries the anchor. func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { // The pages come from the anchors themselves rather than being hardcoded, so a // tunable documented on some other bundled page is checked against that page @@ -98,7 +106,15 @@ func firstRowSections(markdown string) map[string]string { out := map[string]string{} section := "" inCode := false - for _, raw := range strings.Split(markdown, "\n") { + // Only rows inside a real table, and only under a "Field" header — the same two + // conditions renderTable applies. Without the separator check a pipe-prefixed + // prose line counted as a row; without the header check a presets or retired row + // did, and neither is something claimKey would ever anchor. Both would have + // failed the build over a row the renderer ignores, which is the false-failure + // this test has already been fixed for once. + lines := strings.Split(markdown, "\n") + inFieldTable := false + for i, raw := range lines { line := strings.TrimSpace(raw) if strings.HasPrefix(line, "```") { inCode = !inCode @@ -109,6 +125,19 @@ func firstRowSections(markdown string) map[string]string { } if m := sectionHeading.FindStringSubmatch(line); m != nil { section = strings.TrimSpace(m[1]) + inFieldTable = false + continue + } + if !strings.HasPrefix(line, "|") { + inFieldTable = false + continue + } + // A header row is one followed by a separator; that is what starts a table. + if i+1 < len(lines) && tableSeparator.MatchString(strings.TrimSpace(lines[i+1])) { + inFieldTable = firstColumnIsField.MatchString(line) + continue + } + if tableSeparator.MatchString(line) || !inFieldTable { continue } if m := firstCellKey.FindStringSubmatch(line); m != nil { diff --git a/internal/help/keyscope_test.go b/internal/help/keyscope_test.go new file mode 100644 index 0000000..d242fe9 --- /dev/null +++ b/internal/help/keyscope_test.go @@ -0,0 +1,51 @@ +package help + +import ( + "strings" + "testing" +) + +// Only a table that heads its first column "Field" documents config keys. Every +// other lone code span in a first cell is something else — a CIDR range in +// concepts/modes.md, a subcommand in usage/cli.md — and minting `key-` anchors for +// those contradicted Rendered.Keys' own description and, worse, extended the +// collision guard (which aborts the whole app build) to pages with no keys at all. +func TestOnlyFieldHeadedTablesDefineKeys(t *testing.T) { + fields := "| Field | Default |\n|---|---|\n| `vpn.redialWindow` | `30s` |\n" + other := "| Range | Meaning |\n|---|---|\n| `100.64/10` | CGNAT |\n" + + r := Render("probe.md", "## A\n\n"+fields+"\n## B\n\n"+other) + + if !strings.Contains(r.HTML, `

 so a -// contextual help link can land on that key. Every other row is emitted exactly -// as before. claimKey decides the anchor and reports whether this row is the -// one that gets it — see Render, which resolves keys documented in two tables. +// In a table whose first column is headed "Field", a row opening with a lone code +// span — `vpn.redialWindow` — gets an `id` on its , so a contextual help link +// can land on that key. Every other row is emitted exactly as before. claimKey +// decides the anchor and reports whether this row is the one that gets it. +// +// The header gate is what makes "a documented config key" mean something. Without +// it, any lone code span in any first cell minted a `key-` anchor: concepts/modes.md +// produced key-1006410 and key-fc007 from its private-range table, usage/cli.md +// produced key-panic and key-run from its subcommand tables — pages that document no +// config key at all, contradicting Rendered.Keys' own description. The cost was not +// only untidiness: the collision guard aborts the whole app build, so a row added to +// one of cli.md's flag tables could fail `task gui:build` complaining about a "key +// row and a heading" on a page with no keys in it. +// +// It also settles definition-versus-summary structurally. docs/usage/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 at all — where before, the right row +// winning depended on the order the sections happened to appear in. func renderTable(out *strings.Builder, text *strings.Builder, lines []string, renderInline func(string) string, claimKey func(string) (string, bool)) int { header := splitRow(lines[0]) @@ -479,6 +492,9 @@ func renderTable(out *strings.Builder, text *strings.Builder, lines []string, out.WriteString("\n\n") text.WriteString("\n") + definesKeys := len(header) > 0 && + strings.EqualFold(strings.TrimSpace(stripInline(header[0])), "Field") + used := 2 // header + separator for _, line := range lines[2:] { s := strings.TrimSpace(line) @@ -487,8 +503,10 @@ func renderTable(out *strings.Builder, text *strings.Builder, lines []string, } cells := splitRow(s) anchor := "" - if key, ok := rowKey(cells); ok { - anchor, _ = claimKey(key) + if definesKeys { + if key, ok := rowKey(cells); ok { + anchor, _ = claimKey(key) + } } if anchor != "" { fmt.Fprintf(out, "", anchor) From a65639974f3b9a3523781795c430500e11e43a0a Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 14:17:55 +0330 Subject: [PATCH 09/19] fix(gui): say what the caption actually rests on, and wrap switchMenu once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 3 ++- docs/contribute/testing.md | 8 ++++-- docs/usage/cli.md | 17 +++++++----- .../Sources/DezhbanCore/ActionCaption.swift | 4 ++- .../Sources/DezhbanMenu/OverviewView.swift | 26 ++++++++++++------- internal/config/schema.go | 20 +++++++++----- 6 files changed, 52 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5e8319..095cf56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,8 @@ current as you land changes. "Resume now", and one **Cancel** in place of "Cancel redial window" and "Cancel VPN switch" (which window it closes is in the caption). The sentence each title used to carry inline moved to a caption line beneath the row that follows the - pointer and the keyboard focus, falling back to the posture headline. The same + pointer and the keyboard focus, and reads "Point at a button to see what it + does." when neither is on the row. The same sentence remains the tooltip, and the caption wraps to a second line rather than truncating, since its tail is where the password expectation is stated. The menubar's items are unchanged: a menu has no caption line to delegate to, and diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index c4f69b0..d7f058e 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1007,8 +1007,12 @@ task gui:build && open dist/Dezhban.app control's sentence — in particular, hovering **Pause** must say it uses your real ISP IP, which is the warning its old title carried — and the whole sentence must be readable, including the password expectation at its tail, - which is what a single truncated line used to cut. Moving off the row restores - the posture headline. + which is what a single truncated line used to cut. Moving off the row leaves a + prompt ("Point at a button to see what it does."), *not* the posture headline — + the status hero already shows that a few lines above, and the caption used to + repeat it verbatim. + A disabled Block or Unblock says why it is disabled rather than describing the + action it will not perform. Tab through the row with Full Keyboard Access on and confirm focus drives the caption too, **including with the pointer left resting on a different diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 16c7566..7c9fb2b 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -621,12 +621,14 @@ promise. Build it with `task gui:build` (see [development.md](../contribute/deve Two surfaces, split by urgency: - **Menubar dropdown — the safety/glance core.** One status line (posture, exit - country/provider), **Open Dezhban…**, **Block now/Unblock**, the VPN switch - window (Switching VPN… / Cancel VPN switch, with a live countdown) when in VPN - mode, - **Panic — force unblock…**, Quit. These are the time-critical and - lockout-recovery actions; they never depend on the main window opening. Items - enable/disable from the current state. + country/provider), **Open Dezhban…**, the VPN switch window (Switching VPN… / + Cancel VPN switch / Resume now, with a live countdown), **Pause — use my real + IP**, hold-the-line, **Panic — force unblock…**, Quit. These are the + time-critical and lockout-recovery actions; they never depend on the main window + opening. Items enable/disable from the current state. Manual **Block** and + **Unblock** are deliberately *not* here — somebody who wants to cut their own + internet can turn off Wi-Fi, so blocking by hand is a power-user affordance and + lives in the window's Overview (`AppDelegate` says so at the menu's top). - **Main window — everything else**, opened from the dropdown or by clicking the Dock icon (never automatically at launch). @@ -646,7 +648,8 @@ The main window's sidebar sections: Pause, Guard down, Panic…, and Resume / Cancel while a window is open) with a **caption line beneath the row** carrying the sentence each one used to hold inline — it follows the pointer - and the keyboard focus, and falls back to the posture headline. The same + and the keyboard focus, and reads a prompt when neither is on the row (not the + posture headline, which the status hero already shows just above). The same sentence is also the control's tooltip, so the two can never disagree. The menubar's items keep their long, self-explaining titles: a menu has no caption line to delegate to. diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift index 56de545..664792a 100644 --- a/gui/macos/Sources/DezhbanCore/ActionCaption.swift +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -26,7 +26,9 @@ public enum ActionCaption { /// so tabbing moved the focus ring and the Space key while the caption kept /// describing something else. /// - /// With neither, the posture headline stands in. Never empty and never a + /// With neither, `fallback` stands in — a prompt, in the app's case, not the + /// posture headline: the hero already renders that string in title2 just above, + /// so echoing it here said the same thing twice. Never empty and never a /// placeholder like "—": the line must not collapse and reflow the row every /// time the pointer crosses a gap between two buttons. public static func text(hovered: String?, focused: String?, fallback: String) -> String { diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index d2c35b8..c72a70d 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -13,8 +13,8 @@ struct OverviewView: View { /// there — see `captioned`. @State private var hoveredAction: (id: String, hint: String)? /// Last hint handed over by keyboard focus. Read only while - /// `focusedAction != nil`, so blurring the row falls back to the posture - /// headline instead of stranding the caption on a control nobody is on. + /// `focusedAction != nil`, so blurring the row falls back to the resting + /// prompt instead of stranding the caption on a control nobody is on. @State private var focusedHint = "" @FocusState private var focusedAction: String? @@ -452,11 +452,19 @@ struct OverviewView: View { /// learned endpoint is attributed to it (`switch --no-wait --name `). /// Plain button when there are no profiles to pick from — a menu with a /// single "Any known VPN" entry would just be a worse button. - @ViewBuilder + /// One `captioned` wrapper around both shapes, not one per branch. + /// + /// Both used the id "switch", and `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. Pointing + /// at "Switch VPN…" while profiles loaded therefore dropped the caption to the + /// resting prompt until the mouse moved. Wrapping once means the modifiers never + /// disappear, only their content changes. private var switchMenu: some View { let hint = state.routineHint("Briefly relaxes the guard so a new VPN can connect.") - if let profiles = state.profiles, !profiles.profiles.isEmpty { - captioned("switch", hint) { + return captioned("switch", hint) { + if let profiles = state.profiles, !profiles.profiles.isEmpty { Menu("Switch VPN…") { Button("Any known VPN") { AppActions.routine(["switch", "--no-wait"], "open a switch window") @@ -469,10 +477,10 @@ struct OverviewView: View { } } } - } - } else { - captioned("switch", hint) { - Button("Switch VPN…") { AppActions.routine(["switch", "--no-wait"], "open a switch window") } + } else { + Button("Switch VPN…") { + AppActions.routine(["switch", "--no-wait"], "open a switch window") + } } } } diff --git a/internal/config/schema.go b/internal/config/schema.go index 983ebbd..40c7bcf 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -525,17 +525,25 @@ func Tunables() []Tunable { // names the exceptions, and TestEveryTunableDocAnchorResolves fails the build // naming any key whose derived anchor does not exist — so a key that loses its // row cannot silently fall back to landing somewhere plausible and wrong. -func docKeyAnchorFor(key, declared string) string { +func docKeyAnchorFor(key, _ string) string { if keysDocumentedInProse[key] { return "" } - page, _, ok := strings.Cut(declared, "#") - if !ok || page == "" { - return "" - } - return page + "#key-" + anchorSlug(key) + return keyReferencePage + "#key-" + anchorSlug(key) } +// keyReferencePage is where the key *rows* live — the one page whose Field-headed +// tables the help renderer anchors (help.renderTable). +// +// Named here rather than taken from a key's section anchor, which is what this used +// to do. That coupled two things which need not agree: point one key's DocAnchor at +// a section on another page — a concept-heavy key at concepts/modes.md, say — and +// its derived row anchor named that page too, so the resolution test failed saying +// nothing there carries the anchor, even though 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. +const keyReferencePage = "usage/config.md" + // keysDocumentedInProse are the keys docs/usage/config.md covers outside a table // row, which therefore have no row anchor to land on. They get an empty // DocKeyAnchor and are reached through their section anchor alone. Adding a row From a31d9b58fa825e60aa9506ef77369cb840fc356d Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 14:33:02 +0330 Subject: [PATCH 10/19] fix(gui): rank the caption's inputs instead of erasing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../Sources/DezhbanCore/ActionCaption.swift | 33 +++++++---- .../Sources/DezhbanMenu/OverviewView.swift | 41 +++++++------ .../DezhbanCoreTests/ActionCaptionTests.swift | 40 ++++++++++--- internal/config/schema.go | 8 ++- internal/help/markdown.go | 58 +++++++++---------- internal/help/slugcollide_test.go | 7 ++- 6 files changed, 117 insertions(+), 70 deletions(-) diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift index 664792a..0162882 100644 --- a/gui/macos/Sources/DezhbanCore/ActionCaption.swift +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -13,26 +13,37 @@ import Foundation /// getting it wrong means a kill switch whose most dangerous button explains /// itself to nobody. public enum ActionCaption { + /// Which input the user reached for most recently. + public enum Aim: Equatable { + case pointer + case keyboard + } + /// The caption to show. /// - /// `hovered` is the hint of the control the pointer is over, `focused` the - /// hint of the keyboard-focused control. Hover wins when both are present: - /// the pointer is the thing the user is currently aiming, and focus often - /// lingers on whatever was last clicked. + /// `hovered` is the hint of the control the pointer is over, `focused` the hint + /// of the keyboard-focused control, and `aim` says which of the two the user + /// touched last. Most-recent-interaction wins, which is the only rule that + /// satisfies both directions: a pointer parked on one button must not outrank the + /// keyboard indefinitely (tabbing moved the focus ring and the Space key while + /// the caption described something else), and tabbing must not erase the fact + /// that the pointer is still resting somewhere. /// - /// That order is only safe because the caller clears `hovered` when focus - /// moves — most-recent-interaction wins. Preferring hover *unconditionally* - /// meant a pointer parked on one button outranked the keyboard indefinitely, - /// so tabbing moved the focus ring and the Space key while the caption kept - /// describing something else. + /// Ranking rather than clearing, deliberately. The caller used to answer this by + /// setting `hovered` to nil when focus moved, which threw the pointer's position + /// away instead of deprioritising it: tab into the row and back out to anything + /// outside it and *both* were empty, so the caption fell to the prompt while the + /// pointer sat on Block, until it moved off and back on. /// /// With neither, `fallback` stands in — a prompt, in the app's case, not the /// posture headline: the hero already renders that string in title2 just above, /// so echoing it here said the same thing twice. Never empty and never a /// placeholder like "—": the line must not collapse and reflow the row every /// time the pointer crosses a gap between two buttons. - public static func text(hovered: String?, focused: String?, fallback: String) -> String { - for candidate in [hovered, focused] { + public static func text(hovered: String?, focused: String?, + aim: Aim, fallback: String) -> String { + let order = aim == .keyboard ? [focused, hovered] : [hovered, focused] + for candidate in order { if let c = candidate, !c.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { return c } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index c72a70d..effc0ee 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -16,6 +16,9 @@ struct OverviewView: View { /// `focusedAction != nil`, so blurring the row falls back to the resting /// prompt instead of stranding the caption on a control nobody is on. @State private var focusedHint = "" + /// Which input the user reached for last, so focus can outrank a parked pointer + /// without the pointer's position being thrown away. See `ActionCaption.Aim`. + @State private var aim: ActionCaption.Aim = .pointer @FocusState private var focusedAction: String? var body: some View { @@ -69,7 +72,7 @@ struct OverviewView: View { VStack(alignment: .leading, spacing: 6) { actionButtons(s) - actionCaption(s) + actionCaption() } Spacer(minLength: 12) @@ -378,6 +381,7 @@ struct OverviewView: View { .onHover { inside in if inside { hoveredAction = (id, hint) + aim = .pointer } else if hoveredAction?.id == id { hoveredAction = nil } @@ -385,22 +389,24 @@ struct OverviewView: View { .onChange(of: focusedAction) { _ in guard focusedAction == id else { return } focusedHint = hint - // Focus supersedes a parked pointer. Hover used to win - // unconditionally, so tabbing with the pointer resting on another - // button moved the focus ring and the Space key while the caption went - // on describing whatever the mouse happened to be over. + // Focus outranks a parked pointer, and does it by ranking rather + // than by clearing: `hoveredAction` used to be set to nil here, which + // threw the pointer's position away instead of deprioritising it — tab + // into the row and back out to anything outside it and both were empty, + // so the caption fell to the resting prompt while the pointer sat on a + // button, until it moved off and back on. // - // The pointer takes it back by *entering* a control — the same one or - // another. There is deliberately no "the mouse moved a little" re-arm: - // every version of it read a proxy for movement rather than movement. - // `onContinuousHover`'s `.active` fires when a tracking area is merely - // established, and these controls re-measure constantly (a window's - // countdown retitles "Cancel (m:ss left)" every second); comparing the - // reported point did not help either, being in local space, so a banner - // appearing above the row moved the control under a stationary mouse - // and read as movement. Jiggling inside the control you are already on - // aims at nothing new, so nothing is lost. - hoveredAction = nil + // The pointer takes the caption back by *entering* a control, the same + // one or another. There is deliberately no "the mouse moved a little" + // re-arm: every version of it read a proxy for movement rather than + // movement. `onContinuousHover`'s `.active` fires when a tracking area + // is merely established, and these controls re-measure constantly (a + // window's countdown retitles "Cancel (m:ss left)" every second); + // comparing the reported point did not help either, being in local + // space, so a banner appearing above the row moved the control under a + // stationary mouse and read as movement. Jiggling inside the control + // you are already on aims at nothing new, so nothing is lost. + aim = .keyboard } .onChange(of: hint) { newHint in // The captured string has to track the live one. `state.routineHint` @@ -425,7 +431,7 @@ struct OverviewView: View { /// The one line that explains whatever the user is pointing at. Always /// present and always one line high — a caption that vanished between two /// buttons would reflow the row under the pointer. - private func actionCaption(_ s: Snapshot) -> some View { + private func actionCaption() -> some View { // The resting text is a prompt, not the posture. `PostureUI.humanPosture` is // exactly `display.headline`, which the hero already renders in title2 a // hundred points above — so with the pointer off the row, which is the normal @@ -433,6 +439,7 @@ struct OverviewView: View { // fallback is that the line never collapses and reflows the row. Text(ActionCaption.text(hovered: hoveredAction?.hint, focused: focusedAction == nil ? nil : focusedHint, + aim: aim, fallback: "Point at a button to see what it does.")) .font(.callout) .foregroundStyle(.secondary) diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift index 6a792cc..87f8cf3 100644 --- a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift @@ -2,24 +2,50 @@ import Testing @testable import DezhbanCore struct ActionCaptionTests { - @Test func hoverWins() { + @Test func thePointerWinsWhileItIsWhatTheUserLastUsed() { #expect(ActionCaption.text(hovered: "cut everything", focused: "release the line", - fallback: "guard is up") == "cut everything") + aim: .pointer, + fallback: "point at a button") == "cut everything") + } + + /// Tabbing has to take the caption from a parked pointer, or the focus ring and + /// the Space key end up on one button while the caption describes another. + @Test func theKeyboardWinsAfterTabbing() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: "release the line", + aim: .keyboard, + fallback: "point at a button") == "release the line") + } + + /// Ranking, not erasing. Tab into the row and back out of it — focus is gone, the + /// pointer has not moved — and the caption must still describe what it is on. + /// The caller used to clear the hover state instead, which left this case showing + /// the resting prompt over a control the pointer was physically resting on. + @Test func aParkedPointerSurvivesFocusLeavingTheRow() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: nil, + aim: .keyboard, + fallback: "point at a button") == "cut everything") } @Test func focusIsUsedWhenNothingIsHovered() { #expect(ActionCaption.text(hovered: nil, focused: "release the line", - fallback: "guard is up") == "release the line") + aim: .pointer, + fallback: "point at a button") == "release the line") } /// The line must never go blank between two buttons: an empty caption /// collapses its own height and reflows the row under the pointer. @Test func fallbackFillsEveryGap() { - #expect(ActionCaption.text(hovered: nil, focused: nil, - fallback: "guard is up") == "guard is up") - #expect(ActionCaption.text(hovered: "", focused: " ", - fallback: "guard is up") == "guard is up") + for aim: ActionCaption.Aim in [.pointer, .keyboard] { + #expect(ActionCaption.text(hovered: nil, focused: nil, + aim: aim, fallback: "point at a button") + == "point at a button") + #expect(ActionCaption.text(hovered: "", focused: " ", + aim: aim, fallback: "point at a button") + == "point at a button") + } } } diff --git a/internal/config/schema.go b/internal/config/schema.go index 40c7bcf..8b04cb5 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -499,7 +499,7 @@ func Tunables() []Tunable { for i, t := range tunables { t.Default = defaults[t.Key] t.RestartReason = restartReasonFor(t.Key) - t.DocKeyAnchor = docKeyAnchorFor(t.Key, t.DocAnchor) + t.DocKeyAnchor = docKeyAnchorFor(t.Key) out[i] = t } return out @@ -525,7 +525,7 @@ func Tunables() []Tunable { // names the exceptions, and TestEveryTunableDocAnchorResolves fails the build // naming any key whose derived anchor does not exist — so a key that loses its // row cannot silently fall back to landing somewhere plausible and wrong. -func docKeyAnchorFor(key, _ string) string { +func docKeyAnchorFor(key string) string { if keysDocumentedInProse[key] { return "" } @@ -542,6 +542,10 @@ func docKeyAnchorFor(key, _ string) string { // nothing there carries the anchor, even though 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. +// +// docKeyAnchorFor takes no anchor argument at all, for the same reason: threading +// the section anchor in only to ignore it leaves the next edit one `_` away from +// reintroducing the coupling this constant exists to remove. const keyReferencePage = "usage/config.md" // keysDocumentedInProse are the keys docs/usage/config.md covers outside a table diff --git a/internal/help/markdown.go b/internal/help/markdown.go index c5fd059..0cba1a5 100644 --- a/internal/help/markdown.go +++ b/internal/help/markdown.go @@ -129,31 +129,28 @@ func Render(source, markdown string) Rendered { } // claimKey hands out the anchor for a key row, the FIRST time that key is seen - // on this page. A key documented in more than one table — several are, e.g. - // `pollInterval` in both the field reference and the presets comparison — can - // only have one id, since duplicates are invalid HTML and a browser jumps to - // whichever came first; the choice is made here rather than left to chance. + // on this page. A key documented in more than one Field-headed table can only + // have one id, since duplicates are invalid HTML and a browser jumps to whichever + // came first; the choice is made here rather than left to chance. // - // "First" is the whole rule, and it is not the same claim as "its definition". - // It lands on the definition because docs/usage/config.md is ordered that way, - // which is an assumption about the document, not something this function can - // check — a duplicate is refused silently, because refusing loudly would fail - // the build on every legitimately repeated key. Reorder the reference so the - // Presets comparison precedes the field tables and every `?` for those keys - // would quietly deep-link to the presets row instead, with the resolution test - // still green because *some* row carries the anchor. TestKeyRowsAnchorToTheir - // DefinitionSection pins the ordering this relies on. - // anchor -> the key that claimed it, not merely "claimed". The distinction is - // the whole point: refusing a *repeat* of the same key is benign and therefore - // silent (config.md documents several keys in more than one table, and failing - // the build on that would be absurd), but two *different* keys landing on one - // anchor is a silently wrong deep link. `Anchor` lowercases and strips `.`, `[` - // and `]`, so `vpn.pauseMax` and `vpn.pause-max` — or any rename that changes - // only punctuation or case — slug identically. The "Renamed keys" table precedes - // the definition tables and its first cell is the OLD name, so such a rename - // would hand the anchor to the old-name row and send every `?` for the live key - // there, with a green build: the resolution test only asks whether *some* row - // carries the fragment. Reported the same way a heading collision is. + // The map is anchor -> the key that claimed it, not merely "claimed", because + // those are two different situations. Refusing a *repeat of the same key* is + // benign and therefore silent: config.md documents several keys in more than one + // table and failing the build on that would be absurd. Two *different* keys + // landing on one anchor is a silently wrong deep link, and is reported the way a + // heading collision is. + // + // They can meet because `Anchor` lowercases and drops `.` (hyphens survive, so + // `vpn.pause-max` is not an example — checked). The real pairs are a case-only + // rename, `vpn.armAtBoot` against `vpn.armatboot`, and one that removes a dot, + // `vpn.pauseMax` against `vpnpauseMax`. + // + // What this cannot judge is which of two *Field* tables is the definition: it + // takes the first, and that is the definition because the reference is arranged + // that way. config.md's presets and retired tables are headed "Key" and its + // rename table "Old name", so none of them can claim an anchor at all — + // renderTable's header gate settles that. Two Field tables naming one key is the + // remaining case, and config.TestKeyRowsAnchorToTheirDefinitionSection pins it. claimedKeys := map[string]string{} // Heading anchors are collected up front, because a heading can appear *after* // the row that would collide with it while r.Headings is filled in the same @@ -168,12 +165,13 @@ func Render(source, markdown string) Rendered { // written links depend on, and the row is *noted* rather than quietly skipped: // Unsupported fails the bundle build (bundle.go), so this becomes a build error // naming the page instead of a link that misses. - // Through headingRe and outside code fences, so the pre-scan sees exactly what - // the main pass will call a heading. Accepting "any trimmed line starting with - // #" was looser: `#nospace`, seven hashes, and — worst — a shell comment inside a - // fenced block all registered as phantom headings, and one that happened to slug - // to a real row's anchor would refuse that row and fail the whole app build - // naming a heading nobody can find. + // + // The scan goes through headingRe and skips code fences, so it sees exactly what + // the main pass will call a heading. "Any trimmed line starting with #" was + // looser: `#nospace`, seven hashes, and — worst — a shell comment inside a fenced + // block all registered as phantom headings, and one that happened to slug onto a + // real row's anchor would refuse that row and fail the whole app build naming a + // heading nobody can find. headingAnchors := map[string]bool{} scanInCode := false for _, line := range strings.Split(markdown, "\n") { diff --git a/internal/help/slugcollide_test.go b/internal/help/slugcollide_test.go index a8c7622..a1104fc 100644 --- a/internal/help/slugcollide_test.go +++ b/internal/help/slugcollide_test.go @@ -14,9 +14,10 @@ import ( // a dot (`vpn.pauseMax` / `vpnpauseMax`). Hyphens survive, so `vpn.pause-max` is // *not* one of them — checked, rather than assumed, when this test was written. // -// The "Renamed keys" table lists the OLD name and precedes the definition tables, -// so the old row would take the anchor and every contextual link for the live key -// would land there. +// Both have to be inside a Field-headed table to arise at all: renderTable's header +// gate means config.md's presets ("Key"), retired ("Key") and rename ("Old name") +// tables cannot claim an anchor however early they appear. So the live hazard is a +// rename or addition *within* the field reference itself. func TestTwoKeysCollidingOnOneAnchorIsReported(t *testing.T) { md := "## Fields\n\n| Field | Default |\n|---|---|\n" + "| `vpn.armatboot` | old |\n| `vpn.armAtBoot` | new |\n" From d5398fa06f3361cbd86e57c6ecd44c202e3724fe Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 14:49:18 +0330 Subject: [PATCH 11/19] docs: a checklist step that cannot be run is not coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- README.md | 7 ++++--- docs/contribute/testing.md | 12 ++++++++++-- gui/macos/Sources/DezhbanMenu/AppState.swift | 13 ++++++++++--- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 9c33383..ab3182c 100644 --- a/README.md +++ b/README.md @@ -68,9 +68,10 @@ Gatekeeper friction** and asks for your password exactly once. Details, the Open the app and everything else happens by clicking: - **Menubar dropdown** — the safety core. One glance tells you the posture - (e.g. "Guard — NL via Mullvad"); **Block now** / **Unblock**, the VPN switch - window with a live countdown, and **Panic**. These never require the main - window to be open. + (e.g. "Guard — NL via Mullvad"); the VPN switch window with a live countdown, + **Pause**, and **Panic**. These never require the main window to be open. + Manual **Block** / **Unblock** are not here — they live in Overview, since + anyone who wants to cut their own internet can turn off Wi-Fi. - **Overview** — live status, the daily controls, and guided recovery: if the service isn't installed or is stopped, there's an inline button for exactly that, not an error message. diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index d7f058e..5f5c2f2 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1026,8 +1026,16 @@ task gui:build && open dist/Dezhban.app parked on **Pause**, open a switch window from the menubar: the button under the pointer becomes **Cancel**, and the caption must stop describing Pause without waiting for the mouse to move. And with the pointer parked on - **Block**, stop the daemon: the caption's password clause must follow the - tooltip's rather than keeping the answer it was given at hover-enter. + **Block**, make the control socket unreachable *without* stopping the daemon — + `dezhban config set control.enabled=false` then `dezhban restart` — and the + caption's password clause must follow the tooltip's rather than keeping the + answer it was given at hover-enter. + + Not by stopping the daemon: `state.isLive` goes false, Overview renders its + guided "stopped" layout, and the action row and its caption line are gone + before any of this could be observed. `routineHint` keys on + `controlIsReachable`, which is the socket, not the posture — so the socket has + to be the thing that changes. - [ ] **VoiceOver still hears what each button does.** With VoiceOver on, move through the action row: each control announces its short title *and* its consequence as a hint — Cancel in particular must say whether it closes the diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index b419d91..07424cc 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -234,9 +234,16 @@ final class AppState: ObservableObject { /// forty-key reference is not a useful place to land. func openHelp(preferring targets: [HelpTarget]) { guard !targets.isEmpty else { return } - // Decoded once and kept. `bundled()` reads the payload off disk and JSON- - // decodes every page's full text plus its per-row key list, which is not a - // thing to do synchronously on the main thread on every click of a `?`. + // Decoded once and kept, rather than on every click of a `?`: `bundled()` + // reads the payload off disk and JSON-decodes every page's full text plus its + // per-row key list. + // + // It does not make that the app's only decode — `HelpView` evaluates + // `bundled()` in its own `@State` initialiser, so a second copy exists and is + // rebuilt more often than this one. Sharing them is worth doing and is not + // this change's business; the point here is only that resolving a target must + // not add a decode of its own. + // // Nil (a bare SwiftPM binary with no help payload) leaves the preferred // target, which is the right guess when nothing can be checked. if cachedHelpBundle == nil { cachedHelpBundle = HelpBundle.bundled() } From 06685754acf99c7fb4e6a10e626418b4d1d90c6d Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 15:03:45 +0330 Subject: [PATCH 12/19] fix(gui): bind focus to a control, not to a branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/usage/cli.md | 6 ++- .../Sources/DezhbanMenu/OverviewView.swift | 39 ++++++++++++++++++- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 7c9fb2b..4212df9 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -645,12 +645,14 @@ The main window's sidebar sections: *expected* unknown exit stays a plain row. With profiles configured, "Switch VPN…" becomes a menu so a switch window can target one by name. The action row's titles are deliberately short (Block, Unblock, Switch VPN…, - Pause, Guard down, Panic…, and Resume / Cancel while a window is open) with a + Pause, Guard down, and Resume / Cancel while a window is open) with a **caption line beneath the row** carrying the sentence each one used to hold inline — it follows the pointer and the keyboard focus, and reads a prompt when neither is on the row (not the posture headline, which the status hero already shows just above). The same - sentence is also the control's tooltip, so the two can never disagree. The + sentence is also the control's tooltip, so the two can never disagree. **Panic…** + is not in that row — it sits below with its own fixed caption, so hovering it + leaves the row's caption alone. The menubar's items keep their long, self-explaining titles: a menu has no caption line to delegate to. Degraded states are guided: CLI missing, service not installed, and daemon diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index effc0ee..9de5653 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -364,10 +364,22 @@ struct OverviewView: View { /// shown. Hover is cleared only by the control that owns the current hint, /// so the pointer leaving A after it has already entered B does not blank /// out B's caption. + /// `focusable: false` skips the `.focused` binding, for a caller whose content is + /// a branch rather than a control. + /// + /// `.focused(_:equals:)` binds focus for a *focusable view*. Every slot but one + /// passes its Button straight through, so the binding lands on the control; the + /// switch slot passes an if/else, and a `_ConditionalContent` wrapper is not + /// something focus is defined to attach to. Rather than depend on whether it + /// happens to work, that caller applies the binding to the concrete Menu and + /// Button inside each branch and turns this off — the rest of the wrapper, which + /// is what must not disappear across the branch swap, is unaffected. @ViewBuilder private func captioned(_ id: String, _ hint: String, + focusable: Bool = true, @ViewBuilder content: () -> Content) -> some View { content() + .modifier(FocusBinding(id: focusable ? id : nil, focus: $focusedAction)) .help(hint) // And as an accessibility hint, not only as `help`. Shortening the titles // moved the explanation into a caption line that is @@ -377,7 +389,6 @@ struct OverviewView: View { // redial window or one the user opened. A hint is where a control's // consequence belongs, and it is announced after the label. .accessibilityHint(hint) - .focused($focusedAction, equals: id) .onHover { inside in if inside { hoveredAction = (id, hint) @@ -470,7 +481,7 @@ struct OverviewView: View { /// disappear, only their content changes. private var switchMenu: some View { let hint = state.routineHint("Briefly relaxes the guard so a new VPN can connect.") - return captioned("switch", hint) { + return captioned(Self.switchID, hint, focusable: false) { if let profiles = state.profiles, !profiles.profiles.isEmpty { Menu("Switch VPN…") { Button("Any known VPN") { @@ -484,14 +495,20 @@ struct OverviewView: View { } } } + .focused($focusedAction, equals: Self.switchID) } else { Button("Switch VPN…") { AppActions.routine(["switch", "--no-wait"], "open a switch window") } + .focused($focusedAction, equals: Self.switchID) } } } + /// Shared by both switch shapes and by the caption wrapper around them, so the + /// two cannot drift apart. + private static let switchID = "switch" + private var panicRow: some View { HStack(alignment: .firstTextBaseline, spacing: PaneMetrics.controlSpacing) { Button(role: .destructive) { @@ -601,3 +618,21 @@ struct OverviewView: View { .padding(24) } } + +/// Applies `.focused(_:equals:)` only when there is an id to bind. +/// +/// A modifier rather than an `if` in the view builder, so the branch does not change +/// the view's identity — which is the whole point of `captioned` wrapping its content +/// once (see `switchMenu`). +private struct FocusBinding: ViewModifier { + let id: String? + let focus: FocusState.Binding + + func body(content: Content) -> some View { + if let id { + content.focused(focus, equals: id) + } else { + content + } + } +} From b4387f132cbf833d212327710dce2e9f5a92add9 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 15:18:25 +0330 Subject: [PATCH 13/19] test(config): mirror the renderer's header rule instead of approximating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/contribute/testing.md | 5 ++- .../Sources/DezhbanMenu/OverviewView.swift | 29 +++++++++++----- internal/config/keyanchororder_test.go | 34 ++++++++++++++++--- 3 files changed, 53 insertions(+), 15 deletions(-) diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 5f5c2f2..e3abbd3 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1007,7 +1007,10 @@ task gui:build && open dist/Dezhban.app control's sentence — in particular, hovering **Pause** must say it uses your real ISP IP, which is the warning its old title carried — and the whole sentence must be readable, including the password expectation at its tail, - which is what a single truncated line used to cut. Moving off the row leaves a + which is what a single truncated line used to cut. Narrow the window until the + action row wraps and check it again there: two reserved lines were enough at a + comfortable width and put the ellipsis back on the password clause at a small + one, which is why the caption reserves three. Moving off the row leaves a prompt ("Point at a button to see what it does."), *not* the posture headline — the status hero already shows that a few lines above, and the caption used to repeat it verbatim. diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 9de5653..97799b1 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -454,14 +454,20 @@ struct OverviewView: View { fallback: "Point at a button to see what it does.")) .font(.callout) .foregroundStyle(.secondary) - // Two lines, always reserved. One line truncated the tail of every long + // Three lines, always reserved. One line truncated the tail of every long // caption, and the tail is where `routineHint` appends the password // expectation — so Pause read "…Will ask for your pass…" and dropped the - // clause warning that a prompt is coming. Reserving the space keeps the - // row from reflowing as the pointer crosses it, which is why this was - // one line to begin with; the panic caption twelve lines below already - // made the same trade for the same reason. - .lineLimit(2, reservesSpace: true) + // clause warning that a prompt is coming. Two was enough at a comfortable + // width and not at a narrow one: the pane is resizable, ActionRow is built + // to wrap, and the longest caption is ~137 characters, so the ellipsis + // landed back on the password clause exactly when the window was small. + // + // Reserving the space is what keeps the row from reflowing as the pointer + // crosses it, which is why this was capped at all; the cost of the third + // line is a little whitespace at wide widths, against a safety clause + // disappearing at narrow ones. The panic caption below makes the same + // trade by wrapping freely. + .lineLimit(3, reservesSpace: true) .frame(maxWidth: .infinity, alignment: .leading) .accessibilityHidden(true) } @@ -621,9 +627,14 @@ struct OverviewView: View { /// Applies `.focused(_:equals:)` only when there is an id to bind. /// -/// A modifier rather than an `if` in the view builder, so the branch does not change -/// the view's identity — which is the whole point of `captioned` wrapping its content -/// once (see `switchMenu`). +/// This does *not* make the branch identity-stable: `if let id { … } else { … }` in a +/// `ViewModifier.body` produces `_ConditionalContent` exactly as an inline `if` +/// would. It is safe because `focusable:` is a compile-time constant at every call +/// site, so `id` never flips for a given view. +/// +/// Which is the constraint to keep. Make `focusable` depend on state and the branch +/// swap destroys the wrapper's `onHover`/`onDisappear` state — the caption loss that +/// `switchMenu` was restructured to fix by wrapping both shapes once. private struct FocusBinding: ViewModifier { let id: String? let focus: FocusState.Binding diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go index 33301e2..9500a55 100644 --- a/internal/config/keyanchororder_test.go +++ b/internal/config/keyanchororder_test.go @@ -29,13 +29,37 @@ var firstCellKey = regexp.MustCompile("^\\|\\s*`([^`]+)`\\s*\\|") var sectionHeading = regexp.MustCompile(`^#{2,6}\s+(.*)$`) -// A table's separator row, and a header whose first column is "Field" — the pair -// help.renderTable uses to decide that a table documents config keys. +// A table's separator row, and the first cell of a header row. var ( - tableSeparator = regexp.MustCompile(`^\|[\s:|-]+\|$`) - firstColumnIsField = regexp.MustCompile(`^\|\s*[Ff]ield\s*\|`) + tableSeparator = regexp.MustCompile(`^\|[\s:|-]+\|$`) + firstHeaderCell = regexp.MustCompile(`^\|([^|]*)\|`) ) +// headsAKeyTable mirrors help.renderTable's gate: inline markup stripped, then a +// case-insensitive compare against "Field". +// +// A literal regex on the raw cell did not mirror it. `| `Field` |`, `| **Field** |` +// and `| FIELD |` all make the *renderer* mint row anchors from that table while a +// pattern matching bare "Field" stops treating it as a key table — so the guarantee +// this file exists to pin would silently stop covering it, and the `checked == 0` +// fatal does not help because it only fires when *every* key falls out, not a +// subset. Copied rather than called: internal/help's own tests import this package, +// so importing help from here would close an import cycle. +func headsAKeyTable(headerLine string) bool { + m := firstHeaderCell.FindStringSubmatch(headerLine) + if m == nil { + return false + } + cell := strings.Map(func(r rune) rune { + switch r { + case '`', '*', '_': + return -1 + } + return r + }, m[1]) + return strings.EqualFold(strings.TrimSpace(cell), "Field") +} + // TestKeyRowsAnchorToTheirDefinitionSection pins the document arrangement that // help.claimKey depends on. // @@ -134,7 +158,7 @@ func firstRowSections(markdown string) map[string]string { } // A header row is one followed by a separator; that is what starts a table. if i+1 < len(lines) && tableSeparator.MatchString(strings.TrimSpace(lines[i+1])) { - inFieldTable = firstColumnIsField.MatchString(line) + inFieldTable = headsAKeyTable(line) continue } if tableSeparator.MatchString(line) || !inFieldTable { From c36b95bfd8e99efa80491c9ff54d40433f31ad97 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 15:34:55 +0330 Subject: [PATCH 14/19] test(cli): pin the JSON field names the app decodes by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 10 ++- cmd/dezhban/schemawire_test.go | 90 +++++++++++++++++++ docs/contribute/testing.md | 9 +- .../Sources/DezhbanMenu/OverviewView.swift | 9 +- 4 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 cmd/dezhban/schemawire_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 095cf56..5632e70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,10 +31,12 @@ current as you land changes. VPN switch" (which window it closes is in the caption). The sentence each title used to carry inline moved to a caption line beneath the row that follows the pointer and the keyboard focus, and reads "Point at a button to see what it - does." when neither is on the row. The same - sentence remains the tooltip, and the caption wraps to a second line rather than - truncating, since its tail is where the password expectation is stated. The - menubar's items are unchanged: a menu has no caption line to delegate to, and + does." when neither is on the row. Whichever of the two you used last wins, so + tabbing takes the caption from a resting pointer and moving the pointer takes it + back. The same sentence remains the tooltip and is also announced by VoiceOver as + the control's hint, and the caption wraps rather than truncating — its tail is + where the password expectation is stated, and that has to survive a narrow window. + The menubar's items are unchanged: a menu has no caption line to delegate to, and a guided empty state's panic button keeps its full title for the same reason. - **The advanced tunables table names its keys in full** (`vpn.advanced.redialBudget` rather than `redialBudget`), matching every other table in the reference. diff --git a/cmd/dezhban/schemawire_test.go b/cmd/dezhban/schemawire_test.go new file mode 100644 index 0000000..6856bed --- /dev/null +++ b/cmd/dezhban/schemawire_test.go @@ -0,0 +1,90 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/behnam-rk/dezhban/internal/config" +) + +// TestSchemaWireNamesTheAppDecodes pins the JSON field names `config schema --json` +// emits, because the macOS app decodes them by name and fails silently if one moves. +// +// `ConfigTunable`'s CodingKeys name `docAnchor` and `docKeyAnchor`, and +// `docKeyAnchor` is decoded with `decodeIfPresent` so an older CLI degrades to the +// section anchor. That tolerance is what would make a rename invisible: rename the Go +// tag and every key silently loses its row anchor, the `?` goes back to landing on +// section headings, and nothing on either side fails — the Swift test builds its +// fixture by hand, so it would keep passing too. +// +// Asserted against `schemaEntry`, not `config.Tunable`: the wire shape is the CLI's +// wrapper, which embeds the Tunable and adds `preset`. Marshalling the Tunable alone +// omits that field, so a test written against it both misses a name the app decodes +// and fails for a reason that is not a defect. This is where the tag would be +// changed, so this is where the assertion belongs. +func TestSchemaWireNamesTheAppDecodes(t *testing.T) { + tunables := config.Tunables() + if len(tunables) == 0 { + t.Fatal("no tunables — this test is pinning nothing") + } + + entries := make([]schemaEntry, len(tunables)) + written := presetWritten() + for i, tun := range tunables { + entries[i] = schemaEntry{Tunable: tun, Preset: written[tun.Key]} + } + + raw, err := json.Marshal(entries) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var decoded []map[string]json.RawMessage + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + // Every name in ConfigSchema.swift's CodingKeys, so a Go-side rename of any of + // them fails here rather than degrading the app in silence. + required := []string{ + "key", "label", "kind", "default", "disablable", "advanced", "preset", + "help", "docAnchor", + } + for i, obj := range decoded { + for _, name := range required { + if _, ok := obj[name]; !ok { + t.Errorf("%s: no %q field on the wire", tunables[i].Key, name) + } + } + } + + // docKeyAnchor is omitempty, so it is absent exactly for the keys documented in + // prose — and present, naming a row on the reference page, for the rest. + withRow := 0 + for i, obj := range decoded { + key := tunables[i].Key + frag, present := obj["docKeyAnchor"] + if tunables[i].DocKeyAnchor == "" { + if present { + t.Errorf("%s: docKeyAnchor should be omitted when empty, got %s", key, frag) + } + continue + } + if !present { + t.Errorf("%s: docKeyAnchor missing from the wire", key) + continue + } + var s string + if err := json.Unmarshal(frag, &s); err != nil { + t.Errorf("%s: docKeyAnchor is not a string: %v", key, err) + continue + } + if !strings.Contains(s, "#key-") { + t.Errorf("%s: docKeyAnchor is %q, which does not name a key row", key, s) + } + withRow++ + } + if withRow == 0 { + t.Fatal("no tunable carried a row anchor — the app would have nothing to deep-link to") + } +} diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index e3abbd3..6f2388e 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1022,8 +1022,13 @@ task gui:build && open dist/Dezhban.app button** — focus is meant to supersede a parked pointer. Then move the pointer onto a control (the same one or another) and confirm it takes over again: re-entering is what hands it back, deliberately, since jiggling inside the - control you are already on aims at nothing new. The line must never go blank or change - height, which would reflow the row under the pointer. + control you are already on aims at nothing new. + + Then, still with the pointer parked on a button, Tab *out* of the row + altogether. The caption must go back to describing the button under the pointer + — not to the resting prompt. Focus outranks a parked pointer while it is in the + row; it does not erase where the pointer is. The line must never go blank or + change height either, which would reflow the row under the pointer. Two more, both about a caption outliving what it described. With the pointer parked on **Pause**, open a switch window from the menubar: the button under diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 97799b1..7192f82 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -364,6 +364,7 @@ struct OverviewView: View { /// shown. Hover is cleared only by the control that owns the current hint, /// so the pointer leaving A after it has already entered B does not blank /// out B's caption. + /// /// `focusable: false` skips the `.focused` binding, for a caller whose content is /// a branch rather than a control. /// @@ -439,9 +440,11 @@ struct OverviewView: View { } } - /// The one line that explains whatever the user is pointing at. Always - /// present and always one line high — a caption that vanished between two - /// buttons would reflow the row under the pointer. + /// The line that explains whatever the user is pointing at or has focused. + /// + /// Always present and always the same height — three reserved lines, see below. + /// A caption that collapsed between two buttons would reflow the row under the + /// pointer, which is the one thing it may never do. private func actionCaption() -> some View { // The resting text is a prompt, not the posture. `PostureUI.humanPosture` is // exactly `display.headline`, which the hero already renders in title2 a From dcda5b73b4f10e1c9b12548d3d16f7ece9db8e95 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 22 Aug 2026 15:54:48 +0330 Subject: [PATCH 15/19] fix(gui): a re-established tracking area is not the user aiming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- cmd/dezhban/schemawire_test.go | 36 +++++++++++++- docs/contribute/testing.md | 49 ++++++++++++------- .../Sources/DezhbanMenu/OverviewView.swift | 27 +++++++--- 3 files changed, 86 insertions(+), 26 deletions(-) diff --git a/cmd/dezhban/schemawire_test.go b/cmd/dezhban/schemawire_test.go index 6856bed..4d696f4 100644 --- a/cmd/dezhban/schemawire_test.go +++ b/cmd/dezhban/schemawire_test.go @@ -44,8 +44,7 @@ func TestSchemaWireNamesTheAppDecodes(t *testing.T) { t.Fatalf("unmarshal: %v", err) } - // Every name in ConfigSchema.swift's CodingKeys, so a Go-side rename of any of - // them fails here rather than degrading the app in silence. + // The names every tunable carries unconditionally. required := []string{ "key", "label", "kind", "default", "disablable", "advanced", "preset", "help", "docAnchor", @@ -58,6 +57,39 @@ func TestSchemaWireNamesTheAppDecodes(t *testing.T) { } } + // And the `omitempty` ones, asserted per tunable that actually has a value. + // + // Listing only the unconditional names and calling that "every CodingKey" was + // the first shape of this test, and it left the most consequential field + // unpinned: rename `restartReason` and `ConfigTunable.appliesLive` — which is + // `(restartReason ?? "").isEmpty` — returns true for every key, so Settings + // tells the user a restart-required key applies live. Nothing would have failed. + optional := map[string]func(config.Tunable) string{ + "capKey": func(t config.Tunable) string { return t.CapKey }, + "unit": func(t config.Tunable) string { return t.Unit }, + "restartReason": func(t config.Tunable) string { return t.RestartReason }, + } + covered := map[string]int{} + for i, obj := range decoded { + for name, value := range optional { + has := value(tunables[i]) != "" + _, onWire := obj[name] + switch { + case has && !onWire: + t.Errorf("%s: %q has a value but is missing from the wire", tunables[i].Key, name) + case !has && onWire: + t.Errorf("%s: %q is empty but present on the wire", tunables[i].Key, name) + case has: + covered[name]++ + } + } + } + for name := range optional { + if covered[name] == 0 { + t.Errorf("no tunable carries %q, so its wire name is not being pinned", name) + } + } + // docKeyAnchor is omitempty, so it is absent exactly for the keys documented in // prose — and present, naming a row on the reference page, for the rest. withRow := 0 diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 6f2388e..6ef5c70 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1030,20 +1030,33 @@ task gui:build && open dist/Dezhban.app row; it does not erase where the pointer is. The line must never go blank or change height either, which would reflow the row under the pointer. - Two more, both about a caption outliving what it described. With the pointer - parked on **Pause**, open a switch window from the menubar: the button under - the pointer becomes **Cancel**, and the caption must stop describing Pause - without waiting for the mouse to move. And with the pointer parked on - **Block**, make the control socket unreachable *without* stopping the daemon — - `dezhban config set control.enabled=false` then `dezhban restart` — and the - caption's password clause must follow the tooltip's rather than keeping the - answer it was given at hover-enter. - - Not by stopping the daemon: `state.isLive` goes false, Overview renders its - guided "stopped" layout, and the action row and its caption line are gone - before any of this could be observed. `routineHint` keys on - `controlIsReachable`, which is the socket, not the posture — so the socket has - to be the thing that changes. + Two more, both about a caption outliving what it described, and both needing + the pointer to stay put — so trigger them from a terminal that already has + focus, with the command pre-typed. Reaching for the menubar moves the pointer + off the control, which fires a hover-exit and clears the state the step is + trying to observe. + + With the pointer parked on **Pause**, press Return on a waiting + `sudo dezhban switch --no-wait`: the button under the pointer becomes + **Cancel**, and the caption must stop describing Pause without waiting for the + mouse to move. Leave the pointer there through the next few countdown ticks — + the caption must stay on Cancel, and must not be handed back to the pointer if + you had tabbed elsewhere first, since retitling re-establishes the tracking area + under a stationary mouse. + + Then, with the pointer parked on **Block**, run a pre-typed action that ends in + a `refreshServiceState()` — any Overview action will do — with the control + socket having gone away underneath (`control.enabled=false` plus a restart + beforehand). The caption's password clause must follow the tooltip's rather + than keeping the answer it was given at hover-enter. It has to be an action + rather than simply waiting: the 1-second timer polls the state file and + repaints only, so `controlIsReachable` changes at launch, when either surface + opens, and after an action sequence — not on a tick. + + Do not reach for either by stopping the daemon: `state.isLive` goes false, + Overview renders its guided "stopped" layout, and the action row and its + caption line are gone before any of this could be observed. `routineHint` keys + on `controlIsReachable`, which is the socket, not the posture. - [ ] **VoiceOver still hears what each button does.** With VoiceOver on, move through the action row: each control announces its short title *and* its consequence as a hint — Cancel in particular must say whether it closes the @@ -1052,10 +1065,10 @@ task gui:build && open dist/Dezhban.app - [ ] **The degraded states keep their long panic title.** With the CLI missing or the service not installed, the panic button still reads "Panic — force unblock…" — there is no caption line there to carry the explanation. -- [ ] **Routine ops are passwordless with a live daemon.** Block/Unblock and the - switch window complete over the control socket with **no** prompt, from both - the menubar and Overview; the switch countdown ticks in both surfaces and - matches. +- [ ] **Routine ops are passwordless with a live daemon.** Block/Unblock (Overview + only — they are deliberately not in the menubar) and the switch window (both + surfaces) complete over the control socket with **no** prompt; the switch + countdown ticks in both surfaces and matches. - [ ] **Pause and Resume, from both surfaces.** Pause opens with no password (`control.allowPauseOps` default true); Overview shows "Resume (m:ss left)" and the menubar "Resume now (m:ss left)" in place of the switch-window diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 7192f82..c1f7582 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -392,8 +392,18 @@ struct OverviewView: View { .accessibilityHint(hint) .onHover { inside in if inside { + // `aim` moves only on an actual transition — this control was not + // the hovered one a moment ago. `onHover(true)` also fires when a + // tracking area is re-established under a stationary pointer, and + // these controls re-measure constantly: a window's countdown + // retitles "Cancel (m:ss left)" every second. Setting `aim` + // unconditionally therefore handed the caption back to a parked + // pointer about a second after the keyboard took it — the focus + // ring and the Space key on one button, the caption describing + // another. Exactly the layout churn the movement re-arm was + // deleted for, arriving through the other door. + if hoveredAction?.id != id { aim = .pointer } hoveredAction = (id, hint) - aim = .pointer } else if hoveredAction?.id == id { hoveredAction = nil } @@ -422,11 +432,16 @@ struct OverviewView: View { } .onChange(of: hint) { newHint in // The captured string has to track the live one. `state.routineHint` - // flips on `controlIsReachable`, which a poll updates — so a hint - // captured at hover-enter went stale under a stationary pointer and - // the caption said "No password needed" while the tooltip, recomputed - // each pass, said the opposite. That disagreement is the one thing - // this wrapper exists to prevent. + // flips on `controlIsReachable`, and `.help` is recomputed every body + // pass while a hint captured at hover-enter is not — so the caption + // could say "No password needed" while the tooltip said the opposite, + // which is the one disagreement this wrapper exists to prevent. + // + // Not from the 1-second timer, which only polls the state file and + // repaints: `refreshServiceState()` runs at launch, when either + // surface opens, and after an action sequence. So the reachable + // window is narrower than "any moment" — an action completing while + // a control is hovered or focused is the realistic one. if hoveredAction?.id == id { hoveredAction = (id, newHint) } if focusedAction == id { focusedHint = newHint } } From 0c673cc07f4afe059bbf21ac46d575f6d6661689 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 23 Aug 2026 15:14:53 +0330 Subject: [PATCH 16/19] fix(gui): the explanations were wrong where it mattered most MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 30 ++++++ docs/contribute/testing.md | 46 +++++++++ .../Sources/DezhbanCore/ActionCaption.swift | 35 +++++++ gui/macos/Sources/DezhbanCore/HelpIndex.swift | 40 ++++++++ gui/macos/Sources/DezhbanCore/PostureUI.swift | 63 +++++++++++++ .../Sources/DezhbanMenu/AppDelegate.swift | 2 +- gui/macos/Sources/DezhbanMenu/HelpView.swift | 9 +- .../Sources/DezhbanMenu/OverviewView.swift | 89 +++++++++++++----- .../DezhbanCoreTests/ActionCaptionTests.swift | 38 ++++++++ .../DezhbanCoreTests/HelpIndexTests.swift | 61 ++++++++++++ .../DezhbanCoreTests/PostureUITests.swift | 93 ++++++++++++++++++- .../SettingsFieldsTests.swift | 22 ++++- internal/config/keyanchororder_test.go | 8 +- 13 files changed, 503 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5632e70..fde3c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,36 @@ current as you land changes. ### Fixed +- **A contextual help link stays on the row it landed on.** The Help pane spends + a deep link's anchor as soon as WebKit has scrolled to it, and the view update + that followed asked for the same page without the fragment — which was loaded + again, putting the reader back at the top of the page a fraction of a second + after arriving. The pane now treats a target that differs only by a dropped + fragment as the page already on screen. Links *between* anchors on one page + still navigate, and clicking the same search hit again after scrolling away + still scrolls back to it. +- **The caption no longer follows a button that moved under a resting pointer.** + When a switch window opens, Cancel replaces Pause beneath wherever the mouse + happens to be; that counted as the pointer aiming at something and took the + caption away from the keyboard's own focused button. It now asks whether the + mouse actually moved, in screen coordinates, so only a hand that went somewhere + counts. A control that arrives already holding keyboard focus also writes its + own sentence now, rather than leaving the outgoing button's on screen. +- **Unblock says what it will actually release.** The button is offered both + while egress is cut by a standing block and while the guard is holding a downed + tunnel, and it described both as releasing a manual block and resuming + monitoring. Whenever the tunnel is down — under the guard *or* under a full + block — it now warns that enforcement stops and traffic uses your real IP until + the VPN reconnects, which is what `vpn.autoArm` does; with the tunnel up it + says the full block is lifted and the guard re-blocks the exit if it is still + forbidden. +- **Panic explains itself to VoiceOver and to a tooltip.** Shortening its title + to "Panic…" left the pane's one destructive control announcing nothing but its + name, with its explanation in an adjacent label no pointer or VoiceOver user + reached from the button. It now carries that sentence as both tooltip and + accessibility hint, with the visible copy hidden from VoiceOver so it is not + read twice — the same arrangement the action row already used. + - **"Open minimized" now actually decides whether the window opens.** The app used to infer a login launch from `NSApplication.launchIsDefaultUserInfoKey`, which reported wrong in both directions — the window appeared at login with diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 6ef5c70..8b1ae07 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1017,6 +1017,22 @@ task gui:build && open dist/Dezhban.app A disabled Block or Unblock says why it is disabled rather than describing the action it will not perform. + **Unblock names what it will actually release.** The rule is the tunnel + first, the posture second, because the daemon's unblock handler branches on + `AutoArm && !tunnelUp && !standby` without looking at why egress was cut. + So check three states, not two: (a) guard holding a downed tunnel — pull the + VPN; (b) `dezhban block` with the VPN **off**, which is a full block over a + downed tunnel; (c) `dezhban block` with the VPN **up**. (a) and (b) must both + warn that enforcement stops and traffic uses your **real IP** until the VPN + reconnects (`vpn.autoArm` is on by default, so both drop the daemon to + STANDBY); only (c) may say the block is lifted and the guard re-blocks. + It may not claim the block was manual or automatic in any of them: + `postureName` derives `full-block` from `blocked` alone, so both arrive as + the same posture string and nothing on the wire tells them apart. And read + the whole caption at a narrow width — these sentences share the three-line + reservation with the row's longest existing hint, so an overlong one + truncates the password clause off the tail. + Tab through the row with Full Keyboard Access on and confirm focus drives the caption too, **including with the pointer left resting on a different button** — focus is meant to supersede a parked pointer. Then move the pointer @@ -1044,6 +1060,20 @@ task gui:build && open dist/Dezhban.app you had tabbed elsewhere first, since retitling re-establishes the tracking area under a stationary mouse. + Run that swap **once more with keyboard focus on Block first**, pointer still + parked over Pause and not moved. Cancel replaces Pause underneath it, which is + a *new* control arriving rather than the same one retitling — but the hand has + not moved, so this is not the user aiming either: the caption must stay on + **Block**, where the focus ring and the Space key are. Then move the mouse onto + any control and confirm the pointer takes it back immediately. The same applies + when an enforcement-error banner appears above the row and shifts a different + button under a stationary pointer. + + And with focus on **Pause**, open a window the same way: focus lands on the + replacement control, and the caption must describe **Cancel** rather than + keeping Pause's sentence — "uses your real ISP IP" under a button that ends a + window is the opposite of what it does. + Then, with the pointer parked on **Block**, run a pre-typed action that ends in a `refreshServiceState()` — any Overview action will do — with the control socket having gone away underneath (`control.enabled=false` plus a restart @@ -1062,6 +1092,10 @@ task gui:build && open dist/Dezhban.app consequence as a hint — Cancel in particular must say whether it closes the automatic redial window or one you opened, since the titles no longer carry that and the caption line is hidden from VoiceOver to avoid reading it twice. + **Panic… below the row too** — it is not in the action row and has no caption + line, so its own hint is the only thing that says it force-unblocks; its + visible sentence beside it is hidden from VoiceOver for the same + read-it-twice reason. Hovering Panic… must also produce a tooltip. - [ ] **The degraded states keep their long panic title.** With the CLI missing or the service not installed, the panic button still reads "Panic — force unblock…" — there is no caption line there to carry the explanation. @@ -1238,6 +1272,18 @@ traffic, so the check that matters is the one CI cannot run: with egress gone. does ("Open the documentation for …"): the key's own one-line help is already on the control beside it, so repeating it here left nothing telling a pointer user that the button navigates at all. + **And it stays there.** Watch the pane for a second after it opens: the row + must remain on screen, highlighted by `:target`. The deep link's anchor is + spent by the navigation it triggers, so the very next view update asks for the + same page with no fragment — answering that by loading again scrolled the + reader back to the top a fraction of a second after arriving + (`HelpNavigation.shouldLoad`). Then click a *different* key's **?** on the same + page and confirm it still moves: the fix must not turn into "never navigate + within a page again". Finally, search in the Help pane, click a heading hit, + scroll away by hand, and click **the same hit again** — it must scroll back. + An anchored request is always honoured, including a repeat of the anchor + already showing; suppressing it as "already there" made the second click do + nothing. - [ ] **A CLI newer than the bundled help still lands on the section.** The **?** offers the key's row first and its section second, so an app bundle predating row ids must land on the section heading rather than the top of the page. diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift index 0162882..c63b2b8 100644 --- a/gui/macos/Sources/DezhbanCore/ActionCaption.swift +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -1,3 +1,4 @@ +import CoreGraphics import Foundation /// What the single description line under the action row says. @@ -50,4 +51,38 @@ public enum ActionCaption { } return fallback } + + /// Whether a hover-enter is the user aiming at something, or a control arriving + /// under a pointer that never moved. + /// + /// `onHover(true)` cannot tell the two apart on its own: it fires both when the + /// pointer crosses into a control and when a tracking area is established + /// beneath a stationary one. Two guards, and they cover different halves: + /// + /// - `previousHoverID != id` rejects the *same* control re-firing, which these + /// controls do constantly — a window's countdown retitles "Cancel (m:ss + /// left)" every second. + /// - the pointer comparison rejects a *different* control arriving underneath, + /// which the first guard cannot see. Pause becomes Cancel when a window + /// opens, and the replacement's enter arrives with a new id while the user's + /// hand has not moved — handing the caption back to the pointer a moment + /// after the keyboard took it, the very failure the first guard was added + /// for, through the other door. + /// + /// `pointer` must be in SCREEN coordinates (`NSEvent.mouseLocation`), which is + /// what makes this different from the local-space comparison that was tried and + /// removed: a banner appearing above the row moves a control under a stationary + /// mouse, so the pointer's position *within* that control changes while the + /// mouse itself has not. On screen it has not changed, which is the question + /// being asked. + /// + /// `lastPointer` is where the pointer was at the previous hover event, enter or + /// exit — nil before any, which counts as movement so the first entry of a + /// session is never swallowed. + public static func hoverIsAim(previousHoverID: String?, id: String, + pointer: CGPoint, lastPointer: CGPoint?) -> Bool { + guard previousHoverID != id else { return false } + guard let lastPointer else { return true } + return pointer != lastPointer + } } diff --git a/gui/macos/Sources/DezhbanCore/HelpIndex.swift b/gui/macos/Sources/DezhbanCore/HelpIndex.swift index c5f81f1..c024fa0 100644 --- a/gui/macos/Sources/DezhbanCore/HelpIndex.swift +++ b/gui/macos/Sources/DezhbanCore/HelpIndex.swift @@ -292,3 +292,43 @@ public enum HelpURL { return c?.url ?? absolute } } + +/// Whether a help URL has to be handed to the web view at all, given what is +/// already showing. +/// +/// Separate from the web view — and from the executable target, which cannot be +/// `@testable import`ed — because the answer is not "are these URLs equal". A +/// deep link's 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. Answering that second ask by +/// loading again reloads the page and puts the reader back at the top — undoing +/// the scroll the "?" button exists to perform, a fraction of a second after it +/// happened. +public enum HelpNavigation { + /// True when `target` is not already on screen. + /// + /// `loaded` is the last URL handed to the web view, fragment included. + /// + /// The rule is about the *fragment*, not about equality. A target that carries + /// one is an explicit "scroll here" and is always honoured — including a repeat + /// of the anchor already showing, because between the two requests the reader + /// may have scrolled away, and clicking the same search hit again to get back is + /// the obvious way to ask. Suppressing that on "you are already there" made the + /// second click do nothing at all. + /// + /// A target with no fragment is suppressed when it names the page already + /// loaded, which is the spent-anchor follow-up this type exists for. Nothing is + /// lost: a bare request for the page on screen is a request to show what is + /// already shown. + public static func shouldLoad(target: URL, loaded: URL?) -> Bool { + guard let loaded else { return true } + // Compared absolute, for the reason `HelpURL` spends a paragraph on: a URL + // built from `Bundle.main.resourceURL` carries a base, and the same page + // reached the two ways this method sees it — freshly derived (based) and + // remembered from a previous `appendingFragment` (absolute) — is otherwise + // unequal to itself. That inequality would silently restore the reload. + let bare = HelpURL.deletingFragment(target) + if bare != target.absoluteURL { return true } // carries a fragment + return bare != HelpURL.deletingFragment(loaded) + } +} diff --git a/gui/macos/Sources/DezhbanCore/PostureUI.swift b/gui/macos/Sources/DezhbanCore/PostureUI.swift index 1cb0fd7..0f9e0c1 100644 --- a/gui/macos/Sources/DezhbanCore/PostureUI.swift +++ b/gui/macos/Sources/DezhbanCore/PostureUI.swift @@ -96,6 +96,69 @@ public enum PostureUI { return !tuns.contains(where: { $0.up }) } + /// What Unblock actually releases, which is not a manual block in the state + /// that matters most. + /// + /// Overview enables it on `blocked || guardHoldsDownedTunnel`, and the sentence + /// describing it is no longer a tooltip a reader may never see but the caption + /// under the row — the primary pre-click explanation. "Releases a manual block + /// and resumes monitoring" covered both cases and was wrong about the second: + /// + /// - A tunnel that is not up. `runner`'s unblock handler branches on + /// `AutoArm && !tunnelUp && !standby` and does **not** look at why egress + /// was cut, so this has to be asked first, for every posture. With + /// `vpn.autoArm` (default on) an explicit unblock there is read as "the VPN + /// is off on purpose": the daemon drops to STANDBY, which installs no rules + /// at all. Monitoring is precisely what does *not* resume — the guard + /// re-arms when a tunnel comes back, not before. + /// - Otherwise `blocked`: egress cut by a standing block, which the snapshot + /// reports as posture "full-block" whether an operator asked for it or a + /// blocked-country reading did — `postureName` derives it from `blocked` + /// alone and nothing on the wire tells the two apart, so this must not claim + /// either. What is true of both: the daemon restores the guard and hands the + /// geo state machine back the wheel, so a still-forbidden exit carries + /// traffic until the next reading re-escalates. + /// + /// `vpn.autoArm` is assumed on, since the snapshot does not carry it and it is + /// the default. Turned off, the daemon restores the guard instead — which with + /// no tunnel is still a total cut, so the error is a caption warning about an + /// exposure that does not happen. That is the survivable direction; promising + /// "resumes monitoring" over a drop to standby is not. + /// + /// Returned bare, without the password expectation `AppState.routineHint` + /// appends — which is also why these stay short. That suffix is 60 characters, + /// and the caption reserves three lines sized for the longest existing hint + /// (~78 bare); a sentence that overruns it truncates, and the tail it drops is + /// the password clause. + public static func unblockConsequence(_ s: Snapshot?) -> String { + guard let s = s else { return "Releases the block and resumes monitoring." } + if releasingDropsToStandby(s) { + return "Stops enforcing until the VPN reconnects — traffic uses your real IP." + } + if s.posture == "full-block" { + return "Lifts the full block; traffic uses the exit until the guard re-blocks it." + } + return "Releases the block and resumes monitoring." + } + + /// Whether an explicit unblock would land in the daemon's autoArm branch and + /// leave nothing enforcing. + /// + /// `guardHoldsDownedTunnel` answers this for the guard posture and is preferred + /// where it applies, because the daemon's own Display is authoritative there. + /// It cannot answer for FULL BLOCK: it returns false for any posture but + /// "guard", and `fullBlockDisplay` reports the blocked key whether the tunnel is + /// up or down, so the tunnel list is the only thing left to read. + /// + /// Nothing reported as up counts as down, an absent list included — the same + /// reading `guardHoldsDownedTunnel` gives an empty list, and the direction that + /// errs toward the warning rather than toward the false promise. + private static func releasingDropsToStandby(_ s: Snapshot) -> Bool { + if guardHoldsDownedTunnel(s) { return true } + guard let tuns = s.tunnels, tuns.contains(where: { $0.up }) else { return true } + return false + } + /// SwiftUI accent for a brand state — used where the bundled bitmap isn't /// (SF Symbol fallback, text highlights). public static func color(for state: String) -> Color { diff --git a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift index 476ea88..58ff7d5 100644 --- a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift +++ b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift @@ -105,7 +105,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { menu.delegate = self // We compute item enablement ourselves (see addAction); without this, AppKit's // automatic validation force-enables any item whose target responds to its - // selector, so the gating on "Block now" etc. would be ignored. + // selector, so the gating on "Pause — use my real IP" etc. would be ignored. menu.autoenablesItems = false statusItem.menu = menu watchdog.start() diff --git a/gui/macos/Sources/DezhbanMenu/HelpView.swift b/gui/macos/Sources/DezhbanMenu/HelpView.swift index ecdc438..395ea40 100644 --- a/gui/macos/Sources/DezhbanMenu/HelpView.swift +++ b/gui/macos/Sources/DezhbanMenu/HelpView.swift @@ -279,7 +279,14 @@ struct HelpWebView: NSViewRepresentable { let target = anchored.isFileURL ? anchored : url.absoluteURL defer { clearAnchor() } guard target.isFileURL, readAccess.isFileURL else { return } - guard target != loaded else { return } + // Not `target != loaded`. The anchor is one-shot (see clearAnchor), so + // every anchored load is followed one runloop turn later by an identical + // call with no anchor — which differs from `loaded` only by the dropped + // fragment, and reloading on that took the reader straight back to the + // top of the page the "?" had just scrolled into. `HelpNavigation` owns + // the rule so it can be tested; DezhbanMenu is an executable target and + // this method cannot be. + guard HelpNavigation.shouldLoad(target: target, loaded: loaded) else { return } loaded = target web.loadFileURL(target, allowingReadAccessTo: readAccess) } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index c1f7582..4c0b5fd 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -19,6 +19,10 @@ struct OverviewView: View { /// Which input the user reached for last, so focus can outrank a parked pointer /// without the pointer's position being thrown away. See `ActionCaption.Aim`. @State private var aim: ActionCaption.Aim = .pointer + /// Where the mouse was at the last hover event, in screen coordinates — what + /// tells the pointer arriving somewhere from a control arriving under it. Nil + /// until the first one. See `ActionCaption.hoverIsAim`. + @State private var lastPointer: CGPoint? @FocusState private var focusedAction: String? var body: some View { @@ -310,9 +314,15 @@ struct OverviewView: View { Button("Block") { AppActions.routine(["block"], "block") } .disabled(blocked) } + // Not one sentence for all three: this control is offered while a manual + // block is standing, while the guard holds a downed tunnel, and during + // FULL BLOCK, and it releases something different in each. See + // `PostureUI.unblockConsequence` — describing the other two as "a manual + // block" was survivable while this was a tooltip and is not now that it + // is the caption a user reads before clicking. captioned("unblock", !(blocked || guardHolds) ? "Disabled — there is no manual block or guard hold to release." - : state.routineHint("Releases a manual block and resumes monitoring.")) { + : state.routineHint(PostureUI.unblockConsequence(s))) { Button("Unblock") { AppActions.routine(["unblock"], "unblock") } .disabled(!(blocked || guardHolds)) } @@ -391,18 +401,21 @@ struct OverviewView: View { // consequence belongs, and it is announced after the label. .accessibilityHint(hint) .onHover { inside in + // Where the mouse is, in SCREEN coordinates. Recorded on every hover + // event, enter and exit, and only on hover events: a keyboard + // interaction does not move the pointer, so it must not refresh what + // "has not moved since" means. + let pointer = NSEvent.mouseLocation + defer { lastPointer = pointer } if inside { - // `aim` moves only on an actual transition — this control was not - // the hovered one a moment ago. `onHover(true)` also fires when a - // tracking area is re-established under a stationary pointer, and - // these controls re-measure constantly: a window's countdown - // retitles "Cancel (m:ss left)" every second. Setting `aim` - // unconditionally therefore handed the caption back to a parked - // pointer about a second after the keyboard took it — the focus - // ring and the Space key on one button, the caption describing - // another. Exactly the layout churn the movement re-arm was - // deleted for, arriving through the other door. - if hoveredAction?.id != id { aim = .pointer } + // Whether this is the user aiming or a control arriving under a + // stationary pointer — see `ActionCaption.hoverIsAim`, which holds + // both halves of that question and is tested. It is asked here and + // nowhere else, so a control cannot opt out of it. + if ActionCaption.hoverIsAim(previousHoverID: hoveredAction?.id, id: id, + pointer: pointer, lastPointer: lastPointer) { + aim = .pointer + } hoveredAction = (id, hint) } else if hoveredAction?.id == id { hoveredAction = nil @@ -418,15 +431,17 @@ struct OverviewView: View { // so the caption fell to the resting prompt while the pointer sat on a // button, until it moved off and back on. // - // The pointer takes the caption back by *entering* a control, the same - // one or another. There is deliberately no "the mouse moved a little" - // re-arm: every version of it read a proxy for movement rather than - // movement. `onContinuousHover`'s `.active` fires when a tracking area - // is merely established, and these controls re-measure constantly (a - // window's countdown retitles "Cancel (m:ss left)" every second); - // comparing the reported point did not help either, being in local - // space, so a banner appearing above the row moved the control under a - // stationary mouse and read as movement. Jiggling inside the control + // The pointer takes the caption back by *entering* a control it was + // not already on, having actually moved to get there + // (`ActionCaption.hoverIsAim`). There is deliberately no "the mouse + // moved a little" re-arm on top of that: `onContinuousHover`'s + // `.active` fires when a tracking area is merely established, and + // these controls re-measure constantly (a window's countdown retitles + // "Cancel (m:ss left)" every second). The point it reports is in local + // space, so a banner appearing above the row moves the control under a + // stationary mouse and reads as movement — which is why the enter + // handler compares `NSEvent.mouseLocation` instead, on the screen, + // where a hand that has not moved has not moved. Jiggling inside the control // you are already on aims at nothing new, so nothing is lost. aim = .keyboard } @@ -445,6 +460,17 @@ struct OverviewView: View { if hoveredAction?.id == id { hoveredAction = (id, newHint) } if focusedAction == id { focusedHint = newHint } } + .onAppear { + // The mirror of `onDisappear` below, and needed for the same branch + // swap. `onChange` does not fire for a view inserted already matching + // the value it observes, so a control that arrives holding focus — + // AppKit can move first responder to the replacement before SwiftUI + // runs the outgoing view's `onDisappear`, which then declines to clear + // a hint it no longer owns — never wrote its own hint. The caption was + // left describing Pause while focus sat on Cancel, whose consequence is + // the opposite one. + if focusedAction == id { focusedHint = hint } + } .onDisappear { // A control can be replaced under a stationary pointer — Pause // becomes Cancel the moment a window opens — and the removed view @@ -534,7 +560,12 @@ struct OverviewView: View { private static let switchID = "switch" private var panicRow: some View { - HStack(alignment: .firstTextBaseline, spacing: PaneMetrics.controlSpacing) { + // The sentence the shortened title no longer carries, said once and + // delivered three ways — visibly beside the button, as its tooltip, and as + // its VoiceOver hint. Held in a `let` so the three cannot drift. + let consequence = + "Force unblock: removes every dezhban firewall rule, even with dezhban not running." + return HStack(alignment: .firstTextBaseline, spacing: PaneMetrics.controlSpacing) { Button(role: .destructive) { guard AppActions.confirmPanic() else { return } AppActions.capturedPrivileged(["panic"]) { result in @@ -545,12 +576,24 @@ struct OverviewView: View { } .tint(.red) .fixedSize() - Text("Force unblock: removes every dezhban firewall rule, even with dezhban not running.") + // Same rule the action row's `captioned` applies, and for the same + // reason: shortening a title moves its explanation somewhere a pointer + // user and VoiceOver can each still reach it. Left off, the one + // destructive control on the pane announced "Panic…, button" and offered + // no tooltip at all — the gap `captioned` was written to close, open on + // the button where it costs the most. + .help(consequence) + .accessibilityHint(consequence) + Text(consequence) .font(.callout) .foregroundStyle(.secondary) // Wrap to a second line rather than truncate: this caption is // the sentence that stops someone pressing panic by accident. .fixedSize(horizontal: false, vertical: true) + // Hidden from VoiceOver because the button now carries it as a + // hint — exactly as the action row's caption line is hidden. Read + // from both, it would be announced twice. + .accessibilityHidden(true) } } diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift index 87f8cf3..927014f 100644 --- a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift @@ -1,3 +1,4 @@ +import CoreGraphics import Testing @testable import DezhbanCore @@ -36,6 +37,43 @@ struct ActionCaptionTests { fallback: "point at a button") == "release the line") } + // MARK: - hoverIsAim + + private let p1 = CGPoint(x: 100, y: 200) + private let p2 = CGPoint(x: 140, y: 200) + + /// The pointer crossing from one control to another, having moved to do it. + @Test func movingOntoAnotherControlIsAim() { + #expect(ActionCaption.hoverIsAim(previousHoverID: "block", id: "pause", + pointer: p2, lastPointer: p1)) + } + + /// The first hover of a session has nothing to compare against and must not be + /// swallowed. + @Test func theFirstHoverIsAim() { + #expect(ActionCaption.hoverIsAim(previousHoverID: nil, id: "block", + pointer: p1, lastPointer: nil)) + } + + /// The same control re-firing. These retitle every second while a window + /// counts down, and each retitle re-establishes the tracking area. + @Test func theSameControlReFiringIsNotAim() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: "cancel-window", + id: "cancel-window", + pointer: p2, lastPointer: p1)) + } + + /// The half the id check cannot see: Pause becomes Cancel when a window opens, + /// so the enter arrives with a *new* id while the hand has not moved. Read as + /// aiming, it handed the caption back to the pointer a moment after the keyboard + /// took it — the failure the id check was added for, through the other door. + @Test func aControlArrivingUnderAStationaryPointerIsNotAim() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: nil, id: "cancel-window", + pointer: p1, lastPointer: p1)) + #expect(!ActionCaption.hoverIsAim(previousHoverID: "pause", id: "cancel-window", + pointer: p1, lastPointer: p1)) + } + /// The line must never go blank between two buttons: an empty caption /// collapses its own height and reflows the row under the pointer. @Test func fallbackFillsEveryGap() { diff --git a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift index aaa731a..f95bdb8 100644 --- a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift @@ -206,3 +206,64 @@ struct HelpIndexTests { == "file:///Applications/Dezhban.app/Contents/Resources/help/usage-config.html") } } + +/// The rule that keeps a contextual "?" from undoing its own scroll. +struct HelpNavigationTests { + private let page = URL(fileURLWithPath: "/A/help/usage-config.html") + private var anchored: URL { HelpURL.appendingFragment(page, "key-vpnredialwindow") } + + @Test func nothingLoadedMeansLoad() { + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: nil)) + } + + /// The same *bare* page is not reloaded. + @Test func theSamePageIsNotReloaded() { + #expect(!HelpNavigation.shouldLoad(target: page, loaded: page)) + } + + /// The same *anchor* is. An anchored target is an explicit "scroll here", and + /// the reader may have scrolled away since — clicking the same search hit again + /// to get back is how you ask. Answering it with "you are already there" made + /// the second click do nothing at all. + @Test func repeatingAnAnchorScrollsBackToIt() { + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: anchored)) + } + + /// The regression this exists for. The anchor is spent by the navigation it + /// triggered, so the very next view update asks for the same page with no + /// fragment — and answering it with a load put the reader back at the top of a + /// forty-key reference a fraction of a second after the "?" scrolled them to + /// their key. + @Test func droppingTheSpentAnchorDoesNotReload() { + #expect(!HelpNavigation.shouldLoad(target: page, loaded: anchored)) + } + + /// The other direction still navigates: a target that names an anchor is a + /// request to scroll somewhere, including from one key's row to another's. + @Test func aDifferentAnchorOnTheSamePageStillLoads() { + #expect(HelpNavigation.shouldLoad(target: HelpURL.appendingFragment(page, "key-other"), + loaded: anchored)) + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: page)) + } + + @Test func anotherPageAlwaysLoads() { + let other = URL(fileURLWithPath: "/A/help/usage-cli.html") + #expect(HelpNavigation.shouldLoad(target: other, loaded: anchored)) + #expect(HelpNavigation.shouldLoad(target: other, loaded: page)) + } + + /// The based-URL trap `HelpURL` exists for, one layer up. `Bundle.main + /// .resourceURL` carries a base, so the page derived fresh each view update is + /// based while the one remembered from `appendingFragment` is absolute. Compared + /// as written they are unequal, and the reload — and the jump back to the top of + /// the page — comes straight back. + @Test func aBasedURLIsStillRecognisedAsTheLoadedPage() { + let app = URL(fileURLWithPath: "/Applications/Dezhban.app/", isDirectory: true) + let based = URL(string: "Contents/Resources/help/usage-config.html", relativeTo: app)! + #expect(based.baseURL != nil, "the fixture must reproduce a based URL, or it tests nothing") + + let anchoredAbsolute = HelpURL.appendingFragment(based, "key-vpnredialwindow") + #expect(!HelpNavigation.shouldLoad(target: based, loaded: anchoredAbsolute)) + #expect(!HelpNavigation.shouldLoad(target: based, loaded: based.absoluteURL)) + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift index b85ed6a..4a63660 100644 --- a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift @@ -19,10 +19,13 @@ struct PostureUITests { obj["display"] = ["key": d.key, "headline": d.headline, "detail": d.detail] } if let e = enforcementErr { obj["enforcementErr"] = e } + // nil OMITS the key, rather than writing an empty array. `tunnels` is + // `omitempty` on the wire and `[Tunnel]?` here, so absent and `[]` decode to + // two different values and are two different cases — and writing `[]` for + // nil made the test for the absent one silently exercise the empty one, + // which another test already covered. Pass `[]` for the empty case. if let tuns = tunnels { obj["tunnels"] = tuns.map { ["name": $0.name, "up": $0.up] } - } else { - obj["tunnels"] = [] } let data = try! JSONSerialization.data(withJSONObject: obj) return StateReader.decode(data)! @@ -148,6 +151,92 @@ struct PostureUITests { snapshot(posture: "guard", display: nil, tunnels: [(name: "utun4", up: true)]))) } + // MARK: - unblockConsequence + + /// Overview enables Unblock in two states and its caption is now the primary + /// pre-click explanation, so the sentence has to name what is actually being + /// released. + /// + /// `postureName` derives "full-block" from `blocked` alone, so an operator's + /// own block and a blocked-country escalation are the same string on the wire. + /// The caption may therefore claim neither — only what is true of both. + @Test func unblockConsequenceNamesTheFullBlockItLifts() { + let text = PostureUI.unblockConsequence(snapshot(posture: "full-block", + display: Self.downedGuardDisplay)) + #expect(text.contains("full block")) + #expect(!text.contains("manual")) + } + + /// The posture check may not come first. `runner`'s unblock handler branches on + /// `AutoArm && !tunnelUp && !standby` without looking at *why* egress was cut, + /// so a full block standing over a downed tunnel — `dezhban block` with the VPN + /// off, or a tunnel that dropped under a geo block, which opens no redial window + /// — drops to STANDBY exactly like the guard case does. Answering that with + /// "resumes monitoring" is the same false promise this function removed from the + /// guard branch, in the branch the first version did not cover. + @Test func unblockConsequenceWarnsWhenAFullBlockSitsOverADownedTunnel() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "full-block", display: Self.downedGuardDisplay, + tunnels: [(name: "utun4", up: false)])) + #expect(text.contains("real IP")) + #expect(!text.contains("resumes monitoring")) + } + + /// …and an absent tunnel list reads the same way. It is what the daemon sends + /// when it has none (`omitempty`), and guessing "up" there would put the false + /// promise back for the one snapshot that says least. + @Test func unblockConsequenceTreatsNoTunnelAsDown() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "full-block", display: Self.downedGuardDisplay, tunnels: nil)) + #expect(text.contains("real IP")) + } + + /// The caption reserves three lines sized for the longest hint the row already + /// had, and `AppState.routineHint` appends 60 characters to whatever this + /// returns. A longer sentence truncates, and the tail that disappears is the + /// password clause — the exact failure the three-line reservation was + /// introduced to prevent. + @Test func everyUnblockSentenceFitsTheCaption() { + let states = [ + snapshot(posture: "guard"), + snapshot(posture: "guard", display: Self.downedGuardDisplay), + snapshot(posture: "full-block", display: Self.downedGuardDisplay), + snapshot(posture: "full-block", display: Self.downedGuardDisplay, + tunnels: [(name: "utun4", up: false)]), + ] + for s in states { + let text = PostureUI.unblockConsequence(s) + #expect(text.count <= 80, "too long for the caption (\(text.count)): \(text)") + } + #expect(PostureUI.unblockConsequence(nil).count <= 80) + } + + /// With `vpn.autoArm` on — the default — an explicit unblock with the tunnel + /// down drops the daemon to STANDBY, which installs nothing. Saying + /// "resumes monitoring" there was the opposite of what happens. + @Test func unblockConsequenceNamesTheRealIPExposure() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "guard", display: Self.downedGuardDisplay)) + #expect(text.contains("real IP")) + #expect(!text.contains("resumes monitoring")) + } + + /// A healthy guard does not offer the button at all (`blocked` is false and the + /// guard is not holding), so this is the default rather than a live state — but + /// it must still be a sentence, and must not inherit either warning. + @Test func unblockConsequenceFallsBackToThePlainSentence() { + let text = PostureUI.unblockConsequence(snapshot(posture: "guard")) + #expect(text.contains("resumes monitoring")) + #expect(!text.contains("real IP")) + #expect(!text.contains("full block")) + } + + /// No snapshot is not a state the button is offered in, but the function must + /// still answer with a sentence rather than an empty caption. + @Test func unblockConsequenceHasATextForNoSnapshot() { + #expect(!PostureUI.unblockConsequence(nil).isEmpty) + } + // MARK: - dockState / mmss / agoString @Test func dockStateCoarsensToBlockedOrOn() { diff --git a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift index b940b34..7aa75f5 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift @@ -5,8 +5,9 @@ import Testing /// Builds a schema good enough to exercise the pane's use of it, without /// shelling out to the CLI. Kinds and labels match what `config schema --json` /// reports for these keys. -/// Mirrors Go's `config.anchorSlug` / `help.Anchor`: lowercase, then keep only -/// letters, digits and hyphens — so a dot is **dropped**, not turned into a hyphen. +/// Mirrors Go's `config.anchorSlug` / `help.Anchor`: lowercase, keep ASCII +/// letters, digits and hyphens, turn a space into a hyphen, drop everything else — +/// so a dot is **dropped**, not turned into a hyphen. /// /// Spelled out here because the fixture is the app-side reference for a /// cross-language contract, and it had the rule wrong: it mapped "." to "-" and @@ -15,8 +16,23 @@ import Testing /// person deriving an anchor from this fixture would have got a fragment resolving /// nowhere. Go's TestKeyAnchorSlugMatchesTheRenderer pins the two derivations that /// actually ship; this keeps the fixture honest about them. +/// +/// Written over `unicodeScalars` with explicit ASCII ranges rather than +/// `isLowercase`/`isNumber`, which was the same near-miss one layer down: those +/// accept letters and digits outside ASCII that Go's `r >= '0' && r <= '9'` +/// rejects, and neither spelling handled the space at all. No config key exercises +/// either today — which is precisely why the fixture, not a key, has to carry the +/// rule correctly. private func goAnchorSlug(_ key: String) -> String { - key.lowercased().filter { $0.isLowercase && $0.isASCII || $0.isNumber || $0 == "-" } + var out = "" + for scalar in key.lowercased().unicodeScalars { + switch scalar { + case "a"..."z", "0"..."9", "-": out.unicodeScalars.append(scalar) + case " ": out.append("-") + default: break + } + } + return out } private func testSchema() -> ConfigSchema { diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go index 9500a55..d37a962 100644 --- a/internal/config/keyanchororder_test.go +++ b/internal/config/keyanchororder_test.go @@ -75,9 +75,11 @@ func headsAKeyTable(headerLine string) bool { // the reference's definition headings. Nothing else notices if that changes: the // resolution test stays green as long as some row carries the anchor. func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { - // The pages come from the anchors themselves rather than being hardcoded, so a - // tunable documented on some other bundled page is checked against that page - // instead of being reported as having no row at all. + // Read out of the anchors rather than restated here. Today that always yields + // exactly one page — docKeyAnchorFor builds every anchor from keyReferencePage — + // so this is not extra coverage; it is the test declining to hold a second copy + // of a constant it does not own. If the reference is ever split across pages, + // this reads the split instead of failing on it. pages := map[string]bool{} for _, tun := range Tunables() { if tun.DocKeyAnchor == "" { From a52bab418e4513a92b557a7985369eef3eaea4c4 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 23 Aug 2026 15:20:55 +0330 Subject: [PATCH 17/19] fix(gui): measure movement from when the keyboard took the caption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/contribute/testing.md | 32 ++++++++++----- .../Sources/DezhbanCore/ActionCaption.swift | 20 +++++++--- gui/macos/Sources/DezhbanCore/HelpIndex.swift | 25 ++++++++---- gui/macos/Sources/DezhbanMenu/HelpView.swift | 15 ++++++- .../Sources/DezhbanMenu/OverviewView.swift | 36 +++++++++++------ .../DezhbanCoreTests/ActionCaptionTests.swift | 39 ++++++++++++++----- .../DezhbanCoreTests/HelpIndexTests.swift | 34 +++++++++++----- internal/config/keyanchororder_test.go | 20 +++++++++- 8 files changed, 165 insertions(+), 56 deletions(-) diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 8b1ae07..a968b02 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -1061,13 +1061,23 @@ task gui:build && open dist/Dezhban.app under a stationary mouse. Run that swap **once more with keyboard focus on Block first**, pointer still - parked over Pause and not moved. Cancel replaces Pause underneath it, which is - a *new* control arriving rather than the same one retitling — but the hand has - not moved, so this is not the user aiming either: the caption must stay on - **Block**, where the focus ring and the Space key are. Then move the mouse onto - any control and confirm the pointer takes it back immediately. The same applies - when an enforcement-error banner appears above the row and shifts a different - button under a stationary pointer. + parked over Pause. Cancel replaces Pause underneath it, which is a *new* + control arriving rather than the same one retitling — but the hand has not + moved since Tab, so this is not the user aiming either: the caption must stay + on **Block**, where the focus ring and the Space key are. Then move the mouse + onto any control and confirm the pointer takes it back immediately. The same + applies when an enforcement-error banner appears above the row and shifts a + different button under a stationary pointer. + + Do that one twice more, because the reference point is *where the mouse was + when Tab moved the focus*, not where it was at the last hover event — and the + two differ in both directions. First **nudge the pointer a few points inside + Pause before tabbing** (no hover event fires, so a reading taken at the last + boundary would be stale): the caption must still stay on Block when Cancel + arrives. Then, with focus on Block, **flick the mouse quickly from one button + to the next** rather than easing across: the exit and the enter can be + dispatched from a single mouse-moved event, and the pointer must still take the + caption back. And with focus on **Pause**, open a window the same way: focus lands on the replacement control, and the caption must describe **Cancel** rather than @@ -1281,9 +1291,13 @@ traffic, so the check that matters is the one CI cannot run: with egress gone. page and confirm it still moves: the fix must not turn into "never navigate within a page again". Finally, search in the Help pane, click a heading hit, scroll away by hand, and click **the same hit again** — it must scroll back. - An anchored request is always honoured, including a repeat of the anchor + An anchored request is honoured again on a repeat, including of the anchor already showing; suppressing it as "already there" made the second click do - nothing. + nothing. It must not be honoured *twice in one breath*, though: watch the pane + for a flash or a half-drawn page as it opens, which is a second `loadFileURL` + cancelling the first — `makeNSView` and `updateNSView` both run in the turn the + anchor is still pending, and only the served-anchor record tells that duplicate + from a real repeat. - [ ] **A CLI newer than the bundled help still lands on the section.** The **?** offers the key's row first and its section second, so an app bundle predating row ids must land on the section heading rather than the top of the page. diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift index c63b2b8..f15d5a2 100644 --- a/gui/macos/Sources/DezhbanCore/ActionCaption.swift +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -76,13 +76,21 @@ public enum ActionCaption { /// mouse itself has not. On screen it has not changed, which is the question /// being asked. /// - /// `lastPointer` is where the pointer was at the previous hover event, enter or - /// exit — nil before any, which counts as movement so the first entry of a - /// session is never swallowed. + /// `keyboardAimedAt` is where the pointer was when the keyboard last took the + /// caption, and nil when it has not. That is the reference point, NOT the + /// pointer's position at the previous hover event, which was the first shape of + /// this and was wrong in both directions. `onHover` fires on enter and exit + /// only, so a reading taken there is the position at the last boundary crossing: + /// a hand that drifted a few points while reading (no hover event) then made a + /// swap look like movement, and a fast flick across the gap between two buttons + /// — whose exit and enter are dispatched from a single mouse-moved event, so + /// both sample the same location — looked like stillness and silently refused to + /// take the caption back. Anchored to the moment the keyboard took over, the + /// question is the one actually being asked: has the hand gone anywhere since? public static func hoverIsAim(previousHoverID: String?, id: String, - pointer: CGPoint, lastPointer: CGPoint?) -> Bool { + pointer: CGPoint, keyboardAimedAt: CGPoint?) -> Bool { guard previousHoverID != id else { return false } - guard let lastPointer else { return true } - return pointer != lastPointer + guard let keyboardAimedAt else { return true } + return pointer != keyboardAimedAt } } diff --git a/gui/macos/Sources/DezhbanCore/HelpIndex.swift b/gui/macos/Sources/DezhbanCore/HelpIndex.swift index c024fa0..481e9ba 100644 --- a/gui/macos/Sources/DezhbanCore/HelpIndex.swift +++ b/gui/macos/Sources/DezhbanCore/HelpIndex.swift @@ -310,25 +310,34 @@ public enum HelpNavigation { /// `loaded` is the last URL handed to the web view, fragment included. /// /// The rule is about the *fragment*, not about equality. A target that carries - /// one is an explicit "scroll here" and is always honoured — including a repeat - /// of the anchor already showing, because between the two requests the reader - /// may have scrolled away, and clicking the same search hit again to get back is - /// the obvious way to ask. Suppressing that on "you are already there" made the - /// second click do nothing at all. + /// one is an explicit "scroll here" and is honoured again on a repeat, because + /// between the two requests the reader may have scrolled away, and clicking the + /// same search hit again to get back is the obvious way to ask. Suppressing that + /// on "you are already there" made the second click do nothing at all. + /// + /// `servedAnchor` is what keeps "again" from meaning "twice in one breath". + /// `NSViewRepresentable` calls `updateNSView` immediately after `makeNSView`, in + /// the same runloop turn, while the anchor is still pending — it is cleared + /// asynchronously — so opening the pane on a deep link asks for the identical + /// anchored URL twice, and honouring both started a second load that cancelled + /// the first mid-flight. The caller records what it just served and clears it on + /// the following bare call, so a genuine repeat click (which arrives after that + /// clear) still navigates and the same-turn duplicate does not. /// /// A target with no fragment is suppressed when it names the page already /// loaded, which is the spent-anchor follow-up this type exists for. Nothing is /// lost: a bare request for the page on screen is a request to show what is /// already shown. - public static func shouldLoad(target: URL, loaded: URL?) -> Bool { - guard let loaded else { return true } + public static func shouldLoad(target: URL, loaded: URL?, servedAnchor: URL?) -> Bool { // Compared absolute, for the reason `HelpURL` spends a paragraph on: a URL // built from `Bundle.main.resourceURL` carries a base, and the same page // reached the two ways this method sees it — freshly derived (based) and // remembered from a previous `appendingFragment` (absolute) — is otherwise // unequal to itself. That inequality would silently restore the reload. + let here = target.absoluteURL let bare = HelpURL.deletingFragment(target) - if bare != target.absoluteURL { return true } // carries a fragment + if bare != here { return here != servedAnchor } // carries a fragment + guard let loaded else { return true } return bare != HelpURL.deletingFragment(loaded) } } diff --git a/gui/macos/Sources/DezhbanMenu/HelpView.swift b/gui/macos/Sources/DezhbanMenu/HelpView.swift index 395ea40..8eaada6 100644 --- a/gui/macos/Sources/DezhbanMenu/HelpView.swift +++ b/gui/macos/Sources/DezhbanMenu/HelpView.swift @@ -254,6 +254,11 @@ struct HelpWebView: NSViewRepresentable { var parent: HelpWebView /// What was last handed to the web view, fragment included. private var loaded: URL? + /// The anchored URL served in the episode still in progress, cleared by the + /// bare call that ends it. Distinct from `loaded`, which never forgets — this + /// one exists only to tell a repeat click apart from the same-turn duplicate + /// `makeNSView` + `updateNSView` produce. See `HelpNavigation.shouldLoad`. + private var servedAnchor: URL? init(_ parent: HelpWebView) { self.parent = parent @@ -286,7 +291,15 @@ struct HelpWebView: NSViewRepresentable { // top of the page the "?" had just scrolled into. `HelpNavigation` owns // the rule so it can be tested; DezhbanMenu is an executable target and // this method cannot be. - guard HelpNavigation.shouldLoad(target: target, loaded: loaded) else { return } + // + // That bare follow-up is also what ends an anchored episode: it is the + // one call that can only arrive after the pending anchor was cleared, so + // clearing `servedAnchor` here is what lets a later click on the SAME + // anchor navigate again while the same-turn duplicate below does not. + if anchor == nil { servedAnchor = nil } + guard HelpNavigation.shouldLoad(target: target, loaded: loaded, + servedAnchor: servedAnchor) else { return } + if anchor != nil { servedAnchor = target.absoluteURL } loaded = target web.loadFileURL(target, allowingReadAccessTo: readAccess) } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 4c0b5fd..80b8081 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -19,10 +19,12 @@ struct OverviewView: View { /// Which input the user reached for last, so focus can outrank a parked pointer /// without the pointer's position being thrown away. See `ActionCaption.Aim`. @State private var aim: ActionCaption.Aim = .pointer - /// Where the mouse was at the last hover event, in screen coordinates — what - /// tells the pointer arriving somewhere from a control arriving under it. Nil - /// until the first one. See `ActionCaption.hoverIsAim`. - @State private var lastPointer: CGPoint? + /// Where the mouse was when the keyboard last took the caption, in screen + /// coordinates — the reference that tells the pointer arriving somewhere from a + /// control arriving under a pointer that has not moved. Nil while the keyboard + /// has not taken it, and cleared once the pointer has proved it moved. See + /// `ActionCaption.hoverIsAim`. + @State private var keyboardAimPoint: CGPoint? @FocusState private var focusedAction: String? var body: some View { @@ -401,20 +403,26 @@ struct OverviewView: View { // consequence belongs, and it is announced after the label. .accessibilityHint(hint) .onHover { inside in - // Where the mouse is, in SCREEN coordinates. Recorded on every hover - // event, enter and exit, and only on hover events: a keyboard - // interaction does not move the pointer, so it must not refresh what - // "has not moved since" means. - let pointer = NSEvent.mouseLocation - defer { lastPointer = pointer } if inside { // Whether this is the user aiming or a control arriving under a // stationary pointer — see `ActionCaption.hoverIsAim`, which holds // both halves of that question and is tested. It is asked here and // nowhere else, so a control cannot opt out of it. + // + // The reference is where the mouse was when the KEYBOARD took the + // caption, in screen coordinates, not where it was at the last + // hover event. `onHover` fires on enter and exit only, so a + // reading taken here is the last boundary crossing rather than + // the hand's actual position, and both errors follow from that. if ActionCaption.hoverIsAim(previousHoverID: hoveredAction?.id, id: id, - pointer: pointer, lastPointer: lastPointer) { + pointer: NSEvent.mouseLocation, + keyboardAimedAt: keyboardAimPoint) { aim = .pointer + // Spent. The pointer has demonstrably moved since the keyboard + // took over, so there is nothing left to measure against — and + // a point kept past that would suppress a later entry that + // happened to land on the same pixel. + keyboardAimPoint = nil } hoveredAction = (id, hint) } else if hoveredAction?.id == id { @@ -444,6 +452,12 @@ struct OverviewView: View { // where a hand that has not moved has not moved. Jiggling inside the control // you are already on aims at nothing new, so nothing is lost. aim = .keyboard + // Where the mouse is at this instant, which is the reference the enter + // handler measures against. Sampled here rather than at each hover + // event on purpose: the question a later enter has to answer is "has + // the hand gone anywhere since the keyboard took over", and that is + // the only moment at which the answer's baseline exists. + keyboardAimPoint = NSEvent.mouseLocation } .onChange(of: hint) { newHint in // The captured string has to track the live one. `state.routineHint` diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift index 927014f..83b4d1b 100644 --- a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift @@ -42,17 +42,18 @@ struct ActionCaptionTests { private let p1 = CGPoint(x: 100, y: 200) private let p2 = CGPoint(x: 140, y: 200) - /// The pointer crossing from one control to another, having moved to do it. + /// The pointer crossing from one control to another, having moved since the + /// keyboard took the caption. @Test func movingOntoAnotherControlIsAim() { #expect(ActionCaption.hoverIsAim(previousHoverID: "block", id: "pause", - pointer: p2, lastPointer: p1)) + pointer: p2, keyboardAimedAt: p1)) } - /// The first hover of a session has nothing to compare against and must not be - /// swallowed. - @Test func theFirstHoverIsAim() { + /// The keyboard never took the caption, so there is nothing for the pointer to + /// take it back from and no reading to swallow. + @Test func aHoverIsAimWhileTheKeyboardHasNotTakenIt() { #expect(ActionCaption.hoverIsAim(previousHoverID: nil, id: "block", - pointer: p1, lastPointer: nil)) + pointer: p1, keyboardAimedAt: nil)) } /// The same control re-firing. These retitle every second while a window @@ -60,7 +61,7 @@ struct ActionCaptionTests { @Test func theSameControlReFiringIsNotAim() { #expect(!ActionCaption.hoverIsAim(previousHoverID: "cancel-window", id: "cancel-window", - pointer: p2, lastPointer: p1)) + pointer: p2, keyboardAimedAt: p1)) } /// The half the id check cannot see: Pause becomes Cancel when a window opens, @@ -69,9 +70,29 @@ struct ActionCaptionTests { /// took it — the failure the id check was added for, through the other door. @Test func aControlArrivingUnderAStationaryPointerIsNotAim() { #expect(!ActionCaption.hoverIsAim(previousHoverID: nil, id: "cancel-window", - pointer: p1, lastPointer: p1)) + pointer: p1, keyboardAimedAt: p1)) #expect(!ActionCaption.hoverIsAim(previousHoverID: "pause", id: "cancel-window", - pointer: p1, lastPointer: p1)) + pointer: p1, keyboardAimedAt: p1)) + } + + /// The reference is the moment the keyboard took over, not the last hover + /// event — which is why a hand that drifted a few points while reading, with no + /// hover event to record it, does not make the next swap look like aiming. Here + /// the pointer sits where it was when Tab moved the focus, having wandered + /// within the control since the enter that first put it there. + @Test func driftingInsideAControlDoesNotMakeALaterSwapLookLikeAiming() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: nil, id: "cancel-window", + pointer: p2, keyboardAimedAt: p2)) + } + + /// And the other direction the old reference got wrong: a flick across the gap + /// between two buttons dispatches the exit and the enter from one mouse-moved + /// event, so both read the same location. Measured against the keyboard's + /// moment instead, the hand has plainly moved and the pointer takes the caption + /// back. + @Test func aFlickBetweenButtonsTakesTheCaptionBack() { + #expect(ActionCaption.hoverIsAim(previousHoverID: "block", id: "pause", + pointer: p2, keyboardAimedAt: p1)) } /// The line must never go blank between two buttons: an empty caption diff --git a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift index f95bdb8..b9b873c 100644 --- a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift @@ -213,12 +213,12 @@ struct HelpNavigationTests { private var anchored: URL { HelpURL.appendingFragment(page, "key-vpnredialwindow") } @Test func nothingLoadedMeansLoad() { - #expect(HelpNavigation.shouldLoad(target: anchored, loaded: nil)) + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: nil, servedAnchor: nil)) } /// The same *bare* page is not reloaded. @Test func theSamePageIsNotReloaded() { - #expect(!HelpNavigation.shouldLoad(target: page, loaded: page)) + #expect(!HelpNavigation.shouldLoad(target: page, loaded: page, servedAnchor: nil)) } /// The same *anchor* is. An anchored target is an explicit "scroll here", and @@ -226,7 +226,21 @@ struct HelpNavigationTests { /// to get back is how you ask. Answering it with "you are already there" made /// the second click do nothing at all. @Test func repeatingAnAnchorScrollsBackToIt() { - #expect(HelpNavigation.shouldLoad(target: anchored, loaded: anchored)) + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: anchored, servedAnchor: nil)) + } + + /// …but not twice in one breath. `NSViewRepresentable` calls `updateNSView` + /// immediately after `makeNSView`, in the same runloop turn, while the anchor is + /// still pending — so opening the pane on a deep link asks for the identical + /// anchored URL twice, and honouring both started a load that cancelled the + /// first mid-flight. The caller records what it just served; the bare follow-up + /// clears it, which is what keeps the repeat click above working. + @Test func theSameTurnDuplicateIsNotServedTwice() { + #expect(!HelpNavigation.shouldLoad(target: anchored, loaded: anchored, + servedAnchor: anchored)) + // A different anchor is a different request, pending episode or not. + #expect(HelpNavigation.shouldLoad(target: HelpURL.appendingFragment(page, "key-other"), + loaded: anchored, servedAnchor: anchored)) } /// The regression this exists for. The anchor is spent by the navigation it @@ -235,21 +249,21 @@ struct HelpNavigationTests { /// forty-key reference a fraction of a second after the "?" scrolled them to /// their key. @Test func droppingTheSpentAnchorDoesNotReload() { - #expect(!HelpNavigation.shouldLoad(target: page, loaded: anchored)) + #expect(!HelpNavigation.shouldLoad(target: page, loaded: anchored, servedAnchor: nil)) } /// The other direction still navigates: a target that names an anchor is a /// request to scroll somewhere, including from one key's row to another's. @Test func aDifferentAnchorOnTheSamePageStillLoads() { #expect(HelpNavigation.shouldLoad(target: HelpURL.appendingFragment(page, "key-other"), - loaded: anchored)) - #expect(HelpNavigation.shouldLoad(target: anchored, loaded: page)) + loaded: anchored, servedAnchor: nil)) + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: page, servedAnchor: nil)) } @Test func anotherPageAlwaysLoads() { let other = URL(fileURLWithPath: "/A/help/usage-cli.html") - #expect(HelpNavigation.shouldLoad(target: other, loaded: anchored)) - #expect(HelpNavigation.shouldLoad(target: other, loaded: page)) + #expect(HelpNavigation.shouldLoad(target: other, loaded: anchored, servedAnchor: nil)) + #expect(HelpNavigation.shouldLoad(target: other, loaded: page, servedAnchor: nil)) } /// The based-URL trap `HelpURL` exists for, one layer up. `Bundle.main @@ -263,7 +277,7 @@ struct HelpNavigationTests { #expect(based.baseURL != nil, "the fixture must reproduce a based URL, or it tests nothing") let anchoredAbsolute = HelpURL.appendingFragment(based, "key-vpnredialwindow") - #expect(!HelpNavigation.shouldLoad(target: based, loaded: anchoredAbsolute)) - #expect(!HelpNavigation.shouldLoad(target: based, loaded: based.absoluteURL)) + #expect(!HelpNavigation.shouldLoad(target: based, loaded: anchoredAbsolute, servedAnchor: nil)) + #expect(!HelpNavigation.shouldLoad(target: based, loaded: based.absoluteURL, servedAnchor: nil)) } } diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go index d37a962..d8259c9 100644 --- a/internal/config/keyanchororder_test.go +++ b/internal/config/keyanchororder_test.go @@ -50,16 +50,32 @@ func headsAKeyTable(headerLine string) bool { if m == nil { return false } - cell := strings.Map(func(r rune) rune { + // Links and images reduce to their text FIRST, then emphasis characters go. + // Both steps, because stripInline does both: with only the second, a header + // written `| [Field](../x.md) |` makes the renderer mint row anchors for that + // table while this file stops treating it as a key table — the coverage + // narrowing silently, and the `checked == 0` fatal no help because it fires + // only when EVERY key falls out, never a subset. The same near-miss the + // comment above warns about, one construct over. + cell := inlineImageText.ReplaceAllString(m[1], "$1") + cell = inlineLinkText.ReplaceAllString(cell, "$1") + cell = strings.Map(func(r rune) rune { switch r { case '`', '*', '_': return -1 } return r - }, m[1]) + }, cell) return strings.EqualFold(strings.TrimSpace(cell), "Field") } +// Mirroring help's own inlineImage/inlineLink, image first so `![a](b)` does not +// reduce to a stray `!`. +var ( + inlineImageText = regexp.MustCompile(`!\[([^\]]*)\]\([^)]+\)`) + inlineLinkText = regexp.MustCompile(`\[([^\]]*)\]\([^)]+\)`) +) + // TestKeyRowsAnchorToTheirDefinitionSection pins the document arrangement that // help.claimKey depends on. // From 96fb299e2a32d4f44420556a6fc5e842fc4b6400 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 23 Aug 2026 15:31:17 +0330 Subject: [PATCH 18/19] test(runner): pin the invariant, not the number of probe ticks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/runner/runner_test.go | 58 +++++++++++++++++++++++++++------- 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 353e3d7..66b7c25 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -330,17 +330,53 @@ func TestVPNHoldsFullBlockOnProbeError(t *testing.T) { } // startup guard; IR → full block; each blocked tick probes (lift+re-cut) // but an error never restores guard. - want := []string{ - "apply-guard", // startup guard - "apply-fullblock", // IR → FULL BLOCK - "apply-guard", // probe 1 lift - "apply-fullblock", // probe 1 re-cut (error → hold block) - "apply-guard", // probe 2 lift - "apply-fullblock", // probe 2 re-cut (error → hold block) - "cleanup", - } - if !equal(be.calls, want) { - t.Fatalf("calls = %v, want %v (a probe error must not lift FULL BLOCK)", be.calls, want) + // + // The SHAPE, not an exact tick count. Asserting the literal seven-call + // sequence made this test depend on a race it does not test: fakeMonitor.Once + // cancels while handing out the last result, and the loop then has both + // ctx.Done() and a 1ms geo ticker ready at its next select — which picks + // uniformly among ready cases, so one more probe cycle is a coin flip whenever + // processing the previous one took longer than the interval. That is + // vanishingly rare on a fast machine and ordinary under -race on a loaded CI + // runner, where it failed with one extra "apply-guard apply-fullblock" pair. + // + // What the test is actually for survives intact and is now stated directly: a + // probe lift is ALWAYS followed by a re-cut, so a probe error never leaves the + // guard standing in place of a full block. An extra probe cycle satisfies that + // as fully as the expected number does; a lifted block does not, at any count. + assertProbeNeverLiftsTheBlock(t, be.calls) +} + +// assertProbeNeverLiftsTheBlock pins the call sequence of a daemon that entered +// FULL BLOCK and stayed there: the startup guard, the escalation, then some whole +// number of lift-and-probe pairs, then cleanup. Nothing may end on a lift. +// +// Timing-independent by construction — see the caller for why an exact length +// cannot be. The lower bound is what keeps it from passing vacuously if probing +// stops happening at all. +func assertProbeNeverLiftsTheBlock(t *testing.T, calls []string) { + t.Helper() + if n := len(calls); n < 7 { + t.Fatalf("calls = %v: want the startup guard, the escalation, at least two "+ + "probe pairs and cleanup", calls) + } + if last := calls[len(calls)-1]; last != "cleanup" { + t.Fatalf("calls = %v: last call is %q, want cleanup", calls, last) + } + body := calls[:len(calls)-1] + if len(body)%2 != 0 { + t.Fatalf("calls = %v: %d calls before cleanup is odd, so a lift went "+ + "un-recut — a probe error must not lift FULL BLOCK", calls, len(body)) + } + for i, call := range body { + want := "apply-fullblock" + if i%2 == 0 { + want = "apply-guard" + } + if call != want { + t.Fatalf("calls = %v: call %d is %q, want %q (a probe error must not "+ + "lift FULL BLOCK)", calls, i, call, want) + } } } From 739e2ac43758acb9e932fba8e47722432775681a Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sun, 23 Aug 2026 15:36:27 +0330 Subject: [PATCH 19/19] fix(runner): a cancelled context ends the loop, it does not buy a reading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 10 ++++ docs/contribute/testing.md | 8 ++++ internal/runner/runner.go | 18 +++++++ internal/runner/runner_test.go | 85 ++++++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fde3c1d..9b139b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,16 @@ current as you land changes. ### Fixed +- **Shutting the daemon down no longer buys one last lift-and-probe.** When + FULL BLOCK is held and the tunnel-scoped provider pass cannot be built (no + provider addresses resolved), the recovery probe lifts the guard, looks up + the exit country, and re-cuts. The run loop could take one of those on the + way out: with a cancelled context and a pending geo tick both ready, the + select chose between them at random. Losing that toss meant a stop briefly + opened egress through the forbidden-country exit — to observe a country + nothing was left to act on, moments before teardown removed the rules + anyway. A cancelled context now ends the loop instead, the same as any + other path out. - **A contextual help link stays on the row it landed on.** The Help pane spends a deep link's anchor as soon as WebKit has scrolled to it, and the view update that followed asked for the same page without the fragment — which was loaded diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index a968b02..adda918 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -213,6 +213,14 @@ Only a live host can prove these — CI cannot reach a printer. IP resolves → the daemon logs that recovery will briefly lift the guard, and recovery still works via lift-and-probe. A FULL BLOCK that can never observe its way out would be worse than the leak. +- [ ] **Shutdown takes no last probe.** Still on the unresolvable `providers` + above — the only configuration in which a probe lifts anything — hold FULL + BLOCK and stop the daemon (`Ctrl-C` in the foreground, or + `sudo dezhban stop`) with `pfctl -a dezhban -sr` polling in another shell. + The ruleset must go straight from FULL BLOCK to torn down: no lift-and-recut + in between. Buying a reading on the way out opens egress through the very + exit the block exists for, to observe a country nothing is left to act on. + `TestShutdownTakesNoProbeTick` covers the decision; only this shows the rules. ## Country check (exit country, not physical location) diff --git a/internal/runner/runner.go b/internal/runner/runner.go index ed17604..1a41ae4 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -3016,6 +3016,24 @@ func (o Options) runGuard(ctx context.Context) error { snapshot() } case <-geoTick.C: + // The daemon is going down: Cleanup is deferred and about to remove + // every rule, so a reading taken now can change nothing. Returning is + // exactly what the ctx.Done() case above does — this only stops the + // answer depending on which of two ready cases select happened to pick, + // since it chooses uniformly among them and the ticker is ready whenever + // the previous tick's work outran the interval. + // + // It matters because a tick taken while blocked is not a passive read. + // `probe`'s fallback path — no provider addresses resolved, so the + // tunnel-scoped pass cannot be built — LIFTS the guard, looks, and + // re-cuts. On the way out of FULL BLOCK that is a shutdown that briefly + // opens egress through the forbidden-country exit, to observe a country + // nobody is left to act on. Same reasoning as the manualBlock and + // panic-disarm branches below: when recovery cannot act on what it + // learns, it must not pay a lift to learn it. + if ctx.Err() != nil { + return nil + } // Any state that suspends the geo state machine also ends an // accelerated episode: probing faster is pointless when no reading // will be taken, and this is the one place every such state is diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 66b7c25..d154067 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -380,6 +380,91 @@ func assertProbeNeverLiftsTheBlock(t *testing.T, calls []string) { } } +// slowLastMonitor cancels while handing out its last result and then stays inside +// Once past the geo interval, so the ticker is GUARANTEED ready when the loop +// returns to its select. That is the state the run loop's ctx.Err() check exists +// for, and the only way to reach it on purpose: with both cases ready, select +// picks uniformly, so the scenario is otherwise a coin flip that a fast machine +// almost never loses. +type slowLastMonitor struct { + results []monitor.Result + idx int + cancel context.CancelFunc + stall time.Duration +} + +func (m *slowLastMonitor) Poll(ctx context.Context) <-chan monitor.Result { + ch := make(chan monitor.Result) + close(ch) + return ch +} + +func (m *slowLastMonitor) Once(context.Context) (monitor.Reading, error) { + if m.idx >= len(m.results) { + if m.cancel != nil { + m.cancel() + } + return monitor.Reading{}, context.Canceled + } + r := m.results[m.idx] + m.idx++ + if m.idx >= len(m.results) && m.cancel != nil { + m.cancel() + // Outlast the interval, so the tick that must NOT be taken is pending + // rather than merely possible. + time.Sleep(m.stall) + } + return r.Reading, r.Err +} + +// TestShutdownTakesNoProbeTick pins that a cancelled context ends the loop instead +// of buying one more reading. +// +// A geo tick taken while blocked is not a passive read: with no provider addresses +// resolved — the fixture here, as in every runner test — `probe` lifts the guard, +// looks, and re-cuts. Taking one on the way down means a shutdown that briefly +// opens egress through the forbidden-country exit to observe a country nobody is +// left to act on. So the calls must end at the last real probe's re-cut, with +// nothing between it and cleanup. +// +// Repeated, deliberately. With the check in place this is exact every time; remove +// it and each run is an independent coin flip, so the repetition turns "usually +// caught" into "caught". The failure it guards is otherwise invisible on a +// developer machine and shows up as a flake in CI, which is where it was found. +func TestShutdownTakesNoProbeTick(t *testing.T) { + for i := range 12 { + be := &fakeBackend{} + ctx, cancel := context.WithCancel(context.Background()) + o := Options{ + Monitor: &slowLastMonitor{cancel: cancel, stall: 20 * time.Millisecond, results: []monitor.Result{ + reading("IR"), // enter FULL BLOCK + failResult(), // probe error → hold block + }}, + Decider: decision.New([]string{"IR"}, 1), + Backend: be, + Log: discardLog(), + Interval: time.Millisecond, + Tunnels: []string{"utun4"}, + Endpoints: []netip.Addr{netip.MustParseAddr("203.0.113.7")}, + } + if err := Run(ctx, o); err != nil { + t.Fatal(err) + } + want := []string{ + "apply-guard", // startup guard + "apply-fullblock", // IR → FULL BLOCK + "apply-guard", // probe lift + "apply-fullblock", // probe re-cut (error → hold block) + "cleanup", + } + if !equal(be.calls, want) { + t.Fatalf("run %d: calls = %v, want %v (shutdown must not buy one more "+ + "lift-and-probe, which opens egress through the blocked exit)", + i, be.calls, want) + } + } +} + func TestVPNStartupGuardFailureAborts(t *testing.T) { be := &failingGuardBackend{} o := Options{
" + renderInline(c) + "