Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions static/app/components/modals/explore/saveQueryModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export type SaveQueryModalProps = {
saveQuery: (name: string, starred?: boolean) => Promise<SavedQuery>;
traceItemDataset: TraceItemDataset;
name?: string;
source?: 'toolbar' | 'table';
source?: 'toolbar' | 'table' | 'conversations';
};

type Props = ModalRenderProps & SaveQueryModalProps;
Expand Down Expand Up @@ -59,7 +59,14 @@ function SaveQueryModal({
}
addSuccessMessage(t('Query saved successfully'));
if (defined(source)) {
if (traceItemDataset === TraceItemDataset.LOGS) {
if (source === 'conversations') {
trackAnalytics('conversations.save_query_modal', {
action: 'submit',
save_type: initialName === undefined ? 'save_new_query' : 'rename_query',
ui_source: 'table',
organization,
});
} else if (traceItemDataset === TraceItemDataset.LOGS) {
trackAnalytics('logs.save_query_modal', {
action: 'submit',
save_type: initialName === undefined ? 'save_new_query' : 'rename_query',
Expand Down
6 changes: 6 additions & 0 deletions static/app/utils/analytics/conversationsAnalyticsEvents.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ export type ConversationsEventParameters = {
'conversations.message.click-tool-call': Record<string, unknown>;
'conversations.onboarding.page-view': Record<string, unknown>;
'conversations.page-view': Record<string, unknown>;
'conversations.save_query_modal': {
action: 'open' | 'submit';
save_type?: 'save_new_query' | 'rename_query';
ui_source?: 'table';
};
'conversations.table.page-view': Record<string, unknown>;
'conversations.table.paginate': {
direction: 'next' | 'previous';
Expand All @@ -23,6 +28,7 @@ export type ConversationsEventParameters = {
export const conversationsEventMap: Record<keyof ConversationsEventParameters, string> = {
'conversations.onboarding.page-view': 'Conversations: Onboarding Page View',
'conversations.page-view': 'Conversations: Page View',
'conversations.save_query_modal': 'Conversations: Save Query Modal',
'conversations.table.page-view': 'Conversations: Table Page View',
'conversations.table.paginate': 'Conversations: Table Paginate',
'conversations.detail.expand-thinking': 'Conversations: Detail Expand Thinking',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,94 +1,15 @@
import {Fragment, useState} from 'react';
import * as Sentry from '@sentry/react';
import {parseAsString, useQueryState} from 'nuqs';
import {parseAsArrayOf, parseAsString, useQueryState, useQueryStates} from 'nuqs';

import {Button} from '@sentry/scraps/button';
import {Input} from '@sentry/scraps/input';
import {Container, Flex} from '@sentry/scraps/layout';
import {Switch} from '@sentry/scraps/switch';

import {
addErrorMessage,
addLoadingMessage,
addSuccessMessage,
} from 'sentry/actionCreators/indicator';
import {type ModalRenderProps, openModal} from 'sentry/actionCreators/modal';
import {openSaveQueryModal} from 'sentry/actionCreators/modal';
import {usePageFilters} from 'sentry/components/pageFilters/usePageFilters';
import {t} from 'sentry/locale';
import {trackAnalytics} from 'sentry/utils/analytics';
import {useApi} from 'sentry/utils/useApi';
import {useOrganization} from 'sentry/utils/useOrganization';
import {Mode} from 'sentry/views/explore/contexts/pageParamsContext/mode';
import {useInvalidateSavedQueries} from 'sentry/views/explore/hooks/useGetSavedQueries';

type SaveModalProps = ModalRenderProps & {
onSave: (name: string, starred: boolean) => Promise<void>;
};

function SaveConversationQueryModal({
Header,
Body,
Footer,
closeModal,
onSave,
}: SaveModalProps) {
const [name, setName] = useState('');
const [starred, setStarred] = useState(true);
const [isSaving, setIsSaving] = useState(false);

async function handleSave() {
try {
setIsSaving(true);
addLoadingMessage(t('Saving query...'));
await onSave(name, starred);
addSuccessMessage(t('Query saved successfully'));
closeModal();
} catch (error) {
addErrorMessage(t('Failed to save query'));
Sentry.captureException(error);
} finally {
setIsSaving(false);
}
}

return (
<Fragment>
<Header closeButton>
<h4>{t('New Query')}</h4>
</Header>
<Body>
<Container marginBottom="xl">
<h6>{t('Name')}</h6>
<Input
autoFocus
placeholder={t('Enter a name for your new query')}
onChange={e => setName(e.target.value)}
value={name}
title={t('Enter a name for your new query')}
/>
</Container>
<Flex gap="md" align="center">
<Switch
checked={starred}
onChange={() => setStarred(!starred)}
title={t('Starred')}
/>
<h6>{t('Starred')}</h6>
</Flex>
</Body>
<Footer>
<Flex gap="lg" align="center" justify="end">
<Button onClick={closeModal} disabled={isSaving}>
{t('Cancel')}
</Button>
<Button onClick={handleSave} disabled={!name || isSaving} variant="primary">
{t('Create a New Query')}
</Button>
</Flex>
</Footer>
</Fragment>
);
}
import {TraceItemDataset} from 'sentry/views/explore/types';

export function SaveConversationQueryButton() {
const api = useApi();
Expand All @@ -99,19 +20,21 @@ export function SaveConversationQueryButton() {
'query',
parseAsString.withOptions({history: 'replace'})
);
const [{agent}] = useQueryStates(
{agent: parseAsArrayOf(parseAsString)},
{history: 'replace'}
);

function handleClick() {
trackAnalytics('conversations.save_query_modal', {
action: 'open',
openSaveQueryModal({
organization,
});

openModal(modalProps => (
<SaveConversationQueryModal
{...modalProps}
onSave={async (name, starred) => {
const {datetime, projects, environments} = selection;
await api.requestPromise(`/organizations/${organization.slug}/explore/saved/`, {
source: 'conversations',
traceItemDataset: TraceItemDataset.SPANS,
saveQuery: async (name, starred) => {
const {datetime, projects, environments} = selection;
const response = await api.requestPromise(
`/organizations/${organization.slug}/explore/saved/`,
{
method: 'POST',
data: {
name,
Expand All @@ -121,7 +44,8 @@ export function SaveConversationQueryButton() {
range: datetime.period ?? undefined,
start: datetime.start ?? undefined,
end: datetime.end ?? undefined,
starred,
starred: starred ?? true,
agent: agent?.length ? agent : undefined,
query: [
{
fields: [],
Expand All @@ -130,16 +54,12 @@ export function SaveConversationQueryButton() {
},
],
},
});
invalidateSavedQueries();

trackAnalytics('conversations.save_query_modal', {
action: 'submit',
organization,
});
}}
/>
));
}
);
invalidateSavedQueries();
return response;
},
});
}

return (
Expand Down
40 changes: 36 additions & 4 deletions static/app/views/explore/conversations/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ import {Tooltip} from '@sentry/scraps/tooltip';

import Feature from 'sentry/components/acl/feature';
import {AnalyticsArea} from 'sentry/components/analyticsArea';
import {Breadcrumbs} from 'sentry/components/breadcrumbs';
import {type Crumb, Breadcrumbs} from 'sentry/components/breadcrumbs';
import {FeedbackButton} from 'sentry/components/feedbackButton/feedbackButton';
import {NoAccess} from 'sentry/components/noAccess';
import {NoProjectMessage} from 'sentry/components/noProjectMessage';
import {PageFiltersContainer} from 'sentry/components/pageFilters/container';
import {SentryDocumentTitle} from 'sentry/components/sentryDocumentTitle';
import {defined} from 'sentry/utils/defined';
import {decodeScalar} from 'sentry/utils/queryString';
import {isUUID} from 'sentry/utils/string/isUUID';
import {normalizeUrl} from 'sentry/utils/url/normalizeUrl';
import {useLocation} from 'sentry/utils/useLocation';
Expand Down Expand Up @@ -127,9 +129,7 @@ function ConversationsHeader() {
]}
/>
) : (
<Fragment>
{CONVERSATIONS_LANDING_TITLE} <FeatureBadge type="beta" />
</Fragment>
<ConversationsLandingTitle />
)}
</TopBar.Slot>
<TopBar.Slot name="feedback">
Expand Down Expand Up @@ -165,4 +165,36 @@ function useRestoredListQuery(
: undefined;
}

function ConversationsLandingTitle() {
const organization = useOrganization();
const location = useLocation();
const savedQueryTitle = decodeScalar(location.query.title);
const savedQueryId = decodeScalar(location.query.id);

if (defined(savedQueryId) && defined(savedQueryTitle) && savedQueryTitle.length > 0) {
Comment on lines +172 to +174

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The breadcrumb for a saved conversation query is rendered based on URL parameters (savedQueryId, savedQueryTitle) without validating that the query actually exists on the server.
Severity: LOW

Suggested Fix

To align with the pattern used in logs and traces, the component should use the useGetSavedQuery(savedQueryId) hook. The breadcrumb should only be rendered if the hook returns a valid savedQuery object from the API, and the breadcrumb title should be sourced from savedQuery.name instead of the URL parameter.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/views/explore/conversations/layout.tsx#L172-L174

Potential issue: The code in `ConversationsLandingTitle` renders a breadcrumb based on
`savedQueryId` and `savedQueryTitle` from the URL query parameters. However, it only
checks for the presence of these parameters (`defined(savedQueryId)`) and not their
validity. This allows for the creation of URLs with arbitrary `id` and `title` values,
which will render a fake breadcrumb for a non-existent saved query. This behavior is
inconsistent with parallel implementations for logs and traces, which validate the ID
against the server using `useGetSavedQuery` before rendering the breadcrumb. While not a
security risk, it's incorrect UI behavior.

Did we get this right? 👍 / 👎 to inform future reviews.

const conversationsBaseUrl = normalizeUrl(
`/organizations/${organization.slug}/explore/${CONVERSATIONS_LANDING_SUB_PATH}/`
);
const crumbs: Crumb[] = [
{
label: CONVERSATIONS_SIDEBAR_LABEL,
to: {
pathname: conversationsBaseUrl,
query: {statsPeriod: '24h'},
},
},
{
label: savedQueryTitle,
},
];
return <Breadcrumbs crumbs={crumbs} />;
}

return (
<Fragment>
{CONVERSATIONS_LANDING_TITLE} <FeatureBadge type="beta" />
</Fragment>
);
}

export default ConversationsLayout;
3 changes: 3 additions & 0 deletions static/app/views/explore/hooks/useGetSavedQueries.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export type ReadableSavedQuery = {
projects: number[];
query: [ReadableQuery, ...ReadableQuery[]];
starred: boolean;
agent?: string[];
caseInsensitive?: CaseInsensitive;
changedReason?: ExploreQueryChangedReason | null;
createdBy?: User;
Expand All @@ -145,6 +146,7 @@ export class SavedQuery {
query: [SavedQueryQuery, ...SavedQueryQuery[]];
dataset: ReadableSavedQuery['dataset'];
starred: boolean;
agent?: string[];
changedReason?: ExploreQueryChangedReason | null;
crossEvents?: CrossEvent[];
createdBy?: User;
Expand All @@ -155,6 +157,7 @@ export class SavedQuery {
start?: string | DateString;

constructor(savedQuery: ReadableSavedQuery) {
this.agent = savedQuery.agent;
this.changedReason = savedQuery.changedReason;
this.crossEvents = savedQuery.crossEvents;
this.dateAdded = savedQuery.dateAdded;
Expand Down
5 changes: 4 additions & 1 deletion static/app/views/explore/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,10 @@ function getConversationsUrlFromSavedQueryUrl({
title: savedQuery.name,
};

const queryString = qs.stringify(queryParams, {skipNull: true});
let queryString = qs.stringify(queryParams, {skipNull: true});
if (savedQuery.agent?.length) {
queryString += `&agent=${savedQuery.agent.join(',')}`;
}
const basePath = normalizeUrl(
`/organizations/${organization.slug}/explore/${CONVERSATIONS_LANDING_SUB_PATH}/`
);
Expand Down
Loading