From 39cf06cc02f34139fbe1e03fc292aa7cf889f2cc Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Wed, 5 Aug 2026 08:58:43 -0700 Subject: [PATCH] fix(review): make the Edit Mode opt-in an explicit switch, not a primary CTA The announcement footer's primary Turn it on button read as a generic continue and invited blind clicks: opting into an experimental feature was one reflexive Enter away. The decision is now an explicit Enable Edit Mode switch (defaulting off, with the shared TextShimmer label treatment) beside a single neutral Done button that applies whatever the switch says. Pressing Done untouched is a plain dismissal, and the focused default action is asserted consent-neutral in tests. Announcement cookie version bumped so the revision re-shows (pre-release, no users have the old cookie). --- .../EditModeAnnouncementDialog.test.tsx | 41 ++++++++++++--- .../components/EditModeAnnouncementDialog.tsx | 50 ++++++++++++++----- .../utils/editModeAnnouncement.test.ts | 4 +- .../utils/editModeAnnouncement.ts | 4 +- 4 files changed, 75 insertions(+), 24 deletions(-) diff --git a/packages/review-editor/components/EditModeAnnouncementDialog.test.tsx b/packages/review-editor/components/EditModeAnnouncementDialog.test.tsx index ad62b89aa..ca2857b0e 100644 --- a/packages/review-editor/components/EditModeAnnouncementDialog.test.tsx +++ b/packages/review-editor/components/EditModeAnnouncementDialog.test.tsx @@ -70,8 +70,10 @@ describe('EditModeAnnouncementDialog', () => { expect(dialog?.textContent).toContain('never writes to your files on disk'); expect(dialog?.textContent).toContain('One file at a time'); expect(dialog?.textContent).toContain('Settings → Editor → Edit Code to Suggest'); - expect(buttonWithText('Turn it on')).toBeTruthy(); - expect(buttonWithText('Keep it off')).toBeTruthy(); + const enableSwitch = document.querySelector('[role="switch"]'); + expect(enableSwitch).not.toBeNull(); + expect(enableSwitch?.getAttribute('aria-checked')).toBe('false'); + expect(buttonWithText('Done')).toBeTruthy(); }); test.skipIf(!hasDom)('renders the bundled demo recording by default', async () => { @@ -108,30 +110,53 @@ describe('EditModeAnnouncementDialog', () => { expect(document.querySelector('[data-edit-mode-demo-placeholder]')).toBeNull(); }); - test.skipIf(!hasDom)('"Turn it on" fires onEnable and never onDismiss', async () => { + test.skipIf(!hasDom)('Done with the switch untouched dismisses without enabling', async () => { const onEnable = mock(() => {}); const onDismiss = mock(() => {}); await mountDialog({ onEnable, onDismiss }); - expect(document.activeElement?.textContent).toContain('Turn it on'); + // The focused default action must be the consent-neutral one: pressing it + // blind keeps the feature off. + expect(document.activeElement?.textContent).toContain('Done'); - await act(async () => buttonWithText('Turn it on').click()); + await act(async () => buttonWithText('Done').click()); + + expect(onDismiss).toHaveBeenCalledTimes(1); + expect(onEnable).not.toHaveBeenCalled(); + expect(document.querySelector('[data-edit-mode-announcement-dialog]')).toBeNull(); + }); + + test.skipIf(!hasDom)('flipping the switch then Done fires onEnable and never onDismiss', async () => { + const onEnable = mock(() => {}); + const onDismiss = mock(() => {}); + await mountDialog({ onEnable, onDismiss }); + + const enableSwitch = document.querySelector('[role="switch"]'); + expect(enableSwitch).not.toBeNull(); + await act(async () => enableSwitch!.click()); + expect(enableSwitch!.getAttribute('aria-checked')).toBe('true'); + + await act(async () => buttonWithText('Done').click()); expect(onEnable).toHaveBeenCalledTimes(1); expect(onDismiss).not.toHaveBeenCalled(); expect(document.querySelector('[data-edit-mode-announcement-dialog]')).toBeNull(); }); - test.skipIf(!hasDom)('"Keep it off" dismisses without enabling', async () => { + test.skipIf(!hasDom)('the switch is a toggle: on then off again ends with a plain dismissal', async () => { const onEnable = mock(() => {}); const onDismiss = mock(() => {}); await mountDialog({ onEnable, onDismiss }); - await act(async () => buttonWithText('Keep it off').click()); + const enableSwitch = document.querySelector('[role="switch"]'); + await act(async () => enableSwitch!.click()); + await act(async () => enableSwitch!.click()); + expect(enableSwitch!.getAttribute('aria-checked')).toBe('false'); + + await act(async () => buttonWithText('Done').click()); expect(onDismiss).toHaveBeenCalledTimes(1); expect(onEnable).not.toHaveBeenCalled(); - expect(document.querySelector('[data-edit-mode-announcement-dialog]')).toBeNull(); }); test.skipIf(!hasDom)('Escape dismisses the dialog and restores prior focus', async () => { diff --git a/packages/review-editor/components/EditModeAnnouncementDialog.tsx b/packages/review-editor/components/EditModeAnnouncementDialog.tsx index 9e115425e..d17f0c5ee 100644 --- a/packages/review-editor/components/EditModeAnnouncementDialog.tsx +++ b/packages/review-editor/components/EditModeAnnouncementDialog.tsx @@ -1,6 +1,7 @@ -import React, { useEffect, useRef } from 'react'; +import React, { useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; import { Ban, MessageSquarePlus, Pencil, Send } from 'lucide-react'; +import { TextShimmer } from '@plannotator/ui/components/TextShimmer'; import { EDIT_MODE_DEMO_POSTER_SRC, EDIT_MODE_DEMO_VIDEO_SRC } from './editModeDemoMedia'; /** @@ -127,6 +128,7 @@ export function EditModeAnnouncementDialog({ const primaryActionRef = useRef(null); const previousFocusRef = useRef(null); const onDismissRef = useRef(onDismiss); + const [enableChoice, setEnableChoice] = useState(false); useEffect(() => { onDismissRef.current = onDismiss; @@ -135,6 +137,8 @@ export function EditModeAnnouncementDialog({ useEffect(() => { if (!isOpen) return; + // A reopened dialog must not remember a previously flipped switch. + setEnableChoice(false); previousFocusRef.current = document.activeElement instanceof HTMLElement ? document.activeElement : null; @@ -198,8 +202,8 @@ export function EditModeAnnouncementDialog({ className="mt-1.5 max-w-3xl text-sm leading-relaxed text-muted-foreground" > A new way to give review feedback: make the change you want to see, right in the - diff. It is experimental and off by default. Turn it on now or keep it off, and - nothing else changes. + diff. It is experimental and off by default. Turn it on with the switch below, or + leave it off, and nothing else changes.

