diff --git a/.changeset/filter-sources-merge-without-losing-any.md b/.changeset/filter-sources-merge-without-losing-any.md index 336d54f05..e2b5b2f64 100644 --- a/.changeset/filter-sources-merge-without-losing-any.md +++ b/.changeset/filter-sources-merge-without-losing-any.md @@ -31,8 +31,17 @@ parseFilterAST(same) ``` That second line is a predicate over three columns named `field`, `operator` -and `value` — which don't exist. Reachable whenever a view with a filter meets a -user filter value. +and `value` — which don't exist. + +> **Correction.** The first version of this note said the spread was "reachable +> whenever a view with a filter meets a user filter value". That was wrong for +> `ObjectView`: the branch required a non-empty user filter, and nothing ever +> wrote the state it was built from, so it could never run. The shape is +> genuinely broken — a live server answers it with a 400 — and the adapter-level +> defence added alongside is still warranted for any producer that emits it, but +> **this particular site was dead code, not a live defect.** Defect 1 above was +> live: it sat on the always-taken path. The dead machinery behind the wrong +> claim is removed in a follow-up. New in `@object-ui/core`: `toFilterNode` normalizes one source (rule array / AST / MongoDB object) and `mergeFilterNodes` combines sources as siblings under one diff --git a/.changeset/objectview-dead-filter-bar.md b/.changeset/objectview-dead-filter-bar.md new file mode 100644 index 000000000..e16eced13 --- /dev/null +++ b/.changeset/objectview-dead-filter-bar.md @@ -0,0 +1,35 @@ +--- +"@object-ui/plugin-view": patch +--- + +refactor(view): remove ObjectView's filter/sort bar, which was never connected + +`ObjectView` carried its own filter and sort bar: `filterValues` / `sortConfig` +state, a `filter-ui` schema and a `sort-ui` schema, ~80 lines of field +introspection to build them. None of it was wired. No setter was ever called and +neither schema was ever rendered — both states sat at their initial empty value +for the component's entire life. + +Removed rather than wired, because the real filter and sort UI belongs to the +renderer this component delegates to. `showFilters`, `showSort` and +`filterableFields` are forwarded downstream and `ListView` implements them for +real. Connecting the local copy would have produced a *second* filter bar +competing with that one. + +The dead state was not inert, though — it left a branch in every merge path that +could never run, and those branches read as live code: + +- The fetch path merged `baseFilter` with a `userFilter` that was always `[]`. +- `mergedFilters` (what the `renderListView` slot receives, used by the Studio + design surface) opened with a branch that **replaced** the view's filter with + the user's instead of combining them — which would have been a real bug had + the state ever been written. + +Two "defects" reported against these branches during #3081 were unreachable for +exactly this reason; that changeset carries the correction. Keeping code that +looks live and cannot run is what made the misreading possible twice, which is +the argument for deleting it rather than leaving it for the next reader. + +No behaviour change: every removed branch was unreachable, and the surviving +paths are pinned by new tests covering both what the component queries with and +what it hands the delegated renderer. diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index fb62aee78..4b213355d 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -29,8 +29,6 @@ import type { ObjectFormSchema, DataSource, ViewSwitcherSchema, - FilterUISchema, - SortUISchema, ViewType, NamedListView, ViewNavigationConfig, @@ -282,9 +280,20 @@ export const ObjectView: React.FC = ({ const [data, setData] = useState([]); const [loading, setLoading] = useState(false); - // Filter & Sort state - const [filterValues, setFilterValues] = useState>({}); - const [sortConfig, setSortConfig] = useState>([]); + // NOTE: this component used to carry its own filter/sort BAR — `filterValues` + // and `sortConfig` state, a `filter-ui` schema and a `sort-ui` schema. None of + // it was ever connected: no setter was called and neither schema was rendered, + // so both states stayed at their initial empty value for the component's whole + // life. It was removed rather than wired, because the real filter and sort UI + // belongs to the renderer this component delegates to — `showFilters`, + // `showSort` and `filterableFields` are forwarded downstream (see `baseProps` + // and the list-view schema below) and `ListView` implements them. Wiring the + // local copy would have produced a SECOND filter bar competing with that one. + // + // The dead state was not harmless: every merge path here had a branch keyed on + // it that could never run, and those branches read as live code. Two separate + // "bugs" reported against them during objectui#3081 were unreachable for this + // reason — see the correction in that PR. // --- Named listViews --- const namedListViews = schema.listViews; @@ -358,32 +367,19 @@ export const ObjectView: React.FC = ({ setLoading(true); try { - // Build filter. Each source becomes its OWN child of the `and`, never - // spread into it: `['and', ...baseFilter]` is only correct when the - // source happens to be an array of AST nodes, and `activeView.filter` is - // a spec `ViewFilterRule[]` — spreading put bare rule objects where the - // AST expects nodes, which `isFilterAST` rejects (a 400 since - // objectstack#4121) and `parseFilterAST` misreads as a Mongo condition - // on columns literally named `field` / `operator` / `value`. - // - // `mergeFilterNodes` also rescues an OBJECT source: `table.defaultFilters` - // is declared `Record`, and the old `baseFilter.length > 0` - // test read false for it — so a view's default filter was dropped and - // every record came back. The grid path (ObjectGrid) always assigned it - // directly and was unaffected, so the same view filtered correctly as a - // grid and returned everything as a calendar/kanban/gallery. - const userFilter = Object.entries(filterValues) - .filter(([, v]) => v !== undefined && v !== '' && v !== null) - .map(([field, value]) => [field, '=', value]); + // `mergeFilterNodes` rescues an OBJECT source: `table.defaultFilters` is + // declared `Record`, and the `baseFilter.length > 0` test + // this replaced read false for it — so a view's default filter was + // dropped and every record came back. The grid path (ObjectGrid) always + // assigned it directly and was unaffected, so the same view filtered + // correctly as a grid and returned everything as a calendar/kanban/ + // gallery. It also keeps each source as its own child of the `and` + // rather than spreading it, which is what a `ViewFilterRule[]` needs. const finalFilter = mergeFilterNodes( currentNamedViewConfig?.filter || activeView?.filter || schema.table?.defaultFilters, - ...userFilter, ); - // Build sort - const sort = sortConfig.length > 0 - ? sortConfig.map(s => ({ field: s.field, order: s.direction })) - : (currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort || undefined); + const sort = currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort || undefined; // Auto-inject $expand for lookup/master_detail fields. // Use a ref instead of the state variable to avoid re-running this effect @@ -424,7 +420,7 @@ export const ObjectView: React.FC = ({ return () => { isMounted = false; }; // objectSchema intentionally omitted from deps — read via ref to prevent double-fetch // eslint-disable-next-line react-hooks/exhaustive-deps - }, [schema.objectName, dataSource, currentViewType, filterValues, sortConfig, refreshKey, currentNamedViewConfig, activeView, renderListView]); + }, [schema.objectName, dataSource, currentViewType, refreshKey, currentNamedViewConfig, activeView, renderListView]); // Determine layout mode. #2578: default the record surface from how heavy the // object is — a field-heavy object opens create/edit/detail as a full page, a @@ -608,84 +604,6 @@ export const ObjectView: React.FC = ({ setActiveNamedView(viewKey); }, []); - // --- FilterUI schema (auto-generated from objectSchema or filterableFields) --- - const filterSchema: FilterUISchema | null = useMemo(() => { - if (schema.showFilters === false) return null; - - // If filterableFields specified, use only those - const filterableFieldNames = schema.filterableFields; - const fields = (objectSchema as any)?.fields || {}; - - const fieldEntries = filterableFieldNames - ? filterableFieldNames.map(name => [name, fields[name] || { label: name }] as [string, any]) - : Object.entries(fields).filter(([, f]: [string, any]) => !f.hidden).slice(0, 8); - - const filterableFieldDefs = fieldEntries.map(([key, f]: [string, any]) => { - const fieldType = f.type || 'text'; - let filterType: 'text' | 'number' | 'select' | 'multi-select' | 'date' | 'boolean' = 'text'; - let options: Array<{ label: string; value: any }> | undefined; - - if (fieldType === 'number' || fieldType === 'currency' || fieldType === 'percent') { - filterType = 'number'; - } else if (fieldType === 'boolean' || fieldType === 'toggle') { - filterType = 'boolean'; - } else if (fieldType === 'date' || fieldType === 'datetime') { - filterType = 'date'; - } else if (fieldType === 'select' || fieldType === 'status' || f.options) { - filterType = 'select'; - options = (f.options || []).map((o: any) => - typeof o === 'string' ? { label: o, value: o } : { label: o.label, value: o.value }, - ); - } else if (fieldType === 'lookup' || fieldType === 'master_detail' || fieldType === 'user' || fieldType === 'owner') { - if (f.options && f.options.length > 0) { - filterType = 'multi-select'; - options = (f.options || []).map((o: any) => - typeof o === 'string' ? { label: o, value: o } : { label: o.label, value: o.value }, - ); - } - } - return { - field: key, - label: f.label || key, - type: filterType, - placeholder: `Filter ${f.label || key}...`, - ...(options ? { options } : {}), - }; - }); - - if (filterableFieldDefs.length === 0) return null; - - return { - type: 'filter-ui' as const, - layout: 'popover' as const, - showClear: true, - showApply: true, - filters: filterableFieldDefs, - values: filterValues, - }; - }, [schema.showFilters, schema.filterableFields, objectSchema, filterValues]); - - // --- SortUI schema --- - const showSort = (schema as ObjectViewSchema).showSort; - const sortSchema: SortUISchema | null = useMemo(() => { - if (showSort === false) return null; - - const fields = (objectSchema as any)?.fields || {}; - const sortableFields = Object.entries(fields) - .filter(([, f]: [string, any]) => !f.hidden) - .slice(0, 10) - .map(([key, f]: [string, any]) => ({ field: key, label: f.label || key })); - - if (sortableFields.length === 0) return null; - - return { - type: 'sort-ui' as const, - variant: 'dropdown' as const, - multiple: false, - fields: sortableFields, - sort: sortConfig, - }; - }, [objectSchema, sortConfig, showSort]); // --- Generate view component schema for non-grid views --- const generateViewSchema = useCallback((viewType: string): any => { @@ -977,24 +895,21 @@ export const ObjectView: React.FC = ({ ); - // Compute merged filters for the list - const mergedFilters = useMemo(() => { - const hasUserFilters = Object.keys(filterValues).some( - k => filterValues[k] !== undefined && filterValues[k] !== '' && filterValues[k] !== null, - ); - if (hasUserFilters) { - return Object.entries(filterValues) - .filter(([, v]) => v !== undefined && v !== '' && v !== null) - .map(([field, value]) => ({ field, operator: 'equals' as const, value })); - } - return currentNamedViewConfig?.filter || activeView?.filter || schema.table?.defaultFilters; - }, [filterValues, currentNamedViewConfig, activeView, schema.table?.defaultFilters]); - - const mergedSort = useMemo(() => { - return sortConfig.length > 0 - ? sortConfig - : currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort; - }, [sortConfig, currentNamedViewConfig, activeView, schema.table?.defaultSort]); + // The filter and sort this component hands to the renderer it delegates to. + // + // Both used to open with a branch keyed on the local filter/sort state — and + // the filter one REPLACED the view's own filter with the user's rather than + // combining them, which would have been a bug had the state ever been + // written. It never was (see the note by the state declarations), so both + // branches were dead. They are gone rather than corrected: the delegated + // renderer owns the filter/sort UI and does its own combining. + const mergedFilters = currentNamedViewConfig?.filter + || activeView?.filter + || schema.table?.defaultFilters; + + const mergedSort = currentNamedViewConfig?.sort + || activeView?.sort + || schema.table?.defaultSort; // --- Content renderer --- const renderContent = () => { diff --git a/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx index a76871c60..0b1853de6 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx @@ -102,3 +102,56 @@ describe('ObjectView carries every filter source into the query', () => { expect(await queriedFilter(renderCalendar({ table: { defaultFilters: [] } as any }))).toBeUndefined(); }); }); + +/** + * `mergedFilters` / `mergedSort` — what ObjectView hands the renderer it + * delegates to (the `renderListView` slot, used by the Studio design surface). + * + * Both used to open with a branch keyed on ObjectView's own filter/sort state, + * and the filter one REPLACED the view's filter with the user's rather than + * combining them. Nothing ever wrote that state, so neither branch could run — + * they were deleted rather than corrected, because the delegated renderer owns + * the filter UI and does its own combining. These pin what survives. + */ +describe('ObjectView hands the view filter to the delegated renderer', () => { + beforeEach(() => vi.clearAllMocks()); + + function renderDelegated(schema: Partial) { + const seen: any[] = []; + const ds: any = { + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), create: vi.fn(), update: vi.fn(), delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ name: 'task', fields: {} }), + }; + render( + { seen.push(s); return
; }} + />, + ); + return seen; + } + + it('forwards an object table.defaultFilters unchanged', () => { + const seen = renderDelegated({ table: { defaultFilters: { status: 'active' } } as any }); + expect(seen[0]?.filter).toEqual({ status: 'active' }); + }); + + it('forwards a ViewFilterRule[] unchanged', () => { + const rules = [{ field: 'stage', operator: 'eq', value: 'won' }]; + const seen = renderDelegated({ table: { defaultFilters: rules } as any }); + expect(seen[0]?.filter).toEqual(rules); + }); + + it('forwards the sort alongside it', () => { + const seen = renderDelegated({ table: { defaultSort: [{ field: 'name', direction: 'asc' }] } as any }); + expect(seen[0]?.sort).toEqual([{ field: 'name', direction: 'asc' }]); + }); + + it('forwards nothing when the view declares neither', () => { + const seen = renderDelegated({}); + expect(seen[0]?.filter).toBeUndefined(); + expect(seen[0]?.sort).toBeUndefined(); + }); +});