fix: prevent duplicate junction rows from double-fired checkbox clicks in multi-select menu items (#22737)

## 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 `<span role="checkbox">` plus a hidden `<input>`. 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.  <a href="https://cubic.dev/pr/twentyhq/twenty/pull/22737?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
2026-07-09 15:54:55 +02:00
parent 7475b5f16f
commit a22fdf1d8e
7 changed files with 75 additions and 11 deletions
@@ -0,0 +1,4 @@
+.container {
+  align-items: center;
+  display: flex;
+}
@@ -0,0 +1,4 @@
+declare const classNames: {
+  readonly container: 'container';
+};
+export default classNames;
@@ -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
+    <div
+      className={styles.container}
+      onClick={(event) => event.stopPropagation()}
+    >
+      <Checkbox
+        checked={selected}
+        onCheckedChange={onSelectChange}
+        aria-label={ariaLabel}
+      />
+    </div>
+  );
+};
@@ -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}
    >
      <div className={styles.leftContentWithCheckboxContainer}>
-        <Checkbox checked={selected} aria-label={text} />
+        <MenuItemMultiSelectCheckbox
+          selected={selected}
+          onSelectChange={onSelectChange}
+          ariaLabel={text}
+        />
        {color ? (
          <Tag color={color} text={text} Icon={LeftIcon} />
        ) : (
@@ -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}
    >
      <div className={styles.leftContentWithCheckboxContainer}>
-        <Checkbox
-          checked={selected}
-          aria-label={text ?? contextualText ?? 'Select item'}
+        <MenuItemMultiSelectCheckbox
+          selected={selected}
+          onSelectChange={onSelectChange}
+          ariaLabel={text ?? contextualText ?? 'Select item'}
        />
        <StyledMenuItemLeftContent>
          {avatar}
@@ -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<Story, typeof MenuItemMultiSelectAvatar> = {
  args: { text: 'Menu item' },
  argTypes: {
@@ -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}
    >
      <StyledMenuItemLeftContent>
-        <Checkbox
-          size={CheckboxSize.Small}
-          shape={CheckboxShape.Squared}
-          checked={selected}
+        <MenuItemMultiSelectCheckbox
+          selected={selected}
+          onSelectChange={() => onClick?.()}
+          ariaLabel={text}
        />
        <Tag color={color} text={text} Icon={Icon} />
      </StyledMenuItemLeftContent>