From df1a7a76d07819438d49ea71883d2f92248afb7a Mon Sep 17 00:00:00 2001 From: Jeremy Date: Sun, 12 Apr 2026 18:07:36 -0500 Subject: [PATCH] fix(tui): overlay subscription + Ctrl+Shift+P shortcut conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace notification overlay 3s polling with onNotificationStoreChange subscription for immediate updates; keep 30s safety-net for cross-process - Remove Ctrl+Shift+P parallel fallback that conflicts with cycleModelBackward - Add hasFallback flag to GSDShortcutDef so hint text is accurate - Fix misleading _withLock comment; rename ownsLock → createdLock Closes gsd-build/gsd-2#4076 --- .../gsd/bootstrap/register-shortcuts.ts | 7 +--- .../extensions/gsd/notification-overlay.ts | 38 +++++++++++++------ .../extensions/gsd/notification-store.ts | 9 +++-- src/resources/extensions/gsd/shortcut-defs.ts | 9 ++++- .../gsd/tests/format-shortcut.test.ts | 16 ++++++++ .../gsd/tests/register-shortcuts.test.ts | 5 ++- 6 files changed, 61 insertions(+), 23 deletions(-) diff --git a/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts b/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts index 8a6f0585f..eb8dc79b8 100644 --- a/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts +++ b/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts @@ -93,9 +93,6 @@ export function registerShortcuts(pi: ExtensionAPI): void { handler: openParallelOverlay, }); - // Fallback for terminals where Ctrl+Alt letter chords are not forwarded reliably. - pi.registerShortcut(Key.ctrlShift(GSD_SHORTCUTS.parallel.key), { - description: shortcutDesc(`${GSD_SHORTCUTS.parallel.action} (fallback)`, GSD_SHORTCUTS.parallel.command), - handler: openParallelOverlay, - }); + // No Ctrl+Shift+P fallback — conflicts with cycleModelBackward (shift+ctrl+p). + // Use Ctrl+Alt+P or /gsd parallel watch instead. } diff --git a/src/resources/extensions/gsd/notification-overlay.ts b/src/resources/extensions/gsd/notification-overlay.ts index 2738a6d5e..98d34785a 100644 --- a/src/resources/extensions/gsd/notification-overlay.ts +++ b/src/resources/extensions/gsd/notification-overlay.ts @@ -9,6 +9,7 @@ import { readNotifications, markAllRead, clearNotifications, + onNotificationStoreChange, type NotificationEntry, type NotifySeverity, } from "./notification-store.js"; @@ -82,6 +83,7 @@ export class GSDNotificationOverlay { private refreshTimer: ReturnType; private disposed = false; private resizeHandler: (() => void) | null = null; + private unsubscribeStore: (() => void) | null = null; constructor( tui: { requestRender: () => void }, @@ -105,19 +107,17 @@ export class GSDNotificationOverlay { }; process.stdout.on("resize", this.resizeHandler); - // Refresh every 3s for new notifications + // Subscribe to store mutations for immediate updates + this.unsubscribeStore = onNotificationStoreChange(() => { + if (this.disposed) return; + this._refreshFromDisk(); + }); + + // 30s safety-net for cross-process edits (web subprocess, parallel workers) this.refreshTimer = setInterval(() => { if (this.disposed) return; - const fresh = readNotifications(); - const signature = notificationSignature(fresh); - if (signature !== this.entriesSignature) { - markAllRead(); - this.entries = readNotifications(); - this.entriesSignature = notificationSignature(this.entries); - this.invalidate(); - this.tui.requestRender(); - } - }, 3000); + this._refreshFromDisk(); + }, 30_000); } private get filter(): FilterMode { @@ -215,12 +215,28 @@ export class GSDNotificationOverlay { dispose(): void { this.disposed = true; clearInterval(this.refreshTimer); + if (this.unsubscribeStore) { + this.unsubscribeStore(); + this.unsubscribeStore = null; + } if (this.resizeHandler) { process.stdout.removeListener("resize", this.resizeHandler); this.resizeHandler = null; } } + private _refreshFromDisk(): void { + const fresh = readNotifications(); + const signature = notificationSignature(fresh); + if (signature !== this.entriesSignature) { + markAllRead(); + this.entries = readNotifications(); + this.entriesSignature = notificationSignature(this.entries); + this.invalidate(); + this.tui.requestRender(); + } + } + private wrapInBox(inner: string[], width: number): string[] { const th = this.theme; const border = (s: string) => th.fg("borderAccent", s); diff --git a/src/resources/extensions/gsd/notification-store.ts b/src/resources/extensions/gsd/notification-store.ts index 218084215..36e6c6a7f 100644 --- a/src/resources/extensions/gsd/notification-store.ts +++ b/src/resources/extensions/gsd/notification-store.ts @@ -323,10 +323,11 @@ function _withLock(basePath: string, fn: () => T): T { } } - // Only run the mutation if we actually own the lock - const ownsLock = fd !== null; + // Best-effort: mutation runs regardless of lock status (idempotent overwrites). + // createdLock gates cleanup only — never skip fn() on lock failure. + const createdLock = fd !== null; try { - if (ownsLock && fd !== null) { + if (createdLock && fd !== null) { // Write our PID timestamp into the lock for stale detection writeFileSync(lockPath, String(Date.now()), "utf-8"); closeSync(fd); @@ -334,7 +335,7 @@ function _withLock(basePath: string, fn: () => T): T { return fn(); } finally { // Only delete the lock if we created it — never remove another process's lock - if (ownsLock) { + if (createdLock) { try { unlinkSync(lockPath); } catch { /* best-effort cleanup */ } } } diff --git a/src/resources/extensions/gsd/shortcut-defs.ts b/src/resources/extensions/gsd/shortcut-defs.ts index d88e5ebc0..79e50e33d 100644 --- a/src/resources/extensions/gsd/shortcut-defs.ts +++ b/src/resources/extensions/gsd/shortcut-defs.ts @@ -8,6 +8,8 @@ type GSDShortcutDef = { key: "g" | "n" | "p"; action: string; command: string; + /** Whether the Ctrl+Shift fallback is registered (false when it conflicts with an app keybinding). */ + hasFallback: boolean; }; export const GSD_SHORTCUTS: Record = { @@ -15,16 +17,19 @@ export const GSD_SHORTCUTS: Record = { key: "g", action: "Open GSD dashboard", command: "/gsd status", + hasFallback: true, }, notifications: { key: "n", action: "Open notification history", command: "/gsd notifications", + hasFallback: true, }, parallel: { key: "p", action: "Open parallel worker monitor", command: "/gsd parallel watch", + hasFallback: false, // Ctrl+Shift+P conflicts with cycleModelBackward }, }; @@ -41,7 +46,9 @@ export function fallbackShortcutCombo(id: GSDShortcutId): string { } export function shortcutPair(id: GSDShortcutId, formatter: (combo: string) => string = (combo) => combo): string { - return `${formatter(primaryShortcutCombo(id))} / ${formatter(fallbackShortcutCombo(id))}`; + const primary = formatter(primaryShortcutCombo(id)); + if (!GSD_SHORTCUTS[id].hasFallback) return primary; + return `${primary} / ${formatter(fallbackShortcutCombo(id))}`; } export function formattedShortcutPair(id: GSDShortcutId): string { diff --git a/src/resources/extensions/gsd/tests/format-shortcut.test.ts b/src/resources/extensions/gsd/tests/format-shortcut.test.ts index bd7067656..01471fad2 100644 --- a/src/resources/extensions/gsd/tests/format-shortcut.test.ts +++ b/src/resources/extensions/gsd/tests/format-shortcut.test.ts @@ -82,3 +82,19 @@ test("shortcut-defs: formats shortcut pair using platform symbols", () => { assert.equal(pair, "Ctrl+Alt+N / Ctrl+Shift+N"); } }); + +test("shortcut-defs: parallel shortcut omits fallback (hasFallback: false)", () => { + const pair = formattedShortcutPair("parallel"); + if (process.platform === "darwin") { + assert.equal(pair, "⌃⌥P", "parallel should only show primary combo"); + } else { + assert.equal(pair, "Ctrl+Alt+P", "parallel should only show primary combo"); + } + // Verify it does NOT contain the fallback separator + assert.ok(!pair.includes("/"), "parallel pair should not contain fallback separator"); +}); + +test("shortcut-defs: dashboard shortcut includes fallback (hasFallback: true)", () => { + const pair = formattedShortcutPair("dashboard"); + assert.ok(pair.includes("/"), "dashboard pair should contain fallback separator"); +}); diff --git a/src/resources/extensions/gsd/tests/register-shortcuts.test.ts b/src/resources/extensions/gsd/tests/register-shortcuts.test.ts index e8103da6f..90f822bd0 100644 --- a/src/resources/extensions/gsd/tests/register-shortcuts.test.ts +++ b/src/resources/extensions/gsd/tests/register-shortcuts.test.ts @@ -69,14 +69,15 @@ test("dashboard shortcut resolves the project root instead of the current worktr assert.ok(customCalls > 0, "shortcut opens the dashboard overlay when project root is resolved"); assert.equal(notices.length, 0, "shortcut does not fall back to the missing-.gsd warning"); - assert.equal(shortcuts.length, 6, "all GSD shortcuts are still registered"); + assert.equal(shortcuts.length, 5, "all GSD shortcuts are still registered"); const keys = shortcuts.map((shortcut) => shortcut.key); assert.ok(keys.includes("ctrl+alt+g"), "primary dashboard shortcut is registered"); assert.ok(keys.includes("ctrl+shift+g"), "fallback dashboard shortcut is registered"); assert.ok(keys.includes("ctrl+alt+n"), "primary notifications shortcut is registered"); assert.ok(keys.includes("ctrl+shift+n"), "fallback notifications shortcut is registered"); assert.ok(keys.includes("ctrl+alt+p"), "primary parallel shortcut is registered"); - assert.ok(keys.includes("ctrl+shift+p"), "fallback parallel shortcut is registered"); + // No Ctrl+Shift+P fallback — conflicts with cycleModelBackward (shift+ctrl+p) + assert.ok(!keys.includes("ctrl+shift+p"), "parallel fallback must not be registered (conflicts with cycleModelBackward)"); }); test("parallel shortcut passes resolved project root into overlay", async (t) => {