-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent countdown timer from showing in history for answered follow-up questions #7625
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.
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
Conversation
…low-up questions - Check if follow-up question has been answered by looking for subsequent messages - For current session, use existing currentFollowUpTs check - For history, check if there are messages after the follow-up question - Fixes #7624
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code 2 minutes ago and already found 5 ways I could've done it better.
| // Check if this follow-up question has been answered | ||
| // For current session: check if it matches currentFollowUpTs | ||
| // For history: check if there's a message after this follow-up question | ||
| const isFollowUpAnswered = (() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice this logic is duplicated from what appears around line 1521. Could we extract this into a reusable function to follow DRY principles? Something like:
| } | ||
| // For historical messages, check if there's a message after this follow-up | ||
| // If there is, it means the follow-up was answered | ||
| const messageIndex = modifiedMessages.findIndex((msg) => msg.ts === messageOrGroup.ts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using on for every render could impact performance with large message lists. Have you considered memoizing this check or perhaps storing the answered state in a more efficient data structure?
| // If there is, it means the follow-up was answered | ||
| const messageIndex = modifiedMessages.findIndex((msg) => msg.ts === messageOrGroup.ts) | ||
| if (messageIndex !== -1 && messageIndex < modifiedMessages.length - 1) { | ||
| // There's at least one message after this follow-up, so it was answered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic assumes any message after a follow-up means it was answered. What if there's an error message or system message immediately after? Should we verify the next message is actually a user response or an action that indicates the follow-up was handled?
|
|
||
| // regular message | ||
| // Check if this follow-up question has been answered | ||
| // For current session: check if it matches currentFollowUpTs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are helpful but could be more concise. Consider combining them:
Description
This PR fixes an issue where the countdown timer would incorrectly reappear when viewing task history, even after a follow-up suggestion had been selected.
Problem
When users:
The countdown timer would incorrectly show again in the history view, even though the suggestion had already been selected.
Solution
The fix adds logic to properly determine if a follow-up question has been answered:
currentFollowUpTscheckTesting
Related Issue
Fixes #7624
Review Confidence
The implementation review showed 95% confidence with no security issues or code quality concerns.
Important
Fixes countdown timer reappearing in task history for answered follow-up questions by adding logic in
ChatView.tsx.ChatView.tsxto check if a follow-up question is answered by verifyingcurrentFollowUpTsfor current sessions and checking subsequent messages for historical ones.This description was created by
for 4fc9982. You can customize this summary. It will automatically update as commits are pushed.