fix: stabilize compact context picker selection#220
Conversation
- keep context table selections visible in compact terminals - avoid premature upward scrolling from the bottom visible row - use amber selected styling for stronger contrast against blue themes - add regression tests for compact context picker navigation
WalkthroughThis PR keeps selected rows visible in compact terminals by adding clamped cursor-setting and navigation helpers, compact-mode sizing/rendering for the context picker, anchor-aware fit-to-height trimming that centers on the selected row, and tests validating these behaviors. ChangesCompact Terminal Selection Visibility
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as handleContextMsg
participant Nav as moveContextTableUp/Down
participant Clamp as setContextTableCursorClamped
participant Table as contextTable
User->>UI: press up/down key
UI->>Nav: route to moveContextTableUp/Down
Nav->>Clamp: compute/clamp index, scroll top, step down
Clamp->>Table: SetCursor(clampedIndex)
Table->>UI: table cursor read -> m.ctxIdx synced
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation successfully addresses the compact context picker stability issues described in #218 and #219. The changes include improved contrast for selected rows using amber styling, smart viewport management to keep selections visible in small terminal windows, and refactored navigation methods that properly sync with the table model. The comprehensive test coverage validates all new behaviors including compact mode detection, selection visibility, and navigation wrapping. The code is well-structured and ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/app/context_table.go`:
- Around line 135-152: The table height calculations currently clamp to a
minimum of 2 visible lines; update them to use the required minimum of 5 visible
lines by replacing all occurrences of max(..., 2) with max(..., 5) in the
contextPickerTableHeight function and any helper used (e.g.,
contextTableHeight), and ensure the intermediate computed height adjustments
(the -6 and -2 offsets and the compact branch using max(m.height-6, ...)) still
call max(..., 5) so visibleLines := max(m.height - N, 5) is enforced; update
references to contextPickerCompact() and contextPickerFilterVisible() logic
accordingly so final returned value never drops below 5.
In `@internal/app/styles_test.go`:
- Around line 231-233: The test currently only checks presence of ">" and
selected separately which can be satisfied if the marker and label are on
different lines; update the assertion in the test that uses variables view and
selected to require the marker and label to appear together (for example assert
that the string "> "+selected is contained in view or parse view into lines and
verify a line equals or contains the marker plus selected), and apply the same
tightening to the similar assertion later in the file (the other check around
lines 255-257) so the test verifies the selected row itself remains visible
rather than just the marker and label existing anywhere in the output.
In `@internal/app/styles.go`:
- Around line 232-296: trimLinesAroundAnchor can produce fewer than height lines
when clamping start/end near the top or bottom removes an ellipsis without
reallocating that slot; update trimLinesAroundAnchor to guarantee len(result) ==
height by recalculating and redistributing the freed line budget when start<1 or
end>len(lines)-1 (adjust remaining, contextBefore, contextAfter or expand
start/end accordingly) before appending lines and ellipses. Specifically, after
computing start and end and performing the clamping steps that modify start/end,
detect how many lines are still needed to reach height (taking into account the
first and last lines and any ellipsis slots created via
beforeEllipsis/afterEllipsis), then expand the available window (increase end or
decrease start as possible) or remove an ellipsis and replace it with an actual
line from the opposite side so that the final result slice built into result
always has exactly height entries; keep references to trimLinesAroundAnchor,
height, anchor, start, end, remaining, contextBefore, contextAfter, result and
dimStyle.Render to locate and modify the logic.
🪄 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 Plus
Run ID: b82d2a37-bf61-4ee0-b328-73c610d11c33
📒 Files selected for processing (5)
internal/app/context_table.gointernal/app/screen_context.gointernal/app/screen_context_test.gointernal/app/styles.gointernal/app/styles_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use lipgloss for styled TUI output — column-aligned tables with dimmed labels in Go implementation files
Implement scroll windowing with formula:visibleLines := max(m.height-N, 5)in Go TUI implementation
Files:
internal/app/styles.gointernal/app/screen_context_test.gointernal/app/styles_test.gointernal/app/screen_context.gointernal/app/context_table.go
⚙️ CodeRabbit configuration file
**/*.go: For Go reviews, look beyond compilation and prioritize nil pointer risks,
context propagation, AWS SDK pagination, error wrapping, deterministic
sorting, and stable table/detail rendering. For new AWS service work,
verify that repository interfaces, model mapping, app integration, and
tests are updated together.
Files:
internal/app/styles.gointernal/app/screen_context_test.gointernal/app/styles_test.gointernal/app/screen_context.gointernal/app/context_table.go
internal/app/**
⚙️ CodeRabbit configuration file
internal/app/**: For Bubble Tea screen changes, verify message routing, key handling,
filter target resets, height-based windowing, help text, and back/home
navigation against the existing screen patterns.
Files:
internal/app/styles.gointernal/app/screen_context_test.gointernal/app/styles_test.gointernal/app/screen_context.gointernal/app/context_table.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests use mock client interfaces (see
rds_test.gopattern) in Go test files
Files:
internal/app/screen_context_test.gointernal/app/styles_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Check that tests cover API errors, mapping edge cases, and navigation
state transitions, not only happy paths. Prefer mock-based tests that do
not depend on external AWS calls.
Files:
internal/app/screen_context_test.gointernal/app/styles_test.go
🔇 Additional comments (5)
internal/app/context_table.go (1)
53-101: LGTM!Also applies to: 154-163
internal/app/screen_context.go (1)
148-150: LGTM!Also applies to: 266-308
internal/app/screen_context_test.go (1)
27-37: LGTM!Also applies to: 130-211
internal/app/styles.go (1)
5-17: LGTM!Also applies to: 203-230
internal/app/styles_test.go (1)
289-296: LGTM!
| return max(terminalHeight-10, 2) | ||
| } | ||
|
|
||
| func (m Model) contextPickerTableHeight() int { | ||
| if m.contextPickerCompact() { | ||
| height := max(m.height-6, 2) | ||
| if m.contextPickerFilterVisible() { | ||
| height -= 2 | ||
| } | ||
| return max(height, 2) | ||
| } | ||
|
|
||
| height := contextTableHeight(m.height) | ||
| if m.contextPickerFilterVisible() { | ||
| height -= 2 | ||
| } | ||
| return max(height, 2) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the required minimum visible window size in table height calculations.
Line 135 and Lines 138-152 use a minimum of 2, but this TUI windowing should clamp to 5 visible lines using max(m.height-N, 5).
💡 Suggested adjustment
func contextTableHeight(terminalHeight int) int {
if terminalHeight <= 0 {
return defaultContextTableHeight
}
- return max(terminalHeight-10, 2)
+ return max(terminalHeight-10, 5)
}
func (m Model) contextPickerTableHeight() int {
- if m.contextPickerCompact() {
- height := max(m.height-6, 2)
- if m.contextPickerFilterVisible() {
- height -= 2
- }
- return max(height, 2)
- }
-
- height := contextTableHeight(m.height)
- if m.contextPickerFilterVisible() {
- height -= 2
- }
- return max(height, 2)
+ offset := 10
+ if m.contextPickerCompact() {
+ offset = 6
+ }
+ if m.contextPickerFilterVisible() {
+ offset += 2
+ }
+ return max(m.height-offset, 5)
}As per coding guidelines, Implement scroll windowing with formula: visibleLines := max(m.height-N, 5) in Go TUI implementation.
📝 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.
| return max(terminalHeight-10, 2) | |
| } | |
| func (m Model) contextPickerTableHeight() int { | |
| if m.contextPickerCompact() { | |
| height := max(m.height-6, 2) | |
| if m.contextPickerFilterVisible() { | |
| height -= 2 | |
| } | |
| return max(height, 2) | |
| } | |
| height := contextTableHeight(m.height) | |
| if m.contextPickerFilterVisible() { | |
| height -= 2 | |
| } | |
| return max(height, 2) | |
| } | |
| return max(terminalHeight-10, 5) | |
| } | |
| func (m Model) contextPickerTableHeight() int { | |
| offset := 10 | |
| if m.contextPickerCompact() { | |
| offset = 6 | |
| } | |
| if m.contextPickerFilterVisible() { | |
| offset += 2 | |
| } | |
| return max(m.height-offset, 5) | |
| } |
🤖 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 `@internal/app/context_table.go` around lines 135 - 152, The table height
calculations currently clamp to a minimum of 2 visible lines; update them to use
the required minimum of 5 visible lines by replacing all occurrences of max(...,
2) with max(..., 5) in the contextPickerTableHeight function and any helper used
(e.g., contextTableHeight), and ensure the intermediate computed height
adjustments (the -6 and -2 offsets and the compact branch using max(m.height-6,
...)) still call max(..., 5) so visibleLines := max(m.height - N, 5) is
enforced; update references to contextPickerCompact() and
contextPickerFilterVisible() logic accordingly so final returned value never
drops below 5.
- require selected markers and labels on the same rendered row - keep anchored height trimming exact near top and bottom edges
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 `@internal/app/styles.go`:
- Around line 251-260: When height is very small (e.g. height == 4) the current
windowSize calculation can drop to 0 and make contextAfter negative so the
anchor row disappears; detect this case and force the anchor to consume one
visible line by ensuring windowSize is at least 1 (or by limiting ellipsis to a
single slot). Concretely, in the block that computes windowSize, contextBefore
and contextAfter (variables: windowSize, contextBefore, contextAfter, anchor,
lines), add a special-case for small heights/interior anchors that sets
windowSize = 1 (or reduces the number of ellipsis slots to one) and then
recompute contextBefore/contextAfter so they are non-negative and guarantee the
computed slice indices (start:end) include the anchor line.
🪄 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 Plus
Run ID: 2f475c64-205d-4bbc-a8d0-88fd54b742a0
📒 Files selected for processing (2)
internal/app/styles.gointernal/app/styles_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use lipgloss for styled TUI output — column-aligned tables with dimmed labels in Go implementation files
Implement scroll windowing with formula:visibleLines := max(m.height-N, 5)in Go TUI implementation
Files:
internal/app/styles.gointernal/app/styles_test.go
⚙️ CodeRabbit configuration file
**/*.go: For Go reviews, look beyond compilation and prioritize nil pointer risks,
context propagation, AWS SDK pagination, error wrapping, deterministic
sorting, and stable table/detail rendering. For new AWS service work,
verify that repository interfaces, model mapping, app integration, and
tests are updated together.
Files:
internal/app/styles.gointernal/app/styles_test.go
internal/app/**
⚙️ CodeRabbit configuration file
internal/app/**: For Bubble Tea screen changes, verify message routing, key handling,
filter target resets, height-based windowing, help text, and back/home
navigation against the existing screen patterns.
Files:
internal/app/styles.gointernal/app/styles_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests use mock client interfaces (see
rds_test.gopattern) in Go test files
Files:
internal/app/styles_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Check that tests cover API errors, mapping edge cases, and navigation
state transitions, not only happy paths. Prefer mock-based tests that do
not depend on external AWS calls.
Files:
internal/app/styles_test.go
| windowSize := height - 2 // first line + footer | ||
| if anchor > 1 { | ||
| windowSize-- | ||
| } | ||
| if anchor < len(lines)-2 { | ||
| windowSize-- | ||
| } | ||
|
|
||
| contextBefore := windowSize / 2 | ||
| contextAfter := windowSize - contextBefore - 1 |
There was a problem hiding this comment.
Preserve the anchor row when only four lines fit.
With height == 4 and an interior anchor, windowSize drops to 0, contextAfter becomes -1, and lines[start:end] is empty. The rendered result is just header/ellipsis/ellipsis/footer, so the selected row disappears again in the smallest compact views. Special-case 4-line rendering (or cap ellipsis usage to one slot there) so the anchor always consumes one visible line.
Possible fix
if height == 3 {
return []string{lines[0], lines[anchor], lines[len(lines)-1]}
}
+ if height == 4 {
+ if anchor <= len(lines)/2 {
+ return []string{lines[0], lines[anchor], dimStyle.Render(" ..."), lines[len(lines)-1]}
+ }
+ return []string{lines[0], dimStyle.Render(" ..."), lines[anchor], lines[len(lines)-1]}
+ }
if anchor <= 0 || anchor >= len(lines)-1 {
return trimLinesWithFixedEdges(lines, height)
}🤖 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 `@internal/app/styles.go` around lines 251 - 260, When height is very small
(e.g. height == 4) the current windowSize calculation can drop to 0 and make
contextAfter negative so the anchor row disappears; detect this case and force
the anchor to consume one visible line by ensuring windowSize is at least 1 (or
by limiting ellipsis to a single slot). Concretely, in the block that computes
windowSize, contextBefore and contextAfter (variables: windowSize,
contextBefore, contextAfter, anchor, lines), add a special-case for small
heights/interior anchors that sets windowSize = 1 (or reduces the number of
ellipsis slots to one) and then recompute contextBefore/contextAfter so they are
non-negative and guarantee the computed slice indices (start:end) include the
anchor line.
1 participant
Summary
Fix compact context picker rendering and navigation so selected rows remain visible in small terminal windows. Selection styling now uses amber foreground/background treatment for stronger contrast against blue terminal themes.
Related Issues
Closes #218
Closes #219
Validation
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests