fix(attachments): address review findings on attachment history drawer (#121)

Follow-up fixes applied during review of PR #121 (all confirmed minor/nit;
no blockers). Security posture verified sound (externalPath never leaves
toState()/the list route; re-registration runs the guard).

- fix(recovery): restoreAttachmentHistory now skips malformed/legacy saved
  items (null, non-object, missing source/fileName) instead of throwing inside
  the Session constructor — a corrupt __attachmentHistory entry could otherwise
  abort the entire mux-recovery loop. (P1)
- fix(routes): the attachment-list route degrades a single failing entry to
  {missing:true} instead of failing the whole drawer. (INT-4)
- fix(ui): give the attachments header button a positioning context so the
  unread badge anchors to the icon, not the header bar. (F1/CSS-1)
- fix(ui): cancel the debounced history refresh on drawer close and guard it
  against a stale session/closed drawer. (F3)
- fix(ui): re-show ("Card") of a detected item now uses the item's own
  timestamp so the cardId is stable — focuses the existing card instead of
  stacking duplicates. (F4)
- fix(ui): Escape now closes the drawer, matching every other panel. (UX-1)
- fix(ui): badge shows "99+" past 99 (was an inconsistent 100/99 cap). (BADGE-1)
- style: drop the duplicate @keyframes notif-badge-pulse (dead CSS). (INT-1/CSS-3)
- style: empty-state used three undefined CSS custom properties
  (--text-primary/--border-color/--bg-tertiary) → use the defined
  --text/--border-light/--bg-input tokens. (CSS-2)
- test: add constructor restore round-trip + malformed-item resilience tests.

Deferred (noted for author): broadcasting the full 100-item history in every
session-state SSE event (payload bloat), "unread" badge semantics, making the
header button opt-in, and app.inject route tests for the two new endpoints.

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

src/session.ts

