From a22fdf1d8e048911b7e599f4b10e34074012fcce Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Thu, 9 Jul 2026 15:54:55 +0200 Subject: [PATCH] fix: prevent duplicate junction rows from double-fired checkbox clicks in multi-select menu items (#22737) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Fixes a bug where selecting a relation from the record picker created **two** rows in a junction object instead of one (issue #22698). ## Root cause Base UI's `Checkbox` renders a styled `` plus a hidden ``. On click of the span it re-dispatches a second, *bubbling* click on the hidden input (to keep the native input in sync) without stopping propagation. Both the original span click and the re-dispatched input click bubble up to the menu-item row, whose `onClick` drives selection — so one physical click on the checkbox fired the row's handler **twice**. This was invisible until now because every consumer's handler was idempotent: - multi-select toggle: both calls pass the same `!selected`, net one toggle - relation attach: setting a foreign key twice is the same result The junction relation feature is the first non-idempotent consumer: each call creates a new junction record with a fresh UUID, so two calls produced two rows. ## Fix Extract a shared `MenuItemMultiSelectCheckbox` part that: - drives selection through the checkbox's own `onCheckedChange` (events up) - wraps the checkbox so its click cannot propagate to the row's `onClick` The row stays clickable for the rest of the item; the checkbox click and the row click are now two clean, single-fire event sources. Applied to all three affected components (`MenuItemMultiSelect`, `MenuItemMultiSelectAvatar`, `MenuItemMultiSelectTag`) so the whole class of bug is fixed once, not patched per component. No change to the shared `Checkbox` API. ## Notes for reviewer - The `oxlint-disable` for the stopPropagation wrapper's `onClick` now lives in exactly one place (the shared part), following the existing precedent in `OverflowingTextWithTooltip`. - Added a regression interaction test on the `MenuItemMultiSelectAvatar` story (one checkbox click = one `onSelectChange`); it exercises the shared part. - `typecheck`, `oxlint`, and `oxfmt` pass. The Storybook vitest-browser runner is currently broken locally for all stories, so the interaction test was not run locally — it runs in CI. Review in cubic --- .../MenuItemMultiSelectCheckbox.module.scss | 4 +++ ...nuItemMultiSelectCheckbox.module.scss.d.ts | 4 +++ .../parts/MenuItemMultiSelectCheckbox.tsx | 32 +++++++++++++++++++ .../MenuItemMultiSelect.tsx | 8 +++-- .../MenuItemMultiSelectAvatar.tsx | 9 +++--- .../MenuItemMultiSelectAvatar.stories.tsx | 19 +++++++++++ .../MenuItemMultiSelectTag.tsx | 10 +++--- 7 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss create mode 100644 packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss.d.ts create mode 100644 packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.tsx diff --git a/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss new file mode 100644 index 0000000000..7775d22c6b --- /dev/null +++ b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss @@ -0,0 +1,4 @@ +.container { + align-items: center; + display: flex; +} diff --git a/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss.d.ts b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss.d.ts new file mode 100644 index 0000000000..8df71b4dea --- /dev/null +++ b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.module.scss.d.ts @@ -0,0 +1,4 @@ +declare const classNames: { + readonly container: 'container'; +}; +export default classNames; diff --git a/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.tsx b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.tsx new file mode 100644 index 0000000000..29e3e69566 --- /dev/null +++ b/packages/twenty-ui/src/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox.tsx @@ -0,0 +1,32 @@ +import { Checkbox } from '@ui/input/Checkbox/Checkbox'; + +import styles from './MenuItemMultiSelectCheckbox.module.scss'; + +type MenuItemMultiSelectCheckboxProps = { + selected: boolean; + onSelectChange?: (selected: boolean) => void; + ariaLabel?: string; +}; + +export const MenuItemMultiSelectCheckbox = ({ + selected, + onSelectChange, + ariaLabel, +}: MenuItemMultiSelectCheckboxProps) => { + // The checkbox handles its own toggle via onCheckedChange. Base UI + // re-dispatches a bubbling click on its hidden input, so we stop propagation + // here to keep the surrounding row's onClick from toggling twice. + return ( + // oxlint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events +
event.stopPropagation()} + > + +
+ ); +}; diff --git a/packages/twenty-ui/src/navigation/MenuItemMultiSelect/MenuItemMultiSelect.tsx b/packages/twenty-ui/src/navigation/MenuItemMultiSelect/MenuItemMultiSelect.tsx index 20e3a896ee..53c8aeb1bf 100644 --- a/packages/twenty-ui/src/navigation/MenuItemMultiSelect/MenuItemMultiSelect.tsx +++ b/packages/twenty-ui/src/navigation/MenuItemMultiSelect/MenuItemMultiSelect.tsx @@ -1,7 +1,7 @@ import { Tag } from '@ui/data-display'; import { type IconComponent } from '@ui/icon'; -import { Checkbox } from '@ui/input/Checkbox/Checkbox'; import { MenuItemLeftContent } from '@ui/navigation/MenuItem/parts/MenuItemLeftContent'; +import { MenuItemMultiSelectCheckbox } from '@ui/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox'; import { type ThemeColor } from '@ui/theme'; import { StyledMenuItemBase } from '@ui/navigation/MenuItem/parts/StyledMenuItemBase'; @@ -41,7 +41,11 @@ export const MenuItemMultiSelect = ({ onClick={handleOnClick} >
- + {color ? ( ) : ( diff --git a/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/MenuItemMultiSelectAvatar.tsx b/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/MenuItemMultiSelectAvatar.tsx index f18396268e..c0963e05c3 100644 --- a/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/MenuItemMultiSelectAvatar.tsx +++ b/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/MenuItemMultiSelectAvatar.tsx @@ -1,7 +1,7 @@ import { type ReactNode } from 'react'; import { OverflowingTextWithTooltip } from '@ui/surfaces'; -import { Checkbox } from '@ui/input/Checkbox/Checkbox'; +import { MenuItemMultiSelectCheckbox } from '@ui/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox'; import { StyledMenuItemBase, StyledMenuItemLabel, @@ -41,9 +41,10 @@ export const MenuItemMultiSelectAvatar = ({ isKeySelected={isKeySelected} >
- {avatar} diff --git a/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/__stories__/MenuItemMultiSelectAvatar.stories.tsx b/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/__stories__/MenuItemMultiSelectAvatar.stories.tsx index d70b633dd4..e23c8c49dd 100644 --- a/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/__stories__/MenuItemMultiSelectAvatar.stories.tsx +++ b/packages/twenty-ui/src/navigation/MenuItemMultiSelectAvatar/__stories__/MenuItemMultiSelectAvatar.stories.tsx @@ -1,4 +1,5 @@ import { type Meta, type StoryObj } from '@storybook/react-vite'; +import { expect, fn, userEvent, within } from 'storybook/test'; import { Avatar } from '@ui/data-display'; import { @@ -31,6 +32,24 @@ export const Default: Story = { decorators: [ComponentDecorator], }; +export const ClickingCheckboxSelectsOnce: Story = { + parameters: { a11y: A11Y_DEFER_COLOR_CONTRAST }, + args: { + text: 'First option', + selected: false, + onSelectChange: fn(), + }, + decorators: [ComponentDecorator], + play: async ({ args, canvasElement }) => { + const canvas = within(canvasElement); + + await userEvent.click(canvas.getByRole('checkbox')); + + await expect(args.onSelectChange).toHaveBeenCalledTimes(1); + await expect(args.onSelectChange).toHaveBeenCalledWith(true); + }, +}; + export const Catalog: CatalogStory = { args: { text: 'Menu item' }, argTypes: { diff --git a/packages/twenty-ui/src/navigation/MenuItemMultiSelectTag/MenuItemMultiSelectTag.tsx b/packages/twenty-ui/src/navigation/MenuItemMultiSelectTag/MenuItemMultiSelectTag.tsx index 26e662c65c..5abe2ca233 100644 --- a/packages/twenty-ui/src/navigation/MenuItemMultiSelectTag/MenuItemMultiSelectTag.tsx +++ b/packages/twenty-ui/src/navigation/MenuItemMultiSelectTag/MenuItemMultiSelectTag.tsx @@ -1,7 +1,7 @@ import { Tag } from '@ui/data-display'; import { type IconComponent } from '@ui/icon'; -import { Checkbox, CheckboxShape, CheckboxSize } from '@ui/input'; import { type ThemeColor } from '@ui/theme'; +import { MenuItemMultiSelectCheckbox } from '@ui/navigation/MenuItem/parts/MenuItemMultiSelectCheckbox'; import { StyledMenuItemBase, StyledMenuItemLeftContent, @@ -33,10 +33,10 @@ export const MenuItemMultiSelectTag = ({ className={className} > - onClick?.()} + ariaLabel={text} />