Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Fetch records from DataSource.find() using reference_to as resource - Support $search query parameter with 300ms debounce - Show loading spinner, error state with retry, and empty state - Map fetched records to LookupOption via reference_field (default: name) - Display total count hint when more results are available - Maintain backward compatibility with static options - Add keyboard Enter/Space to select support - Use Tailwind semantic tokens (accent, muted-foreground, destructive) - Add 8 new tests covering all dynamic loading scenarios Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Add explanatory comments for eslint-disable-next-line exhaustive-deps - Include onChange in handleKeyDown dependency array - Use aria-live="polite" instead of aria-label for loading status - Reduce test timeout from 1000ms to 500ms for debounced search Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 仔细评估是否已经完成了任务中要求的所有开发 |
…s, description, create-new - Read DataSource from SchemaRendererContext (same pattern as plugin-view) so lookup works automatically in forms without explicit dataSource prop - Arrow key (Up/Down) navigation with Enter to select - Description field display via `description_field` config - Quick-create entry (onCreateNew callback) when no results found - ARIA listbox/option roles for accessibility - Active item auto-scroll into view - Updated lookup.mdx documentation with all new features - Updated ROADMAP.md - 3 new tests (description, create-new, arrow keys), all 357 field tests pass Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Use targeted data-lookup-index selector instead of querySelectorAll - Fix test comment to clarify activeIndex starts at -1 - Fix docs sentence readability Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Completed a thorough review. The initial implementation was missing several items from the issue requirements. Here's what was added in commit Critical fix:
Issue requirement coverage:
Also added: ARIA listbox/option roles, updated |
Fixed in The fix: the form renderer in DataSource flow is now: |
|
@copilot 继续仔细排查，目前还是选择不了 |
…ieldRenderer wrapper When LookupField is rendered inside a form via createFieldRenderer, the actual objectSchema field metadata (reference_to, display_field, id_field, etc.) is nested at field.field instead of being at the top level. LookupField now unwraps this nested structure so that reference_to is correctly resolved, enabling dataSource.find() to load lookup options. Also adds a test covering the wrapped-field scenario. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…eedback Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Found and fixed the root cause in When LookupField is rendered inside a form, it goes through the Since The fix: LookupField now unwraps the nested structure — if const innerField = lookupField?.field;
const fieldMeta = (innerField && typeof innerField === 'object' && ('reference_to' in innerField || 'type' in innerField))
? innerField
: lookupField;DataSource flow works as designed: |
|
@copilot 从头开始仔细排查每一个环节，目前还是不行 |
The ObjectStack backend uses `reference` (not `reference_to`) as the property name for lookup field metadata. The entire codebase uses the pattern `fieldDef.reference_to || fieldDef.reference` to handle both conventions, but LookupField only checked `reference_to`. This caused `referenceTo` to always be `undefined` when using the ObjectStack backend, so `hasDataSource` was `false` and `dataSource.find()` was never called — the dialog stayed empty. Fixes: - fieldMeta unwrapping now detects both `reference_to` and `reference` in the nested field object - referenceTo falls back: `fieldMeta.reference_to || fieldMeta.reference` - Added 2 new tests for the `reference` convention (nested + flat) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Found the root cause in Fixes:
All 23 tests pass, 0 CodeQL alerts. |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the LookupField experience to support dynamic option loading (via DataSource.find()), richer option rendering, keyboard navigation, quick-create, and improved states (loading/error/empty) so lookup dialogs work with real backends (not only static options) and integrate cleanly with the console form renderer.
Changes:
- Enhanced
LookupFieldto fetch options dynamically from aDataSource, with debounced search, pagination hint, keyboard navigation, and quick-create UI. - Updated the form renderer to read
dataSourcefromSchemaRendererContextand pass it down to field widgets. - Expanded documentation and roadmap to reflect the new Lookup capabilities.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fields/src/widgets/LookupField.tsx | Adds dynamic DataSource-based fetching, debounced search, keyboard nav, quick-create, and wrapper field metadata unwrapping. |
| packages/fields/src/complex-widgets.test.tsx | Adds tests for dynamic Lookup behavior (fetching, states, search debounce, selection, wrapper metadata, ObjectStack reference). |
| packages/components/src/renderers/form/form.tsx | Propagates dataSource from SchemaRendererContext down into rendered field widgets. |
| content/docs/fields/lookup.mdx | Documents dynamic DataSource loading, search behavior, description field, keyboard nav, and quick-create. |
| ROADMAP.md | Marks Lookup dynamic loading + UX enhancements as completed. |
| const description = descriptionField ? record[descriptionField] : undefined; | ||
| return { value: val, label: String(label), description, ...record }; |
There was a problem hiding this comment.
recordToOption() sets description directly from record[descriptionField] and then renders it as React text; if that field is an object/array this will throw ("Objects are not valid as a React child"). Consider coercing description to a string (or omitting non-primitive values) and avoid spreading the full record into state unless it’s needed, since it can be large/non-serializable.
| const description = descriptionField ? record[descriptionField] : undefined; | |
| return { value: val, label: String(label), description, ...record }; | |
| const rawDescription = descriptionField ? record[descriptionField] : undefined; | |
| const description = | |
| typeof rawDescription === 'string' || | |
| typeof rawDescription === 'number' || | |
| typeof rawDescription === 'boolean' | |
| ? String(rawDescription) | |
| : undefined; | |
| return { ...record, value: val, label: String(label), description }; |
| // Fetch data when dialog opens. | ||
| // We intentionally depend only on `isOpen` so the effect fires once per | ||
| // open/close transition. `fetchLookupData` is stable-enough via its own | ||
| // useCallback deps; including it here would cause spurious re-fetches. | ||
| useEffect(() => { | ||
| if (isOpen && hasDataSource) { | ||
| fetchLookupData(searchQuery || undefined); | ||
| } | ||
| // Clean up fetched data when dialog closes | ||
| if (!isOpen) { | ||
| setSearchQuery(''); | ||
| setError(null); | ||
| setActiveIndex(-1); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isOpen]); | ||
|
|
||
| // Debounced search | ||
| const handleSearchChange = useCallback( | ||
| (query: string) => { | ||
| setSearchQuery(query); | ||
|
|
||
| if (!hasDataSource) return; | ||
|
|
||
| if (debounceTimer.current) { | ||
| clearTimeout(debounceTimer.current); | ||
| } | ||
| debounceTimer.current = setTimeout(() => { | ||
| fetchLookupData(query || undefined); | ||
| }, 300); | ||
| }, | ||
| [hasDataSource, fetchLookupData], | ||
| ); | ||
|
|
There was a problem hiding this comment.
When the dialog closes, the debounce timer isn’t cleared and fetchedOptions/totalCount aren’t reset. This can trigger fetches after close and show stale options immediately on the next open while a new request is in-flight. Clearing any pending debounce on close and resetting fetched state (or guarding fetchLookupData with isOpen) would prevent these stale/late updates.
|
|
||
| // Optional create-new callback | ||
| const onCreateNew: ((searchQuery: string) => void) | undefined = | ||
| (props as any).onCreateNew ?? lookupField?.onCreateNew; |
There was a problem hiding this comment.
onCreateNew is read from props or lookupField, but not from fieldMeta. In the createFieldRenderer wrapper case, quick-create configured on the inner field.field metadata won’t be detected. Consider falling back to fieldMeta?.onCreateNew as well so wrapper and non-wrapper usage behave consistently.
| (props as any).onCreateNew ?? lookupField?.onCreateNew; | |
| (props as any).onCreateNew ?? lookupField?.onCreateNew ?? fieldMeta?.onCreateNew; |
| options: fieldProps.options, | ||
| placeholder: fieldProps.placeholder, | ||
| disabled: disabled || fieldDisabled || readonly || isSubmitting, | ||
| dataSource: contextDataSource, | ||
| })} |
There was a problem hiding this comment.
Passing dataSource: contextDataSource here will override any per-field dataSource present in fieldProps and also forwards dataSource to built-in inputs/selects (which can end up as an unknown DOM prop). Prefer preserving precedence (fieldProps.dataSource ?? contextDataSource) and only including the prop when defined (e.g., conditional spread) so standard DOM inputs don’t receive it.
| mockDataSource.find.mockResolvedValue({ data: [], total: 0 }); | ||
|
|
||
| render(<LookupField {...dynamicProps} />); | ||
|
|
||
| await act(async () => { | ||
| fireEvent.click(screen.getByRole('button', { name: /Select/i })); | ||
| }); | ||
|
|
||
| // Wait for initial load | ||
| await waitFor(() => { | ||
| expect(mockDataSource.find).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| // Type in search | ||
| await act(async () => { | ||
| fireEvent.change(screen.getByPlaceholderText('Search...'), { | ||
| target: { value: 'acme' }, | ||
| }); | ||
| }); | ||
|
|
||
| // Wait for debounced search | ||
| await waitFor(() => { | ||
| expect(mockDataSource.find).toHaveBeenCalledWith('customers', { | ||
| $top: 50, | ||
| $search: 'acme', | ||
| }); | ||
| }, { timeout: 500 }); |
There was a problem hiding this comment.
The debounce test waits on real timers (setTimeout(300)) with a short waitFor timeout, which can be flaky/slow in CI. Consider using vi.useFakeTimers() + vi.advanceTimersByTime(300) (and restoring timers) to make the debounced $search assertion deterministic.
| mockDataSource.find.mockResolvedValue({ data: [], total: 0 }); | |
| render(<LookupField {...dynamicProps} />); | |
| await act(async () => { | |
| fireEvent.click(screen.getByRole('button', { name: /Select/i })); | |
| }); | |
| // Wait for initial load | |
| await waitFor(() => { | |
| expect(mockDataSource.find).toHaveBeenCalledTimes(1); | |
| }); | |
| // Type in search | |
| await act(async () => { | |
| fireEvent.change(screen.getByPlaceholderText('Search...'), { | |
| target: { value: 'acme' }, | |
| }); | |
| }); | |
| // Wait for debounced search | |
| await waitFor(() => { | |
| expect(mockDataSource.find).toHaveBeenCalledWith('customers', { | |
| $top: 50, | |
| $search: 'acme', | |
| }); | |
| }, { timeout: 500 }); | |
| vi.useFakeTimers(); | |
| try { | |
| mockDataSource.find.mockResolvedValue({ data: [], total: 0 }); | |
| render(<LookupField {...dynamicProps} />); | |
| await act(async () => { | |
| fireEvent.click(screen.getByRole('button', { name: /Select/i })); | |
| }); | |
| // Wait for initial load (promise-based) | |
| await waitFor(() => { | |
| expect(mockDataSource.find).toHaveBeenCalledTimes(1); | |
| }); | |
| // Type in search | |
| await act(async () => { | |
| fireEvent.change(screen.getByPlaceholderText('Search...'), { | |
| target: { value: 'acme' }, | |
| }); | |
| }); | |
| // Advance debounce timer deterministically | |
| await act(async () => { | |
| vi.advanceTimersByTime(300); | |
| }); | |
| expect(mockDataSource.find).toHaveBeenCalledWith('customers', { | |
| $top: 50, | |
| $search: 'acme', | |
| }); | |
| } finally { | |
| vi.useRealTimers(); | |
| } |
| ## Dynamic Data Source | ||
|
|
||
| When a `DataSource` is available (via `SchemaRendererContext`, explicit prop, or field config), the Lookup popup **automatically** fetches records from the referenced object: | ||
|
|
||
| ```plaintext | ||
| // Automatic — DataSource from SchemaRendererContext | ||
| // (works out-of-the-box in ObjectForm, DrawerForm, etc.) | ||
| { | ||
| type: 'lookup', | ||
| name: 'customer', | ||
| label: 'Customer', | ||
| reference_to: 'customers', | ||
| reference_field: 'name', // Display field (default: 'name') | ||
| description_field: 'industry', // Optional secondary field | ||
| } |
There was a problem hiding this comment.
This section states that Lookup can obtain a DataSource via SchemaRendererContext, but the current LookupField implementation relies on a dynamic require('@object-ui/react') fallback that won’t resolve under ESM-only @object-ui/react exports. If the intended mechanism is “form renderer passes dataSource down”, consider updating the docs to describe that flow (or update the widget to use a real context import).
| * Resolve SchemaRendererContext from @object-ui/react at runtime. | ||
| * Uses the same dynamic-require fallback that plugin-view uses to avoid | ||
| * a hard dependency on @object-ui/react (which would create a cycle). | ||
| */ | ||
| const FallbackContext = React.createContext<any>(null); | ||
| let SchemaRendererContext: React.Context<any> = FallbackContext; | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const mod = require('@object-ui/react'); | ||
| if (mod.SchemaRendererContext) { | ||
| SchemaRendererContext = mod.SchemaRendererContext; | ||
| } | ||
| } catch { | ||
| // @object-ui/react not available — dataSource must be passed via props | ||
| } |
There was a problem hiding this comment.
SchemaRendererContext resolution via require('@object-ui/react') will not work reliably with the current @object-ui/react packaging (ESM-only exports, no require entry), so this context fallback will always be used and ctx?.dataSource will stay null. Either remove this dynamic-require path and rely on the dataSource prop (now provided by the form renderer), or add an explicit dependency/peerDependency on @object-ui/react and use a normal import so context-based DataSource actually works.
| * Resolve SchemaRendererContext from @object-ui/react at runtime. | |
| * Uses the same dynamic-require fallback that plugin-view uses to avoid | |
| * a hard dependency on @object-ui/react (which would create a cycle). | |
| */ | |
| const FallbackContext = React.createContext<any>(null); | |
| let SchemaRendererContext: React.Context<any> = FallbackContext; | |
| try { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| const mod = require('@object-ui/react'); | |
| if (mod.SchemaRendererContext) { | |
| SchemaRendererContext = mod.SchemaRendererContext; | |
| } | |
| } catch { | |
| // @object-ui/react not available — dataSource must be passed via props | |
| } | |
| * SchemaRendererContext placeholder. | |
| * | |
| * LookupField primarily relies on the `dataSource` prop provided by the | |
| * form renderer. This local context is kept for compatibility with any | |
| * existing `useContext(SchemaRendererContext)` usage but always defaults | |
| * to `null` within this package. | |
| */ | |
| const SchemaRendererContext = React.createContext<any>(null); |
Lookup field popup only rendered static
optionsfrom field schema — when used with a real backend (ObjectStackAdapter), the dialog was empty since no options were passed.Changes
SchemaRendererContextautomatically (same dynamic-require pattern as plugin-view), so lookup fields work out-of-the-box in forms (DrawerForm, ModalForm, etc.) without an explicitdataSourceprop. Falls back through: explicit prop →field.dataSource→ context → none.@object-ui/componentsnow reads DataSource fromSchemaRendererContextand passes it as a prop to all field widgets viarenderFieldComponent(). This replaces the fragilerequire('@object-ui/react')pattern in field widgets, which failed in ESM/Vite production builds because@object-ui/reactis ESM-only and not a dependency of@object-ui/fields.createFieldRenderer, the actual objectSchema field metadata (reference_to,display_field, etc.) is nested atfield.fieldrather than at the top level. LookupField now detects and unwraps this nested structure soreference_toresolves correctly anddataSource.find()executes as expected.referenceconvention support: The ObjectStack backend usesreference(notreference_to) as the property name for lookup field metadata. The codebase consistently usesfieldDef.reference_to || fieldDef.referenceto handle both conventions, and LookupField now follows this same pattern — both for unwrapping nested field metadata detection and for resolving the target object name.reference_to/referenceis set, dialog callsDataSource.find(reference_to, { $top: 50, $search })on open$searchparam to DataSourcereference_field(default:name),id_field(default:_id), and optionaldescription_fieldwith fallback chain (record[field] → record.label → record.name → String(id))description_fieldconfig shows subtitle/description text below each option label for richer UXonCreateNewcallback renders a "Create new" button when no results are found and at the bottom of resultsrole="listbox"on options container,role="option"+aria-selectedon each itemoptionswith local filtering when no DataSource providedbg-gray-100/bg-blue-50withbg-accent/text-accent-foregroundcontent/docs/fields/lookup.mdxwith all new features, schema interface, and dynamic DataSource examplesConsole Integration
The key integration fix ensures LookupField works in the console app's ModalForm/DrawerForm:
packages/components/src/renderers/form/form.tsximportsSchemaRendererContextfrom@object-ui/reactand readsdataSourcefrom contextrenderFieldComponent()now passesdataSourceas a prop to all registered field widgetscreateFieldRendererwrapper — when the form passes field config, the actual objectSchema metadata (reference_to/reference,reference_field, etc.) is nested atfield.field; LookupField detects this and reads from the inner objectreference_to(types convention) andreference(ObjectStack backend convention) property names, matching the fallback pattern used across the codebase (RecordDetailView, DetailSection, RelatedList, etc.)App.tsx (SchemaRendererProvider)→form.tsx (useContext → prop)→createFieldRenderer (wraps into field object)→LookupField (unwraps fieldMeta, reads dataSource)Usage
Tests
14 new tests covering: fetch on open, loading state, error+retry, empty state, debounced
$search, selection, static fallback, pagination hint, description field display, quick-create entry, arrow key navigation, nested field metadata resolution (createFieldRenderer wrapper scenario), ObjectStackreferenceconvention (nested wrapper), and ObjectStackreferenceconvention (flat field). All 23 complex widget tests pass.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.