Implement Topology's List/Graph switch toggle (#10018)#251
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds a shared ChangesGraph/List View Switcher Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Tab as ApplicationResourcesTab
participant View as ApplicationResourcesView
participant Filter as useListPageFilter
participant Table as GitOpsDataViewTable
participant Graph as ApplicationGraphView
participant Switcher as GitOpsViewSwitcher
Tab->>Switcher: viewType, onViewChange
Switcher-->>Tab: toggled viewType
Tab->>View: resources, viewType, argoBaseURL
View->>Filter: filters(resources)
Filter-->>View: filteredResources
alt viewType = list
View->>Table: columns, rows(filteredResources)
else viewType = graph
View->>Graph: filteredResources
end
🚥 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.65% -0.28%
==========================================
Files 154 160 +6
Lines 6272 6427 +155
Branches 2028 2143 +115
==========================================
+ Hits 748 749 +1
- Misses 5524 5677 +153
- Partials 0 1 +1
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
| 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.
| 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.
| <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.
d422cc6 to
6fff12f
Compare
Signed-off-by: Keith Chong <kykchong@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/gitops/components/application/ApplicationResourcesTab.tsx (1)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate persisted-view-type logic.
This
useUserSettings+ local state + syncuseEffect+onViewChangepattern is duplicated almost line-for-line inApplicationSetApplicationsView.tsx(lines 58-77). Extracting a shared hook (e.g.usePersistedViewType(settingKey, defaultView)) would avoid future divergence between the two implementations.♻️ Example shared hook
function usePersistedViewType<T>(settingKey: string, defaultView: T) { const [savedViewType, setSavedViewType, loaded] = useUserSettings<T>(settingKey, defaultView, false); const [viewType, setViewType] = React.useState<T>(defaultView); React.useEffect(() => { if (loaded) setViewType(savedViewType ?? defaultView); }, [savedViewType, loaded]); const onViewChange = React.useCallback( (newViewType: T) => { setViewType(newViewType); setSavedViewType(newViewType); }, [setSavedViewType], ); return [viewType, onViewChange, loaded] as const; }Also applies to: 53-57, 61-67
🤖 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 31 - 39, The persisted view-type handling in ApplicationResourcesTab is duplicated with the same useUserSettings, local state, sync effect, and onViewChange pattern used in ApplicationSetApplicationsView, so factor it into a shared hook such as usePersistedViewType(settingKey, defaultView). Move the repeated state and synchronization logic into the hook, then have ApplicationResourcesTab use the shared hook instead of maintaining its own savedViewType/viewType wiring. Keep the hook generic enough to reuse the same behavior in both components and avoid future divergence.src/gitops/components/shared/ApplicationSetApplicationsView.tsx (1)
85-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGraph/list wrapper markup duplicated with
ApplicationResourcesView.tsx.The
Stack/StackItem/Flextoolbar+content wrapper, panel class toggling, and graph/list conditional structure here is near-identical toApplicationResourcesView.tsx(lines 86-140), differing only in the toolbar component and graph component used. Extracting a sharedGitOpsGraphListLayoutcomponent (parameterized with toolbar/table/graph render props and test-id) would remove this duplication and keep future styling/behavior changes in sync.🤖 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 85 - 147, The `ApplicationSetApplicationsView` wrapper duplicates the same `Stack`/`StackItem`/`Flex` layout and list-vs-graph branching already present in `ApplicationResourcesView`, so refactor this shared shell into a reusable `GitOpsGraphListLayout` component. Make the new component accept render props or slots for the toolbar, list content, graph content, and test-id/class selection, then update `ApplicationSetApplicationsView` to pass its `ListPageFilter`, `GitOpsViewSwitcher`, `GitOpsDataViewTable`, and `ApplicationSetGraphView` into that shared layout.
🤖 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.
Nitpick comments:
In `@src/gitops/components/application/ApplicationResourcesTab.tsx`:
- Around line 31-39: The persisted view-type handling in ApplicationResourcesTab
is duplicated with the same useUserSettings, local state, sync effect, and
onViewChange pattern used in ApplicationSetApplicationsView, so factor it into a
shared hook such as usePersistedViewType(settingKey, defaultView). Move the
repeated state and synchronization logic into the hook, then have
ApplicationResourcesTab use the shared hook instead of maintaining its own
savedViewType/viewType wiring. Keep the hook generic enough to reuse the same
behavior in both components and avoid future divergence.
In `@src/gitops/components/shared/ApplicationSetApplicationsView.tsx`:
- Around line 85-147: The `ApplicationSetApplicationsView` wrapper duplicates
the same `Stack`/`StackItem`/`Flex` layout and list-vs-graph branching already
present in `ApplicationResourcesView`, so refactor this shared shell into a
reusable `GitOpsGraphListLayout` component. Make the new component accept render
props or slots for the toolbar, list content, graph content, and test-id/class
selection, then update `ApplicationSetApplicationsView` to pass its
`ListPageFilter`, `GitOpsViewSwitcher`, `GitOpsDataViewTable`, and
`ApplicationSetGraphView` into that shared layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4e332440-519a-4c2b-8da4-ab9d4bfc3460
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
locales/en/plugin__gitops-plugin.jsonlocales/ja/plugin__gitops-plugin.jsonlocales/ko/plugin__gitops-plugin.jsonlocales/zh/plugin__gitops-plugin.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
✅ Files skipped from review due to trivial changes (3)
- src/gitops/components/shared/GitOpsViewType.ts
- locales/ko/plugin__gitops-plugin.json
- locales/zh/plugin__gitops-plugin.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/gitops/components/application/graph/nodes/ApplicationNode.tsx
- src/gitops/components/application/ApplicationResourcesToolbar.tsx
- locales/en/plugin__gitops-plugin.json
- locales/ja/plugin__gitops-plugin.json
- src/gitops/components/shared/GitOpsGraphListView.scss
- src/gitops/components/shared/GitOpsViewSwitcher.tsx
6fff12f to
52edc50
Compare
See GITOPS-10018 for details