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

Add loading indicator for each tab and add error screens for errors in each tab #1459

Merged
merged 37 commits into from
Aug 20, 2024

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Jul 25, 2024

Screenshot 2024-07-25 at 12 32 06 AM Screenshot 2024-07-25 at 12 32 12 AM

Copy link

relativeci bot commented Jul 25, 2024

#790 Bundle Size — 1.51MiB (+0.45%).

54ce7e9(current) vs 756cf06 main#785(baseline)

Warning

Bundle contains 13 duplicate packages – View duplicate packages

Warning

Bundle introduced one new package: react-error-boundary – View changed packages

Bundle metrics  Change 5 changes Regression 2 regressions
                 Current
#790
     Baseline
#785
Regression  Initial JS 1.47MiB(+0.46%) 1.46MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 92.81% 0%
No change  Chunks 5 5
No change  Assets 12 12
Change  Modules 1213(+0.25%) 1210
No change  Duplicate Modules 45 45
Change  Duplicate Code 3.07%(-0.32%) 3.08%
Regression  Packages 183(+0.55%) 182
No change  Duplicate Packages 10 10
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#790
     Baseline
#785
Regression  JS 1.47MiB (+0.46%) 1.46MiB
No change  IMG 35.85KiB 35.85KiB
No change  HTML 810B 810B
No change  Other 778B 778B

Bundle analysis reportBranch jerel/loading-error-indicatorProject dashboard


Generated by RelativeCIDocumentationReport issue

@jerelmiller jerelmiller force-pushed the jerel/loading-error-indicator branch from dff343e to 71a5949 Compare August 17, 2024 22:18
@jerelmiller jerelmiller marked this pull request as ready for review August 17, 2024 22:51
@jerelmiller jerelmiller requested a review from a team as a code owner August 17, 2024 22:51
Comment on lines +75 to +85
if (error) {
throw error;
}
Copy link
Member

Choose a reason for hiding this comment

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

If I add something along the lines of

  if (2 > 1)
    throw new ApolloError({ networkError: new Error("Network error") });

here, the whole extension always ends up in a "Client not found" state.

Could you look into that a bit more?

Copy link
Member Author

@jerelmiller jerelmiller Aug 19, 2024

Choose a reason for hiding this comment

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

I'm not sure I follow. Let's connect tomorrow (Tuesday).

Edit: I think I understand now that you're adding this arbitrarily to see how the UI behaves as if that error had occurred (that 2 > 1 is so that you can create an "always true" condition ya?).

I'd still love to connect to understand what you're looking for here. I'm intentionally taking over the whole screen to show the error since any error here is unexpected and usually means we can't communicate with the content scripts but perhaps you have some better ideas.

Copy link
Member

@phryneas phryneas Aug 20, 2024

Choose a reason for hiding this comment

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

Yeah, the 2>1 is to keep the TS compiler from yelling that a lot of code after the fixed throw will not be executed.

Generally, I was just throwing errors here to test these changes. What stood out for me was that doing this in the mutation panel had no effects on the connection, but doing so from within the queries panel made the whole connection impossible (probably because this is the first panel we show).
This might be perfectly okay as my test was a bit unrealistic, but it felt a bit weird and I wanted to point it out.

@phryneas
Copy link
Member

I also encountered this error, which might be related - but I cannot reproduce it consistently.

https://github.com/apollographql/apollo-client-devtools/issues/new?body=Error%20on%20Mutations%20tab%3A%0A%0A%60%60%60%0AApolloError%3A%20RPC_MESSAGE_TIMEOUT%0A%0AApolloError%0A%20%20%20%20at%20new%20ApolloError%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A63624%3A28)%0A%20%20%20%20at%20chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A63018%3A47%0A%20%20%20%20at%20both%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A68575%3A31)%0A%20%20%20%20at%20chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A68564%3A72%0A%20%20%20%20at%20new%20Promise%20(%3Canonymous%3E)%0A%20%20%20%20at%20Object.then%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A68564%3A24)%0A%20%20%20%20at%20Object.next%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A68577%3A49)%0A%20%20%20%20at%20notifySubscription%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A176104%3A18)%0A%20%20%20%20at%20onNotify%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A176148%3A3)%0A%20%20%20%20at%20SubscriptionObserver.next%20(chrome-extension%3A%2F%2Fgchmlaakeogdadclibhedcdohhknlhnc%2Fpanel.js%3A176197%3A5)%0A%60%60%60%0A%0A%23%23%23%20Additional%20details%0A%3C!--%20Please%20provide%20any%20additional%20details%20of%20the%20issue%20here%2C%20otherwise%20feel%20free%20to%20delete%20this%20section.%20--%3E%0A%0A%23%23%23%20%60%40apollo%2Fclient%60%20version%0A%3C!--%20Please%20provide%20the%20version%20of%20%60%40apollo%2Fclient%60%20you%20are%20using.%20--%3E%0A%0A%23%23%23%20Apollo%20Client%20Devtools%20version%0A4.17.6&labels=%F0%9F%90%9E+bug

@jerelmiller
Copy link
Member Author

jerelmiller commented Aug 19, 2024

@phryneas that error usually occurs when you "update" the extension, whether thats the browser doing it in the background, reloading the extension in dev, or having the webpack extension reload it for you by updating code. Each of these invalidate the extension context so the devtools script is no longer able to communicate to the background service worker script (since that service worker has now been replaced with the updated extension).

I plan to address this separately. So far I can't seem to find any way to automatically reconnect the devtools script with the new service worker without having the user close and reopen devtools. I suppose that makes sense anyways since there might be new code in the devtools script. For that we should probably show a message to the user that the user needs to close and reopen. I'd like to do that separately (in fact, I should probably create an issue for this edit: #1476).

Previously though, when that update would occur, the timeout would mean that the UI appears frozen since we can no longer get messages from the background script, so you'd stop receiving updates from the client. With this change, we at least see an error page so you know something went wrong so its a "something better than nothing" approach for now.

@jerelmiller jerelmiller force-pushed the jerel/loading-error-indicator branch from d5d468a to 617c080 Compare August 20, 2024 18:28
@jerelmiller jerelmiller merged commit b4a600b into main Aug 20, 2024
9 checks passed
@jerelmiller jerelmiller deleted the jerel/loading-error-indicator branch August 20, 2024 18:47
@github-actions github-actions bot mentioned this pull request Aug 20, 2024
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.

2 participants