Core / Addon Test: Add config UI to Testing Module #29708
Conversation
…nto testing-module-settings
ghengeveld
changed the title
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 2f3ec95. 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 2 targetsSent with 💌 from NxCloud. |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -47,8 +48,6 @@ addons.register(ADDON_ID, (api) => { | |||
| return <ContextMenuItem context={context} state={state} />; | |||
| }, | |||
|
|
|||
| render: (state) => <TestProviderRender api={api} state={state} />, | |||
|
|
|||
| mapStatusUpdate: (state) => | |||
| Object.fromEntries( | |||
| (state.details.testResults || []).flatMap((testResult) => | |||
There was a problem hiding this comment.
style: flatMap().map().filter(Boolean) chain could be simplified to just flatMap with proper return type handling
| ), | ||
| ...baseState, | ||
| watching: true, | ||
| }, | ||
| ]; | ||
|
|
||
| let triggerUpdate: () => void; | ||
| const channel = mockChannel(); | ||
| const managerContext: any = { |
There was a problem hiding this comment.
style: avoid using 'any' type - consider creating proper type definition for managerContext
| const button = await canvas.findByRole('button', { name: /Expand/ }); | ||
| await fireEvent.click(button); | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); |
There was a problem hiding this comment.
style: using regex for button text matching is fragile - consider using a more specific selector or data-testid
| ), | ||
| ...baseState, | ||
| watching: true, | ||
| }, | ||
| ]; | ||
|
|
||
| let triggerUpdate: () => void; |
There was a problem hiding this comment.
logic: triggerUpdate is mutable and could lead to race conditions in tests - consider using a more robust pattern
There was a problem hiding this comment.
9 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| ({ percentage }) => ({ | ||
| width: percentage ? 12 : 6, | ||
| height: percentage ? 12 : 6, | ||
| margin: percentage ? 1 : 4, | ||
| background: percentage | ||
| ? `conic-gradient(var(--status-color) ${percentage}%, var(--status-background) ${percentage + 1}%)` | ||
| : 'var(--status-color)', | ||
| borderRadius: '50%', | ||
| }), |
There was a problem hiding this comment.
style: Consider using CSS custom properties for dimensions (width, height, margin) to make the component more maintainable and configurable
| : 'var(--status-color)', | ||
| borderRadius: '50%', | ||
| }), | ||
| ({ status, theme }) => |
There was a problem hiding this comment.
style: Consider using a switch statement or object literal for status color mapping to avoid repetitive code blocks
| useEffect(() => { | ||
| let timeout: NodeJS.Timeout; | ||
| const handler = () => { | ||
| setUpdated(true); | ||
| timeout = setTimeout(setUpdated, 1000, false); | ||
| }; | ||
| api.on(TESTING_MODULE_CONFIG_CHANGE, handler); | ||
| return () => { | ||
| api.off(TESTING_MODULE_CONFIG_CHANGE, handler); | ||
| clearTimeout(timeout); | ||
| }; | ||
| }, [api]); |
There was a problem hiding this comment.
logic: potential memory leak if component unmounts during the 1000ms timeout - consider using AbortController or similar for cleaner cancellation
| display: testing ? 'block' : 'none', | ||
| maxHeight: collapsed ? 0 : maxHeight, | ||
| maxHeight: collapsed ? 0 : animating ? maxHeight : 'auto', | ||
| }} |
There was a problem hiding this comment.
style: maxHeight transition can cause layout shift when content height changes while expanded - consider using CSS height: auto with transition
| backgroundColor: `var(--sb-sidebar-bottom-card-background, ${theme.background.content})`, | ||
| borderRadius: | ||
| `var(--sb-sidebar-bottom-card-border-radius, ${theme.appBorderRadius + 1}px)` as any, | ||
| boxShadow: `inset 0 0 0 1px ${crashed && !running ? theme.color.negative : updated ? theme.color.positive : theme.appBorderColor}, var(--sb-sidebar-bottom-card-box-shadow, 0 1px 2px 0 rgba(0, 0, 0, 0.05), 0px -5px 20px 10px ${theme.background.app})`, |
There was a problem hiding this comment.
style: long line with complex template literal makes code hard to maintain - consider breaking into multiple lines or variables
… if it's equal to the previous value
What I did
Added a settings/config view to the Testing Module for the Test addon, which allows toggling coverage and a11y tests:
Screen.Recording.2024-11-26.at.09.38.48.mov
Whenever config is updated, the testing module briefly glows green.
Checklist 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!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 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
Added settings UI to the Testing Module in Storybook's test addon, enabling users to toggle coverage and accessibility testing options with visual feedback for configuration changes.
TestProviderRendercomponent with settings panel UI and state management in/code/addons/test/src/components/TestProviderRender.tsxTestStatusIconcomponent in/code/addons/test/src/components/TestStatusIcon.tsxfor visual status indicators with percentage displayListItemcomponent in/code/core/src/components/components/tooltip/ListItem.tsxto support label elements for settings UI/code/core/src/manager/components/sidebar/TestingModule.tsxwith proper cleanup💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!