From 129c04559548cd1bcf67758ec416fb2a48e7b928 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 16:33:47 +0200 Subject: [PATCH] fix(cli): keep the workspace color when settings are synced from git (#11144) * fix(cli): keep the workspace color when settings are synced from git Co-Authored-By: Claude Fable 5.1 * docs(cli): name the sync direction consistently in the identity-field comments Co-Authored-By: Claude Fable 5.1 * fix(cli): apply the workspace color from settings.yaml only when the file sets one Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Claude Fable 5.1 --- cli/src/commands/sync/sync.ts | 21 +++++++ cli/src/core/settings.ts | 19 +++++-- cli/test/push_diff_convergence_unit.test.ts | 26 ++++++++- ..._workspace_settings_identity_unit.test.ts} | 55 +++++++++++++++++-- 4 files changed, 108 insertions(+), 13 deletions(-) rename cli/test/{push_workspace_settings_name_unit.test.ts => push_workspace_settings_identity_unit.test.ts} (54%) diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 287dd6ae12..38b9e87f84 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -2542,6 +2542,21 @@ export function preservePendingScriptLocks( } } +// `sync push` never applies the workspace's display name from settings.yaml and +// applies its color only when the local file carries one (see +// pushWorkspaceSettings), so on a push the fields it would not apply must +// compare equal, or the row is listed on every run. +const isWorkspaceSettingsFile = (p: string) => + /^settings(\.[^./\\]+)?\.(yaml|json)$/.test(p); +function stripUnappliedSettingsFields(local: any, remote: any) { + delete local?.name; + delete remote?.name; + if (local?.color == null) { + delete local?.color; + delete remote?.color; + } +} + export async function compareDynFSElement( els1: DynFSElement, els2: DynFSElement | undefined, @@ -2757,6 +2772,9 @@ export async function compareDynFSElement( delete parsedV?.enabled; delete parsedM2?.enabled; } + if (isEls1Remote === false && isWorkspaceSettingsFile(k)) { + stripUnappliedSettingsFields(parsedV, parsedM2); + } if (deepEqual(parsedV, parsedM2)) { continue; } @@ -2771,6 +2789,9 @@ export async function compareDynFSElement( delete before?.enabled; delete after?.enabled; } + if (isEls1Remote === false && isWorkspaceSettingsFile(k)) { + stripUnappliedSettingsFields(after, before); + } if (deepEqual(before, after)) { continue; } diff --git a/cli/src/core/settings.ts b/cli/src/core/settings.ts index 66fc77d230..4c95090bcf 100644 --- a/cli/src/core/settings.ts +++ b/cli/src/core/settings.ts @@ -214,9 +214,15 @@ export async function pushWorkspaceSettings( } // Exclude fields that are never applied here: slack_team_id/slack_name are OAuth-only, - // and name is not applied on pull (see below), so a name-only diff stays a no-op. + // and name is never applied (see below), so a name-only diff stays a no-op. color is + // applied only when the file carries it, so an unset one leaves the comparison too. const { slack_team_id: _lst, slack_name: _lsn, name: _ln, ...comparableLocal } = localSettings; const { slack_team_id: _rst, slack_name: _rsn, name: _rn, ...comparableRemote } = settings; + const colorManaged = localSettings.color != null; + if (!colorManaged) { + delete comparableLocal.color; + delete comparableRemote.color; + } if (isSuperset(comparableLocal, comparableRemote)) { log.debug(`Workspace settings are up to date`); return; @@ -351,10 +357,9 @@ export async function pushWorkspaceSettings( }); } - // Workspace display name is intentionally not applied on pull: settings.yaml is shared - // across a repo's branches, so applying it would let one workspace's name overwrite - // another's when both sync the same repo. It stays in the file (written on push), but a - // live workspace is only renamed by its owner. + // Workspace display name is intentionally never applied by `sync push`: settings.yaml is + // shared across a repo's branches, so applying it would let one workspace's name overwrite + // another's when both sync the same repo. `sync pull` still records it. if (localSettings.mute_critical_alerts != settings.mute_critical_alerts) { log.debug(`Updating mute critical alerts...`); @@ -366,7 +371,9 @@ export async function pushWorkspaceSettings( }); } - if (localSettings.color != settings.color) { + // A color is applied only when the file carries one: `sync pull` omits the key for a + // workspace without a color, so an unset key means "not managed by git", never "clear". + if (colorManaged && localSettings.color != settings.color) { log.debug(`Updating workspace color...`); await wmill.changeWorkspaceColor({ workspace, diff --git a/cli/test/push_diff_convergence_unit.test.ts b/cli/test/push_diff_convergence_unit.test.ts index 14e248c03e..67f6d5b4dd 100644 --- a/cli/test/push_diff_convergence_unit.test.ts +++ b/cli/test/push_diff_convergence_unit.test.ts @@ -64,6 +64,7 @@ async function diff( remoteEl: Mock, skips: Record, parentOwnsScheduleEnabled?: (scheduleFilePath: string) => boolean, + isEls1Remote = false, ) { const { changes } = await compareDynFSElement( localEl as any, @@ -76,7 +77,7 @@ async function diff( false, undefined, undefined, - false, + isEls1Remote, false, parentOwnsScheduleEnabled, ); @@ -296,3 +297,26 @@ test("push: checkout inline names stay inside the flow folder", async () => { await checkoutInlineNames(join(process.cwd(), "missing.yaml")), ).toEqual({}); }); + +// A push never applies the workspace's display name and applies its color only +// when the local file carries one (see pushWorkspaceSettings), so a file that +// differs only in what would not be applied is not a push change; a pull still +// rewrites the file. +test("push: settings.yaml differing only by name or an unset color is not a change", async () => { + const remote = local({ + "settings.yaml": "name: prod\ncolor: '#ff0000'\nerror_handler: null\n", + }); + const unsetColor = local({ + "settings.yaml": "name: staging\nerror_handler: null\n", + }); + const skips = { includeSettings: true }; + expect(await diff(unsetColor, remote, skips)).toEqual([]); + expect(await diff(remote, unsetColor, skips, undefined, true)).toEqual([ + "edited settings.yaml", + ]); + + const otherColor = local({ + "settings.yaml": "name: staging\ncolor: '#00ff00'\nerror_handler: null\n", + }); + expect(await diff(otherColor, remote, skips)).toEqual(["edited settings.yaml"]); +}); diff --git a/cli/test/push_workspace_settings_name_unit.test.ts b/cli/test/push_workspace_settings_identity_unit.test.ts similarity index 54% rename from cli/test/push_workspace_settings_name_unit.test.ts rename to cli/test/push_workspace_settings_identity_unit.test.ts index ef76ab7aec..c583a53282 100644 --- a/cli/test/push_workspace_settings_name_unit.test.ts +++ b/cli/test/push_workspace_settings_identity_unit.test.ts @@ -1,23 +1,32 @@ /** - * Regression guard: a pull (pushWorkspaceSettings) must not apply the workspace - * display name from settings.yaml. Rationale lives at the apply site in settings.ts. + * Regression guard: `sync push` (pushWorkspaceSettings) must never apply the + * workspace display name from settings.yaml, and must apply the color only when + * the file carries one. Rationale lives at the apply sites in settings.ts. */ import { expect, test, describe, beforeEach, mock } from "bun:test"; let changeWorkspaceNameCalls: unknown[] = []; +let changeWorkspaceColorCalls: unknown[] = []; let editWebhookCalls: unknown[] = []; let remoteName = ""; +let remoteColor: string | undefined = undefined; let remoteWebhook: string | undefined = undefined; // Every wmill.* call reachable from pushWorkspaceSettings is stubbed so the -// function runs without a backend; only the two we assert on record calls. +// function runs without a backend; only the three we assert on record calls. mock.module("../gen/services.gen.ts", () => ({ - getSettings: async (_a: { workspace: string }) => ({ webhook: remoteWebhook }), + getSettings: async (_a: { workspace: string }) => ({ + webhook: remoteWebhook, + color: remoteColor, + }), getWorkspaceName: async (_a: { workspace: string }) => remoteName, changeWorkspaceName: async (a: unknown) => { changeWorkspaceNameCalls.push(a); }, + changeWorkspaceColor: async (a: unknown) => { + changeWorkspaceColorCalls.push(a); + }, editWebhook: async (a: unknown) => { editWebhookCalls.push(a); }, @@ -30,7 +39,6 @@ mock.module("../gen/services.gen.ts", () => ({ editWorkspaceDefaultApp: async () => {}, editDefaultScripts: async () => {}, workspaceMuteCriticalAlertsUi: async () => {}, - changeWorkspaceColor: async () => {}, updateOperatorSettings: async () => {}, editDataTableConfig: async () => {}, editSlackCommand: async () => {}, @@ -40,13 +48,15 @@ mock.module("../gen/services.gen.ts", () => ({ const { pushWorkspaceSettings } = await import("../src/core/settings.ts"); -describe("pushWorkspaceSettings workspace name", () => { +describe("pushWorkspaceSettings workspace identity", () => { const ws = "phoenix"; beforeEach(() => { changeWorkspaceNameCalls = []; + changeWorkspaceColorCalls = []; editWebhookCalls = []; remoteName = "phoenix"; + remoteColor = undefined; remoteWebhook = undefined; }); @@ -69,4 +79,37 @@ describe("pushWorkspaceSettings workspace name", () => { expect(editWebhookCalls.length).toBe(0); expect(changeWorkspaceNameCalls.length).toBe(0); }); + + test("a settings.yaml without a color key does not clear the workspace color", async () => { + remoteColor = "#ff0000"; + remoteWebhook = "https://old"; + await pushWorkspaceSettings(ws, "settings", undefined, { + name: "phoenix", + webhook: "https://new", + }); + expect(editWebhookCalls.length).toBe(1); + expect(changeWorkspaceColorCalls.length).toBe(0); + }); + + test("a color in settings.yaml is applied when it differs from the workspace", async () => { + remoteColor = "#ff0000"; + await pushWorkspaceSettings(ws, "settings", undefined, { + name: "phoenix", + color: "#00ff00", + }); + expect(editWebhookCalls.length).toBe(0); + expect(changeWorkspaceColorCalls).toEqual([ + { workspace: ws, requestBody: { color: "#00ff00" } }, + ]); + }); + + test("a color matching the workspace is a complete no-op", async () => { + remoteColor = "#ff0000"; + await pushWorkspaceSettings(ws, "settings", undefined, { + name: "phoenix", + color: "#ff0000", + }); + expect(editWebhookCalls.length).toBe(0); + expect(changeWorkspaceColorCalls.length).toBe(0); + }); });