Skip to content

fix(cli): hide cursors in unfocused editors - #4733

Open
hey-mira wants to merge 14 commits into
apache:mainfrom
hey-mira:fix/cli-setup-input-focus
Open

fix(cli): hide cursors in unfocused editors#4733
hey-mira wants to merge 14 commits into
apache:mainfrom
hey-mira:fix/cli-setup-input-focus

Conversation

@hey-mira

@hey-mira hey-mira commented Sep 4, 2026

Copy link
Copy Markdown

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-copy and its 7 existing tests — passed; the ANSI cursor helper is classified as rendering infrastructure.
  • Other/composer tests — 17 passed in each of the default, truecolor, and NO_COLOR=1 environments.
  • npm run build, npm run typecheck, npm run lint, and npm run format:check — passed.
  • npx knip --workspace apps/desktop and npx knip --workspace packages/ui — passed.
  • Fixture checks cover complete rows, cursor/IME positions, and preserved colors. Fault injection detects missing IME or visible cursors, residual unfocused cursors, and lost border colors.

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.

1. Setup wizard — Enter moves focus from Name to Slug

Name Work OpenAI<cursor>

Slug openai

After Enter:

Name Work OpenAI

Slug openai<cursor>
2. Other answer — keep a wrapped draft while selecting Preset

After typing:
  Preset
→ Please use the custom provider and
  keep the current model settings for
  this workspace<cursor>

After ↑:
<selected>→ Preset</selected>
  Please use the custom provider and
  keep the current model settings for
  this workspace

After ↓:
  Preset
→ Please use the custom provider and
  keep the current model settings for
  this workspace<cursor>
3. Existing character — move onto the final e, then switch focus

After ←:
  Preset
→ Please use the custom provider and
  keep the current model settings for
  this workspac<cursor>e

After ↑:
<selected>→ Preset</selected>
  Please use the custom provider and
  keep the current model settings for
  this workspace

After ↓:
  Preset
→ Please use the custom provider and
  keep the current model settings for
  this workspac<cursor>e

Enter submits the unchanged draft.
4. Main composer — opening and closing an overlay

Before opening:
────────────────────────────────────────
draft<cursor>
────────────────────────────────────────

With the overlay open:
Picker
────────────────────────────────────────
draft
────────────────────────────────────────

After closing:
────────────────────────────────────────
draft<cursor>
────────────────────────────────────────

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex — root-cause analysis, implementation, regression testing, verification, and issue/PR drafting.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

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
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 4, 2026

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified locally before commenting:

  • Premise checks out. In @earendil-works/pi-tui's Editor.render() the fake cursor (\x1b[7m…\x1b[0m) is painted whenever layoutLine.hasCursor, and focused only gates the zero-width CURSOR_MARKER for 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 stripAnsi branch is a no-op there (and usefully drops the stale cursor during renderModels' saving state, where focusOnly(null) applies).
  • Tests pass. Built the workspace and ran dist/__tests__/pi-tui-runner.test.js under node --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:

  1. OnboardingWizard.renderFieldRow (:1869) — fixed here
  2. ModelPicker.renderFieldRow (:924) — byte-identical to the pre-fix version; currently safe only because its editor is always focused
  3. UserQuestionOverlay.renderInputRow (:673) — still exhibits the behavior this PR fixed. Its comment (#1064) states that focused "both shows the block cursor and emits the hardware-cursor marker", which rests on the same misreading of pi-tui's contract. Reproduced against dist: 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@hey-mira

hey-mira commented Sep 7, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review — good catch. I agree this is a shared rendering issue rather than something specific to /setup. I'll consolidate the editor-row rendering and cover UserQuestionOverlay in this PR as well.

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

  1. Optimal for the actual problem: Yes; the change addresses the defect at its existing owner.
  2. 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.
  3. Low-quality tests that can be deleted or replaced: None identified.
  4. Deeper refactor: No deeper refactor is needed.
  5. Ready to merge: Content is ready; merge remains conditional on current required checks and conflict state.
  6. 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
@hey-mira hey-mira changed the title fix(cli): show setup cursor only on active field fix(cli): hide cursors in unfocused editors Sep 8, 2026
Use named arrow and Enter key constants so the Other answer focus test reads as user actions.

Generated-by: Codex
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 8, 2026
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
Compare the refocused answer row with its rendering before selecting a preset, covering the cursor and IME marker position.

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 me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI /setup displays cursors in both the Name and Slug fields

4 participants