Skip to content

Commit bebaebd

Browse files
os-zhuangclaude
os-zhuang
and
claude
authored
refactor(view): remove ObjectView's filter/sort bar, which was never connected (#3087)
`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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 7eb06a0 commit bebaebd

4 files changed

Lines changed: 138 additions & 126 deletions

File tree

‎.changeset/filter-sources-merge-without-losing-any.md‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,17 @@ parseFilterAST(same)
3131
```
3232

3333
That second line is a predicate over three columns named `field`, `operator`
34-
and `value` — which don't exist. Reachable whenever a view with a filter meets a
35-
user filter value.
34+
and `value` — which don't exist.
35+
36+
> **Correction.** The first version of this note said the spread was "reachable
37+
> whenever a view with a filter meets a user filter value". That was wrong for
38+
> `ObjectView`: the branch required a non-empty user filter, and nothing ever
39+
> wrote the state it was built from, so it could never run. The shape is
40+
> genuinely broken — a live server answers it with a 400 — and the adapter-level
41+
> defence added alongside is still warranted for any producer that emits it, but
42+
> **this particular site was dead code, not a live defect.** Defect 1 above was
43+
> live: it sat on the always-taken path. The dead machinery behind the wrong
44+
> claim is removed in a follow-up.
3645
3746
New in `@object-ui/core`: `toFilterNode` normalizes one source (rule array / AST
3847
/ MongoDB object) and `mergeFilterNodes` combines sources as siblings under one

‎.changeset/objectview-dead-filter-bar.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
"@object-ui/plugin-view": patch
3+
---
4+
5+
refactor(view): remove ObjectView's filter/sort bar, which was never connected
6+
7+
`ObjectView` carried its own filter and sort bar: `filterValues` / `sortConfig`
8+
state, a `filter-ui` schema and a `sort-ui` schema, ~80 lines of field
9+
introspection to build them. None of it was wired. No setter was ever called and
10+
neither schema was ever rendered — both states sat at their initial empty value
11+
for the component's entire life.
12+
13+
Removed rather than wired, because the real filter and sort UI belongs to the
14+
renderer this component delegates to. `showFilters`, `showSort` and
15+
`filterableFields` are forwarded downstream and `ListView` implements them for
16+
real. Connecting the local copy would have produced a *second* filter bar
17+
competing with that one.
18+
19+
The dead state was not inert, though — it left a branch in every merge path that
20+
could never run, and those branches read as live code:
21+
22+
- The fetch path merged `baseFilter` with a `userFilter` that was always `[]`.
23+
- `mergedFilters` (what the `renderListView` slot receives, used by the Studio
24+
design surface) opened with a branch that **replaced** the view's filter with
25+
the user's instead of combining them — which would have been a real bug had
26+
the state ever been written.
27+
28+
Two "defects" reported against these branches during #3081 were unreachable for
29+
exactly this reason; that changeset carries the correction. Keeping code that
30+
looks live and cannot run is what made the misreading possible twice, which is
31+
the argument for deleting it rather than leaving it for the next reader.
32+
33+
No behaviour change: every removed branch was unreachable, and the surviving
34+
paths are pinned by new tests covering both what the component queries with and
35+
what it hands the delegated renderer.

‎packages/plugin-view/src/ObjectView.tsx‎

