Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions frontend/src/initKea.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const ERROR_FILTER_ALLOW_LIST = [
'loadSelfDrivingEvaluationReports', // The self-driving eval table renders its own retry state
'loadToolDataEvents',
'loadInstallRequests', // Polled in the background on Settings → Integrations; the banner just stays hidden
'loadReportDiff', // The Inbox report detail's diff panel renders its own error state
'loadPrChecks', // Polled in the Inbox report detail; the CI checks section renders its own error state
'loadPrComments', // The Inbox report detail's PR comments section renders its own error state
'loadMonitoringSnapshot', // The managed warehouse Monitoring tab renders its own retry state
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,42 @@ describe('inboxReportDetailLogic', () => {
})
})

describe('diff fetching', () => {
let logic: ReturnType<typeof inboxReportDetailLogic.build>

beforeEach(() => {
useMocks({
get: {
'/api/projects/:team_id/signals/reports/:id/artefacts/': { results: [] },
'/api/projects/:team_id/signals/reports/:id/signals/': [],
'/api/projects/:team_id/signals/reports/available_reviewers/': [],
'/api/projects/:team_id/signals/reports/:id/artefacts/:artefact_id/diff/': () => [
404,
{ error: "Branch 'fix-checkout' was not found on GitHub." },
],
},
})
initKeaTests()
silenceKeaLoadersErrors()
logic = inboxReportDetailLogic({ reportId: REPORT.id, report: REPORT })
logic.mount()
})

afterEach(() => {
logic.unmount()
resumeKeaLoadersErrors()
})

// The endpoint returns its reason under `error`, not DRF's `detail`, so a reducer that reads
// the wrong key falls back to guessing at a cause the response already stated.
it('surfaces what the endpoint says it could not find', async () => {
logic.actions.loadReportDiff({ artefactId: 'artefact-1' })
await expectLogic(logic).toFinishAllListeners()

expect(logic.values.reportDiffError).toBe("Branch 'fix-checkout' was not found on GitHub.")
})
})

describe('report task polling', () => {
let logic: ReturnType<typeof inboxReportDetailLogic.build>
let artefactRequests: number
Expand Down
11 changes: 7 additions & 4 deletions products/signals/frontend/inbox/logics/inboxReportDetailLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -851,15 +851,18 @@ export const inboxReportDetailLogic = kea<inboxReportDetailLogicType>([
setFeedbackNoteSubmitting: (_, { submitting }) => submitting,
},
],
// Human-readable diff-load failure (kea-loaders only exposes a boolean loading flag). A failed
// compare usually means the branch was merged, deleted, or force-rewritten away.
// Human-readable diff-load failure (kea-loaders only exposes a boolean loading flag). The
// endpoint says which branch or repository GitHub could not find, so prefer its message and
// keep the guess for a failure that carried none (a network drop, a gateway error page).
reportDiffError: [
null as string | null,
{
loadReportDiff: () => null,
loadReportDiffSuccess: () => null,
loadReportDiffFailure: () =>
"Couldn't load the diff. The branch may have been merged, deleted, or rewritten.",
loadReportDiffFailure: (_state: string | null, { errorObject }: { errorObject?: any }) =>
typeof errorObject?.data?.error === 'string' && errorObject.data.error
? errorObject.data.error
: "Couldn't load the diff. The branch may have been merged, deleted, or rewritten.",
Comment on lines +862 to +865

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve rate-limit details before fallinging back

When GitHub rate-limits this endpoint, github_rate_limited_response returns a 429 body with the reason in detail, not error. Because this commit also suppresses the generic loadReportDiff toast, the reducer now discards that reason and shows the misleading fallback that the branch may have been merged, deleted, or rewritten. Check errorObject.data.detail (or the normalized API error message) before using the fallback.

Useful? React with 👍 / 👎.

},
],
// The commit artefact the current `reportDiff` was loaded for, so the artefact poll re-fetches
Expand Down
Loading