Clear selected rows after update - #694
Conversation
Nikhil-Ashoka
commented
Jul 24, 2026
Nikhil-Ashoka
commented
- Clear selected rows after update in User management page.
- Defect: https://jazz07.rchland.ibm.com:13443/jazz/web/projects/CSSD#action=com.ibm.team.workitem.viewWorkItem&id=800732
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR refactors the user selection clearing mechanism in the User Management page by replacing direct clearSelectedRows(tableRef) function calls with event-based communication using eventBus.emit('clear-selected'). The refactoring improves code maintainability by decoupling the selection clearing logic from the component, allowing the useTableSelectableComposable to handle the event centrally.
🔍 General Feedback
- ✅ Good refactoring approach: Using event-based communication is a cleaner pattern that reduces coupling between components
- ✅ Consistent implementation: The change is applied consistently across all four locations where selection clearing is needed
- ✅ Event listener exists: The
useTableSelectableComposable.jsproperly handles theclear-selectedevent ⚠️ Timing consideration: Two instances emit the event immediately after callinguserManagement.getUsers()without awaiting, which could lead to race conditions where selections are cleared before the user list is fully refreshed
| authenticationStore.logout(); | ||
| } else { | ||
| userManagement.getUsers(); | ||
| eventBus.emit('clear-selected'); |
There was a problem hiding this comment.
🟡 The event emission after userManagement.getUsers() may cause a race condition. The getUsers() call is asynchronous but not awaited, so clear-selected could be emitted before the user list is fully refreshed. This might lead to inconsistent UI state.
Consider awaiting the getUsers() call before emitting the event:
| eventBus.emit('clear-selected'); | |
| await userManagement.getUsers(); | |
| eventBus.emit('clear-selected'); |
| }); | ||
| } | ||
| userManagement.getUsers(); | ||
| eventBus.emit('clear-selected'); |
There was a problem hiding this comment.
🟡 Same race condition issue as above. The getUsers() call is asynchronous but not awaited before emitting clear-selected. This could lead to the selection being cleared before the user list is fully updated.
| eventBus.emit('clear-selected'); | |
| await userManagement.getUsers(); | |
| eventBus.emit('clear-selected'); |
- Clear selected rows after update in User management page. - Defect: https://jazz07.rchland.ibm.com:13443/jazz/web/projects/CSSD#action=com.ibm.team.workitem.viewWorkItem&id=800732 Signed-off-by: Nikhil Ashoka <a.nikhil@ibm.com>
606d2d9 to
132404f
Compare
There was a problem hiding this comment.
📋 Review Summary
This PR standardizes the approach to clearing selected rows after CRUD operations across multiple components by replacing direct clearSelectedRows() calls with event bus emissions. The change improves code consistency and maintainability by centralizing the selection clearing logic.
🔍 General Feedback
- Positive: The refactoring to use
eventBus.emit('clear-selected')creates a more decoupled architecture and follows a consistent pattern across the application - Positive: Removal of redundant comments and duplicate function calls in Sessions.vue improves code cleanliness
- Concern: EventLogs.vue manually sets checkbox states in addition to emitting the event, while other components only emit the event. This inconsistency suggests either the event handler doesn't fully clear all state, or EventLogs has unique state management requirements that should be documented
- Note: Previous review comments about race conditions in UserManagement.vue (lines 532 and 587) remain valid - consider awaiting
getUsers()before emitting the clear event to prevent UI state inconsistencies
| this.reloadEventLogData(); | ||
| eventBus.emit('clear-selected'); | ||
| this.tableHeaderCheckboxModel = false; | ||
| this.tableHeaderCheckboxIndeterminate = false; |
There was a problem hiding this comment.
🟡 Inconsistent approach to clearing selection state. In EventLogs.vue, you're manually setting checkbox states (tableHeaderCheckboxModel and tableHeaderCheckboxIndeterminate), but in other files you're only emitting the event. This suggests the event handler might not be properly clearing all state, or EventLogs has additional state that other components don't have.
Consider either:
- Ensuring the
clear-selectedevent handler clears all necessary state consistently across all components, or - Documenting why EventLogs requires manual state clearing while others don't
| // Clear selection and reset header checkbox after disconnecting sessions | ||
| clearSelectedRows(tableSessionsRef); | ||
| // Clear all selections after disconnect operation completes | ||
| eventBus.emit('clear-selected'); |
There was a problem hiding this comment.
🟢 Good cleanup removing redundant comments and duplicate clearSelectedRows call. The code is now cleaner with just the event emission.
| .finally(() => { | ||
| // Clear selection and reset header checkbox after batch deletion | ||
| clearSelectedRows(tableRef); | ||
| eventBus.emit('clear-selected'); |
There was a problem hiding this comment.
🟢 Good refactoring to use the event bus pattern instead of direct function calls. This makes the code more maintainable and follows a consistent pattern across the application.
2 participants
