app-catalog: Fix early returns before hooks in EditorDialog components by AshutoshSharma-pixel · Pull Request #665 · headlamp-k8s/plugins

AshutoshSharma-pixel · 2026-05-05T18:59:00Z

Fixes #525

Problem

Both EditorDialog components had early returns before hook calls,
violating React's Rules of Hooks which require hooks to be called
unconditionally on every render.

releases/EditorDialog.tsx: if (!release) return null at line 52,
before 10+ hook calls
charts/EditorDialog.tsx: if (!props.chart) return null at line 27,
before all hook calls

Fix

Moved both guards to after all hooks. State initializers in
releases/EditorDialog.tsx made null-safe with optional chaining
(release?.chart?.values, release?.config).

Testing

npm run tsc, npm run lint, and npm run format all pass clean.

skoeva

lgtm!

Copilot

Pull request overview

This PR aims to fix React Rules of Hooks violations in the app-catalog EditorDialog components by removing early returns that occurred before hook calls.

Changes:

Moves the release null-guard in releases/EditorDialog.tsx to after hook declarations and makes initial state creation null-safe.
Moves the chart null-guard in charts/EditorDialog.tsx to occur after some hooks (intended to be after all hooks).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
app-catalog/src/components/releases/EditorDialog.tsx	Moves the `!release` guard after hooks and makes initial state null-safe (but introduces state/effect safety issues when `release` is initially null).
app-catalog/src/components/charts/EditorDialog.tsx	Moves the `!chart` guard (but still leaves it before later `useEffect` hooks, so the hooks violation persists).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

AshutoshSharma-pixel · 2026-05-12T03:38:38Z

@illume Addressed all 3 Copilot comments in the latest commit:

releases/EditorDialog.tsx: state initializers now use empty object defaults {} instead of dereferencing release directly; added useEffect to sync valuesToShow, values, and userValues when release loads; added if (!release) return; inside the useEffect that dereferences release.chart.metadata
charts/EditorDialog.tsx: moved if (!props.chart) return null to after ALL useEffect hooks (line 130, after the last useEffect at line 123)

AshutoshSharma-pixel · 2026-05-15T11:22:02Z

@ashu8912 All Copilot comments have been addressed in the latest commit (214296f). The guards are now after all hooks in both files. @skoeva has approved ready for merge when you get a chance.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

app-catalog/src/components/charts/EditorDialog.tsx:31

Now that the !chart guard is after hooks, Namespace.useList() (and related state/effects) will run even when chart is undefined and the component returns null. That introduces an unnecessary cluster API query and extra work for a render that produces no UI. Consider a wrapper component that returns null when !chart and renders a child component containing the hooks when chart is present.

  const { openEditor, handleEditor, chart, chartProfile } = props;
  const { t } = useTranslation();
  const [installLoading, setInstallLoading] = useState(false);
  const [namespaces, error] = K8s.ResourceClasses.Namespace.useList();
  const [chartValues, setChartValues] = useState<string>('');

  useEffect(() => {
+    if (!release) return;
    let isMounted = true;


AshutoshSharma-pixel · 2026-05-16T08:08:45Z

@skoeva addressed the remaining Copilot comment in the latest commit (e39f537) added release to the useEffect dependency array so chart versions re-fetch correctly when release loads asynchronously.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

+  useEffect(() => {
+    if (!release) return;
+    const merged = Object.assign({}, release.chart.values, release.config);
+    setValuesToShow(merged);
+    setValues(merged);
+    setUserValues(release.config);
+  }, [release]);


+  const [valuesToShow, setValuesToShow] = useState({});
+  const [values, setValues] = useState({});
+  const [userValues, setUserValues] = useState({});


  useEffect(() => {
+    if (!chart) return;
    if (selectedVersion) {
      handleChartValueFetch(chart);
    }
  }, [selectedVersion]);


    }
  }, [selectedVersion]);

+  if (!props.chart) return null;


React's Rules of Hooks require hooks to be called unconditionally on every render. Both EditorDialog components had early returns before hook calls, violating this rule. releases/EditorDialog.tsx: moved 'if (!release) return null' guard to after all hooks and useEffects. State initializers made null-safe with empty object defaults. Added useEffect to sync state when release loads. Added null guard inside useEffect that dereferences release to prevent runtime errors. charts/EditorDialog.tsx: moved 'if (!props.chart) return null' guard to after all hooks and useEffects. Added null guards inside effects that dereference chart. Fixes headlamp-k8s#525 Signed-off-by: Ashutosh Sharma <itsashutosh769@gmail.com>