@@ -919,6 +919,11 @@ export class Session extends EventEmitter {
   restoreAttachmentHistory(history: SessionAttachmentHistoryItem[] | undefined): void {
     this._attachmentHistory = [];
     for (const item of [...(history ?? [])].reverse()) {
+      // Guard against malformed/legacy on-disk entries (null, non-object, or
+      // missing required fields). historyKey() dereferences source/fileName, so
+      // a bad item would otherwise throw inside the constructor and abort the
+      // entire mux-recovery loop.
+      if (!item || typeof item !== 'object' || !item.source || !item.fileName) continue;
       this.upsertAttachmentHistory(item);
     }
   }

src/web/public/app.js

@@ -767,6 +767,7 @@ class CodemanApp {
       if (e.key === 'Escape') {
         this.closeAllPanels();
         this.closeHelp();
+        if (this.attachmentHistoryDrawerOpen) this.closeAttachmentHistory();
       }
 
       // Alt+1-9: switch to Codeman session by index

src/web/public/panels-ui.js

@@ -2561,7 +2561,17 @@ Object.assign(CodemanApp.prototype, {
       if (data.sessionId === this.activeSessionId) {
         this.updateAttachmentHistoryBadge();
         if (this.attachmentHistoryDrawerOpen) {
-          this._debouncedCall('attachmentHistoryRefresh', () => this.loadAttachmentHistory(data.sessionId), 250);
+          this._debouncedCall(
+            'attachmentHistoryRefresh',
+            () => {
+              // The drawer may have closed or the active session changed during
+              // the debounce window — don't refresh for a stale session.
+              if (this.attachmentHistoryDrawerOpen && this.activeSessionId === data.sessionId) {
+                this.loadAttachmentHistory(data.sessionId);
+              }
+            },
+            250
+          );
         }
       }
     }
@@ -2744,7 +2754,7 @@ Object.assign(CodemanApp.prototype, {
     const sessionId = this.activeSessionId;
     const nextCount = count ?? (sessionId ? this.attachmentHistoryCounts.get(sessionId) || 0 : 0);
     if (badge) {
-      badge.textContent = String(Math.min(nextCount, 99));
+      badge.textContent = nextCount > 99 ? '99+' : String(nextCount);
       badge.style.display = nextCount > 0 ? '' : 'none';
     }
     if (button) {
@@ -2804,6 +2814,11 @@ Object.assign(CodemanApp.prototype, {
     const drawer = document.getElementById('attachmentHistoryDrawer');
     this.attachmentHistoryDrawerOpen = false;
     drawer?.classList.remove('open');
+    // Cancel any pending debounced refresh so it can't fire against a closed drawer.
+    if (this._debounceTimers?.attachmentHistoryRefresh) {
+      clearTimeout(this._debounceTimers.attachmentHistoryRefresh);
+      this._debounceTimers.attachmentHistoryRefresh = null;
+    }
     this.updateAttachmentHistoryBadge();
   },
 
@@ -2949,7 +2964,10 @@ Object.assign(CodemanApp.prototype, {
       sessionId: item.sessionId,
       relativePath: item.relativePath,
       fileName: item.fileName,
-      timestamp: Date.now(),
+      // Use the item's own timestamp (not Date.now()) so the derived cardId is
+      // stable across clicks — re-showing focuses the existing card instead of
+      // stacking a duplicate.
+      timestamp: item.timestamp ?? Date.now(),
       size: item.size,
       attachmentType: item.attachmentType,
       extension: item.extension,

src/web/public/styles.css

@@ -9401,6 +9401,12 @@ body.touch-device.cjk-input-visible .main {
   background: #fff;
 }
 
+/* Positioning context so the absolutely-positioned badge anchors to the icon,
+   not the header bar (mirrors .btn-notifications). */
+.btn-attachments-history {
+  position: relative;
+}
+
 .attachment-history-badge {
   position: absolute;
   top: 2px;
@@ -9419,11 +9425,6 @@ body.touch-device.cjk-input-visible .main {
   pointer-events: none;
 }
 
-@keyframes notif-badge-pulse {
-  0%, 100% { transform: scale(1); }
-  50% { transform: scale(1.15); }
-}
-
 /* Attachment History Drawer */
 .attachment-history-drawer {
   position: fixed;
@@ -9495,7 +9496,7 @@ body.touch-device.cjk-input-visible .main {
 }
 
 .attachment-history-empty-title {
-  color: var(--text-primary);
+  color: var(--text);
   font-size: 0.9rem;
   font-weight: 600;
 }
@@ -9503,11 +9504,11 @@ body.touch-device.cjk-input-visible .main {
 .attachment-history-empty code {
   max-width: 100%;
   overflow-wrap: anywhere;
-  border: 1px solid var(--border-color);
+  border: 1px solid var(--border-light);
   border-radius: 6px;
   padding: 5px 7px;
-  color: var(--text-primary);
-  background: var(--bg-tertiary);
+  color: var(--text);
+  background: var(--bg-input);
   font-size: 0.76rem;
 }
 

src/web/routes/file-routes.ts

@@ -714,9 +714,10 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
 
     const items = await Promise.all(
       sessionHistory.history.map((item) =>
-        item.source === 'external'
+        (item.source === 'external'
           ? buildExternalAttachmentRouteItem(id, item, sessionHistory.workingDir)
           : buildDetectedAttachmentRouteItem(id, sessionHistory.workingDir, item)
+        ).catch(() => ({ ...sanitizeAttachmentHistoryItem(item), missing: true }))
       )
     );
 

test/session-attachment-history.test.ts

@@ -113,4 +113,52 @@ describe('session attachment history', () => {
     expect(publicState.attachmentHistory?.[0].id).not.toContain('/mnt/c/private/board-update.pdf');
     expect(persistedHistory?.[0].externalPath).toBe('/mnt/c/private/board-update.pdf');
   });
+
+  it('round-trips persisted history (incl. externalPath) through constructor restore', () => {
+    const a = new Session({ workingDir: '/tmp' });
+    a.upsertAttachmentHistory(
+      buildExternalAttachmentHistoryItem({
+        sessionId: a.id,
+        externalPath: '/mnt/c/docs/deck.docx',
+        fileName: 'deck.docx',
+        extension: 'docx',
+        size: 10,
+        timestamp: 5,
+      })
+    );
+    const persisted = a.getAttachmentHistoryForPersist();
+
+    const b = new Session({ workingDir: '/tmp', attachmentHistory: persisted });
+    // Private copy keeps externalPath (needed to re-register/serve the file)...
+    expect(b.getAttachmentHistoryForPersist()?.[0].externalPath).toBe('/mnt/c/docs/deck.docx');
+    // ...but the public copy stays sanitized after restore.
+    expect(JSON.stringify(b.toState().attachmentHistory)).not.toContain('/mnt/c/docs/deck.docx');
+  });
+
+  it('ignores malformed saved history items during restore instead of throwing', () => {
+    const valid: SessionAttachmentHistoryItem = {
+      id: 'detected:ok.png',
+      sessionId: 's',
+      fileName: 'ok.png',
+      extension: 'png',
+      attachmentType: 'image',
+      size: 1,
+      mtimeMs: 0,
+      timestamp: 1,
+      source: 'detected',
+      relativePath: 'ok.png',
+    };
+    // null, a non-object, and an item missing required fields must be skipped —
+    // historyKey() would otherwise throw inside the constructor and abort recovery.
+    const malformed = [null, 'nope', { partial: true }, valid] as unknown as SessionAttachmentHistoryItem[];
+
+    let session!: Session;
+    expect(() => {
+      session = new Session({ workingDir: '/tmp', attachmentHistory: malformed });
+    }).not.toThrow();
+
+    const persisted = session.getAttachmentHistoryForPersist();
+    expect(persisted).toHaveLength(1);
+    expect(persisted?.[0].fileName).toBe('ok.png');
+  });
 });