fix(tui): add scroll support to plan prompt modal#2623
Conversation
|
Thanks @Implementist for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces scrolling functionality, Vim-style navigation, and an exit confirmation prompt to the PlanPromptView modal in the TUI, along with comprehensive unit tests. The review feedback highlights three key areas for improvement: refining the wrapped_line_count logic to accurately account for word wrapping at word boundaries to prevent content clipping, consolidating separate option arrays into a single unified struct to avoid synchronization issues, and guarding the g/G navigation keys against modifiers like Ctrl or Alt to prevent accidental triggers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks @Implementist for the detailed Plan prompt scroll fix and for calling out the upstream #2615 compile break in the PR body. I have a v0.8.52 stabilization branch now that fixes the SiliconFlow-CN compile failure and the release-gate fallout, so this PR should be able to get a real CI signal once that lands and you rebase or rerun. I am keeping this PR separate from the emergency 0.8.52 release work so your UI change does not get hidden inside the stabilization patch. Sorry main was noisy when you opened this. |
- Add scroll state field to PlanPromptView with PgUp/PgDn, Ctrl+U/D/F/B, Home/End, gg/G vim-style keybindings - Show scroll indicator footer when content overflows the popup - Add confirming_exit state: Esc while scrolled asks for confirmation before discarding, preventing accidental exits on long plans - Clamp scroll in render() so overscroll doesn't hide bottom options - Use wrapped_line_count() with UnicodeWidthStr for accurate overflow detection with CJK characters - Add 11 unit tests covering scroll, keybindings, and exit confirmation
- Use word-wrapping-aware line count to prevent underestimating scroll range (gemini-code-assist / greptile-apps) - Merge PLAN_OPTIONS, PLAN_SHORTCUTS, PLAN_SHORT_LABELS into PlanOption struct (gemini-code-assist) - Remove dead Esc code in handle_key (greptile-apps) - Guard gg/G with modifier checks (gemini-code-assist) - Increase PgUp/PgDn scroll amount from 6 to 12 (greptile-apps) - Use u16::try_from for scroll value to avoid silent truncation (greptile-apps) - Update related unit tests for new scroll values
… key Use clamped (effective) scroll instead of raw `self.scroll` in the Esc handler so a short plan that fits entirely (max_scroll == 0) never triggers the "exit without implementing?" dialog when the user pressed a scroll key (PgDn/Ctrl-D/G/End) beforehand. Co-authored-by: Cursor <cursoragent@cursor.com>
Implementist
force-pushed
the
bugfix/plan-prompt-scroll
branch
from
6e6bc29 to
97a0910
Compare
…e wrap width - wrapped_line_count: compute leading-space width via UnicodeWidthStr instead of byte length, so non-ASCII leading whitespace is measured correctly. - render: hoist popup_area / content_width computation above plan rendering so wrap_text can share the same content_width derived from the actual popup geometry instead of a magic 68.
…rome - Clear pending_g when Esc triggers the exit-confirmation prompt so a stray 'g' press does not leak into and survive the confirmation dialog. - Move render_modal_chrome into the else branch so only one call fires per render pass, eliminating a shadow artifact when confirming_exit is active.
…nder work - wrap_text: replace chars().count() with UnicodeWidthStr::width() so CJK text is wrapped by display columns, consistent with wrapped_line_count and ratatui's Paragraph::wrap. Also fix the hard-split loop to use exclusive byte ranges (..end) instead of inclusive (..=i) so multi-byte UTF-8 prefixes are always valid. - render: hoist the confirming_exit branch to an early return so the plan-content construction (lines, scroll bounds, footer) is skipped entirely when the confirmation dialog is visible.
…breaks at script boundaries
Wrap plan steps via wrap_text() before rendering, breaking only on display-width overflow, not on Latin/CJK Unicode word boundaries. Switch main render path from Wrap { trim: true } to Wrap { trim: false } since all content is pre-wrapped. Replace wrapped_line_count() with lines.len() for accurate scroll bounds. Keep confirm-exit dialog on Wrap { trim: true } (English-only, no risk).
2 participants
Description
The Plan Confirmation modal (
PlanPromptView) had no scrolling capability. Whenupdate_plangenerated a long plan (detailed explanation + many steps), the content overflowed the popup area, the bottom action options and footer were clipped off-screen, and users were forced to operate blind — unable to see what they were selecting.This PR adds full scroll support and fixes a rendering defect discovered during implementation: ratatui's
Wrap { trim: true }forces line breaks at Latin ↔ CJK script boundaries even when horizontal space remains, producing visually jarring mid-line breaks in mixed-language plan steps.What changed
1/ Scroll engine (
scrollfield + keybindings)PgUp/PgDnCtrl+U/Ctrl+DCtrl+F/Ctrl+BHome/Endgg/Gj/kScroll is clamped in
render()— overscrolling past the last visible line never hides the bottom action options. State is tracked withlast_max_scrollso the Esc handler can distinguish "at top" from "scrolled away."2/ Scroll indicator footer
When content overflows the popup, the footer displays:
The indicator shows current position / total range and scroll key hints.
3/ Exit confirmation
Pressing
Escafter scrolling away from the top triggers a secondary confirmation prompt ("You've scrolled through the plan content. Are you sure you want to exit?") withy/n/Escoptions. This prevents accidental exits on long plans.Escat top still exits immediately as before.4/ CJK+Latin mixed-text line wrapping fix
Plan step text (e.g.
实现 WorkspaceSnapshot 数据结构，包含 file_tree: BTreeMap<PathBuf, FileEntry> 和 timestamp: DateTime<Utc> 字段) is now pre-wrapped via a customwrap_text()that measures display width withunicode_width::UnicodeWidthStrand breaks only on column overflow — not on Latin ↔ CJK script boundaries. The resulting lines are rendered withWrap { trim: false }, fully disabling ratatui's word-boundary detection on the main render path.The Esc confirm-exit dialog (hardcoded English text) intentionally keeps
Wrap { trim: true }— no CJK risk, no change needed.5/ Cleanup
wrapped_line_count()— scroll bounds now uselines.len()directlysince all lines are pre-wrapped and width-bounded.
region instead of being arbitrarily cut off.
div_ceilwithusize::div_ceilto satisfy clippy.pending_gflag is now cleared onEscto avoid stalestate.
render_modal_chrome(was called twice in the render path).Scope
crates/tui/src/tui/plan_prompt.rsOne file. All changes contained within
PlanPromptView.Testing
cargo fmt -- --check— passescargo clippy --workspace— passescargo test --workspace— passes (3964 tests, including 11 pre-existingplan prompt tests covering scroll, keybindings, exit confirmation, gg combo,
scroll clamping, and confirmation flow)