From f7e5bb9af06c89b27a793e320dfe67b9a7a4225e Mon Sep 17 00:00:00 2001 From: Elias Hussary Date: Tue, 25 Apr 2023 17:57:33 -0400 Subject: [PATCH 1/2] feat(replays): fetch replay data without projectSlug --- .../replays/hooks/useReplayData.spec.tsx | 48 ++++++++++++++++- .../app/utils/replays/hooks/useReplayData.tsx | 51 +++++++++++++++++-- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/static/app/utils/replays/hooks/useReplayData.spec.tsx b/static/app/utils/replays/hooks/useReplayData.spec.tsx index 67a4e08fdaf957..41345bb22b1cb8 100644 --- a/static/app/utils/replays/hooks/useReplayData.spec.tsx +++ b/static/app/utils/replays/hooks/useReplayData.spec.tsx @@ -5,19 +5,35 @@ import {reactHooks} from 'sentry-test/reactTestingLibrary'; import useReplayData from 'sentry/utils/replays/hooks/useReplayData'; import ReplayReader from 'sentry/utils/replays/replayReader'; +import useProjects from 'sentry/utils/useProjects'; import type {ReplayRecord} from 'sentry/views/replays/types'; jest.useFakeTimers(); jest.spyOn(ReplayReader, 'factory'); +jest.mock('sentry/utils/useProjects'); const {organization, project} = initializeOrg(); +const mockUseProjects = useProjects as jest.MockedFunction; +mockUseProjects.mockReturnValue({ + fetching: false, + projects: [project], + fetchError: null, + hasMore: false, + initiallyLoaded: true, + onSearch: () => Promise.resolve(), + placeholders: [], +}); + const MockedReplayReaderFactory = ReplayReader.factory as jest.MockedFunction< typeof ReplayReader.factory >; function getMockReplayRecord(replayRecord?: Partial) { - const HYDRATED_REPLAY = TestStubs.ReplayRecord(replayRecord); + const HYDRATED_REPLAY = TestStubs.ReplayRecord({ + ...replayRecord, + project_id: project.id, + }); const RAW_REPLAY = { ...HYDRATED_REPLAY, duration: HYDRATED_REPLAY.duration.asSeconds(), @@ -292,4 +308,34 @@ describe('useReplayData', () => { errors: mockErrorResponse, }); }); + + it('should handle a replaySlug without projectSlug', async () => { + const {mockReplayResponse, expectedReplay} = getMockReplayRecord({ + count_errors: 0, + count_segments: 0, + error_ids: [], + }); + + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/replays/${mockReplayResponse.id}/`, + body: {data: mockReplayResponse}, + }); + + const {result, waitForNextUpdate} = reactHooks.renderHook(useReplayData, { + initialProps: { + replaySlug: mockReplayResponse.id, + orgSlug: organization.slug, + }, + }); + + await waitForNextUpdate(); + + expect(result.current).toEqual({ + fetchError: undefined, + fetching: false, + onRetry: expect.any(Function), + replay: expect.any(ReplayReader), + replayRecord: expectedReplay, + }); + }); }); diff --git a/static/app/utils/replays/hooks/useReplayData.tsx b/static/app/utils/replays/hooks/useReplayData.tsx index 01d6362b51fde9..e5a23565cf8ac5 100644 --- a/static/app/utils/replays/hooks/useReplayData.tsx +++ b/static/app/utils/replays/hooks/useReplayData.tsx @@ -6,6 +6,7 @@ import {mapResponseToReplayRecord} from 'sentry/utils/replays/replayDataUtils'; import ReplayReader from 'sentry/utils/replays/replayReader'; import RequestError from 'sentry/utils/requestError/requestError'; import useApi from 'sentry/utils/useApi'; +import useProjects from 'sentry/utils/useProjects'; import type {ReplayError, ReplayRecord} from 'sentry/views/replays/types'; type State = { @@ -94,7 +95,8 @@ function useReplayData({ errorsPerPage = 50, segmentsPerPage = 100, }: Options): Result { - const [projectSlug, replayId] = replaySlug.split(':'); + const {projectSlug, replayId} = parseReplaySlug(replaySlug); + const projects = useProjects(); const api = useApi(); @@ -106,7 +108,7 @@ function useReplayData({ // Fetch every field of the replay. We're overfetching, not every field is used const fetchReplay = useCallback(async () => { const response = await api.requestPromise( - `/projects/${orgSlug}/${projectSlug}/replays/${replayId}/` + makeFetchReplayApiUrl(orgSlug, projectSlug, replayId) ); const mappedRecord = mapResponseToReplayRecord(response.data); setReplayRecord(mappedRecord); @@ -117,6 +119,13 @@ function useReplayData({ if (!replayRecord) { return; } + const replayRecordProjectSlug = projects.projects.find( + p => p.id === replayRecord.project_id + )?.slug; + + if (!replayRecordProjectSlug) { + return; + } if (!replayRecord.count_segments) { setState(prev => ({...prev, fetchingAttachments: false})); @@ -131,7 +140,7 @@ function useReplayData({ await Promise.allSettled( cursors.map(cursor => { const promise = api.requestPromise( - `/projects/${orgSlug}/${projectSlug}/replays/${replayRecord.id}/recording-segments/`, + `/projects/${orgSlug}/${replayRecordProjectSlug}/replays/${replayRecord.id}/recording-segments/`, { query: { download: true, @@ -147,7 +156,7 @@ function useReplayData({ }) ); setState(prev => ({...prev, fetchingAttachments: false})); - }, [segmentsPerPage, api, orgSlug, projectSlug, replayRecord]); + }, [segmentsPerPage, api, orgSlug, replayRecord, projects.projects]); const fetchErrors = useCallback(async () => { if (!replayRecord) { @@ -233,4 +242,38 @@ function useReplayData({ }; } +// see https://github.com/getsentry/sentry/pull/47859 +// replays can apply to many projects when incorporating backend errors +// this makes having project in the `replaySlug` obsolete +// we must keep this url schema for now for backward compat but we should remove it at some point +// TODO: remove support for projectSlug in replay url? +function parseReplaySlug(replaySlug: string) { + const maybeProjectSlugAndReplayId = replaySlug.split(':'); + if (maybeProjectSlugAndReplayId.length === 2) { + const [projectSlug, replayId] = maybeProjectSlugAndReplayId; + return { + projectSlug, + replayId, + }; + } + + // if there is no projectSlug then we assume we just have the replayId + // all other cases would be a malformed url + return { + projectSlug: null, + replayId: maybeProjectSlugAndReplayId[0], + }; +} + +function makeFetchReplayApiUrl( + orgSlug: string, + projectSlug: string | null, + replayId: string +) { + if (projectSlug) { + return `/projects/${orgSlug}/${projectSlug}/replays/${replayId}/`; + } + return `/organizations/${orgSlug}/replays/${replayId}/`; +} + export default useReplayData; From c3569d69a3bedc5b33c128ca689f18af505ec435 Mon Sep 17 00:00:00 2001 From: Elias Hussary Date: Tue, 25 Apr 2023 18:57:58 -0400 Subject: [PATCH 2/2] ignore projectSlug completely --- .../replays/hooks/useReplayData.spec.tsx | 8 +-- .../app/utils/replays/hooks/useReplayData.tsx | 50 +++++++------------ 2 files changed, 21 insertions(+), 37 deletions(-) diff --git a/static/app/utils/replays/hooks/useReplayData.spec.tsx b/static/app/utils/replays/hooks/useReplayData.spec.tsx index 41345bb22b1cb8..55570bb95e744a 100644 --- a/static/app/utils/replays/hooks/useReplayData.spec.tsx +++ b/static/app/utils/replays/hooks/useReplayData.spec.tsx @@ -61,7 +61,7 @@ describe('useReplayData', () => { }); MockApiClient.addMockResponse({ - url: `/projects/${organization.slug}/${project.slug}/replays/${mockReplayResponse.id}/`, + url: `/organizations/${organization.slug}/replays/${mockReplayResponse.id}/`, body: {data: mockReplayResponse}, }); @@ -103,7 +103,7 @@ describe('useReplayData', () => { ]; MockApiClient.addMockResponse({ - url: `/projects/${organization.slug}/${project.slug}/replays/${mockReplayResponse.id}/`, + url: `/organizations/${organization.slug}/replays/${mockReplayResponse.id}/`, body: {data: mockReplayResponse}, }); const mockedSegmentsCall1 = MockApiClient.addMockResponse({ @@ -171,7 +171,7 @@ describe('useReplayData', () => { ]; MockApiClient.addMockResponse({ - url: `/projects/${organization.slug}/${project.slug}/replays/${mockReplayResponse.id}/`, + url: `/organizations/${organization.slug}/replays/${mockReplayResponse.id}/`, body: {data: mockReplayResponse}, }); const mockedErrorsCall1 = MockApiClient.addMockResponse({ @@ -230,7 +230,7 @@ describe('useReplayData', () => { const mockedReplayCall = MockApiClient.addMockResponse({ asyncDelay: 1, - url: `/projects/${organization.slug}/${project.slug}/replays/${mockReplayResponse.id}/`, + url: `/organizations/${organization.slug}/replays/${mockReplayResponse.id}/`, body: {data: mockReplayResponse}, }); diff --git a/static/app/utils/replays/hooks/useReplayData.tsx b/static/app/utils/replays/hooks/useReplayData.tsx index e5a23565cf8ac5..88941a0d7a2080 100644 --- a/static/app/utils/replays/hooks/useReplayData.tsx +++ b/static/app/utils/replays/hooks/useReplayData.tsx @@ -95,7 +95,7 @@ function useReplayData({ errorsPerPage = 50, segmentsPerPage = 100, }: Options): Result { - const {projectSlug, replayId} = parseReplaySlug(replaySlug); + const replayId = parseReplayId(replaySlug); const projects = useProjects(); const api = useApi(); @@ -105,25 +105,23 @@ function useReplayData({ const [errors, setErrors] = useState([]); const [replayRecord, setReplayRecord] = useState(); + const projectSlug = useMemo(() => { + if (!replayRecord) { + return null; + } + return projects.projects.find(p => p.id === replayRecord.project_id)?.slug; + }, [replayRecord, projects.projects]); + // Fetch every field of the replay. We're overfetching, not every field is used const fetchReplay = useCallback(async () => { - const response = await api.requestPromise( - makeFetchReplayApiUrl(orgSlug, projectSlug, replayId) - ); + const response = await api.requestPromise(makeFetchReplayApiUrl(orgSlug, replayId)); const mappedRecord = mapResponseToReplayRecord(response.data); setReplayRecord(mappedRecord); setState(prev => ({...prev, fetchingReplay: false})); - }, [api, orgSlug, projectSlug, replayId]); + }, [api, orgSlug, replayId]); const fetchAttachments = useCallback(async () => { - if (!replayRecord) { - return; - } - const replayRecordProjectSlug = projects.projects.find( - p => p.id === replayRecord.project_id - )?.slug; - - if (!replayRecordProjectSlug) { + if (!replayRecord || !projectSlug) { return; } @@ -140,7 +138,7 @@ function useReplayData({ await Promise.allSettled( cursors.map(cursor => { const promise = api.requestPromise( - `/projects/${orgSlug}/${replayRecordProjectSlug}/replays/${replayRecord.id}/recording-segments/`, + `/projects/${orgSlug}/${projectSlug}/replays/${replayRecord.id}/recording-segments/`, { query: { download: true, @@ -156,7 +154,7 @@ function useReplayData({ }) ); setState(prev => ({...prev, fetchingAttachments: false})); - }, [segmentsPerPage, api, orgSlug, replayRecord, projects.projects]); + }, [segmentsPerPage, api, orgSlug, replayRecord, projectSlug]); const fetchErrors = useCallback(async () => { if (!replayRecord) { @@ -247,32 +245,18 @@ function useReplayData({ // this makes having project in the `replaySlug` obsolete // we must keep this url schema for now for backward compat but we should remove it at some point // TODO: remove support for projectSlug in replay url? -function parseReplaySlug(replaySlug: string) { +function parseReplayId(replaySlug: string) { const maybeProjectSlugAndReplayId = replaySlug.split(':'); if (maybeProjectSlugAndReplayId.length === 2) { - const [projectSlug, replayId] = maybeProjectSlugAndReplayId; - return { - projectSlug, - replayId, - }; + return maybeProjectSlugAndReplayId[1]; } // if there is no projectSlug then we assume we just have the replayId // all other cases would be a malformed url - return { - projectSlug: null, - replayId: maybeProjectSlugAndReplayId[0], - }; + return maybeProjectSlugAndReplayId[0]; } -function makeFetchReplayApiUrl( - orgSlug: string, - projectSlug: string | null, - replayId: string -) { - if (projectSlug) { - return `/projects/${orgSlug}/${projectSlug}/replays/${replayId}/`; - } +function makeFetchReplayApiUrl(orgSlug: string, replayId: string) { return `/organizations/${orgSlug}/replays/${replayId}/`; }