From e8ec3427e5b889088bf8fc5fef7cdc56c2f6544d Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 23 Sep 2026 16:37:53 +0200 Subject: [PATCH] fix(share): the user picked to invite shows in the field before it is sent The share dialog stored the pick as String(value) while its options are keyed by numeric user ids. CustomSelect looked the pick up with a strict comparison, found nothing and left the trigger on its placeholder, although the Invite button was already enabled and the invite went through. The phone opens the same dialog from the Mehr sheet, so it showed the same. CustomSelect now compares the held value and the option values as text, so a caller holding "5" finds the option keyed 5 and the other way round. A held null or undefined still matches nothing. onChange keeps handing back the option value unchanged. No other caller mixes the two kinds today; the place form had worked around the same trap by keying its options as strings, and its comment now says why the string stays. Tests: CustomSelect matches across both kinds and marks the pick in the list, the desktop dialog shows the picked name and clears after the invite, and the phone share sheet does the same through the real dialog. Refs #2478 --- .../src/components/Planner/PlaceFormModal.tsx | 5 +- .../Trips/TripMembersModal.test.tsx | 22 +++- .../components/shared/CustomSelect.test.tsx | 42 ++++++- client/src/components/shared/CustomSelect.tsx | 22 +++- .../mobile/trip/MTripSheets.members.test.tsx | 103 ++++++++++++++++++ 5 files changed, 185 insertions(+), 9 deletions(-) create mode 100644 client/tests/unit/mobile/trip/MTripSheets.members.test.tsx diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index c2eb7086e..6e3878487 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -1196,9 +1196,8 @@ export default function PlaceFormModal(props: PlaceFormModalProps) { options={[ { value: '', label: t('places.noCategory') }, ...(categories || []).map(c => ({ - // form.category_id is a string; CustomSelect matches options by - // strict equality, so the option value must be a string too — - // otherwise the chosen category never renders in the trigger. + // A string like form.category_id, so the picked option hands + // back the same kind of value the form already keeps. value: String(c.id), label: c.name, })), diff --git a/client/src/components/Trips/TripMembersModal.test.tsx b/client/src/components/Trips/TripMembersModal.test.tsx index d0dfb2e97..d82fa8396 100644 --- a/client/src/components/Trips/TripMembersModal.test.tsx +++ b/client/src/components/Trips/TripMembersModal.test.tsx @@ -1,4 +1,4 @@ -// FE-COMP-MEMBERS-001 to FE-COMP-MEMBERS-056 +// FE-COMP-MEMBERS-001 to FE-COMP-MEMBERS-057 import type { Mock } from 'vitest'; import { act, render, screen, fireEvent, waitFor } from '../../../tests/helpers/render'; import userEvent from '@testing-library/user-event'; @@ -725,6 +725,26 @@ describe('TripMembersModal', () => { expect(addToast).toHaveBeenCalledWith('alice added', 'success', undefined); }); + // #2478: the pick showed nowhere, only the Invite button knew about it. + it('FE-COMP-MEMBERS-057: the picked user shows in the field before the invite is sent', async () => { + const user = userEvent.setup(); + server.use(http.post('/api/trips/1/members', () => HttpResponse.json({ success: true }))); + render(); + + await screen.findByText('Invite User'); + const trigger = screen.getByText('Select user…').closest('button') as HTMLButtonElement; + await user.click(trigger); + await user.click(await screen.findByRole('button', { name: 'alice' })); + + expect(trigger).toHaveTextContent('alice'); + expect(screen.queryByText('Select user…')).toBeNull(); + expect(screen.getByRole('button', { name: 'Invite' })).toBeEnabled(); + + // Once sent, the field is free for the next pick again. + await user.click(screen.getByRole('button', { name: 'Invite' })); + await waitFor(() => expect(trigger).toHaveTextContent('Select user…')); + }); + it('FE-COMP-MEMBERS-042: transferring ownership reloads the app', async () => { const onClose = vi.fn(); const reload = vi.fn(); diff --git a/client/src/components/shared/CustomSelect.test.tsx b/client/src/components/shared/CustomSelect.test.tsx index 2f13fc5b6..da081bbc7 100644 --- a/client/src/components/shared/CustomSelect.test.tsx +++ b/client/src/components/shared/CustomSelect.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, fireEvent } from '../../../tests/helpers/render'; +import { render, screen, within } from '../../../tests/helpers/render'; import userEvent from '@testing-library/user-event'; import CustomSelect from './CustomSelect'; @@ -145,6 +145,46 @@ describe('CustomSelect', () => { expect(menu.style.maxWidth).toBe('180px'); }); + // #2478: the trip share dialog stored String(value) against options keyed by numeric + // user ids, so the pick never showed in the trigger although it was made. + const USERS = [ + { value: 7, label: 'alice' }, + { value: 12, label: 'bob' }, + ]; + + it('FE-COMP-SELECT-014: a value held as text finds the option keyed by the same number', async () => { + const user = userEvent.setup(); + render(); + + const trigger = screen.getByRole('button'); + expect(trigger).toHaveTextContent('bob'); + expect(screen.queryByText('Pick a user')).toBeNull(); + + // The open list marks the same option with its check, and only that one. + await user.click(trigger); + const menu = within(screen.getByText('alice').closest('div[style*="position: fixed"]') as HTMLElement); + expect(menu.getByRole('button', { name: 'bob' }).querySelector('svg')).not.toBeNull(); + expect(menu.getByRole('button', { name: 'alice' }).querySelector('svg')).toBeNull(); + }); + + it('FE-COMP-SELECT-015: a numeric value finds the option keyed by the same text', () => { + render(); + expect(screen.getByRole('button')).toHaveTextContent('March'); + }); + + it('FE-COMP-SELECT-016: a caller holding nothing still sees the placeholder', () => { + render(); + expect(screen.getByRole('button')).toHaveTextContent('Pick a user'); + }); + + it('FE-COMP-SELECT-017: picking hands back the option value as it is', async () => { + const user = userEvent.setup(); + render(); + await user.click(screen.getByRole('button')); + await user.click(screen.getByRole('button', { name: 'alice' })); + expect(onChange).toHaveBeenCalledWith(7); + }); + it('FE-COMP-SELECT-008: disabled state prevents the dropdown from opening', async () => { const user = userEvent.setup(); render(); diff --git a/client/src/components/shared/CustomSelect.tsx b/client/src/components/shared/CustomSelect.tsx index 6c02c27d1..82932e8b3 100644 --- a/client/src/components/shared/CustomSelect.tsx +++ b/client/src/components/shared/CustomSelect.tsx @@ -4,8 +4,8 @@ import { ChevronDown, Check } from 'lucide-react' import { useAnchoredPosition, scrollAnchorIntoView } from '../../hooks/useAnchoredPosition' interface SelectOption { - // Callers use both string keys and numeric ids (e.g. day/place ids) as values; - // the component only does strict-equality lookups and key rendering, so either works. + // Callers use both string keys and numeric ids (e.g. day/place ids) as values, and + // the value they hold need not be of the same kind as the options: see sameValue. value: string | number label: string icon?: React.ReactNode @@ -36,6 +36,20 @@ interface CustomSelectProps { menuFit?: 'anchor' | 'content' } +/** + * Whether an option is the one the caller holds. + * + * onChange hands back the option's own value, and callers routinely store it in another + * form: the trip share dialog kept String(value) against options keyed by numeric user + * ids, so a strict comparison never found the pick and the trigger stayed on its + * placeholder while the invite button already knew who was chosen (#2478). An id reads + * the same as a number or as text, so both sides are compared as text. A caller holding + * nothing (null or undefined at runtime) matches no option. + */ +function sameValue(held: string | number | null | undefined, option: string | number): boolean { + return held != null && String(held) === String(option) +} + /** How wide a content-fitted menu may get before it stops reading as a menu. */ const MENU_MAX_WIDTH = 420 @@ -92,7 +106,7 @@ export default function CustomSelect({ return () => document.removeEventListener('mousedown', handleClick) }, [open]) - const selected = options.find(o => o.value === value) + const selected = options.find(o => sameValue(value, o.value)) const filtered = searchable && search ? (() => { const q = search.toLowerCase() @@ -232,7 +246,7 @@ export default function CustomSelect({ ) } - const isSelected = option.value === value + const isSelected = sameValue(value, option.value) return (