Improve cursor handling and add terminal capabilities (title, shape, blink)#119
Open
wuestenfee wants to merge 5 commits intoomarluq:mainfrom
Open
Improve cursor handling and add terminal capabilities (title, shape, blink)#119wuestenfee wants to merge 5 commits intoomarluq:mainfrom
wuestenfee wants to merge 5 commits intoomarluq:mainfrom
Conversation
✅ Deploy Preview for termisu canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors cursor state management by migrating from a standalone Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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
🧹 Nitpick comments (5)
src/termisu/cell.cr (1)
43-43: Use SCREAMING_SNAKE_CASE for the logger constant.
Logworks, but it violates the project’s Crystal constant naming rule. Renaming it keeps style consistent.Suggested refactor
- Log = ::Log.for("termisu.buffer.cell") + LOG = ::Log.for("termisu.buffer.cell") @@ - Log.debug { "Cell: multi-grapheme input truncated (#{grapheme.grapheme_size} graphemes, kept first)" } + LOG.debug { "Cell: multi-grapheme input truncated (#{grapheme.grapheme_size} graphemes, kept first)" }As per coding guidelines "Use SCREAMING_SNAKE_CASE for Crystal constants".
Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/cell.cr` at line 43, The constant `Log` is not SCREAMING_SNAKE_CASE—rename it to a screaming-snake identifier (e.g., `TERMISU_BUFFER_CELL_LOG`) and update every usage in this file (references such as the one at the other occurrence currently using `Log`) to the new name; ensure the constant initialization stays the same (using ::Log.for("termisu.buffer.cell")) but the constant identifier follows SCREAMING_SNAKE_CASE everywhere in this file.src/termisu/terminal/cursor.cr (2)
95-106: Consider clarifying overflow behavior.The
writemethod logs an error on overflow but clamps silently. While this is likely intentional for robustness, the behavior differs from typical write operations. Consider whether an explicit comment or a different log level (warn) would be more appropriate, since this isn't necessarily an application error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal/cursor.cr` around lines 95 - 106, The write method logs an error on overflow but then silently clamps the cursor; update this to clarify intent and reduce noise by either changing Log.error to Log.warn or adding an inline comment explaining why clamping is acceptable for robustness; modify the block in write (the overflow branch that references `@cursor.x`, size, and width and currently calls Log.error) to use Log.warn { "..." } and/or add a one-line comment above it describing that writes past the terminal width are clamped to width - 1 intentionally to avoid exceptions.
19-25: Confusing double-toggle logic inapply_cursor_state.Line 24 is difficult to reason about:
(@cursor.visible= !@cursor.visible?) ? hide_cursor : show_cursor.This toggles visibility, then calls the "opposite" method which toggles it again (since
hide_cursorsetsvisible = falseandshow_cursorsetsvisible = true). The net effect restores the original visibility while emitting the appropriate escape sequence.Consider refactoring for clarity:
♻️ Proposed refactor for clearer intent
private def apply_cursor_state x, y = `@cursor.x`, `@cursor.y` `@cursor.x`, `@cursor.y` = -1, -1 move_cursor(x, y) - (`@cursor.visible` = !@cursor.visible?) ? hide_cursor : show_cursor + if `@cursor.visible`? + `@cursor.visible` = false # Temporarily mark hidden so show_cursor emits sequence + show_cursor + else + `@cursor.visible` = true # Temporarily mark visible so hide_cursor emits sequence + hide_cursor + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal/cursor.cr` around lines 19 - 25, The apply_cursor_state method currently flips `@cursor.visible` then calls hide_cursor or show_cursor, which double-toggles and is confusing; change it to read the current visibility via `@cursor.visible`? and then call the appropriate emitter without first assigning the inverse. Concretely, in apply_cursor_state use the stored x,y handling as-is (move_cursor call) but replace the line with a conditional that calls hide_cursor if `@cursor.visible`? is true and show_cursor if false, ensuring hide_cursor/show_cursor themselves set `@cursor.visible` appropriately rather than relying on a prior assignment.src/termisu.cr (1)
178-180: Minor: Stacked docstrings could be clearer.The two docstrings "Hides the cursor." and "Shows the cursor." are on consecutive lines before a single
delegatestatement. While Crystal handles this, consider separating them for clarity:# Hides the cursor. delegate hide_cursor, to: `@terminal` # Shows the cursor. delegate show_cursor, to: `@terminal`Or use a combined docstring if keeping a single delegate:
- # Hides the cursor. - # Shows the cursor. + # Hides or shows the cursor. delegate hide_cursor, show_cursor, to: `@terminal`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu.cr` around lines 178 - 180, The stacked docstrings for hide_cursor and show_cursor before a single delegate call are ambiguous; split them so each method has its own docstring or replace with a combined docstring. Update the delegate usage around the delegate hide_cursor, show_cursor, to: `@terminal` statement so either: add a separate comment line immediately above a dedicated delegate hide_cursor, to: `@terminal` and another above delegate show_cursor, to: `@terminal`, or write one combined docstring clearly describing both methods before the single delegate declaration referencing `@terminal`.src/termisu/terminal.cr (1)
680-682: Consider adding state caching for title to avoid redundant escape sequences.Setting the same title multiple times will emit the escape sequences each time. While title changes are typically infrequent, adding a guard would align with the caching pattern used elsewhere in Terminal (e.g.,
foreground=,background=).♻️ Optional: Add state caching
def title=(`@title` : String) + return if `@title` == title write(`@terminfo.to_status_line_seq` + `@title` + `@terminfo.from_status_line_seq`) endNote: This requires storing the previous title for comparison, or reordering assignment:
-def title=(`@title` : String) +def title=(title : String) + return if `@title` == title + `@title` = title write(`@terminfo.to_status_line_seq` + `@title` + `@terminfo.from_status_line_seq`) endBased on learnings: "Apply state caching pattern in Terminal to avoid redundant escape sequences."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal.cr` around lines 680 - 682, The title= setter unconditionally emits the status-line escape sequences even if the value hasn't changed; modify Terminal#title= to cache/compare the current title (e.g., `@title`) against the new value and only call write(`@terminfo.to_status_line_seq` + new_title + `@terminfo.from_status_line_seq`) when they differ, ensuring you assign `@title` appropriately (assign after the write or use the new value in the comparison) to mirror the caching pattern used by foreground= and background= and avoid redundant escape output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/support/mock_renderer.cr`:
- Around line 41-43: The abstract Renderer interface's write method signature
currently lacks the columns_advanced parameter but Buffer calls
renderer.write(chars, columns_advanced) and the Cursor wrapper expects it;
update the abstract method declaration (the abstract def write) to read as
accepting a second parameter with default (columns_advanced = 0) so concrete
implementations (including MockRenderer.write) match the contract and compile
against the interface; ensure the method name is unchanged and only the
signature is extended.
In `@spec/termisu/terminal_spec.cr`:
- Around line 538-545: The example in the describe "#show_cursor" block is
missing an it block so the assertions won't run; wrap the existing setup and
expectations (creating Termisu::Terminal via Termisu::Terminal.new, calling
show_cursor and hide_cursor, and asserting cursor.visible?) inside an it "..."
do ... end block (similar to the `#hide_cursor` spec) so the test executes and
reads meaningfully.
- Around line 547-586: The test inside describe "#move_cursor" is missing an it
block and does not ensure terminal cleanup; wrap the existing assertions in an
it "moves the cursor correctly" do ... end block and add an ensure/finally-style
cleanup to call terminal.close (or the appropriate teardown method on
CaptureTerminal) so the terminal is closed even if an assertion fails; keep
references to CaptureTerminal, move_cursor, terminal.size and terminal.cursor
when locating the code to modify.
- Around line 529-536: The spec under describe "#hide_cursor" is missing an it
block, so the assertions run at load time; wrap the test logic in an it block
that describes expected behavior and moves the setup/assertions inside it—locate
the describe "#hide_cursor" block, instantiate Termisu::Terminal.new and call
terminal.hide_cursor / terminal.show_cursor / terminal.hide_cursor and the
cursor.visible? assertions inside a single it "hides the cursor" (or similar)
block so the test executes and reports correctly.
In `@src/termisu/terminal.cr`:
- Around line 670-678: In apply_terminal_state, the code toggles
`@enhanced_keyboard_enabled` which is a typo and creates a new variable instead of
using the existing `@enhanced_keyboard`; change the reference from
`@enhanced_keyboard_enabled` to `@enhanced_keyboard` so the toggle actually flips
the real instance variable and calls disable_enhanced_keyboard or
enable_enhanced_keyboard accordingly; ensure the toggle pattern matches the
mouse block (flip `@enhanced_keyboard` then conditionally call
disable_enhanced_keyboard or enable_enhanced_keyboard) inside the
apply_terminal_state method.
---
Nitpick comments:
In `@src/termisu.cr`:
- Around line 178-180: The stacked docstrings for hide_cursor and show_cursor
before a single delegate call are ambiguous; split them so each method has its
own docstring or replace with a combined docstring. Update the delegate usage
around the delegate hide_cursor, show_cursor, to: `@terminal` statement so either:
add a separate comment line immediately above a dedicated delegate hide_cursor,
to: `@terminal` and another above delegate show_cursor, to: `@terminal`, or write
one combined docstring clearly describing both methods before the single
delegate declaration referencing `@terminal`.
In `@src/termisu/cell.cr`:
- Line 43: The constant `Log` is not SCREAMING_SNAKE_CASE—rename it to a
screaming-snake identifier (e.g., `TERMISU_BUFFER_CELL_LOG`) and update every
usage in this file (references such as the one at the other occurrence currently
using `Log`) to the new name; ensure the constant initialization stays the same
(using ::Log.for("termisu.buffer.cell")) but the constant identifier follows
SCREAMING_SNAKE_CASE everywhere in this file.
In `@src/termisu/terminal.cr`:
- Around line 680-682: The title= setter unconditionally emits the status-line
escape sequences even if the value hasn't changed; modify Terminal#title= to
cache/compare the current title (e.g., `@title`) against the new value and only
call write(`@terminfo.to_status_line_seq` + new_title +
`@terminfo.from_status_line_seq`) when they differ, ensuring you assign `@title`
appropriately (assign after the write or use the new value in the comparison) to
mirror the caching pattern used by foreground= and background= and avoid
redundant escape output.
In `@src/termisu/terminal/cursor.cr`:
- Around line 95-106: The write method logs an error on overflow but then
silently clamps the cursor; update this to clarify intent and reduce noise by
either changing Log.error to Log.warn or adding an inline comment explaining why
clamping is acceptable for robustness; modify the block in write (the overflow
branch that references `@cursor.x`, size, and width and currently calls Log.error)
to use Log.warn { "..." } and/or add a one-line comment above it describing that
writes past the terminal width are clamped to width - 1 intentionally to avoid
exceptions.
- Around line 19-25: The apply_cursor_state method currently flips
`@cursor.visible` then calls hide_cursor or show_cursor, which double-toggles and
is confusing; change it to read the current visibility via `@cursor.visible`? and
then call the appropriate emitter without first assigning the inverse.
Concretely, in apply_cursor_state use the stored x,y handling as-is (move_cursor
call) but replace the line with a conditional that calls hide_cursor if
`@cursor.visible`? is true and show_cursor if false, ensuring
hide_cursor/show_cursor themselves set `@cursor.visible` appropriately rather than
relying on a prior assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9b4ef8d-bbee-41ab-a22f-39c9c7997818
📒 Files selected for processing (31)
examples/keyboard_and_mouse.crspec/support/capture_renderer.crspec/support/capture_terminal.crspec/support/mock_renderer.crspec/termisu/buffer_spec.crspec/termisu/cursor_spec.crspec/termisu/render_state_spec.crspec/termisu/terminal/cursor_spec.crspec/termisu/terminal_caching_spec.crspec/termisu/terminal_spec.crsrc/termisu.crsrc/termisu/buffer.crsrc/termisu/cell.crsrc/termisu/cursor.crsrc/termisu/event/loop.crsrc/termisu/event/poller/kqueue.crsrc/termisu/event/poller/linux.crsrc/termisu/event/poller/poll.crsrc/termisu/event/source/input.crsrc/termisu/event/source/resize.crsrc/termisu/event/source/system_timer.crsrc/termisu/event/source/timer.crsrc/termisu/input/parser.crsrc/termisu/log.crsrc/termisu/reader.crsrc/termisu/render_state.crsrc/termisu/renderer.crsrc/termisu/terminal.crsrc/termisu/terminal/cursor.crsrc/termisu/terminfo.crsrc/termisu/terminfo/database.cr
💤 Files with no reviewable changes (5)
- src/termisu/log.cr
- spec/termisu/buffer_spec.cr
- spec/termisu/render_state_spec.cr
- src/termisu/cursor.cr
- spec/termisu/cursor_spec.cr
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/keyboard_and_mouse.cr (1)
312-312:⚠️ Potential issue | 🔴 CriticalBuild failure:
Termisu::Logshould beTermisu::LOG.This is the root cause of the pipeline failure. The logging constant was renamed from
LogtoLOGthroughout the codebase, but this line was missed.🐛 Proposed fix
- Termisu::Log.info { "Quit requested" } + Termisu::LOG.info { "Quit requested" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/keyboard_and_mouse.cr` at line 312, The code uses the old logging constant Termisu::Log; update this occurrence to the renamed constant Termisu::LOG so the call becomes Termisu::LOG.info (replace Termisu::Log with Termisu::LOG), ensuring consistency with other references to Termisu::LOG in the codebase.
🧹 Nitpick comments (1)
src/termisu/terminal.cr (1)
671-687: The toggle-then-call pattern is confusing but functional.The logic here works but is non-obvious:
- Flip the flag to bypass the guard in
enable_mouse/disable_mouse- Call the method to force re-emit escape sequences
- Method restores the flag to the original value
This ensures terminal state is re-synchronized after mode transitions, but the inverted logic is hard to follow.
♻️ Clearer alternative using a force parameter
- private def apply_terminal_state - if `@mouse_enabled` - `@mouse_enabled` = false - enable_mouse - else - `@mouse_enabled` = true - disable_mouse - end - - if `@enhanced_keyboard` - `@enhanced_keyboard` = false - enable_enhanced_keyboard - else - `@enhanced_keyboard` = true - disable_enhanced_keyboard - end - end + private def apply_terminal_state + # Force re-emit escape sequences to restore terminal state + # Temporarily flip flags to bypass early-return guards + mouse_was_enabled = `@mouse_enabled` + keyboard_was_enabled = `@enhanced_keyboard` + + `@mouse_enabled` = false + `@enhanced_keyboard` = false + + enable_mouse if mouse_was_enabled + enable_enhanced_keyboard if keyboard_was_enabled + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal.cr` around lines 671 - 687, The current apply_terminal_state flips `@mouse_enabled` and `@enhanced_keyboard` flags to bypass guards in enable_mouse/disable_mouse and enable_enhanced_keyboard/disable_enhanced_keyboard which is confusing; modify those four methods (enable_mouse, disable_mouse, enable_enhanced_keyboard, disable_enhanced_keyboard) to accept an optional force (or force: true) parameter that skips the guard and always re-emits sequences, then change apply_terminal_state to call the appropriate enable_* or disable_* with force set so you no longer toggle the instance variables temporarily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/termisu/terminal.cr`:
- Around line 689-693: In the title= setter, the write call uses the old
instance variable `@title` instead of the new title parameter; change the write
invocation in def title=(title : String) to concatenate title (the parameter)
with `@terminfo.to_status_line_seq` and `@terminfo.from_status_line_seq` so the
terminal receives the updated string, and then keep setting `@title` = title
afterwards; ensure the early return when title == `@title` remains unchanged.
In `@src/termisu/terminal/cursor.cr`:
- Line 115: Replace the incorrect logger call Log.error with the class-level
logger LOG.error used by Terminal; locate the overflow log in cursor.cr (the
line calling Log.error { "Overflow: attempting to write beyond the terminal's
width" }) and change it to use LOG.error so it uses the existing LOG =
::Log.for("termisu.terminal") instance for consistent logging.
---
Outside diff comments:
In `@examples/keyboard_and_mouse.cr`:
- Line 312: The code uses the old logging constant Termisu::Log; update this
occurrence to the renamed constant Termisu::LOG so the call becomes
Termisu::LOG.info (replace Termisu::Log with Termisu::LOG), ensuring consistency
with other references to Termisu::LOG in the codebase.
---
Nitpick comments:
In `@src/termisu/terminal.cr`:
- Around line 671-687: The current apply_terminal_state flips `@mouse_enabled` and
`@enhanced_keyboard` flags to bypass guards in enable_mouse/disable_mouse and
enable_enhanced_keyboard/disable_enhanced_keyboard which is confusing; modify
those four methods (enable_mouse, disable_mouse, enable_enhanced_keyboard,
disable_enhanced_keyboard) to accept an optional force (or force: true)
parameter that skips the guard and always re-emits sequences, then change
apply_terminal_state to call the appropriate enable_* or disable_* with force
set so you no longer toggle the instance variables temporarily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4442e5e-8531-442a-8647-a17390bae8f5
📒 Files selected for processing (31)
examples/keyboard_and_mouse.crspec/support/capture_renderer.crspec/support/capture_terminal.crspec/support/mock_renderer.crspec/termisu/buffer_spec.crspec/termisu/cursor_spec.crspec/termisu/render_state_spec.crspec/termisu/terminal/cursor_spec.crspec/termisu/terminal_caching_spec.crspec/termisu/terminal_spec.crsrc/termisu.crsrc/termisu/buffer.crsrc/termisu/cell.crsrc/termisu/cursor.crsrc/termisu/event/loop.crsrc/termisu/event/poller/kqueue.crsrc/termisu/event/poller/linux.crsrc/termisu/event/poller/poll.crsrc/termisu/event/source/input.crsrc/termisu/event/source/resize.crsrc/termisu/event/source/system_timer.crsrc/termisu/event/source/timer.crsrc/termisu/input/parser.crsrc/termisu/log.crsrc/termisu/reader.crsrc/termisu/render_state.crsrc/termisu/renderer.crsrc/termisu/terminal.crsrc/termisu/terminal/cursor.crsrc/termisu/terminfo.crsrc/termisu/terminfo/database.cr
💤 Files with no reviewable changes (4)
- spec/termisu/cursor_spec.cr
- spec/termisu/render_state_spec.cr
- spec/termisu/buffer_spec.cr
- src/termisu/cursor.cr
🚧 Files skipped from review as they are similar to previous changes (9)
- src/termisu/event/poller/linux.cr
- spec/termisu/terminal_spec.cr
- spec/support/capture_terminal.cr
- src/termisu/buffer.cr
- src/termisu/event/source/input.cr
- src/termisu/event/source/system_timer.cr
- src/termisu/event/source/resize.cr
- src/termisu/input/parser.cr
- spec/termisu/terminal/cursor_spec.cr
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/keyboard_and_mouse.cr (1)
300-313:⚠️ Potential issue | 🟡 MinorComplete the logger migration in this example.
Line 312 still uses
Termisu::Log.infowhile surrounding changes useTermisu::LOG. This leaves mixed logging APIs in the same flow.💡 Suggested fix
- Termisu::Log.info { "Quit requested" } + Termisu::LOG.info { "Quit requested" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/keyboard_and_mouse.cr` around lines 300 - 313, The example still uses the old logger API: replace the Termisu::Log.info call inside the main loop (the Quit on ESC or Ctrl+C branch) with the new logger constant Termisu::LOG.info so the code consistently uses Termisu::LOG (update any other stray Termisu::Log.* usages in this file to Termisu::LOG.* as well); locate the call where key_event.key.escape? || key_event.ctrl_c? triggers a quit and change the logger reference accordingly.
🧹 Nitpick comments (1)
src/termisu/terminal.cr (1)
671-687: Make terminal-state reapplication explicit instead of boolean toggling side-effects.Line 671-687 currently flips state flags purely to force
enable_*/disable_*calls. It works, but it’s hard to reason about and fragile for future edits.♻️ Suggested refactor
private def apply_terminal_state - if `@mouse_enabled` - `@mouse_enabled` = false - enable_mouse - else - `@mouse_enabled` = true - disable_mouse - end - - if `@enhanced_keyboard` - `@enhanced_keyboard` = false - enable_enhanced_keyboard - else - `@enhanced_keyboard` = true - disable_enhanced_keyboard - end + reapply_mouse_state(`@mouse_enabled`) + reapply_enhanced_keyboard_state(`@enhanced_keyboard`) end + +private def reapply_mouse_state(enabled : Bool) + if enabled + write(MOUSE_ENABLE_SGR) + write(MOUSE_ENABLE_NORMAL) + else + write(MOUSE_DISABLE_SGR) + write(MOUSE_DISABLE_NORMAL) + end + flush +end + +private def reapply_enhanced_keyboard_state(enabled : Bool) + if enabled + write(KITTY_KEYBOARD_ENABLE) + write(MODIFY_OTHER_KEYS_ENABLE) + else + write(KITTY_KEYBOARD_DISABLE) + write(MODIFY_OTHER_KEYS_DISABLE) + end + flush +end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal.cr` around lines 671 - 687, The current apply_terminal_state method flips the `@mouse_enabled` and `@enhanced_keyboard` booleans to force calls to enable_mouse/disable_mouse and enable_enhanced_keyboard/disable_enhanced_keyboard, which is fragile; change apply_terminal_state to reapply each feature deterministically by calling enable_mouse when `@mouse_enabled` is true and disable_mouse when false (without mutating `@mouse_enabled`), and likewise call enable_enhanced_keyboard when `@enhanced_keyboard` is true and disable_enhanced_keyboard when false, so state is explicit and side-effect free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@examples/keyboard_and_mouse.cr`:
- Around line 300-313: The example still uses the old logger API: replace the
Termisu::Log.info call inside the main loop (the Quit on ESC or Ctrl+C branch)
with the new logger constant Termisu::LOG.info so the code consistently uses
Termisu::LOG (update any other stray Termisu::Log.* usages in this file to
Termisu::LOG.* as well); locate the call where key_event.key.escape? ||
key_event.ctrl_c? triggers a quit and change the logger reference accordingly.
---
Nitpick comments:
In `@src/termisu/terminal.cr`:
- Around line 671-687: The current apply_terminal_state method flips the
`@mouse_enabled` and `@enhanced_keyboard` booleans to force calls to
enable_mouse/disable_mouse and
enable_enhanced_keyboard/disable_enhanced_keyboard, which is fragile; change
apply_terminal_state to reapply each feature deterministically by calling
enable_mouse when `@mouse_enabled` is true and disable_mouse when false (without
mutating `@mouse_enabled`), and likewise call enable_enhanced_keyboard when
`@enhanced_keyboard` is true and disable_enhanced_keyboard when false, so state is
explicit and side-effect free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98033a21-a871-4bc3-9111-4e0b8ed3e377
📒 Files selected for processing (18)
examples/keyboard_and_mouse.crspec/support/capture_renderer.crspec/support/capture_terminal.crspec/support/mock_renderer.crspec/termisu/buffer_spec.crspec/termisu/cursor_spec.crspec/termisu/render_state_spec.crspec/termisu/terminal/cursor_spec.crspec/termisu/terminal_caching_spec.crspec/termisu/terminal_spec.crsrc/termisu.crsrc/termisu/buffer.crsrc/termisu/cursor.crsrc/termisu/render_state.crsrc/termisu/renderer.crsrc/termisu/terminal.crsrc/termisu/terminal/cursor.crsrc/termisu/terminfo.cr
💤 Files with no reviewable changes (4)
- spec/termisu/render_state_spec.cr
- src/termisu/cursor.cr
- spec/termisu/buffer_spec.cr
- spec/termisu/cursor_spec.cr
🚧 Files skipped from review as they are similar to previous changes (6)
- src/termisu/renderer.cr
- src/termisu/terminal/cursor.cr
- spec/support/capture_terminal.cr
- src/termisu/terminfo.cr
- spec/termisu/terminal_spec.cr
- spec/termisu/terminal_caching_spec.cr
wuestenfee
force-pushed
the
main
branch
2 times, most recently
from
ce9627d to
c5e32b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
spec/termisu/terminal_spec.cr (1)
529-548:⚠️ Potential issue | 🟠 MajorClose terminal instances in these specs to prevent resource leakage.
Line 531 and Line 542 create real terminals but never close them. This can leak fds and interfere with later examples.
Proposed fix
describe "#hide_cursor" do it "hides the cursor" do terminal = Termisu::Terminal.new terminal.hide_cursor terminal.cursor.visible?.should be_false terminal.show_cursor terminal.hide_cursor terminal.cursor.visible?.should be_false + ensure + terminal.try &.close end end describe "#show_cursor" do it "shows the cursor" do terminal = Termisu::Terminal.new terminal.show_cursor terminal.cursor.visible?.should be_true terminal.hide_cursor terminal.show_cursor terminal.cursor.visible?.should be_true + ensure + terminal.try &.close end endAs per coding guidelines: Use ensure for terminal cleanup, mode restoration, file closing, and similar lifecycle safety in Crystal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/termisu/terminal_spec.cr` around lines 529 - 548, The specs create real Termisu::Terminal instances (Termisu::Terminal.new) in the "#hide_cursor" and "#show_cursor" examples but never close them; update each example to ensure the terminal is closed in an ensure block (e.g., call terminal.close in ensure) so file descriptors and terminal mode are always cleaned up even on failure; if the Terminal API exposes a mode restore (e.g., restore_mode or similar), call that in ensure as well to guarantee mode restoration.
🧹 Nitpick comments (2)
src/termisu/event/source/system_timer.cr (1)
35-35: Normalize logger namespace key casing for consistency.Line 35 uses
"termisu.event.systemTimer"; the mixed casing makes component-level log filtering inconsistent with the rest of the project’s naming pattern.Proposed change
- LOG = ::Log.for("termisu.event.systemTimer") + LOG = ::Log.for("termisu.event.system_timer")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/event/source/system_timer.cr` at line 35, The logger namespace in the LOG constant uses mixed casing ("termisu.event.systemTimer"); update the LOG initialization (LOG = ::Log.for(...)) to use a normalized, lowercase key such as "termisu.event.system_timer" so it matches the project's component-level logging conventions and enables consistent filtering.src/termisu/terminal.cr (1)
671-687: Clarify intent:apply_terminal_stateforces escape sequence re-emission.The toggle pattern (flip flag, then call enable/disable) is intentional to force re-emission of mouse/keyboard escape sequences after
with_modeblocks where external programs may have changed terminal state. However, the logic could be clearer.Consider adding a brief comment explaining the intent:
📝 Suggested documentation
private def apply_terminal_state + # Force re-emission of mouse/keyboard escape sequences. + # After with_mode blocks, external programs may have changed terminal state. + # Toggle flags to bypass early-return guards in enable/disable methods. if `@mouse_enabled` `@mouse_enabled` = false enable_mouse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal.cr` around lines 671 - 687, Add a brief explanatory comment to the apply_terminal_state method clarifying that the flag-flip then call pattern (toggling `@mouse_enabled` and `@enhanced_keyboard` before calling enable_mouse/disable_mouse and enable_enhanced_keyboard/disable_enhanced_keyboard) is intentional to force re-emission of escape sequences after external programs change terminal state (e.g., inside with_mode blocks); reference the method name apply_terminal_state and the flags `@mouse_enabled` and `@enhanced_keyboard` in the comment so future readers understand why the code deliberately flips the state rather than simply calling the enable/disable methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/termisu/terminal_spec.cr`:
- Around line 551-609: The examples under describe "#move_cursor" share a single
CaptureTerminal instance causing order-dependent, flaky tests; fix by making
each example use an isolated terminal (e.g., replace the shared `terminal =
CaptureTerminal.new` with per-example setup such as initializing `terminal`
inside a before_each hook or creating a local `terminal = CaptureTerminal.new`
at the start of each `it`), and ensure tests reference the same symbols
(`move_cursor`, `terminal.size`, `terminal.cursor`) so state changes from one
example cannot affect others.
In `@src/termisu/log.cr`:
- Around line 44-50: The change removed the previously exported logger names
(Termisu::Log and Termisu::Logs::*) and only defines LOG, which will break
downstream callers; restore backward compatibility by adding aliases/shims that
map the old public symbols to the new LOG-based instance (e.g., reintroduce
Termisu::Log and the Termisu::Logs module constants as references to LOG or
wrappers delegating to LOG) so both LOG and the legacy names resolve, and
include a TODO comment marking them deprecated for removal in the next major
release.
In `@src/termisu/terminal/cursor.cr`:
- Around line 106-115: The write method can divide/mod by zero when size returns
(0,0); guard against zero dimensions by reading size into width and height, and
if width == 0 or height == 0 either return early or use a safe nonzero
width/height (e.g., treat width as 1 for modulo/division and set/keep `@cursor.y`
within valid range) before computing x = `@cursor.x` + columns_advanced, `@cursor.x`
= x % safe_width and `@cursor.y` = (`@cursor.y` + x // safe_width). Ensure you
reference the write method, the size call, `@cursor.x/`@cursor.y, width/height and
columns_advanced when making the change.
---
Duplicate comments:
In `@spec/termisu/terminal_spec.cr`:
- Around line 529-548: The specs create real Termisu::Terminal instances
(Termisu::Terminal.new) in the "#hide_cursor" and "#show_cursor" examples but
never close them; update each example to ensure the terminal is closed in an
ensure block (e.g., call terminal.close in ensure) so file descriptors and
terminal mode are always cleaned up even on failure; if the Terminal API exposes
a mode restore (e.g., restore_mode or similar), call that in ensure as well to
guarantee mode restoration.
---
Nitpick comments:
In `@src/termisu/event/source/system_timer.cr`:
- Line 35: The logger namespace in the LOG constant uses mixed casing
("termisu.event.systemTimer"); update the LOG initialization (LOG =
::Log.for(...)) to use a normalized, lowercase key such as
"termisu.event.system_timer" so it matches the project's component-level logging
conventions and enables consistent filtering.
In `@src/termisu/terminal.cr`:
- Around line 671-687: Add a brief explanatory comment to the
apply_terminal_state method clarifying that the flag-flip then call pattern
(toggling `@mouse_enabled` and `@enhanced_keyboard` before calling
enable_mouse/disable_mouse and
enable_enhanced_keyboard/disable_enhanced_keyboard) is intentional to force
re-emission of escape sequences after external programs change terminal state
(e.g., inside with_mode blocks); reference the method name apply_terminal_state
and the flags `@mouse_enabled` and `@enhanced_keyboard` in the comment so future
readers understand why the code deliberately flips the state rather than simply
calling the enable/disable methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 430b8e40-bc69-4669-85dd-50b5d28a23dd
📒 Files selected for processing (31)
examples/keyboard_and_mouse.crspec/support/capture_renderer.crspec/support/capture_terminal.crspec/support/mock_renderer.crspec/termisu/buffer_spec.crspec/termisu/cursor_spec.crspec/termisu/render_state_spec.crspec/termisu/terminal/cursor_spec.crspec/termisu/terminal_caching_spec.crspec/termisu/terminal_spec.crsrc/termisu.crsrc/termisu/buffer.crsrc/termisu/cell.crsrc/termisu/cursor.crsrc/termisu/event/loop.crsrc/termisu/event/poller/kqueue.crsrc/termisu/event/poller/linux.crsrc/termisu/event/poller/poll.crsrc/termisu/event/source/input.crsrc/termisu/event/source/resize.crsrc/termisu/event/source/system_timer.crsrc/termisu/event/source/timer.crsrc/termisu/input/parser.crsrc/termisu/log.crsrc/termisu/reader.crsrc/termisu/render_state.crsrc/termisu/renderer.crsrc/termisu/terminal.crsrc/termisu/terminal/cursor.crsrc/termisu/terminfo.crsrc/termisu/terminfo/database.cr
💤 Files with no reviewable changes (4)
- spec/termisu/render_state_spec.cr
- src/termisu/cursor.cr
- spec/termisu/buffer_spec.cr
- spec/termisu/cursor_spec.cr
🚧 Files skipped from review as they are similar to previous changes (4)
- spec/support/capture_terminal.cr
- src/termisu/event/source/input.cr
- src/termisu/reader.cr
- spec/termisu/terminal/cursor_spec.cr
Comment on lines
+551
to
+609
| describe "#move_cursor" do | ||
| terminal = CaptureTerminal.new | ||
|
|
||
| it "defaults at 0, 0" do | ||
| terminal.cursor.x.should eq 0 | ||
| terminal.cursor.y.should eq 0 | ||
| end | ||
|
|
||
| it "does not move when no arguments" do | ||
| terminal.move_cursor | ||
| terminal.cursor.x.should eq 0 | ||
| terminal.cursor.y.should eq 0 | ||
| end | ||
|
|
||
| it "moves the cursor to the specified position" do | ||
| terminal.move_cursor(10, 10) | ||
| terminal.cursor.x.should eq 10 | ||
| terminal.cursor.y.should eq 10 | ||
| end | ||
|
|
||
| it "does not move when no arguments" do | ||
| terminal.move_cursor | ||
| terminal.cursor.x.should eq 10 | ||
| terminal.cursor.y.should eq 10 | ||
| end | ||
|
|
||
| it "does not move beyond the terminal's size" do | ||
| terminal.move_cursor(100, 100) | ||
| terminal.cursor.x.should eq 79 | ||
| terminal.cursor.y.should eq 23 | ||
| end | ||
|
|
||
| it "moves the cursor into bounds after resize" do | ||
| terminal.size = {10, 10} | ||
| terminal.move_cursor | ||
| terminal.cursor.x.should eq 9 | ||
| terminal.cursor.y.should eq 9 | ||
| end | ||
|
|
||
| it "does not move the cursor after size increase" do | ||
| terminal.size = {100, 100} | ||
| terminal.move_cursor | ||
| terminal.cursor.x.should eq 9 | ||
| terminal.cursor.y.should eq 9 | ||
| end | ||
|
|
||
| it "moves the cursor to the specified position" do | ||
| terminal.move_cursor(50, 50) | ||
| terminal.cursor.x.should eq 50 | ||
| terminal.cursor.y.should eq 50 | ||
| end | ||
|
|
||
| it "does not move beyond the terminal's size" do | ||
| terminal.size = {10, 10} | ||
| terminal.move_cursor(50, 50) | ||
| terminal.cursor.x.should eq 9 | ||
| terminal.cursor.y.should eq 9 | ||
| end | ||
| end |
There was a problem hiding this comment.
#move_cursor specs are order-dependent due to shared mutable terminal state.
Line 552 defines one shared CaptureTerminal for all it blocks, and multiple examples assert state established by previous examples. This makes the group flaky and non-isolated.
Proposed fix (single isolated scenario with guaranteed cleanup)
describe "#move_cursor" do
- terminal = CaptureTerminal.new
-
- it "defaults at 0, 0" do
- terminal.cursor.x.should eq 0
- terminal.cursor.y.should eq 0
- end
-
- it "does not move when no arguments" do
- terminal.move_cursor
- terminal.cursor.x.should eq 0
- terminal.cursor.y.should eq 0
- end
-
- it "moves the cursor to the specified position" do
- terminal.move_cursor(10, 10)
- terminal.cursor.x.should eq 10
- terminal.cursor.y.should eq 10
- end
-
- it "does not move when no arguments" do
- terminal.move_cursor
- terminal.cursor.x.should eq 10
- terminal.cursor.y.should eq 10
- end
-
- it "does not move beyond the terminal's size" do
- terminal.move_cursor(100, 100)
- terminal.cursor.x.should eq 79
- terminal.cursor.y.should eq 23
- end
-
- it "moves the cursor into bounds after resize" do
- terminal.size = {10, 10}
- terminal.move_cursor
- terminal.cursor.x.should eq 9
- terminal.cursor.y.should eq 9
- end
-
- it "does not move the cursor after size increase" do
- terminal.size = {100, 100}
- terminal.move_cursor
- terminal.cursor.x.should eq 9
- terminal.cursor.y.should eq 9
- end
-
- it "moves the cursor to the specified position" do
- terminal.move_cursor(50, 50)
- terminal.cursor.x.should eq 50
- terminal.cursor.y.should eq 50
- end
-
- it "does not move beyond the terminal's size" do
- terminal.size = {10, 10}
- terminal.move_cursor(50, 50)
- terminal.cursor.x.should eq 9
- terminal.cursor.y.should eq 9
- end
+ it "moves and clamps cursor position across size changes" do
+ terminal = CaptureTerminal.new
+
+ terminal.cursor.x.should eq 0
+ terminal.cursor.y.should eq 0
+
+ terminal.move_cursor
+ terminal.cursor.x.should eq 0
+ terminal.cursor.y.should eq 0
+
+ terminal.move_cursor(10, 10)
+ terminal.cursor.x.should eq 10
+ terminal.cursor.y.should eq 10
+
+ terminal.move_cursor
+ terminal.cursor.x.should eq 10
+ terminal.cursor.y.should eq 10
+
+ terminal.move_cursor(100, 100)
+ terminal.cursor.x.should eq 79
+ terminal.cursor.y.should eq 23
+
+ terminal.size = {10, 10}
+ terminal.move_cursor
+ terminal.cursor.x.should eq 9
+ terminal.cursor.y.should eq 9
+
+ terminal.size = {100, 100}
+ terminal.move_cursor
+ terminal.cursor.x.should eq 9
+ terminal.cursor.y.should eq 9
+
+ terminal.move_cursor(50, 50)
+ terminal.cursor.x.should eq 50
+ terminal.cursor.y.should eq 50
+
+ terminal.size = {10, 10}
+ terminal.move_cursor(50, 50)
+ terminal.cursor.x.should eq 9
+ terminal.cursor.y.should eq 9
+ ensure
+ terminal.try &.close
+ end
end📝 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.
Suggested change
| describe "#move_cursor" do | |
| terminal = CaptureTerminal.new | |
| it "defaults at 0, 0" do | |
| terminal.cursor.x.should eq 0 | |
| terminal.cursor.y.should eq 0 | |
| end | |
| it "does not move when no arguments" do | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 0 | |
| terminal.cursor.y.should eq 0 | |
| end | |
| it "moves the cursor to the specified position" do | |
| terminal.move_cursor(10, 10) | |
| terminal.cursor.x.should eq 10 | |
| terminal.cursor.y.should eq 10 | |
| end | |
| it "does not move when no arguments" do | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 10 | |
| terminal.cursor.y.should eq 10 | |
| end | |
| it "does not move beyond the terminal's size" do | |
| terminal.move_cursor(100, 100) | |
| terminal.cursor.x.should eq 79 | |
| terminal.cursor.y.should eq 23 | |
| end | |
| it "moves the cursor into bounds after resize" do | |
| terminal.size = {10, 10} | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| end | |
| it "does not move the cursor after size increase" do | |
| terminal.size = {100, 100} | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| end | |
| it "moves the cursor to the specified position" do | |
| terminal.move_cursor(50, 50) | |
| terminal.cursor.x.should eq 50 | |
| terminal.cursor.y.should eq 50 | |
| end | |
| it "does not move beyond the terminal's size" do | |
| terminal.size = {10, 10} | |
| terminal.move_cursor(50, 50) | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| end | |
| end | |
| describe "#move_cursor" do | |
| it "moves and clamps cursor position across size changes" do | |
| terminal = CaptureTerminal.new | |
| terminal.cursor.x.should eq 0 | |
| terminal.cursor.y.should eq 0 | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 0 | |
| terminal.cursor.y.should eq 0 | |
| terminal.move_cursor(10, 10) | |
| terminal.cursor.x.should eq 10 | |
| terminal.cursor.y.should eq 10 | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 10 | |
| terminal.cursor.y.should eq 10 | |
| terminal.move_cursor(100, 100) | |
| terminal.cursor.x.should eq 79 | |
| terminal.cursor.y.should eq 23 | |
| terminal.size = {10, 10} | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| terminal.size = {100, 100} | |
| terminal.move_cursor | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| terminal.move_cursor(50, 50) | |
| terminal.cursor.x.should eq 50 | |
| terminal.cursor.y.should eq 50 | |
| terminal.size = {10, 10} | |
| terminal.move_cursor(50, 50) | |
| terminal.cursor.x.should eq 9 | |
| terminal.cursor.y.should eq 9 | |
| ensure | |
| terminal.try &.close | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/termisu/terminal_spec.cr` around lines 551 - 609, The examples under
describe "#move_cursor" share a single CaptureTerminal instance causing
order-dependent, flaky tests; fix by making each example use an isolated
terminal (e.g., replace the shared `terminal = CaptureTerminal.new` with
per-example setup such as initializing `terminal` inside a before_each hook or
creating a local `terminal = CaptureTerminal.new` at the start of each `it`),
and ensure tests reference the same symbols (`move_cursor`, `terminal.size`,
`terminal.cursor`) so state changes from one example cannot affect others.
src/termisu/log.cr
Outdated
Comment on lines
44
to
50
| # Termisu::LOG.debug { "Input byte: #{byte}" } | ||
| # Termisu::Terminal::LOG.trace { "Rendering cell at #{x},#{y}" } | ||
| # ``` | ||
|
|
||
| # Main log instance for Termisu library | ||
| Log = ::Log.for("termisu") | ||
| LOG = ::Log.for("termisu") | ||
|
|
There was a problem hiding this comment.
This renames a documented public logging surface without a compatibility path.
LOG is now the only public constant exposed here, and the PR removes the previous Termisu::Log / Termisu::Logs::* names. That will compile-break downstream callers that followed the old logging API. Unless the next release is intentionally breaking, please keep aliases/shims for one cycle or call the migration out explicitly before merging.
If you want, I can draft the minimal compatibility layer for the old logger constants and module names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/termisu/log.cr` around lines 44 - 50, The change removed the previously
exported logger names (Termisu::Log and Termisu::Logs::*) and only defines LOG,
which will break downstream callers; restore backward compatibility by adding
aliases/shims that map the old public symbols to the new LOG-based instance
(e.g., reintroduce Termisu::Log and the Termisu::Logs module constants as
references to LOG or wrappers delegating to LOG) so both LOG and the legacy
names resolve, and include a TODO comment marking them deprecated for removal in
the next major release.
Comment on lines
+106
to
+115
| def write(data : String, columns_advanced = 0) | ||
| @backend.write(data) | ||
|
|
||
| width, height = size | ||
|
|
||
| x = @cursor.x + columns_advanced | ||
|
|
||
| @cursor.x = x % width | ||
| @cursor.y = (@cursor.y + x // width).clamp(0, height - 1) | ||
| end |
There was a problem hiding this comment.
Critical: Division by zero when terminal size is zero.
The pipeline failures indicate DivisionByZeroError at lines 113-114. When size returns (0, 0) (possible during initialization or in test environments), the modulo and integer division operations crash.
🐛 Proposed fix to guard against zero dimensions
def write(data : String, columns_advanced = 0)
`@backend.write`(data)
width, height = size
+ return if width == 0 || height == 0
x = `@cursor.x` + columns_advanced
`@cursor.x` = x % width
`@cursor.y` = (`@cursor.y` + x // width).clamp(0, height - 1)
end🧰 Tools
🪛 GitHub Actions: Tests
[error] 113-113: Division by 0 (DivisionByZeroError) encountered during test execution in write path.
[error] 106-106: Division by 0 (DivisionByZeroError) encountered during test execution in write path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/termisu/terminal/cursor.cr` around lines 106 - 115, The write method can
divide/mod by zero when size returns (0,0); guard against zero dimensions by
reading size into width and height, and if width == 0 or height == 0 either
return early or use a safe nonzero width/height (e.g., treat width as 1 for
modulo/division and set/keep `@cursor.y` within valid range) before computing x =
`@cursor.x` + columns_advanced, `@cursor.x` = x % safe_width and `@cursor.y` =
(`@cursor.y` + x // safe_width). Ensure you reference the write method, the size
call, `@cursor.x/`@cursor.y, width/height and columns_advanced when making the
change.
Owner
|
@wuestenfee this looks great tysm! |
Contributor
Author
|
Thank you! |
Cursor operations were previously spread across Terminal, Buffer, its Cursor, and RenderState with several delegations. This change consolidates the logic and simplifies the call flow.
Owner
@wuestenfee Im open to simplifying logging but I think it should be in its own pr, that way this pr complexity can go down, if we can get that done and get the specs passing I would happy to merge this and cut a new release ❤️ P.S you dont have to open a second pr just reverting the logging stuff is enough |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/termisu/terminal/cursor.cr (1)
106-115:⚠️ Potential issue | 🔴 CriticalGuard against zero terminal dimensions in write method.
When
sizereturns(0, 0)— possible during initialization or on non-standard terminals — the modulo and division operations at lines 113–114 will crash with division by zero.Add a guard at the method start:
Proposed fix
def write(data : String, columns_advanced = 0) `@backend.write`(data) width, height = size + return if width == 0 || height == 0 x = `@cursor.x` + columns_advanced `@cursor.x` = x % width `@cursor.y` = (`@cursor.y` + x // width).clamp(0, height - 1) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal/cursor.cr` around lines 106 - 115, After calling `@backend.write`(data) and obtaining width,height = size in write, guard against zero dimensions to avoid division/modulo by zero: if width == 0 or height == 0, do not perform the x % width or x // width updates and simply return early (leaving `@cursor` unchanged). Update the write method (referencing write, size, `@backend.write`, `@cursor.x` and `@cursor.y`) to check for zero width/height immediately before computing x % width and x // width and return when either dimension is zero.
🧹 Nitpick comments (4)
src/termisu/render_state.cr (1)
1-5: Documentation mentions cursor position but cursor tracking was removed.The struct doc comment on line 4 still says "what colors, attributes, and cursor position are currently set" but cursor tracking has been removed from RenderState.
📝 Suggested doc update
# Tracks the current terminal rendering state for optimization. # # Used to avoid emitting redundant escape sequences by tracking -# what colors, attributes, and cursor position are currently set. +# what colors and attributes are currently set. # Only emits escape sequences when the state actually changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/render_state.cr` around lines 1 - 5, The RenderState struct's doc comment still mentions "cursor position" even though cursor tracking was removed; update the documentation on the RenderState struct to reflect the current responsibilities (e.g., tracking colors and attributes only) and remove or rephrase any reference to cursor position so the comment accurately describes the fields and behavior of RenderState.src/termisu/terminal.cr (1)
671-687: Verify the toggle-then-call pattern in apply_terminal_state.This pattern flips the flag to the opposite value, then calls the enable/disable method (which checks the flag and emits sequences). While clever, the implementation is somewhat indirect:
if `@mouse_enabled` `@mouse_enabled` = false # flip to false enable_mouse # enable_mouse checks "return if `@mouse_enabled`", sets true, emits else `@mouse_enabled` = true # flip to true disable_mouse # disable_mouse checks "return unless `@mouse_enabled`", sets false, emits endThe logic works because:
- If mouse was enabled: flip→false, enable_mouse sees false→emits enable sequence→sets true
- If mouse was disabled: flip→true, disable_mouse sees true→emits disable sequence→sets false
This effectively re-emits the current state's sequences. The pattern is correct but could be clearer with a comment.
📝 Suggested clarifying comment
private def apply_terminal_state + # Re-emit terminal protocol sequences by temporarily inverting state flags. + # This ensures terminal state matches internal state after mode transitions. if `@mouse_enabled` `@mouse_enabled` = false enable_mouse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal.cr` around lines 671 - 687, add a brief clarifying comment above apply_terminal_state explaining the intentional "flip-then-call" pattern: note that the method flips `@mouse_enabled` and `@enhanced_keyboard` before calling enable_mouse/disable_mouse and enable_enhanced_keyboard/disable_enhanced_keyboard because those methods inspect the flag, emit the appropriate control sequences and then set the flag back; explicitly describe the two cases (was enabled → flip to false → call enable_* to re-emit and set true, was disabled → flip to true → call disable_* to re-emit and set false) so future readers understand this non-obvious flow.src/termisu/buffer.cr (1)
7-13: Documentation references cursor tracking but Buffer no longer manages cursor.Lines 7 ("Cursor position and visibility") and 13 ("Tracks cursor position to minimize move_cursor calls") describe cursor management that has been moved to Terminal. The Buffer now only calls
renderer.move_cursor()without tracking cursor state.📝 Suggested doc update
# Buffer manages a 2D grid of cells with double buffering support. # # Buffer maintains: # - Front buffer: What's currently displayed on screen # - Back buffer: Where new content is written # - Diff algorithm: Only redraws cells that have changed -# - Cursor position and visibility # - Render state tracking for escape sequence optimization # # Performance Optimizations: # - Only emits color/attribute escape sequences when they change # - Batches consecutive cells on the same row with the same styling -# - Tracks cursor position to minimize move_cursor calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/buffer.cr` around lines 7 - 13, Update the top-of-file comment in Buffer to remove or revise references that claim Buffer manages cursor position/visibility and tracks cursor state; explicitly state that cursor management is handled by Terminal and that Buffer simply calls renderer.move_cursor() without tracking cursor state. Locate the Buffer class and the header comments containing the phrases "Cursor position and visibility" and "Tracks cursor position to minimize move_cursor calls" and replace them with a brief note that Terminal performs cursor tracking/visibility while Buffer focuses on render-state and escape-sequence optimizations (e.g., batching cells, emitting styles only on change). Ensure the performance optimization lines remain accurate but do not claim cursor management responsibility for Buffer.src/termisu/terminal/cursor.cr (1)
71-89: Verify move_cursor handles zero-dimension edge case.Lines 76-77 clamp x and y to
width - 1andheight - 1. Ifsizereturns(0, 0), these become-1, which is then clamped byclamp(0, ...)to0. The early return at line 79 prevents emitting sequences when nothing changes.However, clamping to
0when width/height is0may still lead to inconsistent state. Consider adding a guard:🛡️ Suggested guard for zero dimensions
def move_cursor( x : Int32 = `@cursor.x`, y : Int32 = `@cursor.y`, ) width, height = size + return if width == 0 || height == 0 + x = x.clamp(0, width - 1) y = y.clamp(0, height - 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termisu/terminal/cursor.cr` around lines 71 - 89, The move_cursor method can produce inconsistent behavior when size returns (0,0); after retrieving width,height = size add a guard: if width == 0 || height == 0 then return immediately to avoid clamping to -1->0 and emitting sequences or updating `@cursor`; keep the early-return semantics (do not write or modify `@cursor`) so move_cursor is a no-op for zero-dimension terminals. Use the existing symbols (move_cursor, size, width, height, `@cursor`) to locate where to add this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/termisu/terminal/cursor.cr`:
- Around line 19-31: The apply_cursor_state toggles visibility and calls
show_cursor/hide_cursor which eventually may call write() and trigger a
division-by-zero; to fix, add a guard at the start of the write method (the
write function that write_cursor may call) that returns early if the terminal
dimensions are zero (e.g., if `@size.width` == 0 or `@size.height` == 0 or
equivalent), preventing any downstream division or layout calculations, so
show_cursor/hide_cursor and write_cursor can safely be invoked from
apply_cursor_state and move_cursor without causing exceptions.
---
Duplicate comments:
In `@src/termisu/terminal/cursor.cr`:
- Around line 106-115: After calling `@backend.write`(data) and obtaining
width,height = size in write, guard against zero dimensions to avoid
division/modulo by zero: if width == 0 or height == 0, do not perform the x %
width or x // width updates and simply return early (leaving `@cursor` unchanged).
Update the write method (referencing write, size, `@backend.write`, `@cursor.x` and
`@cursor.y`) to check for zero width/height immediately before computing x % width
and x // width and return when either dimension is zero.
---
Nitpick comments:
In `@src/termisu/buffer.cr`:
- Around line 7-13: Update the top-of-file comment in Buffer to remove or revise
references that claim Buffer manages cursor position/visibility and tracks
cursor state; explicitly state that cursor management is handled by Terminal and
that Buffer simply calls renderer.move_cursor() without tracking cursor state.
Locate the Buffer class and the header comments containing the phrases "Cursor
position and visibility" and "Tracks cursor position to minimize move_cursor
calls" and replace them with a brief note that Terminal performs cursor
tracking/visibility while Buffer focuses on render-state and escape-sequence
optimizations (e.g., batching cells, emitting styles only on change). Ensure the
performance optimization lines remain accurate but do not claim cursor
management responsibility for Buffer.
In `@src/termisu/render_state.cr`:
- Around line 1-5: The RenderState struct's doc comment still mentions "cursor
position" even though cursor tracking was removed; update the documentation on
the RenderState struct to reflect the current responsibilities (e.g., tracking
colors and attributes only) and remove or rephrase any reference to cursor
position so the comment accurately describes the fields and behavior of
RenderState.
In `@src/termisu/terminal.cr`:
- Around line 671-687: add a brief clarifying comment above apply_terminal_state
explaining the intentional "flip-then-call" pattern: note that the method flips
`@mouse_enabled` and `@enhanced_keyboard` before calling enable_mouse/disable_mouse
and enable_enhanced_keyboard/disable_enhanced_keyboard because those methods
inspect the flag, emit the appropriate control sequences and then set the flag
back; explicitly describe the two cases (was enabled → flip to false → call
enable_* to re-emit and set true, was disabled → flip to true → call disable_*
to re-emit and set false) so future readers understand this non-obvious flow.
In `@src/termisu/terminal/cursor.cr`:
- Around line 71-89: The move_cursor method can produce inconsistent behavior
when size returns (0,0); after retrieving width,height = size add a guard: if
width == 0 || height == 0 then return immediately to avoid clamping to -1->0 and
emitting sequences or updating `@cursor`; keep the early-return semantics (do not
write or modify `@cursor`) so move_cursor is a no-op for zero-dimension terminals.
Use the existing symbols (move_cursor, size, width, height, `@cursor`) to locate
where to add this guard.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32077b4d-d20f-46be-a8f1-704e91a8d8ec
📒 Files selected for processing (18)
examples/keyboard_and_mouse.crspec/support/capture_renderer.crspec/support/capture_terminal.crspec/support/mock_renderer.crspec/termisu/buffer_spec.crspec/termisu/cursor_spec.crspec/termisu/render_state_spec.crspec/termisu/terminal/cursor_spec.crspec/termisu/terminal_caching_spec.crspec/termisu/terminal_spec.crsrc/termisu.crsrc/termisu/buffer.crsrc/termisu/cursor.crsrc/termisu/render_state.crsrc/termisu/renderer.crsrc/termisu/terminal.crsrc/termisu/terminal/cursor.crsrc/termisu/terminfo.cr
💤 Files with no reviewable changes (4)
- spec/termisu/render_state_spec.cr
- spec/termisu/buffer_spec.cr
- spec/termisu/cursor_spec.cr
- src/termisu/cursor.cr
✅ Files skipped from review due to trivial changes (2)
- spec/termisu/terminal/cursor_spec.cr
- spec/support/capture_renderer.cr
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/support/capture_terminal.cr
- src/termisu/renderer.cr
- examples/keyboard_and_mouse.cr
- spec/support/mock_renderer.cr
- spec/termisu/terminal_spec.cr
Comment on lines
+19
to
+31
| private def apply_cursor_state | ||
| x, y = @cursor.x, @cursor.y | ||
| @cursor.x, @cursor.y = -1, -1 | ||
| move_cursor(x, y) | ||
|
|
||
| if @cursor.visible? | ||
| @cursor.visible = false | ||
| show_cursor | ||
| else | ||
| @cursor.visible = true | ||
| hide_cursor | ||
| end | ||
| end |
There was a problem hiding this comment.
The apply_cursor_state toggle pattern may trigger division by zero.
The apply_cursor_state method calls move_cursor(x, y) at line 22, and the visibility toggle at lines 24-30 calls show_cursor or hide_cursor. The show_cursor method calls write_cursor which doesn't directly use size, but if the visibility methods internally trigger any write operations, they could cascade to the division-by-zero issue in write().
The pipeline shows errors at line 35 (hide_cursor) propagating from the write path. Adding the zero-dimension guard in write() should resolve this cascade.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/termisu/terminal/cursor.cr` around lines 19 - 31, The apply_cursor_state
toggles visibility and calls show_cursor/hide_cursor which eventually may call
write() and trigger a division-by-zero; to fix, add a guard at the start of the
write method (the write function that write_cursor may call) that returns early
if the terminal dimensions are zero (e.g., if `@size.width` == 0 or `@size.height`
== 0 or equivalent), preventing any downstream division or layout calculations,
so show_cursor/hide_cursor and write_cursor can safely be invoked from
apply_cursor_state and move_cursor without causing exceptions.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Cannot open a stacked PR on a forked repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a few terminal capability improvements and simplifies how cursor state is handled internally.
Features
Refactor
Cursor-related logic was previously spread across
Terminal,Buffer,Cursor, andRenderState, with several delegations between them.This PR centralizes cursor operations, which simplifies the call flow and makes cursor state easier to reason about.
Improvements
Terminal#with_modeLogsmoduleTested in: