Implement Topology's List/Graph switch toggle (#10018)#251
Open
keithchong wants to merge 4 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 2 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 selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds persisted graph/list view selection for application resources and ApplicationSet applications. It introduces shared view components, refactors rendering paths, adds layout styling and view labels, and changes healthy application icon styling. ChangesGraph/List View Switcher Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ApplicationResourcesTab
participant GitOpsViewSwitcher
participant ApplicationResourcesView
participant useListPageFilter
participant GitOpsDataViewTable
participant ApplicationGraphView
ApplicationResourcesTab->>GitOpsViewSwitcher: viewType and onViewChange
GitOpsViewSwitcher-->>ApplicationResourcesTab: toggled viewType
ApplicationResourcesTab->>ApplicationResourcesView: resources and viewType
ApplicationResourcesView->>useListPageFilter: apply resource filters
useListPageFilter-->>ApplicationResourcesView: filtered resources
alt list view
ApplicationResourcesView->>GitOpsDataViewTable: columns and rows
else graph view
ApplicationResourcesView->>ApplicationGraphView: filtered resources
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.72% -0.20%
==========================================
Files 154 163 +9
Lines 6272 6420 +148
Branches 2028 2212 +184
==========================================
+ Hits 748 753 +5
+ Misses 5524 5439 -85
- Partials 0 228 +228
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+69
to
+76
| 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.
Comment on lines
+161
to
+164
| 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.
Collaborator
There was a problem hiding this comment.
@keithchong wdyt about this? to
aValue = a.syncWave ?? '';
bValue = b.syncWave ?? '';
and down line 278 to?
cell: <>{resource.syncWave ?? '-'}</>,
|
|
||
| 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.
Suggested change
| 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.
Suggested change
| 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.
Comment on lines
+125
to
+133
| <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.
Suggested change
| <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.
keithchong
force-pushed
the
10018-GraphListToggle
branch
from
July 3, 2026 20:59
d422cc6 to
6fff12f
Compare
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
keithchong
force-pushed
the
10018-GraphListToggle
branch
from
July 3, 2026 21:05
6fff12f to
52edc50
Compare
aali309
reviewed
Jul 7, 2026
aali309
left a comment
aali309
left a comment
Collaborator
There was a problem hiding this comment.
LGTM apart from the conflict, just a few questions.
keithchong
force-pushed
the
10018-GraphListToggle
branch
from
July 16, 2026 20:35
52edc50 to
6f2205b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/gitops/components/shared/ApplicationList.tsx (1)
258-258: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the search-filtered applications to the graph view.
The graph view is currently receiving
filteredData(which ignores the URL search query), whilerowsandisEmptyusefilteredBySearch. Update this prop to usefilteredBySearchso that the URL search query correctly filters the graph view.🐛 Proposed fix
- 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 258, Update the graph view’s filteredApplications prop in ApplicationList to pass filteredBySearch instead of filteredData, matching the search-filtered data used by rows and isEmpty.src/gitops/components/shared/ApplicationSetApplicationsView.tsx (1)
122-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the error state in list view.
Passing
activeState={... : null}overridesGitOpsDataViewTable’s built-in error resolution, causing load errors to render as a normal table instead oferrorState.Remove the
activeStateprop entirely soGitOpsDataViewTablecan fall back to its internal empty/error logic.🐛 Proposed fix
<GitOpsDataViewTable columns={columns} rows={rows} isEmpty={isEmpty} emptyState={emptyState} errorState={errorState} isError={isError} - activeState={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 122 - 130, Remove the activeState prop from the GitOpsDataViewTable usage in ApplicationSetApplicationsView so its built-in state resolution can render errorState on load failures while preserving the existing emptyState, errorState, and isError props.src/gitops/components/shared/GitOpsGraphListView.scss (1)
10-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't force the graph height onto the list panel.
gitops-graph-list-view__list-panelgets amin-height: 1000px, which makes short or empty list views render with a page-sized blank region. Keep the fixed height on the graph panel only.🐛 Proposed fix
- &__panel, - &__list-panel { + &__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, Update the shared panel styles so the min-height constraint applies only to gitops-graph-list-view__panel, removing gitops-graph-list-view__list-panel from the min-height selector while preserving its existing flex layout rules.
🧹 Nitpick comments (1)
src/gitops/components/application/ApplicationResourcesView.tsx (1)
47-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the new view behavior.
Codecov reports all 138 changed lines in this file uncovered. Please test persisted view selection, graph/list switching, sorting—including sync wave
0—filtering, and empty states.🤖 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 47 - 77, Add tests for ApplicationResourcesView covering persisted view selection, graph/list view switching, sorting by each supported column including sync-wave value 0, resource filtering, and empty filtered and unfiltered states. Exercise the component through its existing hooks and callbacks, including useGitOpsDataViewSort, useResourceColumnsDV, useListPageFilter, and useResourceRowsDV, while asserting the rendered view and displayed resources.
🤖 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.
Duplicate comments:
In `@src/gitops/components/shared/ApplicationList.tsx`:
- Line 258: Update the graph view’s filteredApplications prop in ApplicationList
to pass filteredBySearch instead of filteredData, matching the search-filtered
data used by rows and isEmpty.
In `@src/gitops/components/shared/ApplicationSetApplicationsView.tsx`:
- Around line 122-130: Remove the activeState prop from the GitOpsDataViewTable
usage in ApplicationSetApplicationsView so its built-in state resolution can
render errorState on load failures while preserving the existing emptyState,
errorState, and isError props.
In `@src/gitops/components/shared/GitOpsGraphListView.scss`:
- Around line 10-18: Update the shared panel styles so the min-height constraint
applies only to gitops-graph-list-view__panel, removing
gitops-graph-list-view__list-panel from the min-height selector while preserving
its existing flex layout rules.
---
Nitpick comments:
In `@src/gitops/components/application/ApplicationResourcesView.tsx`:
- Around line 47-77: Add tests for ApplicationResourcesView covering persisted
view selection, graph/list view switching, sorting by each supported column
including sync-wave value 0, resource filtering, and empty filtered and
unfiltered states. Exercise the component through its existing hooks and
callbacks, including useGitOpsDataViewSort, useResourceColumnsDV,
useListPageFilter, and useResourceRowsDV, while asserting the rendered view and
displayed resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1f8e1c4e-58f0-4e2e-8d04-ec0eaa3fcdec
📒 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 as they are similar to previous changes (7)
- src/gitops/components/shared/GitOpsViewType.ts
- src/gitops/components/shared/GitOpsViewSwitcher.tsx
- src/gitops/components/application/ApplicationResourcesViewType.ts
- src/gitops/components/application/ApplicationResourcesToolbar.tsx
- locales/zh/plugin__gitops-plugin.json
- locales/ja/plugin__gitops-plugin.json
- locales/en/plugin__gitops-plugin.json
keithchong
force-pushed
the
10018-GraphListToggle
branch
from
July 16, 2026 20:54
6f2205b to
480ec3e
Compare
Signed-off-by: Keith Chong <kykchong@redhat.com>
keithchong
force-pushed
the
10018-GraphListToggle
branch
from
July 16, 2026 21:14
480ec3e to
0a5c567
Compare
Signed-off-by: Keith Chong <kykchong@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@locales/zh/plugin__gitops-plugin.json`:
- Around line 337-338: Replace the English values for the “List view” and “Graph
view” entries in the Chinese locale with the project’s approved Chinese
translations, keeping the existing translation keys unchanged.
In `@src/gitops/components/application/ApplicationResourcesView.tsx`:
- Around line 356-374: Use a locale-independent sentinel for missing health
status in the resource-health filter. Update the reducer, filter logic, and item
construction around the resource-health configuration to share a stable
non-translated ID for absent status, while retaining t('None') only for the
displayed title.
🪄 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: 45e52330-09ec-4975-905d-fc1a4c656f64
📒 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 as they are similar to previous changes (11)
- src/gitops/components/shared/GitOpsViewType.ts
- src/gitops/components/application/graph/nodes/ApplicationNode.tsx
- src/gitops/components/application/ApplicationResourcesViewType.ts
- src/gitops/components/shared/GitOpsViewSwitcher.tsx
- src/gitops/components/shared/GitOpsGraphListView.scss
- src/gitops/components/application/ApplicationResourcesToolbar.tsx
- src/gitops/components/shared/ApplicationSetApplicationsView.tsx
- src/gitops/components/shared/ApplicationList.tsx
- locales/ko/plugin__gitops-plugin.json
- locales/en/plugin__gitops-plugin.json
- locales/ja/plugin__gitops-plugin.json
Signed-off-by: Keith Chong <kykchong@redhat.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
aali309
reviewed
Jul 17, 2026
aali309
left a comment
aali309
left a comment
Collaborator
There was a problem hiding this comment.
@keithchong, left a few minor comments
Comment on lines
+161
to
+164
| case 'sync-wave': | ||
| aValue = a.syncWave || ''; | ||
| bValue = b.syncWave || ''; | ||
| break; |
Collaborator
There was a problem hiding this comment.
@keithchong wdyt about this? to
aValue = a.syncWave ?? '';
bValue = b.syncWave ?? '';
and down line 278 to?
cell: <>{resource.syncWave ?? '-'}</>,
| </div> | ||
| ), | ||
| id: resource.name + '-' + index, | ||
| dataLabel: 'Name', |
Collaborator
There was a problem hiding this comment.
Suggested change
| dataLabel: 'Name', | |
| dataLabel: t('Name'), |
| { | ||
| id: 'sync-wave-' + index, | ||
| cell: <>{resource.syncWave || '-'}</>, | ||
| dataLabel: 'Sync Order', |
Collaborator
There was a problem hiding this comment.
Suggested change
| dataLabel: 'Sync Order', | |
| dataLabel: t('Sync Wave'), |
?
| <Td colSpan={columnsDV.length}> | ||
| <EmptyState headingLevel="h4" icon={CubesIcon} titleText={t('No resources')}> | ||
| <EmptyStateBody> | ||
| {t('There are no resources associated with the application.')} |
Collaborator
There was a problem hiding this comment.
This could be misleading, both resources.length === 0 and resources.length > 0 && filteredResources.length === 0 display the same message. May be for the filter we could have something like No resources match the current filters (try clearing filters) to distinguish between empty resources and empty filtered list?
Comment on lines
+62
to
+66
| React.useEffect(() => { | ||
| if (viewSettingsLoaded) { | ||
| setViewType(savedViewType ?? GitOpsViewType.graph); | ||
| } | ||
| }, [savedViewType, viewSettingsLoaded]); |
Collaborator
There was a problem hiding this comment.
More efficient I think, this will show list immediately by not waiting for useEffect. wdyt?
Suggested change
| React.useEffect(() => { | |
| if (viewSettingsLoaded) { | |
| setViewType(savedViewType ?? GitOpsViewType.graph); | |
| } | |
| }, [savedViewType, viewSettingsLoaded]); | |
| const [viewType, setViewType] = React.useState<GitOpsViewType>(); | |
| const activeViewType = viewType ?? savedViewType ?? GitOpsViewType.graph; |
| isListView ? 'gitops-graph-list-view__list-panel' : 'gitops-graph-list-view__panel' | ||
| } | ||
| > | ||
| {isListView ? ( |
Collaborator
There was a problem hiding this comment.
Suggested change
| {isListView ? ( | |
| {isListView ? ( | |
| {isError && errorState ? ( | |
| errorState | |
| ) : isListView ? ( | |
Apply the same failed application load on graph view too?
| <ApplicationSetApplicationsView | ||
| applicationSet={appset as ApplicationSetKind} | ||
| ownedApps={ownedApps as ApplicationKind[]} | ||
| filteredApplications={filteredData as ApplicationKind[]} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See GITOPS-10018 for details