External/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-08-18 06:50:31 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(desktop): defer channel visibility change to Save (#5203)

Browse Source 
## 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 <chung@squareup.com>
Co-authored-by: Fizz <e3f95089179cc1bcc68d70c334b9bdf670d0470496db90bcdbb20386963432da@buzz.block.builderlab.xyz>
This commit is contained in:
Kevin Chung
2026-08-07 17:25:13 +00:00
committed by GitHub
co-authored by Fizz
parent fb73561e64
commit ef2ecaf873
3 changed files with 88 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
desktop/src/features/channels/ui/ChannelManagementSheet.tsx
+6 -28
View File
@@ -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 (
<DialogPrimitive.Root
modal={!isSplitLayout}
@@ -441,7 +419,7 @@ export function ChannelManagementSheet({
<div className="flex max-h-[85vh] flex-col">
<DialogHeader className="shrink-0 border-b border-border/60 px-6 py-5 pr-14">
<DialogTitle>
Edit {currentVisibility === "private" ? "private" : "public"}{" "}
Edit {nextVisibility === "private" ? "private" : "public"}{" "}
channel
</DialogTitle>
</DialogHeader>
@@ -525,10 +503,10 @@ export function ChannelManagementSheet({
/>
<ChannelPermissionsSettings
disabled={isSavingChannelEdits}
isPending={isConvertingVisibility}
onVisibilityChange={(visibility) =>
void handleConvertVisibility(visibility)
}
onVisibilityChange={(visibility) => {
setIsPrivateDraft(visibility === "private");
setHasUserEditedChannelDraft(true);
}}
testIdPrefix="channel-management"
visibility={isPrivateDraft ? "private" : "open"}
/>
desktop/src/features/channels/ui/ChannelPermissionsSettings.tsx
+4 -18
View File
@@ -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({
<DropdownMenu modal={false}>
<DropdownMenuTrigger asChild>
<Button
aria-busy={isPending}
aria-label={
isPending
? "Updating visibility"
: `Visibility: ${visibilityLabel}`
}
aria-label={`Visibility: ${visibilityLabel}`}
className="-mr-2.5 ml-auto h-9 w-fit justify-end px-2.5 text-right text-sm font-medium text-foreground hover:bg-muted/50"
data-testid={`${testIdPrefix}-permissions`}
disabled={disabled}
@@ -51,16 +44,9 @@ export function ChannelPermissionsSettings({
variant="ghost"
>
<span aria-live="polite" className="text-right">
{isPending ? "Updating…" : visibilityLabel}
{visibilityLabel}
</span>
{isPending ? (
<LoaderCircle
aria-hidden="true"
className="size-4 shrink-0 text-muted-foreground/70 motion-safe:animate-spin"
/>
) : (
<ChevronDown className="size-4 shrink-0 text-muted-foreground/70" />
)}
<ChevronDown className="size-4 shrink-0 text-muted-foreground/70" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent
desktop/tests/e2e/channel-controls.spec.ts
+78 -19
View File
@@ -3,7 +3,8 @@ import { expect, test } from "@playwright/test";
import { installMockBridge } from "../helpers/bridge";
// `general` seeds the mock identity as owner, so the owner/admin-gated
// visibility + ephemeral controls are live and interactive.
// visibility + ephemeral controls are editable. Visibility is a deferred
// draft: selecting a value only updates local state; it persists on Save.
async function openManagementSheet(page: import("@playwright/test").Page) {
await page.goto("/");
await page.getByTestId("channel-general").click();
@@ -83,7 +84,7 @@ test.describe("channel controls", () => {
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");
});
});