fix(taints): keep the effect field visible in the add-row - #564
Conversation
Typing a long taint key pushed the effect selector off the right edge, where it rendered as "eff~". The effect is a cycling control with no scroll of its own, so a truncated one leaves the user unable to see which effect they are about to stage -- on a change that reshapes scheduling for the whole node. Two causes. The overlay box was sized by OverlayListWidth over the taint LIST rows only, so the add-row never influenced the box width. The add-row was then joined into one line and truncated as a unit, and the effect sits last. Size the box against the add-row as well while an input is focused, and stack the fields onto separate lines when they cannot share one instead of truncating the row as a unit. Each input's value is fitted to its own budget, so the field label survives; the focused value truncates from the front so the caret end -- what the user is typing right now -- stays visible. Adds two ui primitives: OverlayContentWidth, extracted from OverlayListWidth so any overlay can size itself against content the list does not include, and TruncateStart, the mirror of Truncate for text whose end matters more than its start.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughAdds width-aware left truncation and extracts overlay content width calculation. Refactors taint editor add-row rendering to size fields individually, preserve the focused field’s tail, widen the overlay when needed, and stack fields when horizontal space is insufficient. Adds tests for long and extreme keys, caret visibility, overlay sizing, effect visibility, and Unicode-aware truncation. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/app/view_overlay_taint_editor.go (1)
161-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant recomputation of
taintAddRowFields.
fields := taintAddRowFields(p, -1)is computed here, thentaintAddRowNaturalWidth(p)immediately recomputes the sametaintAddRowFields(p, -1)internally to measure width. Consider havingtaintAddRowNaturalWidthaccept the already-builtfieldsslice (or a width helper operating onfields) to avoid building the field strings twice per render.♻️ Proposed refactor
-func taintAddRowNaturalWidth(p taintEditorState) int { - fields := taintAddRowFields(p, -1) - w := len(taintAddRowSep) * (len(fields) - 1) +func taintAddRowFieldsWidth(fields []string) int { + w := lipgloss.Width(taintAddRowSep) * (len(fields) - 1) for _, f := range fields { w += lipgloss.Width(f) } return w } + +func taintAddRowNaturalWidth(p taintEditorState) int { + return taintAddRowFieldsWidth(taintAddRowFields(p, -1)) +}fields := taintAddRowFields(p, -1) sep := taintAddRowSep - if taintAddRowNaturalWidth(p) > innerW { + if taintAddRowFieldsWidth(fields) > innerW { fields = taintAddRowFields(p, innerW) sep = "\n" }🤖 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/view_overlay_taint_editor.go` around lines 161 - 166, Refactor the add-row rendering flow around taintAddRowFields and taintAddRowNaturalWidth to reuse the already-built fields slice when measuring natural width. Update taintAddRowNaturalWidth or introduce a fields-based width helper, then use it in place of recomputing taintAddRowFields(p, -1), while preserving the existing wrapping and separator behavior.
🤖 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/view_overlay_taint_editor.go`:
- Around line 152-171: Update renderTaintEditorAddRow’s final field truncation
to preserve the focused field’s caret: use the start-preserving truncation path
for the focused field and the existing right-side truncation for other fields,
matching taintAddRowFields’ behavior when the field still exceeds innerW.
---
Nitpick comments:
In `@internal/app/view_overlay_taint_editor.go`:
- Around line 161-166: Refactor the add-row rendering flow around
taintAddRowFields and taintAddRowNaturalWidth to reuse the already-built fields
slice when measuring natural width. Update taintAddRowNaturalWidth or introduce
a fields-based width helper, then use it in place of recomputing
taintAddRowFields(p, -1), while preserving the existing wrapping and separator
behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62bf6dae-3b4f-4851-9cf5-e840461b68de
📒 Files selected for processing (5)
internal/app/taint_editor_addrow_test.gointernal/app/view_overlay_taint_editor.gointernal/ui/explorer_format.gointernal/ui/explorer_test.gointernal/ui/overlay_list.go
The per-field budget floors at one column, so on a terminal narrow enough that a field's label alone outgrows the line, taintAddRowFields returns a field wider than innerW and the final truncation runs. That cut was unconditional and right-side, which strips exactly the caret the front-truncation had just preserved -- undoing the fix at narrow widths. Pick the truncation direction by focus there too. The test helper anchored on the "add:" label to find the block; at these widths the label is itself truncated away, so it now anchors on the blank line the renderer joins the block with.
Typing a long taint key pushed the effect selector off the right edge of the node taint editor, where it rendered as
eff~:The effect is a cycling control with no scroll of its own, so a truncated one leaves you unable to see which effect you are about to stage — on a change that reshapes scheduling for the whole node.
Root cause
Two compounding causes:
ui.OverlayListWidthover the taint list rows only, so the add-row never influenced the box width.renderTaintEditorAddRowjoined the three fields into one line and truncated the result as a unit — and the effect sits last, so it was always what got cut.Changes
Two new
internal/uiprimitives:OverlayContentWidthOverlayListWidthso any overlay can size itself against content the list does not include.OverlayListWidthnow delegates to it — no behavior change.TruncateStartTruncate, keeping the tail with a leading~.Test plan
internal/app/taint_editor_addrow_test.go— 4 tests: long key keeps the effect, an extreme key still shows it without overflowing the box, the focused field keeps its caret end, and the box widens for a fitting add-rowinternal/ui/explorer_test.go—TestTruncateStartgo test -race ./...— all packages passgolangci-lint run ./...— 0 issues