From 2c4ac844f12abc1430a2bf2791eb50280c754105 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Mon, 6 Apr 2026 17:34:45 -0500 Subject: [PATCH 1/3] fix(gsd): add backdrop dimming and viewport padding to notification overlay The notification overlay was rendering too small with few entries, allowing underlying content to bleed through. Added viewport padding to fill the overlay box and a new `backdrop` option to OverlayOptions that dims the background behind modal overlays. --- packages/pi-tui/src/overlay-layout.ts | 13 ++++++++++++- packages/pi-tui/src/tui.ts | 2 ++ .../extensions/gsd/bootstrap/register-shortcuts.ts | 1 + .../gsd/commands/handlers/notifications-handler.ts | 1 + .../extensions/gsd/notification-overlay.ts | 5 +++++ 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/pi-tui/src/overlay-layout.ts b/packages/pi-tui/src/overlay-layout.ts index 1896c5bba..d3c2d8b6a 100644 --- a/packages/pi-tui/src/overlay-layout.ts +++ b/packages/pi-tui/src/overlay-layout.ts @@ -6,7 +6,7 @@ */ import type { OverlayAnchor, OverlayOptions, SizeValue } from "./tui.js"; -import { extractSegments, sliceByColumn, sliceWithWidth, truncateToWidth, visibleWidth } from "./utils.js"; +import { applyBackgroundToLine, extractSegments, sliceByColumn, sliceWithWidth, truncateToWidth, visibleWidth } from "./utils.js"; import { isImageLine } from "./terminal-image.js"; import { CURSOR_MARKER } from "./tui.js"; @@ -324,6 +324,17 @@ export function compositeOverlays( const viewportStart = Math.max(0, workingHeight - termHeight); + // Apply backdrop dimming if any visible overlay requests it + const hasBackdrop = visibleEntries.some((e) => e.options?.backdrop); + if (hasBackdrop) { + const dimFn = (text: string) => `\x1b[2m${text}\x1b[22m`; + for (let i = viewportStart; i < result.length; i++) { + if (!isImageLine(result[i]) && result[i].length > 0) { + result[i] = applyBackgroundToLine(result[i], termWidth, dimFn); + } + } + } + // Composite each overlay for (const { overlayLines, row, col, w } of rendered) { for (let i = 0; i < overlayLines.length; i++) { diff --git a/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index 8e3db6f05..0bdb8c746 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -141,6 +141,8 @@ export interface OverlayOptions { visible?: (termWidth: number, termHeight: number) => boolean; /** If true, don't capture keyboard focus when shown */ nonCapturing?: boolean; + /** If true, dim the background behind the overlay */ + backdrop?: boolean; } /** diff --git a/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts b/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts index 9abe3fbb8..1e0faf7a0 100644 --- a/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts +++ b/src/resources/extensions/gsd/bootstrap/register-shortcuts.ts @@ -44,6 +44,7 @@ export function registerShortcuts(pi: ExtensionAPI): void { minWidth: 60, maxHeight: "88%", anchor: "center", + backdrop: true, }, }, ); diff --git a/src/resources/extensions/gsd/commands/handlers/notifications-handler.ts b/src/resources/extensions/gsd/commands/handlers/notifications-handler.ts index e66309b95..16d30d49a 100644 --- a/src/resources/extensions/gsd/commands/handlers/notifications-handler.ts +++ b/src/resources/extensions/gsd/commands/handlers/notifications-handler.ts @@ -105,6 +105,7 @@ export async function handleNotificationsCommand( minWidth: 60, maxHeight: "88%", anchor: "center", + backdrop: true, }, }, ); diff --git a/src/resources/extensions/gsd/notification-overlay.ts b/src/resources/extensions/gsd/notification-overlay.ts index ec25c440a..6ab73f729 100644 --- a/src/resources/extensions/gsd/notification-overlay.ts +++ b/src/resources/extensions/gsd/notification-overlay.ts @@ -164,6 +164,11 @@ export class GSDNotificationOverlay { this.scrollOffset = Math.min(this.scrollOffset, maxScroll); const visibleContent = content.slice(this.scrollOffset, this.scrollOffset + visibleContentRows); + // Pad to fill viewport so the overlay covers underlying content + while (visibleContent.length < visibleContentRows) { + visibleContent.push(""); + } + const lines = this.wrapInBox(visibleContent, width); this.cachedWidth = width; From d5534557329de72fb07594535b67d401a84e2ee4 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Mon, 6 Apr 2026 17:39:44 -0500 Subject: [PATCH 2/3] fix(gsd): only unlink notification lock when owned, prevent foreign lock deletion _withLock() was unconditionally unlinking the lock file in finally, even when lock acquisition failed. This could delete another process's lock and allow unlocked concurrent writes. Now tracks ownership and only cleans up locks we created. --- src/resources/extensions/gsd/notification-store.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/resources/extensions/gsd/notification-store.ts b/src/resources/extensions/gsd/notification-store.ts index 54a600061..d79d4a33c 100644 --- a/src/resources/extensions/gsd/notification-store.ts +++ b/src/resources/extensions/gsd/notification-store.ts @@ -275,14 +275,19 @@ function _withLock(basePath: string, fn: () => T): T { } } + // Only run the mutation if we actually own the lock + const ownsLock = fd !== null; try { - // Write our PID timestamp into the lock for stale detection - if (fd !== null) { + if (ownsLock && fd !== null) { + // Write our PID timestamp into the lock for stale detection writeFileSync(lockPath, String(Date.now()), "utf-8"); closeSync(fd); } return fn(); } finally { - try { unlinkSync(lockPath); } catch { /* best-effort cleanup */ } + // Only delete the lock if we created it — never remove another process's lock + if (ownsLock) { + try { unlinkSync(lockPath); } catch { /* best-effort cleanup */ } + } } } From 9d1e343e41f58bbbea10c7e2901b95c0142faa78 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Mon, 6 Apr 2026 17:44:34 -0500 Subject: [PATCH 3/3] test(gsd): add overlay backdrop and notification lock safety tests - Overlay layout: verify backdrop dims base lines, no dim without flag, overlay composites on top of dimmed background - Notification store: verify markAllRead and clearNotifications do not delete a foreign process's lock file --- .../src/__tests__/overlay-layout.test.ts | 65 +++++++++++++++++++ .../gsd/tests/notification-store.test.ts | 35 +++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 packages/pi-tui/src/__tests__/overlay-layout.test.ts diff --git a/packages/pi-tui/src/__tests__/overlay-layout.test.ts b/packages/pi-tui/src/__tests__/overlay-layout.test.ts new file mode 100644 index 000000000..4f7d7817c --- /dev/null +++ b/packages/pi-tui/src/__tests__/overlay-layout.test.ts @@ -0,0 +1,65 @@ +// pi-tui — Overlay Layout Tests (backdrop dimming) + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { compositeOverlays, type OverlayEntry } from "../overlay-layout.js"; + +function makeEntry( + lines: string[], + options?: OverlayEntry["options"], +): OverlayEntry { + return { + component: { render: () => lines }, + options, + hidden: false, + focusOrder: 1, + }; +} + +describe("compositeOverlays — backdrop", () => { + it("dims base lines when backdrop is true", () => { + const base = ["hello world", "second line"]; + const overlay = makeEntry(["OVERLAY"], { + width: 7, + anchor: "top-left", + backdrop: true, + }); + + const result = compositeOverlays(base, [overlay], 20, 20, 2); + + // All base lines in viewport should contain dim escape (\x1b[2m) + // The overlay line itself is composited on top, but underlying lines get dimmed + const dimmedLine = result.find((l) => l.includes("second line")); + assert.ok(dimmedLine, "should have a line containing 'second line'"); + assert.ok(dimmedLine.includes("\x1b[2m"), "base line should be dimmed"); + }); + + it("does not dim when backdrop is false/absent", () => { + const base = ["hello world", "second line"]; + const overlay = makeEntry(["OVERLAY"], { + width: 7, + anchor: "top-left", + }); + + const result = compositeOverlays(base, [overlay], 20, 20, 2); + + // Lines not covered by overlay should remain undimmed + const secondLine = result.find((l) => l.includes("second line")); + assert.ok(secondLine, "should have a line containing 'second line'"); + assert.ok(!secondLine.includes("\x1b[2m"), "base line should not be dimmed"); + }); + + it("overlay content renders on top of dimmed background", () => { + const base = ["aaaaaaaaaa"]; + const overlay = makeEntry(["XX"], { + width: 2, + anchor: "top-left", + backdrop: true, + }); + + const result = compositeOverlays(base, [overlay], 10, 10, 1); + + // The first line should contain the overlay text + assert.ok(result[0].includes("XX"), "overlay text should be composited"); + }); +}); diff --git a/src/resources/extensions/gsd/tests/notification-store.test.ts b/src/resources/extensions/gsd/tests/notification-store.test.ts index 2d8dd105a..8f13fb873 100644 --- a/src/resources/extensions/gsd/tests/notification-store.test.ts +++ b/src/resources/extensions/gsd/tests/notification-store.test.ts @@ -2,7 +2,7 @@ import { describe, test, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, mkdirSync, rmSync, readFileSync, existsSync } from "node:fs"; +import { mkdtempSync, mkdirSync, rmSync, readFileSync, existsSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; @@ -246,4 +246,37 @@ describe("notification-store", () => { assert.equal(getUnreadCount(), 0); assert.equal(getLineCount(), 0); }); + + test("markAllRead does not delete a foreign lock file", () => { + initNotificationStore(tmp); + appendNotification("msg1", "info"); + + // Simulate another process holding the lock + const lockPath = join(tmp, ".gsd", "notifications.lock"); + writeFileSync(lockPath, String(Date.now()), "utf-8"); + + // markAllRead should still work (best-effort) but not delete the foreign lock + markAllRead(); + + assert.ok(existsSync(lockPath), "foreign lock file should not be deleted"); + + // Clean up the lock so afterEach doesn't leave artifacts + rmSync(lockPath, { force: true }); + }); + + test("clearNotifications does not delete a foreign lock file", () => { + initNotificationStore(tmp); + appendNotification("msg1", "info"); + + // Simulate another process holding the lock + const lockPath = join(tmp, ".gsd", "notifications.lock"); + writeFileSync(lockPath, String(Date.now()), "utf-8"); + + // clearNotifications should still work but not delete the foreign lock + clearNotifications(); + + assert.ok(existsSync(lockPath), "foreign lock file should not be deleted"); + + rmSync(lockPath, { force: true }); + }); });