TestAddon: Refactor UI & add config options #29662
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| } else if (state.failed && !errorMessage) { | ||
| description = ''; |
There was a problem hiding this comment.
logic: Setting description to empty string when failed without error message could lead to confusing UI state. Consider showing a default failure message instead.
| @@ -1,6 +1,23 @@ | |||
| import { useEffect, useState } from 'react'; | |||
|
|
|||
| import { getRelativeTimeString } from '../manager'; | |||
| export function getRelativeTimeString(date: Date): string { | |||
| const delta = Math.round((date.getTime() - Date.now()) / 1000); | |||
There was a problem hiding this comment.
logic: delta calculation will be negative for future dates, but Math.abs is only used later - could cause incorrect time displays
| const divisor = unitIndex ? cutoffs[unitIndex - 1] : 1; | ||
| const rtf = new Intl.RelativeTimeFormat('en', { numeric: 'auto' }); | ||
| return rtf.format(Math.floor(delta / divisor), units[unitIndex]); | ||
| } | ||
|
|
||
| export const RelativeTime = ({ timestamp, testCount }: { timestamp: Date; testCount: number }) => { | ||
| const [relativeTimeString, setRelativeTimeString] = useState(null); |
There was a problem hiding this comment.
style: initial state should be typed as useState<string | null>(null)
| import { TestProviderRender } from './TestProviderRender'; | ||
|
|
||
| type Story = StoryObj<typeof TestProviderRender>; | ||
| const managerContext: any = { |
There was a problem hiding this comment.
style: Avoid using 'any' type for managerContext. Consider creating a proper type definition based on ManagerContext requirements.
| import { Title } from './components/Title'; | ||
| import { ADDON_ID, PANEL_ID, TEST_PROVIDER_ID } from './constants'; | ||
| import type { TestResult } from './node/reporter'; | ||
| import { ADDON_ID, type Details, PANEL_ID, TEST_PROVIDER_ID } from './constants'; | ||
|
|
||
| const statusMap: Record<any['status'], API_StatusValue> = { |
There was a problem hiding this comment.
style: statusMap uses 'any' type for status key which could be more strictly typed
| @@ -4,7 +4,8 @@ type DateNow = number; | |||
|
|
|||
| export type TestProviderId = Addon_TestProviderType['id']; | |||
| export type TestProviderConfig = Addon_TestProviderType; | |||
| export type TestProviderState = Addon_TestProviderState; | |||
| export type TestProviderState<Details extends { [key: string]: any } = NonNullable<unknown>> = | |||
| Addon_TestProviderState<Details>; | |||
|
|
|||
| export type TestProviders = Record<TestProviderId, TestProviderConfig & TestProviderState>; | |||
There was a problem hiding this comment.
logic: TestProviders type needs to be updated to use the generic parameter, should be Record<TestProviderId, TestProviderConfig & TestProviderState>
ndelangen
changed the title
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 3f34f9d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
…config be passed to emit
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 7 | 7 | 0 |
| Self size | 1.89 MB | 1.89 MB | 0 B |
| Dependency size | 10.12 MB | 10.27 MB | 🚨 +142 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 223 | 223 | 0 |
| Self size | 79 KB | 79 KB | 0 B |
| Dependency size | 29.56 MB | 29.60 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 258 | 258 | 0 |
| Self size | 362 KB | 362 KB | 0 B |
| Dependency size | 34.49 MB | 34.54 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 250 | 250 | 0 |
| Self size | 22 KB | 22 KB | 0 B |
| Dependency size | 31.96 MB | 32.00 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 233 | 233 | 0 |
| Self size | 6 KB | 6 KB | 0 B |
| Dependency size | 30.10 MB | 30.15 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 587 | 587 | 0 |
| Self size | 464 KB | 464 KB | 0 B |
| Dependency size | 83.77 MB | 83.81 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 231 | 231 | 0 |
| Self size | 6 KB | 6 KB | 0 B |
| Dependency size | 29.68 MB | 29.72 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 309 | 309 | 0 |
| Self size | 6 KB | 6 KB | 0 B |
| Dependency size | 40.84 MB | 40.88 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 241 | 241 | 0 |
| Self size | 14 KB | 14 KB | 0 B |
| Dependency size | 31.08 MB | 31.12 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 296 | 296 | 0 |
| Self size | 6 KB | 6 KB | 0 B |
| Dependency size | 37.67 MB | 37.71 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 107 | 107 | 0 |
| Self size | 15 KB | 15 KB | 0 B |
| Dependency size | 42.52 MB | 42.55 MB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 483 | 483 | 0 |
| Self size | 6 KB | 6 KB | 0 B |
| Dependency size | 54.08 MB | 54.12 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 231 | 231 | 0 |
| Self size | 5 KB | 5 KB | 0 B |
| Dependency size | 29.73 MB | 29.77 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 390 | 390 | 0 |
| Self size | 484 KB | 484 KB | 0 B |
| Dependency size | 74.48 MB | 74.64 MB | 🚨 +154 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 270 | 270 | 0 |
| Self size | 612 KB | 612 KB | 0 B |
| Dependency size | 64.48 MB | 64.63 MB | 🚨 +154 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 105 | 105 | 0 |
| Self size | 1.11 MB | 1.11 MB | 0 B |
| Dependency size | 42.34 MB | 42.48 MB | 🚨 +145 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/source-loader
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 5 | 5 | 0 |
| Self size | 41 KB | 41 KB | 0 B |
| Dependency size | 10.07 MB | 10.21 MB | 🚨 +142 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-vue3-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 366 | 366 | 0 |
| Self size | 9 KB | 9 KB | 0 B |
| Dependency size | 45.00 MB | 45.04 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
…hing watchMode event
…bookjs/storybook into norbert/testmodule-options
…bookjs/storybook into norbert/testmodule-options
There was a problem hiding this comment.
18 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile
| play: async ({ canvasElement }) => { | ||
| const screen = within(canvasElement); | ||
|
|
||
| screen.getByLabelText('Edit').click(); | ||
| }, |
There was a problem hiding this comment.
style: Add error handling in case Edit button is not found
| {!isEditing ? ( | ||
| <Fragment> | ||
| {Object.entries(config).map(([key, value]) => ( | ||
| <div key={key}> | ||
| {key}: {value ? 'ON' : 'OFF'} | ||
| </div> | ||
| ))} | ||
| </Fragment> |
There was a problem hiding this comment.
style: Missing semantic HTML and accessibility attributes for clickable config toggles. Consider using buttons or checkboxes with proper ARIA roles.
| function useConfig(id: string, config: Config, api: API) { | ||
| const data = useRef<Config>(config); | ||
| data.current = config || { | ||
| a11y: false, | ||
| coverage: false, | ||
| }; |
There was a problem hiding this comment.
logic: data.current is reassigned on every render, defeating the purpose of useRef. Consider using useState instead.
| }} | ||
| onRerun={() => { | ||
| setIsModalOpen(false); | ||
| api.runTestProvider(TEST_PROVIDER_ID); |
There was a problem hiding this comment.
logic: Using hardcoded TEST_PROVIDER_ID here but state.id is used elsewhere. Should be consistent and use state.id.
| const resizeObserver = new ResizeObserver(() => { | ||
| if (spacer && wrapper) { | ||
| spacer.style.height = `${wrapper.clientHeight}px`; | ||
| if (spacerRef.current && wrapperRef.current) { | ||
| spacerRef.current.style.height = `${wrapperRef.current.scrollHeight}px`; | ||
| } |
There was a problem hiding this comment.
style: scrollHeight may cause layout issues if content includes margins - consider using offsetHeight or getBoundingClientRect() for more reliable measurements
| requestAnimationFrame(() => { | ||
| if (contentRef.current && !isCollapsed) { | ||
| const height = contentRef.current?.getBoundingClientRect().height || DEFAULT_HEIGHT; | ||
|
|
||
| setMaxHeight(height); | ||
| } | ||
| }); |
There was a problem hiding this comment.
logic: requestAnimationFrame callback may run after component unmount, causing memory leak if contentRef.current is accessed
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } |
There was a problem hiding this comment.
logic: timeoutRef cleanup missing in useEffect, could cause memory leak on unmount
| transition: isSlow ? 'max-height 250ms' : 'max-height 0ms', | ||
| display: hasTestProviders ? 'block' : 'none', | ||
| maxHeight: isCollapsed ? 0 : maxHeight, |
There was a problem hiding this comment.
style: redundant hasTestProviders check in display style since parent already checks condition
| @@ -66,7 +66,7 @@ test.describe("component testing", () => { | |||
| testStoryElement.click(); | |||
| } | |||
|
|
|||
| const testingModuleDescription = await page.locator('[data-module-id="storybook/test/test-provider"]').locator('#testing-module-description'); | |||
| const testingModuleDescription = await page.locator('#testing-module-description'); | |||
There was a problem hiding this comment.
logic: Selector change from '[data-module-id="storybook/test/test-provider"].locator(#testing-module-description)' to '#testing-module-description' could make the test more fragile if multiple elements with this ID exist.
There was a problem hiding this comment.
18 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| <LinkComponent | ||
| isButton | ||
| onClick={() => { | ||
| setIsModalOpen(true); | ||
| }} | ||
| > | ||
| {state.error?.name || 'View full error'} | ||
| </LinkComponent> | ||
| </> |
There was a problem hiding this comment.
style: Fragment wrapper is unnecessary since LinkComponent is the only child
| description = state.progress | ||
| ? `Testing... ${state.progress.numPassedTests}/${state.progress.numTotalTests}` | ||
| : 'Starting...'; |
There was a problem hiding this comment.
logic: numTotalTests could be undefined here, which would show 'Testing... 5/undefined'
| aria-label={`Start ${state.name}`} | ||
| variant="ghost" | ||
| padding="small" | ||
| onClick={() => api.runTestProvider(state.id, {})} |
There was a problem hiding this comment.
logic: missing config parameter here but it's used in the useConfig hook. Should pass current config: api.runTestProvider(state.id, { config })
| {Object.entries(config).map(([key, value]) => ( | ||
| <div | ||
| key={key} | ||
| onClick={() => { | ||
| changeConfig({ [key]: !value }); | ||
| }} | ||
| > | ||
| {key}: {value ? 'ON' : 'OFF'} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
style: duplicate code block from non-editing state. Consider extracting to a shared component to avoid maintenance issues
| <div | ||
| style={{ | ||
| height, | ||
| transition: '1s height', |
There was a problem hiding this comment.
style: transition duration (1s) should match the one used in TestingModule component for consistency
| const screen = await within(canvasElement); | ||
|
|
||
| const toggleButton = await screen.getByLabelText('Collapse testing module'); | ||
| const content = await screen.findByText('CUSTOM CONTENT WITH DYNAMIC HEIGHT'); | ||
| const collapse = await screen.getByTestId('collapse'); |
There was a problem hiding this comment.
style: queries should have explicit timeouts since dynamic content may take longer to appear
| const crashed = testProviders.some((tp) => tp.crashed); | ||
| const failed = testProviders.some((tp) => tp.failed); | ||
| const testing = testProviders.length > 0; | ||
| if (!hasTestProviders && (!errorCount || !warningCount)) { |
There was a problem hiding this comment.
logic: condition should be (!hasTestProviders && !errorCount && !warningCount) to properly handle case when only warnings exist
What I did
configproperty to theTestProviderStateisEditingstate to addon test's renderChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Here's a concise summary of the key changes in this pull request:
Refactors the Test addon UI components and adds configuration support for improved organization and functionality.
Descriptioncomponent incode/addons/test/src/components/to handle test status display with proper state managementRelativeTimecomponent withIntl.RelativeTimeFormatfor localized time formattingTestProviderRendercomponent to handle test configuration and execution UIConfiginterface inconstants.tsTestingModulecomponentThe changes focus on better component organization, type safety, and new configuration capabilities while maintaining core functionality.
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!