fix: Upgrade cmdk for combobox and add test for combobox #434
Conversation
ENG-493 Combobox - design update
kingston could we make the options inside the combobox to be more like shadcn in terms of that check icon is on the left since we made it like that in Figma? Design: https://www.figma.com/design/syKDQG1N5vFDH0y67eUNXC/Baseplate-Design-System-2024?node-id=444-6768 Shadcn/ui example: |
🦋 Changeset detectedLatest commit: 0ae99ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request updates several packages by upgrading the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test File
participant R as renderWithProviders()
participant P as UiComponentProviders
participant CSP as ComponentStringsProvider
participant C as Component Under Test
T->>R: Invoke renderWithProviders(component)
R->>P: Wrap component with providers
P->>CSP: Inject default component strings
CSP->>C: Render component with context
C-->>T: Return rendered output
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/tools/vitest.config.react.js (2)
4-16: Move setup instructions to documentation.Consider moving the setup instructions from comments to a dedicated README or documentation file for better maintainability and visibility.
25-25: Consider making setup file path configurable.The hardcoded setup file path
'./tests/setup.ts'might not work for all projects. Consider making it configurable through environment variables or a configuration file.- setupFiles: ['./tests/setup.ts'], + setupFiles: [process.env.VITEST_SETUP_FILE || './tests/setup.ts'],packages/ui-components/src/tests/setup.ts (1)
14-24: Consider enhancing ResizeObserver mock.The current ResizeObserver mock is minimal. For more accurate testing, consider implementing basic size tracking:
globalThis.ResizeObserver = class ResizeObserver { + private callback: ResizeObserverCallback; + private elements = new Set<Element>(); + + constructor(callback: ResizeObserverCallback) { + this.callback = callback; + } + observe(element: Element): void { - // do nothing + this.elements.add(element); } + unobserve(element: Element): void { - // do nothing + this.elements.delete(element); } + disconnect(): void { - // do nothing + this.elements.clear(); } };packages/ui-components/src/tests/render.test-helper.tsx (1)
10-23: LGTM! Consider making providers composable.The implementation is clean and follows best practices. For future extensibility, consider making the providers composable:
+type ProvidersProps = { + children: React.ReactNode; + providers?: React.ComponentType<{ children: React.ReactNode }>[]; +}; + const UiComponentProviders = ({ children, + providers = [], -}: { - children: React.ReactNode; -}): React.JSX.Element => ( +}: ProvidersProps): React.JSX.Element => ( + providers.reduce( + (acc, Provider) => <Provider>{acc}</Provider>, + <ComponentStringsProvider value={DEFAULT_COMPONENT_STRINGS}> + {children} + </ComponentStringsProvider> + ) - <ComponentStringsProvider value={DEFAULT_COMPONENT_STRINGS}> - {children} - </ComponentStringsProvider> ); export const renderWithProviders = ( ui: ReactElement, - options?: Omit<RenderOptions, 'wrapper'>, + options?: Omit<RenderOptions, 'wrapper'> & { providers?: ProvidersProps['providers'] }, -): RenderResult => render(ui, { wrapper: UiComponentProviders, ...options }); +): RenderResult => render(ui, { + wrapper: ({ children }) => ( + <UiComponentProviders providers={options?.providers}>{children}</UiComponentProviders> + ), + ...options +});packages/ui-components/src/components/Combobox/Combobox.test.tsx (3)
48-71: LGTM! Consider adding accessibility tests.The rendering tests are well-structured. Consider adding tests for:
- ARIA labels and roles
- Screen reader announcements
- Focus management
73-126: LGTM! Consider adding error state tests.The user interaction tests are comprehensive. Consider adding tests for:
- Invalid input handling
- Error state display
- Network error handling
128-198: LGTM! Consider adding loading state tests.The keyboard navigation tests are thorough. Consider adding tests for:
- Loading state display
- Async option loading
- Debounced search
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/serious-feet-hide.md(1 hunks)packages/tools/package.json(1 hunks)packages/tools/vitest.config.react.js(1 hunks)packages/ui-components/package.json(2 hunks)packages/ui-components/src/components/Combobox/Combobox.stories.tsx(1 hunks)packages/ui-components/src/components/Combobox/Combobox.test.tsx(1 hunks)packages/ui-components/src/components/Combobox/Combobox.tsx(3 hunks)packages/ui-components/src/components/Select/Select.stories.tsx(1 hunks)packages/ui-components/src/dummy.test.ts(0 hunks)packages/ui-components/src/tests/render.test-helper.tsx(1 hunks)packages/ui-components/src/tests/setup.ts(1 hunks)packages/ui-components/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/ui-components/src/dummy.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/ui-components/vitest.config.ts
- packages/ui-components/src/components/Select/Select.stories.tsx
- packages/ui-components/src/components/Combobox/Combobox.stories.tsx
🔇 Additional comments (9)
packages/ui-components/src/tests/setup.ts (1)
1-10: LGTM!The setup is well-documented with clear references to GitHub issues for the workarounds. The cleanup implementation is correct.
Also applies to: 27-29
packages/ui-components/src/components/Combobox/Combobox.test.tsx (1)
200-226: LGTM! Edge cases are well covered.The edge case tests effectively cover important scenarios like empty results and maintaining selected values.
packages/ui-components/src/components/Combobox/Combobox.tsx (3)
59-59: LGTM! Good practice extracting the default option.Extracting the default option into a named constant improves code maintainability and reduces magic values.
486-488: LGTM! Good documentation.The added documentation improves code maintainability by clearly describing the component's purpose.
218-229: Verify the impact of removingisOpenfrom dependency array.The removal of
isOpenfrom thehandleKeydowndependency array could lead to stale closure issues. The function might not have access to the latestisOpenstate.Run this script to check if
isOpenis used within the function:✅ Verification successful
Final Determination: No Stale Closure Issue Found
The ast-grep check confirms that the
isOpenvariable is not referenced within thehandleKeydowncallback. Since the function only callssetIsOpenbased on certain key events and does not depend on the currentisOpenvalue, omitting it from the dependency array is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if isOpen is used within handleKeydown function ast-grep --pattern $'const handleKeydown = React.useCallback( ($_, $_) => { $$$ isOpen $$$ }, $$$ )'Length of output: 119
.changeset/serious-feet-hide.md (1)
1-6: LGTM! Clear and concise changeset.The changeset accurately describes the package upgrade and test additions.
packages/tools/package.json (1)
30-31: LGTM! Good addition of vitest-react configuration.The addition of the vitest-react export supports the PR objective of adding combobox tests.
packages/ui-components/package.json (2)
94-97: LGTM! Good addition of testing dependencies.The addition of React Testing Library dependencies aligns with the PR objective of adding combobox tests.
71-71: Verify cmdk upgrade compatibility.Please verify that the upgrade from cmdk 1.0.0 to 1.0.4 doesn't introduce breaking changes.
✅ Verification successful
Cmdk Upgrade Compatibility Verification
The cmdk upgrade from 1.0.0 to 1.0.4 does not introduce any additional breaking changes. All the breaking changes were established in the v1.0.0 release (such as case sensitivity for the
valueprop, mandatory wrapping with<Command.List>, and updated CSS attribute selectors). Versions 1.0.1 through 1.0.4 focus on bug fixes and dependency updates.No further modifications are required beyond what was addressed for v1.0.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What are the breaking changes between cmdk versions 1.0.0 and 1.0.4?Length of output: 3747
Summary by CodeRabbit
New Features
Tests
Chores