[LG-5362] Add chat/suggestions #2963
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
stephl3
reviewed
Jul 18, 2025
chat/suggestion-card/src/SuggestionCard/SuggestionCard.types.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.types.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
|
Size Change: -1.88 kB (-0.1%) Total Size: 1.91 MB
ℹ️ View Unchanged
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
stephl3
reviewed
Jul 23, 2025
Comment on lines
7
to
8
| Unset: 'unset', | ||
| Apply: 'apply', |
There was a problem hiding this comment.
can you share more on how/when this 'unset' status will be used? seems like all we really need is 'apply' but curious if there's context I'm missing
There was a problem hiding this comment.
aha i'm seeing it now great point! i must have intuitively thought there would be a case where we don't want users to apply suggestions but it looks like that's not included in the designs at all so will remove this case!
chat/suggestion-card/package.json
Outdated
| @@ -0,0 +1,52 @@ | |||
|
|
|||
| { | |||
| "name": "@lg-chat/suggestion-card", | |||
There was a problem hiding this comment.
What do you think about this renamed structure for the package and main component export?
package: @lg-chat/suggestions (not limited to the card UI)
main component: SuggestedActions (has both the main card but also a status banner attached)
There was a problem hiding this comment.
SGTM! will update the naming
Comment on lines
24
to
43
| /** | ||
| * Determines rendering of the status banner: | ||
| * - `'Unset'` will render suggestions | ||
| * - `'Apply'` will render suggestions and the "Apply" button | ||
| * - `'Success'` will render success banner with applied suggestions | ||
| * - `'Error'` will render error banner with instructions to manually apply suggestions | ||
| */ | ||
| status: Status; | ||
| /** | ||
| * Suggested configuration parameters as key-value pairs | ||
| */ | ||
| suggestedConfigurationParameters: ConfigurationParameters; | ||
| /** | ||
| * Parameters that were successfully applied | ||
| */ | ||
| appliedParameters?: ConfigurationParameters; | ||
| /** | ||
| * Parameters that failed to apply | ||
| */ | ||
| failedParameters?: ConfigurationParameters; |
There was a problem hiding this comment.
does this API make sense for how data will flow through this component?
also curious if we might be able to DRY this up by attaching a status to each config param?
There was a problem hiding this comment.
mmm gotcha will consolidate the parameters to be filtered by status!
i think having the status, configurationParameters, and onClickApply make sense, we have separate utils set up in mms to handle the actual application of these configurations and handle errors, so we can dynamically update this component with these props
stephl3
reviewed
Jul 23, 2025
chat/suggestion-card/src/SuggestionCard/SuggestionCard.styles.ts
Outdated
Show resolved
Hide resolved
Comment on lines
5
to
9
| import { | ||
| bannerWrapperStyles, | ||
| boldedTextStyle, | ||
| listStyles, | ||
| } from '../SuggestionCard/SuggestionCard.styles'; |
There was a problem hiding this comment.
can you move these styles into a separate StatusBanner.styles.ts?
Comment on lines
20
to
24
| }: { | ||
| status: Status; | ||
| appliedParameters: SuggestionCardProps['appliedParameters']; | ||
| failedParameters: SuggestionCardProps['failedParameters']; | ||
| }) => { |
There was a problem hiding this comment.
There's a circular import between StatusBanner + SuggestionCard because
StatusBannerimports types defined fromSuggestionCardSuggestionCardimportsStatusBanner
Can the status + appliedParameters + failedParameters be defined in a separate StatusBanner.types.ts or a separate src/shared.types.ts file? ConfigurationParameters type can also be defined there
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new SuggestionCard component (@lg-chat/suggestions) that displays MongoDB cluster configuration suggestions with support for applying changes and showing success/error feedback. The component supports three states: applying suggestions with an "Apply" button, showing success banner with applied parameters, and showing error banner with failed parameters.
- Adds a new
SuggestedActionscomponent with table display for configuration parameters - Implements status-based rendering (Apply/Success/Error) with appropriate banners
- Includes comprehensive test coverage and Storybook integration
Reviewed Changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tools/install/src/ALL_PACKAGES.ts |
Adds the new package to the global package list |
chat/suggestions/tsconfig.json |
TypeScript configuration for the new package |
chat/suggestions/src/ |
Core component implementation with types, styles, and utilities |
chat/suggestions/package.json |
Package configuration and dependencies |
chat/suggestions/README.md |
Documentation with usage examples and API reference |
README.md |
Updates main README to include new package in table of contents |
Comments suppressed due to low confidence (2)
chat/suggestions/src/testing/getTestUtils.spec.tsx:7
- Empty test case provides no validation. The test should either be removed or implemented to actually test the getTestUtils function.
test('condition', () => {});
| import React from 'react'; | ||
| import { render } from '@testing-library/react'; | ||
|
|
||
| import { SuggestedActions } from '.'; |
There was a problem hiding this comment.
The import SuggestedActions from '.' is incorrect. The current directory contains test utilities, not the component. This should import from '../' or '../SuggestedActions' to access the actual component.
Suggested change
| import { SuggestedActions } from '.'; | |
| import { SuggestedActions } from '../SuggestedActions'; |
Copilot uses AI. Check for mistakes.
Comment on lines
+1
to
+2
| import { findByLgId, getByLgId, queryByLgId } from '@lg-tools/test-harnesses'; | ||
|
|
There was a problem hiding this comment.
The imported test utility functions findByLgId, getByLgId, and queryByLgId are not used in the function. Either implement the test utilities or remove the unused imports.
Suggested change
| import { findByLgId, getByLgId, queryByLgId } from '@lg-tools/test-harnesses'; |
Copilot uses AI. Check for mistakes.
stephl3
approved these changes
Jul 24, 2025
Comment on lines
8
to
14
| /** | ||
| * Determines rendering of the status banner: | ||
| * - `'Apply'` will render suggestions and the "Apply" button | ||
| * - `'Success'` will render success banner with applied suggestions | ||
| * - `'Error'` will render error banner with instructions to manually apply suggestions | ||
| */ | ||
| status: Status; |
There was a problem hiding this comment.
Sorry, I should have surfaced this in a prior review: can we rename the prop state, rename the type State and use Unset: 'unset' for the default? This would match conventions in the rest of our codebase
joyjwang
changed the title
joyjwang
force-pushed
the
joyjwang/suggestion-card
branch
from
52231e9 to
25aaeb5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
add SuggestedActions component
🎟 _Jira ticket: LG-5362
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes