Skip to content

Conversation

rushilpatel0
Copy link
Contributor

Motivation

Content

Testing

Please check the following before marking your PR as ready for review

  • I have added tests for my changes
  • I have updated the documentation or added new documentation as needed

@rushilpatel0 rushilpatel0 requested review from a team and codegen-team as code owners August 18, 2025 23:15
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

❌ Patch coverage is 3.70370% with 52 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/codegen/cli/tui/app.py 3.70% 52 Missing ⚠️

# Handle escape sequences (arrow keys)
if ch == "\x1b": # ESC
# Peek for additional bytes to distinguish bare ESC vs sequences
ready, _, _ = select.select([sys.stdin], [], [], 0.03)
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug: Incomplete escape sequence returns "ESC[" which no handler recognizes
Treat incomplete sequences as a bare ESC to avoid emitting a partial key that downstream logic won't match.

Suggested change
ready, _, _ = select.select([sys.stdin], [], [], 0.03)
if not ready2:
return "\x1b" # treat as bare Esc on incomplete sequence

self.current_tab = 0 # Switch to recents tab
self.input_mode = False
self._load_agent_runs() # Refresh the data
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug: Selecting "go to recents" doesn’t switch to the recents tab
Currently it only refreshes the data. It should navigate to the recents tab and exit input mode.

Suggested change
break
self.current_tab = 0 # switch to recents
self.input_mode = False
self._load_agent_runs() # Refresh the data
break

for line in lines:
sys.stdout.write("\x1b[2K\r") # Clear entire line and return carriage
sys.stdout.write(f"{line}\n")
count += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug: inplace_print leaves stale lines when new frame has fewer lines
When the current render has fewer lines than the previous one, the extra old lines are not cleared, causing visual artifacts.

Suggested change
count += 1
# Clear any leftover lines from previous render and reposition
for _ in range(prev_lines_rendered - count):
sys.stdout.write("\x1b[2K\r\n") # Clear and move to next line
if prev_lines_rendered > count:
sys.stdout.write(f"\x1b[{prev_lines_rendered - count}F") # Move back up to end of new block
sys.stdout.flush()

if total <= window_size:
start = 0
end = total
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug: Off-by-one highlighting when list is scrolled
prefix compares i against self.selected_index, but i is the sliced-window index while self.selected_index is absolute, so the selected row loses its arrow when start > 0.

Suggested change
else:
is_selected = (i == self.selected_index)
# After slicing, compare against absolute index
# Alternatively compute: is_selected = (i == self.selected_index)
# Fix by using the absolute index when iterating
for idx in range(start, end):
agent_run = self.agent_runs[idx]
is_selected = (idx == self.selected_index) and not self.show_action_menu
prefix = "\u2192 " if is_selected else " "
...

The number of lines rendered this call. Use as prev_lines_rendered on the next call.
"""
# Move cursor up to the start of the previous block (if any)
if prev_lines_rendered > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Terminal control bug: Using CSI F moves to column 1; use Cursor Up (A) to preserve column
\x1b[{n}F moves to beginning of line n lines up, which can break positioning on terminals; \x1b[{n}A is the standard for moving up without changing column.

Suggested change
if prev_lines_rendered > 0:
sys.stdout.write(f"\x1b[{prev_lines_rendered}A") # Move cursor up N lines without changing column

if i == self.selected_index and not self.show_action_menu:
# Blue timestamp and summary for selected row, but preserve status colors
line = f"\033[34m{prefix}{created:<10}\033[0m {status} \033[34m{summary}\033[0m"
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic bug: Padding logic pads too many lines when list is empty
_pad_to_lines is called with total_printed instead of printed_rows, causing over-padding when menu_lines > 0 or under some conditions.

Suggested change
else:
# Pass the number of already printed content rows, not the total including menu
self._pad_to_lines(printed_rows + menu_lines)

if ch == "\x1b": # ESC
# Peek for additional bytes to distinguish bare ESC vs sequences
ready, _, _ = select.select([sys.stdin], [], [], 0.03)
if not ready:
Copy link
Contributor

Choose a reason for hiding this comment

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

Robustness: _get_char swallows unexpected ESC sequences
Currently any ESC followed by a non-[ starts the read of ch3 only if [, otherwise it falls through without returning. Ensure a return path for other ESC-prefixed sequences.

Suggested change
if not ready:
else:
# Unknown ESC sequence, treat as bare ESC
return "\x1b"

Copy link
Contributor

codegen-sh bot commented Aug 18, 2025

Found 6 issues. Please review my inline comments above.

🔍 View my analysis

@rushilpatel0 rushilpatel0 merged commit 90a79c4 into develop Aug 18, 2025
14 of 18 checks passed
@rushilpatel0 rushilpatel0 deleted the rpatel/cli-fixes branch August 18, 2025 23:25
Copy link
Contributor

🎉 This PR is included in version 0.56.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants