Skip to content

Commit e4c2783

Browse files
os-zhuangclaude
andauthored
fix(view): the chart view gets a label and an icon in the view switcher (#2916) (#3040)
`ViewSwitcher`'s two exhaustive `Record<ViewType, …>` maps — `DEFAULT_VIEW_LABELS` and `DEFAULT_VIEW_ICONS` — were each missing the `chart` key. `chart` is a member of `ViewType` and `plugin-charts` is a registered view, so a chart tab rendered with no icon at all and with its raw type key as the label, while every sibling view showed a glyph and a capitalized name. The label and glyph are not a fresh product call: `plugin-list`'s own switcher, `app-shell`'s `ObjectView` and `CreateViewDialog`, and the `console.objectView.viewTypeChart` translation already agree on `BarChart3` and `'Chart'`, so adopting them makes the switcher stop disagreeing with the rest of the UI. An explicit per-view `label`/`icon` still overrides the default. Why the compiler never caught a hole in a map whose whole purpose is to be exhaustive: `@object-ui/plugin-view` had no `type-check` script, so CI never evaluated `Record<ViewType, …>`. The package now type-checks its sources (`tsc --noEmit`) and, chained off the same script, its tests (`tsconfig.test.json`) — and its `DEBT` entry in `scripts/check-type-check-coverage.mjs` is deleted, as that guard requires once the gap closes. Reverting either `chart` entry now fails the build, which is how this fix was verified rather than assumed. The package's third reported error is fixed too: `ViewTabBar` typed the drag listener bag as a hand-written `Record<string, (...args: any[]) => void>`, a structural fork of a type `dnd-kit` already exports. It now uses `DraggableSyntheticListeners`, so the fork cannot drift again. Compiling the tests for the first time surfaced two real findings. Three `renderListView` spies destructured a `listSchema` they never read (every assertion reads the schema off `mock.calls`); they now take `_props`, matching the argless spies already in that file. And the test program pulls in `ObjectView.tsx`, which gates a spec-compliance warning on `process.env.NODE_ENV` — so `tsconfig.test.json` names `node` alongside `@testing-library/jest-dom`, exactly as `plugin-report` (the other package whose source reads `process.env`) already does. New `ViewSwitcher.test.tsx` covers the user-visible half the type gate cannot: that every `ViewType` renders a label and an icon, that `chart` reads "Chart" rather than falling back to its type key, and that an explicit label/icon still wins. Both regression tests fail with either `chart` entry removed. Refs #2911, #2915 Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 03bd53b commit e4c2783

9 files changed

Lines changed: 175 additions & 10 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
"@object-ui/plugin-view": patch
3+
---
4+
5+
fix(view): the chart view gets a label and an icon in the view switcher — objectui#2916
6+
7+
`ViewSwitcher`'s two exhaustive `Record<ViewType, …>` maps — `DEFAULT_VIEW_LABELS`
8+
and `DEFAULT_VIEW_ICONS` — were each missing the `chart` key. `chart` is a member
9+
of `ViewType` and `plugin-charts` is a registered view, so a chart tab rendered
10+
with no icon and with its raw type key `chart` as the label, while every sibling
11+
view showed a glyph and a capitalized name.
12+
13+
Both maps now carry `chart`, using the same `BarChart3` glyph and `'Chart'` label
14+
that `plugin-list`'s switcher, `app-shell`'s `ObjectView`/`CreateViewDialog`, and
15+
the `console.objectView.viewTypeChart` translation already agree on — so the
16+
switcher no longer disagrees with the rest of the UI. An explicit per-view
17+
`label`/`icon` still overrides the default, unchanged.
18+
19+
Why the compiler did not catch it: `@object-ui/plugin-view` had no `type-check`
20+
script, so `Record<ViewType, …>` — the exhaustiveness guard that exists precisely
21+
to make a missing member a compile error — was never evaluated by CI. The package
22+
now type-checks both its sources and its tests, and its `DEBT` entry in
23+
`scripts/check-type-check-coverage.mjs` is deleted. Compiling the tests for the
24+
first time also surfaced three unused destructured spy parameters, and the
25+
package's one remaining reported error (a `dnd-kit` `SyntheticListenerMap`
26+
mismatch in `ViewTabBar`) is fixed by typing the listener bag as `dnd-kit`'s own
27+
exported `DraggableSyntheticListeners` rather than a hand-written structural fork.
28+
29+
Refs objectui#2911, objectui#2915.

