Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a comprehensive suite of features for managing groups (server and VM) in the application. It adds strongly typed group APIs, a Pinia store for group state, and multiple Vue components for group creation, listing, actions, details, and visual flow. The history and server modules are also enhanced with new types, filtering, and paginated loading. Extensive localization updates support the new UI and workflows. Changes
Sequence Diagram(s)Group Creation and Management FlowsequenceDiagram
participant User
participant GroupView
participant CreateGroupModal
participant GroupStore
participant GroupAPI
User->>GroupView: Click "Create Group"
GroupView->>CreateGroupModal: Open modal
CreateGroupModal->>User: User fills form & submits
CreateGroupModal->>GroupView: Emit 'create' with payload
GroupView->>GroupStore: addGroup(payload)
GroupStore->>GroupAPI: createGroup(payload)
GroupAPI-->>GroupStore: Return new group
GroupStore-->>GroupView: Update state
GroupView-->>User: Show success, update UI
Group List Loading with PaginationsequenceDiagram
participant User
participant GroupList
participant GroupStore
participant GroupAPI
User->>GroupList: Scrolls or clicks "Load More"
GroupList->>GroupStore: loadMore()
GroupStore->>GroupAPI: fetchGroups(params, append=true)
GroupAPI-->>GroupStore: Return page of groups
GroupStore-->>GroupList: Append to group list
GroupList-->>User: Display additional groups
History Event Detail ViewingsequenceDiagram
participant User
participant HistoryListView
participant HistoryEventDetail
User->>HistoryListView: Clicks "Details" on event
HistoryListView->>HistoryEventDetail: Open modal with event data
HistoryEventDetail-->>User: Show detailed event info
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 23
🧹 Nitpick comments (15)
src/features/history/api.ts (1)
30-37: Fix incomplete JSDoc documentation.The JSDoc example is incomplete and cuts off mid-sentence. Complete the example to show the full response structure.
Apply this diff to fix the documentation:
- /** Get Available entity types for history filtering - * @returns Promise resolving with an array of entity type strings. - * @example - * { - * "entityTypes": [ - * "user", - * "organization", - */ + /** Get Available entity types for history filtering + * @returns Promise resolving with an array of entity type strings. + * @example + * { + * "entityTypes": [ + * "user", + * "organization", + * "server", + * "network" + * ] + * } + */src/features/history/store.ts (1)
37-52: Improve JSDoc formatting consistency.The method implementation is solid and follows the store patterns well. However, the JSDoc formatting could be improved for consistency.
Apply this diff to improve the JSDoc formatting:
- /** Get Available entity types for history filtering by calling API - * @returns Promise resolving with an array of entity type strings. - * @example - * { - * "entityTypes": [ - * "user", - * "organization", - * "server", - * "network", - * ] - */ + /** + * Get Available entity types for history filtering by calling API + * @returns Promise resolving with an array of entity type strings. + * @example + * { + * "entityTypes": [ + * "user", + * "organization", + * "server", + * "network" + * ] + * } + */src/features/groups/components/GroupFlow.vue (5)
241-249: Improve type safety in getNodeColorThe type assertion on line 248 could be avoided with better type checking.
Consider using a type guard or avoiding the assertion:
const getNodeColor = (node: Node) => { const priorityColors = { }; - return priorityColors[node.data.priority as 1 | 2 | 3 | 4] || '#6b7280'; + const priority = node.data.priority; + if (priority in priorityColors) { + return priorityColors[priority as keyof typeof priorityColors]; + } + return '#6b7280'; };
9-9: Simplify the arrow function call.The arrow function wrapper is unnecessary when calling
fitViewwithout arguments.- @click="() => fitView()" + @click="fitView"
155-160: Extract magic numbers as constants for better maintainability.The position values and spacing should be configurable constants.
+const HORIZONTAL_SPACING = 300; +const VERTICAL_SPACING = 120; +const NODE_WIDTH = 250; + const nodes = computed<Node[]>(() => { const priorityPositions = { - 1: { x: 0, y: 0 }, - 2: { x: 300, y: 0 }, - 3: { x: 600, y: 0 }, - 4: { x: 900, y: 0 }, + 1: { x: 0 * HORIZONTAL_SPACING, y: 0 }, + 2: { x: 1 * HORIZONTAL_SPACING, y: 0 }, + 3: { x: 2 * HORIZONTAL_SPACING, y: 0 }, + 4: { x: 3 * HORIZONTAL_SPACING, y: 0 }, };
179-181: Use consistent spacing constants.Replace the hardcoded value with the constant for consistency.
const position = orientation.value === 'horizontal' - ? { x: basePos.x, y: index * 120 } - : { x: index * 250, y: basePos.x }; + ? { x: basePos.x, y: index * VERTICAL_SPACING } + : { x: index * NODE_WIDTH, y: basePos.x };
192-195: Simplify nested ternary operators for better readability.Extract the pluralization logic into a helper function.
+const getResourceLabel = (count: number, type: 'server' | 'vm') => { + if (type === 'server') { + return count === 1 ? 'server' : 'servers'; + } + return count === 1 ? 'VM' : 'VMs'; +}; + nodeList.push({ id: group.id, type: 'group', position, data: { label: group.name, priority: group.priority, type: group.type, cascade: group.cascade, resourceCount: group.type === 'server' ? group.serverIds.length : group.vmIds.length, - resourceLabel: group.type === 'server' - ? (group.serverIds.length === 1 ? 'server' : 'servers') - : (group.vmIds.length === 1 ? 'VM' : 'VMs'), + resourceLabel: getResourceLabel( + group.type === 'server' ? group.serverIds.length : group.vmIds.length, + group.type + ), group, }, });src/features/groups/components/CreateGroupModal.vue (1)
143-156: Consider comprehensive form validationThe current validation only checks for non-empty name, but the form has other required fields (type, priority) that should be validated.
Add validation for all required fields:
const isValid = computed(() => { - return formData.value.name.trim().length > 0; + return formData.value.name.trim().length > 0 && + formData.value.type && + formData.value.priority >= 1 && formData.value.priority <= 4; });Also, if groups require resources at creation time, consider adding a resource selection UI or document that resources are added post-creation.
src/features/groups/api.ts (1)
12-71: Mock data uses hardcoded IDs that may not match actual resourcesThe mock groups reference specific server and VM IDs (e.g., '1', '4', 'vm-1') that might not exist in the actual system.
Consider either:
- Using placeholder IDs that are clearly mock data
- Fetching actual resource IDs to create realistic mock data
- Documenting that these are example IDs only
export const getMockServerGroups = (): ServerGroup[] => [ { id: 'group-web', name: 'Web Servers', description: 'Frontend web server cluster', type: 'server', priority: 1, cascade: true, - serverIds: ['1', '4', '5'], + serverIds: ['mock-server-1', 'mock-server-2', 'mock-server-3'], roomId: 'room-1', },src/features/history/types.ts (1)
85-97: Consider using icon components instead of emojiUsing emoji characters for icons may lead to inconsistent rendering across different platforms, browsers, and operating systems.
Consider using icon identifiers that map to proper icon components:
export const actionStyles: Record<string, { color: string; icon: string }> = { - CREATE: { color: 'text-green-600', icon: '➕' }, - UPDATE: { color: 'text-blue-600', icon: '✏️' }, - DELETE: { color: 'text-red-600', icon: '🗑️' }, + CREATE: { color: 'text-green-600', icon: 'plus' }, + UPDATE: { color: 'text-blue-600', icon: 'pencil' }, + DELETE: { color: 'text-red-600', icon: 'trash' }, // ... rest of the mappings };Then use a mapping function in your components to render the appropriate icon component based on the string identifier.
src/features/groups/components/GroupList.vue (1)
1-256: Consider decomposing this large componentThis component is handling multiple responsibilities and could benefit from being broken down into smaller, more focused components.
Consider extracting:
- The search/filter toolbar into a
GroupListToolbar.vuecomponent- Each view mode (sections, grid, list) into separate components like
GroupSectionView.vue,GroupGridView.vue,GroupListItemView.vue- The empty state into a
GroupEmptyState.vuecomponentThis would improve maintainability, testability, and make the code easier to understand.
src/features/servers/views/ServerListView.vue (1)
317-317: Simplify the click handler.The arrow function wrapper is unnecessary here.
- @click="() => loadMore(pageSize)" + @click="loadMore(pageSize)"src/features/history/components/HistoryEventDetail.vue (2)
22-25: Useunknowninstead ofanyfor better type safety.Using
anybypasses TypeScript's type checking.unknownis safer and forces proper type guards.-const formatJson = (data: any) => { +const formatJson = (data: unknown) => { if (!data) return null; return JSON.stringify(data, null, 2); };
377-377: Improve readability of metadata key filtering.The long inline condition is hard to read and maintain.
Define the excluded keys as a constant:
+// At the top of the script section +const CONTEXTUAL_METADATA_KEYS = new Set([ + 'userEmail', 'identifier', 'loginMethod', 'requires2FA', + 'operationType', 'targetRoleName', 'targetRoleIsAdmin', + 'previousRoleCount', 'newRoleCount', 'isElevationToAdmin', + 'isRemovalFromAdmin', 'serverType', 'operatingSystem', + 'assignedToGroup' +]); // In the template - <div v-if="!['userEmail', 'identifier', 'loginMethod', 'requires2FA', 'operationType', 'targetRoleName', 'targetRoleIsAdmin', 'previousRoleCount', 'newRoleCount', 'isElevationToAdmin', 'isRemovalFromAdmin', 'serverType', 'operatingSystem', 'assignedToGroup'].includes(String(key))" + <div v-if="!CONTEXTUAL_METADATA_KEYS.has(String(key))"src/features/history/views/HistoryListView.vue (1)
270-273: Consider extracting tooltip position calculation for better readability.The inline ternary logic for tooltip positioning is complex. Consider extracting it to a computed property or method for better maintainability.
+const getTooltipPosition = (index: number, total: number) => { + if (index === 0 && total === 1) return 'top-full mt-8'; + if (index === 0) return 'top-full mt-2'; + return 'bottom-full mb-2'; +};Then use it in the template:
- evIdx === 0 && events.length === 1 ? 'top-full mt-8' : evIdx === 0 ? 'top-full mt-2' : 'bottom-full mb-2' + getTooltipPosition(evIdx, events.length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/features/groups/api.ts(1 hunks)src/features/groups/components/CreateGroupModal.vue(1 hunks)src/features/groups/components/GroupActionMenu.vue(1 hunks)src/features/groups/components/GroupCard.vue(1 hunks)src/features/groups/components/GroupDetailsModal.vue(1 hunks)src/features/groups/components/GroupFlow.vue(1 hunks)src/features/groups/components/GroupList.vue(1 hunks)src/features/groups/store.ts(1 hunks)src/features/groups/types.ts(1 hunks)src/features/groups/views/GroupView.vue(1 hunks)src/features/groups/views/HelloWorld.vue(0 hunks)src/features/history/api.ts(1 hunks)src/features/history/components/HistoryEventDetail.vue(1 hunks)src/features/history/store.ts(2 hunks)src/features/history/types.ts(3 hunks)src/features/history/views/HistoryListView.vue(3 hunks)src/features/servers/api.ts(1 hunks)src/features/servers/store.ts(2 hunks)src/features/servers/views/ServerListView.vue(5 hunks)src/features/ups/store.ts(4 hunks)src/features/ups/views/UpsListView.vue(5 hunks)src/locales/en.json(2 hunks)src/locales/fr.json(2 hunks)src/router/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/features/groups/views/HelloWorld.vue
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/features/servers/api.ts (1)
src/features/servers/types.ts (1)
Server(6-23)
src/features/history/store.ts (2)
src/features/history/api.ts (1)
historyApi(16-47)src/features/history/types.ts (1)
HistoryFilter(65-71)
src/features/ups/store.ts (2)
src/features/ups/types.ts (3)
UpsResponseDto(34-41)UpsListResponse(43-48)UpsUpdateDto(24-32)src/features/ups/api.ts (1)
upsApi(15-80)
src/features/groups/api.ts (1)
src/features/groups/types.ts (7)
ServerGroup(15-18)VmGroup(20-23)Group(25-25)GroupListParams(52-57)GroupListResponse(45-50)CreateGroupPayload(27-35)UpdateGroupPayload(37-43)
src/features/servers/store.ts (2)
src/features/servers/types.ts (4)
Server(6-23)ServerListParams(49-52)ServerListResponse(42-47)CreateServerPayload(25-40)src/features/servers/api.ts (3)
fetchServers(87-92)getServersAdmin(116-119)createServer(111-114)
🔇 Additional comments (19)
src/features/history/api.ts (1)
38-46: LGTM! Clean implementation following existing patterns.The method implementation is well-structured and follows the same authentication and error handling patterns as the existing
getListmethod.src/router/index.ts (1)
63-63: LGTM! Clean route update for groups feature.The route update correctly replaces the placeholder HelloWorld component with the new GroupView component, aligning with the PR's group management feature implementation.
src/features/servers/api.ts (1)
116-119: No change required for authentication on getServersAdmin.The shared
apiinstance insrc/services/api.tsautomatically injects theAuthorizationheader—first viaapi.defaults.headers.commonwhen a token is present, then again in its request interceptor. All requests made throughapi.get(including/server/admin/all) will carry the bearer token if available.Notably, other admin calls such as
getAllRolesalso omit explicit auth headers and rely on the same interceptor. You can continue using the current pattern without addinggetAuthHeaders().src/features/history/store.ts (2)
21-21: LGTM! Clean state variable addition.The new
availableEntityTypesstate variable is well-placed and follows the existing reactive state pattern.
73-75: LGTM! Proper exposure of new state and method.The new state variable and method are correctly exposed in the store's return object, maintaining consistency with the existing pattern.
src/features/groups/components/GroupActionMenu.vue (4)
1-37: LGTM! Well-structured template with consistent styling.The template is clean and follows good Vue practices with proper event handling, consistent styling classes, and appropriate use of Heroicons for visual consistency.
39-63: LGTM! Proper TypeScript setup with clean interfaces.The props interface and emit definitions are well-typed and the component setup follows Vue 3 composition API best practices.
64-82: LGTM! Clean action handling with proper event emission.The action handler correctly maps different actions to their respective events and ensures the menu closes after each action.
84-110: LGTM! Excellent viewport overflow prevention and cleanup.The positioning logic correctly handles viewport boundaries, and the component properly manages event listeners with cleanup in
onUnmounted. This prevents memory leaks and ensures the menu always stays within the viewport.src/locales/fr.json (1)
936-1031: LGTM! Comprehensive French translationsThe translations are well-structured and cover all the new features. The French text is grammatically correct and follows consistent patterns.
src/features/groups/types.ts (1)
1-65: Well-structured type definitionsThe type definitions are comprehensive and well-organized, using discriminated unions effectively for the different group types. Good use of TypeScript features for type safety.
src/features/servers/store.ts (1)
22-53: Well-implemented pagination logic!The
fetchServersmethod properly handles pagination with the append functionality for infinite scrolling, updates all necessary state variables, and includes proper error handling.src/locales/en.json (1)
943-1111: Localization entries are well-structured.The new localization keys for common actions, history details, and groups functionality follow consistent naming conventions and are properly organized.
src/features/ups/views/UpsListView.vue (2)
33-49: LGTM!The computed property correctly uses
.valuefor reactive refs and implements proper client-side filtering.
67-67: Verify server-side search support for paginated loading.The code passes
searchQuery.valuetoloadMore, but the filtering infilteredUpsis client-side. This could cause inconsistencies if the server doesn't support search filtering or if the filtering logic differs.Please verify that the UPS API supports search query parameters for paginated requests, or remove the search parameter from the
loadMorecall if filtering should remain client-side only.src/features/history/views/HistoryListView.vue (2)
82-87: Excellent use of concurrent data fetching!Using
Promise.allto fetch history and entity types concurrently improves initial load performance.
184-195: Good enhancement for datetime precision!Changing to
datetime-localinputs allows users to filter events with time precision, which is valuable for audit logs.src/features/groups/components/GroupFlow.vue (2)
1-250: Well-implemented flow visualization component!The component effectively uses VueFlow to create an interactive group visualization with:
- Clean separation of template, script, and style sections
- Proper TypeScript typing (with minor exceptions noted)
- Good dark mode support
- Intuitive priority-based coloring and positioning
The suggested improvements are mostly for enhanced maintainability and user experience.
8-22: Add accessibility attributes to interactive elements.The buttons should have proper aria-labels for screen reader users.
<button @click="fitView" class="px-3 py-1.5 text-sm bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 dark:hover:bg-gray-600 text-gray-700 dark:text-gray-300 rounded-md transition-colors flex items-center gap-1" + :aria-label="$t('groups.fitView')" > <ArrowsPointingInIcon class="w-4 h-4" /> {{ $t('groups.fitView') }} </button> <button @click="toggleOrientation" class="px-3 py-1.5 text-sm bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 dark:hover:bg-gray-600 text-gray-700 dark:text-gray-300 rounded-md transition-colors flex items-center gap-1" + :aria-label="$t('groups.toggleOrientation')" >Likely an incorrect or invalid review comment.
| const createUps = async (payload: UpsCreationDto) => { | ||
| const created = await upsApi.create(payload); | ||
| await fetchUps(); | ||
| await fetchUps(1, 12); | ||
| return created; | ||
| }; | ||
|
|
||
| const updateUps = async (id: string, payload: UpsUpdateDto) => { | ||
| const updated = await upsApi.update(id, payload); | ||
| await fetchUps(); | ||
| await fetchUps(1, 12); | ||
| return updated; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preserving current pagination state after create/update
After creating or updating a UPS item, the store resets to page 1 with a hardcoded limit of 12. This might not match the user's current view settings.
Consider preserving the current pagination state or make the reset behavior configurable:
const createUps = async (payload: UpsCreationDto) => {
const created = await upsApi.create(payload);
- await fetchUps(1, 12);
+ // Option 1: Preserve current view
+ await fetchUps(currentPage.value, 12);
+ // Option 2: Make configurable
+ // await fetchUps(1, currentLimit.value || 12);
return created;
};
const updateUps = async (id: string, payload: UpsUpdateDto) => {
const updated = await upsApi.update(id, payload);
- await fetchUps(1, 12);
+ await fetchUps(currentPage.value, 12);
return updated;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const createUps = async (payload: UpsCreationDto) => { | |
| const created = await upsApi.create(payload); | |
| await fetchUps(); | |
| await fetchUps(1, 12); | |
| return created; | |
| }; | |
| const updateUps = async (id: string, payload: UpsUpdateDto) => { | |
| const updated = await upsApi.update(id, payload); | |
| await fetchUps(); | |
| await fetchUps(1, 12); | |
| return updated; | |
| }; | |
| const createUps = async (payload: UpsCreationDto) => { | |
| const created = await upsApi.create(payload); | |
| // Option 1: Preserve current view | |
| await fetchUps(currentPage.value, 12); | |
| // Option 2: Make configurable | |
| // await fetchUps(1, currentLimit.value || 12); | |
| return created; | |
| }; | |
| const updateUps = async (id: string, payload: UpsUpdateDto) => { | |
| const updated = await upsApi.update(id, payload); | |
| await fetchUps(currentPage.value, 12); | |
| return updated; | |
| }; |
🤖 Prompt for AI Agents
In src/features/ups/store.ts around lines 66 to 76, the createUps and updateUps
functions reset the pagination to page 1 with a limit of 12 after each
operation, ignoring the user's current pagination state. Modify these functions
to retrieve and use the current page and limit values from the store or pass
them as parameters, so that fetchUps is called with the user's current
pagination settings instead of hardcoded values.
src/features/groups/api.ts
Outdated
| export const removeResourcesFromGroup = async (id: string, resourceIds: string[]) => { | ||
| const response = await api.delete<Group>(`/groups/${id}/resources`, { | ||
| data: { resourceIds }, | ||
| }); | ||
| return response.data; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DELETE with request body might not be universally supported
Using DELETE with a request body is uncommon and might not work with all HTTP clients or proxies.
Consider using a POST endpoint for removing resources or passing IDs as query parameters:
export const removeResourcesFromGroup = async (id: string, resourceIds: string[]) => {
- const response = await api.delete<Group>(`/groups/${id}/resources`, {
- data: { resourceIds },
- });
+ // Option 1: Use POST for removal
+ const response = await api.post<Group>(`/groups/${id}/resources/remove`, { resourceIds });
+ // Option 2: Use query parameters (if few IDs)
+ // const params = new URLSearchParams();
+ // resourceIds.forEach(rid => params.append('resourceId', rid));
+ // const response = await api.delete<Group>(`/groups/${id}/resources?${params}`);
return response.data;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const removeResourcesFromGroup = async (id: string, resourceIds: string[]) => { | |
| const response = await api.delete<Group>(`/groups/${id}/resources`, { | |
| data: { resourceIds }, | |
| }); | |
| return response.data; | |
| }; | |
| export const removeResourcesFromGroup = async (id: string, resourceIds: string[]) => { | |
| // Option 1: Use POST for removal | |
| const response = await api.post<Group>(`/groups/${id}/resources/remove`, { resourceIds }); | |
| // Option 2: Use query parameters (if few IDs) | |
| // const params = new URLSearchParams(); | |
| // resourceIds.forEach(rid => params.append('resourceId', rid)); | |
| // const response = await api.delete<Group>(`/groups/${id}/resources?${params}`); | |
| return response.data; | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/api.ts around lines 146 to 151, the
removeResourcesFromGroup function uses a DELETE request with a request body,
which is not universally supported by all HTTP clients or proxies. To fix this,
change the API call to use a POST request to a dedicated endpoint for removing
resources or modify the DELETE request to pass resource IDs as query parameters
instead of in the body. Update the backend accordingly if needed to support this
change.
| const loadGroups = async () => { | ||
| try { | ||
| await groupStore.fetchGroups({ page: 1, limit: 12 }); | ||
| } catch (error) { | ||
| const mockGroups = getMockGroups(); | ||
| groupStore.list = mockGroups; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Notify users when falling back to mock data.
The error is silently caught and mock data is used without informing the user. This could lead to confusion about whether they're viewing real or test data.
try {
await groupStore.fetchGroups({ page: 1, limit: 12 });
} catch (error) {
+ toast.error(t('groups.loadError'));
+ console.error('Failed to load groups:', error);
const mockGroups = getMockGroups();
groupStore.list = mockGroups;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loadGroups = async () => { | |
| try { | |
| await groupStore.fetchGroups({ page: 1, limit: 12 }); | |
| } catch (error) { | |
| const mockGroups = getMockGroups(); | |
| groupStore.list = mockGroups; | |
| } | |
| }; | |
| const loadGroups = async () => { | |
| try { | |
| await groupStore.fetchGroups({ page: 1, limit: 12 }); | |
| } catch (error) { | |
| toast.error(t('groups.loadError')); | |
| console.error('Failed to load groups:', error); | |
| const mockGroups = getMockGroups(); | |
| groupStore.list = mockGroups; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/views/GroupView.vue around lines 158 to 165, the catch
block silently falls back to mock data without notifying the user. Modify the
catch block to include a user notification, such as a toast message or alert,
informing them that real data could not be loaded and mock data is being
displayed instead. This will improve user awareness and transparency.
| const dependentGroups = groupStore.list.filter( | ||
| other => other.priority === g.priority + 1 && other.cascade | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle priority gaps in cascade logic.
The current logic only finds groups with exactly priority + 1, which will miss dependent groups if there are gaps in priority values.
const dependentGroups = groupStore.list.filter(
- other => other.priority === g.priority + 1 && other.cascade
+ other => other.priority > g.priority && other.cascade
+ ).sort((a, b) => a.priority - b.priority);This ensures all dependent groups with higher priority are included and processed in order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dependentGroups = groupStore.list.filter( | |
| other => other.priority === g.priority + 1 && other.cascade | |
| ); | |
| const dependentGroups = groupStore.list.filter( | |
| other => other.priority > g.priority && other.cascade | |
| ).sort((a, b) => a.priority - b.priority); |
🤖 Prompt for AI Agents
In src/features/groups/views/GroupView.vue around lines 252 to 254, the
filtering logic only selects groups with priority exactly one greater than the
current group, missing groups with higher priorities if gaps exist. Modify the
filter condition to include all groups with priority greater than the current
group's priority and with cascade true, then sort these groups by priority
ascending to ensure correct processing order.
| const startGroupResources = async (group: Group) => { | ||
| console.log('Starting resources for group:', group.name); | ||
| }; | ||
|
|
||
| const stopGroupResources = async (group: Group) => { | ||
| console.log('Stopping resources for group:', group.name); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TODO comments for placeholder implementations.
These functions are placeholders that only log to console. They should be clearly marked for future implementation.
const startGroupResources = async (group: Group) => {
+ // TODO: Implement actual resource start logic
console.log('Starting resources for group:', group.name);
+ // await groupApi.startGroup(group.id);
};
const stopGroupResources = async (group: Group) => {
+ // TODO: Implement actual resource stop logic
console.log('Stopping resources for group:', group.name);
+ // await groupApi.stopGroup(group.id);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startGroupResources = async (group: Group) => { | |
| console.log('Starting resources for group:', group.name); | |
| }; | |
| const stopGroupResources = async (group: Group) => { | |
| console.log('Stopping resources for group:', group.name); | |
| }; | |
| const startGroupResources = async (group: Group) => { | |
| // TODO: Implement actual resource start logic | |
| console.log('Starting resources for group:', group.name); | |
| // await groupApi.startGroup(group.id); | |
| }; | |
| const stopGroupResources = async (group: Group) => { | |
| // TODO: Implement actual resource stop logic | |
| console.log('Stopping resources for group:', group.name); | |
| // await groupApi.stopGroup(group.id); | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/views/GroupView.vue around lines 269 to 275, the
functions startGroupResources and stopGroupResources currently only log messages
to the console as placeholders. Add clear TODO comments inside each function
indicating that these are placeholder implementations and need to be properly
implemented in the future.
| const toggleOrientation = () => { | ||
| orientation.value = orientation.value === 'horizontal' ? 'vertical' : 'horizontal'; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trigger fitView after orientation change for better UX.
When toggling orientation, the view should automatically fit to show all repositioned nodes.
const toggleOrientation = () => {
orientation.value = orientation.value === 'horizontal' ? 'vertical' : 'horizontal';
+ // Wait for DOM update before fitting view
+ nextTick(() => {
+ fitView();
+ });
};Don't forget to import nextTick from Vue:
-import { ref, computed } from 'vue';
+import { ref, computed, nextTick } from 'vue';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toggleOrientation = () => { | |
| orientation.value = orientation.value === 'horizontal' ? 'vertical' : 'horizontal'; | |
| }; | |
| // At the top of the file | |
| -import { ref, computed } from 'vue'; | |
| +import { ref, computed, nextTick } from 'vue'; | |
| // … | |
| const toggleOrientation = () => { | |
| orientation.value = orientation.value === 'horizontal' | |
| ? 'vertical' | |
| : 'horizontal'; | |
| // Wait for DOM update before fitting view | |
| nextTick(() => { | |
| fitView(); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/components/GroupFlow.vue around lines 237 to 239, after
toggling the orientation value, call nextTick to wait for the DOM update and
then trigger the fitView method to adjust the view to show all repositioned
nodes. Import nextTick from Vue at the top of the file if not already imported.
This ensures the view updates correctly after the orientation change for better
user experience.
| const handleNodeClick = (event: any) => { | ||
| const node = event.node; | ||
| if (node?.data?.group) { | ||
| emit('group-click', node.data.group); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Use proper TypeScript types instead of any.
Replace the any type with the proper VueFlow event type for better type safety.
-const handleNodeClick = (event: any) => {
+import type { NodeMouseEvent } from '@vue-flow/core';
+
+const handleNodeClick = (event: NodeMouseEvent) => {Don't forget to add the import at the top of the script section.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/features/groups/components/GroupFlow.vue around lines 230 to 235, replace
the use of the `any` type for the event parameter in the handleNodeClick
function with the correct VueFlow event type to improve type safety. Identify
and import the appropriate event type from VueFlow at the top of the script
section, then update the function signature to use this type instead of `any`.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/features/groups/components/GroupSelection.vue (1)
251-259: Extract duplicated getPriorityClass functionsrc/features/groups/api.ts (1)
16-27: Inconsistent error handling across API functionsAlso applies to: 29-40, 42-52, 54-64
src/features/groups/views/GroupView.vue (4)
280-286: Add TODO comments for placeholder implementations.These functions are placeholders that only log to console. They should be clearly marked for future implementation.
const startGroupResources = async (group: Group) => { + // TODO: Implement actual resource start logic console.log('Starting resources for group:', group.name); + // await groupApi.startGroup(group.id); }; const stopGroupResources = async (group: Group) => { + // TODO: Implement actual resource stop logic console.log('Stopping resources for group:', group.name); + // await groupApi.stopGroup(group.id); };
144-148: Critical: Remove Math.random() from computed property.Using
Math.random()in a computed property causes the VM state to change on every re-render, breaking Vue's reactivity and causing UI inconsistencies.map[group.id] = (vmGroup.vmIds || []).map(id => ({ id, name: `VM ${id}`, - state: Math.random() > 0.5 ? 'active' : 'inactive', + state: 'inactive', // TODO: Fetch actual VM states from API or store }));
263-265: Handle priority gaps in cascade logic.The current logic only finds groups with exactly
priority + 1, which will miss dependent groups if there are gaps in priority values.const dependentGroups = groupStore.allGroups.filter( - (other: Group) => other.priority === g.priority + 1 && other.cascade + (other: Group) => other.priority > g.priority && other.cascade + ).sort((a, b) => a.priority - b.priority);
170-176: Notify users when group loading fails.The error is silently caught without informing the user. This could lead to confusion about whether they're viewing real or fallback data.
const loadGroups = async () => { try { await groupStore.loadAllGroups(); } catch (error) { + toast.error(t('groups.loadError')); console.error('Failed to load groups:', error); } };
🧹 Nitpick comments (5)
src/features/groups/views/GroupShutdownView.vue (1)
26-37: Consider using a toast notification system instead of fixed positioningThe fixed positioning of the success message could overlap with other UI elements. Consider using a toast notification library or Vue portal to ensure proper stacking and positioning.
You could use a library like
vue-toastificationor implement a portal-based solution:-<div - v-if="showSuccessMessage" - class="fixed bottom-4 right-4 p-4 bg-green-100 border border-green-200 rounded-lg shadow-lg dark:bg-green-900/20 dark:border-green-800" -> +<Teleport to="body"> + <div + v-if="showSuccessMessage" + class="fixed bottom-4 right-4 p-4 bg-green-100 border border-green-200 rounded-lg shadow-lg dark:bg-green-900/20 dark:border-green-800 z-50" + role="status" + aria-live="polite" + >src/features/groups/components/ShutdownPreviewDialog.vue (1)
38-41: Add defensive checks for pagination displayThe pagination display calculation could fail with edge cases.
{{ $t('pagination.showing') }} -{{ ((groupStore.shutdownPreview.currentPage - 1) * 10) + 1 }}-{{ Math.min(groupStore.shutdownPreview.currentPage * 10, groupStore.shutdownPreview.totalItems) }} +{{ groupStore.shutdownPreview.totalItems > 0 ? ((groupStore.shutdownPreview.currentPage - 1) * 10) + 1 : 0 }}-{{ Math.min(groupStore.shutdownPreview.currentPage * 10, groupStore.shutdownPreview.totalItems) }} {{ $t('pagination.of') }} {{ groupStore.shutdownPreview.totalItems }}src/features/groups/api.ts (3)
20-25: Use optional chaining for cleaner codeStatic analysis correctly identifies that optional chaining would be cleaner here.
export const fetchServerGroups = async (params: ServerGroupListParams = {}) => { const response = await api.get<GroupServerListResponseDto>('/group/server', { params, }); - if (response.data && response.data.items) { - response.data.items = response.data.items.map((item) => ({ + if (response.data?.items) { + response.data.items = response.data.items.map((item) => ({ ...item, type: 'server' as const, })); } return response; }; export const fetchVmGroups = async (params: VmGroupListParams = {}) => { const response = await api.get<GroupVmListResponseDto>('/group/vm', { params, }); - if (response.data && response.data.items) { - response.data.items = response.data.items.map((item) => ({ + if (response.data?.items) { + response.data.items = response.data.items.map((item) => ({ ...item, type: 'vm' as const, })); } return response; };Also applies to: 33-38
75-77: Simplify redundant type assertionsThe type assertions are unnecessary with proper typing.
- return { ...response.data, type } as - | GroupServerResponseDto - | GroupVmResponseDto; + return { ...response.data, type };TypeScript should be able to infer the correct type based on the discriminated union with the
typeproperty.Also applies to: 93-95, 113-115
136-184: Move mock data to a separate fileMock data generation doesn't belong in the API module. This violates separation of concerns.
Create a separate file
src/features/groups/mocks/index.tsorsrc/features/groups/utils/mock-data.tsto house mock data generators. This keeps the API module focused on actual API interactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/features/auth/components/TwoFAForm.vue(6 hunks)src/features/auth/views/Enable2FAView.vue(3 hunks)src/features/auth/views/TwoFAView.vue(1 hunks)src/features/groups/api.ts(1 hunks)src/features/groups/components/GroupCard.vue(1 hunks)src/features/groups/components/GroupDetailsModal.vue(1 hunks)src/features/groups/components/GroupFlow.vue(1 hunks)src/features/groups/components/GroupList.vue(1 hunks)src/features/groups/components/GroupPagination.vue(1 hunks)src/features/groups/components/GroupSelection.vue(1 hunks)src/features/groups/components/ShutdownPreviewDialog.vue(1 hunks)src/features/groups/store.ts(1 hunks)src/features/groups/types.ts(1 hunks)src/features/groups/views/GroupShutdownView.vue(1 hunks)src/features/groups/views/GroupView.vue(1 hunks)src/features/servers/store.ts(2 hunks)src/features/ups/components/MaintenanceInformation.vue(1 hunks)src/features/ups/components/PowerSpecifications.vue(1 hunks)src/features/ups/components/UpsCard.vue(2 hunks)src/features/ups/store.ts(4 hunks)src/features/ups/types.ts(1 hunks)src/features/ups/views/UpsDetailsView.vue(2 hunks)src/features/ups/views/UpsListView.vue(5 hunks)src/locales/en.json(4 hunks)src/locales/fr.json(4 hunks)src/router/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/features/auth/views/TwoFAView.vue
- src/features/auth/components/TwoFAForm.vue
- src/features/auth/views/Enable2FAView.vue
- src/features/ups/components/PowerSpecifications.vue
- src/features/ups/components/MaintenanceInformation.vue
🚧 Files skipped from review as they are similar to previous changes (10)
- src/router/index.ts
- src/features/groups/components/GroupFlow.vue
- src/features/ups/store.ts
- src/features/groups/components/GroupList.vue
- src/features/groups/components/GroupCard.vue
- src/features/groups/components/GroupDetailsModal.vue
- src/features/groups/store.ts
- src/features/servers/store.ts
- src/features/ups/views/UpsListView.vue
- src/features/groups/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/features/groups/api.ts (1)
src/features/groups/types.ts (11)
ServerGroupListParams(65-70)GroupServerListResponseDto(28-33)VmGroupListParams(72-75)GroupVmListResponseDto(35-40)ShutdownRequestDto(61-63)ShutdownPreviewParams(77-80)ShutdownPreviewListResponseDto(52-59)GroupServerResponseDto(4-14)GroupVmResponseDto(16-26)CreateGroupPayload(82-91)UpdateGroupPayload(93-100)
🪛 Biome (1.9.4)
src/features/groups/api.ts
[error] 20-20: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
src/features/ups/types.ts (1)
41-41: LGTM! Clean addition to support server count display.The addition of
serverCountproperty to theUpsResponseDtointerface is well-designed and follows the existing patterns in the codebase.src/features/ups/components/UpsCard.vue (2)
15-15: LGTM! Optional prop addition follows Vue best practices.The optional
serverCountprop is well-designed and maintains backward compatibility.
179-179: Excellent improvement replacing random data with actual prop data.This change eliminates non-deterministic behavior and ensures consistent server count display across the application. The nullish coalescing operator provides a sensible fallback.
src/features/ups/views/UpsDetailsView.vue (2)
27-28: LGTM! Good modular component imports.The import of dedicated components for power specifications and maintenance information improves code organization.
415-424: Verification Complete: Component Implementation ConfirmedBoth
PowerSpecifications.vueandMaintenanceInformation.vueare present undersrc/features/ups/components/and correctly declare their props viadefineProps<Props>()in the<script setup>block. No further changes are needed—approving the refactoring.src/features/groups/api.ts (1)
125-134: Cascade toggling applies only to server groups
A search for a/group/vm/.../cascadeendpoint returned no results, and there’s no VM-specific cascade support in the API. This behavior is intentional—VM groups don’t currently support cascade toggling. If VM cascade toggling is required in the future, a dedicated endpoint and client function will need to be added.src/locales/fr.json (3)
154-157: LGTM: Consistent pagination translations added.The pagination control translations are properly structured and follow standard French conventions.
994-1037: LGTM: Comprehensive history event localization.The history namespace provides thorough coverage of audit events with properly structured actions, descriptions, and details. The use of placeholders like
{method},{email},{hostname}etc. enables dynamic content insertion.
1039-1126: LGTM: Complete groups feature localization.The groups namespace comprehensively covers all UI elements for the new group management feature including view modes, actions, form fields, status messages, and shutdown workflows. Translation quality appears consistent and professional.
src/locales/en.json (3)
1007-1050: LGTM: Well-structured history event localization.The history namespace provides comprehensive coverage with clear action names, descriptive text with proper placeholder usage, and detailed audit information labels. The English translations are professional and consistent.
1052-1139: LGTM: Complete groups management localization.The groups namespace thoroughly covers all aspects of the group management feature including UI labels, form fields, status messages, view modes, and shutdown workflows. The translations are clear and user-friendly.
949-959: LGTM: Essential common UI additions.The new common UI action labels (
create,edit,delete,save, etc.) provide necessary building blocks for the enhanced interface components.
| const onShutdownSuccess = () => { | ||
| showSuccessMessage.value = true; | ||
| groupStore.clearSelection(); | ||
|
|
||
| setTimeout(() => { | ||
| showSuccessMessage.value = false; | ||
| }, 5000); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make success message timeout configurable and add error handling
The timeout is hardcoded and there's no error handling for the store operation.
+const SUCCESS_MESSAGE_TIMEOUT = 5000; // or import from a constants file
+
const onShutdownSuccess = () => {
showSuccessMessage.value = true;
- groupStore.clearSelection();
+ try {
+ groupStore.clearSelection();
+ } catch (error) {
+ console.error('Failed to clear selection:', error);
+ }
setTimeout(() => {
showSuccessMessage.value = false;
- }, 5000);
+ }, SUCCESS_MESSAGE_TIMEOUT);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onShutdownSuccess = () => { | |
| showSuccessMessage.value = true; | |
| groupStore.clearSelection(); | |
| setTimeout(() => { | |
| showSuccessMessage.value = false; | |
| }, 5000); | |
| }; | |
| // Make the success message timeout configurable | |
| const SUCCESS_MESSAGE_TIMEOUT = 5000; // or import from a constants file | |
| const onShutdownSuccess = () => { | |
| showSuccessMessage.value = true; | |
| try { | |
| groupStore.clearSelection(); | |
| } catch (error) { | |
| console.error('Failed to clear selection:', error); | |
| } | |
| setTimeout(() => { | |
| showSuccessMessage.value = false; | |
| }, SUCCESS_MESSAGE_TIMEOUT); | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/views/GroupShutdownView.vue around lines 52 to 59, the
success message timeout is hardcoded and there is no error handling for the
groupStore.clearSelection() call. Refactor the code to make the timeout duration
configurable via a variable or prop, and wrap the groupStore.clearSelection()
call in a try-catch block to handle potential errors gracefully, logging or
displaying an error message if needed.
| <button | ||
| @click="$emit('page-change', currentPage - 1)" | ||
| :disabled="currentPage === 1" | ||
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | ||
| > | ||
| {{ $t('pagination.previous') }} | ||
| </button> | ||
|
|
||
| <div class="flex gap-1"> | ||
| <button | ||
| v-for="page in visiblePages" | ||
| :key="page" | ||
| @click="$emit('page-change', page)" | ||
| class="px-3 py-1 text-sm rounded-md" | ||
| :class="[ | ||
| page === currentPage | ||
| ? 'bg-blue-600 text-white' | ||
| : 'text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800' | ||
| ]" | ||
| > | ||
| {{ page }} | ||
| </button> | ||
| </div> | ||
|
|
||
| <button | ||
| @click="$emit('page-change', currentPage + 1)" | ||
| :disabled="currentPage === totalPages" | ||
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | ||
| > | ||
| {{ $t('pagination.next') }} | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility with ARIA labels
The pagination controls lack proper ARIA labels for screen reader users.
<button
@click="$emit('page-change', currentPage - 1)"
:disabled="currentPage === 1"
+ :aria-label="$t('pagination.goToPreviousPage')"
class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800"
>
{{ $t('pagination.previous') }}
</button>
<button
v-for="page in visiblePages"
:key="page"
@click="$emit('page-change', page)"
+ :aria-label="`${$t('pagination.goToPage')} ${page}`"
+ :aria-current="page === currentPage ? 'page' : undefined"
class="px-3 py-1 text-sm rounded-md"
:class="[
page === currentPage
? 'bg-blue-600 text-white'
: 'text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800'
]"
>
{{ page }}
</button>
<button
@click="$emit('page-change', currentPage + 1)"
:disabled="currentPage === totalPages"
+ :aria-label="$t('pagination.goToNextPage')"
class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800"
>
{{ $t('pagination.next') }}
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| @click="$emit('page-change', currentPage - 1)" | |
| :disabled="currentPage === 1" | |
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | |
| > | |
| {{ $t('pagination.previous') }} | |
| </button> | |
| <div class="flex gap-1"> | |
| <button | |
| v-for="page in visiblePages" | |
| :key="page" | |
| @click="$emit('page-change', page)" | |
| class="px-3 py-1 text-sm rounded-md" | |
| :class="[ | |
| page === currentPage | |
| ? 'bg-blue-600 text-white' | |
| : 'text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800' | |
| ]" | |
| > | |
| {{ page }} | |
| </button> | |
| </div> | |
| <button | |
| @click="$emit('page-change', currentPage + 1)" | |
| :disabled="currentPage === totalPages" | |
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | |
| > | |
| {{ $t('pagination.next') }} | |
| </button> | |
| <button | |
| @click="$emit('page-change', currentPage - 1)" | |
| :disabled="currentPage === 1" | |
| :aria-label="$t('pagination.goToPreviousPage')" | |
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | |
| > | |
| {{ $t('pagination.previous') }} | |
| </button> | |
| <div class="flex gap-1"> | |
| <button | |
| v-for="page in visiblePages" | |
| :key="page" | |
| @click="$emit('page-change', page)" | |
| :aria-label="`${$t('pagination.goToPage')} ${page}`" | |
| :aria-current="page === currentPage ? 'page' : undefined" | |
| class="px-3 py-1 text-sm rounded-md" | |
| :class="[ | |
| page === currentPage | |
| ? 'bg-blue-600 text-white' | |
| : 'text-gray-700 hover:bg-gray-100 dark:text-gray-300 dark:hover:bg-gray-800' | |
| ]" | |
| > | |
| {{ page }} | |
| </button> | |
| </div> | |
| <button | |
| @click="$emit('page-change', currentPage + 1)" | |
| :disabled="currentPage === totalPages" | |
| :aria-label="$t('pagination.goToNextPage')" | |
| class="px-3 py-1 border border-gray-300 rounded-md text-sm hover:bg-gray-50 disabled:opacity-50 disabled:cursor-not-allowed dark:border-gray-600 dark:hover:bg-gray-800" | |
| > | |
| {{ $t('pagination.next') }} | |
| </button> |
🤖 Prompt for AI Agents
In src/features/groups/components/GroupPagination.vue between lines 8 and 38,
the pagination buttons lack ARIA labels, which reduces accessibility for screen
reader users. Add appropriate aria-label attributes to the previous, next, and
page number buttons to clearly describe their function, such as "Go to previous
page", "Go to next page", and "Go to page X" respectively. This will improve
screen reader support and overall accessibility.
| const getPriorityClass = (priority: number) => { | ||
| if (priority >= 8) { | ||
| return 'bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400'; | ||
| } else if (priority >= 5) { | ||
| return 'bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400'; | ||
| } else { | ||
| return 'bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated getPriorityClass function to shared utility
This function is duplicated in GroupSelection.vue.
Create a shared utility file src/features/groups/utils/priority.ts:
export const getPriorityClass = (priority: number): string => {
if (priority >= 8) {
return 'bg-red-100 text-red-800 dark:bg-red-900/20 dark:text-red-400';
} else if (priority >= 5) {
return 'bg-yellow-100 text-yellow-800 dark:bg-yellow-900/20 dark:text-yellow-400';
} else {
return 'bg-green-100 text-green-800 dark:bg-green-900/20 dark:text-green-400';
}
};Then import and use it in both components.
🤖 Prompt for AI Agents
In src/features/groups/components/ShutdownPreviewDialog.vue around lines 210 to
218, the getPriorityClass function is duplicated in GroupSelection.vue. To fix
this, create a new shared utility file at src/features/groups/utils/priority.ts
containing the getPriorityClass function. Then, remove the local definition from
both components and import the function from the new utility file to use it in
both places.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/features/groups/views/GroupView.vue (3)
310-316: Add TODO comments for placeholder implementations.These functions are placeholders that only log to console. They should be clearly marked for future implementation.
const startGroupResources = async (group: Group) => { + // TODO: Implement actual resource start logic console.log('Starting resources for group:', group.name); + // await groupApi.startGroup(group.id); }; const stopGroupResources = async (group: Group) => { + // TODO: Implement actual resource stop logic console.log('Stopping resources for group:', group.name); + // await groupApi.stopGroup(group.id); };
156-162: Remove Math.random() from computed property to prevent UI inconsistencies.Using
Math.random()in a computed property causes the VM state to change on every re-render, breaking Vue's reactivity.map[group.id] = (vmGroup.vmIds || []).map(id => ({ id, name: `VM ${id}`, - state: Math.random() > 0.5 ? 'active' : 'inactive', + state: 'inactive', // TODO: Fetch actual VM states from API or store }));
293-295: Fix cascade logic to handle priority gaps.The current logic only finds groups with exactly
priority + 1, which will miss dependent groups if there are gaps in priority values.const dependentGroups = groupStore.allGroups.filter( - (other: Group) => other.priority === g.priority + 1 && other.cascade + (other: Group) => other.priority > g.priority && other.cascade + ).sort((a, b) => a.priority - b.priority);
🧹 Nitpick comments (4)
src/features/groups/components/GroupDetailsModal.vue (1)
224-228: Avoid usinganytype for better type safety.The room store list should be properly typed to avoid runtime errors and improve code maintainability.
const roomName = computed(() => { if (!props.group?.roomId) return null; - const room = roomStore.list?.find((r: any) => r.id === props.group?.roomId); + const room = roomStore.list?.find(r => r.id === props.group?.roomId); return room?.name; });Ensure the
roomStore.listis properly typed in the room store definition.src/features/groups/components/GroupEditModal.vue (1)
306-308: Missing VM loading implementation.The TODO comment indicates that VM loading functionality is not implemented yet.
Would you like me to help implement the VM loading logic or create an issue to track this missing functionality?
src/features/groups/components/ResourceSelector.vue (1)
141-141: Removeanytype annotation for better type safety.The rooms computed property should already be properly typed from the room store.
- {{ resource.roomId ? rooms.find((r: any) => r.id === resource.roomId)?.name : $t('groups.noRoom') }} + {{ resource.roomId ? rooms.find(r => r.id === resource.roomId)?.name : $t('groups.noRoom') }}src/features/groups/views/GroupView.vue (1)
184-189: Add user notification for group loading errors.Errors are silently logged to console without informing the user about the failure.
const loadGroups = async () => { try { await groupStore.loadAllGroups(); } catch (error) { + toast.error(t('groups.loadError', { error: error.message })); console.error('Failed to load groups:', error); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.json(1 hunks)src/features/groups/components/GroupDetailsModal.vue(1 hunks)src/features/groups/components/GroupEditModal.vue(1 hunks)src/features/groups/components/ResourceSelector.vue(1 hunks)src/features/groups/views/GroupView.vue(1 hunks)src/locales/en.json(5 hunks)src/locales/fr.json(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/locales/fr.json
[error] 1129-1130: The key servers was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1130-1130: The key vms was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (5)
src/locales/en.json (5)
154-157: 👍 Pagination additions look correct but mind existing duplicatesKeys themselves are fine, however note that
"previous"/"next"now exist in at least three namespaces (rooms,servers,pagination).
Keeping identical wording in one sharedcommon/paginationarea and referencing it from the UI prevents future drift.
[ suggest_optional_refactor ]
426-435: Potential naming collision with existing"type"/"priority"keys
"type_label"/"priority_label"introduce a second set of labels for the same concepts already exposed via"type"/"priority"a few lines above.
Double-source strings age badly when wording must be updated.Diff-level fix:
- "type_label": "Server Type", - "priority_label": "Priority Level", + "type_label": "@:servers.type", // reuse existing + "priority_label": "@:servers.priority"(i18n-ally style, adjust to your tooling).
If different wording is really required, rename with a clearer scope like"form_type_label".
[ request_verification ]
[ suggest_essential_refactor ]
940-953: Mixed camelCase / snake_case breaks key-style consistencyThe newly added
commonkeys ("loadMore","view_details"…) switch to camelCase while the rest of the file uses snake_case (search_placeholder,load_more, …).Pick one convention and stick to it. The same applies to other additions below (
searchPlaceholder,createGroup, …).
[ suggest_essential_refactor ]
993-998: Minor: keep history-detail keys orthogonal
"general_info","metadata","changes"live under"administration.history_details", while similar concepts also appear in the top-level"history.details"object that follows.
Consider re-using the same keys (or aliasing) to avoid translators having to maintain two copies.
[ suggest_nitpick ]
1046-1155: Duplicate placeholders & inconsistent key style ingroupssection
"namePlaceholder"/"descriptionPlaceholder"are defined twice – insidegroups.form(L1089-1093) and again at root (L1136-1138).
One of them will be unreachable at runtime. Remove or rename the root-level copies.@@ - "namePlaceholder": "Enter group name", - "descriptionPlaceholder": "Enter group description (optional)",
This block introduces many camelCase keys (
searchPlaceholder,startAll,stopAll…).
The rest of the codebase predominantly uses snake_case (search_placeholder,load_more).
Mixing both styles increases cognitive load and complicates automation (e.g. extracting missing keys).Prefer aligning with the existing convention, e.g.:
- "searchPlaceholder": "Search groups...", + "search_placeholder": "Search groups...", - "startAll": "Start All", + "start_all": "Start All",
- Optional – you could move generic strings such as
"start","stop","viewDetails"to thecommonnamespace to avoid future duplication.Failing to address (1) will silently override one of the definitions at JSON-parse time – hard to track down later.
[ raise_critical_issue ]
…roups - Handle empty string serverGroupId by converting to undefined in GroupEditModal - Prevent sending null or empty serverGroupId values in API payload - Fix VM group creation to properly associate with parent server group
- Initialize serverGroupId as empty string instead of undefined - Exclude serverGroupId from payload when empty to avoid sending null - Ensure consistent handling across create and edit modes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/features/groups/components/GroupEditModal.vue (1)
277-277: Fix type assertion to use proper type narrowing.The past review comment correctly identified this type assertion issue. Instead of using
any, use proper type narrowing with the imported types.Apply this fix:
- serverGroupId: group.type === 'vm' ? (group as any).serverGroupId : undefined, + serverGroupId: group.type === 'vm' ? (group as GroupVmResponseDto).serverGroupId : undefined,
🧹 Nitpick comments (2)
src/features/groups/api.ts (2)
20-26: Use optional chaining for safer property access.The static analysis correctly suggests using optional chaining for more concise and safer code.
Apply this improvement:
- if (response.data && response.data.items) { - response.data.items = response.data.items.map((item) => ({ + response.data?.items && (response.data.items = response.data.items.map((item) => ({ ...item, type: 'server' as const, - })); - } + })));
33-39: Use optional chaining for safer property access.Same improvement applies here for consistency and safety.
Apply this improvement:
- if (response.data && response.data.items) { - response.data.items = response.data.items.map((item) => ({ + response.data?.items && (response.data.items = response.data.items.map((item) => ({ ...item, type: 'vm' as const, - })); - } + })));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/features/groups/api.ts(1 hunks)src/features/groups/components/GroupEditModal.vue(1 hunks)src/features/groups/components/ResourceSelector.vue(1 hunks)src/features/groups/types.ts(1 hunks)src/locales/fr.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/groups/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/features/groups/api.ts (1)
src/features/groups/types.ts (11)
ServerGroupListParams(65-70)GroupServerListResponseDto(28-33)VmGroupListParams(72-75)GroupVmListResponseDto(35-40)ShutdownRequestDto(61-63)ShutdownPreviewParams(77-80)ShutdownPreviewListResponseDto(52-59)GroupServerResponseDto(4-14)GroupVmResponseDto(16-26)CreateGroupPayload(82-91)UpdateGroupPayload(93-101)
🪛 Biome (1.9.4)
src/features/groups/api.ts
[error] 20-20: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
src/features/groups/components/GroupEditModal.vue (1)
1-393: Well-implemented group management component.The component demonstrates excellent Vue 3 practices with proper TypeScript typing, comprehensive form handling, internationalization, and accessibility features. The reactive state management and event handling are well-structured.
src/features/groups/components/ResourceSelector.vue (1)
1-254: Excellent resource selector implementation.This component showcases strong Vue 3 practices with:
- Proper TypeScript typing and interfaces
- Comprehensive accessibility support
- Responsive design with view mode switching
- Efficient filtering and selection logic
- Good internationalization integration
The component is well-structured and follows best practices.
src/features/groups/api.ts (1)
42-191: Well-structured API module with comprehensive functionality.The API module provides excellent type safety and comprehensive group management capabilities. The endpoint design, payload transformation, and error handling are well-implemented.
src/locales/fr.json (5)
154-158: Approve new pagination translations.The new pagination translations are properly integrated and provide good French localization for the pagination controls.
216-218: Approve new UPS efficiency and maintenance translations.These new keys provide necessary localization for enhanced UPS monitoring features.
942-949: Approve new common action translations.These common UI action translations enhance the internationalization support across the application.
997-1041: Comprehensive history event localization.The new history namespace provides excellent French translations for user actions, descriptions, and details. The structured approach with actions, descriptions, and details subsections is well-organized.
1042-1151: Excellent groups feature localization.The comprehensive groups namespace provides complete French translations for all group management features including forms, actions, shutdown sequences, and resource management. The translations are well-structured and maintain consistency with the application's terminology.
…mation modal - Create DeleteConfirmModal component with proper UI/UX - Add warning for groups containing resources - Support async deletion with loading state - Add French and English translations for modal content - Fix issue where users with disabled alerts wouldn't see confirmation
- Load virtual machines instead of physical servers when editing VM groups - Call loadResources() when opening edit modal to ensure proper resource filtering - Fix resource type filtering based on group type
- Remove !isEditing condition to allow parent group selection in edit mode - Enable users to change the parent server group of existing VM groups
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/locales/fr.json (1)
1-1342: Duplicate keys detected in JSON files – please remove collisionsThe audit script uncovered duplicate property names that will silently override earlier entries:
• src/locales/fr.json: duplicate keys → action, actions, active, active_servers, activity, admin, admin_role, admin_url_label, admin_url_placeholder, all_loaded, all_rooms, apply, auth, back, brand_label, brand_other, brand_select, cancel, capacity_hint, capacity_label, capacity_placeholder, card_view, cascade, changelog, connected_servers, cooling_air, cooling_hybrid, cooling_liquid, create, create_button, create_description, create_modal_subtitle, create_title, creating, creation_error, creation_info, dashboard, date, default_room_main, default_room_setup, delete, description, descriptionPlaceholder, details, edit, edit_role, edit_title, email, entity, error, errors, filters, finish, form, general_title, grace_off, grace_off_hint, grace_off_label, grace_off_placeholder, grace_on, grace_on_hint, grace_on_label, grace_on_placeholder, grid, group, groups, headers, history, inactive, ip, ip_address, ip_hint, ip_invalid, ip_label, ip_placeholder, list, list_title, load_rooms_error, loading, loading_rooms, location_hint, location_placeholder, location_title, login, login_placeholder, login_required, login_success, logout, member_since, model_label, model_placeholder, monitoring, monitoring_coming_soon, monitoring_placeholder, name, namePlaceholder, name_hint, name_label, name_placeholder, name_required, negative_delay_error, network_title, next, noRoom, no_results, no_roles, no_room, no_servers_found, no_ups_found, offline, online, overview, page, password, password_placeholder, password_required, preferences, previous, priority, priority_hint, priority_label, profile, register_success, reset_password, results, role, role_added, roles, room, room_created, room_label, rooms, runtime_text, runtime_title, save, search_placeholder, security, selectAll, select_role, select_room, select_room_error, select_room_hint, select_ups_hint, selected, selected_id, selected_name, server_capacity_text, server_capacity_title, server_created, servers, settings, settings_extra, shutdown, start, state, stats, stop, submit, submitting, subtitle, table_view, tabs, temperature, timeline, title, total, total_servers, try_different_search, type, type_label, type_physical, type_virtual, unexpected_error, unknown, ups, ups_created, ups_label, ups_units, user, user_roles, username, username_placeholder, users, view, view_details, virtual_machines, vms, warning, welcome_title
• src/locales/en.json: same duplicate keys as above
• biome.json: duplicate keys → all, enabled, linter, parser, recommended
• tsconfig.json: duplicate key → path
Please dedupe these entries so that each JSON key is unique and earlier values aren’t inadvertently overridden at runtime.
♻️ Duplicate comments (1)
src/locales/en.json (1)
1038-1046: Repeated camelCase keys incommonblock – naming-style issue still unresolvedThe earlier review (see past comments) already flagged
loadMore,selectAll,selectNoneas camelCase outliers in a snake_case file.
They are still here and will keep tripping i18n-lint / grep.- "loadMore": "Load More", - "selectAll": "Select All", - "selectNone": "Select None", + "load_more": "Load more", + "select_all": "Select all", + "select_none": "Select none",(Apply the same rule to any future additions.)
🧹 Nitpick comments (3)
src/locales/fr.json (3)
1178-1291: Consider splitting the new “groups” block into a dedicated locale file (or deeper nesting).The
groupssection just added is ~110 keys long. Keeping everything in one 1-3000-line JSON file makes duplicate-key mistakes and merge conflicts almost inevitable.
Most i18n loaders (vue-i18n, i18next, lingui, …) let you load per-namespace JSON files, e.g.src/locales/fr/ common.json groups.json ← move this section history.jsonNo functional issue, but refactoring now will pay off when further group-related strings are added.
555-579: Key naming insettings_extrais drifting – normalise before it calcifies.Inside
settings_extrawe now have
two_factor,two_factor_enabled,two_factor_disabled.Unless the UI really needs three distinct keys, collapse to one sub-object for clarity:
"two_factor": { "label": "Authentification à deux facteurs", "enabled": "activée", "disabled": "désactivée" }Reduces mental load and avoids translators having to remember subtle variations.
172-176: French wording for pagination “showing” feels awkward.
"showing": "Affichage"is literal and will read like “Affichage 1–20 sur 53”.
"affichage"is a noun; what you need is a verb phrase, e.g.-"showing": "Affichage" +"showing": "Affiche"or
"Affichage de"depending on sentence construction.
Minor UX polish.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/charts/BaseChart.vue(0 hunks)src/components/ui/StatCard.vue(0 hunks)src/features/ilos/views/HelloWorld.vue(0 hunks)src/features/profile/components/ProfileDangerZone.vue(0 hunks)src/features/profile/components/ProfileHeader.vue(0 hunks)src/features/profile/components/ProfileInfoCard.vue(0 hunks)src/features/profile/components/ProfileSecurityCard.vue(0 hunks)src/features/profile/components/ResetPasswordModal.vue(1 hunks)src/features/rooms/views/CreateRoom.vue(0 hunks)src/features/ups/views/CreateUps.vue(0 hunks)src/features/vms/views/HelloWorld.vue(0 hunks)src/locales/en.json(9 hunks)src/locales/fr.json(9 hunks)src/router/index.ts(1 hunks)
💤 Files with no reviewable changes (10)
- src/features/ilos/views/HelloWorld.vue
- src/features/vms/views/HelloWorld.vue
- src/features/rooms/views/CreateRoom.vue
- src/components/ui/StatCard.vue
- src/features/profile/components/ProfileHeader.vue
- src/features/profile/components/ProfileSecurityCard.vue
- src/components/charts/BaseChart.vue
- src/features/ups/views/CreateUps.vue
- src/features/profile/components/ProfileDangerZone.vue
- src/features/profile/components/ProfileInfoCard.vue
✅ Files skipped from review due to trivial changes (1)
- src/features/profile/components/ResetPasswordModal.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/router/index.ts
| "groups": { | ||
| "title": "Groups", | ||
| "description": "Manage server and VM groups with priority-based orchestration", | ||
| "searchPlaceholder": "Search groups...", | ||
| "allTypes": "All Types", | ||
| "serverGroups": "Server Groups", | ||
| "vmGroups": "VM Groups", | ||
| "allPriorities": "All Priorities", | ||
| "priority1": "Priority 1", | ||
| "priority2": "Priority 2", | ||
| "priority3": "Priority 3", | ||
| "priority4": "Priority 4", | ||
| "priorityDesc1": "Critical", | ||
| "priorityDesc2": "High", | ||
| "priorityDesc3": "Medium", | ||
| "priorityDesc4": "Low", | ||
| "createGroup": "Create Group", | ||
| "noGroups": "No groups found", | ||
| "viewDetails": "View Details", | ||
| "start": "Start", | ||
| "stop": "Stop", | ||
| "startAll": "Start All", | ||
| "stopAll": "Stop All", | ||
| "type": "Type", | ||
| "priority": "Priority", | ||
| "cascade": "Cascade", | ||
| "cascadeEnabled": "Cascade enabled", | ||
| "cascadeDisabled": "Cascade disabled", | ||
| "resources": "Resources", | ||
| "noResources": "No resources assigned", | ||
| "room": "Room", | ||
| "serverGroup": "Server Group", | ||
| "vmGroup": "VM Group", | ||
| "flowView": "Flow View", | ||
| "fitView": "Fit View", | ||
| "toggleOrientation": "Toggle Orientation", | ||
| "view": { | ||
| "grid": "Grid View", | ||
| "list": "List View", | ||
| "sections": "Sections View", | ||
| "flow": "Flow View" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
groups section introduces a wave of camelCase keys – please switch to snake_case before this spreads
Almost every key in the new groups object (searchPlaceholder, allTypes, priorityDesc1, …) deviates from the established snake_case convention used in the rest of the file. Mixing styles hurts DX and guarantees duplicated keys down the road (cf. view_details / viewDetails déjà-vu).
Representative fix:
- "searchPlaceholder": "Search groups...",
- "allTypes": "All Types",
- "serverGroups": "Server Groups",
- "vmGroups": "VM Groups",
- "allPriorities": "All Priorities",
+ "search_placeholder": "Search groups...",
+ "all_types": "All types",
+ "server_groups": "Server groups",
+ "vm_groups": "VM groups",
+ "all_priorities": "All priorities",Before merging, run an i18n-linter (or simple script) to enforce one naming style across the whole file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "groups": { | |
| "title": "Groups", | |
| "description": "Manage server and VM groups with priority-based orchestration", | |
| "searchPlaceholder": "Search groups...", | |
| "allTypes": "All Types", | |
| "serverGroups": "Server Groups", | |
| "vmGroups": "VM Groups", | |
| "allPriorities": "All Priorities", | |
| "priority1": "Priority 1", | |
| "priority2": "Priority 2", | |
| "priority3": "Priority 3", | |
| "priority4": "Priority 4", | |
| "priorityDesc1": "Critical", | |
| "priorityDesc2": "High", | |
| "priorityDesc3": "Medium", | |
| "priorityDesc4": "Low", | |
| "createGroup": "Create Group", | |
| "noGroups": "No groups found", | |
| "viewDetails": "View Details", | |
| "start": "Start", | |
| "stop": "Stop", | |
| "startAll": "Start All", | |
| "stopAll": "Stop All", | |
| "type": "Type", | |
| "priority": "Priority", | |
| "cascade": "Cascade", | |
| "cascadeEnabled": "Cascade enabled", | |
| "cascadeDisabled": "Cascade disabled", | |
| "resources": "Resources", | |
| "noResources": "No resources assigned", | |
| "room": "Room", | |
| "serverGroup": "Server Group", | |
| "vmGroup": "VM Group", | |
| "flowView": "Flow View", | |
| "fitView": "Fit View", | |
| "toggleOrientation": "Toggle Orientation", | |
| "view": { | |
| "grid": "Grid View", | |
| "list": "List View", | |
| "sections": "Sections View", | |
| "flow": "Flow View" | |
| }, | |
| "groups": { | |
| "title": "Groups", | |
| "description": "Manage server and VM groups with priority-based orchestration", | |
| "search_placeholder": "Search groups...", | |
| "all_types": "All types", | |
| "server_groups": "Server groups", | |
| "vm_groups": "VM groups", | |
| "all_priorities": "All priorities", | |
| "priority1": "Priority 1", | |
| "priority2": "Priority 2", | |
| "priority3": "Priority 3", | |
| "priority4": "Priority 4", | |
| "priorityDesc1": "Critical", | |
| "priorityDesc2": "High", | |
| "priorityDesc3": "Medium", | |
| "priorityDesc4": "Low", | |
| "createGroup": "Create Group", | |
| "noGroups": "No groups found", | |
| "viewDetails": "View Details", | |
| "start": "Start", | |
| "stop": "Stop", | |
| "startAll": "Start All", | |
| "stopAll": "Stop All", | |
| "type": "Type", | |
| "priority": "Priority", | |
| "cascade": "Cascade", | |
| "cascadeEnabled": "Cascade enabled", | |
| "cascadeDisabled": "Cascade disabled", | |
| "resources": "Resources", | |
| "noResources": "No resources assigned", | |
| "room": "Room", | |
| "serverGroup": "Server Group", | |
| "vmGroup": "VM Group", | |
| "flowView": "Flow View", | |
| "fitView": "Fit View", | |
| "toggleOrientation": "Toggle Orientation", | |
| "view": { | |
| "grid": "Grid View", | |
| "list": "List View", | |
| "sections": "Sections View", | |
| "flow": "Flow View" | |
| } | |
| }, |
🤖 Prompt for AI Agents
In src/locales/en.json between lines 1176 and 1217, the keys in the "groups"
section use camelCase naming like "searchPlaceholder" and "priorityDesc1" which
conflicts with the established snake_case convention in the file. To fix this,
rename all camelCase keys in this section to snake_case equivalents (e.g.,
"search_placeholder", "priority_desc1") ensuring consistency. After renaming,
run an i18n-linter or a script to verify that the entire file uniformly uses
snake_case keys to prevent duplication and maintain developer experience.
- Add Props interface with isReadOnly property to CreateServerStep, VmDiscoveryStep, and CompleteStep components - Disable all form inputs when isReadOnly is true using :disabled prop binding - Replace submit buttons with read-only messages when in read-only mode - Load saved data from store when components mount in read-only mode - Update VmTable component to hide edit buttons in read-only mode - Save VM discovery data to store for later retrieval - Add i18n translations for read-only messages in English locale This allows users to view but not modify data when navigating back to previous setup steps
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/features/setup/components/steps/VmDiscoveryStep.vue (1)
118-136: Move mock data loading inside the component lifecycleThe mock VM loading logic is executing at the module level, which causes timing issues. The
setTimeoutcalls are registered immediately when the module loads, before the component is mounted and props are available. This means the mock data will be added even whenisReadOnlyis true.Move the mock data loading inside
onMountedto ensure it respects theisReadOnlyprop:-const MOCK_VMS = [ - { id: 'vm-1', name: 'PROD-WEB-01', ip: '192.168.1.201', state: 'active' }, - { id: 'vm-2', name: 'DB-INT-02', ip: '192.168.1.202', state: 'inactive' }, - { id: 'vm-3', name: 'APP-BACK-03', ip: '192.168.1.203', state: 'active' }, -]; - -// Only load mock data if not in read-only mode -if (!props.isReadOnly) { - setTimeout(() => { - vms.value.push(MOCK_VMS[0]); - }, 1000); - setTimeout(() => { - vms.value.push(MOCK_VMS[1]); - }, 1700); - setTimeout(() => { - vms.value.push(MOCK_VMS[2]); - isLoading.value = false; - }, 2300); -} onMounted(async () => { if (!setupStore.setupStatus) { await setupStore.checkSetupStatus(); } const steps = await setupStore.getSetupProgress(); const serverStep = steps.find( (step: any) => step.step === SetupStep.CREATE_SERVER, ); serverId.value = serverStep?.metadata?.id ?? null; if (!serverId.value) { isLoading.value = false; toast.error(t('vm_discovery.no_server_error')); router.push('/setup/create-server'); return; } // Load saved data if in read-only mode if (props.isReadOnly) { const vmData = setupStore.getStepData(SetupStep.VM_DISCOVERY); if (vmData && vmData.vms) { vms.value = vmData.vms; isLoading.value = false; return; } } + + // Only load mock data if not in read-only mode + if (!props.isReadOnly) { + const MOCK_VMS = [ + { id: 'vm-1', name: 'PROD-WEB-01', ip: '192.168.1.201', state: 'active' }, + { id: 'vm-2', name: 'DB-INT-02', ip: '192.168.1.202', state: 'inactive' }, + { id: 'vm-3', name: 'APP-BACK-03', ip: '192.168.1.203', state: 'active' }, + ]; + + setTimeout(() => { + vms.value.push(MOCK_VMS[0]); + }, 1000); + setTimeout(() => { + vms.value.push(MOCK_VMS[1]); + }, 1700); + setTimeout(() => { + vms.value.push(MOCK_VMS[2]); + isLoading.value = false; + }, 2300); + } });
♻️ Duplicate comments (3)
src/locales/en.json (2)
1066-1079: Inconsistent key naming style persists incommonblockThe mixing of camelCase and snake_case conventions was already flagged in a previous review. This inconsistency still needs to be addressed.
1209-1250:groupssection still uses camelCase keys inconsistentlyThe camelCase naming convention in the groups namespace was already flagged in a previous review. This needs to be converted to snake_case for consistency.
src/locales/fr.json (1)
1307-1308: Critical: Duplicate JSON keys will cause translation failuresThe keys
"servers"and"vms"are defined multiple times within the groups namespace. Only the last definition will be used, causing translation issues.Either remove these duplicate keys if they're redundant, or rename them to more specific keys:
- "servers": "Serveurs", - "vms": "Machines virtuelles", + "resourceServers": "Serveurs", + "resourceVms": "Machines virtuelles",
🧹 Nitpick comments (3)
src/features/setup/components/steps/CreateUpsStep.vue (1)
469-507: Resolve TODO comments before mergingThere are several TODO comments that need to be addressed:
- Whether to include brand, model, and capacity fields in the creation DTO (lines 477-480)
- Which fields to send to the backend (line 489)
- Navigation handling after UPS creation (line 496)
These implementation details should be clarified with the backend team and product requirements.
Would you like me to help implement the proper field handling based on the backend API requirements?
src/features/setup/components/steps/CreateServerStep.vue (1)
614-616: Replace console.error with proper error loggingUsing
console.errordirectly is not recommended for production code. Consider using a centralized logging service or error tracking solution.Replace console.error with a logging service:
} catch (error) { - console.error('Erreur lors du chargement des salles:', error); + // Use your logging service + // logger.error('Failed to load rooms', { error }); if (roomData.id) {And:
} catch (error) { - console.error('Erreur lors du chargement des UPS:', error); + // Use your logging service + // logger.error('Failed to load UPS', { error }); if (upsData.id) {Also applies to: 639-641
src/features/setup/components/SetupProgress.vue (1)
205-214: Consider allowing navigation to all previous stepsThe current implementation only allows navigation back to the welcome step. Since previous steps are displayed in read-only mode, users should be able to review all their previous configurations.
Allow navigation to any previous step:
const handleStepClick = (stepKey: string, index: number) => { // Allow navigation to current step if (index === currentStepIndex.value) { emit('go-to-step', stepKey); } - // Allow navigation to welcome step only among previous steps - else if (index < currentStepIndex.value && stepKey === 'welcome') { + // Allow navigation to any previous step + else if (index < currentStepIndex.value) { emit('go-to-step', stepKey); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
package.json(2 hunks)src/features/auth/components/AuthInput.vue(3 hunks)src/features/profile/components/ResetPasswordModal.vue(1 hunks)src/features/rooms/views/RoomListView.vue(8 hunks)src/features/setup/components/SetupProgress.vue(1 hunks)src/features/setup/components/steps/CompleteStep.vue(14 hunks)src/features/setup/components/steps/CreateRoomStep.vue(7 hunks)src/features/setup/components/steps/CreateServerStep.vue(24 hunks)src/features/setup/components/steps/CreateUpsStep.vue(23 hunks)src/features/setup/components/steps/VmDiscoveryStep.vue(8 hunks)src/features/setup/components/steps/VmTable.vue(4 hunks)src/features/setup/components/steps/WelcomeStep.vue(1 hunks)src/features/setup/router/router.ts(0 hunks)src/features/setup/views/SetupView.vue(1 hunks)src/locales/en.json(17 hunks)src/locales/fr.json(14 hunks)
💤 Files with no reviewable changes (1)
- src/features/setup/router/router.ts
✅ Files skipped from review due to trivial changes (2)
- src/features/rooms/views/RoomListView.vue
- src/features/auth/components/AuthInput.vue
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/features/setup/components/steps/VmTable.vue
- src/features/setup/components/steps/CompleteStep.vue
- src/features/setup/components/steps/CreateRoomStep.vue
- src/features/profile/components/ResetPasswordModal.vue
🔇 Additional comments (3)
src/features/setup/components/steps/WelcomeStep.vue (3)
1-119: Well-structured template with excellent UX enhancements!The refactor from static cards to dynamic feature rendering is clean and maintainable. Good use of animations, gradients, and conditional rendering based on the
isReadOnlyprop.
121-217: Clean script implementation with proper TypeScript usage!The utility functions are well-implemented, and the component properly handles the setup flow navigation. Good use of Vue 3 Composition API patterns.
219-265: Smooth animations enhance the user experience!The CSS animations are well-crafted with appropriate timing and transforms. The scoped styles ensure no conflicts with other components.
| <div | ||
| class="fixed inset-0 bg-[url('/grid.svg')] bg-center opacity-[0.02] dark:opacity-[0.04] pointer-events-none" | ||
| ></div> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify the grid.svg asset exists
The background references a hardcoded /grid.svg path. Ensure this asset exists in the public directory to avoid 404 errors.
🏁 Script executed:
#!/bin/bash
# Description: Check if grid.svg exists in common public asset locations
# Check for grid.svg in public directory
fd -t f "grid.svg" public/
# Check for grid.svg in src/assets
fd -t f "grid.svg" src/assets/
# Check for any svg files that might be the grid pattern
fd -e svg -t f | rg -i "grid"Length of output: 87
grid.svg asset is missing
I ran searches against public/ and src/assets/ and didn’t find any grid.svg file, which will lead to 404s at runtime. Please ensure the asset exists or update the reference:
• Add grid.svg under public/ (so it’s served at "/grid.svg")
• Or place it in src/assets/ and update the URL (e.g. background-image: url('@/assets/grid.svg'))
• If the file lives elsewhere, adjust the path accordingly
🤖 Prompt for AI Agents
In src/features/setup/views/SetupView.vue around lines 5 to 7, the background
image references a missing grid.svg asset, causing 404 errors at runtime. Fix
this by either adding the grid.svg file to the public/ directory so it is
accessible at "/grid.svg", or move the file to src/assets/ and update the URL in
the background-image style to use the correct relative path such as
"@/assets/grid.svg". Ensure the path matches the actual location of the asset.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/features/profile/views/ProfileView.vue (1)
1-624: Consider breaking down this large component.This component is quite large (600+ lines) and handles multiple responsibilities:
- User profile display
- Tab navigation
- Multiple modals
- Activity management
- Stats computation
Consider extracting some functionality into smaller, focused components or composables to improve maintainability.
Potential extractions:
useProfileTabscomposable for tab managementProfileOverviewTab,ProfileSecurityTab, etc. as separate componentsuseUserStatscomposable for stats computationuseUserActivitiescomposable for activity management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/composables/useApiCache.ts(1 hunks)src/features/auth/store.ts(2 hunks)src/features/dashboard/store.ts(1 hunks)src/features/dashboard/views/DashboardView.vue(5 hunks)src/features/profile/views/ProfileView.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/dashboard/views/DashboardView.vue
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/features/dashboard/store.ts (3)
src/features/dashboard/types.ts (1)
FullDashboardStatsDto(1-15)src/composables/useApiCache.ts (1)
useApiCache(6-84)src/features/dashboard/api.ts (1)
dashboardApi(20-85)
🔇 Additional comments (8)
src/features/auth/store.ts (2)
82-91: LGTM! Caching implementation improves performance.The addition of the
forceparameter and caching logic prevents redundant API calls when the user data is already available. The implementation correctly handles the token validation and maintains backward compatibility.
203-203: Good refactoring to reuse cached user fetching logic.The update to use
fetchCurrentUser()instead of directly callinggetMe()ensures consistent user data handling and leverages the new caching mechanism.src/composables/useApiCache.ts (1)
1-84: Excellent caching composable implementation.This is a well-designed generic caching utility with the following strengths:
- Clean API with proper TypeScript generics
- Automatic TTL-based expiration with cleanup
- Convenient
withCachemethod for cache-or-fetch patterns- Configurable TTL per operation
- Memory-efficient with automatic cleanup of expired entries
The 5-minute default TTL is appropriate for most API data scenarios.
src/features/dashboard/store.ts (3)
5-5: Good integration of the caching composable.The import and usage of the API cache composable aligns well with the overall caching strategy introduced in this PR.
12-26: Effective caching implementation for dashboard stats.The 2-minute cache TTL is appropriate for dashboard statistics, balancing freshness with performance. The
forceparameter provides flexibility for cache invalidation when needed.
34-42: Smart dynamic cache key strategy for history data.The cache key construction
dashboard-history-${entity}-${months}ensures proper cache isolation for different entity types and time periods. The 5-minute TTL is reasonable for historical data.src/features/profile/views/ProfileView.vue (2)
5-5: Good integration of presence store.The presence store integration provides real-time online/offline status for users, enhancing the profile view functionality.
138-149: Good presence integration in onMounted.The presence fetching logic is well-implemented with proper error handling and loading state management.
| const stats = computed(() => { | ||
| if (!user.value) return []; | ||
|
|
||
| const createdDate = new Date(user.value.createdAt); | ||
| const daysSinceCreation = Math.floor( | ||
| (Date.now() - createdDate.getTime()) / (1000 * 60 * 60 * 24), | ||
| ); | ||
|
|
||
| return [ | ||
| { | ||
| label: t('profile.stats.member_since'), | ||
| value: daysSinceCreation + ' ' + t('profile.stats.days'), | ||
| }, | ||
| { | ||
| label: t('profile.stats.total_logins'), | ||
| value: '157', | ||
| }, | ||
| { | ||
| label: t('profile.stats.last_login'), | ||
| value: t('profile.stats.today'), | ||
| }, | ||
| { | ||
| label: t('profile.stats.security_score'), | ||
| value: user.value.isTwoFactorEnabled ? '95%' : '60%', | ||
| }, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting stats computation logic.
The stats computation logic is complex and contains hardcoded values like '157' for total logins. Consider extracting this into a separate composable and making the data dynamic.
-const stats = computed(() => {
- if (!user.value) return [];
- // ... complex computation with hardcoded values
-});
+const { stats, loading: statsLoading } = useUserStats(user);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/features/profile/views/ProfileView.vue around lines 110 to 136, the stats
computation logic is complex and includes hardcoded values such as '157' for
total logins. Extract this logic into a separate composable function to improve
code organization and reusability. Modify the composable to accept dynamic data
inputs for total logins and other stats instead of hardcoded values, ensuring
the stats are computed based on actual user data.
| const handleToggle2FA = async () => { | ||
| if (!user.value) return; | ||
| if (user.value.isTwoFactorEnabled) { | ||
| if (await auth.disable2FAUser()) user.value.isTwoFactorEnabled = false; | ||
| if (await auth.disable2FAUser()) { | ||
| toast.success(t('toast.twofa_disabled')); | ||
| } | ||
| } else { | ||
| router.push({ name: 'Enable2FA' }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors in 2FA toggle.
The 2FA toggle logic should have error handling for the case where disable2FAUser() fails.
const handleToggle2FA = async () => {
if (!user.value) return;
- if (user.value.isTwoFactorEnabled) {
- if (await auth.disable2FAUser()) {
- toast.success(t('toast.twofa_disabled'));
- }
- } else {
- router.push({ name: 'Enable2FA' });
- }
+ try {
+ if (user.value.isTwoFactorEnabled) {
+ if (await auth.disable2FAUser()) {
+ toast.success(t('toast.twofa_disabled'));
+ }
+ } else {
+ router.push({ name: 'Enable2FA' });
+ }
+ } catch (error: any) {
+ toast.error(error.message || t('toast.twofa_error'));
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleToggle2FA = async () => { | |
| if (!user.value) return; | |
| if (user.value.isTwoFactorEnabled) { | |
| if (await auth.disable2FAUser()) user.value.isTwoFactorEnabled = false; | |
| if (await auth.disable2FAUser()) { | |
| toast.success(t('toast.twofa_disabled')); | |
| } | |
| } else { | |
| router.push({ name: 'Enable2FA' }); | |
| } | |
| }; | |
| const handleToggle2FA = async () => { | |
| if (!user.value) return; | |
| try { | |
| if (user.value.isTwoFactorEnabled) { | |
| if (await auth.disable2FAUser()) { | |
| toast.success(t('toast.twofa_disabled')); | |
| } | |
| } else { | |
| router.push({ name: 'Enable2FA' }); | |
| } | |
| } catch (error: any) { | |
| toast.error(error.message || t('toast.twofa_error')); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/features/profile/views/ProfileView.vue around lines 163 to 172, the
handleToggle2FA function lacks error handling for the async call to
disable2FAUser(). Update the function to catch any errors thrown by
disable2FAUser(), and handle them appropriately, such as showing an error toast
message or logging the error, to ensure the UI responds correctly if disabling
2FA fails.
| const mockActivities = ref([ | ||
| { | ||
| id: '1', | ||
| type: 'login' as const, | ||
| title: t('profile.activity.login'), | ||
| description: t('profile.activity.login_desc'), | ||
| timestamp: new Date(Date.now() - 1000 * 60 * 30), | ||
| metadata: { | ||
| ip: '192.168.1.100', | ||
| browser: 'Chrome 120.0', | ||
| }, | ||
| }, | ||
| { | ||
| id: '2', | ||
| type: 'profile_update' as const, | ||
| title: t('profile.activity.profile_update'), | ||
| description: t('profile.activity.profile_update_desc'), | ||
| timestamp: new Date(Date.now() - 1000 * 60 * 60 * 2), | ||
| }, | ||
| { | ||
| id: '3', | ||
| type: 'password_change' as const, | ||
| title: t('profile.activity.password_change'), | ||
| description: t('profile.activity.password_change_desc'), | ||
| timestamp: new Date(Date.now() - 1000 * 60 * 60 * 24), | ||
| }, | ||
| { | ||
| id: '4', | ||
| type: 'failed_login' as const, | ||
| title: t('profile.activity.failed_login'), | ||
| description: t('profile.activity.failed_login_desc'), | ||
| timestamp: new Date(Date.now() - 1000 * 60 * 60 * 48), | ||
| metadata: { | ||
| ip: '10.0.0.50', | ||
| }, | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded mock activity data with dynamic data.
The mock activities are hardcoded with static data, which is not suitable for production. This should be replaced with actual user activity data from an API.
Consider creating a user activity API endpoint and composable:
-const mockActivities = ref([
- {
- id: '1',
- type: 'login' as const,
- title: t('profile.activity.login'),
- description: t('profile.activity.login_desc'),
- timestamp: new Date(Date.now() - 1000 * 60 * 30),
- metadata: {
- ip: '192.168.1.100',
- browser: 'Chrome 120.0',
- },
- },
- // ... other hardcoded activities
-]);
+const { activities, loading: activitiesLoading, fetchActivities } = useUserActivities();
+
+onMounted(async () => {
+ if (user.value?.id) {
+ await fetchActivities(user.value.id);
+ }
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/features/profile/views/ProfileView.vue between lines 72 and 108, replace
the hardcoded mockActivities array with dynamic data fetched from an API. Create
a user activity API endpoint to retrieve real user activity data and implement a
composable function to call this API and manage the state. Update the component
to use this composable to load and display actual user activities instead of
static mock data.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
src/features/groups/components/panel/ViewModeContent.vue (1)
344-348: Replace browser confirm() with proper confirmation handling.Using the browser's native
confirm()dialog provides poor UX and doesn't match the application's design system. This was also flagged in previous reviews for other components.Consider emitting a confirmation request event instead:
const confirmDelete = () => { - if (confirm(t('groups.deleteConfirmMessage', { name: props.group.name }))) { - emit('delete', props.group); - } + emit('delete', props.group); };The parent component should handle the confirmation with a proper modal dialog to ensure consistent UX across the application.
src/features/groups/components/GroupCard.vue (1)
151-157: Fix hardcoded strings for proper internationalization.The
resourceLabelcomputed property still uses hardcoded English strings ("server", "servers", "VM", "VMs"), which was flagged in previous reviews but appears unresolved.Import the i18n composable and use proper translation keys:
+import { useI18n } from 'vue-i18n'; + +const { t } = useI18n(); const resourceLabel = computed(() => { const count = resourceCount.value; if (props.group.type === 'SERVER') { - return count === 1 ? 'server' : 'servers'; + return t('servers.label', count); } - return count === 1 ? 'VM' : 'VMs'; + return t('vms.label', count); });Ensure the translation keys support pluralization in your i18n configuration.
src/features/groups/components/GroupManagementPanel.vue (1)
318-331: Replace browser confirm() with proper confirmation modal.Using the browser's native
confirm()dialog provides poor UX and doesn't match the application's design system. This pattern should be consistent across the application.Consider using a proper confirmation modal component:
const setMode = async (newMode: PanelMode) => { if (newMode === 'view' && hasChanges.value) { - if (!confirm(t('common.unsavedChangesConfirm'))) { - return; - } + // Emit event for parent to handle confirmation + emit('confirm-mode-change', { newMode, hasChanges: true }); + return; } mode.value = newMode; // ... rest of logic };src/features/groups/views/GroupView.vue (2)
289-295: Add TODO comments for placeholder implementations.These functions are placeholders that only log to console. They should be clearly marked for future implementation as suggested in previous reviews.
const startGroupResources = async (group: GroupResponseDto) => { + // TODO: Implement actual resource start logic console.log('Starting resources for group:', group.name); + // await groupApi.startGroup(group.id); }; const stopGroupResources = async (group: GroupResponseDto) => { + // TODO: Implement actual resource stop logic console.log('Stopping resources for group:', group.name); + // await groupApi.stopGroup(group.id); };
182-188: Improve error handling with user feedback.The error is silently logged to console without informing the user. Consider adding a toast notification to improve user experience.
const loadGroups = async () => { try { await groupStore.fetchAllGroups(); } catch (error) { console.error('Failed to load groups:', error); + toast.error(t('groups.loadError')); } };src/locales/en.json (2)
1216-1349: Fix camelCase keys to maintain consistency.The new
groupssection introduces many camelCase keys (searchPlaceholder,allTypes,serverGroups,priorityDesc1, etc.) which conflicts with the established snake_case convention used throughout the file. This was flagged in previous reviews but hasn't been addressed.Representative fix for the most common violations:
- "searchPlaceholder": "Search groups...", - "allTypes": "All Types", - "serverGroups": "Server Groups", - "vmGroups": "VM Groups", - "allPriorities": "All Priorities", - "priorityDesc1": "Critical", - "createGroup": "Create Group", - "noGroups": "No groups found", - "viewDetails": "View Details", - "startAll": "Start All", - "stopAll": "Stop All", + "search_placeholder": "Search groups...", + "all_types": "All types", + "server_groups": "Server groups", + "vm_groups": "VM groups", + "all_priorities": "All priorities", + "priority_desc1": "Critical", + "create_group": "Create group", + "no_groups": "No groups found", + "view_details": "View details", + "start_all": "Start all", + "stop_all": "Stop all",This pattern should be applied to all camelCase keys in this section to maintain consistency.
1072-1086: Fix remaining camelCase keys in common section.The
commonsection still contains camelCase keys that violate the established snake_case convention, as noted in previous reviews.- "loadMore": "Load More", - "selectAll": "Select All", - "deselectAll": "Deselect All", - "selectNone": "Select None", - "lastUpdated": "Last updated", - "unsavedChanges": "You have unsaved changes", - "unsavedChangesConfirm": "You have unsaved changes. Are you sure you want to discard them?" + "load_more": "Load more", + "select_all": "Select all", + "deselect_all": "Deselect all", + "select_none": "Select none", + "last_updated": "Last updated", + "unsaved_changes": "You have unsaved changes", + "unsaved_changes_confirm": "You have unsaved changes. Are you sure you want to discard them?"
🧹 Nitpick comments (7)
src/features/groups/components/GroupFlow.vue (1)
213-215: Dynamic node coloring not implementedThe
getNodeColorfunction always returns green regardless of the node's properties.Implement dynamic coloring based on group priority or type:
-const getNodeColor = () => { - return '#22c55e'; // green +const getNodeColor = (node: Node) => { + const priorityClass = node.data?.priorityClass; + switch (priorityClass) { + case 'high': return '#ef4444'; // red + case 'medium': return '#eab308'; // yellow + default: return '#22c55e'; // green + } };src/features/groups/components/SimpleResourceSelector.vue (1)
149-149: Use English for code commentsThe comment is in French. Please use English for consistency.
- excludeGroupId?: string; // Pour exclure le groupe en cours d'édition + excludeGroupId?: string; // To exclude the group being editedsrc/features/groups/components/panel/EditModeContent.vue (1)
271-271: Remove unused codeThe empty destructuring from
useI18n()is not needed since$tis used directly in the template.-const {} = useI18n();src/features/groups/store.ts (1)
135-150: Consider optimizing group list refresh after creation.While calling
fetchAllGroups()after creating a group ensures data consistency, it might be more efficient to append the new group to the existing list instead of refetching all groups, especially for large datasets.Consider this optimization if performance becomes an issue:
const addGroup = async (payload: CreateGroupDto) => { isCreating.value = true; error.value = null; try { const newGroup = await createGroup(payload); - await fetchAllGroups(); + if (groups.value) { + groups.value.data.unshift(newGroup); + groups.value.total++; + } else { + await fetchAllGroups(); + } return newGroup; } catch (err: any) { error.value = err.response?.data?.message ?? err.message ?? 'Error creating group'; throw err; } finally { isCreating.value = false; } };src/features/groups/views/GroupView.vue (2)
190-192: Implement placeholder function.This function is a placeholder that only logs to console and needs proper implementation.
const loadMoreGroups = async () => { - console.log('Load more groups'); + // TODO: Implement pagination logic + try { + await groupStore.fetchMoreGroups(); + } catch (error) { + console.error('Failed to load more groups:', error); + toast.error(t('groups.loadMoreError')); + } };
266-284: Reduce toast notification spam.Showing multiple toast messages in sequence for a single action can overwhelm users. Consider showing only the final result.
const handleGroupStart = async (group: GroupResponseDto) => { try { await startGroupResources(group); - toast.success(t('groups.startingGroup', { name: group.name })); toast.success(t('groups.startSuccess')); } catch (error) { toast.error(t('groups.startError')); } }; const handleGroupStop = async (group: GroupResponseDto) => { try { await stopGroupResources(group); - toast.success(t('groups.stoppingGroup', { name: group.name })); toast.success(t('groups.stopSuccess')); } catch (error) { toast.error(t('groups.stopError')); } };src/locales/fr.json (1)
1-1392: Monolithic locale file is reaching maintenance limits
fr.jsonis now >1 400 lines. Merging, reviewing and spotting duplicates already proved tricky (#1129/1130 earlier).
Most modern Vue i18n setups support loading locale fragments per-feature (e.g.src/locales/fr/groups.json,.../history.json, …).Advantages:
• Avoids duplicate keys at compile-time
• Minimises merge conflicts between parallel feature branches
• Keeps PRs and reviews focused
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/features/groups/api.ts(1 hunks)src/features/groups/components/CreateGroupModal.vue(1 hunks)src/features/groups/components/DeleteConfirmModal.vue(1 hunks)src/features/groups/components/GroupCard.vue(1 hunks)src/features/groups/components/GroupDetailsModal.vue(1 hunks)src/features/groups/components/GroupEditModal.vue(1 hunks)src/features/groups/components/GroupFlow.vue(1 hunks)src/features/groups/components/GroupList.vue(1 hunks)src/features/groups/components/GroupManagementPanel.vue(1 hunks)src/features/groups/components/GroupSelection.vue(1 hunks)src/features/groups/components/ShutdownPreviewDialog.vue(1 hunks)src/features/groups/components/SimpleResourceSelector.vue(1 hunks)src/features/groups/components/panel/EditModeContent.vue(1 hunks)src/features/groups/components/panel/ViewModeContent.vue(1 hunks)src/features/groups/store.ts(1 hunks)src/features/groups/types.ts(1 hunks)src/features/groups/views/GroupView.vue(1 hunks)src/features/servers/api.ts(1 hunks)src/features/vms/api.ts(1 hunks)src/locales/en.json(17 hunks)src/locales/fr.json(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/features/servers/api.ts
- src/features/groups/components/GroupSelection.vue
- src/features/groups/components/ShutdownPreviewDialog.vue
- src/features/groups/components/DeleteConfirmModal.vue
- src/features/groups/components/GroupEditModal.vue
- src/features/groups/api.ts
- src/features/groups/components/GroupList.vue
- src/features/groups/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/features/groups/store.ts (2)
src/features/groups/types.ts (8)
PaginatedGroupResponseDto(13-19)PreviewShutdownResponseDto(29-36)GroupResponseDto(3-11)GroupQueryDto(43-48)GroupShutdownDto(38-41)CreateGroupDto(50-54)UpdateGroupDto(56-59)GroupType(1-1)src/features/groups/api.ts (7)
fetchGroups(12-17)previewGroupShutdown(39-44)executeGroupShutdown(46-52)fetchGroupById(19-22)createGroup(24-27)updateGroup(29-32)deleteGroup(34-37)
🔇 Additional comments (11)
src/features/groups/components/GroupFlow.vue (1)
197-199: Incomplete edge implementationThe edges array is always empty, suggesting the flow visualization doesn't show relationships between groups.
If group relationships are planned, consider implementing edge generation. Otherwise, document why edges are not needed in this visualization.
src/features/groups/components/CreateGroupModal.vue (1)
336-414: Well-implemented resource assignment with error handlingThe two-step process of creating a group and then assigning resources with proper error handling for partial failures is a robust approach.
src/features/groups/store.ts (2)
55-78: LGTM! Clean delegation pattern for type-specific fetching.The
fetchServerGroupsandfetchVmGroupsmethods now properly delegate tofetchAllGroupswith type filtering, which is much cleaner than the previous implementation and addresses the pagination state consistency issues mentioned in past reviews.
172-192: Excellent improvement: Proper data refresh after deletion.The
removeGroupmethod now callsfetchAllGroups()instead of manually decrementingtotalItems, which correctly addresses the pagination consistency issue mentioned in past reviews. This ensures accurate counts when filters or pagination are active.src/features/groups/components/GroupDetailsModal.vue (2)
390-427: Good error handling and resource loading logic.The
loadGroupResourcesmethod properly handles both server and VM types, includes error handling with console logging, and resets the resource list on errors. The filtering logic for group-specific resources is correct.
441-445: Smart optimization for room data loading.The component efficiently checks if rooms are already loaded before making an unnecessary API call. This prevents redundant requests and improves performance.
src/features/groups/components/panel/ViewModeContent.vue (1)
110-143: Excellent use of SVG for progress visualization.The circular progress indicator using SVG is well-implemented with smooth transitions and proper percentage calculations. The dynamic stroke-dasharray based on active resources ratio creates an effective visual representation.
src/features/groups/components/GroupCard.vue (1)
164-172: Nice hover effect with smooth transitions.The SCSS hover transform effect adds a pleasant interactive feel to the cards without being overwhelming. The use of CSS custom properties for timing is a good practice.
src/features/groups/components/GroupManagementPanel.vue (2)
372-383: Smart resource filtering logic for group editing.The resource filtering logic correctly handles the difference between edit mode (allowing resources from current group) and create mode (only unassigned resources). This prevents conflicts and provides a good user experience.
120-159: Excellent use of transitions for smooth mode switching.The TransitionGroup with slide-fade animations provides a polished user experience when switching between view and edit modes. The absolute positioning ensures smooth transitions without layout shifts.
src/locales/fr.json (1)
1064-1083: Verification passed: no duplicate translation keys in src/locales/fr.jsonI ran a duplicate-key detection script against
src/locales/fr.jsonand found no repeated keys. The newly added entries undercommonwill not silently override any existing values.
| export const patchVm = async ( | ||
| id: string, | ||
| data: { groupId?: string | null }, | ||
| ) => { | ||
| const response = await api.patch(`/vm/${id}`, data); | ||
| return response.data; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency with existing API patterns
The patchVm function uses async/await while other functions in this file return promises directly. Also, the endpoint uses singular /vm/{id} while others use plural /vms.
Apply this diff to maintain consistency:
-export const patchVm = async (
- id: string,
- data: { groupId?: string | null },
-) => {
- const response = await api.patch(`/vm/${id}`, data);
- return response.data;
+export const patchVm = (id: string, data: { groupId?: string | null }) => {
+ return api.patch(`/vms/${id}`, data);
};Also consider adding error handling consistent with how it's handled in other parts of the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const patchVm = async ( | |
| id: string, | |
| data: { groupId?: string | null }, | |
| ) => { | |
| const response = await api.patch(`/vm/${id}`, data); | |
| return response.data; | |
| }; | |
| export const patchVm = (id: string, data: { groupId?: string | null }) => { | |
| return api.patch(`/vms/${id}`, data); | |
| }; |
🤖 Prompt for AI Agents
In src/features/vms/api.ts around lines 11 to 17, refactor the patchVm function
to return the promise directly instead of using async/await to maintain
consistency with other API functions. Change the endpoint from singular
`/vm/${id}` to plural `/vms/${id}` to match the existing pattern. Additionally,
add error handling consistent with other functions in this file, such as
catching errors and handling or propagating them appropriately.
| nodeList.push({ | ||
| id: group.id, | ||
| type: 'group', | ||
| position, | ||
| data: { | ||
| label: group.name, | ||
| priorityClass: 'low', | ||
| type: group.type, | ||
| resourceCount: 0, | ||
| resourceLabel: group.type === 'SERVER' ? 'servers' : 'VMs', | ||
| group, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Use actual group data instead of hardcoded values
The node data uses hardcoded values for priorityClass and resourceCount instead of deriving them from the actual group data.
Consider calculating these values from the group object:
nodeList.push({
id: group.id,
type: 'group',
position,
data: {
label: group.name,
- priorityClass: 'low',
+ priorityClass: getPriorityClass(group.priority),
type: group.type,
- resourceCount: 0,
+ resourceCount: group.servers?.length || group.vms?.length || 0,
resourceLabel: group.type === 'SERVER' ? 'servers' : 'VMs',
group,
},
});Add a helper function to map priority values to classes:
const getPriorityClass = (priority?: number): string => {
if (!priority) return 'low';
if (priority >= 8) return 'high';
if (priority >= 5) return 'medium';
return 'low';
};🤖 Prompt for AI Agents
In src/features/groups/components/GroupFlow.vue between lines 179 and 191, the
node data currently uses hardcoded values for priorityClass and resourceCount.
Replace these with dynamic values derived from the group object by adding a
helper function that maps the group's priority number to a priority class string
(e.g., 'low', 'medium', 'high') and use the actual resource count from the group
data instead of zero. Update the node data construction to call this helper for
priorityClass and set resourceCount based on the group's real data.
| } catch (error) { | ||
| console.error('Error loading servers:', error); | ||
| } finally { | ||
| loadingServers.value = false; | ||
| } | ||
| }; | ||
|
|
||
| const loadVms = async () => { | ||
| if (props.groupType !== 'VM') return; | ||
|
|
||
| loadingVms.value = true; | ||
| try { | ||
| const response = await fetchUvms(); | ||
| vms.value = Array.isArray(response.data) ? response.data : []; | ||
| } catch (error) { | ||
| console.error('Error loading VMs:', error); | ||
| vms.value = []; | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with user feedback
Errors are only logged to console without notifying the user.
Consider showing error messages to users:
} catch (error) {
console.error('Error loading servers:', error);
+ // Consider using a toast notification or error state
+ // toast.error($t('groups.form.errorLoadingServers'));
} finally { } catch (error) {
console.error('Error loading VMs:', error);
vms.value = [];
+ // Consider using a toast notification or error state
+ // toast.error($t('groups.form.errorLoadingVms'));
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Error loading servers:', error); | |
| } finally { | |
| loadingServers.value = false; | |
| } | |
| }; | |
| const loadVms = async () => { | |
| if (props.groupType !== 'VM') return; | |
| loadingVms.value = true; | |
| try { | |
| const response = await fetchUvms(); | |
| vms.value = Array.isArray(response.data) ? response.data : []; | |
| } catch (error) { | |
| console.error('Error loading VMs:', error); | |
| vms.value = []; | |
| } finally { | |
| } catch (error) { | |
| console.error('Error loading servers:', error); | |
| // Consider using a toast notification or error state | |
| // toast.error($t('groups.form.errorLoadingServers')); | |
| } finally { | |
| loadingServers.value = false; | |
| } | |
| }; | |
| const loadVms = async () => { | |
| if (props.groupType !== 'VM') return; | |
| loadingVms.value = true; | |
| try { | |
| const response = await fetchUvms(); | |
| vms.value = Array.isArray(response.data) ? response.data : []; | |
| } catch (error) { | |
| console.error('Error loading VMs:', error); | |
| vms.value = []; | |
| // Consider using a toast notification or error state | |
| // toast.error($t('groups.form.errorLoadingVms')); | |
| } finally { |
🤖 Prompt for AI Agents
In src/features/groups/components/SimpleResourceSelector.vue around lines 224 to
241, error handling currently only logs errors to the console without informing
the user. To fix this, add user-visible error notifications inside the catch
blocks for both server and VM loading functions. Use a UI notification method or
component to display a clear error message so users are aware of the failure,
while keeping the existing console.error for debugging.
| const selectedServerIds = ref<string[]>([]); | ||
| const selectedVmIds = ref<string[]>([]); | ||
| const isCreating = ref(false); | ||
| const availableResources = ref<any[]>([]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper TypeScript interface for resources
The availableResources uses any[] type which reduces type safety.
Define a proper interface:
interface Resource {
id: string;
name: string;
state: string;
roomId?: string;
type: 'server' | 'vm';
}
const availableResources = ref<Resource[]>([]);🤖 Prompt for AI Agents
In src/features/groups/components/CreateGroupModal.vue at line 271, the
availableResources variable is typed as any[], which reduces type safety. Define
a TypeScript interface named Resource with properties id (string), name
(string), state (string), optional roomId (string), and type ('server' | 'vm').
Then update the availableResources ref to use this Resource[] type instead of
any[] to improve type safety.
| const handleSave = async () => { | ||
| if (!isValid.value || isSaving.value) return; | ||
|
|
||
| isSaving.value = true; | ||
|
|
||
| try { | ||
| let group: GroupResponseDto; | ||
|
|
||
| if (mode.value === 'edit' && currentGroup.value) { | ||
| // Update existing group | ||
| const updatePayload: UpdateGroupDto = { | ||
| name: formData.value.name.trim(), | ||
| description: formData.value.description?.trim() || undefined, | ||
| }; | ||
| group = await updateGroup(currentGroup.value.id, updatePayload); | ||
|
|
||
| // Handle resource changes | ||
| const currentServerIds = | ||
| formData.value.type === 'SERVER' ? selectedServerIds.value : []; | ||
| const currentVmIds = | ||
| formData.value.type === 'VM' ? selectedVmIds.value : []; | ||
|
|
||
| // Calculate changes | ||
| const serversToAdd = currentServerIds.filter( | ||
| (id) => !originalServerIds.value.includes(id), | ||
| ); | ||
| const serversToRemove = originalServerIds.value.filter( | ||
| (id) => !currentServerIds.includes(id), | ||
| ); | ||
| const vmsToAdd = currentVmIds.filter( | ||
| (id) => !originalVmIds.value.includes(id), | ||
| ); | ||
| const vmsToRemove = originalVmIds.value.filter( | ||
| (id) => !currentVmIds.includes(id), | ||
| ); | ||
|
|
||
| // Apply changes | ||
| const promises = []; | ||
| for (const serverId of serversToAdd) { | ||
| promises.push(patchServer(serverId, { groupId: group.id })); | ||
| } | ||
| for (const serverId of serversToRemove) { | ||
| promises.push(patchServer(serverId, { groupId: null })); | ||
| } | ||
| for (const vmId of vmsToAdd) { | ||
| promises.push(patchVm(vmId, { groupId: group.id })); | ||
| } | ||
| for (const vmId of vmsToRemove) { | ||
| promises.push(patchVm(vmId, { groupId: null })); | ||
| } | ||
|
|
||
| await Promise.allSettled(promises); | ||
| toast.success(t('groups.updateSuccess')); | ||
| } else { | ||
| // Create new group | ||
| const createPayload: CreateGroupDto = { | ||
| name: formData.value.name.trim(), | ||
| description: formData.value.description?.trim() || undefined, | ||
| type: formData.value.type, | ||
| }; | ||
| group = await createGroup(createPayload); | ||
|
|
||
| // Assign selected resources | ||
| const resourceIds = | ||
| formData.value.type === 'SERVER' | ||
| ? selectedServerIds.value | ||
| : selectedVmIds.value; | ||
| if (resourceIds.length > 0) { | ||
| const promises = resourceIds.map((id) => | ||
| formData.value.type === 'SERVER' | ||
| ? patchServer(id, { groupId: group.id }) | ||
| : patchVm(id, { groupId: group.id }), | ||
| ); | ||
| await Promise.allSettled(promises); | ||
| } | ||
|
|
||
| toast.success(t('groups.createSuccess')); | ||
| } | ||
|
|
||
| // Refresh server store to get updated groupIds | ||
| await serverStore.fetchServers(); | ||
|
|
||
| emit('success', group); | ||
| if (mode.value === 'create') { | ||
| emit('close'); | ||
| } else { | ||
| // Update the current group data | ||
| currentGroup.value = group; | ||
| mode.value = 'view'; | ||
| await loadGroupResources(); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error saving group:', error); | ||
| toast.error( | ||
| mode.value === 'edit' ? t('groups.updateError') : t('groups.createError'), | ||
| ); | ||
| } finally { | ||
| isSaving.value = false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the lengthy handleSave method.
The handleSave method is quite long (100 lines) and handles multiple responsibilities. Consider breaking it down into smaller, focused methods for better maintainability.
Consider extracting methods like:
const handleSave = async () => {
if (!isValid.value || isSaving.value) return;
isSaving.value = true;
try {
let group: GroupResponseDto;
if (mode.value === 'edit' && currentGroup.value) {
- group = await updateGroup(currentGroup.value.id, updatePayload);
- // Handle resource changes...
+ group = await handleGroupUpdate();
} else {
- group = await createGroup(createPayload);
- // Assign selected resources...
+ group = await handleGroupCreation();
}
await serverStore.fetchServers();
emit('success', group);
// ... rest of success handling
} catch (error) {
// ... error handling
} finally {
isSaving.value = false;
}
};
+const handleGroupUpdate = async () => {
+ // Update logic here
+};
+const handleGroupCreation = async () => {
+ // Creation logic here
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/features/groups/components/GroupManagementPanel.vue between lines 431 and
530, the handleSave method is too long and handles multiple tasks. Refactor by
extracting smaller helper functions such as updateGroupResources for handling
resource additions/removals, createNewGroup for creating groups and assigning
resources, and a separate function for updating the current group state after
save. Then call these helper functions from handleSave to improve readability
and maintainability.
| "groups": { | ||
| "title": "Groupes", | ||
| "description": "Gérer les groupes de serveurs et VMs avec orchestration par priorité", | ||
| "searchPlaceholder": "Rechercher des groupes...", | ||
| "allTypes": "Tous Types", | ||
| "serverGroups": "Groupes Serveurs", | ||
| "vmGroups": "Groupes VMs", | ||
| "allPriorities": "Toutes Priorités", | ||
| "priority1": "Priorité 1", | ||
| "priority2": "Priorité 2", | ||
| "priority3": "Priorité 3", | ||
| "priority4": "Priorité 4", | ||
| "priorityDesc1": "Critique", | ||
| "priorityDesc2": "Haute", | ||
| "priorityDesc3": "Moyenne", | ||
| "priorityDesc4": "Basse", | ||
| "createGroup": "Créer un Groupe", | ||
| "noGroups": "Aucun groupe trouvé", | ||
| "viewDetails": "Voir Détails", | ||
| "start": "Démarrer", | ||
| "stop": "Arrêter", | ||
| "startAll": "Tout Démarrer", | ||
| "stopAll": "Tout Arrêter", | ||
| "type": "Type", | ||
| "priority": "Priorité", | ||
| "cascade": "Cascade", | ||
| "cascadeEnabled": "Cascade activée", | ||
| "cascadeDisabled": "Cascade désactivée", | ||
| "resources": "Ressources", | ||
| "noResources": "Aucune ressource assignée", | ||
| "room": "Salle", | ||
| "serverGroup": "Groupe Serveur", | ||
| "vmGroup": "Groupe VM", | ||
| "flowView": "Vue Flux", | ||
| "fitView": "Ajuster la Vue", | ||
| "toggleOrientation": "Basculer l'Orientation", | ||
| "totalServers": "Total serveurs", | ||
| "totalVms": "Total VMs", | ||
| "activeResources": "Actives", | ||
| "inactiveResources": "Inactives", | ||
| "assignedServers": "Serveurs assignés", | ||
| "assignedVms": "VMs assignées", | ||
| "addResources": "Ajouter des ressources", | ||
| "view": { | ||
| "grid": "Vue Grille", | ||
| "list": "Vue Liste", | ||
| "sections": "Vue Sections", | ||
| "flow": "Vue Flux" | ||
| }, | ||
| "form": { | ||
| "name": "Nom", | ||
| "namePlaceholder": "Entrer le nom du groupe", | ||
| "description": "Description", | ||
| "descriptionPlaceholder": "Entrer la description du groupe", | ||
| "type": "Type", | ||
| "priority": "Priorité", | ||
| "cascade": "Activer cascade", | ||
| "cascadeHelp": "Démarrer/arrêter automatiquement les groupes dépendants", | ||
| "room": "Salle", | ||
| "noRoom": "Aucune salle spécifique", | ||
| "selectedResources": "Ressources sélectionnées", | ||
| "selectServers": "Sélectionner des serveurs", | ||
| "selectVms": "Sélectionner des VMs", | ||
| "noServersAvailable": "Aucun serveur disponible", | ||
| "noVmsAvailable": "Aucune VM disponible", | ||
| "loadingServers": "Chargement des serveurs...", | ||
| "loadingVms": "Chargement des VMs...", | ||
| "serversSelected": "{count} serveurs sélectionnés", | ||
| "vmsSelected": "{count} VMs sélectionnées", | ||
| "alreadyAssigned": "Déjà assigné", | ||
| "partialAssignmentError": "Assignation partielle : {assigned} réussies, {failed} échouées", | ||
| "allResourcesAssigned": "Les {count} ressources ont été assignées avec succès", | ||
| "selectResourcesHelp": "Sélectionnez les ressources que vous souhaitez assigner à ce groupe" | ||
| }, | ||
| "createSuccess": "Groupe créé avec succès", | ||
| "createError": "Erreur lors de la création du groupe", | ||
| "deleteConfirm": "Êtes-vous sûr de vouloir supprimer le groupe '{name}' ?", | ||
| "deleteConfirmTitle": "Confirmer la suppression", | ||
| "deleteConfirmMessage": "Êtes-vous sûr de vouloir supprimer le groupe '{name}' ? Cette action est irréversible.", | ||
| "deleteWarning": "Attention : Ce groupe contient des ressources", | ||
| "deleteWarningDetails": "Ce groupe contient {count} ressource(s) qui seront dissociées lors de la suppression.", | ||
| "deleteSuccess": "Groupe supprimé avec succès", | ||
| "deleteError": "Erreur lors de la suppression du groupe", | ||
| "startSuccess": "Groupe démarré avec succès", | ||
| "startError": "Erreur lors du démarrage du groupe", | ||
| "stopSuccess": "Groupe arrêté avec succès", | ||
| "stopError": "Erreur lors de l'arrêt du groupe", | ||
| "startingGroup": "Démarrage de {name}...", | ||
| "stoppingGroup": "Arrêt de {name}...", | ||
| "groupShutdown": "Arrêt de Groupes", | ||
| "groupShutdownDescription": "Prévisualiser et exécuter des séquences d'arrêt contrôlées pour les groupes de serveurs et VMs", | ||
| "selectGroups": "Sélectionner des Groupes", | ||
| "selectAll": "Tout Sélectionner", | ||
| "clearAll": "Tout Effacer", | ||
| "total": "total", | ||
| "serversCount": "serveurs", | ||
| "vmsCount": "VMs", | ||
| "groupsSelected": "groupes sélectionnés", | ||
| "previewShutdown": "Prévisualiser l'Arrêt", | ||
| "shutdownPreview": "Aperçu de l'Arrêt", | ||
| "shutdownWarning": "Ceci va arrêter les groupes sélectionnés dans l'ordre de priorité. Les VMs seront arrêtées avant les serveurs.", | ||
| "shutdownOrder": "Ordre", | ||
| "name": "Nom", | ||
| "group": "Groupe", | ||
| "loadingPreview": "Chargement de l'aperçu...", | ||
| "confirmationRequired": "Confirmation Requise", | ||
| "shutdownConfirmationText": "Cette action va arrêter des serveurs et VMs. Assurez-vous que tous les services sont préparés pour l'arrêt.", | ||
| "confirmUnderstandConsequences": "Je comprends les conséquences de cet arrêt", | ||
| "executeShutdown": "Exécuter l'Arrêt", | ||
| "executing": "Exécution...", | ||
| "shutdownExecutedSuccessfully": "Arrêt exécuté avec succès", | ||
| "namePlaceholder": "Entrez le nom du groupe", | ||
| "descriptionPlaceholder": "Entrez la description du groupe (optionnel)", | ||
| "priorityHelp": "Valeurs faibles = priorité élevée", | ||
| "noRoom": "Aucune salle spécifique", | ||
| "parentServerGroup": "Groupe serveur parent", | ||
| "selectServerGroup": "Sélectionnez un groupe serveur", | ||
| "servers": "Serveurs", | ||
| "vms": "Machines virtuelles", | ||
| "selected": "sélectionné(s)", | ||
| "searchResources": "Rechercher des ressources...", | ||
| "noResourcesFound": "Aucune ressource trouvée", | ||
| "selectedCount": "{count} sur {total} sélectionné(s)", | ||
| "serverCount": "Serveurs", | ||
| "vmCount": "VMs", | ||
| "highPriority": "Priorité Élevée", | ||
| "mediumPriority": "Priorité Moyenne", | ||
| "lowPriority": "Priorité Faible" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent casing & duplicate-looking identifiers inside groups namespace
-
Key style abruptly switches to mixed camelCase (
searchPlaceholder,allTypes, …) while most other top-level namespaces use snake_case.
A consistent naming convention reduces cognitive overhead and makes grepping easier. -
Top-level keys
"name","noRoom","selected"duplicate identifiers already present insidegroups.form.
Although they live in different sub-objects and won’t collide at runtime, they are error-prone during refactors ($t('groups.name')vs$t('groups.form.name')). Consider prefixing or keeping all resource-specific keys inside theformobject only.
Example refactor:
- "searchPlaceholder": "Rechercher des groupes...",
+ "search_placeholder": "Rechercher des groupes...",
@@
- "name": "Nom",
+ "group_name": "Nom",🤖 Prompt for AI Agents
In src/locales/fr.json between lines 1213 and 1341, the keys inside the "groups"
namespace use inconsistent casing, mixing camelCase with snake_case, and some
top-level keys like "name", "noRoom", and "selected" duplicate keys inside the
"form" object. To fix this, standardize all keys in "groups" to use snake_case
for consistency and move or prefix duplicated keys so that resource-specific
keys remain only inside the "form" object, avoiding ambiguity and reducing
refactor errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/features/vms/api.ts (1)
11-17: Address previous review feedback regarding consistency.The
patchVmfunction still uses async/await while other functions in this file return promises directly, creating an inconsistency in the API pattern.src/features/groups/components/GroupFlow.vue (2)
98-116: Use proper TypeScript types for drag events.The
onServerDragEndfunction still usesanytype for the event parameter, which was flagged in previous reviews for type safety concerns.
134-163: Use proper TypeScript types for drag events.The
onVMDragEndfunction also usesanytype for the event parameter, reducing type safety.src/features/groups/stores/usePriorityStore.ts (2)
141-150: Handle partial failures in concurrent requests.Similar to
swapServerPriorities, this function needs to handle partial failures.
153-155: Loading state conflicts in concurrent fetches.When both
fetchServers()andfetchVMs()run concurrently, the sharedloadingref will cause incorrect loading states. This issue will be resolved if you implement separate loading states as suggested earlier.
🧹 Nitpick comments (3)
src/features/groups/stores/usePriorityStore.ts (3)
29-36: Use optional chaining for cleaner code.The static analysis correctly identifies an opportunity to use optional chaining.
Apply this change:
-if (response.data && response.data.items) { - servers.value = response.data.items || []; -} else { - servers.value = []; -} +servers.value = response.data?.items || [];
49-56: Use optional chaining for cleaner code.Similar to the
fetchServersfunction, use optional chaining here.Apply this change:
-if (response.data && response.data.items) { - vms.value = response.data.items || []; -} else { - vms.value = []; -} +vms.value = response.data?.items || [];
29-31: TODO: Future endpoint migration.The TODO indicates that dedicated priority endpoints will be created. Would you like me to help create a migration plan or abstraction layer to make this transition easier when the new endpoints are available?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(2 hunks)src/features/groups/components/GroupFlow.vue(1 hunks)src/features/groups/components/priority/ServerCard.vue(1 hunks)src/features/groups/components/priority/VmCard.vue(1 hunks)src/features/groups/stores/usePriorityStore.ts(1 hunks)src/features/groups/types/priority.ts(1 hunks)src/features/groups/utils/priority-utils.ts(1 hunks)src/features/vms/api.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/groups/types/priority.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/features/groups/utils/priority-utils.ts (1)
src/features/groups/types/priority.ts (1)
Priority(1-1)
src/features/groups/stores/usePriorityStore.ts (3)
src/features/groups/types/priority.ts (3)
ServerWithPriority(3-9)VMWithPriority(11-17)Priority(1-1)src/features/groups/utils/priority-utils.ts (2)
sortByPriority(71-78)swapPriorities(3-23)src/features/servers/api.ts (1)
fetchServers(87-92)
🪛 Biome (1.9.4)
src/features/groups/stores/usePriorityStore.ts
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 52-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/features/groups/components/priority/VmCard.vue (1)
1-60: Well-implemented Vue component with proper TypeScript support.The component follows Vue best practices with:
- Proper TypeScript prop definitions
- Dynamic styling based on state
- Appropriate use of utility functions for priority colors
- Clean template structure with slots for extensibility
src/features/groups/components/priority/ServerCard.vue (1)
1-85: Clean server card implementation with comprehensive display logic.The component demonstrates good Vue practices with:
- Proper TypeScript interface definitions
- Conditional rendering for optional data (IP address, VM count)
- Consistent priority color/label utilities usage
- Well-structured template with appropriate slots
src/features/groups/utils/priority-utils.ts (3)
3-23: Well-implemented priority swap utility.The
swapPrioritiesfunction correctly handles edge cases and maintains immutability with proper type constraints.
45-69: Consistent priority color and label mapping.The
getPriorityColorandgetPriorityLabelfunctions provide consistent UI mapping with proper dark mode support and clear priority semantics.
25-43: Fix logic error in reorderByPosition function.The function finds
draggedIndexafter sorting the items, but it should find the dragged item in the original array before sorting. This could lead to incorrect reordering.Apply this diff to fix the logic:
export function reorderByPosition<T extends { id: string; priority: Priority }>( items: T[], draggedId: string, targetPosition: number, ): T[] { - const draggedIndex = items.findIndex((item) => item.id === draggedId); - if (draggedIndex === -1) return items; - const sortedItems = [...items].sort((a, b) => a.priority - b.priority); - const draggedItem = sortedItems[draggedIndex]; + const draggedIndex = sortedItems.findIndex((item) => item.id === draggedId); + if (draggedIndex === -1) return items; + + const draggedItem = sortedItems[draggedIndex]; sortedItems.splice(draggedIndex, 1); sortedItems.splice(targetPosition, 0, draggedItem);Likely an incorrect or invalid review comment.
| const loading = ref(false); | ||
| const error = ref<string | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using separate loading and error states for different operations.
The shared loading and error refs across all operations can lead to race conditions and confusing UX. For example, if fetchServers() and fetchVMs() are called simultaneously, the loading state will be incorrect.
Consider using a more granular approach:
-const loading = ref(false);
-const error = ref<string | null>(null);
+const loadingStates = ref({
+ servers: false,
+ vms: false,
+ updateServer: false,
+ updateVm: false,
+});
+const errors = ref({
+ servers: null as string | null,
+ vms: null as string | null,
+ lastOperation: null as string | null,
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loading = ref(false); | |
| const error = ref<string | null>(null); | |
| const loadingStates = ref({ | |
| servers: false, | |
| vms: false, | |
| updateServer: false, | |
| updateVm: false, | |
| }); | |
| const errors = ref({ | |
| servers: null as string | null, | |
| vms: null as string | null, | |
| lastOperation: null as string | null, | |
| }); |
🤖 Prompt for AI Agents
In src/features/groups/stores/usePriorityStore.ts around lines 14 to 15, the
current single loading and error refs are shared across all operations, which
can cause race conditions and inaccurate UI states when multiple operations run
concurrently. Refactor the code to use separate loading and error states for
each asynchronous operation, such as distinct refs for fetchServers and
fetchVMs, to ensure each operation's status is tracked independently and the UI
reflects the correct state.
| try { | ||
| await Promise.all([ | ||
| api.patch(`/server/${server1Id}`, { | ||
| priority: server1.priority, | ||
| }), | ||
| api.patch(`/server/${server2Id}`, { | ||
| priority: server2.priority, | ||
| }), | ||
| ]); | ||
| } catch (err) { | ||
| servers.value = originalServers; | ||
| error.value = 'Failed to swap server priorities'; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Handle partial failures in concurrent requests.
The current implementation doesn't handle the case where one PATCH request succeeds and another fails, leading to inconsistent state between frontend and backend.
Consider using Promise.allSettled to handle partial failures:
try {
- await Promise.all([
+ const results = await Promise.allSettled([
api.patch(`/server/${server1Id}`, {
priority: server1.priority,
}),
api.patch(`/server/${server2Id}`, {
priority: server2.priority,
}),
]);
+
+ const failures = results.filter(r => r.status === 'rejected');
+ if (failures.length > 0) {
+ // Fetch fresh data to ensure consistency
+ await fetchServers();
+ error.value = 'Failed to swap server priorities';
+ throw new Error('Partial failure in swap operation');
+ }
} catch (err) {
servers.value = originalServers;
error.value = 'Failed to swap server priorities';
throw err;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await Promise.all([ | |
| api.patch(`/server/${server1Id}`, { | |
| priority: server1.priority, | |
| }), | |
| api.patch(`/server/${server2Id}`, { | |
| priority: server2.priority, | |
| }), | |
| ]); | |
| } catch (err) { | |
| servers.value = originalServers; | |
| error.value = 'Failed to swap server priorities'; | |
| throw err; | |
| } | |
| try { | |
| const results = await Promise.allSettled([ | |
| api.patch(`/server/${server1Id}`, { | |
| priority: server1.priority, | |
| }), | |
| api.patch(`/server/${server2Id}`, { | |
| priority: server2.priority, | |
| }), | |
| ]); | |
| const failures = results.filter(r => r.status === 'rejected'); | |
| if (failures.length > 0) { | |
| // Fetch fresh data to ensure consistency | |
| await fetchServers(); | |
| error.value = 'Failed to swap server priorities'; | |
| throw new Error('Partial failure in swap operation'); | |
| } | |
| } catch (err) { | |
| servers.value = originalServers; | |
| error.value = 'Failed to swap server priorities'; | |
| throw err; | |
| } |
🤖 Prompt for AI Agents
In src/features/groups/stores/usePriorityStore.ts around lines 113 to 126, the
current code uses Promise.all for concurrent PATCH requests, which does not
handle partial failures properly. Replace Promise.all with Promise.allSettled to
wait for all requests to complete regardless of success or failure. Then, check
the results to detect any failed requests; if any fail, revert servers.value to
originalServers, set the error message, and throw an error to maintain
consistent frontend and backend state.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/features/groups/components/GroupFlow.vue (2)
104-104: Use proper TypeScript types instead ofany.Replace the
anytype with the proper VueDraggable event type for better type safety.-const onServerDragEnd = async (event: any) => { +import type { SortableEvent } from 'sortablejs'; + +const onServerDragEnd = async (event: SortableEvent) => {
141-141: Use proper TypeScript types instead ofany.Replace the
anytype with the proper VueDraggable event type for better type safety.-const onVMDragEnd = async (event: any) => { +const onVMDragEnd = async (event: SortableEvent) => {src/features/groups/stores/usePriorityStore.ts (1)
18-19: Consider using separate loading and error states for different operations.The shared
loadinganderrorrefs across all operations can lead to race conditions and confusing UX. For example, iffetchServers()andfetchVMs()are called simultaneously, the loading state will be incorrect.Consider using a more granular approach:
-const loading = ref(false); -const error = ref<string | null>(null); +const loadingStates = ref({ + servers: false, + vms: false, + updateServer: false, + updateVm: false, +}); +const errors = ref({ + servers: null as string | null, + vms: null as string | null, + lastOperation: null as string | null, +});
🧹 Nitpick comments (2)
src/features/groups/stores/usePriorityStore.ts (2)
43-46: Consider preserving existing data on fetch errors.When fetch operations fail, the current implementation clears the data by setting it to empty arrays. This might cause a jarring UX where data disappears on transient network errors.
Consider keeping existing data on error:
} catch (err) { error.value = 'Failed to fetch servers'; console.error('Error fetching servers:', err); - servers.value = []; + // Keep existing data on error } // Same for VMs: } catch (err) { error.value = 'Failed to fetch VMs'; console.error('Error fetching VMs:', err); - vms.value = []; + // Keep existing data on error }Also applies to: 65-68
119-119: Use consistent language in code comments.For better team collaboration, use English for code comments to maintain consistency.
- // Mettre à jour le state local avec la réponse + // Update local state with the responseAlso applies to: 146-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(2 hunks)src/features/groups/components/GroupFlow.vue(1 hunks)src/features/groups/components/priority/ServerCard.vue(1 hunks)src/features/groups/components/priority/VmCard.vue(1 hunks)src/features/groups/stores/usePriorityStore.ts(1 hunks)src/features/groups/types/priority.ts(1 hunks)src/features/groups/utils/priority-utils.ts(1 hunks)src/features/vms/api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/features/vms/api.ts
- src/features/groups/components/priority/VmCard.vue
- src/features/groups/components/priority/ServerCard.vue
- src/features/groups/utils/priority-utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/features/groups/stores/usePriorityStore.ts (2)
src/features/groups/types/priority.ts (7)
ServerWithPriority(3-9)VMWithPriority(11-17)ServerPriorityResponse(28-34)VmPriorityResponse(36-42)Priority(1-1)SwapServerPriorityResponse(44-47)SwapVmPriorityResponse(49-52)src/features/groups/utils/priority-utils.ts (1)
sortByPriority(71-78)
🔇 Additional comments (2)
src/features/groups/components/GroupFlow.vue (1)
34-44: No action needed:data-server-idis already boundThe
ServerCardcomponent in
- src/features/groups/components/priority/ServerCard.vue (line 8)
already declares:data-server-id="server.id"on its root element, so the DOM queries in
GroupFlow.vuewill find the attribute as expected.Likely an incorrect or invalid review comment.
src/features/groups/types/priority.ts (1)
1-52: Well-structured type definitions!The type definitions are comprehensive and properly typed. The use of union types for
Priorityand state values provides good type safety.
| const updateServerVMs = (serverId: string, vms: VMWithPriority[]) => { | ||
| const allVMs = [...priorityStore.vms]; | ||
|
|
||
| allVMs.forEach((vm) => { | ||
| if (vm.serverId === serverId) { | ||
| const newVm = vms.find((v) => v.id === vm.id); | ||
| if (newVm) { | ||
| const index = allVMs.findIndex((v) => v.id === vm.id); | ||
| allVMs[index] = { ...vm, priority: newVm.priority }; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| priorityStore.vms = allVMs; | ||
| }; |
There was a problem hiding this comment.
Avoid directly modifying store state outside of actions.
The function directly assigns to priorityStore.vms which violates Pinia best practices. Store state should only be modified through actions.
Consider adding an action in the store to handle this update:
// In usePriorityStore.ts, add:
+function updateVMsForServer(serverId: string, updatedVMs: VMWithPriority[]) {
+ vms.value = vms.value.map(vm => {
+ if (vm.serverId === serverId) {
+ const updated = updatedVMs.find(v => v.id === vm.id);
+ return updated || vm;
+ }
+ return vm;
+ });
+}
// In GroupFlow.vue:
-const updateServerVMs = (serverId: string, vms: VMWithPriority[]) => {
- const allVMs = [...priorityStore.vms];
- // ... current logic
- priorityStore.vms = allVMs;
-};
+const updateServerVMs = (serverId: string, vms: VMWithPriority[]) => {
+ priorityStore.updateVMsForServer(serverId, vms);
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateServerVMs = (serverId: string, vms: VMWithPriority[]) => { | |
| const allVMs = [...priorityStore.vms]; | |
| allVMs.forEach((vm) => { | |
| if (vm.serverId === serverId) { | |
| const newVm = vms.find((v) => v.id === vm.id); | |
| if (newVm) { | |
| const index = allVMs.findIndex((v) => v.id === vm.id); | |
| allVMs[index] = { ...vm, priority: newVm.priority }; | |
| } | |
| } | |
| }); | |
| priorityStore.vms = allVMs; | |
| }; | |
| // In GroupFlow.vue, replace the entire updateServerVMs function with: | |
| const updateServerVMs = (serverId: string, vms: VMWithPriority[]) => { | |
| priorityStore.updateVMsForServer(serverId, vms); | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/components/GroupFlow.vue around lines 125 to 139, the
function updateServerVMs directly modifies the Pinia store state by assigning to
priorityStore.vms, which violates best practices. To fix this, move the logic
that updates the VMs into a dedicated action inside the priorityStore. Then,
call that action from this component instead of directly assigning to the state.
This ensures state mutations happen only within store actions.
| const fromServerId = from | ||
| .closest('.server-column') | ||
| ?.querySelector('[data-server-id]') | ||
| ?.getAttribute('data-server-id'); | ||
| const toServerId = to | ||
| .closest('.server-column') | ||
| ?.querySelector('[data-server-id]') | ||
| ?.getAttribute('data-server-id'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fragile DOM queries with a more robust approach.
The current implementation relies on DOM structure which is fragile and could break with template changes.
Consider using VueDraggable's data attributes or item properties:
// In the template, add data attributes to VueDraggable:
<VueDraggable
:model-value="getVMsByServerId(server.id)"
+ :data-server-id="server.id"
@update:model-value="(vms: VMWithPriority[]) => updateServerVMs(server.id, vms)"
// ... other props
>
// In onVMDragEnd:
-const fromServerId = from
- .closest('.server-column')
- ?.querySelector('[data-server-id]')
- ?.getAttribute('data-server-id');
-const toServerId = to
- .closest('.server-column')
- ?.querySelector('[data-server-id]')
- ?.getAttribute('data-server-id');
+const fromServerId = from.getAttribute('data-server-id');
+const toServerId = to.getAttribute('data-server-id');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fromServerId = from | |
| .closest('.server-column') | |
| ?.querySelector('[data-server-id]') | |
| ?.getAttribute('data-server-id'); | |
| const toServerId = to | |
| .closest('.server-column') | |
| ?.querySelector('[data-server-id]') | |
| ?.getAttribute('data-server-id'); | |
| <!-- Template: add a data attribute for the server ID --> | |
| <VueDraggable | |
| :model-value="getVMsByServerId(server.id)" | |
| :data-server-id="server.id" | |
| @update:model-value="(vms: VMWithPriority[]) => updateServerVMs(server.id, vms)" | |
| <!-- ... other props ... --> | |
| > | |
| <!-- ... --> | |
| </VueDraggable> | |
| <!-- Script: simplify the drag-end lookup to read the attribute directly --> | |
| const onVMDragEnd = (event: SortableEvent) => { | |
| const { from, to } = event; | |
| const fromServerId = from.getAttribute('data-server-id'); | |
| const toServerId = to.getAttribute('data-server-id'); | |
| // ... rest of your logic ... | |
| }; |
🤖 Prompt for AI Agents
In src/features/groups/components/GroupFlow.vue around lines 146 to 154, the
code uses fragile DOM queries to get server IDs by traversing the DOM, which can
break if the template changes. Instead, refactor to use VueDraggable's provided
data attributes or item properties directly to access the server IDs, avoiding
reliance on DOM structure. Update the logic to retrieve these IDs from the
draggable event or item context rather than querying the DOM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/features/servers/views/ServerDetailsView.vue (2)
204-223: Implement proper credential cleanup based on remember preference.The current implementation doesn't respect the user's choice when they uncheck "Remember for this session". This is the same issue identified in previous reviews.
387-394: Add error handling for SSH connection failures.The SshTerminal component should emit connection errors, and the parent should handle them to provide user feedback. This mirrors the previous review comment.
src/features/servers/store.ts (1)
81-81: Extract magic number to a constant.The page size of 12 is still hardcoded here and in other places (lines 28, 71, 81). This creates a maintenance burden as identified in previous reviews.
src/locales/en.json (2)
1063-1090: Still mixing camelCase & snake_case incommonkeysKeys like
loadMore,view_details,selectAll, … remain camelCase while the rest of the file is snake_case – the same concern raised previously. Keeping two styles side-by-side hurts grep-ability and will lead to duplicated keys (view_detailsalready exists elsewhere in snake_case).Convert the new camelCase keys to snake_case before they spread further.
1219-1352:groupsnamespace still introduces a wave of camelCase keysNearly every key (
searchPlaceholder,allTypes,priorityDesc1, …) deviates from the established snake_case convention in this repository. Please normalise to snake_case for consistency and maintainability.src/locales/fr.json (2)
1061-1087: Mélange persistant camelCase / snake_case danscommonLes clés
loadMore,view_details,selectAll, … sont toujours en camelCase alors que le reste du fichier utilise snake_case. Même remarque que lors des précédentes revues : standardisez en snake_case.
1216-1344: Le namespacegroupsreste en camelCase
searchPlaceholder,allTypes,priorityDesc1, etc. ne respectent pas le snake_case utilisé ailleurs. Merci d’harmoniser avant que ces clés ne se dupliquent dans le code.
🧹 Nitpick comments (1)
src/features/groups/components/PriorityInput.vue (1)
74-103: Consider making preset ranges configurable.The preset ranges are hardcoded, which might not be flexible enough for different use cases. Consider extracting these to props or a configuration object to allow customization.
+interface PresetConfig { + label: string; + value: number; + min: number; + max: number; + color: string; +} interface Props { modelValue: number; inputId?: string; inputClass?: string; required?: boolean; disabled?: boolean; showPresets?: boolean; hint?: string; + presets?: PresetConfig[]; } const props = withDefaults(defineProps<Props>(), { // ... existing defaults + presets: () => [ + { + label: 'Critical', + value: 1, + min: 1, + max: 99, + color: 'bg-red-100 text-red-800 dark:bg-red-900 dark:text-red-200', + }, + // ... other presets + ], }); -const presets = [ - // ... hardcoded presets -];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/features/groups/components/PriorityInput.vue(1 hunks)src/features/groups/types/priority.ts(1 hunks)src/features/groups/utils/priority-utils.ts(1 hunks)src/features/servers/api.ts(1 hunks)src/features/servers/components/CreateServer.vue(7 hunks)src/features/servers/components/ServerEditModal.vue(4 hunks)src/features/servers/store.ts(2 hunks)src/features/servers/views/ServerDetailsView.vue(8 hunks)src/features/setup/components/steps/CreateServerStep.vue(24 hunks)src/layouts/views/DefaultLayout.vue(3 hunks)src/locales/en.json(18 hunks)src/locales/fr.json(15 hunks)src/router/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/router/index.ts
- src/layouts/views/DefaultLayout.vue
- src/features/servers/components/CreateServer.vue
- src/features/groups/utils/priority-utils.ts
- src/features/setup/components/steps/CreateServerStep.vue
- src/features/groups/types/priority.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/features/servers/api.ts (1)
src/features/servers/types.ts (1)
Server(6-23)
src/features/servers/store.ts (2)
src/features/servers/types.ts (4)
Server(6-23)ServerListParams(49-52)ServerListResponse(42-47)CreateServerPayload(25-40)src/features/servers/api.ts (5)
fetchServers(87-92)getServersAdmin(115-118)createServer(110-113)fetchServerById(96-108)updateServer(128-134)
🔇 Additional comments (12)
src/features/servers/api.ts (3)
115-118: LGTM! Clean admin API implementation.The function follows the established patterns and provides proper type safety for admin-level server retrieval.
120-126: LGTM! Focused patch function for group assignment.The function is well-designed for its specific use case of updating server group assignments. The limited scope of the
dataparameter (groupId?: string | null) ensures this function stays focused on group management operations.
128-134: LGTM! Comprehensive server update function.The function provides full server update capability with proper type safety using
Partial<Server>, allowing flexible updates while maintaining type checking.src/features/groups/components/PriorityInput.vue (1)
109-113: LGTM! Proper input validation and bounds checking.The bounds enforcement ensures data integrity while providing a good user experience by automatically correcting out-of-range values.
src/features/servers/components/ServerEditModal.vue (2)
41-58: LGTM! Robust form state management.The watcher properly handles synchronization between the server prop and form state, including nested iLO properties. The immediate flag ensures the form is initialized correctly.
150-156: LGTM! Proper integration of PriorityInput component.The component is well-integrated with appropriate props and styling that matches the form's design system.
src/features/servers/views/ServerDetailsView.vue (2)
126-133: LGTM! Proper store integration.The refactoring to use the server store improves consistency and leverages centralized state management effectively.
135-153: LGTM! Clean edit workflow implementation.The edit functionality properly uses the store's methods and provides good user feedback via toast notifications.
src/features/servers/store.ts (4)
28-55: LGTM! Excellent pagination implementation.The fetchServers method properly handles pagination with append functionality, updating all relevant state variables. The error handling and loading states are well-managed.
71-74: LGTM! Proper load more implementation.The method correctly checks for loading state and hasMore flag to prevent duplicate requests and unnecessary API calls.
109-133: LGTM! Comprehensive server editing with state synchronization.The editServer method properly updates both the list and current server, ensuring UI consistency across components.
152-155: LGTM! Thoughtful backward compatibility.The legacy aliases ensure existing code continues to work while the store is being migrated, providing a smooth transition path.
| const handleSave = () => { | ||
| if (props.server) { | ||
| const updatedData = { | ||
| ...props.server, | ||
| name: form.name, | ||
| ip: form.ip, | ||
| type: form.type, | ||
| priority: form.priority, | ||
| adminUrl: form.adminUrl, | ||
| login: form.login, | ||
| grace_period_on: form.grace_period_on, | ||
| grace_period_off: form.grace_period_off, | ||
| ilo: form.ilo.name || form.ilo.ip || form.ilo.login ? form.ilo : null, | ||
| }; | ||
|
|
||
| // Update the parent's server data | ||
| Object.assign(props.server, updatedData); | ||
| emit('save'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding direct prop mutation.
The current implementation mutates the props.server object directly via Object.assign. This violates Vue's principle of props being read-only and could cause reactivity issues.
const handleSave = () => {
if (props.server) {
const updatedData = {
...props.server,
name: form.name,
ip: form.ip,
type: form.type,
priority: form.priority,
adminUrl: form.adminUrl,
login: form.login,
grace_period_on: form.grace_period_on,
grace_period_off: form.grace_period_off,
ilo: form.ilo.name || form.ilo.ip || form.ilo.login ? form.ilo : null,
};
- // Update the parent's server data
- Object.assign(props.server, updatedData);
- emit('save');
+ emit('save', updatedData);
}
};Update the emit interface:
interface Emits {
(e: 'close'): void;
- (e: 'save'): void;
+ (e: 'save', server: Server): void;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSave = () => { | |
| if (props.server) { | |
| const updatedData = { | |
| ...props.server, | |
| name: form.name, | |
| ip: form.ip, | |
| type: form.type, | |
| priority: form.priority, | |
| adminUrl: form.adminUrl, | |
| login: form.login, | |
| grace_period_on: form.grace_period_on, | |
| grace_period_off: form.grace_period_off, | |
| ilo: form.ilo.name || form.ilo.ip || form.ilo.login ? form.ilo : null, | |
| }; | |
| // Update the parent's server data | |
| Object.assign(props.server, updatedData); | |
| emit('save'); | |
| } | |
| }; | |
| const handleSave = () => { | |
| if (props.server) { | |
| const updatedData = { | |
| ...props.server, | |
| name: form.name, | |
| ip: form.ip, | |
| type: form.type, | |
| priority: form.priority, | |
| adminUrl: form.adminUrl, | |
| login: form.login, | |
| grace_period_on: form.grace_period_on, | |
| grace_period_off: form.grace_period_off, | |
| ilo: form.ilo.name || form.ilo.ip || form.ilo.login ? form.ilo : null, | |
| }; | |
| emit('save', updatedData); | |
| } | |
| }; |
| const handleSave = () => { | |
| if (props.server) { | |
| const updatedData = { | |
| ...props.server, | |
| name: form.name, | |
| ip: form.ip, | |
| type: form.type, | |
| priority: form.priority, | |
| adminUrl: form.adminUrl, | |
| login: form.login, | |
| grace_period_on: form.grace_period_on, | |
| grace_period_off: form.grace_period_off, | |
| ilo: form.ilo.name || form.ilo.ip || form.ilo.login ? form.ilo : null, | |
| }; | |
| // Update the parent's server data | |
| Object.assign(props.server, updatedData); | |
| emit('save'); | |
| } | |
| }; | |
| interface Emits { | |
| (e: 'close'): void; | |
| (e: 'save', server: Server): void; | |
| } |
🤖 Prompt for AI Agents
In src/features/servers/components/ServerEditModal.vue around lines 60 to 79,
the code directly mutates the props.server object using Object.assign, which
violates Vue's rule that props are read-only and can cause reactivity problems.
To fix this, avoid mutating props.server directly; instead, create a new updated
server object with the form data and emit it via the 'save' event so the parent
component can update its state accordingly. Adjust the emit call to pass the
updated server data as a parameter.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores