Fix: [BUG] [v1.1.0] Auxiliary activity bar: border side frozen when position changes at runtime#39613
Conversation
…n when position changes at runtime Signed-off-by: willkhinz <hinzwilliam52@gmail.com>
…n when position changes at runtime Signed-off-by: willkhinz <hinzwilliam52@gmail.com>
…n when position changes at runtime Signed-off-by: willkhinz <hinzwilliam52@gmail.com>
📝 WalkthroughWalkthroughThe changes introduce a position tracking system (left/right) across multiple TUI modules, allowing users to toggle between positions via keyboard input ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
bins/bounty-cli/src/tui/leaderboard.rs (1)
20-25: Consider using the sharedAuxiliaryActivityBartype or an enum for position.The
mod.rsfile introducesAuxiliaryActivityBarwith position management, but this file maintains its ownposition: Stringfield. This creates:
- Duplicate position-handling logic across modules
- Risk of typos with string-based position values (e.g.,
"Left"vs"left")Consider either:
- Reusing
AuxiliaryActivityBarfrommod.rs- Defining a shared
Positionenum for type safety🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bins/bounty-cli/src/tui/leaderboard.rs` around lines 20 - 25, The App struct currently maintains a string-typed position field which duplicates position logic from AuxiliaryActivityBar in mod.rs and risks typos; replace App::position: String by reusing the shared AuxiliaryActivityBar (or a shared Position enum) to centralize position state: locate the App struct and any code that reads/writes position and change it to hold either an AuxiliaryActivityBar instance or the shared Position enum, update usages in rendering/scroll logic to call the AuxiliaryActivityBar's position accessors or match on Position instead of string comparisons, and remove any string-based position handling to ensure type-safe, single-source position management.bins/bounty-cli/src/tui/mod.rs (1)
42-50: Identical style returned for both positions.Both
"right"and"left"branches return the same style (fg(White).bg(Black).add_modifier(BOLD)), making the conditional logic redundant. Either simplify to a single style or differentiate as intended.♻️ Suggested simplification
pub fn get_activity_bar_style(&self) -> Style { - let mut style = Style::default(); - if self.position == "right" { - style = style.fg(Color::White).bg(Color::Black).add_modifier(Modifier::BOLD); - } else if self.position == "left" { - style = style.fg(Color::White).bg(Color::Black).add_modifier(Modifier::BOLD); - } - style + Style::default() + .fg(Color::White) + .bg(Color::Black) + .add_modifier(Modifier::BOLD) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bins/bounty-cli/src/tui/mod.rs` around lines 42 - 50, The get_activity_bar_style method currently returns an identical Style for both branches of the if/else on self.position, making the conditional redundant; either collapse the branches and return a single common Style (e.g., create and return Style::default().fg(Color::White).bg(Color::Black).add_modifier(Modifier::BOLD) directly) or implement the intended differentiation by returning distinct Styles for the "left" and "right" cases (update get_activity_bar_style to match on self.position and return the appropriate Style for each branch, referencing the get_activity_bar_style method, the position field, and the Style/Color/Modifier types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bins/bounty-cli/src/tui/leaderboard.rs`:
- Around line 191-194: The function in leaderboard.rs exits without restoring
the terminal; ensure you call super::restore_terminal(&mut terminal)? before any
return paths (including the early exits on 'q' or 'Esc') and immediately before
the final Ok(()) return so raw mode and the alternate screen are cleaned up;
locate uses of the local variable terminal and add the call to
super::restore_terminal(&mut terminal)? in those exit branches and prior to the
final return.
In `@bins/bounty-cli/src/tui/mod.rs`:
- Around line 57-59: Replace the deprecated call to Frame::size() with
Frame::area() in the layout creation: locate the code that builds the Layout
using Layout::default().constraints([...]).split(f.size()) and change
split(f.size()) to split(f.area()), ensuring the use of the same frame variable
(f) as in other files (e.g., leaderboard.rs) and keeping Layout, Constraint and
split unchanged.
- Around line 71-77: The code incorrectly applies the `?` operator to
`Frame::render_widget`, which returns `()` (not a `Result`); remove the `?` from
the `f.render_widget(...)` call (in the block that builds the
`Paragraph::new(activity_bar_text).style(activity_bar_style).block(Block::default().borders(border_style))`)
and ensure the surrounding function still returns `Ok(())` (or propagates an
actual `Result`) as appropriate.
In `@bins/bounty-cli/src/tui/stats.rs`:
- Line 141: The default value for the TUI position in this module is
inconsistent: change the variable named position (currently set via let mut
position = "left" in stats.rs) to use the same default as leaderboard.rs (i.e.,
"right") so all TUI screens share the same default placement; update the
initialization of position to "right" and run a quick build/test to ensure no
other logic relies on the old default.
- Around line 120-134: The border Block is being drawn last (using
border_style/borders and frame.render_widget with frame.area()), which causes it
to overlay and potentially overwrite the title, stats grid, and help widgets in
ui(); move the border rendering to the start of ui() so the border is drawn
first, or alternatively compute a dedicated side region and render the border
Block into that region instead of frame.area() (adjust the code around the
borders variable, border_style, and the Block::default() call to use the new
region).
---
Nitpick comments:
In `@bins/bounty-cli/src/tui/leaderboard.rs`:
- Around line 20-25: The App struct currently maintains a string-typed position
field which duplicates position logic from AuxiliaryActivityBar in mod.rs and
risks typos; replace App::position: String by reusing the shared
AuxiliaryActivityBar (or a shared Position enum) to centralize position state:
locate the App struct and any code that reads/writes position and change it to
hold either an AuxiliaryActivityBar instance or the shared Position enum, update
usages in rendering/scroll logic to call the AuxiliaryActivityBar's position
accessors or match on Position instead of string comparisons, and remove any
string-based position handling to ensure type-safe, single-source position
management.
In `@bins/bounty-cli/src/tui/mod.rs`:
- Around line 42-50: The get_activity_bar_style method currently returns an
identical Style for both branches of the if/else on self.position, making the
conditional redundant; either collapse the branches and return a single common
Style (e.g., create and return
Style::default().fg(Color::White).bg(Color::Black).add_modifier(Modifier::BOLD)
directly) or implement the intended differentiation by returning distinct Styles
for the "left" and "right" cases (update get_activity_bar_style to match on
self.position and return the appropriate Style for each branch, referencing the
get_activity_bar_style method, the position field, and the Style/Color/Modifier
types).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1275c39-0c2f-4e1c-8682-5cf529f1f023
📒 Files selected for processing (3)
bins/bounty-cli/src/tui/leaderboard.rsbins/bounty-cli/src/tui/mod.rsbins/bounty-cli/src/tui/stats.rs
| } | ||
|
|
||
| super::restore_terminal(&mut terminal)?; | ||
| Ok(()) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical: Missing restore_terminal call leaves terminal in corrupted state.
When the user exits the leaderboard (via q or Esc), the function returns without restoring the terminal. This leaves raw mode enabled and the alternate screen active, corrupting the user's terminal session.
The stats.rs module correctly calls super::restore_terminal(&mut terminal)? before returning (see line 174 in stats.rs). This file should follow the same pattern.
🐛 Proposed fix to restore terminal before returning
}
+ super::restore_terminal(&mut terminal)?;
Ok(())
}📝 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.
| } | |
| super::restore_terminal(&mut terminal)?; | |
| Ok(()) | |
| } | |
| } | |
| } | |
| super::restore_terminal(&mut terminal)?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bins/bounty-cli/src/tui/leaderboard.rs` around lines 191 - 194, The function
in leaderboard.rs exits without restoring the terminal; ensure you call
super::restore_terminal(&mut terminal)? before any return paths (including the
early exits on 'q' or 'Esc') and immediately before the final Ok(()) return so
raw mode and the alternate screen are cleaned up; locate uses of the local
variable terminal and add the call to super::restore_terminal(&mut terminal)? in
those exit branches and prior to the final return.
| let chunks = Layout::default() | ||
| .constraints([Constraint::Percentage(100)].as_ref()) | ||
| .split(f.size()); |
There was a problem hiding this comment.
Use f.area() instead of deprecated f.size().
In Ratatui 0.29.0, Frame::size() is deprecated in favor of Frame::area(). Other files in this PR (e.g., leaderboard.rs line 67) correctly use frame.area().
🔧 Proposed fix
let chunks = Layout::default()
.constraints([Constraint::Percentage(100)].as_ref())
- .split(f.size());
+ .split(f.area());📝 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.
| let chunks = Layout::default() | |
| .constraints([Constraint::Percentage(100)].as_ref()) | |
| .split(f.size()); | |
| let chunks = Layout::default() | |
| .constraints([Constraint::Percentage(100)].as_ref()) | |
| .split(f.area()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bins/bounty-cli/src/tui/mod.rs` around lines 57 - 59, Replace the deprecated
call to Frame::size() with Frame::area() in the layout creation: locate the code
that builds the Layout using
Layout::default().constraints([...]).split(f.size()) and change split(f.size())
to split(f.area()), ensuring the use of the same frame variable (f) as in other
files (e.g., leaderboard.rs) and keeping Layout, Constraint and split unchanged.
| f.render_widget( | ||
| Paragraph::new(activity_bar_text) | ||
| .style(activity_bar_style) | ||
| .block(Block::default().borders(border_style)), | ||
| chunks[0], | ||
| )?; | ||
| Ok(()) |
There was a problem hiding this comment.
Compilation error: render_widget returns (), not Result.
Frame::render_widget() in Ratatui returns (), so the ? operator cannot be applied. This code will fail to compile.
🐛 Proposed fix
f.render_widget(
Paragraph::new(activity_bar_text)
.style(activity_bar_style)
.block(Block::default().borders(border_style)),
chunks[0],
- )?;
+ );
Ok(())
}What is the return type of Frame::render_widget in ratatui 0.29?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bins/bounty-cli/src/tui/mod.rs` around lines 71 - 77, The code incorrectly
applies the `?` operator to `Frame::render_widget`, which returns `()` (not a
`Result`); remove the `?` from the `f.render_widget(...)` call (in the block
that builds the
`Paragraph::new(activity_bar_text).style(activity_bar_style).block(Block::default().borders(border_style))`)
and ensure the surrounding function still returns `Ok(())` (or propagates an
actual `Result`) as appropriate.
|
|
||
| // Update the border based on the position | ||
| let border_style = Style::default().fg(Color::Cyan); | ||
| let borders = match position { | ||
| "left" => Borders::LEFT, | ||
| "right" => Borders::RIGHT, | ||
| _ => Borders::NONE, | ||
| }; | ||
| frame.render_widget( | ||
| Block::default() | ||
| .borders(borders) | ||
| .border_style(border_style), | ||
| frame.area(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Border overlay renders on top of all content, potentially obscuring widgets.
The border Block is rendered at frame.area() (full frame) after the title, stats grid, and help widgets. In Ratatui, later render_widget calls draw over earlier ones. Depending on the terminal and border character rendering, this may partially overwrite the existing content.
Consider rendering the border first (before other widgets) or rendering it in a dedicated side region rather than the full frame.
🔧 Suggested approach: Render border first
Move the border rendering to the beginning of ui():
fn ui(frame: &mut Frame, stats: &StatsData, error: &Option<String>, position: &str) {
+ // Render position-based border first (background layer)
+ let border_style = Style::default().fg(Color::Cyan);
+ let borders = match position {
+ "left" => Borders::LEFT,
+ "right" => Borders::RIGHT,
+ _ => Borders::NONE,
+ };
+ frame.render_widget(
+ Block::default()
+ .borders(borders)
+ .border_style(border_style),
+ frame.area(),
+ );
+
let outer = Layout::default()
.direction(Direction::Vertical)
// ... rest of function
-
- // Update the border based on the position
- let border_style = Style::default().fg(Color::Cyan);
- let borders = match position {
- "left" => Borders::LEFT,
- "right" => Borders::RIGHT,
- _ => Borders::NONE,
- };
- frame.render_widget(
- Block::default()
- .borders(borders)
- .border_style(border_style),
- frame.area(),
- );
}📝 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.
| // Update the border based on the position | |
| let border_style = Style::default().fg(Color::Cyan); | |
| let borders = match position { | |
| "left" => Borders::LEFT, | |
| "right" => Borders::RIGHT, | |
| _ => Borders::NONE, | |
| }; | |
| frame.render_widget( | |
| Block::default() | |
| .borders(borders) | |
| .border_style(border_style), | |
| frame.area(), | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bins/bounty-cli/src/tui/stats.rs` around lines 120 - 134, The border Block is
being drawn last (using border_style/borders and frame.render_widget with
frame.area()), which causes it to overlay and potentially overwrite the title,
stats grid, and help widgets in ui(); move the border rendering to the start of
ui() so the border is drawn first, or alternatively compute a dedicated side
region and render the border Block into that region instead of frame.area()
(adjust the code around the borders variable, border_style, and the
Block::default() call to use the new region).
| let mut stats = StatsData::default(); | ||
| let mut error: Option<String> = None; | ||
| let mut last_fetch = Instant::now() - Duration::from_secs(10); | ||
| let mut position = "left"; // default position |
There was a problem hiding this comment.
Inconsistent default position across modules.
This module defaults to "left" while leaderboard.rs defaults to "right". For a consistent user experience, consider aligning the default position across all TUI screens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bins/bounty-cli/src/tui/stats.rs` at line 141, The default value for the TUI
position in this module is inconsistent: change the variable named position
(currently set via let mut position = "left" in stats.rs) to use the same
default as leaderboard.rs (i.e., "right") so all TUI screens share the same
default placement; update the initialization of position to "right" and run a
quick build/test to ensure no other logic relies on the old default.
1 participant
Description
This PR fixes the issue with the auxiliary activity bar's border side becoming frozen when its position changes at runtime. The changes include updates to the event handling logic and the use of
crosstermandratatuilibraries to handle terminal events and render the UI correctly.Related Issue
Fixes #https://github.com/PlatformNetwork/bounty-challenge
Type of Change
Checklist
Testing
To verify the changes, the following commands were run:
cargo test cargo clippyAdditionally, manual testing was performed by running the application and simulating position change events at runtime.
Screenshots (if applicable)
No screenshots are necessary for this change, as it is a bug fix that affects the internal logic of the application rather than its visual appearance.
🔍 Analysis
The issue arises from the auxiliary activity bar's border side becoming frozen when its position changes at runtime. This suggests a problem with the handling of the bar's layout or redraw logic when the position is updated.
🛠️ Implementation
To address this issue, the following changes were made:
Appstruct to include apositionfield to track the current position of the auxiliary activity bar.positionfield when a position change event is received.crosstermandratatuilibraries to handle terminal events and render the UI correctly.anyhow::Resultto ensure robustness.✅ Verification
To verify the fix, the following steps were taken:
Summary by CodeRabbit