From 55e2c3c911ddc52d29e56ee4bca2eb77b289b5af Mon Sep 17 00:00:00 2001 From: Fizz Date: Fri, 7 Aug 2026 09:53:22 -0700 Subject: [PATCH] fix(desktop): defer channel visibility change to Save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Edit channel dialog persisted Public<>Private the instant a value was selected, bypassing the Save changes button while every other field (name, description, temporary, TTL) waited for an explicit save. This surprised users and gave no chance to cancel a flip (e.g. a private->public change that instantly exposes history). Make visibility a deferred draft like the other fields: selecting a value only updates local `isPrivateDraft` state and marks the draft dirty; the change is committed by `handleSaveChannelEdits` (which already supports visibility) when Save is clicked, and discarded on Cancel. The dialog title now reflects the pending draft, and the edit-dialog reset restores `isPrivateDraft` from server state. Removes the now-unused `handleConvertVisibility` handler, the `isConvertingVisibility` state, the `channelIdRef` race guard it needed, and the dead `isPending`/"Updating…" spinner path in `ChannelPermissionsSettings` (no caller passes `isPending` anymore). Tests: rewrite channel-controls e2e `02` to assert deferred-save behavior (select -> Save enabled -> Save -> persisted), extend `09` to cover visibility discard-on-cancel, and repurpose `10` (the stale-update race is architecturally gone) to assert an unsaved draft does not leak across a channel switch. Co-authored-by: Kevin Chung Signed-off-by: Kevin Chung --- .../channels/ui/ChannelManagementSheet.tsx | 34 ++------ .../ui/ChannelPermissionsSettings.tsx | 22 +---- desktop/tests/e2e/channel-controls.spec.ts | 83 ++++++++++++++++--- 3 files changed, 81 insertions(+), 58 deletions(-) diff --git a/desktop/src/features/channels/ui/ChannelManagementSheet.tsx b/desktop/src/features/channels/ui/ChannelManagementSheet.tsx index 3c1727e00a..ee963ad01d 100644 --- a/desktop/src/features/channels/ui/ChannelManagementSheet.tsx +++ b/desktop/src/features/channels/ui/ChannelManagementSheet.tsx @@ -124,8 +124,6 @@ export function ChannelManagementSheet({ const deleteChannelMutation = useDeleteChannelMutation(channelId); const joinChannelMutation = useJoinChannelMutation(channelId); const leaveChannelMutation = useLeaveChannelMutation(channelId); - const channelIdRef = React.useRef(channelId); - channelIdRef.current = channelId; const detail = detailsQuery.data ?? channel; const members = React.useMemo(() => { @@ -167,8 +165,6 @@ export function ChannelManagementSheet({ ); const [isDeleteDialogOpen, setIsDeleteDialogOpen] = React.useState(false); const [isEditDialogOpen, setIsEditDialogOpen] = React.useState(false); - const [isConvertingVisibility, setIsConvertingVisibility] = - React.useState(false); const [hasUserEditedChannelDraft, setHasUserEditedChannelDraft] = React.useState(false); const [activeView, setActiveView] = React.useState<"summary" | "canvas">( @@ -268,6 +264,7 @@ export function ChannelManagementSheet({ if (!next) { setNameDraft(resolvedChannel.name); setDescriptionDraft(resolvedChannel.description); + setIsPrivateDraft(currentVisibility === "private"); setIsEphemeralDraft(currentTtlSeconds !== null); setTtlSecondsDraft(currentTtlSeconds ?? DEFAULT_EPHEMERAL_TTL_SECONDS); setHasUserEditedChannelDraft(false); @@ -298,25 +295,6 @@ export function ChannelManagementSheet({ } } - async function handleConvertVisibility(visibility: "open" | "private") { - if (visibility === currentVisibility) { - return; - } - setIsConvertingVisibility(true); - try { - const updatedChannel = await updateChannelDetailsMutation.mutateAsync({ - visibility, - }); - if (channelIdRef.current === updatedChannel.id) { - setIsPrivateDraft(visibility === "private"); - } - } catch { - // React Query stores mutation errors; keep the dialog open and render them. - } finally { - setIsConvertingVisibility(false); - } - } - return ( - Edit {currentVisibility === "private" ? "private" : "public"}{" "} + Edit {nextVisibility === "private" ? "private" : "public"}{" "} channel @@ -525,10 +503,10 @@ export function ChannelManagementSheet({ /> - void handleConvertVisibility(visibility) - } + onVisibilityChange={(visibility) => { + setIsPrivateDraft(visibility === "private"); + setHasUserEditedChannelDraft(true); + }} testIdPrefix="channel-management" visibility={isPrivateDraft ? "private" : "open"} /> diff --git a/desktop/src/features/channels/ui/ChannelPermissionsSettings.tsx b/desktop/src/features/channels/ui/ChannelPermissionsSettings.tsx index 7082941365..eb7ee4739c 100644 --- a/desktop/src/features/channels/ui/ChannelPermissionsSettings.tsx +++ b/desktop/src/features/channels/ui/ChannelPermissionsSettings.tsx @@ -1,4 +1,4 @@ -import { ChevronDown, LoaderCircle } from "lucide-react"; +import { ChevronDown } from "lucide-react"; import type { ChannelVisibility } from "@/shared/api/types"; import { Button } from "@/shared/ui/button"; @@ -13,13 +13,11 @@ import { cn } from "@/shared/lib/cn"; export function ChannelPermissionsSettings({ disabled, - isPending = false, onVisibilityChange, testIdPrefix, visibility, }: { disabled?: boolean; - isPending?: boolean; onVisibilityChange: (visibility: ChannelVisibility) => void; testIdPrefix: string; visibility: ChannelVisibility; @@ -38,12 +36,7 @@ export function ChannelPermissionsSettings({ { await settle(page); }); - test("02 — visibility updates immediately", async ({ page }) => { + test("02 — visibility defers to Save", async ({ page }) => { await installMockBridge(page, { updateChannelDelayMs: 500 }); await openManagementSheet(page); await openEditDialog(page); @@ -91,20 +92,34 @@ test.describe("channel controls", () => { const lifecycle = page.getByTestId("channel-management-lifecycle"); await lifecycle.scrollIntoViewIfNeeded(); const permissions = page.getByTestId("channel-management-permissions"); + + // Save starts disabled with no pending edits. + await expect( + page.getByTestId("channel-management-save-changes"), + ).toBeDisabled(); + + // Selecting a visibility only updates the local draft: the dialog title + // reflects the pending choice and Save becomes enabled, but nothing is + // persisted yet (no "Updating…" state). await permissions.click(); await page .getByTestId("channel-management-permissions-option-private") .click(); - await expect(permissions).toHaveAttribute("aria-busy", "true"); - await expect(permissions).toContainText("Updating…"); await expect( page.getByRole("dialog", { name: "Edit private channel" }), ).toBeVisible(); await expect(permissions).toHaveAccessibleName("Visibility: Private"); + await expect(permissions).not.toHaveAttribute("aria-busy", "true"); await expect( page.getByTestId("channel-management-save-changes"), - ).toBeDisabled(); + ).toBeEnabled(); + // Wait for the dropdown to fully close before reopening it, so the + // trigger stays mounted/stable for the next interaction. + await expect( + page.getByTestId("channel-management-permissions-option-private"), + ).toHaveCount(0); + // Toggling back to the original value clears the draft and disables Save. await permissions.click(); await page .getByTestId("channel-management-permissions-option-open") @@ -116,6 +131,36 @@ test.describe("channel controls", () => { await expect( page.getByTestId("channel-management-save-changes"), ).toBeDisabled(); + await expect( + page.getByTestId("channel-management-permissions-option-open"), + ).toHaveCount(0); + + // Choose Private again and commit via Save. + await permissions.click(); + await page + .getByTestId("channel-management-permissions-option-private") + .click(); + await expect( + page.getByTestId("channel-management-save-changes"), + ).toBeEnabled(); + await page.getByTestId("channel-management-save-changes").click(); + await expect( + page.getByTestId("channel-management-save-changes"), + ).toHaveText("Saving..."); + await expect( + page.getByRole("dialog", { + name: /Edit (?:public|private) channel/, + }), + ).toHaveCount(0); + + // Reopen to confirm the visibility change persisted. + await openEditDialog(page); + await expect( + page.getByRole("dialog", { name: "Edit private channel" }), + ).toBeVisible(); + await expect( + page.getByTestId("channel-management-permissions"), + ).toHaveAccessibleName("Visibility: Private"); await settle(page); }); @@ -272,6 +317,13 @@ test.describe("channel controls", () => { await page .getByRole("textbox", { name: "Description" }) .fill("This description should be discarded"); + await page.getByTestId("channel-management-permissions").click(); + await page + .getByTestId("channel-management-permissions-option-private") + .click(); + await expect( + page.getByRole("dialog", { name: "Edit private channel" }), + ).toBeVisible(); await selectTemporaryChannelType(page); await expect( page.getByTestId("channel-management-save-changes"), @@ -291,6 +343,12 @@ test.describe("channel controls", () => { await expect( page.getByRole("textbox", { name: "Description" }), ).toHaveValue("General discussion for everyone"); + await expect( + page.getByRole("dialog", { name: "Edit public channel" }), + ).toBeVisible(); + await expect( + page.getByTestId("channel-management-permissions"), + ).toHaveAccessibleName("Visibility: Public"); await expect( page.getByTestId("channel-management-channel-type"), ).toContainText("Ongoing"); @@ -302,7 +360,7 @@ test.describe("channel controls", () => { ).toBeDisabled(); }); - test("10 — stale visibility updates do not affect a new channel", async ({ + test("10 — unsaved visibility draft does not leak to a new channel", async ({ page, }) => { await installMockBridge(page, { updateChannelDelayMs: 1_500 }); @@ -314,7 +372,10 @@ test.describe("channel controls", () => { await page .getByTestId("channel-management-permissions-option-private") .click(); - await expect(permissions).toHaveAttribute("aria-busy", "true"); + // Draft only — never saved. The dialog title reflects the pending choice. + await expect( + page.getByRole("dialog", { name: "Edit private channel" }), + ).toBeVisible(); const agentsChannelId = await page .getByTestId("channel-agents") @@ -335,14 +396,12 @@ test.describe("channel controls", () => { window.dispatchEvent(new PopStateEvent("popstate")); }, agentsChannelId); await expect(page.getByTestId("chat-title")).toHaveText("agents"); - await expect( - page.getByRole("dialog", { name: "Edit public channel" }), - ).toBeVisible(); - await expect(permissions).toHaveAttribute("aria-busy", "false"); - await expect(permissions).toHaveAccessibleName("Visibility: Public"); + // The unsaved draft must not carry over: the new channel re-syncs from + // server state and shows its own (public) visibility. await expect( page.getByRole("dialog", { name: "Edit public channel" }), ).toBeVisible(); + await expect(permissions).toHaveAccessibleName("Visibility: Public"); }); });