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

Integ 231 improve error handling within analytics app component #2919

Merged

Conversation

lilbitner
Copy link
Contributor

@lilbitner lilbitner commented Mar 24, 2023

Purpose

This PR accomplishes the following:

  1. Adds more robust error handling for the common errors when rendering the GA4 app in the Sidebar. ⛔
  2. Fixes some small UI issues. ✨

Approach

Error handling:

I added a component here called the ErrorDisplay component that is rendered in place of the Chart itself when an error is thrown. This component handles the anticipated two most common errors, providing an invalid argument somehow which is unlikely (invalid propertyId.), or a disabled Data api error. If the error does not belong to one of these error types, then the provided error message is displayed. I added a comment on this code to perhaps adjust the logic to display an actionable default error message if the error is not properly identified by the frontend, instead of the "mystery" error message.

Example error message for invalid arguments:

Screenshot 2023-03-24 at 4 35 48 PM

Small UI fixes:

Note content not wrapping appropriately:

Before:

Screenshot 2023-03-24 at 9 03 30 AM

After:

Screenshot 2023-03-24 at 9 03 15 AM

Outline of Date range Select menu cut-off on right side:

Before:

Screenshot 2023-03-27 at 8 27 54 AM

After:

Screenshot 2023-03-27 at 8 27 35 AM

@lilbitner lilbitner force-pushed the INTEG-231-Improve-error-handling-within-AnalyticsApp-component branch from 0b40b81 to 0bc8116 Compare March 24, 2023 21:16
@lilbitner lilbitner changed the title DRAFT (not ready for review) Integ 231 improve error handling within analytics app component Integ 231 improve error handling within analytics app component Mar 27, 2023
@lilbitner lilbitner marked this pull request as ready for review March 27, 2023 17:14
@lilbitner lilbitner requested a review from a team as a code owner March 27, 2023 17:14
Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

Looks great to me! Just added a couple small comments/questions!

Copy link
Contributor

@ryunsong-contentful ryunsong-contentful left a comment

Choose a reason for hiding this comment

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

Everything looked good as far as the stuff you mentioned UI/Error Handling and overall great work on much needed fixes!

Two things however

  1. Storing the React component in state.
  2. I found two unhandled error cases (which produces the screenshots below)
    a. One where you kill the lambda (ie no server/server error/500s)
    b. Invalid service key

Screenshot 2023-03-27 at 4 25 50 PM
Screenshot 2023-03-27 at 4 26 53 PM


Now this PR only says to improve the error handling. So if this is just a foundation PR for handling Errors, I'm cool with you creating a ticket and handling those fixes separately as your next task (I'm also fine with you fixing them in this PR).
The second point will be a relatively annoying refactor for you but I think it'll prevent a lot of headaches down the road and is worth fixing in this PR.

@lilbitner lilbitner force-pushed the INTEG-231-Improve-error-handling-within-AnalyticsApp-component branch from 84bf27e to 94d1dcc Compare March 28, 2023 17:41
@lilbitner lilbitner merged commit 71f9e56 into master Mar 28, 2023
@lilbitner lilbitner deleted the INTEG-231-Improve-error-handling-within-AnalyticsApp-component branch March 28, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants