Move thinking indicator inline into message row#45
Conversation
Replace the floating ThinkingIndicator overlay with an inline indicator rendered directly inside each streaming assistant message. Shows animated "Thinking..." dots with an elapsed seconds counter, then transitions to "Worked for Xs" when the run completes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 refactors the user interface to improve the display of the 'thinking' state during AI assistant responses. Instead of a static, globally positioned indicator, the system now provides a more contextual and integrated visual cue directly within the assistant's message stream, enhancing the user experience by clearly associating the thinking process with the relevant message. Highlights
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. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- InlineThinkingIndicator initializes
elapsedwithDate.now()during render, which can cause server/client hydration mismatches; consider initializing to 0 and computing the initial value in an effect instead. - The thinking timer logic is duplicated from the previous overlay-style ThinkingIndicator; it might be worth extracting shared timing logic into a small hook or utility to keep behavior consistent between indicators and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- InlineThinkingIndicator initializes `elapsed` with `Date.now()` during render, which can cause server/client hydration mismatches; consider initializing to 0 and computing the initial value in an effect instead.
- The thinking timer logic is duplicated from the previous overlay-style ThinkingIndicator; it might be worth extracting shared timing logic into a small hook or utility to keep behavior consistent between indicators and make future changes easier.
## Individual Comments
### Comment 1
<location path="components/MessageRow.tsx" line_range="434-440" />
<code_context>
return null;
}
+function InlineThinkingIndicator({ startTime }: { startTime?: number }) {
+ const [elapsed, setElapsed] = useState(() =>
+ startTime ? Math.floor((Date.now() - startTime) / 1000) : 0,
+ );
+
+ useEffect(() => {
+ if (!startTime) return;
+ const id = setInterval(() => {
</code_context>
<issue_to_address>
**suggestion:** Sync elapsed immediately when `startTime` becomes available or changes, not only on the next interval tick.
Because `elapsed` is only initialized from `startTime` on mount, if `startTime` is initially undefined and later set (or changed), the displayed value stays stale until the next 1s tick. To keep the UI in sync with prop changes, update `elapsed` immediately inside the effect before starting the interval:
```ts
useEffect(() => {
if (!startTime) return;
setElapsed(Math.floor((Date.now() - startTime) / 1000));
const id = setInterval(() => {
setElapsed(Math.floor((Date.now() - startTime) / 1000));
}, 1000);
return () => clearInterval(id);
}, [startTime]);
```
```suggestion
useEffect(() => {
if (!startTime) return;
// Sync immediately when startTime becomes available or changes
setElapsed(Math.floor((Date.now() - startTime) / 1000));
const id = setInterval(() => {
setElapsed(Math.floor((Date.now() - startTime) / 1000));
}, 1000);
return () => clearInterval(id);
}, [startTime]);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 successfully moves the thinking indicator to be inline with the message row, removing the floating overlay. The implementation of the new InlineThinkingIndicator is clean, but I've found a minor issue with how it handles state updates for the elapsed time, which could lead to a stale timer display in some edge cases. I've provided a suggestion to make it more robust. Otherwise, the changes look good and align with the goal of the PR.
| const [elapsed, setElapsed] = useState(() => | ||
| startTime ? Math.floor((Date.now() - startTime) / 1000) : 0, | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (!startTime) return; | ||
| const id = setInterval(() => { | ||
| setElapsed(Math.floor((Date.now() - startTime) / 1000)); | ||
| }, 1000); | ||
| return () => clearInterval(id); | ||
| }, [startTime]); |
There was a problem hiding this comment.
The current implementation of the InlineThinkingIndicator has a potential bug where the elapsed time could become stale or not reset correctly.
- If
startTimechanges from a defined value toundefined, theuseEffectwill clear the interval but won't resetelapsed, causing a stale timer value to remain on screen. - If
startTimechanges from one timestamp to another (less likely, but possible if the component is reused), theelapsedstate will not be updated immediately, showing a stale value for up to one second until the new interval fires.
A more robust implementation would use useEffect to handle all updates to startTime, ensuring the elapsed state is always in sync.
const [elapsed, setElapsed] = useState(0);
useEffect(() => {
if (!startTime) {
setElapsed(0);
return;
}
setElapsed(Math.floor((Date.now() - startTime) / 1000));
const timerId = setInterval(() => {
setElapsed(Math.floor((Date.now() - startTime) / 1000));
}, 1000);
return () => clearInterval(timerId);
}, [startTime]);
…indicator # Conflicts: # components/MessageRow.tsx # components/chat/ChatViewport.tsx
1 participant
Summary
ThinkingIndicatoroverlay that was absolutely positioned above the composer barInlineThinkingIndicatorcomponent rendered directly inside each streaming assistant message, showing animated "Thinking..." dots with an elapsed seconds counterTest plan