Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RHSTOR-7003: Disaster recovery UI for Kubevirt VM - Discovered(Standalone) #1889

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

RHSTOR-7003: Disaster recovery UI for Kubevirt VM - Discovered(Standalone) #1889

wants to merge 1 commit into from

Conversation

GowthamShanmugam
Copy link
Contributor

@GowthamShanmugam GowthamShanmugam commented Mar 5, 2025 •
edited
Loading

On top of: #1856

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid jira ticket of any type label Mar 5, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 5, 2025 •
edited by openshift-ci bot
Loading

@GowthamShanmugam: This pull request references RHSTOR-7003 which is a valid jira issue.

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.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Mar 5, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 7, 2025 •
edited by openshift-ci bot
Loading

@GowthamShanmugam: This pull request references RHSTOR-7003 which is a valid jira issue.

In response to this:

On top of: #1856

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.

@GowthamShanmugam GowthamShanmugam mentioned this pull request Mar 11, 2025
Open
@GowthamShanmugam GowthamShanmugam changed the title RHSTOR-7003: Disaster recovery UI for Kubevirt VM - Discovered(Standalone) - WIP RHSTOR-7003: Disaster recovery UI for Kubevirt VM - Discovered(Standalone) Mar 12, 2025
@GowthamShanmugam
Copy link
Contributor Author

Screenshot 2025-03-12 at 2 36 51 PM Screenshot 2025-03-12 at 2 36 54 PM Screenshot 2025-03-12 at 2 36 58 PM

@GowthamShanmugam GowthamShanmugam force-pushed the RHSTOR-7003 branch 2 times, most recently from 7250785 to ef83318 Compare March 14, 2025 08:35
@GowthamShanmugam GowthamShanmugam force-pushed the RHSTOR-7003 branch 4 times, most recently from f2ecf9c to 0226a85 Compare March 14, 2025 11:38
SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
packages/shared/src/input-with-requirements/nameInputWithValidation.tsx Outdated
Comment on lines 66 to 68
React.useEffect(() => {
onChange(isValid ? newName : '');
}, [isValid, name, onChange, newName]);
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

textInputProps has a onChange prop, do we need useEffect explicitly ?? Can we try using textInputProps's onChange instead.

Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use of this useEffect ?? Why do we want to automatically reset the name if it's invalid ??

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design of TextInputWithFieldRequirements relies on receiving the TextInput value from the watch function, which is why useEffect is used throughout. It resets to an empty value intentionally to prevent the user from moving to the next wizard step, as the validation requires name !== ''.

However, this reset doesn’t clear the already typed text in the input box(its a separate state) — it only returns an empty value to the caller.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got your point that other places are using useEffect as well, but they are just misusing the hook... a generic component should not make same mistake again... instead of useEffect u can utilise the onChange prop...
check: https://github.com/red-hat-storage/odf-console/blob/master/packages/odf/components/attach-storage-storagesystem/storageclass-form.tsx#L252-L257

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, i will change to this and I will change the onChange function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this logic. The val is compared with isValid before the new val is fully validated, meaning isValid may still hold an outdated result. Only after the state updates does isValid refresh, but by then, the state value is already set incorrectly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack !!

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
packages/shared/src/input-with-requirements/nameInputWithValidation.tsx Outdated
Comment on lines 66 to 68
React.useEffect(() => {
onChange(isValid ? newName : '');
}, [isValid, name, onChange, newName]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't seems like something which should be part of shared component...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will work for all, #1889 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name.input.validation.mp4

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
packages/shared/src/input-with-requirements/nameInputWithValidation.tsx Outdated
Comment on lines 66 to 68
React.useEffect(() => {
onChange(isValid ? newName : '');
}, [isValid, name, onChange, newName]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure ref of onChange is stable if we are adding it to the dep list of the hook...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added useRef for onChange, please check

@GowthamShanmugam GowthamShanmugam force-pushed the RHSTOR-7003 branch 2 times, most recently from d46ed8b to 62b4753 Compare March 19, 2025 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

Assignees
No one assigned
Labels
approved jira/valid-reference Indicates that this PR references a valid jira ticket of any type
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GowthamShanmugam @openshift-ci-robot @SanjalKatiyar @openshift-merge-robot