fix(use-input): sync the inputValue with domRef.current.value when hovering the input #3481

jijiseong · 2024-07-16T10:14:13Z

Closes #3024
Closes #3436

📝 Description

Add a brief description

Problem

setting the input's value via its ref
(but not updated inputValue)
hovering the input
isHovered is updated, so re-render occurs.
<Input/> uses the inputValue that is not updated to new value.

Solve

when hover start, setInputValues(domRef.current.value)

⛳️ Current behavior (updates)

Please describe the current behavior that you are modifying

2024-07-16.7.37.30.mov

🚀 New behavior

Please describe the behavior or changes this PR adds

2024-07-16.6.45.31.mov

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

New Features
- Added a hover functionality that sets the input value when hovering starts.
Tests
- Added test cases to ensure input value syncs correctly when hovering and clicking on the input element in the Input component when used with React Hook Form.

…input

changeset-bot · 2024-07-16T10:14:17Z

🦋 Changeset detected

Latest commit: f6afb96

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@nextui-org/input	Patch
@nextui-org/autocomplete	Patch
@nextui-org/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-07-16T10:14:18Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 17, 2024 5:26am

vercel · 2024-07-16T10:14:19Z

@jijiseong is attempting to deploy a commit to the NextUI Inc Team on Vercel.

A member of the Team first needs to authorize it.

coderabbitai · 2024-07-16T10:14:25Z

Walkthrough

The changes introduce a new onHoverStart callback within the useHover hook in the useInput function, ensuring the input value syncs with the current DOM reference when hovering starts. In addition, new test cases have been added to verify that the input value updates correctly when interacting with the input element, particularly when used with React Hook Form.

Changes

File Path	Summary of Changes
`packages/components/input/src/use-input.ts`	Added an `onHoverStart` callback within the `useHover` hook to set the input value based on DOM ref.
`packages/components/input/__tests__/input.test.tsx`	Added test cases to ensure `ref.current.value` syncs with the input's value after hover and click.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant InputComponent
    participant useInput
    participant useHover
    participant DOMRef

    User ->> InputComponent: Hover/Click
    InputComponent ->> useInput: Trigger onHoverStart/onClick
    useInput ->> useHover: Handle Hover/Click
    useHover ->> DOMRef: Fetch current value
    DOMRef -->> useHover: Current value
    useHover -->> useInput: Set value based on ref
    useInput -->> InputComponent: Update input value
    InputComponent -->> User: Display updated value

Assessment against linked issues

