-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/mcp templates #278
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
base: main
Are you sure you want to change the base?
Feat/mcp templates #278
Conversation
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 implements MCP (Managed Control Plane) templates feature to provide predefined configurations when creating managed control planes. The feature includes utility functions for handling prefixes/suffixes and IDP prefix stripping, type definitions for templates, and UI components to support template-based creation workflows.
- Adds utility functions for name formatting and IDP prefix handling
- Implements MCP template type definitions and helper functions
- Integrates template support into the creation wizard with form modifications and validation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
src/utils/stripIdpPrefix.ts | Utility function to remove IDP prefixes from principal names |
src/utils/buildNameWithPrefixesAndSufixes.ts | Function to apply name prefixes and suffixes with deduplication logic |
src/lib/api/types/templates/mcpTemplate.ts | Type definitions and helpers for MCP templates |
src/components/Wizards/CreateManagedControlPlane/CreateManagedControlPlaneWizardContainer.tsx | Main wizard container with template integration and form handling |
src/components/Dialogs/MetadataForm.tsx | Enhanced metadata form with prefix/suffix support and charging field controls |
src/components/ControlPlanes/List/ControlPlaneListWorkspaceGridTile.tsx | Grid tile component with member deduplication and template support |
src/components/ControlPlanes/ControlPlanesListMenu.tsx | Menu component for template selection |
src/components/ComponentsSelection/ComponentsSelectionContainer.tsx | Component selection with template defaults and validation |
src/components/ComponentsSelection/ComponentsSelection.tsx | Component selection UI with version validation |
public/locales/en.json | Localization strings for component version selection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
sufix?: string; | ||
}; | ||
name: { | ||
prefix?: string; | ||
sufix?: string; |
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.
The property name 'sufix' should be spelled 'suffix' for consistency with standard English spelling.
sufix?: string; | |
}; | |
name: { | |
prefix?: string; | |
sufix?: string; | |
suffix?: string; | |
}; | |
name: { | |
prefix?: string; | |
suffix?: string; |
Copilot uses AI. Check for mistakes.
sufix?: string; | ||
}; | ||
name: { | ||
prefix?: string; | ||
sufix?: string; |
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.
The property name 'sufix' should be spelled 'suffix' for consistency with standard English spelling.
sufix?: string; | |
}; | |
name: { | |
prefix?: string; | |
sufix?: string; | |
suffix?: string; | |
}; | |
name: { | |
prefix?: string; | |
suffix?: string; |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,25 @@ | |||
export function buildNameWithPrefixesAndSufixes( |
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.
The function name should be 'buildNameWithPrefixesAndSuffixes' - 'Sufixes' should be spelled 'Suffixes'.
export function buildNameWithPrefixesAndSufixes( | |
export function buildNameWithPrefixesAndSuffixes( |
Copilot uses AI. Check for mistakes.
|
||
const normalizeChargingTargetType = useCallback((val?: string | null) => (val ?? '').trim().toLowerCase(), []); | ||
|
||
// Here we will use OnboardingAPI to get all avaliable templates |
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.
The word 'avaliable' should be spelled 'available'.
// Here we will use OnboardingAPI to get all avaliable templates | |
// Here we will use OnboardingAPI to get all available templates |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,50 @@ | |||
import { buildNameWithPrefixesAndSufixes } from './buildNameWithPrefixesAndSufixes'; |
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.
The import should be updated to 'buildNameWithPrefixesAndSuffixes' to match the corrected function name.
Copilot uses AI. Check for mistakes.
nameSuffix: selectedTemplate?.spec.meta.name?.sufix ?? '', | ||
displayNamePrefix: selectedTemplate?.spec.meta.displayName?.prefix ?? '', | ||
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.sufix ?? '', |
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.
The property access should be 'suffix' instead of 'sufix' to match the corrected type definition.
nameSuffix: selectedTemplate?.spec.meta.name?.sufix ?? '', | |
displayNamePrefix: selectedTemplate?.spec.meta.displayName?.prefix ?? '', | |
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.sufix ?? '', | |
nameSuffix: selectedTemplate?.spec.meta.name?.suffix ?? '', | |
displayNamePrefix: selectedTemplate?.spec.meta.displayName?.prefix ?? '', | |
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.suffix ?? '', |
Copilot uses AI. Check for mistakes.
namePrefix: selectedTemplate?.spec.meta.name?.prefix ?? '', | ||
nameSuffix: selectedTemplate?.spec.meta.name?.sufix ?? '', | ||
displayNamePrefix: selectedTemplate?.spec.meta.displayName?.prefix ?? '', | ||
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.sufix ?? '', |
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.
The property access should be 'suffix' instead of 'sufix' to match the corrected type definition.
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.sufix ?? '', | |
displayNameSuffix: selectedTemplate?.spec.meta.displayName?.suffix ?? '', |
Copilot uses AI. Check for mistakes.
@@ -39,12 +39,16 @@ import { MetadataForm } from '../../Dialogs/MetadataForm.tsx'; | |||
import { EditMembers } from '../../Members/EditMembers.tsx'; | |||
import { ComponentsSelectionContainer } from '../../ComponentsSelection/ComponentsSelectionContainer.tsx'; | |||
import { IllustratedBanner } from '../../Ui/IllustratedBanner/IllustratedBanner.tsx'; | |||
import { ManagedControlPlaneTemplate, noTemplateValue } from '../../../lib/api/types/templates/mcpTemplate.ts'; | |||
import { buildNameWithPrefixesAndSufixes } from '../../../utils/buildNameWithPrefixesAndSufixes.ts'; |
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.
The import path should be updated to reflect the corrected function name 'buildNameWithPrefixesAndSuffixes'.
import { buildNameWithPrefixesAndSufixes } from '../../../utils/buildNameWithPrefixesAndSufixes.ts'; | |
import { buildNameWithPrefixesAndSuffixes } from '../../../utils/buildNameWithPrefixesAndSuffixes.ts'; |
Copilot uses AI. Check for mistakes.
]); | ||
|
||
const normalizeMemberRole = useCallback((r?: string | null) => { | ||
const v = (r ?? '').toString().trim().toLowerCase(); |
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.
please use meaningful variable names
@@ -123,17 +184,25 @@ export const CreateManagedControlPlaneWizardContainer: FC<CreateManagedControlPl | |||
); | |||
const componentsList = watch('componentsList'); | |||
|
|||
const hasMissingComponentVersions = useMemo(() => { | |||
const list = (componentsList ?? []) as ComponentsListItem[]; | |||
return list.some((c) => c?.isSelected && !c?.selectedVersion); |
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.
please use meaningful variable name or destructure object
return list.some(({isSelected,selectedVersion }) => isSelected && !selectedVersion);
<Input id="displayName" {...register('displayName')} className={styles.input} /> | ||
|
||
{resolvedDisplayNamePrefix || resolvedDisplayNameSuffix ? ( | ||
<div style={{ display: 'flex', gap: 8, alignItems: 'center' }}> |
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.
Move style to .css file
/> | ||
|
||
{resolvedNamePrefix || resolvedNameSuffix ? ( | ||
<div style={{ display: 'flex', gap: 8, alignItems: 'center' }}> |
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.
Move style to .css file
const resolvedDisplayNameSuffix = (propDisplayNameSuffix || '').trim(); | ||
|
||
const computeCore = (full: string, prefix: string, suffix: string) => { | ||
let v = full ?? ''; |
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.
please change to meaningful variable name
}, [availableManagedComponentsListData, setComponentsList]); | ||
}, [availableManagedComponentsListData, setComponentsList, defaultComponents]); | ||
|
||
useEffect(() => { |
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.
Can you check if this useEffect is necessary? Also the one 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.
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.
Maybe you can at least add comments describing what code inside these useEffects do to make it easier to understand
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.
Ok I think I misunderstood this request from @andreaskienle . I simplified it, merged it to one useEffect. The last useEffect was needed when we have template step at the beginning, but in this scenario it's not actually needed.
…ct/ui-frontend into feat/mcp-templates
…ct/ui-frontend into feat/mcp-templates
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.
LGTM :)
What this PR does / why we need it:
This is MCP templates feature without connected API
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: