mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(combobox): show selected values in multi-select trigger label (#4721)
* fix(combobox): show selected values in multi-select trigger label
The collapsed trigger was reading only `selectedOption` (the single-value
path) and falling back to the placeholder when nothing matched, so a
multi-select dropdown with 1+ checked items still rendered "Select one or
more channels" instead of the actual selections.
Added `multiSelectLabel` derived from `multiSelectValues`:
- 1 value → that label
- 2 values → "A, B"
- 3+ → "A, B +N"
Trigger now prefers `multiSelectLabel` when present and falls back to the
single-select label / placeholder otherwise. Muted-text color also flips
off when multi has any selection.
* chore(kb-connectors): strip redundant field-level descriptions
Removed 41 inline `description:` lines from configFields across 16 connectors
(Slack, MS Teams, GCal, Gmail, Notion, Linear-adjacent, Discord, Dropbox,
Evernote, Fireflies, Google Sheets, Intercom, Obsidian, Outlook, Reddit,
ServiceNow, WordPress, Zendesk). They mostly restated the field title (e.g.
"Channels to sync messages from" under a "Channels" label) and cluttered
the add/edit modal. Field titles + placeholders already communicate intent.
Connector-level `description` (used in the connector picker grid) is
unchanged.
* test(leader-lock): use fake timers to deterministically test follower polling
The "follower does a final read after timeout" test (and the
"follower returns null after timeout" test) relied on real-clock
`setTimeout` and `Date.now()` with very tight bounds (pollIntervalMs=5,
maxWaitMs=9). Any CI scheduler jitter of >4ms would cause the second
in-loop poll to be skipped, the polls counter to end at 2 instead of 3,
and the assertion `expect(result).toBe('late-leader')` to fail.
Switched both tests to `vi.useFakeTimers()` so the schedule is driven by
mocked time advanced via `vi.advanceTimersByTimeAsync`. The intent is
unchanged — verify that the in-loop deadline triggers exactly one
post-deadline last-chance call to `onFollower` — but the assertions no
longer depend on wall-clock timing.
Verified across 5 sequential runs with zero flakes.
* improvement(kb-connectors): restore field descriptions as info-icon tooltips
Restores the 41 field-level `description` lines stripped in fc644210d, but
instead of rendering them as inline muted-text paragraphs they're shown via a
small Info icon next to each field title. Hovering or focusing the icon
reveals the description in the existing emcn Tooltip. Keeps the modal layout
tight while preserving the per-field guidance.
Used <button type="button"> as the tooltip trigger (Radix asChild) rather
than <span tabIndex={0}> to satisfy a11y/noNoninteractiveTabindex.
This commit is contained in:
+22
-9
@@ -1,7 +1,7 @@
|
||||
'use client'
|
||||
|
||||
import { useMemo, useState } from 'react'
|
||||
import { ArrowLeft, ArrowLeftRight, Plus, Search } from 'lucide-react'
|
||||
import { ArrowLeft, ArrowLeftRight, Info, Plus, Search } from 'lucide-react'
|
||||
import { useParams } from 'next/navigation'
|
||||
import {
|
||||
Button,
|
||||
@@ -347,12 +347,28 @@ export function AddConnectorModal({
|
||||
return (
|
||||
<div key={field.id} className='flex flex-col gap-2'>
|
||||
<div className='flex items-center justify-between'>
|
||||
<Label>
|
||||
{field.title}
|
||||
{field.required && (
|
||||
<span className='ml-0.5 text-[var(--text-error)]'>*</span>
|
||||
<div className='flex items-center gap-1'>
|
||||
<Label>
|
||||
{field.title}
|
||||
{field.required && (
|
||||
<span className='ml-0.5 text-[var(--text-error)]'>*</span>
|
||||
)}
|
||||
</Label>
|
||||
{field.description && (
|
||||
<Tooltip.Root>
|
||||
<Tooltip.Trigger asChild>
|
||||
<button
|
||||
type='button'
|
||||
className='flex size-[14px] cursor-help items-center justify-center text-[var(--text-muted)] transition-colors hover-hover:text-[var(--text-secondary)]'
|
||||
aria-label={`About ${field.title}`}
|
||||
>
|
||||
<Info className='size-[12px]' />
|
||||
</button>
|
||||
</Tooltip.Trigger>
|
||||
<Tooltip.Content side='top'>{field.description}</Tooltip.Content>
|
||||
</Tooltip.Root>
|
||||
)}
|
||||
</Label>
|
||||
</div>
|
||||
{hasCanonicalPair && canonicalId && (
|
||||
<Tooltip.Root>
|
||||
<Tooltip.Trigger asChild>
|
||||
@@ -372,9 +388,6 @@ export function AddConnectorModal({
|
||||
</Tooltip.Root>
|
||||
)}
|
||||
</div>
|
||||
{field.description && (
|
||||
<p className='text-[var(--text-muted)] text-xs'>{field.description}</p>
|
||||
)}
|
||||
{field.type === 'selector' && field.selectorKey ? (
|
||||
<ConnectorSelectorField
|
||||
field={field as ConnectorConfigField & { selectorKey: SelectorKey }}
|
||||
|
||||
+21
-8
@@ -2,7 +2,7 @@
|
||||
|
||||
import { useMemo, useState } from 'react'
|
||||
import { createLogger } from '@sim/logger'
|
||||
import { ArrowLeftRight, ExternalLink, RotateCcw } from 'lucide-react'
|
||||
import { ArrowLeftRight, ExternalLink, Info, RotateCcw } from 'lucide-react'
|
||||
import {
|
||||
Button,
|
||||
ButtonGroup,
|
||||
@@ -385,10 +385,26 @@ function SettingsTab({
|
||||
return (
|
||||
<div key={field.id} className='flex flex-col gap-2'>
|
||||
<div className='flex items-center justify-between'>
|
||||
<Label>
|
||||
{field.title}
|
||||
{field.required && <span className='ml-0.5 text-[var(--text-error)]'>*</span>}
|
||||
</Label>
|
||||
<div className='flex items-center gap-1'>
|
||||
<Label>
|
||||
{field.title}
|
||||
{field.required && <span className='ml-0.5 text-[var(--text-error)]'>*</span>}
|
||||
</Label>
|
||||
{field.description && (
|
||||
<Tooltip.Root>
|
||||
<Tooltip.Trigger asChild>
|
||||
<button
|
||||
type='button'
|
||||
className='flex size-[14px] cursor-help items-center justify-center text-[var(--text-muted)] transition-colors hover-hover:text-[var(--text-secondary)]'
|
||||
aria-label={`About ${field.title}`}
|
||||
>
|
||||
<Info className='size-[12px]' />
|
||||
</button>
|
||||
</Tooltip.Trigger>
|
||||
<Tooltip.Content side='top'>{field.description}</Tooltip.Content>
|
||||
</Tooltip.Root>
|
||||
)}
|
||||
</div>
|
||||
{hasCanonicalPair && canonicalId && (
|
||||
<Tooltip.Root>
|
||||
<Tooltip.Trigger asChild>
|
||||
@@ -406,9 +422,6 @@ function SettingsTab({
|
||||
</Tooltip.Root>
|
||||
)}
|
||||
</div>
|
||||
{field.description && (
|
||||
<p className='text-[var(--text-muted)] text-xs'>{field.description}</p>
|
||||
)}
|
||||
{field.type === 'selector' && field.selectorKey ? (
|
||||
<ConnectorSelectorField
|
||||
field={field as ConnectorConfigField & { selectorKey: SelectorKey }}
|
||||
|
||||
@@ -214,6 +214,22 @@ const Combobox = memo(
|
||||
[allOptions, effectiveSelectedValue]
|
||||
)
|
||||
|
||||
/**
|
||||
* Label rendered in the collapsed trigger for multi-select mode.
|
||||
* Shows the single label when one value is picked, comma-joined labels
|
||||
* for two, or "first, second +N" when more are selected. Falls back to
|
||||
* the raw value if an option for it hasn't loaded yet.
|
||||
*/
|
||||
const multiSelectLabel = useMemo(() => {
|
||||
if (!multiSelect || !multiSelectValues || multiSelectValues.length === 0) return null
|
||||
const labelFor = (v: string) => allOptions.find((opt) => opt.value === v)?.label ?? v
|
||||
if (multiSelectValues.length === 1) return labelFor(multiSelectValues[0])
|
||||
if (multiSelectValues.length === 2) {
|
||||
return `${labelFor(multiSelectValues[0])}, ${labelFor(multiSelectValues[1])}`
|
||||
}
|
||||
return `${labelFor(multiSelectValues[0])}, ${labelFor(multiSelectValues[1])} +${multiSelectValues.length - 2}`
|
||||
}, [multiSelect, multiSelectValues, allOptions])
|
||||
|
||||
/**
|
||||
* Filter options based on current value or search query
|
||||
*/
|
||||
@@ -590,11 +606,11 @@ const Combobox = memo(
|
||||
<span
|
||||
className={cn(
|
||||
'flex-1 truncate',
|
||||
!selectedOption && 'text-[var(--text-muted)]',
|
||||
!selectedOption && !multiSelectLabel && 'text-[var(--text-muted)]',
|
||||
overlayContent && 'text-transparent'
|
||||
)}
|
||||
>
|
||||
{selectedOption ? selectedOption.label : placeholder}
|
||||
{multiSelectLabel ?? (selectedOption ? selectedOption.label : placeholder)}
|
||||
</span>
|
||||
<ChevronDown
|
||||
className={cn(
|
||||
|
||||
@@ -120,39 +120,62 @@ describe('withLeaderLock', () => {
|
||||
it('follower does a final read after timeout to catch a just-finished leader', async () => {
|
||||
redisConfigMockFns.mockAcquireLock.mockResolvedValueOnce(false)
|
||||
|
||||
// pollInterval=5, maxWait=9 → loop exits after 2 in-loop polls (T+5, T+10);
|
||||
// the third call (polls=3) is the post-deadline last-chance read.
|
||||
let polls = 0
|
||||
const onFollower = vi.fn(async () => {
|
||||
polls += 1
|
||||
if (polls <= 2) return null
|
||||
return 'late-leader'
|
||||
})
|
||||
/**
|
||||
* The intent: after the in-loop poll deadline is reached, the follower
|
||||
* does exactly one more (last-chance) `onFollower` call to catch a leader
|
||||
* that finished between the previous poll and the timeout. Using fake
|
||||
* timers makes the timing deterministic — pollInterval=10 and maxWait=15
|
||||
* cause two in-loop polls (T+10, T+20) and one last-chance read (T+20),
|
||||
* but the schedule is driven by mocked time, not the CI wall clock.
|
||||
*/
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
let polls = 0
|
||||
const onFollower = vi.fn(async () => {
|
||||
polls += 1
|
||||
if (polls <= 2) return null
|
||||
return 'late-leader'
|
||||
})
|
||||
|
||||
const result = await withLeaderLock<string>({
|
||||
key: 'k',
|
||||
pollIntervalMs: 5,
|
||||
maxWaitMs: 9,
|
||||
onLeader: async () => 'should-not-run',
|
||||
onFollower,
|
||||
})
|
||||
const promise = withLeaderLock<string>({
|
||||
key: 'k',
|
||||
pollIntervalMs: 10,
|
||||
maxWaitMs: 15,
|
||||
onLeader: async () => 'should-not-run',
|
||||
onFollower,
|
||||
})
|
||||
|
||||
expect(result).toBe('late-leader')
|
||||
expect(onFollower).toHaveBeenCalledTimes(3)
|
||||
await vi.advanceTimersByTimeAsync(30)
|
||||
const result = await promise
|
||||
|
||||
expect(result).toBe('late-leader')
|
||||
expect(onFollower).toHaveBeenCalledTimes(3)
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('follower returns null after timeout', async () => {
|
||||
redisConfigMockFns.mockAcquireLock.mockResolvedValueOnce(false)
|
||||
|
||||
const result = await withLeaderLock<string>({
|
||||
key: 'k',
|
||||
pollIntervalMs: 5,
|
||||
maxWaitMs: 20,
|
||||
onLeader: async () => 'should-not-run',
|
||||
onFollower: async () => null,
|
||||
})
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
const onFollower = vi.fn(async () => null)
|
||||
const promise = withLeaderLock<string>({
|
||||
key: 'k',
|
||||
pollIntervalMs: 10,
|
||||
maxWaitMs: 25,
|
||||
onLeader: async () => 'should-not-run',
|
||||
onFollower,
|
||||
})
|
||||
|
||||
expect(result).toBeNull()
|
||||
await vi.advanceTimersByTimeAsync(50)
|
||||
const result = await promise
|
||||
|
||||
expect(result).toBeNull()
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('only one of N concurrent callers acquires the lock', async () => {
|
||||
|
||||
Reference in New Issue
Block a user