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.

Sign up for GitHub

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

chore: improve error notifications in annotations and notebooks #6927

Merged
merged 5 commits into from
Sep 10, 2024
Merged

chore: improve error notifications in annotations and notebooks #6927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d390f20
chore: improve error notifications in annotations and notebooks
philjb Aug 20, 2024
c509e4f
fix: improve failure error message notifications
philjb Aug 23, 2024
5f5d22d
chore: run yarn prettier:fix
philjb Aug 26, 2024
9e1c761
chore: whitespace to trigger ci
philjb Aug 28, 2024
44e5209
chore: Merge branch 'master' into pjb/sqlite-error-messages
philjb Sep 10, 2024
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
10 changes: 6 additions & 4 deletions src/annotations/components/annotationForm/AnnotationForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {useDispatch} from 'react-redux'
// Utils
import {event} from 'src/cloud/utils/reporting'
import classnames from 'classnames'
import {getErrorMessage} from 'src/utils/api'

// Components
import {
Expand Down Expand Up @@ -58,7 +59,8 @@ export const START_TIME_IN_FUTURE_MESSAGE = 'Start Time cannot be in the future'

/**
* Form for editing and creating annotations.
* It does support multi-line annotations, but the tradeoff is that the user cannot then press 'return' to submit the form.
* It does support multi-line annotations, but the tradeoff is that the user
* cannot then press 'return' to submit the form.
* */
export const AnnotationForm: FC<Props> = (props: Props) => {
const [startTime, setStartTime] = useState(props.startTime)
Expand Down Expand Up @@ -129,7 +131,7 @@ export const AnnotationForm: FC<Props> = (props: Props) => {
}
}

const handleDelete = () => {
const handleDelete = async () => {
const editedAnnotation = {
summary,
startTime,
Expand All @@ -140,7 +142,7 @@ export const AnnotationForm: FC<Props> = (props: Props) => {
}

try {
dispatch(deleteAnnotations(editedAnnotation))
await dispatch(deleteAnnotations(editedAnnotation))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blegesse-w, @wdoconnell - perhaps you can help me here? I don't know react/redux so I'm just guessing.

What I am observing is that when the api call within deleteAnnotations fails - it throws an Error if the http code is >= 300 - it is not caught at line 149 - so this delete annotation always appears to be successful in the ui even if the api call fails.

I guess that the issue is that the dispatch is async so this code advances to the notify success before the error arrives, but this isn't an async context so I added

const handleDelete = async (): Promise<void> => {

but that still doesn't catch the error thrown.

Any idea?

I have noticed that if editing an annotation or creating an annotation fails at the api, those errors are caught in their catch blocks but I can't see what is different in those code paths and this code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see here that the delete and put api calls are structured identically to my eye and these are where api errors are thrown

ui/src/annotations/api/index.ts

Lines 77 to 101 in f46e47d

export const updateAnnotation = async (
newAnnotation: AnnotationEvent
): Promise<AnnotationEvent> => {
const params = {annotationID: newAnnotation.id, data: newAnnotation}
const res = await putAnnotation(params)
if (res.status >= 300) {
throw new Error(res.data?.message)
}
return res.data
}
export const deleteAnnotation = async (
annotationToDelete: DeleteAnnotation
): Promise<number> => {
const params = {annotationID: annotationToDelete.id}
const res = await deleteAnnotationApi(params)
if (res.status >= 300) {
throw new Error(res.data?.message)
}
return res.status
}

The put/edit errors are caught tho and the delete one isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This edit annotation seems to catch thrown api errors so i was trying to make the delete code look the same

ui/src/annotations/components/EditAnnotationOverlay.tsx

Lines 41 to 57 in f46e47d

const handleSubmit = async (
editedAnnotation: EditAnnotation
): Promise<void> => {
try {
await dispatch(editAnnotation(editedAnnotation))
event(
`${clickedAnnotation.eventPrefix}.annotations.edit_annotation.success`
)
dispatch(notify(editAnnotationSuccess()))
onClose()
} catch (err) {
event(
`${clickedAnnotation.eventPrefix}.annotations.edit_annotation.failure`
)
dispatch(notify(editAnnotationFailed(getErrorMessage(err))))
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Hey @philjb thanks for tagging me in. I can't say what the problem is for sure but from a quick glance at the code, it does look like the issue might be with how the deleteAnnotations API call is handling errors. From what I can see, even though you're using await in your try...catch block, the error isn't getting caught as expected. This could be happening if the deleteAnnotations function isn't throwing the error properly when the API call fails.
Here is what i suggest. Double-check that deleteAnnotations is an asynchronous action creator that throws an error when the API responds with a status code of 300 or higher. This way, the error will propagate correctly up to your try...catch block.

Copy link
Member

Choose a reason for hiding this comment

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

Also it's probably worth checking that the redux thunk is not swallowing the error before it reaches your catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look Beth! I think those are good suggestions, I just don't know how to do them. I don't know what a asynchronous action creator is or how to check it. I did walk through the code with the browser debugger and it appears to throw the error - it's hard for me to see how it couldn't throw the error at these lines:

ui/src/annotations/api/index.ts

Lines 96 to 98 in f46e47d

if (res.status >= 300) {
throw new Error(res.data?.message)
}

redux thunk is not swallowing the error

Presumably the error is getting caught somewhere since it isn't caught where I want, but I don't know how to check where it is getting caught.

I'm happy to learn to fish here but you'll be teaching me react/redux - for instance, i don't know what a redux thunk is.

Copy link
Member

@blegesse-w blegesse-w Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@philjb No worries, Phil! I’ll take a closer look at this tomorrow with Bill, and we’ll figure out what's going on. I know the whole async/Redux world can be a bit tricky so I'll make sure to loop you in as I go

Copy link
Member

@blegesse-w blegesse-w Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did some debugging on this locally and reached a similar solution to yours—adding await before the dispatch(deleteAnnotations(editedAnnotation)) call. But, one key detail is that the handleDelete function itself needs to be marked as async to make sure the await statement works as intended.
Once you add the async keyword to handleDelete, everything should work perfectly. Nice job identifying the main fix!
Checkout the UI behavior after I forced an error in the deleteAnnotations thunk to simulate a failed API request:
https://github.com/user-attachments/assets/fa7a2925-73e1-4b2f-9be0-187fb943b674
error propagation is working and the UI displays the correct message on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was an issue with my local testing. I thought yarn build would rebuild everything but it apparently doesn't so i'm cleaning the build/ dir now and then rebuilding. I was on the right track- sorry i didn't figure it out entirely myself. thank you for your help!! very validating.

event(`annotations.delete_annotation.success`, {
prefix: props.eventPrefix,
})
Expand All @@ -150,7 +152,7 @@ export const AnnotationForm: FC<Props> = (props: Props) => {
event(`annotations.delete_annotation.failure`, {
prefix: props.eventPrefix,
})
dispatch(notify(deleteAnnotationFailed(err)))
dispatch(notify(deleteAnnotationFailed(getErrorMessage(err))))
}
}

Expand Down
7 changes: 4 additions & 3 deletions src/dashboards/actions/thunks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {setCloneName} from 'src/utils/naming'
import {isLimitError} from 'src/cloud/utils/limits'
import {getOrg} from 'src/organizations/selectors'
import {getByID, getStatus} from 'src/resources/selectors'
import {getErrorMessage} from 'src/utils/api'

// Constants
import * as copy from 'src/shared/copy/notifications'
Expand Down Expand Up @@ -111,7 +112,7 @@ export const createDashboard =
if (isLimitError(error)) {
dispatch(notify(copy.resourceLimitReached('dashboards')))
} else {
dispatch(notify(copy.dashboardCreateFailed()))
dispatch(notify(copy.dashboardCreateFailed(getErrorMessage(error))))
}
}
}
Expand Down Expand Up @@ -199,7 +200,7 @@ export const cloneDashboard =
if (isLimitError(error)) {
dispatch(notify(copy.resourceLimitReached('dashboards')))
} else {
dispatch(notify(copy.dashboardCreateFailed()))
dispatch(notify(copy.dashboardCreateFailed(getErrorMessage(error))))
}
}
}
Expand Down Expand Up @@ -442,7 +443,7 @@ export const updateDashboard =
dispatch(creators.editDashboard(updatedDashboard))
} catch (error) {
console.error(error)
dispatch(notify(copy.dashboardUpdateFailed()))
dispatch(notify(copy.dashboardUpdateFailed(getErrorMessage(error))))
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/flows/context/api.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from 'src/client/notebooksRoutes'
import {notebookUpdateFail} from 'src/shared/copy/notifications'
import {notify} from 'src/shared/actions/notifications'
import {getErrorMessage} from 'src/utils/api'

const DEFAULT_API_FLOW: PatchNotebookParams = {
id: '',
Expand Down Expand Up @@ -99,10 +100,10 @@ export const migrateLocalFlowsToAPI = async (
delete flows[localID]
flows[id] = flow
})
).catch(() => {
).catch(err => {
// do not throw the error because some flows might have saved and we
// need to save the new IDs to avoid creating duplicates next time.
dispatch(notify(notebookUpdateFail()))
dispatch(notify(notebookUpdateFail(getErrorMessage(err))))
})
}
return flows
Expand Down
3 changes: 2 additions & 1 deletion src/flows/context/flow.current.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import PageSpinner from 'src/perf/components/PageSpinner'
import {pageTitleSuffixer} from 'src/shared/utils/pageTitles'
import {RemoteDataState} from '@influxdata/clockface'
import {setCloneName} from 'src/utils/naming'
import {getErrorMessage} from 'src/utils/api'

const prettyid = customAlphabet('abcdefghijklmnop0123456789', 12)

Expand Down Expand Up @@ -84,7 +85,7 @@ export const FlowProvider: FC = ({children}) => {
dispatch(notify(notebookDeleteSuccess()))
return id
} catch (error) {
dispatch(notify(notebookDeleteFail()))
dispatch(notify(notebookDeleteFail(getErrorMessage(error))))
}
}, [dispatch, id])

