Skip to content

Comments

Refactor HelmAppList to use standardized Table component#3057

Open
Copilot wants to merge 5 commits intodevelopfrom
copilot/replace-custom-table-with-standard-table
Open

Refactor HelmAppList to use standardized Table component#3057
Copilot wants to merge 5 commits intodevelopfrom
copilot/replace-custom-table-with-standard-table

Conversation

Copy link

Copilot AI commented Feb 11, 2026

Description

Replaces custom table implementation in HelmAppList.tsx with standardized Table component from @devtron-labs/devtron-fe-common-lib, following the pattern established in GenericAppList (PR #3042).

Changes

New files:

  • HelmAppListCellComponents.tsx - Custom cell renderers for app name (with icon + chart name), status, environment, and last deployed timestamp
  • HelmAppListViewWrapper.tsx - Wrapper for cluster selection messages, error strips, and guided content cards

Modified files:

  • HelmAppList.tsx - Removed ~200 lines of manual table rendering (headers, rows, pagination, shimmer loading). Replaced with <Table> component, client-side filter function, and row click handler. Dynamically disables column sorting during SSE connection.
  • list.utils.ts - Added getHelmAppListColumns() with sortable columns (APP_NAME, LAST_DEPLOYED) and conditional AppStatus column
  • AppListType.ts - Added HelmAppListRowType and HelmAppListAdditionalProps types
  • styles.scss - Added styles for #table-wrapper-table__helm-app-list

Preserved functionality:

  • SSE connection management for external helm apps
  • Filtering (searchKey, project, environment, namespace) and sorting
  • All empty states (select cluster, clear filters, connect cluster, AllCheckModal)
  • Permission message strips and guided content cards
// Before: Manual table rendering
renderHeaders() { ... }
renderHelmAppLink() { ... }
{filteredHelmAppList.map(app => renderHelmAppLink(app))}
<Pagination ... />

// After: Standardized Table component
<Table
  columns={columns}
  rows={rows}
  filter={filter}
  onRowClick={onRowClick}
  ViewWrapper={HelmAppListViewWrapper}
/>

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • TypeScript compilation successful (no errors)
  • Code review passed
  • CodeQL security scan passed (0 alerts)
  • Manual UI testing (pending)

Checklist:

  • The title of the PR states what changed and the related issues number (used for the release note).
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
Original prompt

Problem

The file src/components/app/list-new/HelmAppList.tsx uses a custom table implementation with manual header rendering, sorting, pagination, sticky headers, shimmer loading, and row rendering. This should be replaced with the standardized Table component from @devtron-labs/devtron-fe-common-lib.

Current Implementation

The current HelmAppList.tsx at commit 8eb63524e15159c952dfec199bdb5c48729f3555 has:

  • Manual renderHeaders() with SortableTableHeaderCell, useStickyEvent, getClassNameForStickyHeaderWithShadow
  • Manual renderHelmAppLink() rendering each row as a <Link> with inline layout
  • Manual useMemo for filtering by searchKey, project, environment, namespace and sorting by AppListSortableKeys.APP_NAME or AppListSortableKeys.LAST_DEPLOYED
  • Manual Pagination component usage
  • Manual shimmer/loading state rendering with appListLoadingArray
  • Manual empty state handling with multiple conditions (askToSelectClusterId, askToClearFiltersWithSelectClusterTip, askToConnectAClusterForNoResult, renderAllCheckModal)
  • Uses handleSorting, changePage, changePageSize from parent filterConfig

Requirements

Replace the custom table with the Table component following the patterns established in these reference PRs:

Specific Changes Needed

  1. Define table columns in a utility file or constants file (can be in list.utils.ts or a new file). Columns should include:

    • App Name (with chart icon + app name + chart name) — sortable, uses AppListSortableKeys.APP_NAME, needs a custom CellComponent that renders the app icon via LazyImage, the app name, and the chart name below it
    • App Status (conditionally shown when isArgoInstalled is true) — uses AppStatus component
    • Environment — with info tooltip (ENVIRONMENT_HEADER_TIPPY_CONTENT), shows environmentName or clusterName__namespace
    • Cluster — shows clusterName
    • Namespace — shows namespace
    • Last Deployed At — sortable, uses AppListSortableKeys.LAST_DEPLOYED, needs a custom CellComponent that shows handleUTCTime with a Tippy tooltip showing the full date
  2. Transform HelmApp data into table rows using useMemo:

    rows = [...devtronInstalledHelmAppsList, ...externalHelmAppsList].map(app => ({
        id: app.appId,
        data: {
            detail: app, // keep original app data for CellComponents
            [AppListSortableKeys.APP_NAME]: app.appName,
            // ... other fields mapped to column field keys
        }
    }))
  3. Create CellComponents in a new file (e.g., HelmAppListCellComponents.tsx):

    • HelmAppNameCellComponent — renders LazyImage icon + app name + chart name
    • HelmAppStatusCellComponent — renders <AppStatus>
    • HelmAppEnvironmentCellComponent — renders environment name or cluster__namespace fallback
    • HelmAppLastDeployedCellComponent — renders handleUTCTime with Tippy tooltip
  4. Create a filter function (memoized with useCallback):

    • Filter by searchKey (app name or chart name includes search)
    • Filter by project (matching projectId)
    • Filter by environment (matching environmentId)
    • Filter by namespace (matching clusterId_namespace)
  5. Handle the onRowClick callback to navigate to the app detail URL (using _buildAppDetailUrl logic). Wrap with useCallback.

  6. Use FiltersTypeEnum.URL since the parent already manages URL-based filters, but NOTE: The Table component will manage its own URL filters internally. The parent component passes filterConfig which includes sortBy, sortOrder, offset, pageSize etc. Since HelmAppList uses FE-only data (the rows prop, not getRows), use rows prop with filter function. The Table component will handle sorting via column comparator functions and pagination internally.

  7. Handle conditional rendering outside the Table:

    • The various empty states (askToSelectClusterId, renderHelmPermissionMessageStrip, etc.) should be rendered OUTSIDE the Table when appropriate
    • The clusterIdsCsv check, SSE connection logic, and data fetching should remain as-is
    • The info strip about selecting clusters and the error strips should be rendered above/around the Table
    • The guided content cards (showGuidedContentCards) can be handled via a ViewWrapper component
  8. Create a ViewWrapper component that renders:

    • The cluster select message strip (when !clusterIdsCsv)
    • The external helm list fetch error messages
    • The guided content cards (when showGuidedContentCards)
    • The {children} (table content)
  9. Add styles in a new styles.scss file with selector ...

This pull request was created from Copilot chat.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 4 commits February 11, 2026 05:09
Co-authored-by: Elessar1802 <66767648+Elessar1802@users.noreply.github.com>
Co-authored-by: Elessar1802 <66767648+Elessar1802@users.noreply.github.com>
Co-authored-by: Elessar1802 <66767648+Elessar1802@users.noreply.github.com>
Co-authored-by: Elessar1802 <66767648+Elessar1802@users.noreply.github.com>
@sonarqubecloud
Copy link

Copilot AI changed the title [WIP] Replace custom table implementation with standardized Table component Refactor HelmAppList to use standardized Table component Feb 11, 2026
Copilot AI requested a review from Elessar1802 February 11, 2026 05:18
@Elessar1802 Elessar1802 marked this pull request as ready for review February 12, 2026 06:47
Copilot AI review requested due to automatic review settings February 12, 2026 06:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the Helm app listing to use the standardized Table component from @devtron-labs/devtron-fe-common-lib, aligning HelmAppList with the approach used by GenericAppList and consolidating table rendering concerns (columns, cells, empty states, wrappers).

Changes:

  • Replaced manual Helm table rendering in HelmAppList.tsx with <Table> using column definitions + row transformation + row-click navigation.
  • Added Helm-specific CellComponents and a ViewWrapper to keep custom UI (message strips, error strips, guided cards) around the Table.
  • Introduced Helm table column factory (getHelmAppListColumns) and new row/additional-props types.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/components/app/list-new/HelmAppList.tsx Switches Helm list rendering to standardized Table, adds row mapping, filter, onRowClick, wrapper usage, and empty-state handling.
src/components/app/list-new/HelmAppListCellComponents.tsx Adds custom cell renderers for name/icon, status, environment, and last deployed timestamp.
src/components/app/list-new/HelmAppListViewWrapper.tsx Wraps the Table view with cluster-select note, fetch error strips, and guided content cards.
src/components/app/list-new/list.utils.ts Adds getHelmAppListColumns() to define Helm columns (including sortable ones and conditional status column).
src/components/app/list-new/AppListType.ts Adds HelmAppListRowType and HelmAppListAdditionalProps types for the new Table integration.
src/components/app/list-new/styles.scss Adds table-specific styling for table__helm-app-list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +421 to +422
title: APP_LIST_EMPTY_STATE_MESSAGING.noHelmChartsFound,
subTitle: APP_LIST_EMPTY_STATE_MESSAGING.connectClusterInfoText,
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getNoRowsForFilterConfig returns the "No helm charts found / Connect a cluster" messaging for the general filtered-empty case. Previously (and in GenericAppList) filtered-empty states use the "No apps found" messaging with a clear-filters CTA; the current config can mislead users into thinking they must connect a cluster when the issue is restrictive filters/search. Consider returning APP_LIST_EMPTY_STATE_MESSAGING.noAppsFound/noAppsFoundInfoText (and relying on clearFilters) for non-"all clusters" filter cases.

Suggested change
title: APP_LIST_EMPTY_STATE_MESSAGING.noHelmChartsFound,
subTitle: APP_LIST_EMPTY_STATE_MESSAGING.connectClusterInfoText,
title: APP_LIST_EMPTY_STATE_MESSAGING.noAppsFound,
subTitle: APP_LIST_EMPTY_STATE_MESSAGING.noAppsFoundInfoText,

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +132
if (project.length) {
const projectMap = new Map<string, true>(project.map((projectId) => [projectId, true]))
isMatch = isMatch && (projectMap.get(String(app.detail.projectId)) ?? false)
}
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table filter callback creates new Maps (e.g., for project) during each row evaluation. Since the filter runs once per row, this can add significant overhead on large lists. Consider precomputing Set/Map lookups via useMemo and using them inside the callback.

Copilot uses AI. Check for mistakes.
Comment on lines +471 to +473
filtersVariant={FiltersTypeEnum.URL}
paginationVariant={PaginationEnum.PAGINATED}
emptyStateConfig={{
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the switch to <Table>, pagination is always enabled even while external apps are still streaming via SSE. Previously pagination was disabled while fetchingExternalApps was true to avoid confusing empty pages/jumping results during data population. Consider disabling pagination (or forcing a loading state) while sseConnection/fetchingExternalApps is active to preserve the earlier UX.

Copilot uses AI. Check for mistakes.
* limitations under the License.
*/

import { ComponentSizeType, DocLink, FiltersTypeEnum, TableViewWrapperProps, URLS as CommonURLS } from '@devtron-labs/devtron-fe-common-lib'
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URLS as CommonURLS is imported from @devtron-labs/devtron-fe-common-lib but never used in this file. Consider removing the unused import to keep the module clean and avoid potential lint/noUnusedLocals failures.

Suggested change
import { ComponentSizeType, DocLink, FiltersTypeEnum, TableViewWrapperProps, URLS as CommonURLS } from '@devtron-labs/devtron-fe-common-lib'
import { ComponentSizeType, DocLink, FiltersTypeEnum, TableViewWrapperProps } from '@devtron-labs/devtron-fe-common-lib'

Copilot uses AI. Check for mistakes.
Comment on lines +58 to +62
<p className="dc__truncate-text m-0" data-testid={`${app.environmentDetail.environmentName}-environment`}>
{app.environmentDetail.environmentName
? app.environmentDetail.environmentName
: `${app.environmentDetail.clusterName}__${app.environmentDetail.namespace}`}
</p>
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data-testid here is based on environmentName, which can be empty (the UI falls back to cluster__namespace). This can produce unstable/empty test ids (e.g., "-environment"). Consider using a stable identifier (like environmentId when present, or clusterName__namespace) for data-testid.

Copilot uses AI. Check for mistakes.
import { ReactComponent as CloseIcon } from '../../../assets/icons/ic-close.svg'
import { ReactComponent as AlertTriangleIcon } from '../../../assets/icons/ic-alert-triangle.svg'
import { ReactComponent as ArrowRight } from '../../../assets/icons/ic-arrow-right.svg'
import noChartInClusterImage from '../../../assets/img/ic-no-chart-in-clusters@2x.png'
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noChartInClusterImage is imported but no longer used after switching to the Table empty-state configs (the image is now referenced via imgName). Consider removing this unused import to avoid dead code and potential lint/noUnusedLocals failures.

Suggested change
import noChartInClusterImage from '../../../assets/img/ic-no-chart-in-clusters@2x.png'

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +126
isMatch &&
(app.detail.appName.toLowerCase().includes(searchLowerCase) ||
app.detail.chartName.toLowerCase().includes(searchLowerCase))
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use of variable 'isMatch' always evaluates to true.

Suggested change
isMatch &&
(app.detail.appName.toLowerCase().includes(searchLowerCase) ||
app.detail.chartName.toLowerCase().includes(searchLowerCase))
app.detail.appName.toLowerCase().includes(searchLowerCase) ||
app.detail.chartName.toLowerCase().includes(searchLowerCase)

Copilot uses AI. Check for mistakes.

// Filter function for the table
const filter = useCallback(
({ data: app }) => {
Copy link
Member

@AbhishekA1509 AbhishekA1509 Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is type of app available?

[searchKey, project, environment, namespace],
)

const onRowClick = useCallback(({ data: helmApp }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add type for param

)

const onRowClick = useCallback(({ data: helmApp }) => {
const buildAppDetailUrl = (app: HelmApp) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this to util this is a pure method

}

filteredHelmAppList = filteredHelmAppList.slice(offset, offset + pageSize)
function _isAnyFilterationAppliedExceptClusterAndNs() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be arrow methods

const _removeExternalAppFetchError = (index: number) => {
const _externalHelmListFetchErrors = [...externalHelmListFetchErrors]
_externalHelmListFetchErrors.splice(index, 1)
setExternalHelmListFetchErrors(_externalHelmListFetchErrors)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can directly use callback from setState and filter method

}, [push])

const filteredListTotalSize = filteredHelmAppList.length
const _removeExternalAppFetchError = (index: number) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these methods are starting with _?


filteredHelmAppList = filteredHelmAppList.slice(offset, offset + pageSize)
function _isAnyFilterationAppliedExceptClusterAndNs() {
return project.length || searchKey.length || environment.length || appStatus.length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls make it boolean if return type is intended as boolean, same for below method

* Returns null to indicate that the empty state should be handled externally (outside the Table component).
* The external handling is done in the conditional rendering below.
*/
const getNoRowsConfig = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is returning null every time

return (
<>
<div className="app-list__cell--icon">
<LazyImage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a component called ImageWithFallback can use that

}

return (
<Tippy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use Tooltip

</div>
<div className="app-list__cell app-list__cell--name flex column left">
<div className="dc__truncate-text m-0 value cb-5">{app.appName}</div>
<div className="dc__truncate-text fs-12 m-0">{app.chartName}</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if tooltip is required in any case, pls check all cell components

<ErrorExclamationIcon className="icon-dim-20" />
</span>
<span>{externalHelmListFetchError}</span>
<CloseIcon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Button and move onClick to a handler

<div className="h-8" />
<div className="cluster-select-message-strip above-header-message flex left">
<span className="mr-8 flex">
<InfoFillPurple className="icon-dim-20" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must be a Icon name present for this, check across this fill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants