Fixes #38806 - Stabilize job detail table and fix unresponsive actions by kmalyjur · Pull Request #1009 · theforeman/foreman_remote_execution

kmalyjur · 2025-10-06T16:46:33Z

Initial issue:
The "Rerun" button in the Job Details table would inconsistently become unresponsive.

My thoughts:
I couldn't reproduce this bug, however, I still decided to make this change. When a row was collapsed, the react state was destroyed, which could break event handlers on other rows during a re-render.

Solution:
Lifting the component state to the JobInvocationHostTable.

MariaAga · 2025-10-10T08:12:16Z

webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js

-    act(() => {
-      fireEvent.click(screen.getByText('STDOUT'));
-      fireEvent.click(screen.getByText('DEBUG'));
-      fireEvent.click(screen.getByText('STDERR'));
-    });
-    expect(
-      screen.queryAllByText('No output for the selected filters')
-    ).toHaveLength(1);
-    expect(screen.queryAllByText('Exit status: 1')).toHaveLength(0);
-    expect(
-      screen.queryAllByText('StandardError: Job execution failed')
-    ).toHaveLength(0);


These tests need to be moved, not removed

MariaAga · 2025-10-10T10:08:50Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+  const getInvocationState = hostId =>
+    hostInvocationStates[hostId] || {
+      showOutputType: { stderr: true, stdout: true, debug: true },
+      showTemplatePreview: false,
+      showCommand: false,
+    };
+


It also needs to be used in webpack/JobInvocationDetail/TemplateInvocationPage.js

Copilot

Pull Request Overview

This PR fixes issue #38806 by stabilizing the job detail table and resolving unresponsive action buttons. The solution lifts component state from individual TemplateInvocation components to the parent JobInvocationHostTable to prevent state destruction when rows are collapsed/expanded.

Moved template invocation state (output toggles, preview settings) from child to parent component
Refactored tests to remove interactive event testing and use prop-based state management
Added CSS styling to properly hide collapsed rows

Reviewed Changes

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

Show a summary per file

File	Description
TemplateInvocation.js	Removed internal state management, now receives state as props
JobInvocationHostTable.js	Added centralized state management for all host invocation states
OutputToggleGroup.js	Updated to use direct state values instead of functional updates
TemplateInvocation.test.js	Refactored tests to use prop-based state instead of simulated user interactions
JobInvocationDetail.scss	Added CSS rule to hide collapsed table rows

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-10T10:11:13Z

webpack/JobInvocationDetail/TemplateInvocationComponents/OutputToggleGroup.js

  const handleSTDERRClick = _isSelected => {
-    setShowOutputType(prevShowOutputType => ({
-      ...prevShowOutputType,
+    setShowOutputType({
+      ...showOutputType,
      stderr: _isSelected,
-    }));
+    });
  };
-
  const handleSTDOUTClick = _isSelected => {
-    setShowOutputType(prevShowOutputType => ({
-      ...prevShowOutputType,
+    setShowOutputType({
+      ...showOutputType,
      stdout: _isSelected,
-    }));
+    });
  };
  const handleDEBUGClick = _isSelected => {
-    setShowOutputType(prevShowOutputType => ({
-      ...prevShowOutputType,
+    setShowOutputType({
+      ...showOutputType,
      debug: _isSelected,
-    }));
+    });
  };


The state update functions create new objects on every call, which could cause unnecessary re-renders. Consider using functional updates or memoizing these handlers to avoid creating new objects when the current state hasn't changed.

Copilot · 2025-10-10T10:11:14Z

webpack/JobInvocationDetail/TemplateInvocation.js

  jobID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
  isInTableView: PropTypes.bool,
  isExpanded: PropTypes.bool,
+  showOutputType: PropTypes.object.isRequired,


Using PropTypes.object is too generic. Consider using PropTypes.shape() to define the specific structure expected: PropTypes.shape({ stderr: PropTypes.bool, stdout: PropTypes.bool, debug: PropTypes.bool }).isRequired

Suggested change

showOutputType: PropTypes.object.isRequired,

showOutputType: PropTypes.shape({

stderr: PropTypes.bool,

stdout: PropTypes.bool,

debug: PropTypes.bool,

}).isRequired,

MariaAga · 2025-10-10T10:19:48Z

webpack/JobInvocationDetail/JobInvocationDetail.scss

+.pf-v5-c-table__tbody > tr.row-hidden {
+  display: none;
+}


This needs to be more specific

MariaAga · 2025-10-10T10:21:00Z

webpack/JobInvocationDetail/TemplateInvocation.js


