fix: abnormal-width on text select #658

stealth-bombeer · 2023-04-23T10:52:21Z

Brief Information

This pull request is in the type of (more info about types):

Related issues (all available keywords):

fix [Bug] Abnormal width variation of OSS-GPT chat widget when selecting text in it. #624

Details

The react-chat-widget has bug in itself for it's resizing props, may be because it had event listener all over the container, so we don't use the inbuilt property of resizing instead we use jquery to select the container and allow it resize only when the left border of the container is dragged we watch for mutations in the useeffect when the conversation container becomes active

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA

Others

N.A

tyn1998 · 2023-04-23T10:57:53Z

Hi @stealth-bombeer, thank you for raising this PR. I will review it later when I have time so just take a break 😃.

Would you mind changing the "keyword" in the PR description?

wxharry

LGTM. Nice work! @stealth-bombeer

Is this also fix #620 or not? I could not reproduce the issue. So, we might need Lam to give it a test. @tyn1998

stealth-bombeer · 2023-04-24T15:15:48Z

LGTM. Nice work! @stealth-bombeer

Is this also fix #620 or not? I could not reproduce the issue. So, we might need Lam to give it a test. @tyn1998

No still #620 persists,once these changes are reviewed as they seems to be related I can start working on it 💯

wxharry · 2023-04-24T16:13:56Z

No still #620 persists,once these changes are reviewed as they seems to be related I can start working on it 💯

That would be great. Appreciate it!

Meanwhile, i am not sure if the react-chat-widget project is still under maintenance, but I think you could help them or other developers encountered the same issue by showing how you solve it in their community. It would be a good start point to embrace open source community. 😄

stealth-bombeer · 2023-04-25T03:27:42Z

No still #620 persists,once these changes are reviewed as they seems to be related I can start working on it 💯

That would be great. Appreciate it!

Meanwhile, i am not sure if the react-chat-widget project is still under maintenance, but I think you could help them or other developers encountered the same issue by showing how you solve it in their community. It would be a good start point to embrace open source community. 😄

Thanks @wxharry ,I think this would be a great start and will surely look after it 😄

tyn1998 · 2023-04-27T02:21:33Z

Share with you an interesting find:

if a PR target branch name is as same as a local branch name(like this case), then gh pr checkout 658 will substitue the local one (unexpectedly).

So we'd better always create a new branch on the base branch before working on it :)

stealth-bombeer · 2023-04-27T04:24:15Z

Share with you an interesting find:

if a PR target branch name is as same as a local branch name(like this case), then gh pr checkout 658 will substitue the local one (unexpectedly).

So we'd better always create a new branch on the base branch before working on it :)

Oops,i'll make the necessary changes and commit it again ....

tyn1998 · 2023-04-27T04:29:12Z

That is not a big deal, don't worry :)

BTW, as far as I know, you cannot change the PR branch name. That automatically closes a PR.

And I'm reviewing your code, I'll give you feedback soon.

tyn1998

Hi @stealth-bombeer, thanks for your PR. It works good, yet can still be improved :)

I commented serveral lines of code. Besides, I'm wondering if it is necessary to put the code in a MutationObserver callback?

tyn1998 · 2023-04-27T04:37:05Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

@@ -35,6 +35,8 @@ const View = ({ theme, currentRepo, currentDocsName }: Props): JSX.Element => {
  const [inited, setInited] = useState(false);
  const [settings, setSettings] = useState(new Settings());
  const [history, setHistory] = useState<[string, string]>(['', '']);
+  // Toggle to allow resizing of the widget
+  const [resizable, setResizable] = useState(true);


resizable doesn't need to be a React state. setResizable is never used.

Actually we can make the chat-widget always resizable so do not need an option to toggle it. Just make it as default, no if else.

The reason why this needs to be placed in mutation callback is we watch for the changes in rcw-widget-container as this should be fired only when the container is active

tyn1998 · 2023-04-27T04:38:20Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

+          let isDragging = false;
+          let initialMouseX: number;
+          let initialWidth: number;
+          if (conversationResizer.length) {


please use exists helper function to check if an element exsists instead.

tyn1998 · 2023-04-27T04:39:36Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

+
+      if ($('.rcw-conversation-resizer')) {
+        $('.rcw-conversation-resizer').off('mousedown');
+        $(document).off('mousemove mouseup');


Will .off remove all listeners (including GitHub's own listeners) for mousemove and mouseup?

Maybe you can move the listeners more accurately.

stealth-bombeer · 2023-04-28T03:50:51Z

hi @tyn1998 ,i've made all the changes as suggested by you the recent commit ensures that the exact same functions that were added as event listeners are removed rather than relying jquery's default behaviour and created a helper function to check validity of selector

tyn1998

Hi @stealth-bombeer, I tried a cleaner way to implement our own resizer. What do you think?

import React, { useState, useEffect, useRef } from 'react';
import $ from 'jquery';
import {
  Widget,
  addResponseMessage,
  deleteMessages,
  toggleMsgLoader,
  toggleInputDisabled,
} from 'react-chat-widget';

import { getAnswer } from './service';
import './rcw.scss';
import exists from '../../../../helpers/exists';
import { getMessageByLocale } from '../../../../utils/utils';
import Settings, { loadSettings } from '../../../../utils/settings';

interface Props {
  theme: 'light' | 'dark';
  currentRepo: string;
  currentDocsName: string | null;
}

const displayWelcome = (repoName: string, locale: string) => {
  addResponseMessage(
    getMessageByLocale('OSS_GPT_welcome', locale).replace('%v', repoName)
  );
};

const displayNotAvailable = (repoName: string, locale: string) => {
  addResponseMessage(
    getMessageByLocale('OSS_GPT_notAvailable', locale).replace('%v', repoName)
  );
};

const View = ({ theme, currentRepo, currentDocsName }: Props): JSX.Element => {
  const [inited, setInited] = useState(false);
  const [settings, setSettings] = useState(new Settings());
  const [history, setHistory] = useState<[string, string]>(['', '']);
  const mouseDownX = useRef(0); // X position when mouse down
  const rcwWidth = useRef(0); // rcw width when mouse down

  useEffect(() => {
    const initSettings = async () => {
      const temp = await loadSettings();
      setSettings(temp);
      setInited(true);
    };
    if (!inited) {
      initSettings();
    }
  }, [inited, settings]);

  const subtitle = currentDocsName
    ? getMessageByLocale('OSS_GPT_subtitle', settings.locale).replace(
        '%v',
        currentRepo
      )
    : getMessageByLocale(
        'OSS_GPT_subtitle_notAvailable',
        settings.locale
      ).replace('%v', currentRepo);

  const handleNewUserMessage = async (newMessage: string) => {
    toggleMsgLoader();
    toggleInputDisabled();

    if (currentDocsName) {
      const answer = await getAnswer(currentDocsName, newMessage, history);
      addResponseMessage(answer);
      setHistory([newMessage, answer]); // update history
    } else {
      displayNotAvailable(currentRepo, settings.locale);
    }

    toggleMsgLoader();
    toggleInputDisabled();
  };

  useEffect(() => {
    // when repo changes
    deleteMessages(Infinity); // delete all messages after repo switching
    setHistory(['', '']); // clear history
    if (currentDocsName) {
      // if docs for current repo is available
      displayWelcome(currentRepo, settings.locale);
    } else {
      displayNotAvailable(currentRepo, settings.locale);
    }
  }, [settings, currentRepo, currentDocsName]);

  const handleMouseDown = (event: JQuery.MouseDownEvent) => {
    mouseDownX.current = event.clientX;
    rcwWidth.current = parseInt(
      $('#rcw-conversation-container').css('width'),
      10
    );
    $(document).on('mousemove', handleMouseMove);
    $(document).on('mouseup', handleMouseUp);
  };

  const handleMouseMove = (event: JQuery.MouseMoveEvent) => {
    $('#rcw-conversation-container').css(
      'width',
      `${rcwWidth.current - (event.clientX - mouseDownX.current)}px`
    );
  };

  const handleMouseUp = (event: JQuery.MouseUpEvent) => {
    $(document).off('mousemove', handleMouseMove);
    $(document).off('mouseup', handleMouseUp);
  };

  // we cannot change emoji-mart theme with an option, so we have to use MutationObserver and jquery to change the css
  useEffect(() => {
    // Select the node that will be observed for mutations
    const targetNode = $('div.rcw-widget-container')[0]!;
    // Options for the observer (which mutations to observe)
    const config = { attributes: true, childList: true, subtree: true };
    // Callback function to execute when mutations are observed
    const callback: MutationCallback = (mutationList, observer) => {
      // hacking code for resizer
      if (
        !exists('.rcw-conversation-resizer') &&
        exists('.rcw-conversation-container')
      ) {
        //we only add a resizer when there is no resizer and there is an active  conversation container
        const resizerDiv = $('<div>');
        resizerDiv.attr('class', 'rcw-conversation-resizer');
        resizerDiv.on('mousedown', handleMouseDown);
        $('#rcw-conversation-container').prepend(resizerDiv);
      }
      // hacking code for emoji-mart
      if (exists('section.emoji-mart')) {
        $('section.emoji-mart').addClass(`emoji-mart emoji-mart-${theme}`);
      }
    };
    // Create an observer instance linked to the callback function
    const observer = new MutationObserver(callback);
    // Start observing the target node for configured mutations
    observer.observe(targetNode, config);

    return () => {
      // Stop observing
      observer.disconnect();
    };
  }, []);

  return (
    <div className={theme}>
      <Widget
        title="OSS-GPT"
        subtitle={subtitle}
        emojis={true} // will be enabled after style is fine tuned for two themes
        handleNewUserMessage={handleNewUserMessage}
        showBadge={false}
        profileAvatar={chrome.runtime.getURL('main.png')}
      />
    </div>
  );
};

export default View;

tyn1998 · 2023-04-28T15:28:52Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

+          handleMouseMove = (event: JQuery.MouseMoveEvent) => {
+            if (isDragging) {
+              console.log(document);
+              const mouseX = event.clientX;
+              const widthDiff = mouseX - initialMouseX;
+              conversationContainer.css(
+                'width',
+                `${initialWidth - widthDiff}px`
+              );
+            }
+          };


I tried adding console.log(anything) in handleMouseMove, and I found the times of this function being called is doubled every time after I resizing rcw. That slows the page noticeably.

I think this is because a new listener is added every time the useEffect that contains the callback function is triggered.

tyn1998 · 2023-04-28T15:29:16Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

+    function exists(selector: JQuery<HTMLElement>) {
+      return $(selector).length > 0;
+    }


We have an exists in codebase already :)

tyn1998 · 2023-04-28T15:30:52Z

src/pages/ContentScripts/features/oss-gpt/view.tsx

+      const conversationResizer = $('.rcw-conversation-resizer');
+      if (exists(conversationResizer)) {
+        const handleMouseDown = conversationResizer.data('handleMouseDown');
+        const handleMouseMove = conversationResizer.data('handleMouseMove');
+        const handleMouseUp = conversationResizer.data('handleMouseUp');
+        conversationResizer.off('mousedown', handleMouseDown);
+        $(document).off('mousemove', handleMouseMove);
+        $(document).off('mouseup', handleMouseUp);
+      }


Here, jQuery element.data() won't work as you expected. only undefined you will get.

stealth-bombeer · 2023-04-29T05:56:59Z

Hi @stealth-bombeer, I tried a cleaner way to implement our own resizer. What do you think?

import React, { useState, useEffect, useRef } from 'react';
import $ from 'jquery';
import {
  Widget,
  addResponseMessage,
  deleteMessages,
  toggleMsgLoader,
  toggleInputDisabled,
} from 'react-chat-widget';

import { getAnswer } from './service';
import './rcw.scss';
import exists from '../../../../helpers/exists';
import { getMessageByLocale } from '../../../../utils/utils';
import Settings, { loadSettings } from '../../../../utils/settings';

interface Props {
  theme: 'light' | 'dark';
  currentRepo: string;
  currentDocsName: string | null;
}

const displayWelcome = (repoName: string, locale: string) => {
  addResponseMessage(
    getMessageByLocale('OSS_GPT_welcome', locale).replace('%v', repoName)
  );
};

const displayNotAvailable = (repoName: string, locale: string) => {
  addResponseMessage(
    getMessageByLocale('OSS_GPT_notAvailable', locale).replace('%v', repoName)
  );
};

const View = ({ theme, currentRepo, currentDocsName }: Props): JSX.Element => {
  const [inited, setInited] = useState(false);
  const [settings, setSettings] = useState(new Settings());
  const [history, setHistory] = useState<[string, string]>(['', '']);
  const mouseDownX = useRef(0); // X position when mouse down
  const rcwWidth = useRef(0); // rcw width when mouse down

  useEffect(() => {
    const initSettings = async () => {
      const temp = await loadSettings();
      setSettings(temp);
      setInited(true);
    };
    if (!inited) {
      initSettings();
    }
  }, [inited, settings]);

  const subtitle = currentDocsName
    ? getMessageByLocale('OSS_GPT_subtitle', settings.locale).replace(
        '%v',
        currentRepo
      )
    : getMessageByLocale(
        'OSS_GPT_subtitle_notAvailable',
        settings.locale
      ).replace('%v', currentRepo);

  const handleNewUserMessage = async (newMessage: string) => {
    toggleMsgLoader();
    toggleInputDisabled();

    if (currentDocsName) {
      const answer = await getAnswer(currentDocsName, newMessage, history);
      addResponseMessage(answer);
      setHistory([newMessage, answer]); // update history
    } else {
      displayNotAvailable(currentRepo, settings.locale);
    }

    toggleMsgLoader();
    toggleInputDisabled();
  };

  useEffect(() => {
    // when repo changes
    deleteMessages(Infinity); // delete all messages after repo switching
    setHistory(['', '']); // clear history
    if (currentDocsName) {
      // if docs for current repo is available
      displayWelcome(currentRepo, settings.locale);
    } else {
      displayNotAvailable(currentRepo, settings.locale);
    }
  }, [settings, currentRepo, currentDocsName]);

  const handleMouseDown = (event: JQuery.MouseDownEvent) => {
    mouseDownX.current = event.clientX;
    rcwWidth.current = parseInt(
      $('#rcw-conversation-container').css('width'),
      10
    );
    $(document).on('mousemove', handleMouseMove);
    $(document).on('mouseup', handleMouseUp);
  };

  const handleMouseMove = (event: JQuery.MouseMoveEvent) => {
    $('#rcw-conversation-container').css(
      'width',
      `${rcwWidth.current - (event.clientX - mouseDownX.current)}px`
    );
  };

  const handleMouseUp = (event: JQuery.MouseUpEvent) => {
    $(document).off('mousemove', handleMouseMove);
    $(document).off('mouseup', handleMouseUp);
  };

  // we cannot change emoji-mart theme with an option, so we have to use MutationObserver and jquery to change the css
  useEffect(() => {
    // Select the node that will be observed for mutations
    const targetNode = $('div.rcw-widget-container')[0]!;
    // Options for the observer (which mutations to observe)
    const config = { attributes: true, childList: true, subtree: true };
    // Callback function to execute when mutations are observed
    const callback: MutationCallback = (mutationList, observer) => {
      // hacking code for resizer
      if (
        !exists('.rcw-conversation-resizer') &&
        exists('.rcw-conversation-container')
      ) {
        //we only add a resizer when there is no resizer and there is an active  conversation container
        const resizerDiv = $('<div>');
        resizerDiv.attr('class', 'rcw-conversation-resizer');
        resizerDiv.on('mousedown', handleMouseDown);
        $('#rcw-conversation-container').prepend(resizerDiv);
      }
      // hacking code for emoji-mart
      if (exists('section.emoji-mart')) {
        $('section.emoji-mart').addClass(`emoji-mart emoji-mart-${theme}`);
      }
    };
    // Create an observer instance linked to the callback function
    const observer = new MutationObserver(callback);
    // Start observing the target node for configured mutations
    observer.observe(targetNode, config);

    return () => {
      // Stop observing
      observer.disconnect();
    };
  }, []);

  return (
    <div className={theme}>
      <Widget
        title="OSS-GPT"
        subtitle={subtitle}
        emojis={true} // will be enabled after style is fine tuned for two themes
        handleNewUserMessage={handleNewUserMessage}
        showBadge={false}
        profileAvatar={chrome.runtime.getURL('main.png')}
      />
    </div>
  );
};

export default View;

This is great and works completely fine and I think it resolves above issues @tyn1998 and I think my code is quite repetitive at certain instances :(

tyn1998 · 2023-04-29T06:09:01Z

@stealth-bombeer your code taught me how to implement "drag to resize". That is of great value! If my implementation is good to you, you may push another commit to this branch and I'll merge this PR :)

stealth-bombeer · 2023-04-29T06:37:44Z

@stealth-bombeer your code taught me how to implement "drag to resize". That is of great value! If my implementation is good to you, you may push another commit to this branch and I'll merge this PR :)

Thanks, I'll commit making the final changes

tyn1998

LGTM, thank you so much @stealth-bombeer !

fix:Abnormal-width on text select

236694d

menbotics bot added the kind/bug Category issues or prs related to bug. label Apr 23, 2023

tyn1998 changed the title ~~fix:Abnormal-width on text select~~ fix: Abnormal-width on text select Apr 23, 2023

tyn1998 changed the title ~~fix: Abnormal-width on text select~~ fix: abnormal-width on text select Apr 23, 2023

wxharry reviewed Apr 23, 2023

View reviewed changes

tyn1998 requested changes Apr 27, 2023

View reviewed changes

fix:removes event listeners more accurately

5e4fa18

tyn1998 requested changes Apr 28, 2023

View reviewed changes

tyn1998 reviewed Apr 28, 2023

View reviewed changes

fix:resize implementation of gpt widget

df465d1

tyn1998 approved these changes Apr 29, 2023

View reviewed changes

tyn1998 merged commit 9bfbddc into hypertrons:v1.9.0 Apr 29, 2023

menbotics bot mentioned this pull request May 1, 2023

[WeeklyReport] Weekly report for hypertrons-crx 4/24/2023 to 5/1/2023 #665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: abnormal-width on text select #658

fix: abnormal-width on text select #658

stealth-bombeer commented Apr 23, 2023 •

edited

Loading

tyn1998 commented Apr 23, 2023

wxharry left a comment

stealth-bombeer commented Apr 24, 2023

wxharry commented Apr 24, 2023 •

edited

Loading

stealth-bombeer commented Apr 25, 2023

tyn1998 commented Apr 27, 2023

stealth-bombeer commented Apr 27, 2023

tyn1998 commented Apr 27, 2023

tyn1998 left a comment

tyn1998 Apr 27, 2023

stealth-bombeer Apr 28, 2023 •

edited

Loading

tyn1998 Apr 27, 2023 •

edited

Loading

tyn1998 Apr 27, 2023 •

edited

Loading

stealth-bombeer commented Apr 28, 2023 •

edited

Loading

tyn1998 left a comment

tyn1998 Apr 28, 2023 •

edited

Loading

stealth-bombeer Apr 29, 2023

tyn1998 Apr 28, 2023

tyn1998 Apr 28, 2023

stealth-bombeer commented Apr 29, 2023 •

edited

Loading

tyn1998 commented Apr 29, 2023

stealth-bombeer commented Apr 29, 2023

tyn1998 left a comment

fix: abnormal-width on text select #658

fix: abnormal-width on text select #658

Conversation

stealth-bombeer commented Apr 23, 2023 • edited Loading

Brief Information

Details

Checklist

Others

tyn1998 commented Apr 23, 2023

wxharry left a comment

Choose a reason for hiding this comment

stealth-bombeer commented Apr 24, 2023

wxharry commented Apr 24, 2023 • edited Loading

stealth-bombeer commented Apr 25, 2023

tyn1998 commented Apr 27, 2023

stealth-bombeer commented Apr 27, 2023

tyn1998 commented Apr 27, 2023

tyn1998 left a comment

Choose a reason for hiding this comment

tyn1998 Apr 27, 2023

Choose a reason for hiding this comment

stealth-bombeer Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

tyn1998 Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

tyn1998 Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

stealth-bombeer commented Apr 28, 2023 • edited Loading

tyn1998 left a comment

Choose a reason for hiding this comment

tyn1998 Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

stealth-bombeer Apr 29, 2023

Choose a reason for hiding this comment

tyn1998 Apr 28, 2023

Choose a reason for hiding this comment

tyn1998 Apr 28, 2023

Choose a reason for hiding this comment

stealth-bombeer commented Apr 29, 2023 • edited Loading

tyn1998 commented Apr 29, 2023

stealth-bombeer commented Apr 29, 2023

tyn1998 left a comment

Choose a reason for hiding this comment

stealth-bombeer commented Apr 23, 2023 •

edited

Loading

wxharry commented Apr 24, 2023 •

edited

Loading

stealth-bombeer Apr 28, 2023 •

edited

Loading

tyn1998 Apr 27, 2023 •

edited

Loading

tyn1998 Apr 27, 2023 •

edited

Loading

stealth-bombeer commented Apr 28, 2023 •

edited

Loading

tyn1998 Apr 28, 2023 •

edited

Loading

stealth-bombeer commented Apr 29, 2023 •

edited

Loading