Objective	Addressed	Explanation
Ensure the input value updates correctly via its ref when used with React Hook Form. (#3024, #3436)	✅
Verify that the input value persists after interactions such as hovering and clicking. (#3436)	✅

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b762141 and a0231b0.

Files selected for processing (1)

packages/components/input/src/use-input.ts (1 hunks)

Additional comments not posted (1)

packages/components/input/src/use-input.ts (1)

208-214: Ensure synchronization of input value on hover

The addition of the onHoverStart callback within the useHover hook is a targeted solution to the synchronization issue described. By setting the input value directly from the DOM when hovering starts, it ensures that any changes made directly to the DOM are reflected in the component's state.

However, this approach might introduce performance implications if setInputValue triggers more re-renders than necessary. It's also important to ensure that this behavior does not interfere with other interactions or expected behaviors, especially with form libraries like react-hook-form.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a0231b0 and f5d1773.

Files selected for processing (1)

.changeset/angry-icons-jump.md (1 hunks)

Additional comments not posted (2)

.changeset/angry-icons-jump.md (2)

2-2: Versioning is correctly labeled as a patch.

The changes described are a bug fix, which correctly warrants a patch version increment according to semantic versioning.

5-5: Clear and concise description of the changes.

The description succinctly explains the bug fix, making it clear what the patch addresses.

wingkwong

please include the test as well

coderabbitai

Actionable comments posted: 2

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f5d1773 and 9313561.

Files selected for processing (1)

packages/components/input/tests/input.test.tsx (1 hunks)

Additional context used

Biome

packages/components/input/__tests__/input.test.tsx

[error] 189-189: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/components/input/__tests__/input.test.tsx

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9313561 and 528d579.

Files selected for processing (2)

.changeset/angry-icons-jump.md (1 hunks)
packages/components/input/tests/input.test.tsx (1 hunks)

Files skipped from review as they are similar to previous changes (2)

.changeset/angry-icons-jump.md
packages/components/input/tests/input.test.tsx

wingkwong · 2024-07-17T05:50:07Z

@jijiseong is it ready for re-review?

jijiseong · 2024-07-17T05:50:54Z

@jijiseong is it ready for re-review?

yes!

wingkwong

what about finding a way to sync the ref value to inputValue instead just handling the hover case only? I remember I've seen another issue also caused by this reason.

jijiseong · 2024-07-17T15:33:20Z

what about finding a way to sync the ref value to inputValue instead just handling the hover case only? I remember I've seen another issue also caused by this reason.

Thankyou for your feedback.

I tried to find another way, but I think it is the best way,
because the input component is not generally rerendered when hovered.
But NextUI Input component is rerendered.
Therefore, It needs to set the ref.current.value to inputValue using onHoverStart before the isHover is updated.

I'll look for another way, If you tell me another issue.
I don't know another issue caused by this reason.

AnthonyPaulO · 2024-07-17T20:34:46Z

what about finding a way to sync the ref value to inputValue instead just handling the hover case only? I remember I've seen another issue also caused by this reason.

Thankyou for your feedback.

I tried to find another way, but I think it is the best way, because the input component is not generally rerendered when hovered. But NextUI Input component is rerendered. Therefore, It needs to set the ref.current.value to inputValue using onHoverStart before the isHover is updated.

I'll look for another way, If you tell me another issue. I don't know another issue caused by this reason.

I guess a couple of things to check for are, if the ref value is set, instead of mouse hovering you either 1) have the parent component re-render, and see if the component re-renders correctly, or 2) tab into the component or finger-click into it on those touch-screen tablets because they don't trigger the hover event.

wingkwong · 2024-07-18T02:36:08Z

@jijiseong you may take a look at #2966. Even it's assigned, but i don't think he's working on it. I think if we fix the sync issue, then the hover issue will be resolved as well.

jijiseong · 2024-07-18T07:37:40Z

@jijiseong you may take a look at #2966. Even it's assigned, but i don't think he's working on it. I think if we fix the sync issue, then the hover issue will be resolved as well.

I don't reproduce this issue.

wingkwong · 2024-07-18T07:51:34Z

@jijiseong I'll review hover approach then.

jijiseong · 2024-07-18T07:54:06Z

@jijiseong I'll review hover approach then.

Thankyou I also handled the case for tabbing

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 528d579 and 4c41c12.

Files selected for processing (2)

packages/components/input/tests/input.test.tsx (2 hunks)
packages/components/input/src/use-input.ts (3 hunks)

Files skipped from review as they are similar to previous changes (2)

packages/components/input/tests/input.test.tsx
packages/components/input/src/use-input.ts

jijiseong · 2024-07-18T08:18:08Z

I am ready to re-review

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4c41c12 and cd9d9db.

Files selected for processing (2)

.changeset/angry-icons-jump.md (1 hunks)
packages/components/input/tests/input.test.tsx (1 hunks)

Files skipped from review as they are similar to previous changes (2)

.changeset/angry-icons-jump.md
packages/components/input/tests/input.test.tsx

packages/components/input/__tests__/input.test.tsx

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cd9d9db and 68050a8.

Files selected for processing (1)

packages/components/input/tests/input.test.tsx (2 hunks)

Files skipped from review as they are similar to previous changes (1)

packages/components/input/tests/input.test.tsx

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)

packages/components/input/__tests__/input.test.tsx (2)
184-206: Improve test case readability and maintainability.

Consider using assertions instead of throwing errors for better readability and maintainability.
-  if (!ref.current) {
-    throw new Error("ref is null");
-  }
+  expect(ref.current).not.toBeNull();
209-230: Improve test case readability and maintainability.

Consider using assertions instead of throwing errors for better readability and maintainability.
-  if (!ref.current) {
-    throw new Error("ref is null");
-  }
+  expect(ref.current).not.toBeNull();

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 68050a8 and f6afb96.

Files selected for processing (1)

packages/components/input/tests/input.test.tsx (2 hunks)

AnthonyPaulO · 2024-07-22T20:43:11Z

I am ready to re-review

@jijiseong @wingkwong I think we should be looking at this fix from a preventative rather than a reactive perspective; that is, we should implement a fix that keeps the state synced instead of trying to figure out which cases will result in desync and trying to patch that up. In this particular case, the issue is that the ref assignment results in the ref value becoming out of sync with the internal state. What we need to do is make sure that the synchronization happens at the time the ref value assignment happens. This would involve intercepting the ref value setter (via Imperative Handle) and calling the setInputValue function within it in order to keep the state synced. Here is a quick implementation of this approach:

  //const domRef = useDOMRef<T>(ref); // comment out this line and replace with the code below

  const domRef = useRef<T>(null);

  let proxy: any = undefined;

  useImperativeHandle(
    ref,
    () => {
      if (proxy === undefined) {
        proxy = new Proxy(domRef.current!, {
          get(target, prop, receiver) {
            const value = target[prop];

            if (value instanceof Function) {
              return value.bind(target);
            }

            return value;
          },
          set(target, prop, value, receiver) {
            target[prop] = value;

            if (prop === "value") {
              setInputValue(value);
            }

            return true;
          },
        });
      }

      return proxy;
    },
    [domRef.current],
  );

The above code makes sure that as soon as the ref value assignment occurs, the setter is intercepted and a call to setInputValue occurs which will sync the state with the ref value immediately. There's no edge cases to consider such as hover, tabbing, etc... because it's synced right away.

AnthonyPaulO · 2024-07-23T03:29:16Z

I went ahead and put in a pull-request with the fix I mentioned above: #3533

wingkwong · 2024-07-23T03:36:47Z

@AnthonyPaulO Thanks. I also prefer that. I'll take a look at your solution tonight.

wingkwong · 2024-08-11T13:15:40Z

Closing this PR. Will handle by #3533

fix: sync the inputValue with domRef.current.value when hovering the …

a0231b0

…input

jijiseong requested a review from jrgarciadev as a code owner July 16, 2024 10:14

vercel bot deployed to Preview – nextui-storybook-v2 July 16, 2024 10:16 View deployment

coderabbitai bot reviewed Jul 16, 2024

View reviewed changes

chore: add changeset

f5d1773

coderabbitai bot reviewed Jul 16, 2024

View reviewed changes

vercel bot deployed to Preview – nextui-storybook-v2 July 16, 2024 10:24 View deployment

jijiseong changed the title ~~fix: sync the inputValue with domRef.current.value when hovering the input~~ fix(use-input): sync the inputValue with domRef.current.value when hovering the input Jul 16, 2024

wingkwong self-assigned this Jul 16, 2024

wingkwong requested changes Jul 16, 2024

View reviewed changes

wingkwong added the 🚧 Status: Changes Requested label Jul 16, 2024

wingkwong assigned jijiseong and unassigned wingkwong Jul 16, 2024

test: add test code

9313561

jijiseong requested a review from wingkwong July 16, 2024 12:14

coderabbitai bot reviewed Jul 16, 2024

View reviewed changes

packages/components/input/__tests__/input.test.tsx Outdated Show resolved Hide resolved

packages/components/input/__tests__/input.test.tsx Outdated Show resolved Hide resolved

vercel bot deployed to Preview – nextui-storybook-v2 July 16, 2024 12:16 View deployment

jijiseong added 2 commits July 16, 2024 21:17

test: refactor test

75365ab

test: refactor test

9121dc4

vercel bot deployed to Preview – nextui-storybook-v2 July 16, 2024 12:23 View deployment

chore: add issue link in changeset

528d579

coderabbitai bot reviewed Jul 17, 2024

View reviewed changes

vercel bot deployed to Preview – nextui-storybook-v2 July 17, 2024 05:26 View deployment

wingkwong removed the 🚧 Status: Changes Requested label Jul 17, 2024

wingkwong reviewed Jul 17, 2024

View reviewed changes

wingkwong assigned jijiseong and unassigned wingkwong Jul 17, 2024

wingkwong assigned wingkwong and unassigned jijiseong Jul 18, 2024

jijiseong added 2 commits July 18, 2024 16:52

refactor: handled the case for tabbing out

cf54ff9

test: refactor hover -> click

4c41c12

coderabbitai bot reviewed Jul 18, 2024

View reviewed changes

jijiseong added 2 commits July 18, 2024 17:15

chore: fix typo

b8f8ecf

chore: modify change set

cd9d9db

jijiseong requested a review from wingkwong July 18, 2024 08:18

coderabbitai bot reviewed Jul 18, 2024

View reviewed changes

wingkwong requested changes Jul 18, 2024

View reviewed changes

packages/components/input/__tests__/input.test.tsx Outdated Show resolved Hide resolved

packages/components/input/__tests__/input.test.tsx Outdated Show resolved Hide resolved

packages/components/input/__tests__/input.test.tsx Show resolved Hide resolved

test: refactor test

68050a8

coderabbitai bot reviewed Jul 18, 2024

View reviewed changes

test: add test for hovering case

f6afb96

coderabbitai bot reviewed Jul 18, 2024

View reviewed changes

jijiseong requested a review from wingkwong July 18, 2024 10:16

wingkwong closed this Aug 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(use-input): sync the inputValue with domRef.current.value when hovering the input #3481

fix(use-input): sync the inputValue with domRef.current.value when hovering the input #3481

jijiseong commented Jul 16, 2024 •

edited by coderabbitai bot

Loading

changeset-bot bot commented Jul 16, 2024 •

edited

Loading

vercel bot commented Jul 16, 2024 •

edited

Loading

vercel bot commented Jul 16, 2024

coderabbitai bot commented Jul 16, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

coderabbitai bot left a comment

wingkwong left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

wingkwong commented Jul 17, 2024

jijiseong commented Jul 17, 2024

wingkwong left a comment

jijiseong commented Jul 17, 2024 •

edited

Loading

AnthonyPaulO commented Jul 17, 2024

wingkwong commented Jul 18, 2024

jijiseong commented Jul 18, 2024

wingkwong commented Jul 18, 2024

jijiseong commented Jul 18, 2024 •

edited

Loading

coderabbitai bot left a comment

jijiseong commented Jul 18, 2024

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

AnthonyPaulO commented Jul 22, 2024 •

edited

Loading

AnthonyPaulO commented Jul 23, 2024

wingkwong commented Jul 23, 2024

wingkwong commented Aug 11, 2024

fix(use-input): sync the inputValue with domRef.current.value when hovering the input #3481

fix(use-input): sync the inputValue with domRef.current.value when hovering the input #3481

Conversation

jijiseong commented Jul 16, 2024 • edited by coderabbitai bot Loading

📝 Description

Problem

Solve

⛳️ Current behavior (updates)

🚀 New behavior

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

changeset-bot bot commented Jul 16, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Jul 16, 2024 • edited Loading

vercel bot commented Jul 16, 2024

coderabbitai bot commented Jul 16, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

wingkwong left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

wingkwong commented Jul 17, 2024

jijiseong commented Jul 17, 2024

wingkwong left a comment

Choose a reason for hiding this comment

jijiseong commented Jul 17, 2024 • edited Loading

AnthonyPaulO commented Jul 17, 2024

wingkwong commented Jul 18, 2024

jijiseong commented Jul 18, 2024

wingkwong commented Jul 18, 2024

jijiseong commented Jul 18, 2024 • edited Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

jijiseong commented Jul 18, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

AnthonyPaulO commented Jul 22, 2024 • edited Loading

AnthonyPaulO commented Jul 23, 2024

wingkwong commented Jul 23, 2024

wingkwong commented Aug 11, 2024

jijiseong commented Jul 16, 2024 •

edited by coderabbitai bot

Loading

changeset-bot bot commented Jul 16, 2024 •

edited

Loading

vercel bot commented Jul 16, 2024 •

edited

Loading

coderabbitai bot commented Jul 16, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

jijiseong commented Jul 17, 2024 •

edited

Loading

jijiseong commented Jul 18, 2024 •

edited

Loading

AnthonyPaulO commented Jul 22, 2024 •

edited

Loading