From 14b3211523f369c6a9b1a14f4e77637383559c33 Mon Sep 17 00:00:00 2001 From: Calvin Yau Date: Mon, 12 Jan 2026 15:52:26 -0800 Subject: [PATCH] fix: Handle missing segments from ImpactedFileComparison response --- .../shared/CommitFileDiff/CommitFileDiff.tsx | 10 +- .../CommitFileDiff/CommitFileDiff.tsx | 10 +- .../PullFileDiff/PullFileDiff.tsx | 71 +++++++++-- .../PullFileDiff/PullFileDiff.tsx | 71 +++++++++-- src/services/comparison/utils.ts | 2 +- src/services/pull/fragments.ts | 49 +++++--- ...useSingularImpactedFileComparison.test.tsx | 111 +++++++++++++++--- .../useSingularImpactedFileComparison.tsx | 13 +- .../utils/transformImpactedPullFileToDiff.ts | 2 +- 9 files changed, 267 insertions(+), 72 deletions(-) diff --git a/src/pages/CommitDetailPage/CommitCoverage/routes/FilesChangedTab/shared/CommitFileDiff/CommitFileDiff.tsx b/src/pages/CommitDetailPage/CommitCoverage/routes/FilesChangedTab/shared/CommitFileDiff/CommitFileDiff.tsx index 59cfcdf36b..181b914414 100644 --- a/src/pages/CommitDetailPage/CommitCoverage/routes/FilesChangedTab/shared/CommitFileDiff/CommitFileDiff.tsx +++ b/src/pages/CommitDetailPage/CommitCoverage/routes/FilesChangedTab/shared/CommitFileDiff/CommitFileDiff.tsx @@ -19,16 +19,20 @@ import { } from 'ui/VirtualRenderers/VirtualDiffRenderer' function transformSegmentsToLineData( - segments: ImpactedFileType['segments']['results'] | undefined, + segments: ImpactedFileType['segments'] | undefined, ignoredUploadIds: number[] ) { - if (!segments) { + if ( + !segments || + segments.__typename !== 'SegmentComparisons' || + !segments.results + ) { return [] } const ignoredUploadIdsSet = new Set(ignoredUploadIds) - return segments.map((segment) => { + return segments.results.map((segment) => { // we need to create a string of the diff content for the virtual diff renderer text area let newDiffContent = '' const lineData: LineData[] = [] diff --git a/src/pages/CommitDetailPage/CommitCoverage/routes/IndirectChangesTab/IndirectChangesTable/CommitFileDiff/CommitFileDiff.tsx b/src/pages/CommitDetailPage/CommitCoverage/routes/IndirectChangesTab/IndirectChangesTable/CommitFileDiff/CommitFileDiff.tsx index 007aaa5f01..6e7874abd2 100644 --- a/src/pages/CommitDetailPage/CommitCoverage/routes/IndirectChangesTab/IndirectChangesTable/CommitFileDiff/CommitFileDiff.tsx +++ b/src/pages/CommitDetailPage/CommitCoverage/routes/IndirectChangesTab/IndirectChangesTable/CommitFileDiff/CommitFileDiff.tsx @@ -20,16 +20,20 @@ import { } from 'ui/VirtualRenderers/VirtualDiffRenderer' function transformSegmentsToLineData( - segments: ImpactedFileType['segments']['results'] | undefined, + segments: ImpactedFileType['segments'] | undefined, ignoredUploadIds: number[] ) { - if (!segments) { + if ( + !segments || + segments.__typename !== 'SegmentComparisons' || + !segments.results + ) { return [] } const ignoredUploadIdsSet = new Set(ignoredUploadIds) - return segments.map((segment) => { + return segments.results.map((segment) => { // we need to create a string of the diff content for the virtual diff renderer text area let newDiffContent = '' const lineData: LineData[] = [] diff --git a/src/pages/PullRequestPage/PullCoverage/routes/FilesChangedTab/FilesChanged/PullFileDiff/PullFileDiff.tsx b/src/pages/PullRequestPage/PullCoverage/routes/FilesChangedTab/FilesChanged/PullFileDiff/PullFileDiff.tsx index 03b263edbe..4ff015c122 100644 --- a/src/pages/PullRequestPage/PullCoverage/routes/FilesChangedTab/FilesChanged/PullFileDiff/PullFileDiff.tsx +++ b/src/pages/PullRequestPage/PullCoverage/routes/FilesChangedTab/FilesChanged/PullFileDiff/PullFileDiff.tsx @@ -2,10 +2,8 @@ import { Fragment, useMemo } from 'react' import { useLocation, useParams } from 'react-router-dom' import { useNavLinks } from 'services/navigation/useNavLinks' -import { - type PullImpactedFile, - useSingularImpactedFileComparison, -} from 'services/pull/useSingularImpactedFileComparison' +import { useSingularImpactedFileComparison } from 'services/pull/useSingularImpactedFileComparison' +import { transformImpactedPullFileToDiff } from 'services/pull/utils' import { useRepoOverview } from 'services/repo' import A from 'ui/A' import CodeRendererInfoRow from 'ui/CodeRenderer/CodeRendererInfoRow' @@ -14,13 +12,21 @@ import { CoverageValue } from 'ui/VirtualRenderers/types' import { LineData } from 'ui/VirtualRenderers/VirtualDiffRenderer' function transformSegmentsToLineData( - segments: PullImpactedFile['segments']['results'] | undefined + segments: + | NonNullable< + ReturnType + >['segments'] + | undefined ) { - if (!segments) { + if ( + !segments || + segments.__typename !== 'SegmentComparisons' || + !segments.results + ) { return [] } - return segments.map((segment) => { + return segments.results.map((segment) => { // we need to create a string of the diff content for the virtual diff renderer text area let newDiffContent = '' const lineData: LineData[] = [] @@ -57,7 +63,7 @@ function DiffRenderer({ impactedFile, path, }: { - impactedFile: ReturnType['data'] + impactedFile: ReturnType path: string }) { const { pullFileView } = useNavLinks() @@ -134,13 +140,44 @@ function ErrorDisplayMessage() { ) } +function UnknownPathErrorDisplayMessage({ path }: { path: string }) { + const location = useLocation() + + return ( +

+ There was a problem getting the source code from your provider by path + for: {path}. Unable to show line by line coverage. +
+ + If you continue to experience this issue, please try{' '} + + logging in + {' '} + again to refresh your credentials. Otherwise, please visit our{' '} + + Path Fixing + {' '} + documentation for troubleshooting tips. + +

+ ) +} + interface PullFileDiffProps { path: string | null | undefined } function PullFileDiff({ path }: PullFileDiffProps) { const { provider, owner, repo, pullId } = useParams() - const { data } = useSingularImpactedFileComparison({ + const { data: impactedFile } = useSingularImpactedFileComparison({ provider, owner, repo, @@ -149,10 +186,24 @@ function PullFileDiff({ path }: PullFileDiffProps) { filters: { hasUnintendedChanges: false }, }) - if (!data || typeof path !== 'string') { + const segments = impactedFile?.segments + + if ( + !impactedFile || + typeof path !== 'string' || + !segments || + segments.__typename === 'ProviderError' + ) { return } + if (segments.__typename === 'UnknownPath') { + return + } + + // Transform the data to form needed for rendering + const data = transformImpactedPullFileToDiff(impactedFile) + return } diff --git a/src/pages/PullRequestPage/PullCoverage/routes/IndirectChangesTab/PullFileDiff/PullFileDiff.tsx b/src/pages/PullRequestPage/PullCoverage/routes/IndirectChangesTab/PullFileDiff/PullFileDiff.tsx index c66cfa282c..5bc3130f85 100644 --- a/src/pages/PullRequestPage/PullCoverage/routes/IndirectChangesTab/PullFileDiff/PullFileDiff.tsx +++ b/src/pages/PullRequestPage/PullCoverage/routes/IndirectChangesTab/PullFileDiff/PullFileDiff.tsx @@ -2,10 +2,8 @@ import { Fragment, useMemo } from 'react' import { useLocation, useParams } from 'react-router-dom' import { useNavLinks } from 'services/navigation/useNavLinks' -import { - PullImpactedFile, - useSingularImpactedFileComparison, -} from 'services/pull/useSingularImpactedFileComparison' +import { useSingularImpactedFileComparison } from 'services/pull/useSingularImpactedFileComparison' +import { transformImpactedPullFileToDiff } from 'services/pull/utils' import { useRepoOverview } from 'services/repo' import A from 'ui/A' import CodeRendererInfoRow from 'ui/CodeRenderer/CodeRendererInfoRow' @@ -16,12 +14,20 @@ import { } from 'ui/VirtualRenderers/VirtualDiffRenderer' function transformSegmentsToLineData( - segments: PullImpactedFile['segments']['results'] | undefined + segments: + | NonNullable< + ReturnType + >['segments'] + | undefined ) { - if (!segments) { + if ( + !segments || + segments.__typename !== 'SegmentComparisons' || + !segments.results + ) { return [] } - return segments.map((segment) => { + return segments.results.map((segment) => { // we need to create a string of the diff content for the virtual diff renderer text area let newDiffContent = '' const lineData: LineData[] = [] @@ -54,7 +60,7 @@ function DiffRenderer({ impactedFile, path, }: { - impactedFile: ReturnType['data'] + impactedFile: ReturnType path: string }) { const { pullFileView } = useNavLinks() @@ -128,13 +134,44 @@ function ErrorDisplayMessage() { ) } +function UnknownPathErrorDisplayMessage({ path }: { path: string }) { + const location = useLocation() + + return ( +

+ There was a problem getting the source code from your provider by path + for: {path}. Unable to show line by line coverage. +
+ + If you continue to experience this issue, please try{' '} + + logging in + {' '} + again to refresh your credentials. Otherwise, please visit our{' '} + + Path Fixing + {' '} + documentation for troubleshooting tips. + +

+ ) +} + interface PullFileDiffProps { path: string | null | undefined } function PullFileDiff({ path }: PullFileDiffProps) { const { provider, owner, repo, pullId } = useParams() - const { data } = useSingularImpactedFileComparison({ + const { data: impactedFile } = useSingularImpactedFileComparison({ provider, owner, repo, @@ -143,10 +180,24 @@ function PullFileDiff({ path }: PullFileDiffProps) { filters: { hasUnintendedChanges: true }, }) - if (!data || typeof path !== 'string') { + const segments = impactedFile?.segments + + if ( + !impactedFile || + typeof path !== 'string' || + !segments || + segments.__typename === 'ProviderError' + ) { return } + if (segments.__typename === 'UnknownPath') { + return + } + + // Transform the data to form needed for rendering + const data = transformImpactedPullFileToDiff(impactedFile) + return } diff --git a/src/services/comparison/utils.ts b/src/services/comparison/utils.ts index 4eb0283ad3..453fadd34e 100644 --- a/src/services/comparison/utils.ts +++ b/src/services/comparison/utils.ts @@ -32,7 +32,7 @@ export function transformImpactedFileToDiff(impactedFile: ImpactedFileType) { return { fileLabel, headName: impactedFile?.headName, - segments: impactedFile?.segments?.results, + segments: impactedFile?.segments, ...(!!hashedPath && { hashedPath }), } } diff --git a/src/services/pull/fragments.ts b/src/services/pull/fragments.ts index 6178319898..9cd9360b5d 100644 --- a/src/services/pull/fragments.ts +++ b/src/services/pull/fragments.ts @@ -254,6 +254,33 @@ fragment FileComparisonWithBase on Pull { const CoverageLineSchema = z.enum(['H', 'M', 'P']) +const SegmentComparisonsSchema = z.object({ + __typename: z.literal('SegmentComparisons'), + results: z.array( + z.object({ + header: z.string(), + hasUnintendedChanges: z.boolean(), + lines: z.array( + z.object({ + baseNumber: z.string().nullable(), + headNumber: z.string().nullable(), + baseCoverage: CoverageLineSchema.nullable(), + headCoverage: CoverageLineSchema.nullable(), + content: z.string().nullable(), + }) + ), + }) + ), +}) + +const UnknownPathSchema = z.object({ + __typename: z.literal('UnknownPath'), +}) + +const ProviderErrorSchema = z.object({ + __typename: z.literal('ProviderError'), +}) + export const ImpactedFileSchema = z.object({ headName: z.string().nullable(), hashedPath: z.string(), @@ -276,23 +303,11 @@ export const ImpactedFileSchema = z.object({ percentCovered: z.number().nullable(), }) .nullable(), - segments: z.object({ - results: z.array( - z.object({ - header: z.string(), - hasUnintendedChanges: z.boolean(), - lines: z.array( - z.object({ - baseNumber: z.string().nullable(), - headNumber: z.string().nullable(), - baseCoverage: CoverageLineSchema.nullable(), - headCoverage: CoverageLineSchema.nullable(), - content: z.string().nullable(), - }) - ), - }) - ), - }), + segments: z.discriminatedUnion('__typename', [ + SegmentComparisonsSchema, + UnknownPathSchema, + ProviderErrorSchema, + ]), }) export const ComparisonSchema = z.object({ diff --git a/src/services/pull/useSingularImpactedFileComparison.test.tsx b/src/services/pull/useSingularImpactedFileComparison.test.tsx index 98c70ce3b1..9f79591b93 100644 --- a/src/services/pull/useSingularImpactedFileComparison.test.tsx +++ b/src/services/pull/useSingularImpactedFileComparison.test.tsx @@ -112,12 +112,41 @@ const mockMissingBaseCommitResponse = { }, } +const mockUnknownPathResponse = { + owner: { + repository: { + __typename: 'Repository', + pull: { + compareWithBase: { + __typename: 'Comparison', + impactedFile: { + __typename: 'ImpactedFile', + hashedPath: 'hashedPath', + headName: 'headName', + isRenamedFile: false, + isNewFile: false, + isDeletedFile: false, + baseCoverage: { percentCovered: 23 }, + headCoverage: { percentCovered: 24 }, + patchCoverage: { percentCovered: 25 }, + changeCoverage: 0, + segments: { + __typename: 'UnknownPath', + }, + }, + }, + }, + }, + }, +} + describe('useSingularImpactedFileComparison', () => { function setup({ isNotFoundError = false, isOwnerNotActivatedError = false, isUnsuccessfulParseError = false, isMissingBaseCommit = false, + isUnknownPath = false, }) { server.use( graphql.query('ImpactedFileComparison', () => { @@ -129,6 +158,8 @@ describe('useSingularImpactedFileComparison', () => { return HttpResponse.json({ data: mockIncorrectResponse }) } else if (isMissingBaseCommit) { return HttpResponse.json({ data: mockMissingBaseCommitResponse }) + } else if (isUnknownPath) { + return HttpResponse.json({ data: mockUnknownPathResponse }) } return HttpResponse.json({ data: mockResponse }) }) @@ -158,24 +189,33 @@ describe('useSingularImpactedFileComparison', () => { await waitFor(() => expect(result.current.data).toEqual({ - fileLabel: null, hashedPath: 'hashedPath', headName: 'headName', - segments: [ - { - hasUnintendedChanges: false, - header: 'header', - lines: [ - { - baseCoverage: 'M', - baseNumber: '1', - content: 'content', - headCoverage: 'H', - headNumber: '1', - }, - ], - }, - ], + isRenamedFile: false, + isNewFile: false, + isDeletedFile: false, + baseCoverage: { percentCovered: 23 }, + headCoverage: { percentCovered: 24 }, + patchCoverage: { percentCovered: 25 }, + changeCoverage: 0, + segments: { + __typename: 'SegmentComparisons', + results: [ + { + hasUnintendedChanges: false, + header: 'header', + lines: [ + { + baseCoverage: 'M', + baseNumber: '1', + content: 'content', + headCoverage: 'H', + headNumber: '1', + }, + ], + }, + ], + }, }) ) }) @@ -286,4 +326,43 @@ describe('useSingularImpactedFileComparison', () => { await waitFor(() => expect(result.current.error).toBeNull()) }) }) + + describe('when segments is UnknownPath', () => { + it('returns data without segments.results', async () => { + setup({ isUnknownPath: true }) + const { result } = renderHook( + () => + useSingularImpactedFileComparison({ + provider: 'gh', + owner: 'codecov', + repo: 'gazebo', + pullId: '1', + path: 'path', + }), + { + wrapper, + } + ) + + await waitFor(() => result.current.isLoading) + await waitFor(() => !result.current.isLoading) + + await waitFor(() => + expect(result.current.data).toEqual({ + hashedPath: 'hashedPath', + headName: 'headName', + isRenamedFile: false, + isNewFile: false, + isDeletedFile: false, + baseCoverage: { percentCovered: 23 }, + headCoverage: { percentCovered: 24 }, + patchCoverage: { percentCovered: 25 }, + changeCoverage: 0, + segments: { + __typename: 'UnknownPath', + }, + }) + ) + }) + }) }) diff --git a/src/services/pull/useSingularImpactedFileComparison.tsx b/src/services/pull/useSingularImpactedFileComparison.tsx index 2ce839166e..327047f371 100644 --- a/src/services/pull/useSingularImpactedFileComparison.tsx +++ b/src/services/pull/useSingularImpactedFileComparison.tsx @@ -13,14 +13,7 @@ import Api from 'shared/api' import { rejectNetworkError } from 'shared/api/rejectNetworkError' import A from 'ui/A' -import { - ComparisonSchema, - FileComparisonWithBase, - ImpactedFileSchema, -} from './fragments' -import { transformImpactedPullFileToDiff } from './utils' - -export type PullImpactedFile = z.infer +import { ComparisonSchema, FileComparisonWithBase } from './fragments' const query = ` query ImpactedFileComparison($owner: String!, $repo: String!, $pullId: Int!, $path: String!, $filters: SegmentsFilters) { @@ -158,9 +151,7 @@ export function useSingularImpactedFileComparison({ data?.owner?.repository?.pull?.compareWithBase?.__typename === 'Comparison' ) { - return transformImpactedPullFileToDiff( - data?.owner?.repository?.pull?.compareWithBase?.impactedFile - ) + return data?.owner?.repository?.pull?.compareWithBase?.impactedFile } // we can set to null, and use the error display message we currently have, rather than throwing diff --git a/src/services/pull/utils/transformImpactedPullFileToDiff.ts b/src/services/pull/utils/transformImpactedPullFileToDiff.ts index b2237a72be..d3b4bb6515 100644 --- a/src/services/pull/utils/transformImpactedPullFileToDiff.ts +++ b/src/services/pull/utils/transformImpactedPullFileToDiff.ts @@ -35,7 +35,7 @@ export function transformImpactedPullFileToDiff( return { fileLabel, headName: impactedFile?.headName, - segments: impactedFile?.segments?.results, + segments: impactedFile?.segments, ...(!!hashedPath && { hashedPath }), } }