packages/plugin-view/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
"scripts": {
1818
"build": "vite build",
1919
"test": "vitest run",
20+
"type-check": "tsc --noEmit && tsc -p tsconfig.test.json",
2021
"lint": "eslint ."
2122
},
2223
"dependencies": {

packages/plugin-view/src/ViewSwitcher.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { SchemaRenderer } from '@object-ui/react';
2424
import type { ViewSwitcherSchema, ViewType } from '@object-ui/types';
2525
import {
2626
Activity,
27+
BarChart3,
2728
Calendar,
2829
FileText,
2930
GanttChartSquare,
@@ -64,6 +65,7 @@ const DEFAULT_VIEW_LABELS: Record<ViewType, string> = {
6465
map: 'Map',
6566
gallery: 'Gallery',
6667
gantt: 'Gantt',
68+
chart: 'Chart',
6769
tree: 'Tree',
6870
};
6971

@@ -77,6 +79,7 @@ const DEFAULT_VIEW_ICONS: Record<ViewType, LucideIcon> = {
7779
map: Map,
7880
gallery: Images,
7981
gantt: GanttChartSquare,
82+
chart: BarChart3,
8083
tree: ListTree,
8184
};
8285

packages/plugin-view/src/ViewTabBar.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ import {
7878
useSensor,
7979
useSensors,
8080
type DragEndEvent,
81+
type DraggableSyntheticListeners,
8182
} from '@dnd-kit/core';
8283
import {
8384
SortableContext,
@@ -210,7 +211,7 @@ const SortableTab: React.FC<{
210211
children: (props: {
211212
setNodeRef: (node: HTMLElement | null) => void;
212213
style: React.CSSProperties;
213-
listeners: Record<string, (...args: any[]) => void> | undefined;
214+
listeners: DraggableSyntheticListeners;
214215
attributes: Record<string, unknown>;
215216
isDragging: boolean;
216217
}) => React.ReactNode;
@@ -413,7 +414,7 @@ export const ViewTabBar: React.FC<ViewTabBarProps> = ({
413414
const visibilityIcon = getVisibilityIcon(view);
414415

415416
const buildTabContent = (dragHandleProps?: {
416-
listeners: Record<string, (...args: any[]) => void> | undefined;
417+
listeners: DraggableSyntheticListeners;
417418
attributes: Record<string, unknown>;
418419
isDragging?: boolean;
419420
}) => (

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,7 @@ describe('ObjectView', () => {
626626
showSort: false,
627627
};
628628

629-
const renderListViewSpy = vi.fn(({ schema: listSchema }: any) => (
629+
const renderListViewSpy = vi.fn((_props: any) => (
630630
<div data-testid="custom-list">Custom ListView</div>
631631
));
632632

@@ -651,7 +651,7 @@ describe('ObjectView', () => {
651651
objectName: 'contacts',
652652
};
653653

654-
const renderListViewSpy = vi.fn(({ schema: listSchema }: any) => (
654+
const renderListViewSpy = vi.fn((_props: any) => (
655655
<div data-testid="custom-list">Custom ListView</div>
656656
));
657657

@@ -744,7 +744,7 @@ describe('ObjectView', () => {
744744
objectName: 'contacts',
745745
};
746746

747-
const renderListViewSpy = vi.fn(({ schema: listSchema }: any) => (
747+
const renderListViewSpy = vi.fn((_props: any) => (
748748
<div data-testid="custom-list">Custom ListView</div>
749749
));
750750

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
import { describe, it, expect, vi } from 'vitest';
10+
import { render, screen } from '@testing-library/react';
11+
import { ViewSwitcher } from '../ViewSwitcher';
12+
import type { ViewSwitcherSchema, ViewType } from '@object-ui/types';
13+
14+
// Mock @object-ui/react to avoid circular dependency issues; mirrors
15+
// ObjectView.test.tsx, including the data-invalidation bus that
16+
// @object-ui/components imports at module-eval time.
17+
vi.mock('@object-ui/react', async () => {
18+
const React = await import('react');
19+
return {
20+
SchemaRenderer: ({ schema }: any) => (
21+
<div data-testid="schema-renderer" data-schema-type={schema?.type}>
22+
{schema?.type}
23+
</div>
24+
),
25+
SchemaRendererContext: React.createContext(null),
26+
subscribeDataChanges: () => () => {},
27+
notifyDataChanged: () => {},
28+
};
29+
});
30+
31+
// Every member of `ViewType`. Declared as `ViewType[]` rather than inferred, so
32+
// adding a member to the union without extending this list is a compile error
33+
// here too — the same guard `DEFAULT_VIEW_LABELS` / `DEFAULT_VIEW_ICONS` get
34+
// from being `Record<ViewType, ...>` (#2916).
35+
const ALL_VIEW_TYPES: ViewType[] = [
36+
'list',
37+
'detail',
38+
'grid',
39+
'kanban',
40+
'calendar',
41+
'timeline',
42+
'map',
43+
'gallery',
44+
'gantt',
45+
'chart',
46+
'tree',
47+
];
48+
49+
function schemaFor(types: ViewType[]): ViewSwitcherSchema {
50+
return {
51+
type: 'view-switcher',
52+
variant: 'buttons',
53+
views: types.map(type => ({ type })),
54+
};
55+
}
56+
57+
describe('ViewSwitcher default view labels and icons', () => {
58+
it('renders a non-empty label and an icon for every ViewType', () => {
59+
render(<ViewSwitcher schema={schemaFor(ALL_VIEW_TYPES)} />);
60+
61+
for (const type of ALL_VIEW_TYPES) {
62+
// A missing entry falls back to the raw type key for the label and to no
63+
// icon at all, which is what a hole in either Record<ViewType, ...> map
64+
// looked like on screen.
65+
const button = screen
66+
.getAllByRole('button')
67+
.find(b => b.textContent?.trim().toLowerCase() === type || b.textContent?.trim() === type);
68+
expect(button, `no button rendered for view type "${type}"`).toBeDefined();
69+
expect(button!.querySelector('svg'), `view type "${type}" rendered without an icon`).not.toBeNull();
70+
}
71+
});
72+
73+
it('labels the chart view "Chart" rather than falling back to the type key', () => {
74+
render(<ViewSwitcher schema={schemaFor(['chart'])} />);
75+
76+
expect(screen.getByText('Chart')).toBeInTheDocument();
77+
expect(screen.queryByText('chart')).toBeNull();
78+
});
79+
80+
it('still lets an explicit label and icon override the defaults', () => {
81+
render(
82+
<ViewSwitcher
83+
schema={{
84+
type: 'view-switcher',
85+
variant: 'buttons',
86+
views: [{ type: 'chart', label: 'Revenue', icon: 'pie-chart' }],
87+
}}
88+
/>
89+
);
90+
91+
expect(screen.getByText('Revenue')).toBeInTheDocument();
92+
expect(screen.queryByText('Chart')).toBeNull();
93+
});
94+
});

packages/plugin-view/tsconfig.json

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,19 @@
22
"extends": "../../tsconfig.json",
33
"compilerOptions": {
44
"outDir": "dist",
5-
"jsx": "react-jsx"
5+
"jsx": "react-jsx",
6+
// Drop the root tsconfig's source-tree `paths` (keeping only the local `@/*`
7+
// alias) so `@object-ui/*` and `@objectstack/spec` resolve through the
8+
// workspace dependency's built `.d.ts` instead of pulling sibling package
9+
// sources in as program inputs — ~104 TS6059 rootDir errors otherwise (#2915).
10+
"baseUrl": ".",
11+
"paths": {
12+
"@/*": ["src/*"]
13+
}
614
},
7-
"include": ["src"]
15+
"include": ["src"],
16+
// Tests are type-checked by `tsconfig.test.json`, which the `type-check`
17+
// script chains: they need `@testing-library/jest-dom`'s global matcher
18+
// augmentation, and this project is the build, so they must not reach `dist`.
19+
"exclude": ["node_modules", "dist", "**/*.test.ts", "**/*.test.tsx"]
820
}
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
{
2+
// Type-checks this package's TESTS, which `tsconfig.json` excludes.
3+
// See `packages/types/tsconfig.test.json` for why that exclusion was a hole:
4+
// the build correctly keeps tests out of `dist`, but nothing else compiled
5+
// them, so a test could assert a contract the compiler never checked.
6+
"extends": "../../tsconfig.json",
7+
"compilerOptions": {
8+
"noEmit": true,
9+
// The package build emits `dist`; this project emits nothing.
10+
"composite": false,
11+
// Naming `types` at all switches off automatic `@types/*` inclusion, so
12+
// BOTH entries are load-bearing:
13+
// - jest-dom: the `toBeInTheDocument` matchers `ObjectView.test.tsx` uses
14+
// are a global augmentation, not an import, and it does not live under
15+
// `@types/` so it is never picked up automatically.
16+
// - node: the test program pulls in `ObjectView.tsx`, which gates its
17+
// spec-compliance warning on `process.env.NODE_ENV`.
18+
"types": ["@testing-library/jest-dom", "node"],
19+
// Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and
20+
// `@objectstack/spec` resolve through the workspace dependency's built
21+
// `.d.ts` instead of pulling sibling sources in as program inputs (TS6059).
22+
"paths": {}
23+
},
24+
"include": ["src/**/*.test.ts", "src/**/*.test.tsx"]
25+
}

scripts/check-type-check-coverage.mjs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ const root = resolve(dirname(fileURLToPath(import.meta.url)), "..");
3939
// `"type-check": "tsc --noEmit"`, then delete the entry. Counts are from the
4040
// #2911 sweep (bare `tsc --noEmit` with the `paths` override its type-checked
4141
// peers already carry, so the TS6059 rootDir noise is excluded).
42-
const DEBT = {
43-
"@object-ui/plugin-view": { errors: 3, issue: 2916, note: "Record<ViewType,...> missing the 'chart' key" },
44-
};
42+
// Empty, and worth keeping that way: every workspace package now either carries
43+
// a `type-check` script or is declared in one of the exemption lists below.
44+
const DEBT = {};
4545

4646
// Packages that are not compiled at all: documentation snippets with no build
4747
// script and no tsconfig, whose sources are read rather than run. Re-validated

0 commit comments

Comments
 (0)