Expand Down
7 changes: 4 additions & 3 deletions src/flows/context/flow.list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
} from 'src/shared/copy/notifications'
import {setCloneName} from 'src/utils/naming'
import {CLOUD} from 'src/shared/constants'
import {getErrorMessage} from 'src/utils/api'

export interface FlowListContextType extends FlowList {
add: (flow?: Flow) => Promise<string | void>
Expand Down Expand Up @@ -238,8 +239,8 @@ export const FlowListProvider: FC = ({children}) => {

return flow.id
})
.catch(() => {
dispatch(notify(notebookCreateFail()))
.catch(err => {
dispatch(notify(notebookCreateFail(getErrorMessage(err))))
})
}

Expand Down Expand Up @@ -295,7 +296,7 @@ export const FlowListProvider: FC = ({children}) => {
await deleteAPI({id})
dispatch(notify(notebookDeleteSuccess()))
} catch (error) {
dispatch(notify(notebookDeleteFail()))
dispatch(notify(notebookDeleteFail(getErrorMessage(error))))
}

delete _flows[id]
Expand Down
3 changes: 2 additions & 1 deletion src/flows/context/version.publish.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
publishNotebookSuccessful,
} from 'src/shared/copy/notifications'
import {event} from 'src/cloud/utils/reporting'
import {getErrorMessage} from 'src/utils/api'

