-
Notifications
You must be signed in to change notification settings - Fork 228
refactor: Update terminal ref types to be non-nullable. #2292
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
refactor: Update terminal ref types to be non-nullable. #2292
Conversation
Signed-off-by: arpit529srivastava <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@Arpit529Srivastava: The following test failed, say
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. |
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 attempts to fix a build error by removing the | null union type from React ref types for terminal elements. However, this change introduces a type safety issue. The refs are initialized with null in the parent component (WecsDetailsPanel.tsx), and React's RefObject type correctly requires the current property to be nullable until it's attached to a DOM element.
Key Changes:
- Updated
terminalReftype in LogsTab fromReact.RefObject<HTMLDivElement | null>toReact.RefObject<HTMLDivElement> - Updated
execTerminalReftype in ExecTab fromReact.RefObject<HTMLDivElement | null>toReact.RefObject<HTMLDivElement>
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/components/wecs_details/tabs/LogsTab.tsx | Removed null union type from terminalRef prop type |
| frontend/src/components/wecs_details/tabs/ExecTab.tsx | Removed null union type from execTerminalRef prop type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| theme: string; | ||
| t: (key: string, options?: Record<string, unknown>) => string; | ||
| terminalRef: React.RefObject<HTMLDivElement | null>; | ||
| terminalRef: React.RefObject<HTMLDivElement>; |
Copilot
AI
Dec 20, 2025
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 ref is initialized as useRef<HTMLDivElement>(null) in WecsDetailsPanel.tsx (line 115), which means the current property can be null. Changing the type to React.RefObject<HTMLDivElement> creates a type mismatch because RefObject.current can be null or undefined until the ref is attached to a DOM element.
The code already handles this correctly with null checks (e.g., line 66: if (!terminalRef.current || ...)), so the type should remain as React.RefObject<HTMLDivElement | null> to accurately reflect that the ref can be null.
| terminalRef: React.RefObject<HTMLDivElement>; | |
| terminalRef: React.RefObject<HTMLDivElement | null>; |
| loadingContainers: boolean; | ||
| isTerminalMaximized: boolean; | ||
| execTerminalRef: React.RefObject<HTMLDivElement | null>; | ||
| execTerminalRef: React.RefObject<HTMLDivElement>; |
Copilot
AI
Dec 20, 2025
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 ref is initialized as useRef<HTMLDivElement>(null) in WecsDetailsPanel.tsx (line 116), which means the current property can be null. Changing the type to React.RefObject<HTMLDivElement> creates a type mismatch because RefObject.current can be null or undefined until the ref is attached to a DOM element.
The code already handles this correctly with null checks (e.g., lines 85, 100, 138: checking for !execTerminalRef.current), so the type should remain as React.RefObject<HTMLDivElement | null> to accurately reflect that the ref can be null.
| execTerminalRef: React.RefObject<HTMLDivElement>; | |
| execTerminalRef: React.RefObject<HTMLDivElement | null>; |
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.
yes understood made the change and closing the PR.
Description
fixed the build error in the frontend
Related Issue
Fixes #<issue_number>
Changes Made
Checklist
Please ensure the following before submitting your PR:
Screenshots or Logs (if applicable)
Additional Notes