Conversation
HenryHengZJ
commented
Mar 7, 2026
HenryHengZJ
commented
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the bot's user experience by adding a 'thinking message' feature. This allows the bot to communicate its internal processing state to the user in real-time, improving transparency and reducing perceived latency. The changes involve updating core message structures, implementing new UI components, and integrating event handling for the thinking process. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
this goes in after #347 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "thinking" message to improve the user experience by showing when the bot is processing. The implementation is generally well-done, with updates to the message type, state management, and a new UI component for the thinking indicator. I've identified a few areas for improvement to enhance code robustness and maintainability. Specifically, I've suggested making an event handler more resilient, simplifying some data handling logic, and refactoring a new component to use CSS classes instead of inline styles for better maintainability. Overall, this is a valuable feature addition.
| const handleThinkingEvent = (data: string, duration?: number) => { | ||
| if (data && duration === undefined) { | ||
| setIsThinking(true); | ||
| setMessages((prevMessages) => { | ||
| const lastMsg = prevMessages[prevMessages.length - 1]; | ||
| if (lastMsg.type === 'userMessage') return prevMessages; | ||
| const allMessages = [...prevMessages.slice(0, -1), { ...lastMsg, thinking: (lastMsg.thinking || '') + data, isThinking: true }]; | ||
| addChatMessage(allMessages); | ||
| return allMessages; | ||
| }); | ||
| } else if (data === '' && duration !== undefined) { | ||
| setIsThinking(false); | ||
| setMessages((prevMessages) => { | ||
| const lastMsg = prevMessages[prevMessages.length - 1]; | ||
| if (lastMsg.type === 'userMessage') return prevMessages; | ||
| const allMessages = [...prevMessages.slice(0, -1), { ...lastMsg, thinkingDuration: duration, isThinking: false }]; | ||
| addChatMessage(allMessages); | ||
| return allMessages; | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The logic in handleThinkingEvent seems a bit brittle. It relies on a strict contract where either data is provided for streaming, or an empty data string and a duration are provided for completion. If the backend sends a payload that doesn't match these exact conditions (e.g., data is undefined but duration is present), the event might be ignored. Consider making the logic more robust by prioritizing the duration check to signify the final event, which makes the handler more resilient to variations in the event payload.
const handleThinkingEvent = (data: string, duration?: number) => {
if (duration !== undefined) {
// This is the final thinking event, which includes the duration.
setIsThinking(false);
setMessages((prevMessages) => {
const lastMsg = prevMessages[prevMessages.length - 1];
if (lastMsg.type === 'userMessage') return prevMessages;
const thinkingText = data ? (lastMsg.thinking || '') + data : lastMsg.thinking;
const allMessages = [...prevMessages.slice(0, -1), { ...lastMsg, thinking: thinkingText, thinkingDuration: duration, isThinking: false }];
addChatMessage(allMessages);
return allMessages;
});
} else if (data) {
// This is a streaming thinking event.
setIsThinking(true);
setMessages((prevMessages) => {
const lastMsg = prevMessages[prevMessages.length - 1];
if (lastMsg.type === 'userMessage') return prevMessages;
const allMessages = [...prevMessages.slice(0, -1), { ...lastMsg, thinking: (lastMsg.thinking || '') + data, isThinking: true }];
addChatMessage(allMessages);
return allMessages;
});
}
};
| if ((message as any).reasonContent && typeof (message as any).reasonContent === 'object') { | ||
| chatHistory.thinking = (message as any).reasonContent.thinking; | ||
| chatHistory.thinkingDuration = (message as any).reasonContent.thinkingDuration; | ||
| } | ||
| if (message.thinking) chatHistory.thinking = message.thinking; | ||
| if (message.thinkingDuration !== undefined) chatHistory.thinkingDuration = message.thinkingDuration; |
There was a problem hiding this comment.
This logic for populating thinking and thinkingDuration from local storage is verbose and relies on casting to any, which reduces type safety. It appears to handle two different structures for the same data (reasonContent object vs. top-level properties), with the top-level properties taking precedence. This can be simplified for better readability and maintainability.
const reasonContent = (message as any).reasonContent;
chatHistory.thinking = message.thinking ?? reasonContent?.thinking;
if (message.thinkingDuration !== undefined) {
chatHistory.thinkingDuration = message.thinkingDuration;
} else {
chatHistory.thinkingDuration = reasonContent?.thinkingDuration;
}
| return ( | ||
| <div | ||
| style={{ | ||
| width: '100%', | ||
| 'border-radius': '8px', | ||
| border: '1px solid rgba(0, 0, 0, 0.12)', | ||
| background: props.backgroundColor || '#f7f8ff', | ||
| 'margin-bottom': '8px', | ||
| overflow: 'hidden', | ||
| }} | ||
| > | ||
| <button | ||
| type="button" | ||
| onClick={() => setIsExpanded(!isExpanded())} | ||
| style={{ | ||
| display: 'flex', | ||
| 'align-items': 'center', | ||
| width: '100%', | ||
| padding: '10px 12px', | ||
| cursor: 'pointer', | ||
| 'user-select': 'none', | ||
| gap: '8px', | ||
| 'font-size': '0.85rem', | ||
| 'font-family': 'inherit', | ||
| color: textColor(), | ||
| 'text-align': 'left', | ||
| border: 'none', | ||
| background: 'transparent', | ||
| }} | ||
| aria-expanded={isExpanded()} | ||
| > | ||
| <span style={{ display: 'flex', 'align-items': 'center', 'flex-shrink': '0', color: '#7b61ff' }}> | ||
| <Show when={props.isThinking} fallback={<BrainIcon />}> | ||
| <span class="thinking-spinner" style={{ display: 'flex', 'align-items': 'center' }}> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="#7b61ff" stroke-width="2.5"> | ||
| <circle cx="12" cy="12" r="10" opacity="0.25" /> | ||
| <path d="M12 2a10 10 0 0 1 10 10" stroke-linecap="round" /> | ||
| </svg> | ||
| </span> | ||
| </Show> | ||
| </span> | ||
| <span | ||
| style={{ | ||
| flex: '1', | ||
| 'font-weight': '500', | ||
| 'white-space': 'nowrap', | ||
| overflow: 'hidden', | ||
| 'text-overflow': 'ellipsis', | ||
| }} | ||
| > | ||
| {getHeaderText()} | ||
| </span> | ||
| <span | ||
| style={{ | ||
| display: 'flex', | ||
| 'align-items': 'center', | ||
| 'justify-content': 'center', | ||
| 'flex-shrink': '0', | ||
| transition: 'transform 0.2s ease', | ||
| transform: isExpanded() ? 'rotate(180deg)' : 'rotate(0deg)', | ||
| opacity: '0.5', | ||
| }} | ||
| aria-hidden="true" | ||
| > | ||
| <ChevronDownIcon /> | ||
| </span> | ||
| </button> | ||
| <Show when={isExpanded()}> | ||
| <div | ||
| style={{ | ||
| 'max-height': '300px', | ||
| 'overflow-y': 'auto', | ||
| 'overflow-x': 'hidden', | ||
| padding: '10px 12px', | ||
| 'border-top': '1px solid rgba(0, 0, 0, 0.08)', | ||
| background: 'rgba(0, 0, 0, 0.02)', | ||
| }} | ||
| > | ||
| <Show | ||
| when={parseThinkingContent(props.thinking).length > 0} | ||
| fallback={ | ||
| <p | ||
| style={{ | ||
| margin: '0', | ||
| 'font-style': 'italic', | ||
| color: textColor(), | ||
| opacity: '0.7', | ||
| 'white-space': 'pre-wrap', | ||
| 'word-break': 'break-word', | ||
| 'font-size': '0.85rem', | ||
| 'line-height': '1.5', | ||
| }} | ||
| > | ||
| {props.thinking} | ||
| </p> | ||
| } | ||
| > | ||
| <ul style={{ margin: '0', padding: '0 0 0 20px', 'list-style-type': 'disc' }}> | ||
| <For each={parseThinkingContent(props.thinking)}> | ||
| {(line) => ( | ||
| <li | ||
| style={{ | ||
| 'margin-bottom': '4px', | ||
| color: textColor(), | ||
| opacity: '0.7', | ||
| }} | ||
| > | ||
| <span | ||
| style={{ | ||
| 'font-style': 'italic', | ||
| 'font-size': '0.85rem', | ||
| 'line-height': '1.5', | ||
| 'word-break': 'break-word', | ||
| }} | ||
| > | ||
| {line} | ||
| </span> | ||
| </li> | ||
| )} | ||
| </For> | ||
| </ul> | ||
| </Show> | ||
| </div> | ||
| </Show> | ||
| <style> | ||
| {` | ||
| @keyframes thinking-spin { | ||
| to { transform: rotate(360deg); } | ||
| } | ||
| .thinking-spinner svg { | ||
| animation: thinking-spin 1s linear infinite; | ||
| } | ||
| `} | ||
| </style> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This new component makes heavy use of inline styles, which makes the JSX verbose and harder to maintain. It also misses out on the benefits of using CSS classes, such as better performance, reusability, and separation of concerns. Consider extracting these styles into CSS classes. For example, instead of <div style={{ width: '100%', 'border-radius': '8px', ... }}>, you could use utility classes like w-full rounded-lg border ..., which would make the component's structure much cleaner.
1 participant
