fix(tui): avoid stale Windows autocomplete rows - #409
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
The Windows stale-autocomplete-row problem is worth fixing, but this implementation needs changes before merge. Please address the three inline findings: unsolicited global preference persistence, success notification after failed persistence, and overriding an explicit clear-on-shrink preference.
Validation: reviewed commit 441a467; bun run check and the 4 focused tests passed in the earlier same-head validation; required CI is currently green. Re-ran controlled probes with the locked real Pi SettingsManager and a simulated win32 extension context to reproduce the findings. This is not a native Windows visual acceptance test. No merge performed.
| return; | ||
| } | ||
|
|
||
| settingsManager.setTuiMode("fullscreen"); |
There was a problem hiding this comment.
[P2] Keep the Windows workaround from silently changing global Pi preferences
Every interactive Windows session without an explicit tuiMode reaches this write; there is no detection of stale rows. Pi SettingsManager.setTuiMode persists a global setting, so opening OpenPI in one workspace also changes subsequent Pi sessions in other workspaces. I reproduced this with the locked real SettingsManager and two temporary workspace paths sharing one agent directory. This exceeds a session-local rendering workaround and conflicts with the repository requirement to preserve Pi-owned preferences and side-effect-safe installation. Please make the persistent mode change an explicit user choice through the native Pi settings flow, or keep the mitigation scoped to the affected session.
There was a problem hiding this comment.
Addressed in 4465e9d. The extension no longer calls setTuiMode, flush, or any settings writer. It only applies the renderer workaround to the current interactive Windows session; persistent mode selection remains with Pi's native settings flow.
| } | ||
|
|
||
| settingsManager.setTuiMode("fullscreen"); | ||
| await settingsManager.flush?.(); |
There was a problem hiding this comment.
[P2] Verify persistence before announcing that fullscreen was selected
The locked Pi SettingsManager records settings load/write errors internally; flush() waits for its queue but does not necessarily reject. With a malformed global settings.json, setTuiMode does not save, flush resolves, and this code still tells the user to restart to apply fullscreen. I reproduced this using the real SettingsManager: the invalid file remained unchanged, drainErrors() reported a global error, and the success/restart notification was emitted. Check the settings error/result and persisted readback before announcing success; report an actionable failure otherwise. The current mock cannot cover this behavior.
There was a problem hiding this comment.
Addressed in 4465e9d. The persistence path and success/restart notification were removed entirely, so a failed settings write can no longer be reported as successful. The extension now reads settings only and fails closed when the read reports errors.
| (tui) => { | ||
| // The renderer can be replaced at runtime when the user switches TUI | ||
| // modes, so apply this when the factory receives the active renderer. | ||
| if (tui.mode === "regular") tui.setClearOnShrink(true); |
There was a problem hiding this comment.
[P2] Respect an explicit terminal.clearOnShrink preference
For a user who explicitly selects regular mode and terminal.clearOnShrink:false, mounting this widget unconditionally flips the live renderer to true while the stored/native setting remains false. I reproduced that mismatch with SettingsManager.inMemory and the widget factory. This overrides an existing Pi rendering preference (including users reducing redraws on slow terminals), despite preserving their explicit regular mode. Apply the fallback only when the user has not explicitly configured clear-on-shrink, or require an explicit opt-in, and add a regression for the false setting.
There was a problem hiding this comment.
Addressed in 4465e9d. The fallback is enabled only when neither global nor project settings explicitly defines terminal.clearOnShrink; an explicit false (or true) is left untouched. The focused regression covers the explicit false case.
|
Addressed all three requested P2 findings in commit
All required PR CI jobs are green: Node 22, Node 24, and Background terminals (Windows). |
Problem
Fixes #407. On Windows, Pi's regular (main-screen) TUI renderer can leave stale autocomplete rows on screen during differential redraw, making slash commands appear duplicated even though the command registry is correct.
Value
The compatibility extension removes the stale-row symptom for affected interactive Windows sessions while preserving Pi's existing TUI and terminal preferences. It does not alter RPC, print, JSON, or non-Windows sessions.
Approach
terminal.clearOnShrinksettings. If neither scope explicitly configures the setting, enable Pi's supported clear-on-shrink behavior on the current regular renderer only.terminal.clearOnShrinkvalue, includingfalse, and leave the selectedtuiModeunchanged. Settings read errors fail closed./settings,settings.json, or--tui-modeflow.Validation
4465e9d: Node 22.19.0, Node 24, and Background terminals (Windows) — all passed.Impact