Refactor links #1811
Refactor links #1811
Conversation
WalkthroughThe pull request encompasses a series of consistent updates across multiple Vue components and views. The changes primarily focus on enhancing button behavior during busy states, introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Button
participant Component
User->>Button: Interact
alt Is Busy
Button->>Component: Check isBusy state
Component-->>Button: isBusy = true
Button->>Button: Disable/Change Rendering
Button-->>User: Prevent Interaction
else Not Busy
Button->>Component: Perform Action
Component->>Component: Execute Operation
end
This sequence diagram illustrates the new button interaction pattern where the 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1811 +/- ##
=============================================
- Coverage 81.47% 77.60% -3.87%
Complexity 1410 1410
=============================================
Files 364 365 +1
Lines 9554 9582 +28
Branches 870 871 +1
=============================================
- Hits 7784 7436 -348
- Misses 1770 2146 +376 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
resources/js/components/AppBanner.vue (1)
37-39: Consolidate styling approach for better maintainability.The button styling uses a mix of approaches:
- Conditional inline styles based on
linkStyle- Conditional classes
- Severity prop
This makes the styling logic harder to maintain and understand.Consider consolidating the styling into a computed property:
+ <script setup> + import { computed } from 'vue'; + + const props = defineProps({ + // ... existing props ... + }); + + const buttonClass = computed(() => ({ + 'p-0 underline': props.linkStyle === 'link', + })); + + const buttonStyle = computed(() => + props.linkStyle === 'link' ? { color: props.color } : {} + ); + </script> <Button v-if="link" as="a" :href="link" :target="linkTarget" :link="linkStyle === 'link'" - :style="linkStyle === 'link' ? { color: color } : {}" - :class="linkStyle === 'link' ? 'p-0 underline' : ''" + :style="buttonStyle" + :class="buttonClass" :severity="linkStyle" :label="linkText ? linkText : link" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/js/components/AppBanner.vue(1 hunks)resources/js/views/MeetingsIndex.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Backend
- GitHub Check: Docker Build
PILOS
|
||||||||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
refactor-links
|
| Run status |
|
| Run duration | 08m 49s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
239
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
tests/Frontend/e2e/UserProfileSecurity.cy.js • 1 failed test • Frontend tests
| Test | Artifacts | |
|---|---|---|
| User Profile Security > load sessions error |
Test Replay
Screenshots
|
|
SamuelWei
force-pushed
the
refactor-links
branch
from
be44520 to
bfaabb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
resources/js/components/AppBanner.vue (3)
30-42: Consider removing the wrapper div.The
<div>wrapper around the<Button>component appears unnecessary as it doesn't provide any styling or structural benefit.- <div> <Button v-if="link" as="a" :href="link" :target="`_${linkTarget}`" :link="linkStyle === 'link'" :style="buttonStyle" :class="buttonClass" :severity="linkStyle" :label="linkText ? linkText : link" /> - </div>
40-40: Make the button label more semantic.Instead of using the link URL as a fallback for the button label, consider providing a more descriptive default text or making
linkTextrequired.- :label="linkText ? linkText : link" + :label="linkText || $t('common.learn_more')"
97-103: Consider combining the button styling logic.The link style check is duplicated in both computed properties. Consider combining them into a single computed property for better maintainability.
-const buttonClass = computed(() => { - return props.linkStyle === "link" ? "p-0 underline" : ""; -}); - -const buttonStyle = computed(() => { - return props.linkStyle === "link" ? { color: props.color } : {}; -}); +const buttonStyling = computed(() => { + if (props.linkStyle === "link") { + return { + class: "p-0 underline", + style: { color: props.color } + }; + } + return { class: "", style: {} }; +});Then update the template:
- :style="buttonStyle" - :class="buttonClass" + :style="buttonStyling.style" + :class="buttonStyling.class"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/js/components/AppBanner.vue(2 hunks)resources/js/views/MeetingsIndex.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/views/MeetingsIndex.vue
🧰 Additional context used
📓 Learnings (1)
resources/js/components/AppBanner.vue (1)
Learnt from: SamuelWei
PR: THM-Health/PILOS#1811
File: resources/js/components/AppBanner.vue:35-35
Timestamp: 2025-01-23T12:12:36.617Z
Learning: In the PILOS project, the `LinkTarget` enum (`app/Enums/LinkTarget.php`) stores target values without underscores ('blank', 'self'). The underscore prefix for HTML target attributes ('_blank', '_self') is added at the component level where the enum values are used.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (1)
resources/js/components/AppBanner.vue (1)
Line range hint
30-103: Overall implementation looks good!The refactoring of the link to use the
Buttoncomponent is well-implemented. It maintains the existing functionality while providing a more consistent component structure. The computed properties effectively handle the styling variations.
There was a problem hiding this comment.
Other Link buttons on index pages inside tables contain :disabled="isBusy" (e.g. AdminServersIndex.vue)
Therefore I would have normally suggested to add this here too, but when testing it locally it did not really have any effect on the link button.
The Link button is still actionable when the meetings are loading even if the disabled is added.
There is an overlay that prevents clicking on anything inside the table but the link button can still be used when navigating with the keyboard (Other link buttons seem to have the same behaviour)
It would maybe be better if the link buttons would not be actionable while loading. If that is not possible maybe the :disabled="isBusy" should be removed for all link buttons to prevent confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
resources/js/components/SettingsServerPoolsDeleteButton.vue (1)
73-76: Newdisabledprop is well-defined.
Defining thedisabledprop as a boolean with a default value offalseis straightforward.
Consider destructuring yourpropsto improve readability (e.g.,const { disabled } = defineProps(...)) if you plan to useprops.disabledin multiple places. This is optional and purely for code clarity.-const props = defineProps({ - ... - disabled: { - type: Boolean, - default: false, - }, -}); +const { id, name, disabled } = defineProps({ + id: { + type: Number, + required: true + }, + name: { + type: String, + required: true + }, + disabled: { + type: Boolean, + default: false + }, +});resources/js/components/SettingsUsersResetPasswordButton.vue (1)
84-87: Consider adding a short inline comment or documentation for the newdisabledprop.While this implementation is correct, providing a descriptive comment helps clarify usage across teams and improves maintainability.
email: { type: String, required: true, }, + /** + * When set to true, this button is disabled regardless of isBusy state. + */ disabled: { type: Boolean, default: false, },resources/js/views/AdminServerPoolsView.vue (1)
27-27: Enhance user feedback for disabled state.By disabling the delete button when
isBusy, you prevent interactions during ongoing operations. Consider adding a loading indicator or spinner to improve user feedback and clarify why the button is disabled.resources/js/components/SettingsUsersDeleteButton.vue (1)
15-15: Ensure coverage for new:disabledlogicUsing a logical OR between
isBusyandprops.disabledis appropriate. However, confirm that you have adequate tests verifying that the button remains disabled when either condition istrue.resources/js/components/SettingsRolesDeleteButton.vue (1)
56-59: Consider aligning naming conventions for Boolean props.You may rename this prop to
isDisabledfor consistency withisBusy, ensuring an intuitive and uniform naming convention for flags. This is merely a suggestion, asdisabledis still valid and clear.-disabled: { +isDisabled: { type: Boolean, default: false, },resources/js/views/AdminRoomTypesIndex.vue (1)
77-77: Use visual feedback when disabled.
As a minor improvement, consider displaying a loading indicator or modifying the button style whenisBusyis true. This provides explicit feedback that navigation is temporarily blocked.resources/js/components/RoomTabRecordingsDownloadButton.vue (1)
4-4: Consider conditionally toggling or removing irrelevant attributes for<button>vs.<a>When
props.disabledis true, this component renders a<button>, yet thetarget="_blank"and:href="downloadUrl"attributes remain in the template, which may not be meaningful for a button element and might cause confusion. To improve semantics and clarity, consider conditionally toggling these attributes or removing them whenprops.disabledis true:<Button v-tooltip:top="$t('rooms.recordings.download')" :as="props.disabled ? 'button' : 'a'" - target="_blank" - :href="downloadUrl" severity="help" icon="fa-solid fa-download" :disabled="props.disabled" :aria-label="$t('rooms.recordings.download')" data-test="room-recordings-download-button" + :target="props.disabled ? null : '_blank'" + :href="props.disabled ? null : downloadUrl" />resources/js/components/SettingsServersDeleteButton.vue (1)
54-57: Add unit tests for the newdisabledprop.The
disabledprop introduction is well-structured with a default value offalse. However, consider adding test coverage to confirm that the button correctly behaves as disabled when this prop is set totrue.resources/js/views/AdminServerPoolsIndex.vue (3)
Line range hint
88-99: Consider using a disabled visual cue for improved user feedback.While conditionally switching the button to
'button'and disabling it whenisBusyis a good way to avoid accidental navigation, it might be helpful to display a loading state or disabled styling (e.g., a spinner or dimming effect) for better user feedback. This way, users clearly understand that navigation actions are temporarily unavailable.
Line range hint
104-119: Align approach with “View” button logic for consistency.The conditional
:as="isBusy ? 'button' : 'router-link'"logic is repeated here. Consider extracting this logic into a shared helper method or component prop to maintain consistent behavior across similar buttons. This refactor would help enforce uniform styling and reduce duplicated code.
120-120: Clarify disabled state with user feedback.Disabling the delete button when
isBusyis consistent with the other actions, preventing accidental deletions. However, for clarity, consider adding a tooltip or messages that inform users why the button is disabled (e.g., “Please wait for the current operation to finish.”). This small addition can enhance usability and user experience.resources/js/components/SettingsRoomTypesDeleteButton.vue (1)
95-98: Consider aligning naming conventions for boolean props.While
disabledis perfectly valid, you might consider naming itisDisabledto match theisBusypattern for consistent readability (e.g.,isDisabled,isBusy). This is a minor style suggestion and it's fine to keep as is if team standards differ.resources/js/views/AdminUsersIndex.vue (2)
188-188: Conditionally remove the 'to' prop when using'button'.When
isBusyis true, the component is rendered as a regular button, yet the:toprop is still set. Although this often works without errors, conditionally removing or nullifying:tocan help prevent unintended warnings or confusion.Example fix:
<Button ... - :as="isBusy ? 'button' : 'router-link'" - :to="{ name: 'admin.users.view', params: { id: slotProps.data.id } }" + :as="isBusy ? 'button' : 'router-link'" + v-bind="!isBusy ? { to: { name: 'admin.users.view', params: { id: slotProps.data.id } } } : {}" ... />
210-210: Apply conditional handling for the 'to' prop similarly.This edit button also toggles between
'button'and'router-link'depending on theisBusystate. Consider using the same pattern above to avoid potential misconfiguration or confusion.resources/js/views/AdminRolesView.vue (2)
7-7: Ensure a consistent user experience when busy.
You're conditionally rendering the<Button>as'button'whenisBusyistrueand disabling it to prevent navigation. This approach is clear and prevents unintended routing while busy, but you might also consider providing a visual indicator (e.g., a spinner) to inform users that the UI is in a loading state.
16-16: Remove redundant navigation attribute when busy.
WhenisBusyistrue, the button is disabled and acts as a non-routable<button>. To simplify, you may want to conditionally omit or nullify the:toprop altogether ifisBusyistrue. This ensures clarity in the rendered DOM, as there is no route to navigate to when the button is disabled.Example diff:
-:as="isBusy ? 'button' : 'router-link'" -:to="{ name: 'admin.roles.edit', params: { id: model.id } }" +:as="!isBusy ? 'router-link' : 'button'" +:to="!isBusy ? { name: 'admin.roles.edit', params: { id: model.id } } : null"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
CHANGELOG.md(3 hunks)package.json(3 hunks)resources/js/components/RoomTabRecordingsDownloadButton.vue(1 hunks)resources/js/components/SettingsRolesDeleteButton.vue(2 hunks)resources/js/components/SettingsRoomTypesDeleteButton.vue(2 hunks)resources/js/components/SettingsServerPoolsDeleteButton.vue(2 hunks)resources/js/components/SettingsServersDeleteButton.vue(2 hunks)resources/js/components/SettingsUsersDeleteButton.vue(2 hunks)resources/js/components/SettingsUsersResetPasswordButton.vue(2 hunks)resources/js/views/AdminRolesIndex.vue(3 hunks)resources/js/views/AdminRolesView.vue(3 hunks)resources/js/views/AdminRoomTypesIndex.vue(3 hunks)resources/js/views/AdminRoomTypesView.vue(3 hunks)resources/js/views/AdminServerPoolsIndex.vue(3 hunks)resources/js/views/AdminServerPoolsView.vue(3 hunks)resources/js/views/AdminServersIndex.vue(3 hunks)resources/js/views/AdminServersView.vue(3 hunks)resources/js/views/AdminUsersIndex.vue(3 hunks)resources/js/views/ForgotPassword.vue(4 hunks)
🔇 Additional comments (32)
resources/js/components/SettingsServerPoolsDeleteButton.vue (1)
5-5: Good use of thedisabledprop alongsideisBusy.
When both considerations (isBusy || props.disabled) combine, it ensures the button is reliably disabled under multiple conditions.resources/js/components/SettingsUsersResetPasswordButton.vue (1)
16-16: Good implementation of the combined disable logic.Using
:disabled="isBusy || props.disabled"makes the button's disabled state flexible, ensuring it remains unclickable during busy operations or when explicitly disabled.resources/js/views/AdminServerPoolsView.vue (2)
7-7: Great use of conditional rendering to block navigation when busy.Conditionally switching the
asprop to'button'during a busy state cleanly prevents premature navigation attempts. This logic is straightforward and helps guard against unintended user actions.
16-16: Consistent approach for both edit and view modes.Mirroring the busy-state logic between this button and the previous one ensures a uniform user experience and code consistency.
resources/js/components/SettingsUsersDeleteButton.vue (1)
74-77: Naming and usage look goodDefining the
disabledprop aligns well with standard HTML attributes. Just ensure that naming is consistent with other Boolean props in your codebase. Otherwise, this looks great!resources/js/components/SettingsRolesDeleteButton.vue (1)
5-5: Good use of combined state for thedisabledattribute.By grouping the
isBusyflag and the newdisabledprop, you ensure that the button properly reflects the intended state under both conditions. This is aligned with best practices for disabling UI elements in Vue.resources/js/views/AdminRoomTypesIndex.vue (2)
93-93: Conditional link/button logic looks solid.
Converting the link to a button while busy prevents accidental navigation and enhances overall reliability.
109-109: Disabling destructive actions is a good safety measure.
Locking the delete button during busy states helps prevent conflicting requests and unintended actions.resources/js/components/SettingsServersDeleteButton.vue (1)
6-6: Good use of combineddisabledlogic.Using
:disabled="isBusy || props.disabled"ensures that both scenarios—busy state and externally disabled prop—prevent interaction. This is a clean and concise approach.resources/js/views/AdminRoomTypesView.vue (3)
7-7: Great approach for preventing navigation when busy.
Using:as="isBusy ? 'button' : 'router-link'"effectively disables navigation while the component is in a busy state, reducing the chance of unintended routing or double-click issues.
16-16: Consistent busy-state handling.
Applying the same pattern to switch the button’s element type based onisBusymaintains uniform behavior across different button actions.
27-27: Appropriate disable behavior.
Disabling the delete button whenisBusyhelps prevent conflicting operations and ensures the user cannot trigger multiple requests simultaneously.resources/js/components/SettingsRoomTypesDeleteButton.vue (1)
5-5: Use the combined disabled condition to prevent undesired interactions during busy states.This ensures the button is properly disabled when the component is busy or explicitly disabled from the outside, improving user experience and consistency.
resources/js/views/AdminRolesIndex.vue (3)
94-94: Smoothly degrade link to button when busy.Conditionally rendering the button as
'button'instead of'router-link'is a great approach to prevent navigation while the app is in a busy state.
108-108: Keep the user experience consistent.Applying the same
:as="isBusy ? 'button' : 'router-link'"logic ensures consistent behavior for both “View” and “Edit” actions, preventing confusion for end users.
124-124: Disable the delete button while busy.Tying the
disabledprop toisBusyprevents accidental clicks and keeps the UI consistent with other disabled states in the component.resources/js/views/ForgotPassword.vue (5)
26-26: Good practice to disable the button while loading.Allowing the button to be disabled while the request is in progress is a reliable way to prevent duplicate submissions and improve user experience.
61-61: Great addition of clearing form errors before a new request.This ensures that outdated error messages are removed, showing only relevant validation feedback on subsequent requests.
62-62: No issues found.
77-84: Granular handling of form validation errors looks good.This approach properly checks for 422 (Unprocessable Entity) and sets the appropriate field errors, giving users helpful feedback. The fallback
api.error(error)call should handle all other unexpected error scenarios.
46-46: Confirm that your environment variable correctly stores HTTP codes.Since you’re loading the constant from
env.js, ensure thatenv.HTTP_UNPROCESSABLE_ENTITYis properly defined (e.g.,422). Otherwise, the logic will not behave as expected.✅ Verification successful
HTTP_UNPROCESSABLE_ENTITY is correctly defined as 422
The environment variable is properly defined in
env.jswith the standard HTTP status code 422 for Unprocessable Entity. The implementation will work as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking the definition of HTTP_UNPROCESSABLE_ENTITY in env.js fd -e js | xargs rg "HTTP_UNPROCESSABLE_ENTITY" -A 5Length of output: 371
resources/js/views/AdminServersView.vue (3)
7-7: Clean approach to disable navigation while busySwitching the
<Button>from a linked component (router-link) to a normal button ('button') whenisBusyistrueis an elegant way to prevent navigation while asynchronous operations are ongoing. Good implementation.
16-16: Consistent button-as-link strategyThis mirrors the same approach from the cancel button, preventing edit navigation when in a busy state. The consistency in logic is commendable.
27-27: Nice consistency for the delete buttonDisabling the delete button while
isBusyaligns it with the other buttons, ensuring no conflicting operations occur during loading states.resources/js/views/AdminServersIndex.vue (3)
Line range hint
212-220: LGTM! Good handling of busy states.The conditional rendering of the view button as either a router link or regular button based on the
isBusystate, combined with thedisabledprop, effectively prevents navigation during loading states. This helps avoid potential race conditions.
Line range hint
228-239: LGTM! Consistent implementation with view button.The edit button follows the same pattern as the view button, maintaining consistency in the UI behavior during busy states.
247-247: Verify disabled state handling in SettingsServersDeleteButton.While the addition of the
disabledprop is consistent with other button changes, let's verify that theSettingsServersDeleteButtoncomponent properly handles the disabled state internally.✅ Verification successful
Disabled state is properly handled in SettingsServersDeleteButton
The component correctly implements the disabled state by combining both the local busy state and the parent-provided disabled prop:
:disabled="isBusy || props.disabled". This ensures the button remains disabled during deletion operations and when explicitly disabled by the parent component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if SettingsServersDeleteButton properly handles the disabled prop # Search for the component definition and its usage of the disabled prop ast-grep --pattern 'const SettingsServersDeleteButton = defineComponent({ $$$ props: { $$$ disabled: $_, $$$ }, $$$ })' # Search for any conditional logic based on the disabled prop rg -A 5 'disabled.*prop' resources/js/components/SettingsServersDeleteButton.vueLength of output: 412
resources/js/views/AdminUsersIndex.vue (2)
234-234: Great move disabling the reset password action during busy states.Disabling this button while the application is busy prevents accidental clicks or multiple requests. This is a helpful user experience improvement.
242-242: Consistent disabling logic for delete action.Nice job ensuring that destructive actions are also protected when the application is busy, preventing partial or conflicting operations.
package.json (2)
23-23: Development dependency updates look good.The following updates are low-risk as they only affect the development environment:
- @eslint/json: ^0.9.1 -> ^0.10.0
- eslint-plugin-jsonc: ^2.18.2 -> ^2.19.0
- prettier-plugin-tailwindcss: ^0.6.10 -> ^0.6.11
Also applies to: 30-30, 39-39
87-87: Verify the vue-multiselect update impact.The update from
3.1.0to3.2.0is a minor version bump for a runtime dependency. Since this could affect the application's behavior, particularly in components using multiselect functionality, verification is needed.Run this script to:
- Check for breaking changes in the changelog
- Find usage of vue-multiselect in the codebase
resources/js/views/AdminRolesView.vue (1)
27-27: Consistently disabling destructive actions while busy.
The:disabled="isBusy"prop prevents deletions from being triggered during ongoing operations. This is a solid pattern that aligns with the other button elements and ensures a safer user flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/js/components/SettingsRoomTypesDeleteButton.vue (1)
104-107: Add prop documentation and runtime validation.While the prop definition is correct, consider these improvements:
- Add JSDoc documentation to explain the prop's purpose
- Consider adding runtime validation for development environments
+/** + * Controls the button's disabled state independently of the busy state. + * When true, prevents user interaction with the delete button. + */ disabled: { type: Boolean, default: false, + validator(value) { + if (process.env.NODE_ENV === 'development' && typeof value !== 'boolean') { + console.warn('SettingsRoomTypesDeleteButton: disabled prop should be a boolean'); + return false; + } + return true; + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)resources/js/components/SettingsRoomTypesDeleteButton.vue(2 hunks)resources/js/views/AdminRoomTypesIndex.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/js/views/AdminRoomTypesIndex.vue
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Frontend Tests (5)
- GitHub Check: Frontend Tests (4)
- GitHub Check: Frontend Tests (3)
- GitHub Check: Frontend Tests (2)
- GitHub Check: System Tests
- GitHub Check: Frontend Tests (1)
- GitHub Check: Visual Tests
- GitHub Check: Backend
🔇 Additional comments (1)
resources/js/components/SettingsRoomTypesDeleteButton.vue (1)
5-5: LGTM! Good enhancement to button state control.The modification properly combines the existing busy state with the new disabled prop, maintaining component accessibility while adding more flexibility for external state control.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
resources/js/components/UserTabOtherSettings.vue (2)
62-62: LGTM! Consider adding TypeScript for better type safety.The implementation of the busy state management is clean and follows Vue's best practices. The watcher correctly propagates the busy state to the parent component.
Consider adding TypeScript to improve type safety. Here's how you could type the emits:
const emit = defineEmits<{ (event: 'updateUser', user: User): void (event: 'notFoundError', error: Error): void (event: 'staleError', error: unknown): void (event: 'busy', value: boolean): void }>()Also applies to: 78-80
Line range hint
4-28: Consider adding a loading overlay for better user feedback.While the busy state is correctly handled for individual components, consider adding a loading overlay to provide clearer visual feedback when the entire form is processing.
Here's a suggested implementation:
<form v-if="model" class="flex flex-col gap-4" @submit="save"> + <div + v-if="isBusy" + class="absolute inset-0 bg-gray-50/50 flex items-center justify-center" + data-test="form-loading-overlay" + > + <i class="fas fa-circle-notch fa-spin text-2xl text-primary"></i> + </div> <div class="field grid grid-cols-12 gap-4" data-test="bbb-skip-check-audio-field" >resources/js/components/UserTabSecurity.vue (2)
7-11: LGTM! Consider using a method for event handlers.The busy state management is consistently implemented across all child components. The implementation follows Vue's best practices for prop and event handling.
Consider extracting the busy event handler to a method for better maintainability:
+ function handleBusyState(state) { + isBusy.value = state; + } <UserTabSecurityRolesAndPermissionsSection :disabled="isBusy" - @busy="(state) => (isBusy = state)" + @busy="handleBusyState" />Also applies to: 21-24, 29-32
53-53: LGTM! Consider adding comments for better documentation.The busy state management implementation in the script section is well-structured and follows Vue's Composition API best practices.
Consider adding comments to improve code documentation:
const emit = defineEmits(["staleError", "updateUser", "notFoundError", "busy"]); + // Tracks the loading state of child components const isBusy = ref(false); + // Propagate busy state changes to parent component watch(isBusy, () => { emit("busy", isBusy.value); });Also applies to: 58-59, 70-72
resources/js/components/UserTabSection.vue (5)
9-12: Consolidate tab disabling states for better UX.All four
<Tab>elements disable themselves only whenisLoadingActionis true. Consider also disabling them ifisBusyis set, or explore merging the two loading states if that aligns better with your UX logic.Also applies to: 16-19, 22-25, 29-32
45-45: Use a shared method to handle busy states.Each child component (
UserTabProfile,UserTabEmail, etc.) emits abusyevent that setsisLoadingAction. Since all four lines have essentially the same inline callback, you could centralize the logic in a reusable handler function.Also applies to: 54-54, 64-64, 74-74
132-132: Ensure consistent naming for loading states.
isLoadingActionis descriptive, but consider if it should matchisBusymore closely, e.g.,isActionBusyorisActionLoading, to maintain naming consistency across the codebase.
145-149: Remove or replace debug console logs.Leaving
console.logstatements in production code can clutter logs. Remove or replace them with a proper logging mechanism if further monitoring is necessary.Here’s a possible diff:
-watch(isBusy, () => { - console.log(isBusy.value); +watch(isBusy, () => { + // Provide an alternative if logging is needed, or remove entirely. emit("busy", isBusy.value); });
150-153: Remove or replace debug console logs inisLoadingActionwatch.Similar to
isBusy, consider removing or replacing theconsole.logstatement within this watch.-watch(isLoadingAction, () => { - console.log(isLoadingAction.value); +watch(isLoadingAction, () => { + // Provide an alternative if logging is needed, or remove entirely. emit("loadingAction", isLoadingAction.value); });resources/js/views/AdminUsersView.vue (2)
46-52: Clear event propagation and reactivity.
The@busyand@loading-actionevent listeners effectively update theisBusyandisLoadingActionstates. Consider adding or extending tests to confirm that these events are emitted at the right times and that the UI responds correctly.
66-67: Neat addition of local reactive states.
DeclaringisBusyandisLoadingActionas reactive references is straightforward. If multiple components rely on these flags, evaluate factoring them into a shared store or context for a more scalable approach.resources/js/components/UserTabEmail.vue (1)
109-109: Consider documenting the new"busy"event.
You've introduced a new"busy"event in addition to"updateUser"and"notFoundError". For clarity and maintenance, it could help future developers if you add a brief note or docstring describing how this event is meant to be used.resources/js/components/UserTabSecurityRolesAndPermissionsSection.vue (4)
16-19: Consider refactoring the nested conditions into a computed property.
While these conditions correctly disable theRoleSelectcomponent, moving them into a computed property (e.g.,isRoleSelectDisabled) can improve readability and maintainability by centralizing the logic.- :disabled=" - isBusy || - viewOnly || - !userPermissions.can('editUserRole', model) || - disabled - " + :disabled="isRoleSelectDisabled"
32-32: Unify the disable state logic for the Button.
This set of conditions is similar to those for theRoleSelect. Consider creating a common computed property for the entire form or for each component to avoid duplication and allow for easier updates in the future.
61-64: Validate prop naming and clarify usage.
Usingdisabledas a prop name is acceptable, but it can sometimes be confused with the native HTMLdisabledattribute when applied to elements. Consider renaming the prop toisDisabledfor clearer intent.
101-104: Consider watching and emitting additional related states.
Currently, only theisBusystate triggers the"busy"event. If you want to track other related states (e.g.,rolesLoading) at a higher level, consider providing a unified “busy” or “loading” signal for the entire component. This can unify the UI’s loading behavior across different operations.resources/js/components/UserTabSecuritySessionsSection.vue (2)
3-3: Consider providing distinct feedback for busy vs error states.
Currently, the overlay is triggered by bothisBusyandloadingError, yet they share the same UI. Differentiating the visual cues for loading and errors can improve user experience.
Line range hint
141-153: Include error handling in 'deleteAllSessions'.
Currently, an API failure may not be evident to the user. SettingloadingError.valuein the catch block or showing a notification would better convey errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
resources/js/components/UserTabEmail.vue(3 hunks)resources/js/components/UserTabOtherSettings.vue(2 hunks)resources/js/components/UserTabProfile.vue(2 hunks)resources/js/components/UserTabSection.vue(7 hunks)resources/js/components/UserTabSecurity.vue(4 hunks)resources/js/components/UserTabSecurityPasswordSection.vue(7 hunks)resources/js/components/UserTabSecurityRolesAndPermissionsSection.vue(4 hunks)resources/js/components/UserTabSecuritySessionsSection.vue(6 hunks)resources/js/views/AdminUsersIndex.vue(3 hunks)resources/js/views/AdminUsersView.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/views/AdminUsersIndex.vue
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (24)
resources/js/components/UserTabSecurity.vue (1)
Line range hint
1-89: Verify alignment with PR objectives.The implementation adds busy state management, which seems to extend beyond the stated PR objective of "refactoring nested button links". While the changes are well-implemented, please clarify if this additional functionality was intended to be part of this PR.
resources/js/components/UserTabSection.vue (1)
128-128: Validate new emits.The additional
busyandloadingActionevents are correctly declared. Verify that parent components are listening to these events if needed.resources/js/components/UserTabProfile.vue (2)
187-187: Event emission for busy state added.The introduction of the
"busy"event indefineEmitsis consistent with the approach of providing clear inter-component communication when the component is undergoing a potential loading or processing state.
211-214: Watch forisBusyto emit parent notification.Watching
isBusyand emitting its updated value up the chain is clean and maintains the unidirectional data flow. Ensure the parent or other consumers of this"busy"event are prepared to handle repeated or null states, preventing potential infinite loops if the parent also togglesisBusy.resources/js/views/AdminUsersView.vue (4)
7-12: Dynamic link-button approach looks good!
Switching theButtoncomponent'sasprop from'router-link'to'button'prevents unexpected navigation during loading states. This enhances user experience. No further changes needed here.
16-22: Consistent usage of disabled logic on edit button.
Applying the sameaslogic and:disabledbinding ensures a uniform experience for users, preventing accidental navigation while busy. Well done!
33-33: Ensuring reset password actions are blocked during busy states.
Good job on adding:disabled="isBusy || isLoadingAction"to the reset password button to avoid collisions in user actions. This reduces the risk of concurrency issues.
40-40: Preventing deletions when the system is busy.
Adding the disabled logic to the delete button aligns with best practices for critical user actions—ensuring stable, predictable behavior.resources/js/components/UserTabEmail.vue (2)
90-90: Importing thewatchfunction is well-aligned with the new busy state handling.
This import is necessary for monitoring and reacting toisBusy. Nothing appears to be amiss.
132-135: Verify the parent is using the"busy"event effectively.
Emitting the"busy"event on eachisBusychange is a solid approach. Ensure the parent component is actually handling this event; otherwise, it won’t provide the intended UI feedback.✅ Verification successful
The
"busy"event is properly handled by the parent component
The parent componentUserTabSection.vuecorrectly handles the busy event by updating itsisLoadingActionstate, which aligns with the intended UI feedback pattern used throughout the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the "busy" event throughout the codebase. rg '@busy=' -A3Length of output: 5029
resources/js/components/UserTabSecurityRolesAndPermissionsSection.vue (1)
67-67: Event emission for busy state looks good.
Emitting"busy"allows parent components to respond and coordinate UI changes effectively. This approach is consistent with Vue best practices.resources/js/components/UserTabSecurityPasswordSection.vue (8)
21-21: Prop usage for input fields is consistent.
Using bothisBusyanddisabledto control theInputText's disabled state is a clear, combined condition that ensures the field is unusable when either the form is busy or programmatically disabled.
42-42: Aligned disabled condition on new password field.
Maintaining consistency helps users immediately see that all password fields share the same disabled logic.
66-66: Properly handled disabled state for confirmation field.
Ensuring all related input fields use the same condition helps maintain a coherent user experience.
77-77: Synchronized button disabling.
Including:disabled="isBusy || disabled"on the Button prevents users from submitting changes until the form is ready, enhancing usability.
92-92: Importedwatchfrom Vue.
This addition indicates a new reactive approach to observe property changes. It's a straightforward change, and usage looks appropriate.
103-106: Newdisabledprop extends flexibility.
Defining a dedicateddisabledprop is clean and consistent with Vue's best practices. However, confirm there’s no naming conflict with built-in attributes in child components (e.g.,<Button>).
109-109: Emitting"busy"event adds lifecycle awareness.
This event allows parent components to stay in sync with the busy state, promoting better control and improved UX. Consider adding a small note in the component’s documentation for clarity.
126-128: Watch handler neatly communicates state changes.
By emitting"busy"wheneverisBusychanges, you allow external components to respond effectively. This is a solid approach for reactive design.resources/js/components/UserTabSecuritySessionsSection.vue (5)
57-61: Unified button states look good.
Disabling the button and showing a loading spinner based onisBusy,loadingError, or thedisabledprop is consistent and straightforward for users.
71-71: Appropriate imports detected.
All composables and external libraries appear to be in use with no extraneous imports.
77-82: New 'disabled' prop is well-defined.
Defining a boolean prop with a default ensures clarity when toggling button states externally.
84-87: Well-structured reactive references.
isBusyandloadingErrorare straightforward, and thedisabledprop integrates nicely as well. If you need more detailed error handling, consider storing the entire error object rather than a boolean.
94-96: Emitting a ‘busy’ event through watch is a clean approach.
This pattern neatly synchronizes loading states with parent components without extraneous boilerplate.
| isBusy.value = true; | ||
| api | ||
| .call("sessions") | ||
| .then((response) => { | ||
| loadingError.value = false; | ||
| isBusy.value = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify error handling in the API call flow.
Setting isBusy to true in the catch block is reverted by the final block, making it ineffective. If you intend to keep an error overlay visible, remove or modify line 117, and consider setting loadingError.value to the error for better feedback.
Also applies to: 117-121
Type
Checklist
Changes
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
disabledprop to various delete and action buttons across admin settings components.Improvements
Bug Fixes
UI/UX Changes