fix: address review findings on label colors

Key label rows by the verbatim name: item-side `labels text[]` stores whatever
was typed, so trimming the key colored a different label — `" release "` wrote a
row for `"release"` and the vocabulary then listed both. The trim now only
answers whether the name is blank.

Cache labels per workspace with a generation guard, so a response that lost the
race no longer replaces a newer workspace's entry, and thread the operating
workspace through `LabelsInput` instead of reading `$workspaceStore`: a session
editor works on a fork while the sidebar still browses the origin, and a color
must be written where the item lives.

Also: carry `label_settings` into a forked workspace, bound the name length
server-side, refetch the vocabulary on every suggestion-list open, and drop the
banned `$bindable(false)` on an optional prop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QNUCuRqmzNukjR7FDewN7W
This commit is contained in:
Guilhem Lemouel
2026-08-26 17:57:25 +02:00
co-authored by Claude Opus 5
parent dd55b1720a
commit a20d037020
21 changed files with 168 additions and 84 deletions
@@ -0,0 +1,15 @@
{
"db_name": "PostgreSQL",
"query": "INSERT INTO label_settings (workspace_id, name, color)\n SELECT $2, name, color\n FROM label_settings\n WHERE workspace_id = $1",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text",
"Varchar"
]
},
"nullable": []
},
"hash": "0d09c68d21552ca98d31e5c1dc468a76a3cc6d19974be318944953c54ca86f3d"
}
@@ -1,22 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "SELECT DISTINCT unnest(labels) as \"label!\" FROM (\n SELECT labels FROM script WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM flow WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM resource WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM variable WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM schedule WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM app WHERE workspace_id = $1 AND labels IS NOT NULL\n UNION ALL SELECT labels FROM folder WHERE workspace_id = $1 AND labels IS NOT NULL\n ) t ORDER BY 1",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "label!",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text"
]
},
"nullable": [
null
]
},
"hash": "7fc4dcc5f46da2a1ac74bf80237a6ae28f1b817b2e1ff275a3f470bb88fc500c"
}
+2
View File
@@ -128,6 +128,8 @@ job_stats: workspace_id(char), job_id(uuid), metric_id(char), metric_name(char),
kafka_pending_commits: id(bigint), workspace_id(char), kafka_trigger_path(char), topic(char), partition(int), offset(bigint), created_at(ts)
FK: (workspace_id, kafka_trigger_path) -> kafka_trigger(workspace_id, path)
kafka_trigger: path(char), kafka_resource_path(char), topics(char), group_id(char), script_path(char), is_flow(bool), workspace_id(char), edited_by(char), email(char), edited_at(ts), extra_perms(jsonb), server_id(char), last_server_ping(ts), error(text), error_handler_path(char), error_handler_args(jsonb), retry(jsonb), mode(trigger_mode), filters(jsonb[]), auto_commit(bool), labels(text[])
label_settings: workspace_id(char), name(text), color(char)
FK: (workspace_id) -> workspace(id)
log_file: hostname(char), log_ts(ts), ok_lines(bigint), err_lines(bigint), mode(log_mode), worker_group(char), file_path(char), json_fmt(bool)
macro_definition: workspace_id(char), name(char), provider_path(char), params(text), body(text), is_table_macro(bool), created_at(ts)
FK: (workspace_id) -> workspace(id)
@@ -5456,6 +5456,10 @@ async fn clone_workspace_data(
// Clone folders
clone_folders(tx, source_workspace_id, target_workspace_id).await?;
// Clone label colours. Folders and items carry their `labels` arrays above;
// without this the fork keeps every label and loses every colour.
clone_label_settings(tx, source_workspace_id, target_workspace_id).await?;
// Clone groups
clone_groups(tx, source_workspace_id, target_workspace_id).await?;
@@ -5917,6 +5921,25 @@ async fn clone_folders(
Ok(())
}
async fn clone_label_settings(
tx: &mut Transaction<'_, Postgres>,
source_workspace_id: &str,
target_workspace_id: &str,
) -> Result<()> {
sqlx::query!(
"INSERT INTO label_settings (workspace_id, name, color)
SELECT $2, name, color
FROM label_settings
WHERE workspace_id = $1",
source_workspace_id,
target_workspace_id,
)
.execute(&mut **tx)
.await?;
Ok(())
}
async fn clone_groups(
tx: &mut Transaction<'_, Postgres>,
source_workspace_id: &str,
+22 -5
View File
@@ -19,13 +19,20 @@ use windmill_common::{
error::{Error, JsonResult, Result},
};
/// Mirrors `NoteColor` in `frontend/src/lib/components/graph/labelColors.ts`.
/// Kept as a list rather than an enum so an unknown value coming back from an
/// older row is a rejected write, not a failed deserialization of the whole row.
/// Mirrors the `LabelColor` enum in `openapi.yaml`, from which
/// `frontend/src/lib/components/labels/labelColors.ts` is generated. Kept as a
/// list rather than an enum so an unknown value coming back from an older row is
/// a rejected write, not a failed deserialization of the whole row.
const PALETTE: [&str; 10] = [
"yellow", "blue", "green", "purple", "pink", "orange", "red", "cyan", "lime", "gray",
];
/// The column is TEXT so the registry never caps what a label may be called, but
/// a write still has to be bounded: the handler runs on the privileged pool, and
/// every row it accepts is returned to every member by `labels/list`. Well above
/// the 50 characters the label input allows.
const MAX_NAME_LEN: usize = 255;
pub fn workspaced_service() -> Router {
Router::new()
.route("/list", get(list_workspace_labels))
@@ -97,10 +104,20 @@ async fn update_label(
}
check_scopes(&authed, || "labels:write".to_string())?;
let name = nl.name.trim();
if name.is_empty() {
// The name keys a label that already exists verbatim in some `labels text[]`,
// which stores whatever was typed. Trimming here would colour a *different*
// label — `" release "` would write a row for `"release"` — so the raw name is
// the key and the trim only answers whether it is blank.
let name = nl.name.as_str();
if name.trim().is_empty() {
return Err(Error::BadRequest("Label name cannot be empty".to_string()));
}
if name.chars().count() > MAX_NAME_LEN {
return Err(Error::BadRequest(format!(
"Label name cannot exceed {} characters",
MAX_NAME_LEN
)));
}
match nl.color.as_deref() {
Some(color) => {
@@ -1255,7 +1255,7 @@
kind="resource"
/>
</Label>
<LabelsInput bind:labels class="-mt-5" />
<LabelsInput bind:labels workspace={$workspaceStore} class="-mt-5" />
{#if deployTo}
<Label
label="Workspace specific"
@@ -1379,7 +1379,7 @@
/>
{/if}
<LabelsInput bind:labels class="-mt-5" />
<LabelsInput bind:labels workspace={$workspaceStore} class="-mt-5" />
{#if supportsClientCredentials}
<div class="flex flex-col gap-1">
@@ -1507,7 +1507,7 @@
kind="resource"
/>
</Label>
<LabelsInput bind:labels class="-mt-5" />
<LabelsInput bind:labels workspace={$workspaceStore} class="-mt-5" />
{#if deployTo}
<Label
label="Workspace specific"
@@ -306,7 +306,7 @@
are labeled with them.
</div>
{#if can_write}
<LabelsInput bind:labels onchange={saveLabels} />
<LabelsInput bind:labels workspace={$workspaceStore} onchange={saveLabels} />
{:else}
<div class="inline-flex items-center gap-1 h-5">
{#each labels ?? [] as label (label)}
+18 -12
View File
@@ -1,20 +1,25 @@
<script lang="ts">
import Button from './common/button/Button.svelte'
import { Plus, Tag, X } from 'lucide-svelte'
import { workspaceStore } from '$lib/stores'
import type { Label, LabelColor } from '$lib/gen'
import LabelBadge from './labels/LabelBadge.svelte'
import LabelColorPicker from './labels/LabelColorPicker.svelte'
import { labelColorCache, labelColorOf, loadLabels, setLabelColor } from './labels/labelStore'
import { labelCache, labelColorOf, loadLabels, setLabelColor } from './labels/labelStore'
import { LABEL_COLOR_SWATCHES } from './labels/labelColors'
interface Props {
labels: string[] | undefined
/**
* The workspace the edited item lives in. Not always the navigated one — a
* session editor operates on a fork while the sidebar still browses the
* origin — so a color picked here must be written where the item is.
*/
workspace: string | undefined
onchange?: () => void
class?: string
}
let { labels = $bindable(), onchange, class: clazz = '' }: Props = $props()
let { labels = $bindable(), workspace, onchange, class: clazz = '' }: Props = $props()
let adding = $state(false)
let inputValue = $state('')
@@ -41,23 +46,24 @@
// The chips need colors before the suggestion list is ever opened, and
// `loadLabels` is cached per workspace, so this costs one request per workspace.
$effect(() => {
const workspace = $workspaceStore
if (workspace) {
loadLabels(workspace).catch(() => {})
}
})
async function loadExistingLabels(force = false) {
if (!$workspaceStore) return
// Always refetches: labels appear the moment anyone types a new one onto an
// item, so a cached vocabulary would stop suggesting labels created since the
// page loaded.
async function loadExistingLabels() {
if (!workspace) return
try {
existingLabels = await loadLabels($workspaceStore, force)
existingLabels = await loadLabels(workspace, true)
} catch {}
}
async function pickColor(name: string, color: LabelColor | undefined) {
if (!$workspaceStore) return
await setLabelColor($workspaceStore, name, color)
await loadExistingLabels(true)
if (!workspace) return
existingLabels = await setLabelColor(workspace, name, color)
}
function startAdding() {
@@ -125,13 +131,13 @@
{#each labels ?? [] as label (label)}
<div class="inline-flex items-center">
<LabelColorPicker
color={labelColorOf($labelColorCache, $workspaceStore, label)}
color={labelColorOf($labelCache, workspace, label)}
onSelect={(c) => pickColor(label, c)}
>
{#snippet anchor()}
<LabelBadge
{label}
workspace={$workspaceStore}
{workspace}
title="Pick a color for {label}"
class="rounded-r-none pr-0.5"
/>
@@ -168,7 +168,7 @@
</Label>
</div>
{/if}
<LabelsInput bind:labels class="-mt-4" />
<LabelsInput bind:labels workspace={$workspaceStore} class="-mt-4" />
{#if deployTo}
<Label
@@ -1258,7 +1258,7 @@
}}
/>
</Label>
<LabelsInput bind:labels={script.labels} class="-mt-4" />
<LabelsInput bind:labels={script.labels} workspace={opWorkspace} class="-mt-4" />
<Label label="Path">
{#snippet header()}
<Tooltip
@@ -17,6 +17,12 @@
path?: string
labels?: string[] | undefined
inheritedLabels?: string[] | undefined
/**
* The workspace holding the item. Defaults to the navigated one, which is
* also where the rest of this component saves; pass it explicitly from a
* caller that operates on a different workspace.
*/
workspace?: string | undefined
editable?: boolean
onSaved?: (newPath: string) => void
kind?: 'flow' | 'script'
@@ -27,11 +33,14 @@
path = $bindable(''),
labels = $bindable(),
inheritedLabels = undefined,
workspace = undefined,
editable = false,
onSaved,
kind = 'flow'
}: Props = $props()
let labelWorkspace = $derived(workspace ?? $workspaceStore)
let editSummary = $state('')
let editPath = $state('')
let dirtyPath = $state(false)
@@ -112,11 +121,11 @@
{#if labels?.length}
<div class="flex items-center gap-0.5">
{#each labels as label}
<LabelBadge {label} workspace={$workspaceStore} size="verySmall" />
<LabelBadge {label} workspace={labelWorkspace} size="verySmall" />
{/each}
</div>
{/if}
<InheritedLabels labels={inheritedLabels} workspace={$workspaceStore} />
<InheritedLabels labels={inheritedLabels} workspace={labelWorkspace} />
</div>
</div>
{/snippet}
@@ -141,12 +150,13 @@
<div class="-mt-4 flex items-center gap-2">
<LabelsInput
bind:labels
workspace={labelWorkspace}
onchange={() => {
labelsDirty = true
}}
/>
{#if inheritedLabels?.length}
<InheritedLabels labels={inheritedLabels} workspace={$workspaceStore} />
<InheritedLabels labels={inheritedLabels} workspace={labelWorkspace} />
{/if}
</div>
{#if inheritedLabels?.length}
@@ -79,7 +79,7 @@
kind="variable"
workspaceOverride={workspace}
/>
<LabelsInput bind:labels />
<LabelsInput bind:labels workspace={$workspaceStore} />
</div>
<label class="flex flex-col gap-1">
<span class="text-xs font-semibold text-emphasis">Secret</span>
@@ -214,7 +214,7 @@
/>
</div>
<div class="pt-3"></div>
<LabelsInput bind:labels class="-mt-4" />
<LabelsInput bind:labels workspace={opWs} class="-mt-4" />
<div class="py-6"></div>
<label for="deploymentMsg" class="text-emphasis text-xs font-semibold">Deployment message</label>
<div class="w-full pt-1">
@@ -1,4 +1,7 @@
<script lang="ts" generics="T extends string">
import { twMerge } from 'tailwind-merge'
import Button from '$lib/components/common/button/Button.svelte'
interface Props {
colors: readonly T[]
swatches: Record<T, string>
@@ -13,13 +16,17 @@
<div class="grid grid-cols-5 gap-1" style="min-width: 140px">
{#each colors as color (color)}
<button
class="w-6 h-6 rounded-full hover:scale-110 transition-transform duration-100 {swatches[
color
]} {selected === color ? 'ring-2 ring-accent' : 'dark:border-gray-600'}"
onclick={() => onSelect(color)}
<Button
variant="subtle"
unifiedSize="2xs"
title={color.charAt(0).toUpperCase() + color.slice(1)}
aria-label={`Select ${color} color`}
></button>
onclick={() => onSelect(color)}
btnClasses={twMerge(
'w-6 h-6 p-0 rounded-full hover:scale-110 transition-transform duration-100',
swatches[color],
selected === color ? 'ring-2 ring-accent' : 'dark:border-gray-600'
)}
/>
{/each}
</div>
@@ -143,7 +143,11 @@
/>
</Label>
<!-- prettier-ignore -->
<LabelsInput bind:labels={(flowStore.val as any).labels} class="-mt-4" />
<LabelsInput
bind:labels={(flowStore.val as any).labels}
workspace={opWorkspace?.() ?? $workspaceStore}
class="-mt-4"
/>
{#if !noEditor && customUi?.topBar?.editablePath != false}
<Label label="Path">
@@ -4,7 +4,7 @@
import { twMerge } from 'tailwind-merge'
import Badge from '$lib/components/common/badge/Badge.svelte'
import { labelBadgeColor } from './labelColors'
import { labelColorCache, labelColorOf } from './labelStore'
import { labelCache, labelColorOf } from './labelStore'
interface Props {
label: string
@@ -37,7 +37,7 @@
children
}: Props = $props()
let color = $derived(labelBadgeColor(labelColorOf($labelColorCache, workspace, label)))
let color = $derived(labelBadgeColor(labelColorOf($labelCache, workspace, label)))
</script>
<Badge
@@ -11,10 +11,11 @@
/** `undefined` clears the color and drops the label's row. */
onSelect: (color: LabelColor | undefined) => void
anchor: Snippet
isOpen?: boolean
}
let { color, onSelect, anchor, isOpen = $bindable(false) }: Props = $props()
let { color, onSelect, anchor }: Props = $props()
let isOpen = $state(false)
function pick(next: LabelColor | undefined) {
isOpen = false
@@ -31,8 +31,7 @@
async function pick(name: string, color: LabelColor | undefined) {
if (!workspace) return
try {
await setLabelColor(workspace, name, color)
await refresh()
labels = await setLabelColor(workspace, name, color)
} catch (err) {
sendUserToast(`Could not update label ${name}: ${err}`, true)
}
@@ -1,8 +1,14 @@
import { derived, get, writable } from 'svelte/store'
import { get, writable } from 'svelte/store'
import { LabelService, type Label, type LabelColor } from '$lib/gen'
type LabelColorMap = Record<string, LabelColor>
interface WorkspaceLabels {
workspace: string
labels: Label[]
colors: LabelColorMap
}
/**
* Colors are workspace-scoped, but a label chip is handed a name and nothing
* else — a run row, an inherited-labels list, a picker in a session targeting
@@ -10,14 +16,16 @@ type LabelColorMap = Record<string, LabelColor>
* check it, so a chip rendered for a workspace we have not loaded falls back to
* the default color instead of borrowing the navigation workspace's.
*/
const cache = writable<{ workspace: string; colors: LabelColorMap } | undefined>(undefined)
const cache = writable<Record<string, WorkspaceLabels>>({})
export const labelColorCache = { subscribe: cache.subscribe }
export const labelCache = { subscribe: cache.subscribe }
/** All labels of a workspace, colored or not, as `labels/list` returns them. */
export const labelList = writable<{ workspace: string; labels: Label[] } | undefined>(undefined)
export const labelNames = derived(labelList, (l) => l?.labels.map((label) => label.name) ?? [])
/**
* Counts the loads started per workspace so a response that lost the race is
* dropped rather than overwriting a newer one. Without it, a slow request for
* workspace A finishing after a fast one for A leaves the older list in place.
*/
const generation: Record<string, number> = {}
function toColorMap(labels: Label[]): LabelColorMap {
return Object.fromEntries(
@@ -26,36 +34,50 @@ function toColorMap(labels: Label[]): LabelColorMap {
}
export async function loadLabels(workspace: string, force = false): Promise<Label[]> {
const cached = get(labelList)
if (!force && cached?.workspace === workspace) {
const cached = get(cache)[workspace]
if (!force && cached != undefined) {
return cached.labels
}
const started = (generation[workspace] = (generation[workspace] ?? 0) + 1)
const labels = await LabelService.listLabels({ workspace })
labelList.set({ workspace, labels })
cache.set({ workspace, colors: toColorMap(labels) })
if (started !== generation[workspace]) {
// A newer load for this workspace already landed; keep what it wrote.
return get(cache)[workspace]?.labels ?? labels
}
cache.update((c) => ({ ...c, [workspace]: { workspace, labels, colors: toColorMap(labels) } }))
return labels
}
/** Drops a workspace's entry so the next read refetches. */
export function invalidateLabels(workspace: string): void {
generation[workspace] = (generation[workspace] ?? 0) + 1
cache.update((c) => {
const { [workspace]: _dropped, ...rest } = c
return rest
})
}
/**
* `undefined` both when the label has no color and when the cache holds another
* workspace — the caller renders the default in either case.
* `undefined` both when the label has no color and when that workspace is not
* loaded — the caller renders the default in either case.
*/
export function labelColorOf(
entry: { workspace: string; colors: LabelColorMap } | undefined,
entries: Record<string, WorkspaceLabels>,
workspace: string | undefined,
name: string
): LabelColor | undefined {
if (entry == undefined || workspace == undefined || entry.workspace !== workspace) {
if (workspace == undefined) {
return undefined
}
return entry.colors[name]
return entries[workspace]?.colors[name]
}
/** Returns the workspace's labels as they stand after the write. */
export async function setLabelColor(
workspace: string,
name: string,
color: LabelColor | undefined
): Promise<void> {
): Promise<Label[]> {
await LabelService.updateLabel({ workspace, requestBody: { name, color: color ?? null } })
await loadLabels(workspace, true)
return await loadLabels(workspace, true)
}
@@ -7,7 +7,7 @@
import type { Item } from '$lib/utils'
import { badgeColors, badgeSelectedColors } from '$lib/components/common/badge/model'
import { labelBadgeColor } from '$lib/components/labels/labelColors'
import { labelColorCache, labelColorOf } from '$lib/components/labels/labelStore'
import { labelCache, labelColorOf } from '$lib/components/labels/labelStore'
interface Props {
job: Job
@@ -66,7 +66,7 @@
// The chips keep their own geometry — the overflow split above is computed from
// LABEL_MAX_WIDTH — so they take only the color half of the badge styling.
function chipClass(label: string): string {
const color = labelBadgeColor(labelColorOf($labelColorCache, job?.workspace_id, label))
const color = labelBadgeColor(labelColorOf($labelCache, job?.workspace_id, label))
return activeLabel == label ? badgeSelectedColors[color] : badgeColors[color]
}
@@ -821,7 +821,7 @@
bind:value={summary}
/>
</label>
<LabelsInput bind:labels class="-mt-4" />
<LabelsInput bind:labels workspace={$workspaceStore} class="-mt-4" />
<div class="flex flex-col gap-1">
<label for="path" class="text-xs font-semibold text-emphasis">Path</label>