Skip to content

Implement Topology's List/Graph switch toggle (#10018)#251

Open
keithchong wants to merge 1 commit into
redhat-developer:mainfrom
keithchong:10018-GraphListToggle
Open

Implement Topology's List/Graph switch toggle (#10018)#251
keithchong wants to merge 1 commit into
redhat-developer:mainfrom
keithchong:10018-GraphListToggle

Conversation

@keithchong

Copy link
Copy Markdown
Collaborator

See GITOPS-10018 for details

@openshift-ci openshift-ci Bot requested a review from wtam2018 June 29, 2026 12:26
@keithchong keithchong requested a review from aali309 June 29, 2026 12:26
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@keithchong, you've reached your PR review limit, so we couldn't start this review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 4aaa7f3a-4d06-44f1-b91d-f8bae1c18540

📥 Commits

Reviewing files that changed from the base of the PR and between 6fff12f and 52edc50.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (14)
  • locales/en/plugin__gitops-plugin.json
  • locales/ja/plugin__gitops-plugin.json
  • locales/ko/plugin__gitops-plugin.json
  • locales/zh/plugin__gitops-plugin.json
  • src/gitops/components/application/ApplicationResourcesTab.tsx
  • src/gitops/components/application/ApplicationResourcesToolbar.tsx
  • src/gitops/components/application/ApplicationResourcesView.tsx
  • src/gitops/components/application/ApplicationResourcesViewType.ts
  • src/gitops/components/application/graph/nodes/ApplicationNode.tsx
  • src/gitops/components/shared/ApplicationList.tsx
  • src/gitops/components/shared/ApplicationSetApplicationsView.tsx
  • src/gitops/components/shared/GitOpsGraphListView.scss
  • src/gitops/components/shared/GitOpsViewSwitcher.tsx
  • src/gitops/components/shared/GitOpsViewType.ts
📝 Walkthrough

Walkthrough

This PR adds a shared GitOpsViewType enum and a reusable GitOpsViewSwitcher component enabling toggling between list and graph views. New ApplicationResourcesView and ApplicationSetApplicationsView components persist user view preference and render list/graph content. ApplicationResourcesTab and ApplicationList are refactored to delegate to these views, an icon styling tweak is applied, and locale files are updated with reordered strings and new view labels.

Changes

Graph/List View Switcher Feature

Layer / File(s) Summary
Shared view-type enum and setting keys
src/gitops/components/shared/GitOpsViewType.ts, src/gitops/components/application/ApplicationResourcesViewType.ts
Defines GitOpsViewType enum (graph/list) and persisted setting-key constants, re-exported as ApplicationResourcesViewType.
View switcher button and layout styles
src/gitops/components/shared/GitOpsViewSwitcher.tsx, src/gitops/components/application/ApplicationResourcesToolbar.tsx, src/gitops/components/shared/GitOpsGraphListView.scss
New tooltip-wrapped toggle button switching view types, a toolbar wrapper for resources, and flex-layout SCSS for the graph/list container.
ApplicationResourcesView: sort, filter, columns, rows, rendering
src/gitops/components/application/ApplicationResourcesView.tsx
New component sorting/filtering resources and rendering either a data-view table or graph view, with column/row builders and action cell logic.
ApplicationResourcesTab refactor
src/gitops/components/application/ApplicationResourcesTab.tsx, src/gitops/components/application/graph/nodes/ApplicationNode.tsx
Tab now persists view type and delegates rendering to ApplicationResourcesView; health icon styling switched from fill to color.
ApplicationSetApplicationsView: persisted view and rendering
src/gitops/components/shared/ApplicationSetApplicationsView.tsx
New component persisting appset view type, rendering a toolbar with optional filters and switcher, and toggling between table and graph views.
ApplicationList refactor
src/gitops/components/shared/ApplicationList.tsx
Replaces inline appset graph rendering with conditional delegation to ApplicationSetApplicationsView or table rendering.
Locale string updates
locales/en/..., locales/ja/..., locales/ko/..., locales/zh/...
Reorders and reformats translation entries and adds "List view"/"Graph view" labels across four locales.

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
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description only points to an issue link and doesn't summarize the changeset. Add a brief description of the main code and UI changes, such as the new list/graph switch and affected components.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change by calling out the new List/Graph toggle for Topology.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 230 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.65%. Comparing base (01e9db1) to head (52edc50).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...omponents/application/ApplicationResourcesView.tsx 0.00% 138 Missing ⚠️
...mponents/shared/ApplicationSetApplicationsView.tsx 0.00% 35 Missing ⚠️
...components/application/ApplicationResourcesTab.tsx 0.00% 18 Missing ⚠️
...rc/gitops/components/shared/GitOpsViewSwitcher.tsx 0.00% 18 Missing ⚠️
src/gitops/components/shared/ApplicationList.tsx 0.00% 7 Missing ⚠️
src/gitops/components/shared/GitOpsViewType.ts 0.00% 5 Missing and 1 partial ⚠️
...onents/application/ApplicationResourcesToolbar.tsx 0.00% 4 Missing ⚠️
...onents/application/ApplicationResourcesViewType.ts 0.00% 3 Missing ⚠️
...onents/application/graph/nodes/ApplicationNode.tsx 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
unit-tests 11.65% <0.00%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 01e9db1 and d422cc6.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (18)
  • locales/en/plugin__gitops-plugin.json
  • locales/en/topology.json
  • locales/ja/plugin__gitops-plugin.json
  • locales/ja/topology.json
  • locales/ko/plugin__gitops-plugin.json
  • locales/ko/topology.json
  • locales/zh/plugin__gitops-plugin.json
  • locales/zh/topology.json
  • src/gitops/components/application/ApplicationResourcesTab.tsx
  • src/gitops/components/application/ApplicationResourcesToolbar.tsx
  • src/gitops/components/application/ApplicationResourcesView.tsx
  • src/gitops/components/application/ApplicationResourcesViewType.ts
  • src/gitops/components/application/graph/nodes/ApplicationNode.tsx
  • src/gitops/components/shared/ApplicationList.tsx
  • src/gitops/components/shared/ApplicationSetApplicationsView.tsx
  • src/gitops/components/shared/GitOpsGraphListView.scss
  • src/gitops/components/shared/GitOpsViewSwitcher.tsx
  • src/gitops/components/shared/GitOpsViewType.ts

