Local Python autocomplete (Phase 1) - #74
Conversation
|
Implementation handoff for independent review:
The PR intentionally remains draft. The remaining pre-merge validation is a real Android soft-keyboard/IME and native Undo/Redo pass; Python-only/end-of-source/no-dotted/no-AI limits are deliberate. |
s243a
left a comment
There was a problem hiding this comment.
Independent review — Phase 1 local Python autocomplete (head a793975)
Reviewed against docs/AUTOCOMPLETE_DESIGN.md @ 0d0b118. Method: read every changed file, ran test_autocomplete_core.mjs (38/38) and test_autocomplete.mjs (94/94) locally with fetched vendor runtimes and the SW lock verified (v198, 63 entries), confirmed CI green on this exact head, then reproduced every claim below with Playwright against this head before writing it. Pro #48 is the same change (shared modules byte-identical; only comments and one import-helper name differ) — findings apply to both.
What holds up (verified, not assumed)
- No kernel call in the typing path.
completionSymbols()is a prelude-captured lambda over the genuine__main__dict, keys only,type is str(subclasses excluded), bounded 4096/128 — nothing user-defined can run from a keystroke. Measured: 953 names afterimport math, json, sys, refresh 0.5 ms. - Lifecycle generations work as specified: settled-after-success/reported-error/throw, execution-id tagging on the latest start, synchronous invalidation before a slow
destroy(), out-of-order settles, late refresh discarded. Core suite covers each. - Surface contract: Tab accepts, Right Arrow does not, Escape dismisses then falls through to cancel-edit, IME composition suppresses both suggestions and app shortcuts without cancelling the IME default,
execCommand('insertText')gives native Undo/Redo,aria-autocomplete="inline"+aria-hiddenmirror + labelled Accept, teardown on cancel/delete/notebook removal/VFS delete/import, network sentinel. Real keyboard typing in a short cell, a 40-line cell, and a 60-line composer all render and align. - Free/Pro parity is real; the release gating (
ci.yml,build-release.yml, Pages guard with fail-propagatingif/else) is in place.
Findings
1. HIGH — Showing the ghost changes the card's own width, and the fit check then withholds a correct suggestion (desktop, content-sized cards).
Instrumented timeline (40-line Python cell, last line 'w'*45 + ' ', then typed prin):
sync: ta.offsetWidth=477 wrapper=477 mirrorDisplay=none ← before the suggestion
sync: ta.offsetWidth=333 wrapper=333 mirrorDisplay=block ← mirror shown → card collapses 477→333
fits=false mirror.style.width=333px ← measured against the collapsed layout
sync … fits=false / sync … fits=false ← ResizeObserver re-fires, 3 passes, all fail
FINAL shown=false textarea=477 mirror.style.width=333px ← withheld; mirror width left stale
In edit mode the card is content-sized (335 px with a short or wrapped last line, 479 px with a mid-length one), and displaying the mirror alters that size; _syncMirrorMetrics copies the collapsed offsetWidth, _presentationFits rejects, _clearVisual hides the mirror, the card springs back, ResizeObserver reschedules — a loop that converges on no suggestion and costs three forced layouts per keystroke. Reproduced with real keystrokes for last-line lengths of 45–57 characters ('w'*40..52 + ' prin'), both with and without scrollbars. It does not appear at phone widths (the card is viewport-constrained) or in the suite (its cells contain pri only), which is why 94/94 passes.
Fix direction: make the mirror unable to influence its container's intrinsic size (size the wrapper from the textarea, not vice-versa; or measure with the mirror's text set but visibility:hidden before toggling display), and add a browser regression for a long-last-line cell on a ≥800 px viewport.
2. MODERATE — With classic scrollbars the mirror is 15 px wider than the textarea's content box, and the ghost renders one line above the caret.
_syncMirrorMetrics sets mirror.style.width = textarea.offsetWidth, which includes the vertical scrollbar gutter; the textarea wraps at clientWidth. Measured: taClientW 462 / mirrorClientW 477; same text → 43 vs 42 visual lines. With real typing in a 40-line cell (classic scrollbars, ignoreDefaultArgs: ['--hide-scrollbars']), prin→t at last-line lengths 70–72 draws the ghost one line above the caret line, and Valu→eError at 68–72 is withheld (mirror wraps ValueError to a line the textarea doesn't have). Headless Chromium hides scrollbars by default, so the suite cannot see this; Windows/Linux desktop and the Electron shell have classic scrollbars, Android/macOS overlay ones.
Fix direction: size the mirror from clientWidth/clientHeight (+ borders) rather than offsetWidth/offsetHeight; run the browser suite once with ignoreDefaultArgs: ['--hide-scrollbars'].
3. LOW (UX/design) — The Run⇄Accept swap is unscoped, so Run vanishes on desktop while a ghost is visible.
The CSS comment says "On a phone the composer already has only enough width…", but .composer-primary-action.completion-visible #run-btn { visibility:hidden } and the .cell-run-btn { display:none } rule apply at every width, and the suite asserts it at 800×700. Measured at 1000 px: typing print(x) → Run visible at p, hidden at i, visible again at t. Mid-word Run flicker on every matching identifier is the kind of thing users notice on desktop, where there is room for both controls. Suggest scoping the swap to narrow viewports / coarse pointers and rendering Accept beside Run otherwise. (Doc-compliant as written — §3 says "exposed … while a suggestion is genuinely visible" — so this is a design question, not a defect.)
4. LOW — Prelude helper names leak into completions. After any execution the snapshot includes _sci_repl_stdout, _sci_repl_old_stdout, _is_sympy, _is_sympy_list, _sympy_to_latex, _sympy_list_to_latex, _setup_matplotlib_hook, _SYMPY_AVAILABLE, _micropip, _pyodide_core (18 underscore names of 953). Typing _sci ghosts _repl_; _is ghosts _sympy. Cheap, precise fix: capture frozenset(_g) at the end of the prelude and exclude that set at refresh (user _private names stay).
5. INFO — Fail-closed on the Accept control withholds keyboard suggestions on desktop when the cell toolbar is off-screen. At a 420 px-tall viewport with a 300 px textarea, the toolbar sits below the scroller (527–565 vs bottom 318) and no ghost appears even though the caret line is visible and Tab would work. Design-compliant (§3), but on fine-pointer devices requiring only the suffix to be visible, and showing Accept only when it fits, would serve keyboard users better.
6. NIT — check('…', true) markers (e.g. "matching input shrinks the suffix", "Auto enables local completion on a desktop pointer", "restoring usable notebook height restores cell completion"). They aren't vacuous — the preceding waitFor… throws on timeout — but they read as if they assert nothing; returning the awaited condition into the check keeps the count honest.
What I could not reproduce
Appending the suffix re-wrapping the caret's own token (prefix line moving) — scanned line lengths 30–75, no shift. And an earlier scan of mine that suggested suppression at many lengths turned out to be an artifact of running three scans in one surface; the numbers above come from fresh contexts and real keystrokes.
Verdict
Not merge-ready as-is because of #1 (the feature is silently absent for a common desktop case) and #2 (visible misrender on classic-scrollbar desktops); both are contained to completion_surface.js's mirror sizing/measurement and neither touches the provider, lifecycle, or security properties, which are solid. #3 is a call for the design owners. Happy to hand over the reproduction scripts.
Claude-Session: https://claude.ai/code/session_01Wvyb3rH3LhW1ijrQoQQgrS
Summary
Deliberate Phase 1 limits
Python only; collapsed caret at end of source; no dotted lookup, popup list, Markdown completion, or AI/provider code. The shared surface and matcher are byte-identical to Pro.
Paired Pro implementation: SciREPL-Pro #48. Reviewed design contract: SciREPL-Pro #45.
Verification
git diff --checkclean.Review state
Draft pending GitHub CI, independent review of the exact pushed head, and a real-Android soft-keyboard/IME + Undo/Redo pass before merge.