Avoid onboarding screen if /api/organizationsIsEmpty fails #8356
Conversation
📝 WalkthroughWalkthroughThe pull request addresses an issue with the onboarding screen appearing incorrectly when a request to check organization existence fails. The changes modify error handling in the main application initialization process, specifically in the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/main.tsx (1)
98-109: LGTM! Good error handling implementation.The changes effectively prevent the onboarding screen from appearing on API failures and provide good user feedback.
Consider enhancing the error message for better accessibility:
- <p>Failed to load WEBKNOSSOS. Please try again or check the console for details.</p>, + <p role="alert" style={{ margin: '20px' }}> + Failed to load WEBKNOSSOS. Please try again or check the console for details. + </p>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/main.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/main.tsx (1)
76-77: LGTM! Good simplification of error handling.Removing the try-catch block here allows errors to propagate up to the main error handler, which aligns with the PR objectives of properly handling API failures.
CHANGELOG.unreleased.md (1)
19-19: LGTM! Clear and well-placed changelog entry.The entry accurately describes the fix and follows the changelog format.
|
@philippotto So this is my initial suggestion. I am a little insecure whether I tested it enough and all the way, because I made changes in such a meaningful component. I am looking forward to hear what you think! |
frontend/javascripts/main.tsx
Outdated
| await Promise.all([loadFeatureToggles(), loadActiveUser(), loadHasOrganizations()]); | ||
| await Promise.all([loadOrganization()]); |
There was a problem hiding this comment.
there are now 4 function calls in this new try block. for loadHasOrganizations, you already did the testing. but what about the others? I'm not sure what their failure would cause on master (for example, what happens if loadOrganization fails). maybe the new error makes sense, but maybe it's too strict 🤔 could you test these scenarios, too?
There was a problem hiding this comment.
you are right that I did not test this enough!
If loadOrganizations fails, the same failure screen is rendered as when loadHasOrganizations fails. This is probably too strict.
An obvious solution would be to move all other api calls outside the try and catch block. the disadvantage would be that one more single await is introduced where it is not needed (within the try-catch). But thats probably the trade-off that has to be made, I dont have a simple solution where this is not the case 😅
There was a problem hiding this comment.
I noticed that if loadFeatureToggles yields an error, a blank screen is rendered, so I included it in the try-catch.
There was a problem hiding this comment.
I noticed that if
loadFeatureTogglesyields an error, a blank screen is rendered, so I included it in the try-catch.
Isn't the same true for loadOrganization? My understanding of the current master is the following in the error cases:
- loadFeatureToggles: causes blank screen; therefore, needs try-catch with error screen
- loadActiveUser: causes logged-out screen (this is alright); has try-catch + ignore error in implementation
- loadHasOrganizations: causes onboarding screen; had try-catch + ignore but should not have it. instead, needs try-catch with error screen.
- loadOrganization: causes blank screen; therefore, needs try-catch with error screen
so, in summary, 3 of the functions need a try-catch with error screen. your original commit was good 🙈 sorry for the confusion, I just wanted to know if everything is "as good as or better than before".
my final suggestion would be:
// note: rename loadActiveUser to tryToLoadActiveUser
try {
await Promise.all([
loadFeatureToggles(),
// This function call cannot error as it has a try-catch built-in
tryToLoadActiveUser(),
// *Don't* ignore errors in this request. We only want
// to show an onboarding screen if the back-end replied
// with hasOrganizations==true.
loadHasOrganizations(),
]);
await loadOrganization();
} catch (exc) {
// ...
}what do you think? :)
There was a problem hiding this comment.
I think its good that we talk about this in detail! 🙏
loadOrganization: causes blank screen; therefore, needs try-catch with error screen
Oh, I understand that this depends on the test scenario! During login, I got an Action Not Found toast and I thought that was better than the error screen. But if the user is logged in and suddenly the request fails, the blank screen is rendered.
Right, of cause loadActiveUser cant cause an error, so it can be included into the try-catch. 👍
There was a problem hiding this comment.
Oh, I understand that this depends on the test scenario! During login, I got an
Action Not Foundtoast and I thought that was better than the error screen. But if the user is logged in and suddenly the request fails, the blank screen is rendered.
could it be that you tested the "open wk & log in" scenario and I tested the "open wk while being logged in"? I also simply added throw Error() to the loadOrganization function (not sure whether that makes a difference).
There was a problem hiding this comment.
could it be that you tested the "open wk & log in" scenario and I tested the "open wk while being logged in"?
yes, I think thats right 😅
knollengewaechs
deleted the
avoid-onboarding-screen-when-organizationisempty-fails
branch
URL of deployed dev instance (used for testing):
Steps to test:
organizationstable was altered when the issue occured, so maybe try to do that. This somehow did not trigger the described behaviour for me though.)checkAnyOrganizationExists()because this triggers a 404 response and the described behaviour, redirecting you to the onboarding screen.TODOs:
I asked for help in this thread
Issues:
(Please delete unneeded items, merge only when none are left open)