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

#4489 Report tab slice errors properly #4623

Merged
merged 3 commits into from
Nov 7, 2022
Merged

Conversation

BLoe
Copy link
Collaborator

@BLoe BLoe commented Nov 7, 2022

What does this PR do?

Demo

https://www.loom.com/share/2e5948b1265444c19f6affe01c05b0ce

Checklist

  • Add tests
  • Run Storybook and manually confirm that all stories are working
  • Designate a primary reviewer - @BALEHOK

@BLoe BLoe requested a review from BALEHOK November 7, 2022 16:25
@BLoe BLoe self-assigned this Nov 7, 2022
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #4623 (f8fee64) into main (69bdc55) will increase coverage by 0.48%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##             main    #4623      +/-   ##
==========================================
+ Coverage   50.90%   51.39%   +0.48%     
==========================================
  Files         915      915              
  Lines       26985    27122     +137     
  Branches     5515     5515              
==========================================
+ Hits        13736    13938     +202     
+ Misses      12313    12250      -63     
+ Partials      936      934       -2     
Impacted Files Coverage Δ
src/pageEditor/tabState/tabStateSlice.ts 33.33% <0.00%> (+33.33%) ⬆️
src/testUtils/testHelpers.tsx 96.07% <ø> (ø)
src/pageEditor/EditorContent.tsx 74.57% <100.00%> (+74.57%) ⬆️
src/pageEditor/slices/editorSlice.ts 55.73% <100.00%> (+1.34%) ⬆️
src/pageEditor/testHelpers.ts 100.00% <100.00%> (ø)
src/pageEditor/slices/editorSelectors.ts 84.92% <0.00%> (+1.58%) ⬆️
src/pageEditor/utils.ts 78.57% <0.00%> (+2.38%) ⬆️
src/components/Alert.tsx 75.00% <0.00%> (+15.00%) ⬆️
src/pageEditor/tabs/logs/useLogsBadgeState.ts 29.41% <0.00%> (+29.41%) ⬆️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@BLoe BLoe merged commit 9360f02 into main Nov 7, 2022
@BLoe BLoe deleted the bug/4489-thunk-errors-not-reported branch November 7, 2022 17:55
@fregante
Copy link
Collaborator

fregante commented Nov 8, 2022

I'm not familiar with how slices work. How likely is it that two "nested slices" get the same error and report it twice?

@BLoe
Copy link
Collaborator Author

BLoe commented Nov 8, 2022

I'm not familiar with how slices work. How likely is it that two "nested slices" get the same error and report it twice?

Redux slices created with redux toolkit aren't nested within each other, is that what you're talking about?

@fregante
Copy link
Collaborator

fregante commented Nov 8, 2022

Basically we had issues in the past with "reportError and re-throw", which meant that the error was caught again downstream and reported again. Can that happen here?

I see that each slice has to "return an error" (state.error = error) while also reporting it, meaning that the error will be available outside the slice. If the returned error is used exclusively for display, then it won't happen and this fine.

@@ -133,6 +134,7 @@ export const tabStateSlice = createSlice({
"PixieBrix was updated or restarted. Reload the Page Editor to continue."
);
state.error = serializeError(error);
reportError(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be dropped, context invalidation errors are not reported.

@twschiller twschiller modified the milestones: 1.7.13, 1.7.12 Nov 9, 2022
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.

RTK errors are swallowed (no log, no report)
4 participants