fix: dialogs textboxes cursor #147
Conversation
WalkthroughPropagated a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/ui/components/dialogs/label_dialogs.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @src/ui/components/dialogs/label_dialogs.rs:
- Around line 54-55: The cursor x calculation in label_dialogs.rs uses an
unchecked cast of cursor_position to u16 in f.set_cursor_position((chunks[0].x +
1 + cursor_position as u16, chunks[0].y + 1)), which can overflow for very long
inputs; fix it by clamping or safely converting cursor_position to u16 before
adding the chunk offset (e.g., compute let offset = chunks[0].x + 1; let
safe_pos =
u16::try_from(cursor_position).unwrap_or(u16::MAX.saturating_sub(offset)); then
use f.set_cursor_position((offset.saturating_add(safe_pos), chunks[0].y + 1))),
ensuring you reference the same symbols: cursor_position, chunks[0].x,
f.set_cursor_position.
In @src/ui/components/dialogs/project_dialogs.rs:
- Around line 70-71: The cursor position calculation for f.set_cursor_position
uses a cast of cursor_position to u16 which can overflow for very large inputs;
update the logic around f.set_cursor_position (the expression using chunks[0].x
and cursor_position) to safely compute the final u16 by checking/clamping the
value before casting (e.g., compute base = chunks[0].x + 1, compute desired =
base as u32 + cursor_position as u32, then if desired > u16::MAX clamp to
u16::MAX or skip setting the cursor), and use the clamped/u16-safe value in
f.set_cursor_position to avoid overflow and misplacement.
- Around line 112-113: Guard the u16 cast for cursor_position before computing
the cursor x; replace the direct cast in f.set_cursor_position((chunks[0].x + 1
+ cursor_position as u16, chunks[0].y + 1)) by first converting cursor_position
with u16::try_from(cursor_position).unwrap_or(u16::MAX) (or similar), then
compute the final x using saturating_add (e.g., let cursor_u16 =
u16::try_from(cursor_position).unwrap_or(u16::MAX); let x =
chunks[0].x.saturating_add(1).saturating_add(cursor_u16);) and call
f.set_cursor_position((x, chunks[0].y.saturating_add(1))); this ensures no
overflow when calculating the cursor position for f.set_cursor_position and
references cursor_position, chunks, and f.set_cursor_position.
🧹 Nitpick comments (1)
src/ui/components/dialogs/common.rs (1)
18-30: Consider adding a doc comment explaining the cursor handling change.The
_cursor_positionparameter is unused because cursor rendering responsibility has shifted to callers (who usef.set_cursor_position()). While the underscore prefix correctly indicates this, a doc comment would help future maintainers understand why the parameter exists despite being unused.📝 Suggested documentation
-/// Creates an input field block with a visual cursor -pub fn create_input_paragraph<'a>(input_buffer: &'a str, _cursor_position: usize, field_title: &str) -> Paragraph<'a> { +/// Creates an input field block. +/// +/// Note: cursor_position is accepted for signature consistency but unused. +/// Callers are responsible for positioning the terminal cursor via Frame::set_cursor_position(). +pub fn create_input_paragraph<'a>(input_buffer: &'a str, _cursor_position: usize, field_title: &str) -> Paragraph<'a> {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/ui/components/dialog_component.rssrc/ui/components/dialogs/common.rssrc/ui/components/dialogs/label_dialogs.rssrc/ui/components/dialogs/project_dialogs.rssrc/ui/components/dialogs/task_dialogs.rssrc/ui/renderer.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/ui/components/dialogs/common.rs (1)
src/ui/components/dialog_component.rs (2)
default(66-68)new(72-92)
src/ui/components/dialogs/project_dialogs.rs (2)
src/ui/components/dialogs/common.rs (1)
create_input_paragraph(19-30)src/ui/components/dialog_component.rs (1)
render_project_edit_dialog(338-340)
src/ui/components/dialogs/label_dialogs.rs (2)
src/ui/components/dialogs/common.rs (1)
create_input_paragraph(19-30)src/ui/components/dialog_component.rs (1)
render_label_creation_dialog(342-344)
src/ui/components/dialogs/task_dialogs.rs (1)
src/ui/components/dialogs/common.rs (1)
create_input_paragraph(19-30)
src/ui/components/dialog_component.rs (2)
src/ui/components/dialogs/project_dialogs.rs (1)
render_project_edit_dialog(74-114)src/ui/components/dialogs/label_dialogs.rs (2)
render_label_creation_dialog(58-66)render_label_edit_dialog(68-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (7)
src/ui/renderer.rs (1)
32-33: Verify cursor visibility doesn't interfere with non-dialog UI elements.Enabling the cursor globally after terminal initialization means the cursor will be visible throughout the application, not just in dialogs. While cursor positioning is only explicitly set in dialog rendering, the visible cursor could appear in unexpected locations when navigating other UI components.
Please verify that the cursor doesn't appear in distracting locations when:
- Navigating the task list
- Scrolling through projects
- Using other non-dialog UI components
If issues arise, consider toggling cursor visibility when entering/exiting dialogs rather than enabling it globally.
src/ui/components/dialog_component.rs (1)
319-319: LGTM!The cursor_position propagation is consistent across all dialog rendering functions.
Also applies to: 332-332, 339-339, 343-343, 347-347, 365-365
src/ui/components/dialogs/label_dialogs.rs (1)
11-18: LGTM!The cursor_position parameter is correctly propagated through the label dialog rendering functions, maintaining consistency with the broader cursor handling refactor.
Also applies to: 37-37, 58-76
src/ui/components/dialogs/project_dialogs.rs (1)
11-72: LGTM with one caveat.The cursor_position parameter is correctly integrated into the project creation dialog. However, watch for the overflow issue noted below.
src/ui/components/dialogs/task_dialogs.rs (3)
17-17: LGTM: Cursor position parameter additions.The addition of the
cursor_positionparameter to all three functions is consistent and appropriate for propagating cursor state through the dialog rendering pipeline.Also applies to: 90-90, 111-111
41-41: LGTM: Parameter passed for API consistency.The
cursor_positionis passed tocreate_input_paragrapheven though it's currently unused there (indicated by the_cursor_positionparameter name in common.rs). This maintains a consistent API across dialog types, with actual cursor positioning handled explicitly at line 81.
94-104: LGTM: Wrapper functions correctly propagate cursor position.Both
render_task_creation_dialogandrender_task_edit_dialogcorrectly accept thecursor_positionparameter and pass it through to the corerender_task_dialogfunction with appropriateis_editingflags.Also applies to: 115-125
| } | ||
|
|
||
| // Display the input buffer with cursor highlighting (safe for multi-byte chars) | ||
| f.set_cursor_position((layout[0].x + 1 + self.cursor_position as u16, layout[0].y + 1)); |
There was a problem hiding this comment.
Guard against potential u16 overflow in cursor position calculation.
When cursor_position exceeds 65,535 characters, casting to u16 will truncate the value, causing the cursor to appear at an incorrect position. While unlikely for typical text input, this could occur with very long content (e.g., pasting large text).
🔎 Proposed fix with bounds check
- f.set_cursor_position((layout[0].x + 1 + self.cursor_position as u16, layout[0].y + 1));
+ let cursor_x = layout[0].x + 1 + (self.cursor_position.min(u16::MAX as usize)) as u16;
+ f.set_cursor_position((cursor_x, layout[0].y + 1));Note: This same pattern appears in other dialog files and should be addressed consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.set_cursor_position((layout[0].x + 1 + self.cursor_position as u16, layout[0].y + 1)); | |
| let cursor_x = layout[0].x + 1 + (self.cursor_position.min(u16::MAX as usize)) as u16; | |
| f.set_cursor_position((cursor_x, layout[0].y + 1)); |
|
|
||
| // Set terminal cursor position | ||
| f.set_cursor_position((chunks[0].x + 1 + cursor_position as u16, chunks[0].y + 1)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file and see the context around lines 79-81
sed -n '70,90p' src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 751
🏁 Script executed:
# Check file length to understand the full context
wc -l src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 109
🏁 Script executed:
# Search for the function definitions that contain this code
rg -n "fn render_task" src/ui/components/dialogs/task_dialogs.rs -A 3Repository: romaintb/terminalist
Length of output: 385
🏁 Script executed:
# Find all call sites for these render functions
rg -n "render_task_(dialog|creation_dialog|edit_dialog)" --type rust -B 2 -A 5Repository: romaintb/terminalist
Length of output: 5586
🏁 Script executed:
# Now let's check how cursor_position is managed in dialog_component.rs
rg -n "cursor_position" src/ui/components/dialog_component.rs -B 2 -A 2 | head -80Repository: romaintb/terminalist
Length of output: 3193
🏁 Script executed:
# Look for where cursor_position is modified/incremented
rg -n "cursor_position\s*=" src/ui/components/dialog_component.rs -B 3 -A 3Repository: romaintb/terminalist
Length of output: 2114
🏁 Script executed:
# Check the input_buffer and cursor behavior - find handle_input or key handling
rg -n "fn handle_input|KeyCode::" src/ui/components/dialog_component.rs -A 5 | head -100Repository: romaintb/terminalist
Length of output: 4281
🏁 Script executed:
# Let's see the full context of cursor_position field in DialogComponent struct
rg -n "struct DialogComponent" src/ui/components/dialog_component.rs -A 30Repository: romaintb/terminalist
Length of output: 1315
🏁 Script executed:
# Let's check handle_submit to see if there are any length limits on input
rg -n "fn handle_submit" src/ui/components/dialog_component.rs -A 50 | head -80Repository: romaintb/terminalist
Length of output: 2577
🏁 Script executed:
# Check if there are any constants or limits defined for input field width
rg -n "const|MAX|LIMIT" src/ui/components/dialogs/task_dialogs.rs | head -20Repository: romaintb/terminalist
Length of output: 89
🏁 Script executed:
# Let's look more closely at how the field width is actually constrained
sed -n '1,35p' src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 1186
🏁 Script executed:
# Let's check the actual width of the dialog and field
sed -n '20,45p' src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 1079
🏁 Script executed:
# Search for any comments or issues related to cursor positioning or overflow
rg -i "cursor|overflow|bound" src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 426
🏁 Script executed:
# Check git history to see if there were any cursor-related fixes
git log --oneline -20 src/ui/components/dialogs/task_dialogs.rsRepository: romaintb/terminalist
Length of output: 101
🏁 Script executed:
# Let's verify that cursor_position cannot grow beyond input_buffer length
# by checking all places where cursor_position is modified
rg -n "cursor_position\s*[+=-]" src/ui/components/dialog_component.rsRepository: romaintb/terminalist
Length of output: 1063
🏁 Script executed:
# Let's check if there's input validation or length limiting anywhere
rg -n "input_buffer" src/ui/components/dialog_component.rs -B 1 -A 1 | grep -i "len\|max\|limit"Repository: romaintb/terminalist
Length of output: 337
🏁 Script executed:
# Check what centered_rect_lines does to understand actual dialog width
rg -n "fn centered_rect_lines" src/ui/layout.rs -A 15Repository: romaintb/terminalist
Length of output: 575
🏁 Script executed:
# Let's trace the actual character-to-visual-width calculation
sed -n '600,680p' src/ui/components/dialog_component.rsRepository: romaintb/terminalist
Length of output: 3542
🏁 Script executed:
# Let's check the git commit that fixed cursor positioning to understand what the issue was
git show 73d91b6 --statRepository: romaintb/terminalist
Length of output: 7387
🏁 Script executed:
# Let's see the actual changes in that commit
git show 73d91b6Repository: romaintb/terminalist
Length of output: 50377
🏁 Script executed:
# Check if there are any input length limits in the codebase
rg -n "len()|.len|MAX_|LIMIT" src/ui/components/dialog_component.rs | grep -i "input\|buffer\|length" | head -15Repository: romaintb/terminalist
Length of output: 227
Add input length limit or use saturating cast for safer cursor positioning.
The cursor_position as u16 cast at line 81 could overflow if the input buffer exceeds 65,535 characters. While cursor movement is already validated (bounded to char_count), there's no explicit length limit on input_buffer itself.
For practical robustness, either:
- Add a
MAX_INPUT_LENGTHconstant (e.g., 4,096 chars) and enforce it when inserting characters, or - Use
cursor_position.saturating_cast::<u16>()to safely clamp overflow
This is a cosmetic edge case (misplaced cursor rendering rather than a crash), but fixing it improves defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/ui/components/dialogs/task_dialogs.rs:
- Around line 80-82: The cursor placement call
f.set_cursor_position((chunks[0].x + 1 + cursor_position as u16, chunks[0].y +
1)) can push the cursor past the input field's right edge; update the logic so
cursor x is clamped to the visible input region (e.g., min(chunks[0].x +
chunks[0].width - 1, chunks[0].x + 1 + cursor_position as u16)) or ensure
dialog_component.rs (functions that manage cursor_position/scrolling) implements
a viewport offset so cursor_position never exceeds the visible width; locate
f.set_cursor_position in task_dialogs.rs and the related cursor/scrolling code
in dialog_component.rs and add bounds checking or viewport offset adjustments
and avoid unchecked as u16 casts.
🧹 Nitpick comments (1)
src/ui/components/dialogs/task_dialogs.rs (1)
42-42: Consider removing the unused cursor_position parameter from create_input_paragraph.The
cursor_positionparameter is passed tocreate_input_paragraphbut is unused (prefixed with_in the function signature). Since cursor positioning is now handled explicitly viaf.set_cursor_position()at line 82, consider whether this parameter should be removed from thecreate_input_paragraphAPI to improve clarity and reduce API surface area.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/components/dialogs/task_dialogs.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/components/dialogs/task_dialogs.rs (1)
src/ui/components/dialogs/common.rs (1)
create_input_paragraph(19-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (ubuntu-latest, beta)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (3)
src/ui/components/dialogs/task_dialogs.rs (3)
12-22: LGTM! Clippy attribute appropriately applied.The
#[allow(clippy::too_many_arguments)]attribute is justified here, as the function now has 8 parameters (exceeding Clippy's default limit of 7). The additionalcursor_positionparameter is necessary for proper cursor positioning in dialog input fields.
86-105: LGTM! Wrapper function correctly propagates cursor_position.The
render_task_creation_dialogfunction properly accepts and passes through thecursor_positionparameter torender_task_dialog, maintaining backward compatibility while supporting the new cursor positioning functionality.
107-126: LGTM! Wrapper function correctly propagates cursor_position.The
render_task_edit_dialogfunction properly accepts and passes through thecursor_positionparameter torender_task_dialog, maintaining consistency with the creation dialog wrapper and supporting the new cursor positioning functionality.
|
|
||
| // Set terminal cursor position | ||
| f.set_cursor_position((chunks[0].x + 1 + cursor_position as u16, chunks[0].y + 1)); |
There was a problem hiding this comment.
Verify cursor doesn't render beyond input field boundaries.
The cursor positioning calculation chunks[0].x + 1 + cursor_position as u16 doesn't check if the cursor would be positioned beyond the right edge of the input field (i.e., beyond chunks[0].x + chunks[0].width - 1). If cursor_position is large relative to the input field width, the cursor could render outside the visible input boundaries.
Please verify that the caller (likely dialog_component.rs) implements scrolling or view windowing logic to ensure cursor_position never exceeds the visible width of the input field.
#!/bin/bash
# Verify if dialog_component.rs handles cursor scrolling or bounds checking
rg -n "cursor_position|scroll|view.*window" src/ui/components/dialog_component.rs -B 2 -A 2Note: The as u16 cast overflow concern has already been flagged in a previous review comment.
🤖 Prompt for AI Agents
In @src/ui/components/dialogs/task_dialogs.rs around lines 80 - 82, The cursor
placement call f.set_cursor_position((chunks[0].x + 1 + cursor_position as u16,
chunks[0].y + 1)) can push the cursor past the input field's right edge; update
the logic so cursor x is clamped to the visible input region (e.g.,
min(chunks[0].x + chunks[0].width - 1, chunks[0].x + 1 + cursor_position as
u16)) or ensure dialog_component.rs (functions that manage
cursor_position/scrolling) implements a viewport offset so cursor_position never
exceeds the visible width; locate f.set_cursor_position in task_dialogs.rs and
the related cursor/scrolling code in dialog_component.rs and add bounds checking
or viewport offset adjustments and avoid unchecked as u16 casts.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.