From 4a04336b52f9b25c5bc85861102cdb00453b69d7 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:28:31 +0800 Subject: [PATCH] refactor(view): remove ObjectView's filter/sort bar, which was never connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 (its own filter panel, and a `filterableFields` whitelist). 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 — a real bug, had the state ever been written. CORRECTION TO #3081. That PR reported two ObjectView defects. Only the first was live: the object `table.defaultFilters` drop sat on the always-taken path. The second — rule objects spread into the `and` — required a non-empty user filter, so it could not run here. The shape is genuinely broken (a live server answers it with a 400, measured) and the adapter-level defence added alongside is still warranted for any producer that emits it, but that site was dead code, not a live defect. #3081's changeset is unreleased and now carries the correction. Keeping code that looks live and cannot run is what made that misreading possible — twice in one session — which is the argument for deleting it rather than leaving it for the next reader. No behaviour change: every removed branch was unreachable. Net -85 lines. The surviving paths are pinned by 4 new tests covering what the component hands the delegated renderer, alongside the 5 from #3081 covering what it queries with. Full suite 763 files / 8915 tests green; tsc clean; eslint 0 errors. Co-Authored-By: Claude Opus 5 --- ...filter-sources-merge-without-losing-any.md | 13 +- .changeset/objectview-dead-filter-bar.md | 35 ++++ packages/plugin-view/src/ObjectView.tsx | 163 +++++------------- .../ObjectView.filterSources.test.tsx | 53 ++++++ 4 files changed, 138 insertions(+), 126 deletions(-) create mode 100644 .changeset/objectview-dead-filter-bar.md 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(); + }); +});