-
Notifications
You must be signed in to change notification settings - Fork 44
Add Advisor IopRecommendationsCell #1052
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 GuideReplaces the previous Scalprum-based IopRecommendationsCell with a direct API-backed implementation that fetches the hit count via useAPI, constructs the Foreman Insights link with foremanUrl, and updates RecommendationsCell to use the new component. Sequence diagram for IopRecommendationsCell API-backed hit count fetchsequenceDiagram
participant RecommendationsCell
participant IopRecommendationsCell
participant API as Insights API
participant Browser
RecommendationsCell->>IopRecommendationsCell: Render with hostDetails
IopRecommendationsCell->>API: GET /insights_cloud/api/insights/v1/system/:uuid
API-->>IopRecommendationsCell: { hits }
IopRecommendationsCell->>Browser: Render <a href=...>{hits}</a>
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 @mhuth - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `webpack/ForemanColumnExtensions/index.js:30` </location>
<code_context>
- <ScalprumComponent scope={scope} module={module} />
- </span>
- );
+ const hostname = hostDetails?.name;
+ const encodedHostname = encodeURIComponent(hostname);
+ const hitsUrl = foremanUrl(`/new/hosts/${encodedHostname}#/Insights`);
+ return !hits ? '—' : <a href={hitsUrl}>{hits}</a>;
</code_context>
<issue_to_address>
No fallback for missing or empty hostname before encoding.
Add a check or default value for hostDetails.name to prevent generating invalid URLs when the hostname is missing or empty.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const hostname = hostDetails?.name;
const encodedHostname = encodeURIComponent(hostname);
const hitsUrl = foremanUrl(`/new/hosts/${encodedHostname}#/Insights`);
return !hits ? '—' : <a href={hitsUrl}>{hits}</a>;
=======
const hostname = hostDetails?.name && hostDetails.name.trim() !== '' ? hostDetails.name : 'unknown-host';
const encodedHostname = encodeURIComponent(hostname);
const hitsUrl = foremanUrl(`/new/hosts/${encodedHostname}#/Insights`);
return !hits ? '—' : <a href={hitsUrl}>{hits}</a>;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `webpack/ForemanColumnExtensions/index.js:28` </location>
<code_context>
+ if (!uuid) return '—';
+ const key = `HOST_RECS_COUNT_${uuid}`;
+ const response = useAPI('get', `/insights_cloud/api/insights/v1/system/${uuid}`, {key,});
+ const hits = response.response?.hits;
- return (
- <span className="rh-cloud-insights-recommendations-cell">
- <ScalprumComponent scope={scope} module={module} />
- </span>
- );
+ const hostname = hostDetails?.name;
+ const encodedHostname = encodeURIComponent(hostname);
+ const hitsUrl = foremanUrl(`/new/hosts/${encodedHostname}#/Insights`);
+ return !hits ? '—' : <a href={hitsUrl}>{hits}</a>;
};
</code_context>
<issue_to_address>
Falsy check on hits may hide valid zero values.
Since 0 is falsy in JavaScript, this check will display '—' for 0 hits. Use a more precise condition, such as 'hits == null' or 'typeof hits !== "number"', to ensure zero is handled correctly.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Thanks @mhuth ! Works well, just resolve the eslint problems and we're good.
77ce4df
to
ad69fe2
Compare
ad69fe2
to
fd2cfd4
Compare
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.
Looks fine to me, @jeremylenz did you want to do a final review?
Adds the IopRecommendationsCell needed for producing a count of the number of hits for each Insights host, as per the screenshot:
No automated tests at this stage. Just manual testing via the browser.
Summary by Sourcery
Implement a new Advisor recommendations cell that uses the useAPI hook to fetch and display the number of hits for each Insights host and link to the host's Insights page, replacing the previous Scalprum-based implementation.
New Features:
Enhancements:
Tests: