OCPBUGS-70340: Ensure OpenAPI in monaco stays fresh #15890
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-70340, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a function to build Monaco YAML schemas, refactors Monaco YAML registration to return a shared instance, updates the code editor to store and refresh that instance and subscribe to a refresh event, and emits a DOM event when swagger definitions update to trigger schema refreshes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/shared
|
@logonoff: This pull request references Jira Issue OCPBUGS-70340, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![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 (1)
frontend/packages/console-shared/src/components/editor/CodeEditor.tsx (1)
61-68: LGTM!The effect correctly initializes YAML schemas and listens for swagger refresh events. The cleanup ensures no memory leaks.
The effect re-runs when
monacoYamlchanges, causing the event listener to be removed and re-added. While this works correctly, you could optimize by separating concerns:🔎 Optional optimization to separate initial update from event listener setup
+ useEffect(() => { + updateSwaggerDefinitions(); + }, [updateSwaggerDefinitions]); + - useEffect(() => { - updateSwaggerDefinitions(); - + useEffect(() => { window.addEventListener('console_swagger_refresh', updateSwaggerDefinitions); return () => { window.removeEventListener('console_swagger_refresh', updateSwaggerDefinitions); }; }, [updateSwaggerDefinitions]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/packages/console-shared/src/components/editor/CodeEditor.tsxfrontend/packages/console-shared/src/components/editor/yaml-editor-utils.tsfrontend/public/module/k8s/swagger.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-shared/src/components/editor/yaml-editor-utils.tsfrontend/public/module/k8s/swagger.tsfrontend/packages/console-shared/src/components/editor/CodeEditor.tsx
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (1)
frontend/public/module/k8s/swagger.ts (1)
getSwaggerDefinitions(26-26)
frontend/packages/console-shared/src/components/editor/CodeEditor.tsx (2)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts (2)
CodeEditorRef(692-695)CodeEditorProps(680-690)frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (2)
registerYAMLinMonaco(118-141)getConsoleYamlSchemas(95-109)
🔇 Additional comments (9)
frontend/public/module/k8s/swagger.ts (1)
40-40: LGTM!The event dispatch is correctly placed after successful swagger definition assignment, enabling listeners to reactively update YAML schemas.
frontend/packages/console-shared/src/components/editor/CodeEditor.tsx (4)
8-13: LGTM!The import additions correctly support the new YAML schema synchronization workflow.
21-23: LGTM!The state correctly captures the YAML Monaco instance for subsequent schema updates.
38-38: LGTM!Correctly stores the YAML Monaco instance returned by
registerYAMLinMonacofor later schema updates.
53-59: LGTM!The callback correctly fetches fresh YAML schemas and updates the Monaco YAML instance. The null check ensures safe operation before the editor mounts.
frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (4)
3-3: LGTM!The
SchemasSettingstype import is correctly added to support the new schema configuration functionality.
116-116: LGTM!The module-level variable correctly maintains a singleton instance of the Monaco YAML configuration, preventing duplicate registrations.
118-141: LGTM!The function correctly returns the Monaco YAML instance for later schema updates. The check on line 128 prevents duplicate language registrations.
Note: This function depends on
getConsoleYamlSchemas()which should be updated to handle null swagger definitions (see previous comment).
95-109: openAPItoJSONSchema already handles null input gracefully.The concern about passing null to
openAPItoJSONSchema()without validation is not applicable here. The function already includes a null check (lines 93-94 of openapi-to-json-schema.ts) that returnsnullif the input is falsy, preventing any runtime errors.However, returning a schema object with a
nullschema property when swagger definitions haven't loaded yet is suboptimal. Returning an empty array would be preferable to avoid registering an incomplete schema.Likely an incorrect or invalid review comment.
logonoff
force-pushed
the
OCPBUGS-70340-yolo-swagger
branch
from
14ae623 to
32493e3
Compare
logonoff
force-pushed
the
OCPBUGS-70340-yolo-swagger
branch
from
32493e3 to
f7ba996
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (1)
115-115: InitializecurrentMonacoYamlInstanceexplicitly.The variable is typed as
| nullbut is implicitlyundefinedinitially. For type consistency, explicitly initialize it to match the type annotation.♻️ Proposed fix
-let currentMonacoYamlInstance: ReturnType<typeof configureMonacoYaml> | null = null; +let currentMonacoYamlInstance: ReturnType<typeof configureMonacoYaml> | null = null;Actually, looking more carefully at the existing code, it appears the initialization is missing. Add
= null:-let currentMonacoYamlInstance: ReturnType<typeof configureMonacoYaml> | null; +let currentMonacoYamlInstance: ReturnType<typeof configureMonacoYaml> | null = null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/packages/console-shared/src/components/editor/CodeEditor.tsxfrontend/packages/console-shared/src/components/editor/yaml-editor-utils.tsfrontend/public/module/k8s/swagger.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/module/k8s/swagger.tsfrontend/packages/console-shared/src/components/editor/yaml-editor-utils.tsfrontend/packages/console-shared/src/components/editor/CodeEditor.tsx
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (1)
frontend/public/module/k8s/swagger.ts (1)
getSwaggerDefinitions(26-26)
frontend/packages/console-shared/src/components/editor/CodeEditor.tsx (3)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts (2)
CodeEditorRef(692-695)CodeEditorProps(680-690)frontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (2)
registerYAMLinMonaco(117-139)getConsoleYamlSchemas(95-109)frontend/public/module/auth.js (1)
window(5-12)
🔇 Additional comments (8)
frontend/public/module/k8s/swagger.ts (1)
40-40: LGTM!The event dispatch is correctly placed after updating
swaggerDefinitionsand uses a standard DOM event pattern. This enables other components to react to swagger definition updates.frontend/packages/console-shared/src/components/editor/CodeEditor.tsx (5)
8-13: LGTM!The additional imports from
yaml-editor-utilsare necessary for the YAML schema refresh functionality and are all properly utilized in the component.
21-23: LGTM!The
monacoYAMLstate correctly stores the Monaco YAML instance with proper typing, enabling schema updates after initialization.
38-38: LGTM!Storing the Monaco YAML instance enables dynamic schema updates. The placement within the YAML language case is correct.
53-59: LGTM!The callback correctly guards against null
monacoYAMLand fetches fresh schemas on each invocation. The dependency array is accurate.
61-68: LGTM!The effect correctly:
- Updates schemas on mount (when swagger may already be loaded)
- Subscribes to the
console_swagger_refreshevent- Cleans up the listener on unmount
- Re-runs when
monacoYAMLbecomes available, ensuring proper subscription timingfrontend/packages/console-shared/src/components/editor/yaml-editor-utils.ts (2)
3-3: LGTM!The
SchemasSettingsimport is necessary for the newgetConsoleYamlSchemasfunction.
117-139: LGTM!The singleton pattern is correctly implemented:
- The guard prevents re-registration by checking YAML language count
- The function returns the shared instance for later updates
- The logic handles both initial registration and subsequent calls appropriately
|
@logonoff: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/assign @yapei |
3 participants
Changes:
registerYAMLinMonacoreturns a pointer to the monacoyaml.configurefunction, so that we can update the monaco integration at a later timeCodeEditornow saves theyaml.configurepointer and will always update monaco YAML definitions on mountconsole_swagger_refreshwindow event whenever swagger refreshes, andCodeEditorwill listen to it on mount, callingyaml.configurewhenever there is a change to the swagger definitions (which refresh every 5 minutes)