-
Notifications
You must be signed in to change notification settings - Fork 50
SAT-37817 - Hide Recommendations page for non-registered hosts #1098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR refactors the InsightsTab component to derive advisor engine selection from a global hook instead of host-specific response data, adds a strict Boolean check for clarity, and removes unused PropTypes and defaultProps for cleaner code. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- You can remove the empty propTypes and defaultProps declarations entirely since this component no longer accepts any props.
- Consider renaming
isLocalAdvisorEngineto something likeisIopModeEnabledto better reflect that it’s driven by youruseAdvisorEngineConfighook. - Double-check that
useAdvisorEngineConfigalways returns a strict boolean so you don’t need the=== truecheck (or provide a default value of false inside the hook).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You can remove the empty propTypes and defaultProps declarations entirely since this component no longer accepts any props.
- Consider renaming `isLocalAdvisorEngine` to something like `isIopModeEnabled` to better reflect that it’s driven by your `useAdvisorEngineConfig` hook.
- Double-check that `useAdvisorEngineConfig` always returns a strict boolean so you don’t need the `=== true` check (or provide a default value of false inside the hook).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Not in the scope of this PR, but I've been wanting to do this. It's no longer just "advisor," so we should call it |
083626c to
41dfcac
Compare
Changed the names, let me know what you think? Also updated the changes you and AI wanted. |
jeremylenz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but thinking about it more ..
- I don't think an unregistered host will ever have recommendations.
- For Vulnerabilities, we just don't show the tab at all.
I think we should just not show the tab at all for unregistered hosts. Thoughts?
f89919a to
51992db
Compare
Makes sense, updated the pr to: The host is not a RHEL host (existing logic), OR This means that for a host to show the Recommendations tab, it must be: A RHEL host (Red Hat Enterprise Linux), AND |
51992db to
563211b
Compare
jeremylenz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
df4799c to
2d85255
Compare
@jeremylenz updated, let me know what you think. I tested it on a host that didn't have an |
2d85255 to
9a64055
Compare
jeremylenz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


What are the changes introduced in this pull request?
Remove the dependency on host-specific response data
This approach is consistent with how other components in the codebase handle this configuration and ensures that all hosts (with or without insights_facet) properly determine which advisor engine to display based on the global system settings.
Now the Insights/Recommendations tab will be hidden if either:
This means that for a host to show the Recommendations tab, it must be:
Considerations taken when implementing this change?
What are the testing steps for this pull request?