Assistant: Fix scroll glitch on initial render#804
Conversation
| useEffect( () => { | ||
| if ( ! lastMessage || lastMessage.role === 'user' ) { | ||
| clearTimeout( timeoutId.current ); | ||
| setShowReminder( false ); | ||
| return; | ||
| } | ||
|
|
||
| if ( lastMessage.id === currentMessage.current ) { | ||
| return; | ||
| } | ||
| let timeoutId: NodeJS.Timeout; | ||
| const nextValue = shouldShowReminder( lastMessage ); | ||
| setShowReminder( nextValue ); | ||
|
|
||
| const messageTime = Date.now() - lastMessage?.createdAt; | ||
| const timeToRemind = CLEAR_HISTORY_REMINDER_TIME - messageTime; | ||
| if ( timeToRemind > 0 ) { | ||
| clearTimeout( timeoutId.current ); | ||
| setShowReminder( false ); | ||
| timeoutId.current = setTimeout( () => { | ||
| if ( nextValue && lastMessage ) { | ||
| timeoutId = setTimeout( () => { | ||
| setShowReminder( true ); | ||
| }, timeToRemind ); | ||
| } else { | ||
| setShowReminder( true ); | ||
| }, Date.now() - lastMessage.createdAt ); | ||
| } | ||
|
|
||
| return () => { | ||
| clearTimeout( timeoutId.current ); | ||
| clearTimeout( timeoutId ); | ||
| }; |
There was a problem hiding this comment.
There's no need to store timeoutId outside the hook callback, so a big part of this change is just simplifying the existing logic and moving timeoutId inside this closure.
|
@fredrikekelund Assistant still lives its own life and scrolls up and down when I switch tabs. Is it caused by double rendering in dev mode? Should we test PRs related to the scrolling using proper build instead of running the app on local? |
|
@wojtekn, oh yeah, diff --git a/src/renderer.ts b/src/renderer.ts
index c496ade..32a275c 100644
--- a/src/renderer.ts
+++ b/src/renderer.ts
@@ -128,6 +128,6 @@ getIpcApi()
const rootEl = document.getElementById( 'root' );
if ( rootEl ) {
const root = createRoot( rootEl );
- root.render( createElement( StrictMode, null, createElement( Root ) ) );
+ root.render( createElement( Root ) );
}
} );
|
|
@fredrikekelund applying the patch improves it, so it doesn't scroll to the top of the message. However, the glitch is still there: ai-scroll.mp4 |
Great 👍
The movement that happens when you open the Assistant tab in your screencast is actually an animation—not the viewport scrolling 🙂 |
Not sure. The scrollbar appears, and the content moves until the view becomes static. 🤔 |
|
I applied the following diff to confirm my theory. You can do the same to check, @wojtekn. diff --git a/src/components/content-tab-assistant.tsx b/src/components/content-tab-assistant.tsx
index 74da799..d5cec49 100644
--- a/src/components/content-tab-assistant.tsx
+++ b/src/components/content-tab-assistant.tsx
@@ -240,24 +240,11 @@ const AuthenticatedView = memo(
>
<AnimatePresence mode="wait">
{ showThinking ? (
- <motion.div
- key="thinking"
- initial="initial"
- animate="animate"
- exit="exit"
- variants={ thinkingAnimation }
- transition={ { duration: 0.3 } }
- >
+ <div key="thinking">
<MessageThinking />
- </motion.div>
+ </div>
) : (
- <motion.div
- key="content"
- variants={ messageAnimation }
- transition={ { duration: 0.3 } }
- initial="initial"
- animate="animate"
- >
+ <div key="content">
<MarkDownWithCode
message={ message }
siteId={ siteId }
@@ -265,7 +252,7 @@ const AuthenticatedView = memo(
content={ message.content }
/>
{ children }
- </motion.div>
+ </div>
) }
</AnimatePresence>
</ChatMessage> |
|
Thanks for checking it more @fredrikekelund . It looks much better after removing those animations. It seems we've added them under https://github.com/Automattic/dotcom-forge/issues/7679. Let's remove those if we can't fix them. While testing that, I noticed small remaining issue and added https://github.com/Automattic/dotcom-forge/issues/10254 for that. CC @sejas |
|
I created https://github.com/Automattic/dotcom-forge/issues/10256 for removing the animation on the most recent message when the tab is opened. I think it's a nice touch when you're chatting with the Assistant and a new message appears, but I agree it makes less sense on initial render. |
2 participants
Related issues
Proposed Changes
As described in https://github.com/Automattic/dotcom-forge/issues/9281, there are currently some glitches in how the viewport scrolls when the Assistant tab is opened. This PR fixes those by changing
endOfMessagesRefinto a ref on the topmost wrapper in the Assistant tab and by computing whetherAIClearHistoryRemindershould be displayed instantly instead of doing it in auseEffectcallback.Testing Instructions
Pre-merge Checklist