// Types
import {RemoteDataState} from 'src/types'
Expand Down Expand Up @@ -103,7 +104,7 @@ export const VersionPublishProvider: FC = ({children}) => {
setPublishLoading(RemoteDataState.Done)
handleGetNotebookVersions()
} catch (error) {
dispatch(notify(publishNotebookFailed(flow.name)))
dispatch(notify(publishNotebookFailed(flow.name, getErrorMessage(error))))
setPublishLoading(RemoteDataState.Error)
}
}, [dispatch, handleGetNotebookVersions, flow.id, flow.name])
Expand Down
7 changes: 4 additions & 3 deletions src/labels/actions/thunks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
import {getOrg} from 'src/organizations/selectors'
import {viewableLabels} from 'src/labels/selectors'
import {getStatus} from 'src/resources/selectors'
import {getErrorMessage} from 'src/utils/api'

export const getLabels =
() => async (dispatch: Dispatch<Action>, getState: GetState) => {
Expand Down Expand Up @@ -99,7 +100,7 @@ export const createLabel =
dispatch(setLabel(resp.data.label.id, RemoteDataState.Done, label))
} catch (error) {
console.error(error)
dispatch(notify(createLabelFailed()))
dispatch(notify(createLabelFailed(getErrorMessage(error))))
}
}

Expand All @@ -121,7 +122,7 @@ export const updateLabel =
dispatch(setLabel(id, RemoteDataState.Done, label))
} catch (error) {
console.error(error)
dispatch(notify(updateLabelFailed()))
dispatch(notify(updateLabelFailed(getErrorMessage(error))))
}
}

Expand All @@ -137,6 +138,6 @@ export const deleteLabel =
dispatch(removeLabel(id))
} catch (error) {
console.error(error)
dispatch(notify(deleteLabelFailed()))
dispatch(notify(deleteLabelFailed(getErrorMessage(error))))
}
}
8 changes: 4 additions & 4 deletions src/shared/copy/notifications/categories/dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ export const dashboardsGetFailed = (error: string): Notification => ({
message: `Failed to retrieve dashboards: ${error}`,
})

export const dashboardUpdateFailed = (): Notification => ({
export const dashboardUpdateFailed = (error: string): Notification => ({
...defaultErrorNotification,
icon: IconFont.DashH,
message: 'Could not update dashboard',
message: `Could not update dashboard: ${error}`,
})

export const dashboardDeleted = (name: string): Notification => ({
Expand All @@ -35,9 +35,9 @@ export const dashboardDeleted = (name: string): Notification => ({
message: `Dashboard ${name} deleted successfully.`,
})

export const dashboardCreateFailed = () => ({
export const dashboardCreateFailed = (error: string) => ({
...defaultErrorNotification,
message: 'Failed to create dashboard.',
message: `Failed to create dashboard: ${error}`,
})

export const dashboardCreateSuccess = () => ({
Expand Down
19 changes: 11 additions & 8 deletions src/shared/copy/notifications/categories/notebooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ export const panelCopyLinkFail = (): Notification => ({
message: `Failed to copy the panel link`,
})

export const notebookCreateFail = (): Notification => ({
export const notebookCreateFail = (error: string): Notification => ({
...defaultErrorNotification,
message: `Failed to create Notebook, please try again.`,
message: `Failed to create Notebook: ${error}`,
})

export const notebookUpdateFail = (): Notification => ({
export const notebookUpdateFail = (error: string): Notification => ({
...defaultErrorNotification,
message: `Failed to save changes to Notebook, please try again.`,
message: `Failed to save changes to Notebook: ${error}`,
})

export const notebookDeleteFail = (): Notification => ({
export const notebookDeleteFail = (error: string): Notification => ({
...defaultErrorNotification,
message: `Failed to delete Notebook, please try again.`,
message: `Failed to delete Notebook: ${error}`,
})

export const notebookDeleteSuccess = (): Notification => ({
Expand All @@ -58,7 +58,10 @@ export const publishNotebookSuccessful = (name: string): Notification => ({
message: `Successfully saved this version to ${name}'s version history.`,
})

export const publishNotebookFailed = (name: string): Notification => ({
export const publishNotebookFailed = (
name: string,
error: string
): Notification => ({
...defaultErrorNotification,
message: `Failed to save this version to ${name}'s version history`,
message: `Failed to save this version to ${name}'s version history: ${error}`,
})
12 changes: 6 additions & 6 deletions src/shared/copy/notifications/categories/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@ export const getLabelsFailed = (): Notification => ({
message: 'Failed to fetch labels',
})

export const createLabelFailed = (): Notification => ({
export const createLabelFailed = (error: string): Notification => ({
...defaultErrorNotification,
message: 'Failed to create label',
message: `Failed to create label: ${error}`,
})

export const updateLabelFailed = (): Notification => ({
export const updateLabelFailed = (error: string): Notification => ({
...defaultErrorNotification,
message: 'Failed to update label',
message: `Failed to update label: ${error}`,
})

export const deleteLabelFailed = (): Notification => ({
export const deleteLabelFailed = (error: string): Notification => ({
...defaultErrorNotification,
message: 'Failed to delete label',
message: `Failed to delete label: ${error}`,
})

export const taskNotCreated = (additionalMessage: string): Notification => ({
Expand Down