@@ -251,21 +255,41 @@ export function EditModeAnnouncementDialog({

Change this anytime in Settings → Editor → Edit Code to Suggest.

-
- + {/* The enable decision is an explicit switch, deliberately separate from the + dismiss action: a primary "Turn it on" button reads as a generic continue + and gets clicked blind. Done applies whatever the switch says; with the + switch untouched it is a plain dismissal. */} +
+
+ + + Enable Edit Mode + + + +
diff --git a/packages/review-editor/utils/editModeAnnouncement.test.ts b/packages/review-editor/utils/editModeAnnouncement.test.ts index 6af673a3e..4c10824e4 100644 --- a/packages/review-editor/utils/editModeAnnouncement.test.ts +++ b/packages/review-editor/utils/editModeAnnouncement.test.ts @@ -35,7 +35,7 @@ describe('Edit Mode announcement persistence', () => { markEditModeAnnouncementSeen(); - expect(stored.get(SEEN_KEY)).toBe('1'); + expect(stored.get(SEEN_KEY)).toBe('3'); expect(needsEditModeAnnouncement()).toBe(false); }); @@ -48,7 +48,7 @@ describe('Edit Mode announcement persistence', () => { enableEditSuggestionsFromAnnouncement(); expect(stored.get(SETTING_KEY)).toBe('true'); - expect(stored.get(SEEN_KEY)).toBe('1'); + expect(stored.get(SEEN_KEY)).toBe('3'); expect(needsEditModeAnnouncement()).toBe(false); }); }); diff --git a/packages/review-editor/utils/editModeAnnouncement.ts b/packages/review-editor/utils/editModeAnnouncement.ts index 926145679..e7bded223 100644 --- a/packages/review-editor/utils/editModeAnnouncement.ts +++ b/packages/review-editor/utils/editModeAnnouncement.ts @@ -8,7 +8,9 @@ import { configStore } from '@plannotator/ui/config'; */ const STORAGE_KEY = 'plannotator-edit-mode-announcement-seen'; // Bump to re-show the announcement after a meaningful revision. -const CURRENT_VERSION = '1'; +// '3': shimmer label on the enable switch. '2': footer redesigned from a Turn it on / Keep it off button pair to an +// explicit enable switch plus a neutral Done (pre-release, so nobody re-sees). +const CURRENT_VERSION = '3'; export function needsEditModeAnnouncement(): boolean { return storage.getItem(STORAGE_KEY) !== CURRENT_VERSION;