fix(editor): capture-phase keydown + popup leak + cache warmup

Addresses Critical #1 + Important #2/#3/#4 from the Task 9 code review.

CRITICAL — Tab/Enter were stolen by CodeJar before the popup handler
saw them. CodeJar registers its keydown listener during construction
(line ~159), so it ran first in bubble order: Tab handler
preventDefaulted and inserted 2 spaces, Enter handler preventDefaulted
+ stopPropagation'd (with leading indent), so the popup-accept either
ran on corrupted state or never fired at all. Fix: register the popup
listener with {capture: true} and call stopPropagation on the keys we
own — that way capture phase fires before CodeJar's bubble listener
and the key is fully consumed by the popup while it's visible. Normal
typing (popup hidden) early-returns without stopPropagation, so
CodeJar's tab-indent + enter-preserve-indent still work when there's
no autocomplete to accept.

IMPORTANT — destroy() leaked the popup <ul> into document.body. Each
mount/destroy cycle (e.g. modal close/reopen) left an orphan popup.
Fix: pop.remove() in destroy().

IMPORTANT — async refreshPopup could race in stale renders if the
first keystroke fired the vocab fetch and the second keystroke
captured a different ctx before the fetch resolved. Fix: warm the
cache with a fire-and-forget loadVocab(language) at mount, so the
first user keystroke hits cache. Eliminates the only realistic window
for the race.

IMPORTANT — acceptCompletion's Range.setStart could throw
IndexSizeError on pathological state (caret inside a tokenized span
where the fragment isn't fully upstream). Fix: try/catch the entire
DOM mutation block, log + dismiss on failure. Plus an inline comment
documenting the single-text-node invariant the current grammars hold.

Plan source updated for the capture-phase fix (most important for
future regeneration); the other fixes are smaller and only mirrored
into the actual code.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
mwiegand 2026-05-16 20:49:23 +02:00
parent 4bace3ab5a
commit 10cf0da3d2
No known key found for this signature in database
2 changed files with 67 additions and 15 deletions

View file

@ -1065,26 +1065,43 @@ code.addEventListener("blur", function () {
setTimeout(hidePopup, 100);
});
// IMPORTANT: capture-phase listener + stopPropagation.
//
// CodeJar registers its own keydown handler on this same element during
// construction (line ~159 above). Its Tab handler unconditionally
// preventDefaults and inserts 2 spaces; its Enter handler does the same
// (+ stopPropagation when there's leading indent). Bubble-phase order is
// registration order — CodeJar would consume Tab/Enter before our popup
// handler ever sees them.
//
// Capture-phase listener fires first. Calling stopPropagation here
// prevents CodeJar's bubble handler from running for the keys we own
// while the popup is visible. Normal typing (popup hidden) early-returns
// without stopPropagation so CodeJar's other behavior is preserved.
code.addEventListener("keydown", function (e) {
if (!popup || popup.style.display === "none" || !popupItems.length) return;
if (e.key === "ArrowDown") {
popupActive = (popupActive + 1) % Math.min(popupItems.length, 8);
renderPopup(popup, popupItems, popupActive);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "ArrowUp") {
popupActive =
(popupActive - 1 + Math.min(popupItems.length, 8)) %
Math.min(popupItems.length, 8);
renderPopup(popup, popupItems, popupActive);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "Tab" || e.key === "Enter") {
acceptCompletion(popupItems[popupActive]);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "Escape") {
hidePopup();
e.preventDefault();
e.stopPropagation();
}
});
}, true);
```
- [ ] **Step 4: Manual smoke**

View file

@ -169,6 +169,11 @@
attachOnUpdate(jar);
// Warm the vocab cache so the first keystroke doesn't race a network
// fetch. Fire-and-forget; loadVocab already caches the empty result
// on failure so this is idempotent.
if (language !== "plain") loadVocab(language);
let popup = null;
let popupItems = [];
let popupActive = 0;
@ -201,19 +206,30 @@
return;
}
// Replace the trailing word fragment with the chosen identifier.
const sel = window.getSelection();
const range = sel.getRangeAt(0);
range.setStart(range.endContainer, range.endOffset - ctx.fragment.length);
range.deleteContents();
range.insertNode(document.createTextNode(entry.name));
range.collapse(false);
sel.removeAllRanges();
sel.addRange(range);
// Force CodeJar to re-highlight + emit onUpdate.
// Use instance.jar (not bare `jar` closure) for consistency with
// setLanguage's tear-down-and-remount — the jar reference can be
// swapped at runtime.
instance.jar.updateCode(instance.jar.toString());
// Assumes the fragment lives in one text node — true for the current
// srccfg and bash grammars (identifiers tokenize as a single \b…\b
// chunk). If a future grammar splits identifiers across nodes,
// setStart(endContainer, endOffset - fragment.length) will land
// inside the wrong node and the deleteContents will corrupt text.
// The try/catch below converts IndexSizeError or any other DOM
// exception into a graceful popup-dismiss rather than a broken caret.
try {
const sel = window.getSelection();
const range = sel.getRangeAt(0);
range.setStart(range.endContainer, range.endOffset - ctx.fragment.length);
range.deleteContents();
range.insertNode(document.createTextNode(entry.name));
range.collapse(false);
sel.removeAllRanges();
sel.addRange(range);
// Force CodeJar to re-highlight + emit onUpdate.
// Use instance.jar (not bare `jar` closure) for consistency with
// setLanguage's tear-down-and-remount — the jar reference can be
// swapped at runtime.
instance.jar.updateCode(instance.jar.toString());
} catch (err) {
console.warn("[editor] acceptCompletion failed; dismissing popup", err);
}
hidePopup();
}
@ -251,26 +267,44 @@
setTimeout(hidePopup, 100);
});
// IMPORTANT: capture-phase listener.
//
// CodeJar registers its own keydown handler on this same element during
// construction (window.CodeJar at line ~159 above), and it uses
// preventDefault + insert-tab-spaces on Tab and preventDefault +
// stopPropagation on Enter (with leading indent). Bubble-phase order =
// registration order: CodeJar runs first and consumes those keys before
// our popup handler ever sees them.
//
// Capture phase fires before bubble. By calling stopPropagation here,
// we prevent CodeJar's bubble handler from running for the keys we own
// while the popup is visible. Normal typing (popup hidden) still flows
// through to CodeJar unchanged because we early-return without
// stopPropagation in that case.
code.addEventListener("keydown", function (e) {
if (!popup || popup.style.display === "none" || !popupItems.length) return;
if (e.key === "ArrowDown") {
popupActive = (popupActive + 1) % Math.min(popupItems.length, 8);
renderPopup(popup, popupItems, popupActive);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "ArrowUp") {
popupActive =
(popupActive - 1 + Math.min(popupItems.length, 8)) %
Math.min(popupItems.length, 8);
renderPopup(popup, popupItems, popupActive);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "Tab" || e.key === "Enter") {
acceptCompletion(popupItems[popupActive]);
e.preventDefault();
e.stopPropagation();
} else if (e.key === "Escape") {
hidePopup();
e.preventDefault();
e.stopPropagation();
}
});
}, true);
const instance = {
textarea,
@ -312,6 +346,7 @@
},
destroy: function () {
instance.jar.destroy();
if (popup) popup.remove();
shell.remove();
textarea.style.display = "";
delete textarea._codeEditor;