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

Assistant: Fix scroll glitch on initial render #804

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Assistant: Fix scroll glitch on initial render #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a86b645
Assistant: Fix scroll glitch on initial render
fredrikekelund Jan 14, 2025
be13399
Fix tests
fredrikekelund Jan 14, 2025
8dc1bad
Fixed last test
fredrikekelund Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 15 additions & 22 deletions src/components/ai-clear-history-reminder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,43 +6,36 @@ import { Message as MessageType } from '../hooks/use-assistant';
import { getIpcApi } from '../lib/get-ipc-api';
import Button from './button';

function shouldShowReminder( lastMessage?: MessageType ) {
return (
lastMessage?.role === 'assistant' &&
Date.now() - ( lastMessage?.createdAt ?? 0 ) > CLEAR_HISTORY_REMINDER_TIME
);
}

function AIClearHistoryReminder( {
lastMessage,
clearConversation,
}: {
lastMessage?: MessageType;
clearConversation: () => void;
} ) {
const [ showReminder, setShowReminder ] = useState( false );
const timeoutId = useRef< NodeJS.Timeout >();
const currentMessage = useRef< number >();
const [ showReminder, setShowReminder ] = useState( shouldShowReminder( lastMessage ) );
const elementRef = useRef< HTMLDivElement >( null );

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 );
};
Comment on lines -22 to 39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}, [ lastMessage ] );

Expand Down
12 changes: 7 additions & 5 deletions src/components/content-tab-assistant.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ interface AuthenticatedViewProps {
markMessageAsFeedbackReceived: ReturnType<
typeof useAssistant
>[ 'markMessageAsFeedbackReceived' ];
wrapperRef: React.RefObject< HTMLDivElement >;
}

const AuthenticatedView = memo(
Expand All @@ -122,8 +123,8 @@ const AuthenticatedView = memo(
siteId,
submitPrompt,
markMessageAsFeedbackReceived,
wrapperRef,
}: AuthenticatedViewProps ) => {
const endOfMessagesRef = useRef< HTMLDivElement >( null );
const lastMessageRef = useRef< HTMLDivElement >( null );
const [ showThinking, setShowThinking ] = useState( isAssistantThinking );
const lastMessage = useMemo(
Expand Down Expand Up @@ -151,7 +152,7 @@ const AuthenticatedView = memo(
let timer: NodeJS.Timeout;
// Scroll to the end of the messages when the tab is opened or site ID changes
if ( isInitialRenderRef.current ) {
endOfMessagesRef.current?.scrollIntoView( { behavior: 'instant' } );
wrapperRef.current?.scrollIntoView( { block: 'end', behavior: 'instant' } );
isInitialRenderRef.current = false;
}
// Scroll when a new message is added
Expand All @@ -166,7 +167,7 @@ const AuthenticatedView = memo(
}
// For user messages, scroll to the end of the messages
else {
endOfMessagesRef.current?.scrollIntoView( { behavior: 'smooth' } );
wrapperRef.current?.scrollIntoView( { block: 'end', behavior: 'smooth' } );
}
}

Expand Down Expand Up @@ -300,7 +301,6 @@ const AuthenticatedView = memo(
</div>
</RenderLastMessage>
) }
<div ref={ endOfMessagesRef } />
</>
);
}
Expand Down Expand Up @@ -351,6 +351,7 @@ const UnauthenticatedView = ( { onAuthenticate }: { onAuthenticate: () => void }

export function ContentTabAssistant( { selectedSite }: ContentTabAssistantProps ) {
const inputRef = useRef< HTMLTextAreaElement >( null );
const wrapperRef = useRef< HTMLDivElement >( null );
const chatContext = useChatContext();
const { isAuthenticated, authenticate, user } = useAuth();
const instanceId = user?.id ? `${ user.id }_${ selectedSite.id }` : selectedSite.id;
Expand Down Expand Up @@ -467,7 +468,7 @@ export function ContentTabAssistant( { selectedSite }: ContentTabAssistantProps
const disabled = isOffline || ! isAuthenticated || ! userCanSendMessage || hasFailedMessage;

return (
<div className="relative min-h-full flex flex-col bg-gray-50">
<div className="relative min-h-full flex flex-col bg-gray-50" ref={ wrapperRef }>
<div
data-testid="assistant-chat"
className={ cx(
Expand Down Expand Up @@ -497,6 +498,7 @@ export function ContentTabAssistant( { selectedSite }: ContentTabAssistantProps
markMessageAsFeedbackReceived={ markMessageAsFeedbackReceived }
siteId={ selectedSite.id }
submitPrompt={ submitPrompt }
wrapperRef={ wrapperRef }
/>
</>
) : (
Expand Down
15 changes: 8 additions & 7 deletions src/components/tests/ai-clear-history-reminder.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
import { render, screen, waitFor, fireEvent } from '@testing-library/react';
import { CLEAR_HISTORY_REMINDER_TIME } from '../../constants';
import { getIpcApi } from '../../lib/get-ipc-api';
import AIClearHistoryReminder from '../ai-clear-history-reminder';
import type { Message } from '../../hooks/use-assistant';
Expand All @@ -7,15 +8,15 @@ jest.mock( '../../lib/get-ipc-api' );

describe( 'AIClearHistoryReminder', () => {
let clearConversation: jest.Mock;
const MOCKED_TIME = 1718882159928;
const TWO_HOURS_DIFF = 2 * 60 * 60 * 1000;
const MOCKED_CURRENT_TIME = 1718882159928;
const OLD_MESSAGE_TIME = MOCKED_CURRENT_TIME - CLEAR_HISTORY_REMINDER_TIME - 1;

beforeEach( () => {
window.HTMLElement.prototype.scrollIntoView = jest.fn();
clearConversation = jest.fn();
jest.clearAllMocks();
jest.useFakeTimers();
jest.setSystemTime( MOCKED_TIME );
jest.setSystemTime( MOCKED_CURRENT_TIME );
} );

afterEach( () => {
Expand All @@ -25,7 +26,7 @@ describe( 'AIClearHistoryReminder', () => {
it( 'should display a reminder when the conversation is stale', () => {
const message: Message = {
id: 0,
createdAt: MOCKED_TIME - TWO_HOURS_DIFF,
createdAt: OLD_MESSAGE_TIME,
content: '',
role: 'assistant',
};
Expand All @@ -42,7 +43,7 @@ describe( 'AIClearHistoryReminder', () => {
} );
const message: Message = {
id: 0,
createdAt: MOCKED_TIME - TWO_HOURS_DIFF,
createdAt: OLD_MESSAGE_TIME,
content: '',
role: 'assistant',
};
Expand All @@ -65,7 +66,7 @@ describe( 'AIClearHistoryReminder', () => {
} );
const message: Message = {
id: 0,
createdAt: MOCKED_TIME - TWO_HOURS_DIFF,
createdAt: OLD_MESSAGE_TIME,
content: '',
role: 'assistant',
};
Expand Down
9 changes: 5 additions & 4 deletions src/components/tests/content-tab-assistant.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { render, screen, fireEvent, waitFor, act } from '@testing-library/react';
import { userEvent } from '@testing-library/user-event';
import { CLEAR_HISTORY_REMINDER_TIME } from '../../constants';
import { useAuth } from '../../hooks/use-auth';
import { ChatProvider } from '../../hooks/use-chat-context';
import { useGetWpVersion } from '../../hooks/use-get-wp-version';
Expand Down Expand Up @@ -338,10 +339,10 @@ describe( 'ContentTabAssistant', () => {
} );

test( 'clears history via reminder when last message is two hours old', async () => {
const MOCKED_TIME = 1718882159928;
const TWO_HOURS_DIFF = 2 * 60 * 60 * 1000;
const MOCKED_CURRENT_TIME = 1718882159928;
const OLD_MESSAGE_TIME = MOCKED_CURRENT_TIME - CLEAR_HISTORY_REMINDER_TIME - 1;
jest.useFakeTimers();
jest.setSystemTime( MOCKED_TIME );
jest.setSystemTime( MOCKED_CURRENT_TIME );

const storageKey = 'ai_chat_messages';
localStorage.setItem(
Expand All @@ -353,7 +354,7 @@ describe( 'ContentTabAssistant', () => {
id: 1,
content: 'Initial message 2',
role: 'assistant',
createdAt: MOCKED_TIME - TWO_HOURS_DIFF,
createdAt: OLD_MESSAGE_TIME,
},
],
} )
Expand Down
Loading