CNV-69165: Use preserveSession flag for VNC connections - part 2 #3229
Conversation
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds HTTP/HTTPS constants, implements abnormal VNC disconnect detection (WS close code 1006) with an HTTP validation fallback to determine if a VNC session is in use, and replaces a no-op session check with a real Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (5)**/*.tsx📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
**/*.{ts,tsx,js,jsx,scss}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
**/*.ts📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-11-28T11:04:05.062ZApplied to files:
📚 Learning: 2025-11-28T11:04:05.062ZApplied to files:
🧬 Code graph analysis (1)src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rszwajko
changed the title
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
rszwajko
force-pushed
the
preserveSessionVncErrorHandling
branch
from
a5804d6 to
c823143
Compare
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
rszwajko
requested review from
batyana,
galkremer1,
metalice,
pcbailey and
yfrimanm
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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: 1
🧹 Nitpick comments (2)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (2)
42-43: Add explicit type annotations tobuildUrl.Per coding guidelines, functions should have explicit return types and avoid implicit
anytypes. The parameter object lacks type definition.Apply this diff:
-const buildUrl = ({ hostname, path, port, preserveSession, protocol }): string => +type BuildUrlParams = { + hostname: string; + path: string; + port: string; + preserveSession: boolean; + protocol: string; +}; + +const buildUrl = ({ hostname, path, port, preserveSession, protocol }: BuildUrlParams): string => `${protocol}://${hostname}:${port}${path}?preserveSession=${Boolean(preserveSession).toString()}`;
170-184: Add dependency array touseEffect.Per coding guidelines, always specify dependencies in
useEffecthooks. The current implementation runs on every render, relying on therfbRef.currentguard. This should use an empty array[]to run only once on mount.Apply this diff:
useEffect(() => { if (!rfbRef.current) { connect(); setVncState((prev) => ({ actions: { // keep the methods bound during connect() ...prev.actions, // add stable methods (semi-constant) connect, disconnect, }, })); } return () => disconnect(); - }); + }, []);Note: If
connectanddisconnectare intentionally meant to be updated on re-renders, include them in the dependency array:[connect, disconnect].
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/components/Consoles/components/utils/ConsoleConsts.ts(1 hunks)src/utils/components/Consoles/components/vnc-console/VncConsole.tsx(3 hunks)src/utils/components/Consoles/components/vnc-console/utils/util.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities.
Files:
src/utils/components/Consoles/components/utils/ConsoleConsts.tssrc/utils/components/Consoles/components/vnc-console/utils/util.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic (e.g., API calls, data transformation, form handling).
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized (e.g., usefetchUserDatainstead ofgetData).
Keep functions short and focused on one action. Apply Red → Green → Refactor: write a failing solution, implement a working solution, then refactor for readability and performance.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid using theanytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript to infer them from the implementation.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Keep global/store state minimal and straightforward. Obtain approval in the PR before adding new values to prevent bloating.
Files:
src/utils/components/Consoles/components/utils/ConsoleConsts.tssrc/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx,js,jsx}: Avoid comments whenever possible; write self-explanatory code. Use comments sparingly for unusual values or decisions to explain the rationale.
Avoid circular dependencies. Use index files cautiously to prevent situations that lead to circular imports.
Use Husky to run linters before commits and lint-staged to make it configurable. Do not skip linting.
Use Cypress or Playwright for end-to-end (E2E) testing to ensure comprehensive UI coverage.
Files:
src/utils/components/Consoles/components/utils/ConsoleConsts.tssrc/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.{ts,tsx,js,jsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use ESLint (with React and TypeScript plugins) and Prettier for consistent formatting and linting.
Files:
src/utils/components/Consoles/components/utils/ConsoleConsts.tssrc/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components. One component per file with no nested components.
Use PascalCase for all component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Extract as much logic as possible from components into custom hooks or utility files to improve testability and avoid bloated components.
Use default exports for all components.
Keep component files under 150 lines whenever possible.
Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Lazy load components withReact.lazyandSuspensefor performance optimization.
Always specify dependencies inuseEffecthooks to avoid unnecessary re-renders or missed updates. Pass an empty array[]to run the effect only once if no dependencies are required.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-28T11:04:05.062Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-11-28T11:04:05.062Z
Learning: Applies to **/*.{ts,tsx} : Define constants in utility files with uppercase and underscore-separated naming (e.g., `API_URL`).
Applied to files:
src/utils/components/Consoles/components/utils/ConsoleConsts.ts
📚 Learning: 2025-11-28T11:04:05.062Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-11-28T11:04:05.062Z
Learning: Applies to **/*.{ts,tsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Applied to files:
src/utils/components/Consoles/components/utils/ConsoleConsts.ts
🧬 Code graph analysis (1)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (3)
src/utils/components/Consoles/utils/utils.ts (1)
isConnectionEncrypted(1-1)src/utils/components/Consoles/utils/constants.ts (2)
SECURE(1-1)INSECURE(2-2)src/utils/components/Consoles/components/utils/ConsoleConsts.ts (4)
WSS(18-18)WS(17-17)HTTPS(20-20)HTTP(19-19)
🔇 Additional comments (4)
src/utils/components/Consoles/components/utils/ConsoleConsts.ts (1)
19-20: LGTM!The HTTP and HTTPS constants follow the uppercase naming convention and are logically grouped with the existing WS/WSS protocol constants. Based on learnings, this aligns with the coding standard for defining constants in utility files.
src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
13-17: LGTM with minor observation.The implementation correctly checks for the VNC-in-use error message. The optional chaining on
error?.message?is appropriate for null-safety.Note: The
?.onincludes(i.e.,includes?.()) is technically unnecessary sinceString.prototype.includesis always a function whenmessageis a string. However, this doesn't affect correctness.src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (2)
102-107: LGTM - proper event coordination.The disconnect listener correctly checks
abnormalDisconnectto avoid duplicate cleanup when the abnormal path handles cleanup separately via the HTTP check flow.
124-134: LGTM - HTTP validation flow for abnormal disconnects.The logic correctly uses an HTTP request to differentiate between a true disconnect and a "VNC session in use" scenario. The flow handles both success (not in use) and failure (check error message) cases appropriately.
rszwajko
force-pushed
the
preserveSessionVncErrorHandling
branch
from
c823143 to
b6aaa6f
Compare
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
13-17: Improve typing and simplifyisSessionAlreadyInUseimplementationRight now the function is typed as
(error: Error)but is called from acatchwhere the value is oftenunknownand you’re defensively using optional chaining. It’s safer and clearer to acceptunknownand narrow toErrorexplicitly, which also avoids relying on optional chaining here.Suggested change:
-const VNC_IN_USE_ERROR_TEXT = 'Active VNC connection. Request denied.'; - -export const isSessionAlreadyInUse = (error: Error): boolean => { - return error?.message?.includes?.(VNC_IN_USE_ERROR_TEXT) ?? false; -}; +const VNC_IN_USE_ERROR_TEXT = 'Active VNC connection. Request denied.'; + +export const isSessionAlreadyInUse = (error: unknown): boolean => + error instanceof Error && error.message.includes(VNC_IN_USE_ERROR_TEXT);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/components/Consoles/components/utils/ConsoleConsts.ts(1 hunks)src/utils/components/Consoles/components/vnc-console/VncConsole.tsx(3 hunks)src/utils/components/Consoles/components/vnc-console/utils/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/components/Consoles/components/utils/ConsoleConsts.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components. One component per file with no nested components.
Use PascalCase for all component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Extract as much logic as possible from components into custom hooks or utility files to improve testability and avoid bloated components.
Use default exports for all components.
Keep component files under 150 lines whenever possible.
Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Lazy load components withReact.lazyandSuspensefor performance optimization.
Always specify dependencies inuseEffecthooks to avoid unnecessary re-renders or missed updates. Pass an empty array[]to run the effect only once if no dependencies are required.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic (e.g., API calls, data transformation, form handling).
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized (e.g., usefetchUserDatainstead ofgetData).
Keep functions short and focused on one action. Apply Red → Green → Refactor: write a failing solution, implement a working solution, then refactor for readability and performance.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid using theanytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript to infer them from the implementation.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Keep global/store state minimal and straightforward. Obtain approval in the PR before adding new values to prevent bloating.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsxsrc/utils/components/Consoles/components/vnc-console/utils/util.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx,js,jsx}: Avoid comments whenever possible; write self-explanatory code. Use comments sparingly for unusual values or decisions to explain the rationale.
Avoid circular dependencies. Use index files cautiously to prevent situations that lead to circular imports.
Use Husky to run linters before commits and lint-staged to make it configurable. Do not skip linting.
Use Cypress or Playwright for end-to-end (E2E) testing to ensure comprehensive UI coverage.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsxsrc/utils/components/Consoles/components/vnc-console/utils/util.ts
**/*.{ts,tsx,js,jsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use ESLint (with React and TypeScript plugins) and Prettier for consistent formatting and linting.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsxsrc/utils/components/Consoles/components/vnc-console/utils/util.ts
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities.
Files:
src/utils/components/Consoles/components/vnc-console/utils/util.ts
🧠 Learnings (1)
📚 Learning: 2025-11-28T11:04:05.062Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-11-28T11:04:05.062Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Avoid comments whenever possible; write self-explanatory code. Use comments sparingly for unusual values or decisions to explain the rationale.
Applied to files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx
🧬 Code graph analysis (1)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (4)
src/utils/components/Consoles/utils/utils.ts (1)
isConnectionEncrypted(1-1)src/utils/components/Consoles/utils/constants.ts (2)
SECURE(1-1)INSECURE(2-2)src/utils/components/Consoles/components/utils/ConsoleConsts.ts (4)
WSS(18-18)WS(17-17)HTTPS(20-20)HTTP(19-19)src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
isSessionAlreadyInUse(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: i18n
- GitHub Check: unit-test
rszwajko
force-pushed
the
preserveSessionVncErrorHandling
branch
from
b6aaa6f to
3ceb0ba
Compare
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (1)
185-199: Add dependency array touseEffectto prevent running on every render.The effect currently has no dependency array, which causes it to run after every render and the cleanup (
disconnect()) to run before every re-render. This is inefficient and violates the coding guidelines: "Always specify dependencies inuseEffecthooks to avoid unnecessary re-renders or missed updates. Pass an empty array[]to run the effect only once if no dependencies are required."Since the comment states "auto-connect only on first load" and the logic is guarded by
if (!rfbRef.current), the intent is mount/unmount-only behavior.Apply this diff to fix the dependency array:
useEffect(() => { if (!rfbRef.current) { connect(); setVncState((prev) => ({ actions: { ...prev.actions, connect, disconnect, }, })); } return () => disconnect(); - }); + }, [connect, disconnect, setVncState]);Note: Since
connect,disconnect, andsetVncStateare all memoized withuseCallback, they should be stable and this dependency array will effectively run once on mount (plus cleanup on unmount).As per coding guidelines: explicit dependencies in useEffect hooks.
♻️ Duplicate comments (1)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (1)
42-55: FixbuildUrltyping forpreserveSessionparameter.The
preserveSessionparameter is typed asstring(line 52) but all call sites (lines 108, 144) pass a boolean value from theconnectfunction's boolean parameter (line 94:preserveSession = true). This type mismatch will fail strict TypeScript compilation and obscures intent.Apply this diff to fix the type signature:
const buildUrl = ({ hostname, path, port, preserveSession, protocol, }: { hostname: string; path: string; port: string; - preserveSession: string; + preserveSession: boolean; protocol: string; }): string => - `${protocol}://${hostname}:${port}${path}?preserveSession=${Boolean(preserveSession).toString()}`; + `${protocol}://${hostname}:${port}${path}?preserveSession=${preserveSession.toString()}`;
🧹 Nitpick comments (1)
src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
13-13: Consider exportingVNC_IN_USE_ERROR_TEXTfor testability.The constant is currently not exported, which limits the ability to write tests that verify the exact error message being checked. Exporting it would improve testability without changing runtime behavior.
Apply this diff if you want to export the constant:
-const VNC_IN_USE_ERROR_TEXT = 'Active VNC connection. Request denied.'; +export const VNC_IN_USE_ERROR_TEXT = 'Active VNC connection. Request denied.';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/components/Consoles/components/utils/ConsoleConsts.ts(1 hunks)src/utils/components/Consoles/components/vnc-console/VncConsole.tsx(3 hunks)src/utils/components/Consoles/components/vnc-console/utils/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/components/Consoles/components/utils/ConsoleConsts.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities.
Files:
src/utils/components/Consoles/components/vnc-console/utils/util.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Hooks should contain only logic and side effects, not return JSX. Keep JSX in components while using hooks for extracting reusable or unit-testable logic (e.g., API calls, data transformation, form handling).
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized (e.g., usefetchUserDatainstead ofgetData).
Keep functions short and focused on one action. Apply Red → Green → Refactor: write a failing solution, implement a working solution, then refactor for readability and performance.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
Avoid using theanytype in TypeScript as it compromises type safety. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript to infer them from the implementation.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Keep global/store state minimal and straightforward. Obtain approval in the PR before adding new values to prevent bloating.
Files:
src/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx,js,jsx}: Avoid comments whenever possible; write self-explanatory code. Use comments sparingly for unusual values or decisions to explain the rationale.
Avoid circular dependencies. Use index files cautiously to prevent situations that lead to circular imports.
Use Husky to run linters before commits and lint-staged to make it configurable. Do not skip linting.
Use Cypress or Playwright for end-to-end (E2E) testing to ensure comprehensive UI coverage.
Files:
src/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.{ts,tsx,js,jsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use ESLint (with React and TypeScript plugins) and Prettier for consistent formatting and linting.
Files:
src/utils/components/Consoles/components/vnc-console/utils/util.tssrc/utils/components/Consoles/components/vnc-console/VncConsole.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components. One component per file with no nested components.
Use PascalCase for all component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Extract as much logic as possible from components into custom hooks or utility files to improve testability and avoid bloated components.
Use default exports for all components.
Keep component files under 150 lines whenever possible.
Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Lazy load components withReact.lazyandSuspensefor performance optimization.
Always specify dependencies inuseEffecthooks to avoid unnecessary re-renders or missed updates. Pass an empty array[]to run the effect only once if no dependencies are required.
Files:
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx
🧬 Code graph analysis (1)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (3)
src/utils/components/Consoles/utils/utils.ts (1)
isConnectionEncrypted(1-1)src/utils/components/Consoles/utils/constants.ts (2)
SECURE(1-1)INSECURE(2-2)src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
isSessionAlreadyInUse(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build
🔇 Additional comments (3)
src/utils/components/Consoles/components/vnc-console/VncConsole.tsx (2)
93-120: LGTM: Session tracking and disconnect flow are correct.The session tracking logic (
isMySession()) and conditional disconnect handling (!abnormalDisconnect) work together correctly to prevent race conditions between concurrent connection attempts and to distinguish between normal and abnormal disconnects.
122-150: LGTM: Abnormal disconnect detection and HTTP fallback are correctly implemented.The noVNC private property workaround is well-documented, the abnormal disconnect detection (code 1006) is correct, and the HTTP fallback properly guards against session conflicts with
isMySession()checks in both success and error paths.src/utils/components/Consoles/components/vnc-console/utils/util.ts (1)
15-17: LGTM: Implementation is correct and defensive.The function correctly uses safe optional chaining to prevent runtime errors and explicitly returns a boolean. The logic correctly identifies the VNC-in-use error by checking for the specific error message text.
noVNC provides no direct access the error code returned by
socket.on("close") callback. As a workaround the original callback is
replaced by a custom proxy.
The backend returns the message introduced in #15267 for HTTP requests.
For the websocket requests we receive error 1006 (abnormal disconnect).
As a workaround an additional dedicated HTTP request is triggered to
retrieve the error.
Reference-Url: kubevirt/kubevirt#15267
Reference-Url: https://github.com/kubevirt/kubevirt/blob/46aa1b24982cd6fcfd37c950825c8d3acd3dbaf3/pkg/virt-handler/rest/console.go#L163
Reference-Url: https://github.com/novnc/noVNC/blob/d44f7e04fc456844836c7c5ac911d0f4e8dd06e6/core/rfb.js#L662
Reference-Url: https://datatracker.ietf.org/doc/html/rfc6455#section-7.4
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
rszwajko
force-pushed
the
preserveSessionVncErrorHandling
branch
from
3ceb0ba to
64c7412
Compare
|
@rszwajko: This pull request references CNV-69165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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: rszwajko, upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
openshift-merge-bot
bot
merged commit d5b9cbb
into
kubevirt-ui:main
3 participants
📝 Description
Follow up to #3085
Detect VNC-in-use error state
noVNC provides no direct access the error code returned by
socket.on("close") callback. As a workaround the original callback is
replaced by a custom proxy.
The backend returns the message introduced in #15267 for HTTP requests.
For the websocket requests we receive error 1006 (abnormal disconnect).
As a workaround an additional dedicated HTTP request is triggered to
retrieve the error.
Reference-Url: kubevirt/kubevirt#15267
Reference-Url: https://github.com/kubevirt/kubevirt/blob/46aa1b24982cd6fcfd37c950825c8d3acd3dbaf3/pkg/virt-handler/rest/console.go#L163
Reference-Url: https://github.com/novnc/noVNC/blob/d44f7e04fc456844836c7c5ac911d0f4e8dd06e6/core/rfb.js#L662
Reference-Url: https://datatracker.ietf.org/doc/html/rfc6455#section-7.4
Signed-off-by: Radoslaw Szwajkowski [email protected]
🎥 Demo
Screencast.From.2025-12-01.11-50-40.mp4
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.