-
Notifications
You must be signed in to change notification settings - Fork 3
Create MCP - Components Selection #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This reverts commit 8be43e6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a components-selection step to the Managed Control Plane (MCP) creation flow, including a utility to sort component versions, API type updates, and new UI components for selecting and displaying available components.
- Introduces
sortVersions
helper with unit tests - Extends
CreateManagedControlPlane
API/type to accept selected components and integrates selection logic in the wizard - Implements
ComponentsSelectionContainer
andComponentsSelection
UI with updatedInfobox
styling and i18n updates
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/utils/componentsVersions.ts | New version-sorting function |
src/utils/componentsVersions.spec.ts | Test suite covering version sorting and fallback behavior |
src/lib/api/types/crate/listManagedComponents.ts | Defines ListManagedComponents API resource |
src/lib/api/types/crate/createManagedControlPlane.ts | Updates MCP creation type to include component selection |
src/components/Wizards/CreateManagedControlPlaneWizardContainer.tsx | Adds “Component Selection” wizard step and state management |
src/components/Ui/Infobox/Infobox.tsx | Adds fullWidth prop |
src/components/Ui/Infobox/Infobox.module.css | Styles for full-width infobox |
src/components/Members/MemberRoleSelect.tsx | Switches static “Role” label to use translation key |
src/components/ControlPlanes/List/ControlPlaneListAllWorkspaces.tsx | Changes “Help” button text to use translation key |
src/components/ComponentsSelection/ComponentsSelectionContainer.tsx | New container fetches components list and initializes selection |
src/components/ComponentsSelection/ComponentsSelection.tsx | New UI for filtering, selecting, and listing components |
src/components/ComponentsSelection/ComponentsSelection.module.css | Styles for component list rows |
public/locales/en.json | Adds translation entries for component-selection UI |
Comments suppressed due to low confidence (5)
src/lib/api/types/crate/createManagedControlPlane.ts:58
- [nitpick] Using a positional
idpPrefix
parameter alongside anoptional
object may lead to confusion. Consider grouping both under a single options object for clarity.
idpPrefix?: string,
src/lib/api/types/crate/createManagedControlPlane.ts:60
- The new component-selection logic in
CreateManagedControlPlane
is substantial and currently untested. Consider adding unit tests for scenarios with and without selected components.
const componentsObject: Components =
src/components/Members/MemberRoleSelect.tsx:44
- Translation key
MemberTable.columnRoleHeader
is referenced but not defined inen.json
. Add this key (e.g.{ "MemberTable": { "columnRoleHeader": "Role" } }
) to avoid missing text.
{t('MemberTable.columnRoleHeader')}
src/components/ControlPlanes/List/ControlPlaneListAllWorkspaces.tsx:51
- Translation key
IllustratedBanner.helpButton
is used here but not defined inen.json
. Add an entry (e.g."IllustratedBanner": { "helpButton": "Help" }
) to your locale file.
{t('IllustratedBanner.helpButton')}
src/components/Ui/Infobox/Infobox.module.css:7
- The
.full-width
class does not set an explicit width, sofullWidth
may not expand the infobox as intended. Consider addingwidth: 100%;
or appropriate width constraints.
.full-width {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heyo! Good job with the new components, the UI looks clean and polished. It’s also great that you added tests for the version sorting and implemented proper types. 👍
From an architectural point of view, I think we can make the state management even clearer and more robust. Currently, the selectedComponents
state in <CreateManagedControlPlaneWizardContainer>
is responsible for two things at once: It holds the list of all available components from the backend, and it also tracks the user’s selections via the isSelected
flag. A child component is responsible for fetching the data and setting this state, which makes the data flow a bit hard to follow.
I think it would be cleaner to separate these two concerns:
- (1) Available Components: A read-only list of components/versions fetched from the backend.
- (2) Selected Components: The user‘s actual selections, which are part of the form data.
For (1), we can keep this local, i.e. in the <ComponentsSelectionContainer>
(this is also where we currently fetch the data).
To manage the selected components (2), what do you think about integrating it fully into React Hook Form, just like name
and displayName
? We could remove the useState
for components and instead register components as a field in RHF. This would give us a single source of truth for our form data and simplify validation logic.
This approach should make the data flow strictly unidirectional and easier to reason about. What are your thoughts?
Apart from this, I also added some minor suggestions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with the condition to make changes in another PR - to show our feature at d-com :)
What this PR does / why we need it:
Create MCP - Components Selection form