chore: improve error notifications in annotations and notebooks

[philjb]

I noticed that annotations, dashboards, labels, and notebooks sometimes display the api error when requests fail and sometimes don't- they are not consistent about it. I'd like to have it be consistent to be clear about errors.

I did not improve all error messages: variables, secrets, tasks, etc could also be improved but this is progress!

This needs to be ported to the OSS release branch.

Examples of the error notification popups:

Checklist

Authors and Reviewer(s), please verify the following:

• A PR description, regardless of the triviality of this change, that communicates the value of this PR
• Well-formatted conventional commit messages that provide context into the change
• Documentation updated or issue created (provide link to issue/PR)
• Signed CLA (if not already signed)
• Feature flagged, if applicable

just noticed you marked this back to "Draft". Ping me when it's ready for a review and I'll take a look

[philjb]

@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.

[philjb]

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.

[philjb]

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))))
	}
	}

[blegesse-w]

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.

[blegesse-w]

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

[philjb]

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.

[blegesse-w]

@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

[blegesse-w]

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.

[philjb]

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.

[blegesse-w]

PR LGTM. But I do see a few unrelated failed monitor ci tests. You might need to push an empty commit so the tests can run on a clean pipeline.

[philjb]

PR LGTM. But I do see a few unrelated failed monitor ci tests. You might need to push an empty commit so the tests can run on a clean pipeline.

Thanks Beth!

These are the test failing that i see

2024-08-28T20:23:53.210Z        WARN      - job remocal-tsm-ui-test-chrome-1-tsm-shared/annotations.band failed (status: failed)
2024-08-28T20:23:53.210Z        WARN      - job remocal-tsm-ui-test-chrome-1-tsm-shared/tasks-searching failed (status: failed)

[wdoconnell]

@philjb UI feature branches are clearing our tests properly now. You should be able to rebase against the current master branch and force-push to successfully re-run. Thanks for your patience.

[philjb]

master merged in!

[blegesse-w]

Thank you for doing this!