Skip to content

Fix registration test timeout in plugin-detail #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hotlong merged 6 commits into from
Feb 5, 2026
Merged

Fix registration test timeout in plugin-detail #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ff331c
Initial plan
Copilot Feb 4, 2026
969ac69
Fix registration test timeout by increasing hook timeout to 15s
Copilot Feb 4, 2026
45473bf
Initial plan
Copilot Feb 5, 2026
9237710
Fix plugin-list tests to match updated component implementation
Copilot Feb 5, 2026
e28dd11
Fix all test timeouts and add chartComponents export
Copilot Feb 5, 2026
34ada07
Merge pull request #385 from objectstack-ai/copilot/fix-all-and-test
hotlong Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/plugin-aggrid/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('Plugin AgGrid', () => {
// Import all renderers to register them
beforeAll(async () => {
await import('./index');
});
}, 15000); // Increase timeout to 15 seconds for async import

describe('aggrid component', () => {
it('should be registered in ComponentRegistry', () => {
Expand Down
6 changes: 6 additions & 0 deletions packages/plugin-charts/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ export type { BarChartSchema } from './types';
export { ChartBarRenderer, ChartRenderer };
export { ObjectChart } from './ObjectChart';

// Standard Export Protocol - for manual integration
export const chartComponents = {
'bar-chart': ChartBarRenderer,
'chart': ChartRenderer,
};
Comment on lines +18 to +22
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Standard Export Protocol" pattern for manual integration is introduced here but lacks documentation. According to Rule #2 (Documentation Driven Development), features must be documented in the package README.md or content/docs/guide/*.md. This pattern appears across multiple plugins (aggrid, dashboard, editor, kanban, markdown) but is not explained anywhere, making it unclear when and how developers should use these exported component objects versus the ComponentRegistry.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +18 to +22
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR description states it only fixes timeout issues in plugin-detail registration tests, but the actual changes include: (1) timeout fixes in 4 different plugins (detail, markdown, kanban, aggrid), (2) test updates in plugin-list to change from 'list' to 'kanban' view type and from 'search' to 'find' placeholder, and (3) a new Standard Export Protocol in plugin-charts. These additional changes are not mentioned in the PR description. The PR should either have a more comprehensive description or be split into separate PRs for different concerns.

Copilot uses AI. Check for mistakes.

// Register the component with the ComponentRegistry
ComponentRegistry.register(
'bar-chart',
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-detail/src/registration.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ComponentRegistry } from '@object-ui/core';
describe('Plugin Detail Registration', () => {
beforeAll(async () => {
await import('./index');
});
}, 15000); // Increase timeout to 15 seconds for async import
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout value of 15 seconds is inconsistent with other registration tests in the codebase. The plugin-charts registration.test.tsx uses a 30-second timeout (line 9), which is double this value. Consider standardizing timeout values across all registration tests to ensure consistent behavior in CI environments. If plugin-charts needs 30 seconds, it's possible these other plugins may also need the same or similar timeout values to prevent future timeout issues.

Suggested change
}, 15000); // Increase timeout to 15 seconds for async import
}, 30000); // Increase timeout to 30 seconds for async import to align with other registration tests

Copilot uses AI. Check for mistakes.

it('registers detail-view component', () => {
// We must use getConfig to retrieve the metadata (label, category, etc.)
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-kanban/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('Plugin Kanban', () => {
// Import all renderers to register them
beforeAll(async () => {
await import('./index');
});
}, 15000); // Increase timeout to 15 seconds for async import

describe('kanban component', () => {
it('should be registered in ComponentRegistry', () => {
Expand Down
26 changes: 14 additions & 12 deletions packages/plugin-list/src/__tests__/ListView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('ListView', () => {
};

renderWithProvider(<ListView schema={schema} />);
const searchInput = screen.getByPlaceholderText(/search/i);
const searchInput = screen.getByPlaceholderText(/find/i);
expect(searchInput).toBeInTheDocument();
});

Expand All @@ -89,7 +89,7 @@ describe('ListView', () => {
};

renderWithProvider(<ListView schema={schema} onSearchChange={onSearchChange} />);
const searchInput = screen.getByPlaceholderText(/search/i);
const searchInput = screen.getByPlaceholderText(/find/i);

fireEvent.change(searchInput, { target: { value: 'test' } });
expect(onSearchChange).toHaveBeenCalledWith('test');
Expand All @@ -101,22 +101,24 @@ describe('ListView', () => {
objectName: 'contacts',
viewType: 'grid',
fields: ['name', 'email'],
options: {
kanban: {
groupField: 'status',
},
},
};

renderWithProvider(<ListView schema={schema} />);

// Find list view button and click it
// Using getAllByRole because there might be multiple buttons
const buttons = screen.getAllByRole('radio'); // ToggleGroup usually uses radio role if type="single"
// However, if it's implemented as buttons using ToggleGroup which is roving tabindex...
// Let's try finding by aria-label which ViewSwitcher sets
const listButton = screen.getByLabelText('List');

fireEvent.click(listButton);
// Find kanban view button and click it
// ViewSwitcher uses buttons with aria-label
const kanbanButton = screen.getByLabelText('Kanban');

fireEvent.click(kanbanButton);

// localStorage should be set with new view
const storageKey = 'listview-contacts-view';
expect(localStorageMock.getItem(storageKey)).toBe('list');
expect(localStorageMock.getItem(storageKey)).toBe('kanban');
});

it('should call onViewChange when view is changed', () => {
Expand Down Expand Up @@ -194,7 +196,7 @@ describe('ListView', () => {
};

renderWithProvider(<ListView schema={schema} />);
const searchInput = screen.getByPlaceholderText(/search/i) as HTMLInputElement;
const searchInput = screen.getByPlaceholderText(/find/i) as HTMLInputElement;

// Type in search
fireEvent.change(searchInput, { target: { value: 'test' } });
Expand Down
48 changes: 31 additions & 17 deletions packages/plugin-list/src/__tests__/ListViewPersistence.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,22 @@ describe('ListView Persistence', () => {
id: 'my-custom-view',
objectName: 'tasks',
viewType: 'grid', // Start with grid
options: {
kanban: {
groupField: 'status',
},
},
};

renderWithProvider(<ListView schema={schema} />);

// Simulate changing to list view
const listButton = screen.getByLabelText('List');
fireEvent.click(listButton);
// Simulate changing to kanban view
const kanbanButton = screen.getByLabelText('Kanban');
fireEvent.click(kanbanButton);

// Check scoped storage key
const expectedKey = 'listview-tasks-my-custom-view-view';
expect(localStorageMock.getItem(expectedKey)).toBe('list');
expect(localStorageMock.getItem(expectedKey)).toBe('kanban');

// Check fallback key is NOT set
expect(localStorageMock.getItem('listview-tasks-view')).toBeNull();
Expand All @@ -69,44 +74,53 @@ describe('ListView Persistence', () => {
// Setup: View A (Global/Default) prefers Grid
localStorageMock.setItem('listview-tasks-view', 'grid');

// Setup: View B (Special) prefers List
// We define View B with valid options for Kanban to force it to render the button just in case,
// but we will test switching between Grid/List.
// Setup: View B (Special) prefers Kanban
// We define View B with valid options for Kanban to force it to render the button

const viewB_Schema: ListViewSchema = {
type: 'list-view',
id: 'special-view',
objectName: 'tasks',
viewType: 'list' // Default to List
viewType: 'kanban', // Default to Kanban
options: {
kanban: {
groupField: 'status',
},
},
};

renderWithProvider(<ListView schema={viewB_Schema} />);

// Should use the schema default 'list' (since no storage exists for THIS view id)
// Should use the schema default 'kanban' (since no storage exists for THIS view id)
// It should NOT use 'grid' from the global/default view.

const listButton = screen.getByLabelText('List');
expect(listButton.getAttribute('data-state')).toBe('on');
const kanbanButton = screen.getByLabelText('Kanban');
expect(kanbanButton.getAttribute('data-state')).toBe('on');

const gridButton = screen.getByLabelText('Grid');
expect(gridButton.getAttribute('data-state')).toBe('off');
});

it('should switch correctly when storage has a value for THIS view', () => {
// Setup: This specific view was previously set to 'list'
localStorageMock.setItem('listview-tasks-my-board-view', 'list');
// Setup: This specific view was previously set to 'kanban'
localStorageMock.setItem('listview-tasks-my-board-view', 'kanban');

const schema: ListViewSchema = {
type: 'list-view',
id: 'my-board',
objectName: 'tasks',
viewType: 'grid' // Default in schema is grid
viewType: 'grid', // Default in schema is grid
options: {
kanban: {
groupField: 'status',
},
},
};

renderWithProvider(<ListView schema={schema} />);

// Should respect storage ('list') over schema ('grid')
const listButton = screen.getByLabelText('List');
expect(listButton.getAttribute('data-state')).toBe('on');
// Should respect storage ('kanban') over schema ('grid')
const kanbanButton = screen.getByLabelText('Kanban');
expect(kanbanButton.getAttribute('data-state')).toBe('on');
});
});
2 changes: 1 addition & 1 deletion packages/plugin-markdown/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// Import all renderers to register them
beforeAll(async () => {
await import('./index');
});
}, 15000); // Increase timeout to 15 seconds for async import

describe('markdown component', () => {
it('should be registered in ComponentRegistry', () => {
Expand All @@ -32,7 +32,7 @@

it('should have expected inputs', () => {
const config = ComponentRegistry.getConfig('markdown');
const inputNames = config?.inputs?.map((input: any) => input.name) || [];

Check warning on line 35 in packages/plugin-markdown/src/index.test.ts

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected any. Specify a different type

expect(inputNames).toContain('content');
expect(inputNames).toContain('className');
Expand All @@ -40,7 +40,7 @@

it('should have content as required input', () => {
const config = ComponentRegistry.getConfig('markdown');
const contentInput = config?.inputs?.find((input: any) => input.name === 'content');

Check warning on line 43 in packages/plugin-markdown/src/index.test.ts

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected any. Specify a different type

expect(contentInput).toBeDefined();
expect(contentInput?.required).toBe(true);
Expand Down
Loading