feat: dynamic table row counts based on terminal height#61
Open
yfarjoun wants to merge 3 commits intofg-labs:mainfrom
Open
feat: dynamic table row counts based on terminal height#61yfarjoun wants to merge 3 commits intofg-labs:mainfrom
yfarjoun wants to merge 3 commits intofg-labs:mainfrom
Conversation
Replace hardcoded max_visible (10/8) with dynamic calculation based on terminal height, layout mode, and number of panels per column. Tables now fill available space on tall terminals and shrink gracefully on short ones. Minimum 3 rows always shown. Closes fg-labs#59 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces hardcoded table row limits with dynamic per-panel sizing based on terminal height and layout. Adds Changes
Sequence Diagram(s)(omitted — changes are local sizing logic without multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
♻️ Duplicate comments (1)
snakesee/tui/monitor.py (1)
3025-3027:⚠️ Potential issue | 🟠 MajorPanel count logic is correct but executed too late.
The panel count calculation correctly determines whether 3 or 2 panels are visible based on whether
failed_jobs_listandincomplete_jobs_listare non-empty. However, as noted in the previous comment, these updates happen after tables have already been rendered using stale panel counts from the previous frame.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakesee/tui/monitor.py` around lines 3025 - 3027, The panel count assignment for self._left_panel_count and self._right_panel_count is happening too late (after tables are rendered) so move the logic that sets these counts based on failed_jobs_list and incomplete_jobs_list to run before any table rendering or layout calculation; locate the code paths that render/update the UI (e.g., the method that builds or draws the panels/tables) and update those to compute self._left_panel_count = 3 if self.incomplete_jobs_list else 2 and self._right_panel_count = 3 if self.failed_jobs_list else 2 prior to calling the render/draw functions so the layout uses the fresh counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakesee/tui/monitor.py`:
- Around line 1645-1657: The _max_visible_rows method currently uses a hardcoded
chrome height (15) valid for FULL layout but wrong for COMPACT; change
_max_visible_rows to accept a layout_mode parameter (or inspect
self.layout_mode) and compute body_height based on layout-specific chrome (e.g.,
FULL: header+progress+summary_footer+footer=15, COMPACT: omit summary_footer =>
12), then return the same panel math; update all call sites (including the
COMPACT caller that currently invokes _max_visible_rows without layout) to pass
the correct layout_mode or let the method read self.layout_mode so the
calculation matches the active UI mode.
- Around line 2053-2054: The code reads self._left_panel_count to compute max
rows before it’s updated later in _make_layout, causing a one-frame lag; extract
the panel-count determination into a new helper (e.g. _update_left_panel_count)
that computes num_panels and sets self._left_panel_count based on current panel
visibility/LayoutMode and call it before computing max_visible =
self._max_visible_rows(num_panels) in the render path; then remove the duplicate
panel-count logic from _make_layout so both rendering and layout use the same
pre-updated value and eliminate the race.
---
Duplicate comments:
In `@snakesee/tui/monitor.py`:
- Around line 3025-3027: The panel count assignment for self._left_panel_count
and self._right_panel_count is happening too late (after tables are rendered) so
move the logic that sets these counts based on failed_jobs_list and
incomplete_jobs_list to run before any table rendering or layout calculation;
locate the code paths that render/update the UI (e.g., the method that builds or
draws the panels/tables) and update those to compute self._left_panel_count = 3
if self.incomplete_jobs_list else 2 and self._right_panel_count = 3 if
self.failed_jobs_list else 2 prior to calling the render/draw functions so the
layout uses the fresh counts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
nh13
reviewed
Apr 5, 2026
snakesee/tui/monitor.py
Outdated
| Returns at least 3 rows so tables are always usable. | ||
| """ | ||
| height = self.console.height or 24 | ||
| # Fixed chrome: header(3) + progress(6) + summary_footer(3) + footer(3) = 15 |
Collaborator
There was a problem hiding this comment.
The hardcoded 15 is correct for FULL layout but wrong for COMPACT/MINIMAL (no summary_footer, so chrome is 12). This wastes 3 rows in COMPACT mode.
Suggestion:
fixed_chrome = 15 if self._layout_mode == LayoutMode.FULL else 12
body_height = height - fixed_chromeAlso, since this is a pure computation of terminal_height, num_panels, and layout_mode, consider extracting it as a standalone function — it would be trivially testable without needing a full WorkflowMonitorTUI instance.
Contributor
Author
There was a problem hiding this comment.
Fixed in 7c0a21e:
fixed_chrome = 15 if self._layout_mode == LayoutMode.FULL else 12Re: extracting as a standalone function — it still reads self.console.height and self._layout_mode, so it would need those passed in. I kept it as a method for now since the tests can set those attributes directly on the TUI instance, but happy to extract if you prefer.
| @@ -2034,7 +2050,8 @@ def _make_running_table(self, progress: WorkflowProgress) -> Panel: # noqa: C90 | |||
| job_data = self._sort_running_job_data(job_data) | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
No tests were added for _max_visible_rows. It's a pure computation that's straightforward to unit test. At minimum:
- Standard terminal height with 2 and 3 panels
- Floor of 3 rows on a very small terminal
- COMPACT vs FULL chrome difference (once the chrome fix is applied)
Contributor
Author
There was a problem hiding this comment.
Added in 7c0a21e — TestMaxVisibleRows with 6 tests:
- FULL layout with 2 and 3 panels
- COMPACT layout with 1 panel
- COMPACT vs FULL comparison (more rows in COMPACT)
- Small terminal (height=10) hits floor of 3
Noneheight falls back to 24
- Fix chrome calculation for COMPACT layout (12 not 15, no summary_footer) - Move panel count update to start of _make_layout to avoid one-frame lag - Extract _update_panel_counts helper method - Add TestMaxVisibleRows tests covering FULL/COMPACT/small terminal/floor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_tui.py (1)
2368-2423: Consider adding an explicit MINIMAL-layout regression test.
_max_visible_rowscurrently treatsLayoutMode.MINIMALvia the non-FULL branch. A dedicated assertion would lock this behavior and catch future branch drift.Proposed test addition
class TestMaxVisibleRows: @@ def test_compact_vs_full_gives_more_rows(self, tui: WorkflowMonitorTUI) -> None: """COMPACT layout yields more rows than FULL for the same panel count.""" from snakesee.tui.monitor import LayoutMode @@ full_rows = tui._max_visible_rows(2) assert compact_rows > full_rows + + def test_minimal_layout_matches_compact_chrome(self, tui: WorkflowMonitorTUI) -> None: + """MINIMAL layout should follow the same chrome math as COMPACT.""" + from snakesee.tui.monitor import LayoutMode + + tui._layout_mode = LayoutMode.MINIMAL + minimal_rows = tui._max_visible_rows(2) + tui._layout_mode = LayoutMode.COMPACT + compact_rows = tui._max_visible_rows(2) + assert minimal_rows == compact_rows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_tui.py` around lines 2368 - 2423, Add a regression test asserting that LayoutMode.MINIMAL is handled by the non-FULL branch of _max_visible_rows: create a new test (alongside the other layout tests) that sets tui._layout_mode = LayoutMode.MINIMAL, calls tui._max_visible_rows with a representative panel count (e.g., 1 or 2), and asserts the expected row count (matching the COMPACT/non-FULL behavior used elsewhere); reference the LayoutMode.MINIMAL enum and the _max_visible_rows method so future changes that move MINIMAL into a separate branch are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_tui.py`:
- Around line 2368-2423: Add a regression test asserting that LayoutMode.MINIMAL
is handled by the non-FULL branch of _max_visible_rows: create a new test
(alongside the other layout tests) that sets tui._layout_mode =
LayoutMode.MINIMAL, calls tui._max_visible_rows with a representative panel
count (e.g., 1 or 2), and asserts the expected row count (matching the
COMPACT/non-FULL behavior used elsewhere); reference the LayoutMode.MINIMAL enum
and the _max_visible_rows method so future changes that move MINIMAL into a
separate branch are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22753d55-4269-4cfb-9128-a7ddda611fa4
📒 Files selected for processing (2)
snakesee/tui/monitor.pytests/test_tui.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snakesee/tui/monitor.py
Ensures MINIMAL layout uses the same chrome calculation as COMPACT, locking this behavior against future branch drift. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Summary
max_visiblevalues (10 for running/pending, 8 for completions/stats) with dynamic calculation based on terminal heightCloses #59
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit