load scalprum - #998
Closed
MariaAga wants to merge 1 commit into
Closed
Conversation
Reviewer's GuideIntegrate Scalprum dynamic plugin loading into the inventory UI by introducing a context wrapper with ScalprumProvider and adding the required Scalprum dependencies. Class diagram for ScalprumContext and ScalprumContextWrapper integrationclassDiagram
class ScalprumContextWrapper {
+config: object
+setConfig(newConfig)
+mockUser: object
+render()
}
class ScalprumContext {
<<context>>
}
ScalprumContextWrapper --> ScalprumContext : provides
ScalprumContextWrapper --> ScalprumProvider : wraps
ScalprumProvider <|-- ScalprumContextWrapper : conditional render
Class diagram for ScalprumProvider pluginSDKOptions and API mockclassDiagram
class ScalprumProvider {
+pluginSDKOptions: object
+api: object
+config: object
+children: ReactNode
}
class pluginSDKOptions {
+pluginLoaderOptions: object
}
class pluginLoaderOptions {
+transformPluginManifest(manifest)
}
class api {
+chrome: object
}
class chrome {
+isBeta()
+on()
+auth: object
}
class auth {
+getUser()
}
ScalprumProvider --> pluginSDKOptions
pluginSDKOptions --> pluginLoaderOptions
ScalprumProvider --> api
api --> chrome
chrome --> auth
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
MariaAga
marked this pull request as draft
June 6, 2025 15:16
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MariaAga - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Invalid JSX comment syntax (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 4 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ShimShtein
reviewed
Jun 11, 2025
MariaAga
force-pushed
the
import-scalprum
branch
from
June 17, 2025 10:07
e461890 to
b2ef443
Compare
jeremylenz
reviewed
Jun 27, 2025
jeremylenz
left a comment
jeremylenz
left a comment
Collaborator
There was a problem hiding this comment.
@MariaAga Can you please remove the changes to ForemanInventoryUpload from this pr? Then we can merge it, we've tested and it's working well. And then we can add the scalprum-wrapped components in the correct places.
MariaAga
marked this pull request as ready for review
July 1, 2025 09:36
There was a problem hiding this comment.
Hey @MariaAga - I've reviewed your changes - here's some feedback:
- Add a loading fallback (e.g., React.Suspense or an error boundary) around the ScalprumProvider/ScalprumComponent to handle slow or failed plugin loads gracefully.
- Extract the mockUser stub out of the ScalprumContextWrapper into a dedicated fixture or util and swap in the real identity provider before production rollout.
- Consider externalizing the manifestLocation and cdnPath construction (instead of hard-coding window.location.origin) to make the plugin host paths configurable per environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a loading fallback (e.g., React.Suspense or an error boundary) around the ScalprumProvider/ScalprumComponent to handle slow or failed plugin loads gracefully.
- Extract the mockUser stub out of the ScalprumContextWrapper into a dedicated fixture or util and swap in the real identity provider before production rollout.
- Consider externalizing the manifestLocation and cdnPath construction (instead of hard-coding window.location.origin) to make the plugin host paths configurable per environment.
## Individual Comments
### Comment 1
<location> `webpack/common/ScalprumModule/ScalprumContext.js:39` </location>
<code_context>
+ ),
+ };
+
+ const mockUser = {
+ entitlements: {},
+ identity: {
+ account_number: 'string',
+ org_id: 'string',
+ internal: {
+ org_id: 'string',
+ account_id: 'string',
+ },
+ type: 'string',
+ user: {
</code_context>
<issue_to_address>
Hardcoded mockUser may cause confusion or issues in production.
If this is for development only, clearly indicate that or allow it to be overridden in production.
</issue_to_address>
### Comment 2
<location> `webpack/common/ScalprumModule/ScalprumContext.js:88` </location>
<code_context>
+ api={{
+ chrome: {
+ isBeta: () => false,
+ on: () => {},
+ auth: {
+ getUser: () => Promise.resolve(mockUser),
</code_context>
<issue_to_address>
The chrome.on method is a no-op, which may break consumers expecting event handling.
Returning a no-op here may cause silent failures for consumers relying on event subscriptions. Suggest logging a warning or implementing a minimal event emitter.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
api={{
chrome: {
isBeta: () => false,
on: () => {},
auth: {
getUser: () => Promise.resolve(mockUser),
},
},
}}
=======
api={{
chrome: (() => {
// Minimal event emitter for mock chrome.on
const listeners = {};
return {
isBeta: () => false,
on: (event, callback) => {
if (!listeners[event]) {
listeners[event] = [];
}
listeners[event].push(callback);
// Log a warning for visibility in tests/mocks
// eslint-disable-next-line no-console
console.warn(`[Mock chrome.on] Subscribed to event: ${event}`);
// Return unsubscribe function for compatibility
return () => {
listeners[event] = listeners[event].filter(cb => cb !== callback);
};
},
// Optionally, provide a way to emit events in tests
__emit: (event, ...args) => {
if (listeners[event]) {
listeners[event].forEach(cb => {
try {
cb(...args);
} catch (e) {
// eslint-disable-next-line no-console
console.error(`[Mock chrome.on] Error in event handler for ${event}:`, e);
}
});
}
// eslint-disable-next-line no-console
console.warn(`[Mock chrome.on] Emitted event: ${event}`, ...args);
},
auth: {
getUser: () => Promise.resolve(mockUser),
},
};
})(),
}}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `webpack/common/ScalprumModule/ScalprumContext.js:67` </location>
<code_context>
+ <ScalprumProvider
+ pluginSDKOptions={{
+ pluginLoaderOptions: {
+ transformPluginManifest: manifest => {
+ if (
+ manifest.baseURL === 'auto' &&
+ config[manifest.name]?.cdnPath
+ ) {
+ const _cdnPath = config[manifest.name]?.cdnPath;
+ return {
+ ...manifest,
+ baseURL: _cdnPath,
+ loadScripts: manifest.loadScripts.map(
+ script => `${_cdnPath}${script}`
+ ),
+ };
+ }
+ return manifest;
+ },
+ },
</code_context>
<issue_to_address>
transformPluginManifest mutates loadScripts URLs but assumes all scripts are relative.
This logic may fail if loadScripts includes absolute URLs. Please ensure only relative paths are prefixed, or clearly document the assumption that all scripts are relative.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
loadScripts: manifest.loadScripts.map(
script => `${_cdnPath}${script}`
),
=======
loadScripts: manifest.loadScripts.map(script => {
// Only prefix if script is a relative path
if (
typeof script === 'string' &&
!/^([a-z][a-z0-9+\-.]*:)?\/\//i.test(script)
) {
return `${_cdnPath}${script}`;
}
return script;
}),
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+76
to
+83
| loadScripts: manifest.loadScripts.map( | ||
| script => `${_cdnPath}${script}` | ||
| ), |
There was a problem hiding this comment.
suggestion (bug_risk): transformPluginManifest mutates loadScripts URLs but assumes all scripts are relative.
This logic may fail if loadScripts includes absolute URLs. Please ensure only relative paths are prefixed, or clearly document the assumption that all scripts are relative.
Suggested change
| loadScripts: manifest.loadScripts.map( | |
| script => `${_cdnPath}${script}` | |
| ), | |
| loadScripts: manifest.loadScripts.map(script => { | |
| // Only prefix if script is a relative path | |
| if ( | |
| typeof script === 'string' && | |
| !/^([a-z][a-z0-9+\-.]*:)?\/\//i.test(script) | |
| ) { | |
| return `${_cdnPath}${script}`; | |
| } | |
| return script; | |
| }), |
Member
Author
|
Updated the docs comment with more info |
Collaborator
|
Once theforeman/foreman#10342 is in (and any conversations above are addressed and resolved) I think this should be ready to go |
4 tasks
Member
|
@MariaAga Do we need this one if we use theforeman/foreman#10598 ? |
Member
Author
|
This is a generic component, I think we should re-write the |
Member
|
If you think it's a waste of effort, let's get rid of it. Do you have the "manual" for what we need to do in each of our PRs? |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To setup:
Create /etc/httpd/conf.d/05-foreman-ssl.d/consoledot.conf
with
ProxyPass /scalprum http://HOSTNAME:8001(change HOSTNAME)run
sudo systemctl restart httpdand run
podman run -p 8001:8000 quay.io/redhat-services-prod/rh-platform-experien-tenant/landing-page-frontend:latestAlso needs Fixes #37882 - Remove @theforeman/vendor foreman#10342
This pr deletes the use of frontend components as they clash with scalprum and should be replaces in #973
Summary by Sourcery
Enable Scalprum-based module federation by introducing a Scalprum context and provider wrapper for dynamic plugin loading, and adding the necessary Scalprum dependencies.
New Features:
Enhancements: