From f0a7ed758ff615a26c2783449ebef0019ea788f2 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Mon, 25 May 2026 23:48:28 -0700 Subject: [PATCH] =?UTF-8?q?fix(#1391):=20Priority+=20nav=20=E2=80=94=20act?= =?UTF-8?q?ive-route=20pill=20must=20NEVER=20drop=20high-priority=20links?= =?UTF-8?q?=20into=20orphaned=20More=20dropdown=20(#1394)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Pins the active-route `.nav-link` inline at any viewport ≥768px so Priority+ never shoves it into the More dropdown. Fixes the operator's screenshot of `/#/perf` at ~1080px where the navbar showed only the active "Perf" pill missing — and an inverse failure where the active pill was the only thing **in** the dropdown. This is the 20th regression of nav Priority+. Single-loop fix only; no algorithm redesign (per issue out-of-scope). ## Root cause `public/app.js` `applyNavPriority()` had two places that ignored the active state: 1. **≤1100 narrow-desktop CSS branch (line ~1197):** `if (a.dataset.priority !== 'high') a.classList.add('is-overflow')` blindly overflowed every non-high link — including the active pill. 2. **>1100 measurement loop (line ~1267):** `overflowQueue` is `non-high reversed + high reversed`. The active non-high link enters the queue and the loop's only break condition is `priority === 'high'`. fits() keeps returning false (active pill is wider — has the `.active` background/padding), so the loop walks the entire non-high tail and orphans the active route in More. The acceptance criterion "Active-route pill MUST always be visible inline" was never encoded — #1311's floor only protected `data-priority="high"`. ## Why prior #1311 / #1148 / #1139 floors didn't catch this - **#1311** floored at `data-priority="high"` only. `/#/perf` is `data-priority=""` so it had no protection. - **#1148 / #1139** floored the *More menu* at ≥2 items but didn't constrain *which* links could be promoted/dropped. - **#1106** narrow-desktop CSS branch (≤1100) was written before active-pill width drift was a known issue. ## Fix One conceptual rule applied at three points: 1. In `overflowQueue` construction, skip any link with `.active` (treat active like high-priority — never enqueue). 2. In the ≤1100 CSS branch, skip the active link when assigning `.is-overflow`. 3. In the >1100 loop, also break on `.active` (defensive — queue already excludes it). Approach chosen over "pin active-pill max-width during measurement": measurement-pinning would silently shrink the pill visually mid-resize, and width drift from #1378's new `--mc-*` vars made that fragile. Treating active as a hard inline pin matches the documented contract and is one greppable invariant. ## TDD red → green - **Red commit `34d69012`:** added `test-nav-priority-1391-e2e.js` covering `/#/perf, /#/audio-lab, /#/analytics, /#/observers` at `1024, 1080, 1100, 1101, 1200, 1300px`. Asserts (1) active pill not in overflow, (2) all 5 high-pri still inline (#1311 guard), (3) every overflowed link mirrored in More dropdown (no orphans). 0/24 passed locally on red. - **Green commit:** same test 24/24 pass. Existing #1311 (20/20), #1139 floor, #1102 contract still green. ## Manual verification Local fixture server (`./corescope-server -port 13581 -db test-fixtures/e2e-fixture.db -public public`): - `/#/perf` @ 1080×800: brand + 5 high-pri inline + "Perf" pill inline + "More ▾" containing the 5 low-pri links (Channels, Tools, Observers, Analytics, Audio Lab). ✅ - `/#/perf` @ 1300×800: brand + 5 high-pri + "Perf" inline; More hidden (only 4 low-pri items overflow). ✅ - `/#/perf` @ 800×800 (narrow): hamburger code path untouched. ✅ - Inverse `/#/home` @ 1080×800 (active IS high-pri): no behaviour change. ✅ ## Preflight `bash ~/.openclaw/skills/pr-preflight/scripts/run-all.sh origin/master` — exit 0. Browser verified: local fixture server + Playwright on Chromium (`/usr/bin/chromium`). E2E assertion added: `test-nav-priority-1391-e2e.js:138-148` (`activeOverflowed === false`). Fixes #1391 --------- Co-authored-by: openclaw-bot --- .github/workflows/deploy.yml | 1 + public/app.js | 41 +++++++- test-nav-priority-1102-e2e.js | 46 ++++++--- test-nav-priority-1391-e2e.js | 183 ++++++++++++++++++++++++++++++++++ 4 files changed, 250 insertions(+), 21 deletions(-) create mode 100644 test-nav-priority-1391-e2e.js diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index dbdb91fe..85664485 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -265,6 +265,7 @@ jobs: CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-fluid-1055-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-priority-1102-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-priority-1311-e2e.js 2>&1 | tee -a e2e-output.txt + CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-priority-1391-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-nav-more-floor-1139-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-bottom-nav-1061-e2e.js 2>&1 | tee -a e2e-output.txt CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-gestures-1062-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/public/app.js b/public/app.js index 04904a80..bb264625 100644 --- a/public/app.js +++ b/public/app.js @@ -1133,9 +1133,23 @@ window.addEventListener('DOMContentLoaded', () => { // only signal — if you ever need finer ordering, switch to a numeric // attribute (e.g. data-overflow-order="3") rather than re-shuffling // index in HTML. - const overflowQueue = allLinks.filter(a => a.dataset.priority !== 'high') - .reverse() // right-to-left - .concat(allLinks.filter(a => a.dataset.priority === 'high').reverse()); + // #1391: ALSO exclude the currently-active link from the queue. + // The active pill has wider rendered width (background + padding), + // and acceptance for #1391 requires "Active-route pill MUST always + // be visible inline (never overflowed to More) at any viewport + // ≥768px." The queue is rebuilt on hashchange (applyNavPriority + // is wired to hashchange below), so the exclusion tracks the + // current route automatically. + function buildOverflowQueue() { + var isPinned = function(a) { + return a.dataset.priority === 'high' || a.classList.contains('active'); + }; + return allLinks.filter(a => !isPinned(a)) + .reverse() // right-to-left + .concat(allLinks.filter(a => a.dataset.priority === 'high' && !a.classList.contains('active')).reverse()); + } + var overflowQueue = buildOverflowQueue(); + function rebuildMoreMenu() { navMoreMenu.innerHTML = ''; @@ -1194,7 +1208,14 @@ window.addEventListener('DOMContentLoaded', () => { // owns the decision (and at 2560px nothing overflows). if (window.innerWidth <= 1100) { allLinks.forEach(a => { - if (a.dataset.priority !== 'high') a.classList.add('is-overflow'); + // #1391: never overflow the active-route pill, even in the + // narrow-desktop CSS branch — acceptance requires it stay + // inline at any viewport ≥768px. Without this guard, a + // non-high-priority active route (e.g. /#/perf) would be + // shoved into More alongside the rest. + if (a.dataset.priority !== 'high' && !a.classList.contains('active')) { + a.classList.add('is-overflow'); + } }); rebuildMoreMenu(); return; @@ -1251,6 +1272,11 @@ window.addEventListener('DOMContentLoaded', () => { return needed <= window.innerWidth; } let i = 0; + // #1391: rebuild queue here so it reflects the CURRENT active + // link (hashchange wakes applyNavPriority, but the queue was + // captured at init-time; we need to re-evaluate which link is + // active on every run). Cheap — just filters allLinks twice. + overflowQueue = buildOverflowQueue(); // #1311 floor: protect data-priority="high" links from being // dropped by the greedy fit loop. The bug was that on a non-high // active route (e.g. /#/perf, /#/audio-lab) at ~1101-1200px, the @@ -1263,8 +1289,13 @@ window.addEventListener('DOMContentLoaded', () => { // still doesn't fit at that point, that's a layout issue (e.g. // shrink the active pill, drop nav-stats earlier) — never the // measurer's call to delete primary navigation. + // + // #1391: also break on .active — buildOverflowQueue already + // excludes the active link from the queue, but the break is a + // defensive belt for any future code that re-enqueues it. while (!fits() && i < overflowQueue.length) { if (overflowQueue[i].dataset.priority === 'high') break; + if (overflowQueue[i].classList.contains('active')) break; overflowQueue[i].classList.add('is-overflow'); i++; } @@ -1283,7 +1314,7 @@ window.addEventListener('DOMContentLoaded', () => { // it just to satisfy the >=2 More-menu floor. A degenerate // 1-item dropdown is a smaller UX paper-cut than nuking a // primary nav link. - if (i < overflowQueue.length && overflowQueue[i].dataset.priority !== 'high') { + if (i < overflowQueue.length && overflowQueue[i].dataset.priority !== 'high' && !overflowQueue[i].classList.contains('active')) { overflowQueue[i].classList.add('is-overflow'); i++; } else { diff --git a/test-nav-priority-1102-e2e.js b/test-nav-priority-1102-e2e.js index 64db40b3..d1e76ffe 100644 --- a/test-nav-priority-1102-e2e.js +++ b/test-nav-priority-1102-e2e.js @@ -143,12 +143,14 @@ async function main() { } } - // #1105 MINOR 9: when at a collapsed width, navigating to a route - // whose link overflows into the More menu must light up #navMoreBtn - // with .active. Verifies rebuildMoreMenu() correctly mirrors the - // active state from the inline (cloned) link to the More button on - // each hashchange (applyNavPriority is wired to hashchange and runs - // after the route handler's class toggles). + // #1105 MINOR 9 (updated by #1391): the active-route pill is now + // PINNED inline at any viewport ≥768px — even if it is not a + // data-priority="high" link. So when we navigate to /#/observers + // (non-high) at 1080px, the observers link MUST stay inline and the + // More menu MUST NOT contain it. The navMoreBtn .active mirror only + // fires when the active route is actually in the dropdown — under + // #1391 that can no longer happen at any width ≥768px, so this test + // verifies the inverse contract. await page.setViewportSize({ width: 1080, height: HEIGHT }); await page.goto(`${BASE}/#/observers`, { waitUntil: 'domcontentloaded' }); await page.waitForSelector('.top-nav .nav-links'); @@ -171,29 +173,41 @@ async function main() { const activeMirror = await page.evaluate(() => { const observersInline = document.querySelector('.nav-links .nav-link[href="#/observers"]'); const inlineHidden = observersInline && observersInline.classList.contains('is-overflow'); + const inlineActive = observersInline && observersInline.classList.contains('active'); + const inlineWidth = observersInline ? observersInline.getBoundingClientRect().width : 0; const moreBtn = document.getElementById('navMoreBtn'); const moreBtnActive = moreBtn ? moreBtn.classList.contains('active') : false; - const moreMenuActiveHrefs = Array.from(document.querySelectorAll('#navMoreMenu .nav-link.active')) + const moreMenuHrefs = Array.from(document.querySelectorAll('#navMoreMenu .nav-link')) .map(a => a.getAttribute('href')); - return { inlineHidden, moreBtnActive, moreMenuActiveHrefs }; + return { inlineHidden, inlineActive, inlineWidth, moreBtnActive, moreMenuHrefs }; }); const mirrorReasons = []; - if (!activeMirror.inlineHidden) { - mirrorReasons.push('precondition: #/observers should be in the More menu at 1080px (not visible inline)'); + // #1391: active link MUST stay inline (not overflowed). + if (activeMirror.inlineHidden) { + mirrorReasons.push('#1391 contract: #/observers is active route — MUST stay inline at 1080px, not in More'); } - if (!activeMirror.moreBtnActive) { - mirrorReasons.push('navMoreBtn missing .active class while #/observers is the active route'); + if (!activeMirror.inlineActive) { + mirrorReasons.push('inline #/observers link missing .active class'); } - if (!activeMirror.moreMenuActiveHrefs.includes('#/observers')) { - mirrorReasons.push(`More-menu clone of #/observers missing .active (active hrefs in menu: [${activeMirror.moreMenuActiveHrefs.join(', ')}])`); + if (activeMirror.inlineWidth === 0) { + mirrorReasons.push('inline #/observers has zero width (clipped)'); + } + // #1391: navMoreBtn should NOT have .active because the active link + // is inline, not in the dropdown. + if (activeMirror.moreBtnActive) { + mirrorReasons.push('navMoreBtn has .active but active route #/observers is inline (mirror should be off)'); + } + // #1391: More menu must NOT contain the active link. + if (activeMirror.moreMenuHrefs.includes('#/observers')) { + mirrorReasons.push(`More menu contains active route #/observers (must be inline only): menu=[${activeMirror.moreMenuHrefs.join(', ')}]`); } if (mirrorReasons.length === 0) { passes++; - console.log(` ✅ active-mirror @1080 #/observers: navMoreBtn.active=true, menu .active=#/observers`); + console.log(` ✅ active-pinned @1080 #/observers: inline + .active set, More mirror off, menu excludes active`); } else { failures++; - console.log(` ❌ active-mirror @1080 #/observers: ${mirrorReasons.join(' | ')}`); + console.log(` ❌ active-pinned @1080 #/observers: ${mirrorReasons.join(' | ')}`); } await browser.close(); diff --git a/test-nav-priority-1391-e2e.js b/test-nav-priority-1391-e2e.js new file mode 100644 index 00000000..27bdd30f --- /dev/null +++ b/test-nav-priority-1391-e2e.js @@ -0,0 +1,183 @@ +#!/usr/bin/env node +/* Issue #1391 — 20th Priority+ nav regression. + * + * Symptom: at viewport ~1080-1200px on a non-high-priority active route + * (e.g. /#/perf, /#/audio-lab), the active-route pill is shoved into the + * More dropdown instead of staying visible inline. Operator screenshot at + * ~1080px on /#/perf showed the navbar with only the "Perf" pill visible + * (or, in the inverse failure mode, NO inline pill at all, with More + * containing only the orphaned active route). + * + * Acceptance (from issue #1391): + * - Active-route pill MUST always be visible inline (never overflowed + * to More) at any viewport ≥768px. + * - If active route is NOT a high-priority link (e.g. /#/perf), the + * high-priority links MUST still be inline ≥768px. + * - Every link in overflow MUST be reachable via the More dropdown + * (the existing #1311/#1139 contract — don't regress). + * + * Mutation guard: removing the "pin active inline" rule in applyNavPriority + * must make this test fail (active link gets overflowed at 1080px on /#/perf). + */ +'use strict'; + +const assert = require('node:assert'); +const { chromium } = require('playwright'); + +const BASE = process.env.BASE_URL || 'http://localhost:13581'; +const HIGH_PRIORITY_HREFS = ['#/home', '#/packets', '#/map', '#/live', '#/nodes']; + +// Routes whose link is NOT data-priority="high" (verified via +// `grep data-priority public/index.html`). These exercise the +// "active pill is non-high" branch where the bug surfaces. +const NON_HIGH_ROUTES = ['#/perf', '#/audio-lab', '#/analytics', '#/observers']; + +// Operator screenshot was ~1080px. Cover the narrow-desktop CSS branch +// (≤1100) AND the measurement-loop branch (>1100) — bug reproduces in +// both, and the #1311 fix only addressed >1100. +const WIDTHS = [1024, 1080, 1100, 1101, 1200, 1300]; +const HEIGHT = 800; + +async function main() { + let browser; + try { + browser = await chromium.launch({ + headless: true, + executablePath: process.env.CHROMIUM_PATH || undefined, + args: ['--no-sandbox', '--disable-gpu', '--disable-dev-shm-usage'], + }); + } catch (err) { + if (process.env.CHROMIUM_REQUIRE === '1') { + console.error(`test-nav-priority-1391-e2e.js: FAIL — Chromium required but unavailable: ${err.message}`); + process.exit(1); + } + console.log(`test-nav-priority-1391-e2e.js: SKIP (Chromium unavailable: ${err.message.split('\n')[0]})`); + process.exit(0); + } + + let failures = 0; + let passes = 0; + const ctx = await browser.newContext(); + const page = await ctx.newPage(); + page.setDefaultTimeout(15000); + + for (const w of WIDTHS) { + for (const route of NON_HIGH_ROUTES) { + await page.setViewportSize({ width: w, height: HEIGHT }); + await page.goto(`${BASE}/${route}`, { waitUntil: 'domcontentloaded' }); + await page.waitForSelector('.top-nav .nav-links'); + await page.evaluate(() => document.fonts && document.fonts.ready ? document.fonts.ready : null); + // Settle layout (two consecutive frames identical for nav-right). + await page.waitForFunction(() => { + const el = document.querySelector('.top-nav .nav-right'); + if (!el) return false; + const r1 = el.getBoundingClientRect(); + return new Promise((resolve) => { + requestAnimationFrame(() => requestAnimationFrame(() => { + const r2 = el.getBoundingClientRect(); + resolve(r1.right === r2.right && r1.left === r2.left); + })); + }); + }, null, { timeout: 5000 }); + await page.evaluate(() => new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r)))); + + const data = await page.evaluate((route) => { + const links = Array.from(document.querySelectorAll('.nav-links .nav-link')); + let activeHref = null; + let activeOverflowed = false; + let activeWidth = 0; + const visibleHighPri = []; + const overflowedHighPri = []; + for (const a of links) { + const href = a.getAttribute('href'); + const isActive = a.classList.contains('active'); + const isOverflow = a.classList.contains('is-overflow'); + const w = a.getBoundingClientRect().width; + if (isActive) { + activeHref = href; + activeOverflowed = isOverflow; + activeWidth = w; + } + if (a.dataset.priority === 'high') { + if (isOverflow || w === 0) overflowedHighPri.push({ href, isOverflow, w }); + else visibleHighPri.push(href); + } + } + // Open More dropdown and capture its items (clones live in + // .nav-more-menu, the originals stay in .nav-links). + const moreBtn = document.getElementById('navMoreBtn'); + const moreWrap = document.querySelector('.nav-more-wrap'); + const moreMenu = document.getElementById('navMoreMenu'); + const moreVisible = moreWrap && !moreWrap.classList.contains('is-hidden'); + const moreItems = moreMenu + ? Array.from(moreMenu.querySelectorAll('.nav-link')).map(a => a.getAttribute('href')) + : []; + // Every inline-overflowed link must appear in the More dropdown + // (otherwise it's unreachable). + const overflowedHrefs = links + .filter(a => a.classList.contains('is-overflow')) + .map(a => a.getAttribute('href')); + const missingFromMore = overflowedHrefs.filter(h => !moreItems.includes(h)); + return { + activeHref, activeOverflowed, activeWidth, + visibleHighPri, overflowedHighPri, + moreVisible, moreItems, overflowedHrefs, missingFromMore, + }; + }, route); + + const tag = `${w}px @ ${route}`; + const expectedActive = route; + + try { + // (1) Active pill is correctly identified and present inline. + assert.strictEqual( + data.activeHref, expectedActive, + `${tag}: expected active=${expectedActive}, got ${data.activeHref}` + ); + assert.strictEqual( + data.activeOverflowed, false, + `${tag}: active-route pill ${expectedActive} MUST NOT be in overflow ` + + `(was overflowed=${data.activeOverflowed}, width=${data.activeWidth})` + ); + assert.ok( + data.activeWidth > 0, + `${tag}: active-route pill ${expectedActive} must have non-zero width inline ` + + `(got width=${data.activeWidth})` + ); + + // (2) All high-priority links must be inline (regression guard for #1311). + assert.deepStrictEqual( + [...data.visibleHighPri].sort(), + [...HIGH_PRIORITY_HREFS].sort(), + `${tag}: expected all 5 high-pri inline, got [${data.visibleHighPri.join(', ')}] ` + + `overflowed=[${data.overflowedHighPri.map(o => o.href).join(', ')}]` + ); + + // (3) Every overflowed link is reachable via the More dropdown + // (no orphaned overflow links). + assert.deepStrictEqual( + data.missingFromMore, [], + `${tag}: overflowed links missing from More dropdown: [${data.missingFromMore.join(', ')}] ` + + `(more=[${data.moreItems.join(', ')}])` + ); + + passes++; + console.log(` ✅ ${tag}: active inline + ${data.visibleHighPri.length}/5 high-pri inline + ` + + `More has ${data.moreItems.length} item(s)`); + } catch (e) { + failures++; + console.log(` ❌ ${tag}: ${e.message}`); + } + } + } + + await browser.close(); + const total = WIDTHS.length * NON_HIGH_ROUTES.length; + console.log(`\ntest-nav-priority-1391-e2e.js: ${failures === 0 ? 'OK' : 'FAIL'} — ${passes}/${total} passed`); + process.exit(failures === 0 ? 0 : 1); +} + +main().catch((err) => { + console.error('test-nav-priority-1391-e2e.js: fatal', err); + process.exit(1); +});