From 34f7f56714df93c3a83049b8e8ace33d9ee09403 Mon Sep 17 00:00:00 2001 From: nova28 Date: Sat, 22 Aug 2026 15:55:19 +0000 Subject: [PATCH] fix(office): persist workspace name and description edits on save (#2859) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- apps/backend/internal/office/dashboard/dto.go | 2 - .../components/settings-content.test.tsx | 314 +++++++++++++++ .../settings/components/settings-content.tsx | 365 +++++++++++++----- apps/web/e2e/tests/office/dashboard.spec.ts | 24 +- .../office/workspace-settings-ui.spec.ts | 38 ++ .../lib/api/domains/office-extended-api.ts | 2 - apps/web/src/office-routes.tsx | 9 +- 7 files changed, 645 insertions(+), 109 deletions(-) create mode 100644 apps/web/app/office/workspace/settings/components/settings-content.test.tsx diff --git a/apps/backend/internal/office/dashboard/dto.go b/apps/backend/internal/office/dashboard/dto.go index b6eb6e49f4..b0aefe1f4e 100644 --- a/apps/backend/internal/office/dashboard/dto.go +++ b/apps/backend/internal/office/dashboard/dto.go @@ -449,8 +449,6 @@ type CreateCommentRequest struct { // UpdateWorkspaceSettingsRequest is the request body for updating workspace settings. type UpdateWorkspaceSettingsRequest struct { - Name *string `json:"name"` - Description *string `json:"description"` PermissionHandlingMode *string `json:"permission_handling_mode"` RecoveryLookbackHours *int `json:"recovery_lookback_hours"` RequireApprovalForNewAgents *bool `json:"require_approval_for_new_agents"` diff --git a/apps/web/app/office/workspace/settings/components/settings-content.test.tsx b/apps/web/app/office/workspace/settings/components/settings-content.test.tsx new file mode 100644 index 0000000000..8f5c66a03a --- /dev/null +++ b/apps/web/app/office/workspace/settings/components/settings-content.test.tsx @@ -0,0 +1,314 @@ +import { act, cleanup, fireEvent, renderHook, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useSettingsState } from "./settings-content"; +import { updateWorkspaceAction } from "@/app/actions/workspaces"; +import { getWorkspaceSettings } from "@/lib/api/domains/office-api"; +import type { WorkspaceState } from "@/lib/state/slices/workspace/types"; +import { + SettingsSaveProvider, + useSettingsSaveCoordinator, +} from "@/components/settings/settings-save-provider"; + +vi.mock("@/app/actions/workspaces", () => ({ + updateWorkspaceAction: vi.fn(), +})); + +vi.mock("@/lib/api/domains/office-api", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, updateWorkspaceSettings: vi.fn(), getWorkspaceSettings: vi.fn() }; +}); + +const mockUpdateWorkspaceAction = vi.mocked(updateWorkspaceAction); +const mockGetWorkspaceSettings = vi.mocked(getWorkspaceSettings); + +type Workspace = WorkspaceState["items"][number]; + +const WORKSPACE_ID = "ws-1"; +const NEW_DESCRIPTION = "New description"; +const REMOTE_NAME = "Remote Name"; +const REMOTE_DESCRIPTION = "Remote description"; + +function workspace(overrides: Partial = {}): Workspace { + return { + id: WORKSPACE_ID, + name: "Original Name", + description: "Original description", + owner_id: "user-1", + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + ...overrides, + }; +} + +// Mirrors the slice of `StoreApi` the appearance-save handler +// reads: a live `getState()` accessor, not a snapshot passed at render time. +// This is what lets the test below prove a concurrent store write survives. +function makeStoreApi(initialItems: Workspace[], onSetWorkspaces?: (items: Workspace[]) => void) { + let items = initialItems; + const setWorkspaces = vi.fn((next: Workspace[]) => { + items = next; + onSetWorkspaces?.(next); + }); + return { + storeApi: { + getState: () => ({ workspaces: { items, activeId: items[0]?.id ?? null }, setWorkspaces }), + }, + setWorkspaces, + getItems: () => items, + setItems: (next: Workspace[]) => { + items = next; + }, + }; +} + +function renderSettings( + activeWorkspace: Workspace, + storeApi: ReturnType["storeApi"], +) { + return renderHook(({ ws }: { ws: Workspace }) => useSettingsState(ws, storeApi), { + initialProps: { ws: activeWorkspace }, + wrapper: SettingsSaveProvider, + }); +} + +function renderCoordinatedSettings( + activeWorkspace: Workspace, + storeApi: ReturnType["storeApi"], +) { + return renderHook( + ({ ws }: { ws: Workspace }) => { + const settings = useSettingsState(ws, storeApi); + const coordinator = useSettingsSaveCoordinator(); + return { settings, coordinator }; + }, + { initialProps: { ws: activeWorkspace }, wrapper: SettingsSaveProvider }, + ); +} + +// Combines the two setup calls every test needs: a store double plus the +// hook rendered against it. +function renderWithStore(items: Workspace[], onSetWorkspaces?: (items: Workspace[]) => void) { + const store = makeStoreApi(items, onSetWorkspaces); + return { ...store, ...renderSettings(items[0], store.storeApi) }; +} + +async function saveAppearance(result: ReturnType["result"]) { + await act(async () => { + await result.current.handleSaveAppearance(); + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + mockGetWorkspaceSettings.mockResolvedValue({ settings: {} } as never); +}); + +afterEach(cleanup); + +describe("useSettingsState appearance save", () => { + it("saves the trimmed name and description through updateWorkspaceAction and syncs the store", async () => { + const ws = workspace(); + const { setWorkspaces, result, rerender } = renderWithStore([ws]); + mockUpdateWorkspaceAction.mockResolvedValue({ + ...ws, + name: "Renamed", + description: NEW_DESCRIPTION, + } as never); + + act(() => { + result.current.setName(" Renamed "); + result.current.setDescription(NEW_DESCRIPTION); + }); + + await saveAppearance(result); + + expect(mockUpdateWorkspaceAction).toHaveBeenCalledWith(WORKSPACE_ID, { + name: "Renamed", + description: NEW_DESCRIPTION, + }); + expect(setWorkspaces).toHaveBeenCalledWith([ + { ...ws, name: "Renamed", description: NEW_DESCRIPTION }, + ]); + + // The submitted draft had leading/trailing whitespace; the echo guard + // must compare against that raw draft, not its trimmed form, or the + // input keeps the whitespace forever and the dirty flag never clears. + rerender({ ws: { ...ws, name: "Renamed", description: NEW_DESCRIPTION } }); + expect(result.current.name).toBe("Renamed"); + expect(result.current.appearanceDirty).toBe(false); + }); + + it("clears the dirty flag once the store reflects the saved workspace", async () => { + const { getItems, result, rerender } = renderWithStore([workspace()]); + mockUpdateWorkspaceAction.mockResolvedValue({ ...workspace(), name: "Renamed" } as never); + + act(() => result.current.setName("Renamed")); + expect(result.current.appearanceDirty).toBe(true); + + await saveAppearance(result); + + // Simulate the Zustand store propagating the update back into + // `activeWorkspace`, the way SettingsContent's subscription would on the + // next render. + rerender({ ws: getItems()[0] }); + + expect(result.current.appearanceDirty).toBe(false); + }); + + it("refuses to save an all-whitespace name", async () => { + const { setWorkspaces, result } = renderWithStore([workspace()]); + + act(() => result.current.setName(" ")); + + await saveAppearance(result); + + expect(mockUpdateWorkspaceAction).not.toHaveBeenCalled(); + expect(setWorkspaces).not.toHaveBeenCalled(); + }); + + // A newer keystroke landing before the PATCH from an older one resolves + // must survive that response, not get overwritten by the stale echo. + it("keeps a draft typed during an in-flight save instead of reverting it", async () => { + const ws = workspace(); + let resolveUpdate!: (value: Workspace) => void; + mockUpdateWorkspaceAction.mockImplementation( + () => new Promise((resolve) => (resolveUpdate = resolve)) as never, + ); + + const { result } = renderWithStore([ws]); + act(() => result.current.setName("Alpha")); + let savePromise!: Promise; + act(() => { + savePromise = result.current.handleSaveAppearance(); + }); + act(() => result.current.setName("Alpha2")); + + await act(async () => { + resolveUpdate({ ...ws, name: "Alpha" }); + await savePromise; + }); + + expect(result.current.name).toBe("Alpha2"); + }); + + // A workspace.deleted WS event elsewhere can reassign the active workspace + // without this page remounting; the draft must reset to the new one's values. + it("resets the draft when the active workspace's identity changes", async () => { + const { result, rerender } = renderWithStore([workspace()]); + act(() => result.current.setName("Unsaved Draft")); + expect(result.current.appearanceDirty).toBe(true); + rerender({ ws: workspace({ id: "ws-2", name: "Other Workspace", description: "Other desc" }) }); + expect(result.current.name).toBe("Other Workspace"); + expect(result.current.description).toBe("Other desc"); + expect(result.current.appearanceDirty).toBe(false); + }); + + it("does not clobber a workspace added to the store while the save was in flight", async () => { + const ws = workspace(); + const concurrent = workspace({ id: "ws-2", name: "Concurrent Workspace" }); + const { setWorkspaces, setItems, result } = renderWithStore([ws]); + + // Simulate a `workspace.created` WS event landing mid-PATCH: the store + // gains a second workspace between render (when the old code captured + // its snapshot) and the moment the save handler actually reads state. + mockUpdateWorkspaceAction.mockImplementation(async () => { + setItems([ws, concurrent]); + return { ...ws, name: "Renamed", description: NEW_DESCRIPTION } as never; + }); + + act(() => { + result.current.setName("Renamed"); + result.current.setDescription(NEW_DESCRIPTION); + }); + + await saveAppearance(result); + + expect(setWorkspaces).toHaveBeenCalledWith([ + { ...ws, name: "Renamed", description: NEW_DESCRIPTION }, + concurrent, + ]); + }); +}); + +describe("coordinated appearance save", () => { + it("registers appearance drafts with the shared save coordinator", async () => { + const ws = workspace(); + const store = makeStoreApi([ws]); + const { result } = renderCoordinatedSettings(ws, store.storeApi); + mockUpdateWorkspaceAction.mockResolvedValue({ ...ws, name: "Renamed" } as never); + + act(() => result.current.settings.setName("Renamed")); + + expect(result.current.coordinator.hasDirty).toBe(true); + await act(async () => { + await expect(result.current.coordinator.saveAll()).resolves.toMatchObject({ + canLeave: true, + }); + }); + + expect(mockUpdateWorkspaceAction).toHaveBeenCalledWith(WORKSPACE_ID, { + name: "Renamed", + description: ws.description, + }); + expect(store.setWorkspaces).toHaveBeenCalled(); + }); + + it("propagates save failures after clearing saving state", async () => { + mockUpdateWorkspaceAction.mockRejectedValue(new Error("network error")); + const { setWorkspaces, result } = renderWithStore([workspace()]); + + act(() => result.current.setName("Changed")); + await expect( + act(async () => { + await result.current.handleSaveAppearance(); + }), + ).rejects.toThrow("network error"); + + expect(setWorkspaces).not.toHaveBeenCalled(); + expect(result.current.savingAppearance).toBe(false); + }); + + it("resets the appearance draft to the authoritative workspace baseline", async () => { + const ws = workspace(); + const { result } = renderCoordinatedSettings(ws, makeStoreApi([ws]).storeApi); + + act(() => { + result.current.settings.setName("Unsaved name"); + result.current.settings.setDescription("Unsaved description"); + }); + + expect(screen.getByTestId("settings-floating-save")).toBeTruthy(); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /reset/i })); + }); + + expect(result.current.settings.name).toBe(ws.name); + expect(result.current.settings.description).toBe(ws.description); + expect(result.current.settings.appearanceDirty).toBe(false); + }); +}); + +describe("same-workspace appearance updates", () => { + it("syncs an untouched draft when the same workspace receives a remote update", () => { + const ws = workspace(); + const { result, rerender } = renderWithStore([ws]); + + rerender({ ws: { ...ws, name: REMOTE_NAME, description: REMOTE_DESCRIPTION } }); + + expect(result.current.name).toBe(REMOTE_NAME); + expect(result.current.description).toBe(REMOTE_DESCRIPTION); + expect(result.current.appearanceDirty).toBe(false); + }); + + it("preserves edited fields while syncing untouched fields from a remote update", () => { + const ws = workspace(); + const { result, rerender } = renderWithStore([ws]); + act(() => result.current.setName("Local Draft")); + + rerender({ ws: { ...ws, name: REMOTE_NAME, description: REMOTE_DESCRIPTION } }); + + expect(result.current.name).toBe("Local Draft"); + expect(result.current.description).toBe(REMOTE_DESCRIPTION); + expect(result.current.appearanceDirty).toBe(true); + }); +}); diff --git a/apps/web/app/office/workspace/settings/components/settings-content.tsx b/apps/web/app/office/workspace/settings/components/settings-content.tsx index 81f8b6ee21..45e3e6f0bd 100644 --- a/apps/web/app/office/workspace/settings/components/settings-content.tsx +++ b/apps/web/app/office/workspace/settings/components/settings-content.tsx @@ -1,14 +1,25 @@ "use client"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { + useCallback, + useEffect, + useRef, + useState, + type Dispatch, + type SetStateAction, +} from "react"; +import type { TFunction } from "i18next"; import Image from "@/components/routing/app-image"; import { IconUpload, IconDeviceFloppy } from "@tabler/icons-react"; import { toast } from "@/lib/toast/sonner"; import { Input } from "@kandev/ui/input"; import { Switch } from "@kandev/ui/switch"; import { Button } from "@kandev/ui/button"; -import { useAppStore } from "@/components/state-provider"; +import { useAppStore, useAppStoreApi } from "@/components/state-provider"; +import { useSettingsSaveContributor } from "@/components/settings/settings-save-provider"; import { updateWorkspaceSettings, getWorkspaceSettings } from "@/lib/api/domains/office-api"; +import { updateWorkspaceAction } from "@/app/actions/workspaces"; +import type { AppState } from "@/lib/state/store"; import type { WorkspaceState } from "@/lib/state/slices/workspace/types"; import { ConfigSection } from "./config-section"; import { DangerZoneSection } from "./danger-zone-section"; @@ -58,24 +69,18 @@ function AppearanceSection({ logoPreview, initial, fileInputRef, - dirty, - saving, onNameChange, onDescriptionChange, onLogoChange, - onSave, }: { name: string; description: string; logoPreview: string | null; initial: string; fileInputRef: React.RefObject; - dirty: boolean; - saving: boolean; onNameChange: (v: string) => void; onDescriptionChange: (v: string) => void; onLogoChange: (e: React.ChangeEvent) => void; - onSave: () => void; }) { const { t } = useTranslation(); return ( @@ -133,14 +138,6 @@ function AppearanceSection({ className="mt-1" /> - {dirty && ( -
- -
- )} ); } @@ -249,6 +246,147 @@ function RecoverySection({ type Workspace = WorkspaceState["items"][number]; +type WorkspaceStoreApi = { + getState: () => Pick; +}; + +type SaveAppearanceOptions = { + activeWorkspace: Workspace | undefined; + name: string; + description: string; + storeApi: WorkspaceStoreApi; + nameRef: React.RefObject; + descriptionRef: React.RefObject; + setName: (v: string) => void; + setDescription: (v: string) => void; + setSaving: (v: boolean) => void; + t: TFunction; +}; + +function buildSaveAppearanceHandler({ + activeWorkspace, + name, + description, + storeApi, + nameRef, + descriptionRef, + setName, + setDescription, + setSaving, + t, +}: SaveAppearanceOptions) { + return async () => { + if (!activeWorkspace) return; + const trimmedName = name.trim(); + if (!trimmedName) { + toast.error(t("workspaces:workspaceNameIsRequired")); + return; + } + setSaving(true); + try { + const updated = await updateWorkspaceAction(activeWorkspace.id, { + name: trimmedName, + description, + }); + // Only echo the server response back into the draft fields if the user + // hasn't kept typing since this save started: the PATCH round trip can + // take long enough for a newer keystroke to land before the response + // does, and unconditionally overwriting the draft here would silently + // discard it (refs, not the closed-over name/description, hold the + // latest value across that round trip). + if (nameRef.current === name) setName(updated.name); + if (descriptionRef.current === description) setDescription(updated.description ?? ""); + // Read the store's current list at save time, not the array captured at + // render/handler-build time: an in-flight workspace.created/updated/deleted + // WS event can land during the PATCH round trip, and a stale snapshot here + // would clobber it with a whole-array replace (setWorkspaces has no merge + // semantics). Same idiom as config-chat-agent-section.tsx. + const { workspaces: current, setWorkspaces } = storeApi.getState(); + setWorkspaces( + current.items.map((ws) => + ws.id === updated.id + ? { ...ws, name: updated.name, description: updated.description } + : ws, + ), + ); + toast.success(t("office:appearanceSettingsSaved")); + } catch (err) { + toast.error(err instanceof Error ? err.message : t("office:failedToSaveSettings")); + throw err; + } finally { + setSaving(false); + } + }; +} + +function usePermissionsState(activeWorkspace: Workspace | undefined) { + const { t } = useTranslation(); + const [approvalNewAgents, setApprovalNewAgents] = useState(true); + const [approvalTaskCompletion, setApprovalTaskCompletion] = useState(false); + const [approvalSkillChanges, setApprovalSkillChanges] = useState(true); + const [origApprovalNewAgents, setOrigApprovalNewAgents] = useState(true); + const [origApprovalTaskCompletion, setOrigApprovalTaskCompletion] = useState(false); + const [origApprovalSkillChanges, setOrigApprovalSkillChanges] = useState(true); + const [savingPermissions, setSavingPermissions] = useState(false); + const activeWorkspaceId = activeWorkspace?.id; + + useEffect(() => { + if (!activeWorkspaceId) return; + void getWorkspaceSettings(activeWorkspaceId) + .then((res) => { + const s = res.settings; + if (s.require_approval_for_new_agents !== undefined) { + setApprovalNewAgents(s.require_approval_for_new_agents); + setOrigApprovalNewAgents(s.require_approval_for_new_agents); + } + if (s.require_approval_for_task_completion !== undefined) { + setApprovalTaskCompletion(s.require_approval_for_task_completion); + setOrigApprovalTaskCompletion(s.require_approval_for_task_completion); + } + if (s.require_approval_for_skill_changes !== undefined) { + setApprovalSkillChanges(s.require_approval_for_skill_changes); + setOrigApprovalSkillChanges(s.require_approval_for_skill_changes); + } + }) + .catch(() => {}); + }, [activeWorkspaceId]); + + const handleSavePermissions = useCallback(async () => { + if (!activeWorkspace) return; + setSavingPermissions(true); + try { + await updateWorkspaceSettings(activeWorkspace.id, { + require_approval_for_new_agents: approvalNewAgents, + require_approval_for_task_completion: approvalTaskCompletion, + require_approval_for_skill_changes: approvalSkillChanges, + }); + setOrigApprovalNewAgents(approvalNewAgents); + setOrigApprovalTaskCompletion(approvalTaskCompletion); + setOrigApprovalSkillChanges(approvalSkillChanges); + toast.success(t("office:permissionSettingsSaved")); + } catch (err) { + toast.error(err instanceof Error ? err.message : t("office:failedToSaveSettings")); + } finally { + setSavingPermissions(false); + } + }, [activeWorkspace, approvalNewAgents, approvalTaskCompletion, approvalSkillChanges]); + + return { + approvalNewAgents, + setApprovalNewAgents, + approvalTaskCompletion, + setApprovalTaskCompletion, + approvalSkillChanges, + setApprovalSkillChanges, + savingPermissions, + permissionsDirty: + approvalNewAgents !== origApprovalNewAgents || + approvalTaskCompletion !== origApprovalTaskCompletion || + approvalSkillChanges !== origApprovalSkillChanges, + handleSavePermissions, + }; +} + function useRecoveryState(activeWorkspace: Workspace | undefined) { const { t } = useTranslation(); const [lookbackHours, setLookbackHours] = useState(24); @@ -294,85 +432,131 @@ function useRecoveryState(activeWorkspace: Workspace | undefined) { }; } -function useSettingsState(activeWorkspace: Workspace | undefined) { +type AppearanceBaseline = { + id: string | undefined; + name: string; + description: string; +}; + +function getAppearanceBaseline(workspace: Workspace | undefined): AppearanceBaseline { + return { + id: workspace?.id, + name: workspace?.name ?? "", + description: workspace?.description ?? "", + }; +} + +function reconcileAppearanceDraft( + previous: AppearanceBaseline, + next: AppearanceBaseline, + setName: Dispatch>, + setDescription: Dispatch>, +) { + if (next.id !== previous.id) { + setName(next.name); + setDescription(next.description); + return; + } + + setName((current) => (current === previous.name ? next.name : current)); + setDescription((current) => (current === previous.description ? next.description : current)); +} + +export function useSettingsState( + activeWorkspace: Workspace | undefined, + storeApi: WorkspaceStoreApi, +) { const { t } = useTranslation(); - const [name, setName] = useState(activeWorkspace?.name ?? ""); - const [description, setDescription] = useState(activeWorkspace?.description ?? ""); + const appearance = getAppearanceBaseline(activeWorkspace); + const [name, setName] = useState(appearance.name); + const [description, setDescription] = useState(appearance.description); const [logoPreview, setLogoPreview] = useState(null); const fileInputRef = useRef(null); - const [approvalNewAgents, setApprovalNewAgents] = useState(true); - const [approvalTaskCompletion, setApprovalTaskCompletion] = useState(false); - const [approvalSkillChanges, setApprovalSkillChanges] = useState(true); - const [origApprovalNewAgents, setOrigApprovalNewAgents] = useState(true); - const [origApprovalTaskCompletion, setOrigApprovalTaskCompletion] = useState(false); - const [origApprovalSkillChanges, setOrigApprovalSkillChanges] = useState(true); const [savingAppearance, setSavingAppearance] = useState(false); - const [savingPermissions, setSavingPermissions] = useState(false); const recovery = useRecoveryState(activeWorkspace); + const permissions = usePermissionsState(activeWorkspace); + const { id: appearanceId, name: appearanceName, description: appearanceDescription } = appearance; + const appearanceBaselineRef = useRef(appearance); - const activeWorkspaceId = activeWorkspace?.id; - + // If the active workspace's identity changes without this page + // remounting - e.g. a workspace.deleted WS event elsewhere reassigns + // workspaces.activeId while this page stays mounted - the draft must + // reset to the new workspace's values. Otherwise the stale draft from + // the old workspace looks "dirty" against the new one's name, and Save + // would persist the old workspace's data onto the new, unrelated one. + // For same-workspace updates, only fields that still match the previous + // baseline are refreshed. This keeps remote changes visible without + // overwriting a local draft. useEffect(() => { - if (!activeWorkspaceId) return; - void getWorkspaceSettings(activeWorkspaceId) - .then((res) => { - const s = res.settings; - if (s.require_approval_for_new_agents !== undefined) { - setApprovalNewAgents(s.require_approval_for_new_agents); - setOrigApprovalNewAgents(s.require_approval_for_new_agents); - } - if (s.require_approval_for_task_completion !== undefined) { - setApprovalTaskCompletion(s.require_approval_for_task_completion); - setOrigApprovalTaskCompletion(s.require_approval_for_task_completion); - } - if (s.require_approval_for_skill_changes !== undefined) { - setApprovalSkillChanges(s.require_approval_for_skill_changes); - setOrigApprovalSkillChanges(s.require_approval_for_skill_changes); - } - }) - .catch(() => {}); - }, [activeWorkspaceId]); + const next = { + id: appearanceId, + name: appearanceName, + description: appearanceDescription, + }; + const previous = appearanceBaselineRef.current; + + reconcileAppearanceDraft(previous, next, setName, setDescription); + appearanceBaselineRef.current = next; + }, [appearanceDescription, appearanceId, appearanceName]); + + // Latest-value refs so the in-flight save handler (a closure fixed at the + // moment Save was clicked) can tell whether the draft has moved on since, + // instead of blindly trusting the name/description it captured at submit + // time. + const nameRef = useRef(name); + const descriptionRef = useRef(description); + useEffect(() => { + nameRef.current = name; + }, [name]); + useEffect(() => { + descriptionRef.current = description; + }, [description]); const handleLogoChange = (e: React.ChangeEvent) => { const file = e.target.files?.[0]; if (file) setLogoPreview(URL.createObjectURL(file)); }; - const handleSaveAppearance = useCallback(async () => { - if (!activeWorkspace) return; - setSavingAppearance(true); - try { - await updateWorkspaceSettings(activeWorkspace.id, { name, description }); - toast.success(t("office:appearanceSettingsSaved")); - } catch (err) { - toast.error(err instanceof Error ? err.message : t("office:failedToSaveSettings")); - } finally { - setSavingAppearance(false); - } - }, [activeWorkspace, name, description]); + // useCallback (matching handleSavePermissions/handleSaveRecovery) so the + // handler only gets a new identity when an input it actually reads changes. + const handleSaveAppearance = useCallback( + buildSaveAppearanceHandler({ + activeWorkspace, + name, + description, + storeApi, + nameRef, + descriptionRef, + setName, + setDescription, + setSaving: setSavingAppearance, + t, + }), + [activeWorkspace, name, description, storeApi, nameRef, descriptionRef, t], + ); - const handleSavePermissions = useCallback(async () => { - if (!activeWorkspace) return; - setSavingPermissions(true); - try { - await updateWorkspaceSettings(activeWorkspace.id, { - require_approval_for_new_agents: approvalNewAgents, - require_approval_for_task_completion: approvalTaskCompletion, - require_approval_for_skill_changes: approvalSkillChanges, - }); - setOrigApprovalNewAgents(approvalNewAgents); - setOrigApprovalTaskCompletion(approvalTaskCompletion); - setOrigApprovalSkillChanges(approvalSkillChanges); - toast.success(t("office:permissionSettingsSaved")); - } catch (err) { - toast.error(err instanceof Error ? err.message : t("office:failedToSaveSettings")); - } finally { - setSavingPermissions(false); - } - }, [activeWorkspace, approvalNewAgents, approvalTaskCompletion, approvalSkillChanges]); + const appearanceDirty = + Boolean(appearanceId) && (name !== appearanceName || description !== appearanceDescription); + const appearanceRevision = JSON.stringify({ + id: appearanceId, + name, + description, + }); - const origName = activeWorkspace?.name ?? ""; - const origDescription = activeWorkspace?.description ?? ""; + useSettingsSaveContributor({ + id: "office-workspace-appearance", + order: 10, + revision: appearanceRevision, + isDirty: appearanceDirty, + canSave: Boolean(name.trim()), + invalidReason: name.trim() ? undefined : t("workspaces:workspaceNameIsRequired"), + save: handleSaveAppearance, + discard: (revision) => { + if (!Object.is(revision, appearanceRevision)) return; + setName(appearanceName); + setDescription(appearanceDescription); + }, + }); return { name, @@ -381,33 +565,23 @@ function useSettingsState(activeWorkspace: Workspace | undefined) { setDescription, logoPreview, fileInputRef, - approvalNewAgents, - setApprovalNewAgents, - approvalTaskCompletion, - setApprovalTaskCompletion, - approvalSkillChanges, - setApprovalSkillChanges, + ...permissions, ...recovery, - appearanceDirty: name !== origName || description !== origDescription, - permissionsDirty: - approvalNewAgents !== origApprovalNewAgents || - approvalTaskCompletion !== origApprovalTaskCompletion || - approvalSkillChanges !== origApprovalSkillChanges, + appearanceDirty, savingAppearance, - savingPermissions, handleLogoChange, handleSaveAppearance, - handleSavePermissions, }; } export function SettingsContent() { const { t } = useTranslation(); + const storeApi = useAppStoreApi(); const workspaces = useAppStore((s) => s.workspaces); const setWorkspaces = useAppStore((s) => s.setWorkspaces); const setActiveWorkspace = useAppStore((s) => s.setActiveWorkspace); const activeWorkspace = workspaces.items.find((w) => w.id === workspaces.activeId); - const s = useSettingsState(activeWorkspace); + const s = useSettingsState(activeWorkspace, storeApi); const initial = (s.name || "W").charAt(0).toUpperCase(); return ( @@ -420,12 +594,9 @@ export function SettingsContent() { logoPreview={s.logoPreview} initial={initial} fileInputRef={s.fileInputRef} - dirty={s.appearanceDirty} - saving={s.savingAppearance} onNameChange={s.setName} onDescriptionChange={s.setDescription} onLogoChange={s.handleLogoChange} - onSave={s.handleSaveAppearance} /> diff --git a/apps/web/e2e/tests/office/dashboard.spec.ts b/apps/web/e2e/tests/office/dashboard.spec.ts index f8fda4c842..17a902503b 100644 --- a/apps/web/e2e/tests/office/dashboard.spec.ts +++ b/apps/web/e2e/tests/office/dashboard.spec.ts @@ -65,12 +65,24 @@ test.describe("Dashboard", () => { }); test("update workspace settings", async ({ officeApi, officeSeed }) => { - const updated = await officeApi.updateWorkspaceSettings(officeSeed.workspaceId, { - name: "E2E Workspace Updated", - }); - expect(updated).toBeDefined(); + const { settings: originalSettings } = await officeApi.getWorkspaceSettings( + officeSeed.workspaceId, + ); + const originalLookbackHours = (originalSettings as { recovery_lookback_hours: number }) + .recovery_lookback_hours; - const settings = await officeApi.getWorkspaceSettings(officeSeed.workspaceId); - expect(settings).toBeDefined(); + try { + const updated = await officeApi.updateWorkspaceSettings(officeSeed.workspaceId, { + recovery_lookback_hours: 42, + }); + expect(updated).toEqual({ ok: true }); + + const { settings } = await officeApi.getWorkspaceSettings(officeSeed.workspaceId); + expect((settings as { recovery_lookback_hours: number }).recovery_lookback_hours).toBe(42); + } finally { + await officeApi.updateWorkspaceSettings(officeSeed.workspaceId, { + recovery_lookback_hours: originalLookbackHours, + }); + } }); }); diff --git a/apps/web/e2e/tests/office/workspace-settings-ui.spec.ts b/apps/web/e2e/tests/office/workspace-settings-ui.spec.ts index 6c421c7cb5..838616855b 100644 --- a/apps/web/e2e/tests/office/workspace-settings-ui.spec.ts +++ b/apps/web/e2e/tests/office/workspace-settings-ui.spec.ts @@ -1,8 +1,46 @@ import { test, expect } from "../../fixtures/office-fixture"; +import { waitForHttp } from "../../helpers/causal-waits"; test.describe("Workspace settings UI", () => { test("settings page shows danger zone", async ({ testPage, officeSeed: _ }) => { await testPage.goto("/office/workspace/settings"); await expect(testPage.getByText(/danger zone/i)).toBeVisible({ timeout: 10_000 }); }); + + test("renaming the workspace persists and updates the sidebar", async ({ + testPage, + apiClient, + officeSeed, + }) => { + const renamed = "E2E Workspace Renamed"; + let originalName: string | undefined; + try { + await testPage.goto("/office/workspace/settings"); + const nameInput = testPage.getByPlaceholder("Workspace name"); + await expect(nameInput).toBeVisible({ timeout: 10_000 }); + originalName = await nameInput.inputValue(); + await nameInput.fill(renamed); + const saveBar = testPage.getByTestId("settings-floating-save"); + const saveButton = saveBar.getByRole("button", { name: "Save changes" }); + await expect(saveBar).toBeVisible(); + + const workspaceSaved = waitForHttp( + testPage, + "PATCH", + new RegExp(`/api/v1/workspaces/${officeSeed.workspaceId}$`), + ); + await saveButton.click(); + await workspaceSaved; + + await expect(saveBar).toHaveAttribute("data-status", "saved"); + await expect(testPage.getByTestId("sidebar-workspace-trigger")).toContainText(renamed); + + await testPage.reload(); + await expect(nameInput).toHaveValue(renamed); + } finally { + await apiClient.updateWorkspace(officeSeed.workspaceId, { + name: originalName ?? "E2E Workspace", + }); + } + }); }); diff --git a/apps/web/lib/api/domains/office-extended-api.ts b/apps/web/lib/api/domains/office-extended-api.ts index 0aefba90fd..65427e6b79 100644 --- a/apps/web/lib/api/domains/office-extended-api.ts +++ b/apps/web/lib/api/domains/office-extended-api.ts @@ -625,8 +625,6 @@ export function getWorkspaceSettings(workspaceId: string, options?: ApiRequestOp export function updateWorkspaceSettings( workspaceId: string, data: { - name?: string; - description?: string; require_approval_for_new_agents?: boolean; require_approval_for_task_completion?: boolean; require_approval_for_skill_changes?: boolean; diff --git a/apps/web/src/office-routes.tsx b/apps/web/src/office-routes.tsx index 10b8f38dd2..f1785f4ede 100644 --- a/apps/web/src/office-routes.tsx +++ b/apps/web/src/office-routes.tsx @@ -48,6 +48,7 @@ import { import { RoutineDetailRoute } from "./office-routine-client-routes"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { Trans, useTranslation } from "react-i18next"; +import { SettingsSaveProvider } from "@/components/settings/settings-save-provider"; type RouteRenderer = () => React.ReactNode; @@ -62,7 +63,11 @@ const OFFICE_ROUTES: Record = { "/office/workspace/costs": () => , "/office/workspace/skills": () => , "/office/workspace/routing": () => , - "/office/workspace/settings": () => , + "/office/workspace/settings": () => ( + + + + ), "/office/workspace/settings/sync": () => , "/office/workspace/org": () => , }; @@ -103,7 +108,7 @@ export function OfficeRoutes({ pathname }: { pathname: string }) { return ( -
+
{renderOfficeRoute(normalizedPathname)}