feat(taints): add a picker for widely used taints - #565
Conversation
Adding a taint meant typing a long key by hand (node-role.kubernetes.io/control-plane, node.kubernetes.io/out-of-service, the cloud-provider spot taints). A typo produces a taint nothing tolerates, silently. Press p in the taint editor's list to pick from 12 real, documented taints, each with a note on what applying it does. Selecting one fills the add-row key, value and effect and focuses the key field, so a preset is a starting point rather than a commitment: all three fields stay editable and the taint still goes through the normal validation and staging path. p mirrors a for manual add. A ctrl binding was avoidable here, and the app-wide "." presets key was unusable: a taint key legitimately contains dots, so the add-row inputs must keep it. The node-controller-managed condition taints (not-ready, unreachable, memory-pressure, disk-pressure, pid-pressure, network-unavailable) are deliberately excluded -- the controller adds and removes them from live node conditions, so a hand-applied copy is either redundant or fought over. A test enforces the exclusion. Both taint overlays share one case in handleOverlayKeySecondary and renderOverlayContentExtended and split in handleTaintOverlayKey / renderTaintOverlay: those dispatchers sit at the gocyclo cap, and view_overlays.go at the file-length cap.
Summary by CodeRabbit
WalkthroughAdds a model-level collection of common Kubernetes taint presets with validation tests. Integrates a preset picker overlay into the taint editor, including cursor and scroll state, keyboard navigation, selection application, rendering, routing, and hint bars. Adds application tests covering picker lifecycle, selection, staging, cancellation, cursor bounds, rendering, and hints. Updates keybinding documentation with the new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
internal/app/taint_presets_test.go (1)
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the picker through the top-level Bubble Tea flow.
These tests call picker internals directly, leaving the new overlay key/render dispatch paths untested. Add an integration test that opens, navigates, selects, and renders through the model update/view path using
basePush80Model()andstripANSI(view).As per coding guidelines, “Create Unit, Integration, and E2E tests for critical flows” and “For TUI tests using Bubble Tea, build a model with basePush80Model() and assert on stripANSI(view).”
🤖 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/taint_presets_test.go` around lines 15 - 32, Extend taint preset coverage with an integration test that uses basePush80Model() and drives the picker through the top-level Bubble Tea update flow: open the overlay, navigate, select an item, and verify rendering via stripANSI(view). Keep the existing taintPresetKey helper for direct unit coverage, but assert the new overlay dispatch and view behavior through the model’s update and view methods.Source: Coding guidelines
🤖 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 `@docs/keybindings.md`:
- Line 1059: Update the README’s taint-editor keybinding documentation to
include the new p shortcut for picking a common taint, matching the entry added
near the taint binding in docs/keybindings.md. Keep the help, hint bar, and
documentation descriptions synchronized.
In `@internal/app/taint_presets.go`:
- Around line 64-83: Update the picker motion handling around presetCursor to
consume count prefixes via the shared consumeCountPrefix flow, without manually
parsing digit keys. Apply the consumed count to j/k, g/G, and all paging
motions, preserving clamping at valid cursor bounds and existing single-step
behavior when no count is provided.
---
Nitpick comments:
In `@internal/app/taint_presets_test.go`:
- Around line 15-32: Extend taint preset coverage with an integration test that
uses basePush80Model() and drives the picker through the top-level Bubble Tea
update flow: open the overlay, navigate, select an item, and verify rendering
via stripANSI(view). Keep the existing taintPresetKey helper for direct unit
coverage, but assert the new overlay dispatch and view behavior through the
model’s update and view methods.
🪄 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: 904e5b47-c389-4572-85a2-1a5176e6f388
📒 Files selected for processing (11)
docs/keybindings.mdinternal/app/app_types.gointernal/app/overlay_hintbar.gointernal/app/taint_editor.gointernal/app/taint_editor_keys.gointernal/app/taint_presets.gointernal/app/taint_presets_test.gointernal/app/update_overlays.gointernal/app/view_overlays.gointernal/model/taint_presets.gointernal/model/taint_presets_test.go
Adding a taint meant typing a long key by hand —
node-role.kubernetes.io/control-plane,node.kubernetes.io/out-of-service, the cloud-provider spot taints. A typo produces a taint nothing tolerates, silently.Press
pin the taint editor's list to pick from 12 real, documented taints, each with a note on what applying it does.Selecting one fills the add-row key, value and effect and focuses the key field — a preset is a starting point, not a commitment: all three fields stay editable and the taint still goes through
ValidateTaintand the normal staging path.Design notes
Why
p. It mirrorsafor manual add. A ctrl binding was avoidable, and the app-wide.presets key was unusable here — a taint key legitimately contains dots, so the add-row inputs must keep it. Esc backs out without adopting the highlighted preset.What is deliberately excluded. The node-controller-managed condition taints (
not-ready,unreachable,memory-pressure,disk-pressure,pid-pressure,network-unavailable) are absent: the controller adds and removes them from live node conditions, so a hand-applied copy is either redundant or fought over. A test enforces the exclusion.Dispatcher headroom. Both taint overlays share one case in
handleOverlayKeySecondaryandrenderOverlayContentExtendedand split inhandleTaintOverlayKey/renderTaintOverlay. Those dispatchers sit at the gocyclo cap (30) andview_overlays.goat the file-length cap (800) — two separate cases would breach all three.Docs
p common taintsin the editor, plus the picker's own hint bardocs/keybindings.md: Node Actions entry updatedREADME and the in-app help do not document taint-editor internals at all (not even the existing
a), so nothing was added there — the hint bar is the surface for these keys.Test plan
internal/model/taint_presets_test.go— every preset passesValidateTaint, no duplicate key+effect identities, no controller-managed condition taints,dedicatedships without a valueinternal/app/taint_presets_test.go— open, select-fills-and-focuses, picked-preset-stages, esc-adopts-nothing, cursor clamping, render, both hint barsgo test -race ./...— all packages passgolangci-lint run ./...— 0 issues