summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAgando <roduccomm@gmail.com>2026-11-07 18:56:04 +0100
committerAgando <roduccomm@gmail.com>2026-11-07 18:56:04 +0100
commitfc24fdbe5d5bb504a967989122a66df9f9a7c6d7 (patch)
tree63a9cad462aef2dcc2899dbcc26a22675bc77679
parente894b666fd9d51f41fd9f9c7a2a3e0cf4fef2511 (diff)
fix(newos4): four-model review — lazy-load race, restore order, a11y, key handling
Reviewed the shipped change set with kimi-k2.7-code, deepseek-v4-pro, glm-5.3 and minimax-m3. Findings below are the ones that survived verification against file:line; the rest are in .hermes/plans/2026-11-07_163420-newos4-six-ui-modes.review-log.md Accepted and fixed: - apply() had no generation token: a lazy mode load that finished after the user switched to another mode force-applied the abandoned one (`apply(want,{force:true})` in the .then). Every apply() now bumps `_loadSeq` and the callback acts only while its token is newest. Same token also removes a double mount: registerMode() already applies the mode as it registers, so the callback was mounting every lazy mode twice (teardown+cleanup twice). - _restoreStock() used the recorded `nextSibling` as insert anchor. Every real mode moves SEVERAL stock nodes (terminal, input, status bar), so the recorded sibling is inside the mode container and every insert fell through to append. Now anchors on the next stock node that is already back home. A raw child index also fails (elements removed shift the indices) — measured order terminal,input,status-bar,quick-actions. - _abortToClassic() left the broken mode persisted and `_pending` set, so the next reload retried the same failure with no way out. It now clears both and stores 'classic'. - ?ui= reached classList.add('wx11-ui-' + id) unvalidated: a space or colon throws a DOMException and boot dies. Validated against /^[a-z][a-z0-9-]{0,15}$/ and no longer persisted (the comment always claimed it was a request, not a preference). - Modes hide .prompt-label, so #command-input had no accessible name at all. The shell sets aria-label once when a mode reparents it. - cockpit: preventDefault on all F1-F8 killed help, the address bar and devtools. F1/F12 are left to the browser and nothing is swallowed when focus is in a text field. - commander: Ctrl/Cmd+K opened the palette even while typing in the composer. Ignored from inside editable elements now. - picker: role=dialog + aria-modal + aria-labelledby + focus on the first card. - api.capture(): strip the quoting characters from argv (capturePipeOutput takes argv). - stale `wx11-ui-*` body class removed on switch instead of accumulating. Rejected with evidence: - "loadModuleFromURL may not return a boolean" — modules/core/module-manager.js:56 returns true on success and :60 false in the catch. Verified by reading the function. - "_abortToClassic does not clear _pending" — it did not; that one is accepted above. The sibling claim that it "leaves the broken mode persisted" was verified by reading the function: it never touched localStorage, so also accepted. - "mm.LATEST_VER / webx11_cache_ver are never set, so the mode URL falls back to ?v=0" — modules/core/module-loader.js:12 sets window.webx11.moduleManager.LATEST_VER and :14-15 writes webx11_cache_ver. Verified live: the mode fetch carries ?v=v4.6.152. - "moduleManager.modules may be a Map" — it is a plain object (terminal.moduleManager.modules .CryptoGzipModule is the documented access path), so ui-commander's Object.keys works. Verified: node modules/test/test-ui-shell.js -> 25/25 (the test now moves TWO stock nodes; a raw-index restore fails it with "terminal-container,input-area,status-bar,quick-actions"); probe-ui-modes.py --cycle against production -> ALL PASS, 13 checks.
-rw-r--r--modules/core/core-bundle.js64
-rw-r--r--modules/core/module-uishell.js64
-rw-r--r--modules/ext/ui-cockpit.js6
-rw-r--r--modules/ext/ui-commander.js4
-rw-r--r--modules/test/test-ui-shell.js6
5 files changed, 122 insertions, 22 deletions
diff --git a/modules/core/core-bundle.js b/modules/core/core-bundle.js
index 924ab14..07be2ce 100644
--- a/modules/core/core-bundle.js
+++ b/modules/core/core-bundle.js
@@ -22979,6 +22979,7 @@ class UIShellModule {
this._root = null;
this._cleanup = null;
this._where = null; // [{ el, parent, next }] captured on first mount
+ this._loadSeq = 0; // bumps on every apply(): a stale lazy load must never win
this._terminalEl = null; // identity of the one real output node
this._cssIds = [];
this._pending = null; // mode requested before its ext module loaded
@@ -23010,9 +23011,13 @@ class UIShellModule {
let wanted = localStorage.getItem(this.key) || 'classic';
try {
const q = new URLSearchParams(location.search).get('ui');
- if (q) { wanted = q; this._fromQuery = true; }
+ // Validate before it reaches classList.add('wx11-ui-' + id): a space or a colon in the
+ // query throws a DOMException there and boot dies with no interface at all.
+ if (q && /^[a-z][a-z0-9-]{0,15}$/.test(q)) { wanted = q; this._fromQuery = true; }
} catch (e) { /* no URLSearchParams: ignore the request */ }
- const applied = this.apply(wanted, { silent: true, persist: this._fromQuery });
+ // persist:false — a shared link requests a view, it must not rewrite the user's own default
+ // (the code comment already claimed this; the flag did the opposite).
+ const applied = this.apply(wanted, { silent: true, persist: false });
if (!applied) this._pending = wanted; // ext modules register ~800 ms later
// A requested/stored mode that never registers (404, parse error) is forgotten rather than
@@ -23064,6 +23069,9 @@ class UIShellModule {
apply(id, opts) {
const o = opts || {};
const want = id || 'classic';
+ // Every apply supersedes an in-flight lazy load. Without this token a load that finishes
+ // after the user moved on force-applies the abandoned mode (two reviewers, same finding).
+ const token = ++this._loadSeq;
// A user-initiated switch cancels a pending boot restore, or a slow ext module would later
// yank the interface back to whatever boot asked for.
if (!o.silent) { this._pending = null; this._closePicker(); }
@@ -23078,12 +23086,15 @@ class UIShellModule {
this._pending = want;
if (this.terminal) this.toast('Loading ' + want + '…', 1500);
this._loadMode(want).then((ok) => {
+ // registerMode() applies the mode the moment it registers — which already bumped this
+ // token — and so does a user switch. Only act while this load is still the newest.
+ if (this._loadSeq !== token) return;
if (ok) this.apply(want, { force: true, retried: true, persist: o.persist });
else if (this.terminal) {
this.terminal.print(`UI mode '${want}' could not be loaded — available: ` +
(['classic'].concat(Object.keys(this.modes)).join(', ')), 'warning');
}
- });
+ }).catch((e) => this._loadFail(want, 'load rejected: ' + (e && e.message)));
return false;
}
if (want === this.current && !o.force) {
@@ -23098,6 +23109,11 @@ class UIShellModule {
this._teardown();
this.current = want;
document.body.dataset.uiMode = want;
+ // Drop the previous mode's class: two wx11-ui-* classes at once keep any rule keyed on the
+ // old one matching, and the next switch then has to clean up after two modes.
+ Array.from(document.body.classList)
+ .filter((c) => c.indexOf('wx11-ui-') === 0 && c !== 'wx11-ui-' + want)
+ .forEach((c) => document.body.classList.remove(c));
document.body.classList.add('wx11-ui-' + want);
if (o.persist !== false) localStorage.setItem(this.key, want);
@@ -23123,6 +23139,9 @@ class UIShellModule {
return false;
}
this._dockFooter();
+ // Every mode hides .prompt-label, so without this the real input has no accessible name.
+ const inp = document.getElementById('command-input');
+ if (inp && !inp.getAttribute('aria-label')) inp.setAttribute('aria-label', 'Terminal command input');
document.dispatchEvent(new CustomEvent('wx11:ui-changed', { detail: { mode: want } }));
return true;
}
@@ -23131,6 +23150,10 @@ class UIShellModule {
if (this.terminal) this.terminal.print(`UI mode '${id}' ${why} — reverted to classic.`, 'error');
try { if (this._cleanup) this._cleanup(); } catch (e) { /* mode is already broken */ }
this._cleanup = null;
+ this._pending = null; // a sweep or an in-flight load must not re-apply it
+ // Roll the stored preference back: leaving the broken mode persisted means every reload fails
+ // the same way, with no way out except devtools.
+ try { localStorage.setItem(this.key, 'classic'); } catch (e) { /* private mode */ }
this._restoreStock();
if (this._root) this._root.innerHTML = '';
this.current = 'classic';
@@ -23209,16 +23232,25 @@ class UIShellModule {
_snapshotStock() {
if (this._where) return;
this._terminalEl = document.getElementById('terminal-container');
- this._where = this._stockEls().map(el => ({ el, parent: el.parentNode, next: el.nextSibling || null }));
+ this._where = this._stockEls().map(el => ({
+ el,
+ parent: el.parentNode,
+ index: Array.prototype.indexOf.call(el.parentNode.children, el),
+ }));
}
_restoreStock() {
if (!this._where) return;
- for (const w of this._where) {
- if (!w.parent) continue;
- const ref = w.next && w.next.parentNode === w.parent ? w.next : null;
- try { w.parent.insertBefore(w.el, ref); } catch (e) { /* detached parent */ }
- }
+ // Ascending original index, anchored on the next stock node that is ALREADY home. Neither
+ // `nextSibling` (a mode moves several nodes, so the recorded sibling lives inside the mode)
+ // nor a raw child index (removing nodes shifts every later index) reproduces the order —
+ // measured: `body.children[idx]` gave terminal,input,status-bar,quick-actions.
+ const list = this._where.slice().sort((a, b) => a.index - b.index);
+ list.forEach((w) => {
+ if (!w.parent || w.el.parentNode === w.parent) return; // already in place
+ const anchor = list.find((c) => c.index > w.index && c.el.parentNode === w.parent);
+ try { w.parent.insertBefore(w.el, anchor ? anchor.el : null); } catch (e) { /* detached */ }
+ });
}
/* ---------- chrome ---------- */
@@ -23352,7 +23384,9 @@ class UIShellModule {
// Do NOT use terminal._parseArgs — it does not exist on the terminal instance; every
// panel rendered "unavailable: term._parseArgs is not a function" (measured 2026-11-07).
const raw = String(line).trim();
- const parts = raw.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || [];
+ // Strip the quoting characters: capturePipeOutput receives argv, not shell words.
+ const parts = (raw.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || [])
+ .map((a) => a.replace(/^["']/, '').replace(/["']$/, ''));
const cmd = parts[0];
if (!cmd) return Promise.resolve('');
return term.capturePipeOutput(cmd, parts.slice(1), raw);
@@ -23406,15 +23440,23 @@ class UIShellModule {
const self = this;
const wrap = document.createElement('div');
wrap.id = 'wx11-ui-picker';
+ // A full-screen overlay that is not announced as a dialog is a keyboard trap in reverse: Tab
+ // walks straight into the page behind it. (Found by review.)
+ wrap.setAttribute('role', 'dialog');
+ wrap.setAttribute('aria-modal', 'true');
+ wrap.setAttribute('aria-labelledby', 'wx11-ui-picker-title');
wrap.style.cssText = 'position:fixed;inset:0;z-index:9998;background:rgba(0,0,0,.86);' +
'display:flex;flex-direction:column;justify-content:center;padding:20px;overflow:auto;gap:10px;';
wrap.innerHTML = '<div style="max-width:720px;margin:0 auto;width:100%">' +
- '<h2 style="margin:0 0 4px;font-size:1.15em">How do you want newOS4 to look?</h2>' +
+ '<h2 id="wx11-ui-picker-title" style="margin:0 0 4px;font-size:1.15em">How do you want newOS4 to look?</h2>' +
'<p class="wx11-dim" style="margin:0 0 14px">Pick one — you can switch any time with <code>ui</code>.</p>' +
'<div id="wx11-ui-picker-list" style="display:grid;gap:10px"></div>' +
'<button class="wx11-btn" id="wx11-ui-picker-skip" style="margin-top:12px;width:100%;text-align:center">' +
'Keep the classic terminal</button></div>';
document.body.appendChild(wrap);
+ // Focus the first card so Escape/Tab/Enter work without a pointer.
+ const firstCard = wrap.querySelector('#wx11-ui-picker-list button');
+ if (firstCard && firstCard.focus) firstCard.focus();
const list = wrap.querySelector('#wx11-ui-picker-list');
// Fewer than 4 registered means the ext modules are still in flight — use the static list so
// the user never sees a one-card chooser (the card handler re-checks and warns if missing).
diff --git a/modules/core/module-uishell.js b/modules/core/module-uishell.js
index ac14b20..9914ec0 100644
--- a/modules/core/module-uishell.js
+++ b/modules/core/module-uishell.js
@@ -18,6 +18,7 @@ class UIShellModule {
this._root = null;
this._cleanup = null;
this._where = null; // [{ el, parent, next }] captured on first mount
+ this._loadSeq = 0; // bumps on every apply(): a stale lazy load must never win
this._terminalEl = null; // identity of the one real output node
this._cssIds = [];
this._pending = null; // mode requested before its ext module loaded
@@ -49,9 +50,13 @@ class UIShellModule {
let wanted = localStorage.getItem(this.key) || 'classic';
try {
const q = new URLSearchParams(location.search).get('ui');
- if (q) { wanted = q; this._fromQuery = true; }
+ // Validate before it reaches classList.add('wx11-ui-' + id): a space or a colon in the
+ // query throws a DOMException there and boot dies with no interface at all.
+ if (q && /^[a-z][a-z0-9-]{0,15}$/.test(q)) { wanted = q; this._fromQuery = true; }
} catch (e) { /* no URLSearchParams: ignore the request */ }
- const applied = this.apply(wanted, { silent: true, persist: this._fromQuery });
+ // persist:false — a shared link requests a view, it must not rewrite the user's own default
+ // (the code comment already claimed this; the flag did the opposite).
+ const applied = this.apply(wanted, { silent: true, persist: false });
if (!applied) this._pending = wanted; // ext modules register ~800 ms later
// A requested/stored mode that never registers (404, parse error) is forgotten rather than
@@ -103,6 +108,9 @@ class UIShellModule {
apply(id, opts) {
const o = opts || {};
const want = id || 'classic';
+ // Every apply supersedes an in-flight lazy load. Without this token a load that finishes
+ // after the user moved on force-applies the abandoned mode (two reviewers, same finding).
+ const token = ++this._loadSeq;
// A user-initiated switch cancels a pending boot restore, or a slow ext module would later
// yank the interface back to whatever boot asked for.
if (!o.silent) { this._pending = null; this._closePicker(); }
@@ -117,12 +125,15 @@ class UIShellModule {
this._pending = want;
if (this.terminal) this.toast('Loading ' + want + '…', 1500);
this._loadMode(want).then((ok) => {
+ // registerMode() applies the mode the moment it registers — which already bumped this
+ // token — and so does a user switch. Only act while this load is still the newest.
+ if (this._loadSeq !== token) return;
if (ok) this.apply(want, { force: true, retried: true, persist: o.persist });
else if (this.terminal) {
this.terminal.print(`UI mode '${want}' could not be loaded — available: ` +
(['classic'].concat(Object.keys(this.modes)).join(', ')), 'warning');
}
- });
+ }).catch((e) => this._loadFail(want, 'load rejected: ' + (e && e.message)));
return false;
}
if (want === this.current && !o.force) {
@@ -137,6 +148,11 @@ class UIShellModule {
this._teardown();
this.current = want;
document.body.dataset.uiMode = want;
+ // Drop the previous mode's class: two wx11-ui-* classes at once keep any rule keyed on the
+ // old one matching, and the next switch then has to clean up after two modes.
+ Array.from(document.body.classList)
+ .filter((c) => c.indexOf('wx11-ui-') === 0 && c !== 'wx11-ui-' + want)
+ .forEach((c) => document.body.classList.remove(c));
document.body.classList.add('wx11-ui-' + want);
if (o.persist !== false) localStorage.setItem(this.key, want);
@@ -162,6 +178,9 @@ class UIShellModule {
return false;
}
this._dockFooter();
+ // Every mode hides .prompt-label, so without this the real input has no accessible name.
+ const inp = document.getElementById('command-input');
+ if (inp && !inp.getAttribute('aria-label')) inp.setAttribute('aria-label', 'Terminal command input');
document.dispatchEvent(new CustomEvent('wx11:ui-changed', { detail: { mode: want } }));
return true;
}
@@ -170,6 +189,10 @@ class UIShellModule {
if (this.terminal) this.terminal.print(`UI mode '${id}' ${why} — reverted to classic.`, 'error');
try { if (this._cleanup) this._cleanup(); } catch (e) { /* mode is already broken */ }
this._cleanup = null;
+ this._pending = null; // a sweep or an in-flight load must not re-apply it
+ // Roll the stored preference back: leaving the broken mode persisted means every reload fails
+ // the same way, with no way out except devtools.
+ try { localStorage.setItem(this.key, 'classic'); } catch (e) { /* private mode */ }
this._restoreStock();
if (this._root) this._root.innerHTML = '';
this.current = 'classic';
@@ -248,16 +271,25 @@ class UIShellModule {
_snapshotStock() {
if (this._where) return;
this._terminalEl = document.getElementById('terminal-container');
- this._where = this._stockEls().map(el => ({ el, parent: el.parentNode, next: el.nextSibling || null }));
+ this._where = this._stockEls().map(el => ({
+ el,
+ parent: el.parentNode,
+ index: Array.prototype.indexOf.call(el.parentNode.children, el),
+ }));
}
_restoreStock() {
if (!this._where) return;
- for (const w of this._where) {
- if (!w.parent) continue;
- const ref = w.next && w.next.parentNode === w.parent ? w.next : null;
- try { w.parent.insertBefore(w.el, ref); } catch (e) { /* detached parent */ }
- }
+ // Ascending original index, anchored on the next stock node that is ALREADY home. Neither
+ // `nextSibling` (a mode moves several nodes, so the recorded sibling lives inside the mode)
+ // nor a raw child index (removing nodes shifts every later index) reproduces the order —
+ // measured: `body.children[idx]` gave terminal,input,status-bar,quick-actions.
+ const list = this._where.slice().sort((a, b) => a.index - b.index);
+ list.forEach((w) => {
+ if (!w.parent || w.el.parentNode === w.parent) return; // already in place
+ const anchor = list.find((c) => c.index > w.index && c.el.parentNode === w.parent);
+ try { w.parent.insertBefore(w.el, anchor ? anchor.el : null); } catch (e) { /* detached */ }
+ });
}
/* ---------- chrome ---------- */
@@ -391,7 +423,9 @@ class UIShellModule {
// Do NOT use terminal._parseArgs — it does not exist on the terminal instance; every
// panel rendered "unavailable: term._parseArgs is not a function" (measured 2026-11-07).
const raw = String(line).trim();
- const parts = raw.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || [];
+ // Strip the quoting characters: capturePipeOutput receives argv, not shell words.
+ const parts = (raw.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || [])
+ .map((a) => a.replace(/^["']/, '').replace(/["']$/, ''));
const cmd = parts[0];
if (!cmd) return Promise.resolve('');
return term.capturePipeOutput(cmd, parts.slice(1), raw);
@@ -445,15 +479,23 @@ class UIShellModule {
const self = this;
const wrap = document.createElement('div');
wrap.id = 'wx11-ui-picker';
+ // A full-screen overlay that is not announced as a dialog is a keyboard trap in reverse: Tab
+ // walks straight into the page behind it. (Found by review.)
+ wrap.setAttribute('role', 'dialog');
+ wrap.setAttribute('aria-modal', 'true');
+ wrap.setAttribute('aria-labelledby', 'wx11-ui-picker-title');
wrap.style.cssText = 'position:fixed;inset:0;z-index:9998;background:rgba(0,0,0,.86);' +
'display:flex;flex-direction:column;justify-content:center;padding:20px;overflow:auto;gap:10px;';
wrap.innerHTML = '<div style="max-width:720px;margin:0 auto;width:100%">' +
- '<h2 style="margin:0 0 4px;font-size:1.15em">How do you want newOS4 to look?</h2>' +
+ '<h2 id="wx11-ui-picker-title" style="margin:0 0 4px;font-size:1.15em">How do you want newOS4 to look?</h2>' +
'<p class="wx11-dim" style="margin:0 0 14px">Pick one — you can switch any time with <code>ui</code>.</p>' +
'<div id="wx11-ui-picker-list" style="display:grid;gap:10px"></div>' +
'<button class="wx11-btn" id="wx11-ui-picker-skip" style="margin-top:12px;width:100%;text-align:center">' +
'Keep the classic terminal</button></div>';
document.body.appendChild(wrap);
+ // Focus the first card so Escape/Tab/Enter work without a pointer.
+ const firstCard = wrap.querySelector('#wx11-ui-picker-list button');
+ if (firstCard && firstCard.focus) firstCard.focus();
const list = wrap.querySelector('#wx11-ui-picker-list');
// Fewer than 4 registered means the ext modules are still in flight — use the static list so
// the user never sees a one-card chooser (the card handler re-checks and warns if missing).
diff --git a/modules/ext/ui-cockpit.js b/modules/ext/ui-cockpit.js
index ffb6c2a..92dd60b 100644
--- a/modules/ext/ui-cockpit.js
+++ b/modules/ext/ui-cockpit.js
@@ -106,6 +106,12 @@ body[data-ui-mode="cockpit"] #ui-root { display: grid; grid-template-rows: 1fr 1
const onKey = (ev) => {
const m = /^F([1-8])$/.exec(String(ev.key));
if (!m) return;
+ const t = ev.target;
+ // Never swallow keys aimed at a text field, and leave F1/F12 to the browser: the handler
+ // used to preventDefault on all eight, so help, the address bar and devtools were dead
+ // while the cockpit was open (found by review).
+ if (t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable)) return;
+ if (ev.key === 'F1' || ev.key === 'F12') return;
ev.preventDefault();
const idx = parseInt(m[1], 10);
if (idx === 5) { refresh(); return; } // F5 = refresh, as labelled
diff --git a/modules/ext/ui-commander.js b/modules/ext/ui-commander.js
index 7f980ea..4ffdc8b 100644
--- a/modules/ext/ui-commander.js
+++ b/modules/ext/ui-commander.js
@@ -172,6 +172,10 @@ body[data-ui-mode="commander"] #ui-root { display: grid; grid-template-columns:
if (ev.key === 'Enter' && filtered[sel]) { pick(filtered[sel]); ev.preventDefault(); }
});
const onKey = (ev) => {
+ const t = ev.target;
+ // Ctrl/Cmd+K belongs to the focused field (kill-line in a terminal input); only intercept
+ // it from the page body, or typing in the composer opens the palette instead.
+ if (t && (t.tagName === 'INPUT' || t.tagName === 'TEXTAREA' || t.isContentEditable)) return;
if ((ev.ctrlKey || ev.metaKey) && String(ev.key).toLowerCase() === 'k') {
ev.preventDefault(); openPalette();
}
diff --git a/modules/test/test-ui-shell.js b/modules/test/test-ui-shell.js
index ea4dc84..aced2e0 100644
--- a/modules/test/test-ui-shell.js
+++ b/modules/test/test-ui-shell.js
@@ -128,6 +128,12 @@ shell.registerMode({
const slot = api.el('div', { id: 'probe-slot' });
api.root.appendChild(slot);
api.attach(slot, api.stock.output); // reparent the real terminal
+ // Move a SECOND stock node: every real mode moves the input and the status bar too, and with
+ // only one node moved the `next`-sibling restore heuristic looked correct while being wrong.
+ const slot2 = api.el('div', { id: 'probe-slot2' });
+ api.root.appendChild(slot2);
+ const input = document.getElementById('input-area');
+ if (input) slot2.appendChild(input);
return () => { unmounted++; };
}
});