From 5c5b3c6d90af6a6ca56da0c42e2df1f723cd3919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 27 Aug 2026 12:05:22 +0000 Subject: [PATCH] fix: stop plan bubble menu transaction loop (#3083) --- .../editors/tiptap/plan-bubble-menu.test.tsx | 147 +++++++++++++++++- .../editors/tiptap/plan-bubble-menu.tsx | 37 ++++- .../plan-bubble-menu-transaction-loop/plan.md | 111 +++++++++++++ ...sk-01-stop-bubble-menu-transaction-loop.md | 99 ++++++++++++ .../responsive-plan-formatting.md | 17 ++ 5 files changed, 404 insertions(+), 7 deletions(-) create mode 100644 docs/plans/plan-bubble-menu-transaction-loop/plan.md create mode 100644 docs/plans/plan-bubble-menu-transaction-loop/task-01-stop-bubble-menu-transaction-loop.md diff --git a/apps/web/components/editors/tiptap/plan-bubble-menu.test.tsx b/apps/web/components/editors/tiptap/plan-bubble-menu.test.tsx index d0e477bb11..680ba519ca 100644 --- a/apps/web/components/editors/tiptap/plan-bubble-menu.test.tsx +++ b/apps/web/components/editors/tiptap/plan-bubble-menu.test.tsx @@ -1,4 +1,5 @@ import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { useEffect, useRef } from "react"; import type { Editor } from "@tiptap/core"; import { afterEach, describe, expect, it, vi } from "vitest"; @@ -14,6 +15,13 @@ const mocks = vi.hoisted(() => ({ keyboardOpen: false, viewportBottom: 800, }, + bubbleMenu: { + optionUpdateTransactions: 0, + renderCount: 0, + testId: "plan-selection-bubble", + visibleValue: "true", + hiddenValue: "false", + }, })); vi.mock("@/hooks/use-responsive-breakpoint", () => ({ @@ -49,16 +57,32 @@ vi.mock("react-i18next", () => ({ vi.mock("@tiptap/react/menus", () => ({ BubbleMenu: ({ editor, + options, shouldShow, children, }: { editor: Editor; + options?: object; shouldShow?: (props: { editor: Editor; state: Editor["state"] }) => boolean; children: React.ReactNode; }) => { + mocks.bubbleMenu.renderCount += 1; + const previousOptions = useRef(options); + useEffect(() => { + if (previousOptions.current === options) return; + previousOptions.current = options; + mocks.bubbleMenu.optionUpdateTransactions += 1; + if (mocks.bubbleMenu.optionUpdateTransactions < 3) { + (editor as FakeEditor).emit("transaction"); + } + }, [editor, options]); + const isVisible = shouldShow?.({ editor, state: editor.state }) ?? true; return ( -
+
{isVisible ? children : null}
); @@ -68,11 +92,14 @@ vi.mock("@tiptap/react/menus", () => ({ import { PlanBubbleMenu } from "./plan-bubble-menu"; const PLAN_TOOLBAR_LABEL = "editors:planFormattingToolbar"; +const VISIBILITY_ATTRIBUTE = "data-visible"; type FakeEditor = Editor & { emit: (event: string) => void; setSelection: (from: number, to: number, text?: string) => void; setFocused: (focused: boolean) => void; + setActiveMark: (name: string, active: boolean) => void; + setCodeBlock: (active: boolean) => void; chainMock: { focus: ReturnType; toggleBold: ReturnType; @@ -95,6 +122,8 @@ function createEditor({ } = {}): FakeEditor { const handlers = new Map void>>(); const selection = { from, to }; + let codeBlockState = codeBlock; + const activeMarks = new Set(); const chainMock = { focus: vi.fn(), toggleBold: vi.fn(), @@ -109,7 +138,9 @@ function createEditor({ selection, doc: { textBetween: vi.fn(() => text) }, }, - isActive: vi.fn((name: string) => name === "codeBlock" && codeBlock), + isActive: vi.fn((name: string) => + name === "codeBlock" ? codeBlockState : activeMarks.has(name), + ), getAttributes: vi.fn(() => ({})), chain: vi.fn(() => chainMock), commands: { focus: vi.fn() }, @@ -137,6 +168,16 @@ function createEditor({ editor.setFocused = (nextFocused) => { editor.isFocused = nextFocused; }; + editor.setActiveMark = (name, active) => { + if (active) { + activeMarks.add(name); + } else { + activeMarks.delete(name); + } + }; + editor.setCodeBlock = (active) => { + codeBlockState = active; + }; editor.chainMock = chainMock; return editor; } @@ -150,6 +191,8 @@ afterEach(() => { mocks.viewport.keyboardOpen = false; mocks.viewport.bottomOffset = 0; mocks.viewport.viewportBottom = 800; + mocks.bubbleMenu.optionUpdateTransactions = 0; + mocks.bubbleMenu.renderCount = 0; }); describe("PlanBubbleMenu responsive presentation", () => { @@ -159,7 +202,7 @@ describe("PlanBubbleMenu responsive presentation", () => { render(); expect(screen.getByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeTruthy(); - expect(screen.queryByTestId("plan-selection-bubble")).toBeNull(); + expect(screen.queryByTestId(mocks.bubbleMenu.testId)).toBeNull(); }); }); @@ -214,6 +257,31 @@ describe("PlanBubbleMenu mobile selection behavior", () => { }); describe("PlanBubbleMenu responsive layout", () => { + it("@covers AC-UI-RESPONSIVE-PLAN-FORMATTING-001.1: deduplicates unchanged metadata transactions", () => { + mocks.breakpoint.isFinePointer = true; + mocks.breakpoint.isMobile = false; + mocks.breakpoint.usesDesktopWorkbench = true; + const editor = createEditor({ focused: false }); + + render(); + + act(() => { + editor.setFocused(true); + editor.emit("focus"); + }); + + const renderCountAfterFocus = mocks.bubbleMenu.renderCount; + const optionUpdatesAfterFocus = mocks.bubbleMenu.optionUpdateTransactions; + + act(() => { + editor.emit("transaction"); + }); + + expect(mocks.bubbleMenu.renderCount).toBe(renderCountAfterFocus); + expect(mocks.bubbleMenu.optionUpdateTransactions).toBe(optionUpdatesAfterFocus); + expect(optionUpdatesAfterFocus).toBe(0); + }); + it("keeps the dock visible while the link input owns focus", () => { const editor = createEditor(); @@ -278,10 +346,14 @@ describe("PlanBubbleMenu responsive layout", () => { render(); - expect(screen.getByTestId("plan-selection-bubble").getAttribute("data-visible")).toBe("true"); + expect(screen.getByTestId(mocks.bubbleMenu.testId).getAttribute(VISIBILITY_ATTRIBUTE)).toBe( + mocks.bubbleMenu.visibleValue, + ); expect(screen.queryByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeNull(); }); +}); +describe("PlanBubbleMenu reactive editor state", () => { it("updates mobile visibility from editor focus and transaction events", () => { const editor = createEditor({ focused: false }); render(); @@ -300,6 +372,45 @@ describe("PlanBubbleMenu responsive layout", () => { }); expect(screen.queryByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeNull(); }); + + it("updates active mark controls from transaction events", () => { + const editor = createEditor(); + render(); + + const bold = screen.getByRole("button", { name: "editors:boldCmdB" }); + expect(bold.getAttribute("aria-pressed")).toBe(mocks.bubbleMenu.hiddenValue); + + act(() => { + editor.setActiveMark("bold", true); + editor.emit("transaction"); + }); + expect(bold.getAttribute("aria-pressed")).toBe(mocks.bubbleMenu.visibleValue); + + act(() => { + editor.setActiveMark("bold", false); + editor.emit("transaction"); + }); + expect(bold.getAttribute("aria-pressed")).toBe(mocks.bubbleMenu.hiddenValue); + }); + + it("updates mobile toolbar visibility from code-block transactions", () => { + const editor = createEditor(); + render(); + + expect(screen.getByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeTruthy(); + + act(() => { + editor.setCodeBlock(true); + editor.emit("transaction"); + }); + expect(screen.queryByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeNull(); + + act(() => { + editor.setCodeBlock(false); + editor.emit("transaction"); + }); + expect(screen.getByRole("toolbar", { name: PLAN_TOOLBAR_LABEL })).toBeTruthy(); + }); }); describe("PlanBubbleMenu code-block suppression", () => { @@ -319,6 +430,32 @@ describe("PlanBubbleMenu code-block suppression", () => { render(); - expect(screen.getByTestId("plan-selection-bubble").getAttribute("data-visible")).toBe("false"); + expect(screen.getByTestId(mocks.bubbleMenu.testId).getAttribute(VISIBILITY_ATTRIBUTE)).toBe( + mocks.bubbleMenu.hiddenValue, + ); + }); + + it("updates desktop selection bubble visibility from code-block transactions", () => { + mocks.breakpoint.isFinePointer = true; + mocks.breakpoint.isMobile = false; + mocks.breakpoint.usesDesktopWorkbench = true; + const editor = createEditor(); + + render(); + + const bubble = screen.getByTestId(mocks.bubbleMenu.testId); + expect(bubble.getAttribute(VISIBILITY_ATTRIBUTE)).toBe(mocks.bubbleMenu.visibleValue); + + act(() => { + editor.setCodeBlock(true); + editor.emit("transaction"); + }); + expect(bubble.getAttribute(VISIBILITY_ATTRIBUTE)).toBe(mocks.bubbleMenu.hiddenValue); + + act(() => { + editor.setCodeBlock(false); + editor.emit("transaction"); + }); + expect(bubble.getAttribute(VISIBILITY_ATTRIBUTE)).toBe(mocks.bubbleMenu.visibleValue); }); }); diff --git a/apps/web/components/editors/tiptap/plan-bubble-menu.tsx b/apps/web/components/editors/tiptap/plan-bubble-menu.tsx index b1f58f33a1..2a83bd524f 100644 --- a/apps/web/components/editors/tiptap/plan-bubble-menu.tsx +++ b/apps/web/components/editors/tiptap/plan-bubble-menu.tsx @@ -60,6 +60,26 @@ type EditorSnapshot = { isLink: boolean; }; +const PLAN_BUBBLE_MENU_OPTIONS = { placement: "top" } as const; + +const EDITOR_SNAPSHOT_KEYS = { + isFocused: "isFocused", + isCodeBlock: "isCodeBlock", + hasTextSelection: "hasTextSelection", + hasCommentSelection: "hasCommentSelection", + isBold: "isBold", + isItalic: "isItalic", + isUnderline: "isUnderline", + isStrike: "isStrike", + isCode: "isCode", + isHighlight: "isHighlight", + isLink: "isLink", +} as const satisfies { [K in keyof EditorSnapshot]: K }; + +const EDITOR_SNAPSHOT_KEY_VALUES = Object.values(EDITOR_SNAPSHOT_KEYS) as Array< + keyof EditorSnapshot +>; + function readEditorSnapshot(editor: Editor): EditorSnapshot { const { from, to } = editor.state.selection; const selectedText = editor.state.doc.textBetween(from, to, " ").trim(); @@ -79,11 +99,20 @@ function readEditorSnapshot(editor: Editor): EditorSnapshot { }; } +function areEditorSnapshotsEqual(first: EditorSnapshot, second: EditorSnapshot): boolean { + return EDITOR_SNAPSHOT_KEY_VALUES.every((key) => first[key] === second[key]); +} + function useEditorSnapshot(editor: Editor): EditorSnapshot { const [snapshot, setSnapshot] = useState(() => readEditorSnapshot(editor)); useEffect(() => { - const update = () => setSnapshot(readEditorSnapshot(editor)); + const update = () => { + setSnapshot((current) => { + const next = readEditorSnapshot(editor); + return areEditorSnapshotsEqual(current, next) ? current : next; + }); + }; editor.on("transaction", update); editor.on("focus", update); editor.on("blur", update); @@ -558,7 +587,11 @@ export function PlanBubbleMenu({ } return ( - + {content} ); diff --git a/docs/plans/plan-bubble-menu-transaction-loop/plan.md b/docs/plans/plan-bubble-menu-transaction-loop/plan.md new file mode 100644 index 0000000000..5a6a61ef8c --- /dev/null +++ b/docs/plans/plan-bubble-menu-transaction-loop/plan.md @@ -0,0 +1,111 @@ +--- +created: 2026-08-27 +status: done +requirements: + - REQ-UI-RESPONSIVE-PLAN-FORMATTING-001 +system_design: + - ../../specs/ui/system-design/responsive-plan-formatting.md +legacy_specs: [] +--- + +# Implementation Plan: Stop the Plan Bubble Menu Transaction Loop + +## Overview + +Stop the desktop Plan editor from running a continuous React and Tiptap +transaction loop. Implement one focused TDD work order because the fault and +its regression boundary are in one component. + +The trace shows approximately 44,800 React scheduler callbacks in 3.56 +seconds. The DOM stays at 4,862 elements. The source-mapped stack shows a +Tiptap BubbleMenu option update followed by an editor transaction and another +PlanBubbleMenu render. + +## Scope + +### In scope + +- Keep the desktop BubbleMenu option identity stable across unchanged renders. +- Publish a new editor snapshot only when its derived values change. +- Add a focused regression that models the real BubbleMenu option-update + transaction. +- Preserve the desktop selection bubble and the mobile docked formatting + strip. + +### Out of scope + +- Change Plan formatting commands, layout, touch geometry, or viewport + behavior. +- Change Tiptap, ProseMirror, task persistence, or WebSocket code. +- Add performance telemetry or a general React render monitor. +- Change the public plugin editor contract. + +## Technical approach + +### Stable BubbleMenu inputs + +Define the placement options once in +`apps/web/components/editors/tiptap/plan-bubble-menu.tsx`. Pass that stable +object to the desktop BubbleMenu. + +### Deduplicated editor snapshots + +Compare the next `EditorSnapshot` with the current snapshot. Return the +current state when all derived values are equal. Publish the next snapshot +only after focus, selection, context, or active marks change. + +### Regression boundary + +Extend `plan-bubble-menu.test.tsx` with a controlled BubbleMenu fake. The fake +models Tiptap's option dependency and its metadata transaction. The test fails +on the current code because the transaction repeats after each render. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-UI-RESPONSIVE-PLAN-FORMATTING-001.1` | The focused Vitest regression proves that the desktop BubbleMenu settles after one option update. Focus, selection, active-mark, and desktop/mobile code-block transitions remain reactive. | +| `AC-UI-RESPONSIVE-PLAN-FORMATTING-001.2` through `.8` | The existing mobile component tests and `mobile-plan-formatting-toolbar.spec.ts` prove that the docked presentation keeps its current behavior. | + +## E2E tests + +This fix changes render-state normalization only. It does not change layout, +touch behavior, scrolling, navigation, or viewport-dependent interaction. + +The existing `mobile-plan-formatting-toolbar.spec.ts` remains the mobile +parity evidence. Run its `mobile-chrome` scenario after the focused unit test. + +## Work orders + +- [x] [Task 01: Stop the BubbleMenu transaction loop](task-01-stop-bubble-menu-transaction-loop.md) (done) + +## Verification results + +- Red regression: the controlled BubbleMenu fake observed four option-update + transactions before the fix and failed the settlement assertion. +- `cd apps/web && pnpm exec vitest run components/editors/tiptap/plan-bubble-menu.test.tsx` + passed, 13 tests, including active-mark and code-block transaction changes. +- `cd apps/web && pnpm run typecheck` passed. +- `cd apps/web && pnpm exec eslint components/editors/tiptap/plan-bubble-menu.tsx + components/editors/tiptap/plan-bubble-menu.test.tsx` passed. +- `cd apps/web && pnpm e2e:run --project mobile-chrome + tests/task/mobile-plan-formatting-toolbar.spec.ts` passed, 1 test. +- Fresh desktop and mobile PR screenshots were captured, inspected, validated, + and compressed. The disposable desktop capture spec was removed. + +## Risks + +- An incomplete snapshot comparison can leave pressed formatting state stale. +- An unstable BubbleMenu prop can reintroduce metadata transactions on each + render. +- A weak menu mock can hide the Tiptap effect that caused the fault. + +## Public documentation + +None. This fix changes no public command, configuration, API, workflow, or +plugin contract. + +## Decisions + +No ADR is required. The fix completes the current component design without a +new ownership or public contract boundary. diff --git a/docs/plans/plan-bubble-menu-transaction-loop/task-01-stop-bubble-menu-transaction-loop.md b/docs/plans/plan-bubble-menu-transaction-loop/task-01-stop-bubble-menu-transaction-loop.md new file mode 100644 index 0000000000..fe8d6d3ee9 --- /dev/null +++ b/docs/plans/plan-bubble-menu-transaction-loop/task-01-stop-bubble-menu-transaction-loop.md @@ -0,0 +1,99 @@ +--- +id: "01-stop-bubble-menu-transaction-loop" +title: "Stop the BubbleMenu transaction loop" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-UI-RESPONSIVE-PLAN-FORMATTING-001 +acceptance_criteria: + - AC-UI-RESPONSIVE-PLAN-FORMATTING-001.1 + - AC-UI-RESPONSIVE-PLAN-FORMATTING-001.2 +system_design: + - ../../specs/ui/system-design/responsive-plan-formatting.md +--- + +# Task 01: Stop the BubbleMenu Transaction Loop + +## Summary + +Make the desktop BubbleMenu inputs stable and deduplicate the reactive editor +snapshot. Add a focused regression that models Tiptap's option-update +transaction and proves that the component settles. + +## In scope + +- Add the failing transaction-loop regression before production changes. +- Keep the desktop BubbleMenu placement options referentially stable. +- Compare all `EditorSnapshot` fields before publishing state. +- Keep focus, selection, code-block, and active-mark changes reactive. +- Run the existing mobile formatting scenario after the focused checks pass. + +## Out of scope + +- Change rendered layout, action order, touch targets, or keyboard geometry. +- Change Plan content, comments, autosave, or revisions. +- Change Tiptap or ProseMirror package code. +- Add a new browser performance harness. + +## Acceptance + +- The regression fails before production changes because BubbleMenu option + updates cause repeated editor transactions. +- The corrected component settles after one option update when the editor + snapshot is unchanged. +- Real focus, selection, code-block, and active-mark changes still update the + formatting controls. + +## Verification + +```bash +cd apps/web +pnpm exec vitest run components/editors/tiptap/plan-bubble-menu.test.tsx +pnpm run typecheck +pnpm exec eslint components/editors/tiptap/plan-bubble-menu.tsx components/editors/tiptap/plan-bubble-menu.test.tsx +pnpm e2e:run --project mobile-chrome tests/task/mobile-plan-formatting-toolbar.spec.ts +``` + +## Files likely touched + +- `apps/web/components/editors/tiptap/plan-bubble-menu.tsx` +- `apps/web/components/editors/tiptap/plan-bubble-menu.test.tsx` +- `docs/plans/plan-bubble-menu-transaction-loop/plan.md` +- `docs/plans/plan-bubble-menu-transaction-loop/task-01-stop-bubble-menu-transaction-loop.md` + +## Dependencies + +None. + +## Risks + +- The test fake must model the Tiptap options effect without creating an + unbounded test loop. +- The snapshot comparison must include every field in `EditorSnapshot`. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-UI-RESPONSIVE-PLAN-FORMATTING-001` acceptance criteria `.1` and `.2`. +- `docs/specs/ui/system-design/responsive-plan-formatting.md`. +- The performance trace attached to the investigation task. +- Tiptap BubbleMenu's option-update effect in `@tiptap/react`. +- Existing PlanBubbleMenu component tests and the mobile formatting E2E. + +## Results + +- Added a controlled BubbleMenu fake that reproduces repeated option-update + transactions and fails before the fix. +- Stabilized the desktop BubbleMenu placement options at module scope. +- Deduplicated all `EditorSnapshot` fields before publishing React state. +- Extended the fake editor regression to verify active-mark button state and + mobile and desktop code-block visibility changes after transactions. +- Focused Vitest, typecheck, changed-file ESLint, and the existing mobile Plan + formatting E2E all pass. +- No mobile E2E change was needed because this internal normalization preserves + the existing mobile presentation; the existing scenario covers it. diff --git a/docs/specs/ui/system-design/responsive-plan-formatting.md b/docs/specs/ui/system-design/responsive-plan-formatting.md index 54ea3f9478..9f54599d9c 100644 --- a/docs/specs/ui/system-design/responsive-plan-formatting.md +++ b/docs/specs/ui/system-design/responsive-plan-formatting.md @@ -100,6 +100,23 @@ The desktop BubbleMenu retains its non-empty-selection visibility condition. It consumes the same reactive active-mark snapshot as the docked strip so pressed styling follows selection and command transactions in both variants. +## Render and transaction stability + +The desktop BubbleMenu receives stable option and callback identities when +their values do not change. Tiptap dispatches a metadata transaction when its +menu options change. A new options object on each render can create a feedback +loop between that transaction and the editor snapshot subscription. + +The editor snapshot subscription compares each derived snapshot with the +current snapshot. It publishes state only when focus, selection, code-block +context, or active marks change. Metadata-only transactions do not cause a +render. Selection and formatting transactions still publish the changed +snapshot. + +These two controls keep the desktop selection bubble reactive without making +Tiptap option updates part of the React render cycle. The mobile presentation +uses the same deduplicated editor snapshot. + ## Viewport and scroll contract The docked strip has a stable measured height. Its position resolves as follows: