aichat: scroll is interruptable #21235

nullhook · 2023-12-05T05:04:14Z

This PR allows user to interrupts the autoscroll while an answer is being generated. Additionally, it will resume back to autoscroll if the user's scroll is near the bottom.

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

brave-builds · 2023-12-05T06:14:02Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-12-05T18:33:25Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-12-06T19:03:57Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-12-06T22:00:26Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-12-07T17:50:08Z

A Storybook has been deployed to preview UI for the latest push

petemill

It isn't auto-scrolling the user message anymore. So when I press enter on my message, I don't see it until a couple seconds later when the AI response starts being received.

brave-builds · 2023-12-16T00:15:57Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2023-12-27T11:00:28Z

components/ai_chat/resources/page/components/main/index.tsx

+      return
+    }
+
+    if (scrollPos.current.isAtBottom) {


Why can't we just here check if scrollerElement is at the bottom and not need to have handleScroll and scrollPos?

i did try that initially, but it looks like there's a timing issue. basically, computing isAtBottom in the same call stack is flawed. additionally, it doesn't monitor when the user has scrolled to the bottom, which we need for auto scrolling behavior.

Well, perhaps we can overcome the potential performance issue another way then. I'm seeing handleScroll called on every scroll when it's not neccessary until the content of the "last element" changes.
I'm assuming your issue when you tried this initially is because handleLastElementHeightChange is called immediately when the content of the react properties change but before they are rendered (and definitely before the html layout is done). So couldn't that be fixed with a requestAnimationFrame?

i have also tried using rAF to sync with browser's rendering within the same callstack. however, we are still off.

still, this approach doesn't resolve the need to continuously monitor the user's scroll while the generation is occurring. if the user decides to scroll back to the bottom, we need to switch back to the scroll-to-bottom behavior.

i agree that handleScroll doesn't really need to be called on every tick. we can overcome this issue by throttling?

Yes, throttle at the very least. Or useLayoutEffect?

actually, we do need to monitor every tick. throttling can be unreliable because if the user is scrolling towards the bottom, it may compute positions at a later time, potentially missing the opportunity to autoscroll.

we could run the scroll handler in rAF and make it tick with the monitor's refresh rate. ex. https://codepen.io/dcorb/pen/pgOKKw - potentially lowers the number of ticks compared to before but not significantly as throttling (depends on how limit).

another idea: what if we run or attach the scroll handler only if context.isGenerating? this satisfies the need to avoid unnecessary ticking and computing positions in a separate call stack.

changed to only monitor when it's generating

brave-builds · 2023-12-27T23:56:30Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-01-12T17:39:39Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-01-12T23:42:58Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-01-16T20:14:56Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2024-01-17T06:21:24Z

components/ai_chat/resources/page/components/main/index.tsx

@@ -85,6 +89,23 @@ function Main() {
    }
  }

+  const handleScroll = (e: React.UIEvent<HTMLDivElement>) => {
+    // Monitor scroll positions only when Assistant is generating
+    if (!context.isGenerating) return


nit: please don't put return on same line

stephendonner · 2024-01-18T23:59:16Z

Verified `PASSED` using

Brave | 1.64.3 Chromium: 121.0.6167.57 (Official Build) nightly (64-bit)
-- | --
Revision | 0ecd8a5470d06ce96921cb7585ecbc0f9878f3cf
OS | Windows 10 Version 22H2 (Build 19045.3930)

Steps:

installed 1.64.3
launched Brave using --env-leo=staging --env-ai-chat.bsg=dev --env-ai-chat-premium.bsg=dev --enable-logging=stderr --v=2
loaded https://constitutioncenter.org/the-constitution/full-text
clicked on Summarize this page
wait for auto-scrolling to start
swipe up and wait
scroll down

Confirmed auto-scrolling is interrupted by mouse/trackpad motion, with the viewport position held until further scrolling

Confirmed after the above that you can resume auto-scrolling once at or near the bottom of the content

2024-01-18_15h57_26.mp4

* aichat: input is growable (#21124) * aichat: scroll is interruptable (#21235) * aichat: model maker text shouldnt look like a link (#21220) * aichat: code formatting (#21342) * make claude output formatted code (#21599)

* Merge pull request #21134 from brave/cr121 Upgrade from Chromium 120 to Chromium 121. * Remove the assert for patch_ffmpeg.py (#21184) * Merge pull request #21539 from brave/ffmpeg-dynamic-alloc Use dynamic allocation for ffmpeg fft tables on Windows. * Merge pull request #21585 from brave/issues/35318 Remove dynamic allocation of ffmpeg ff_tx tables. * Disables a flaky upstream browser test. * Merge pull request #21584 from brave/fix_new_tab_button_plus_misaligned Fixed new tab button's plus icon is mis-aligned with horizontal tab * Merge pull request #21600 from brave/121.0.6167.75_master Upgrade from Chromium 121.0.6167.57 to Chromium 121.0.6167.75. * Merge pull request #21628 from brave/maxk-disable-reading-mode Hides `Open in Reading Mode` context menu item. * [Uplift 1.62.x] AI chat issues cr121 1.62.x (#21629) * aichat: input is growable (#21124) * aichat: scroll is interruptable (#21235) * aichat: model maker text shouldnt look like a link (#21220) * aichat: code formatting (#21342) * make claude output formatted code (#21599) --------- Co-authored-by: Mikhail <[email protected]> Co-authored-by: Aleksey Khoroshilov <[email protected]> Co-authored-by: Simon Hong <[email protected]> Co-authored-by: taher <[email protected]>

github-actions bot assigned nullhook Dec 5, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Dec 5, 2023

nullhook force-pushed the ai-chat-scroll-issue branch from a530c67 to 44614e0 Compare December 5, 2023 18:13

nullhook requested a review from petemill December 6, 2023 21:42

nullhook marked this pull request as ready for review December 6, 2023 21:42

nullhook force-pushed the ai-chat-scroll-issue branch from c2b2e31 to f021ca3 Compare December 7, 2023 16:50

petemill requested changes Dec 14, 2023

View reviewed changes

nullhook force-pushed the ai-chat-scroll-issue branch from f021ca3 to 1c22956 Compare December 15, 2023 23:09

nullhook requested a review from petemill December 15, 2023 23:09

petemill reviewed Dec 27, 2023

View reviewed changes

nullhook force-pushed the ai-chat-scroll-issue branch from 1c22956 to 324eada Compare December 27, 2023 22:53

nullhook force-pushed the ai-chat-scroll-issue branch from 324eada to ff91d0a Compare January 12, 2024 16:32

nullhook requested a review from petemill January 14, 2024 01:29

nullhook added 5 commits January 16, 2024 11:17

aichat: scroll is interruptable

419c239

use ref callback

ba133d2

cleanup

33a8683

should scroll to bottom on user message also

dd67ec8

monitor only when its generating

8b82674

nullhook force-pushed the ai-chat-scroll-issue branch from f4ec482 to 8b82674 Compare January 16, 2024 19:17

petemill reviewed Jan 17, 2024

View reviewed changes

petemill approved these changes Jan 17, 2024

View reviewed changes

nullhook merged commit ce69a31 into master Jan 17, 2024
18 checks passed

nullhook deleted the ai-chat-scroll-issue branch January 17, 2024 16:16

github-actions bot added this to the 1.63.x - Nightly milestone Jan 17, 2024

nullhook added a commit that referenced this pull request Jan 18, 2024

aichat: scroll is interruptable (#21235)

7e314a7

nullhook mentioned this pull request Jan 18, 2024

[Uplift 1.62.x] AI chat issues cr121 1.62.x #21629

Merged

kjozwiak mentioned this pull request Jan 20, 2024

Upgrade from Chromium 120 to Chromium 121 (1.62.x). #21593

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aichat: scroll is interruptable #21235

aichat: scroll is interruptable #21235

nullhook commented Dec 5, 2023 •

edited

Loading

brave-builds commented Dec 5, 2023

brave-builds commented Dec 5, 2023

brave-builds commented Dec 6, 2023

brave-builds commented Dec 6, 2023

brave-builds commented Dec 7, 2023

petemill left a comment

brave-builds commented Dec 16, 2023

petemill Dec 27, 2023

nullhook Dec 28, 2023

petemill Jan 4, 2024

nullhook Jan 5, 2024 •

edited

Loading

petemill Jan 9, 2024

nullhook Jan 12, 2024 •

edited

Loading

nullhook Jan 12, 2024 •

edited

Loading

nullhook Jan 16, 2024

brave-builds commented Dec 27, 2023

brave-builds commented Jan 12, 2024

brave-builds commented Jan 12, 2024

brave-builds commented Jan 16, 2024

petemill Jan 17, 2024

stephendonner commented Jan 18, 2024 •

edited

Loading

aichat: scroll is interruptable #21235

aichat: scroll is interruptable #21235

Conversation

nullhook commented Dec 5, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Dec 5, 2023

brave-builds commented Dec 5, 2023

brave-builds commented Dec 6, 2023

brave-builds commented Dec 6, 2023

brave-builds commented Dec 7, 2023

petemill left a comment

Choose a reason for hiding this comment

brave-builds commented Dec 16, 2023

petemill Dec 27, 2023

Choose a reason for hiding this comment

nullhook Dec 28, 2023

Choose a reason for hiding this comment

petemill Jan 4, 2024

Choose a reason for hiding this comment

nullhook Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

petemill Jan 9, 2024

Choose a reason for hiding this comment

nullhook Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

nullhook Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

nullhook Jan 16, 2024

Choose a reason for hiding this comment

brave-builds commented Dec 27, 2023

brave-builds commented Jan 12, 2024

brave-builds commented Jan 12, 2024

brave-builds commented Jan 16, 2024

petemill Jan 17, 2024

Choose a reason for hiding this comment

stephendonner commented Jan 18, 2024 • edited Loading

Verified PASSED using

Steps:

Confirmed auto-scrolling is interrupted by mouse/trackpad motion, with the viewport position held until further scrolling

Confirmed after the above that you can resume auto-scrolling once at or near the bottom of the content

nullhook commented Dec 5, 2023 •

edited

Loading

nullhook Jan 5, 2024 •

edited

Loading

nullhook Jan 12, 2024 •

edited

Loading

nullhook Jan 12, 2024 •

edited

Loading

stephendonner commented Jan 18, 2024 •

edited

Loading

Verified `PASSED` using