fix: address PR #4 follow-ups#5
Conversation
Honor low_motion in the default tool transcript path and align composer cursor padding with the rendered placeholder. Add focused regression tests for both behaviors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several follow-up items identified after the merge of PR #4, which focused on consolidating modes to an Agent-centric workflow. The changes primarily enhance the user interface and experience by correctly applying Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR refines TUI rendering behavior by standardizing the composer’s vertical padding logic and ensuring transcript tool cells respect the “low motion” rendering option.
Changes:
- Introduce
composer_top_padding()and use it for both composer rendering and cursor positioning. - Simplify composer cursor padding logic by using
layout_input()’s visible line count. - Ensure
HistoryCell::ToolrespectsTranscriptRenderOptions.low_motionin the default rendering path, with a regression test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/tui/src/tui/widgets/mod.rs |
Switch composer padding to composer_top_padding() and add a cursor/placeholder alignment test. |
crates/tui/src/tui/history.rs |
Apply low_motion to tool rendering in lines_with_options() and add a test validating symbol behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
|
|
||
| let top_padding = composer_vertical_padding(input_lines.len(), input_rows_budget); | ||
| let top_padding = composer_top_padding(input_lines.len(), input_rows_budget); |
There was a problem hiding this comment.
top_padding is computed from input_lines.len(), but in the empty-input case input_lines contains a single placeholder Line that can wrap to multiple terminal rows (because Paragraph::wrap is enabled). On narrower widths this can undercount the real rendered height, causing the placeholder to be partially clipped and the vertical alignment to be off. Consider basing padding on the wrapped placeholder line count (e.g., wrap the placeholder text to content_width and use that length), or render the placeholder as already-wrapped lines so input_lines.len() reflects the actual row usage.
| let top_padding = composer_top_padding(input_lines.len(), input_rows_budget); | |
| // Estimate how many terminal rows the input will occupy. For non-empty input, | |
| // use the logical line count. For the empty-input placeholder, assume it may | |
| // occupy up to the entire input_rows_budget so we do not undercount its height | |
| // when it wraps on narrow widths. | |
| let input_visual_rows = if self.app.input.is_empty() { | |
| input_rows_budget | |
| } else { | |
| input_lines.len() | |
| }; | |
| let top_padding = composer_top_padding(input_visual_rows, input_rows_budget); |
| .max(1); | ||
| composer_vertical_padding(visible_count.min(input_rows_budget), input_rows_budget) | ||
| }; | ||
| let top_padding = composer_top_padding(visible_lines.len(), input_rows_budget); |
There was a problem hiding this comment.
Cursor padding is derived from visible_lines.len(), which is always 1 for an empty input (layout_input injects an empty string). However, the rendered placeholder can wrap to multiple rows. This means the cursor Y position can drift relative to what is actually on-screen at narrow widths. Consider computing cursor padding using the same wrapped-line count logic as rendering (especially for the empty-input placeholder) so cursor placement stays consistent with what the user sees.
![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 correctly addresses two follow-up items from a previous PR by honoring low_motion for tool cells and aligning the composer cursor padding. The code changes are clean, and the new regression tests are a great addition to prevent future issues. I have one suggestion to improve the robustness of a new test case to avoid potential flakiness in CI environments.
| let animated_symbol = animated[0].spans[0].content.trim(); | ||
| let low_motion_symbol = low_motion[0].spans[0].content.trim(); | ||
|
|
||
| assert_ne!(animated_symbol, TOOL_RUNNING_SYMBOLS[0]); |
There was a problem hiding this comment.
This assert_ne! could cause flaky tests in a slow CI environment. If test execution is slow, the animation might cycle back to the first frame, causing this assertion to fail. A more robust check would account for this timing possibility.
if animated_symbol == TOOL_RUNNING_SYMBOLS[0] {
// This can happen if the test is slow. A full animation cycle takes 7.2s.
// We start at an offset of 1.8s, so the next time the index is 0 is after an additional 5.4s.
assert!(
started_at.unwrap().elapsed() > std::time::Duration::from_secs(5),
"Animation symbol unexpectedly matched static symbol without significant delay."
);
}
…_app The test had inner `use` statements for Config, App, TuiOptions, and PathBuf that duplicated the module-level test imports. It also manually constructed App instead of calling the existing create_test_app() helper.
…ls_affordance_line
…ded settings Settings::load() may return a non-default composer_density on some CI environments. Explicitly set ComposerDensity::Comfortable so the expected cursor position is deterministic across all platforms.
Use a 2× cycle offset so the animated frame index is 2 (maximally distant from 0), giving 1800 ms of headroom before the animation could wrap back to index 0. The previous 1× offset left only ~15 ms of margin, causing flaky failures on Windows where Instant resolution is approximately 15.6 ms.
- Fix rustfmt line-length issue in history.rs tool animation test - Settings::path() now respects DEEPSEEK_CONFIG_PATH for Windows test compat - doctor_check_mcp_server recognizes Unix-style absolute paths on Windows - Use checked_sub for Instant arithmetic in web_run tests to prevent underflow on freshly-booted Windows CI runners Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| fn placeholder_visual_lines(content_width: usize) -> usize { | ||
| wrap_text(COMPOSER_PLACEHOLDER, content_width).len().max(1) |
There was a problem hiding this comment.
🟡 placeholder_visual_lines uses character-level wrapping while rendering uses word-level Paragraph::wrap, causing line-count mismatch
placeholder_visual_lines (crates/tui/src/tui/widgets/mod.rs:818-819) calls wrap_text(COMPOSER_PLACEHOLDER, content_width) which does character-level wrapping. However, the actual rendering at crates/tui/src/tui/widgets/mod.rs:268-270 uses Paragraph::new(lines).wrap(Wrap { trim: false }), which does word-level wrapping. The wrap_text function has a known behavior where text that exactly fills the width produces a trailing empty string (the current_width >= width check at line 1020 flushes and resets, then lines.push(current) at line 1027 appends ""). For COMPOSER_PLACEHOLDER ("Write a task or use /.", exactly 22 ASCII chars), at content_width=22 (terminal width 24 with borders), wrap_text returns ["Write a task or use /.", ""] (2 elements) while Paragraph::wrap renders 1 visual line. The same issue occurs at content_width=11. This causes composer_top_padding to subtract 1 extra row, positioning the placeholder text 1 row higher than intended. Both render() and cursor_pos() use the same estimate, so the cursor remains consistent with the rendered text.
Prompt for agents
In crates/tui/src/tui/widgets/mod.rs, the placeholder_visual_lines function at line 818-819 estimates how many visual rows the placeholder will occupy by calling wrap_text(), which does character-level wrapping. However, the actual rendering (line 268-270) uses Paragraph::wrap(Wrap { trim: false }) which does word-level wrapping. These can disagree on the number of lines, especially when text exactly fills the width (wrap_text produces a trailing empty line due to the flush-then-push pattern at lines 1020-1027).
Two possible fixes:
1. Pre-wrap the placeholder text using wrap_text (the same way non-empty input is handled), creating multiple Line objects instead of relying on Paragraph::wrap. This makes the estimate trivially correct.
2. Strip trailing empty strings from the wrap_text result in placeholder_visual_lines, e.g.: wrap_text(COMPOSER_PLACEHOLDER, content_width).into_iter().filter(|l| !l.is_empty() || ...).count(). However this is fragile.
Option 1 is recommended: in the render() method around line 214-218, instead of creating a single Line from COMPOSER_PLACEHOLDER, wrap it with wrap_text(COMPOSER_PLACEHOLDER, content_width) and create one Line per wrapped segment, just like the non-empty input path at lines 220-225. Then placeholder_visual_lines can be replaced with input_lines.len().
Was this helpful? React with 👍 or 👎 to provide feedback.
* fix: address PR #4 follow-ups Honor low_motion in the default tool transcript path and align composer cursor padding with the rendered placeholder. Add focused regression tests for both behaviors. * lint: remove redundant imports in empty_state test, reuse create_test_app The test had inner `use` statements for Config, App, TuiOptions, and PathBuf that duplicated the module-level test imports. It also manually constructed App instead of calling the existing create_test_app() helper. * fix: replace useless format!("{text}") with text.to_string() in details_affordance_line * test: pin composer_density in cursor test to avoid sensitivity to loaded settings Settings::load() may return a non-default composer_density on some CI environments. Explicitly set ComposerDensity::Comfortable so the expected cursor position is deterministic across all platforms. * fix: make tool low_motion test robust against coarse Windows timers Use a 2× cycle offset so the animated frame index is 2 (maximally distant from 0), giving 1800 ms of headroom before the animation could wrap back to index 0. The previous 1× offset left only ~15 ms of margin, causing flaky failures on Windows where Instant resolution is approximately 15.6 ms. * fix: correct headroom comment in tool animation test (3600ms, not 1800ms) * fix: resolve lint, parity, and Windows test failures - Fix rustfmt line-length issue in history.rs tool animation test - Settings::path() now respects DEEPSEEK_CONFIG_PATH for Windows test compat - doctor_check_mcp_server recognizes Unix-style absolute paths on Windows - Use checked_sub for Instant arithmetic in web_run tests to prevent underflow on freshly-booted Windows CI runners * fix: expand ~ in DEEPSEEK_CONFIG_PATH when resolving settings path ---------
Sub-area #5 of the v0.6.6 UI redesign (issue #121). Spout strip: - Replace box-drawing arc cups (`╭───╮`) with paired crests (`⌒‿`) over the existing water-surface (`─`). Crests read as gentle ripples instead of hard architectural arches — calmer eye-feel. - Two crests at independent cadences: A advances every 4 ticks (~600 ms), B every 6 ticks (~900 ms). Phase jitter every 17 ticks (~2.5 s) keeps the pattern from settling into a strict beat. - Frame counter cadence in `ui.rs` retimed from 80 ms to 150 ms so the 4×6×17 tick math lands on the spec'd timings. Footer left cluster consolidates "what costs you what": - Cost chip moves from the right-hand parade to the left, between model and status: `mode · model · cost · status`. - Priority drop is now status → cost → truncate model → mode-only. Cost outranks status because it's steady info; status is a transient signal. - Right cluster shrinks to coherence / agents / replay / cache. Tests: existing strip determinism / position-advances tests are retuned for the new tick math (12-tick window covers both crests). New tests pin the cost slot's order on wide widths and confirm cost survives status drop in the priority cascade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sub-area Hmbown#5 of the v0.6.6 UI redesign (issue Hmbown#121). Spout strip: - Replace box-drawing arc cups (`╭───╮`) with paired crests (`⌒‿`) over the existing water-surface (`─`). Crests read as gentle ripples instead of hard architectural arches — calmer eye-feel. - Two crests at independent cadences: A advances every 4 ticks (~600 ms), B every 6 ticks (~900 ms). Phase jitter every 17 ticks (~2.5 s) keeps the pattern from settling into a strict beat. - Frame counter cadence in `ui.rs` retimed from 80 ms to 150 ms so the 4×6×17 tick math lands on the spec'd timings. Footer left cluster consolidates "what costs you what": - Cost chip moves from the right-hand parade to the left, between model and status: `mode · model · cost · status`. - Priority drop is now status → cost → truncate model → mode-only. Cost outranks status because it's steady info; status is a transient signal. - Right cluster shrinks to coherence / agents / replay / cache. Tests: existing strip determinism / position-advances tests are retuned for the new tick math (12-tick window covers both crests). New tests pin the cost slot's order on wide widths and confirm cost survives status drop in the priority cascade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🟡 Hmbown#5 Memory dedup (remember tool): - Before writing to vector DB, search for existing memories with score > 0.85 - Skip duplicate writes to prevent vector store pollution - Adds dedup check in remember tool execute() 🟡 Hmbown#6 InMemoryBackend OOM prevention: - Added MAX_IN_MEMORY_ITEMS = 1000 cap - New max_items field with with_max_items() builder - Eviction in store_memory(): oldest first, expired-TTL prioritized - Summary cap tracking in store_summary() 🟡 Hmbown#10 TTL delete three-tier fallback: - Tier 1: SQL CAST(ttl AS INT64) < nanos - Tier 2: SQL ttl < nanos (native timestamp compare) - Tier 3: InMemoryBackend handles cleanup, LanceDB ages out Tests: 17/17 vector_db ✓, 62/62 compaction ✓
Summary
low_motionfor tool cells in the default transcript rendering pathContext
Follow-up to #4 ("Remove Normal mode and consolidate to Agent"). These address the unresolved review findings that remained after merge.
Validation
cargo fmt --all --checkcargo checkcargo test --workspace --all-featurescargo check -p deepseek-tuicargo test -p deepseek-tui tool_lines_with_options_respects_low_motion_in_default_pathcargo test -p deepseek-tui empty_composer_cursor_matches_placeholder_padding