Implement Topology's List/Graph switch toggle (#10018)#251
Conversation
Signed-off-by: Keith Chong <kykchong@redhat.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a shared ChangesGraph/List View Switcher Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #251 +/- ##
==========================================
- Coverage 11.92% 11.62% -0.31%
==========================================
Files 154 160 +6
Lines 6272 6445 +173
Branches 2028 2210 +182
==========================================
+ Hits 748 749 +1
+ Misses 5524 5464 -60
- Partials 0 232 +232
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gitops/components/application/ApplicationResourcesTab.tsx`:
- Around line 69-76: The Argo CD URL is being constructed before the server
settings are available, so `ArgoCDLink` and the resource actions can receive a
malformed `argoBaseURL` like `:///applications/...`. Update
`ApplicationResourcesTab` so the URL is only built and passed once `argoServer`
has valid protocol/host values, and guard the render of the link/actions until
the full URL is available; apply the same check wherever `argoBaseURL` is used
in the referenced block.
- Line 8: The import block in ApplicationResourcesTab should be reformatted to
match Prettier’s expected style so ESLint stops flagging it. Update the import
statement near the top of ApplicationResourcesTab.tsx using the existing React
Core import as the target, and run the file through Prettier so the formatting
is automatically corrected.
In `@src/gitops/components/application/ApplicationResourcesView.tsx`:
- Around line 161-164: The sync wave handling in ApplicationResourcesView is
treating valid value 0 as missing because it uses a fallback that coerces falsy
values, so wave 0 gets sorted/displayed like absent data. Update the sync-wave
branches in the comparator/rendering logic to use a nullish check instead of a
falsy fallback so 0 is preserved, and apply the same fix anywhere else the
syncWave value is normalized in this component.
- Line 18: The import block in ApplicationResourcesView has a Prettier
formatting issue that is causing ESLint to fail. Reformat the
`@patternfly/react-core` import so it matches the project’s Prettier style,
keeping the same imported symbols (EmptyState, EmptyStateBody, Flex, FlexItem,
Stack, StackItem) but adjusting the layout/spacing to the formatter’s expected
output.
In `@src/gitops/components/shared/ApplicationList.tsx`:
- Line 256: The graph view is still receiving the unsearched filteredData set,
so URL search filters can be ignored in appset graph mode. Update the
ApplicationList render path to pass the same search-filtered collection used by
rows and isEmpty, namely filteredBySearch, into the graph view via
filteredApplications. Keep the fix localized to the ApplicationList component so
the list and graph stay in sync.
- Line 114: The destructuring assignment in
ApplicationList/useGitOpsDataViewSort needs Prettier formatting to satisfy lint.
Reformat the declaration so the imported values from
useGitOpsDataViewSort(columnSortConfig) follow the project’s standard line
wrapping and spacing, keeping the same symbols searchParams, sortBy, direction,
and getSortParams intact.
In `@src/gitops/components/shared/ApplicationSetApplicationsView.tsx`:
- Around line 14-17: The import block in ApplicationSetApplicationsView should
be reformatted to match Prettier so lint passes; update the grouped import from
GitOpsViewType to use the standard single-line or wrapped style that Prettier
would generate, keeping APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY and
GitOpsViewType in the same import statement.
- Around line 125-133: The list view is overriding GitOpsDataViewTable’s
internal error handling by always passing a defined activeState from
ApplicationSetApplicationsView, which causes errors to render like a normal
table. Update the activeState prop so it is only set when the view is actually
empty, and leave it undefined otherwise so GitOpsDataViewTable can fall back to
errorState when isError is true.
In `@src/gitops/components/shared/GitOpsGraphListView.scss`:
- Around line 10-18: The shared GitOpsGraphListView stylesheet is forcing a
large minimum height on both the graph and list containers, which incorrectly
affects the list/table view rendered by ApplicationSetApplicationsView. Update
GitOpsGraphListView.scss so the fixed min-height remains only on the graph panel
selector, and remove it from the list panel selector while keeping its flex
column layout intact. Verify the list path inside
gitops-graph-list-view__list-panel no longer gets the page-sized blank area
after toggling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 62e858d1-aab1-48ac-ab31-1e10fd49d2d1
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
locales/en/plugin__gitops-plugin.jsonlocales/en/topology.jsonlocales/ja/plugin__gitops-plugin.jsonlocales/ja/topology.jsonlocales/ko/plugin__gitops-plugin.jsonlocales/ko/topology.jsonlocales/zh/plugin__gitops-plugin.jsonlocales/zh/topology.jsonsrc/gitops/components/application/ApplicationResourcesTab.tsxsrc/gitops/components/application/ApplicationResourcesToolbar.tsxsrc/gitops/components/application/ApplicationResourcesView.tsxsrc/gitops/components/application/ApplicationResourcesViewType.tssrc/gitops/components/application/graph/nodes/ApplicationNode.tsxsrc/gitops/components/shared/ApplicationList.tsxsrc/gitops/components/shared/ApplicationSetApplicationsView.tsxsrc/gitops/components/shared/GitOpsGraphListView.scsssrc/gitops/components/shared/GitOpsViewSwitcher.tsxsrc/gitops/components/shared/GitOpsViewType.ts
| import { CubesIcon } from '@patternfly/react-icons'; | ||
| import { Tbody, Td, Tr } from '@patternfly/react-table'; | ||
| import { useK8sModel, useUserSettings } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { Flex, FlexItem, PageBody, PageSection, PageSectionVariants, Title } from '@patternfly/react-core'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run Prettier on this import block.
ESLint reports this as a Prettier error, so it can block lint checks.
Proposed fix
-import { Flex, FlexItem, PageBody, PageSection, PageSectionVariants, Title } from '`@patternfly/react-core`';
+import {
+ Flex,
+ FlexItem,
+ PageBody,
+ PageSection,
+ PageSectionVariants,
+ Title,
+} from '`@patternfly/react-core`';📝 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.
| import { Flex, FlexItem, PageBody, PageSection, PageSectionVariants, Title } from '@patternfly/react-core'; | |
| import { | |
| Flex, | |
| FlexItem, | |
| PageBody, | |
| PageSection, | |
| PageSectionVariants, | |
| Title, | |
| } from '`@patternfly/react-core`'; |
🧰 Tools
🪛 ESLint
[error] 8-8: Replace ·Flex,·FlexItem,·PageBody,·PageSection,·PageSectionVariants,·Title· with ⏎··Flex,⏎··FlexItem,⏎··PageBody,⏎··PageSection,⏎··PageSectionVariants,⏎··Title,⏎
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/application/ApplicationResourcesTab.tsx` at line 8, The
import block in ApplicationResourcesTab should be reformatted to match
Prettier’s expected style so ESLint stops flagging it. Update the import
statement near the top of ApplicationResourcesTab.tsx using the existing React
Core import as the target, and run the file through Prettier so the formatting
is automatically corrected.
Source: Linters/SAST tools
| const argoBaseURL = | ||
| argoServer.protocol + | ||
| '://' + | ||
| argoServer.host + | ||
| '/applications/' + | ||
| obj?.metadata?.namespace + | ||
| '/' + | ||
| obj?.metadata?.name, | ||
| ); | ||
| '://' + | ||
| argoServer.host + | ||
| '/applications/' + | ||
| obj?.metadata?.namespace + | ||
| '/' + | ||
| obj?.metadata?.name; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t render Argo CD links/actions with an incomplete URL.
argoBaseURL is built while argoServer is still empty, so ArgoCDLink and resource actions can receive malformed URLs like :///applications/....
Proposed fix
- const argoBaseURL =
- argoServer.protocol +
- '://' +
- argoServer.host +
- '/applications/' +
- obj?.metadata?.namespace +
- '/' +
- obj?.metadata?.name;
+ const applicationNamespace = obj?.metadata?.namespace;
+ const applicationName = obj?.metadata?.name;
+ const argoBaseURL =
+ argoServer.protocol && argoServer.host && applicationNamespace && applicationName
+ ? `${argoServer.protocol}://${argoServer.host}/applications/${applicationNamespace}/${applicationName}`
+ : undefined;- <ArgoCDLink href={argoBaseURL} />
+ {argoBaseURL && <ArgoCDLink href={argoBaseURL} />}- {obj?.metadata && (
+ {obj?.metadata && argoBaseURL && (
<ApplicationResourcesViewAlso applies to: 100-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/application/ApplicationResourcesTab.tsx` around lines
69 - 76, The Argo CD URL is being constructed before the server settings are
available, so `ArgoCDLink` and the resource actions can receive a malformed
`argoBaseURL` like `:///applications/...`. Update `ApplicationResourcesTab` so
the URL is only built and passed once `argoServer` has valid protocol/host
values, and guard the render of the link/actions until the full URL is
available; apply the same check wherever `argoBaseURL` is used in the referenced
block.
| RowFilterItem, | ||
| useListPageFilter, | ||
| } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { EmptyState, EmptyStateBody, Flex, FlexItem, Stack, StackItem } from '@patternfly/react-core'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run Prettier on this import block.
ESLint reports this as a Prettier error, so it can block lint checks.
Proposed fix
-import { EmptyState, EmptyStateBody, Flex, FlexItem, Stack, StackItem } from '`@patternfly/react-core`';
+import {
+ EmptyState,
+ EmptyStateBody,
+ Flex,
+ FlexItem,
+ Stack,
+ StackItem,
+} from '`@patternfly/react-core`';📝 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.
| import { EmptyState, EmptyStateBody, Flex, FlexItem, Stack, StackItem } from '@patternfly/react-core'; | |
| import { | |
| EmptyState, | |
| EmptyStateBody, | |
| Flex, | |
| FlexItem, | |
| Stack, | |
| StackItem, | |
| } from '`@patternfly/react-core`'; |
🧰 Tools
🪛 ESLint
[error] 18-18: Replace ·EmptyState,·EmptyStateBody,·Flex,·FlexItem,·Stack,·StackItem· with ⏎··EmptyState,⏎··EmptyStateBody,⏎··Flex,⏎··FlexItem,⏎··Stack,⏎··StackItem,⏎
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/application/ApplicationResourcesView.tsx` at line 18,
The import block in ApplicationResourcesView has a Prettier formatting issue
that is causing ESLint to fail. Reformat the `@patternfly/react-core` import so it
matches the project’s Prettier style, keeping the same imported symbols
(EmptyState, EmptyStateBody, Flex, FlexItem, Stack, StackItem) but adjusting the
layout/spacing to the formatter’s expected output.
Source: Linters/SAST tools
| case 'sync-wave': | ||
| aValue = a.syncWave || ''; | ||
| bValue = b.syncWave || ''; | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve valid sync wave 0.
syncWave is an optional number, and || treats 0 as missing, so wave 0 sorts/displays as -.
Proposed fix
case 'sync-wave':
- aValue = a.syncWave || '';
- bValue = b.syncWave || '';
+ aValue =
+ a.syncWave ?? (direction === 'asc' ? Number.POSITIVE_INFINITY : Number.NEGATIVE_INFINITY);
+ bValue =
+ b.syncWave ?? (direction === 'asc' ? Number.POSITIVE_INFINITY : Number.NEGATIVE_INFINITY);
break;- cell: <>{resource.syncWave || '-'}</>,
+ cell: <>{resource.syncWave ?? '-'}</>,Also applies to: 272-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/application/ApplicationResourcesView.tsx` around lines
161 - 164, The sync wave handling in ApplicationResourcesView is treating valid
value 0 as missing because it uses a fallback that coerces falsy values, so wave
0 gets sorted/displayed like absent data. Update the sync-wave branches in the
comparator/rendering logic to use a nullish check instead of a falsy fallback so
0 is preserved, and apply the same fix anywhere else the syncWave value is
normalized in this component.
|
|
||
| const { searchParams, sortBy, direction, getSortParams } = | ||
| useGitOpsDataViewSort(columnSortConfig); | ||
| const { searchParams, sortBy, direction, getSortParams } = useGitOpsDataViewSort(columnSortConfig); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply Prettier formatting to unblock lint.
Proposed fix
- const { searchParams, sortBy, direction, getSortParams } = useGitOpsDataViewSort(columnSortConfig);
+ const { searchParams, sortBy, direction, getSortParams } = useGitOpsDataViewSort(
+ columnSortConfig,
+ );📝 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 { searchParams, sortBy, direction, getSortParams } = useGitOpsDataViewSort(columnSortConfig); | |
| const { searchParams, sortBy, direction, getSortParams } = useGitOpsDataViewSort( | |
| columnSortConfig, | |
| ); |
🧰 Tools
🪛 ESLint
[error] 114-114: Insert ⏎···
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/shared/ApplicationList.tsx` at line 114, The
destructuring assignment in ApplicationList/useGitOpsDataViewSort needs Prettier
formatting to satisfy lint. Reformat the declaration so the imported values from
useGitOpsDataViewSort(columnSortConfig) follow the project’s standard line
wrapping and spacing, keeping the same symbols searchParams, sortBy, direction,
and getSortParams intact.
Source: Linters/SAST tools
| <ApplicationSetApplicationsView | ||
| applicationSet={appset as ApplicationSetKind} | ||
| ownedApps={ownedApps as ApplicationKind[]} | ||
| filteredApplications={filteredData as ApplicationKind[]} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the search-filtered applications to the graph view.
Line 256 sends filteredData, while rows and isEmpty use filteredBySearch. In appset graph mode, URL search filters can be ignored and show applications that the list view hides.
Proposed fix
- filteredApplications={filteredData as ApplicationKind[]}
+ filteredApplications={filteredBySearch as ApplicationKind[]}📝 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.
| filteredApplications={filteredData as ApplicationKind[]} | |
| filteredApplications={filteredBySearch as ApplicationKind[]} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/shared/ApplicationList.tsx` at line 256, The graph view
is still receiving the unsearched filteredData set, so URL search filters can be
ignored in appset graph mode. Update the ApplicationList render path to pass the
same search-filtered collection used by rows and isEmpty, namely
filteredBySearch, into the graph view via filteredApplications. Keep the fix
localized to the ApplicationList component so the list and graph stay in sync.
| import { | ||
| APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY, | ||
| GitOpsViewType, | ||
| } from './GitOpsViewType'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply Prettier formatting to unblock lint.
Proposed fix
-import {
- APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY,
- GitOpsViewType,
-} from './GitOpsViewType';
+import { APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY, GitOpsViewType } from './GitOpsViewType';📝 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.
| import { | |
| APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY, | |
| GitOpsViewType, | |
| } from './GitOpsViewType'; | |
| import { APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY, GitOpsViewType } from './GitOpsViewType'; |
🧰 Tools
🪛 ESLint
[error] 14-17: Replace ⏎··APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY,⏎··GitOpsViewType,⏎ with ·APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY,·GitOpsViewType·
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/shared/ApplicationSetApplicationsView.tsx` around lines
14 - 17, The import block in ApplicationSetApplicationsView should be
reformatted to match Prettier so lint passes; update the grouped import from
GitOpsViewType to use the standard single-line or wrapped style that Prettier
would generate, keeping APPLICATION_SET_APPLICATIONS_VIEW_SETTING_KEY and
GitOpsViewType in the same import statement.
Source: Linters/SAST tools
| <GitOpsDataViewTable | ||
| columns={columns} | ||
| rows={rows} | ||
| isEmpty={isEmpty} | ||
| emptyState={emptyState} | ||
| errorState={errorState} | ||
| isError={isError} | ||
| activeState={isEmpty ? DataViewState.empty : null} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the error state in list view.
Line 132 always passes a defined activeState; null overrides GitOpsDataViewTable’s built-in error resolution, so load errors render as a normal table instead of errorState.
Proposed fix
- activeState={isEmpty ? DataViewState.empty : null}
+ activeState={isError ? DataViewState.error : isEmpty ? DataViewState.empty : 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.
| <GitOpsDataViewTable | |
| columns={columns} | |
| rows={rows} | |
| isEmpty={isEmpty} | |
| emptyState={emptyState} | |
| errorState={errorState} | |
| isError={isError} | |
| activeState={isEmpty ? DataViewState.empty : null} | |
| /> | |
| <GitOpsDataViewTable | |
| columns={columns} | |
| rows={rows} | |
| isEmpty={isEmpty} | |
| emptyState={emptyState} | |
| errorState={errorState} | |
| isError={isError} | |
| activeState={isError ? DataViewState.error : isEmpty ? DataViewState.empty : null} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/shared/ApplicationSetApplicationsView.tsx` around lines
125 - 133, The list view is overriding GitOpsDataViewTable’s internal error
handling by always passing a defined activeState from
ApplicationSetApplicationsView, which causes errors to render like a normal
table. Update the activeState prop so it is only set when the view is actually
empty, and leave it undefined otherwise so GitOpsDataViewTable can fall back to
errorState when isError is true.
| &__panel, | ||
| &__list-panel { | ||
| min-height: 1000px; | ||
| } | ||
|
|
||
| &__list-panel { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't force the graph height onto the list panel.
src/gitops/components/shared/ApplicationSetApplicationsView.tsx:91-102 renders the table path inside gitops-graph-list-view__list-panel. Giving that container min-height: 1000px makes short or empty list views render with a page-sized blank region after every toggle. Keep the fixed height on the graph panel only.
Suggested fix
- &__panel,
- &__list-panel {
+ &__panel {
min-height: 1000px;
}
&__list-panel {
+ min-height: 0;
display: flex;
flex-direction: column;
}As per path instructions, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| &__panel, | |
| &__list-panel { | |
| min-height: 1000px; | |
| } | |
| &__list-panel { | |
| display: flex; | |
| flex-direction: column; | |
| } | |
| &__panel { | |
| min-height: 1000px; | |
| } | |
| &__list-panel { | |
| min-height: 0; | |
| display: flex; | |
| flex-direction: column; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitops/components/shared/GitOpsGraphListView.scss` around lines 10 - 18,
The shared GitOpsGraphListView stylesheet is forcing a large minimum height on
both the graph and list containers, which incorrectly affects the list/table
view rendered by ApplicationSetApplicationsView. Update GitOpsGraphListView.scss
so the fixed min-height remains only on the graph panel selector, and remove it
from the list panel selector while keeping its flex column layout intact. Verify
the list path inside gitops-graph-list-view__list-panel no longer gets the
page-sized blank area after toggling.
Source: Path instructions
See GITOPS-10018 for details