Merge pull request #1095 from objectstack-ai/copilot/fix-popup-selection-feedback

hotlong · web-flow · commit 94f85c77fc1b · 2026-03-19T18:29:18.000+08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **Lookup Field Selection Display Fix** (`@object-ui/fields`): Fixed a bug where selecting a record from the RecordPickerDialog (Level 2 popup) produced no visible feedback in the LookupField. The root cause: `findOption` only searched static and popover-fetched options, which did not include records loaded by the dialog. Added `onSelectRecords` callback to `RecordPickerDialogProps` that returns full record objects alongside IDs. LookupField now caches selected records from the dialog and displays their labels/badges correctly. Both single-select and multi-select modes are supported. Includes a `selectedRecordsMap` ref in RecordPickerDialog that persists selected record data across page navigation for multi-select scenarios.
+
 - **RecordPickerDialog UI/UX Overhaul** (`@object-ui/fields`): Major enterprise-grade improvements referencing mainstream low-code platforms:
   - **Skeleton Loading Screen**: Replaced simple spinner with a table-shaped skeleton screen during initial data load, matching the column layout for a polished loading experience.
   - **Sticky Table Header**: Table header now sticks to the top during vertical scroll, keeping column labels visible at all times.
diff --git a/packages/fields/src/record-picker.test.tsx b/packages/fields/src/record-picker.test.tsx
@@ -1451,3 +1451,246 @@ describe('RecordPickerDialog — Loading Overlay', () => {
     });
   });
 });
+
+// ------------- RecordPickerDialog — onSelectRecords callback -------------
+
+describe('RecordPickerDialog — onSelectRecords callback', () => {
+  const basePickerProps = {
+    open: true,
+    onOpenChange: vi.fn(),
+    dataSource: mockDataSource as any,
+    objectName: 'customers',
+    onSelect: vi.fn(),
+  };
+
+  it('calls onSelectRecords with the full record on single-select', async () => {
+    const onSelect = vi.fn();
+    const onSelectRecords = vi.fn();
+    const onOpenChange = vi.fn();
+
+    mockDataSource.find.mockResolvedValue({
+      data: [
+        { id: '1', name: 'Acme Corp', email: 'acme@test.com' },
+        { id: '2', name: 'Beta Inc', email: 'beta@test.com' },
+      ],
+      total: 2,
+    });
+
+    render(
+      <RecordPickerDialog
+        {...basePickerProps}
+        onSelect={onSelect}
+        onSelectRecords={onSelectRecords}
+        onOpenChange={onOpenChange}
+      />,
+    );
+
+    await waitFor(() => {
+      expect(screen.getByText('Acme Corp')).toBeInTheDocument();
+    });
+
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-1'));
+    });
+
+    expect(onSelect).toHaveBeenCalledWith('1');
+    expect(onSelectRecords).toHaveBeenCalledWith([
+      expect.objectContaining({ id: '1', name: 'Acme Corp' }),
+    ]);
+    expect(onOpenChange).toHaveBeenCalledWith(false);
+  });
+
+  it('calls onSelectRecords with full records on multi-select confirm', async () => {
+    const onSelect = vi.fn();
+    const onSelectRecords = vi.fn();
+    const onOpenChange = vi.fn();
+
+    mockDataSource.find.mockResolvedValue({
+      data: [
+        { id: '1', name: 'Alpha' },
+        { id: '2', name: 'Beta' },
+        { id: '3', name: 'Gamma' },
+      ],
+      total: 3,
+    });
+
+    render(
+      <RecordPickerDialog
+        {...basePickerProps}
+        multiple
+        onSelect={onSelect}
+        onSelectRecords={onSelectRecords}
+        onOpenChange={onOpenChange}
+      />,
+    );
+
+    await waitFor(() => {
+      expect(screen.getByText('Alpha')).toBeInTheDocument();
+    });
+
+    // Select two records
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-1'));
+    });
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-3'));
+    });
+
+    // Click confirm
+    await act(async () => {
+      fireEvent.click(screen.getByText('Confirm'));
+    });
+
+    expect(onSelect).toHaveBeenCalledWith(expect.arrayContaining(['1', '3']));
+    expect(onSelectRecords).toHaveBeenCalledWith(
+      expect.arrayContaining([
+        expect.objectContaining({ id: '1', name: 'Alpha' }),
+        expect.objectContaining({ id: '3', name: 'Gamma' }),
+      ]),
+    );
+    expect(onOpenChange).toHaveBeenCalledWith(false);
+  });
+});
+
+// ------------- LookupField — Selection Display After RecordPickerDialog -------------
+
+describe('LookupField — Selection Display After RecordPickerDialog', () => {
+  const mockField = {
+    name: 'customer',
+    label: 'Customer',
+    reference_to: 'customers',
+    reference_field: 'name',
+  } as any;
+
+  it('displays selected record label after single-select in RecordPickerDialog', async () => {
+    const onChange = vi.fn();
+    let currentValue: any = undefined;
+
+    mockDataSource.find.mockResolvedValue({
+      data: Array.from({ length: 50 }, (_, i) => ({
+        id: String(i + 1),
+        name: `Customer ${i + 1}`,
+      })),
+      total: 200,
+    });
+
+    const { rerender } = render(
+      <LookupField
+        field={mockField}
+        value={currentValue}
+        onChange={onChange}
+        readonly={false}
+        dataSource={mockDataSource as any}
+      />,
+    );
+
+    // Open RecordPickerDialog via "Browse All" button
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('browse-all-records'));
+    });
+
+    await waitFor(() => {
+      expect(screen.getByTestId('record-picker-dialog')).toBeInTheDocument();
+    });
+
+    // Wait for records to load
+    await waitFor(() => {
+      expect(screen.getByText('Customer 1')).toBeInTheDocument();
+    });
+
+    // Select a record (single-select — should immediately close)
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-1'));
+    });
+
+    expect(onChange).toHaveBeenCalledWith('1');
+
+    // Simulate parent re-rendering with the new value
+    currentValue = '1';
+    rerender(
+      <LookupField
+        field={mockField}
+        value={currentValue}
+        onChange={onChange}
+        readonly={false}
+        dataSource={mockDataSource as any}
+      />,
+    );
+
+    // The selected record's name should be visible as a badge
+    await waitFor(() => {
+      expect(screen.getByText('Customer 1')).toBeInTheDocument();
+    });
+  });
+
+  it('displays selected record badges after multi-select in RecordPickerDialog', async () => {
+    const multiField = { ...mockField, multiple: true } as any;
+    const onChange = vi.fn();
+    let currentValue: any = [];
+
+    mockDataSource.find.mockResolvedValue({
+      data: Array.from({ length: 50 }, (_, i) => ({
+        id: String(i + 1),
+        name: `Customer ${i + 1}`,
+      })),
+      total: 200,
+    });
+
+    const { rerender } = render(
+      <LookupField
+        field={multiField}
+        value={currentValue}
+        onChange={onChange}
+        readonly={false}
+        dataSource={mockDataSource as any}
+      />,
+    );
+
+    // Open RecordPickerDialog via "Browse All" button
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('browse-all-records'));
+    });
+
+    await waitFor(() => {
+      expect(screen.getByTestId('record-picker-dialog')).toBeInTheDocument();
+    });
+
+    // Wait for records to load
+    await waitFor(() => {
+      expect(screen.getByText('Customer 1')).toBeInTheDocument();
+    });
+
+    // Select two records (multi-select — needs confirm)
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-1'));
+    });
+    await act(async () => {
+      fireEvent.click(screen.getByTestId('record-row-3'));
+    });
+
+    // Click confirm
+    await act(async () => {
+      fireEvent.click(screen.getByText('Confirm'));
+    });
+
+    expect(onChange).toHaveBeenCalledWith(expect.arrayContaining(['1', '3']));
+
+    // Simulate parent re-rendering with the new value
+    currentValue = ['1', '3'];
+    rerender(
+      <LookupField
+        field={multiField}
+        value={currentValue}
+        onChange={onChange}
+        readonly={false}
+        dataSource={mockDataSource as any}
+      />,
+    );
+
+    // Both selected records should be visible as badges
+    await waitFor(() => {
+      expect(screen.getByText('Customer 1')).toBeInTheDocument();
+      expect(screen.getByText('Customer 3')).toBeInTheDocument();
+    });
+  });
+});
diff --git a/packages/fields/src/widgets/LookupField.tsx b/packages/fields/src/widgets/LookupField.tsx
@@ -97,6 +97,11 @@ export function LookupField({ value, onChange, field, readonly, ...props }: Fiel
   const [totalCount, setTotalCount] = useState(0);
   const debounceTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
 
+  // Records selected via RecordPickerDialog (Level 2).
+  // Stored as LookupOption so that findOption can resolve display labels
+  // even when the record wasn't part of the Level 1 popover fetch.
+  const [pickerResolvedRecords, setPickerResolvedRecords] = useState<LookupOption[]>([]);
+
   // Arrow-key active index (-1 = none)
   const [activeIndex, setActiveIndex] = useState(-1);
   const listRef = useRef<HTMLDivElement>(null);
@@ -255,15 +260,16 @@ export function LookupField({ value, onChange, field, readonly, ...props }: Fiel
     };
   }, []);
 
-  // Get selected option(s) — check both static and fetched options
+  // Get selected option(s) — check static, fetched, and picker-resolved options
   const findOption = useCallback(
     (v: any): LookupOption | undefined => {
       return (
         staticOptions.find(opt => opt.value === v) ??
-        fetchedOptions.find(opt => opt.value === v)
+        fetchedOptions.find(opt => opt.value === v) ??
+        pickerResolvedRecords.find(opt => opt.value === v)
       );
     },
-    [staticOptions, fetchedOptions],
+    [staticOptions, fetchedOptions, pickerResolvedRecords],
   );
 
   const selectedOptions = multiple
@@ -298,6 +304,16 @@ export function LookupField({ value, onChange, field, readonly, ...props }: Fiel
     }
   };
 
+  // Callback from RecordPickerDialog — caches selected records so that
+  // findOption can resolve display labels after the dialog closes.
+  const handlePickerSelectRecords = useCallback(
+    (records: any[]) => {
+      const mapped = records.map(r => recordToOption(r, displayField, idField, descriptionField));
+      setPickerResolvedRecords(mapped);
+    },
+    [displayField, idField, descriptionField],
+  );
+
   // Keyboard handler for the search input — arrow keys + Enter
   const handleSearchKeyDown = useCallback(
     (e: React.KeyboardEvent) => {
@@ -574,6 +590,7 @@ export function LookupField({ value, onChange, field, readonly, ...props }: Fiel
           pageSize={lookupPageSize}
           value={value}
           onSelect={onChange}
+          onSelectRecords={handlePickerSelectRecords}
           lookupFilters={lookupFilters}
           cellRenderer={getCellRendererResolver()}
           filterColumns={filterColumns}
diff --git a/packages/fields/src/widgets/RecordPickerDialog.tsx b/packages/fields/src/widgets/RecordPickerDialog.tsx
@@ -237,6 +237,13 @@ export interface RecordPickerDialogProps {
   /** Called when selection changes */
   onSelect: (value: any) => void;
 
+  /**
+   * Called with the full record objects corresponding to the selected value(s).
+   * Useful for parent components (e.g. LookupField) that need display data
+   * (labels, descriptions) for the selected records beyond just their IDs.
+   */
+  onSelectRecords?: (records: any[]) => void;
+
   /**
    * Base filters applied to every query.
    * Converted from LookupFieldMetadata.lookup_filters.
@@ -313,6 +320,7 @@ export function RecordPickerDialog({
   pageSize = DEFAULT_PAGE_SIZE,
   value,
   onSelect,
+  onSelectRecords,
   lookupFilters,
   cellRenderer,
   filterColumns,
@@ -334,6 +342,11 @@ export function RecordPickerDialog({
   // For multi-select, track pending selections before confirming
   const [pendingSelection, setPendingSelection] = useState<Set<any>>(new Set());
 
+  // Cache selected record objects across page navigations (multi-select).
+  // When the user toggles a row the full record is stored here so that
+  // onSelectRecords can return complete objects even after paging away.
+  const selectedRecordsMap = useRef<Map<any, any>>(new Map());
+
   // Keyboard navigation: focused row index
   const [focusedRow, setFocusedRow] = useState(-1);
   const tableBodyRef = useRef<HTMLTableSectionElement>(null);
@@ -479,6 +492,7 @@ export function RecordPickerDialog({
       setPendingSelection(new Set(
         multiple ? (Array.isArray(value) ? value : []) : [],
       ));
+      selectedRecordsMap.current.clear();
     }
     // Intentionally depends only on `open` — `multiple` and `value` are
     // captured at close-time and don't need to trigger resets while closed.
@@ -570,25 +584,34 @@ export function RecordPickerDialog({
           const next = new Set(prev);
           if (next.has(rid)) {
             next.delete(rid);
+            selectedRecordsMap.current.delete(rid);
           } else {
             next.add(rid);
+            selectedRecordsMap.current.set(rid, record);
           }
           return next;
         });
       } else {
         // Single select — immediately close
         onSelect(rid);
+        onSelectRecords?.([record]);
         onOpenChange(false);
       }
     },
-    [multiple, getRecordId, onSelect, onOpenChange],
+    [multiple, getRecordId, onSelect, onSelectRecords, onOpenChange],
   );
 
   // Confirm multi-select
   const handleConfirm = useCallback(() => {
-    onSelect(Array.from(pendingSelection));
+    const ids = Array.from(pendingSelection);
+    onSelect(ids);
+    // Build the full record array from the cache for the caller
+    const selectedRecords = ids
+      .map(id => selectedRecordsMap.current.get(id))
+      .filter(Boolean);
+    onSelectRecords?.(selectedRecords);
     onOpenChange(false);
-  }, [pendingSelection, onSelect, onOpenChange]);
+  }, [pendingSelection, onSelect, onSelectRecords, onOpenChange]);
 
   // Page navigation
   const handlePrevPage = useCallback(() => {