Comment thread src/gitops/components/application/ApplicationResourcesTab.tsx Outdated
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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 && (
             <ApplicationResourcesView

Also 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 thread src/gitops/components/application/ApplicationResourcesView.tsx Outdated
Comment on lines +161 to +164
case 'sync-wave':
aValue = a.syncWave || '';
bValue = b.syncWave || '';
break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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[]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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 thread src/gitops/components/shared/ApplicationSetApplicationsView.tsx Outdated
Comment on lines +125 to +133
<GitOpsDataViewTable
columns={columns}
rows={rows}
isEmpty={isEmpty}
emptyState={emptyState}
errorState={errorState}
isError={isError}
activeState={isEmpty ? DataViewState.empty : null}
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

Comment thread src/gitops/components/shared/GitOpsGraphListView.scss
@keithchong keithchong force-pushed the 10018-GraphListToggle branch from d422cc6 to 6fff12f Compare July 3, 2026 20:59
Signed-off-by: Keith Chong <kykchong@redhat.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
src/gitops/components/application/ApplicationResourcesTab.tsx (1)

31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate persisted-view-type logic.

This useUserSettings + local state + sync useEffect + onViewChange pattern is duplicated almost line-for-line in ApplicationSetApplicationsView.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 win

Graph/list wrapper markup duplicated with ApplicationResourcesView.tsx.

The Stack/StackItem/Flex toolbar+content wrapper, panel class toggling, and graph/list conditional structure here is near-identical to ApplicationResourcesView.tsx (lines 86-140), differing only in the toolbar component and graph component used. Extracting a shared GitOpsGraphListLayout component (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

📥 Commits

Reviewing files that changed from the base of the PR and between d422cc6 and 6fff12f.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (14)
  • locales/en/plugin__gitops-plugin.json
  • locales/ja/plugin__gitops-plugin.json
  • locales/ko/plugin__gitops-plugin.json
  • locales/zh/plugin__gitops-plugin.json
  • src/gitops/components/application/ApplicationResourcesTab.tsx
  • src/gitops/components/application/ApplicationResourcesToolbar.tsx
  • src/gitops/components/application/ApplicationResourcesView.tsx
  • src/gitops/components/application/ApplicationResourcesViewType.ts
  • src/gitops/components/application/graph/nodes/ApplicationNode.tsx
  • src/gitops/components/shared/ApplicationList.tsx
  • src/gitops/components/shared/ApplicationSetApplicationsView.tsx
  • src/gitops/components/shared/GitOpsGraphListView.scss
  • src/gitops/components/shared/GitOpsViewSwitcher.tsx
  • src/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 keithchong force-pushed the 10018-GraphListToggle branch from 6fff12f to 52edc50 Compare July 3, 2026 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants