OCPBUGS-77912: Fix TypeError in OLS code import to console#16119
OCPBUGS-77912: Fix TypeError in OLS code import to console#16119rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
The "Import to console" action from OpenShift Lightspeed was failing with "TypeError: Cannot use 'in' operator to search for 'editor' in undefined" when redirecting to the YAML import page. The issue occurred in getEditor() where the 'in' operator was used on monacoRef.current before checking if it was defined. This created a race condition where the OLS code import useEffect would run when editorMounted became true, but monacoRef.current was still undefined. The fix adds a check to ensure monacoRef.current exists before using the 'in' operator, preventing the TypeError and allowing the OLS code to be properly injected into the editor. Added regression tests to verify the fix handles all cases correctly. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77912, which is invalid:
Comment 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. |
|
/assign @logonoff |
openshift-ci
bot
added
the
approved
openshift-ci-robot
added
jira/valid-bug
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77912, 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: 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. |
📝 WalkthroughWalkthroughThis change adds comprehensive unit test coverage for the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/public/components/__tests__/edit-yaml.spec.tsx (2)
5-48: Good regression test coverage for the bug fix.The three scenarios are well-chosen and the comments documenting OCPBUGS-77912 will help future maintainers understand why these tests exist.
One consideration: the
getEditorlogic is duplicated in each test case (lines 13-14, 29-30, 43-44). SincegetEditoris a private function inside the component, this copy-paste approach is understandable, but it does mean these tests won't catch if someone accidentally changes the component's implementation without updating tests.♻️ Optional: Extract duplicated getEditor logic
+// Mirrors the getEditor function from edit-yaml.tsx +// If the implementation changes there, update this helper accordingly +const createGetEditor = (monacoRef: React.RefObject<CodeEditorRef | undefined>) => () => + monacoRef?.current && 'editor' in monacoRef.current ? monacoRef.current.editor : undefined; + describe('EditYAML: getEditor function', () => { it('should handle undefined monacoRef.current without throwing TypeError', () => { // This test verifies the fix for OCPBUGS-77912 // The bug occurred when monacoRef.current was undefined and the 'in' operator was used const { result } = renderHook(() => useRef<CodeEditorRef>()); const monacoRef = result.current; - // This simulates the getEditor function from edit-yaml.tsx - const getEditor = () => - monacoRef?.current && 'editor' in monacoRef.current ? monacoRef.current.editor : undefined; + const getEditor = createGetEditor(monacoRef); // Before the fix, this would throw: "TypeError: Cannot use 'in' operator to search for 'editor' in undefined" // After the fix, it should return undefined gracefully expect(() => getEditor()).not.toThrow(); expect(getEditor()).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/__tests__/edit-yaml.spec.tsx` around lines 5 - 48, Tests duplicate the inline getEditor logic across three cases which makes maintenance brittle; extract a single helper function (e.g., getEditorFromRef) inside the describe block that accepts the monaco ref (monacoRef) and returns monacoRef?.current && 'editor' in monacoRef.current ? monacoRef.current.editor : undefined, then replace the three inline getEditor lambdas with calls to this helper so all tests use the same logic (update the uses in the three it blocks referencing monacoRef and the mockEditor).
39-41: Minor: Type coercion withas anyreduces type safety.The
as anycast bypasses TypeScript's type checking. Since this is a test file and you're intentionally creating a partial mock, this is acceptable, but a slightly safer approach would be to use a more precise cast.♻️ Optional: Use Partial type for mock
const mockEditor = { getValue: jest.fn(), setValue: jest.fn() }; // Set monacoRef.current to an object with an 'editor' property - monacoRef.current = { editor: mockEditor } as any; + monacoRef.current = { editor: mockEditor } as unknown as CodeEditorRef;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/__tests__/edit-yaml.spec.tsx` around lines 39 - 41, The test currently uses a blanket cast "as any" when assigning monacoRef.current which weakens type safety; replace the any cast with a more precise partial type cast (e.g., use Partial<typeof monacoRef.current> or Partial<MonacoRefType> depending on your monacoRef declaration) when assigning { editor: mockEditor } to monacoRef.current so the mockEditor and the editor property are type-checked while still allowing a partial mock; update the assignment of monacoRef.current and/or import or reference the appropriate monacoRef type to perform the Partial<> cast instead of using as any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/public/components/__tests__/edit-yaml.spec.tsx`:
- Around line 5-48: Tests duplicate the inline getEditor logic across three
cases which makes maintenance brittle; extract a single helper function (e.g.,
getEditorFromRef) inside the describe block that accepts the monaco ref
(monacoRef) and returns monacoRef?.current && 'editor' in monacoRef.current ?
monacoRef.current.editor : undefined, then replace the three inline getEditor
lambdas with calls to this helper so all tests use the same logic (update the
uses in the three it blocks referencing monacoRef and the mockEditor).
- Around line 39-41: The test currently uses a blanket cast "as any" when
assigning monacoRef.current which weakens type safety; replace the any cast with
a more precise partial type cast (e.g., use Partial<typeof monacoRef.current> or
Partial<MonacoRefType> depending on your monacoRef declaration) when assigning {
editor: mockEditor } to monacoRef.current so the mockEditor and the editor
property are type-checked while still allowing a partial mock; update the
assignment of monacoRef.current and/or import or reference the appropriate
monacoRef type to perform the Partial<> cast instead of using as any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78b9db18-51a7-4bd7-9ebc-b6271fe6ce45
📒 Files selected for processing (2)
frontend/public/components/__tests__/edit-yaml.spec.tsxfrontend/public/components/edit-yaml.tsx
|
/jira refresh |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77912, 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto 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 |
|
/retest |
|
@rhamilto: all tests passed! 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. |
4 participants
Summary
Fixes the "Import to console" action from OpenShift Lightspeed that was failing with a TypeError on OpenShift 4.20+.
Bug: OCPBUGS-77912
Changes
getEditor()function in edit-yaml.tsxinoperator to prevent TypeError whenmonacoRef.currentis undefinedRoot Cause
When users clicked "Import to console" from OpenShift Lightspeed, the page redirected with
?ols=truequery parameter. A race condition occurred where:useEffectran wheneditorMountedbecametruemonacoRef.currentwas stillundefinedat that moment'editor' in monacoRef?.currentthrewTypeError: Cannot use 'in' operator to search for 'editor' in undefinedThe Fix
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests