From 99a3b96b6168f7d02d642594d0a3e0a90244721b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 04:10:30 +0000 Subject: [PATCH] [trees] fix sequential right-click closing the context menu (#664) Right-clicking file B while file A's context menu was open closed the menu entirely instead of re-anchoring it onto B. When menu B opened, the superseded menu A's consumer layer (Radix DropdownMenu onOpenChange) fired its async close() and tore down menu B, because closeContextMenu had no way to tell which menu instance requested the close. Give each opened menu a monotonic open-generation token stored on the menu state. Each context.close closure captures the token it was created with, and closeContextMenu no-ops when the captured token is no longer the active one, so a stale close from a superseded menu can no longer dismiss the menu that replaced it. Fixes #664 --- packages/trees/src/render/FileTreeView.tsx | 36 +++++- ...ontext-menu-sequential-right-click.test.ts | 112 ++++++++++++++++++ 2 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 packages/trees/test/file-tree-context-menu-sequential-right-click.test.ts diff --git a/packages/trees/src/render/FileTreeView.tsx b/packages/trees/src/render/FileTreeView.tsx index de219c62b..6d7992990 100644 --- a/packages/trees/src/render/FileTreeView.tsx +++ b/packages/trees/src/render/FileTreeView.tsx @@ -1300,9 +1300,15 @@ export function FileTreeView({ item: FileTreeContextMenuItem; path: string; source: 'button' | 'keyboard' | 'right-click'; + // Monotonic open-generation token identifying this menu instance. A + // superseded menu's async close() carries the token it was opened with, so + // a stale close cannot tear down the menu that replaced it (#664). + token: number; } | null>(null); const contextMenuStateRef = useRef(contextMenuState); contextMenuStateRef.current = contextMenuState; + // Bumped on every open so each opened menu gets a unique identity token. + const contextMenuOpenTokenRef = useRef(0); const pendingStickyFocusPathRef = useRef(null); const pendingStickyKeyboardFocusPathRef = useRef(null); @@ -1576,16 +1582,27 @@ export function FileTreeView({ const restoreFocusToTreeRef = useRef(restoreFocusToTree); restoreFocusToTreeRef.current = restoreFocusToTree; const shouldRestoreContextMenuFocusRef = useRef(true); - const closeContextMenuRef = useRef<(restoreFocus?: boolean) => void>( - () => {} - ); + const closeContextMenuRef = useRef< + (restoreFocus?: boolean, expectedToken?: number) => void + >(() => {}); const closeContextMenu = useCallback( - (restoreFocus: boolean = true): void => { + (restoreFocus: boolean = true, expectedToken?: number): void => { const currentContextMenuState = contextMenuStateRef.current; if (currentContextMenuState == null) { return; } + // Ignore a close() issued by a superseded menu instance. Sequential + // right-clicks open a new menu (new token) before the previous menu's + // consumer layer fires its async onOpenChange(false); only the menu that + // is currently open may close itself (#664). + if ( + expectedToken != null && + expectedToken !== currentContextMenuState.token + ) { + return; + } + shouldRestoreContextMenuFocusRef.current = shouldRestoreContextMenuFocusRef.current && restoreFocus; setContextMenuState(null); @@ -1641,11 +1658,13 @@ export function FileTreeView({ item.focus(); updateTriggerPosition(anchorButton); shouldRestoreContextMenuFocusRef.current = true; + contextMenuOpenTokenRef.current += 1; setContextMenuState({ anchorRect: options?.anchorRect ?? null, item: createContextMenuItem(row, targetPath), path: targetPath, source: options?.source ?? 'keyboard', + token: contextMenuOpenTokenRef.current, }); }, [controller, getTriggerAnchorButton, updateTriggerPosition] @@ -2922,7 +2941,12 @@ export function FileTreeView({ currentState.anchorRect ?? serializeAnchorRect(anchorElement.getBoundingClientRect()), close: (options) => { - closeContextMenuRef.current(options?.restoreFocus ?? true); + // Capture this menu instance's token so a close() from a menu that has + // since been superseded by a sequential right-click is ignored (#664). + closeContextMenuRef.current( + options?.restoreFocus ?? true, + currentState.token + ); }, restoreFocus: () => { if (!shouldRestoreContextMenuFocusRef.current) { @@ -3616,6 +3640,7 @@ export function FileTreeView({ updateTriggerPosition(triggerButton); shouldRestoreContextMenuFocusRef.current = true; + contextMenuOpenTokenRef.current += 1; setContextMenuState({ anchorRect: null, item: { @@ -3625,6 +3650,7 @@ export function FileTreeView({ }, path: triggerItem.getPath(), source: 'button', + token: contextMenuOpenTokenRef.current, }); }; diff --git a/packages/trees/test/file-tree-context-menu-sequential-right-click.test.ts b/packages/trees/test/file-tree-context-menu-sequential-right-click.test.ts new file mode 100644 index 000000000..99790f5e5 --- /dev/null +++ b/packages/trees/test/file-tree-context-menu-sequential-right-click.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, test } from 'bun:test'; +import { JSDOM } from 'jsdom'; + +import type { FileTreeContextMenuOpenContext } from '../src/model/publicTypes'; +import { flushDom, installDom } from './helpers/dom'; + +function getItemButton( + shadowRoot: ShadowRoot | null | undefined, + dom: JSDOM, + path: string +): HTMLButtonElement { + const button = shadowRoot?.querySelector(`[data-item-path="${path}"]`); + if (!(button instanceof dom.window.HTMLButtonElement)) { + throw new Error(`missing button for ${path}`); + } + + return button; +} + +// Regression for #664: right-clicking file B while file A's menu is open must +// re-anchor the menu onto B rather than dismiss it. The failure mode is a +// stale close() from menu A's superseded consumer layer (Radix +// `onOpenChange(false)`) firing asynchronously after menu B has already opened +// and tearing menu B down because the close was not tied to a menu instance. +describe('file-tree sequential right-click context menu', () => { + test('right-clicking a second row re-anchors the menu instead of closing it', async () => { + const { cleanup, dom } = installDom(); + try { + const { FileTree } = await import('../src/render/FileTree'); + const mount = dom.window.document.createElement('div'); + dom.window.document.body.appendChild(mount); + + // Capture each opened menu's context keyed by the row it was opened for, + // so we can later fire the stale close() belonging to menu A. + const contextByPath = new Map(); + + const fileTree = new FileTree({ + composition: { + contextMenu: { + enabled: true, + render: (item, context): HTMLElement => { + contextByPath.set(item.path, context); + const menu = dom.window.document.createElement('div'); + menu.dataset.testMenu = 'true'; + menu.dataset.itemPath = item.path; + return menu as unknown as HTMLElement; + }, + }, + }, + flattenEmptyDirectories: true, + initialExpansion: 'open', + paths: ['a.ts', 'b.ts'], + initialVisibleRowCount: 120 / 30, + }); + + fileTree.render({ containerWrapper: mount }); + await flushDom(); + + const host = fileTree.getFileTreeContainer(); + const shadowRoot = host?.shadowRoot; + + // Open menu A via right-click. + getItemButton(shadowRoot, dom, 'a.ts').dispatchEvent( + new dom.window.MouseEvent('contextmenu', { + bubbles: true, + clientX: 10, + clientY: 10, + }) + ); + await flushDom(); + + let slotted = host?.querySelector( + '[slot="context-menu"][data-test-menu]' + ); + expect(slotted).not.toBeNull(); + expect((slotted as HTMLElement | null)?.dataset.itemPath).toBe('a.ts'); + + // Right-click a different row while menu A is open. Menu B should open + // anchored to b.ts. + getItemButton(shadowRoot, dom, 'b.ts').dispatchEvent( + new dom.window.MouseEvent('contextmenu', { + bubbles: true, + clientX: 20, + clientY: 40, + }) + ); + await flushDom(); + + slotted = host?.querySelector('[slot="context-menu"][data-test-menu]'); + expect(slotted).not.toBeNull(); + expect((slotted as HTMLElement | null)?.dataset.itemPath).toBe('b.ts'); + + // Simulate menu A's superseded consumer layer firing its captured close() + // asynchronously after menu B has opened. + const staleCloseA = contextByPath.get('a.ts'); + if (staleCloseA == null) { + throw new Error('expected captured context for a.ts'); + } + staleCloseA.close(); + await flushDom(2); + + // The menu must still be open and anchored to b.ts. + slotted = host?.querySelector('[slot="context-menu"][data-test-menu]'); + expect(slotted).not.toBeNull(); + expect((slotted as HTMLElement | null)?.dataset.itemPath).toBe('b.ts'); + + fileTree.cleanUp(); + } finally { + cleanup(); + } + }); +});