Skip to content

Fix js lints#425

Merged
adamruzicka merged 1 commit intotheforeman:masterfrom
adamruzicka:js-lints
Jul 8, 2025
Merged

Fix js lints#425
adamruzicka merged 1 commit intotheforeman:masterfrom
adamruzicka:js-lints

Conversation

@adamruzicka
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses JavaScript lint errors by adding ouiaId attributes for better testing and updating the useEffect dependencies to satisfy React hooks rules.

  • Added unique ouiaId props to each <Tr> in createPuppetMetricsTable
  • Included getLastReport in the useEffect dependency array
Comments suppressed due to low confidence (1)

webpack/src/Extends/Host/PuppetTab/SubTabs/Reports/components/ConfigStatusCard/index.js:151

  • If getLastReport isn’t memoized (e.g., via useCallback), adding it to the dependencies will cause the effect to run on every render. Consider wrapping getLastReport in useCallback or ensuring it’s stable to avoid unintended loops.
  }, [hostName, reports, getLastReport]);

useEffect(() => {
getLastReport();
}, [hostName, reports]);
}, [hostName, reports, getLastReport]);
Copy link
Copy Markdown
Member

@ofedoren ofedoren Jul 8, 2025

Choose a reason for hiding this comment

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

That can lead to side-effects. We tend to ignore that rule if we unsure about following this rule blindly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

From what I've read it should be fine as long as the dependency is memoized with useCallback (which it is), but I wouldn't mind dropping that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But yeah, let's play it safe. Dropped

Copy link
Copy Markdown
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @adamruzicka !

UPD: Agh... I don't have merge rights.

@adamruzicka
Copy link
Copy Markdown
Contributor Author

And we're green

@adamruzicka adamruzicka merged commit 8973e24 into theforeman:master Jul 8, 2025
16 checks passed
@adamruzicka
Copy link
Copy Markdown
Contributor Author

Thank you @ofedoren for keeping me on the right track

@adamruzicka adamruzicka deleted the js-lints branch July 8, 2025 13:10
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.

3 participants