-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(explorer): share chat links #105376
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(explorer): share chat links #105376
Conversation
❌ 9 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
@cursoragent review |
|
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor |
| const parsedRunId = Number(paramValue); | ||
| if (!Number.isNaN(parsedRunId)) { | ||
| openExplorerPanel(); | ||
| setRunId(parsedRunId); |
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.
Zero or empty runId opens panel without loading data
When the URL contains ?explorerRunId=0 or ?explorerRunId= (empty value), Number() returns 0, which passes the !Number.isNaN(parsedRunId) check. This causes openExplorerPanel() to be called and runId to be set to 0. However, since !!0 evaluates to false, the API query's enabled condition (!!runId && !!orgSlug) is not satisfied, so no data is fetched. The result is the panel opens in an empty state with no error feedback to the user. Adding a validation for parsedRunId > 0 would prevent this confusing behavior.
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.
think this is ok, people shouldn't be making links w these values. I tested the behavior when run id doesn't exist (api fails) or api doesn't run - the panel just renders as blank. Can iterate on error states later (made a ticket)
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.
it's definitely possible for the url to contain invalid run ids, like if the link is old and our TTL cleaned up the run. So we should probably handle it nicely and not get the user stuck in a blank screen
|
requires https://github.com/getsentry/seer/pull/4345 to disable editing for other teammates. We can also do that as a followup. At this stage of the feature, I have no problem allowing all teammates to edit a chat, the only thing is we'll have undefined behavior when 2 people send chat requests at the same time. |
roaga
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.
should definitely follow up on the error state or address it now (shouldn't be that hard?)
I kinda wanna delete the copy chat markdown button lol but we don't have to
| const parsedRunId = Number(paramValue); | ||
| if (!Number.isNaN(parsedRunId)) { | ||
| openExplorerPanel(); | ||
| setRunId(parsedRunId); |
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.
it's definitely possible for the url to contain invalid run ids, like if the link is old and our TTL cleaned up the run. So we should probably handle it nicely and not get the user stuck in a blank screen
| const handleCopyLink = useCallback(async () => { | ||
| if (!runId) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const url = new URL(window.location.href); | ||
| url.searchParams.set(RUN_ID_QUERY_PARAM, String(runId)); | ||
| await navigator.clipboard.writeText(url.toString()); | ||
| addSuccessMessage('Copied link to current chat'); | ||
| } catch { | ||
| addErrorMessage('Failed to copy link to current chat'); | ||
| } | ||
| }, [runId]); |
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 also copies the url to the current page, not just the chat. but i guess that's a good thing?
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.
yup that was intended, so user can also share what else they're looking at
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.
sounds good we can follow up on the error state
Allows opening other chats from your org on any page, with the query param
?explorerRunId. Adds a top bar button to copy link to current chat.This is supported by moving the top-level
isOpenstate (toggled with cmd + /) to a global context, so child components can open/close the panel.Note on read-only (follow-up)
Other users' chats won't appear in session history and can only be reopened through a link when closed. At the moment all members with explorer access can edit/continue the chat. This is fine IMO but there are no guards for parallel chat requests (this is a problem for experimental autofix too). The quickest way to deal w this is to make chats read-only for everyone except the original user - handling parallel requests is a larger, separate scope
TODO: