-
Notifications
You must be signed in to change notification settings - Fork 249
[WIP] feat: @ mentions in AI prompt #3473
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
base: qa
Are you sure you want to change the base?
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jim/ai-empty-state #3473 +/- ##
===================================================
Coverage 89.76% 89.76%
===================================================
Files 437 437
Lines 90129 90129
===================================================
Hits 80905 80905
Misses 9224 9224 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
useEffect(() => { | ||
const handleClickOutside = (event: MouseEvent) => { | ||
if (mentionState.isOpen && textareaRef.current) { | ||
const target = event.target as Node; | ||
const isInTextarea = textareaRef.current.contains(target); | ||
const isInMentionsDropdown = document.querySelector('[data-mentions-dropdown]')?.contains(target); | ||
|
||
if (!isInTextarea && !isInMentionsDropdown) { | ||
setMentionState((prev) => ({ ...prev, isOpen: false })); | ||
} | ||
} | ||
}; | ||
|
||
if (mentionState.isOpen) { | ||
document.addEventListener('mousedown', handleClickOutside); | ||
return () => document.removeEventListener('mousedown', handleClickOutside); | ||
} | ||
}, [mentionState.isOpen, setMentionState, textareaRef]); |
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 cleanup function in this useEffect
is conditionally returned only when mentionState.isOpen
is true. This creates a potential memory leak if the component unmounts while the dropdown is closed. Consider always returning the cleanup function regardless of the dropdown state:
useEffect(() => {
const handleClickOutside = (event: MouseEvent) => {
if (mentionState.isOpen && textareaRef.current) {
// existing logic
}
};
document.addEventListener('mousedown', handleClickOutside);
return () => document.removeEventListener('mousedown', handleClickOutside);
}, [mentionState.isOpen, setMentionState, textareaRef]);
This ensures proper cleanup on component unmount in all cases.
useEffect(() => { | |
const handleClickOutside = (event: MouseEvent) => { | |
if (mentionState.isOpen && textareaRef.current) { | |
const target = event.target as Node; | |
const isInTextarea = textareaRef.current.contains(target); | |
const isInMentionsDropdown = document.querySelector('[data-mentions-dropdown]')?.contains(target); | |
if (!isInTextarea && !isInMentionsDropdown) { | |
setMentionState((prev) => ({ ...prev, isOpen: false })); | |
} | |
} | |
}; | |
if (mentionState.isOpen) { | |
document.addEventListener('mousedown', handleClickOutside); | |
return () => document.removeEventListener('mousedown', handleClickOutside); | |
} | |
}, [mentionState.isOpen, setMentionState, textareaRef]); | |
useEffect(() => { | |
const handleClickOutside = (event: MouseEvent) => { | |
if (mentionState.isOpen && textareaRef.current) { | |
const target = event.target as Node; | |
const isInTextarea = textareaRef.current.contains(target); | |
const isInMentionsDropdown = document.querySelector('[data-mentions-dropdown]')?.contains(target); | |
if (!isInTextarea && !isInMentionsDropdown) { | |
setMentionState((prev) => ({ ...prev, isOpen: false })); | |
} | |
} | |
}; | |
document.addEventListener('mousedown', handleClickOutside); | |
return () => document.removeEventListener('mousedown', handleClickOutside); | |
}, [mentionState.isOpen, setMentionState, textareaRef]); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
setTimeout(() => { | ||
const mention = detectMentionInText(newValue, cursorPos + 1); | ||
|
||
if (mention) { | ||
const position = getMentionCursorPosition(textarea); | ||
setMentionState((prev) => ({ | ||
...prev, | ||
isOpen: true, | ||
query: mention.query, | ||
startIndex: mention.startIndex, | ||
endIndex: mention.endIndex, | ||
position, | ||
selectedIndex: 0, | ||
})); | ||
} | ||
|
||
// Focus and position cursor after @ | ||
textarea.focus(); | ||
textarea.setSelectionRange(cursorPos + 1, cursorPos + 1); | ||
}, 0); |
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 setTimeout
approach for detecting mentions after inserting the @
character introduces potential race conditions with fast typing. Consider directly triggering mention detection with the known cursor position instead. Since you already have the exact position where the @
was inserted (cursorPos + 1
), you could construct the mention state immediately rather than relying on a timeout to detect it from the textarea value.
setTimeout(() => { | |
const mention = detectMentionInText(newValue, cursorPos + 1); | |
if (mention) { | |
const position = getMentionCursorPosition(textarea); | |
setMentionState((prev) => ({ | |
...prev, | |
isOpen: true, | |
query: mention.query, | |
startIndex: mention.startIndex, | |
endIndex: mention.endIndex, | |
position, | |
selectedIndex: 0, | |
})); | |
} | |
// Focus and position cursor after @ | |
textarea.focus(); | |
textarea.setSelectionRange(cursorPos + 1, cursorPos + 1); | |
}, 0); | |
// Directly detect mention without setTimeout | |
const mention = detectMentionInText(newValue, cursorPos + 1); | |
if (mention) { | |
const position = getMentionCursorPosition(textarea); | |
setMentionState((prev) => ({ | |
...prev, | |
isOpen: true, | |
query: mention.query, | |
startIndex: mention.startIndex, | |
endIndex: mention.endIndex, | |
position, | |
selectedIndex: 0, | |
})); | |
} | |
// Focus and position cursor after @ | |
textarea.focus(); | |
textarea.setSelectionRange(cursorPos + 1, cursorPos + 1); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | ||
// TODO: | ||
// We want to keep track of the last focused prompt input in the AI analyst and | ||
// when the user uses this action via the grid, we want to (open if closed) | ||
// insert/append a reference to the selection in the last focused prompt input. | ||
console.log('TODO(ayush): pass reference to AI analyst chat'); |
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 implementation for the StartChatInAIAnalyst
action contains a TODO comment and console log statement that need to be addressed before this feature can work properly. When users select "Reference in chat" from context menus, the current code won't actually pass the reference to the AI analyst chat. Consider completing this implementation by replacing the placeholder code with the actual logic to track the last focused prompt input and insert the reference into it when the action is triggered.
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | |
// TODO: | |
// We want to keep track of the last focused prompt input in the AI analyst and | |
// when the user uses this action via the grid, we want to (open if closed) | |
// insert/append a reference to the selection in the last focused prompt input. | |
console.log('TODO(ayush): pass reference to AI analyst chat'); | |
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | |
const { dispatch, getState } = store; | |
const state = getState(); | |
// Open AI Analyst panel if it's not already open | |
if (!state.ui.isAIAnalystOpen) { | |
dispatch(uiActions.openAIAnalyst()); | |
} | |
// Get the last focused prompt input or use the default one | |
const lastFocusedInput = state.aiAnalyst.lastFocusedPromptInput || 'mainPrompt'; | |
// Insert/append the reference to the selection in the prompt input | |
dispatch(aiAnalystActions.appendToPrompt({ | |
inputId: lastFocusedInput, | |
text: ` ${reference.text || reference.cellReference || ''}`, | |
})); | |
}, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
export function detectMentionInText(text: string, cursorPos: number) { | ||
const textBeforeCursor = text.substring(0, cursorPos); | ||
const atIndex = textBeforeCursor.lastIndexOf('@'); | ||
|
||
if (atIndex === -1) return null; | ||
|
||
// Check if there's a space after @ (not a mention) | ||
const textAfterAt = textBeforeCursor.substring(atIndex + 1); | ||
if (textAfterAt.includes(' ')) return null; | ||
|
||
// Check if we're still typing the mention (no space after @) | ||
const textAfterCursor = text.substring(cursorPos); | ||
const nextSpaceIndex = textAfterCursor.indexOf(' '); | ||
const endOfText = textAfterCursor.length === 0; | ||
|
||
if (nextSpaceIndex === 0 && !endOfText) return null; | ||
|
||
const query = textAfterAt; | ||
const startIndex = atIndex; | ||
const endIndex = cursorPos; | ||
|
||
return { | ||
query, | ||
startIndex, | ||
endIndex, | ||
}; |
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 logic in detectMentionInText
for determining if a user is still typing a mention appears problematic. The condition nextSpaceIndex === 0 && !endOfText
will return null
if there's a space immediately after the cursor position, even when the user might still be in the process of typing a mention.
Consider revising this check to handle edge cases more robustly. For example:
// Instead of checking for space at cursor position
if (nextSpaceIndex === 0 && !endOfText) return null;
// Consider a different approach that better handles cursor positions
// within partially typed mentions
This would improve the autocomplete experience when users are typing mentions with spaces nearby.
export function detectMentionInText(text: string, cursorPos: number) { | |
const textBeforeCursor = text.substring(0, cursorPos); | |
const atIndex = textBeforeCursor.lastIndexOf('@'); | |
if (atIndex === -1) return null; | |
// Check if there's a space after @ (not a mention) | |
const textAfterAt = textBeforeCursor.substring(atIndex + 1); | |
if (textAfterAt.includes(' ')) return null; | |
// Check if we're still typing the mention (no space after @) | |
const textAfterCursor = text.substring(cursorPos); | |
const nextSpaceIndex = textAfterCursor.indexOf(' '); | |
const endOfText = textAfterCursor.length === 0; | |
if (nextSpaceIndex === 0 && !endOfText) return null; | |
const query = textAfterAt; | |
const startIndex = atIndex; | |
const endIndex = cursorPos; | |
return { | |
query, | |
startIndex, | |
endIndex, | |
}; | |
export function detectMentionInText(text: string, cursorPos: number) { | |
const textBeforeCursor = text.substring(0, cursorPos); | |
const atIndex = textBeforeCursor.lastIndexOf('@'); | |
if (atIndex === -1) return null; | |
// Check if there's a space between @ and cursor (not a mention) | |
const textBetweenAtAndCursor = textBeforeCursor.substring(atIndex + 1); | |
if (textBetweenAtAndCursor.includes(' ')) return null; | |
// Check if we're at a valid position for a mention | |
// A mention is valid if it's at the start of text or has a space before it | |
if (atIndex > 0 && textBeforeCursor[atIndex - 1] !== ' ' && textBeforeCursor[atIndex - 1] !== '\n') return null; | |
const query = textBetweenAtAndCursor; | |
const startIndex = atIndex; | |
const endIndex = cursorPos; | |
return { | |
query, | |
startIndex, | |
endIndex, | |
}; | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
className="absolute z-50 w-80 rounded-md border bg-popover p-1 text-popover-foreground shadow-md" | ||
style={{ | ||
position: 'fixed', | ||
top: mentionState.position.top - 24, | ||
left: mentionState.position.left, | ||
transform: 'translateY(-100%)', | ||
pointerEvents: 'auto', | ||
}} |
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 dropdown positioning calculation doesn't account for textarea scrolling. When the textarea contains scrolled content, the dropdown will appear at an incorrect position relative to the mention. Consider adjusting the position calculation in getMentionCursorPosition()
to include textarea.scrollTop
to ensure the dropdown remains properly aligned with the cursor position regardless of scroll state.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
onChange={handlePromptChange} | ||
onKeyDown={handleKeyDown} | ||
autoComplete="off" | ||
placeholder={enableMentions ? 'Ask a question (type @ to reference data)…' : 'Ask a question…'} |
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 placeholder text no longer handles the waitingOnMessageIndex
condition that was present in the original code. To maintain this functionality while adding the new mentions feature, consider updating the placeholder logic:
placeholder={
waitingOnMessageIndex !== undefined
? 'Waiting to send message...'
: (enableMentions
? 'Ask a question (type @ to reference data)…'
: 'Ask a question…')
}
This preserves the waiting state message while incorporating the new mentions-related placeholder text.
placeholder={enableMentions ? 'Ask a question (type @ to reference data)…' : 'Ask a question…'} | |
placeholder={ | |
waitingOnMessageIndex !== undefined | |
? 'Waiting to send message...' | |
: (enableMentions | |
? 'Ask a question (type @ to reference data)…' | |
: 'Ask a question…') | |
} |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | ||
// TODO: | ||
// We want to keep track of the last focused prompt input in the AI analyst and | ||
// when the user uses this action via the grid, we want to (open if closed) | ||
// insert/append a reference to the selection in the last focused prompt input. | ||
console.log('TODO(ayush): pass reference to AI analyst chat'); | ||
if (!pixiAppSettings.setAIAnalystState) return; | ||
pixiAppSettings.setAIAnalystState((prev) => { | ||
const newState = { | ||
...prev, | ||
showAIAnalyst: true, | ||
initialPrompt: reference ? `@${reference} ` : '', | ||
currentChat: { id: '', name: '', lastUpdated: Date.now(), messages: [] }, | ||
}; | ||
return newState; | ||
}); |
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 implementation doesn't match the TODO comment's intent. While the code opens the AI analyst and creates a new chat with the reference, it doesn't track the last focused prompt or append to an existing prompt if the analyst is already open. This means users could lose their in-progress work when using this feature.
Consider implementing the described behavior where:
- If AI analyst is closed: open it and insert the reference
- If AI analyst is open: append the reference to the last focused prompt instead of creating a new chat
This would provide a more seamless experience and prevent potential data loss when users reference items from the grid.
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | |
// TODO: | |
// We want to keep track of the last focused prompt input in the AI analyst and | |
// when the user uses this action via the grid, we want to (open if closed) | |
// insert/append a reference to the selection in the last focused prompt input. | |
console.log('TODO(ayush): pass reference to AI analyst chat'); | |
if (!pixiAppSettings.setAIAnalystState) return; | |
pixiAppSettings.setAIAnalystState((prev) => { | |
const newState = { | |
...prev, | |
showAIAnalyst: true, | |
initialPrompt: reference ? `@${reference} ` : '', | |
currentChat: { id: '', name: '', lastUpdated: Date.now(), messages: [] }, | |
}; | |
return newState; | |
}); | |
run: (reference: ViewActionArgs[Action.StartChatInAIAnalyst]) => { | |
if (!pixiAppSettings.setAIAnalystState) return; | |
pixiAppSettings.setAIAnalystState((prev) => { | |
// If AI Analyst is already open and there's an existing chat, append to it | |
if (prev.showAIAnalyst && prev.currentChat?.id) { | |
return { | |
...prev, | |
// Append the reference to the existing prompt if there is one | |
appendToPrompt: reference ? `@${reference} ` : '', | |
}; | |
} | |
// Otherwise, open the AI Analyst and start a new chat with the reference | |
return { | |
...prev, | |
showAIAnalyst: true, | |
initialPrompt: reference ? `@${reference} ` : '', | |
currentChat: { id: '', name: '', lastUpdated: Date.now(), messages: [] }, | |
}; | |
}); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
import { DropdownMenuSeparator } from '@/shared/shadcn/ui/dropdown-menu'; | ||
|
||
export function GridContextMenuCodeTableColumn() { | ||
const { cursorString } = useCursorPosition(); |
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.
This component uses cursorString
while similar components use cursorStringWithSheetName
. This inconsistency means references in chat will lack sheet name context. Consider using cursorStringWithSheetName
here for consistency with other components and to ensure complete references.
const { cursorString } = useCursorPosition(); | |
const { cursorStringWithSheetName } = useCursorPosition(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Description
@
button picker in AI prompt box that, when clicked, inserts a@
character in your prompt and triggers showing the autocomplete@
in the AI prompt which triggers showing an autocompleteScreenshots
Prompt with popup:
Context menu on grid:
Prompt with styled @-mention (color class names:
text-primary bg-border
)Todos
@
followed by a string of text, they're likely referring to some data in the file, like a named code cell, table, sheet, or even just a range like@A1:A5
"To Test
Grid context menus:
@Python1
@Table1
F17
Sheet1!A1:A5
@Postgres1[Agent_Rating]
AI prompt:
@
button inserts a@
into the current prompt box and triggers showing the autocomplete box@
triggers showing autocomplete box