From ef2ecaf8730b4f04bc370cfaa2370cf074507154 Mon Sep 17 00:00:00 2001 From: Kevin Chung Date: Fri, 7 Aug 2026 10:25:13 -0700 Subject: [PATCH] fix(desktop): defer channel visibility change to Save (#5203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem In the **Edit channel** dialog, flipping visibility (Public <> Private) persisted **immediately on selection**, 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 channel history. Reported in the Buzz "Welcome" channel by Kevin Chung. ## Root cause The visibility dropdown was wired to `handleConvertVisibility()`, which called the update mutation on selection. This was intentional at the time (there was even an e2e test named `02 — visibility updates immediately` and an "Updating…" spinner), but it is inconsistent with the rest of the dialog and is the surprising behavior reported. ## Change (defer to Save) - Visibility becomes a **deferred draft** like the other fields: selecting a value updates local `isPrivateDraft` and marks the draft dirty. The change commits via `handleSaveChannelEdits` (which already handled visibility) on **Save**, and is discarded on **Cancel**. - The dialog title now reflects the **pending draft** (`nextVisibility`), so the pending choice is visible before saving. - The edit-dialog reset restores `isPrivateDraft` from server state. - Removed the now-dead `handleConvertVisibility` handler, `isConvertingVisibility` state, the `channelIdRef` race guard it needed, and the unused `isPending`/"Updating…" spinner path in `ChannelPermissionsSettings` (no caller passes `isPending` anymore). ## Tests - Rewrote e2e `02` -> **`visibility defers to Save`**: select -> Save enabled -> title reflects draft -> Save -> persists; toggling back to the original value clears the draft and disables Save. - Extended `09` (cancel discards drafts) to also cover a visibility change. - Repurposed `10`: the stale-update race it guarded is architecturally gone, so it now asserts an **unsaved visibility draft does not leak across a channel switch**. ## Validation - `pnpm typecheck` — clean - `biome check` (changed files) — clean - `pnpm test` — **4497 passed / 0 failed** - `playwright test --project=smoke channel-controls` — **10 passed** Signed-off-by: Kevin Chung Co-authored-by: Fizz --- .../channels/ui/ChannelManagementSheet.tsx | 34 ++----- .../ui/ChannelPermissionsSettings.tsx | 22 +---- desktop/tests/e2e/channel-controls.spec.ts | 97 +++++++++++++++---- 3 files changed, 88 insertions(+), 65 deletions(-) diff --git a/desktop/src/features/channels/ui/ChannelManagementSheet.tsx b/desktop/src/features/channels/ui/ChannelManagementSheet.tsx index 3c1727e00..ee963ad01 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 708294136..eb7ee4739 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"); - 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"); + + // 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( + 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"), + ).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"); }); });