app-catalog: Fix bit-rotted Storybooks#509
app-catalog: Fix bit-rotted Storybooks#509Ansita20 wants to merge 1 commit intoheadlamp-k8s:mainfrom
Conversation
- Update deprecated Storybook imports from @storybook/react/types-6-0 to @storybook/react - Remove deprecated storyshots test file and snapshot directories - Fix story templates to properly pass props and configure Redux store - Update config structure to use 'app-catalog' key for plugin settings - Add proper TypeScript types for story component props Fixes headlamp-k8s#425 Signed-off-by: ansita20 <ansitasingh20@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes bit-rotted Storybooks in the app-catalog plugin following the Vite migration. The Storybooks were not working or being tested due to the migration, requiring updates to deprecated imports, removal of storyshots, and fixes to story templates and Redux store configuration.
Changes:
- Updated Storybook imports from deprecated
@storybook/react/types-6-0to@storybook/react - Removed deprecated storyshots test infrastructure and snapshot files
- Fixed Redux store configuration to use nested 'app-catalog' key for plugin settings and added drawerMode state
- Added TypeScript interfaces for story component props
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app-catalog/src/storybook.test.tsx | Removed deprecated storyshots test file that relied on old testing infrastructure |
| app-catalog/src/components/releases/snapshots/ReleaseList.stories.storyshot | Removed obsolete snapshot file for ReleaseList stories |
| app-catalog/src/components/releases/ReleaseList.stories.tsx | Updated imports, added drawerMode to Redux store, improved TypeScript typing and props handling |
| app-catalog/src/components/charts/snapshots/SettingsLink.stories.storyshot | Removed obsolete snapshot file for SettingsLink stories |
| app-catalog/src/components/charts/snapshots/ChartsList.stories.storyshot | Removed obsolete snapshot file for ChartsList stories |
| app-catalog/src/components/charts/snapshots/AppCatalogTitle.stories.storyshot | Removed obsolete snapshot file for non-existent AppCatalogTitle stories |
| app-catalog/src/components/charts/ChartsList.stories.tsx | Updated imports, restructured config with 'app-catalog' key, added TypeScript types and initialState prop handling (but with critical bugs in mock implementations) |
Comments suppressed due to low confidence (1)
app-catalog/src/components/charts/ChartsList.stories.tsx:113
- The mock fetchCharts function has incorrect implementation. It should:
- Accept parameters (search: string, verified: boolean, category: { title: string; value: number }, page: number, limit?: number) to match the actual fetchChartsFromArtifact signature
- Return the correct structure { data: { packages: [], facets: [...] }, total: '0' } instead of directly returning { packages: [], facets: [...] }
The component calls fetchCharts with these parameters at line 205 in List.tsx and expects the { data, total } structure to destructure.
fetchCharts: () =>
Promise.resolve({
packages: [],
facets: [
{
title: 'Category',
options: [{ name: 'All', total: 0 }],
},
],
}),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const Template: StoryFn = ({ initialState, ...args }) => { | ||
| interface TemplateProps { | ||
| initialState?: any; | ||
| fetchCharts?: () => Promise<any>; |
There was a problem hiding this comment.
The type signature for fetchCharts is incorrect. According to the actual implementation in List.tsx (line 205) and the API function fetchChartsFromArtifact (line 36-41 in api/charts.tsx), fetchCharts should accept parameters: (search: string, verified: boolean, category: { title: string; value: number }, page: number, limit?: number) and return Promise with { data, total } structure. The current type signature accepts no parameters and doesn't specify the correct return type structure.
| fetchCharts: () => | ||
| Promise.resolve({ | ||
| packages: mockCharts, |
There was a problem hiding this comment.
The mock fetchCharts function has incorrect implementation. It should:
- Accept parameters (search: string, verified: boolean, category: { title: string; value: number }, page: number, limit?: number) to match the actual fetchChartsFromArtifact signature
- Return the correct structure { data: { packages: mockCharts, facets: [...] }, total: '2' } instead of directly returning { packages: mockCharts, facets: [...] }
The component calls fetchCharts with these parameters at line 205 in List.tsx and expects the { data, total } structure to destructure. Note: This same issue exists in WithShowOnlyVerifiedTrue and WithShowOnlyVerifiedFalse stories which are not in the changed region.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface TemplateProps { | ||
| initialState?: any; | ||
| fetchCharts?: () => Promise<any>; | ||
| } |
There was a problem hiding this comment.
The TemplateProps interface uses 'any' for both initialState and the return type of fetchCharts. For better type safety, consider defining explicit types. For example, initialState could be typed as a specific Redux state shape, and fetchCharts could return a specific type like Promise<{ packages: Array, facets: Array }>.
|
|
||
| const Template: StoryFn = args => ( | ||
| interface TemplateProps { | ||
| fetchReleases?: () => Promise<any>; |
There was a problem hiding this comment.
The type definition for fetchReleases should be more specific to ensure type safety. Consider defining a proper type that matches the expected return value structure, such as: fetchReleases?: () => Promise<{ releases: Array }>. This would make the interface more explicit about what the function should return.
| fetchReleases?: () => Promise<any>; | |
| fetchReleases?: () => Promise<{ releases: Array<any> }>; |
illume
left a comment
There was a problem hiding this comment.
Thanks for this!
Can you please check the commit message follows the git message guidelines mentioned in the headlamp contributing guide? See git log for other examples. It could be you have two commits. One for ReleaseList and the other for ChartsList
Please see the open review comments?
|
Note, there is a GitHub check failure to look at too. |
Fixes #425