fix notification click doing nothing by removing the per-session tag

c001be7 added `tag: vibeyard-session-<id>` on the assumption that macOS
coalesced same-named banners. It doesn't — Electron's macOS presenter gives
every banner a fresh NSUUID and ignores the tag. The tag instead took effect
in Chromium, where blink uses a non-empty tag as the non-persistent
notification's token, so registering a second notification for a session
erased the first one's click listener while its banner was still on screen.
Clicking that banner then only raised the app.

Drop the tag, dismiss a session's previous banner explicitly, and use the
owning project's name as the title so same-named sessions stay tellable
apart. Also expose AppState.findSessionWithProject so this module stops
open-coding a session lookup the class already had twice, privately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: elirantutia

src/renderer/notification-desktop.test.ts

@@ -6,11 +6,9 @@ const mockAppState = vi.hoisted(() => {
     projects: [
       {
         id: 'proj-1',
+        name: 'Proj One',
         activeSessionId: 'active-session',
-        sessions: [
-          { id: 'active-session', name: 'Active Session' },
-          { id: 'bg-session', name: 'Background Session' },
-        ],
+        sessions: [] as Array<{ id: string; name: string }>,
       },
     ],
     preferences: {
@@ -19,9 +17,20 @@ const mockAppState = vi.hoisted(() => {
     get activeProject() {
       return state.projects.find(p => p.id === state.activeProjectId);
     },
+    findSessionWithProject(sessionId: string) {
+      for (const project of state.projects) {
+        const session = project.sessions.find(s => s.id === sessionId);
+        if (session) return { project, session };
+      }
+      return undefined;
+    },
     setActiveProject: vi.fn(),
     setActiveSession: vi.fn(),
-    on: vi.fn(),
+    // Capture registered appState listeners so tests can fire them.
+    listeners: new Map<string, (data: unknown) => void>(),
+    on: vi.fn((event: string, cb: (data: unknown) => void) => {
+      state.listeners.set(event, cb);
+    }),
   };
   return state;
 });
@@ -35,22 +44,30 @@ import { _resetForTesting as resetDesktopNotif, initNotificationDesktop } from '
 
 
 // Track Notification constructor calls
-let notificationInstances: Array<{ title: string; options: NotificationOptions; onclick: (() => void) | null }> = [];
+let notificationInstances: MockNotification[] = [];
+// Interleaved log of construction/close events, so tests can assert ordering.
+let notificationEvents: string[] = [];
 let mockHasFocus = false;
 
 class MockNotification {
   static permission = 'granted';
   static requestPermission = vi.fn().mockResolvedValue('granted');
   title: string;
   options: NotificationOptions;
+  index: number;
+  closed = false;
   onclick: (() => void) | null = null;
   onclose: (() => void) | null = null;
   constructor(title: string, options: NotificationOptions = {}) {
     this.title = title;
     this.options = options;
-    notificationInstances.push(this as any);
+    this.index = notificationInstances.length;
+    notificationInstances.push(this);
+    notificationEvents.push(`create:${this.index}`);
   }
   close(): void {
+    this.closed = true;
+    notificationEvents.push(`close:${this.index}`);
     this.onclose?.();
   }
 }
@@ -62,16 +79,23 @@ if (typeof globalThis.document === 'undefined') {
 } else {
   vi.spyOn(document, 'hasFocus').mockImplementation(() => mockHasFocus);
 }
+const focusSpy = vi.fn();
+(globalThis as any).window = { vibeyard: { app: { focus: focusSpy } } };
 
 describe('notification-desktop', () => {
   beforeEach(() => {
     resetActivity();
     resetDesktopNotif();
     notificationInstances = [];
+    notificationEvents = [];
     mockHasFocus = false;
     mockAppState.preferences.notificationsDesktop = true;
     mockAppState.activeProjectId = 'proj-1';
     mockAppState.projects[0].activeSessionId = 'active-session';
+    mockAppState.projects[0].sessions = [
+      { id: 'active-session', name: 'Active Session' },
+      { id: 'bg-session', name: 'Background Session' },
+    ];
     mockAppState.setActiveProject.mockClear();
     mockAppState.setActiveSession.mockClear();
     MockNotification.permission = 'granted';
@@ -84,7 +108,9 @@ describe('notification-desktop', () => {
     setHookStatus('bg-session', 'waiting');
 
     expect(notificationInstances).toHaveLength(1);
-    expect(notificationInstances[0].title).toBe('Vibeyard');
+    // Title is the owning project, so same-named sessions in different projects
+    // are distinguishable in the banner.
+    expect(notificationInstances[0].title).toBe('Proj One');
     expect(notificationInstances[0].options.body).toBe('Background Session is waiting for input');
     expect(notificationInstances[0].options.silent).toBe(true);
   });
@@ -155,31 +181,44 @@ describe('notification-desktop', () => {
   });
 
   it('should focus app and switch session on notification click', () => {
-    const focusSpy = vi.fn();
-    (globalThis as any).window = {
-      focus: focusSpy,
-      vibeyard: { app: { focus: focusSpy } },
-    };
-
     initSession('bg-session');
     setHookStatus('bg-session', 'working');
     setHookStatus('bg-session', 'waiting');
 
     expect(notificationInstances).toHaveLength(1);
-    expect(notificationInstances[0].options.tag).toBe('vibeyard-session-bg-session');
+    focusSpy.mockClear();
     notificationInstances[0].onclick!();
 
+    expect(focusSpy).toHaveBeenCalled();
     expect(mockAppState.setActiveProject).toHaveBeenCalledWith('proj-1');
     expect(mockAppState.setActiveSession).toHaveBeenCalledWith('proj-1', 'bg-session');
   });
 
-  it('should route clicks to the correct session id when two sessions share a name', () => {
-    const focusSpy = vi.fn();
-    (globalThis as any).window = {
-      focus: focusSpy,
-      vibeyard: { app: { focus: focusSpy } },
-    };
+  it('should not set a tag, which would destroy the previous notification\'s click listener', () => {
+    initSession('bg-session');
+    setHookStatus('bg-session', 'working');
+    setHookStatus('bg-session', 'waiting');
+
+    expect(notificationInstances[0].options.tag).toBeUndefined();
+  });
+
+  it('should close the previous banner before posting a new one for the same session', () => {
+    initSession('bg-session');
+    setHookStatus('bg-session', 'working');
+    setHookStatus('bg-session', 'completed');
+    setHookStatus('bg-session', 'working');
+    setHookStatus('bg-session', 'completed');
 
+    expect(notificationInstances).toHaveLength(2);
+    // The stale banner is dismissed before the replacement is constructed, so
+    // the user is never left with an inert notification to click.
+    expect(notificationEvents).toEqual(['create:0', 'close:0', 'create:1']);
+
+    notificationInstances[1].onclick!();
+    expect(mockAppState.setActiveSession).toHaveBeenLastCalledWith('proj-1', 'bg-session');
+  });
+
+  it('should route clicks to the correct session id when two sessions share a name', () => {
     // Two distinct sessions that happen to share the same display name.
     mockAppState.projects[0].sessions.push(
       { id: 'dup-a', name: 'Session 6' } as any,
@@ -195,12 +234,11 @@ describe('notification-desktop', () => {
     setHookStatus('dup-b', 'completed');
 
     expect(notificationInstances).toHaveLength(2);
-    // Identical body, but distinct per-session tags keep them separate at the OS level.
+    // Identical text, but each notification keeps its own live click listener.
     expect(notificationInstances[0].options.body).toBe('Session 6 has completed');
     expect(notificationInstances[1].options.body).toBe('Session 6 has completed');
-    expect(notificationInstances[0].options.tag).toBe('vibeyard-session-dup-a');
-    expect(notificationInstances[1].options.tag).toBe('vibeyard-session-dup-b');
-    expect(notificationInstances[0].options.tag).not.toBe(notificationInstances[1].options.tag);
+    // Neither banner is closed by the other — different sessions, different entries.
+    expect(notificationInstances.some(n => n.closed)).toBe(false);
 
     // Clicking each notification focuses the id that produced it, not the other same-named session.
     notificationInstances[0].onclick!();
@@ -210,6 +248,19 @@ describe('notification-desktop', () => {
     expect(mockAppState.setActiveSession).toHaveBeenLastCalledWith('proj-1', 'dup-b');
   });
 
+  it('should dismiss a live banner when its session is removed', () => {
+    initSession('bg-session');
+    setHookStatus('bg-session', 'working');
+    setHookStatus('bg-session', 'completed');
+
+    expect(notificationInstances).toHaveLength(1);
+    expect(notificationInstances[0].closed).toBe(false);
+
+    mockAppState.listeners.get('session-removed')!({ sessionId: 'bg-session' });
+
+    expect(notificationInstances[0].closed).toBe(true);
+  });
+
   it('should not notify when Notification permission is not granted', () => {
     MockNotification.permission = 'denied';
 
@@ -220,12 +271,13 @@ describe('notification-desktop', () => {
     expect(notificationInstances).toHaveLength(0);
   });
 
-  it('should fall back to "Session" name for unknown session', () => {
+  it('should fall back to "Vibeyard" / "Session" for an unknown session', () => {
     initSession('unknown-session');
     setHookStatus('unknown-session', 'working');
     setHookStatus('unknown-session', 'waiting');
 
     expect(notificationInstances).toHaveLength(1);
+    expect(notificationInstances[0].title).toBe('Vibeyard');
     expect(notificationInstances[0].options.body).toBe('Session is waiting for input');
   });
 });

src/renderer/notification-desktop.ts

@@ -4,56 +4,58 @@ import { onChange, type SessionStatus } from './session-activity.js';
 const previousStatus = new Map<string, SessionStatus>();
 
 // Retain live notifications so their onclick closures aren't garbage-collected
-// before the user clicks, and so a stale notification can be closed/replaced.
+// before the user clicks, and so a stale banner can be dismissed explicitly.
 const activeNotifications = new Map<string, Notification>();
 
-function getSessionName(sessionId: string): string {
-  for (const project of appState.projects) {
-    const session = project.sessions.find(s => s.id === sessionId);
-    if (session) return session.name;
-  }
-  return 'Session';
-}
-
 function bodyForStatus(name: string, status: SessionStatus): string {
   if (status === 'input') return `${name} needs your input to continue`;
   if (status === 'completed') return `${name} has completed`;
   return `${name} is waiting for input`;
 }
 
+/** Dismiss this session's live banner, if any, and forget it. */
+function dismiss(sessionId: string): void {
+  activeNotifications.get(sessionId)?.close();
+  activeNotifications.delete(sessionId);
+}
+
 function showNotification(sessionId: string, status: SessionStatus): void {
   if (typeof Notification === 'undefined' || Notification.permission !== 'granted') return;
 
-  const name = getSessionName(sessionId);
-  const notification = new Notification('Vibeyard', {
-    body: bodyForStatus(name, status),
-    // A per-session tag keeps notifications for different sessions distinct at
-    // the OS level (so same-named sessions don't coalesce and misroute clicks),
-    // while a newer notification for the same session replaces its own stale one.
-    tag: `vibeyard-session-${sessionId}`,
+  const found = appState.findSessionWithProject(sessionId);
+
+  // Dismiss this session's previous banner ourselves — macOS gives every banner
+  // its own identifier, so nothing replaces it automatically.
+  dismiss(sessionId);
+
+  // Deliberately no `tag`: Chromium uses the tag as a non-persistent
+  // notification's id, and registering a second notification under an id it
+  // already knows destroys the first one's click listener — while that banner
+  // is still sitting in Notification Center. Clicking it then does nothing but
+  // raise the app. Without a tag each notification gets its own id and listener.
+  const notification = new Notification(found?.project.name ?? 'Vibeyard', {
+    body: bodyForStatus(found?.session.name ?? 'Session', status),
     silent: true,
   });
   activeNotifications.set(sessionId, notification);
 
   notification.onclick = () => {
     window.vibeyard.app.focus();
-    const project = appState.projects.find(p =>
-      p.sessions.some(s => s.id === sessionId),
-    );
+    // Re-resolve rather than capturing `found`: the session may have been
+    // renamed or moved, and capturing it would pin the whole ProjectRecord
+    // alive for as long as this retained notification lives.
+    const project = appState.findSessionWithProject(sessionId)?.project;
     if (project) {
       appState.setActiveProject(project.id);
       appState.setActiveSession(project.id, sessionId);
     }
-    notification.close();
-    activeNotifications.delete(sessionId);
+    notification.close(); // `onclose` evicts the map entry.
   };
 
   notification.onclose = () => {
-    // Only drop the entry if it still points at this instance — a replacement
-    // notification for the same session must not be clobbered.
-    if (activeNotifications.get(sessionId) === notification) {
-      activeNotifications.delete(sessionId);
-    }
+    // An OS-initiated close can arrive after a replacement was registered, so
+    // only drop the entry if it still points at this instance.
+    if (activeNotifications.get(sessionId) === notification) activeNotifications.delete(sessionId);
   };
 }
 
@@ -78,8 +80,7 @@ export function initNotificationDesktop(): void {
     const sessionId = (data as { sessionId: string })?.sessionId;
     if (sessionId) {
       previousStatus.delete(sessionId);
-      activeNotifications.get(sessionId)?.close();
-      activeNotifications.delete(sessionId);
+      dismiss(sessionId);
     }
   });
 }

src/renderer/state.ts

@@ -127,8 +127,19 @@ class AppState {
     this.nav.prune(sessionId);
   }
 
+  /** Resolve a session id to both the session and its owning project in one scan. */
+  findSessionWithProject(
+    sessionId: string,
+  ): { project: ProjectRecord; session: SessionRecord } | undefined {
+    for (const project of this.state.projects) {
+      const session = project.sessions.find((s) => s.id === sessionId);
+      if (session) return { project, session };
+    }
+    return undefined;
+  }
+
   private findProjectBySession(sessionId: string): ProjectRecord | undefined {
-    return this.state.projects.find((p) => p.sessions.some((s) => s.id === sessionId));
+    return this.findSessionWithProject(sessionId)?.project;
   }
 
   navigateBack(): void {
@@ -896,11 +907,7 @@ class AppState {
   }
 
   private findSessionById(sessionId: string): SessionRecord | undefined {
-    for (const project of this.state.projects) {
-      const session = project.sessions.find((s) => s.id === sessionId);
-      if (session) return session;
-    }
-    return undefined;
+    return this.findSessionWithProject(sessionId)?.session;
   }
 
   updateSessionCost(sessionId: string, cost: CostInfo): void {