-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(issue-details): Add universal copy hotkey #87325
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds a universal copy hotkey (cmd/ctrl+alt+c) to copy issue details as Markdown for LLM prompts. Key changes include the implementation of the new hook (useCopyIssueDetails) in the issue details view, accompanying tests to validate the functionality, and minor updates to autofix utility functions and group summary interfaces to support the feature.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
static/app/views/issueDetails/streamline/hooks/useCopyIssueDetails.tsx | Adds a new hook to copy issue details as Markdown using a hotkey |
static/app/views/issueDetails/streamline/hooks/useCopyIssueDetails.spec.tsx | Implements tests for the copy functionality and Markdown formatting |
static/app/components/events/autofix/utils.tsx | Updates helper functions for formatting autofix-related texts |
static/app/components/group/groupSummary.tsx | Exports GroupSummaryData and adds a corresponding hook for summary data retrieval |
static/app/components/events/autofix/autofixSolution.tsx | Enhances the CopyToClipboardButton with a title for better accessibility |
static/app/views/issueDetails/groupEventDetails/groupEventDetailsContent.tsx | Integrates the new useCopyIssueDetails hook into the issue details view |
static/app/components/group/groupSummaryWithAutofix.tsx | Removes redundant helper functions in favor of using centralized utility functions |
Comments suppressed due to low confidence (1)
static/app/views/issueDetails/streamline/hooks/useCopyIssueDetails.tsx:145
- Consider checking for the existence of navigator.clipboard before calling writeText to safeguard against runtime errors in environments where the Clipboard API may be unsupported or unavailable.
navigator.clipboard.writeText(text)
|
||
const {data, isPending} = useApiQuery<GroupSummaryData>(queryKey, { | ||
staleTime: Infinity, | ||
enabled: false, |
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 think this should stop it. Query can still run if refetch
is used anywhere, but I think we're good?
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.
tested locally... it doesn't call it
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.
just to check my understanding - this is being used to get the group summary if it exists already, but won't create a new summary if one doesn't exist (so we don't need any checks around it)
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.
yep, that is the intent and my understanding of how this hook works, other than just not rendering that hook, if we disable it via a check it'll be passing a false
to enabled
anyways, but this is always false
static/app/views/issueDetails/streamline/hooks/useCopyIssueDetails.tsx
Outdated
Show resolved
Hide resolved
); | ||
|
||
// Mock the internal function for our tests | ||
const mockIssueAndEventToMarkdown = jest.fn( |
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.
does this need to be mocked like this? Doesn't make sense to reimplement the entire thing
const issueAndEventToMarkdown = jest.requireMock( | ||
'sentry/views/issueDetails/streamline/hooks/useCopyIssueDetails' | ||
).__mocks__.issueAndEventToMarkdown; |
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 should be an import at the top of the document and use jest.mocked(issueAndEventToMarkdown) if you need the types
}); | ||
|
||
// Get access to the mocked useCopyToClipboard | ||
const useCopyToClipboard = jest.requireMock('sentry/utils/useCopyToClipboard').default; |
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.
same here, import normally
}); | ||
|
||
it('formats basic issue information correctly', () => { | ||
issueAndEventToMarkdown(group, event, null, null); |
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.
is this just testing the mock above? somewhat confused
|
||
const text = useMemo(() => { | ||
if (!event) { | ||
addErrorMessage(t('Could not copy issue to clipboard')); |
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 should be in its own useEffect if you want to display an error
As per discussed, we're adding a hotkey
cmd+option+c
/ctrl+alt+c
to copy the issue & relevant content as markdown for LLM prompts.There's no discoverable button right now, we will follow up on that, but right now we'll put it behind a hotkey that's just commonly used for AI use cases
Includes Issue summary & autofix when present
Example of a copy: