-
Notifications
You must be signed in to change notification settings - Fork 57
feat: use Table in DevtronAppList #3034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Some linked issues are invalid. Please update the issue links:\nIssue # in is not found or invalid (HTTP }404).\n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the DevtronAppList component to use a generic Table component from the common library instead of a custom implementation. The change simplifies the codebase by removing custom expand/collapse logic and leveraging the Table component's built-in features for expandable rows, pagination, and filtering.
- Migrates from custom list rendering to using the generic Table component with built-in pagination and expandable row support
- Removes custom expanded row component and related state management (expandedRow, isAllExpanded, isAllExpandable)
- Refactors cell rendering into modular CellComponent functions that handle both regular and expanded rows
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/app/list/types.tsx | Removes DevtronAppExpandedState interface and adds TableAdditionalPropsType for Table component props |
| src/components/app/list/list.scss | Removes custom cell styling classes and adds generic-table row padding classes |
| src/components/app/list/expandedRow/types.tsx | Deletes file - no longer needed with Table component |
| src/components/app/list/expandedRow/expandedRow.css | Deletes file - styling now handled by Table component |
| src/components/app/list/expandedRow/ExpandedRow.tsx | Deletes file - expanded row logic integrated into Table component |
| src/components/app/list/constants.tsx | Deletes file - INITIAL_EXPANDED_STATE no longer needed |
| src/components/app/list/DevtronAppListContainer.tsx | Complete refactor to use Table component with cell components for rendering, removes manual expand/collapse logic and pagination handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| status={isExpandedRow ? env.status : app.defaultEnv.appStatus} | ||
| isVirtualEnv={isExpandedRow ? env.isVirtualEnvironment : app.defaultEnv.isVirtualEnvironment} |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent null safety handling for app.defaultEnv. Line 194 and 195 access app.defaultEnv.appStatus and app.defaultEnv.isVirtualEnvironment without null checking, but the defaultEnv type is nullable (Environment | null). This could cause a runtime error if defaultEnv is null. Consider using optional chaining like the code does on lines 202, 214 (app.defaultEnv?.clusterName, app.defaultEnv?.namespace).
| }) | ||
| } | ||
| if (field === AppListSortableKeys.LAST_DEPLOYED) { | ||
| const lastDeployedTime = isExpandedRow ? env.lastDeployedTime : app.defaultEnv.lastDeployedTime |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference error. The code accesses app.defaultEnv.lastDeployedTime without null checking, but defaultEnv is nullable (Environment | null according to the types). This could cause a runtime error if defaultEnv is null. Consider using optional chaining: app.defaultEnv?.lastDeployedTime
| const lastDeployedTime = isExpandedRow ? env.lastDeployedTime : app.defaultEnv.lastDeployedTime | |
| const lastDeployedTime = isExpandedRow ? env.lastDeployedTime : app.defaultEnv?.lastDeployedTime |
| totalRows: totalCount, | ||
| } | ||
| }, | ||
| [syncListData, JSON.stringify(filterConfig)], |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency array uses JSON.stringify(filterConfig) which can be inefficient and may cause unnecessary re-renders. JSON.stringify is not recommended for dependency arrays because it creates a new string on every render even if the object content hasn't changed. Consider using individual dependencies from filterConfig (like searchKey, offset, pageSize, appStatus, etc.) or using a more efficient deep comparison approach.
| [syncListData, JSON.stringify(filterConfig)], | |
| [syncListData, filterConfig], |
| data: app, | ||
| id: String(app.id), | ||
| expandableRows: | ||
| app.defaultEnv.name && app.environments.length |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference error. Line 408 accesses app.defaultEnv.name without null checking. Since defaultEnv is nullable (Environment | null), this could cause a runtime error if defaultEnv is null. Consider using optional chaining: app.defaultEnv?.name
| app.defaultEnv.name && app.environments.length | |
| app.defaultEnv?.name && app.environments.length |
| const namespace = isExpandedRow ? env.namespace : app.defaultEnv?.namespace ?? '' | ||
| return ( | ||
| <div className="flex left"> | ||
| <p data-testid={`${namespace}-namespace`} className="dc__truncate-text m-0"> |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace detected. There's double spacing between 'dc__truncate-text' and 'm-0' in the className. This should be a single space for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
| placement="top" | ||
| content={moment(lastDeployedTime).format(DATE_TIME_FORMATS.TWELVE_HOURS_FORMAT)} | ||
| > | ||
| <p className="dc__truncate-text m-0" data-testid="last-deployed-time"> |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace detected. There's double spacing between 'dc__truncate-text' and 'm-0' in the className. This should be a single space for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
| const isEnvConfigured = app.defaultEnv.name | ||
|
|
||
| const isSearchOrFilterApplied = | ||
| searchKey || appStatus.length || project.length || environment.length || namespace.length || cluster.length | ||
| if (!envCount || isRowInExpandState) { | ||
| return null | ||
| } | ||
|
|
||
| const abortControllerRef = useRef<AbortController>(new AbortController()) | ||
| const [appListResponseLoading, appListResponse, appListError, appListReload] = useAsync( | ||
| () => | ||
| abortPreviousRequests( | ||
| () => | ||
| getAppList(getDevtronAppListPayload(filterConfig, environmentList, namespaceList), { | ||
| signal: abortControllerRef.current.signal, | ||
| }), | ||
| abortControllerRef, | ||
| ), | ||
| [filterConfig, syncListData], | ||
| !appFiltersResponseLoading, // We need to wait until environment filters are created from cluster and namespace | ||
| return ( | ||
| <div className="flex left dc__gap-6"> | ||
| <span | ||
| data-testid={`${app.defaultEnv.name}-environment`} | ||
| className={`dc__truncate cn-9 ${isEnvConfigured ? '' : 'cn5'}`} | ||
| > | ||
| {isEnvConfigured ? app.defaultEnv.name : 'Not configured'} |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference error. Lines 75, 84, and 87 access app.defaultEnv.name without null checking, but defaultEnv is nullable (Environment | null according to the types). This could cause a runtime error if defaultEnv is null. Consider using optional chaining: app.defaultEnv?.name
| const { name } = app | ||
|
|
||
| const onClick = () => { | ||
| push(redirectToAppDetails(app, app.defaultEnv.id)) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference error. Line 144 accesses app.defaultEnv.id without null checking. Since defaultEnv is nullable (Environment | null), this could cause a runtime error if defaultEnv is null. Consider using optional chaining and a fallback value: app.defaultEnv?.id ?? 0
| push(redirectToAppDetails(app, app.defaultEnv.id)) | |
| push(redirectToAppDetails(app, app.defaultEnv?.id ?? 0)) |
| return () => { | ||
| signals.removeEventListener(TableSignalEnum.ENTER_PRESSED, handleEnterPressed) | ||
| } | ||
| }, []) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing dependencies in useEffect. The effect uses 'id' and 'signals' but the dependency array is empty. This could lead to stale closures where the effect references outdated values. Consider adding 'id' and 'signals' to the dependency array.
| }, []) | |
| }, [id, signals]) |
|
|
||
| return ( | ||
| <div className="flex left"> | ||
| <p data-testid={`${clusterName}-cluster`} className="dc__truncate-text m-0"> |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace detected. There's double spacing between 'dc__truncate-text' and 'm-0' in the className. This should be a single space for consistency.
|
Some linked issues are invalid. Please update the issue links:\nIssue # in is not found or invalid (HTTP }404).\n |
|
Some linked issues are invalid. Please update the issue links:\nIssue # in is not found or invalid (HTTP }404).\n |
| }) | ||
|
|
||
| const [expandedState, setExpandedState] = useState<DevtronAppExpandedState>(INITIAL_EXPANDED_STATE) | ||
| const EnvironmentCellComponent = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls move into separate files CellComponents
| } | ||
| const columns = useMemo( | ||
| () => [ | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to utils
| CellComponent, | ||
| }, | ||
| { | ||
| field: AppListSortableKeys.LAST_DEPLOYED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be conditional? Pls verify with deleted ExpandedRow.tsx
| {renderPagination()} | ||
| </> | ||
| <Table<App, FiltersTypeEnum.URL, TableAdditionalPropsType> | ||
| key={JSON.stringify({ syncListData, filterConfig })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be this big key?
| searchKey || appStatus.length || project.length || environment.length || namespace.length || cluster.length | ||
|
|
||
| const redirectToAppDetails = (app, envId: number): string => { | ||
| setCurrentAppName(app.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this since its set in AppHeader, can you confirm? since otherwise should add this in HoverComponent redirection as well?
| event.stopPropagation() | ||
| event.preventDefault() | ||
| expandRow(event.currentTarget.dataset.key) | ||
| const app = data as App |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix this by taking additional type param and based on that and isExpandedRow if its true type for row should toggle
| return ( | ||
| <div className="flex left"> | ||
| {lastDeployedTime && ( | ||
| <Tippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Tooltip with alwaysShowOnHover
| setExpandedState((prevState) => ({ ...prevState, expandedRow: { ...prevState.expandedRow, [id]: true } })) | ||
| return ( | ||
| <div className="flex left dc__gap-4 dc__visible-hover dc__visible-hover--parent"> | ||
| <Tooltip content={name}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be on span?
|
Some linked issues are invalid. Please update the issue links:\nIssue # in is not found or invalid (HTTP }404).\n |
|
Some linked issues are invalid. Please update the issue links:\nIssue # in is not found or invalid (HTTP }404).\n |
|



No description provided.