Merge pull request #3638 from jeremymcs/fix/notification-overlay-backdrop

fix(gsd): notification overlay backdrop dimming and viewport padding

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");
+	});
+});

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++) {

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;
 }
 
 /**

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,
           },
         },
       );

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,
             },
           },
         );

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;

src/resources/extensions/gsd/notification-store.ts

@@ -275,14 +275,19 @@ function _withLock<T>(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 */ }
+    }
   }
 }

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 });
+  });
 });