app-catalog: enchance release management with filter and bulk action#474
app-catalog: enchance release management with filter and bulk action#474Quent1L wants to merge 5 commits intoheadlamp-k8s:mainfrom
Conversation
Quent1L
force-pushed
the
feature/app-catalog-release-filter
branch
from
03e1914 to
52f8756
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the app-catalog plugin's release management interface by adding comprehensive filtering, bulk operations, and inline quick actions. Users can now efficiently manage Helm releases through a more streamlined interface without constantly navigating to detail pages.
Key Changes:
- Added smart filtering with debounced name search and namespace autocomplete
- Implemented bulk delete operations with multi-selection and confirmation dialogs
- Created inline quick actions menu for upgrade, rollback, and delete operations directly from the list view
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
ReleaseFilters.tsx |
New component providing debounced name search and namespace filtering with autocomplete |
ReleaseActionsMenu.tsx |
New component implementing three-dot menu for inline release actions (upgrade, rollback, delete) |
RollbackDialog.tsx |
New component showing release version history with timestamps for rollback selection |
DeleteConfirmDialog.tsx |
New component for single release deletion confirmation |
BulkDeleteDialog.tsx |
New component for confirming bulk deletion of multiple releases |
BulkActionsToolbar.tsx |
New component displaying selection count and bulk action buttons |
List.tsx |
Major refactoring to integrate filtering, bulk operations, inline actions, and proper state management for all operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Select | ||
| value={revertVersion} | ||
| onChange={event => onVersionChange(event.target.value as string)} | ||
| id="revert" |
There was a problem hiding this comment.
Missing ARIA label for the Select component. The InputLabel with id="revert" is used, but the Select component should have an labelId="revert" prop to properly associate it with the label for screen readers.
| id="revert" | |
| id="revert" | |
| labelId="revert" |
There was a problem hiding this comment.
Seems like a good suggestion.
| <Checkbox | ||
| size="small" | ||
| checked={ | ||
| filteredReleases | ||
| ? filteredReleases.length > 0 && | ||
| selectedReleases.size === filteredReleases.length | ||
| : false | ||
| } | ||
| indeterminate={ | ||
| filteredReleases | ||
| ? selectedReleases.size > 0 && selectedReleases.size < filteredReleases.length | ||
| : false | ||
| } | ||
| onChange={handleSelectAll} | ||
| /> |
There was a problem hiding this comment.
The checkbox for selecting all releases should have an aria-label to describe its purpose for screen reader users. Add aria-label="Select all releases" to improve accessibility.
| <Checkbox | ||
| size="small" | ||
| checked={isSelected} | ||
| onChange={() => handleSelectRelease(release.name, release.namespace)} | ||
| /> |
There was a problem hiding this comment.
Each checkbox in the release rows should have an aria-label that identifies which release it's for. Consider adding aria-label={Select ${release.name}} to improve screen reader experience.
There was a problem hiding this comment.
This looks like a good suggestion.
| const handleConfirmDelete = useCallback(() => { | ||
| if (selectedRelease) { | ||
| deleteRelease(selectedRelease.namespace, selectedRelease.name) | ||
| .then(() => { | ||
| setIsDeleting(true); | ||
| enqueueSnackbar(`Delete request for release ${selectedRelease.name} accepted`, { | ||
| variant: 'info', | ||
| }); | ||
| setOpenDeleteAlert(false); | ||
| checkDeleteReleaseStatus(selectedRelease.name, selectedRelease.namespace); |
There was a problem hiding this comment.
Race condition: If the user performs multiple delete operations in quick succession, the deleteStatusTimeoutRef will be overwritten, potentially causing multiple polling loops to run simultaneously. The code should clear any existing timeout before starting a new one in the handleConfirmDelete function.
There was a problem hiding this comment.
I see this code above:
// Clear any existing timeout to prevent race conditions
if (deleteStatusTimeoutRef.current) {
clearTimeout(deleteStatusTimeoutRef.current);
deleteStatusTimeoutRef.current = null;
}
So marking this as resolved.
|
@Quent1L This is really awesome. Please give us some time to look through it :) |
Quent1L
force-pushed
the
feature/app-catalog-release-filter
branch
2 times, most recently
from
6fff8f1 to
65ed4ca
Compare
|
@Quent1L thanks for all those changes. I'll try it out in the next few days. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for this.
I went through all the copilot comments and marked a bunch of them as resolved when I thought they didn't make sense, were done already, or were too nitpicky.
There's some comments left open, can you please have a look?
The github CI check is failing with this typescript error:
npm run tsc
Error: src/components/releases/ReleaseList.stories.tsx(64,51): error TS2322: Type 'Promise<{ releases: { name: string; namespace: string; chart: { metadata: { name: string; appVersion: string; icon: string; }; }; version: string; info: { status: string; last_deployed: string; }; }[]; }>' is not assignable to type 'Promise<ReleasesResponse>'.
Type '{ releases: { name: string; namespace: string; chart: { metadata: { name: string; appVersion: string; icon: string; }; }; version: string; info: { status: string; last_deployed: string; }; }[]; }' is not assignable to type 'ReleasesResponse'.
Types of property 'releases' are incompatible.
Type '{ name: string; namespace: string; chart: { metadata: { name: string; appVersion: string; icon: string; }; }; version: string; info: { status: string; last_deployed: string; }; }[]' is not assignable to type 'Release[]'.
Type '{ name: string; namespace: string; chart: { metadata: { name: string; appVersion: string; icon: string; }; }; version: string; info: { status: string; last_deployed: string; }; }' is not assignable to type 'Release'.
Types of property 'version' are incompatible.
Type 'string' is not assignable to type 'number'.
Problem running tsc inside of "."| <Checkbox | ||
| size="small" | ||
| checked={isSelected} | ||
| onChange={() => handleSelectRelease(release.name, release.namespace)} | ||
| /> |
There was a problem hiding this comment.
This looks like a good suggestion.
| <Checkbox | ||
| size="small" | ||
| checked={ | ||
| filteredReleases | ||
| ? filteredReleases.length > 0 && | ||
| selectedReleases.size === filteredReleases.length | ||
| : false | ||
| } | ||
| indeterminate={ | ||
| filteredReleases | ||
| ? selectedReleases.size > 0 && selectedReleases.size < filteredReleases.length | ||
| : false | ||
| } | ||
| onChange={handleSelectAll} | ||
| /> |
| const handleConfirmDelete = useCallback(() => { | ||
| if (selectedRelease) { | ||
| deleteRelease(selectedRelease.namespace, selectedRelease.name) | ||
| .then(() => { | ||
| setIsDeleting(true); | ||
| enqueueSnackbar(`Delete request for release ${selectedRelease.name} accepted`, { | ||
| variant: 'info', | ||
| }); | ||
| setOpenDeleteAlert(false); | ||
| checkDeleteReleaseStatus(selectedRelease.name, selectedRelease.namespace); |
There was a problem hiding this comment.
I see this code above:
// Clear any existing timeout to prevent race conditions
if (deleteStatusTimeoutRef.current) {
clearTimeout(deleteStatusTimeoutRef.current);
deleteStatusTimeoutRef.current = null;
}
So marking this as resolved.
| console.error('Failed to delete release:', error); | ||
| enqueueSnackbar(`Failed to delete release ${selectedRelease.name}`, { | ||
| variant: 'error', | ||
| }); |
There was a problem hiding this comment.
Can you please check this one?
|
Hello @illume, I believe I've addressed all the review feedback. TypeScript compilation now passes successfully. Summary of changes made:Accessibility improvements:
React hooks dependency fixes:
Memory leak prevention:
Error handling:
State management refactoring:
Responsive design:
Input state synchronization:
Defensive coding:
TypeScript fixes:
Please let me know if there are any additional concerns or improvements needed. 😁 |
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for that!
I think it's in a good shape to get some more feedback from others. Can you please ask for someone else to review and test it in the headlamp Kubernetes slack channel?
btw. Can you please check the formatting? Seems the GitHub check is failing.
npm run format
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setIsBulkDeleting(false); | ||
| setOpenBulkDeleteAlert(false); | ||
| }); | ||
| }, [selectedReleases, enqueueSnackbar, checkBulkDeleteComplete]); |
There was a problem hiding this comment.
The dependency array is missing multiple state setters (setIsBulkDeleting, setOpenBulkDeleteAlert, setSelectedReleases, setRefetchCounter). While state setters are typically stable, for consistency with React best practices and ESLint's exhaustive-deps rule, consider adding them or documenting why they're omitted.
| }, [selectedReleases, enqueueSnackbar, checkBulkDeleteComplete]); | |
| }, [ | |
| selectedReleases, | |
| enqueueSnackbar, | |
| checkBulkDeleteComplete, | |
| setIsBulkDeleting, | |
| setOpenBulkDeleteAlert, | |
| setSelectedReleases, | |
| setRefetchCounter, | |
| ]); |
| if (stillExist) { | ||
| const outerTimeout = setTimeout(() => { | ||
| setRefetchCounter(prev => prev + 1); // Trigger fetch | ||
| const innerTimeout = setTimeout(() => { | ||
| checkBulkDeleteComplete(deletedKeys, retryCount + 1); | ||
| }, 500); | ||
| bulkDeleteTimeoutsRef.current.push(innerTimeout); | ||
| }, DELETE_STATUS_POLLING_INTERVAL); | ||
| bulkDeleteTimeoutsRef.current.push(outerTimeout); |
There was a problem hiding this comment.
The nested setTimeout pattern creates timeouts that are pushed to bulkDeleteTimeoutsRef but are never removed from the array after they execute. This means the array grows indefinitely during bulk delete operations. While the timeouts are cleared in the cleanup effect (line 159), the array references stale timeout IDs. Consider using a more maintainable approach, such as a single polling loop or removing executed timeouts from the array.
| if (stillExist) { | |
| const outerTimeout = setTimeout(() => { | |
| setRefetchCounter(prev => prev + 1); // Trigger fetch | |
| const innerTimeout = setTimeout(() => { | |
| checkBulkDeleteComplete(deletedKeys, retryCount + 1); | |
| }, 500); | |
| bulkDeleteTimeoutsRef.current.push(innerTimeout); | |
| }, DELETE_STATUS_POLLING_INTERVAL); | |
| bulkDeleteTimeoutsRef.current.push(outerTimeout); | |
| const registerTimeout = (callback: () => void, delay: number) => { | |
| const timeout = setTimeout(() => { | |
| try { | |
| callback(); | |
| } finally { | |
| bulkDeleteTimeoutsRef.current = bulkDeleteTimeoutsRef.current.filter( | |
| id => id !== timeout | |
| ); | |
| } | |
| }, delay); | |
| bulkDeleteTimeoutsRef.current.push(timeout); | |
| return timeout; | |
| }; | |
| if (stillExist) { | |
| registerTimeout(() => { | |
| setRefetchCounter(prev => prev + 1); // Trigger fetch | |
| registerTimeout(() => { | |
| checkBulkDeleteComplete(deletedKeys, retryCount + 1); | |
| }, 500); | |
| }, DELETE_STATUS_POLLING_INTERVAL); |
| const handleConfirmRollback = useCallback(() => { | ||
| if (selectedRelease) { | ||
| rollbackRelease(selectedRelease.namespace, selectedRelease.name, revertVersion) | ||
| .then(() => { | ||
| setRollbackPopup(false); | ||
| enqueueSnackbar(`Rollback successful for ${selectedRelease.name}`, { | ||
| variant: 'success', | ||
| }); | ||
| setRefetchCounter(prev => prev + 1); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Failed to rollback release:', error); | ||
| enqueueSnackbar(`Failed to rollback ${selectedRelease.name}`, { | ||
| variant: 'error', | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The rollback operation lacks proper error handling. If rollbackRelease fails, the error is caught and logged (line 318), but the rollback dialog remains open with no visual indication that the operation failed. The user is left in an ambiguous state. Consider either keeping the dialog open with an error message displayed inline, or closing it while showing a detailed error snackbar. The existing Detail.tsx implementation has the same issue but that doesn't make it acceptable to replicate here.
| setIsDeleting(false); | ||
| }); | ||
| } | ||
| }, [selectedRelease, enqueueSnackbar, checkDeleteReleaseStatus]); |
There was a problem hiding this comment.
The dependency array for handleConfirmDelete is missing setIsDeleting and setOpenDeleteAlert. While these are state setters and typically stable, ESLint's exhaustive-deps rule would flag this. For consistency with React best practices, either add these dependencies or use a comment to disable the rule if intentionally omitted.
| }, [selectedRelease, enqueueSnackbar, checkDeleteReleaseStatus]); | |
| }, [selectedRelease, enqueueSnackbar, checkDeleteReleaseStatus, setIsDeleting, setOpenDeleteAlert]); |
| <Checkbox | ||
| size="small" | ||
| checked={ | ||
| filteredReleases | ||
| ? filteredReleases.length > 0 && | ||
| selectedReleases.size === filteredReleases.length | ||
| : false | ||
| } | ||
| indeterminate={ | ||
| filteredReleases | ||
| ? selectedReleases.size > 0 && selectedReleases.size < filteredReleases.length | ||
| : false | ||
| } | ||
| onChange={handleSelectAll} | ||
| aria-label="Select all releases" | ||
| /> |
There was a problem hiding this comment.
The "Select All" checkbox only considers filteredReleases.length when determining the checked state (lines 523-524), but it compares against selectedReleases.size which may include releases not in the filtered list. This creates incorrect checked/indeterminate states. For example, if you select all in one filter view, then change filters, the checkbox state will be wrong. The logic should compare against the intersection of selectedReleases and filteredReleases, not the total selectedReleases.size.
| // Keep dialog open while polling - it will close on success in checkDeleteReleaseStatus | ||
| checkDeleteReleaseStatus(selectedRelease.name, selectedRelease.namespace); |
There was a problem hiding this comment.
The delete dialog behavior is inconsistent with the Detail page. In Detail.tsx (line 112), the dialog is closed immediately after initiating delete (setOpenDeleteAlert(false)), while here the dialog remains open during polling. However, looking at lines 258-259, the dialog is only closed on success. This creates a confusing UX where users cannot close the dialog during the delete operation. Consider either keeping the dialog open with a loading state throughout (as done here), or closing it immediately (as in Detail.tsx) and showing status via snackbars only. The current implementation mixes both approaches inconsistently.
Quent1L
force-pushed
the
feature/app-catalog-release-filter
branch
from
f415997 to
fe865c6
Compare
Signed-off-by: Quentin Lafond <qlafond.dev@gmail.com>
…gration and improved delete handling Signed-off-by: Quentin Lafond <qlafond.dev@gmail.com>
Signed-off-by: Quentin Lafond <qlafond.dev@gmail.com>
…se management Signed-off-by: Quentin Lafond <qlafond.dev@gmail.com>
… and enhance rollback status handling Signed-off-by: Quentin Lafond <qlafond.dev@gmail.com>
Quent1L
force-pushed
the
feature/app-catalog-release-filter
branch
from
fe865c6 to
552a8b7
Compare
|
Hi @illume ! I've rebased the pull request and I also made the following improvements:
Regarding the Slack channel — I managed to find how to join the Kubernetes Slack workspace and I've posted a message in the #headlamp channel to get some feedback and testing from the community! |
|
@Quent1L great, thanks for all of this! I went through and marked all the convos resolved that I could see were already addressed. Can you please resolve the remaining ones open if they've been addressed? (It's fine if you disagree with a comment, just mention why and mark it as resolved.) |
There was a problem hiding this comment.
@Quent1L great, thanks for all of this!
I went through and marked all the convos resolved that I could see were already addressed. Can you please resolve the remaining ones open if they've been addressed? (It's fine if you disagree with a comment, just mention why and mark it as resolved.)
I left some notes about a few things that would be good to document.
For the commits, can you please add app-catalog: and area to the parts changed. Generally it's easier to review if the commits are atomic in one area. See Git commit guidelines in contributing guide and git log for examples.
| import { Dialog } from '@kinvolk/headlamp-plugin/lib/CommonComponents'; | ||
| import { Button, DialogActions, DialogContent, DialogContentText } from '@mui/material'; | ||
|
|
||
| interface DeleteConfirmDialogProps { |
There was a problem hiding this comment.
Would you mind documenting new interfaces and new fields?
| import { ReleaseFilters } from './ReleaseFilters'; | ||
| import { RollbackDialog } from './RollbackDialog'; | ||
|
|
||
| interface ReleaseInfo { |
There was a problem hiding this comment.
Would you mind documenting new interfaces and new fields?
| } | ||
| }, [selectedRelease, enqueueSnackbar, checkDeleteReleaseStatus]); | ||
|
|
||
| const handleConfirmRollback = useCallback(() => { |
There was a problem hiding this comment.
Would you mind documenting this function?
|
|
||
| const ICON_SPACING = { marginRight: 8 }; | ||
|
|
||
| interface Release { |
There was a problem hiding this comment.
Would you mind documenting new interfaces and new fields?
| import { Autocomplete, Box, TextField } from '@mui/material'; | ||
| import { useEffect, useRef, useState } from 'react'; | ||
|
|
||
| interface ReleaseFiltersProps { |
There was a problem hiding this comment.
Would you mind documenting new interfaces and new fields?
| import { Dialog } from '@kinvolk/headlamp-plugin/lib/CommonComponents'; | ||
| import { Button, DialogActions, DialogContent, InputLabel, MenuItem, Select } from '@mui/material'; | ||
|
|
||
| interface Release { |
There was a problem hiding this comment.
Would you mind documenting new interfaces and new fields?
| @@ -0,0 +1,6 @@ | |||
| { | |||
| }); | ||
| }, [filteredReleases]); | ||
|
|
||
| const checkBulkDeleteComplete = useCallback( |
There was a problem hiding this comment.
This one needs documenting as well.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
Can you please have a look at the git commits to see if they meet the contribution guidelines? We use a Linux kernel style of git commits. See the contributing guide for general context, and please see previous git commits with git log for examples.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <description of changes>. - Keep the title and body lines under 72 characters.
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
Can you please address the open review comments?
Enhanced Release Management for App-Catalog Plugin
Overview
This PR significantly improves the user experience for managing Helm releases in the app-catalog plugin by adding filtering, bulk operations, and quick actions directly from the list view.
Key Features
Smart Filtering
Bulk Delete Operations
Promise.allfor efficient bulk operationsQuick Actions Menu
User Impact
Before: Users had to navigate to each release's detail page to perform actions, with no way to filter or bulk manage releases. The rollback dialog only showed version numbers without timestamps.
After: Users can filter hundreds of releases instantly, perform bulk deletions, access all common actions with 2 clicks instead of 4+, and easily identify deployment versions by their timestamps when rolling back.
Testing Considerations
Screenshots