-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: Easy session failure reporting #4183
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
|
Thanks @ki3ani this is a good feature. However I don't think we should dependon a github token set in goosed -- rather, you can do this more simply by opening the system web browser at something like |
caa86fd to
2fd5de7
Compare
| payload.system_info.goose_version, | ||
| payload.timestamp | ||
| ); | ||
| } |
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.
Now that it's a pre-filled github URL, I think you can just remove the "backend" component. No need to have it just to log.
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.
Good point, i have removed the backend feedback endpoint since it was just logging.
| try { | ||
| // Get extension count from the API | ||
| const apiUrl = getApiUrl('/extension/list'); | ||
| const secretKey = getSecretKey(); |
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 know we do this in other places (we shouldn't) but we should avoid constructing the request like this an instead use the generated SDK:
goose/ui/desktop/src/api/sdk.gen.ts
Lines 111 to 116 in 99fa628
| export const getExtensions = <ThrowOnError extends boolean = false>(options?: Options<GetExtensionsData, ThrowOnError>) => { | |
| return (options?.client ?? _heyApiClient).get<GetExtensionsResponses, GetExtensionsErrors, ThrowOnError>({ | |
| url: '/config/extensions', | |
| ...options | |
| }); | |
| }; |
| // For now, collect from browser console errors and any stored logs | ||
| const recentErrors: string[] = []; | ||
|
|
||
| const storedErrors = localStorage.getItem('goose_recent_errors'); |
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 a thing? do we write to goose_recent_errors anywhere?
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.
You're right, that wasn't actually used anywhere. Removed it.
2fd5de7 to
68f4069
Compare
|
|
||
| const collectRecentErrors = useCallback(async () => { | ||
| try { | ||
| // Recent errors are logged to the main process via ErrorBoundary |
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'm still confused about the recentErrors bit - it looks like it's only ever set to an array of one constant string? what is this about main process error boundary?
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 reviewed the mechanism for collecting recent errors. To keep this PR strictly in scope and prevent introducing complex dependencies, I removed the entire collectRecentErrors function and state
| try { | ||
| const secretKey = getSecretKey(); | ||
| const response = await getExtensions({ | ||
| headers: secretKey ? { 'X-Secret-Key': secretKey } : {}, |
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.
you should not to include the secret key here, as it's globally configured for the generated client sdk
| Array.isArray(providerResponse.data) && | ||
| providerResponse.data.length > 0 | ||
| ) { | ||
| info.providerType = providerResponse.data[0].name || 'Unknown Provider'; |
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 don't think you want just the first provider.
The right thing to use here is probably the session metadata for the active session. #4648 adds that to the client API, so this might fit in better once that's merged
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 removed the old provider listing logic and refactored the code to use getSessionHistory from the merged #4648 to correctly retrieve the provider_name
68f4069 to
0a6927f
Compare
|
can you also do the DCO? and @jamadeo can you have another look? |
| @@ -0,0 +1,269 @@ | |||
| import { useState, useEffect, useCallback } from 'react'; | |||
| import { Button } from './ui/button'; | |||
| import { Textarea } from './ui/textarea'; | |||
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.
some of these don't exist:
./ui/textarea
./Modal
getSessionHistory
does this run for you?
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.
@jamadeo Yes it was running but some imports were broken which I hadn't noticed. Fixed now - replaced ./ui/textarea with native textarea, ./Modal with Dialog components, and
getSessionHistory with providers() SDK call. All working properly now.
|
also can you look into the DCO thing? |
bbe7a25 to
5bf7acc
Compare
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
Signed-off-by: ki3ani <[email protected]>
5bf7acc to
3fa79c1
Compare
Signed-off-by: ki3ani <[email protected]>
360b6cb to
7b83e17
Compare
- Remove session metadata fallback approach - Use providers() SDK call directly for reliable detection - Clean up unused imports Signed-off-by: Kenneth Mungai <[email protected]> Signed-off-by: ki3ani <[email protected]>
5fa67e5 to
f81a3b0
Compare
Done |
jamadeo
left a comment
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.
Much better! Only real issue is that it picks the wrong provider. Also consider the text truncation, if we must truncate because it's too much for the URL, we should limit the text area so the user doesn't unexpectedly lose what they wrote
| if (providersResponse.data && Array.isArray(providersResponse.data)) { | ||
| const providersList = providersResponse.data; | ||
| if (providersList.length > 0) { | ||
| info.providerType = providersList[0].name || 'Unknown'; |
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 isn't the right one to use. You can use useModelAndProvider to get the current model and provider
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.
er, looks like the extension count is also wrong. When I tried it, it shows 0 extensions installed, but I have several
|
|
||
| try { | ||
| const issueTitle = encodeURIComponent( | ||
| `[FAILURE REPORT] ${description.substring(0, 60)}${description.length > 60 ? '...' : ''}` |
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.
60 characters seems quite small for what someone might type into this box. If we're limited by url length, you could leave out the txt entry and have the user type in the opened github window. You'd still have the collected system info
| > | ||
| <AlertCircle size={16} /> | ||
| Report a Failure | ||
| </Button> |
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 see that this is now next to a "report a bug" button. I think your button is better because it collects system info, so I'd say go ahead and remove the other "report a bug" button so we don't have two very similar actions
|
closing this since there has been no activity on it. can always reopen |
Summary
Implements #3244 - adds "Report a Failure" button with automated system info collection and GitHub Issues integration.
Users can now report failures directly within the app instead of manually navigating to GitHub. The modal automatically collects system information and opens pre-filled GitHub
Issues in the browser.
Screenshots
Before: Settings Help & Feedback Section
After: New "Report a Failure" Button Added
Report Failure Modal - System Info Auto-Collection
Success State After Submission
GitHub Issue Pre-filled in Browser
Implementation Details
Technical Changes
Frontend (
ui/desktop/)ReportFailureModal.tsxwith system info collection using SDK providers APIAppSettingsSection.tsxto include the new "Report a Failure" buttonpreload.tsandmain.tsto support opening GitHub URLsTest Plan
Notes