diff --git a/lib/public/css/diagnostics.css b/lib/public/css/diagnostics.css index 8ecc0a54..29165556 100644 --- a/lib/public/css/diagnostics.css +++ b/lib/public/css/diagnostics.css @@ -154,17 +154,38 @@ } /* -------------------------------------------------------- - Diagnostic Toast - Extends the base .toast with richer layout. - Sits above the regular toast z-index. + Diagnostic Toast container (lr-e901) + Fixed top-right corner stack, off the composer/input box. + The base .toast family (base.css) is center-bottom over the + input — fine for a single transient message, but diagnostic + toasts can arrive concurrently (lr-e901 doubling defect) and + that spot has no stacking room. Follows the existing corner- + anchored, flex-column-stacked container precedent already + used by .notif-banner-container (notifications-center.css) + rather than introducing a third pattern: normal flow + gap + handles stacking with no per-toast JS offset math needed. -------------------------------------------------------- */ -.toast-diagnostic { +#diagnostic-toast-container { position: fixed; - bottom: 80px; - left: 50%; - transform: translateX(-50%) translateY(8px); + top: 48px; + right: 8px; z-index: 450; + display: flex; + flex-direction: column; + gap: 8px; + pointer-events: none; +} + +/* -------------------------------------------------------- + Diagnostic Toast + Extends the base .toast with richer layout; positioning now + comes from the parent #diagnostic-toast-container (lr-e901), + not from the element itself. + -------------------------------------------------------- */ +.toast-diagnostic { + position: relative; opacity: 0; + transform: translateY(-8px); pointer-events: auto; transition: opacity 0.2s, transform 0.2s; @@ -181,7 +202,7 @@ } .toast-diagnostic.visible { opacity: 1; - transform: translateX(-50%) translateY(0); + transform: translateY(0); } /* Severity border accent */ @@ -259,19 +280,21 @@ Mobile -------------------------------------------------------- */ @media (max-width: 768px) { - .toast-diagnostic { - left: 16px; + /* lr-2fdd: bottom:80px occluded the bottom-nav / input box / content on + mobile (iOS screenshot showed the PREFLIGHT WARNING toast covering the + UPCOMING card just above the Projects/Chat/Team/More nav). Top-anchor + instead, matching the base .toast / .sw-update-banner mobile fix so the + whole toast family lands the same way. lr-e901 keeps this top-anchor + intact — only widens the container to full-width since the desktop + top-right corner box is too narrow/cramped on small viewports. */ + #diagnostic-toast-container { + top: calc(var(--safe-top) + 12px); right: 16px; - transform: translateY(8px); + left: 16px; + } + .toast-diagnostic { max-width: none; - /* lr-2fdd: bottom:80px (inherited from base .toast) occludes the - bottom-nav / input box / content on mobile (iOS screenshot showed - the PREFLIGHT WARNING toast covering the UPCOMING card just above - the Projects/Chat/Team/More nav). Top-anchor instead, matching the - base .toast / .sw-update-banner mobile fix so the whole toast - family lands the same way. */ - top: calc(var(--safe-top) + 12px); - bottom: auto; + transform: translateY(-8px); } .toast-diagnostic.visible { transform: translateY(0); diff --git a/lib/public/modules/diagnostics.js b/lib/public/modules/diagnostics.js index 58d74618..e6fcbc9b 100644 --- a/lib/public/modules/diagnostics.js +++ b/lib/public/modules/diagnostics.js @@ -26,6 +26,32 @@ var panelBadgeEl = null; // Severity priority (higher = more prominent). var SEVERITY_ORDER = { error: 2, warning: 1, info: 0 }; +// lr-e901: dedup window for repeated/duplicate diagnostics. Two independent +// emitters (CLI-stderr capture + deterministic settings-preflight runPreflight()) +// can legitimately produce a diagnostic for the same underlying condition - +// addDiagnostic() had no dedup, so both were pushed and both toasted. Identity +// is (source, scope, message): scope is included because runPreflight() +// intentionally emits one diagnostic per settings file (user vs project, +// lr-7e22) - those are distinct conditions and must NOT be deduped together. +var DEDUP_WINDOW_MS = 5000; +var _recentDiagnosticKeys = Object.create(null); // key -> last-seen timestamp (ms) + +function _diagnosticKey(msg) { + return (msg.source || "") + "|" + (msg.scope || "") + "|" + (msg.message || ""); +} + +/** + * True when an equivalent diagnostic (same source+scope+message) was already + * recorded within DEDUP_WINDOW_MS. Also prunes the key's prior timestamp so + * the map does not grow unbounded across a long session. + */ +function _isDuplicateDiagnostic(msg, now) { + var key = _diagnosticKey(msg); + var last = _recentDiagnosticKeys[key]; + _recentDiagnosticKeys[key] = now; + return typeof last === "number" && (now - last) < DEDUP_WINDOW_MS; +} + // ============================================================ // Init // ============================================================ @@ -86,13 +112,19 @@ export function initDiagnostics() { // ============================================================ export function addDiagnostic(msg) { + var now = Date.now(); + + // lr-e901: same condition can arrive via two independent emitters (CLI-stderr + // capture + deterministic preflight) - collapse to a single visible entry. + if (_isDuplicateDiagnostic(msg, now)) return; + var entry = { severity: msg.severity || "info", source: msg.source || "", scope: msg.scope || "", message: msg.message || "", actionable: msg.actionable || null, - ts: Date.now(), + ts: now, }; diagnostics.push(entry); @@ -117,6 +149,23 @@ export { formatDiagnosticSource } from './diagnostic-format.js'; // Toast // ============================================================ +// lr-e901: fixed top-right corner stack (see diagnostics.css +// #diagnostic-toast-container) so concurrent diagnostic toasts stack via +// normal flex flow instead of piling on top of each other at a single fixed +// position over the input box. +var toastContainerEl = null; + +function _getToastContainer() { + if (toastContainerEl && toastContainerEl.isConnected) return toastContainerEl; + toastContainerEl = document.getElementById("diagnostic-toast-container"); + if (!toastContainerEl) { + toastContainerEl = document.createElement("div"); + toastContainerEl.id = "diagnostic-toast-container"; + document.body.appendChild(toastContainerEl); + } + return toastContainerEl; +} + function _showDiagnosticToast(entry) { var el = document.createElement("div"); // Use a dedicated class so diagnostics are visually distinct from error messages. @@ -156,7 +205,7 @@ function _showDiagnosticToast(entry) { el.appendChild(msgEl); el.appendChild(hintEl); - document.body.appendChild(el); + _getToastContainer().appendChild(el); refreshIcons(); // Animate in. diff --git a/lib/yoke/adapters/diagnostic-patterns.js b/lib/yoke/adapters/diagnostic-patterns.js index 43837939..16e0b61b 100644 --- a/lib/yoke/adapters/diagnostic-patterns.js +++ b/lib/yoke/adapters/diagnostic-patterns.js @@ -11,6 +11,18 @@ var validateDiagnosticEvent = require("../interface").validateDiagnosticEvent; +// VALID_HOOK_EVENTS (lr-e901): the CLI's own "Unknown hook event: X" stderr +// line is matched generically below (source:'hook') without knowing whether +// the Console's allowlist actually considers X unknown. settings-preflight.js +// is the single source of truth for that allowlist (see its header comment — +// "the ONLY place the valid hook events are defined"). Reuse it here instead +// of re-deriving a second copy, so a hook event added to the allowlist stops +// producing a false "unknown hook" toast from BOTH emission paths at once. +var VALID_HOOK_EVENTS = require("../../settings-preflight").VALID_HOOK_EVENTS; + +// Matches the hook event name out of a CLI "Unknown hook event: " line. +var UNKNOWN_HOOK_EVENT_RE = /^Unknown hook event:\s*(\S+)/; + // --------------------------------------------------------------------------- // Pattern table // --------------------------------------------------------------------------- @@ -91,6 +103,19 @@ function parseDiagnosticLine(rawLine) { for (var i = 0; i < PATTERNS.length; i++) { var pattern = PATTERNS[i]; if (matches(line, pattern)) { + // lr-e901: reconcile the CLI-stderr "Unknown hook event" path against + // the Console's own VALID_HOOK_EVENTS allowlist (settings-preflight.js). + // The CLI's stderr message reflects the *installed CLI version's* + // knowledge of hook events, which can lag the allowlist verified here + // (see settings-preflight.js header, lr-7e22). When the named event IS + // in VALID_HOOK_EVENTS, treat this as a stale/false CLI warning and + // suppress it rather than surfacing a contradictory toast — do not + // widen the allowlist to "fix" this, the allowlist is already correct. + var hookMatch = pattern.source === "hook" && UNKNOWN_HOOK_EVENT_RE.exec(line); + if (hookMatch && VALID_HOOK_EVENTS.indexOf(hookMatch[1]) !== -1) { + return null; + } + var event = { type: "diagnostic", severity: pattern.severity, diff --git a/test/diagnostic-patterns.test.js b/test/diagnostic-patterns.test.js index 14f2a185..ed8ebff0 100644 --- a/test/diagnostic-patterns.test.js +++ b/test/diagnostic-patterns.test.js @@ -31,8 +31,13 @@ var SEED_CASES = [ expectedSource: "settings", }, { + // lr-e901: PostToolUse is in VALID_HOOK_EVENTS (settings-preflight.js), + // so a CLI "Unknown hook event: PostToolUse" line is now reconciled away + // as a stale/false warning rather than forwarded (see 9g below). Use a + // genuinely-unknown event name here so this seed case still exercises + // the "Unknown hook event" pattern's happy path. label: "Unknown hook event → warning/hook", - line: "Unknown hook event: PostToolUse", + line: "Unknown hook event: SomeFutureHookEvent", expectedSeverity: "warning", expectedSource: "hook", }, @@ -180,3 +185,31 @@ test("9f: message is truncated to 500 chars for very long lines", function() { assert.ok(result.message.length <= 500, "message must be bounded to 500 chars, got: " + result.message.length); }); + +// --------------------------------------------------------------------------- +// Test 9g (lr-e901 regression) — CLI "Unknown hook event" reconciled against +// VALID_HOOK_EVENTS (settings-preflight.js). A hook the Console's own +// allowlist considers valid must NOT produce a diagnostic, even though the +// CLI's stderr text says "Unknown" (installed-CLI-version lag, not a real +// unknown-hook condition). Genuinely unknown events must still pass through. +// --------------------------------------------------------------------------- + +var VALID_HOOK_EVENTS = require("../lib/settings-preflight").VALID_HOOK_EVENTS; + +test("9g: 'Unknown hook event' line for an event IN VALID_HOOK_EVENTS is suppressed (null)", function() { + for (var i = 0; i < VALID_HOOK_EVENTS.length; i++) { + var event = VALID_HOOK_EVENTS[i]; + var result = parseDiagnosticLine("Unknown hook event: " + event); + assert.strictEqual(result, null, + "'Unknown hook event: " + event + "' must be suppressed — " + event + + " is in VALID_HOOK_EVENTS, so the CLI's warning is stale/false, not a real defect"); + } +}); + +test("9g: 'Unknown hook event' line for an event NOT in VALID_HOOK_EVENTS still produces a diagnostic", function() { + var result = parseDiagnosticLine("Unknown hook event: TotallyMadeUpEvent"); + assert.ok(result !== null, + "a genuinely unknown hook event must still produce a diagnostic"); + assert.strictEqual(result.severity, "warning"); + assert.strictEqual(result.source, "hook"); +}); diff --git a/test/diagnostics-toast-dedup-placement-lr-e901.test.js b/test/diagnostics-toast-dedup-placement-lr-e901.test.js new file mode 100644 index 00000000..a946f416 --- /dev/null +++ b/test/diagnostics-toast-dedup-placement-lr-e901.test.js @@ -0,0 +1,136 @@ +// diagnostics-toast-dedup-placement-lr-e901.test.js — lr-e901 regression coverage. +// +// Two of the three lr-e901 defects live in lib/public/modules/diagnostics.js +// and lib/public/css/diagnostics.css: +// +// (2) doubling — addDiagnostic() pushed + toasted every entry unconditionally, +// so the same underlying condition arriving via two independent emitters +// (CLI-stderr capture + deterministic settings-preflight runPreflight()) +// produced two toasts. Fix: dedup keyed on (source, scope, message) +// within a short time window before pushing/toasting. +// +// (3) placement/stacking — .toast-diagnostic was position:fixed center-bottom, +// directly over the composer/input box, with no stacking offset for +// concurrent toasts. Fix: re-anchor to a fixed top-right corner stack +// (#diagnostic-toast-container), following the existing corner-anchored, +// flex-column-stacked precedent already used by .notif-banner-container +// (notifications-center.css) — normal flow + gap handles stacking with +// no per-toast JS offset math. Mobile keeps the lr-2fdd top-anchor +// behavior, widened to full-width. +// +// diagnostics.js is an ESM module with DOM + icons.js dependencies that this +// project's test runner does not exercise via a DOM harness (see the existing +// diagnostics-panel-pointer-events-lr-b580.test.js convention) — these are +// source-text regression checks matching that same convention. + +"use strict"; + +var test = require("node:test"); +var assert = require("node:assert/strict"); +var fs = require("fs"); +var path = require("path"); + +var DIAGNOSTICS_JS = fs.readFileSync( + path.join(__dirname, "../lib/public/modules/diagnostics.js"), + "utf8" +); +var DIAGNOSTICS_CSS = fs.readFileSync( + path.join(__dirname, "../lib/public/css/diagnostics.css"), + "utf8" +); + +// --------------------------------------------------------------------------- +// Defect 2 — doubling / dedup +// --------------------------------------------------------------------------- + +test("diagnostics.js: addDiagnostic checks for a duplicate before pushing/toasting", () => { + var idx = DIAGNOSTICS_JS.indexOf("export function addDiagnostic"); + assert.ok(idx !== -1, "expected addDiagnostic to be exported"); + var block = DIAGNOSTICS_JS.slice(idx, idx + 400); + + assert.match( + block, + /_isDuplicateDiagnostic\s*\(/, + "addDiagnostic must consult a duplicate check before recording a new entry (lr-e901 doubling defect)" + ); + assert.match( + block, + /return;/, + "addDiagnostic must bail out early (return) when a duplicate is detected, skipping both the panel push and the toast" + ); +}); + +test("diagnostics.js: dedup identity includes source, scope, and message", () => { + var idx = DIAGNOSTICS_JS.indexOf("function _diagnosticKey"); + assert.ok(idx !== -1, "expected a _diagnosticKey helper to exist"); + var block = DIAGNOSTICS_JS.slice(idx, idx + 300); + + assert.match(block, /msg\.source/, "dedup key must include source"); + assert.match(block, /msg\.scope/, + "dedup key must include scope -- runPreflight() intentionally emits one diagnostic " + + "per settings file (user vs project, lr-7e22); those are distinct conditions and " + + "must NOT be collapsed together by dedup" + ); + assert.match(block, /msg\.message/, "dedup key must include message"); +}); + +test("diagnostics.js: dedup uses a bounded time window, not a permanent block", () => { + assert.match( + DIAGNOSTICS_JS, + /DEDUP_WINDOW_MS/, + "expected a DEDUP_WINDOW_MS constant so identical diagnostics are only " + + "collapsed within a short window (not permanently blocked for the rest of the session)" + ); +}); + +// --------------------------------------------------------------------------- +// Defect 3 — placement / stacking +// --------------------------------------------------------------------------- + +test("diagnostics.css: #diagnostic-toast-container is a fixed, corner-anchored, gap-stacked container", () => { + var idx = DIAGNOSTICS_CSS.indexOf("#diagnostic-toast-container {"); + assert.ok(idx !== -1, "expected #diagnostic-toast-container rule to exist"); + var block = DIAGNOSTICS_CSS.slice(idx, idx + 400); + + assert.match(block, /position:\s*fixed/, "container must be position:fixed"); + assert.match(block, /display:\s*flex/, "container must use flex layout for stacking"); + assert.match(block, /flex-direction:\s*column/, "container must stack children in a column"); + assert.match(block, /gap:/, "container must define a gap so stacked toasts do not touch/overlap"); + + // Off the input box: must NOT be center-bottom (the lr-e901 root cause). + assert.ok(block.indexOf("left: 50%") === -1, + "container must not be center-anchored (left:50%) -- that was the lr-e901 placement defect, directly over the composer/input box"); +}); + +test("diagnostics.css: .toast-diagnostic no longer self-positions with position:fixed", () => { + var idx = DIAGNOSTICS_CSS.indexOf(".toast-diagnostic {"); + assert.ok(idx !== -1, "expected .toast-diagnostic rule to exist"); + var block = DIAGNOSTICS_CSS.slice(idx, idx + 400); + + assert.ok(block.indexOf("position: fixed") === -1, + "individual toasts must not be position:fixed anymore -- positioning now comes from the parent #diagnostic-toast-container so multiple concurrent toasts stack instead of overlapping at the same fixed spot"); +}); + +test("diagnostics.js: toasts are appended into the shared toast container, not document.body directly", () => { + var idx = DIAGNOSTICS_JS.indexOf("function _showDiagnosticToast"); + assert.ok(idx !== -1, "expected _showDiagnosticToast to exist"); + var block = DIAGNOSTICS_JS.slice(idx, idx + 2000); + + assert.match( + block, + /_getToastContainer\(\)\.appendChild\(el\)/, + "_showDiagnosticToast must append into the shared stacking container, not document.body directly (lr-e901 placement/stacking defect)" + ); +}); + +test("diagnostics.css: mobile media query keeps top-anchor behavior on the container (lr-2fdd intact)", () => { + var idx = DIAGNOSTICS_CSS.indexOf("@media (max-width: 768px)"); + assert.ok(idx !== -1, "expected a mobile media query to exist"); + var block = DIAGNOSTICS_CSS.slice(idx, idx + 900); + + assert.match( + block, + /#diagnostic-toast-container\s*\{[^}]*top:\s*calc\(var\(--safe-top\)/, + "mobile must keep the lr-2fdd top-anchor fix (clearing the notch/status bar via --safe-top) on the container" + ); +});