Add spinner to organization creation in work experiences #2683
Conversation
WalkthroughThe changes in Changes
Poem
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
🧹 Outside diff range and nitpick comments (1)
frontend/src/modules/organization/components/shared/organization-select.vue (1)
97-119: Consider improving error handling and validationWhile the loading state management is well implemented, there are a few areas that could be improved:
- The error case silently returns null without user feedback
- The error is caught but not logged, which might make debugging harder
- The organization creation payload could benefit from validation before submission
Consider applying these improvements:
const createOrganization = (value: string) => { isCreatingOrganization.value = true; + + if (!value?.trim()) { + isCreatingOrganization.value = false; + return Promise.reject(new Error('Organization name is required')); + } return OrganizationService.create({ name: value, attributes: { name: { default: value, custom: [value], }, }, }) .then((newOrganization) => ({ id: newOrganization.id, label: newOrganization.displayName || newOrganization.name, displayName: newOrganization.displayName || newOrganization.name, name: newOrganization.displayName || newOrganization.name, })) - .catch(() => null) + .catch((error) => { + console.error('Failed to create organization:', error); + // Assuming you have a toast notification system + useToast().error('Failed to create organization. Please try again.'); + return null; + }) .finally(() => { isCreatingOrganization.value = false; }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/modules/organization/components/shared/organization-select.vue(5 hunks)
🔇 Additional comments (3)
frontend/src/modules/organization/components/shared/organization-select.vue (3)
6-6: LGTM! Well-implemented loading state UI
The loading state implementation provides clear visual feedback through both the placeholder text and spinner component. The conditional rendering is properly implemented using Vue's template syntax.
Also applies to: 14-16
58-58: LGTM! Required imports added
The necessary imports have been added for the new functionality, following the project's import structure.
Also applies to: 67-67
77-77: LGTM! Clear and typed state declaration
The loading state is properly declared using Vue's ref with appropriate typing and initial value.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/src/modules/organization/components/shared/organization-select.vue(5 hunks)
🔇 Additional comments (3)
frontend/src/modules/organization/components/shared/organization-select.vue (3)
6-6: LGTM: Well-implemented loading state feedback!
The dynamic placeholder and loading spinner provide clear visual feedback during organization creation. The conditional rendering is properly structured.
Also applies to: 14-16
58-58: LGTM: Required imports added correctly.
The necessary imports for ref and LfSpinner component are properly added.
Also applies to: 67-67
77-77: LGTM: Well-typed reactive state.
The loading state is properly implemented using a typed ref.
| const createOrganization = (value: string) => { | ||
| isCreatingOrganization.value = true; | ||
|
|
||
| return OrganizationService.create({ | ||
| name: value, | ||
| attributes: { | ||
| name: { | ||
| default: value, | ||
| custom: [value], | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| .then((newOrganization) => ({ | ||
| id: newOrganization.id, | ||
| label: newOrganization.displayName || newOrganization.name, | ||
| displayName: newOrganization.displayName || newOrganization.name, | ||
| name: newOrganization.displayName || newOrganization.name, | ||
| })) | ||
| .catch(() => null); | ||
| }) | ||
| .then((newOrganization) => ({ | ||
| id: newOrganization.id, | ||
| label: newOrganization.displayName || newOrganization.name, | ||
| displayName: newOrganization.displayName || newOrganization.name, | ||
| name: newOrganization.displayName || newOrganization.name, | ||
| })) | ||
| .catch(() => null) | ||
| .finally(() => { | ||
| isCreatingOrganization.value = false; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and user feedback.
While the loading state management is good, there are a few concerns with the error handling:
- Silent error handling might confuse users when organization creation fails
- Returning null in the catch block could lead to unexpected behavior
- The loading state cleanup is correct but could be more robust
Consider this improved implementation:
const createOrganization = (value: string) => {
isCreatingOrganization.value = true;
return OrganizationService.create({
name: value,
attributes: {
name: {
default: value,
custom: [value],
},
},
})
.then((newOrganization) => ({
id: newOrganization.id,
label: newOrganization.displayName || newOrganization.name,
displayName: newOrganization.displayName || newOrganization.name,
name: newOrganization.displayName || newOrganization.name,
}))
- .catch(() => null)
+ .catch((error) => {
+ // Notify user about the error
+ ElMessage.error('Failed to create organization. Please try again.');
+ // Re-throw to prevent selecting a null value
+ throw error;
+ })
.finally(() => {
isCreatingOrganization.value = false;
});
};Committable suggestion skipped: line range outside the PR's diff.
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Bug Fixes