From 05d72563637e88825ff852d803621c567de47e34 Mon Sep 17 00:00:00 2001 From: Agando Date: Sat, 7 Nov 2026 19:03:38 +0100 Subject: fix(newos4): second review round — identity for every stock node, load dedup, sweep race, a11y MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four models (kimi-k2.7-code, deepseek-v4-pro, minimax-m3, glm-5.3) reviewed the shipped change set. Review log with every accepted and rejected finding: .hermes/plans/2026-11-07_163420-newos4-six-ui-modes.review-log.md - identity guard now covers every snapshot node, not just #terminal-container: terminal.js also binds #output, #command-input, #input-area and #status-bar, so a cloned one leaves the terminal writing into a detached node while the page looks perfect - _restoreStock anchors on the next stock node already home, then on the recorded trailing sibling; a raw child index was measured wrong (terminal,input,status-bar,quick-actions) - _loading map: a double-clicked card or a repeated 'ui easy' loads the mode once - _sweepPending returns while that id is in flight instead of declaring failure at 6 s and overwriting the stored preference - _syncChrome() on every switch (quick-actions was left in the tab order behind the mode root) - picker: role=dialog/aria-modal/labelledby/focus, shown after 600 ms instead of waiting for a registration that never happens with lazy modes, and suppressed when ?ui= already applied - ?ui= must be one of the six; _loadMode only accepts packages['ui-']; hasOwnProperty on modes - VFS names are shell-quoted before reaching api.exec (cat/edit) - refresh generation no longer bumps per tick (slow captures left later panes at '...') - easy: padding shorthand reset the 34 px footer reserve (footer overlapped the composer) - cockpit panes get tabIndex and focus; F1/F12 left to the browser; nothing swallowed in inputs - api.el lost its unused innerHTML sink; the test lost its _parseArgs stub (it hid the regression) - v4.6.155 --- modules/ext/ui-cockpit.js | 6 +++++- modules/ext/ui-commander.js | 11 ++++++++++- modules/ext/ui-easy.js | 4 +++- modules/ext/ui-studio.js | 3 ++- modules/ext/ui-workspace.js | 5 ++++- 5 files changed, 24 insertions(+), 5 deletions(-) (limited to 'modules/ext') diff --git a/modules/ext/ui-cockpit.js b/modules/ext/ui-cockpit.js index 92dd60b..73fbe9f 100644 --- a/modules/ext/ui-cockpit.js +++ b/modules/ext/ui-cockpit.js @@ -62,6 +62,7 @@ body[data-ui-mode="cockpit"] #ui-root { display: grid; grid-template-rows: 1fr 1 defs.forEach((d, i) => { const pane = api.el('div', { class: 'pane' }); pane.dataset.pane = String(i + 1); + pane.tabIndex = -1; // so F1-F8 can actually move focus, not just scroll const h = api.el('h5'); h.appendChild(api.el('span', { text: 'F' + (i + 1) + ' ' + d.label })); h.appendChild(api.el('span', { class: 'n' })); @@ -85,7 +86,9 @@ body[data-ui-mode="cockpit"] #ui-root { display: grid; grid-template-rows: 1fr 1 // Generation guard: eight async captures per tick at 5s — a tick can outlive teardown. let gen = 0; const refresh = async () => { - const my = ++gen; + // Same as workspace: bumping per tick cancelled a slow run before the later panes were + // written, leaving them at "…" indefinitely. + const my = gen; for (const d of defs) { try { const text = await api.capture(d.cmd); @@ -117,6 +120,7 @@ body[data-ui-mode="cockpit"] #ui-root { display: grid; grid-template-rows: 1fr 1 if (idx === 5) { refresh(); return; } // F5 = refresh, as labelled const pane = panes.querySelector('[data-pane="' + idx + '"]'); if (!pane) return; + if (pane.focus) pane.focus(); pane.scrollIntoView({ block: 'nearest' }); pane.style.outline = '1px solid var(--accent-color)'; setTimeout(() => { pane.style.outline = ''; }, 900); diff --git a/modules/ext/ui-commander.js b/modules/ext/ui-commander.js index 4ffdc8b..03af5be 100644 --- a/modules/ext/ui-commander.js +++ b/modules/ext/ui-commander.js @@ -82,7 +82,16 @@ body[data-ui-mode="commander"] #ui-root { display: grid; grid-template-columns: const name = String(entry.name); const isDir = entry.type === 'dir' || entry._virtual; const it = api.el('div', { class: 'item', text: (isDir ? '📁 ' : '📄 ') + name }); - it.addEventListener('click', () => api.exec((isDir ? 'ls ' : 'cat ') + name)); + // Quote the path: a VFS name with a space split into two arguments, and a leading `-` + // became an option flag (found by review). + const quoted = "'" + String(name).replace(/'/g, "'\\''") + "'"; + const run = () => api.exec((isDir ? 'ls ' : 'cat ') + quoted); + it.addEventListener('click', run); + it.tabIndex = 0; + it.setAttribute('role', 'button'); + it.addEventListener('keydown', (ev) => { + if (ev.key === 'Enter' || ev.key === ' ') { ev.preventDefault(); run(); } + }); left.appendChild(it); }); } catch (e) { left.appendChild(api.el('div', { class: 'item', text: '(' + e.message + ')' })); } diff --git a/modules/ext/ui-easy.js b/modules/ext/ui-easy.js index eddf11d..cd1388a 100644 --- a/modules/ext/ui-easy.js +++ b/modules/ext/ui-easy.js @@ -16,7 +16,9 @@ class UIEasyMode { mount(api) { api.injectCSS('easy', ` body[data-ui-mode="easy"] { font-family: system-ui, -apple-system, "Segoe UI", sans-serif; } -body[data-ui-mode="easy"] #ui-root { display: flex; flex-direction: column; padding: 14px 14px 0; gap: 12px; } +/* padding-bottom 34px, not 0: #ui-foot is absolutely positioned and 34px tall, and the shorthand + reset the bottom padding the base sheet reserves, so the footer sat on top of the composer. */ +body[data-ui-mode="easy"] #ui-root { display: flex; flex-direction: column; padding: 14px 14px 34px; gap: 12px; } #easy-top { flex: 0 0 auto; } #easy-top h1 { font-size: 1.25em; margin: 0 0 4px; font-weight: 600; } #easy-actions { display: grid; grid-template-columns: 1fr 1fr; gap: 10px; margin-top: 12px; } diff --git a/modules/ext/ui-studio.js b/modules/ext/ui-studio.js index adda6a1..e8341f3 100644 --- a/modules/ext/ui-studio.js +++ b/modules/ext/ui-studio.js @@ -124,7 +124,8 @@ body[data-ui-mode="studio"] #ui-root { display: grid; grid-template-columns: 1.1 const target = filePath || '/notes.md'; const btn = api.el('button', { class: 'wx11-btn', text: '📝 Open the editor for ' + target }); btn.addEventListener('click', () => { - api.exec('edit ' + target); + // Quoted: a filename with a space would arrive as two arguments (found by review). + api.exec("edit '" + String(target).replace(/'/g, "'\\''") + "'"); api.toast('Editor takes over the chat pane — close it to return'); }); bodyEl.appendChild(btn); diff --git a/modules/ext/ui-workspace.js b/modules/ext/ui-workspace.js index be7a5e0..ee20aac 100644 --- a/modules/ext/ui-workspace.js +++ b/modules/ext/ui-workspace.js @@ -107,7 +107,10 @@ body[data-ui-mode="workspace"] #ui-root { display: flex; flex-direction: column; // a detached tree (invisible work, retained DOM). Every refresh claims a generation. let gen = 0; const refresh = async () => { - const my = ++gen; + // Do NOT bump the generation on every tick: a capture slower than the 15 s interval + // invalidated the in-flight run before it reached the later cards, so those stayed at + // "…" forever under slow storage (found by review). Teardown still bumps gen to stop it. + const my = gen; let changed = 0; for (const c of cards) { try { -- cgit v1.2.3