[web-console] Implement pipeline tags UI#6506
Conversation
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Draft, so high-level only.
The shape of the UI is fine — SlidingPanels extraction from DownloadSupportBundle is a clean reuse; the list/create carousel inside Tags.svelte follows the same pattern. Optimistic update + discardPendingListRefresh + server PATCH for tag edits is the right pattern.
One architectural concern worth raising before this is marked ready:
Encoding color as trailing-space padding on the tag string. tags.ts notes the trade-off explicitly:
A consequence is that two tags with the same visible text but different padding (
"foo"vs"foo ") are genuinely different tags with different colors; the UI keeps them distinct rather than collapsing them.
This is a clever workaround for "backend doesn't know about color," but it leaks in places we can't always control:
- Anything outside the web-console that touches
tags—fda, the Python SDK, rawPATCH /pipelines/:name, programmatic creation in tests — sees an opaque whitespace-bearing string. A user who types--tag prodin the CLI and expects to land on the same colored chip they made in the UI will not. - Filtering / search / equality / set operations on tags have to be "padded equality."
matchesSearchhappens to dotrim().toLowerCase()which is fine in isolation, but every future feature that touches tags has to remember the encoding. - Any HTTP/proxy layer or backend handler that normalizes whitespace (a
TRIM()in SQL, a.strip()in Python, a hand-written validator that rejects trailing spaces) silently breaks color. The encoding is invisible from the server side, so the server can't even warn about it. - The "+0 padding" tag (length already lands on Red by accident) is indistinguishable in storage from a tag the user genuinely meant to leave at the default — so re-color is one-way OK but the round-trip semantics are subtle.
Better long-term shapes:
- Backend stores
(tag, color)as a structured value ({name, color_index}orname#color_indexwith a documented separator that is not whitespace). Backend doesn't have to interpret color, but the field is named and the encoding is explicit. - Or: color is purely client-side, derived from a hash of the tag name (you almost already do this —
tag.length % paletteis just a very weak hash). Drop the "user picks color" feature; let the palette be deterministic. This is what GitHub labels-from-strings does, and it removes the whole encoding problem. - Or: store color in localStorage / user prefs, keyed by tag display name. Loses cross-user color consistency, gains a clean tag string.
If you have a specific reason for tying color to the persisted string (e.g. cross-user consistency without a backend round-trip for color), say so in the description so it's a deliberate choice; otherwise I'd avoid landing the padding trick.
Smaller things, save for full review when this is ready:
tagWithColorbuildspadEnd(base.length + padding)— fine, but the math((c - bL) + P) % Pis the kind of "trust me" arithmetic that screams for two assertions (padding < paletteSize,(base.length + padding) % paletteSize === colorIndex) or a unit test. Currently neither exists.Tags.sveltesetTagsfiresapi.patchPipeline(...)but does not await it or.catchit. If the PATCH fails the optimistic update is never reconciled —discardPendingListRefresh()is called eagerly, so the next list refresh won't immediately overwrite. Decide whether failures should toast or roll back.SlidingPanelsis a nice extraction. Consider moving it intocommon-uialongsideTooltip/Popupso the next caller doesn't import it frompipelines/-adjacent paths.
Not blocking anything since it's a draft; the encoding question is the only one I'd want resolved before ready-for-review.
Testing: manual