AshutoshSharma-pixel · 2026-05-17T15:50:13Z

Addressed all 4 Copilot comments in the latest commit (f3767b7):

valuesToShow and values now store independent copies using spread ({...merged}) to avoid shared reference mutation
userValues also stores an independent copy ({...release.config})
All three useState initializers typed as Record<string, unknown>
chart added to selectedVersion useEffect dependency array
Guard changed from props.chart to chart for consistency

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

+  const [valuesToShow, setValuesToShow] = useState<Record<string, unknown>>({});
+  const [values, setValues] = useState<Record<string, unknown>>({});
+  const [userValues, setUserValues] = useState<Record<string, unknown>>({});


+    const merged = Object.assign({}, release.chart.values, release.config);
+    setValuesToShow({ ...merged });
+    setValues({ ...merged });
+    setUserValues({ ...release.config });


  useEffect(() => {
+    if (!chart) return;
    if (chartCfg.chartProfile === chartProfile) {
      const versionsArray =
        AVAILABLE_VERSIONS instanceof Map && AVAILABLE_VERSIONS.get && chart.name


AshutoshSharma-pixel · 2026-05-17T15:53:31Z

Thanks for the feedback. The current implementation is sufficient for the scope of this fix (moving guards after hooks). The lazy initializer suggestion adds complexity beyond what's needed here, and Object.assign({}, ...) already creates a new object so the spread is redundant but harmless. I'll leave these as potential follow-up improvements. The core Rules of Hooks violation is fixed.

skoeva reviewed May 7, 2026

View reviewed changes

Comment thread app-catalog/src/components/releases/EditorDialog.tsx Outdated

skoeva approved these changes May 12, 2026

View reviewed changes

skoeva requested a review from Copilot May 12, 2026 03:21

Copilot started reviewing on behalf of skoeva May 12, 2026 03:22 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread app-catalog/src/components/releases/EditorDialog.tsx Outdated

Comment thread app-catalog/src/components/releases/EditorDialog.tsx

Comment thread app-catalog/src/components/charts/EditorDialog.tsx Outdated

AshutoshSharma-pixel force-pushed the fix/editor-dialog-hooks-order branch from d11f4a8 to 214296f Compare May 12, 2026 03:37

skoeva requested a review from Copilot May 15, 2026 13:39

Copilot started reviewing on behalf of skoeva May 15, 2026 13:39 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread app-catalog/src/components/releases/EditorDialog.tsx

Comment on lines 71 to 73

useEffect(() => {

if (!release) return;

let isMounted = true;

AshutoshSharma-pixel force-pushed the fix/editor-dialog-hooks-order branch from 214296f to e39f537 Compare May 16, 2026 08:08

AshutoshSharma-pixel requested a review from Copilot May 17, 2026 15:04

Copilot AI reviewed May 17, 2026

View reviewed changes

AshutoshSharma-pixel force-pushed the fix/editor-dialog-hooks-order branch from e39f537 to f3767b7 Compare May 17, 2026 15:49

AshutoshSharma-pixel requested a review from Copilot May 17, 2026 15:50

Copilot AI reviewed May 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app-catalog: Fix early returns before hooks in EditorDialog components#665

app-catalog: Fix early returns before hooks in EditorDialog components#665
AshutoshSharma-pixel wants to merge 1 commit into
headlamp-k8s:mainfrom
AshutoshSharma-pixel:fix/editor-dialog-hooks-order

AshutoshSharma-pixel commented May 5, 2026

Uh oh!

Uh oh!

skoeva left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AshutoshSharma-pixel commented May 12, 2026 •

edited

Loading

Uh oh!

AshutoshSharma-pixel commented May 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

AshutoshSharma-pixel commented May 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

AshutoshSharma-pixel commented May 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

AshutoshSharma-pixel commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

AshutoshSharma-pixel commented May 5, 2026

Problem

Fix

Testing

Uh oh!

Uh oh!

skoeva left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AshutoshSharma-pixel commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AshutoshSharma-pixel commented May 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

AshutoshSharma-pixel commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

AshutoshSharma-pixel commented May 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

AshutoshSharma-pixel commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AshutoshSharma-pixel commented May 12, 2026 •

edited

Loading

AshutoshSharma-pixel commented May 16, 2026 •

edited

Loading