Custom Resource Definition Schema based Intellisense for YAML viewer and editor #364
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
lucasgoral
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds schema-based IntelliSense for YAML preview and editing by integrating Custom Resource Definitions (CRDs) with the Monaco YAML editor. The implementation fetches CRD schemas from the Kubernetes API and provides autocomplete, validation, and documentation for YAML resources.
Key Changes
- Integrated
monaco-yamllibrary with CRD schema support for IntelliSense in the YAML editor - Added role-based access control (RBAC) to restrict editing capabilities based on MCP admin rights
- Refactored API resource fetching to support overriding MCP configuration for CRD lookups
Reviewed Changes
Copilot reviewed 31 out of 33 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.js | Added path-browserify polyfill and build configuration for Node.js modules in browser |
| package.json | Added dependencies: monaco-yaml integration library and path-browserify |
| src/utils/getPluralName.ts | Utility to map CRD kind names to their plural forms for API requests |
| src/types/customResourceDefinition.ts | TypeScript interfaces for Kubernetes CRD structure and JSON Schema |
| src/lib/api/useApiResource.ts | Updated to support overriding MCP config for fetching CRDs without MCP context |
| src/lib/api/types/k8s/listCustomResourceDefinition.ts | Removed jq filter to return full list object |
| src/lib/api/types/crate/customResourceDefinitionObject.ts | New resource type for fetching individual CRDs |
| src/lib/api/types/crate/controlPlanes.ts | Added authorization interfaces and included in API response |
| src/spaces/mcp/auth/AuthContextMcp.tsx | Added hasMCPAdminRights flag based on role bindings |
| src/spaces/mcp/pages/McpPage.tsx | Passed role bindings to AuthProviderMcp |
| src/components/YamlEditor/YamlEditor.tsx | Integrated monaco-yaml with schema support, keyboard event handling, and formatting options |
| src/components/YamlEditor/YamlEditor.module.css | Styling for editor layout and validation errors |
| src/components/Yaml/YamlViewerSchemaLoader.tsx | Component to load CRD schema and pass to YamlViewer |
| src/components/Yaml/YamlViewer.tsx | Updated to accept and pass schema to editor |
| src/components/Yaml/YamlViewButton.tsx | Added toolbar content customization for edit button |
| src/components/Yaml/YamlSidePanel.tsx | Integrated schema loader and custom toolbar content |
| src/components/Yaml/YamlSidePanelWithLoader.tsx | Added CRD fetching for schema support |
| src/components/Wizards/CreateManagedControlPlane/YamlSummarize.tsx | Renamed from YamlPanel and integrated schema loader |
| src/components/Wizards/CreateManagedControlPlane/YamlSummarize.module.css | New styling for summarize view |
| src/components/Wizards/CreateManagedControlPlane/SummarizeStep.tsx | Updated to use YamlSummarize with schema support |
| src/components/Wizards/CreateManagedControlPlane/EditManagedControlPlaneWizardDataLoader.tsx | Updated API override parameter |
| src/components/Members/ImportMembersDialog.tsx | Updated API override parameter |
| src/components/Graphs/Graph.tsx | Added API config context to YAML side panel |
| src/components/ControlPlane/ProvidersConfig.tsx | Added edit button to YAML view with admin rights check |
| src/components/ControlPlane/ManagedResources.tsx | Added edit button and Flux-managed resource detection with admin rights |
| src/components/ControlPlane/Kustomizations.tsx | Added edit button with admin rights check |
| src/components/ControlPlane/GitRepositories.tsx | Added edit button with admin rights check |
| src/components/ControlPlane/ActionsMenu.tsx | Added tooltip support for action items |
| public/openapi.json | Sample OpenAPI schema (appears unused in code) |
| public/locales/en.json | Updated translations for new UI elements |
Comments suppressed due to low confidence (1)
src/components/Wizards/CreateManagedControlPlane/YamlSummarize.tsx:48
- The
isEditprop is set tofalsein the YamlSummarize component, but this was previously set totrue(as indicated by the removed line). If this is a summarize/review step,isEdit={false}is likely correct to make it read-only. However, please verify this change is intentional.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Overall, works very nicely with IntelliSense 👍🏻 I have some remarks on the architecture.
Prop drilling with apiConfig
Passing apiConfig seems necessary because some fetches require MCP details. In the main window, components are nested under <AuthProviderMcp><WithinManagedControlPlane>, so the fetch can read these details from the context. In the splitter, that context is not available because the splitter sits higher in the tree.
What do you think of providing a second hook to open the splitter in the context of an MCP, maybe useSplitterMcp, which simply wraps its children inside <AuthProviderMcp><WithinManagedControlPlane>? Then I think we wouldn't need to pass the apiConfig through multiple layers.
export function useSplitterMcp() {
const context = use(SplitterContext);
if (!context) {
throw new Error('useSplitterMcp must be used within an SplitterProvider.');
}
const openInAside = useCallback(
(content: ReactNode) => {
context.openInAside(
<AuthProviderMcp>
<WithinManagedControlPlane>{content}</WithinManagedControlPlane>
</AuthProviderMcp>,
);
},
[context],
);
return useMemo(
() => ({
...context,
openInAside,
}),
[context, openInAside],
);
}Component structure
The component architecture is a bit hard to follow in some places.
Proposal:
- Rename
YamlViewerSchemaLoaderto something likeResourceYamlEditororYamlResourceEditoror similar to reflect that it is an editor for a resource's YAML. - Move extraction of
apiGroupName,apiVersionandkindinto that component to reduce duplication (currently inYamlSidePanelandSummarizeStep.) - That component should then download the CRD file (preferably via a dedicated hook)
I created a follow-up task for some more UI and technical improvements. Please add anything else you'd like to track there: openmcp-project/backlog#358
| icon={'edit'} | ||
| design={'Transparent'} | ||
| disabled={isFluxManaged} | ||
| tooltip={t('yaml.fluxManaged')} |
There was a problem hiding this comment.
We should only show this tooltip if the resource is managed by flux
| "js-yaml": "4.1.1", | ||
| "monaco-editor": "0.54.0", | ||
| "monaco-yaml": "5.4.0", | ||
| "path-browserify": "1.0.1", |
There was a problem hiding this comment.
Just out of curiosity, why is this dependency needed?
| kustomization: 'kustomizations', | ||
| }; | ||
|
|
||
| export const getCustomResourceDefinitionPluralName = (kind?: string): string | undefined => { |
There was a problem hiding this comment.
There is already a hook to retrieve plural names, useResourcePluralNames, can't we use this? Do we need to enhance the other hook? It just feels a bit weird to have these hardcoded constants here
| import { useAuthOnboarding } from '../../onboarding/auth/AuthContextOnboarding.tsx'; | ||
| import { useMcp } from '../../../lib/shared/McpContext.tsx'; | ||
|
|
||
| export function useHasMcpAdminRights(): boolean { |
There was a problem hiding this comment.
Can we add a unit test for this?
There was a problem hiding this comment.
Is this file used anywhere?
| yamlString={yamlString} | ||
| filename={`mcp_${projectName}--ws-${workspaceName}`} | ||
| apiVersion={apiVersion} | ||
| apiGroupName={apiGroupName} |
There was a problem hiding this comment.
Missing kind, without this, the IntelliSense doesn’t work in the create MCP flow. But see also the top comment for the PR about how we could centralize this more. Ideally this is also extracted from the YAML file directly.
| apiGroupName={apiGroupName} | |
| apiGroupName={apiGroupName} | |
| kind="ManagedControlPlane" |
| const customResourceDefinitionName = getCustomResourceDefinitionPluralName(kind); | ||
| const { data: crdData, isLoading } = useApiResource<CustomResourceDefinition>( | ||
| { | ||
| path: `/apis/apiextensions.k8s.io/v1/customresourcedefinitions/${customResourceDefinitionName}.${apiGroupName}`, | ||
| }, | ||
| undefined, | ||
| apiConfig?.mcpConfig, | ||
| !customResourceDefinitionName, | ||
| ); | ||
|
|
||
| const schema = | ||
| crdData?.spec.versions?.find(({ name }) => name === apiVersion)?.schema.openAPIV3Schema ?? | ||
| crdData?.spec.versions?.[0].schema.openAPIV3Schema; | ||
| const editorInstanceSchema = useMemo(() => (schema ? openapiSchemaToJsonSchema(schema) : undefined), [schema]); |
There was a problem hiding this comment.
Can we extract this into a hook, useCustomResourceDefinitionQuery or similar? This hook could then extract the URL parameters, deal with plural names, download the file, convert it, filter the results, convert it back again etc.
And we could add a test for that hook ;)
| crdData?.spec.versions?.[0].schema.openAPIV3Schema; | ||
| const editorInstanceSchema = useMemo(() => (schema ? openapiSchemaToJsonSchema(schema) : undefined), [schema]); | ||
|
|
||
| if (customResourceDefinitionName && isLoading) { |
| return () => dispose(); | ||
| }, [schema]); | ||
|
|
||
| // Capture Space at the window level before document-level handlers and stop propagation if inside editor |
There was a problem hiding this comment.
Out of curiosity, why is this needed? I played around a bit, for me this doesn't seem to do anything special. I use Chrome. Do you remember the use case for that?
| const typedParentOnMount = parentOnMount as | ||
| | ((editor: monaco.editor.IStandaloneCodeEditor, monacoNs: typeof monaco) => void) | ||
| | undefined; | ||
| typedParentOnMount?.(editorInstance, monacoApi); |
There was a problem hiding this comment.
Also here, do you recall why this is needed? I tested a bit and parentOnMount is always undefined for me, so this doesn’t seem to have any effect.
What this PR does / why we need it:
Added schema based Intellisense for YAML preview/edit