-  if (!isExpanded) {
-    return null;
-  }
-
-  if ((status === STATUS.PENDING && isEmpty(response)) || !response) {
-    return <Skeleton />;
+  if (!response || (status === STATUS.PENDING && isEmpty(response))) {
+    return <Skeleton data-testid="template-invocation-skeleton" />;


Could you please explain this change?

if (!isExpanded) { return null; }

Removed it so it hides the row instead of destroying it -> saves its state

data-testid

fixes failing test

Do you think it should be done differently?

That makes sense, thanks!

webpack/JobInvocationDetail/JobInvocationHostTable.js

adamruzicka

The lint issues seem to be a hard requirement

adamruzicka · 2025-10-23T09:02:53Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

+                  />
+                  <RowSelectTd
+                    rowData={result}
+                    {...{ selectOne, isSelected }}


Isn't the splat unnecessary?

Suggested change

{...{ selectOne, isSelected }}

{selectOne, isSelected}

Oh, so the first pair of curly braces is to go from jsx into the "native js" context and then splat the hash inside it? Wouldn't this then be the same thing as

Suggested change

{...{ selectOne, isSelected }}

selectOne

isSelected

I don't insist on it, just trying to figure out how these things work

another way would be just

Suggested change

{...{ selectOne, isSelected }}

selectOne={selecteOne}

isSelected={isSelected}

adamruzicka · 2025-10-23T09:07:25Z

webpack/JobInvocationDetail/JobInvocationHostTable.js

@@ -64,6 +64,13 @@ const JobInvocationHostTable = ({
  const [expandedHost, setExpandedHost] = useState([]);


Although not introduced here, a Set would be a more appropriate data structure, considering how it is being used

This will also need changes in JobInvocationHostTable.js:352

Uncaught TypeError: expandedHost.includes is not a function at JobInvocationHostTable.js:352:46 at Array.every (<anonymous>) at JobInvocationHostTable (JobInvocationHostTable.js:352:17)

kmalyjur · 2025-10-23T10:24:02Z

@MariaAga @adamruzicka thank you for your reviews! Could you please check if I incorporated everything?

adamruzicka

To be explicit about #1009 (comment)

adamruzicka

Looks good, I haven't seen any oddities with this

MariaAga · 2025-11-10T17:42:36Z

webpack/JobInvocationDetail/__tests__/TemplateInvocation.test.js

+jest.mock('@patternfly/react-core', () => {
+  const originalModule = jest.requireActual('@patternfly/react-core');
+  return {
+    ...originalModule,
+    Tooltip: ({ children }) => <>{children}</>,
+    Popper: ({ children }) => <>{children}</>,
+    ClipboardCopyButton: jest.fn(props => (
+      <button data-testid="mock-clipboard-copy-button">
+        {props.children || 'Mock Copy Button'}
+      </button>
+    )),
+  };
+});
+


Could you please explain this mock is needed?

@MariaAga Sure, the tests were crashing while trying to render this button, so I made this mock. If you think it's bad, I can try to find different solution.

I would rather test components fully, so if we need to mock something because it crashes, to me it looks like it might also crash when users are using it

MariaAga

Tested on my dev setup and it seem to work, I just have a question about the mock in the test

MariaAga

lgtm, @adamruzicka can this be merged now, or do we need to wait because of branching?

adamruzicka · 2025-11-12T09:59:23Z

We can get this in straight away, thank you both

pr-processor bot added Not yet reviewed Waiting on contributor labels Oct 6, 2025

kmalyjur force-pushed the rerun-failed branch from 645726b to cd4a4d2 Compare October 6, 2025 17:08

pr-processor bot removed the Waiting on contributor label Oct 6, 2025

kmalyjur changed the title ~~Stabilize job detail table and fix unresponsive actions~~ Fixes #38806 - Stabilize job detail table and fix unresponsive actions Oct 6, 2025

kmalyjur force-pushed the rerun-failed branch 2 times, most recently from 34933fa to 962f783 Compare October 8, 2025 09:03

kmalyjur marked this pull request as ready for review October 8, 2025 09:20

kmalyjur requested a review from MariaAga October 8, 2025 09:20

MariaAga requested changes Oct 10, 2025

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Oct 10, 2025

MariaAga requested a review from Copilot October 10, 2025 10:10

Copilot AI reviewed Oct 10, 2025

View reviewed changes

MariaAga reviewed Oct 10, 2025

View reviewed changes

adamruzicka reviewed Oct 14, 2025

View reviewed changes

webpack/JobInvocationDetail/JobInvocationHostTable.js Outdated Show resolved Hide resolved

kmalyjur force-pushed the rerun-failed branch from 962f783 to c5726c2 Compare October 22, 2025 09:32

pr-processor bot added Needs re-review and removed Waiting on contributor labels Oct 22, 2025

kmalyjur force-pushed the rerun-failed branch from c5726c2 to c20685c Compare October 22, 2025 11:14

adamruzicka requested changes Oct 23, 2025

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Oct 23, 2025

kmalyjur force-pushed the rerun-failed branch from c20685c to a8526ea Compare October 23, 2025 10:08

pr-processor bot added Needs re-review and removed Waiting on contributor labels Oct 23, 2025

pr-processor bot removed the Needs re-review label Oct 23, 2025

adamruzicka requested changes Oct 29, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Oct 29, 2025

kmalyjur force-pushed the rerun-failed branch from a8526ea to ec900df Compare November 3, 2025 17:42

pr-processor bot added Needs re-review and removed Waiting on contributor labels Nov 3, 2025

kmalyjur requested review from MariaAga and adamruzicka November 3, 2025 17:42

adamruzicka approved these changes Nov 4, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 4, 2025

MariaAga reviewed Nov 10, 2025

View reviewed changes

Fixes #38806 - Stabilize job detail table and fix unresponsive actions

d38d478

kmalyjur force-pushed the rerun-failed branch from ec900df to d38d478 Compare November 11, 2025 16:37

kmalyjur requested a review from MariaAga November 11, 2025 16:39

MariaAga approved these changes Nov 12, 2025

View reviewed changes

adamruzicka merged commit 41b8dee into theforeman:master Nov 12, 2025
17 checks passed

-  showOutputType: PropTypes.object.isRequired,
+  showOutputType: PropTypes.shape({
+    stderr: PropTypes.bool,
+    stdout: PropTypes.bool,
+    debug: PropTypes.bool,
+  }).isRequired,

	{...{ selectOne, isSelected }}
	selectOne={selecteOne}
	isSelected={isSelected}

		@@ -64,6 +64,13 @@ const JobInvocationHostTable = ({
		const [expandedHost, setExpandedHost] = useState([]);

Conversation

kmalyjur commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur commented Oct 23, 2025

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kmalyjur commented Oct 6, 2025 •

edited

Loading

kmalyjur Oct 20, 2025 •

edited

Loading

kmalyjur Nov 11, 2025 •

edited

Loading