From 1a363a3e62e8332aaa8e2e9a533e6398f18fce4d Mon Sep 17 00:00:00 2001 From: "Claude (Codeman maintainer)" Date: Sun, 14 Jun 2026 09:30:01 +0200 Subject: [PATCH] fix(attachments): address review findings on attachment history drawer (#121) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/session.ts | 5 +++ src/web/public/app.js | 1 + src/web/public/panels-ui.js | 24 +++++++++++-- src/web/public/styles.css | 19 +++++----- src/web/routes/file-routes.ts | 3 +- test/session-attachment-history.test.ts | 48 +++++++++++++++++++++++++ 6 files changed, 87 insertions(+), 13 deletions(-) diff --git a/src/session.ts b/src/session.ts index 66de0950..fb5b48f3 100644 --- a/src/session.ts +++ b/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); } } diff --git a/src/web/public/app.js b/src/web/public/app.js index 21372c72..e5843288 100644 --- a/src/web/public/app.js +++ b/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 diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 32bce820..3557c240 100644 --- a/src/web/public/panels-ui.js +++ b/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, diff --git a/src/web/public/styles.css b/src/web/public/styles.css index f65a3faa..6ebbebc8 100644 --- a/src/web/public/styles.css +++ b/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; } diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index c9638d56..4358fc35 100644 --- a/src/web/routes/file-routes.ts +++ b/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 })) ) ); diff --git a/test/session-attachment-history.test.ts b/test/session-attachment-history.test.ts index ac83f6b0..c93b009e 100644 --- a/test/session-attachment-history.test.ts +++ b/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'); + }); });