Lines changed: 39 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ import type {
2929
ObjectFormSchema,
3030
DataSource,
3131
ViewSwitcherSchema,
32-
FilterUISchema,
33-
SortUISchema,
3432
ViewType,
3533
NamedListView,
3634
ViewNavigationConfig,
@@ -282,9 +280,20 @@ export const ObjectView: React.FC<ObjectViewProps> = ({
282280
const [data, setData] = useState<any[]>([]);
283281
const [loading, setLoading] = useState(false);
284282

285-
// Filter & Sort state
286-
const [filterValues, setFilterValues] = useState<Record<string, any>>({});
287-
const [sortConfig, setSortConfig] = useState<Array<{ field: string; direction: 'asc' | 'desc' }>>([]);
283+
// NOTE: this component used to carry its own filter/sort BAR — `filterValues`
284+
// and `sortConfig` state, a `filter-ui` schema and a `sort-ui` schema. None of
285+
// it was ever connected: no setter was called and neither schema was rendered,
286+
// so both states stayed at their initial empty value for the component's whole
287+
// life. It was removed rather than wired, because the real filter and sort UI
288+
// belongs to the renderer this component delegates to — `showFilters`,
289+
// `showSort` and `filterableFields` are forwarded downstream (see `baseProps`
290+
// and the list-view schema below) and `ListView` implements them. Wiring the
291+
// local copy would have produced a SECOND filter bar competing with that one.
292+
//
293+
// The dead state was not harmless: every merge path here had a branch keyed on
294+
// it that could never run, and those branches read as live code. Two separate
295+
// "bugs" reported against them during objectui#3081 were unreachable for this
296+
// reason — see the correction in that PR.
288297

289298
// --- Named listViews ---
290299
const namedListViews = schema.listViews;
@@ -358,32 +367,19 @@ export const ObjectView: React.FC<ObjectViewProps> = ({
358367

359368
setLoading(true);
360369
try {
361-
// Build filter. Each source becomes its OWN child of the `and`, never
362-
// spread into it: `['and', ...baseFilter]` is only correct when the
363-
// source happens to be an array of AST nodes, and `activeView.filter` is
364-
// a spec `ViewFilterRule[]` — spreading put bare rule objects where the
365-
// AST expects nodes, which `isFilterAST` rejects (a 400 since
366-
// objectstack#4121) and `parseFilterAST` misreads as a Mongo condition
367-
// on columns literally named `field` / `operator` / `value`.
368-
//
369-
// `mergeFilterNodes` also rescues an OBJECT source: `table.defaultFilters`
370-
// is declared `Record<string, any>`, and the old `baseFilter.length > 0`
371-
// test read false for it — so a view's default filter was dropped and
372-
// every record came back. The grid path (ObjectGrid) always assigned it
373-
// directly and was unaffected, so the same view filtered correctly as a
374-
// grid and returned everything as a calendar/kanban/gallery.
375-
const userFilter = Object.entries(filterValues)
376-
.filter(([, v]) => v !== undefined && v !== '' && v !== null)
377-
.map(([field, value]) => [field, '=', value]);
370+
// `mergeFilterNodes` rescues an OBJECT source: `table.defaultFilters` is
371+
// declared `Record<string, any>`, and the `baseFilter.length > 0` test
372+
// this replaced read false for it — so a view's default filter was
373+
// dropped and every record came back. The grid path (ObjectGrid) always
374+
// assigned it directly and was unaffected, so the same view filtered
375+
// correctly as a grid and returned everything as a calendar/kanban/
376+
// gallery. It also keeps each source as its own child of the `and`
377+
// rather than spreading it, which is what a `ViewFilterRule[]` needs.
378378
const finalFilter = mergeFilterNodes(
379379
currentNamedViewConfig?.filter || activeView?.filter || schema.table?.defaultFilters,
380-
...userFilter,
381380
);
382381

383-
// Build sort
384-
const sort = sortConfig.length > 0
385-
? sortConfig.map(s => ({ field: s.field, order: s.direction }))
386-
: (currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort || undefined);
382+
const sort = currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort || undefined;
387383

388384
// Auto-inject $expand for lookup/master_detail fields.
389385
// Use a ref instead of the state variable to avoid re-running this effect
@@ -424,7 +420,7 @@ export const ObjectView: React.FC<ObjectViewProps> = ({
424420
return () => { isMounted = false; };
425421
// objectSchema intentionally omitted from deps — read via ref to prevent double-fetch
426422
// eslint-disable-next-line react-hooks/exhaustive-deps
427-
}, [schema.objectName, dataSource, currentViewType, filterValues, sortConfig, refreshKey, currentNamedViewConfig, activeView, renderListView]);
423+
}, [schema.objectName, dataSource, currentViewType, refreshKey, currentNamedViewConfig, activeView, renderListView]);
428424

429425
// Determine layout mode. #2578: default the record surface from how heavy the
430426
// object is — a field-heavy object opens create/edit/detail as a full page, a
@@ -608,84 +604,6 @@ export const ObjectView: React.FC<ObjectViewProps> = ({
608604
setActiveNamedView(viewKey);
609605
}, []);
610606

611-
// --- FilterUI schema (auto-generated from objectSchema or filterableFields) ---
612-
const filterSchema: FilterUISchema | null = useMemo(() => {
613-
if (schema.showFilters === false) return null;
614-
615-
// If filterableFields specified, use only those
616-
const filterableFieldNames = schema.filterableFields;
617-
const fields = (objectSchema as any)?.fields || {};
618-
619-
const fieldEntries = filterableFieldNames
620-
? filterableFieldNames.map(name => [name, fields[name] || { label: name }] as [string, any])
621-
: Object.entries(fields).filter(([, f]: [string, any]) => !f.hidden).slice(0, 8);
622-
623-
const filterableFieldDefs = fieldEntries.map(([key, f]: [string, any]) => {
624-
const fieldType = f.type || 'text';
625-
let filterType: 'text' | 'number' | 'select' | 'multi-select' | 'date' | 'boolean' = 'text';
626-
let options: Array<{ label: string; value: any }> | undefined;
627-
628-
if (fieldType === 'number' || fieldType === 'currency' || fieldType === 'percent') {
629-
filterType = 'number';
630-
} else if (fieldType === 'boolean' || fieldType === 'toggle') {
631-
filterType = 'boolean';
632-
} else if (fieldType === 'date' || fieldType === 'datetime') {
633-
filterType = 'date';
634-
} else if (fieldType === 'select' || fieldType === 'status' || f.options) {
635-
filterType = 'select';
636-
options = (f.options || []).map((o: any) =>
637-
typeof o === 'string' ? { label: o, value: o } : { label: o.label, value: o.value },
638-
);
639-
} else if (fieldType === 'lookup' || fieldType === 'master_detail' || fieldType === 'user' || fieldType === 'owner') {
640-
if (f.options && f.options.length > 0) {
641-
filterType = 'multi-select';
642-
options = (f.options || []).map((o: any) =>
643-
typeof o === 'string' ? { label: o, value: o } : { label: o.label, value: o.value },
644-
);
645-
}
646-
}
647-
return {
648-
field: key,
649-
label: f.label || key,
650-
type: filterType,
651-
placeholder: `Filter ${f.label || key}...`,
652-
...(options ? { options } : {}),
653-
};
654-
});
655-
656-
if (filterableFieldDefs.length === 0) return null;
657-
658-
return {
659-
type: 'filter-ui' as const,
660-
layout: 'popover' as const,
661-
showClear: true,
662-
showApply: true,
663-
filters: filterableFieldDefs,
664-
values: filterValues,
665-
};
666-
}, [schema.showFilters, schema.filterableFields, objectSchema, filterValues]);
667-
668-
// --- SortUI schema ---
669-
const showSort = (schema as ObjectViewSchema).showSort;
670-
const sortSchema: SortUISchema | null = useMemo(() => {
671-
if (showSort === false) return null;
672-
673-
const fields = (objectSchema as any)?.fields || {};
674-
const sortableFields = Object.entries(fields)
675-
.filter(([, f]: [string, any]) => !f.hidden)
676-
.slice(0, 10)
677-
.map(([key, f]: [string, any]) => ({ field: key, label: f.label || key }));
678-
679-
if (sortableFields.length === 0) return null;
680-
681-
return {
682-
type: 'sort-ui' as const,
683-
variant: 'dropdown' as const,
684-
multiple: false,
685-
fields: sortableFields,
686-
sort: sortConfig,
687-
};
688-
}, [objectSchema, sortConfig, showSort]);
689607

690608
// --- Generate view component schema for non-grid views ---
691609
const generateViewSchema = useCallback((viewType: string): any => {
@@ -977,24 +895,21 @@ export const ObjectView: React.FC<ObjectViewProps> = ({
977895
</Dialog>
978896
);
979897

980-
// Compute merged filters for the list
981-
const mergedFilters = useMemo(() => {
982-
const hasUserFilters = Object.keys(filterValues).some(
983-
k => filterValues[k] !== undefined && filterValues[k] !== '' && filterValues[k] !== null,
984-
);
985-
if (hasUserFilters) {
986-
return Object.entries(filterValues)
987-
.filter(([, v]) => v !== undefined && v !== '' && v !== null)
988-
.map(([field, value]) => ({ field, operator: 'equals' as const, value }));
989-
}
990-
return currentNamedViewConfig?.filter || activeView?.filter || schema.table?.defaultFilters;
991-
}, [filterValues, currentNamedViewConfig, activeView, schema.table?.defaultFilters]);
992-
993-
const mergedSort = useMemo(() => {
994-
return sortConfig.length > 0
995-
? sortConfig
996-
: currentNamedViewConfig?.sort || activeView?.sort || schema.table?.defaultSort;
997-
}, [sortConfig, currentNamedViewConfig, activeView, schema.table?.defaultSort]);
898+
// The filter and sort this component hands to the renderer it delegates to.
899+
//
900+
// Both used to open with a branch keyed on the local filter/sort state — and
901+
// the filter one REPLACED the view's own filter with the user's rather than
902+
// combining them, which would have been a bug had the state ever been
903+
// written. It never was (see the note by the state declarations), so both
904+
// branches were dead. They are gone rather than corrected: the delegated
905+
// renderer owns the filter/sort UI and does its own combining.
906+
const mergedFilters = currentNamedViewConfig?.filter
907+
|| activeView?.filter
908+
|| schema.table?.defaultFilters;
909+
910+
const mergedSort = currentNamedViewConfig?.sort
911+
|| activeView?.sort
912+
|| schema.table?.defaultSort;
998913

999914
// --- Content renderer ---
1000915
const renderContent = () => {

‎packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,3 +102,56 @@ describe('ObjectView carries every filter source into the query', () => {
102102
expect(await queriedFilter(renderCalendar({ table: { defaultFilters: [] } as any }))).toBeUndefined();
103103
});
104104
});
105+
106+
/**
107+
* `mergedFilters` / `mergedSort` — what ObjectView hands the renderer it
108+
* delegates to (the `renderListView` slot, used by the Studio design surface).
109+
*
110+
* Both used to open with a branch keyed on ObjectView's own filter/sort state,
111+
* and the filter one REPLACED the view's filter with the user's rather than
112+
* combining them. Nothing ever wrote that state, so neither branch could run —
113+
* they were deleted rather than corrected, because the delegated renderer owns
114+
* the filter UI and does its own combining. These pin what survives.
115+
*/
116+
describe('ObjectView hands the view filter to the delegated renderer', () => {
117+
beforeEach(() => vi.clearAllMocks());
118+
119+
function renderDelegated(schema: Partial<ObjectViewSchema>) {
120+
const seen: any[] = [];
121+
const ds: any = {
122+
find: vi.fn().mockResolvedValue({ data: [], total: 0 }),
123+
findOne: vi.fn(), create: vi.fn(), update: vi.fn(), delete: vi.fn(),
124+
getObjectSchema: vi.fn().mockResolvedValue({ name: 'task', fields: {} }),
125+
};
126+
render(
127+
<ObjectView
128+
schema={{ type: 'object-view', objectName: 'task', ...schema } as ObjectViewSchema}
129+
dataSource={ds}
130+
renderListView={({ schema: s }: any) => { seen.push(s); return <div data-testid="delegated" />; }}
131+
/>,
132+
);
133+
return seen;
134+
}
135+
136+
it('forwards an object table.defaultFilters unchanged', () => {
137+
const seen = renderDelegated({ table: { defaultFilters: { status: 'active' } } as any });
138+
expect(seen[0]?.filter).toEqual({ status: 'active' });
139+
});
140+
141+
it('forwards a ViewFilterRule[] unchanged', () => {
142+
const rules = [{ field: 'stage', operator: 'eq', value: 'won' }];
143+
const seen = renderDelegated({ table: { defaultFilters: rules } as any });
144+
expect(seen[0]?.filter).toEqual(rules);
145+
});
146+
147+
it('forwards the sort alongside it', () => {
148+
const seen = renderDelegated({ table: { defaultSort: [{ field: 'name', direction: 'asc' }] } as any });
149+
expect(seen[0]?.sort).toEqual([{ field: 'name', direction: 'asc' }]);
150+
});
151+
152+
it('forwards nothing when the view declares neither', () => {
153+
const seen = renderDelegated({});
154+
expect(seen[0]?.filter).toBeUndefined();
155+
expect(seen[0]?.sort).toBeUndefined();
156+
});
157+
});

0 commit comments

Comments
 (0)