fix(pi-tui): avoid spurious viewport scroll jumps from idle cursor repositioning#1310
fix(pi-tui): avoid spurious viewport scroll jumps from idle cursor repositioning#1310bj456736 wants to merge 4 commits into
Conversation
Closes MoonshotAI#1016 The LLM sometimes passes 'completed' as the status for TodoList items, but the schema only accepted 'pending' | 'in_progress' | 'done'. This produced two problems: 1. Validation failed when the model used 'completed'. 2. Even if validation passed, statusMarker() had no case for 'completed' and fell through to the unreachable default branch. Changes: - Extend TodoStatus union to include 'completed' so it is accepted at the type level. - Map 'completed' -> 'done' in setTodos() so persisted state stays clean. - Handle 'completed' in statusMarker() so it renders as '[done]'. - Update the markdown description to explicitly warn against using 'completed'. - Add a test confirming 'completed' is accepted and mapped to 'done'.
…positioning Closes MoonshotAI#1307 (upstream earendil-works/pi#3753) On terminals with Kitty keyboard protocol support, releasing modifier keys in a specific order (e.g. releasing Command before 'c') generates a spurious key-release event. This triggers a re-render, which calls positionHardwareCursor() unconditionally, writing ANSI cursor sequences even when the cursor hasn't moved. The terminal interprets these sequences as a reason to scroll the viewport back to the cursor. Changes: - Track hardwareCursorCol alongside hardwareCursorRow. - Skip positionHardwareCursor() writes when showHardwareCursor is false and both row and column are unchanged. - Reset hardwareCursorCol on force render to stay consistent with the existing hardwareCursorRow reset. Tested: all 704 pi-tui tests pass.
|
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef1667f65
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| title: z.string().min(1).describe('Short, actionable title for the todo.'), | ||
| status: z.enum(['pending', 'in_progress', 'done']).describe('Current status of the todo.'), | ||
| status: z | ||
| .preprocess((val) => (val === 'completed' ? 'done' : val), z.enum(['pending', 'in_progress', 'done'])) |
There was a problem hiding this comment.
completed through tool-argument validation
In real tool calls, preflightToolCall validates the provider arguments with AJV against tool.parameters and never runs TodoListInputSchema.safeParse, so this preprocess does not normalize the raw "completed" value before validation. The JSON schema advertised from this schema still describes the inner enum (pending/in_progress/done), so a model call using the compatibility spelling is rejected before setTodos can map it to done; the new test bypasses that path by invoking resolveExecution directly. Please make the JSON schema accept completed as well, or run the Zod parse/normalization before AJV rejects the call.
Useful? React with 👍 / 👎.
| if (!this.showHardwareCursor && targetRow === this.hardwareCursorRow && targetCol === this.hardwareCursorCol) { | ||
| return; |
There was a problem hiding this comment.
After a frame rewrites terminal lines, doRender updates hardwareCursorRow to the final rendered row before calling this method, but hardwareCursorCol still contains the previous requested cursor column rather than the column where the write actually left the terminal. When the logical cursor is on that final row and its column is unchanged, this guard returns without emitting the absolute-column move, leaving the physical cursor at the end of the rewritten line instead of at the cursor marker; that breaks IME candidate placement and leaves subsequent cursor state out of sync. Please only suppress the ANSI write for frames that did not already move the terminal cursor, or refresh the tracked column before comparing.
Useful? React with 👍 / 👎.
1 participant
Problem
On terminals with Kitty keyboard protocol support (iTerm2, WezTerm, kitty, Ghostty, etc.), releasing modifier keys in a specific order generates a spurious key-release event. This triggers a re-render, which calls
positionHardwareCursor()unconditionally — writing ANSI cursor positioning sequences even when the cursor hasn't actually moved. The terminal interprets these sequences as a reason to scroll the viewport back to the cursor.Reproduction
Command + c, but releaseCommandfirst, thenc.Fix
hardwareCursorColalongsidehardwareCursorRow.positionHardwareCursor()writes whenshowHardwareCursoris false and both row and column are unchanged.hardwareCursorColon force render to stay consistent with the existinghardwareCursorRowreset.Testing
All 704 pi-tui tests pass.
Upstream reference: earendil-works/pi#3753