[RAPTOR-19762] feat(workload): highlight the selected picker row with a yellow bar - #857
Conversation
|
🎫 Jira: |
|
Wonder how this handles light mode vs dark mode. cc @cdevent |
6f67a4b to
714f250
Compare
Code OwnershipWorkload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
There was a problem hiding this comment.
Nice work — the yellow selection now reads consistently across every list in the wizard, and the · in use lift in the base-image picker is a real usability win. Thanks for using GetAdaptiveColor(tui.DrYellow, tui.DrYellowDark).
One blocking finding (inline): the coloured tag inside a table cell collides with columnsFor's len(cell) sizing and bubbles/table's ANSI-unaware truncation — reproducible with a probe against the real escape bytes. Plus one minor consistency nit on the cursor glyph. Separately, I'd like to have a discussion in the future about making the UI consistent outside the workload wizard — today cmd/templates/list, cmd/llm-gateway/select, cmd/component/shared and tui/hostpicker each carry their own selection vocabulary (mostly non-adaptive purples) — whether that's adopting this wizard's design (a shared exported selectedStyle in tui) or something else entirely. Not a blocker for this PR, which is correctly scoped.
| // rest of the row intact. | ||
| if liveID != "" && ee.ID == liveID { | ||
| last := len(row.cells) - 1 | ||
| row.cells[last] += liveStyle.Render(inUseSuffix) |
There was a problem hiding this comment.
[P1] The coloured tag inside this cell breaks column sizing and can be cut mid-escape-sequence by bubbles/table.
Two downstream consumers don't know this cell now carries ~24 bytes of SGR codes:
columnsFor(table.go:158) sizes columns withlen(cell), so whenever a bound workload exists the ID column is sized ~20 columns wider than its visible content.- Worse:
bubbles/tabletruncates every cell withrunewidth.Truncate(value, col.Width, "…"), and go-runewidth is not ANSI-aware. Verified with a probe against the exact escape bytes lipgloss emits:
len(cell)=62 runewidth.StringWidth(cell)=59 // escape interiors counted as visible
Truncate(cell, 40) => "ee-1a2b3c4d5e6f7a8b9c0d1e2f\x1b[38;2;12…"
The truncated string ends inside the SGR sequence — an unterminated escape that bleeds green over the rest of the row. This fires whenever the overflow branch in columnsFor (or the 56-char maxColumnWidth cap) shrinks the ID column below the tagged cell's escape-inflated width — i.e. a bound workload on an ~80-column terminal, which is exactly this feature's use case. The tests miss it because they run in the no-colour profile, where liveStyle.Render is plain text.
Suggested direction: keep escape sequences out of the measured/truncated path — either measure with an ANSI-aware width consistent with what bubbles/table itself sees (runewidth.StringWidth, making Truncate a no-op) and accept the cosmetic width, or render the picker's suffix as plain text and keep the green tag on the menu screens only.
|
|
||
| if i == c.selected { | ||
| b.WriteString(tui.InfoStyle.Render("> " + line)) | ||
| b.WriteString(selectedStyle.Render("> " + line)) |
There was a problem hiding this comment.
[P3] Two cursor glyphs inside one wizard.
The PR's stated goal is "one selection look", but the menus render the cursor as ASCII "> " here while the pickers render " ❯" (table.go syncRows). Same semantic, sibling-file inconsistency — worth unifying on ❯ while this PR is already touching both.
…re table path Addresses AJ's review on datarobot-oss#857. P1 — the "· in use" tag rode a table cell as a styled string, and two bubbles/table consumers are ANSI-unaware: columnsFor sized the column with len(cell), counting the tag's ~24 escape bytes, and bubbles/table truncates each cell with runewidth.Truncate, which can cut mid-escape on a narrow terminal and bleed colour across the row — exactly the bound-workload-on-80- columns case this feature targets. Sizing the column to the escape-inflated width does not fix it (the overflow branch shrinks the widest column back below that width), so the tag is now plain text in the picker cell. The green live-value tag stays on the menu screens (choice.go), which render their own lines and never truncate. columnsFor now measures display width via lipgloss.Width, matching what bubbles/table truncates against, and documents that cells must stay plain text. P3 — the menus rendered the cursor as ASCII "> " while the pickers use "❯"; choice.go now uses "❯" too, so a selection reads the same everywhere. Adds two regression guards: an execenv-picker test that forces a TrueColor profile (the no-colour test profile is why the original tests missed this) and asserts the built cells carry no escape, and a columnsFor test that a cell containing ANSI is sized by visible width, not byte length. Verified the colour test fails on the pre-fix code. termenv moves to a direct dependency for the forced-profile test. task lint clean on all GOOS; go test -race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
It roughly doubles the diff and pulls unreviewed content into the merge. Worth dropping before this goes further so the review can stay on the actual wizard change. |
| github.com/jeandeaual/go-locale v0.0.0-20250612000132-0ef82f21eade | ||
| github.com/joho/godotenv v1.5.1 | ||
| github.com/muesli/cancelreader v0.2.2 | ||
| github.com/muesli/termenv v0.16.0 |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
Could you please run the wizard manually and attach a screenshot or terminal capture showing the selected picker row? The ticket asks for a full-width yellow highlight bar, but the current style appears to set only yellow foreground text. A visual proof would make it clear whether the intended selected-row treatment is actually rendering in the TUI. |
|
❌ Some smoke tests failed. (Fork PR) ❌ Security Scan: failure |
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
Non-blocking dependency note.
…re table path Addresses AJ's review on datarobot-oss#857. P1 — the "· in use" tag rode a table cell as a styled string, and two bubbles/table consumers are ANSI-unaware: columnsFor sized the column with len(cell), counting the tag's ~24 escape bytes, and bubbles/table truncates each cell with runewidth.Truncate, which can cut mid-escape on a narrow terminal and bleed colour across the row — exactly the bound-workload-on-80- columns case this feature targets. Sizing the column to the escape-inflated width does not fix it (the overflow branch shrinks the widest column back below that width), so the tag is now plain text in the picker cell. The green live-value tag stays on the menu screens (choice.go), which render their own lines and never truncate. columnsFor now measures display width via lipgloss.Width, matching what bubbles/table truncates against, and documents that cells must stay plain text. P3 — the menus rendered the cursor as ASCII "> " while the pickers use "❯"; choice.go now uses "❯" too, so a selection reads the same everywhere. Adds two regression guards: an execenv-picker test that forces a TrueColor profile (the no-colour test profile is why the original tests missed this) and asserts the built cells carry no escape, and a columnsFor test that a cell containing ANSI is sized by visible width, not byte length. Verified the colour test fails on the pre-fix code. termenv moves to a direct dependency for the forced-profile test. task lint clean on all GOOS; go test -race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8e199b9 to
9088f8a
Compare
|
@ajalon1 — thanks for flagging these; you were right that I'd been working the inline review threads and missing the conversation-level comments. Caught up on all three now:
Light vs dark mode — the selection colour is Full-width bar vs the current style — good catch, and you read the code correctly: it's a bold-yellow foreground, not the ticket's black-on-yellow background bar. I built the filled bar to compare and it read as heavy/ugly against the neutral rows, so we're deliberately keeping the lighter yellow-text cue. I've updated the PR description with a "deviation from the ticket" note calling this out explicitly. The mechanism the ticket cared about is still honoured — the table cells are uncoloured so the row-level style survives On the meta-point: fair — I'll read the full conversation, not just the diff comments, from here on. |
…in-use base image Two related fixes to the dr workload up / config wizard's selectors. Selection is now one yellow everywhere it appears — menu options, the advanced-options row, and the list-picker rows — so a selection reads the same across every list the wizard offers. The pickers could not show a colour before: bubbles/table colours each cell first, and those cells' ANSI resets truncate any style wrapped around the finished row. The table's cells are now left uncoloured so the cursor row's colour shows; non-selected rows go neutral, the usual look for a selection list. The base-image picker now marks the environment a bound workload is already built on with a green '· in use' tag and lifts it to the top, matching the menus (the list runs long, so the likely choice should not be scrolled for). The tag trails on the last cell, which keeps it clean under selection: a coloured cell resets colour where it ends, and only at the row's end does that reset land past the highlighted content. RAPTOR-19762 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re table path Addresses AJ's review on datarobot-oss#857. P1 — the "· in use" tag rode a table cell as a styled string, and two bubbles/table consumers are ANSI-unaware: columnsFor sized the column with len(cell), counting the tag's ~24 escape bytes, and bubbles/table truncates each cell with runewidth.Truncate, which can cut mid-escape on a narrow terminal and bleed colour across the row — exactly the bound-workload-on-80- columns case this feature targets. Sizing the column to the escape-inflated width does not fix it (the overflow branch shrinks the widest column back below that width), so the tag is now plain text in the picker cell. The green live-value tag stays on the menu screens (choice.go), which render their own lines and never truncate. columnsFor now measures display width via lipgloss.Width, matching what bubbles/table truncates against, and documents that cells must stay plain text. P3 — the menus rendered the cursor as ASCII "> " while the pickers use "❯"; choice.go now uses "❯" too, so a selection reads the same everywhere. Adds two regression guards: an execenv-picker test that forces a TrueColor profile (the no-colour test profile is why the original tests missed this) and asserts the built cells carry no escape, and a columnsFor test that a cell containing ANSI is sized by visible width, not byte length. Verified the colour test fails on the pre-fix code. termenv moves to a direct dependency for the forced-profile test. task lint clean on all GOOS; go test -race green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Response to the go.mod review note. Extracted the forced-colour setup into a
forceColor(t) helper with a comment explaining why the CLICOLOR_FORCE +
SetDefaultRenderer alternative does not work here: lipgloss pins each style to
the renderer that was default when NewStyle ran (Style{r: r}), so the
package-level styles are bound to the init-time default renderer. Only mutating
that renderer's profile in place (SetColorProfile) reaches them; swapping the
global default leaves them on the old renderer, whose profile earlier tests have
already cached as Ascii — which is why the env trick passed in isolation but
failed in the full suite. Naming the profile needs termenv, so it stays a
direct dependency (test-only).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…berate choice over the bar The ticket asks for a black-on-yellow highlight bar; the filled bar read as heavy against the neutral rows, so the lighter bold-yellow foreground cue was kept. Documented on selectedStyle, along with the adaptive-colour behaviour that keeps it legible in light and dark themes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5d2f366 to
aaab4b7
Compare
…dent contextcheck lint GetAccountInfo ignored its context.Context (drapi.GetJSON takes none), and retrieveAccountInfo only forwarded it there — the whole chain was dead, kept behind a //nolint:contextcheck. contextcheck traces GetJSON->...->resolveToken and its result is Go-toolchain-dependent: it fires on the CI runner's toolchain in one place and not another, so the nolint reads as "used" locally but "unused" in CI, breaking `task lint` there (nolintlint) while passing locally. Removing the unused context parameters from both functions (and their call sites) makes contextcheck deterministically not fire — there is no context to propagate — so no directive is needed and the result no longer depends on the toolchain. Unrelated to the wizard change; included to unblock this PR's CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @adamalpi What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
❌ Some smoke tests failed. (Fork PR) ✅ Security Scan: success |
2 participants
RATIONALE
In the
dr workload up/configwizard, it was hard to see which option was selected. The list pickers (workload binding, base-image selection) marked the cursor row with only a❯— the colour cue that was meant to accompany it never showed, becausebubbles/tablecolours each cell first and those cells' ANSI resets truncate any style wrapped around the finished row. And the "· in use" tag that marks a bound workload's current choice appeared on the menus but not in the base-image picker.Ticket: RAPTOR-19762
CHANGES
GetAdaptiveColor(DrYellow, DrYellowDark), so it adapts to the terminal background and stays legible in both light and dark themes.bubbles/tablerenders the cursor row's colour over plain cells cleanly instead of having it swallowed. Non-selected rows go neutral, the conventional look for a selection list.· in usein the base-image picker. The picker now tags the environment a bound workload is already built on and lifts it to the top — the list runs long, and the option most likely to be kept should not have to be scrolled for. The tag trails on the right, riding the last cell, and is plain text: it lives inside abubbles/tablecell, and the table sizes columns and truncates cells by display width with no awareness of ANSI, so a styled tag's escape bytes would inflate the measured column width and be cut mid-escape on a narrow terminal, bleeding colour across the row. The green live-value colour stays on the menu screens, which render their own lines and never truncate. Scoped to a bound run: a fresh setup has no live environment, so nothing is tagged and the order is left as it came.valueStyle; foldedselectedRowStyleintoselectedStyle.Verified against the rendered escapes: a selected in-use picker row emits yellow across name/language/id under selection, with non-selected rows plain and no escape sequences embedded in any cell. A forced-TrueColor test guards that the picker cells stay plain text (the no-colour test profile is why an earlier styled tag slipped through), and
columnsFornow measures display width so its sizing agrees with what the table truncates against.Review
Addressed AJ's review (#857):
columnsFor'slen(cell)sizing and risking a mid-escape truncation bybubbles/table(green bleed) on an ~80-column terminal. Fixed by rendering the picker tag as plain text and measuring columns withlipgloss.Width; the green tag remains on the menu screens. Added a forced-colour regression test.>while the pickers use❯; unified on❯.PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation tests🤖 Generated with Claude Code