Skip to content
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

fix(dashboards): Indicate 404/400/500 properly #21853

Merged
merged 7 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 3 additions & 3 deletions frontend/src/scenes/dashboard/Dashboard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function DashboardScene(): JSX.Element {
itemsLoading,
filters: dashboardFilters,
dashboardMode,
receivedErrorsFromAPI,
dashboardFailedToLoad,
} = useValues(dashboardLogic)
const { setDashboardMode, setDates, reportDashboardViewed, setProperties, abortAnyRunningQuery } =
useActions(dashboardLogic)
Expand Down Expand Up @@ -96,15 +96,15 @@ function DashboardScene(): JSX.Element {
[setDashboardMode, dashboardMode, placement]
)

if (!dashboard && !itemsLoading && !receivedErrorsFromAPI) {
if (!dashboard && !itemsLoading && !dashboardFailedToLoad) {
return <NotFound object="dashboard" />
}

return (
<div className="dashboard">
{placement == DashboardPlacement.Dashboard && <DashboardHeader />}

{receivedErrorsFromAPI ? (
{dashboardFailedToLoad ? (
<InsightErrorState title="There was an error loading this dashboard" />
) : !tiles || tiles.length === 0 ? (
<EmptyDashboardComponent loading={itemsLoading} canEdit={canEditDashboard} />
Expand Down
15 changes: 15 additions & 0 deletions frontend/src/scenes/dashboard/Dashboards.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const meta: Meta = {
'/api/projects/:team_id/dashboards/': require('./__mocks__/dashboards.json'),
'/api/projects/:team_id/dashboards/1/': require('./__mocks__/dashboard1.json'),
'/api/projects/:team_id/dashboards/1/collaborators/': [],
'/api/projects/:team_id/dashboards/2/': [500, { detail: 'Server error' }],
'/api/projects/:team_id/dashboard_templates/': require('./__mocks__/dashboard_templates.json'),
'/api/projects/:team_id/dashboard_templates/json_schema/': require('./__mocks__/dashboard_template_schema.json'),
'/api/projects/:team_id/dashboards/:dash_id/sharing/': {
Expand Down Expand Up @@ -129,3 +130,17 @@ export const Edit = (): JSX.Element => {
}, [])
return <App />
}

export const NotFound = (): JSX.Element => {
useEffect(() => {
router.actions.push(urls.dashboard(1000))
}, [])
return <App />
}

export const Erroring = (): JSX.Element => {
useEffect(() => {
router.actions.push(urls.dashboard(2))
})
return <App />
}
12 changes: 6 additions & 6 deletions frontend/src/scenes/dashboard/dashboardLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -479,14 +479,14 @@ describe('dashboardLogic', () => {

it('fetches dashboard items on mount', async () => {
await expectLogic(logic)
.toDispatchActions(['loadDashboardItems'])
.toDispatchActions(['loadDashboard'])
.toMatchValues({
dashboard: null,
tiles: [],
insightTiles: [],
textTiles: [],
})
.toDispatchActions(['loadDashboardItemsSuccess'])
.toDispatchActions(['loadDashboardSuccess'])
.toMatchValues({
dashboard: expect.objectContaining(dashboards['5']),
tiles: truth((tiles) => tiles.length === 3),
Expand Down Expand Up @@ -676,7 +676,7 @@ describe('dashboardLogic', () => {
} as InsightModel)
})
.toFinishAllListeners()
.toDispatchActions(['loadDashboardItems'])
.toDispatchActions(['loadDashboard'])
})
})

Expand All @@ -689,7 +689,7 @@ describe('dashboardLogic', () => {
it('fetches dashboard items on mount', async () => {
await expectLogic(logic)
.toFinishAllListeners()
.toDispatchActions(['loadDashboardItemsSuccess'])
.toDispatchActions(['loadDashboardSuccess'])
.toMatchValues({
dashboard: truth(
({ tiles }) => tiles.filter((i: DashboardTile) => i.insight?.result === null).length === 2
Expand Down Expand Up @@ -748,7 +748,7 @@ describe('dashboardLogic', () => {
logic = dashboardLogic({ id: 9 })
logic.mount()
await expectLogic(logic)
.toDispatchActions(['loadDashboardItemsSuccess'])
.toDispatchActions(['loadDashboardSuccess'])
.toNotHaveDispatchedActions(['refreshAllDashboardItems'])
.toFinishListeners()
})
Expand Down Expand Up @@ -819,6 +819,6 @@ describe('dashboardLogic', () => {
}))
).toEqual([])
// Ensuring we do go back to the API for 800, which was added to dashboard 5
expectLogic(fiveLogic).toDispatchActions(['loadDashboardItems']).toFinishAllListeners()
expectLogic(fiveLogic).toDispatchActions(['loadDashboard']).toFinishAllListeners()
})
})
65 changes: 38 additions & 27 deletions frontend/src/scenes/dashboard/dashboardLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
}),

actions({
loadDashboardItems: (payload: { refresh?: boolean; action: string }) => payload,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was some really legacy naming, from back when this literally had to load every dashboard item individually

loadDashboard: (payload: { refresh?: boolean; action: string }) => payload,
triggerDashboardUpdate: (payload) => ({ payload }),
/** The current state in which the dashboard is being viewed, see DashboardMode. */
setDashboardMode: (mode: DashboardMode | null, source: DashboardEventSource | null) => ({ mode, source }),
Expand Down Expand Up @@ -201,12 +201,11 @@ export const dashboardLogic = kea<dashboardLogicType>([
dashboard: [
null as DashboardType | null,
{
loadDashboardItems: async ({ refresh, action }) => {
loadDashboard: async ({ refresh, action }) => {
const dashboardQueryId = uuid()
actions.loadingDashboardItemsStarted(action, dashboardQueryId)

try {
// :TODO: Send dashboardQueryId forward as well if refreshing
const apiUrl = values.apiUrl(refresh)
const dashboardResponse: Response = await api.getResponse(apiUrl)
const dashboard: DashboardType = await getJSONOrNull(dashboardResponse)
Expand Down Expand Up @@ -298,12 +297,11 @@ export const dashboardLogic = kea<dashboardLogicType>([
],
})),
reducers(({ props }) => ({
receivedErrorsFromAPI: [
dashboardFailedToLoad: [
false,
{
loadDashboardItemsSuccess: () => false,
loadDashboardItemsFailure: () => true,
abortQuery: () => false,
loadDashboardSuccess: () => false,
loadDashboardFailure: () => true,
},
],
filters: [
Expand All @@ -318,12 +316,15 @@ export const dashboardLogic = kea<dashboardLogicType>([
...state,
properties: properties || null,
}),
loadDashboardItemsSuccess: (state, { dashboard }) => ({
...state,
date_from: dashboard?.filters.date_from || null,
date_to: dashboard?.filters.date_to || null,
properties: dashboard?.filters.properties || [],
}),
loadDashboardSuccess: (state, { dashboard }) =>
dashboard
? {
...state,
date_from: dashboard?.filters.date_from || null,
date_to: dashboard?.filters.date_to || null,
properties: dashboard?.filters.properties || [],
}
: state,
},
],
dashboard: [
Expand Down Expand Up @@ -433,7 +434,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
},
},
],
loadTimer: [null as Date | null, { loadDashboardItems: () => new Date() }],
loadTimer: [null as Date | null, { loadDashboard: () => new Date() }],
dashboardLoadTimerData: [
{ dashboardQueryId: '', action: '', startTime: 0, responseBytes: 0 },
{
Expand Down Expand Up @@ -513,7 +514,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
shouldReportOnAPILoad: [
/* Whether to report viewed/analyzed events after the API is loaded (and this logic is mounted).
We need this because the DashboardView component might be mounted (and subsequent `useEffect`) before the API request
to `loadDashboardItems` is completed (e.g. if you open PH directly to a dashboard)
to `loadDashboard` is completed (e.g. if you open PH directly to a dashboard)
*/
false,
{
Expand Down Expand Up @@ -686,16 +687,22 @@ export const dashboardLogic = kea<dashboardLogicType>([
},
],
breadcrumbs: [
(s) => [s.dashboard],
(dashboard): Breadcrumb[] => [
(s) => [s.dashboard, s.dashboardLoading, s.dashboardFailedToLoad],
(dashboard, dashboardLoading, dashboardFailedToLoad): Breadcrumb[] => [
{
key: Scene.Dashboards,
name: 'Dashboards',
path: urls.dashboards(),
},
{
key: [Scene.Dashboard, dashboard?.id || 'new'],
name: dashboard?.id ? dashboard.name || 'Unnamed' : null,
name: dashboard?.id
? dashboard.name
: dashboardFailedToLoad
? 'Could not load'
: !dashboardLoading
? 'Not found'
: null,
onRename: async (name) => {
if (dashboard) {
await dashboardsModel.asyncActions.updateDashboard({
Expand Down Expand Up @@ -732,10 +739,10 @@ export const dashboardLogic = kea<dashboardLogicType>([
if (props.id) {
if (props.dashboard) {
// If we already have dashboard data, use it. Should the data turn out to be stale,
// the loadDashboardItemsSuccess listener will initiate a refresh
actions.loadDashboardItemsSuccess(props.dashboard)
// the loadDashboardSuccess listener will initiate a refresh
actions.loadDashboardSuccess(props.dashboard)
} else {
actions.loadDashboardItems({
actions.loadDashboard({
refresh: false,
action: 'initial_load',
})
Expand Down Expand Up @@ -772,17 +779,17 @@ export const dashboardLogic = kea<dashboardLogicType>([
setRefreshError: sharedListeners.reportRefreshTiming,
setRefreshStatuses: sharedListeners.reportRefreshTiming,
setRefreshStatus: sharedListeners.reportRefreshTiming,
loadDashboardItemsFailure: sharedListeners.reportLoadTiming,
loadDashboardFailure: sharedListeners.reportLoadTiming,
[insightsModel.actionTypes.duplicateInsightSuccess]: () => {
// TODO this is a bit hacky, but we need to reload the dashboard to get the new insight
// TODO when duplicated from a dashboard we should carry the context so only one logic needs to reload
// TODO or we should duplicate the tile (and implicitly the insight)
actions.loadDashboardItems({ action: 'update' })
actions.loadDashboard({ action: 'update' })
},
[dashboardsModel.actionTypes.tileAddedToDashboard]: ({ dashboardId }) => {
// when adding an insight to a dashboard, we need to reload the dashboard to get the new insight
if (dashboardId === props.id) {
actions.loadDashboardItems({ action: 'update' })
actions.loadDashboard({ action: 'update' })
}
},
[dashboardsModel.actionTypes.updateDashboardInsight]: ({ insight, extraDashboardIds }) => {
Expand All @@ -796,7 +803,7 @@ export const dashboardLogic = kea<dashboardLogicType>([

if (tileIndex === -1) {
// this is a new tile created from an insight context we need to reload the dashboard
actions.loadDashboardItems({ action: 'update' })
actions.loadDashboard({ action: 'update' })
}
},
updateLayouts: () => {
Expand Down Expand Up @@ -1026,10 +1033,14 @@ export const dashboardLogic = kea<dashboardLogicType>([
}, values.autoRefresh.interval * 1000)
}
},
loadDashboardItemsSuccess: function (...args) {
loadDashboardSuccess: function (...args) {
void sharedListeners.reportLoadTiming(...args)

const dashboard = values.dashboard as DashboardType
if (!values.dashboard) {
return // We hit a 404
}

const dashboard = values.dashboard
const { action, dashboardQueryId, startTime, responseBytes } = values.dashboardLoadTimerData
const lastRefresh = sortDates(dashboard.tiles.map((tile) => tile.last_refresh))

Expand Down
6 changes: 2 additions & 4 deletions frontend/src/scenes/feature-flags/FeatureFlag.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -618,21 +618,19 @@ function UsageTab({ featureFlag }: { id: string; featureFlag: FeatureFlagType })
const { generateUsageDashboard, enrichUsageDashboard } = useActions(featureFlagLogic)
const { featureFlagLoading } = useValues(featureFlagLogic)
let dashboard: DashboardType | null = null
let connectedDashboardExists = false
if (dashboardId) {
// FIXME: Refactor out into <ConnectedDashboard />, as React hooks under conditional branches are no good
const dashboardLogicValues = useValues(
dashboardLogic({ id: dashboardId, placement: DashboardPlacement.FeatureFlag })
)
dashboard = dashboardLogicValues.dashboard
connectedDashboardExists = !dashboardLogicValues.receivedErrorsFromAPI
}

const { closeEnrichAnalyticsNotice } = useActions(featureFlagsLogic)
const { enrichAnalyticsNoticeAcknowledged } = useValues(featureFlagsLogic)

useEffect(() => {
if (
connectedDashboardExists &&
dashboard &&
hasEnrichedAnalytics &&
!(dashboard.tiles?.find((tile) => (tile.insight?.name?.indexOf('Feature Viewed') ?? -1) > -1) !== undefined)
Expand All @@ -652,7 +650,7 @@ function UsageTab({ featureFlag }: { id: string; featureFlag: FeatureFlagType })

return (
<div>
{connectedDashboardExists ? (
{dashboard ? (
<>
{!hasEnrichedAnalytics && !enrichAnalyticsNoticeAcknowledged && (
<LemonBanner type="info" className="mb-3" onClose={() => closeEnrichAnalyticsNotice()}>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/insights/insightLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ describe('insightLogic', () => {
// 1. the dashboard is mounted
const dashLogic = dashboardLogic({ id: 33 })
dashLogic.mount()
await expectLogic(dashLogic).toDispatchActions(['loadDashboardItemsSuccess'])
await expectLogic(dashLogic).toDispatchActions(['loadDashboardSuccess'])

// 2. mount the insight
logic = insightLogic({ dashboardItemId: Insight42, dashboardId: 33 })
Expand All @@ -651,7 +651,7 @@ describe('insightLogic', () => {
// 1. the dashboard is mounted
const dashLogic = dashboardLogic({ id: 33 })
dashLogic.mount()
await expectLogic(dashLogic).toDispatchActions(['loadDashboardItemsSuccess'])
await expectLogic(dashLogic).toDispatchActions(['loadDashboardSuccess'])

// 2. mount the insight
logic = insightLogic({ dashboardItemId: Insight42, dashboardId: 1 })
Expand Down
Loading