refactor: replace TabBar with TamboUI Tabs widget and add scrollbars#93
Open
gnodet wants to merge 3 commits into
Open
refactor: replace TabBar with TamboUI Tabs widget and add scrollbars#93gnodet wants to merge 3 commits into
gnodet wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors TUI rendering: replaces TabBar-based tabs with theme.inlineTabIndicators/Tabs, centralizes tab hit-testing in ToolPanel, and adds scrollbar-aware table rendering via ToolPanel.renderTableWithScrollbar; updates highlight symbol usage and several expand/collapse glyphs across multiple TUI classes. ChangesUI Rendering Infrastructure Modernization
Sequence DiagramsequenceDiagram
participant TUI as TUI (e.g., TreeTui)
participant ToolPanel as ToolPanel
participant Frame as Frame
participant Table as Table Widget
participant Scrollbar as Scrollbar Widget
TUI->>ToolPanel: renderTableWithScrollbar(frame, area, table, state, totalRows)
ToolPanel->>ToolPanel: compute viewport and highlight width
ToolPanel->>Table: decide render mode (full vs. cropped)
alt fits in viewport
ToolPanel->>Frame: render Table in full area
else overflows
ToolPanel->>Scrollbar: create ScrollbarState (content, viewport, pos)
ToolPanel->>Frame: render cropped Table area
ToolPanel->>Frame: render Scrollbar at right edge
end
ToolPanel-->>TUI: render complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pilot-core/src/main/java/eu/maveniverse/maven/pilot/ModuleTreePane.java (1)
236-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject clicks in the scrollbar gutter.
handleMouseClick()still converts any click with a matchingyinto a row selection. Now thatrenderTable()can reserve the last column for the scrollbar, clicking that gutter will still select a row and can toggle expansion on whichever node shares the samey.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/ModuleTreePane.java` around lines 236 - 249, handleMouseClick currently treats clicks in the scrollbar gutter as row clicks; update handleMouseClick to first detect and ignore clicks in the reserved scrollbar column by checking mouse.x() against the right edge of the area (e.g., if mouse.x() >= area.x() + area.width() - 1) and return false in that case so no row selection or node.expanded toggle occurs; keep the existing row calculation, visibleNodes(), tableState.select(row), and arrowX/expansion logic otherwise.pilot-core/src/main/java/eu/maveniverse/maven/pilot/TreeTui.java (1)
186-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe mouse Y offset should be
-1, not-2.The tree table is built without a
.block()at line 546 and callssetTableArea(zones.get(0), null). With no block wrapper, the formula should matchToolPanel.mouseToTableRow()(lines 583–591):mouse.y() - area.y() - 1 + offset. The current-2causes rows to be calculated one line too high. The comment "header + highlight + scroll" is also misleading—only the header row is subtracted; the highlight symbol width is handled separately during column layout, and scroll offset is added independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/TreeTui.java` around lines 186 - 200, In handleTreeMouseClick(MouseEvent mouse, Rect area) the row calculation uses an incorrect vertical offset (-2) causing row selection to be one line too high; change the formula in TreeTui.handleTreeMouseClick from "mouse.y() - area.y() - 2 + tableState.offset()" to use -1 to match ToolPanel.mouseToTableRow, and update the inline comment to state only the header is subtracted (remove "highlight" from the comment) so the calculation reads "mouse.y() - area.y() - 1 + tableState.offset()".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/AuditTui.java`:
- Around line 678-680: Replace the direct assignment of the view in the key
handler with a call to the existing setActiveSubView(int) method so the reset
logic runs; specifically, in AuditTui where you detect TAB (the block with
key.isKey(KeyCode.TAB)), compute the next index as (view.ordinal() + 1) %
View.values().length and call setActiveSubView(nextIndex) instead of assigning
view = View.values()[...]; keep the return true and remove the direct assignment
so search state and SortState are properly reset when switching to
vulnerabilities.
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/Theme.java`:
- Around line 70-71: The method Theme.inlineTabIndicators currently uses a
fully-qualified type for the activeColorFn parameter; add an import for
java.util.function.IntFunction at the top of the file and change the method
signature and any matching usages from java.util.function.IntFunction<Color> to
the simple IntFunction<Color> (i.e., update inlineTabIndicators(int active,
String[] labels, IntFunction<Color> activeColorFn) and adjust any references to
activeColorFn accordingly).
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/ToolPanel.java`:
- Around line 777-799: In renderTableWithScrollbar, compute the viewport height
from the actual table drawing rectangle and the table's header/border padding
(create tableArea first and derive viewportRows = Math.max(0, tableArea.height()
- headerAndBorderLines) using the table's header/border/padding API such as
getHeaderHeight()/getVerticalPadding() or equivalent), use the table's scroll
offset (state.offset() or state.scrollOffset()) for ScrollbarState.position with
a fallback to state.selected() only if no offset exists, and build the scrollbar
column relative to tableArea (use tableArea.right() - 1 when creating
scrollArea) so hit-testing matches the rendered table width; update the
references to viewportRows, tableArea, scrollArea, and ScrollbarState.position
accordingly.
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/UpdatesTui.java`:
- Around line 666-669: The TAB key handler currently mutates the field view
directly which skips the centralized logic in setActiveSubView and leaves stale
search/sort state; change the handler so it calls setActiveSubView(nextView)
instead of assigning view = View.values()[(...)] so setActiveSubView can call
clearSearch(), rebuild sortState for the target view's column count, and trigger
any necessary UI rebuilds (see setActiveSubView, clearSearch,
updateSearchMatches, sortState and onSortChanged references to locate the code
to update).
---
Outside diff comments:
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/ModuleTreePane.java`:
- Around line 236-249: handleMouseClick currently treats clicks in the scrollbar
gutter as row clicks; update handleMouseClick to first detect and ignore clicks
in the reserved scrollbar column by checking mouse.x() against the right edge of
the area (e.g., if mouse.x() >= area.x() + area.width() - 1) and return false in
that case so no row selection or node.expanded toggle occurs; keep the existing
row calculation, visibleNodes(), tableState.select(row), and arrowX/expansion
logic otherwise.
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/TreeTui.java`:
- Around line 186-200: In handleTreeMouseClick(MouseEvent mouse, Rect area) the
row calculation uses an incorrect vertical offset (-2) causing row selection to
be one line too high; change the formula in TreeTui.handleTreeMouseClick from
"mouse.y() - area.y() - 2 + tableState.offset()" to use -1 to match
ToolPanel.mouseToTableRow, and update the inline comment to state only the
header is subtracted (remove "highlight" from the comment) so the calculation
reads "mouse.y() - area.y() - 1 + tableState.offset()".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58f4fee4-83d4-4c3e-8804-bb9e99d08507
📒 Files selected for processing (13)
pilot-core/src/main/java/eu/maveniverse/maven/pilot/AlignTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/AuditTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ConflictsTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/DependenciesTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ModuleTreePane.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/PomTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/SearchTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/TabBar.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/Theme.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ToolPanel.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/TreeTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/UpdatesTui.javapilot-core/src/test/java/eu/maveniverse/maven/pilot/TabBarTest.java
💤 Files with no reviewable changes (2)
- pilot-core/src/test/java/eu/maveniverse/maven/pilot/TabBarTest.java
- pilot-core/src/main/java/eu/maveniverse/maven/pilot/TabBar.java
Comment on lines
+777
to
+799
| static void renderTableWithScrollbar(Frame frame, Rect area, Table table, TableState state, int totalRows) { | ||
| int viewportRows = Math.max(0, area.height() - 1); | ||
| if (totalRows <= viewportRows || area.width() < 3) { | ||
| frame.renderStatefulWidget(table, area, state); | ||
| return; | ||
| } | ||
| Rect tableArea = new Rect(area.x(), area.y(), area.width() - 1, area.height()); | ||
| frame.renderStatefulWidget(table, tableArea, state); | ||
|
|
||
| Rect scrollArea = new Rect(area.right() - 1, area.y() + 1, 1, area.height() - 1); | ||
| ScrollbarState scrollState = new ScrollbarState() | ||
| .contentLength(totalRows) | ||
| .viewportContentLength(viewportRows) | ||
| .position(state.selected() != null ? state.selected() : 0); | ||
| frame.renderStatefulWidget( | ||
| Scrollbar.builder() | ||
| .trackSymbol("│") | ||
| .thumbSymbol("█") | ||
| .beginSymbol(null) | ||
| .endSymbol(null) | ||
| .build(), | ||
| scrollArea, | ||
| scrollState); |
Contributor
There was a problem hiding this comment.
Base the scrollbar on the actual table viewport, not the selected row.
The renderTableWithScrollbar() helper has three synchronization issues:
- Viewport height miscalculation:
int viewportRows = area.height() - 1underestimates space consumed by borders and headers. Bordered or headered tables need 2+ lines, making this calculation off by at least one row. - Scrollbar thumb position:
.position(state.selected())drives the thumb by the selected row index, not the viewport's scroll offset. These are different concepts—when a user selects a row outside the visible viewport, the thumb jumps incorrectly instead of tracking what's actually displayed. - Geometry mismatch in hit-testing: The table is rendered in
area.width() - 1, but downstream callers likely cached the full-widthareafor mouse handlers. The scrollbar column then appears as table space to hit-detection logic, causing click misregistration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/ToolPanel.java` around
lines 777 - 799, In renderTableWithScrollbar, compute the viewport height from
the actual table drawing rectangle and the table's header/border padding (create
tableArea first and derive viewportRows = Math.max(0, tableArea.height() -
headerAndBorderLines) using the table's header/border/padding API such as
getHeaderHeight()/getVerticalPadding() or equivalent), use the table's scroll
offset (state.offset() or state.scrollOffset()) for ScrollbarState.position with
a fallback to state.selected() only if no offset exists, and build the scrollbar
column relative to tableArea (use tableArea.right() - 1 when creating
scrollArea) so hit-testing matches the rendered table width; update the
references to viewportRows, tableArea, scrollArea, and ScrollbarState.position
accordingly.
Contributor
Author
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Not changing this. TamboUI's Table class does not expose getHeaderHeight(), getVerticalPadding(), or any similar API to introspect border/header overhead. Using state.selected() for the scrollbar position works correctly here because the table auto-scrolls to keep the selection visible, so the selected index tracks the viewport position. The current implementation has been empirically validated with scrollbar rendering across all table views.
Contributor
There was a problem hiding this comment.
@gnodet, understood — thanks for the detailed explanation. Given TamboUI's Table lacks header/border introspection APIs, the current area.height() - 1 heuristic is a reasonable approximation. The auto-scroll-to-selection behavior also makes state.selected() a valid proxy for the thumb position. Happy to defer to the empirical validation across all table views.
✏️ Learnings added
Learnt from: gnodet
Repo: maveniverse/pilot PR: 93
File: pilot-core/src/main/java/eu/maveniverse/maven/pilot/ToolPanel.java:777-799
Timestamp: 2026-05-06T12:20:17.105Z
Learning: In pilot (maveniverse/pilot), TamboUI's `Table` widget does not expose APIs like `getHeaderHeight()`, `getVerticalPadding()`, or similar introspection methods for header/border overhead. The `renderTableWithScrollbar()` helper in `ToolPanel.java` intentionally uses `state.selected()` for `ScrollbarState.position()` because TamboUI's table auto-scrolls to keep the selected row visible, meaning the selected index effectively tracks the viewport scroll offset. The implementation uses `area.height() - 1` as a viewport approximation and has been empirically validated across all table views in the project.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
Author
|
Claude Code on behalf of Guillaume Nodet Responding to the two outside-diff comments: ModuleTreePane.java (scrollbar gutter clicks) — Fixed in c07a185. Added an early return TreeTui.java (mouse Y offset -2 vs -1) — Not changing this. The |
- Replace hand-rolled TabBar with TamboUI's Tabs/TabsState widget in ToolPanel, simplifying tab rendering and state management - Add renderTableWithScrollbar() helper to ToolPanel and use it across all primary table views (AlignTui, AuditTui, ConflictsTui, DependenciesTui, ModuleTreePane, PomTui, SearchTui, TreeTui, UpdatesTui) - Update tree expand/collapse arrows from ▾/▸ to ▼/▶ - Fix mouse click coordinate calculation in tree views - Add tabsHighlightStyle() and tabsTitleStyle() to Theme - Delete TabBar and TabBarTest (replaced by TamboUI Tabs) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…icks - Use setActiveSubView() for TAB key in AuditTui and UpdatesTui standalone mode so search state and sort columns are properly reset - Import IntFunction in Theme.java instead of using FQCN - Ignore mouse clicks on scrollbar gutter column in ModuleTreePane Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove redundant ▸ highlight symbol from all tables — the reversed+bold highlightStyle already distinguishes the selected row - Update expand/collapse arrows from ▾/▸ to ▼/▶ in XmlTreeModel, AuditTui, and UpdatesTui trees - Derive highlight width from CharWidth.of(theme.highlightSymbol()) instead of hardcoding, so mouse click offsets stay correct if the symbol changes - Remove dead HIGHLIGHT_SYMBOL constants from AuditTui and DependenciesTui - Centralize all .highlightSymbol() calls to use theme.highlightSymbol() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/PomTui.java`:
- Line 600: The arrow hit-test is using a hardcoded width of 2 while rendering
now uses theme.highlightSymbol(), so update handlePomMouseClick to compute the
highlight width dynamically (e.g., int highlightWidth =
theme.highlightSymbol().length() or Character.charCount(...) if needed) and use
that value instead of 2 when calculating the expand/collapse hitbox; ensure the
same computed width is used in any column/offset math that references the
arrow/expand column so hit-testing aligns with renderTableWithScrollbar and
theme.highlightSymbol().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a66dfbb-b988-4121-b9d6-a2ba350a30bf
📒 Files selected for processing (14)
pilot-core/src/main/java/eu/maveniverse/maven/pilot/AlignTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/AuditTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ConflictsTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/DependenciesTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ModuleTreePane.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/PomTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/SearchTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/TabBar.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/Theme.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/ToolPanel.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/TreeTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/UpdatesTui.javapilot-core/src/main/java/eu/maveniverse/maven/pilot/XmlTreeModel.javapilot-core/src/test/java/eu/maveniverse/maven/pilot/TabBarTest.java
💤 Files with no reviewable changes (2)
- pilot-core/src/main/java/eu/maveniverse/maven/pilot/TabBar.java
- pilot-core/src/test/java/eu/maveniverse/maven/pilot/TabBarTest.java
| .build(); | ||
|
|
||
| frame.renderStatefulWidget(table, area, tableState); | ||
| renderTableWithScrollbar(frame, area, table, tableState, rows.size()); |
Contributor
There was a problem hiding this comment.
Fix arrow hit-testing to use dynamic highlight width.
Line 600 now renders with theme.highlightSymbol() (currently empty), but handlePomMouseClick still assumes a fixed highlight width of 2. That shifts the expand/collapse hitbox and causes mis-clicks.
Proposed fix
+import dev.tamboui.text.CharWidth;
...
- int arrowX = area.x() + 1 + 2 + currentModel().relativeDepth(e) * 2; // border(1) + highlight(2) + indent
+ int arrowX = area.x() + 1 + CharWidth.of(theme.highlightSymbol()) + currentModel().relativeDepth(e) * 2;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pilot-core/src/main/java/eu/maveniverse/maven/pilot/PomTui.java` at line 600,
The arrow hit-test is using a hardcoded width of 2 while rendering now uses
theme.highlightSymbol(), so update handlePomMouseClick to compute the highlight
width dynamically (e.g., int highlightWidth = theme.highlightSymbol().length()
or Character.charCount(...) if needed) and use that value instead of 2 when
calculating the expand/collapse hitbox; ensure the same computed width is used
in any column/offset math that references the arrow/expand column so hit-testing
aligns with renderTableWithScrollbar and theme.highlightSymbol().
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
1 participant
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.
Summary
TabBarwith TamboUI'sTabs/TabsStatewidget inToolPanel, simplifying tab rendering and state management. DeleteTabBarandTabBarTest.renderTableWithScrollbar()helper toToolPaneland use it across all primary table views (AlignTui,AuditTui,ConflictsTui,DependenciesTui,ModuleTreePane,PomTui,SearchTui,TreeTui,UpdatesTui).▾/▸(U+25BE/U+25B8) to▼/▶(U+25BC/U+25B6) for better visibility.ModuleTreePaneandTreeTuitree views for accurate row selection and arrow expand/collapse hit-testing.tabsHighlightStyle()andtabsTitleStyle()toThemefor the new Tabs widget.Test plan
./mvnw compile -B— compiles cleanly./mvnw test -B— all tests pass▼/▶in both module tree and dependency tree▼/▶toggles expand/collapse🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements