diff --git a/Tests/StackNudgePanelCoreTests/ExtensionHostTests.swift b/Tests/StackNudgePanelCoreTests/ExtensionHostTests.swift index b5d6802..3ce5797 100644 --- a/Tests/StackNudgePanelCoreTests/ExtensionHostTests.swift +++ b/Tests/StackNudgePanelCoreTests/ExtensionHostTests.swift @@ -396,6 +396,26 @@ final class ExtensionHostTests: XCTestCase { visible: true, now: now)) } + // A click on a footer hint has to send what the keypress would. resolve() + // reads a row action as belonging to the selected row and a document action + // as belonging to none, so the pairing the footer renders must agree — a + // document action sent with a row id would spawn `--action refresh --row x` + // for a row the action was never about. + func testADocumentActionCarriesNoRowAndARowActionCarriesItsOwn() { + let json = """ + {"schema":1, + "rows":[{"id":"h1","title":"A","actions":[{"id":"open","label":"Open","key":"o"}]}], + "actions":[{"id":"refresh","label":"Sync now","key":"r"}]} + """ + guard case .success(let document) = ExtensionDocument.parse(Data(json.utf8)) else { + return XCTFail("fixture didn't parse") + } + XCTAssertEqual(ExtensionHost.resolve(key: "r", in: document, selectedRow: "h1")?.row, nil) + XCTAssertEqual(ExtensionHost.resolve(key: "o", in: document, selectedRow: "h1")?.row, "h1") + // With nothing selected a row action isn't reachable at all. + XCTAssertNil(ExtensionHost.resolve(key: "o", in: document, selectedRow: nil)) + } + // MARK: - Opening a tab func testOpeningATabRefreshesItUnlessTheManifestOptedOut() { @@ -407,4 +427,135 @@ final class ExtensionHostTests: XCTestCase { host.tabAppeared("radar") XCTAssertEqual(recorder.calls.map(\.id), ["derby"]) } + + // The panel is an NSPanel that is ordered out, not torn down, so its SwiftUI + // tree survives being hidden and `onAppear` — the only thing that calls + // tabAppeared — does not fire again when it comes back. Reopening onto a tab + // you were already on is therefore not an "open" as far as the host is + // concerned, and the pane shows whatever it last fetched. + // + // This asks whether the scheduled refresh covers that gap on its own. + // Coming on screen respects the cadence the manifest asked for, whether it + // was a tab switch, the panel returning, or the pill expanding. The view + // cannot tell those apart — in compact mode the pane leaves the tree + // entirely when the pill collapses, so expanding fires onAppear exactly as + // switching tabs does — and an earlier version exempting "switches" put the + // exemption on the path everyone uses. + // + // The floor is the manifest's own interval, not + // ExtensionManifest.minimumIntervalSeconds. Those are different numbers: + // the minimum is the fastest any extension is *permitted* to poll, not what + // this one chose. An extension asking for 600s against a rate-limited API + // was spawned every 5s by someone pressing the hotkey. + func testComingOnScreenRespectsTheManifestsOwnInterval() { + let recorder = Recorder() + let (host, _, _) = host([manifest("derby", refresh: "{\"intervalSeconds\":600}")], + recorder: recorder) + let now = Date() + host.tabAppeared("derby", now: now) + host.finish("derby", .transient("stub")) + XCTAssertEqual(recorder.calls.count, 1) + + // Well past the schema minimum, nowhere near what the extension asked + // for: the old floor spawned here, 120x the declared cadence. + host.tabAppeared("derby", now: now.addingTimeInterval(30)) + XCTAssertEqual(recorder.calls.count, 1) + + host.tabAppeared("derby", now: now.addingTimeInterval(601)) + XCTAssertEqual(recorder.calls.count, 2) + } + + // ⌘R is the deliberate override, and the only refresh an extension that + // declares no actions of its own has once the floor is in place. + func testForceRefreshIgnoresTheFloor() { + let recorder = Recorder() + let (host, _, _) = host([manifest("derby", refresh: "{\"intervalSeconds\":600}")], + recorder: recorder) + host.tabAppeared("derby") + host.finish("derby", .transient("stub")) + XCTAssertEqual(recorder.calls.count, 1) + + host.forceRefresh("derby") + XCTAssertEqual(recorder.calls.count, 2, "⌘R must not be floored") + } + + // It is an override of the floor, not of everything: a spawn already in + // flight still wins, because two at once is what busy exists to prevent. + // + // The pane is made busy directly because this harness runs the runner + // synchronously — `tabAppeared` would complete the fetch inline and clear + // busy before the second call, which is the one case the real async path + // has and this one does not. + func testForceRefreshStillYieldsToAFetchInFlight() { + let recorder = Recorder() + let (host, _, _) = host([manifest("derby")], recorder: recorder) + var busy = ExtensionHost.Pane() + busy.busy = true + host.replacePaneForTesting(busy, on: "derby") + + host.forceRefresh("derby") + XCTAssertTrue(recorder.calls.isEmpty, "a spawn in flight still wins") + } + + func testTheReopenFloorIsTheManifestsInterval() { + XCTAssertEqual(ExtensionHost.reopenFloor(manifest("a", refresh: "{\"intervalSeconds\":600}")), + 600) + // No interval to read, so the schema minimum is the only sensible value. + XCTAssertEqual(ExtensionHost.reopenFloor(manifest("b", refresh: "{\"onOpen\":true}")), + ExtensionManifest.minimumIntervalSeconds) + // A declared interval below the minimum is clamped at parse time, so it + // can never produce a floor under it. + XCTAssertEqual(ExtensionHost.reopenFloor(manifest("c", refresh: "{\"intervalSeconds\":1}")), + ExtensionManifest.minimumIntervalSeconds) + } + + // An extension that asks only for onOpen has no schedule to fall back on, + // so showing the panel onto its tab is the only thing that can refresh it. + func testATabWithNoScheduleStillRefreshesWhenThePanelComesBack() { + let recorder = Recorder() + let (host, _, _) = host([manifest("derby", refresh: "{\"onOpen\":true}")], + recorder: recorder) + let now = Date() + host.tabAppeared("derby") + host.finish("derby", .transient("stub")) + XCTAssertEqual(recorder.calls.count, 1) + + // Hidden for ten minutes. No interval, so no tick will ever help. + var later = now + for _ in 0..<120 { + later = later.addingTimeInterval(5) + host.tick(visibleTab: nil, now: later) + } + XCTAssertEqual(recorder.calls.count, 1) + + host.tabAppeared("derby", now: later) + XCTAssertEqual(recorder.calls.count, 2, + "showing the panel is the only refresh this extension gets") + } + + // The reported bug, end to end. An earlier version of this test drove only + // `tick` and so passed identically without the fix — it was exercising the + // pre-existing schedule, not the new path. + func testReopeningOntoATabYouWereAlreadyOnRefetchesWithoutAKeypress() { + let recorder = Recorder() + let (host, _, _) = host([manifest("derby", refresh: "{\"intervalSeconds\":30}")], + recorder: recorder) + host.tabAppeared("derby") + host.finish("derby", .transient("stub")) + XCTAssertEqual(recorder.calls.count, 1) + + // Hidden for five minutes: whileFocusedOnly means no polling, by design. + var now = Date() + for _ in 0..<60 { + now = now.addingTimeInterval(5) + host.tick(visibleTab: nil, now: now) + } + XCTAssertEqual(recorder.calls.count, 1, "a hidden pane must not poll") + + // The panel comes back. onAppear does not fire — the view never left + // the tree — so this call is the only thing standing between the user + // and stale numbers. + host.tabAppeared("derby", now: now.addingTimeInterval(5)) + XCTAssertEqual(recorder.calls.count, 2) + } } diff --git a/Tests/StackNudgePanelCoreTests/ExtensionTabTests.swift b/Tests/StackNudgePanelCoreTests/ExtensionTabTests.swift index 7b41378..c0ae5dd 100644 --- a/Tests/StackNudgePanelCoreTests/ExtensionTabTests.swift +++ b/Tests/StackNudgePanelCoreTests/ExtensionTabTests.swift @@ -123,4 +123,71 @@ final class ExtensionTabTests: XCTestCase { n.extensionTabs = [ExtensionTab(id: "derby", label: "Derby renamed")] XCTAssertEqual(n.mode, .extensionTab("derby")) } + + // MARK: - Footer hints + + private func document(_ json: String) -> ExtensionDocument { + guard case .success(let d) = ExtensionDocument.parse(Data(json.utf8)) else { + fatalError("fixture didn't parse") + } + return d + } + + // The footer's hints are now buttons, so what it renders has to be exactly + // what the keyboard can reach — otherwise a click does something no key + // can, and a key cap beside it is a lie. + func testAShadowedDocumentActionIsNotOffered() { + let d = document(""" + {"schema":1, + "rows":[{"id":"h1","title":"A","actions":[{"id":"open","label":"Open","key":"o"}]}], + "actions":[{"id":"help","label":"Help","key":"o"}, + {"id":"refresh","label":"Sync","key":"r"}]} + """) + let hints = ExtensionTabView.hintedActions(in: d, selectedRow: "h1") + // "o" belongs to the row action while that row is selected, exactly as + // ExtensionHost.resolve decides it. + XCTAssertEqual(hints.map(\.id), ["open", "refresh"]) + XCTAssertEqual(ExtensionHost.resolve(key: "o", in: d, selectedRow: "h1")?.action, "open") + } + + // With nothing selected the row action is unreachable, so the document + // action stops being shadowed and comes back. + func testTheDocumentActionReturnsWhenNoRowIsSelected() { + let d = document(""" + {"schema":1, + "rows":[{"id":"h1","title":"A","actions":[{"id":"open","label":"Open","key":"o"}]}], + "actions":[{"id":"help","label":"Help","key":"o"}]} + """) + XCTAssertEqual(ExtensionTabView.hintedActions(in: d, selectedRow: nil).map(\.id), ["help"]) + XCTAssertEqual(ExtensionHost.resolve(key: "o", in: d, selectedRow: nil)?.action, "help") + } + + // Every hint the footer draws must resolve to something, or the button is + // an affordance for nothing. + func testEveryHintResolvesToAnAction() { + let d = document(""" + {"schema":1, + "rows":[{"id":"h1","title":"A","actions":[{"id":"open","label":"Open","key":"o"}]}], + "actions":[{"id":"refresh","label":"Sync","key":"r"}, + {"id":"quiet","label":"No key","key":"!"}]} + """) + for selected in [nil, "h1"] { + for hint in ExtensionTabView.hintedActions(in: d, selectedRow: selected) { + XCTAssertNotNil(hint.key, "a keyless action must not be offered") + XCTAssertNotNil(ExtensionHost.resolve(key: hint.key ?? "", in: d, + selectedRow: selected), + "\(hint.id) is drawn but unreachable") + } + } + } + + // A refused key means no binding, so there is nothing to advertise. + func testAnActionWhoseKeyWasRefusedIsNotOffered() { + let d = document(""" + {"schema":1,"rows":[], + "actions":[{"id":"nope","label":"Nope","key":"cmd+r"}, + {"id":"fine","label":"Fine","key":"r"}]} + """) + XCTAssertEqual(ExtensionTabView.hintedActions(in: d, selectedRow: nil).map(\.id), ["fine"]) + } } diff --git a/panel/ExtensionHost.swift b/panel/ExtensionHost.swift index 4cc5a25..6de818c 100644 --- a/panel/ExtensionHost.swift +++ b/panel/ExtensionHost.swift @@ -112,13 +112,54 @@ final class ExtensionHost: ObservableObject { // MARK: - Invocation - // Opening a tab refreshes it unless the manifest opted out, and unless - // something is already in flight. - func tabAppeared(_ id: String) { + // The pane came on screen: a tab switch, the panel returning, or the pill + // expanding onto the tab someone left it on. + // + // One floor for all of them, because the view cannot tell them apart. In + // compact mode — the default, and effectively the only mode — collapsing to + // the pill removes this pane from the view tree entirely, so expanding it + // again fires `onAppear` exactly as switching tabs does. An earlier version + // of this tried to treat a switch as explicit and exempt it from the floor; + // that put the exemption on the path everyone actually uses and the floor + // on one almost nobody does, so every press of the hotkey spawned a script. + // + // ⌘R is the deliberate override. See `forceRefresh`. + func tabAppeared(_ id: String, now: Date = Date()) { guard let manifest = manifest(id), manifest.refresh.onOpen else { return } + if let attemptedAt = pane(id).attemptedAt, + now.timeIntervalSince(attemptedAt) < TimeInterval(Self.reopenFloor(manifest)) { + return + } refresh(id) } + // What ⌘R does: refetch now, whatever the floor says. + // + // The floor exists so that showing a window cannot spawn a script, which is + // a thing that happens to a user rather than something they ask for. This + // is the opposite, and without it an extension declaring no actions of its + // own has no way to refresh at all — the floor would otherwise have taken + // away the switch-away-and-back that used to serve as one. + func forceRefresh(_ id: String) { refresh(id) } + + // How stale a pane must be before coming on screen refetches it. + // + // The manifest's own interval, not the schema's minimum. Those are + // different numbers and using the minimum was wrong: it is the fastest any + // extension is *permitted* to poll (ExtensionManifest clamps declared + // intervals up to it), not the cadence this one chose. An extension asking + // for 600s against a rate-limited API would have been spawned every 5 + // seconds by someone toggling the panel — 120x what it declared. + // + // Reopening sooner than the interval leaves the pane showing data younger + // than the extension itself called acceptable, which is what it asked for. + // An onOpen-only extension has no interval to read, so it keeps the + // minimum. + static func reopenFloor(_ manifest: ExtensionManifest) -> Int { + max(ExtensionManifest.minimumIntervalSeconds, + manifest.refresh.intervalSeconds ?? ExtensionManifest.minimumIntervalSeconds) + } + func refresh(_ id: String) { invoke(id, action: nil, row: nil) } // A press while busy is ignored rather than queued: the user pressed it diff --git a/panel/ExtensionTabView.swift b/panel/ExtensionTabView.swift index 18819d4..081c26a 100644 --- a/panel/ExtensionTabView.swift +++ b/panel/ExtensionTabView.swift @@ -320,22 +320,66 @@ struct ExtensionTabView: View { private var footer: some View { PageFooter { FooterHint(label: "Hide", keys: ["Esc"]) + // Advertised because it is the only refresh an extension that + // declares no actions has, now that coming on screen is floored to + // the manifest's own interval. + FooterHint(label: "Refresh", keys: ["⌘R"]) if let document = pane.document, !document.rows.isEmpty { FooterHint(label: "Select", keys: ["↑", "↓"]) } // Only bound actions get a hint. An action whose key request was // refused has no shortcut and no button, so advertising it would be // a lie — see the note on ExtensionKey. - ForEach(hintedActions, id: \.id) { action in - FooterHint(label: action.label, keys: [Self.keyCap(action.key ?? "")]) + // Clickable, because it looks clickable. The pane was + // keyboard-only by design and the footer still advertised each + // action with its key cap — which reads as a button, so it gets + // pressed, and nothing happens. A refresh arriving on the poll + // thirty seconds later then looks like the click working. + ForEach(hintedActions, id: \.key) { action in + Button { + // By key, through the same resolver the keyboard uses, + // rather than a pre-baked (action id, row) pair frozen into + // this closure at render time. The document can swap between + // the render and the mouse-up — `finish` clears + // `selectedRow` when a new document drops that row — and a + // frozen pair would then spawn an action or a row the + // extension no longer declares. Re-resolving makes a click + // and a keypress the same call, not two calls that agree. + host.handle(key: action.key ?? "", on: id) + } label: { + FooterHint(label: action.label, + keys: [Self.keyCap(action.key ?? "")]) + } + .buttonStyle(.plain) + .disabled(pane.busy) } } } private var hintedActions: [ExtensionDocument.Action] { guard let document = pane.document else { return [] } - let rowActions = document.rows.first { $0.id == pane.selectedRow }?.actions ?? [] - return (rowActions + document.actions).filter { $0.key != nil } + return Self.hintedActions(in: document, selectedRow: pane.selectedRow) + } + + // Exactly the actions the keyboard can reach, in the order it reaches them. + // + // One per key, because ExtensionHost.resolve checks the selected row's + // actions before the document's: a row action shadows a document action + // sharing its key, leaving that one unreachable by keypress. Rendering both + // drew two identical key caps, one of which was a lie — and now that they + // are buttons it would also offer a click for something no key can do. + // + // Keyless actions are absent for the same reason they always were: there is + // no binding to advertise, and docs/extensions.md tells extensions to give + // every action a valid key until that changes. + static func hintedActions(in document: ExtensionDocument, + selectedRow: String?) -> [ExtensionDocument.Action] { + let rowActions = document.rows.first { $0.id == selectedRow }?.actions ?? [] + var seenKeys = Set() + return (rowActions + document.actions).filter { action in + guard let key = action.key else { return false } + return seenKeys.insert(key).inserted + } } static func keyCap(_ key: String) -> String { diff --git a/panel/Panel.swift b/panel/Panel.swift index 127a479..0ac4b82 100644 --- a/panel/Panel.swift +++ b/panel/Panel.swift @@ -4164,6 +4164,17 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, if case .extensionTab(let id) = nav.mode { let plain = mods.intersection([.command, .control, .option, .shift]).isEmpty guard plain else { return false } + // ⌘R before the plain guard. Coming on screen is floored to the + // manifest's own interval, so this is the only way to say "now" — + // and it matters most for an extension that declares no actions, + // which would otherwise have no refresh of its own at all. Every ⌘ + // combination was unhandled here, so it costs no extension a key, + // and it already means reload on the extensions browser. + if mods.intersection([.command, .control, .option, .shift]) == [.command], + event.keyCode == KeyCode.rKey, !event.isARepeat { + extensions.forceRefresh(id) + return true + } switch event.keyCode { case KeyCode.escape: hidePanel() @@ -4792,12 +4803,23 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, NSApp.activate(ignoringOtherApps: true) panel.makeKeyAndOrderFront(nil) } + refreshVisibleExtensionTab() return } positionPanel() // re-resolve in case the user moved to a different display NSApp.activate(ignoringOtherApps: true) panel.makeKeyAndOrderFront(nil) usageSurfaceDidChange() + refreshVisibleExtensionTab() + } + + // ExtensionTabView.onAppear cannot see the panel coming back: it is ordered + // out rather than torn down, so the view survives being hidden and never + // appears again, leaving the pane on whatever it fetched before. + // + private func refreshVisibleExtensionTab() { + guard case .extensionTab(let id) = nav.mode else { return } + extensions.tabAppeared(id) } // NSApp.hide hides all our windows AND deactivates the app, so the system diff --git a/panel/PanelNav.swift b/panel/PanelNav.swift index c1dc3b7..2458068 100644 --- a/panel/PanelNav.swift +++ b/panel/PanelNav.swift @@ -1072,9 +1072,20 @@ final class PanelNav: ObservableObject { static let eventsPerSessionOptions: [Int] = [3, 5, 10, 20, 50] // Compact widget mode. Shrinks the panel to a glance-only widget pinned // to a screen corner; clicking it expands back to the full panel. - // Compact mode is always-on now. The compactMode field is kept (and - // forced to true in loadFromConfig) to avoid threading the rest of - // the controller's compact-aware code; just don't expose a toggle. + // On by default, and switched off by Settings → Appearance → Widget, which + // persists STACKNUDGE_COMPACT_MODE (see the .widget case in applyCycle). + // loadFromConfig reads it back with a default of true. + // + // Spelled out because two previous versions of this comment were wrong in + // the same direction, and both cost review time on the PR that added the + // extension refresh. It said compact mode was "always-on" and "forced to + // true in loadFromConfig", then that there was no toggle. Neither holds: + // anyone who has turned the Widget off carries false durably. + // + // The distinction is load-bearing rather than pedantic. The non-compact + // panel is ordered out rather than torn down, so its view tree survives + // hiding and onAppear never fires again — a real code path with real users + // that "always-on" invites you to treat as dead. @Published var compactMode: Bool = true @Published var compactCorner: CompactCorner = .topRight // When true (default), releasing a drag snaps the pill to the nearest