From 636e6927f77ad3da3e144637b0208fe5638526ec Mon Sep 17 00:00:00 2001 From: Saoud Rizwan <7799382+saoudrizwan@users.noreply.github.com> Date: Sat, 5 Oct 2024 21:07:39 -0400 Subject: [PATCH] Fixes --- src/core/ClaudeDev.ts | 14 +- webview-ui/src/components/chat/ChatRow.tsx | 6 +- webview-ui/src/components/chat/ChatView.tsx | 202 ++++++-------------- 3 files changed, 69 insertions(+), 153 deletions(-) diff --git a/src/core/ClaudeDev.ts b/src/core/ClaudeDev.ts index 8e47726..e08640d 100644 --- a/src/core/ClaudeDev.ts +++ b/src/core/ClaudeDev.ts @@ -36,20 +36,12 @@ import { calculateApiCost } from "../utils/cost" import { fileExistsAtPath } from "../utils/fs" import { arePathsEqual, getReadablePath } from "../utils/path" import { parseMentions } from "./mentions" -import { - AssistantMessageContent, - TextContent, - ToolParamName, - toolParamNames, - ToolUse, - ToolUseName, - toolUseNames, -} from "./prompts/AssistantMessage" +import { AssistantMessageContent, ToolParamName, ToolUseName } from "./prompts/AssistantMessage" +import { parseAssistantMessage } from "./prompts/parse-assistant-message" import { formatResponse } from "./prompts/responses" import { addCustomInstructions, SYSTEM_PROMPT } from "./prompts/system" import { truncateHalfConversation } from "./sliding-window" import { ClaudeDevProvider, GlobalFileNames } from "./webview/ClaudeDevProvider" -import { parseAssistantMessage } from "./prompts/parse-assistant-message" const cwd = vscode.workspace.workspaceFolders?.map((folder) => folder.uri.fsPath).at(0) ?? path.join(os.homedir(), "Desktop") // may or may not exist but fs checking existence would immediately ask for permission which would be bad UX, need to come up with a better solution @@ -419,7 +411,7 @@ export class ClaudeDev { if (lastApiReqStartedIndex !== -1) { const lastApiReqStarted = modifiedClaudeMessages[lastApiReqStartedIndex] const { cost, cancelReason }: ClaudeApiReqInfo = JSON.parse(lastApiReqStarted.text || "{}") - if (cost === undefined || cancelReason === undefined) { + if (cost === undefined && cancelReason === undefined) { modifiedClaudeMessages.splice(lastApiReqStartedIndex, 1) } } diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 517d892..d375aa8 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -17,7 +17,7 @@ interface ChatRowProps { onToggleExpand: () => void lastModifiedMessage?: ClaudeMessage isLast: boolean - onHeightChange: (height: number) => void + onHeightChange: (isTaller: boolean) => void } interface ChatRowContentProps extends Omit {} @@ -44,10 +44,10 @@ const ChatRow = memo( const isInitialRender = prevHeightRef.current === 0 // prevents scrolling when new element is added since we already scroll for that // height starts off at Infinity if (isLast && height !== 0 && height !== Infinity && height !== prevHeightRef.current) { - prevHeightRef.current = height if (!isInitialRender) { - onHeightChange(height) + onHeightChange(height > prevHeightRef.current) } + prevHeightRef.current = height } }, [height, isLast, onHeightChange, message]) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index a7dbbe3..4d06a8d 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -46,18 +46,13 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie const [enableButtons, setEnableButtons] = useState(false) const [primaryButtonText, setPrimaryButtonText] = useState(undefined) const [secondaryButtonText, setSecondaryButtonText] = useState(undefined) + const [didClickCancel, setDidClickCancel] = useState(false) const virtuosoRef = useRef(null) const [expandedRows, setExpandedRows] = useState>({}) - + const scrollContainerRef = useRef(null) + const disableAutoScrollRef = useRef(false) const [showScrollToBottom, setShowScrollToBottom] = useState(false) - const isAtBottomRef = useRef(false) - - const lastScrollTopRef = useRef(0) - const [didClickScrollToBottom, setDidClickScrollToBottom] = useState(false) - const didJustSendMessageRef = useRef(false) - const didScrollUpRef = useRef(false) - const didJustAddMessagesRef = useRef(false) - const [didClickCancel, setDidClickCancel] = useState(false) + const [isAtBottom, setIsAtBottom] = useState(false) // UI layout depends on the last 2 messages // (since it relies on the content of these messages, we are deep comparing. i.e. the button state after hitting button sets enableButtons to false, and this effect otherwise would have to true again even if messages didn't change @@ -253,12 +248,7 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie setEnableButtons(false) // setPrimaryButtonText(undefined) // setSecondaryButtonText(undefined) - - // when sending a message user should be scrolled to the bottom (this also addresses a bug where sometimes when sending a message virtuoso would jump upwards, possibly due to textarea changing in size) - didJustSendMessageRef.current = true - setTimeout(() => { - didJustSendMessageRef.current = false - }, 400) + disableAutoScrollRef.current = false } }, [messages.length, claudeAsk] @@ -467,7 +457,12 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie [ts]: !prev[ts], })) - if (isCollapsing && isAtBottomRef.current) { + // disable auto scroll when user expands row + if (!isCollapsing) { + disableAutoScrollRef.current = true + } + + if (isCollapsing && isAtBottom) { const timer = setTimeout(() => { scrollToBottomAuto() }, 0) @@ -492,72 +487,26 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie } } }, - [visibleMessages, expandedRows, scrollToBottomAuto] + [visibleMessages, expandedRows, scrollToBottomAuto, isAtBottom] ) - const handleRowHeightChange = useCallback(() => { - if ( - isAtBottomRef.current || - didClickScrollToBottom || - !didScrollUpRef.current || - didJustSendMessageRef.current - ) { - scrollToBottomSmooth() - } - }, [scrollToBottomSmooth, didClickScrollToBottom]) - - /* - - didScrollUp lets us know if the user scrolled up, so we don't auto scroll down anymore during stream - - this variable is important to make sure we don't show the scroll to bottom button during streaming since isAtBottom gets set to false if the streamed in content is taller than the bottom threshold. - - didClickScrollToBottom is used to keep scrolling down when last row height changes, as isAtBottom may not update fast enough during stream - - we use the following scroll listener to detect that the current scroll point is less than the last scroll point, and in atBottomStateChange we set this back to false if isAtBottom. This way we can know when to show the scroll to bottom button. - - interestingly followoutput would scroll even if the isAtBottom param was false or wrong, so if didScrollUp we don't followoutput to mitigate that issue - */ - const handleScroll = useCallback((e: React.UIEvent) => { - if (didJustAddMessagesRef.current) { - // ignore scrolls that occur when new messages are added - return - } - const currentScrollTop = e.currentTarget.scrollTop - if (currentScrollTop < lastScrollTopRef.current) { - didScrollUpRef.current = true - } - lastScrollTopRef.current = currentScrollTop - }, []) - - useEffect(() => { - const lastMessage = messages.at(-1) - if (lastMessage) { - switch (lastMessage.say) { - case "api_req_retried": { - // unique case where the api_req_started row will shrink in size when scrollview at bottom, causing didScrollUp to get set to true and followoutput to break. To mitigate this when an api request is retried, it's safe to assume they're already scrolled to the bottom - const timer = setTimeout(() => { + const handleRowHeightChange = useCallback( + (isTaller: boolean) => { + if (!disableAutoScrollRef.current) { + if (isTaller) { + scrollToBottomSmooth() + } else { + setTimeout(() => { scrollToBottomAuto() - }, 50) - return () => clearTimeout(timer) + }, 0) } - default: - break } - } - }, [messages, scrollToBottomAuto, scrollToBottomSmooth]) + }, + [scrollToBottomSmooth, scrollToBottomAuto] + ) useEffect(() => { - let shouldScroll = false - if (didJustSendMessageRef.current) { - shouldScroll = true - } - if (isAtBottomRef.current) { - shouldScroll = true - } - if (didScrollUpRef.current) { - // would sometimes get set to true even when new items get added. followoutput taking us to bottom will set this back to false - shouldScroll = false - } else { - shouldScroll = true - } - - if (shouldScroll) { + if (!disableAutoScrollRef.current) { setTimeout(() => { scrollToBottomSmooth() }, 50) @@ -565,13 +514,16 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie } }, [visibleMessages.length, scrollToBottomSmooth]) - useEffect(() => { - didJustAddMessagesRef.current = true - const timer = setTimeout(() => { - didJustAddMessagesRef.current = false - }, 100) - return () => clearTimeout(timer) - }, [visibleMessages.length]) + const handleWheel = useCallback((event: Event) => { + const wheelEvent = event as WheelEvent + if (wheelEvent.deltaY && wheelEvent.deltaY < 0) { + if (scrollContainerRef.current?.contains(wheelEvent.target as Node)) { + // user scrolled up + disableAutoScrollRef.current = true + } + } + }, []) + useEvent("wheel", handleWheel, window, { passive: true }) // passive improves scrolling performance const placeholderText = useMemo(() => { const text = task ? "Type a message (@ to add context)..." : "Type your task here (@ to add context)..." @@ -644,61 +596,33 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie )} {task && ( <> - { - // if (didJustSendMessage) { - // return "smooth" - // } - // if (isAtBottom) { - // return "smooth" - // } - // if (didScrollUp) { - // // would sometimes get set to true even when new items get added. followoutput taking us to bottom will set this back to false - // return false - // } else { - // return "smooth" - // } - // // if (isAtBottom) { - // // return "smooth" // can be 'auto' or false to avoid scrolling - // // } else { - // // return false - // // } - // }} - components={{ - Footer: () =>
, // Add empty padding at the bottom - }} - // followOutput={(isAtBottom) => { - // const lastMessage = modifiedMessages.at(-1) - // if (lastMessage && shouldShowChatRow(lastMessage)) { - // return "smooth" - // } - // return false - // }} - // increasing top by 3_000 to prevent jumping around when user collapses a row - increaseViewportBy={{ top: 3_000, bottom: Number.MAX_SAFE_INTEGER }} // hack to make sure the last message is always rendered to get truly perfect scroll to bottom animation when new messages are added (Number.MAX_SAFE_INTEGER is safe for arithmetic operations, which is all virtuoso uses this value for in src/sizeRangeSystem.ts) - data={visibleMessages} // messages is the raw format returned by extension, modifiedMessages is the manipulated structure that combines certain messages of related type, and visibleMessages is the filtered structure that removes messages that should not be rendered - itemContent={itemContent} - atBottomStateChange={(isAtBottom) => { - isAtBottomRef.current = isAtBottom - // setShowScrollToBottom(!value) - if (isAtBottom) { - didScrollUpRef.current = false - setDidClickScrollToBottom(false) // reset for next time user clicks button - } - setShowScrollToBottom(didScrollUpRef.current && !isAtBottom) - }} - atBottomThreshold={10} // anything lower causes issues with followOutput - onScroll={handleScroll} - initialTopMostItemIndex={visibleMessages.length - 1} - /> +
+
, // Add empty padding at the bottom + }} + // increasing top by 3_000 to prevent jumping around when user collapses a row + increaseViewportBy={{ top: 3_000, bottom: Number.MAX_SAFE_INTEGER }} // hack to make sure the last message is always rendered to get truly perfect scroll to bottom animation when new messages are added (Number.MAX_SAFE_INTEGER is safe for arithmetic operations, which is all virtuoso uses this value for in src/sizeRangeSystem.ts) + data={visibleMessages} // messages is the raw format returned by extension, modifiedMessages is the manipulated structure that combines certain messages of related type, and visibleMessages is the filtered structure that removes messages that should not be rendered + itemContent={itemContent} + atBottomStateChange={(isAtBottom) => { + setIsAtBottom(isAtBottom) + if (isAtBottom) { + disableAutoScrollRef.current = false + } + setShowScrollToBottom(disableAutoScrollRef.current && !isAtBottom) + }} + atBottomThreshold={10} // anything lower causes issues with followOutput + initialTopMostItemIndex={visibleMessages.length - 1} + /> +
{showScrollToBottom ? (
{ scrollToBottomSmooth() - setDidClickScrollToBottom(true) + disableAutoScrollRef.current = false }}> @@ -765,7 +689,7 @@ const ChatView = ({ isHidden, showAnnouncement, hideAnnouncement, showHistoryVie onSelectImages={selectImages} shouldDisableImages={shouldDisableImages} onHeightChange={() => { - if (isAtBottomRef.current) { + if (isAtBottom) { scrollToBottomAuto() } }}