-
Notifications
You must be signed in to change notification settings - Fork 51
fix(tui): avoid stale Windows autocomplete rows #409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
441a467
ddc7066
4465e9d
79fdc90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| import type { | ||
| ExtensionAPI, | ||
| ExtensionContext, | ||
| } from "@earendil-works/pi-coding-agent"; | ||
|
|
||
| type TuiMode = "regular" | "fullscreen"; | ||
|
|
||
| interface TuiSettingsManager { | ||
| getGlobalSettings(): { tuiMode?: TuiMode }; | ||
| getProjectSettings(): { tuiMode?: TuiMode }; | ||
| setTuiMode?(mode: TuiMode): void; | ||
| flush?(): Promise<void>; | ||
| } | ||
|
|
||
| type TuiSettingsManagerFactory = ( | ||
| cwd: string, | ||
| ) => TuiSettingsManager | Promise<TuiSettingsManager>; | ||
|
|
||
| const WIDGET_KEY = "openpi-windows-tui-compatibility"; | ||
|
|
||
| /** | ||
| * The main-screen renderer can leave stale autocomplete rows on Windows. | ||
| * Keep the workaround limited to interactive Windows sessions so RPC/print | ||
| * users and non-Windows terminals are unaffected. | ||
| */ | ||
| export function shouldInstallWindowsTuiCompatibility( | ||
| platform: NodeJS.Platform, | ||
| mode: ExtensionContext["mode"], | ||
| ) { | ||
| return platform === "win32" && mode === "tui"; | ||
| } | ||
|
|
||
| export function shouldPreferWindowsFullscreen(options: { | ||
| platform: NodeJS.Platform; | ||
| mode: ExtensionContext["mode"]; | ||
| globalTuiMode?: TuiMode; | ||
| projectTuiMode?: TuiMode; | ||
| explicitCliTuiMode?: boolean; | ||
| }) { | ||
| return ( | ||
| shouldInstallWindowsTuiCompatibility(options.platform, options.mode) && | ||
| options.globalTuiMode === undefined && | ||
| options.projectTuiMode === undefined && | ||
| options.explicitCliTuiMode !== true | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Register the Windows renderer workaround. | ||
| * | ||
| * Pi exposes the renderer to widget factories, but not as a direct property | ||
| * on ExtensionContext. The zero-height widget lets us apply the supported | ||
| * renderer setting without replacing OpenPI's header, footer, or editor. | ||
| */ | ||
| export function registerWindowsTuiCompatibility( | ||
| pi: ExtensionAPI, | ||
| platform: NodeJS.Platform, | ||
| settingsManagerFactory?: TuiSettingsManagerFactory, | ||
| ) { | ||
| let activeUi: ExtensionContext["ui"] | undefined; | ||
|
|
||
| const cleanup = () => { | ||
| const ui = activeUi; | ||
| activeUi = undefined; | ||
| try { | ||
| ui?.setWidget(WIDGET_KEY, undefined); | ||
| } catch { | ||
| // The renderer may already be gone during shutdown. | ||
| } | ||
| }; | ||
|
|
||
| pi.on("session_start", async (_event, ctx) => { | ||
| cleanup(); | ||
| if (!shouldInstallWindowsTuiCompatibility(platform, ctx.mode)) return; | ||
|
|
||
| activeUi = ctx.ui; | ||
| ctx.ui.setWidget( | ||
| WIDGET_KEY, | ||
| (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); | ||
| return { | ||
| render: () => [], | ||
| invalidate() {}, | ||
| }; | ||
| }, | ||
| { placement: "belowEditor" }, | ||
| ); | ||
|
|
||
| if (!settingsManagerFactory) return; | ||
|
|
||
| try { | ||
| const settingsManager = await settingsManagerFactory(ctx.cwd); | ||
| const globalSettings = settingsManager.getGlobalSettings(); | ||
| const projectSettings = settingsManager.getProjectSettings(); | ||
| const explicitCliTuiMode = process.argv.some( | ||
| (arg) => arg === "--tui-mode" || arg.startsWith("--tui-mode="), | ||
| ); | ||
|
|
||
| if ( | ||
| !shouldPreferWindowsFullscreen({ | ||
| platform, | ||
| mode: ctx.mode, | ||
| globalTuiMode: globalSettings.tuiMode, | ||
| projectTuiMode: projectSettings.tuiMode, | ||
| explicitCliTuiMode, | ||
| }) || | ||
| settingsManager.setTuiMode === undefined | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| settingsManager.setTuiMode("fullscreen"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| await settingsManager.flush?.(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in |
||
| ctx.ui.notify( | ||
| "OpenPI detected the Windows regular-TUI redraw issue and selected fullscreen mode for the next start. Restart Pi to apply it.", | ||
| "warning", | ||
| ); | ||
| } catch { | ||
| // A settings write must never prevent the OpenPI session from starting. | ||
| } | ||
| }); | ||
|
|
||
| pi.on("session_shutdown", cleanup); | ||
| } | ||
|
|
||
| export default function windowsTuiCompatibility(pi: ExtensionAPI) { | ||
| registerWindowsTuiCompatibility(pi, process.platform, async (cwd) => { | ||
| const { SettingsManager } = await import("@earendil-works/pi-coding-agent"); | ||
| return SettingsManager.create(cwd); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| import assert from "node:assert/strict"; | ||
| import test from "node:test"; | ||
| import type { | ||
| ExtensionAPI, | ||
| ExtensionContext, | ||
| } from "@earendil-works/pi-coding-agent"; | ||
| import type { Component, TUI } from "@earendil-works/pi-tui"; | ||
| import { | ||
| registerWindowsTuiCompatibility, | ||
| shouldInstallWindowsTuiCompatibility, | ||
| shouldPreferWindowsFullscreen, | ||
| } from "../../../extensions/windows-tui-compatibility/index.ts"; | ||
|
|
||
| type WidgetFactory = ( | ||
| tui: TUI, | ||
| theme: unknown, | ||
| ) => Component & { dispose?(): void }; | ||
|
|
||
| function createHarness( | ||
| platform: NodeJS.Platform, | ||
| mode: ExtensionContext["mode"] = "tui", | ||
| settingsManagerFactory?: Parameters< | ||
| typeof registerWindowsTuiCompatibility | ||
| >[2], | ||
| ) { | ||
| const hooks = new Map< | ||
| string, | ||
| (event: unknown, ctx: ExtensionContext) => unknown | ||
| >(); | ||
| let widgetFactory: WidgetFactory | undefined; | ||
| let widgetCleared = false; | ||
| const notifications: string[] = []; | ||
|
|
||
| const pi = { | ||
| on(event: string, handler: unknown) { | ||
| hooks.set( | ||
| event, | ||
| handler as (event: unknown, ctx: ExtensionContext) => unknown, | ||
| ); | ||
| }, | ||
| } as unknown as ExtensionAPI; | ||
|
|
||
| const ctx = { | ||
| cwd: "C:\\project", | ||
| mode, | ||
| hasUI: mode === "tui", | ||
| ui: { | ||
| setWidget(_key: string, content: WidgetFactory | undefined) { | ||
| if (content) widgetFactory = content; | ||
| else { | ||
| widgetFactory = undefined; | ||
| widgetCleared = true; | ||
| } | ||
| }, | ||
| notify(message: string) { | ||
| notifications.push(message); | ||
| }, | ||
| }, | ||
| } as unknown as ExtensionContext; | ||
|
|
||
| registerWindowsTuiCompatibility(pi, platform, settingsManagerFactory); | ||
|
|
||
| return { | ||
| ctx, | ||
| emit(event: string) { | ||
| return hooks.get(event)?.({}, ctx); | ||
| }, | ||
| mount(tui: TUI) { | ||
| return widgetFactory?.(tui, {}); | ||
| }, | ||
| get widgetFactory() { | ||
| return widgetFactory; | ||
| }, | ||
| get widgetCleared() { | ||
| return widgetCleared; | ||
| }, | ||
| get notifications() { | ||
| return notifications; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| test("installs only for interactive Windows sessions", () => { | ||
| assert.equal(shouldInstallWindowsTuiCompatibility("win32", "tui"), true); | ||
| assert.equal(shouldInstallWindowsTuiCompatibility("linux", "tui"), false); | ||
| assert.equal(shouldInstallWindowsTuiCompatibility("win32", "rpc"), false); | ||
|
|
||
| assert.equal( | ||
| shouldPreferWindowsFullscreen({ platform: "win32", mode: "tui" }), | ||
| true, | ||
| ); | ||
| assert.equal( | ||
| shouldPreferWindowsFullscreen({ | ||
| platform: "win32", | ||
| mode: "tui", | ||
| globalTuiMode: "regular", | ||
| }), | ||
| false, | ||
| ); | ||
| assert.equal( | ||
| shouldPreferWindowsFullscreen({ | ||
| platform: "win32", | ||
| mode: "tui", | ||
| projectTuiMode: "fullscreen", | ||
| }), | ||
| false, | ||
| ); | ||
| assert.equal( | ||
| shouldPreferWindowsFullscreen({ | ||
| platform: "win32", | ||
| mode: "tui", | ||
| explicitCliTuiMode: true, | ||
| }), | ||
| false, | ||
| ); | ||
|
|
||
| const linux = createHarness("linux"); | ||
| linux.emit("session_start"); | ||
| assert.equal(linux.widgetFactory, undefined); | ||
| }); | ||
|
|
||
| test("enables clear-on-shrink for regular TUI but not fullscreen", () => { | ||
| const harness = createHarness("win32"); | ||
| harness.emit("session_start"); | ||
|
|
||
| const clearOnShrink: boolean[] = []; | ||
| harness.mount({ | ||
| mode: "regular", | ||
| setClearOnShrink(enabled: boolean) { | ||
| clearOnShrink.push(enabled); | ||
| }, | ||
| requestRender(force?: boolean) { | ||
| assert.equal(force, undefined); | ||
| }, | ||
| } as TUI); | ||
| assert.deepEqual(clearOnShrink, [true]); | ||
|
|
||
| clearOnShrink.length = 0; | ||
| harness.mount({ | ||
| mode: "fullscreen", | ||
| setClearOnShrink(enabled: boolean) { | ||
| clearOnShrink.push(enabled); | ||
| }, | ||
| requestRender(force?: boolean) { | ||
| assert.equal(force, undefined); | ||
| }, | ||
| } as TUI); | ||
| assert.deepEqual(clearOnShrink, []); | ||
| }); | ||
|
|
||
| test("persists fullscreen only when no TUI mode is configured", async () => { | ||
| let selectedMode: string | undefined; | ||
| let flushCount = 0; | ||
| const unset = createHarness("win32", "tui", async () => ({ | ||
| getGlobalSettings: () => ({}), | ||
| getProjectSettings: () => ({}), | ||
| setTuiMode: (mode: "regular" | "fullscreen") => { | ||
| selectedMode = mode; | ||
| }, | ||
| flush: async () => { | ||
| flushCount += 1; | ||
| }, | ||
| })); | ||
| await unset.emit("session_start"); | ||
| assert.equal(selectedMode, "fullscreen"); | ||
| assert.equal(flushCount, 1); | ||
| assert.match(unset.notifications[0] ?? "", /Restart Pi/); | ||
|
|
||
| const explicit = createHarness("win32", "tui", async () => ({ | ||
| getGlobalSettings: () => ({ tuiMode: "regular" as const }), | ||
| getProjectSettings: () => ({}), | ||
| setTuiMode: () => { | ||
| throw new Error("must not override an explicit mode"); | ||
| }, | ||
| })); | ||
| await explicit.emit("session_start"); | ||
| assert.deepEqual(explicit.notifications, []); | ||
| }); | ||
|
|
||
| test("cleans up the compatibility widget on shutdown", () => { | ||
| const harness = createHarness("win32"); | ||
| harness.emit("session_start"); | ||
| assert.ok(harness.widgetFactory); | ||
|
|
||
| harness.emit("session_shutdown"); | ||
|
|
||
| assert.equal(harness.widgetFactory, undefined); | ||
| assert.equal(harness.widgetCleared, true); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 definesterminal.clearOnShrink; an explicitfalse(ortrue) is left untouched. The focused regression covers the explicitfalsecase.