Skip to content

fix(tui): avoid stale Windows autocomplete rows - #409

Open
zhouyuanxinand wants to merge 3 commits into
openpi-dev:mainfrom
zhouyuanxinand:codex/windows-tui-compatibility
Open

fix(tui): avoid stale Windows autocomplete rows#409
zhouyuanxinand wants to merge 3 commits into
openpi-dev:mainfrom
zhouyuanxinand:codex/windows-tui-compatibility

Conversation

@zhouyuanxinand

@zhouyuanxinand zhouyuanxinand commented Sep 5, 2026

Copy link
Copy Markdown

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

  • Install the compatibility widget only for interactive Windows TUI sessions.
  • Read the raw global and project terminal.clearOnShrink settings. If neither scope explicitly configures the setting, enable Pi's supported clear-on-shrink behavior on the current regular renderer only.
  • Preserve an explicit terminal.clearOnShrink value, including false, and leave the selected tuiMode unchanged. Settings read errors fail closed.
  • Do not call Pi settings setters, flush persistence, or emit a restart/success notification. Users who want a persistent mode or terminal preference can use Pi's native /settings, settings.json, or --tui-mode flow.
  • Clean up the widget on session shutdown and keep fullscreen rendering unchanged.

Validation

  • Required PR CI on commit 4465e9d: Node 22.19.0, Node 24, and Background terminals (Windows) — all passed.
  • Native Windows visual acceptance was not run in this environment; the change is limited to the existing renderer hook and has no visual asset changes.

Impact

  • User-visible behavior: affected Windows regular TUI sessions get session-local stale-row cleanup when no explicit clear-on-shrink preference exists; no restart is required and no success notice is shown.
  • Persisted config/data: read-only inspection only; no global or project settings are written, and explicit values are preserved.
  • Runtime/lifecycle: limited to interactive Windows TUI sessions; the compatibility widget is removed on shutdown.
  • Compatibility/risk: fullscreen, RPC, print, JSON, non-Windows, and explicitly configured terminal preferences remain unchanged.

@github-actions github-actions Bot added documentation Improvements or additions to documentation area:setup OpenPI setup, configuration, or setup documentation labels Sep 5, 2026

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?.();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zhouyuanxinand

zhouyuanxinand commented Sep 6, 2026

Copy link
Copy Markdown
Author

Addressed all three requested P2 findings in commit 4465e9d:

  • Removed the automatic global tuiMode write, persistence flush, and restart/success notification. The renderer workaround is session-local and leaves mode selection to Pi's native /settings, settings.json, or --tui-mode flow.
  • Read global/project terminal.clearOnShrink before installing the workaround; an explicit value (including false) is preserved, and settings-read errors fail closed.
  • Kept the workaround limited to the current interactive Windows regular renderer; fullscreen, RPC, print, JSON, and non-Windows sessions are unchanged.

All required PR CI jobs are green: Node 22, Node 24, and Background terminals (Windows).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:setup OpenPI setup, configuration, or setup documentation documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tui): prevent stale autocomplete rows on Windows terminals

2 participants