Skip to content

[web-console] Implement pipeline tags UI#6506

Draft
Karakatiza666 wants to merge 1 commit into
mainfrom
tags-ui
Draft

[web-console] Implement pipeline tags UI#6506
Karakatiza666 wants to merge 1 commit into
mainfrom
tags-ui

Conversation

@Karakatiza666

Copy link
Copy Markdown
Contributor

Testing: manual

Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>

@mythical-fred mythical-fred left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Anything outside the web-console that touches tagsfda, the Python SDK, raw PATCH /pipelines/:name, programmatic creation in tests — sees an opaque whitespace-bearing string. A user who types --tag prod in the CLI and expects to land on the same colored chip they made in the UI will not.
  2. Filtering / search / equality / set operations on tags have to be "padded equality." matchesSearch happens to do trim().toLowerCase() which is fine in isolation, but every future feature that touches tags has to remember the encoding.
  3. 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.
  4. 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} or name#color_index with 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 % palette is 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:

  • tagWithColor builds padEnd(base.length + padding) — fine, but the math ((c - bL) + P) % P is 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.svelte setTags fires api.patchPipeline(...) but does not await it or .catch it. 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.
  • SlidingPanels is a nice extraction. Consider moving it into common-ui alongside Tooltip/Popup so the next caller doesn't import it from pipelines/-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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants