fix(cli): hide cursors in unfocused editors - #4733
Conversation
Strip Editor styling from inactive onboarding fields so pi-tui's synthetic reverse-video cursor is not rendered twice. Cover the Name-to-Slug transition with field-specific cursor assertions. Generated-by: Codex
me2seeks
left a comment
There was a problem hiding this comment.
Verified locally before commenting:
- Premise checks out. In
@earendil-works/pi-tui'sEditor.render()the fake cursor (\x1b[7m…\x1b[0m) is painted wheneverlayoutLine.hasCursor, andfocusedonly gates the zero-widthCURSOR_MARKERfor IME placement (editor.js:419). The added comment describes the dependency accurately. - Scope is precise. Only the identity phase shows two editors at once; single-editor phases keep their editor focused, so the
stripAnsibranch is a no-op there (and usefully drops the stale cursor duringrenderModels' saving state, wherefocusOnly(null)applies). - Tests pass. Built the workspace and ran
dist/__tests__/pi-tui-runner.test.jsundernode --test: 188/188 pass, including the new Name→Slug field-specific cursor assertions. The new assertions genuinely fail without the fix (the unfocused slug row would still contain\x1b[7m \x1b[0m).
One follow-up observation — not a blocker for this PR, but this fix is the second instance of a pattern worth consolidating:
The "render an Editor minus its border rows, with a prefix" logic now lives in three places in this one file:
OnboardingWizard.renderFieldRow(:1869) — fixed hereModelPicker.renderFieldRow(:924) — byte-identical to the pre-fix version; currently safe only because its editor is always focusedUserQuestionOverlay.renderInputRow(:673) — still exhibits the behavior this PR fixed. Its comment (#1064) states thatfocused"both shows the block cursor and emits the hardware-cursor marker", which rests on the same misreading of pi-tui's contract. Reproduced againstdist: type text (type-to-jump), then press ↑ back to an option row — the now-unfocused input row still renders" hi\u001b[7m \u001b[0m…", i.e. a visible block cursor where the comment's design says there should be none.
A shared focus-aware renderEditorRow(editor, prefix, width) helper would remove two near-duplicate implementations, make the cursor-suppression behavior uniform, and align UserQuestionOverlay with its documented intent. Happy to file that as a follow-up issue if you agree.
Longer-term boundary question (low stakes, fine to defer): the cursor-visibility authority arguably belongs in pi-tui itself (e.g. gate the fake cursor on focused, or a render option). The consumer-side stripAnsi works, but every future multi-field surface has to rediscover this quirk. Worth an upstream issue at some point; the repo currently carries no patches/ entry for pi-tui, so the workaround here is a reasonable place to land the fix today.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 019ddc8e9e4dc51158399da858848b76f340b1a3 (OPEN). Technical GO — no P0–P2, one P3 below.
What it does
Shows the settings cursor only in the focused input. One P3 (non-blocking): the new test hardcodes the \x1b[7m reverse-video sequence to detect the cursor, depending on pi-tui's current rendering details.
No P0–P2. Checks green on this head.
What I could not judge
Live runtime behavior beyond the reviewed paths was not exercised locally.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 019ddc8,未关闭。技术上无阻断问题。等人类拍板。
Astro-Han
left a comment
There was a problem hiding this comment.
Correction to my review above: the P3's precise location is pi-tui-pickers.ts:1874-1876 (comment) with the production change at :1877 (not :1874 as originally written). Substance unchanged: the comment does not state that stripAnsi removes all styling from inactive fields — true but undocumented today, and the premise should be written down. Grade stays technical GO.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Thanks for the detailed review — good catch. I agree this is a shared rendering issue rather than something specific to |
me2seeks
left a comment
There was a problem hiding this comment.
Automated review by OpenAI Codex, operated by me2seeks, at 019ddc8e9e4dc51158399da858848b76f340b1a3. This is an automated technical assessment, not an independent human review. Approval is submitted at the operator's explicit direction.
No blocking correctness findings. Verified that only the inactive editor loses ANSI styling while the dependency still owns wrapping and horizontal scrolling. The exact-source wizard flow passed all new Name-to-Slug cursor assertions. Restoring the old render expression makes the inactive-slug assertion fail (true !== false). Other one-editor phases keep focus and retain styling.
- Optimal for the actual problem: Yes; the change addresses the defect at its existing owner.
- Production code that can be deleted: Nonblocking shared-owner opportunity already discussed on this PR: the three near-duplicate prefixed Editor row renderers can share focus handling; UserQuestionOverlay still carries the old dependency assumption. This is outside the original setup-cursor fix and was explicitly treated as follow-up in the earlier review.
- Low-quality tests that can be deleted or replaced: None identified.
- Deeper refactor: No deeper refactor is needed.
- Ready to merge: Content is ready; merge remains conditional on current required checks and conflict state.
- Residual risks / verification: Focused behavioral assertions passed and original-code mutation failed. The isolated unmutated test process exits 143 because its pre-existing SIGTERM teardown sets process.exitCode; this is not a clean local test-command pass. Full TUI suite was not rerun; exact-head hosted checks are refreshed before publication. Inactive editors intentionally lose all ANSI styling. User-visible cursor behavior changes; material protected-area changes require independent human review under CONTRIBUTING.md.
Share the pi-tui cursor compatibility rendering across inline fields and the main composer. Preserve drafts, borders, and other styles when focus moves to another field or overlay. Cover wizard field switching, Other answer focus and combining characters, and composer focus restoration after an overlay closes. Generated-by: Codex
Use named arrow and Enter key constants so the Other answer focus test reads as user actions. Generated-by: Codex
Show the matched ANSI wrapper and captured text in an aligned diagram, and document the format-based compatibility limit. Generated-by: Codex
Render fields before removing their borders and hiding unfocused cursors. Keep the full rendered output for the main composer, and name the shared layout helper renderFieldRow. Generated-by: Codex
Generated-by: Codex
Compare the refocused answer row with its rendering before selecting a preset, covering the cursor and IME marker position. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Preserve the new session picker imports alongside OnboardingWizard. Run the pre-commit checks manually for this merge: - Biome check --staged passes against files identical to the index. The stdin checker replaces the existing warning symbol with an exclamation mark. - The protocol guard passes against main and the staged tree. Staged mode revalidates older declarations imported from main against the newer epoch. - The staged ASF header and whitespace checks pass. Generated-by: Codex
me2seeks
left a comment
There was a problem hiding this comment.
Automated technical review by OpenAI Codex, operated by me2seeks, for exact head abfeab438ce7764b0685f9ef7f3ce46e44b36545. This does not replace the independent human judgment required by CONTRIBUTING.md.
No blocking correctness or simplification findings. I reproduced the underlying pi-tui behavior with focused=false, verified that the adapter removes only the cursor wrapper on the reviewed paths, and built the exact head. The CLI suite passed: 911 passed, 3 skipped, 0 failed. The focused fixture suites also passed under default, truecolor, and NO_COLOR=1. The inline scene fixtures are appropriate compatibility-contract tests: their oracle is independent of the production helper and checks complete ANSI rows, cursor/IME placement, wrapping, and preserved border styles.
P3, explicitly non-blocking and no change requested in this PR: SessionSearchOverlay still owns a second no-autocomplete prefixed-Editor row formatter, while ModelSearchOverlay and OnboardingWizard retain one-line forwarding methods around the new shared renderFieldRow. Those paths can be collapsed opportunistically when this area is next touched. DirectoryPickerOverlay should remain outside that helper because its Editor has autocomplete rows.
The longer-term cursor-visibility owner is pi-tui, but its current and 0.85.1 Editor still render the software cursor while unfocused, so the bounded Maka adapter is the correct change here. An upstream API or accepted semantic change is a future deletion gate, not a blocker.
Content GO. Merge remains conditional on the exact-head required check and project-required independent human review.
The new helper only transforms ANSI styles and contains no user-facing copy. Register it alongside the other rendering infrastructure so the TUI copy inventory accepts it. Generated-by: Codex
Summary
pi-tui 0.84.4 renders its block cursor even when an Editor is unfocused. Hide that cursor in Maka's compatibility layer, covering setup fields, Other answers, and the main composer behind overlays. Preserve draft text, wrapping, scrolling, and other styles.
Fixes #4732
Verification
npm --workspace maka-agent run test:dist— 911 passed, 3 skipped, 0 failed.npm run check:tui-copyand its 7 existing tests — passed; the ANSI cursor helper is classified as rendering infrastructure.NO_COLOR=1environments.npm run build,npm run typecheck,npm run lint, andnpm run format:check— passed.npx knip --workspace apps/desktopandnpx knip --workspace packages/ui— passed.The illustrations below show the expected behavior after the fix.
<cursor>marks the editing position; the component fixtures require both the visible cursor and its IME anchor there.<selected>…</selected>marks a selected preset. These markers are explanatory notation, not literal terminal text.AI use
Tool(s) and scope: Codex — root-cause analysis, implementation, regression testing, verification, and issue/PR drafting.
Checklist
Does this PR entail a change in behavior?